Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp2756586pxb; Sat, 30 Jan 2021 13:24:50 -0800 (PST) X-Google-Smtp-Source: ABdhPJyRhDihNBzCHWcHG0rBZqvqhc2GAI0qhqVmjZNFtcr3xsGF3KM0V8cLYM7M9iqqU0h1IVuq X-Received: by 2002:a05:6402:254b:: with SMTP id l11mr11590228edb.202.1612041890012; Sat, 30 Jan 2021 13:24:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1612041890; cv=none; d=google.com; s=arc-20160816; b=HZkUPmhJD6AYwVo7wWFlmFmiuvZjcHIdw0o9uD1o0rqnjtPgsBREEkImcxW3377vaS YZ6ZWAJrTk+iN84dajf+E5wMgLNDn2gAZmIGxaajXClcsSBsHKG3aRky6XADwMTgXYK9 IOCyX5xg/kMcMVHHq31aYV9+i8e7v95HWWtAyZ1fG/XlxVl1bZU8M4WmRAmF1WxtTq2i zgRI6seV02nt+ZuLESCh/WSXGt9cWfwj92GiAEpfx+4wDYHiIY2MSqRF+PMAZJ2Mw52b Vz1IcITj5MPardfAR3BqiGNsLybrW9d0Lw+o26aM0H9Ij/YoTgCSneRVfGUj3JBLso2a 3SXQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=GJ1IJlfbrNW13eDOgGPa9erKLapdcpWDQLMOsGCTdTE=; b=GTMco64UDa1gxRiDNYzTxtaR4tgI5Ndr7qpOIdLlyds6f9xN4pYbdnB/T0ZYl+YfKB NlwxlCH1gA0TGoe9/MUVpiwqmuB82tzWRXhiPifUlBTfvmN2xuFXONN+ysPZdQJ5cGL+ ZB88vFicdhteZpGvnbJNsn/KsFFPtzWKpSrnqNmJG5R6BwiX6ESmqNDhhBgGck4FPN8Q iuJLAHw46P5OGCvJyLl3SWgB0/Jh4kWt1BWaIsV5NDFCkhopFzEt9CjguL1n5WDqk+hh Oy6VCoBhj+AamGUhPuHoRAxe6SD+bz9fJsZpujMY2v0KS7Qy2GwiMT5DvKYrSLit4BPu uq3Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id o16si7177893ejr.715.2021.01.30.13.24.25; Sat, 30 Jan 2021 13:24:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232213AbhA3VWo (ORCPT + 99 others); Sat, 30 Jan 2021 16:22:44 -0500 Received: from foss.arm.com ([217.140.110.172]:41346 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230360AbhA3VWi (ORCPT ); Sat, 30 Jan 2021 16:22:38 -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 1CF3F1042; Sat, 30 Jan 2021 13:21:51 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D879A3F73D; Sat, 30 Jan 2021 13:21:47 -0800 (PST) Date: Sat, 30 Jan 2021 21:21:41 +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 , Enrico Granata , Mikhail Golubev , Igor Skalkin , Peter Hilber , Ankit Arora Subject: Re: [RFC PATCH v3 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors Message-ID: <20210130212141.GA7790@e120937-lin> References: <20210121232147.1849509-1-jbhayana@google.com> <20210121232147.1849509-2-jbhayana@google.com> <20210122133740.GB22231@e120937-lin> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jyoti, On Fri, Jan 29, 2021 at 02:43:13PM -0800, Jyoti Bhayana wrote: > Hi Christian, > > I have addressed your feedback in v4 of the patch. Please find below > the answers to some of your questions: > > The initial sensor_config ORed here is NOT initialized nor zeroed. > > There is no need to initialize as it is already initialized to 0 when defined. > True for the sensor_config in scmi_iio_buffer_postdisable(), BUT it is not indeed zero-initialized at definition time in the other similar function scmi_iio_buffer_preenable(), that is what the above comment was referred to. > > being not so familiar with IIO, may I ask when and how much frequently > > these ops are called ? (given they include a register/unregister + > > enable/disable every time) > > These are called whenever the sensor is enabled/disabled by writing > to buffer/enable sysfs node > Ok, thanks. > >AFAIU this is called during IIO init and derives a modifier from the name > >of the sensors which are passed by the platform fw with sensor descriptors, > >so I would not trust them to be weell formed (:D) and try to be picky about > >*name as it was user-input to validate. > > The SCMI specification has mentioned the naming convention for the > axis as below . > "A NULL terminated UTF-8 format string with the sensor axis name, of > up to 16 bytes. It is recommended that the name ends with ‘_’ followed > by the axis of the sensor in uppercase. For example, the name for the > x-axis of a triaxial accelerometer could be “acc_X” or “_X” > > If there is any other better way to get the modifier, please let me know. > What I was trying to say was simply that, despite what the spec says, you cannot trust the fw/platform produced values (fw buggy ? data on channel corrupted ? malicious pkt injections ? fuzzing ? ) and so you should be picky and validate them also against any possible out-of-spec data, since the Kernel must not crash even if presented with unreasonable out-of-spec garbage-like data, thing that indeed you did in v4 by adding the check for NULL against name param. :D (the other case of a malformed name like "acc_" seemed to me handled properly already by the kernel strcmp) > > why not a break to a final 'return ret;' (with ret properly set) ? > > According to Jonathan, direct returns are preferred. > Ok > >Not familiar with IIO, but is it fine to setup ops and modes AFTER > >having attached the buffer to the scmi_iiodev ? > >Is is not 'racy-possible' that the buffer is already operational without > >any ops immediately after being attached ? > > I have looked at other IIO drivers and they all do it this way. > Ok. Thanks, I'll have a look at v4. Cristian > Thanks, > Jyoti > > > > > > > > On Fri, Jan 22, 2021 at 5:37 AM Cristian Marussi > wrote: > > > > Hi Jyoti, > > > > a few remarks below. > > > > On Thu, Jan 21, 2021 at 11:21:47PM +0000, Jyoti Bhayana wrote: > > > This change provides ARM SCMI Protocol based IIO device. > > > This driver provides support for Accelerometer and Gyroscope using > > > new SCMI Sensor Protocol defined by the upcoming SCMIv3.0 > > > > I'd say: > > > > new SCMI Sensor Protocol extension added by the upcoming SCMIv3.0 > > > > given that SCMI Sensor existed already in SCMIv2.0 > > > > > ARM specification > > > > > > Signed-off-by: Jyoti Bhayana > > > --- > > > MAINTAINERS | 6 + > > > 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 | 736 +++++++++++++++++++++ > > > 6 files changed, 767 insertions(+) > > > 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/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..3b76cc54511c > > > --- /dev/null > > > +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c > > > @@ -0,0 +1,736 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +/* > > > + * System Control and Management Interface(SCMI) based IIO sensor driver > > > + * > > > + * Copyright (C) 2020 Google LLC > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#define ilog10(x) (ilog2(x) / const_ilog2(10)) > > > +#define UHZ_PER_HZ 1000000UL > > > +#define ODR_EXPAND(odr, uodr) (((odr) * 1000000ULL) + (uodr)) > > > +#define MAX_NUM_OF_CHANNELS 4 > > > +#define H32(x) (((x) & 0xFFFFFFFF00000000LL) >> 32) > > > +#define L32(x) ((x) & 0xFFFFFFFFLL) > > > > Probaly you want to use GENMASK here to define the masks, and not sure > > but maybe there are already similar macros in linux to extract > > upper/lower 32 bits. > > > > > + > > > +struct scmi_iio_priv { > > > + struct scmi_handle *handle; > > > + const struct scmi_sensor_info *sensor_info; > > > + struct iio_dev *indio_dev; > > > + long long iio_buf[MAX_NUM_OF_CHANNELS]; > > > + struct notifier_block sensor_update_nb; > > > + u32 *freq_avail; > > > + /* > > > + * range_avail = [minRange resolution maxRange] > > > + * with IIO val type as IIO_VAL_FRACTIONAL. > > > + * Hence, array of size 6. > > > + */ > > > + int range_avail[6]; > > > +}; > > > + > > > +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 = iio_get_time_ns(scmi_iio_dev); > > > > This scmi_iio_dev is on the stack and still NOT initialized here right ? > > > > Moreover, if this is meant to attach a timestamp of arrival also to > > NON-timestamped sensor notifications, note that, if you want, scmi_sensor_update_report > > contains a 'timestamp' (as all SCMI notif reports) ktime field (ns) which > > represents the time (ns from boot with ktime_get_boottime()) of reception > > of this notification taken by the SCMI core as soon as it arrives in the > > RX ISR. (so in the OSPM Linux world) > > > > If you grab the timestamp here with iio_get_time_ns() you are indeed > > getting a timestamp inside the notification callback so at the end of > > the notifications processing chain, i.e. in a deferred worker processing > > this queue of events and in turn calling this function, so potentially > > you're timestamping a lot later than when the event arrived really in > > the OSPM world (especially on a loaded system); it is more the time of > > arrival in IIO framework. > > > > On the other side timestamped events handled in the else-branch down > > below carry a timestamp set by the SCMI platform fw early on when the > > event has been detected by the sensor in the real world. > > > > Not sure how these values are used at the end, but just to let you know > > this detail if it was not apparent. > > > > > + } 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 + > > > + ilog10(NSEC_PER_SEC); > > > + 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; > > > + int err; > > > + > > > + if (sensor->sensor_info->timestamped) > > > + sensor_config |= FIELD_PREP(SCMI_SENS_CFG_TSTAMP_ENABLED_MASK, > > > + SCMI_SENS_CFG_TSTAMP_ENABLE); > > > > The initial sensor_config ORed here is NOT initialized nor zeroed. > > > + > > > + 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) > > > + dev_err(&iio_dev->dev, "Error in enabling sensor %s err %d", > > > + sensor->sensor_info->name, err); > > > > Here you do not unregister the notification above here when you fail > > the config_set. > > If you happen to retry this code path again you'll end up registering > > the same notifier_block sensor_update_nb() in the same notification chain > > (same event samme sensor_id), which is broken and probably triggers a WARN > > from the core kernel notification chains machinery too. > > > > In an upcoming series on SCMI core I'll introduce a devm_ variant for > > SCMI notification registration, but as of now you have to properly unregister > > on error paths. > > > > > + > > > + 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); > > > > This alignment here is a bit awkaward. > > > > > + 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, > > > +}; > > > + > > > > Being not so familiar with IIO, may I ask when annd ho wmuch frequently > > these ops are called ? (given they include a register/unregister + > > enable/disable every time) > > > > > +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; > > > + case IIO_CHAN_INFO_RAW: > > > + *vals = sensor->range_avail; > > > + *type = IIO_VAL_FRACTIONAL; > > > + *length = ARRAY_SIZE(sensor->range_avail); > > > + return IIO_AVAIL_RANGE; > > > + default: > > > + return -EINVAL; > > > + } > > > +} > > > + > > > +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); > > > + 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 = ODR_EXPAND(val, 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 = ilog10(((u64)uHz * 0xFFFF) / UHZ_PER_HZ); > > > + > > > + 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 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 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 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) | BIT(IIO_CHAN_INFO_RAW); > > > + 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; > > > +} > > > + > > > +static int scmi_iio_get_chan_modifier(const char *name, > > > + enum iio_modifier *modifier) > > > +{ > > > > AFAIU this is called during IIO init and derives a modifier from the name > > of the sensors which are passed by the platform fw with sensor descriptors, > > so I would not trust them to be weell formed (:D) and try to be picky about > > *name as it was user-input to validate. > > > > > + char *pch; > > > + > > > + pch = strrchr(name, '_'); > > > > What happens if name is NULL ? > > > > > + if (!pch) > > > + return -EINVAL; > > > + > > > + if (strcmp(pch + 1, "X") == 0) { > > > + *modifier = IIO_MOD_X; > > > + return 0; > > > + } else if (strcmp(pch + 1, "Y") == 0) { > > > + *modifier = IIO_MOD_Y; > > > + return 0; > > > + } else if (strcmp(pch + 1, "Z") == 0) { > > > + *modifier = IIO_MOD_Z; > > > + return 0; > > > + } else { > > > + return -EINVAL; > > > + } > > > > Moroever, being a single char compare cannot this be done with a switch ? > > > > char mod = *(pch + 1); > > > > switch(mod) { > > case 'X': > > *modifier = IIO_MOD_X; > > break > > ... > > > > > +} > > > + > > > +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; > > why not a break to a final 'return ret;' (with ret properly set) ? > > > > > + case RADIANS_SEC: > > > + *iio_type = IIO_ANGL_VEL; > > > + return 0; > > > + default: > > > + return -EINVAL; > > > + } > > > +} > > > + > > > +static int scmi_iio_get_sensor_max_range(struct iio_dev *iio_dev, int *val, > > > + int *val2) > > > +{ > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > + int max_range_high, max_range_low; > > > + long long max_range; > > > + > > > + /* > > > + * All the axes are supposed to have the same value for max range. > > > + * We are just using the values from the Axis 0 here. > > > + */ > > > + if (sensor->sensor_info->axis[0].extended_attrs) { > > > + max_range = sensor->sensor_info->axis[0].attrs.max_range; > > > + max_range_high = H32(max_range); > > > + max_range_low = L32(max_range); > > > + > > > + /* > > > + * As IIO Val types have no provision for 64 bit values, > > > + * and currently there are no known sensors using 64 bit > > > + * for the range, this driver only supports sensor with > > > + * 32 bit range value. > > > + */ > > > + if (max_range_high != 0) > > > + return -EINVAL; > > > + > > > + *val = max_range_low; > > > + *val2 = 1; > > > + } > > > + return 0; > > > +} > > > + > > > +static void scmi_iio_get_sensor_resolution(struct iio_dev *iio_dev, int *val, > > > + int *val2) > > > +{ > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > + > > > + /* > > > + * All the axes are supposed to have the same value for resolution > > > + * and exponent. We are just using the values from the Axis 0 here. > > > + */ > > > + if (sensor->sensor_info->axis[0].extended_attrs) { > > > + uint resolution = sensor->sensor_info->axis[0].resolution; > > > + s8 exponent = sensor->sensor_info->axis[0].exponent; > > > + s8 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) { > > > + *val = resolution * int_pow(10, exponent); > > > + *val2 = 1; > > > + } else { > > > + *val = resolution; > > > + *val2 = int_pow(10, abs(exponent)); > > > + } > > > + } > > > +} > > > + > > > +static int scmi_iio_get_sensor_min_range(struct iio_dev *iio_dev, int *val, > > > + int *val2) > > > +{ > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > + int min_range_high, min_range_low; > > > + long long min_range; > > > + > > > + /* > > > + * All the axes are supposed to have the same value for min range. > > > + * 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; > > > + min_range_high = H32(min_range); > > > + min_range_low = L32(min_range); > > > + > > > + /* > > > + * As IIO Val types have no provision for 64 bit values, > > > + * and currently there are no known sensors using 64 bit > > > + * for the range, this driver only supports sensor with > > > + * 32 bit range value. > > > + */ > > > + if (min_range_high != 0xFFFFFFFF) > > > + return -EINVAL; > > > + > > > + *val = min_range_low; > > > + *val2 = 1; > > > + } > > > + return 0; > > > +} > > > + > > > +static int scmi_iio_set_sensor_range_avail(struct iio_dev *iio_dev) > > > +{ > > > + struct scmi_iio_priv *sensor = iio_priv(iio_dev); > > > + int ret; > > > + > > > + ret = scmi_iio_get_sensor_min_range(iio_dev, &sensor->range_avail[0], > > > + &sensor->range_avail[1]); > > > + if (ret) > > > + return ret; > > > + > > > + scmi_iio_get_sensor_resolution(iio_dev, &sensor->range_avail[2], > > > + &sensor->range_avail[3]); > > > + ret = scmi_iio_get_sensor_max_range(iio_dev, &sensor->range_avail[4], > > > + &sensor->range_avail[5]); > > > + return ret; > > > +} > > > + > > > +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(u32) * (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; > > > > Not familiar with IIO, but is it fine to setup ops and modes AFTER > > having attached the buffer to the scmi_iiodev ? > > > > Is is not 'racy-possible' that the buffer is already operational without > > any ops immediately after being attached ? > > > > > + return 0; > > > +} > > > + > > > +static int scmi_alloc_iiodev(struct device *dev, struct scmi_handle *handle, > > > + const struct scmi_sensor_info *sensor_info, > > > + struct iio_dev **scmi_iio_dev) > > > +{ > > > + 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 -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 -ENOMEM; > > > + > > > + scmi_iio_set_sampling_freq_avail(iiodev); > > > + > > > + ret = scmi_iio_set_sensor_range_avail(iiodev); > > > + if (ret) { > > > + dev_err(dev, "Error while setting the sensor %s range %d", > > > + sensor_info->name, ret); > > > + return ret; > > > + } > > > + > > > + 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 ret; > > > + > > > + ret = scmi_iio_get_chan_modifier(sensor_info->axis[i].name, > > > + &modifier); > > > + if (ret < 0) > > > + return 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; > > > + *scmi_iio_dev = iiodev; > > > + return ret; > > > +} > > > + > > > +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, i; > > > + > > > + if (!handle || !handle->sensor_ops || !handle->sensor_ops->count_get || > > > + !handle->sensor_ops->info_get || !handle->sensor_ops->config_get || > > > + !handle->sensor_ops->config_set) { > > > > In all other SCMI driver we just check for: > > > > !handle && !handle->sensor_ops > > > > it should be enough, since all the ops are defined if the stack you are > > using supports SCMIv3.0, if not this driver would not even compile since > > some SCMIv3.0 Sensor Extension sensor_ops would be missing as a whole > > inside the struct itself. > > > > Thanks > > > > Cristian > > > > > + 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 -EINVAL; > > > + } > > > + > > > + dev_dbg(dev, "%d sensors found via SCMI bus\n", nr_sensors); > > > + > > > + 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; > > > + } > > > + > > > + /* Skipping scalar sensor,as this driver only supports accel and gyro */ > > > + if (sensor_info->num_axis == 0) > > > + continue; > > > + > > > + err = scmi_alloc_iiodev(dev, handle, sensor_info, > > > + &scmi_iio_dev); > > > + if (err < 0) { > > > + dev_err(dev, > > > + "failed to allocate IIO device for sensor %s: %d\n", > > > + sensor_info->name, err); > > > + return err; > > > + } > > > + > > > + 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 "); > > > +MODULE_DESCRIPTION("SCMI IIO Driver"); > > > +MODULE_LICENSE("GPL v2"); > > > -- > > > 2.30.0.280.ga3ce27912f-goog > > >