Re: [PATCH] iio/hid-sensors: Fix IIO_CHAN_INFO_RAW returning wrong values for signed numbers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2018-10-30 at 09:00 +0100, Hans de Goede wrote:
> Before this commit sensor_hub_input_attr_get_raw_value() failed to
> take
> the signedness of 16 and 8 bit values into account, returning e.g.
> 65436 instead of -100 for the z-axis reading of an accelerometer.
> 
> This commit adds a new is_signed parameter to the function and makes
> all
> callers pass the appropriate value for this.
> 
> While at it, this commit also fixes up some neighboring lines where
> statements were needlessly split over 2 lines to improve readability.
Looks good. The problem is logical min/max was found to be garbage on
many systems. But by this time hope systems don't exists anymore. By
spec all logical minimum should be less than 0 for all floats.

One minor, need to update the kernel doc for new parameter
sensor_hub_input_attr_get_raw_value().

then you can add
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>

Thanks,
Srinivas

> 
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/hid/hid-sensor-custom.c                  |  2 +-
>  drivers/hid/hid-sensor-hub.c                     | 13 ++++++++++---
>  drivers/iio/accel/hid-sensor-accel-3d.c          |  5 ++++-
>  drivers/iio/gyro/hid-sensor-gyro-3d.c            |  5 ++++-
>  drivers/iio/humidity/hid-sensor-humidity.c       |  3 ++-
>  drivers/iio/light/hid-sensor-als.c               |  8 +++++---
>  drivers/iio/light/hid-sensor-prox.c              |  8 +++++---
>  drivers/iio/magnetometer/hid-sensor-magn-3d.c    |  8 +++++---
>  drivers/iio/orientation/hid-sensor-incl-3d.c     |  8 +++++---
>  drivers/iio/pressure/hid-sensor-press.c          |  8 +++++---
>  drivers/iio/temperature/hid-sensor-temperature.c |  3 ++-
>  drivers/rtc/rtc-hid-sensor-time.c                |  2 +-
>  include/linux/hid-sensor-hub.h                   |  3 ++-
>  13 files changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index e8a114157f87..bb012bc032e0 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -358,7 +358,7 @@ static ssize_t show_value(struct device *dev,
> struct device_attribute *attr,
>  						sensor_inst->hsdev,
>  						sensor_inst->hsdev-
> >usage,
>  						usage, report_id,
> -						SENSOR_HUB_SYNC);
> +						SENSOR_HUB_SYNC,
> false);
>  	} else if (!strncmp(name, "units", strlen("units")))
>  		value = sensor_inst-
> >fields[field_index].attribute.units;
>  	else if (!strncmp(name, "unit-expo", strlen("unit-expo")))
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-
> hub.c
> index 2b63487057c2..4256fdc5cd6d 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -299,7 +299,8 @@ EXPORT_SYMBOL_GPL(sensor_hub_get_feature);
>  int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device
> *hsdev,
>  					u32 usage_id,
>  					u32 attr_usage_id, u32
> report_id,
> -					enum sensor_hub_read_flags
> flag)
> +					enum sensor_hub_read_flags
> flag,
> +					bool is_signed)
>  {
>  	struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
>  	unsigned long flags;
> @@ -331,10 +332,16 @@ int sensor_hub_input_attr_get_raw_value(struct
> hid_sensor_hub_device *hsdev,
>  						&hsdev->pending.ready,
> HZ*5);
>  		switch (hsdev->pending.raw_size) {
>  		case 1:
> -			ret_val = *(u8 *)hsdev->pending.raw_data;
> +			if (is_signed)
> +				ret_val = *(s8 *)hsdev-
> >pending.raw_data;
> +			else
> +				ret_val = *(u8 *)hsdev-
> >pending.raw_data;
>  			break;
>  		case 2:
> -			ret_val = *(u16 *)hsdev->pending.raw_data;
> +			if (is_signed)
> +				ret_val = *(s16 *)hsdev-
> >pending.raw_data;
> +			else
> +				ret_val = *(u16 *)hsdev-
> >pending.raw_data;
>  			break;
>  		case 4:
>  			ret_val = *(u32 *)hsdev->pending.raw_data;
> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c
> b/drivers/iio/accel/hid-sensor-accel-3d.c
> index 41d97faf5013..38ff374a3ca4 100644
> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
> @@ -149,6 +149,7 @@ static int accel_3d_read_raw(struct iio_dev
> *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  	struct hid_sensor_hub_device *hsdev =
>  					accel_state-
> >common_attributes.hsdev;
>  
> @@ -158,12 +159,14 @@ static int accel_3d_read_raw(struct iio_dev
> *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		hid_sensor_power_state(&accel_state->common_attributes, 
> true);
>  		report_id = accel_state->accel[chan-
> >scan_index].report_id;
> +		min = accel_state->accel[chan-
> >scan_index].logical_minimum;
>  		address = accel_3d_addresses[chan->scan_index];
>  		if (report_id >= 0)
>  			*val = sensor_hub_input_attr_get_raw_value(
>  					accel_state-
> >common_attributes.hsdev,
>  					hsdev->usage, address,
> report_id,
> -					SENSOR_HUB_SYNC);
> +					SENSOR_HUB_SYNC,
> +					min < 0);
>  		else {
>  			*val = 0;
>  			hid_sensor_power_state(&accel_state-
> >common_attributes,
> diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> index 36941e69f959..88e857c4baf4 100644
> --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> @@ -111,6 +111,7 @@ static int gyro_3d_read_raw(struct iio_dev
> *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
> @@ -118,13 +119,15 @@ static int gyro_3d_read_raw(struct iio_dev
> *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		hid_sensor_power_state(&gyro_state->common_attributes,
> true);
>  		report_id = gyro_state->gyro[chan-
> >scan_index].report_id;
> +		min = gyro_state->gyro[chan-
> >scan_index].logical_minimum;
>  		address = gyro_3d_addresses[chan->scan_index];
>  		if (report_id >= 0)
>  			*val = sensor_hub_input_attr_get_raw_value(
>  					gyro_state-
> >common_attributes.hsdev,
>  					HID_USAGE_SENSOR_GYRO_3D,
> address,
>  					report_id,
> -					SENSOR_HUB_SYNC);
> +					SENSOR_HUB_SYNC,
> +					min < 0);
>  		else {
>  			*val = 0;
>  			hid_sensor_power_state(&gyro_state-
> >common_attributes,
> diff --git a/drivers/iio/humidity/hid-sensor-humidity.c
> b/drivers/iio/humidity/hid-sensor-humidity.c
> index beab6d6fd6e1..4bc95f31c730 100644
> --- a/drivers/iio/humidity/hid-sensor-humidity.c
> +++ b/drivers/iio/humidity/hid-sensor-humidity.c
> @@ -75,7 +75,8 @@ static int humidity_read_raw(struct iio_dev
> *indio_dev,
>  				HID_USAGE_SENSOR_HUMIDITY,
>  				HID_USAGE_SENSOR_ATMOSPHERIC_HUMIDITY,
>  				humid_st->humidity_attr.report_id,
> -				SENSOR_HUB_SYNC);
> +				SENSOR_HUB_SYNC,
> +				humid_st->humidity_attr.logical_minimum 
> < 0);
>  		hid_sensor_power_state(&humid_st->common_attributes,
> false);
>  
>  		return IIO_VAL_INT;
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index 406caaee9a3c..94f33250ba5a 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -93,6 +93,7 @@ static int als_read_raw(struct iio_dev *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
> @@ -102,8 +103,8 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
>  		case  CHANNEL_SCAN_INDEX_INTENSITY:
>  		case  CHANNEL_SCAN_INDEX_ILLUM:
>  			report_id = als_state->als_illum.report_id;
> -			address =
> -			HID_USAGE_SENSOR_LIGHT_ILLUM;
> +			min = als_state->als_illum.logical_minimum;
> +			address = HID_USAGE_SENSOR_LIGHT_ILLUM;
>  			break;
>  		default:
>  			report_id = -1;
> @@ -116,7 +117,8 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
>  					als_state-
> >common_attributes.hsdev,
>  					HID_USAGE_SENSOR_ALS, address,
>  					report_id,
> -					SENSOR_HUB_SYNC);
> +					SENSOR_HUB_SYNC,
> +					min < 0);
>  			hid_sensor_power_state(&als_state-
> >common_attributes,
>  						false);
>  		} else {
> diff --git a/drivers/iio/light/hid-sensor-prox.c
> b/drivers/iio/light/hid-sensor-prox.c
> index 45107f7537b5..cf5a0c242609 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -73,6 +73,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
> @@ -81,8 +82,8 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  		switch (chan->scan_index) {
>  		case  CHANNEL_SCAN_INDEX_PRESENCE:
>  			report_id = prox_state->prox_attr.report_id;
> -			address =
> -			HID_USAGE_SENSOR_HUMAN_PRESENCE;
> +			min = prox_state->prox_attr.logical_minimum;
> +			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
>  			break;
>  		default:
>  			report_id = -1;
> @@ -95,7 +96,8 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  				prox_state->common_attributes.hsdev,
>  				HID_USAGE_SENSOR_PROX, address,
>  				report_id,
> -				SENSOR_HUB_SYNC);
> +				SENSOR_HUB_SYNC,
> +				min < 0);
>  			hid_sensor_power_state(&prox_state-
> >common_attributes,
>  						false);
>  		} else {
> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> index d55c4885211a..f3c0d41e5a8c 100644
> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> @@ -163,21 +163,23 @@ static int magn_3d_read_raw(struct iio_dev
> *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		hid_sensor_power_state(&magn_state-
> >magn_flux_attributes, true);
> -		report_id =
> -			magn_state->magn[chan->address].report_id;
> +		report_id = magn_state->magn[chan->address].report_id;
> +		min = magn_state->magn[chan->address].logical_minimum;
>  		address = magn_3d_addresses[chan->address];
>  		if (report_id >= 0)
>  			*val = sensor_hub_input_attr_get_raw_value(
>  				magn_state->magn_flux_attributes.hsdev,
>  				HID_USAGE_SENSOR_COMPASS_3D, address,
>  				report_id,
> -				SENSOR_HUB_SYNC);
> +				SENSOR_HUB_SYNC,
> +				min < 0);
>  		else {
>  			*val = 0;
>  			hid_sensor_power_state(
> diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c
> b/drivers/iio/orientation/hid-sensor-incl-3d.c
> index 1e5451d1ff88..bdc5e4554ee4 100644
> --- a/drivers/iio/orientation/hid-sensor-incl-3d.c
> +++ b/drivers/iio/orientation/hid-sensor-incl-3d.c
> @@ -111,21 +111,23 @@ static int incl_3d_read_raw(struct iio_dev
> *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		hid_sensor_power_state(&incl_state->common_attributes,
> true);
> -		report_id =
> -			incl_state->incl[chan->scan_index].report_id;
> +		report_id = incl_state->incl[chan-
> >scan_index].report_id;
> +		min = incl_state->incl[chan-
> >scan_index].logical_minimum;
>  		address = incl_3d_addresses[chan->scan_index];
>  		if (report_id >= 0)
>  			*val = sensor_hub_input_attr_get_raw_value(
>  				incl_state->common_attributes.hsdev,
>  				HID_USAGE_SENSOR_INCLINOMETER_3D,
> address,
>  				report_id,
> -				SENSOR_HUB_SYNC);
> +				SENSOR_HUB_SYNC,
> +				min < 0);
>  		else {
>  			hid_sensor_power_state(&incl_state-
> >common_attributes,
>  						false);
> diff --git a/drivers/iio/pressure/hid-sensor-press.c
> b/drivers/iio/pressure/hid-sensor-press.c
> index 4c437918f1d2..d7b1c00ceb4d 100644
> --- a/drivers/iio/pressure/hid-sensor-press.c
> +++ b/drivers/iio/pressure/hid-sensor-press.c
> @@ -77,6 +77,7 @@ static int press_read_raw(struct iio_dev
> *indio_dev,
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> +	s32 min;
>  
>  	*val = 0;
>  	*val2 = 0;
> @@ -85,8 +86,8 @@ static int press_read_raw(struct iio_dev
> *indio_dev,
>  		switch (chan->scan_index) {
>  		case  CHANNEL_SCAN_INDEX_PRESSURE:
>  			report_id = press_state->press_attr.report_id;
> -			address =
> -			HID_USAGE_SENSOR_ATMOSPHERIC_PRESSURE;
> +			min = press_state->press_attr.logical_minimum;
> +			address =
> HID_USAGE_SENSOR_ATMOSPHERIC_PRESSURE;
>  			break;
>  		default:
>  			report_id = -1;
> @@ -99,7 +100,8 @@ static int press_read_raw(struct iio_dev
> *indio_dev,
>  				press_state->common_attributes.hsdev,
>  				HID_USAGE_SENSOR_PRESSURE, address,
>  				report_id,
> -				SENSOR_HUB_SYNC);
> +				SENSOR_HUB_SYNC,
> +				min < 0);
>  			hid_sensor_power_state(&press_state-
> >common_attributes,
>  						false);
>  		} else {
> diff --git a/drivers/iio/temperature/hid-sensor-temperature.c
> b/drivers/iio/temperature/hid-sensor-temperature.c
> index beaf6fd3e337..b592fc4f007e 100644
> --- a/drivers/iio/temperature/hid-sensor-temperature.c
> +++ b/drivers/iio/temperature/hid-sensor-temperature.c
> @@ -76,7 +76,8 @@ static int temperature_read_raw(struct iio_dev
> *indio_dev,
>  			HID_USAGE_SENSOR_TEMPERATURE,
>  			HID_USAGE_SENSOR_DATA_ENVIRONMENTAL_TEMPERATURE
> ,
>  			temp_st->temperature_attr.report_id,
> -			SENSOR_HUB_SYNC);
> +			SENSOR_HUB_SYNC,
> +			temp_st->temperature_attr.logical_minimum < 0);
>  		hid_sensor_power_state(
>  				&temp_st->common_attributes,
>  				false);
> diff --git a/drivers/rtc/rtc-hid-sensor-time.c b/drivers/rtc/rtc-hid-
> sensor-time.c
> index 2751dba850c6..3e1abb455472 100644
> --- a/drivers/rtc/rtc-hid-sensor-time.c
> +++ b/drivers/rtc/rtc-hid-sensor-time.c
> @@ -213,7 +213,7 @@ static int hid_rtc_read_time(struct device *dev,
> struct rtc_time *tm)
>  	/* get a report with all values through requesting one value */
>  	sensor_hub_input_attr_get_raw_value(time_state-
> >common_attributes.hsdev,
>  			HID_USAGE_SENSOR_TIME, hid_time_addresses[0],
> -			time_state->info[0].report_id,
> SENSOR_HUB_SYNC);
> +			time_state->info[0].report_id, SENSOR_HUB_SYNC,
> false);
>  	/* wait for all values (event) */
>  	ret = wait_for_completion_killable_timeout(
>  			&time_state->comp_last_time, HZ*6);
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-
> sensor-hub.h
> index 331dc377c275..d1ea16abe008 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -190,7 +190,8 @@ enum sensor_hub_read_flags {
>  int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device
> *hsdev,
>   					u32 usage_id,
>   					u32 attr_usage_id, u32
> report_id,
> - 					enum sensor_hub_read_flags flag
> +					enum sensor_hub_read_flags
> flag,
> +					bool is_signed
>  );
>  
>  /**




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux