[PATCH v1 0/2] Enhance feature report APIs

[PATCH v1 0/2] Enhance feature report APIs

[Srinivas Pandruvada]

Current get and set feature report is enhanced to have multiple
values in get and set.

v1
Squashed into two patches one for get and one for set feature report
for bisectability.

v0
Separate patches for each driver/modules

Srinivas Pandruvada (2):
  HID: hid-sensor-hub: Enhance get feature report API
  HID: hid-sensor-hub: Enhance feature report set API

 drivers/hid/hid-sensor-hub.c                       | 38 +++++++++++++++++++---
 .../iio/common/hid-sensors/hid-sensor-attributes.c | 24 +++++++-------
 .../iio/common/hid-sensors/hid-sensor-trigger.c    | 13 ++++----
 include/linux/hid-sensor-hub.h                     | 13 +++++---
 4 files changed, 61 insertions(+), 27 deletions(-)

-- 
1.9.3

[PATCH v1 1/2] HID: hid-sensor-hub: Enhance get feature report API

[Srinivas Pandruvada]

Some hid sensor feature report can contain more than one reports.
This API can now support receiving multiple values from the feature
report.
Also update the parameters in the users of this API.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c                           | 16 ++++++++++++++--
 drivers/iio/common/hid-sensors/hid-sensor-attributes.c | 13 +++++++------
 drivers/iio/common/hid-sensors/hid-sensor-trigger.c    |  4 ++--
 include/linux/hid-sensor-hub.h                         |  8 +++++---
 4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 83b6e15..d17aa67 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -218,10 +218,11 @@ done_proc:
 EXPORT_SYMBOL_GPL(sensor_hub_set_feature);
 
 int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
-				u32 field_index, s32 *value)
+			   u32 field_index, int buffer_size, void *buffer)
 {
 	struct hid_report *report;
 	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
+	int report_size;
 	int ret = 0;
 
 	mutex_lock(&data->mutex);
@@ -233,7 +234,18 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
 	}
 	hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
 	hid_hw_wait(hsdev->hdev);
-	*value = report->field[field_index]->value[0];
+
+	/* calculate number of bytes required to read this field */
+	report_size = report->field[field_index]->report_size / 8;
+	if (report->field[field_index]->report_size % 8)
+		++report_size;
+	report_size *= report->field[field_index]->report_count;
+	if (!report_size) {
+		ret = -EINVAL;
+		goto done_proc;
+	}
+	ret = min(report_size, buffer_size);
+	memcpy(buffer, report->field[field_index]->value, ret);
 
 done_proc:
 	mutex_unlock(&data->mutex);
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
index 25b01e1..e1435e9 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
@@ -153,8 +153,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st)
 	int ret;
 
 	ret = sensor_hub_get_feature(st->hsdev,
-		st->poll.report_id,
-		st->poll.index, &value);
+				     st->poll.report_id,
+				     st->poll.index, sizeof(value), &value);
 
 	if (ret < 0 || value < 0) {
 		return -EINVAL;
@@ -174,8 +174,8 @@ int hid_sensor_read_samp_freq_value(struct hid_sensor_common *st,
 	int ret;
 
 	ret = sensor_hub_get_feature(st->hsdev,
-		st->poll.report_id,
-		st->poll.index, &value);
+				     st->poll.report_id,
+				     st->poll.index, sizeof(value), &value);
 	if (ret < 0 || value < 0) {
 		*val1 = *val2 = 0;
 		return -EINVAL;
@@ -229,8 +229,9 @@ int hid_sensor_read_raw_hyst_value(struct hid_sensor_common *st,
 	int ret;
 
 	ret = sensor_hub_get_feature(st->hsdev,
-		st->sensitivity.report_id,
-		st->sensitivity.index, &value);
+				     st->sensitivity.report_id,
+				     st->sensitivity.index, sizeof(value),
+				     &value);
 	if (ret < 0 || value < 0) {
 		*val1 = *val2 = 0;
 		return -EINVAL;
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 92068cd..ef0c495 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -76,8 +76,8 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 	}
 
 	sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
-					st->power_state.index,
-					&state_val);
+			       st->power_state.index,
+			       sizeof(state_val), &state_val);
 	return 0;
 }
 EXPORT_SYMBOL(hid_sensor_power_state);
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index 0b21c4f..1995bbd 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -203,13 +203,15 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
 * @hsdev:	Hub device instance.
 * @report_id:	Report id to look for
 * @field_index:	Field index inside a report
