On Tue, 16 Feb 2021 10:20:59 +0000 Cristian Marussi <cristian.marussi@xxxxxxx> 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@xxxxxxx/ > > 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@xxxxxxx> > > 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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx/ > > > > > > > > > > > > > > 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@xxxxxxx> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Jyoti > > > > > > > > > > > > > > > > > > some minor things down below. > > > > > > > > > > > > > > > > > > Other than that, FWIW about the SCMI side of this: > > > > > > > > > > > > > > > > > > Reviewed-by: Cristian Marussi <cristian.marussi@xxxxxxx> > > > > > > > > > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > > > > > --- > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > > > > > +L: linux-iio@xxxxxxxxxxxxxxx > > > > > > > > > > +S: Maintained > > > > > > > > > > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > > > > > > > > > > + > > > > > > > > > > IIO SUBSYSTEM AND DRIVERS > > > > > > > > > > M: Jonathan Cameron <jic23@xxxxxxxxxx> > > > > > > > > > > R: Lars-Peter Clausen <lars@xxxxxxxxxx> > > > > > > > > > > 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; > > > > > > > > > > +} > > > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > >
