| Message ID | 20210208211918.1280588-2-jbhayana@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adding support for IIO SCMI based sensors | expand |
Hi Jyoti some minor things down below. Other than that, FWIW about the SCMI side of this: Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks Cristian On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > This change provides ARM SCMI Protocol based IIO device. > This driver provides support for Accelerometer and Gyroscope using > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > --- > MAINTAINERS | 6 + > drivers/firmware/arm_scmi/driver.c | 2 +- > drivers/iio/common/Kconfig | 1 + > drivers/iio/common/Makefile | 1 + > drivers/iio/common/scmi_sensors/Kconfig | 18 + > drivers/iio/common/scmi_sensors/Makefile | 5 + > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > 7 files changed, 705 insertions(+), 1 deletion(-) > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index b516bb34a8d5..ccf37d43ab41 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8567,6 +8567,12 @@ S: Maintained > F: Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > F: drivers/iio/multiplexer/iio-mux.c > > +IIO SCMI BASED DRIVER > +M: Jyoti Bhayana <jbhayana@google.com> > +L: linux-iio@vger.kernel.org > +S: Maintained > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > + > IIO SUBSYSTEM AND DRIVERS > M: Jonathan Cameron <jic23@kernel.org> > R: Lars-Peter Clausen <lars@metafoo.de> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 5392e1fc6b4e..248313bbd473 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > { SCMI_PROTOCOL_RESET, { "reset" },}, > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > }; > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > index 2b9ee9161abd..0334b4954773 100644 > --- a/drivers/iio/common/Kconfig > +++ b/drivers/iio/common/Kconfig > @@ -6,5 +6,6 @@ > source "drivers/iio/common/cros_ec_sensors/Kconfig" > source "drivers/iio/common/hid-sensors/Kconfig" > source "drivers/iio/common/ms_sensors/Kconfig" > +source "drivers/iio/common/scmi_sensors/Kconfig" > source "drivers/iio/common/ssp_sensors/Kconfig" > source "drivers/iio/common/st_sensors/Kconfig" > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > index 4bc30bb548e2..fad40e1e1718 100644 > --- a/drivers/iio/common/Makefile > +++ b/drivers/iio/common/Makefile > @@ -11,5 +11,6 @@ > obj-y += cros_ec_sensors/ > obj-y += hid-sensors/ > obj-y += ms_sensors/ > +obj-y += scmi_sensors/ > obj-y += ssp_sensors/ > obj-y += st_sensors/ > diff --git a/drivers/iio/common/scmi_sensors/Kconfig b/drivers/iio/common/scmi_sensors/Kconfig > new file mode 100644 > index 000000000000..67e084cbb1ab > --- /dev/null > +++ b/drivers/iio/common/scmi_sensors/Kconfig > @@ -0,0 +1,18 @@ > +# > +# IIO over SCMI > +# > +# When adding new entries keep the list in alphabetical order > + > +menu "IIO SCMI Sensors" > + > +config IIO_SCMI > + tristate "IIO SCMI" > + depends on ARM_SCMI_PROTOCOL > + select IIO_BUFFER > + select IIO_KFIFO_BUF > + help > + Say yes here to build support for IIO SCMI Driver. > + This provides ARM SCMI Protocol based IIO device. > + This driver provides support for accelerometer and gyroscope > + sensors available on SCMI based platforms. > +endmenu > diff --git a/drivers/iio/common/scmi_sensors/Makefile b/drivers/iio/common/scmi_sensors/Makefile > new file mode 100644 > index 000000000000..f13140a2575a > --- /dev/null > +++ b/drivers/iio/common/scmi_sensors/Makefile > @@ -0,0 +1,5 @@ > +# SPDX - License - Identifier : GPL - 2.0 - only > +# > +# Makefile for the IIO over SCMI > +# > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c > new file mode 100644 > index 000000000000..093b1fc24e27 > --- /dev/null > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > @@ -0,0 +1,673 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * System Control and Management Interface(SCMI) based IIO sensor driver > + * > + * Copyright (C) 2021 Google LLC > + */ > + > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/kfifo_buf.h> > +#include <linux/iio/sysfs.h> > +#include <linux/kernel.h> > +#include <linux/kthread.h> > +#include <linux/module.h> > +#include <linux/scmi_protocol.h> > +#include <linux/time.h> > +#include <linux/types.h> > + > +#define SCMI_IIO_NUM_OF_AXIS 3 > + > +struct scmi_iio_priv { > + struct scmi_handle *handle; > + const struct scmi_sensor_info *sensor_info; > + struct iio_dev *indio_dev; > + /* adding one additional channel for timestamp */ > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > + struct notifier_block sensor_update_nb; > + u32 *freq_avail; > +}; > + > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct scmi_sensor_update_report *sensor_update = data; > + struct iio_dev *scmi_iio_dev; > + struct scmi_iio_priv *sensor; > + s8 tstamp_scale; > + u64 time, time_ns; > + int i; > + > + if (sensor_update->readings_count == 0) > + return NOTIFY_DONE; > + > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > + > + for (i = 0; i < sensor_update->readings_count; i++) > + sensor->iio_buf[i] = sensor_update->readings[i].value; > + > + if (!sensor->sensor_info->timestamped) { > + time_ns = ktime_to_ns(sensor_update->timestamp); > + } else { > + /* > + * All the axes are supposed to have the same value for timestamp. > + * We are just using the values from the Axis 0 here. > + */ > + time = sensor_update->readings[0].timestamp; > + > + /* > + * Timestamp returned by SCMI is in seconds and is equal to > + * time * power-of-10 multiplier(tstamp_scale) seconds. > + * Converting the timestamp to nanoseconds below. > + */ > + tstamp_scale = sensor->sensor_info->tstamp_scale + > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > + if (tstamp_scale < 0) > + time_ns = > + div64_u64(time, int_pow(10, abs(tstamp_scale))); > + else > + time_ns = time * int_pow(10, tstamp_scale); > + } > + > + scmi_iio_dev = sensor->indio_dev; > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > + time_ns); > + return NOTIFY_OK; > +} > + > +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; > + > + if (sensor->sensor_info->timestamped) > + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > + SCMI_SENS_CFG_TSTAMP_ENABLE); > + > + 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); > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > + sensor->sensor_info->name, err); > + } > + > + return err; > +} > + > +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); > + if (err) { > + dev_err(&iio_dev->dev, > + "Error in disabling sensor %s with err %d", > + sensor->sensor_info->name, err); > + } > + > + return err; > +} > + > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > + .preenable = scmi_iio_buffer_preenable, > + .postdisable = scmi_iio_buffer_postdisable, > +}; This is just a question, I'm not suggesting to change anything here at this point to be clear, since it works just fine as it is. Following up a previous email, given these are called on enable/disable by sysfs, is there a specific reason why you configure here, inside these ops, also timestamping and callbacks i.e. each time the sensor is turned on/off by sysfs ? ... instead of just, as an example, enabling in _preenable the sensor while registering callbacks and enabling timestamping once for all earlier during probe phase ? (likewise for _postdisable -> remove) AFAIU the spec says notifications are emitted for sensors which has requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the sensor is enabled as a whole (via proper CONFIG_SET as you do), so that enabling/disabling the sensor as a whole should result in starting/ stopping the notification flow without the need of unregistering the callbacks everytime. (same goes with the timestamping) In other words, I would expect the sensor to maintain its state (on the platform side) even when going through enable/disable cycles, so that it 'remembers' that timestamping/notifications were enabled across an on/off. This would reduce the number of SCMI messages exchanges between the kernel and the platform and should be supported by both, but as said, it's more of a question for the future, not necessarily for this series. > + [snip] > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > +{ > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns, > + hz, uhz; > + unsigned int cur_interval, low_interval, high_interval, step_size; > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > + int i; > + > + sensor->freq_avail = > + devm_kzalloc(&iio_dev->dev, > + sizeof(*sensor->freq_avail) * > + (sensor->sensor_info->intervals.count * 2), > + GFP_KERNEL); > + if (!sensor->freq_avail) > + return -ENOMEM; > + > + if (sensor->sensor_info->intervals.segmented) { > + low_interval = sensor->sensor_info->intervals > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > + low_interval_ns = scmi_iio_convert_interval_to_ns(low_interval); > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > + sensor->freq_avail[0] = hz; > + sensor->freq_avail[1] = uhz; > + > + step_size = sensor->sensor_info->intervals > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > + sensor->freq_avail[2] = hz; > + sensor->freq_avail[3] = uhz; > + > + high_interval = sensor->sensor_info->intervals > + .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > + high_interval_ns = > + scmi_iio_convert_interval_to_ns(high_interval); > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > + sensor->freq_avail[4] = hz; > + sensor->freq_avail[5] = uhz; > + } else { > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) { > + cur_interval = sensor->sensor_info->intervals.desc[i]; > + cur_interval_ns = > + scmi_iio_convert_interval_to_ns(cur_interval); > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > + sensor->freq_avail[i * 2] = hz; > + sensor->freq_avail[i * 2 + 1] = uhz; > + } > + } > + return 0; > +} > + > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > +{ > + struct iio_buffer *buffer; > + > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > + if (!buffer) > + return -ENOMEM; > + > + iio_device_attach_buffer(scmi_iiodev, buffer); > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > + return 0; > +} > + > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > + struct scmi_handle *handle, > + 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; > + int i, ret; > + > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > + if (!iiodev) > + return ERR_PTR(-ENOMEM); > + > + iiodev->modes = INDIO_DIRECT_MODE; > + iiodev->dev.parent = dev; > + sensor = iio_priv(iiodev); > + sensor->handle = handle; > + sensor->sensor_info = sensor_info; > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > + sensor->indio_dev = iiodev; > + > + /* adding one additional channel for timestamp */ > + iiodev->num_channels = sensor_info->num_axis + 1; > + iiodev->name = sensor_info->name; > + iiodev->info = &scmi_iio_info; > + > + iio_channels = > + devm_kzalloc(dev, > + sizeof(*iio_channels) * (iiodev->num_channels), > + GFP_KERNEL); > + if (!iio_channels) > + return ERR_PTR(-ENOMEM); > + > + scmi_iio_set_sampling_freq_avail(iiodev); You don't check this for retval, and it could fail with -ENOMEM. > + > + for (i = 0; i < sensor_info->num_axis; i++) { > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type); > + if (ret < 0) > + return ERR_PTR(ret); > + > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > + &modifier); > + if (ret < 0) > + return ERR_PTR(ret); > + > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > + sensor_info->axis[i].id); > + } > + > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > + iiodev->channels = iio_channels; > + return iiodev; > +} > +
Hi Jyoti, On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > Hi Cristian, > > Thanks for the feedback. Regarding registering callbacks at the probe time > instead of .preenable, I have tried it before but I think due to some > issues(don't remember it now maybe on the platform side) I kept it at the > .preenable level. > > But you are right, that it will be nice to move it at the probe level > instead. I will try again and test if it works and would move it at the > probe level. Regarding the unregistering of the notifier, is it required at > the remove of iio driver or scmi driver will take care of it? > Because if I add unregister at the iio driver remove level, I would have to > iterate all the sensors again and unregister them. Yes you are right if you move callbacks registration once for all to the .probe step you'll have to unregister them all in a .remove. BUT I think instead you should stick with your current solution given it's working fine anyway and it's supported by the notification framework and also for another reason I'm going to explain down below (which is also the reason why I asked you this at first :D) As you may remember I'm refactoring all the SCMI internals in a separate series to ease modularization and vendor protocols support, and that will lead also to some changes in the SCMI driver interface that you use: amongst other things one interesting addition will be a new devres managed notification registration method, something like: handle->notify_ops->devm_register_notifier(sdev, ...); With such method you could just move your registration to the .probe step and just forget about it, without the need to add any unregistration in the .remove step, since the core will take care to remove all the callbacks at driver unloading time. Now, this series, which is here if you want to have a look: https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ is already taking care to port any existent SCMI driver to the new interface, so when your IIODEV SCMI driver will be finally queued somewhere for merge, I can in turn rebase my series on yours and take care to port your driver too to the new interface applying the changes above in the context of my series. (and ask you to review of course :D) I'm saying that is better if you keep your series as it is for now (old interface + .preenable/.postdisable regs/unregs) because, as said, with the new interface the devm_ methods will ease the registration @probe time, and also especially because the new interface is not (and most probably won't) be part of the v5.4 backport that you are testing against: so if you stick with your current solution you'll have a working patch easily backportable now, and once queued I'll port it to the interface using devm_ (so simplifying it) In this context, it would be indeed important to know if in general moving registration to the probe phase (which should be fine by the spec) poses any kind of problem. (and that's reason why asked it) Hope to have been clear despite the flood of words :D Thanks Cristian > > Thanks, > Jyoti > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com> > wrote: > > > Hi Jyoti > > > > some minor things down below. > > > > Other than that, FWIW about the SCMI side of this: > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > Thanks > > > > Cristian > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > > > This change provides ARM SCMI Protocol based IIO device. > > > This driver provides support for Accelerometer and Gyroscope using > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > --- > > > MAINTAINERS | 6 + > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > drivers/iio/common/Kconfig | 1 + > > > drivers/iio/common/Makefile | 1 + > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > F: > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > +IIO SCMI BASED DRIVER > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > +L: linux-iio@vger.kernel.org > > > +S: Maintained > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > + > > > IIO SUBSYSTEM AND DRIVERS > > > M: Jonathan Cameron <jic23@kernel.org> > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > b/drivers/firmware/arm_scmi/driver.c > > > index 5392e1fc6b4e..248313bbd473 100644 > > > --- a/drivers/firmware/arm_scmi/driver.c > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > }; > > > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > > > index 2b9ee9161abd..0334b4954773 100644 > > > --- a/drivers/iio/common/Kconfig > > > +++ b/drivers/iio/common/Kconfig > > > @@ -6,5 +6,6 @@ > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > source "drivers/iio/common/st_sensors/Kconfig" > > > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > --- a/drivers/iio/common/Makefile > > > +++ b/drivers/iio/common/Makefile > > > @@ -11,5 +11,6 @@ > > > obj-y += cros_ec_sensors/ > > > obj-y += hid-sensors/ > > > obj-y += ms_sensors/ > > > +obj-y += scmi_sensors/ > > > obj-y += ssp_sensors/ > > > obj-y += st_sensors/ > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > b/drivers/iio/common/scmi_sensors/Kconfig > > > new file mode 100644 > > > index 000000000000..67e084cbb1ab > > > --- /dev/null > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > @@ -0,0 +1,18 @@ > > > +# > > > +# IIO over SCMI > > > +# > > > +# When adding new entries keep the list in alphabetical order > > > + > > > +menu "IIO SCMI Sensors" > > > + > > > +config IIO_SCMI > > > + tristate "IIO SCMI" > > > + depends on ARM_SCMI_PROTOCOL > > > + select IIO_BUFFER > > > + select IIO_KFIFO_BUF > > > + help > > > + Say yes here to build support for IIO SCMI Driver. > > > + This provides ARM SCMI Protocol based IIO device. > > > + This driver provides support for accelerometer and gyroscope > > > + sensors available on SCMI based platforms. > > > +endmenu > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > b/drivers/iio/common/scmi_sensors/Makefile > > > new file mode 100644 > > > index 000000000000..f13140a2575a > > > --- /dev/null > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > @@ -0,0 +1,5 @@ > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > +# > > > +# Makefile for the IIO over SCMI > > > +# > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > new file mode 100644 > > > index 000000000000..093b1fc24e27 > > > --- /dev/null > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > @@ -0,0 +1,673 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +/* > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > + * > > > + * Copyright (C) 2021 Google LLC > > > + */ > > > + > > > +#include <linux/delay.h> > > > +#include <linux/err.h> > > > +#include <linux/iio/buffer.h> > > > +#include <linux/iio/iio.h> > > > +#include <linux/iio/kfifo_buf.h> > > > +#include <linux/iio/sysfs.h> > > > +#include <linux/kernel.h> > > > +#include <linux/kthread.h> > > > +#include <linux/module.h> > > > +#include <linux/scmi_protocol.h> > > > +#include <linux/time.h> > > > +#include <linux/types.h> > > > + > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > + > > > +struct scmi_iio_priv { > > > + struct scmi_handle *handle; > > > + const struct scmi_sensor_info *sensor_info; > > > + struct iio_dev *indio_dev; > > > + /* adding one additional channel for timestamp */ > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > + struct notifier_block sensor_update_nb; > > > + u32 *freq_avail; > > > +}; > > > + > > > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > + unsigned long event, void *data) > > > +{ > > > + struct scmi_sensor_update_report *sensor_update = data; > > > + struct iio_dev *scmi_iio_dev; > > > + struct scmi_iio_priv *sensor; > > > + s8 tstamp_scale; > > > + u64 time, time_ns; > > > + int i; > > > + > > > + if (sensor_update->readings_count == 0) > > > + return NOTIFY_DONE; > > > + > > > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > > > + > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > + sensor->iio_buf[i] = sensor_update->readings[i].value; > > > + > > > + if (!sensor->sensor_info->timestamped) { > > > + time_ns = ktime_to_ns(sensor_update->timestamp); > > > + } else { > > > + /* > > > + * All the axes are supposed to have the same value for > > timestamp. > > > + * We are just using the values from the Axis 0 here. > > > + */ > > > + time = sensor_update->readings[0].timestamp; > > > + > > > + /* > > > + * Timestamp returned by SCMI is in seconds and is equal > > to > > > + * time * power-of-10 multiplier(tstamp_scale) seconds. > > > + * Converting the timestamp to nanoseconds below. > > > + */ > > > + tstamp_scale = sensor->sensor_info->tstamp_scale + > > > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > > > + if (tstamp_scale < 0) > > > + time_ns = > > > + div64_u64(time, int_pow(10, > > abs(tstamp_scale))); > > > + else > > > + time_ns = time * int_pow(10, tstamp_scale); > > > + } > > > + > > > + scmi_iio_dev = sensor->indio_dev; > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > > > + time_ns); > > > + return NOTIFY_OK; > > > +} > > > + > > > +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; > > > + > > > + if (sensor->sensor_info->timestamped) > > > + sensor_config |= > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > + > > > + 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); > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err > > %d", > > > + sensor->sensor_info->name, err); > > > + } > > > + > > > + return err; > > > +} > > > + > > > +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); > > > + if (err) { > > > + dev_err(&iio_dev->dev, > > > + "Error in disabling sensor %s with err %d", > > > + sensor->sensor_info->name, err); > > > + } > > > + > > > + return err; > > > +} > > > + > > > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > > > + .preenable = scmi_iio_buffer_preenable, > > > + .postdisable = scmi_iio_buffer_postdisable, > > > +}; > > > > This is just a question, I'm not suggesting to change anything here at > > this point to be clear, since it works just fine as it is. > > > > Following up a previous email, given these are called on enable/disable > > by sysfs, is there a specific reason why you configure here, inside > > these ops, also timestamping and callbacks i.e. each time the sensor is > > turned on/off by sysfs ? ... instead of just, as an example, enabling > > in _preenable the sensor while registering callbacks and enabling > > timestamping once for all earlier during probe phase ? > > (likewise for _postdisable -> remove) > > > > AFAIU the spec says notifications are emitted for sensors which has > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so > > that enabling/disabling the sensor as a whole should result in starting/ > > stopping the notification flow without the need of unregistering the > > callbacks everytime. (same goes with the timestamping) > > > > In other words, I would expect the sensor to maintain its state (on the > > platform side) even when going through enable/disable cycles, so that > > it 'remembers' that timestamping/notifications were enabled across an > > on/off. > > > > This would reduce the number of SCMI messages exchanges between the > > kernel and the platform and should be supported by both, but as said, > > it's more of a question for the future, not necessarily for this series. > > > > > + > > > > [snip] > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > > > +{ > > > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, > > step_size_ns, > > > + hz, uhz; > > > + unsigned int cur_interval, low_interval, high_interval, step_size; > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > + int i; > > > + > > > + sensor->freq_avail = > > > + devm_kzalloc(&iio_dev->dev, > > > + sizeof(*sensor->freq_avail) * > > > + (sensor->sensor_info->intervals.count > > * 2), > > > + GFP_KERNEL); > > > + if (!sensor->freq_avail) > > > + return -ENOMEM; > > > + > > > + if (sensor->sensor_info->intervals.segmented) { > > > + low_interval = sensor->sensor_info->intervals > > > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > + low_interval_ns = > > scmi_iio_convert_interval_to_ns(low_interval); > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > + sensor->freq_avail[0] = hz; > > > + sensor->freq_avail[1] = uhz; > > > + > > > + step_size = sensor->sensor_info->intervals > > > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > + sensor->freq_avail[2] = hz; > > > + sensor->freq_avail[3] = uhz; > > > + > > > + high_interval = sensor->sensor_info->intervals > > > + > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > + high_interval_ns = > > > + scmi_iio_convert_interval_to_ns(high_interval); > > > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > > > + sensor->freq_avail[4] = hz; > > > + sensor->freq_avail[5] = uhz; > > > + } else { > > > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) > > { > > > + cur_interval = > > sensor->sensor_info->intervals.desc[i]; > > > + cur_interval_ns = > > > + > > scmi_iio_convert_interval_to_ns(cur_interval); > > > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > > > + sensor->freq_avail[i * 2] = hz; > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > +{ > > > + struct iio_buffer *buffer; > > > + > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > + if (!buffer) > > > + return -ENOMEM; > > > + > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > + return 0; > > > +} > > > + > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > + struct scmi_handle *handle, > > > + 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; > > > + int i, ret; > > > + > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > + if (!iiodev) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > + iiodev->dev.parent = dev; > > > + sensor = iio_priv(iiodev); > > > + sensor->handle = handle; > > > + sensor->sensor_info = sensor_info; > > > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > + sensor->indio_dev = iiodev; > > > + > > > + /* adding one additional channel for timestamp */ > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > + iiodev->name = sensor_info->name; > > > + iiodev->info = &scmi_iio_info; > > > + > > > + iio_channels = > > > + devm_kzalloc(dev, > > > + sizeof(*iio_channels) * > > (iiodev->num_channels), > > > + GFP_KERNEL); > > > + if (!iio_channels) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > You don't check this for retval, and it could fail with -ENOMEM. > > > > > + > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, > > &type); > > > + if (ret < 0) > > > + return ERR_PTR(ret); > > > + > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > + &modifier); > > > + if (ret < 0) > > > + return ERR_PTR(ret); > > > + > > > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > > > + sensor_info->axis[i].id); > > > + } > > > + > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > + iiodev->channels = iio_channels; > > > + return iiodev; > > > +} > > > + > >
On Wed, 10 Feb 2021 21:46:19 +0000 Cristian Marussi <cristian.marussi@arm.com> wrote: > Hi Jyoti, > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > Hi Cristian, > > > > Thanks for the feedback. Regarding registering callbacks at the probe time > > instead of .preenable, I have tried it before but I think due to some > > issues(don't remember it now maybe on the platform side) I kept it at the > > .preenable level. > > > > But you are right, that it will be nice to move it at the probe level > > instead. I will try again and test if it works and would move it at the > > probe level. Regarding the unregistering of the notifier, is it required at > > the remove of iio driver or scmi driver will take care of it? > > Because if I add unregister at the iio driver remove level, I would have to > > iterate all the sensors again and unregister them. > > Yes you are right if you move callbacks registration once for all to the > .probe step you'll have to unregister them all in a .remove. > > BUT I think instead you should stick with your current solution given > it's working fine anyway and it's supported by the notification > framework and also for another reason I'm going to explain down below > (which is also the reason why I asked you this at first :D) > > As you may remember I'm refactoring all the SCMI internals in a separate > series to ease modularization and vendor protocols support, and that will > lead also to some changes in the SCMI driver interface that you use: > amongst other things one interesting addition will be a new devres managed > notification registration method, something like: > > handle->notify_ops->devm_register_notifier(sdev, ...); > > With such method you could just move your registration to the .probe > step and just forget about it, without the need to add any unregistration > in the .remove step, since the core will take care to remove all the > callbacks at driver unloading time. > > Now, this series, which is here if you want to have a look: > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > is already taking care to port any existent SCMI driver to the new interface, > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I > can in turn rebase my series on yours and take care to port your driver too to > the new interface applying the changes above in the context of my series. > (and ask you to review of course :D) I'm guessing you probably want this driver in an immutable branch then rather than having to wait another cycle for it to tick through to a a sensible upstream? Jonathan > > I'm saying that is better if you keep your series as it is for now > (old interface + .preenable/.postdisable regs/unregs) because, as said, > with the new interface the devm_ methods will ease the registration > @probe time, and also especially because the new interface is not (and > most probably won't) be part of the v5.4 backport that you are testing > against: so if you stick with your current solution you'll have a > working patch easily backportable now, and once queued I'll port it to > the interface using devm_ (so simplifying it) > > In this context, it would be indeed important to know if in general moving > registration to the probe phase (which should be fine by the spec) poses > any kind of problem. (and that's reason why asked it) > > Hope to have been clear despite the flood of words :D > > Thanks > > Cristian > > > > > Thanks, > > Jyoti > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com> > > wrote: > > > > > Hi Jyoti > > > > > > some minor things down below. > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > Thanks > > > > > > Cristian > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > > > > This change provides ARM SCMI Protocol based IIO device. > > > > This driver provides support for Accelerometer and Gyroscope using > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > --- > > > > MAINTAINERS | 6 + > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > drivers/iio/common/Kconfig | 1 + > > > > drivers/iio/common/Makefile | 1 + > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > F: > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > +IIO SCMI BASED DRIVER > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > +L: linux-iio@vger.kernel.org > > > > +S: Maintained > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > + > > > > IIO SUBSYSTEM AND DRIVERS > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > b/drivers/firmware/arm_scmi/driver.c > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > }; > > > > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > --- a/drivers/iio/common/Kconfig > > > > +++ b/drivers/iio/common/Kconfig > > > > @@ -6,5 +6,6 @@ > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > --- a/drivers/iio/common/Makefile > > > > +++ b/drivers/iio/common/Makefile > > > > @@ -11,5 +11,6 @@ > > > > obj-y += cros_ec_sensors/ > > > > obj-y += hid-sensors/ > > > > obj-y += ms_sensors/ > > > > +obj-y += scmi_sensors/ > > > > obj-y += ssp_sensors/ > > > > obj-y += st_sensors/ > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > new file mode 100644 > > > > index 000000000000..67e084cbb1ab > > > > --- /dev/null > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > @@ -0,0 +1,18 @@ > > > > +# > > > > +# IIO over SCMI > > > > +# > > > > +# When adding new entries keep the list in alphabetical order > > > > + > > > > +menu "IIO SCMI Sensors" > > > > + > > > > +config IIO_SCMI > > > > + tristate "IIO SCMI" > > > > + depends on ARM_SCMI_PROTOCOL > > > > + select IIO_BUFFER > > > > + select IIO_KFIFO_BUF > > > > + help > > > > + Say yes here to build support for IIO SCMI Driver. > > > > + This provides ARM SCMI Protocol based IIO device. > > > > + This driver provides support for accelerometer and gyroscope > > > > + sensors available on SCMI based platforms. > > > > +endmenu > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > new file mode 100644 > > > > index 000000000000..f13140a2575a > > > > --- /dev/null > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > @@ -0,0 +1,5 @@ > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > +# > > > > +# Makefile for the IIO over SCMI > > > > +# > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > new file mode 100644 > > > > index 000000000000..093b1fc24e27 > > > > --- /dev/null > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > @@ -0,0 +1,673 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +/* > > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > > + * > > > > + * Copyright (C) 2021 Google LLC > > > > + */ > > > > + > > > > +#include <linux/delay.h> > > > > +#include <linux/err.h> > > > > +#include <linux/iio/buffer.h> > > > > +#include <linux/iio/iio.h> > > > > +#include <linux/iio/kfifo_buf.h> > > > > +#include <linux/iio/sysfs.h> > > > > +#include <linux/kernel.h> > > > > +#include <linux/kthread.h> > > > > +#include <linux/module.h> > > > > +#include <linux/scmi_protocol.h> > > > > +#include <linux/time.h> > > > > +#include <linux/types.h> > > > > + > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > + > > > > +struct scmi_iio_priv { > > > > + struct scmi_handle *handle; > > > > + const struct scmi_sensor_info *sensor_info; > > > > + struct iio_dev *indio_dev; > > > > + /* adding one additional channel for timestamp */ > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > + struct notifier_block sensor_update_nb; > > > > + u32 *freq_avail; > > > > +}; > > > > + > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > + unsigned long event, void *data) > > > > +{ > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > + struct iio_dev *scmi_iio_dev; > > > > + struct scmi_iio_priv *sensor; > > > > + s8 tstamp_scale; > > > > + u64 time, time_ns; > > > > + int i; > > > > + > > > > + if (sensor_update->readings_count == 0) > > > > + return NOTIFY_DONE; > > > > + > > > > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > > > > + > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > + sensor->iio_buf[i] = sensor_update->readings[i].value; > > > > + > > > > + if (!sensor->sensor_info->timestamped) { > > > > + time_ns = ktime_to_ns(sensor_update->timestamp); > > > > + } else { > > > > + /* > > > > + * All the axes are supposed to have the same value for > > > timestamp. > > > > + * We are just using the values from the Axis 0 here. > > > > + */ > > > > + time = sensor_update->readings[0].timestamp; > > > > + > > > > + /* > > > > + * Timestamp returned by SCMI is in seconds and is equal > > > to > > > > + * time * power-of-10 multiplier(tstamp_scale) seconds. > > > > + * Converting the timestamp to nanoseconds below. > > > > + */ > > > > + tstamp_scale = sensor->sensor_info->tstamp_scale + > > > > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > > > > + if (tstamp_scale < 0) > > > > + time_ns = > > > > + div64_u64(time, int_pow(10, > > > abs(tstamp_scale))); > > > > + else > > > > + time_ns = time * int_pow(10, tstamp_scale); > > > > + } > > > > + > > > > + scmi_iio_dev = sensor->indio_dev; > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > > > > + time_ns); > > > > + return NOTIFY_OK; > > > > +} > > > > + > > > > +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; > > > > + > > > > + if (sensor->sensor_info->timestamped) > > > > + sensor_config |= > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > + > > > > + 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); > > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err > > > %d", > > > > + sensor->sensor_info->name, err); > > > > + } > > > > + > > > > + return err; > > > > +} > > > > + > > > > +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); > > > > + if (err) { > > > > + dev_err(&iio_dev->dev, > > > > + "Error in disabling sensor %s with err %d", > > > > + sensor->sensor_info->name, err); > > > > + } > > > > + > > > > + return err; > > > > +} > > > > + > > > > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > > > > + .preenable = scmi_iio_buffer_preenable, > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > +}; > > > > > > This is just a question, I'm not suggesting to change anything here at > > > this point to be clear, since it works just fine as it is. > > > > > > Following up a previous email, given these are called on enable/disable > > > by sysfs, is there a specific reason why you configure here, inside > > > these ops, also timestamping and callbacks i.e. each time the sensor is > > > turned on/off by sysfs ? ... instead of just, as an example, enabling > > > in _preenable the sensor while registering callbacks and enabling > > > timestamping once for all earlier during probe phase ? > > > (likewise for _postdisable -> remove) > > > > > > AFAIU the spec says notifications are emitted for sensors which has > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so > > > that enabling/disabling the sensor as a whole should result in starting/ > > > stopping the notification flow without the need of unregistering the > > > callbacks everytime. (same goes with the timestamping) > > > > > > In other words, I would expect the sensor to maintain its state (on the > > > platform side) even when going through enable/disable cycles, so that > > > it 'remembers' that timestamping/notifications were enabled across an > > > on/off. > > > > > > This would reduce the number of SCMI messages exchanges between the > > > kernel and the platform and should be supported by both, but as said, > > > it's more of a question for the future, not necessarily for this series. > > > > > > > + > > > > > > [snip] > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > > > > +{ > > > > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, > > > step_size_ns, > > > > + hz, uhz; > > > > + unsigned int cur_interval, low_interval, high_interval, step_size; > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > + int i; > > > > + > > > > + sensor->freq_avail = > > > > + devm_kzalloc(&iio_dev->dev, > > > > + sizeof(*sensor->freq_avail) * > > > > + (sensor->sensor_info->intervals.count > > > * 2), > > > > + GFP_KERNEL); > > > > + if (!sensor->freq_avail) > > > > + return -ENOMEM; > > > > + > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > + low_interval = sensor->sensor_info->intervals > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > + low_interval_ns = > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > + sensor->freq_avail[0] = hz; > > > > + sensor->freq_avail[1] = uhz; > > > > + > > > > + step_size = sensor->sensor_info->intervals > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > + sensor->freq_avail[2] = hz; > > > > + sensor->freq_avail[3] = uhz; > > > > + > > > > + high_interval = sensor->sensor_info->intervals > > > > + > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > + high_interval_ns = > > > > + scmi_iio_convert_interval_to_ns(high_interval); > > > > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > > > > + sensor->freq_avail[4] = hz; > > > > + sensor->freq_avail[5] = uhz; > > > > + } else { > > > > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) > > > { > > > > + cur_interval = > > > sensor->sensor_info->intervals.desc[i]; > > > > + cur_interval_ns = > > > > + > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > > > > + sensor->freq_avail[i * 2] = hz; > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > + } > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > +{ > > > > + struct iio_buffer *buffer; > > > > + > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > + if (!buffer) > > > > + return -ENOMEM; > > > > + > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > + return 0; > > > > +} > > > > + > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > + struct scmi_handle *handle, > > > > + 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; > > > > + int i, ret; > > > > + > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > + if (!iiodev) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > + iiodev->dev.parent = dev; > > > > + sensor = iio_priv(iiodev); > > > > + sensor->handle = handle; > > > > + sensor->sensor_info = sensor_info; > > > > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > + sensor->indio_dev = iiodev; > > > > + > > > > + /* adding one additional channel for timestamp */ > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > + iiodev->name = sensor_info->name; > > > > + iiodev->info = &scmi_iio_info; > > > > + > > > > + iio_channels = > > > > + devm_kzalloc(dev, > > > > + sizeof(*iio_channels) * > > > (iiodev->num_channels), > > > > + GFP_KERNEL); > > > > + if (!iio_channels) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > You don't check this for retval, and it could fail with -ENOMEM. > > > > > > > + > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > &type); > > > > + if (ret < 0) > > > > + return ERR_PTR(ret); > > > > + > > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > + &modifier); > > > > + if (ret < 0) > > > > + return ERR_PTR(ret); > > > > + > > > > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > > > > + sensor_info->axis[i].id); > > > > + } > > > > + > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > + iiodev->channels = iio_channels; > > > > + return iiodev; > > > > +} > > > > + > > >
On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote: > On Wed, 10 Feb 2021 21:46:19 +0000 > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > Hi Jyoti, > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > > Hi Cristian, > > > > > > Thanks for the feedback. Regarding registering callbacks at the probe time > > > instead of .preenable, I have tried it before but I think due to some > > > issues(don't remember it now maybe on the platform side) I kept it at the > > > .preenable level. > > > > > > But you are right, that it will be nice to move it at the probe level > > > instead. I will try again and test if it works and would move it at the > > > probe level. Regarding the unregistering of the notifier, is it required at > > > the remove of iio driver or scmi driver will take care of it? > > > Because if I add unregister at the iio driver remove level, I would have to > > > iterate all the sensors again and unregister them. > > > > Yes you are right if you move callbacks registration once for all to the > > .probe step you'll have to unregister them all in a .remove. > > > > BUT I think instead you should stick with your current solution given > > it's working fine anyway and it's supported by the notification > > framework and also for another reason I'm going to explain down below > > (which is also the reason why I asked you this at first :D) > > > > As you may remember I'm refactoring all the SCMI internals in a separate > > series to ease modularization and vendor protocols support, and that will > > lead also to some changes in the SCMI driver interface that you use: > > amongst other things one interesting addition will be a new devres managed > > notification registration method, something like: > > > > handle->notify_ops->devm_register_notifier(sdev, ...); > > > > With such method you could just move your registration to the .probe > > step and just forget about it, without the need to add any unregistration > > in the .remove step, since the core will take care to remove all the > > callbacks at driver unloading time. > > > > Now, this series, which is here if you want to have a look: > > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > > > is already taking care to port any existent SCMI driver to the new interface, > > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I > > can in turn rebase my series on yours and take care to port your driver too to > > the new interface applying the changes above in the context of my series. > > (and ask you to review of course :D) > > I'm guessing you probably want this driver in an immutable branch then > rather than having to wait another cycle for it to tick through to a > a sensible upstream? > Hi Jonathan the above series (still pending a final review from Sudeep) is targeted at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi which in turn goes via the ARM soc branch. Having said that, I'm not really familiar enough with this sort of clashes to know how they should be properly handled, so I'll stick to what you and Sudeep would think it's better :D (..and I'm pinging him to have a say) Thanks Cristian > Jonathan > > > > > I'm saying that is better if you keep your series as it is for now > > (old interface + .preenable/.postdisable regs/unregs) because, as said, > > with the new interface the devm_ methods will ease the registration > > @probe time, and also especially because the new interface is not (and > > most probably won't) be part of the v5.4 backport that you are testing > > against: so if you stick with your current solution you'll have a > > working patch easily backportable now, and once queued I'll port it to > > the interface using devm_ (so simplifying it) > > > > In this context, it would be indeed important to know if in general moving > > registration to the probe phase (which should be fine by the spec) poses > > any kind of problem. (and that's reason why asked it) > > > > Hope to have been clear despite the flood of words :D > > > > Thanks > > > > Cristian > > > > > > > > Thanks, > > > Jyoti > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com> > > > wrote: > > > > > > > Hi Jyoti > > > > > > > > some minor things down below. > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > Thanks > > > > > > > > Cristian > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > > > > > This change provides ARM SCMI Protocol based IIO device. > > > > > This driver provides support for Accelerometer and Gyroscope using > > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > > --- > > > > > MAINTAINERS | 6 + > > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > > drivers/iio/common/Kconfig | 1 + > > > > > drivers/iio/common/Makefile | 1 + > > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > > --- a/MAINTAINERS > > > > > +++ b/MAINTAINERS > > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > > F: > > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > > > +IIO SCMI BASED DRIVER > > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > > +L: linux-iio@vger.kernel.org > > > > > +S: Maintained > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > + > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > > b/drivers/firmware/arm_scmi/driver.c > > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > > }; > > > > > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > > --- a/drivers/iio/common/Kconfig > > > > > +++ b/drivers/iio/common/Kconfig > > > > > @@ -6,5 +6,6 @@ > > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > > --- a/drivers/iio/common/Makefile > > > > > +++ b/drivers/iio/common/Makefile > > > > > @@ -11,5 +11,6 @@ > > > > > obj-y += cros_ec_sensors/ > > > > > obj-y += hid-sensors/ > > > > > obj-y += ms_sensors/ > > > > > +obj-y += scmi_sensors/ > > > > > obj-y += ssp_sensors/ > > > > > obj-y += st_sensors/ > > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > > new file mode 100644 > > > > > index 000000000000..67e084cbb1ab > > > > > --- /dev/null > > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > > @@ -0,0 +1,18 @@ > > > > > +# > > > > > +# IIO over SCMI > > > > > +# > > > > > +# When adding new entries keep the list in alphabetical order > > > > > + > > > > > +menu "IIO SCMI Sensors" > > > > > + > > > > > +config IIO_SCMI > > > > > + tristate "IIO SCMI" > > > > > + depends on ARM_SCMI_PROTOCOL > > > > > + select IIO_BUFFER > > > > > + select IIO_KFIFO_BUF > > > > > + help > > > > > + Say yes here to build support for IIO SCMI Driver. > > > > > + This provides ARM SCMI Protocol based IIO device. > > > > > + This driver provides support for accelerometer and gyroscope > > > > > + sensors available on SCMI based platforms. > > > > > +endmenu > > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > > new file mode 100644 > > > > > index 000000000000..f13140a2575a > > > > > --- /dev/null > > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > > @@ -0,0 +1,5 @@ > > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > > +# > > > > > +# Makefile for the IIO over SCMI > > > > > +# > > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > new file mode 100644 > > > > > index 000000000000..093b1fc24e27 > > > > > --- /dev/null > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > @@ -0,0 +1,673 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > + > > > > > +/* > > > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > > > + * > > > > > + * Copyright (C) 2021 Google LLC > > > > > + */ > > > > > + > > > > > +#include <linux/delay.h> > > > > > +#include <linux/err.h> > > > > > +#include <linux/iio/buffer.h> > > > > > +#include <linux/iio/iio.h> > > > > > +#include <linux/iio/kfifo_buf.h> > > > > > +#include <linux/iio/sysfs.h> > > > > > +#include <linux/kernel.h> > > > > > +#include <linux/kthread.h> > > > > > +#include <linux/module.h> > > > > > +#include <linux/scmi_protocol.h> > > > > > +#include <linux/time.h> > > > > > +#include <linux/types.h> > > > > > + > > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > > + > > > > > +struct scmi_iio_priv { > > > > > + struct scmi_handle *handle; > > > > > + const struct scmi_sensor_info *sensor_info; > > > > > + struct iio_dev *indio_dev; > > > > > + /* adding one additional channel for timestamp */ > > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > > + struct notifier_block sensor_update_nb; > > > > > + u32 *freq_avail; > > > > > +}; > > > > > + > > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > > + unsigned long event, void *data) > > > > > +{ > > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > > + struct iio_dev *scmi_iio_dev; > > > > > + struct scmi_iio_priv *sensor; > > > > > + s8 tstamp_scale; > > > > > + u64 time, time_ns; > > > > > + int i; > > > > > + > > > > > + if (sensor_update->readings_count == 0) > > > > > + return NOTIFY_DONE; > > > > > + > > > > > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > > > > > + > > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > > + sensor->iio_buf[i] = sensor_update->readings[i].value; > > > > > + > > > > > + if (!sensor->sensor_info->timestamped) { > > > > > + time_ns = ktime_to_ns(sensor_update->timestamp); > > > > > + } else { > > > > > + /* > > > > > + * All the axes are supposed to have the same value for > > > > timestamp. > > > > > + * We are just using the values from the Axis 0 here. > > > > > + */ > > > > > + time = sensor_update->readings[0].timestamp; > > > > > + > > > > > + /* > > > > > + * Timestamp returned by SCMI is in seconds and is equal > > > > to > > > > > + * time * power-of-10 multiplier(tstamp_scale) seconds. > > > > > + * Converting the timestamp to nanoseconds below. > > > > > + */ > > > > > + tstamp_scale = sensor->sensor_info->tstamp_scale + > > > > > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > > > > > + if (tstamp_scale < 0) > > > > > + time_ns = > > > > > + div64_u64(time, int_pow(10, > > > > abs(tstamp_scale))); > > > > > + else > > > > > + time_ns = time * int_pow(10, tstamp_scale); > > > > > + } > > > > > + > > > > > + scmi_iio_dev = sensor->indio_dev; > > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > > > > > + time_ns); > > > > > + return NOTIFY_OK; > > > > > +} > > > > > + > > > > > +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; > > > > > + > > > > > + if (sensor->sensor_info->timestamped) > > > > > + sensor_config |= > > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > > + > > > > > + 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); > > > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err > > > > %d", > > > > > + sensor->sensor_info->name, err); > > > > > + } > > > > > + > > > > > + return err; > > > > > +} > > > > > + > > > > > +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); > > > > > + if (err) { > > > > > + dev_err(&iio_dev->dev, > > > > > + "Error in disabling sensor %s with err %d", > > > > > + sensor->sensor_info->name, err); > > > > > + } > > > > > + > > > > > + return err; > > > > > +} > > > > > + > > > > > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > > > > > + .preenable = scmi_iio_buffer_preenable, > > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > > +}; > > > > > > > > This is just a question, I'm not suggesting to change anything here at > > > > this point to be clear, since it works just fine as it is. > > > > > > > > Following up a previous email, given these are called on enable/disable > > > > by sysfs, is there a specific reason why you configure here, inside > > > > these ops, also timestamping and callbacks i.e. each time the sensor is > > > > turned on/off by sysfs ? ... instead of just, as an example, enabling > > > > in _preenable the sensor while registering callbacks and enabling > > > > timestamping once for all earlier during probe phase ? > > > > (likewise for _postdisable -> remove) > > > > > > > > AFAIU the spec says notifications are emitted for sensors which has > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the > > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so > > > > that enabling/disabling the sensor as a whole should result in starting/ > > > > stopping the notification flow without the need of unregistering the > > > > callbacks everytime. (same goes with the timestamping) > > > > > > > > In other words, I would expect the sensor to maintain its state (on the > > > > platform side) even when going through enable/disable cycles, so that > > > > it 'remembers' that timestamping/notifications were enabled across an > > > > on/off. > > > > > > > > This would reduce the number of SCMI messages exchanges between the > > > > kernel and the platform and should be supported by both, but as said, > > > > it's more of a question for the future, not necessarily for this series. > > > > > > > > > + > > > > > > > > [snip] > > > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > > > > > +{ > > > > > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, > > > > step_size_ns, > > > > > + hz, uhz; > > > > > + unsigned int cur_interval, low_interval, high_interval, step_size; > > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > + int i; > > > > > + > > > > > + sensor->freq_avail = > > > > > + devm_kzalloc(&iio_dev->dev, > > > > > + sizeof(*sensor->freq_avail) * > > > > > + (sensor->sensor_info->intervals.count > > > > * 2), > > > > > + GFP_KERNEL); > > > > > + if (!sensor->freq_avail) > > > > > + return -ENOMEM; > > > > > + > > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > > + low_interval = sensor->sensor_info->intervals > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > > + low_interval_ns = > > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > > + sensor->freq_avail[0] = hz; > > > > > + sensor->freq_avail[1] = uhz; > > > > > + > > > > > + step_size = sensor->sensor_info->intervals > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > > + sensor->freq_avail[2] = hz; > > > > > + sensor->freq_avail[3] = uhz; > > > > > + > > > > > + high_interval = sensor->sensor_info->intervals > > > > > + > > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > > + high_interval_ns = > > > > > + scmi_iio_convert_interval_to_ns(high_interval); > > > > > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > > > > > + sensor->freq_avail[4] = hz; > > > > > + sensor->freq_avail[5] = uhz; > > > > > + } else { > > > > > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) > > > > { > > > > > + cur_interval = > > > > sensor->sensor_info->intervals.desc[i]; > > > > > + cur_interval_ns = > > > > > + > > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > > > > > + sensor->freq_avail[i * 2] = hz; > > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > > + } > > > > > + } > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > > +{ > > > > > + struct iio_buffer *buffer; > > > > > + > > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > > + if (!buffer) > > > > > + return -ENOMEM; > > > > > + > > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > > + return 0; > > > > > +} > > > > > + > > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > + struct scmi_handle *handle, > > > > > + 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; > > > > > + int i, ret; > > > > > + > > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > + if (!iiodev) > > > > > + return ERR_PTR(-ENOMEM); > > > > > + > > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > > + iiodev->dev.parent = dev; > > > > > + sensor = iio_priv(iiodev); > > > > > + sensor->handle = handle; > > > > > + sensor->sensor_info = sensor_info; > > > > > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > > + sensor->indio_dev = iiodev; > > > > > + > > > > > + /* adding one additional channel for timestamp */ > > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > > + iiodev->name = sensor_info->name; > > > > > + iiodev->info = &scmi_iio_info; > > > > > + > > > > > + iio_channels = > > > > > + devm_kzalloc(dev, > > > > > + sizeof(*iio_channels) * > > > > (iiodev->num_channels), > > > > > + GFP_KERNEL); > > > > > + if (!iio_channels) > > > > > + return ERR_PTR(-ENOMEM); > > > > > + > > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > > > You don't check this for retval, and it could fail with -ENOMEM. > > > > > > > > > + > > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > > &type); > > > > > + if (ret < 0) > > > > > + return ERR_PTR(ret); > > > > > + > > > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > > + &modifier); > > > > > + if (ret < 0) > > > > > + return ERR_PTR(ret); > > > > > + > > > > > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > > > > > + sensor_info->axis[i].id); > > > > > + } > > > > > + > > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > + iiodev->channels = iio_channels; > > > > > + return iiodev; > > > > > +} > > > > > + > > > > >
On Mon, 15 Feb 2021 09:25:26 +0000 Cristian Marussi <cristian.marussi@arm.com> wrote: > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote: > > On Wed, 10 Feb 2021 21:46:19 +0000 > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > Hi Jyoti, > > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > > > Hi Cristian, > > > > > > > > Thanks for the feedback. Regarding registering callbacks at the probe time > > > > instead of .preenable, I have tried it before but I think due to some > > > > issues(don't remember it now maybe on the platform side) I kept it at the > > > > .preenable level. > > > > > > > > But you are right, that it will be nice to move it at the probe level > > > > instead. I will try again and test if it works and would move it at the > > > > probe level. Regarding the unregistering of the notifier, is it required at > > > > the remove of iio driver or scmi driver will take care of it? > > > > Because if I add unregister at the iio driver remove level, I would have to > > > > iterate all the sensors again and unregister them. > > > > > > Yes you are right if you move callbacks registration once for all to the > > > .probe step you'll have to unregister them all in a .remove. > > > > > > BUT I think instead you should stick with your current solution given > > > it's working fine anyway and it's supported by the notification > > > framework and also for another reason I'm going to explain down below > > > (which is also the reason why I asked you this at first :D) > > > > > > As you may remember I'm refactoring all the SCMI internals in a separate > > > series to ease modularization and vendor protocols support, and that will > > > lead also to some changes in the SCMI driver interface that you use: > > > amongst other things one interesting addition will be a new devres managed > > > notification registration method, something like: > > > > > > handle->notify_ops->devm_register_notifier(sdev, ...); > > > > > > With such method you could just move your registration to the .probe > > > step and just forget about it, without the need to add any unregistration > > > in the .remove step, since the core will take care to remove all the > > > callbacks at driver unloading time. > > > > > > Now, this series, which is here if you want to have a look: > > > > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > > > > > is already taking care to port any existent SCMI driver to the new interface, > > > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I > > > can in turn rebase my series on yours and take care to port your driver too to > > > the new interface applying the changes above in the context of my series. > > > (and ask you to review of course :D) > > > > I'm guessing you probably want this driver in an immutable branch then > > rather than having to wait another cycle for it to tick through to a > > a sensible upstream? > > > > Hi Jonathan > > the above series (still pending a final review from Sudeep) is targeted > at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi > which in turn goes via the ARM soc branch. > > Having said that, I'm not really familiar enough with this sort of clashes > to know how they should be properly handled, so I'll stick to what you > and Sudeep would think it's better :D (..and I'm pinging him to have a say) > > Thanks > > Cristian Hi Cristian, So this driver will also be 5.13 material now (merge window for IIO effectively closes 1-2 weeks before Linus opens the main one). The way we normally handle cases like this where we likely to have dependencies on a patch set from two separate directions is to do what is known as an immutable branch. This is a branch that would probably be based on 5.12-rc1 containing just this driver. Then both trees, in this case IIO and scmi merge that branch. The magic of git then means that when Linus gets the eventual pull requests for the two trees, the git IDs and content will be the same and the history of that particular set of files will be cleanly maintained. This happens quite a lot for certain parts of the kernel because there are a lot of cross dependencies. @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give us a sensible base. Jonathan > > > Jonathan > > > > > > > > I'm saying that is better if you keep your series as it is for now > > > (old interface + .preenable/.postdisable regs/unregs) because, as said, > > > with the new interface the devm_ methods will ease the registration > > > @probe time, and also especially because the new interface is not (and > > > most probably won't) be part of the v5.4 backport that you are testing > > > against: so if you stick with your current solution you'll have a > > > working patch easily backportable now, and once queued I'll port it to > > > the interface using devm_ (so simplifying it) > > > > > > In this context, it would be indeed important to know if in general moving > > > registration to the probe phase (which should be fine by the spec) poses > > > any kind of problem. (and that's reason why asked it) > > > > > > Hope to have been clear despite the flood of words :D > > > > > > Thanks > > > > > > Cristian > > > > > > > > > > > Thanks, > > > > Jyoti > > > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com> > > > > wrote: > > > > > > > > > Hi Jyoti > > > > > > > > > > some minor things down below. > > > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > > > Thanks > > > > > > > > > > Cristian > > > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > > > > > > This change provides ARM SCMI Protocol based IIO device. > > > > > > This driver provides support for Accelerometer and Gyroscope using > > > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > > > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > > > --- > > > > > > MAINTAINERS | 6 + > > > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > > > drivers/iio/common/Kconfig | 1 + > > > > > > drivers/iio/common/Makefile | 1 + > > > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > > > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > > > --- a/MAINTAINERS > > > > > > +++ b/MAINTAINERS > > > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > > > F: > > > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > > > > > +IIO SCMI BASED DRIVER > > > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > > > +L: linux-iio@vger.kernel.org > > > > > > +S: Maintained > > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > + > > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > > > b/drivers/firmware/arm_scmi/driver.c > > > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > > > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > > > }; > > > > > > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > > > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > > > --- a/drivers/iio/common/Kconfig > > > > > > +++ b/drivers/iio/common/Kconfig > > > > > > @@ -6,5 +6,6 @@ > > > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > > > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > > > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > > > --- a/drivers/iio/common/Makefile > > > > > > +++ b/drivers/iio/common/Makefile > > > > > > @@ -11,5 +11,6 @@ > > > > > > obj-y += cros_ec_sensors/ > > > > > > obj-y += hid-sensors/ > > > > > > obj-y += ms_sensors/ > > > > > > +obj-y += scmi_sensors/ > > > > > > obj-y += ssp_sensors/ > > > > > > obj-y += st_sensors/ > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > new file mode 100644 > > > > > > index 000000000000..67e084cbb1ab > > > > > > --- /dev/null > > > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > @@ -0,0 +1,18 @@ > > > > > > +# > > > > > > +# IIO over SCMI > > > > > > +# > > > > > > +# When adding new entries keep the list in alphabetical order > > > > > > + > > > > > > +menu "IIO SCMI Sensors" > > > > > > + > > > > > > +config IIO_SCMI > > > > > > + tristate "IIO SCMI" > > > > > > + depends on ARM_SCMI_PROTOCOL > > > > > > + select IIO_BUFFER > > > > > > + select IIO_KFIFO_BUF > > > > > > + help > > > > > > + Say yes here to build support for IIO SCMI Driver. > > > > > > + This provides ARM SCMI Protocol based IIO device. > > > > > > + This driver provides support for accelerometer and gyroscope > > > > > > + sensors available on SCMI based platforms. > > > > > > +endmenu > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > > > new file mode 100644 > > > > > > index 000000000000..f13140a2575a > > > > > > --- /dev/null > > > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > > > @@ -0,0 +1,5 @@ > > > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > > > +# > > > > > > +# Makefile for the IIO over SCMI > > > > > > +# > > > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > new file mode 100644 > > > > > > index 000000000000..093b1fc24e27 > > > > > > --- /dev/null > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > @@ -0,0 +1,673 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > + > > > > > > +/* > > > > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > > > > + * > > > > > > + * Copyright (C) 2021 Google LLC > > > > > > + */ > > > > > > + > > > > > > +#include <linux/delay.h> > > > > > > +#include <linux/err.h> > > > > > > +#include <linux/iio/buffer.h> > > > > > > +#include <linux/iio/iio.h> > > > > > > +#include <linux/iio/kfifo_buf.h> > > > > > > +#include <linux/iio/sysfs.h> > > > > > > +#include <linux/kernel.h> > > > > > > +#include <linux/kthread.h> > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/scmi_protocol.h> > > > > > > +#include <linux/time.h> > > > > > > +#include <linux/types.h> > > > > > > + > > > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > + > > > > > > +struct scmi_iio_priv { > > > > > > + struct scmi_handle *handle; > > > > > > + const struct scmi_sensor_info *sensor_info; > > > > > > + struct iio_dev *indio_dev; > > > > > > + /* adding one additional channel for timestamp */ > > > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > > > + struct notifier_block sensor_update_nb; > > > > > > + u32 *freq_avail; > > > > > > +}; > > > > > > + > > > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > > > + unsigned long event, void *data) > > > > > > +{ > > > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > > > + struct iio_dev *scmi_iio_dev; > > > > > > + struct scmi_iio_priv *sensor; > > > > > > + s8 tstamp_scale; > > > > > > + u64 time, time_ns; > > > > > > + int i; > > > > > > + > > > > > > + if (sensor_update->readings_count == 0) > > > > > > + return NOTIFY_DONE; > > > > > > + > > > > > > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > > > > > > + > > > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > > > + sensor->iio_buf[i] = sensor_update->readings[i].value; > > > > > > + > > > > > > + if (!sensor->sensor_info->timestamped) { > > > > > > + time_ns = ktime_to_ns(sensor_update->timestamp); > > > > > > + } else { > > > > > > + /* > > > > > > + * All the axes are supposed to have the same value for > > > > > timestamp. > > > > > > + * We are just using the values from the Axis 0 here. > > > > > > + */ > > > > > > + time = sensor_update->readings[0].timestamp; > > > > > > + > > > > > > + /* > > > > > > + * Timestamp returned by SCMI is in seconds and is equal > > > > > to > > > > > > + * time * power-of-10 multiplier(tstamp_scale) seconds. > > > > > > + * Converting the timestamp to nanoseconds below. > > > > > > + */ > > > > > > + tstamp_scale = sensor->sensor_info->tstamp_scale + > > > > > > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > > > > > > + if (tstamp_scale < 0) > > > > > > + time_ns = > > > > > > + div64_u64(time, int_pow(10, > > > > > abs(tstamp_scale))); > > > > > > + else > > > > > > + time_ns = time * int_pow(10, tstamp_scale); > > > > > > + } > > > > > > + > > > > > > + scmi_iio_dev = sensor->indio_dev; > > > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > > > > > > + time_ns); > > > > > > + return NOTIFY_OK; > > > > > > +} > > > > > > + > > > > > > +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; > > > > > > + > > > > > > + if (sensor->sensor_info->timestamped) > > > > > > + sensor_config |= > > > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > > > + > > > > > > + 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); > > > > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err > > > > > %d", > > > > > > + sensor->sensor_info->name, err); > > > > > > + } > > > > > > + > > > > > > + return err; > > > > > > +} > > > > > > + > > > > > > +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); > > > > > > + if (err) { > > > > > > + dev_err(&iio_dev->dev, > > > > > > + "Error in disabling sensor %s with err %d", > > > > > > + sensor->sensor_info->name, err); > > > > > > + } > > > > > > + > > > > > > + return err; > > > > > > +} > > > > > > + > > > > > > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > > > > > > + .preenable = scmi_iio_buffer_preenable, > > > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > > > +}; > > > > > > > > > > This is just a question, I'm not suggesting to change anything here at > > > > > this point to be clear, since it works just fine as it is. > > > > > > > > > > Following up a previous email, given these are called on enable/disable > > > > > by sysfs, is there a specific reason why you configure here, inside > > > > > these ops, also timestamping and callbacks i.e. each time the sensor is > > > > > turned on/off by sysfs ? ... instead of just, as an example, enabling > > > > > in _preenable the sensor while registering callbacks and enabling > > > > > timestamping once for all earlier during probe phase ? > > > > > (likewise for _postdisable -> remove) > > > > > > > > > > AFAIU the spec says notifications are emitted for sensors which has > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so > > > > > that enabling/disabling the sensor as a whole should result in starting/ > > > > > stopping the notification flow without the need of unregistering the > > > > > callbacks everytime. (same goes with the timestamping) > > > > > > > > > > In other words, I would expect the sensor to maintain its state (on the > > > > > platform side) even when going through enable/disable cycles, so that > > > > > it 'remembers' that timestamping/notifications were enabled across an > > > > > on/off. > > > > > > > > > > This would reduce the number of SCMI messages exchanges between the > > > > > kernel and the platform and should be supported by both, but as said, > > > > > it's more of a question for the future, not necessarily for this series. > > > > > > > > > > > + > > > > > > > > > > [snip] > > > > > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > > > > > > +{ > > > > > > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, > > > > > step_size_ns, > > > > > > + hz, uhz; > > > > > > + unsigned int cur_interval, low_interval, high_interval, step_size; > > > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > > + int i; > > > > > > + > > > > > > + sensor->freq_avail = > > > > > > + devm_kzalloc(&iio_dev->dev, > > > > > > + sizeof(*sensor->freq_avail) * > > > > > > + (sensor->sensor_info->intervals.count > > > > > * 2), > > > > > > + GFP_KERNEL); > > > > > > + if (!sensor->freq_avail) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > > > + low_interval = sensor->sensor_info->intervals > > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > > > + low_interval_ns = > > > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > > > + sensor->freq_avail[0] = hz; > > > > > > + sensor->freq_avail[1] = uhz; > > > > > > + > > > > > > + step_size = sensor->sensor_info->intervals > > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > > > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > > > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > > > + sensor->freq_avail[2] = hz; > > > > > > + sensor->freq_avail[3] = uhz; > > > > > > + > > > > > > + high_interval = sensor->sensor_info->intervals > > > > > > + > > > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > > > + high_interval_ns = > > > > > > + scmi_iio_convert_interval_to_ns(high_interval); > > > > > > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > > > > > > + sensor->freq_avail[4] = hz; > > > > > > + sensor->freq_avail[5] = uhz; > > > > > > + } else { > > > > > > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) > > > > > { > > > > > > + cur_interval = > > > > > sensor->sensor_info->intervals.desc[i]; > > > > > > + cur_interval_ns = > > > > > > + > > > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > > > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > > > > > > + sensor->freq_avail[i * 2] = hz; > > > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > > > + } > > > > > > + } > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > > > +{ > > > > > > + struct iio_buffer *buffer; > > > > > > + > > > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > > > + if (!buffer) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > > + struct scmi_handle *handle, > > > > > > + 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; > > > > > > + int i, ret; > > > > > > + > > > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > > + if (!iiodev) > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > + > > > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > > > + iiodev->dev.parent = dev; > > > > > > + sensor = iio_priv(iiodev); > > > > > > + sensor->handle = handle; > > > > > > + sensor->sensor_info = sensor_info; > > > > > > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > > > + sensor->indio_dev = iiodev; > > > > > > + > > > > > > + /* adding one additional channel for timestamp */ > > > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > > > + iiodev->name = sensor_info->name; > > > > > > + iiodev->info = &scmi_iio_info; > > > > > > + > > > > > > + iio_channels = > > > > > > + devm_kzalloc(dev, > > > > > > + sizeof(*iio_channels) * > > > > > (iiodev->num_channels), > > > > > > + GFP_KERNEL); > > > > > > + if (!iio_channels) > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > + > > > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > > > > > You don't check this for retval, and it could fail with -ENOMEM. > > > > > > > > > > > + > > > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > > > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > > > &type); > > > > > > + if (ret < 0) > > > > > > + return ERR_PTR(ret); > > > > > > + > > > > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > > > + &modifier); > > > > > > + if (ret < 0) > > > > > > + return ERR_PTR(ret); > > > > > > + > > > > > > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > > > > > > + sensor_info->axis[i].id); > > > > > > + } > > > > > > + > > > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > > + iiodev->channels = iio_channels; > > > > > > + return iiodev; > > > > > > +} > > > > > > + > > > > > > >
On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote: > Hi Cristian, > > So this driver will also be 5.13 material now (merge window for IIO effectively > closes 1-2 weeks before Linus opens the main one). > I guessed so. > The way we normally handle cases like this where we likely to have dependencies > on a patch set from two separate directions is to do what is known as an > immutable branch. This is a branch that would probably be based on 5.12-rc1 > containing just this driver. > Make sense. > Then both trees, in this case IIO and scmi merge that branch. The magic > of git then means that when Linus gets the eventual pull requests for > the two trees, the git IDs and content will be the same and the history > of that particular set of files will be cleanly maintained. > > This happens quite a lot for certain parts of the kernel because there are > a lot of cross dependencies. > > @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give > us a sensible base. > Since this is just one patch, I can pull in the immutable branch if you could share and then Cristian can make appropriate changes needed for his series on top of that unless you want to merge 30+ patches that I might have with Cristain series
On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote: > On Mon, 15 Feb 2021 09:25:26 +0000 > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote: > > > On Wed, 10 Feb 2021 21:46:19 +0000 > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > Hi Jyoti, > > > > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > > > > Hi Cristian, > > > > > > > > > > Thanks for the feedback. Regarding registering callbacks at the probe time > > > > > instead of .preenable, I have tried it before but I think due to some > > > > > issues(don't remember it now maybe on the platform side) I kept it at the > > > > > .preenable level. > > > > > > > > > > But you are right, that it will be nice to move it at the probe level > > > > > instead. I will try again and test if it works and would move it at the > > > > > probe level. Regarding the unregistering of the notifier, is it required at > > > > > the remove of iio driver or scmi driver will take care of it? > > > > > Because if I add unregister at the iio driver remove level, I would have to > > > > > iterate all the sensors again and unregister them. > > > > > > > > Yes you are right if you move callbacks registration once for all to the > > > > .probe step you'll have to unregister them all in a .remove. > > > > > > > > BUT I think instead you should stick with your current solution given > > > > it's working fine anyway and it's supported by the notification > > > > framework and also for another reason I'm going to explain down below > > > > (which is also the reason why I asked you this at first :D) > > > > > > > > As you may remember I'm refactoring all the SCMI internals in a separate > > > > series to ease modularization and vendor protocols support, and that will > > > > lead also to some changes in the SCMI driver interface that you use: > > > > amongst other things one interesting addition will be a new devres managed > > > > notification registration method, something like: > > > > > > > > handle->notify_ops->devm_register_notifier(sdev, ...); > > > > > > > > With such method you could just move your registration to the .probe > > > > step and just forget about it, without the need to add any unregistration > > > > in the .remove step, since the core will take care to remove all the > > > > callbacks at driver unloading time. > > > > > > > > Now, this series, which is here if you want to have a look: > > > > > > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > > > > > > > is already taking care to port any existent SCMI driver to the new interface, > > > > so when your IIODEV SCMI driver will be finally queued somewhere for merge, I > > > > can in turn rebase my series on yours and take care to port your driver too to > > > > the new interface applying the changes above in the context of my series. > > > > (and ask you to review of course :D) > > > > > > I'm guessing you probably want this driver in an immutable branch then > > > rather than having to wait another cycle for it to tick through to a > > > a sensible upstream? > > > > > > > Hi Jonathan > > > > the above series (still pending a final review from Sudeep) is targeted > > at 5.13 at this point and usually it'd be queued via Sudeep for-next/scmi > > which in turn goes via the ARM soc branch. > > > > Having said that, I'm not really familiar enough with this sort of clashes > > to know how they should be properly handled, so I'll stick to what you > > and Sudeep would think it's better :D (..and I'm pinging him to have a say) > > > > Thanks > > > > Cristian > Hi Cristian, > > So this driver will also be 5.13 material now (merge window for IIO effectively > closes 1-2 weeks before Linus opens the main one). > > The way we normally handle cases like this where we likely to have dependencies > on a patch set from two separate directions is to do what is known as an > immutable branch. This is a branch that would probably be based on 5.12-rc1 > containing just this driver. > > Then both trees, in this case IIO and scmi merge that branch. The magic > of git then means that when Linus gets the eventual pull requests for > the two trees, the git IDs and content will be the same and the history > of that particular set of files will be cleanly maintained. > > This happens quite a lot for certain parts of the kernel because there are > a lot of cross dependencies. > Hi Jonathan thanks for clarifying. Cristian > @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give > us a sensible base. > > Jonathan > > > > > > Jonathan > > > > > > > > > > > I'm saying that is better if you keep your series as it is for now > > > > (old interface + .preenable/.postdisable regs/unregs) because, as said, > > > > with the new interface the devm_ methods will ease the registration > > > > @probe time, and also especially because the new interface is not (and > > > > most probably won't) be part of the v5.4 backport that you are testing > > > > against: so if you stick with your current solution you'll have a > > > > working patch easily backportable now, and once queued I'll port it to > > > > the interface using devm_ (so simplifying it) > > > > > > > > In this context, it would be indeed important to know if in general moving > > > > registration to the probe phase (which should be fine by the spec) poses > > > > any kind of problem. (and that's reason why asked it) > > > > > > > > Hope to have been clear despite the flood of words :D > > > > > > > > Thanks > > > > > > > > Cristian > > > > > > > > > > > > > > Thanks, > > > > > Jyoti > > > > > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi <cristian.marussi@arm.com> > > > > > wrote: > > > > > > > > > > > Hi Jyoti > > > > > > > > > > > > some minor things down below. > > > > > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > > > > > Thanks > > > > > > > > > > > > Cristian > > > > > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana wrote: > > > > > > > This change provides ARM SCMI Protocol based IIO device. > > > > > > > This driver provides support for Accelerometer and Gyroscope using > > > > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification > > > > > > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > > > > --- > > > > > > > MAINTAINERS | 6 + > > > > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > > > > drivers/iio/common/Kconfig | 1 + > > > > > > > drivers/iio/common/Makefile | 1 + > > > > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ > > > > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > > > > --- a/MAINTAINERS > > > > > > > +++ b/MAINTAINERS > > > > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > > > > F: > > > > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > > > > > > > +IIO SCMI BASED DRIVER > > > > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > > > > +L: linux-iio@vger.kernel.org > > > > > > > +S: Maintained > > > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > + > > > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > > > > b/drivers/firmware/arm_scmi/driver.c > > > > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { > > > > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > > > > }; > > > > > > > diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig > > > > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > > > > --- a/drivers/iio/common/Kconfig > > > > > > > +++ b/drivers/iio/common/Kconfig > > > > > > > @@ -6,5 +6,6 @@ > > > > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > > > > diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile > > > > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > > > > --- a/drivers/iio/common/Makefile > > > > > > > +++ b/drivers/iio/common/Makefile > > > > > > > @@ -11,5 +11,6 @@ > > > > > > > obj-y += cros_ec_sensors/ > > > > > > > obj-y += hid-sensors/ > > > > > > > obj-y += ms_sensors/ > > > > > > > +obj-y += scmi_sensors/ > > > > > > > obj-y += ssp_sensors/ > > > > > > > obj-y += st_sensors/ > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > new file mode 100644 > > > > > > > index 000000000000..67e084cbb1ab > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > @@ -0,0 +1,18 @@ > > > > > > > +# > > > > > > > +# IIO over SCMI > > > > > > > +# > > > > > > > +# When adding new entries keep the list in alphabetical order > > > > > > > + > > > > > > > +menu "IIO SCMI Sensors" > > > > > > > + > > > > > > > +config IIO_SCMI > > > > > > > + tristate "IIO SCMI" > > > > > > > + depends on ARM_SCMI_PROTOCOL > > > > > > > + select IIO_BUFFER > > > > > > > + select IIO_KFIFO_BUF > > > > > > > + help > > > > > > > + Say yes here to build support for IIO SCMI Driver. > > > > > > > + This provides ARM SCMI Protocol based IIO device. > > > > > > > + This driver provides support for accelerometer and gyroscope > > > > > > > + sensors available on SCMI based platforms. > > > > > > > +endmenu > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > new file mode 100644 > > > > > > > index 000000000000..f13140a2575a > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > @@ -0,0 +1,5 @@ > > > > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > > > > +# > > > > > > > +# Makefile for the IIO over SCMI > > > > > > > +# > > > > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > new file mode 100644 > > > > > > > index 000000000000..093b1fc24e27 > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > @@ -0,0 +1,673 @@ > > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > > + > > > > > > > +/* > > > > > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > > > > > + * > > > > > > > + * Copyright (C) 2021 Google LLC > > > > > > > + */ > > > > > > > + > > > > > > > +#include <linux/delay.h> > > > > > > > +#include <linux/err.h> > > > > > > > +#include <linux/iio/buffer.h> > > > > > > > +#include <linux/iio/iio.h> > > > > > > > +#include <linux/iio/kfifo_buf.h> > > > > > > > +#include <linux/iio/sysfs.h> > > > > > > > +#include <linux/kernel.h> > > > > > > > +#include <linux/kthread.h> > > > > > > > +#include <linux/module.h> > > > > > > > +#include <linux/scmi_protocol.h> > > > > > > > +#include <linux/time.h> > > > > > > > +#include <linux/types.h> > > > > > > > + > > > > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > > + > > > > > > > +struct scmi_iio_priv { > > > > > > > + struct scmi_handle *handle; > > > > > > > + const struct scmi_sensor_info *sensor_info; > > > > > > > + struct iio_dev *indio_dev; > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > > > > + struct notifier_block sensor_update_nb; > > > > > > > + u32 *freq_avail; > > > > > > > +}; > > > > > > > + > > > > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, > > > > > > > + unsigned long event, void *data) > > > > > > > +{ > > > > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > > > > + struct iio_dev *scmi_iio_dev; > > > > > > > + struct scmi_iio_priv *sensor; > > > > > > > + s8 tstamp_scale; > > > > > > > + u64 time, time_ns; > > > > > > > + int i; > > > > > > > + > > > > > > > + if (sensor_update->readings_count == 0) > > > > > > > + return NOTIFY_DONE; > > > > > > > + > > > > > > > + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); > > > > > > > + > > > > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > > > > + sensor->iio_buf[i] = sensor_update->readings[i].value; > > > > > > > + > > > > > > > + if (!sensor->sensor_info->timestamped) { > > > > > > > + time_ns = ktime_to_ns(sensor_update->timestamp); > > > > > > > + } else { > > > > > > > + /* > > > > > > > + * All the axes are supposed to have the same value for > > > > > > timestamp. > > > > > > > + * We are just using the values from the Axis 0 here. > > > > > > > + */ > > > > > > > + time = sensor_update->readings[0].timestamp; > > > > > > > + > > > > > > > + /* > > > > > > > + * Timestamp returned by SCMI is in seconds and is equal > > > > > > to > > > > > > > + * time * power-of-10 multiplier(tstamp_scale) seconds. > > > > > > > + * Converting the timestamp to nanoseconds below. > > > > > > > + */ > > > > > > > + tstamp_scale = sensor->sensor_info->tstamp_scale + > > > > > > > + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); > > > > > > > + if (tstamp_scale < 0) > > > > > > > + time_ns = > > > > > > > + div64_u64(time, int_pow(10, > > > > > > abs(tstamp_scale))); > > > > > > > + else > > > > > > > + time_ns = time * int_pow(10, tstamp_scale); > > > > > > > + } > > > > > > > + > > > > > > > + scmi_iio_dev = sensor->indio_dev; > > > > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, > > > > > > > + time_ns); > > > > > > > + return NOTIFY_OK; > > > > > > > +} > > > > > > > + > > > > > > > +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; > > > > > > > + > > > > > > > + if (sensor->sensor_info->timestamped) > > > > > > > + sensor_config |= > > > > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > > > > + > > > > > > > + 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); > > > > > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err > > > > > > %d", > > > > > > > + sensor->sensor_info->name, err); > > > > > > > + } > > > > > > > + > > > > > > > + return err; > > > > > > > +} > > > > > > > + > > > > > > > +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); > > > > > > > + if (err) { > > > > > > > + dev_err(&iio_dev->dev, > > > > > > > + "Error in disabling sensor %s with err %d", > > > > > > > + sensor->sensor_info->name, err); > > > > > > > + } > > > > > > > + > > > > > > > + return err; > > > > > > > +} > > > > > > > + > > > > > > > +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { > > > > > > > + .preenable = scmi_iio_buffer_preenable, > > > > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > > > > +}; > > > > > > > > > > > > This is just a question, I'm not suggesting to change anything here at > > > > > > this point to be clear, since it works just fine as it is. > > > > > > > > > > > > Following up a previous email, given these are called on enable/disable > > > > > > by sysfs, is there a specific reason why you configure here, inside > > > > > > these ops, also timestamping and callbacks i.e. each time the sensor is > > > > > > turned on/off by sysfs ? ... instead of just, as an example, enabling > > > > > > in _preenable the sensor while registering callbacks and enabling > > > > > > timestamping once for all earlier during probe phase ? > > > > > > (likewise for _postdisable -> remove) > > > > > > > > > > > > AFAIU the spec says notifications are emitted for sensors which has > > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only if the > > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you do), so > > > > > > that enabling/disabling the sensor as a whole should result in starting/ > > > > > > stopping the notification flow without the need of unregistering the > > > > > > callbacks everytime. (same goes with the timestamping) > > > > > > > > > > > > In other words, I would expect the sensor to maintain its state (on the > > > > > > platform side) even when going through enable/disable cycles, so that > > > > > > it 'remembers' that timestamping/notifications were enabled across an > > > > > > on/off. > > > > > > > > > > > > This would reduce the number of SCMI messages exchanges between the > > > > > > kernel and the platform and should be supported by both, but as said, > > > > > > it's more of a question for the future, not necessarily for this series. > > > > > > > > > > > > > + > > > > > > > > > > > > [snip] > > > > > > > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) > > > > > > > +{ > > > > > > > + u64 cur_interval_ns, low_interval_ns, high_interval_ns, > > > > > > step_size_ns, > > > > > > > + hz, uhz; > > > > > > > + unsigned int cur_interval, low_interval, high_interval, step_size; > > > > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > > > + int i; > > > > > > > + > > > > > > > + sensor->freq_avail = > > > > > > > + devm_kzalloc(&iio_dev->dev, > > > > > > > + sizeof(*sensor->freq_avail) * > > > > > > > + (sensor->sensor_info->intervals.count > > > > > > * 2), > > > > > > > + GFP_KERNEL); > > > > > > > + if (!sensor->freq_avail) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > > > > + low_interval = sensor->sensor_info->intervals > > > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > > > > + low_interval_ns = > > > > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > > > > + sensor->freq_avail[0] = hz; > > > > > > > + sensor->freq_avail[1] = uhz; > > > > > > > + > > > > > > > + step_size = sensor->sensor_info->intervals > > > > > > > + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > > > > + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); > > > > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > > > > + sensor->freq_avail[2] = hz; > > > > > > > + sensor->freq_avail[3] = uhz; > > > > > > > + > > > > > > > + high_interval = sensor->sensor_info->intervals > > > > > > > + > > > > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > > > > + high_interval_ns = > > > > > > > + scmi_iio_convert_interval_to_ns(high_interval); > > > > > > > + convert_ns_to_freq(high_interval_ns, &hz, &uhz); > > > > > > > + sensor->freq_avail[4] = hz; > > > > > > > + sensor->freq_avail[5] = uhz; > > > > > > > + } else { > > > > > > > + for (i = 0; i < sensor->sensor_info->intervals.count; i++) > > > > > > { > > > > > > > + cur_interval = > > > > > > sensor->sensor_info->intervals.desc[i]; > > > > > > > + cur_interval_ns = > > > > > > > + > > > > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > > > > + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); > > > > > > > + sensor->freq_avail[i * 2] = hz; > > > > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > > > > + } > > > > > > > + } > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) > > > > > > > +{ > > > > > > > + struct iio_buffer *buffer; > > > > > > > + > > > > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > > > > + if (!buffer) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > > > + struct scmi_handle *handle, > > > > > > > + 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; > > > > > > > + int i, ret; > > > > > > > + > > > > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > > > + if (!iiodev) > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > + > > > > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > > > > + iiodev->dev.parent = dev; > > > > > > > + sensor = iio_priv(iiodev); > > > > > > > + sensor->handle = handle; > > > > > > > + sensor->sensor_info = sensor_info; > > > > > > > + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; > > > > > > > + sensor->indio_dev = iiodev; > > > > > > > + > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > > > > + iiodev->name = sensor_info->name; > > > > > > > + iiodev->info = &scmi_iio_info; > > > > > > > + > > > > > > > + iio_channels = > > > > > > > + devm_kzalloc(dev, > > > > > > > + sizeof(*iio_channels) * > > > > > > (iiodev->num_channels), > > > > > > > + GFP_KERNEL); > > > > > > > + if (!iio_channels) > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > + > > > > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > > > > > > > You don't check this for retval, and it could fail with -ENOMEM. > > > > > > > > > > > > > + > > > > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > > > > + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > > > > &type); > > > > > > > + if (ret < 0) > > > > > > > + return ERR_PTR(ret); > > > > > > > + > > > > > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > > > > + &modifier); > > > > > > > + if (ret < 0) > > > > > > > + return ERR_PTR(ret); > > > > > > > + > > > > > > > + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, > > > > > > > + sensor_info->axis[i].id); > > > > > > > + } > > > > > > > + > > > > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > > > + iiodev->channels = iio_channels; > > > > > > > + return iiodev; > > > > > > > +} > > > > > > > + > > > > > > > > > >
Hi Jyoti, On Mon, Feb 15, 2021 at 04:14:57PM -0800, Jyoti Bhayana wrote: > Hi Jonathan/Cristian, > > I have uploaded v6 of the IIO SCMI patch. Not sure if you got a chance to > review that version and if any further changes are needed. Seen that, for the SCMI part seemed fine to me, I'll just have still to reply. > My IIO SCMI patch is independent of Cristian series mentioned at > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > right? Yes, that's true, your patch is independent and goes in as it is now, and I'll rebase my series on yours on a dedicated branch and make all the needed (small) interface changes. > and it can be merged without the Cristian Series right? Are there plans to > merge my v6 of the IIO SCMI patch and Cristian latest series in the same > linux tree version? Yes absolutely, your driver can be merged as it is without my series, but due to the timing they ended up targeting the same Linux version 5.13 at the end, so they will be merged both in v5.13, yours first as it is now, mine immediately rebased on top of it aftewards: the end result will be both series in the same v5.13 but as distinct patches. (so you can still backport yours on v5.4 as you're doing now.) Thanks Cristian > > Thanks, > Jyoti > > On Mon, Feb 15, 2021 at 6:48 AM Cristian Marussi <cristian.marussi@arm.com> > wrote: > > > On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote: > > > On Mon, 15 Feb 2021 09:25:26 +0000 > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote: > > > > > On Wed, 10 Feb 2021 21:46:19 +0000 > > > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > > > > > Hi Jyoti, > > > > > > > > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > > > > > > Hi Cristian, > > > > > > > > > > > > > > Thanks for the feedback. Regarding registering callbacks at the > > probe time > > > > > > > instead of .preenable, I have tried it before but I think due to > > some > > > > > > > issues(don't remember it now maybe on the platform side) I kept > > it at the > > > > > > > .preenable level. > > > > > > > > > > > > > > But you are right, that it will be nice to move it at the probe > > level > > > > > > > instead. I will try again and test if it works and would move it > > at the > > > > > > > probe level. Regarding the unregistering of the notifier, is it > > required at > > > > > > > the remove of iio driver or scmi driver will take care of it? > > > > > > > Because if I add unregister at the iio driver remove level, I > > would have to > > > > > > > iterate all the sensors again and unregister them. > > > > > > > > > > > > Yes you are right if you move callbacks registration once for all > > to the > > > > > > .probe step you'll have to unregister them all in a .remove. > > > > > > > > > > > > BUT I think instead you should stick with your current solution > > given > > > > > > it's working fine anyway and it's supported by the notification > > > > > > framework and also for another reason I'm going to explain down > > below > > > > > > (which is also the reason why I asked you this at first :D) > > > > > > > > > > > > As you may remember I'm refactoring all the SCMI internals in a > > separate > > > > > > series to ease modularization and vendor protocols support, and > > that will > > > > > > lead also to some changes in the SCMI driver interface that you > > use: > > > > > > amongst other things one interesting addition will be a new devres > > managed > > > > > > notification registration method, something like: > > > > > > > > > > > > handle->notify_ops->devm_register_notifier(sdev, ...); > > > > > > > > > > > > With such method you could just move your registration to the > > .probe > > > > > > step and just forget about it, without the need to add any > > unregistration > > > > > > in the .remove step, since the core will take care to remove all > > the > > > > > > callbacks at driver unloading time. > > > > > > > > > > > > Now, this series, which is here if you want to have a look: > > > > > > > > > > > > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > > > > > > > > > > > is already taking care to port any existent SCMI driver to the new > > interface, > > > > > > so when your IIODEV SCMI driver will be finally queued somewhere > > for merge, I > > > > > > can in turn rebase my series on yours and take care to port your > > driver too to > > > > > > the new interface applying the changes above in the context of my > > series. > > > > > > (and ask you to review of course :D) > > > > > > > > > > I'm guessing you probably want this driver in an immutable branch > > then > > > > > rather than having to wait another cycle for it to tick through to a > > > > > a sensible upstream? > > > > > > > > > > > > > Hi Jonathan > > > > > > > > the above series (still pending a final review from Sudeep) is targeted > > > > at 5.13 at this point and usually it'd be queued via Sudeep > > for-next/scmi > > > > which in turn goes via the ARM soc branch. > > > > > > > > Having said that, I'm not really familiar enough with this sort of > > clashes > > > > to know how they should be properly handled, so I'll stick to what you > > > > and Sudeep would think it's better :D (..and I'm pinging him to have a > > say) > > > > > > > > Thanks > > > > > > > > Cristian > > > Hi Cristian, > > > > > > So this driver will also be 5.13 material now (merge window for IIO > > effectively > > > closes 1-2 weeks before Linus opens the main one). > > > > > > The way we normally handle cases like this where we likely to have > > dependencies > > > on a patch set from two separate directions is to do what is known as an > > > immutable branch. This is a branch that would probably be based on > > 5.12-rc1 > > > containing just this driver. > > > > > > Then both trees, in this case IIO and scmi merge that branch. The magic > > > of git then means that when Linus gets the eventual pull requests for > > > the two trees, the git IDs and content will be the same and the history > > > of that particular set of files will be cleanly maintained. > > > > > > This happens quite a lot for certain parts of the kernel because there > > are > > > a lot of cross dependencies. > > > > > Hi Jonathan > > > > thanks for clarifying. > > > > Cristian > > > > > @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give > > > us a sensible base. > > > > > > Jonathan > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > I'm saying that is better if you keep your series as it is for now > > > > > > (old interface + .preenable/.postdisable regs/unregs) because, as > > said, > > > > > > with the new interface the devm_ methods will ease the registration > > > > > > @probe time, and also especially because the new interface is not > > (and > > > > > > most probably won't) be part of the v5.4 backport that you are > > testing > > > > > > against: so if you stick with your current solution you'll have a > > > > > > working patch easily backportable now, and once queued I'll port > > it to > > > > > > the interface using devm_ (so simplifying it) > > > > > > > > > > > > In this context, it would be indeed important to know if in > > general moving > > > > > > registration to the probe phase (which should be fine by the spec) > > poses > > > > > > any kind of problem. (and that's reason why asked it) > > > > > > > > > > > > Hope to have been clear despite the flood of words :D > > > > > > > > > > > > Thanks > > > > > > > > > > > > Cristian > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Jyoti > > > > > > > > > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi < > > cristian.marussi@arm.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jyoti > > > > > > > > > > > > > > > > some minor things down below. > > > > > > > > > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > Cristian > > > > > > > > > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana > > wrote: > > > > > > > > > This change provides ARM SCMI Protocol based IIO device. > > > > > > > > > This driver provides support for Accelerometer and Gyroscope > > using > > > > > > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM > > specification > > > > > > > > > > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > > > > > > --- > > > > > > > > > MAINTAINERS | 6 + > > > > > > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > > > > > > drivers/iio/common/Kconfig | 1 + > > > > > > > > > drivers/iio/common/Makefile | 1 + > > > > > > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > > > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 > > +++++++++++++++++++++ > > > > > > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > create mode 100644 > > drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > > > > > > --- a/MAINTAINERS > > > > > > > > > +++ b/MAINTAINERS > > > > > > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > > > > > > F: > > > > > > > > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > > > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > > > > > > > > > > > +IIO SCMI BASED DRIVER > > > > > > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > > > > > > +L: linux-iio@vger.kernel.org > > > > > > > > > +S: Maintained > > > > > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > + > > > > > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > > > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > > > > > > b/drivers/firmware/arm_scmi/driver.c > > > > > > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > > > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > > > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > > > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames > > devnames[] = { > > > > > > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > > > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > > > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > > > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > > > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > > > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > > > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > > > > > > }; > > > > > > > > > diff --git a/drivers/iio/common/Kconfig > > b/drivers/iio/common/Kconfig > > > > > > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > > > > > > --- a/drivers/iio/common/Kconfig > > > > > > > > > +++ b/drivers/iio/common/Kconfig > > > > > > > > > @@ -6,5 +6,6 @@ > > > > > > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > > > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > > > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > > > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > > > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > > > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > > > > > > diff --git a/drivers/iio/common/Makefile > > b/drivers/iio/common/Makefile > > > > > > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > > > > > > --- a/drivers/iio/common/Makefile > > > > > > > > > +++ b/drivers/iio/common/Makefile > > > > > > > > > @@ -11,5 +11,6 @@ > > > > > > > > > obj-y += cros_ec_sensors/ > > > > > > > > > obj-y += hid-sensors/ > > > > > > > > > obj-y += ms_sensors/ > > > > > > > > > +obj-y += scmi_sensors/ > > > > > > > > > obj-y += ssp_sensors/ > > > > > > > > > obj-y += st_sensors/ > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > new file mode 100644 > > > > > > > > > index 000000000000..67e084cbb1ab > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > @@ -0,0 +1,18 @@ > > > > > > > > > +# > > > > > > > > > +# IIO over SCMI > > > > > > > > > +# > > > > > > > > > +# When adding new entries keep the list in alphabetical > > order > > > > > > > > > + > > > > > > > > > +menu "IIO SCMI Sensors" > > > > > > > > > + > > > > > > > > > +config IIO_SCMI > > > > > > > > > + tristate "IIO SCMI" > > > > > > > > > + depends on ARM_SCMI_PROTOCOL > > > > > > > > > + select IIO_BUFFER > > > > > > > > > + select IIO_KFIFO_BUF > > > > > > > > > + help > > > > > > > > > + Say yes here to build support for IIO SCMI Driver. > > > > > > > > > + This provides ARM SCMI Protocol based IIO device. > > > > > > > > > + This driver provides support for accelerometer > > and gyroscope > > > > > > > > > + sensors available on SCMI based platforms. > > > > > > > > > +endmenu > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > new file mode 100644 > > > > > > > > > index 000000000000..f13140a2575a > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > @@ -0,0 +1,5 @@ > > > > > > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > > > > > > +# > > > > > > > > > +# Makefile for the IIO over SCMI > > > > > > > > > +# > > > > > > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > new file mode 100644 > > > > > > > > > index 000000000000..093b1fc24e27 > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > @@ -0,0 +1,673 @@ > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * System Control and Management Interface(SCMI) based IIO > > sensor driver > > > > > > > > > + * > > > > > > > > > + * Copyright (C) 2021 Google LLC > > > > > > > > > + */ > > > > > > > > > + > > > > > > > > > +#include <linux/delay.h> > > > > > > > > > +#include <linux/err.h> > > > > > > > > > +#include <linux/iio/buffer.h> > > > > > > > > > +#include <linux/iio/iio.h> > > > > > > > > > +#include <linux/iio/kfifo_buf.h> > > > > > > > > > +#include <linux/iio/sysfs.h> > > > > > > > > > +#include <linux/kernel.h> > > > > > > > > > +#include <linux/kthread.h> > > > > > > > > > +#include <linux/module.h> > > > > > > > > > +#include <linux/scmi_protocol.h> > > > > > > > > > +#include <linux/time.h> > > > > > > > > > +#include <linux/types.h> > > > > > > > > > + > > > > > > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > > > > + > > > > > > > > > +struct scmi_iio_priv { > > > > > > > > > + struct scmi_handle *handle; > > > > > > > > > + const struct scmi_sensor_info *sensor_info; > > > > > > > > > + struct iio_dev *indio_dev; > > > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > > > > > > + struct notifier_block sensor_update_nb; > > > > > > > > > + u32 *freq_avail; > > > > > > > > > +}; > > > > > > > > > + > > > > > > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block > > *nb, > > > > > > > > > + unsigned long event, void > > *data) > > > > > > > > > +{ > > > > > > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > > > > > > + struct iio_dev *scmi_iio_dev; > > > > > > > > > + struct scmi_iio_priv *sensor; > > > > > > > > > + s8 tstamp_scale; > > > > > > > > > + u64 time, time_ns; > > > > > > > > > + int i; > > > > > > > > > + > > > > > > > > > + if (sensor_update->readings_count == 0) > > > > > > > > > + return NOTIFY_DONE; > > > > > > > > > + > > > > > > > > > + sensor = container_of(nb, struct scmi_iio_priv, > > sensor_update_nb); > > > > > > > > > + > > > > > > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > > > > > > + sensor->iio_buf[i] = > > sensor_update->readings[i].value; > > > > > > > > > + > > > > > > > > > + if (!sensor->sensor_info->timestamped) { > > > > > > > > > + time_ns = > > ktime_to_ns(sensor_update->timestamp); > > > > > > > > > + } else { > > > > > > > > > + /* > > > > > > > > > + * All the axes are supposed to have the same > > value for > > > > > > > > timestamp. > > > > > > > > > + * We are just using the values from the Axis > > 0 here. > > > > > > > > > + */ > > > > > > > > > + time = sensor_update->readings[0].timestamp; > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * Timestamp returned by SCMI is in seconds > > and is equal > > > > > > > > to > > > > > > > > > + * time * power-of-10 > > multiplier(tstamp_scale) seconds. > > > > > > > > > + * Converting the timestamp to nanoseconds > > below. > > > > > > > > > + */ > > > > > > > > > + tstamp_scale = > > sensor->sensor_info->tstamp_scale + > > > > > > > > > + const_ilog2(NSEC_PER_SEC) / > > const_ilog2(10); > > > > > > > > > + if (tstamp_scale < 0) > > > > > > > > > + time_ns = > > > > > > > > > + div64_u64(time, int_pow(10, > > > > > > > > abs(tstamp_scale))); > > > > > > > > > + else > > > > > > > > > + time_ns = time * int_pow(10, > > tstamp_scale); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + scmi_iio_dev = sensor->indio_dev; > > > > > > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, > > sensor->iio_buf, > > > > > > > > > + time_ns); > > > > > > > > > + return NOTIFY_OK; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +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; > > > > > > > > > + > > > > > > > > > + if (sensor->sensor_info->timestamped) > > > > > > > > > + sensor_config |= > > > > > > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > > > > > > + > > SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > > > > > > + > > > > > > > > > + 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); > > > > > > > > > + dev_err(&iio_dev->dev, "Error in enabling > > sensor %s err > > > > > > > > %d", > > > > > > > > > + sensor->sensor_info->name, err); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + return err; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +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); > > > > > > > > > + if (err) { > > > > > > > > > + dev_err(&iio_dev->dev, > > > > > > > > > + "Error in disabling sensor %s with err > > %d", > > > > > > > > > + sensor->sensor_info->name, err); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + return err; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +static const struct iio_buffer_setup_ops > > scmi_iio_buffer_ops = { > > > > > > > > > + .preenable = scmi_iio_buffer_preenable, > > > > > > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > > > > > > +}; > > > > > > > > > > > > > > > > This is just a question, I'm not suggesting to change anything > > here at > > > > > > > > this point to be clear, since it works just fine as it is. > > > > > > > > > > > > > > > > Following up a previous email, given these are called on > > enable/disable > > > > > > > > by sysfs, is there a specific reason why you configure here, > > inside > > > > > > > > these ops, also timestamping and callbacks i.e. each time the > > sensor is > > > > > > > > turned on/off by sysfs ? ... instead of just, as an example, > > enabling > > > > > > > > in _preenable the sensor while registering callbacks and > > enabling > > > > > > > > timestamping once for all earlier during probe phase ? > > > > > > > > (likewise for _postdisable -> remove) > > > > > > > > > > > > > > > > AFAIU the spec says notifications are emitted for sensors > > which has > > > > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only > > if the > > > > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you > > do), so > > > > > > > > that enabling/disabling the sensor as a whole should result in > > starting/ > > > > > > > > stopping the notification flow without the need of > > unregistering the > > > > > > > > callbacks everytime. (same goes with the timestamping) > > > > > > > > > > > > > > > > In other words, I would expect the sensor to maintain its > > state (on the > > > > > > > > platform side) even when going through enable/disable cycles, > > so that > > > > > > > > it 'remembers' that timestamping/notifications were enabled > > across an > > > > > > > > on/off. > > > > > > > > > > > > > > > > This would reduce the number of SCMI messages exchanges > > between the > > > > > > > > kernel and the platform and should be supported by both, but > > as said, > > > > > > > > it's more of a question for the future, not necessarily for > > this series. > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev > > *iio_dev) > > > > > > > > > +{ > > > > > > > > > + u64 cur_interval_ns, low_interval_ns, > > high_interval_ns, > > > > > > > > step_size_ns, > > > > > > > > > + hz, uhz; > > > > > > > > > + unsigned int cur_interval, low_interval, > > high_interval, step_size; > > > > > > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > > > > > + int i; > > > > > > > > > + > > > > > > > > > + sensor->freq_avail = > > > > > > > > > + devm_kzalloc(&iio_dev->dev, > > > > > > > > > + sizeof(*sensor->freq_avail) * > > > > > > > > > + > > (sensor->sensor_info->intervals.count > > > > > > > > * 2), > > > > > > > > > + GFP_KERNEL); > > > > > > > > > + if (!sensor->freq_avail) > > > > > > > > > + return -ENOMEM; > > > > > > > > > + > > > > > > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > > > > > > + low_interval = sensor->sensor_info->intervals > > > > > > > > > + > > .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > > > > > > + low_interval_ns = > > > > > > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > > > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > > > > > > + sensor->freq_avail[0] = hz; > > > > > > > > > + sensor->freq_avail[1] = uhz; > > > > > > > > > + > > > > > > > > > + step_size = sensor->sensor_info->intervals > > > > > > > > > + > > .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > > > > > > + step_size_ns = > > scmi_iio_convert_interval_to_ns(step_size); > > > > > > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > > > > > > + sensor->freq_avail[2] = hz; > > > > > > > > > + sensor->freq_avail[3] = uhz; > > > > > > > > > + > > > > > > > > > + high_interval = sensor->sensor_info->intervals > > > > > > > > > + > > > > > > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > > > > > > + high_interval_ns = > > > > > > > > > + > > scmi_iio_convert_interval_to_ns(high_interval); > > > > > > > > > + convert_ns_to_freq(high_interval_ns, &hz, > > &uhz); > > > > > > > > > + sensor->freq_avail[4] = hz; > > > > > > > > > + sensor->freq_avail[5] = uhz; > > > > > > > > > + } else { > > > > > > > > > + for (i = 0; i < > > sensor->sensor_info->intervals.count; i++) > > > > > > > > { > > > > > > > > > + cur_interval = > > > > > > > > sensor->sensor_info->intervals.desc[i]; > > > > > > > > > + cur_interval_ns = > > > > > > > > > + > > > > > > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > > > > > > + convert_ns_to_freq(cur_interval_ns, > > &hz, &uhz); > > > > > > > > > + sensor->freq_avail[i * 2] = hz; > > > > > > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > > > > > > + } > > > > > > > > > + } > > > > > > > > > + return 0; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +static int scmi_iio_buffers_setup(struct iio_dev > > *scmi_iiodev) > > > > > > > > > +{ > > > > > > > > > + struct iio_buffer *buffer; > > > > > > > > > + > > > > > > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > > > > > > + if (!buffer) > > > > > > > > > + return -ENOMEM; > > > > > > > > > + > > > > > > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > > > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > > > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > > > > > > + return 0; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > > > > > + struct scmi_handle *handle, > > > > > > > > > + 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; > > > > > > > > > + int i, ret; > > > > > > > > > + > > > > > > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > > > > > + if (!iiodev) > > > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > > + > > > > > > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > > > > > > + iiodev->dev.parent = dev; > > > > > > > > > + sensor = iio_priv(iiodev); > > > > > > > > > + sensor->handle = handle; > > > > > > > > > + sensor->sensor_info = sensor_info; > > > > > > > > > + sensor->sensor_update_nb.notifier_call = > > scmi_iio_sensor_update_cb; > > > > > > > > > + sensor->indio_dev = iiodev; > > > > > > > > > + > > > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > > > > > > + iiodev->name = sensor_info->name; > > > > > > > > > + iiodev->info = &scmi_iio_info; > > > > > > > > > + > > > > > > > > > + iio_channels = > > > > > > > > > + devm_kzalloc(dev, > > > > > > > > > + sizeof(*iio_channels) * > > > > > > > > (iiodev->num_channels), > > > > > > > > > + GFP_KERNEL); > > > > > > > > > + if (!iio_channels) > > > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > > + > > > > > > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > > > > > > > > > > > You don't check this for retval, and it could fail with > > -ENOMEM. > > > > > > > > > > > > > > > > > + > > > > > > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > > > > > > + ret = > > scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > > > > > > &type); > > > > > > > > > + if (ret < 0) > > > > > > > > > + return ERR_PTR(ret); > > > > > > > > > + > > > > > > > > > + ret = > > scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > > > > > > + &modifier); > > > > > > > > > + if (ret < 0) > > > > > > > > > + return ERR_PTR(ret); > > > > > > > > > + > > > > > > > > > + scmi_iio_set_data_channel(&iio_channels[i], > > type, modifier, > > > > > > > > > + > > sensor_info->axis[i].id); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > > > > > + iiodev->channels = iio_channels; > > > > > > > > > + return iiodev; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > > > > > > > > > >
On Tue, 16 Feb 2021 10:20:59 +0000 Cristian Marussi <cristian.marussi@arm.com> wrote: > Hi Jyoti, > > On Mon, Feb 15, 2021 at 04:14:57PM -0800, Jyoti Bhayana wrote: > > Hi Jonathan/Cristian, > > > > I have uploaded v6 of the IIO SCMI patch. Not sure if you got a chance to > > review that version and if any further changes are needed. > > Seen that, for the SCMI part seemed fine to me, I'll just have still to > reply. I'll confess I'm not rushing reviewing this, simply because it's proved complex / unusual enough that I want to have another hard look at it + we have a bit of time now. I can't fall back on semi conscious reviewing for this one ;) Apologies for the delay in the meantime! Jonathan > > > My IIO SCMI patch is independent of Cristian series mentioned at > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > right? > > Yes, that's true, your patch is independent and goes in as it is now, and I'll > rebase my series on yours on a dedicated branch and make all the needed > (small) interface changes. > > > and it can be merged without the Cristian Series right? Are there plans to > > merge my v6 of the IIO SCMI patch and Cristian latest series in the same > > linux tree version? > > Yes absolutely, your driver can be merged as it is without my series, but due > to the timing they ended up targeting the same Linux version 5.13 at the > end, so they will be merged both in v5.13, yours first as it is now, mine > immediately rebased on top of it aftewards: the end result will be both > series in the same v5.13 but as distinct patches. (so you can still backport > yours on v5.4 as you're doing now.) > > Thanks > > Cristian > > > > > Thanks, > > Jyoti > > > > On Mon, Feb 15, 2021 at 6:48 AM Cristian Marussi <cristian.marussi@arm.com> > > wrote: > > > > > On Mon, Feb 15, 2021 at 11:07:56AM +0000, Jonathan Cameron wrote: > > > > On Mon, 15 Feb 2021 09:25:26 +0000 > > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > > > On Fri, Feb 12, 2021 at 07:18:58PM +0000, Jonathan Cameron wrote: > > > > > > On Wed, 10 Feb 2021 21:46:19 +0000 > > > > > > Cristian Marussi <cristian.marussi@arm.com> wrote: > > > > > > > > > > > > > Hi Jyoti, > > > > > > > > > > > > > > On Wed, Feb 10, 2021 at 11:19:35AM -0800, Jyoti Bhayana wrote: > > > > > > > > Hi Cristian, > > > > > > > > > > > > > > > > Thanks for the feedback. Regarding registering callbacks at the > > > probe time > > > > > > > > instead of .preenable, I have tried it before but I think due to > > > some > > > > > > > > issues(don't remember it now maybe on the platform side) I kept > > > it at the > > > > > > > > .preenable level. > > > > > > > > > > > > > > > > But you are right, that it will be nice to move it at the probe > > > level > > > > > > > > instead. I will try again and test if it works and would move it > > > at the > > > > > > > > probe level. Regarding the unregistering of the notifier, is it > > > required at > > > > > > > > the remove of iio driver or scmi driver will take care of it? > > > > > > > > Because if I add unregister at the iio driver remove level, I > > > would have to > > > > > > > > iterate all the sensors again and unregister them. > > > > > > > > > > > > > > Yes you are right if you move callbacks registration once for all > > > to the > > > > > > > .probe step you'll have to unregister them all in a .remove. > > > > > > > > > > > > > > BUT I think instead you should stick with your current solution > > > given > > > > > > > it's working fine anyway and it's supported by the notification > > > > > > > framework and also for another reason I'm going to explain down > > > below > > > > > > > (which is also the reason why I asked you this at first :D) > > > > > > > > > > > > > > As you may remember I'm refactoring all the SCMI internals in a > > > separate > > > > > > > series to ease modularization and vendor protocols support, and > > > that will > > > > > > > lead also to some changes in the SCMI driver interface that you > > > use: > > > > > > > amongst other things one interesting addition will be a new devres > > > managed > > > > > > > notification registration method, something like: > > > > > > > > > > > > > > handle->notify_ops->devm_register_notifier(sdev, ...); > > > > > > > > > > > > > > With such method you could just move your registration to the > > > .probe > > > > > > > step and just forget about it, without the need to add any > > > unregistration > > > > > > > in the .remove step, since the core will take care to remove all > > > the > > > > > > > callbacks at driver unloading time. > > > > > > > > > > > > > > Now, this series, which is here if you want to have a look: > > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.marussi@arm.com/ > > > > > > > > > > > > > > is already taking care to port any existent SCMI driver to the new > > > interface, > > > > > > > so when your IIODEV SCMI driver will be finally queued somewhere > > > for merge, I > > > > > > > can in turn rebase my series on yours and take care to port your > > > driver too to > > > > > > > the new interface applying the changes above in the context of my > > > series. > > > > > > > (and ask you to review of course :D) > > > > > > > > > > > > I'm guessing you probably want this driver in an immutable branch > > > then > > > > > > rather than having to wait another cycle for it to tick through to a > > > > > > a sensible upstream? > > > > > > > > > > > > > > > > Hi Jonathan > > > > > > > > > > the above series (still pending a final review from Sudeep) is targeted > > > > > at 5.13 at this point and usually it'd be queued via Sudeep > > > for-next/scmi > > > > > which in turn goes via the ARM soc branch. > > > > > > > > > > Having said that, I'm not really familiar enough with this sort of > > > clashes > > > > > to know how they should be properly handled, so I'll stick to what you > > > > > and Sudeep would think it's better :D (..and I'm pinging him to have a > > > say) > > > > > > > > > > Thanks > > > > > > > > > > Cristian > > > > Hi Cristian, > > > > > > > > So this driver will also be 5.13 material now (merge window for IIO > > > effectively > > > > closes 1-2 weeks before Linus opens the main one). > > > > > > > > The way we normally handle cases like this where we likely to have > > > dependencies > > > > on a patch set from two separate directions is to do what is known as an > > > > immutable branch. This is a branch that would probably be based on > > > 5.12-rc1 > > > > containing just this driver. > > > > > > > > Then both trees, in this case IIO and scmi merge that branch. The magic > > > > of git then means that when Linus gets the eventual pull requests for > > > > the two trees, the git IDs and content will be the same and the history > > > > of that particular set of files will be cleanly maintained. > > > > > > > > This happens quite a lot for certain parts of the kernel because there > > > are > > > > a lot of cross dependencies. > > > > > > > Hi Jonathan > > > > > > thanks for clarifying. > > > > > > Cristian > > > > > > > @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give > > > > us a sensible base. > > > > > > > > Jonathan > > > > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > > > > I'm saying that is better if you keep your series as it is for now > > > > > > > (old interface + .preenable/.postdisable regs/unregs) because, as > > > said, > > > > > > > with the new interface the devm_ methods will ease the registration > > > > > > > @probe time, and also especially because the new interface is not > > > (and > > > > > > > most probably won't) be part of the v5.4 backport that you are > > > testing > > > > > > > against: so if you stick with your current solution you'll have a > > > > > > > working patch easily backportable now, and once queued I'll port > > > it to > > > > > > > the interface using devm_ (so simplifying it) > > > > > > > > > > > > > > In this context, it would be indeed important to know if in > > > general moving > > > > > > > registration to the probe phase (which should be fine by the spec) > > > poses > > > > > > > any kind of problem. (and that's reason why asked it) > > > > > > > > > > > > > > Hope to have been clear despite the flood of words :D > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Cristian > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Jyoti > > > > > > > > > > > > > > > > On Tue, Feb 9, 2021 at 3:56 AM Cristian Marussi < > > > cristian.marussi@arm.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Jyoti > > > > > > > > > > > > > > > > > > some minor things down below. > > > > > > > > > > > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > > > > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > Cristian > > > > > > > > > > > > > > > > > > On Mon, Feb 08, 2021 at 09:19:18PM +0000, Jyoti Bhayana > > > wrote: > > > > > > > > > > This change provides ARM SCMI Protocol based IIO device. > > > > > > > > > > This driver provides support for Accelerometer and Gyroscope > > > using > > > > > > > > > > SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM > > > specification > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jyoti Bhayana <jbhayana@google.com> > > > > > > > > > > --- > > > > > > > > > > MAINTAINERS | 6 + > > > > > > > > > > drivers/firmware/arm_scmi/driver.c | 2 +- > > > > > > > > > > drivers/iio/common/Kconfig | 1 + > > > > > > > > > > drivers/iio/common/Makefile | 1 + > > > > > > > > > > drivers/iio/common/scmi_sensors/Kconfig | 18 + > > > > > > > > > > drivers/iio/common/scmi_sensors/Makefile | 5 + > > > > > > > > > > drivers/iio/common/scmi_sensors/scmi_iio.c | 673 > > > +++++++++++++++++++++ > > > > > > > > > > 7 files changed, 705 insertions(+), 1 deletion(-) > > > > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > > create mode 100644 drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > > create mode 100644 > > > drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > > > > > > index b516bb34a8d5..ccf37d43ab41 100644 > > > > > > > > > > --- a/MAINTAINERS > > > > > > > > > > +++ b/MAINTAINERS > > > > > > > > > > @@ -8567,6 +8567,12 @@ S: Maintained > > > > > > > > > > F: > > > > > > > > > > > > Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt > > > > > > > > > > F: drivers/iio/multiplexer/iio-mux.c > > > > > > > > > > > > > > > > > > > > +IIO SCMI BASED DRIVER > > > > > > > > > > +M: Jyoti Bhayana <jbhayana@google.com> > > > > > > > > > > +L: linux-iio@vger.kernel.org > > > > > > > > > > +S: Maintained > > > > > > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > + > > > > > > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > > > > > > M: Jonathan Cameron <jic23@kernel.org> > > > > > > > > > > R: Lars-Peter Clausen <lars@metafoo.de> > > > > > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > > > > > > > b/drivers/firmware/arm_scmi/driver.c > > > > > > > > > > index 5392e1fc6b4e..248313bbd473 100644 > > > > > > > > > > --- a/drivers/firmware/arm_scmi/driver.c > > > > > > > > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > > > > > > > > @@ -741,7 +741,7 @@ static struct scmi_prot_devnames > > > devnames[] = { > > > > > > > > > > { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, > > > > > > > > > > { SCMI_PROTOCOL_PERF, { "cpufreq" },}, > > > > > > > > > > { SCMI_PROTOCOL_CLOCK, { "clocks" },}, > > > > > > > > > > - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, > > > > > > > > > > + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, > > > > > > > > > > { SCMI_PROTOCOL_RESET, { "reset" },}, > > > > > > > > > > { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, > > > > > > > > > > }; > > > > > > > > > > diff --git a/drivers/iio/common/Kconfig > > > b/drivers/iio/common/Kconfig > > > > > > > > > > index 2b9ee9161abd..0334b4954773 100644 > > > > > > > > > > --- a/drivers/iio/common/Kconfig > > > > > > > > > > +++ b/drivers/iio/common/Kconfig > > > > > > > > > > @@ -6,5 +6,6 @@ > > > > > > > > > > source "drivers/iio/common/cros_ec_sensors/Kconfig" > > > > > > > > > > source "drivers/iio/common/hid-sensors/Kconfig" > > > > > > > > > > source "drivers/iio/common/ms_sensors/Kconfig" > > > > > > > > > > +source "drivers/iio/common/scmi_sensors/Kconfig" > > > > > > > > > > source "drivers/iio/common/ssp_sensors/Kconfig" > > > > > > > > > > source "drivers/iio/common/st_sensors/Kconfig" > > > > > > > > > > diff --git a/drivers/iio/common/Makefile > > > b/drivers/iio/common/Makefile > > > > > > > > > > index 4bc30bb548e2..fad40e1e1718 100644 > > > > > > > > > > --- a/drivers/iio/common/Makefile > > > > > > > > > > +++ b/drivers/iio/common/Makefile > > > > > > > > > > @@ -11,5 +11,6 @@ > > > > > > > > > > obj-y += cros_ec_sensors/ > > > > > > > > > > obj-y += hid-sensors/ > > > > > > > > > > obj-y += ms_sensors/ > > > > > > > > > > +obj-y += scmi_sensors/ > > > > > > > > > > obj-y += ssp_sensors/ > > > > > > > > > > obj-y += st_sensors/ > > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > > new file mode 100644 > > > > > > > > > > index 000000000000..67e084cbb1ab > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Kconfig > > > > > > > > > > @@ -0,0 +1,18 @@ > > > > > > > > > > +# > > > > > > > > > > +# IIO over SCMI > > > > > > > > > > +# > > > > > > > > > > +# When adding new entries keep the list in alphabetical > > > order > > > > > > > > > > + > > > > > > > > > > +menu "IIO SCMI Sensors" > > > > > > > > > > + > > > > > > > > > > +config IIO_SCMI > > > > > > > > > > + tristate "IIO SCMI" > > > > > > > > > > + depends on ARM_SCMI_PROTOCOL > > > > > > > > > > + select IIO_BUFFER > > > > > > > > > > + select IIO_KFIFO_BUF > > > > > > > > > > + help > > > > > > > > > > + Say yes here to build support for IIO SCMI Driver. > > > > > > > > > > + This provides ARM SCMI Protocol based IIO device. > > > > > > > > > > + This driver provides support for accelerometer > > > and gyroscope > > > > > > > > > > + sensors available on SCMI based platforms. > > > > > > > > > > +endmenu > > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > > new file mode 100644 > > > > > > > > > > index 000000000000..f13140a2575a > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/Makefile > > > > > > > > > > @@ -0,0 +1,5 @@ > > > > > > > > > > +# SPDX - License - Identifier : GPL - 2.0 - only > > > > > > > > > > +# > > > > > > > > > > +# Makefile for the IIO over SCMI > > > > > > > > > > +# > > > > > > > > > > +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o > > > > > > > > > > diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > new file mode 100644 > > > > > > > > > > index 000000000000..093b1fc24e27 > > > > > > > > > > --- /dev/null > > > > > > > > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > @@ -0,0 +1,673 @@ > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > > > > > + > > > > > > > > > > +/* > > > > > > > > > > + * System Control and Management Interface(SCMI) based IIO > > > sensor driver > > > > > > > > > > + * > > > > > > > > > > + * Copyright (C) 2021 Google LLC > > > > > > > > > > + */ > > > > > > > > > > + > > > > > > > > > > +#include <linux/delay.h> > > > > > > > > > > +#include <linux/err.h> > > > > > > > > > > +#include <linux/iio/buffer.h> > > > > > > > > > > +#include <linux/iio/iio.h> > > > > > > > > > > +#include <linux/iio/kfifo_buf.h> > > > > > > > > > > +#include <linux/iio/sysfs.h> > > > > > > > > > > +#include <linux/kernel.h> > > > > > > > > > > +#include <linux/kthread.h> > > > > > > > > > > +#include <linux/module.h> > > > > > > > > > > +#include <linux/scmi_protocol.h> > > > > > > > > > > +#include <linux/time.h> > > > > > > > > > > +#include <linux/types.h> > > > > > > > > > > + > > > > > > > > > > +#define SCMI_IIO_NUM_OF_AXIS 3 > > > > > > > > > > + > > > > > > > > > > +struct scmi_iio_priv { > > > > > > > > > > + struct scmi_handle *handle; > > > > > > > > > > + const struct scmi_sensor_info *sensor_info; > > > > > > > > > > + struct iio_dev *indio_dev; > > > > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > > > > + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; > > > > > > > > > > + struct notifier_block sensor_update_nb; > > > > > > > > > > + u32 *freq_avail; > > > > > > > > > > +}; > > > > > > > > > > + > > > > > > > > > > +static int scmi_iio_sensor_update_cb(struct notifier_block > > > *nb, > > > > > > > > > > + unsigned long event, void > > > *data) > > > > > > > > > > +{ > > > > > > > > > > + struct scmi_sensor_update_report *sensor_update = data; > > > > > > > > > > + struct iio_dev *scmi_iio_dev; > > > > > > > > > > + struct scmi_iio_priv *sensor; > > > > > > > > > > + s8 tstamp_scale; > > > > > > > > > > + u64 time, time_ns; > > > > > > > > > > + int i; > > > > > > > > > > + > > > > > > > > > > + if (sensor_update->readings_count == 0) > > > > > > > > > > + return NOTIFY_DONE; > > > > > > > > > > + > > > > > > > > > > + sensor = container_of(nb, struct scmi_iio_priv, > > > sensor_update_nb); > > > > > > > > > > + > > > > > > > > > > + for (i = 0; i < sensor_update->readings_count; i++) > > > > > > > > > > + sensor->iio_buf[i] = > > > sensor_update->readings[i].value; > > > > > > > > > > + > > > > > > > > > > + if (!sensor->sensor_info->timestamped) { > > > > > > > > > > + time_ns = > > > ktime_to_ns(sensor_update->timestamp); > > > > > > > > > > + } else { > > > > > > > > > > + /* > > > > > > > > > > + * All the axes are supposed to have the same > > > value for > > > > > > > > > timestamp. > > > > > > > > > > + * We are just using the values from the Axis > > > 0 here. > > > > > > > > > > + */ > > > > > > > > > > + time = sensor_update->readings[0].timestamp; > > > > > > > > > > + > > > > > > > > > > + /* > > > > > > > > > > + * Timestamp returned by SCMI is in seconds > > > and is equal > > > > > > > > > to > > > > > > > > > > + * time * power-of-10 > > > multiplier(tstamp_scale) seconds. > > > > > > > > > > + * Converting the timestamp to nanoseconds > > > below. > > > > > > > > > > + */ > > > > > > > > > > + tstamp_scale = > > > sensor->sensor_info->tstamp_scale + > > > > > > > > > > + const_ilog2(NSEC_PER_SEC) / > > > const_ilog2(10); > > > > > > > > > > + if (tstamp_scale < 0) > > > > > > > > > > + time_ns = > > > > > > > > > > + div64_u64(time, int_pow(10, > > > > > > > > > abs(tstamp_scale))); > > > > > > > > > > + else > > > > > > > > > > + time_ns = time * int_pow(10, > > > tstamp_scale); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + scmi_iio_dev = sensor->indio_dev; > > > > > > > > > > + iio_push_to_buffers_with_timestamp(scmi_iio_dev, > > > sensor->iio_buf, > > > > > > > > > > + time_ns); > > > > > > > > > > + return NOTIFY_OK; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +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; > > > > > > > > > > + > > > > > > > > > > + if (sensor->sensor_info->timestamped) > > > > > > > > > > + sensor_config |= > > > > > > > > > FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > > > > > > > > + > > > SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > > > > > > > + > > > > > > > > > > + 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); > > > > > > > > > > + dev_err(&iio_dev->dev, "Error in enabling > > > sensor %s err > > > > > > > > > %d", > > > > > > > > > > + sensor->sensor_info->name, err); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + return err; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +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); > > > > > > > > > > + if (err) { > > > > > > > > > > + dev_err(&iio_dev->dev, > > > > > > > > > > + "Error in disabling sensor %s with err > > > %d", > > > > > > > > > > + sensor->sensor_info->name, err); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + return err; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +static const struct iio_buffer_setup_ops > > > scmi_iio_buffer_ops = { > > > > > > > > > > + .preenable = scmi_iio_buffer_preenable, > > > > > > > > > > + .postdisable = scmi_iio_buffer_postdisable, > > > > > > > > > > +}; > > > > > > > > > > > > > > > > > > This is just a question, I'm not suggesting to change anything > > > here at > > > > > > > > > this point to be clear, since it works just fine as it is. > > > > > > > > > > > > > > > > > > Following up a previous email, given these are called on > > > enable/disable > > > > > > > > > by sysfs, is there a specific reason why you configure here, > > > inside > > > > > > > > > these ops, also timestamping and callbacks i.e. each time the > > > sensor is > > > > > > > > > turned on/off by sysfs ? ... instead of just, as an example, > > > enabling > > > > > > > > > in _preenable the sensor while registering callbacks and > > > enabling > > > > > > > > > timestamping once for all earlier during probe phase ? > > > > > > > > > (likewise for _postdisable -> remove) > > > > > > > > > > > > > > > > > > AFAIU the spec says notifications are emitted for sensors > > > which has > > > > > > > > > requested them (via SENSOR_CONTINUOUS_UPDATE_NOTIFY) BUT only > > > if the > > > > > > > > > sensor is enabled as a whole (via proper CONFIG_SET as you > > > do), so > > > > > > > > > that enabling/disabling the sensor as a whole should result in > > > starting/ > > > > > > > > > stopping the notification flow without the need of > > > unregistering the > > > > > > > > > callbacks everytime. (same goes with the timestamping) > > > > > > > > > > > > > > > > > > In other words, I would expect the sensor to maintain its > > > state (on the > > > > > > > > > platform side) even when going through enable/disable cycles, > > > so that > > > > > > > > > it 'remembers' that timestamping/notifications were enabled > > > across an > > > > > > > > > on/off. > > > > > > > > > > > > > > > > > > This would reduce the number of SCMI messages exchanges > > > between the > > > > > > > > > kernel and the platform and should be supported by both, but > > > as said, > > > > > > > > > it's more of a question for the future, not necessarily for > > > this series. > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > +static int scmi_iio_set_sampling_freq_avail(struct iio_dev > > > *iio_dev) > > > > > > > > > > +{ > > > > > > > > > > + u64 cur_interval_ns, low_interval_ns, > > > high_interval_ns, > > > > > > > > > step_size_ns, > > > > > > > > > > + hz, uhz; > > > > > > > > > > + unsigned int cur_interval, low_interval, > > > high_interval, step_size; > > > > > > > > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > > > > > > > > + int i; > > > > > > > > > > + > > > > > > > > > > + sensor->freq_avail = > > > > > > > > > > + devm_kzalloc(&iio_dev->dev, > > > > > > > > > > + sizeof(*sensor->freq_avail) * > > > > > > > > > > + > > > (sensor->sensor_info->intervals.count > > > > > > > > > * 2), > > > > > > > > > > + GFP_KERNEL); > > > > > > > > > > + if (!sensor->freq_avail) > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > + > > > > > > > > > > + if (sensor->sensor_info->intervals.segmented) { > > > > > > > > > > + low_interval = sensor->sensor_info->intervals > > > > > > > > > > + > > > .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; > > > > > > > > > > + low_interval_ns = > > > > > > > > > scmi_iio_convert_interval_to_ns(low_interval); > > > > > > > > > > + convert_ns_to_freq(low_interval_ns, &hz, &uhz); > > > > > > > > > > + sensor->freq_avail[0] = hz; > > > > > > > > > > + sensor->freq_avail[1] = uhz; > > > > > > > > > > + > > > > > > > > > > + step_size = sensor->sensor_info->intervals > > > > > > > > > > + > > > .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; > > > > > > > > > > + step_size_ns = > > > scmi_iio_convert_interval_to_ns(step_size); > > > > > > > > > > + convert_ns_to_freq(step_size_ns, &hz, &uhz); > > > > > > > > > > + sensor->freq_avail[2] = hz; > > > > > > > > > > + sensor->freq_avail[3] = uhz; > > > > > > > > > > + > > > > > > > > > > + high_interval = sensor->sensor_info->intervals > > > > > > > > > > + > > > > > > > > > .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; > > > > > > > > > > + high_interval_ns = > > > > > > > > > > + > > > scmi_iio_convert_interval_to_ns(high_interval); > > > > > > > > > > + convert_ns_to_freq(high_interval_ns, &hz, > > > &uhz); > > > > > > > > > > + sensor->freq_avail[4] = hz; > > > > > > > > > > + sensor->freq_avail[5] = uhz; > > > > > > > > > > + } else { > > > > > > > > > > + for (i = 0; i < > > > sensor->sensor_info->intervals.count; i++) > > > > > > > > > { > > > > > > > > > > + cur_interval = > > > > > > > > > sensor->sensor_info->intervals.desc[i]; > > > > > > > > > > + cur_interval_ns = > > > > > > > > > > + > > > > > > > > > scmi_iio_convert_interval_to_ns(cur_interval); > > > > > > > > > > + convert_ns_to_freq(cur_interval_ns, > > > &hz, &uhz); > > > > > > > > > > + sensor->freq_avail[i * 2] = hz; > > > > > > > > > > + sensor->freq_avail[i * 2 + 1] = uhz; > > > > > > > > > > + } > > > > > > > > > > + } > > > > > > > > > > + return 0; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +static int scmi_iio_buffers_setup(struct iio_dev > > > *scmi_iiodev) > > > > > > > > > > +{ > > > > > > > > > > + struct iio_buffer *buffer; > > > > > > > > > > + > > > > > > > > > > + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); > > > > > > > > > > + if (!buffer) > > > > > > > > > > + return -ENOMEM; > > > > > > > > > > + > > > > > > > > > > + iio_device_attach_buffer(scmi_iiodev, buffer); > > > > > > > > > > + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; > > > > > > > > > > + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; > > > > > > > > > > + return 0; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > +struct iio_dev *scmi_alloc_iiodev(struct device *dev, > > > > > > > > > > + struct scmi_handle *handle, > > > > > > > > > > + 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; > > > > > > > > > > + int i, ret; > > > > > > > > > > + > > > > > > > > > > + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); > > > > > > > > > > + if (!iiodev) > > > > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > > > + > > > > > > > > > > + iiodev->modes = INDIO_DIRECT_MODE; > > > > > > > > > > + iiodev->dev.parent = dev; > > > > > > > > > > + sensor = iio_priv(iiodev); > > > > > > > > > > + sensor->handle = handle; > > > > > > > > > > + sensor->sensor_info = sensor_info; > > > > > > > > > > + sensor->sensor_update_nb.notifier_call = > > > scmi_iio_sensor_update_cb; > > > > > > > > > > + sensor->indio_dev = iiodev; > > > > > > > > > > + > > > > > > > > > > + /* adding one additional channel for timestamp */ > > > > > > > > > > + iiodev->num_channels = sensor_info->num_axis + 1; > > > > > > > > > > + iiodev->name = sensor_info->name; > > > > > > > > > > + iiodev->info = &scmi_iio_info; > > > > > > > > > > + > > > > > > > > > > + iio_channels = > > > > > > > > > > + devm_kzalloc(dev, > > > > > > > > > > + sizeof(*iio_channels) * > > > > > > > > > (iiodev->num_channels), > > > > > > > > > > + GFP_KERNEL); > > > > > > > > > > + if (!iio_channels) > > > > > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > > > + > > > > > > > > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > > > > > > > > > > > > > > > > You don't check this for retval, and it could fail with > > > -ENOMEM. > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > + for (i = 0; i < sensor_info->num_axis; i++) { > > > > > > > > > > + ret = > > > scmi_iio_get_chan_type(sensor_info->axis[i].type, > > > > > > > > > &type); > > > > > > > > > > + if (ret < 0) > > > > > > > > > > + return ERR_PTR(ret); > > > > > > > > > > + > > > > > > > > > > + ret = > > > scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > > > > > > > > + &modifier); > > > > > > > > > > + if (ret < 0) > > > > > > > > > > + return ERR_PTR(ret); > > > > > > > > > > + > > > > > > > > > > + scmi_iio_set_data_channel(&iio_channels[i], > > > type, modifier, > > > > > > > > > > + > > > sensor_info->axis[i].id); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + scmi_iio_set_timestamp_channel(&iio_channels[i], i); > > > > > > > > > > + iiodev->channels = iio_channels; > > > > > > > > > > + return iiodev; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > >
diff --git a/MAINTAINERS b/MAINTAINERS index b516bb34a8d5..ccf37d43ab41 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8567,6 +8567,12 @@ S: Maintained F: Documentation/devicetree/bindings/iio/multiplexer/io-channel-mux.txt F: drivers/iio/multiplexer/iio-mux.c +IIO SCMI BASED DRIVER +M: Jyoti Bhayana <jbhayana@google.com> +L: linux-iio@vger.kernel.org +S: Maintained +F: drivers/iio/common/scmi_sensors/scmi_iio.c + IIO SUBSYSTEM AND DRIVERS M: Jonathan Cameron <jic23@kernel.org> R: Lars-Peter Clausen <lars@metafoo.de> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 5392e1fc6b4e..248313bbd473 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -741,7 +741,7 @@ static struct scmi_prot_devnames devnames[] = { { SCMI_PROTOCOL_SYSTEM, { "syspower" },}, { SCMI_PROTOCOL_PERF, { "cpufreq" },}, { SCMI_PROTOCOL_CLOCK, { "clocks" },}, - { SCMI_PROTOCOL_SENSOR, { "hwmon" },}, + { SCMI_PROTOCOL_SENSOR, { "hwmon", "iiodev" },}, { SCMI_PROTOCOL_RESET, { "reset" },}, { SCMI_PROTOCOL_VOLTAGE, { "regulator" },}, }; diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig index 2b9ee9161abd..0334b4954773 100644 --- a/drivers/iio/common/Kconfig +++ b/drivers/iio/common/Kconfig @@ -6,5 +6,6 @@ source "drivers/iio/common/cros_ec_sensors/Kconfig" source "drivers/iio/common/hid-sensors/Kconfig" source "drivers/iio/common/ms_sensors/Kconfig" +source "drivers/iio/common/scmi_sensors/Kconfig" source "drivers/iio/common/ssp_sensors/Kconfig" source "drivers/iio/common/st_sensors/Kconfig" diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile index 4bc30bb548e2..fad40e1e1718 100644 --- a/drivers/iio/common/Makefile +++ b/drivers/iio/common/Makefile @@ -11,5 +11,6 @@ obj-y += cros_ec_sensors/ obj-y += hid-sensors/ obj-y += ms_sensors/ +obj-y += scmi_sensors/ obj-y += ssp_sensors/ obj-y += st_sensors/ diff --git a/drivers/iio/common/scmi_sensors/Kconfig b/drivers/iio/common/scmi_sensors/Kconfig new file mode 100644 index 000000000000..67e084cbb1ab --- /dev/null +++ b/drivers/iio/common/scmi_sensors/Kconfig @@ -0,0 +1,18 @@ +# +# IIO over SCMI +# +# When adding new entries keep the list in alphabetical order + +menu "IIO SCMI Sensors" + +config IIO_SCMI + tristate "IIO SCMI" + depends on ARM_SCMI_PROTOCOL + select IIO_BUFFER + select IIO_KFIFO_BUF + help + Say yes here to build support for IIO SCMI Driver. + This provides ARM SCMI Protocol based IIO device. + This driver provides support for accelerometer and gyroscope + sensors available on SCMI based platforms. +endmenu diff --git a/drivers/iio/common/scmi_sensors/Makefile b/drivers/iio/common/scmi_sensors/Makefile new file mode 100644 index 000000000000..f13140a2575a --- /dev/null +++ b/drivers/iio/common/scmi_sensors/Makefile @@ -0,0 +1,5 @@ +# SPDX - License - Identifier : GPL - 2.0 - only +# +# Makefile for the IIO over SCMI +# +obj-$(CONFIG_IIO_SCMI) += scmi_iio.o diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c new file mode 100644 index 000000000000..093b1fc24e27 --- /dev/null +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c @@ -0,0 +1,673 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * System Control and Management Interface(SCMI) based IIO sensor driver + * + * Copyright (C) 2021 Google LLC + */ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/iio/buffer.h> +#include <linux/iio/iio.h> +#include <linux/iio/kfifo_buf.h> +#include <linux/iio/sysfs.h> +#include <linux/kernel.h> +#include <linux/kthread.h> +#include <linux/module.h> +#include <linux/scmi_protocol.h> +#include <linux/time.h> +#include <linux/types.h> + +#define SCMI_IIO_NUM_OF_AXIS 3 + +struct scmi_iio_priv { + struct scmi_handle *handle; + const struct scmi_sensor_info *sensor_info; + struct iio_dev *indio_dev; + /* adding one additional channel for timestamp */ + long long iio_buf[SCMI_IIO_NUM_OF_AXIS + 1]; + struct notifier_block sensor_update_nb; + u32 *freq_avail; +}; + +static int scmi_iio_sensor_update_cb(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct scmi_sensor_update_report *sensor_update = data; + struct iio_dev *scmi_iio_dev; + struct scmi_iio_priv *sensor; + s8 tstamp_scale; + u64 time, time_ns; + int i; + + if (sensor_update->readings_count == 0) + return NOTIFY_DONE; + + sensor = container_of(nb, struct scmi_iio_priv, sensor_update_nb); + + for (i = 0; i < sensor_update->readings_count; i++) + sensor->iio_buf[i] = sensor_update->readings[i].value; + + if (!sensor->sensor_info->timestamped) { + time_ns = ktime_to_ns(sensor_update->timestamp); + } else { + /* + * All the axes are supposed to have the same value for timestamp. + * We are just using the values from the Axis 0 here. + */ + time = sensor_update->readings[0].timestamp; + + /* + * Timestamp returned by SCMI is in seconds and is equal to + * time * power-of-10 multiplier(tstamp_scale) seconds. + * Converting the timestamp to nanoseconds below. + */ + tstamp_scale = sensor->sensor_info->tstamp_scale + + const_ilog2(NSEC_PER_SEC) / const_ilog2(10); + if (tstamp_scale < 0) + time_ns = + div64_u64(time, int_pow(10, abs(tstamp_scale))); + else + time_ns = time * int_pow(10, tstamp_scale); + } + + scmi_iio_dev = sensor->indio_dev; + iio_push_to_buffers_with_timestamp(scmi_iio_dev, sensor->iio_buf, + time_ns); + return NOTIFY_OK; +} + +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; + + if (sensor->sensor_info->timestamped) + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, + SCMI_SENS_CFG_TSTAMP_ENABLE); + + 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); + dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", + sensor->sensor_info->name, err); + } + + return err; +} + +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); + if (err) { + dev_err(&iio_dev->dev, + "Error in disabling sensor %s with err %d", + sensor->sensor_info->name, err); + } + + return err; +} + +static const struct iio_buffer_setup_ops scmi_iio_buffer_ops = { + .preenable = scmi_iio_buffer_preenable, + .postdisable = scmi_iio_buffer_postdisable, +}; + +static int scmi_iio_set_odr_val(struct iio_dev *iio_dev, int val, int val2) +{ + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + const unsigned long UHZ_PER_HZ = 1000000UL; + u64 sec, mult, uHz; + u32 sensor_config; + + int err = sensor->handle->sensor_ops->config_get(sensor->handle, + sensor->sensor_info->id, &sensor_config); + if (err) { + dev_err(&iio_dev->dev, + "Error in getting sensor config for sensor %s err %d", + sensor->sensor_info->name, err); + return err; + } + + uHz = val * UHZ_PER_HZ + val2; + + /* + * The seconds field in the sensor interval in SCMI is 16 bits long + * Therefore seconds = 1/Hz <= 0xFFFF. As floating point calculations are + * discouraged in the kernel driver code, to calculate the scale factor (sf) + * (1* 1000000 * sf)/uHz <= 0xFFFF. Therefore, sf <= (uHz * 0xFFFF)/1000000 + */ + mult = ilog2(((u64)uHz * 0xFFFF) / UHZ_PER_HZ) / const_ilog2(10); + + sec = div64_u64(int_pow(10, mult) * UHZ_PER_HZ, uHz); + if (sec == 0) { + dev_err(&iio_dev->dev, + "Trying to set invalid sensor update value for sensor %s", + sensor->sensor_info->name); + return -EINVAL; + } + + sensor_config &= ~SCMI_SENS_CFG_UPDATE_SECS_MASK; + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_SECS_MASK, sec); + sensor_config &= ~SCMI_SENS_CFG_UPDATE_EXP_MASK; + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_UPDATE_EXP_MASK, -mult); + + if (sensor->sensor_info->timestamped) { + sensor_config &= ~SCMI_SENS_CFG_TSTAMP_ENABLED_MASK; + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, + SCMI_SENS_CFG_TSTAMP_ENABLE); + } + + sensor_config &= ~SCMI_SENS_CFG_ROUND_MASK; + 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); + if (err) + dev_err(&iio_dev->dev, + "Error in setting sensor update interval for sensor %s value %u err %d", + sensor->sensor_info->name, sensor_config, err); + + return err; +} + +static int scmi_iio_write_raw(struct iio_dev *iio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + int err; + + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + mutex_lock(&iio_dev->mlock); + err = scmi_iio_set_odr_val(iio_dev, val, val2); + mutex_unlock(&iio_dev->mlock); + return err; + default: + return -EINVAL; + } +} + +static int scmi_iio_read_avail(struct iio_dev *iio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + *vals = sensor->freq_avail; + *type = IIO_VAL_INT_PLUS_MICRO; + *length = sensor->sensor_info->intervals.count * 2; + if (sensor->sensor_info->intervals.segmented) + return IIO_AVAIL_RANGE; + else + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + +static void convert_ns_to_freq(u64 interval_ns, u64 *hz, u64 *uhz) +{ + u64 rem; + + *hz = div64_u64_rem(NSEC_PER_SEC, interval_ns, &rem); + *uhz = (rem * 1000000UL) / interval_ns; +} + +static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2) +{ + u64 sensor_update_interval, sensor_interval_mult, hz, uhz; + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + u32 sensor_config; + int mult; + + int err = sensor->handle->sensor_ops->config_get(sensor->handle, + sensor->sensor_info->id, &sensor_config); + if (err) { + dev_err(&iio_dev->dev, + "Error in getting sensor config for sensor %s err %d", + sensor->sensor_info->name, err); + return err; + } + + sensor_update_interval = + SCMI_SENS_CFG_GET_UPDATE_SECS(sensor_config) * NSEC_PER_SEC; + + mult = SCMI_SENS_CFG_GET_UPDATE_EXP(sensor_config); + if (mult < 0) { + sensor_interval_mult = int_pow(10, abs(mult)); + sensor_update_interval = + sensor_update_interval / sensor_interval_mult; + } else { + sensor_interval_mult = int_pow(10, mult); + sensor_update_interval = + sensor_update_interval * sensor_interval_mult; + } + + convert_ns_to_freq(sensor_update_interval, &hz, &uhz); + *val = hz; + *val2 = uhz; + return 0; +} + +static int scmi_iio_read_raw(struct iio_dev *iio_dev, + struct iio_chan_spec const *ch, int *val, + int *val2, long mask) +{ + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + s8 scale; + int ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + scale = sensor->sensor_info->axis[ch->scan_index].scale; + if (scale < 0) { + *val = 1; + *val2 = int_pow(10, abs(scale)); + return IIO_VAL_FRACTIONAL; + } + *val = int_pow(10, scale); + return IIO_VAL_INT; + case IIO_CHAN_INFO_SAMP_FREQ: + ret = scmi_iio_get_odr_val(iio_dev, val, val2); + return ret ? ret : IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static const struct iio_info scmi_iio_info = { + .read_raw = scmi_iio_read_raw, + .read_avail = scmi_iio_read_avail, + .write_raw = scmi_iio_write_raw, +}; + +static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + unsigned long long resolution, rem; + long long min_range, max_range; + s8 exponent, scale; + int len = 0; + + /* + * All the axes are supposed to have the same value for range and resolution. + * We are just using the values from the Axis 0 here. + */ + if (sensor->sensor_info->axis[0].extended_attrs) { + min_range = sensor->sensor_info->axis[0].attrs.min_range; + max_range = sensor->sensor_info->axis[0].attrs.max_range; + resolution = sensor->sensor_info->axis[0].resolution; + exponent = sensor->sensor_info->axis[0].exponent; + scale = sensor->sensor_info->axis[0].scale; + + /* + * To provide the raw value for the resolution to the userspace, + * need to divide the resolution exponent by the sensor scale + */ + exponent = exponent - scale; + if (exponent < 0) { + resolution = div64_u64_rem(resolution, + int_pow(10, abs(exponent)), + &rem); + len = scnprintf(buf, PAGE_SIZE, + "[%lld %llu.%llu %lld]\n", min_range, + resolution, rem, max_range); + } else { + resolution = resolution * int_pow(10, exponent); + len = scnprintf(buf, PAGE_SIZE, "[%lld %llu %lld]\n", + min_range, resolution, max_range); + } + } + return len; +} + +static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = { + { + .name = "raw_available", + .read = scmi_iio_get_raw_available, + .shared = IIO_SHARED_BY_TYPE, + }, + {}, +}; + +static void scmi_iio_set_timestamp_channel(struct iio_chan_spec *iio_chan, + int scan_index) +{ + iio_chan->type = IIO_TIMESTAMP; + iio_chan->channel = -1; + iio_chan->scan_index = scan_index; + iio_chan->scan_type.sign = 'u'; + iio_chan->scan_type.realbits = 64; + iio_chan->scan_type.storagebits = 64; +} + +static void scmi_iio_set_data_channel(struct iio_chan_spec *iio_chan, + enum iio_chan_type type, + enum iio_modifier mod, int scan_index) +{ + iio_chan->type = type; + iio_chan->modified = 1; + iio_chan->channel2 = mod; + iio_chan->info_mask_separate = BIT(IIO_CHAN_INFO_SCALE); + iio_chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ); + iio_chan->info_mask_shared_by_type_available = + BIT(IIO_CHAN_INFO_SAMP_FREQ); + iio_chan->scan_index = scan_index; + iio_chan->scan_type.sign = 's'; + iio_chan->scan_type.realbits = 64; + iio_chan->scan_type.storagebits = 64; + iio_chan->scan_type.endianness = IIO_LE; + iio_chan->ext_info = scmi_iio_ext_info; +} + +static int scmi_iio_get_chan_modifier(const char *name, + enum iio_modifier *modifier) +{ + char *pch, mod; + + if (!name) + return -EINVAL; + + pch = strrchr(name, '_'); + if (!pch) + return -EINVAL; + + mod = *(pch + 1); + switch (mod) { + case 'X': + *modifier = IIO_MOD_X; + return 0; + case 'Y': + *modifier = IIO_MOD_Y; + return 0; + case 'Z': + *modifier = IIO_MOD_Z; + return 0; + default: + return -EINVAL; + } +} + +static int scmi_iio_get_chan_type(u8 scmi_type, enum iio_chan_type *iio_type) +{ + switch (scmi_type) { + case METERS_SEC_SQUARED: + *iio_type = IIO_ACCEL; + return 0; + case RADIANS_SEC: + *iio_type = IIO_ANGL_VEL; + return 0; + default: + return -EINVAL; + } +} + +static u64 scmi_iio_convert_interval_to_ns(u32 val) +{ + u64 sensor_update_interval = + SCMI_SENS_INTVL_GET_SECS(val) * NSEC_PER_SEC; + u64 sensor_interval_mult; + int mult; + + mult = SCMI_SENS_INTVL_GET_EXP(val); + if (mult < 0) { + sensor_interval_mult = int_pow(10, abs(mult)); + sensor_update_interval = + sensor_update_interval / sensor_interval_mult; + } else { + sensor_interval_mult = int_pow(10, mult); + sensor_update_interval = + sensor_update_interval * sensor_interval_mult; + } + return sensor_update_interval; +} + +static int scmi_iio_set_sampling_freq_avail(struct iio_dev *iio_dev) +{ + u64 cur_interval_ns, low_interval_ns, high_interval_ns, step_size_ns, + hz, uhz; + unsigned int cur_interval, low_interval, high_interval, step_size; + struct scmi_iio_priv *sensor = iio_priv(iio_dev); + int i; + + sensor->freq_avail = + devm_kzalloc(&iio_dev->dev, + sizeof(*sensor->freq_avail) * + (sensor->sensor_info->intervals.count * 2), + GFP_KERNEL); + if (!sensor->freq_avail) + return -ENOMEM; + + if (sensor->sensor_info->intervals.segmented) { + low_interval = sensor->sensor_info->intervals + .desc[SCMI_SENS_INTVL_SEGMENT_LOW]; + low_interval_ns = scmi_iio_convert_interval_to_ns(low_interval); + convert_ns_to_freq(low_interval_ns, &hz, &uhz); + sensor->freq_avail[0] = hz; + sensor->freq_avail[1] = uhz; + + step_size = sensor->sensor_info->intervals + .desc[SCMI_SENS_INTVL_SEGMENT_STEP]; + step_size_ns = scmi_iio_convert_interval_to_ns(step_size); + convert_ns_to_freq(step_size_ns, &hz, &uhz); + sensor->freq_avail[2] = hz; + sensor->freq_avail[3] = uhz; + + high_interval = sensor->sensor_info->intervals + .desc[SCMI_SENS_INTVL_SEGMENT_HIGH]; + high_interval_ns = + scmi_iio_convert_interval_to_ns(high_interval); + convert_ns_to_freq(high_interval_ns, &hz, &uhz); + sensor->freq_avail[4] = hz; + sensor->freq_avail[5] = uhz; + } else { + for (i = 0; i < sensor->sensor_info->intervals.count; i++) { + cur_interval = sensor->sensor_info->intervals.desc[i]; + cur_interval_ns = + scmi_iio_convert_interval_to_ns(cur_interval); + convert_ns_to_freq(cur_interval_ns, &hz, &uhz); + sensor->freq_avail[i * 2] = hz; + sensor->freq_avail[i * 2 + 1] = uhz; + } + } + return 0; +} + +static int scmi_iio_buffers_setup(struct iio_dev *scmi_iiodev) +{ + struct iio_buffer *buffer; + + buffer = devm_iio_kfifo_allocate(&scmi_iiodev->dev); + if (!buffer) + return -ENOMEM; + + iio_device_attach_buffer(scmi_iiodev, buffer); + scmi_iiodev->modes |= INDIO_BUFFER_SOFTWARE; + scmi_iiodev->setup_ops = &scmi_iio_buffer_ops; + return 0; +} + +struct iio_dev *scmi_alloc_iiodev(struct device *dev, + struct scmi_handle *handle, + 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; + int i, ret; + + iiodev = devm_iio_device_alloc(dev, sizeof(*sensor)); + if (!iiodev) + return ERR_PTR(-ENOMEM); + + iiodev->modes = INDIO_DIRECT_MODE; + iiodev->dev.parent = dev; + sensor = iio_priv(iiodev); + sensor->handle = handle; + sensor->sensor_info = sensor_info; + sensor->sensor_update_nb.notifier_call = scmi_iio_sensor_update_cb; + sensor->indio_dev = iiodev; + + /* adding one additional channel for timestamp */ + iiodev->num_channels = sensor_info->num_axis + 1; + iiodev->name = sensor_info->name; + iiodev->info = &scmi_iio_info; + + iio_channels = + devm_kzalloc(dev, + sizeof(*iio_channels) * (iiodev->num_channels), + GFP_KERNEL); + if (!iio_channels) + return ERR_PTR(-ENOMEM); + + scmi_iio_set_sampling_freq_avail(iiodev); + + for (i = 0; i < sensor_info->num_axis; i++) { + ret = scmi_iio_get_chan_type(sensor_info->axis[i].type, &type); + if (ret < 0) + return ERR_PTR(ret); + + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, + &modifier); + if (ret < 0) + return ERR_PTR(ret); + + scmi_iio_set_data_channel(&iio_channels[i], type, modifier, + sensor_info->axis[i].id); + } + + scmi_iio_set_timestamp_channel(&iio_channels[i], i); + iiodev->channels = iio_channels; + return iiodev; +} + +static int scmi_iio_dev_probe(struct scmi_device *sdev) +{ + const struct scmi_sensor_info *sensor_info; + struct scmi_handle *handle = sdev->handle; + struct device *dev = &sdev->dev; + struct iio_dev *scmi_iio_dev; + u16 nr_sensors; + int err = -ENODEV, i; + + if (!handle || !handle->sensor_ops) { + dev_err(dev, "SCMI device has no sensor interface\n"); + return -EINVAL; + } + + nr_sensors = handle->sensor_ops->count_get(handle); + 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); + if (!sensor_info) { + dev_err(dev, "SCMI sensor %d has missing info\n", i); + return -EINVAL; + } + + /* This driver only supports 3-axis accel and gyro, skipping other sensors */ + if (sensor_info->num_axis != SCMI_IIO_NUM_OF_AXIS) + continue; + + /* This driver only supports 3-axis accel and gyro, skipping other sensors */ + if (sensor_info->axis[0].type != METERS_SEC_SQUARED && + sensor_info->axis[0].type != RADIANS_SEC) + continue; + + scmi_iio_dev = scmi_alloc_iiodev(dev, handle, sensor_info); + if (IS_ERR(scmi_iio_dev)) { + dev_err(dev, + "failed to allocate IIO device for sensor %s: %ld\n", + sensor_info->name, PTR_ERR(scmi_iio_dev)); + return PTR_ERR(scmi_iio_dev); + } + + err = scmi_iio_buffers_setup(scmi_iio_dev); + if (err < 0) { + dev_err(dev, + "IIO buffer setup error at sensor %s: %d\n", + sensor_info->name, err); + return err; + } + + err = devm_iio_device_register(dev, scmi_iio_dev); + if (err) { + dev_err(dev, + "IIO device registration failed at sensor %s: %d\n", + sensor_info->name, err); + return err; + } + } + return err; +} + +static const struct scmi_device_id scmi_id_table[] = { + { SCMI_PROTOCOL_SENSOR, "iiodev" }, + {}, +}; + +MODULE_DEVICE_TABLE(scmi, scmi_id_table); + +static struct scmi_driver scmi_iiodev_driver = { + .name = "scmi-sensor-iiodev", + .probe = scmi_iio_dev_probe, + .id_table = scmi_id_table, +}; + +module_scmi_driver(scmi_iiodev_driver); + +MODULE_AUTHOR("Jyoti Bhayana <jbhayana@google.com>"); +MODULE_DESCRIPTION("SCMI IIO Driver"); +MODULE_LICENSE("GPL v2");
This change provides ARM SCMI Protocol based IIO device. This driver provides support for Accelerometer and Gyroscope using SCMI Sensor Protocol extensions added in the SCMIv3.0 ARM specification Signed-off-by: Jyoti Bhayana <jbhayana@google.com> --- MAINTAINERS | 6 + drivers/firmware/arm_scmi/driver.c | 2 +- drivers/iio/common/Kconfig | 1 + drivers/iio/common/Makefile | 1 + drivers/iio/common/scmi_sensors/Kconfig | 18 + drivers/iio/common/scmi_sensors/Makefile | 5 + drivers/iio/common/scmi_sensors/scmi_iio.c | 673 +++++++++++++++++++++ 7 files changed, 705 insertions(+), 1 deletion(-) create mode 100644 drivers/iio/common/scmi_sensors/Kconfig create mode 100644 drivers/iio/common/scmi_sensors/Makefile create mode 100644 drivers/iio/common/scmi_sensors/scmi_iio.c