-* @value:	Place holder for return value
+* @buffer_size:	size of the buffer
+* @buffer:	buffer to copy output
 *
 * Used to get a field in feature report. For example this can get polling
-* interval, sensitivity, activate/deactivate state.
+* interval, sensitivity, activate/deactivate state. On success it returns
+* number of bytes copied to buffer. On failure, it returns value < 0.
 */
 int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
-			u32 field_index, s32 *value);
+			   u32 field_index, int buffer_size, void *buffer);
 
 /* hid-sensor-attributes */
 
-- 
1.9.3

[PATCH v1 2/2] HID: hid-sensor-hub: Enhance feature report set API

[Srinivas Pandruvada]

Current API only allows setting one offset in the field. This API
is extended to set multiple offsets in the field report.
Also update parameters in the users of this API.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c                       | 22 ++++++++++++++++++++--
 .../iio/common/hid-sensors/hid-sensor-attributes.c | 11 +++++------
 .../iio/common/hid-sensors/hid-sensor-trigger.c    |  9 +++++----
 include/linux/hid-sensor-hub.h                     |  5 +++--
 4 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index d17aa67..8bed109 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -194,10 +194,14 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
 EXPORT_SYMBOL_GPL(sensor_hub_remove_callback);
 
 int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
-				u32 field_index, s32 value)
+			   u32 field_index, int buffer_size, void *buffer)
 {
 	struct hid_report *report;
 	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
+	__s32 *buf32 = (__s32 *)buffer;
+	int i = 0;
+	int remaining_bytes;
+	__s32 value;
 	int ret = 0;
 
 	mutex_lock(&data->mutex);
@@ -206,7 +210,21 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
 		ret = -EINVAL;
 		goto done_proc;
 	}
-	hid_set_field(report->field[field_index], 0, value);
+
+	remaining_bytes = do_div(buffer_size, sizeof(__s32));
+	if (buffer_size) {
+		for (i = 0; i < buffer_size; ++i) {
+			hid_set_field(report->field[field_index], i,
+				      cpu_to_le32(*buf32));
+			++buf32;
+		}
+	}
+	if (remaining_bytes) {
+		value = 0;
+		memcpy(&value, (u8 *)buf32, remaining_bytes);
+		hid_set_field(report->field[field_index], i,
+			      cpu_to_le32(value));
+	}
 	hid_hw_request(hsdev->hdev, report, HID_REQ_SET_REPORT);
 	hid_hw_wait(hsdev->hdev);
 
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
index e1435e9..e81f434 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
@@ -212,9 +212,8 @@ int hid_sensor_write_samp_freq_value(struct hid_sensor_common *st,
 		else
 			value = 0;
 	}
-	ret = sensor_hub_set_feature(st->hsdev,
-		st->poll.report_id,
-		st->poll.index, value);
+	ret = sensor_hub_set_feature(st->hsdev, st->poll.report_id,
+				     st->poll.index, sizeof(value), &value);
 	if (ret < 0 || value < 0)
 		ret = -EINVAL;
 
