Re: [PATCH] iio: common: cros_ec_sensors: Add label attribute

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 Apr 2022 14:07:03 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> When sensor location is known, populate iio sysfs "label" attribute:
> 
> * "accel-base" : the sensor is in the base of the convertible (2-1)
>   device.
> * "accel-display" : the sensor is in the lid/display plane of the
>   device.
> 
> It apply to standalone accelerometer and IMU (accelerometer +
> gyroscope).
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

I'm happy with this change, but I think it would
be good for the patch description to include 'why' you want to
add this. I assume that's to make use of the standard ABI rather
than the custom ABI we introduced for this driver. We'd have
used label for this in the first place if it had been an option
at the time, but given we didn't what is the logic for duplicating
this information?

Thanks,

Jonathan

> ---
>  .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c    | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 989b109a82f47..f86b938b36bf8 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -325,6 +325,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  	indio_dev->name = pdev->name;
>  
>  	if (physical_device) {
> +		enum motionsensor_location loc;
> +
>  		state->param.cmd = MOTIONSENSE_CMD_INFO;
>  		state->param.info.sensor_num = sensor_platform->sensor_num;
>  		ret = cros_ec_motion_send_host_cmd(state, 0);
> @@ -333,7 +335,12 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
>  			return ret;
>  		}
>  		state->type = state->resp->info.type;
> -		state->loc = state->resp->info.location;
> +		loc = state->resp->info.location;
> +		if (loc == MOTIONSENSE_LOC_BASE)
> +			indio_dev->label = "accel-base";
> +		else if (loc == MOTIONSENSE_LOC_LID)
> +			indio_dev->label = "accel-display";
> +		state->loc = loc;
>  
>  		/* Set sign vector, only used for backward compatibility. */
>  		memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS);