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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

On Sat, 2015-03-28 at 21:58 +0100, Hartmut Knaack wrote: 
> Srinivas Pandruvada schrieb am 17.01.2015 um 01:39:
> > 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.
> > 
Hi Hartmut, 
> Hi Srinivas,
> I noticed this series got applied already to linux-input. Yet I was wondering
> if it wouldn't be better to have size variables like buffer_size, report_size
> and remaining_bytes (from the next patch) defined as unsigned.
> Also a minor indentation issue spotted inline, in case you get to work on that
> section again some time.
> 
This module will go further enhancements soon, then I will take care of
this.

Thanks,
Srinivas 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  drivers/hid/hid-sensor-hub.c                           | 15 +++++++++++++--
> >  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, 27 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index e54ce10..4d22bd4 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -232,10 +232,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);
> > @@ -247,7 +248,17 @@ 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 = DIV_ROUND_UP(report->field[field_index]->report_size,
> > +				   8) *
> > +				   report->field[field_index]->report_count;
> The last line should not be indented with the opening parenthesis any more.
> What do you think about this solution:
> 
> 	report_size = DIV_ROUND_UP(report->field[field_index]->report_size, 8);
> 	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 4173a8f..dfdf6ef 100644
> > --- a/include/linux/hid-sensor-hub.h
> > +++ b/include/linux/hid-sensor-hub.h
> > @@ -179,13 +179,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 */
> >  
> > 
>