| Message ID | 20220412210703.3842914-1-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: common: cros_ec_sensors: Add label attribute | expand |
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);
On Sat, Apr 16, 2022 at 7:02 AM Jonathan Cameron <jic23@kernel.org> wrote: > > 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? I removed the duplication in v2, I needed to add a define for the narrow case where a sensor is in a camera subassembly. Note the location attribute can apply to other sensors than accelerometers (gyroscope, light sensor). Thanks, Gwendal. > > 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); >
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);
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> --- .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)