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

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2024-06-24 at 18:26 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
> 
> On 6/24/24 17:22, Nuno Sá wrote:
> > 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@foss.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@foss.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.
> > 
> 
> Thanks for clarification. Your previous message was actually clear 
> enough regarding iio_backend_read_raw() API.
> 
> However, your comment about extend_chan_spec(), let me think that I 
> could maybe spare a new API, and just re-use iio_backend_ext_info_get() 
> callback.
> Nevertheless, this API cannot be used directly, as it can be used only 
> for a frontend associated to a single backend. There is a comment in 
> iio_backend_ext_info_get() about the need of another API for such case.
> 
> So I considered introducing this new API (instead of read_raw):
> ssize_t iio_backend_ext_info_get_from_backend(struct iio_backend *back, 
> uintptr_t private, const struct iio_chan_spec *chan, char *buf)
> (I'm not sure this name is the most relevant).

Yeah, don't think that's the way to go... If you have multiple backends the idea
is to add a .get_backend() callback into struct iio_info so we can get the
backend handle of the frontend device. It was not done because we still don't
have a valid user for such a callback.

But having the said the above, I also don't think we should use any extended
info API to handle scale and offset as those are standard attributes. That would
open a dangerous precedence :).
 
> 
> But if you don't like this alternative too much, I will keep the initial 
> "catch all" iio_backend_read_raw() API.

Right...

- Nuno Sá
> > >