@@ -254,9 +253,9 @@ int hid_sensor_write_raw_hyst_value(struct hid_sensor_common *st,
 	value = convert_to_vtf_format(st->sensitivity.size,
 				st->sensitivity.unit_expo,
 				val1, val2);
-	ret = sensor_hub_set_feature(st->hsdev,
-		st->sensitivity.report_id,
-		st->sensitivity.index, value);
+	ret = sensor_hub_set_feature(st->hsdev, st->sensitivity.report_id,
+				     st->sensitivity.index, sizeof(value),
+				     &value);
 	if (ret < 0 || value < 0)
 		ret = -EINVAL;
 
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index ef0c495..910e82a 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -64,15 +64,16 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 	if (state_val >= 0) {
 		state_val += st->power_state.logical_minimum;
 		sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
-					st->power_state.index,
-					(s32)state_val);
+				       st->power_state.index, sizeof(state_val),
+				       &state_val);
 	}
 
 	if (report_val >= 0) {
 		report_val += st->report_state.logical_minimum;
 		sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
-					st->report_state.index,
-					(s32)report_val);
+				       st->report_state.index,
+				       sizeof(report_val),
+				       &report_val);
 	}
 
 	sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index 1995bbd..a51c768 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -190,13 +190,14 @@ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
 * @hsdev:	Hub device instance.
 * @report_id:	Report id to look for
 * @field_index:	Field index inside a report
-* @value:	Value to set
+* @buffer_size: size of the buffer
+* @buffer:	buffer to use in the feature set
 *
 * Used to set a field in feature report. For example this can set polling
 * interval, sensitivity, activate/deactivate state.
 */
 int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
-			u32 field_index, s32 value);
+			   u32 field_index, int buffer_size, void *buffer);
 
 /**
 * sensor_hub_get_feature() - Feature get request
-- 
1.9.3

Re: [PATCH v1 1/2] HID: hid-sensor-hub: Enhance get feature report API

[Antonio Borneo]

On Mon, Jan 12, 2015 at 9:55 AM, Srinivas Pandruvada
<srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Some hid sensor feature report can contain more than one reports.
> This API can now support receiving multiple values from the feature
> report.
> Also update the parameters in the users of this API.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/hid/hid-sensor-hub.c                           | 16 ++++++++++++++--
>  drivers/iio/common/hid-sensors/hid-sensor-attributes.c | 13 +++++++------
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c    |  4 ++--
>  include/linux/hid-sensor-hub.h                         |  8 +++++---
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 83b6e15..d17aa67 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -218,10 +218,11 @@ done_proc:
>  EXPORT_SYMBOL_GPL(sensor_hub_set_feature);
>
>  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -                               u32 field_index, s32 *value)
> +                          u32 field_index, int buffer_size, void *buffer)
>  {
>         struct hid_report *report;
>         struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +       int report_size;
>         int ret = 0;
>
>         mutex_lock(&data->mutex);
> @@ -233,7 +234,18 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>         }
>         hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
>         hid_hw_wait(hsdev->hdev);
> -       *value = report->field[field_index]->value[0];
> +
> +       /* calculate number of bytes required to read this field */
> +       report_size = report->field[field_index]->report_size / 8;
> +       if (report->field[field_index]->report_size % 8)
> +               ++report_size;
> +       report_size *= report->field[field_index]->report_count;

Hi Srinivas,

I resend my reply.
It was discarded by the mailing list due to wrong setting with html formatting.

I think would be more clear writing directly:

        report_size = DIV_ROUND_UP(report->field[field_index]->report_size, 8) *
                      report->field[field_index]->report_count;

Regards,
Antonio

