linux-kernel - Re: [PATCH 7/8] iio: add sd modulator generic iio backend

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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 24 Jun 2024 17:22:54 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Olivier MOYSAN <olivier.moysan@...s.st.com>, Jonathan Cameron
	 <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Liam Girdwood
 <lgirdwood@...il.com>,  Mark Brown <broonie@...nel.org>,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 7/8] iio: add sd modulator generic iio backend

Hi Olivier,

On Mon, 2024-06-24 at 14:43 +0200, Olivier MOYSAN wrote:
> Hi Jonathan,
> 
> On 6/23/24 17:11, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 18:08:33 +0200
> > Olivier Moysan <olivier.moysan@...s.st.com> wrote:
> > 
> > > Add a generic driver to support sigma delta modulators.
> > > Typically, this device is a hardware connected to an IIO device
> > > in charge of the conversion. The device is exposed as an IIO backend
> > > device. This backend device and the associated conversion device
> > > can be seen as an aggregate device from IIO framework.
> > > 
> > > Signed-off-by: Olivier Moysan <olivier.moysan@...s.st.com>
> > 
> > Trivial comments inline.
> > 
> > > diff --git a/drivers/iio/adc/sd_adc_backend.c
> > > b/drivers/iio/adc/sd_adc_backend.c
> > > new file mode 100644
> > > index 000000000000..556a49dc537b
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/sd_adc_backend.c
> > > @@ -0,0 +1,110 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Generic sigma delta modulator IIO backend
> > > + *
> > > + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
> > > + */
> > > +
> > > +#include <linux/iio/backend.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +struct iio_sd_backend_priv {
> > > +	struct regulator *vref;
> > > +	int vref_mv;
> > > +};
> > > +
> > > +static int sd_backend_enable(struct iio_backend *backend)
> > > +{
> > > +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> > > +
> > > +	return regulator_enable(priv->vref);
> > > +};
> > > +
> > > +static void sd_backend_disable(struct iio_backend *backend)
> > > +{
> > > +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> > > +
> > > +	regulator_disable(priv->vref);
> > > +};
> > > +
> > > +static int sd_backend_read(struct iio_backend *backend, int *val, int *val2,
> > > long mask)
> > Nothing to do with this patch as such:
> > 
> > One day I'm going to bit the bullet and fix that naming.
> > Long long ago when the Earth was young it actually was a bitmap which
> > I miscalled a mask - it only had one bit ever set, which was a dumb
> > bit of API.  It's not been true for a long time.
> > Anyhow, one more instances isn't too much of a problem I guess.
> > 
> 
> I changed the callback .read_raw to .ext_info_get to take Nuno's comment 
> about iio_backend_read_raw() API, into account.
> So, I changed this function to
> static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t 
> private, const struct iio_chan_spec *chan, char *buf)
> for v2 version.
> 

Maybe I'm missing something but I think I did not explained myself very well. What I
had in mind was that since you're calling .read_raw() from IIO_CHAN_INFO_SCALE and
IIO_CHAN_INFO_OFFSET, it could make sense to have more dedicated API's. Meaning:

iio_backend_read_scale(...)
iio_backend_read_offset(...)

The iio_backend_read_raw() may make sense when frontends call
iio_backend_extend_chan_spec() and have no idea what the backend may have added to
the channel. So, in those cases something like this could make sense:

switch (mask)
IIO_CHAN_INFO_RAW:

...

default:
	return iio_backend_read_raw();

but like I said maybe this is me over-complicating and a simple
iio_backend_read_raw() is sufficient. But I think I never mentioned something like
.read_raw -> .ext_info_get.

The other thing I mentioned was to instead of having:


if (child) {
	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
				 BIT(IIO_CHAN_INFO_SCALE) |
				 BIT(IIO_CHAN_INFO_OFFSET);
}

You could use iio_backend_extend_chan_spec() and have the backend set the SCALE and
OFFSET bits for you as it seems these functionality depends on the backend.

But none of the above were critical or things that I feel to strong about.

Anyways, just wanted to give some clarification as it seems there were some
misunderstandings (I think).

- Nuno Sá

> >