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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > >
> > > > 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á
> > >