> +       if (!report_size) {
> +               ret = -EINVAL;
> +               goto done_proc;
> +       }
> +       ret = min(report_size, buffer_size);
> +       memcpy(buffer, report->field[field_index]->value, ret);
>
>  done_proc:
>         mutex_unlock(&data->mutex);
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> index 25b01e1..e1435e9 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -153,8 +153,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st)
>         int ret;
>
>         ret = sensor_hub_get_feature(st->hsdev,
> -               st->poll.report_id,
> -               st->poll.index, &value);
> +                                    st->poll.report_id,
> +                                    st->poll.index, sizeof(value), &value);
>
>         if (ret < 0 || value < 0) {
>                 return -EINVAL;
> @@ -174,8 +174,8 @@ int hid_sensor_read_samp_freq_value(struct hid_sensor_common *st,
>         int ret;
>
>         ret = sensor_hub_get_feature(st->hsdev,
> -               st->poll.report_id,
> -               st->poll.index, &value);
> +                                    st->poll.report_id,
> +                                    st->poll.index, sizeof(value), &value);
>         if (ret < 0 || value < 0) {
>                 *val1 = *val2 = 0;
>                 return -EINVAL;
> @@ -229,8 +229,9 @@ int hid_sensor_read_raw_hyst_value(struct hid_sensor_common *st,
>         int ret;
>
>         ret = sensor_hub_get_feature(st->hsdev,
> -               st->sensitivity.report_id,
> -               st->sensitivity.index, &value);
> +                                    st->sensitivity.report_id,
> +                                    st->sensitivity.index, sizeof(value),
> +                                    &value);
>         if (ret < 0 || value < 0) {
>                 *val1 = *val2 = 0;
>                 return -EINVAL;
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 92068cd..ef0c495 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -76,8 +76,8 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>         }
>
>         sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
> -                                       st->power_state.index,
> -                                       &state_val);
> +                              st->power_state.index,
> +                              sizeof(state_val), &state_val);
>         return 0;
>  }
>  EXPORT_SYMBOL(hid_sensor_power_state);
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index 0b21c4f..1995bbd 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -203,13 +203,15 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  * @hsdev:      Hub device instance.
>  * @report_id:  Report id to look for
>  * @field_index:        Field index inside a report
> -* @value:      Place holder for return value
> +* @buffer_size:        size of the buffer
> +* @buffer:     buffer to copy output
>  *
>  * Used to get a field in feature report. For example this can get polling
> -* interval, sensitivity, activate/deactivate state.
> +* interval, sensitivity, activate/deactivate state. On success it returns
> +* number of bytes copied to buffer. On failure, it returns value < 0.
>  */
>  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -                       u32 field_index, s32 *value);
> +                          u32 field_index, int buffer_size, void *buffer);
>
>  /* hid-sensor-attributes */
>
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] HID: hid-sensor-hub: Enhance get feature report API

[Srinivas Pandruvada]

On Mon, 2015-01-12 at 11:57 +0800, Antonio Borneo wrote: 
> On Mon, Jan 12, 2015 at 9:55 AM, Srinivas Pandruvada
> <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > Some hid sensor feature report can contain more than one reports.
> > This API can now support receiving multiple values from the feature
> > report.
> > Also update the parameters in the users of this API.
> >
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >  drivers/hid/hid-sensor-hub.c                           | 16 ++++++++++++++--
> >  drivers/iio/common/hid-sensors/hid-sensor-attributes.c | 13 +++++++------
> >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c    |  4 ++--
> >  include/linux/hid-sensor-hub.h                         |  8 +++++---
> >  4 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index 83b6e15..d17aa67 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -218,10 +218,11 @@ done_proc:
> >  EXPORT_SYMBOL_GPL(sensor_hub_set_feature);
> >
> >  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> > -                               u32 field_index, s32 *value)
> > +                          u32 field_index, int buffer_size, void *buffer)
> >  {
> >         struct hid_report *report;
> >         struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > +       int report_size;
> >         int ret = 0;
> >
> >         mutex_lock(&data->mutex);
> > @@ -233,7 +234,18 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> >         }
> >         hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
> >         hid_hw_wait(hsdev->hdev);
> > -       *value = report->field[field_index]->value[0];
> > +
> > +       /* calculate number of bytes required to read this field */
> > +       report_size = report->field[field_index]->report_size / 8;
> > +       if (report->field[field_index]->report_size % 8)
> > +               ++report_size;
> > +       report_size *= report->field[field_index]->report_count;
> 
> Hi Srinivas,
Hi Antonio, 
> 
> I resend my reply.
> It was discarded by the mailing list due to wrong setting with html formatting.
> 
> I think would be more clear writing directly:
> 
>         report_size = DIV_ROUND_UP(report->field[field_index]->report_size, 8) *
>                       report->field[field_index]->report_count;
Correct. I will resubmit this one.

Thanks,
Srinivas

