From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 811B6C43381 for ; Tue, 9 Feb 2021 11:58:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4A73764E3B for ; Tue, 9 Feb 2021 11:58:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229725AbhBIL6n (ORCPT ); Tue, 9 Feb 2021 06:58:43 -0500 Received: from foss.arm.com ([217.140.110.172]:50406 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229564AbhBIL5b (ORCPT ); Tue, 9 Feb 2021 06:57:31 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8F18BED1; Tue, 9 Feb 2021 03:56:44 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D0CCD3F73B; Tue, 9 Feb 2021 03:56:41 -0800 (PST) Date: Tue, 9 Feb 2021 11:56:39 +0000 From: Cristian Marussi To: Jyoti Bhayana Cc: Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Mauro Carvalho Chehab , "David S. Miller" , Rob Herring , Lukas Bulwahn , linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, sudeep.holla@arm.com, egranata@google.com, mikhail.golubev@opensynergy.com, Igor.Skalkin@opensynergy.com, Peter.hilber@opensynergy.com, ankitarora@google.com, cristian.marussi@arm.com Subject: Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Message-ID: <20210209115639.GC6873@e120937-lin> References: <20210208211918.1280588-1-jbhayana@google.com> <20210208211918.1280588-2-jbhayana@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210208211918.1280588-2-jbhayana@google.com> User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jyoti some minor things down below. Other than that, FWIW about the SCMI side of this: Reviewed-by: Cristian Marussi 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 > --- > 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 > +L: linux-iio@vger.kernel.org > +S: Maintained > +F: drivers/iio/common/scmi_sensors/scmi_iio.c > + > IIO SUBSYSTEM AND DRIVERS > M: Jonathan Cameron > R: Lars-Peter Clausen > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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; > +} > +