linux-kernel - Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 18 Jul 2022 10:01:36 +0000
From:   Dmitry Rokosov <DDRokosov@...rdevices.ru>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "stano.jakubek@...il.com" <stano.jakubek@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "stephan@...hold.net" <stephan@...hold.net>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

Jonathan,

On Sat, Jul 16, 2022 at 04:51:21PM +0100, Jonathan Cameron wrote:
> On Fri, 1 Jul 2022 13:49:10 +0000
> Dmitry Rokosov <DDRokosov@...rdevices.ru> wrote:
> 
> > Hello Jonathan,
> > 
> > Sorry for the delayed response.
> > 
> > On Sun, Jun 19, 2022 at 01:27:03PM +0100, Jonathan Cameron wrote:
> > > On Thu, 16 Jun 2022 10:42:14 +0000
> > > Dmitry Rokosov <DDRokosov@...rdevices.ru> wrote:
> > >   
> > > > MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > > > sensitivity consumer applications. It has dynamical user selectable full
> > > > scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > > > with output data rates from 1Hz to 1000Hz.
> > > > 
> > > > Datasheet can be found at following URL:
> > > > https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > > > 
> > > > This driver supports following MSA311 features:
> > > >     - IIO interface
> > > >     - Different power modes: NORMAL and SUSPEND (using pm_runtime)
> > > >     - ODR (Output Data Rate) selection
> > > >     - Scale and samp_freq selection
> > > >     - IIO triggered buffer, IIO reg access
> > > >     - NEW_DATA interrupt + trigger
> > > > 
> > > > Below features to be done:
> > > >     - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
> > > >     - Low Power mode
> > > > 
> > > > Signed-off-by: Dmitry Rokosov <ddrokosov@...rdevices.ru>  
> > > Hi Dmitry,
> > > 
> > > A few things I missed before + I'm still not happy with the runtime
> > > pm handling.  One case that isn't covered well is !CONFIG_RUNTIME_PM
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > >   
> > 
> > ...
> > 
> > > > +static irqreturn_t msa311_buffer_thread(int irq, void *p)
> > > > +{
> > > > +	struct iio_poll_func *pf = p;
> > > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > > +	struct msa311_priv *msa311 = iio_priv(indio_dev);
> > > > +	struct device *dev = &msa311->i2c->dev;
> > > > +	const struct iio_chan_spec *chan;
> > > > +	__le16 axis;
> > > > +	int bit = 0, err, i = 0;
> > > > +
> > > > +	/* Ensure correct alignment of time stamp when present */
> > > > +	struct {
> > > > +		__le16 channels[MSA311_SI_Z + 1];
> > > > +		s64 ts __aligned(8);
> > > > +	} buf;
> > > > +
> > > > +	memset(&buf, 0, sizeof(buf));
> > > > +
> > > > +	mutex_lock(&msa311->lock);
> > > > +
> > > > +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > +			 indio_dev->masklength) {
> > > > +		chan = &msa311_channels[bit];  
> > > 
> > > Nothing to do with your driver, but feels like it's worth
> > > exploring a
> > > 	for_each_chan_in_iio_scan(struct iio_chan_spec, struct iio_dev) macro.
> > > 
> > > I'll add that to my todo list.
> > >   
> > 
> > If you don't mind, I can prepare such a patch.
> 
> I had a look at this whilst travelling and it's a lot more complex than I
> thought it would be because of gaps in the scan_index in some drivers (not
> all channels have scan indexes and not all scan indexes are used)
> 
> If we write such a thing we need to resolve that in the core and I suspect
> it will require creation of an indirection structure that lets us
> do scan_index based look up of channels.  Whilst that works in many drivers
> because there is a nice 1 to 1 mapping, there are exceptions.
> Hence I think we would be looking at:
> 
> 1) Check at registration time on whether scan_index == location in
> iio_dev->channels, if so set another pointer say iio_dev->channels_linear =
> iio_dev->channels.
> 2) If not, create a lookup table and make iio_dev->channels_linear
> point to that.
> 3) Finally introduce a macro that uses channels_linear.
> 
> What fun ;)
> 
> Jonathan
>  

Okay, I'll try looking for proper solution and prepare RFC patch.

-- 
Thank you,
Dmitry