> 
> Regards,
> Antonio
> 
> > +       if (!report_size) {
> > +               ret = -EINVAL;
> > +               goto done_proc;
> > +       }
> > +       ret = min(report_size, buffer_size);
> > +       memcpy(buffer, report->field[field_index]->value, ret);
> >
> >  done_proc:
> >         mutex_unlock(&data->mutex);
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > index 25b01e1..e1435e9 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> > @@ -153,8 +153,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st)
> >         int ret;
> >
> >         ret = sensor_hub_get_feature(st->hsdev,
> > -               st->poll.report_id,
> > -               st->poll.index, &value);
> > +                                    st->poll.report_id,
> > +                                    st->poll.index, sizeof(value), &value);
> >
> >         if (ret < 0 || value < 0) {
> >                 return -EINVAL;
> > @@ -174,8 +174,8 @@ int hid_sensor_read_samp_freq_value(struct hid_sensor_common *st,
> >         int ret;
> >
> >         ret = sensor_hub_get_feature(st->hsdev,
> > -               st->poll.report_id,
> > -               st->poll.index, &value);
> > +                                    st->poll.report_id,
> > +                                    st->poll.index, sizeof(value), &value);
> >         if (ret < 0 || value < 0) {
> >                 *val1 = *val2 = 0;
> >                 return -EINVAL;
> > @@ -229,8 +229,9 @@ int hid_sensor_read_raw_hyst_value(struct hid_sensor_common *st,
> >         int ret;
> >
> >         ret = sensor_hub_get_feature(st->hsdev,
> > -               st->sensitivity.report_id,
> > -               st->sensitivity.index, &value);
> > +                                    st->sensitivity.report_id,
> > +                                    st->sensitivity.index, sizeof(value),
> > +                                    &value);
> >         if (ret < 0 || value < 0) {
> >                 *val1 = *val2 = 0;
> >                 return -EINVAL;
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > index 92068cd..ef0c495 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > @@ -76,8 +76,8 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
> >         }
> >
> >         sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
> > -                                       st->power_state.index,
> > -                                       &state_val);
> > +                              st->power_state.index,
> > +                              sizeof(state_val), &state_val);
> >         return 0;
> >  }
> >  EXPORT_SYMBOL(hid_sensor_power_state);
> > diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> > index 0b21c4f..1995bbd 100644
> > --- a/include/linux/hid-sensor-hub.h
> > +++ b/include/linux/hid-sensor-hub.h
> > @@ -203,13 +203,15 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> >  * @hsdev:      Hub device instance.
> >  * @report_id:  Report id to look for
> >  * @field_index:        Field index inside a report
> > -* @value:      Place holder for return value
> > +* @buffer_size:        size of the buffer
> > +* @buffer:     buffer to copy output
> >  *
> >  * Used to get a field in feature report. For example this can get polling
> > -* interval, sensitivity, activate/deactivate state.
> > +* interval, sensitivity, activate/deactivate state. On success it returns
> > +* number of bytes copied to buffer. On failure, it returns value < 0.
> >  */
> >  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> > -                       u32 field_index, s32 *value);
> > +                          u32 field_index, int buffer_size, void *buffer);
> >
> >  /* hid-sensor-attributes */
> >
> > --
> > 1.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/2] HID: hid-sensor-hub: Enhance get feature report API

[Jonathan Cameron]

