| Message ID | 20210316124903.35011-26-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMI vendor protocols and modularization | expand |
Hi Christian, Thanks for porting this driver to the new interface. I have two questions: 1) In the following code snippet, can the sensor_ops be made as part of struct scmi_iio_priv as scmi_protocol_handle is also part of that struct. Not sure if there are any particular reasons for having the protocol handle as part of the struct but the sensor_ops as global? > +static const struct scmi_sensor_proto_ops *sensor_ops; >+ >.struct scmi_iio_priv { >- struct scmi_handle *handle; >+ struct scmi_protocol_handle *ph; > const struct scmi_sensor_info *sensor_info; > struct iio_dev *indio_dev; 2) Looks like the event notifier registration is SCMI device managed and that's why there is no call to unregister. I am assuming that you have tested the logic of inserting the module, removing it and then again inserting it to make sure that when the event is registered a second time , the logic is all good. > ret = handle->notify_ops->devm_event_notifier_register(sdev, Thanks, Jyoti On Tue, Mar 16, 2021 at 5:50 AM Cristian Marussi <cristian.marussi@arm.com> wrote: > > Port driver to the new SCMI Sensor interface based on protocol handles > and common devm_get_ops(). > > Cc: Jyoti Bhayana <jbhayana@google.com> > Cc: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > 1 file changed, 41 insertions(+), 50 deletions(-) > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > index 872d87ca6256..b4bdc3f3a946 100644 > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > @@ -21,8 +21,10 @@ > > #define SCMI_IIO_NUM_OF_AXIS 3 > > +static const struct scmi_sensor_proto_ops *sensor_ops; > + > struct scmi_iio_priv { > - struct scmi_handle *handle; > + struct scmi_protocol_handle *ph; > const struct scmi_sensor_info *sensor_info; > struct iio_dev *indio_dev; > /* adding one additional channel for timestamp */ > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > { > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > - u32 sensor_id = sensor->sensor_info->id; > u32 sensor_config = 0; > int err; > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > SCMI_SENS_CFG_SENSOR_ENABLE); > - > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > - &sensor_id, &sensor->sensor_update_nb); > - if (err) { > - dev_err(&iio_dev->dev, > - "Error in registering sensor update notifier for sensor %s err %d", > - sensor->sensor_info->name, err); > - return err; > - } > - > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > - sensor->sensor_info->id, sensor_config); > - if (err) { > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > - &sensor->sensor_update_nb); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > + if (err) > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > sensor->sensor_info->name, err); > - } > > return err; > } > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > { > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > - u32 sensor_id = sensor->sensor_info->id; > u32 sensor_config = 0; > int err; > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > SCMI_SENS_CFG_SENSOR_DISABLE); > - > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > - &sensor_id, &sensor->sensor_update_nb); > - if (err) { > - dev_err(&iio_dev->dev, > - "Error in unregistering sensor update notifier for sensor %s err %d", > - sensor->sensor_info->name, err); > - return err; > - } > - > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > - sensor_config); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in disabling sensor %s with err %d", > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > u32 sensor_config; > char buf[32]; > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > - sensor->sensor_info->id, &sensor_config); > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > + &sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in getting sensor config for sensor %s err %d", > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > sensor_config |= > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > - sensor->sensor_info->id, sensor_config); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > if (err) > dev_err(&iio_dev->dev, > "Error in setting sensor update interval for sensor %s value %u err %d", > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > u32 sensor_config; > int mult; > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > - sensor->sensor_info->id, &sensor_config); > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > + &sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in getting sensor config for sensor %s err %d", > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > return 0; > } > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > - struct scmi_handle *handle, > - const struct scmi_sensor_info *sensor_info) > +static struct iio_dev * > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > + const struct scmi_sensor_info *sensor_info) > { > struct iio_chan_spec *iio_channels; > struct scmi_iio_priv *sensor; > enum iio_modifier modifier; > enum iio_chan_type type; > struct iio_dev *iiodev; > + struct device *dev = &sdev->dev; > + const struct scmi_handle *handle = sdev->handle; > int i, ret; > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > iiodev->modes = INDIO_DIRECT_MODE; > iiodev->dev.parent = dev; > sensor = iio_priv(iiodev); > - sensor->handle = handle; > + sensor->ph = ph; > sensor->sensor_info = sensor_info; > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > sensor->indio_dev = iiodev; > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > sensor_info->axis[i].id); > } > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > + &sensor->sensor_info->id, > + &sensor->sensor_update_nb); > + if (ret) { > + dev_err(&iiodev->dev, > + "Error in registering sensor update notifier for sensor %s err %d", > + sensor->sensor_info->name, ret); > + return ERR_PTR(ret); > + } > + > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > iiodev->channels = iio_channels; > return iiodev; > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > { > const struct scmi_sensor_info *sensor_info; > struct scmi_handle *handle = sdev->handle; > + struct scmi_protocol_handle *ph; > struct device *dev = &sdev->dev; > struct iio_dev *scmi_iio_dev; > u16 nr_sensors; > int err = -ENODEV, i; > > - if (!handle || !handle->sensor_ops) { > + if (!handle) > + return -ENODEV; > + > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > + if (IS_ERR(sensor_ops)) { > dev_err(dev, "SCMI device has no sensor interface\n"); > - return -EINVAL; > + return PTR_ERR(sensor_ops); > } > > - nr_sensors = handle->sensor_ops->count_get(handle); > + nr_sensors = sensor_ops->count_get(ph); > if (!nr_sensors) { > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > return -ENODEV; > } > > for (i = 0; i < nr_sensors; i++) { > - sensor_info = handle->sensor_ops->info_get(handle, i); > + sensor_info = sensor_ops->info_get(ph, i); > if (!sensor_info) { > dev_err(dev, "SCMI sensor %d has missing info\n", i); > return -EINVAL; > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > sensor_info->axis[0].type != RADIANS_SEC) > continue; > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > if (IS_ERR(scmi_iio_dev)) { > dev_err(dev, > "failed to allocate IIO device for sensor %s: %ld\n", > -- > 2.17.1 >
Hi Jyoti, On Tue, Mar 16, 2021 at 10:51:48AM -0700, Jyoti Bhayana wrote: > Hi Christian, > > Thanks for porting this driver to the new interface. I have two questions: > > 1) In the following code snippet, can the sensor_ops be made as part > of struct scmi_iio_priv as scmi_protocol_handle is also part of that > struct. Not sure if there are any particular reasons for having the > protocol handle as part of the struct but the sensor_ops as global? > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > >+ > >.struct scmi_iio_priv { > >- struct scmi_handle *handle; > >+ struct scmi_protocol_handle *ph; > > const struct scmi_sensor_info *sensor_info; > > struct iio_dev *indio_dev; > So, the protocol_handle represents a reference to an instance of the sensor protocol initialized against the specific SCMI platform instance referred by the handle provided in the probe: so that if you should have multiple SCMI platforms defined in the DT, this driver (as all SCMI drivers) would be probed multiple times each time with a different handle and it would obtain a different protocol_handle each time it issued a probe() => devm_protocol_get(). For this reason the obtained protocol_handle cannot be kept as a global and it seemed a good fit to stay in the existent scmi_iio_priv. On the other side, the sensor_ops are just plain function pointers (as they were before), so on any possible probe they are returned the same (for the same proto): it is indeed the specified protocol_handle used at invocation time that determines against which instance they actually operates (if multiple instances present at all); so the sensor_ops seemed a good fit for a global, also because that way tehy are more easily accessible throughout the driver. Does it make sense ? > 2) Looks like the event notifier registration is SCMI device managed > and that's why there is no call to unregister. I am assuming that you > have tested the logic of inserting the module, removing it and then > again inserting it to make sure that when the event is registered a > second time , the logic is all good. > > > ret = handle->notify_ops->devm_event_notifier_register(sdev, > Sure, I tested load/unload in general for also other SCMI drivers just to be sure not to have broken completely the world with this series. Please find below a load/unload/load sequence for scmi_iio.ko. (there are 2 emulated sensors, only one is timestamped) Thanks Cristian --- (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ (debian-arm64)root@debarm64:~# insmod ./scmi_iio.ko (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ iio:device0 iio:device1 (debian-arm64)root@debarm64:~# cat /sys/bus/iio/devices/iio\:device0/name emu8_accel (debian-arm64)root@debarm64:~# cat /sys/bus/iio/devices/iio\:device1/name emu9_gyro (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 0 iio device number being used is 0 trigger-less mode selected Enabling all channels Enabling: in_accel_x_en Enabling: in_accel_z_en Enabling: in_timestamp_en Enabling: in_accel_y_en -7737 -7729 -7721 16725578467193651200.000000 -7736 -7728 -7720 16725578467193651200.000000 -7735 -7727 -7719 16725578467193651200.000000 -7734 -7726 -7718 16725578467193651200.000000 -7733 -7725 -7717 16725578467193651200.000000 -7732 -7724 -7716 16725578467193651200.000000 -7731 -7723 -7715 16725578467193651200.000000 -7730 -7722 -7714 17725578795097784320.000000 -7729 -7721 -7713 17725578795097784320.000000 -7728 -7720 -7712 17725578795097784320.000000 -7727 -7719 -7711 17725578795097784320.000000 -7726 -7718 -7710 17725578795097784320.000000 -7725 -7717 -7709 17725578795097784320.000000 -7724 -7716 -7708 17725578795097784320.000000 -7723 -7715 -7707 17725578795097784320.000000 -7722 -7714 -7706 17725578795097784320.000000 -7721 -7713 -7705 17725578795097784320.000000 ^CCaught signal 2 Disabling: in_accel_x_en Disabling: in_accel_z_en Disabling: in_timestamp_en Disabling: in_accel_y_en (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 1 iio device number being used is 1 trigger-less mode selected Enabling all channels Enabling: in_accel_x_en Enabling: in_accel_z_en Enabling: in_timestamp_en Enabling: in_accel_y_en -3583 -3574 -3565 270679244800.000000 -3582 -3573 -3564 270780203008.000000 -3581 -3572 -3563 270880358400.000000 -3580 -3571 -3562 270980431872.000000 -3579 -3570 -3561 271080521728.000000 -3578 -3569 -3560 271180611584.000000 -3577 -3568 -3559 271280685056.000000 -3576 -3567 -3558 271380774912.000000 -3575 -3566 -3557 271480848384.000000 -3574 -3565 -3556 271580938240.000000 -3573 -3564 -3555 271681028096.000000 -3572 -3563 -3554 271781101568.000000 -3571 -3562 -3553 271881191424.000000 -3570 -3561 -3552 271981264896.000000 -3569 -3560 -3551 272081354752.000000 ^CCaught signal 2 Disabling: in_accel_x_en Disabling: in_accel_z_en Disabling: in_timestamp_en Disabling: in_accel_y_en (debian-arm64)root@debarm64:~# (debian-arm64)root@debarm64:~# rmmod ./scmi_iio.ko (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ (debian-arm64)root@debarm64:~# insmod ./scmi_iio.ko (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ iio:device0 iio:device1 (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 0 iio device number being used is 0 trigger-less mode selected Enabling all channels Enabling: in_accel_x_en Enabling: in_accel_z_en Enabling: in_timestamp_en Enabling: in_accel_y_en -7308 -7300 -7292 4385346052291559424.000000 -7307 -7299 -7291 4385346052291559424.000000 -7306 -7298 -7290 4385346052291559424.000000 -7305 -7297 -7289 4385346052291559424.000000 -7304 -7296 -7288 4385346052291559424.000000 -7303 -7295 -7287 4385346052291559424.000000 -7302 -7294 -7286 4385346052291559424.000000 -7301 -7293 -7285 5385346380195692544.000000 -7300 -7292 -7284 5385346380195692544.000000 -7299 -7291 -7283 5385346380195692544.000000 -7298 -7290 -7282 5385346380195692544.000000 -7297 -7289 -7281 5385346380195692544.000000 -7296 -7288 -7280 5385346380195692544.000000 ^CCaught signal 2 Disabling: in_accel_x_en Disabling: in_accel_z_en Disabling: in_timestamp_en Disabling: in_accel_y_en (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 1 iio device number being used is 1 trigger-less mode selected Enabling all channels Enabling: in_accel_x_en Enabling: in_accel_z_en Enabling: in_timestamp_en Enabling: in_accel_y_en -3161 -3152 -3143 312944197632.000000 -3160 -3151 -3142 313044303872.000000 -3159 -3150 -3141 313144377344.000000 -3158 -3149 -3140 313244483584.000000 -3157 -3148 -3139 313344851968.000000 -3156 -3147 -3138 313445023744.000000 -3155 -3146 -3137 313545097216.000000 -3154 -3145 -3136 313645465600.000000 -3153 -3144 -3135 313745604608.000000 -3152 -3143 -3134 313845710848.000000 -3151 -3142 -3133 313945784320.000000 -3150 -3141 -3132 314045890560.000000 -3149 -3140 -3131 314145964032.000000 -3148 -3139 -3130 314246070272.000000 -3147 -3138 -3129 314346143744.000000 -3146 -3137 -3128 314446249984.000000 -3145 -3136 -3127 314546618368.000000 -3144 -3135 -3126 314647052288.000000 ^CCaught signal 2 Disabling: in_accel_x_en Disabling: in_accel_z_en Disabling: in_timestamp_en Disabling: in_accel_y_en > Thanks, > Jyoti > > On Tue, Mar 16, 2021 at 5:50 AM Cristian Marussi > <cristian.marussi@arm.com> wrote: > > > > Port driver to the new SCMI Sensor interface based on protocol handles > > and common devm_get_ops(). > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > index 872d87ca6256..b4bdc3f3a946 100644 > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > @@ -21,8 +21,10 @@ > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > + > > struct scmi_iio_priv { > > - struct scmi_handle *handle; > > + struct scmi_protocol_handle *ph; > > const struct scmi_sensor_info *sensor_info; > > struct iio_dev *indio_dev; > > /* adding one additional channel for timestamp */ > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > { > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > - u32 sensor_id = sensor->sensor_info->id; > > u32 sensor_config = 0; > > int err; > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > SCMI_SENS_CFG_SENSOR_ENABLE); > > - > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > - &sensor_id, &sensor->sensor_update_nb); > > - if (err) { > > - dev_err(&iio_dev->dev, > > - "Error in registering sensor update notifier for sensor %s err %d", > > - sensor->sensor_info->name, err); > > - return err; > > - } > > - > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > - sensor->sensor_info->id, sensor_config); > > - if (err) { > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > - &sensor->sensor_update_nb); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > + if (err) > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > sensor->sensor_info->name, err); > > - } > > > > return err; > > } > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > { > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > - u32 sensor_id = sensor->sensor_info->id; > > u32 sensor_config = 0; > > int err; > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > SCMI_SENS_CFG_SENSOR_DISABLE); > > - > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > - &sensor_id, &sensor->sensor_update_nb); > > - if (err) { > > - dev_err(&iio_dev->dev, > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > - sensor->sensor_info->name, err); > > - return err; > > - } > > - > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > - sensor_config); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in disabling sensor %s with err %d", > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > u32 sensor_config; > > char buf[32]; > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > - sensor->sensor_info->id, &sensor_config); > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > + &sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in getting sensor config for sensor %s err %d", > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > sensor_config |= > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > - sensor->sensor_info->id, sensor_config); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > if (err) > > dev_err(&iio_dev->dev, > > "Error in setting sensor update interval for sensor %s value %u err %d", > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > u32 sensor_config; > > int mult; > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > - sensor->sensor_info->id, &sensor_config); > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > + &sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in getting sensor config for sensor %s err %d", > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > return 0; > > } > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > - struct scmi_handle *handle, > > - const struct scmi_sensor_info *sensor_info) > > +static struct iio_dev * > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > + const struct scmi_sensor_info *sensor_info) > > { > > struct iio_chan_spec *iio_channels; > > struct scmi_iio_priv *sensor; > > enum iio_modifier modifier; > > enum iio_chan_type type; > > struct iio_dev *iiodev; > > + struct device *dev = &sdev->dev; > > + const struct scmi_handle *handle = sdev->handle; > > int i, ret; > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > iiodev->modes = INDIO_DIRECT_MODE; > > iiodev->dev.parent = dev; > > sensor = iio_priv(iiodev); > > - sensor->handle = handle; > > + sensor->ph = ph; > > sensor->sensor_info = sensor_info; > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > sensor->indio_dev = iiodev; > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > sensor_info->axis[i].id); > > } > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > + &sensor->sensor_info->id, > > + &sensor->sensor_update_nb); > > + if (ret) { > > + dev_err(&iiodev->dev, > > + "Error in registering sensor update notifier for sensor %s err %d", > > + sensor->sensor_info->name, ret); > > + return ERR_PTR(ret); > > + } > > + > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > iiodev->channels = iio_channels; > > return iiodev; > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > { > > const struct scmi_sensor_info *sensor_info; > > struct scmi_handle *handle = sdev->handle; > > + struct scmi_protocol_handle *ph; > > struct device *dev = &sdev->dev; > > struct iio_dev *scmi_iio_dev; > > u16 nr_sensors; > > int err = -ENODEV, i; > > > > - if (!handle || !handle->sensor_ops) { > > + if (!handle) > > + return -ENODEV; > > + > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > + if (IS_ERR(sensor_ops)) { > > dev_err(dev, "SCMI device has no sensor interface\n"); > > - return -EINVAL; > > + return PTR_ERR(sensor_ops); > > } > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > + nr_sensors = sensor_ops->count_get(ph); > > if (!nr_sensors) { > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > return -ENODEV; > > } > > > > for (i = 0; i < nr_sensors; i++) { > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > + sensor_info = sensor_ops->info_get(ph, i); > > if (!sensor_info) { > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > return -EINVAL; > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > sensor_info->axis[0].type != RADIANS_SEC) > > continue; > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > if (IS_ERR(scmi_iio_dev)) { > > dev_err(dev, > > "failed to allocate IIO device for sensor %s: %ld\n", > > -- > > 2.17.1 > >
Hi Christian, Thanks for the detailed explanation. Sounds good to me. Thanks, Jyoti On Tue, Mar 16, 2021 at 3:22 PM Cristian Marussi <cristian.marussi@arm.com> wrote: > > Hi Jyoti, > > On Tue, Mar 16, 2021 at 10:51:48AM -0700, Jyoti Bhayana wrote: > > Hi Christian, > > > > Thanks for porting this driver to the new interface. I have two questions: > > > > 1) In the following code snippet, can the sensor_ops be made as part > > of struct scmi_iio_priv as scmi_protocol_handle is also part of that > > struct. Not sure if there are any particular reasons for having the > > protocol handle as part of the struct but the sensor_ops as global? > > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > >+ > > >.struct scmi_iio_priv { > > >- struct scmi_handle *handle; > > >+ struct scmi_protocol_handle *ph; > > > const struct scmi_sensor_info *sensor_info; > > > struct iio_dev *indio_dev; > > > > So, the protocol_handle represents a reference to an instance of > the sensor protocol initialized against the specific SCMI platform > instance referred by the handle provided in the probe: so that if you > should have multiple SCMI platforms defined in the DT, this driver > (as all SCMI drivers) would be probed multiple times each time with a > different handle and it would obtain a different protocol_handle each > time it issued a probe() => devm_protocol_get(). > For this reason the obtained protocol_handle cannot be kept as a global > and it seemed a good fit to stay in the existent scmi_iio_priv. > > On the other side, the sensor_ops are just plain function pointers (as > they were before), so on any possible probe they are returned the same > (for the same proto): it is indeed the specified protocol_handle used > at invocation time that determines against which instance they actually > operates (if multiple instances present at all); so the sensor_ops seemed > a good fit for a global, also because that way tehy are more easily > accessible throughout the driver. > > Does it make sense ? > > > 2) Looks like the event notifier registration is SCMI device managed > > and that's why there is no call to unregister. I am assuming that you > > have tested the logic of inserting the module, removing it and then > > again inserting it to make sure that when the event is registered a > > second time , the logic is all good. > > > > > ret = handle->notify_ops->devm_event_notifier_register(sdev, > > > > Sure, I tested load/unload in general for also other SCMI drivers just > to be sure not to have broken completely the world with this series. > > Please find below a load/unload/load sequence for scmi_iio.ko. > (there are 2 emulated sensors, only one is timestamped) > > Thanks > > Cristian > > --- > > (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ > (debian-arm64)root@debarm64:~# insmod ./scmi_iio.ko > (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ > iio:device0 iio:device1 > (debian-arm64)root@debarm64:~# cat /sys/bus/iio/devices/iio\:device0/name > emu8_accel > (debian-arm64)root@debarm64:~# cat /sys/bus/iio/devices/iio\:device1/name > emu9_gyro > (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 0 > iio device number being used is 0 > trigger-less mode selected > Enabling all channels > Enabling: in_accel_x_en > Enabling: in_accel_z_en > Enabling: in_timestamp_en > Enabling: in_accel_y_en > -7737 -7729 -7721 16725578467193651200.000000 > -7736 -7728 -7720 16725578467193651200.000000 > -7735 -7727 -7719 16725578467193651200.000000 > -7734 -7726 -7718 16725578467193651200.000000 > -7733 -7725 -7717 16725578467193651200.000000 > -7732 -7724 -7716 16725578467193651200.000000 > -7731 -7723 -7715 16725578467193651200.000000 > -7730 -7722 -7714 17725578795097784320.000000 > -7729 -7721 -7713 17725578795097784320.000000 > -7728 -7720 -7712 17725578795097784320.000000 > -7727 -7719 -7711 17725578795097784320.000000 > -7726 -7718 -7710 17725578795097784320.000000 > -7725 -7717 -7709 17725578795097784320.000000 > -7724 -7716 -7708 17725578795097784320.000000 > -7723 -7715 -7707 17725578795097784320.000000 > -7722 -7714 -7706 17725578795097784320.000000 > -7721 -7713 -7705 17725578795097784320.000000 > ^CCaught signal 2 > Disabling: in_accel_x_en > Disabling: in_accel_z_en > Disabling: in_timestamp_en > Disabling: in_accel_y_en > (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 1 > iio device number being used is 1 > trigger-less mode selected > Enabling all channels > Enabling: in_accel_x_en > Enabling: in_accel_z_en > Enabling: in_timestamp_en > Enabling: in_accel_y_en > -3583 -3574 -3565 270679244800.000000 > -3582 -3573 -3564 270780203008.000000 > -3581 -3572 -3563 270880358400.000000 > -3580 -3571 -3562 270980431872.000000 > -3579 -3570 -3561 271080521728.000000 > -3578 -3569 -3560 271180611584.000000 > -3577 -3568 -3559 271280685056.000000 > -3576 -3567 -3558 271380774912.000000 > -3575 -3566 -3557 271480848384.000000 > -3574 -3565 -3556 271580938240.000000 > -3573 -3564 -3555 271681028096.000000 > -3572 -3563 -3554 271781101568.000000 > -3571 -3562 -3553 271881191424.000000 > -3570 -3561 -3552 271981264896.000000 > -3569 -3560 -3551 272081354752.000000 > ^CCaught signal 2 > Disabling: in_accel_x_en > Disabling: in_accel_z_en > Disabling: in_timestamp_en > Disabling: in_accel_y_en > (debian-arm64)root@debarm64:~# > (debian-arm64)root@debarm64:~# rmmod ./scmi_iio.ko > (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ > (debian-arm64)root@debarm64:~# insmod ./scmi_iio.ko > (debian-arm64)root@debarm64:~# ls /sys/bus/iio/devices/ > iio:device0 iio:device1 > (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 0 > iio device number being used is 0 > trigger-less mode selected > Enabling all channels > Enabling: in_accel_x_en > Enabling: in_accel_z_en > Enabling: in_timestamp_en > Enabling: in_accel_y_en > -7308 -7300 -7292 4385346052291559424.000000 > -7307 -7299 -7291 4385346052291559424.000000 > -7306 -7298 -7290 4385346052291559424.000000 > -7305 -7297 -7289 4385346052291559424.000000 > -7304 -7296 -7288 4385346052291559424.000000 > -7303 -7295 -7287 4385346052291559424.000000 > -7302 -7294 -7286 4385346052291559424.000000 > -7301 -7293 -7285 5385346380195692544.000000 > -7300 -7292 -7284 5385346380195692544.000000 > -7299 -7291 -7283 5385346380195692544.000000 > -7298 -7290 -7282 5385346380195692544.000000 > -7297 -7289 -7281 5385346380195692544.000000 > -7296 -7288 -7280 5385346380195692544.000000 > ^CCaught signal 2 > Disabling: in_accel_x_en > Disabling: in_accel_z_en > Disabling: in_timestamp_en > Disabling: in_accel_y_en > (debian-arm64)root@debarm64:~# /root/iio_generic_buffer -a -c -1 -g -l 100 -N 1 > iio device number being used is 1 > trigger-less mode selected > Enabling all channels > Enabling: in_accel_x_en > Enabling: in_accel_z_en > Enabling: in_timestamp_en > Enabling: in_accel_y_en > -3161 -3152 -3143 312944197632.000000 > -3160 -3151 -3142 313044303872.000000 > -3159 -3150 -3141 313144377344.000000 > -3158 -3149 -3140 313244483584.000000 > -3157 -3148 -3139 313344851968.000000 > -3156 -3147 -3138 313445023744.000000 > -3155 -3146 -3137 313545097216.000000 > -3154 -3145 -3136 313645465600.000000 > -3153 -3144 -3135 313745604608.000000 > -3152 -3143 -3134 313845710848.000000 > -3151 -3142 -3133 313945784320.000000 > -3150 -3141 -3132 314045890560.000000 > -3149 -3140 -3131 314145964032.000000 > -3148 -3139 -3130 314246070272.000000 > -3147 -3138 -3129 314346143744.000000 > -3146 -3137 -3128 314446249984.000000 > -3145 -3136 -3127 314546618368.000000 > -3144 -3135 -3126 314647052288.000000 > ^CCaught signal 2 > Disabling: in_accel_x_en > Disabling: in_accel_z_en > Disabling: in_timestamp_en > Disabling: in_accel_y_en > > > Thanks, > > Jyoti > > > > On Tue, Mar 16, 2021 at 5:50 AM Cristian Marussi > > <cristian.marussi@arm.com> wrote: > > > > > > Port driver to the new SCMI Sensor interface based on protocol handles > > > and common devm_get_ops(). > > > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > > Cc: Jonathan Cameron <jic23@kernel.org> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > --- > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > index 872d87ca6256..b4bdc3f3a946 100644 > > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > @@ -21,8 +21,10 @@ > > > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > > + > > > struct scmi_iio_priv { > > > - struct scmi_handle *handle; > > > + struct scmi_protocol_handle *ph; > > > const struct scmi_sensor_info *sensor_info; > > > struct iio_dev *indio_dev; > > > /* adding one additional channel for timestamp */ > > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > { > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > - u32 sensor_id = sensor->sensor_info->id; > > > u32 sensor_config = 0; > > > int err; > > > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > SCMI_SENS_CFG_SENSOR_ENABLE); > > > - > > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > - &sensor_id, &sensor->sensor_update_nb); > > > - if (err) { > > > - dev_err(&iio_dev->dev, > > > - "Error in registering sensor update notifier for sensor %s err %d", > > > - sensor->sensor_info->name, err); > > > - return err; > > > - } > > > - > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > - sensor->sensor_info->id, sensor_config); > > > - if (err) { > > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, > > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > > - &sensor->sensor_update_nb); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > + if (err) > > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > > sensor->sensor_info->name, err); > > > - } > > > > > > return err; > > > } > > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > > { > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > - u32 sensor_id = sensor->sensor_info->id; > > > u32 sensor_config = 0; > > > int err; > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > SCMI_SENS_CFG_SENSOR_DISABLE); > > > - > > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > - &sensor_id, &sensor->sensor_update_nb); > > > - if (err) { > > > - dev_err(&iio_dev->dev, > > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > > - sensor->sensor_info->name, err); > > > - return err; > > > - } > > > - > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > > - sensor_config); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in disabling sensor %s with err %d", > > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > u32 sensor_config; > > > char buf[32]; > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > - sensor->sensor_info->id, &sensor_config); > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > + &sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in getting sensor config for sensor %s err %d", > > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > sensor_config |= > > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > - sensor->sensor_info->id, sensor_config); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > if (err) > > > dev_err(&iio_dev->dev, > > > "Error in setting sensor update interval for sensor %s value %u err %d", > > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > > u32 sensor_config; > > > int mult; > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > - sensor->sensor_info->id, &sensor_config); > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > + &sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in getting sensor config for sensor %s err %d", > > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > return 0; > > > } > > > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > - struct scmi_handle *handle, > > > - const struct scmi_sensor_info *sensor_info) > > > +static struct iio_dev * > > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > > + const struct scmi_sensor_info *sensor_info) > > > { > > > struct iio_chan_spec *iio_channels; > > > struct scmi_iio_priv *sensor; > > > enum iio_modifier modifier; > > > enum iio_chan_type type; > > > struct iio_dev *iiodev; > > > + struct device *dev = &sdev->dev; > > > + const struct scmi_handle *handle = sdev->handle; > > > int i, ret; > > > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > iiodev->modes = INDIO_DIRECT_MODE; > > > iiodev->dev.parent = dev; > > > sensor = iio_priv(iiodev); > > > - sensor->handle = handle; > > > + sensor->ph = ph; > > > sensor->sensor_info = sensor_info; > > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > sensor->indio_dev = iiodev; > > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > sensor_info->axis[i].id); > > > } > > > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > + &sensor->sensor_info->id, > > > + &sensor->sensor_update_nb); > > > + if (ret) { > > > + dev_err(&iiodev->dev, > > > + "Error in registering sensor update notifier for sensor %s err %d", > > > + sensor->sensor_info->name, ret); > > > + return ERR_PTR(ret); > > > + } > > > + > > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > iiodev->channels = iio_channels; > > > return iiodev; > > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > { > > > const struct scmi_sensor_info *sensor_info; > > > struct scmi_handle *handle = sdev->handle; > > > + struct scmi_protocol_handle *ph; > > > struct device *dev = &sdev->dev; > > > struct iio_dev *scmi_iio_dev; > > > u16 nr_sensors; > > > int err = -ENODEV, i; > > > > > > - if (!handle || !handle->sensor_ops) { > > > + if (!handle) > > > + return -ENODEV; > > > + > > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > > + if (IS_ERR(sensor_ops)) { > > > dev_err(dev, "SCMI device has no sensor interface\n"); > > > - return -EINVAL; > > > + return PTR_ERR(sensor_ops); > > > } > > > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > > + nr_sensors = sensor_ops->count_get(ph); > > > if (!nr_sensors) { > > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > > return -ENODEV; > > > } > > > > > > for (i = 0; i < nr_sensors; i++) { > > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > > + sensor_info = sensor_ops->info_get(ph, i); > > > if (!sensor_info) { > > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > > return -EINVAL; > > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > sensor_info->axis[0].type != RADIANS_SEC) > > > continue; > > > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > > if (IS_ERR(scmi_iio_dev)) { > > > dev_err(dev, > > > "failed to allocate IIO device for sensor %s: %ld\n", > > > -- > > > 2.17.1 > > >
On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote: > Hi Christian, > > Thanks for the detailed explanation. Sounds good to me. > Can I get official Reviewed-by or Acked-by please if you are fine with the change ? I definitely need one from Jonathan to merge this and one from Jyoti is added bonus
iio/scmi: port driver to the new scmi_sensor_proto_ops interface
Port driver to the new SCMI Sensor interface based on protocol handles
and common devm_get_ops().
Cc: Jyoti Bhayana <jbhayana@google.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Acked-by: Jyoti Bhayana <jbhayana@google.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
On Thu, Mar 18, 2021 at 5:12 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote:
> > Hi Christian,
> >
> > Thanks for the detailed explanation. Sounds good to me.
> >
>
> Can I get official Reviewed-by or Acked-by please if you are fine with the
> change ?
>
> I definitely need one from Jonathan to merge this and one from Jyoti is
> added bonus .
>
> --
> Regards,
> Sudeep
Hi Jonathan, On Thu, Mar 18, 2021 at 12:12:02PM +0000, Sudeep Holla wrote: > On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote: > > Hi Christian, > > > > Thanks for the detailed explanation. Sounds good to me. > > > > Can I get official Reviewed-by or Acked-by please if you are fine with the > change ? > I need your ack to this via arm-soc.
On Tue, 23 Mar 2021 09:48:28 +0000 Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Jonathan, > > On Thu, Mar 18, 2021 at 12:12:02PM +0000, Sudeep Holla wrote: > > On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote: > > > Hi Christian, > > > > > > Thanks for the detailed explanation. Sounds good to me. > > > > > > > Can I get official Reviewed-by or Acked-by please if you are fine with the > > change ? > > > > I need your ack to this via arm-soc. > Sorry, missed it until now as wasn't cc'd to linux-iio@vger.kernel.org so my filters didn't sweep it to the right place and it got lost in the flood. Will take a look now. Jonathan
On Tue, 16 Mar 2021 12:48:50 +0000 Cristian Marussi <cristian.marussi@arm.com> wrote: > Port driver to the new SCMI Sensor interface based on protocol handles > and common devm_get_ops(). > > Cc: Jyoti Bhayana <jbhayana@google.com> > Cc: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> +CC linux-iio@vger.kernel.org Rule of thumb if it doesn't go there it ends up in randomly location based on other lists and I might not see it for a few weeks :( > --- > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > 1 file changed, 41 insertions(+), 50 deletions(-) > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > index 872d87ca6256..b4bdc3f3a946 100644 > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > @@ -21,8 +21,10 @@ > > #define SCMI_IIO_NUM_OF_AXIS 3 > > +static const struct scmi_sensor_proto_ops *sensor_ops; Hmm. I'm not keen on globals when they really should not be necessary. They just result in lifetimes being out of sync. Here you are fine because you set it to an appropriate value as the first thing you do in probe, and I assume the function only ever returns on answer on repeated calls. Why not put a copy of that pointer inside the struct scmi_iio_priv structures? Otherwise this all looks like straight forward refactoring so given the above is more a 'bad smell' than a bug and I'm rather late to the game. Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > + > struct scmi_iio_priv { > - struct scmi_handle *handle; > + struct scmi_protocol_handle *ph; > const struct scmi_sensor_info *sensor_info; > struct iio_dev *indio_dev; > /* adding one additional channel for timestamp */ > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > { > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > - u32 sensor_id = sensor->sensor_info->id; > u32 sensor_config = 0; > int err; > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > SCMI_SENS_CFG_SENSOR_ENABLE); > - > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > - &sensor_id, &sensor->sensor_update_nb); > - if (err) { > - dev_err(&iio_dev->dev, > - "Error in registering sensor update notifier for sensor %s err %d", > - sensor->sensor_info->name, err); > - return err; > - } > - > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > - sensor->sensor_info->id, sensor_config); > - if (err) { > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > - &sensor->sensor_update_nb); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > + if (err) > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > sensor->sensor_info->name, err); > - } > > return err; > } > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > { > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > - u32 sensor_id = sensor->sensor_info->id; > u32 sensor_config = 0; > int err; > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > SCMI_SENS_CFG_SENSOR_DISABLE); > - > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > - &sensor_id, &sensor->sensor_update_nb); > - if (err) { > - dev_err(&iio_dev->dev, > - "Error in unregistering sensor update notifier for sensor %s err %d", > - sensor->sensor_info->name, err); > - return err; > - } > - > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > - sensor_config); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in disabling sensor %s with err %d", > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > u32 sensor_config; > char buf[32]; > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > - sensor->sensor_info->id, &sensor_config); > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > + &sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in getting sensor config for sensor %s err %d", > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > sensor_config |= > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > - sensor->sensor_info->id, sensor_config); > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > + sensor_config); > if (err) > dev_err(&iio_dev->dev, > "Error in setting sensor update interval for sensor %s value %u err %d", > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > u32 sensor_config; > int mult; > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > - sensor->sensor_info->id, &sensor_config); > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > + &sensor_config); > if (err) { > dev_err(&iio_dev->dev, > "Error in getting sensor config for sensor %s err %d", > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > return 0; > } > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > - struct scmi_handle *handle, > - const struct scmi_sensor_info *sensor_info) > +static struct iio_dev * > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > + const struct scmi_sensor_info *sensor_info) > { > struct iio_chan_spec *iio_channels; > struct scmi_iio_priv *sensor; > enum iio_modifier modifier; > enum iio_chan_type type; > struct iio_dev *iiodev; > + struct device *dev = &sdev->dev; > + const struct scmi_handle *handle = sdev->handle; > int i, ret; > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > iiodev->modes = INDIO_DIRECT_MODE; > iiodev->dev.parent = dev; > sensor = iio_priv(iiodev); > - sensor->handle = handle; > + sensor->ph = ph; > sensor->sensor_info = sensor_info; > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > sensor->indio_dev = iiodev; > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > sensor_info->axis[i].id); > } > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > + &sensor->sensor_info->id, > + &sensor->sensor_update_nb); > + if (ret) { > + dev_err(&iiodev->dev, > + "Error in registering sensor update notifier for sensor %s err %d", > + sensor->sensor_info->name, ret); > + return ERR_PTR(ret); > + } > + > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > iiodev->channels = iio_channels; > return iiodev; > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > { > const struct scmi_sensor_info *sensor_info; > struct scmi_handle *handle = sdev->handle; > + struct scmi_protocol_handle *ph; > struct device *dev = &sdev->dev; > struct iio_dev *scmi_iio_dev; > u16 nr_sensors; > int err = -ENODEV, i; > > - if (!handle || !handle->sensor_ops) { > + if (!handle) > + return -ENODEV; > + > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > + if (IS_ERR(sensor_ops)) { > dev_err(dev, "SCMI device has no sensor interface\n"); > - return -EINVAL; > + return PTR_ERR(sensor_ops); > } > > - nr_sensors = handle->sensor_ops->count_get(handle); > + nr_sensors = sensor_ops->count_get(ph); > if (!nr_sensors) { > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > return -ENODEV; > } > > for (i = 0; i < nr_sensors; i++) { > - sensor_info = handle->sensor_ops->info_get(handle, i); > + sensor_info = sensor_ops->info_get(ph, i); > if (!sensor_info) { > dev_err(dev, "SCMI sensor %d has missing info\n", i); > return -EINVAL; > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > sensor_info->axis[0].type != RADIANS_SEC) > continue; > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > if (IS_ERR(scmi_iio_dev)) { > dev_err(dev, > "failed to allocate IIO device for sensor %s: %ld\n",
Hi Jonathan, On Tue, Mar 30, 2021 at 12:33:25PM +0100, Jonathan Cameron wrote: > On Tue, 16 Mar 2021 12:48:50 +0000 > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > Port driver to the new SCMI Sensor interface based on protocol handles > > and common devm_get_ops(). > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > +CC linux-iio@vger.kernel.org > > Rule of thumb if it doesn't go there it ends up in randomly location based > on other lists and I might not see it for a few weeks :( > Ah sorry, I thought the direct CC was enough. > > --- > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > index 872d87ca6256..b4bdc3f3a946 100644 > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > @@ -21,8 +21,10 @@ > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > Hmm. I'm not keen on globals when they really should not be necessary. > They just result in lifetimes being out of sync. Here you are fine because > you set it to an appropriate value as the first thing you do in probe, and > I assume the function only ever returns on answer on repeated calls. > > Why not put a copy of that pointer inside the struct scmi_iio_priv structures? > The reason for this, as I said to Jyoyi who made the same comment indeed, from my point of view (maybe wrong..) was that while the protocol_handle, and previously the handle, are 'per-instance data' (so that you get a different one each time this driver is possibly probed against a different platform-handle) and as such are stored in scmi_iio_priv, the _ops are just plain code pointers and are returned always the same for the same protocol no matter how many times you probe this driver: you just end up calling them against the proper different saved protocol_handle; so it seemed to me an unneeded duplication to stick a copy of the same _ops inside each per-instance scmi_iio_priv, and at the same time it seemed also more straigthforward to access them without too many indirections from inside the scmi_iio_priv struct). But if these are not valid points I can change this in IIO now, and in the future also in all the other SCMI drivers that currently use this same API and pattern of usage with global ops. (..at least because I'd have to collect again all the other ACks agains and it's a bit later for that now) Thanks Cristian > Otherwise this all looks like straight forward refactoring so given the > above is more a 'bad smell' than a bug and I'm rather late to the game. > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > + > > struct scmi_iio_priv { > > - struct scmi_handle *handle; > > + struct scmi_protocol_handle *ph; > > const struct scmi_sensor_info *sensor_info; > > struct iio_dev *indio_dev; > > /* adding one additional channel for timestamp */ > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > { > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > - u32 sensor_id = sensor->sensor_info->id; > > u32 sensor_config = 0; > > int err; > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > SCMI_SENS_CFG_SENSOR_ENABLE); > > - > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > - &sensor_id, &sensor->sensor_update_nb); > > - if (err) { > > - dev_err(&iio_dev->dev, > > - "Error in registering sensor update notifier for sensor %s err %d", > > - sensor->sensor_info->name, err); > > - return err; > > - } > > - > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > - sensor->sensor_info->id, sensor_config); > > - if (err) { > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > - &sensor->sensor_update_nb); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > + if (err) > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > sensor->sensor_info->name, err); > > - } > > > > return err; > > } > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > { > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > - u32 sensor_id = sensor->sensor_info->id; > > u32 sensor_config = 0; > > int err; > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > SCMI_SENS_CFG_SENSOR_DISABLE); > > - > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > - &sensor_id, &sensor->sensor_update_nb); > > - if (err) { > > - dev_err(&iio_dev->dev, > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > - sensor->sensor_info->name, err); > > - return err; > > - } > > - > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > - sensor_config); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in disabling sensor %s with err %d", > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > u32 sensor_config; > > char buf[32]; > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > - sensor->sensor_info->id, &sensor_config); > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > + &sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in getting sensor config for sensor %s err %d", > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > sensor_config |= > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > - sensor->sensor_info->id, sensor_config); > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > + sensor_config); > > if (err) > > dev_err(&iio_dev->dev, > > "Error in setting sensor update interval for sensor %s value %u err %d", > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > u32 sensor_config; > > int mult; > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > - sensor->sensor_info->id, &sensor_config); > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > + &sensor_config); > > if (err) { > > dev_err(&iio_dev->dev, > > "Error in getting sensor config for sensor %s err %d", > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > return 0; > > } > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > - struct scmi_handle *handle, > > - const struct scmi_sensor_info *sensor_info) > > +static struct iio_dev * > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > + const struct scmi_sensor_info *sensor_info) > > { > > struct iio_chan_spec *iio_channels; > > struct scmi_iio_priv *sensor; > > enum iio_modifier modifier; > > enum iio_chan_type type; > > struct iio_dev *iiodev; > > + struct device *dev = &sdev->dev; > > + const struct scmi_handle *handle = sdev->handle; > > int i, ret; > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > iiodev->modes = INDIO_DIRECT_MODE; > > iiodev->dev.parent = dev; > > sensor = iio_priv(iiodev); > > - sensor->handle = handle; > > + sensor->ph = ph; > > sensor->sensor_info = sensor_info; > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > sensor->indio_dev = iiodev; > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > sensor_info->axis[i].id); > > } > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > + &sensor->sensor_info->id, > > + &sensor->sensor_update_nb); > > + if (ret) { > > + dev_err(&iiodev->dev, > > + "Error in registering sensor update notifier for sensor %s err %d", > > + sensor->sensor_info->name, ret); > > + return ERR_PTR(ret); > > + } > > + > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > iiodev->channels = iio_channels; > > return iiodev; > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > { > > const struct scmi_sensor_info *sensor_info; > > struct scmi_handle *handle = sdev->handle; > > + struct scmi_protocol_handle *ph; > > struct device *dev = &sdev->dev; > > struct iio_dev *scmi_iio_dev; > > u16 nr_sensors; > > int err = -ENODEV, i; > > > > - if (!handle || !handle->sensor_ops) { > > + if (!handle) > > + return -ENODEV; > > + > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > + if (IS_ERR(sensor_ops)) { > > dev_err(dev, "SCMI device has no sensor interface\n"); > > - return -EINVAL; > > + return PTR_ERR(sensor_ops); > > } > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > + nr_sensors = sensor_ops->count_get(ph); > > if (!nr_sensors) { > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > return -ENODEV; > > } > > > > for (i = 0; i < nr_sensors; i++) { > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > + sensor_info = sensor_ops->info_get(ph, i); > > if (!sensor_info) { > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > return -EINVAL; > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > sensor_info->axis[0].type != RADIANS_SEC) > > continue; > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > if (IS_ERR(scmi_iio_dev)) { > > dev_err(dev, > > "failed to allocate IIO device for sensor %s: %ld\n", >
On Tue, 30 Mar 2021 13:51:13 +0100 Cristian Marussi <cristian.marussi@arm.com> wrote: > Hi Jonathan, > > On Tue, Mar 30, 2021 at 12:33:25PM +0100, Jonathan Cameron wrote: > > On Tue, 16 Mar 2021 12:48:50 +0000 > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > Port driver to the new SCMI Sensor interface based on protocol handles > > > and common devm_get_ops(). > > > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > > Cc: Jonathan Cameron <jic23@kernel.org> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > +CC linux-iio@vger.kernel.org > > > > Rule of thumb if it doesn't go there it ends up in randomly location based > > on other lists and I might not see it for a few weeks :( > > > > Ah sorry, I thought the direct CC was enough. No problem. Too much email :) > > > > --- > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > index 872d87ca6256..b4bdc3f3a946 100644 > > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > @@ -21,8 +21,10 @@ > > > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > > > Hmm. I'm not keen on globals when they really should not be necessary. > > They just result in lifetimes being out of sync. Here you are fine because > > you set it to an appropriate value as the first thing you do in probe, and > > I assume the function only ever returns on answer on repeated calls. > > > > Why not put a copy of that pointer inside the struct scmi_iio_priv structures? > > > > The reason for this, as I said to Jyoyi who made the same comment indeed, > from my point of view (maybe wrong..) was that while the protocol_handle, > and previously the handle, are 'per-instance data' (so that you get a > different one each time this driver is possibly probed against a different > platform-handle) and as such are stored in scmi_iio_priv, the _ops are > just plain code pointers and are returned always the same for the same > protocol no matter how many times you probe this driver: As that's the case, I'm a little confused to why you have added the complexity of a query interface in the first place? Why not just export the ops and have the various drivers access them directly? If there is only one set of scmi_sensor_ops etc, then let drivers at it directly, or indeed export the functions that make up the ops structure directly. This sounds like a bit of abstraction that only serves to make the code harder to read. > you just end up > calling them against the proper different saved protocol_handle; so it > seemed to me an unneeded duplication to stick a copy of the same _ops > inside each per-instance scmi_iio_priv, and at the same time it seemed > also more straigthforward to access them without too many indirections > from inside the scmi_iio_priv struct). > > But if these are not valid points I can change this in IIO now, and in > the future also in all the other SCMI drivers that currently use this > same API and pattern of usage with global ops. (..at least because I'd > have to collect again all the other ACks agains and it's a bit later for > that now) I'm fine with leaving it as is. There's no fundamental issue, it's just a little bit ugly and I'm fussy :) > > Thanks > > Cristian > > > Otherwise this all looks like straight forward refactoring so given the > > above is more a 'bad smell' than a bug and I'm rather late to the game. > > > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > + > > > struct scmi_iio_priv { > > > - struct scmi_handle *handle; > > > + struct scmi_protocol_handle *ph; > > > const struct scmi_sensor_info *sensor_info; > > > struct iio_dev *indio_dev; > > > /* adding one additional channel for timestamp */ > > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > { > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > - u32 sensor_id = sensor->sensor_info->id; > > > u32 sensor_config = 0; > > > int err; > > > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > SCMI_SENS_CFG_SENSOR_ENABLE); > > > - > > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > - &sensor_id, &sensor->sensor_update_nb); > > > - if (err) { > > > - dev_err(&iio_dev->dev, > > > - "Error in registering sensor update notifier for sensor %s err %d", > > > - sensor->sensor_info->name, err); > > > - return err; > > > - } > > > - > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > - sensor->sensor_info->id, sensor_config); > > > - if (err) { > > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, > > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > > - &sensor->sensor_update_nb); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > + if (err) > > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > > sensor->sensor_info->name, err); > > > - } > > > > > > return err; > > > } > > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > > { > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > - u32 sensor_id = sensor->sensor_info->id; > > > u32 sensor_config = 0; > > > int err; > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > SCMI_SENS_CFG_SENSOR_DISABLE); > > > - > > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > - &sensor_id, &sensor->sensor_update_nb); > > > - if (err) { > > > - dev_err(&iio_dev->dev, > > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > > - sensor->sensor_info->name, err); > > > - return err; > > > - } > > > - > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > > - sensor_config); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in disabling sensor %s with err %d", > > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > u32 sensor_config; > > > char buf[32]; > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > - sensor->sensor_info->id, &sensor_config); > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > + &sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in getting sensor config for sensor %s err %d", > > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > sensor_config |= > > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > - sensor->sensor_info->id, sensor_config); > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > + sensor_config); > > > if (err) > > > dev_err(&iio_dev->dev, > > > "Error in setting sensor update interval for sensor %s value %u err %d", > > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > > u32 sensor_config; > > > int mult; > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > - sensor->sensor_info->id, &sensor_config); > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > + &sensor_config); > > > if (err) { > > > dev_err(&iio_dev->dev, > > > "Error in getting sensor config for sensor %s err %d", > > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > return 0; > > > } > > > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > - struct scmi_handle *handle, > > > - const struct scmi_sensor_info *sensor_info) > > > +static struct iio_dev * > > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > > + const struct scmi_sensor_info *sensor_info) > > > { > > > struct iio_chan_spec *iio_channels; > > > struct scmi_iio_priv *sensor; > > > enum iio_modifier modifier; > > > enum iio_chan_type type; > > > struct iio_dev *iiodev; > > > + struct device *dev = &sdev->dev; > > > + const struct scmi_handle *handle = sdev->handle; > > > int i, ret; > > > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > iiodev->modes = INDIO_DIRECT_MODE; > > > iiodev->dev.parent = dev; > > > sensor = iio_priv(iiodev); > > > - sensor->handle = handle; > > > + sensor->ph = ph; > > > sensor->sensor_info = sensor_info; > > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > sensor->indio_dev = iiodev; > > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > sensor_info->axis[i].id); > > > } > > > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > + &sensor->sensor_info->id, > > > + &sensor->sensor_update_nb); > > > + if (ret) { > > > + dev_err(&iiodev->dev, > > > + "Error in registering sensor update notifier for sensor %s err %d", > > > + sensor->sensor_info->name, ret); > > > + return ERR_PTR(ret); > > > + } > > > + > > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > iiodev->channels = iio_channels; > > > return iiodev; > > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > { > > > const struct scmi_sensor_info *sensor_info; > > > struct scmi_handle *handle = sdev->handle; > > > + struct scmi_protocol_handle *ph; > > > struct device *dev = &sdev->dev; > > > struct iio_dev *scmi_iio_dev; > > > u16 nr_sensors; > > > int err = -ENODEV, i; > > > > > > - if (!handle || !handle->sensor_ops) { > > > + if (!handle) > > > + return -ENODEV; > > > + > > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > > + if (IS_ERR(sensor_ops)) { > > > dev_err(dev, "SCMI device has no sensor interface\n"); > > > - return -EINVAL; > > > + return PTR_ERR(sensor_ops); > > > } > > > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > > + nr_sensors = sensor_ops->count_get(ph); > > > if (!nr_sensors) { > > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > > return -ENODEV; > > > } > > > > > > for (i = 0; i < nr_sensors; i++) { > > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > > + sensor_info = sensor_ops->info_get(ph, i); > > > if (!sensor_info) { > > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > > return -EINVAL; > > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > sensor_info->axis[0].type != RADIANS_SEC) > > > continue; > > > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > > if (IS_ERR(scmi_iio_dev)) { > > > dev_err(dev, > > > "failed to allocate IIO device for sensor %s: %ld\n", > >
Hi Jonathan On Tue, Mar 30, 2021 at 06:34:04PM +0100, Jonathan Cameron wrote: > On Tue, 30 Mar 2021 13:51:13 +0100 > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > Hi Jonathan, > > > > On Tue, Mar 30, 2021 at 12:33:25PM +0100, Jonathan Cameron wrote: > > > On Tue, 16 Mar 2021 12:48:50 +0000 > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > Port driver to the new SCMI Sensor interface based on protocol handles > > > > and common devm_get_ops(). > > > > > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > > > Cc: Jonathan Cameron <jic23@kernel.org> > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > +CC linux-iio@vger.kernel.org > > > > > > Rule of thumb if it doesn't go there it ends up in randomly location based > > > on other lists and I might not see it for a few weeks :( > > > > > > > Ah sorry, I thought the direct CC was enough. > > No problem. Too much email :) > > > > > > > --- > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > index 872d87ca6256..b4bdc3f3a946 100644 > > > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > @@ -21,8 +21,10 @@ > > > > > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > > > > > Hmm. I'm not keen on globals when they really should not be necessary. > > > They just result in lifetimes being out of sync. Here you are fine because > > > you set it to an appropriate value as the first thing you do in probe, and > > > I assume the function only ever returns on answer on repeated calls. > > > > > > Why not put a copy of that pointer inside the struct scmi_iio_priv structures? > > > > > > > The reason for this, as I said to Jyoyi who made the same comment indeed, > > from my point of view (maybe wrong..) was that while the protocol_handle, > > and previously the handle, are 'per-instance data' (so that you get a > > different one each time this driver is possibly probed against a different > > platform-handle) and as such are stored in scmi_iio_priv, the _ops are > > just plain code pointers and are returned always the same for the same > > protocol no matter how many times you probe this driver: > > As that's the case, I'm a little confused to why you have added the complexity > of a query interface in the first place? Why not just export the ops and > have the various drivers access them directly? If there is only > one set of scmi_sensor_ops etc, then let drivers at it directly, or > indeed export the functions that make up the ops structure directly. > > This sounds like a bit of abstraction that only serves to make the > code harder to read. > Thanks for your comments, let me explain a bit. While the ops are indeed per-protocol common code available to SCMI drivers, the protocol handle, which you also obtain with devm_protocol_get(), is instead an opaque reference bound to the specific protocol instance associated to the platform handle you're using, so that, in case there are multiple SCMI platforms defined on the system, you'll get, at each probe a specific and distinct protocol_handle to use with your ops: this way you'll act upon a completely distinct initialized protocol stack, using a distinct underlying transport layer toward your platform of choice. Since this series wanted to unify SCMI standard and custom protocol interfaces and enable modularization too, the get/put abstraction is there indeed to be able to track internally protocol users and do resource accounting so that an SCMI driver has to explicitly ask to use a protocol: the protocols instances are then initialized on demand only when the first user shows up and more importantly a hold is put on the protocol module refs avoiding its possible unloading while still in use (even though only custom protocol are allowed as loadable modules as of now...) Coming to the ops instead, the reason not to simply export them was...well... ...not to export new symbols, and not just to stick to the old handle->ops() interface way of non-exporting ops, or because I'm pavid at exporting new symbols (I am :D), but because the idea was that in this way it would also have been easier for vendors writing custom protocol modules to be able to just use them in their own SCMI driver without the need to export also their new custom ops. (and same goes generally for any new possible future standard protocol which will not require endlessly further exports) Just for these reasons I attached the ops retrieval the same protocol_get() interface already introduced to handle all of the above. > > you just end up > > calling them against the proper different saved protocol_handle; so it > > seemed to me an unneeded duplication to stick a copy of the same _ops > > inside each per-instance scmi_iio_priv, and at the same time it seemed > > also more straigthforward to access them without too many indirections > > from inside the scmi_iio_priv struct). > > > > But if these are not valid points I can change this in IIO now, and in > > the future also in all the other SCMI drivers that currently use this > > same API and pattern of usage with global ops. (..at least because I'd > > have to collect again all the other ACks agains and it's a bit later for > > that now) > > I'm fine with leaving it as is. There's no fundamental issue, it's just > a little bit ugly and I'm fussy :) > I'm not fullly liking it too, but it was the best I could come up to cope with the above reqs (and the limited amount of my grey-matter :D) But if in the future we can come up with something better, or some reqs are dropped/revisited I'll be happy to flood the list with another jumbo-series. Thanks Cristian > > > > Thanks > > > > Cristian > > > > > Otherwise this all looks like straight forward refactoring so given the > > > above is more a 'bad smell' than a bug and I'm rather late to the game. > > > > > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > + > > > > struct scmi_iio_priv { > > > > - struct scmi_handle *handle; > > > > + struct scmi_protocol_handle *ph; > > > > const struct scmi_sensor_info *sensor_info; > > > > struct iio_dev *indio_dev; > > > > /* adding one additional channel for timestamp */ > > > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > { > > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > - u32 sensor_id = sensor->sensor_info->id; > > > > u32 sensor_config = 0; > > > > int err; > > > > > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > > SCMI_SENS_CFG_SENSOR_ENABLE); > > > > - > > > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > - &sensor_id, &sensor->sensor_update_nb); > > > > - if (err) { > > > > - dev_err(&iio_dev->dev, > > > > - "Error in registering sensor update notifier for sensor %s err %d", > > > > - sensor->sensor_info->name, err); > > > > - return err; > > > > - } > > > > - > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > > - sensor->sensor_info->id, sensor_config); > > > > - if (err) { > > > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > > - SCMI_PROTOCOL_SENSOR, > > > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > > > - &sensor->sensor_update_nb); > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > + sensor_config); > > > > + if (err) > > > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > > > sensor->sensor_info->name, err); > > > > - } > > > > > > > > return err; > > > > } > > > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > > > { > > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > - u32 sensor_id = sensor->sensor_info->id; > > > > u32 sensor_config = 0; > > > > int err; > > > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > > SCMI_SENS_CFG_SENSOR_DISABLE); > > > > - > > > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > - &sensor_id, &sensor->sensor_update_nb); > > > > - if (err) { > > > > - dev_err(&iio_dev->dev, > > > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > > > - sensor->sensor_info->name, err); > > > > - return err; > > > > - } > > > > - > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > > > - sensor_config); > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > + sensor_config); > > > > if (err) { > > > > dev_err(&iio_dev->dev, > > > > "Error in disabling sensor %s with err %d", > > > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > > u32 sensor_config; > > > > char buf[32]; > > > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > > - sensor->sensor_info->id, &sensor_config); > > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > > + &sensor_config); > > > > if (err) { > > > > dev_err(&iio_dev->dev, > > > > "Error in getting sensor config for sensor %s err %d", > > > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > > sensor_config |= > > > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > > - sensor->sensor_info->id, sensor_config); > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > + sensor_config); > > > > if (err) > > > > dev_err(&iio_dev->dev, > > > > "Error in setting sensor update interval for sensor %s value %u err %d", > > > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > > > u32 sensor_config; > > > > int mult; > > > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > > - sensor->sensor_info->id, &sensor_config); > > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > > + &sensor_config); > > > > if (err) { > > > > dev_err(&iio_dev->dev, > > > > "Error in getting sensor config for sensor %s err %d", > > > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > return 0; > > > > } > > > > > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > - struct scmi_handle *handle, > > > > - const struct scmi_sensor_info *sensor_info) > > > > +static struct iio_dev * > > > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > > > + const struct scmi_sensor_info *sensor_info) > > > > { > > > > struct iio_chan_spec *iio_channels; > > > > struct scmi_iio_priv *sensor; > > > > enum iio_modifier modifier; > > > > enum iio_chan_type type; > > > > struct iio_dev *iiodev; > > > > + struct device *dev = &sdev->dev; > > > > + const struct scmi_handle *handle = sdev->handle; > > > > int i, ret; > > > > > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > iiodev->modes = INDIO_DIRECT_MODE; > > > > iiodev->dev.parent = dev; > > > > sensor = iio_priv(iiodev); > > > > - sensor->handle = handle; > > > > + sensor->ph = ph; > > > > sensor->sensor_info = sensor_info; > > > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > sensor->indio_dev = iiodev; > > > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > sensor_info->axis[i].id); > > > > } > > > > > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > + &sensor->sensor_info->id, > > > > + &sensor->sensor_update_nb); > > > > + if (ret) { > > > > + dev_err(&iiodev->dev, > > > > + "Error in registering sensor update notifier for sensor %s err %d", > > > > + sensor->sensor_info->name, ret); > > > > + return ERR_PTR(ret); > > > > + } > > > > + > > > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > iiodev->channels = iio_channels; > > > > return iiodev; > > > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > > { > > > > const struct scmi_sensor_info *sensor_info; > > > > struct scmi_handle *handle = sdev->handle; > > > > + struct scmi_protocol_handle *ph; > > > > struct device *dev = &sdev->dev; > > > > struct iio_dev *scmi_iio_dev; > > > > u16 nr_sensors; > > > > int err = -ENODEV, i; > > > > > > > > - if (!handle || !handle->sensor_ops) { > > > > + if (!handle) > > > > + return -ENODEV; > > > > + > > > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > > > + if (IS_ERR(sensor_ops)) { > > > > dev_err(dev, "SCMI device has no sensor interface\n"); > > > > - return -EINVAL; > > > > + return PTR_ERR(sensor_ops); > > > > } > > > > > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > > > + nr_sensors = sensor_ops->count_get(ph); > > > > if (!nr_sensors) { > > > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > > > return -ENODEV; > > > > } > > > > > > > > for (i = 0; i < nr_sensors; i++) { > > > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > > > + sensor_info = sensor_ops->info_get(ph, i); > > > > if (!sensor_info) { > > > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > > > return -EINVAL; > > > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > > sensor_info->axis[0].type != RADIANS_SEC) > > > > continue; > > > > > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > > > if (IS_ERR(scmi_iio_dev)) { > > > > dev_err(dev, > > > > "failed to allocate IIO device for sensor %s: %ld\n", > > > >
On Wed, 31 Mar 2021 09:32:19 +0100 Cristian Marussi <cristian.marussi@arm.com> wrote: > Hi Jonathan > > On Tue, Mar 30, 2021 at 06:34:04PM +0100, Jonathan Cameron wrote: > > On Tue, 30 Mar 2021 13:51:13 +0100 > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > Hi Jonathan, > > > > > > On Tue, Mar 30, 2021 at 12:33:25PM +0100, Jonathan Cameron wrote: > > > > On Tue, 16 Mar 2021 12:48:50 +0000 > > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > > > Port driver to the new SCMI Sensor interface based on protocol handles > > > > > and common devm_get_ops(). > > > > > > > > > > Cc: Jyoti Bhayana <jbhayana@google.com> > > > > > Cc: Jonathan Cameron <jic23@kernel.org> > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > +CC linux-iio@vger.kernel.org > > > > > > > > Rule of thumb if it doesn't go there it ends up in randomly location based > > > > on other lists and I might not see it for a few weeks :( > > > > > > > > > > Ah sorry, I thought the direct CC was enough. > > > > No problem. Too much email :) > > > > > > > > > > --- > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ > > > > > 1 file changed, 41 insertions(+), 50 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > index 872d87ca6256..b4bdc3f3a946 100644 > > > > > --- a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > @@ -21,8 +21,10 @@ > > > > > > > > > > #define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > > > > > +static const struct scmi_sensor_proto_ops *sensor_ops; > > > > > > > > Hmm. I'm not keen on globals when they really should not be necessary. > > > > They just result in lifetimes being out of sync. Here you are fine because > > > > you set it to an appropriate value as the first thing you do in probe, and > > > > I assume the function only ever returns on answer on repeated calls. > > > > > > > > Why not put a copy of that pointer inside the struct scmi_iio_priv structures? > > > > > > > > > > The reason for this, as I said to Jyoyi who made the same comment indeed, > > > from my point of view (maybe wrong..) was that while the protocol_handle, > > > and previously the handle, are 'per-instance data' (so that you get a > > > different one each time this driver is possibly probed against a different > > > platform-handle) and as such are stored in scmi_iio_priv, the _ops are > > > just plain code pointers and are returned always the same for the same > > > protocol no matter how many times you probe this driver: > > > > As that's the case, I'm a little confused to why you have added the complexity > > of a query interface in the first place? Why not just export the ops and > > have the various drivers access them directly? If there is only > > one set of scmi_sensor_ops etc, then let drivers at it directly, or > > indeed export the functions that make up the ops structure directly. > > > > This sounds like a bit of abstraction that only serves to make the > > code harder to read. > > > > Thanks for your comments, let me explain a bit. > > While the ops are indeed per-protocol common code available to SCMI drivers, > the protocol handle, which you also obtain with devm_protocol_get(), is > instead an opaque reference bound to the specific protocol instance > associated to the platform handle you're using, so that, in case there are > multiple SCMI platforms defined on the system, you'll get, at each probe a > specific and distinct protocol_handle to use with your ops: this way > you'll act upon a completely distinct initialized protocol stack, using > a distinct underlying transport layer toward your platform of choice. > > Since this series wanted to unify SCMI standard and custom protocol interfaces > and enable modularization too, the get/put abstraction is there indeed to be > able to track internally protocol users and do resource accounting so that an > SCMI driver has to explicitly ask to use a protocol: the protocols instances are > then initialized on demand only when the first user shows up and more importantly > a hold is put on the protocol module refs avoiding its possible unloading > while still in use (even though only custom protocol are allowed as loadable > modules as of now...) > > Coming to the ops instead, the reason not to simply export them was...well... > ...not to export new symbols, and not just to stick to the old handle->ops() > interface way of non-exporting ops, or because I'm pavid at exporting new symbols > (I am :D), but because the idea was that in this way it would also have been > easier for vendors writing custom protocol modules to be able to just use them > in their own SCMI driver without the need to export also their new custom ops. > (and same goes generally for any new possible future standard protocol > which will not require endlessly further exports) I'll let this rest, but I'm unconvinced. This smacks of reinventing the wheel and a false layer of abstraction + unnecessary indirection. Current interface strongly hints that those ops structure pointers are decided at runtime. For a custom protocol, you will need a header that defines the signatures of those functions anyway. So you are only one step from just exporting them and relying on the existing nice module dependency tracking and loading etc. The only difference is that loading the SCMI driver module would require the protocol module to be loaded rather than it being based on an instantiation. (however, I'd hope that the SCMI driver modules are only loaded if we think there is something there for them to use!) Now if there was a chance that different providers of a protocols could provide different _ops structures then this interface might make sense, but then putting them in a global pointer is the wrong approach because of lifetimes being mismatched. Meh, it's your problem to maintain it not mine :) Jonathan > > Just for these reasons I attached the ops retrieval the same protocol_get() > interface already introduced to handle all of the above. > > > > you just end up > > > calling them against the proper different saved protocol_handle; so it > > > seemed to me an unneeded duplication to stick a copy of the same _ops > > > inside each per-instance scmi_iio_priv, and at the same time it seemed > > > also more straigthforward to access them without too many indirections > > > from inside the scmi_iio_priv struct). > > > > > > But if these are not valid points I can change this in IIO now, and in > > > the future also in all the other SCMI drivers that currently use this > > > same API and pattern of usage with global ops. (..at least because I'd > > > have to collect again all the other ACks agains and it's a bit later for > > > that now) > > > > I'm fine with leaving it as is. There's no fundamental issue, it's just > > a little bit ugly and I'm fussy :) > > > > I'm not fullly liking it too, but it was the best I could come up to cope > with the above reqs (and the limited amount of my grey-matter :D) > > But if in the future we can come up with something better, or some reqs > are dropped/revisited I'll be happy to flood the list with another > jumbo-series. > > Thanks > > Cristian > > > > > > > Thanks > > > > > > Cristian > > > > > > > Otherwise this all looks like straight forward refactoring so given the > > > > above is more a 'bad smell' than a bug and I'm rather late to the game. > > > > > > > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > > > > + > > > > > struct scmi_iio_priv { > > > > > - struct scmi_handle *handle; > > > > > + struct scmi_protocol_handle *ph; > > > > > const struct scmi_sensor_info *sensor_info; > > > > > struct iio_dev *indio_dev; > > > > > /* adding one additional channel for timestamp */ > > > > > @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > > static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > { > > > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > - u32 sensor_id = sensor->sensor_info->id; > > > > > u32 sensor_config = 0; > > > > > int err; > > > > > > > > > > @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > > > SCMI_SENS_CFG_SENSOR_ENABLE); > > > > > - > > > > > - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, > > > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > > - &sensor_id, &sensor->sensor_update_nb); > > > > > - if (err) { > > > > > - dev_err(&iio_dev->dev, > > > > > - "Error in registering sensor update notifier for sensor %s err %d", > > > > > - sensor->sensor_info->name, err); > > > > > - return err; > > > > > - } > > > > > - > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > > > - sensor->sensor_info->id, sensor_config); > > > > > - if (err) { > > > > > - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > > > - SCMI_PROTOCOL_SENSOR, > > > > > - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, > > > > > - &sensor->sensor_update_nb); > > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > > + sensor_config); > > > > > + if (err) > > > > > dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > > > > sensor->sensor_info->name, err); > > > > > - } > > > > > > > > > > return err; > > > > > } > > > > > @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) > > > > > static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) > > > > > { > > > > > struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > - u32 sensor_id = sensor->sensor_info->id; > > > > > u32 sensor_config = 0; > > > > > int err; > > > > > > > > > > sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, > > > > > SCMI_SENS_CFG_SENSOR_DISABLE); > > > > > - > > > > > - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, > > > > > - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > > - &sensor_id, &sensor->sensor_update_nb); > > > > > - if (err) { > > > > > - dev_err(&iio_dev->dev, > > > > > - "Error in unregistering sensor update notifier for sensor %s err %d", > > > > > - sensor->sensor_info->name, err); > > > > > - return err; > > > > > - } > > > > > - > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, > > > > > - sensor_config); > > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > > + sensor_config); > > > > > if (err) { > > > > > dev_err(&iio_dev->dev, > > > > > "Error in disabling sensor %s with err %d", > > > > > @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > > > u32 sensor_config; > > > > > char buf[32]; > > > > > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > > > - sensor->sensor_info->id, &sensor_config); > > > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > > > + &sensor_config); > > > > > if (err) { > > > > > dev_err(&iio_dev->dev, > > > > > "Error in getting sensor config for sensor %s err %d", > > > > > @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) > > > > > sensor_config |= > > > > > FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); > > > > > > > > > > - err = sensor->handle->sensor_ops->config_set(sensor->handle, > > > > > - sensor->sensor_info->id, sensor_config); > > > > > + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, > > > > > + sensor_config); > > > > > if (err) > > > > > dev_err(&iio_dev->dev, > > > > > "Error in setting sensor update interval for sensor %s value %u err %d", > > > > > @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) > > > > > u32 sensor_config; > > > > > int mult; > > > > > > > > > > - int err = sensor->handle->sensor_ops->config_get(sensor->handle, > > > > > - sensor->sensor_info->id, &sensor_config); > > > > > + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, > > > > > + &sensor_config); > > > > > if (err) { > > > > > dev_err(&iio_dev->dev, > > > > > "Error in getting sensor config for sensor %s err %d", > > > > > @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > > return 0; > > > > > } > > > > > > > > > > -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > - struct scmi_handle *handle, > > > > > - const struct scmi_sensor_info *sensor_info) > > > > > +static struct iio_dev * > > > > > +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, > > > > > + const struct scmi_sensor_info *sensor_info) > > > > > { > > > > > struct iio_chan_spec *iio_channels; > > > > > struct scmi_iio_priv *sensor; > > > > > enum iio_modifier modifier; > > > > > enum iio_chan_type type; > > > > > struct iio_dev *iiodev; > > > > > + struct device *dev = &sdev->dev; > > > > > + const struct scmi_handle *handle = sdev->handle; > > > > > int i, ret; > > > > > > > > > > iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > iiodev->modes = INDIO_DIRECT_MODE; > > > > > iiodev->dev.parent = dev; > > > > > sensor = iio_priv(iiodev); > > > > > - sensor->handle = handle; > > > > > + sensor->ph = ph; > > > > > sensor->sensor_info = sensor_info; > > > > > sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > > sensor->indio_dev = iiodev; > > > > > @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > sensor_info->axis[i].id); > > > > > } > > > > > > > > > > + ret = handle->notify_ops->devm_event_notifier_register(sdev, > > > > > + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, > > > > > + &sensor->sensor_info->id, > > > > > + &sensor->sensor_update_nb); > > > > > + if (ret) { > > > > > + dev_err(&iiodev->dev, > > > > > + "Error in registering sensor update notifier for sensor %s err %d", > > > > > + sensor->sensor_info->name, ret); > > > > > + return ERR_PTR(ret); > > > > > + } > > > > > + > > > > > scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > iiodev->channels = iio_channels; > > > > > return iiodev; > > > > > @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > > > { > > > > > const struct scmi_sensor_info *sensor_info; > > > > > struct scmi_handle *handle = sdev->handle; > > > > > + struct scmi_protocol_handle *ph; > > > > > struct device *dev = &sdev->dev; > > > > > struct iio_dev *scmi_iio_dev; > > > > > u16 nr_sensors; > > > > > int err = -ENODEV, i; > > > > > > > > > > - if (!handle || !handle->sensor_ops) { > > > > > + if (!handle) > > > > > + return -ENODEV; > > > > > + > > > > > + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); > > > > > + if (IS_ERR(sensor_ops)) { > > > > > dev_err(dev, "SCMI device has no sensor interface\n"); > > > > > - return -EINVAL; > > > > > + return PTR_ERR(sensor_ops); > > > > > } > > > > > > > > > > - nr_sensors = handle->sensor_ops->count_get(handle); > > > > > + nr_sensors = sensor_ops->count_get(ph); > > > > > if (!nr_sensors) { > > > > > dev_dbg(dev, "0 sensors found via SCMI bus\n"); > > > > > return -ENODEV; > > > > > } > > > > > > > > > > for (i = 0; i < nr_sensors; i++) { > > > > > - sensor_info = handle->sensor_ops->info_get(handle, i); > > > > > + sensor_info = sensor_ops->info_get(ph, i); > > > > > if (!sensor_info) { > > > > > dev_err(dev, "SCMI sensor %d has missing info\n", i); > > > > > return -EINVAL; > > > > > @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) > > > > > sensor_info->axis[0].type != RADIANS_SEC) > > > > > continue; > > > > > > > > > > - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); > > > > > + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); > > > > > if (IS_ERR(scmi_iio_dev)) { > > > > > dev_err(dev, > > > > > "failed to allocate IIO device for sensor %s: %ld\n", > > > > > >
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c index 872d87ca6256..b4bdc3f3a946 100644 --- a/drivers/iio/common/scmi_sensors/scmi_iio.c +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c @@ -21,8 +21,10 @@ #define SCMI_IIO_NUM_OF_AXIS 3 +static const struct scmi_sensor_proto_ops *sensor_ops; + struct scmi_iio_priv { - struct scmi_handle *handle; + struct scmi_protocol_handle *ph; const struct scmi_sensor_info *sensor_info; struct iio_dev *indio_dev; /* adding one additional channel for timestamp */ @@ -82,7 +84,6 @@ static int scmi_iio_sensor_update_cb(struct notifier_block *nb, static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) { struct scmi_iio_priv *sensor = iio_priv(iio_dev); - u32 sensor_id = sensor->sensor_info->id; u32 sensor_config = 0; int err; @@ -92,27 +93,11 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, SCMI_SENS_CFG_SENSOR_ENABLE); - - err = sensor->handle->notify_ops->register_event_notifier(sensor->handle, - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, - &sensor_id, &sensor->sensor_update_nb); - if (err) { - dev_err(&iio_dev->dev, - "Error in registering sensor update notifier for sensor %s err %d", - sensor->sensor_info->name, err); - return err; - } - - err = sensor->handle->sensor_ops->config_set(sensor->handle, - sensor->sensor_info->id, sensor_config); - if (err) { - sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, - SCMI_PROTOCOL_SENSOR, - SCMI_EVENT_SENSOR_UPDATE, &sensor_id, - &sensor->sensor_update_nb); + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, + sensor_config); + if (err) dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", sensor->sensor_info->name, err); - } return err; } @@ -120,25 +105,13 @@ static int scmi_iio_buffer_preenable(struct iio_dev *iio_dev) static int scmi_iio_buffer_postdisable(struct iio_dev *iio_dev) { struct scmi_iio_priv *sensor = iio_priv(iio_dev); - u32 sensor_id = sensor->sensor_info->id; u32 sensor_config = 0; int err; sensor_config |= FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK, SCMI_SENS_CFG_SENSOR_DISABLE); - - err = sensor->handle->notify_ops->unregister_event_notifier(sensor->handle, - SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, - &sensor_id, &sensor->sensor_update_nb); - if (err) { - dev_err(&iio_dev->dev, - "Error in unregistering sensor update notifier for sensor %s err %d", - sensor->sensor_info->name, err); - return err; - } - - err = sensor->handle->sensor_ops->config_set(sensor->handle, sensor_id, - sensor_config); + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, + sensor_config); if (err) { dev_err(&iio_dev->dev, "Error in disabling sensor %s with err %d", @@ -161,8 +134,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) u32 sensor_config; char buf[32]; - int err = sensor->handle->sensor_ops->config_get(sensor->handle, - sensor->sensor_info->id, &sensor_config); + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, + &sensor_config); if (err) { dev_err(&iio_dev->dev, "Error in getting sensor config for sensor %s err %d", @@ -208,8 +181,8 @@ static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) sensor_config |= FIELD_PREP(SCMI_SENS_CFG_ROUND_MASK, SCMI_SENS_CFG_ROUND_AUTO); - err = sensor->handle->sensor_ops->config_set(sensor->handle, - sensor->sensor_info->id, sensor_config); + err = sensor_ops->config_set(sensor->ph, sensor->sensor_info->id, + sensor_config); if (err) dev_err(&iio_dev->dev, "Error in setting sensor update interval for sensor %s value %u err %d", @@ -274,8 +247,8 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) u32 sensor_config; int mult; - int err = sensor->handle->sensor_ops->config_get(sensor->handle, - sensor->sensor_info->id, &sensor_config); + int err = sensor_ops->config_get(sensor->ph, sensor->sensor_info->id, + &sensor_config); if (err) { dev_err(&iio_dev->dev, "Error in getting sensor config for sensor %s err %d", @@ -542,15 +515,17 @@ static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) return 0; } -static struct iio_dev *scmi_alloc_iiodev(struct device *dev, - struct scmi_handle *handle, - const struct scmi_sensor_info *sensor_info) +static struct iio_dev * +scmi_alloc_iiodev(struct scmi_device *sdev, struct scmi_protocol_handle *ph, + const struct scmi_sensor_info *sensor_info) { struct iio_chan_spec *iio_channels; struct scmi_iio_priv *sensor; enum iio_modifier modifier; enum iio_chan_type type; struct iio_dev *iiodev; + struct device *dev = &sdev->dev; + const struct scmi_handle *handle = sdev->handle; int i, ret; iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); @@ -560,7 +535,7 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, iiodev->modes = INDIO_DIRECT_MODE; iiodev->dev.parent = dev; sensor = iio_priv(iiodev); - sensor->handle = handle; + sensor->ph = ph; sensor->sensor_info = sensor_info; sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; sensor->indio_dev = iiodev; @@ -595,6 +570,17 @@ static struct iio_dev *scmi_alloc_iiodev(struct device *dev, sensor_info->axis[i].id); } + ret = handle->notify_ops->devm_event_notifier_register(sdev, + SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE, + &sensor->sensor_info->id, + &sensor->sensor_update_nb); + if (ret) { + dev_err(&iiodev->dev, + "Error in registering sensor update notifier for sensor %s err %d", + sensor->sensor_info->name, ret); + return ERR_PTR(ret); + } + scmi_iio_set_timestamp_channel(&iio_channels[i], i); iiodev->channels = iio_channels; return iiodev; @@ -604,24 +590,29 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) { const struct scmi_sensor_info *sensor_info; struct scmi_handle *handle = sdev->handle; + struct scmi_protocol_handle *ph; struct device *dev = &sdev->dev; struct iio_dev *scmi_iio_dev; u16 nr_sensors; int err = -ENODEV, i; - if (!handle || !handle->sensor_ops) { + if (!handle) + return -ENODEV; + + sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph); + if (IS_ERR(sensor_ops)) { dev_err(dev, "SCMI device has no sensor interface\n"); - return -EINVAL; + return PTR_ERR(sensor_ops); } - nr_sensors = handle->sensor_ops->count_get(handle); + nr_sensors = sensor_ops->count_get(ph); if (!nr_sensors) { dev_dbg(dev, "0 sensors found via SCMI bus\n"); return -ENODEV; } for (i = 0; i < nr_sensors; i++) { - sensor_info = handle->sensor_ops->info_get(handle, i); + sensor_info = sensor_ops->info_get(ph, i); if (!sensor_info) { dev_err(dev, "SCMI sensor %d has missing info\n", i); return -EINVAL; @@ -636,7 +627,7 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev) sensor_info->axis[0].type != RADIANS_SEC) continue; - scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); + scmi_iio_dev = scmi_alloc_iiodev(sdev, ph, sensor_info); if (IS_ERR(scmi_iio_dev)) { dev_err(dev, "failed to allocate IIO device for sensor %s: %ld\n",
Port driver to the new SCMI Sensor interface based on protocol handles and common devm_get_ops(). Cc: Jyoti Bhayana <jbhayana@google.com> Cc: Jonathan Cameron <jic23@kernel.org> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/iio/common/scmi_sensors/scmi_iio.c | 91 ++++++++++------------ 1 file changed, 41 insertions(+), 50 deletions(-)