Re: [PATCH 3/3] IO: hid-sensor-magn-3d: Add in support for True/Magnetic North HID usages

From: Srinivas Pandruvada
Date: Wed May 28 2014 - 17:52:57 EST


Hi Reyad,

On 05/28/2014 02:35 PM, Reyad Attiyat wrote:
Hey Srinivas,

Well I could use sensor_hub_input_get_attribute_info() for each usage
attribute. I was just thinking that since each usage attribute is
found in a row, one for each field I think, it'd be easier to create
iio channels that way. This would eliminate running the for loop
search for usage id each time.
It shouldn't be an issue for few more attributes. The idea is that
we don't expose the report level parsing information to the client drivers.
The client shouldn't worry about which collection or report it belongs to.

In this way if we have to enhance the parse function for newer
FW changes or quirks, it is only done at one place. Clients are not
affected at all.

Alternatively
we can enhance API, which takes multiple usage ids and fills information
in a single call. What do you think about this?

Thanks,
Srinivas


I realize that my parse_report function is quite ugly espically ending
in four closing parenthesis and copying code from
sensor_hub_input_get_attribute_info(). I will change it if you don't
think it will slow down the driver or have any negative effects.

Thanks,
Reyad



On Wed, May 28, 2014 at 4:25 PM, Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
On 05/28/2014 02:15 PM, Reyad Attiyat wrote:
+static void sensor_hub_fill_attr_info(
+ struct hid_sensor_hub_attribute_info *info,
+ s32 index, s32 report_id, struct hid_field *field)
+{
+ info->index = index;
+ info->report_id = report_id;
+ info->units = field->unit;
+ info->unit_expo = field->unit_exponent;
+ info->size = (field->report_size * field->report_count)/8;
+ info->logical_minimum = field->logical_minimum;
+ info->logical_maximum = field->logical_maximum;
}
I copied this function from hid/hid-sensor-hub.c as it is marked
static in that file. I use it to fill attributes as I find them.
Should I create an another patch to make it non-static?

I didn't check your implementation. But
sensor_hub_input_get_attribute_info()
function is not enough? We are already using to get other attributes for x,
y and Z.

Thanks,
Srinivas



+ list_for_each_entry(report, &report_enum->report_list, list) {
+ for (i = 0; i < report->maxfield; ++i) {
+ field = report->field[i];
+
+ for (j = 0; j < field->maxusage; j++) {
+ usage = &(field->usage[j]);
+
This is how I mange to find all possible hid reports in the parse
reports function. I noticed that in the other function that was used,
sensor_hub_input_get_attribute_info(), it only uses field->usage[0].
Is there a reason for this and should I change my current
implementation?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/