On 12/01/15 01:55, Srinivas Pandruvada wrote:
> Some hid sensor feature report can contain more than one reports.
> This API can now support receiving multiple values from the feature
> report.
> Also update the parameters in the users of this API.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Looks like a straight forward addition to me.  With the change Antonio suggested

Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/hid/hid-sensor-hub.c                           | 16 ++++++++++++++--
>  drivers/iio/common/hid-sensors/hid-sensor-attributes.c | 13 +++++++------
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c    |  4 ++--
>  include/linux/hid-sensor-hub.h                         |  8 +++++---
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 83b6e15..d17aa67 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -218,10 +218,11 @@ done_proc:
>  EXPORT_SYMBOL_GPL(sensor_hub_set_feature);
>  
>  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -				u32 field_index, s32 *value)
> +			   u32 field_index, int buffer_size, void *buffer)
>  {
>  	struct hid_report *report;
>  	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +	int report_size;
>  	int ret = 0;
>  
>  	mutex_lock(&data->mutex);
> @@ -233,7 +234,18 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  	}
>  	hid_hw_request(hsdev->hdev, report, HID_REQ_GET_REPORT);
>  	hid_hw_wait(hsdev->hdev);
> -	*value = report->field[field_index]->value[0];
> +
> +	/* calculate number of bytes required to read this field */
> +	report_size = report->field[field_index]->report_size / 8;
> +	if (report->field[field_index]->report_size % 8)
> +		++report_size;
> +	report_size *= report->field[field_index]->report_count;
> +	if (!report_size) {
> +		ret = -EINVAL;
> +		goto done_proc;
> +	}
> +	ret = min(report_size, buffer_size);
> +	memcpy(buffer, report->field[field_index]->value, ret);
>  
>  done_proc:
>  	mutex_unlock(&data->mutex);
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> index 25b01e1..e1435e9 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -153,8 +153,8 @@ s32 hid_sensor_read_poll_value(struct hid_sensor_common *st)
>  	int ret;
>  
>  	ret = sensor_hub_get_feature(st->hsdev,
> -		st->poll.report_id,
> -		st->poll.index, &value);
> +				     st->poll.report_id,
> +				     st->poll.index, sizeof(value), &value);
>  
>  	if (ret < 0 || value < 0) {
>  		return -EINVAL;
> @@ -174,8 +174,8 @@ int hid_sensor_read_samp_freq_value(struct hid_sensor_common *st,
>  	int ret;
>  
>  	ret = sensor_hub_get_feature(st->hsdev,
> -		st->poll.report_id,
> -		st->poll.index, &value);
> +				     st->poll.report_id,
> +				     st->poll.index, sizeof(value), &value);
>  	if (ret < 0 || value < 0) {
>  		*val1 = *val2 = 0;
>  		return -EINVAL;
> @@ -229,8 +229,9 @@ int hid_sensor_read_raw_hyst_value(struct hid_sensor_common *st,
>  	int ret;
>  
>  	ret = sensor_hub_get_feature(st->hsdev,
> -		st->sensitivity.report_id,
> -		st->sensitivity.index, &value);
> +				     st->sensitivity.report_id,
> +				     st->sensitivity.index, sizeof(value),
> +				     &value);
>  	if (ret < 0 || value < 0) {
>  		*val1 = *val2 = 0;
>  		return -EINVAL;
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 92068cd..ef0c495 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -76,8 +76,8 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  	}
>  
>  	sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
> -					st->power_state.index,
> -					&state_val);
> +			       st->power_state.index,
> +			       sizeof(state_val), &state_val);
>  	return 0;
>  }
>  EXPORT_SYMBOL(hid_sensor_power_state);
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index 0b21c4f..1995bbd 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -203,13 +203,15 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  * @hsdev:	Hub device instance.
>  * @report_id:	Report id to look for
>  * @field_index:	Field index inside a report
> -* @value:	Place holder for return value
> +* @buffer_size:	size of the buffer
> +* @buffer:	buffer to copy output
>  *
>  * Used to get a field in feature report. For example this can get polling
> -* interval, sensitivity, activate/deactivate state.
> +* interval, sensitivity, activate/deactivate state. On success it returns
> +* number of bytes copied to buffer. On failure, it returns value < 0.
>  */
>  int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -			u32 field_index, s32 *value);
> +			   u32 field_index, int buffer_size, void *buffer);
>  
>  /* hid-sensor-attributes */
>  
> 

Re: [PATCH v1 2/2] HID: hid-sensor-hub: Enhance feature report set API

[Jonathan Cameron]

On 12/01/15 01:55, Srinivas Pandruvada wrote:
> Current API only allows setting one offset in the field. This API
> is extended to set multiple offsets in the field report.
> Also update parameters in the users of this API.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Again, looks to be a straightforward addition.  One trivial point inline.
With that fixed

Reviewed-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/hid/hid-sensor-hub.c                       | 22 ++++++++++++++++++++--
>  .../iio/common/hid-sensors/hid-sensor-attributes.c | 11 +++++------
>  .../iio/common/hid-sensors/hid-sensor-trigger.c    |  9 +++++----
>  include/linux/hid-sensor-hub.h                     |  5 +++--
>  4 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index d17aa67..8bed109 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -194,10 +194,14 @@ int sensor_hub_remove_callback(struct hid_sensor_hub_device *hsdev,
>  EXPORT_SYMBOL_GPL(sensor_hub_remove_callback);
>  
>  int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -				u32 field_index, s32 value)
> +			   u32 field_index, int buffer_size, void *buffer)
>  {
>  	struct hid_report *report;
>  	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> +	__s32 *buf32 = (__s32 *)buffer;
Don't need this cast.  Can assign void * to any pointer directly.
> +	int i = 0;
> +	int remaining_bytes;
> +	__s32 value;
>  	int ret = 0;
>  
>  	mutex_lock(&data->mutex);
> @@ -206,7 +210,21 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>  		ret = -EINVAL;
>  		goto done_proc;
>  	}
> -	hid_set_field(report->field[field_index], 0, value);
> +
> +	remaining_bytes = do_div(buffer_size, sizeof(__s32));
> +	if (buffer_size) {
> +		for (i = 0; i < buffer_size; ++i) {
> +			hid_set_field(report->field[field_index], i,
> +				      cpu_to_le32(*buf32));
> +			++buf32;
> +		}
> +	}
> +	if (remaining_bytes) {
> +		value = 0;
> +		memcpy(&value, (u8 *)buf32, remaining_bytes);
> +		hid_set_field(report->field[field_index], i,
> +			      cpu_to_le32(value));
> +	}
>  	hid_hw_request(hsdev->hdev, report, HID_REQ_SET_REPORT);
>  	hid_hw_wait(hsdev->hdev);
>  
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> index e1435e9..e81f434 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -212,9 +212,8 @@ int hid_sensor_write_samp_freq_value(struct hid_sensor_common *st,
>  		else
>  			value = 0;
>  	}
> -	ret = sensor_hub_set_feature(st->hsdev,
> -		st->poll.report_id,
> -		st->poll.index, value);
> +	ret = sensor_hub_set_feature(st->hsdev, st->poll.report_id,
> +				     st->poll.index, sizeof(value), &value);
>  	if (ret < 0 || value < 0)
>  		ret = -EINVAL;
>  
> @@ -254,9 +253,9 @@ int hid_sensor_write_raw_hyst_value(struct hid_sensor_common *st,
>  	value = convert_to_vtf_format(st->sensitivity.size,
>  				st->sensitivity.unit_expo,
>  				val1, val2);
> -	ret = sensor_hub_set_feature(st->hsdev,
> -		st->sensitivity.report_id,
> -		st->sensitivity.index, value);
> +	ret = sensor_hub_set_feature(st->hsdev, st->sensitivity.report_id,
> +				     st->sensitivity.index, sizeof(value),
> +				     &value);
>  	if (ret < 0 || value < 0)
>  		ret = -EINVAL;
>  
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index ef0c495..910e82a 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -64,15 +64,16 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  	if (state_val >= 0) {
>  		state_val += st->power_state.logical_minimum;
>  		sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
> -					st->power_state.index,
> -					(s32)state_val);
> +				       st->power_state.index, sizeof(state_val),
> +				       &state_val);
>  	}
>  
>  	if (report_val >= 0) {
>  		report_val += st->report_state.logical_minimum;
>  		sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
> -					st->report_state.index,
> -					(s32)report_val);
> +				       st->report_state.index,
> +				       sizeof(report_val),
> +				       &report_val);
>  	}
>  
>  	sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index 1995bbd..a51c768 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -190,13 +190,14 @@ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
>  * @hsdev:	Hub device instance.
>  * @report_id:	Report id to look for
>  * @field_index:	Field index inside a report
> -* @value:	Value to set
> +* @buffer_size: size of the buffer
> +* @buffer:	buffer to use in the feature set
>  *
>  * Used to set a field in feature report. For example this can set polling
>  * interval, sensitivity, activate/deactivate state.
>  */
>  int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> -			u32 field_index, s32 value);
> +			   u32 field_index, int buffer_size, void *buffer);
>  
>  /**
>  * sensor_hub_get_feature() - Feature get request
>