| Message ID | 20240618160836.945242-8-olivier.moysan@foss.st.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: dfsdm: add scaling support | expand |
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. > +{ > + struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend); > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + *val = priv->vref_mv; This doesn't really feel right as what are we calling to? Using it to pass the reference voltage doesn't make sense under normal handling of these. So at very least needs a comment. > + *val2 = 0; No need to set val2. > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_OFFSET: > + *val = 0; > + *val2 = 0; > + return IIO_VAL_INT; Normally we just don't provide this but I guess you are requiring all of these? Long term that won't scale, so you need your caller to handle a suitable error return, -EINVAL will work to say not supported. > + } > + > + return -EINVAL; > +}; > + > +static const struct iio_backend_ops sd_backend_ops = { > + .enable = sd_backend_enable, > + .disable = sd_backend_disable, > + .read_raw = sd_backend_read, > +}; > + > +static int iio_sd_backend_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct regulator *vref; > + struct iio_sd_backend_priv *priv; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + vref = devm_regulator_get_optional(dev, "vref"); New devm_regulator_get_enable_read_voltage() slightly simplifies this and means you don't need to keep vref around. > + if (IS_ERR(vref)) { > + if (PTR_ERR(vref) != -ENODEV) > + return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n"); > + } else { > + ret = regulator_get_voltage(vref); You haven't turned it on so it's not guaranteed to give you a useful answer. Use the enable_read_voltage variant and that will handle this for you. > + if (ret < 0) > + return ret; > + > + priv->vref = vref; > + priv->vref_mv = ret / 1000; > + } > + > + ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv); > + if (ret) > + return ret; > + > + return 0; return devm_iio_.... > +}; > + > +static const struct of_device_id sd_backend_of_match[] = { > + { .compatible = "sd-backend" }, > + { .compatible = "ads1201" }, Conor pointed out ti,ads1201 At least I assume ti? > + { } > +};
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. >> +{ >> + struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend); >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_SCALE: >> + *val = priv->vref_mv; > > This doesn't really feel right as what are we calling to? Using it to pass the > reference voltage doesn't make sense under normal handling of these. So at very > least needs a comment. > > >> + *val2 = 0; > No need to set val2. Removed in v2 also. >> + return IIO_VAL_INT; >> + >> + case IIO_CHAN_INFO_OFFSET: >> + *val = 0; >> + *val2 = 0; >> + return IIO_VAL_INT; > Normally we just don't provide this but I guess you are requiring all of these? > Long term that won't scale, so you need your caller to handle a suitable > error return, -EINVAL will work to say not supported. > Offset support is not strictly required for time being, as reported as zero. The aim was to anticipate further needs, but it may be dropped for now. I will remove it from v2, If you suggest keeping only the bare essentials here. Error changed to "-EOPNOTSUPP" for unknown requests. >> + } >> + >> + return -EINVAL; >> +}; >> + >> +static const struct iio_backend_ops sd_backend_ops = { >> + .enable = sd_backend_enable, >> + .disable = sd_backend_disable, >> + .read_raw = sd_backend_read, >> +}; >> + >> +static int iio_sd_backend_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct regulator *vref; >> + struct iio_sd_backend_priv *priv; >> + int ret; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + vref = devm_regulator_get_optional(dev, "vref"); > > New devm_regulator_get_enable_read_voltage() slightly simplifies this > and means you don't need to keep vref around. > >> + if (IS_ERR(vref)) { >> + if (PTR_ERR(vref) != -ENODEV) >> + return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n"); >> + } else { >> + ret = regulator_get_voltage(vref); > You haven't turned it on so it's not guaranteed to give you a useful > answer. > My understanding is that regulator_get_voltage() always returns the regulator voltage, whatever the regulator state, as documented in the API description: "* NOTE: If the regulator is disabled it will return the voltage value. * This function should not be used to determine regulator state." So, my logic was to enable the regulator only when requested, through enable/disable callbacks to manage power. Please, let me know if I missed something here. > Use the enable_read_voltage variant and that will handle this for you. > >> + if (ret < 0) >> + return ret; >> + >> + priv->vref = vref; >> + priv->vref_mv = ret / 1000; >> + } >> + >> + ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv); >> + if (ret) >> + return ret; >> + >> + return 0; > > return devm_iio_.... > Done >> +}; >> + >> +static const struct of_device_id sd_backend_of_match[] = { >> + { .compatible = "sd-backend" }, >> + { .compatible = "ads1201" }, > > Conor pointed out ti,ads1201 > At least I assume ti? > Ack. changed to ti,ads1201 >> + { } >> +}; > > Regards Olivier
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. 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á > >
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). But if you don't like this alternative too much, I will keep the initial "catch all" iio_backend_read_raw() API. BRs Olivier > 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á > >>>
> >> + } > >> + > >> + return -EINVAL; > >> +}; > >> + > >> +static const struct iio_backend_ops sd_backend_ops = { > >> + .enable = sd_backend_enable, > >> + .disable = sd_backend_disable, > >> + .read_raw = sd_backend_read, > >> +}; > >> + > >> +static int iio_sd_backend_probe(struct platform_device *pdev) > >> +{ > >> + struct device *dev = &pdev->dev; > >> + struct regulator *vref; > >> + struct iio_sd_backend_priv *priv; > >> + int ret; > >> + > >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > >> + if (!priv) > >> + return -ENOMEM; > >> + > >> + vref = devm_regulator_get_optional(dev, "vref"); > > > > New devm_regulator_get_enable_read_voltage() slightly simplifies this > > and means you don't need to keep vref around. > > > >> + if (IS_ERR(vref)) { > >> + if (PTR_ERR(vref) != -ENODEV) > >> + return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n"); > >> + } else { > >> + ret = regulator_get_voltage(vref); > > You haven't turned it on so it's not guaranteed to give you a useful > > answer. > > > > My understanding is that regulator_get_voltage() always returns the > regulator voltage, whatever the regulator state, as documented in the > API description: > "* NOTE: If the regulator is disabled it will return the voltage value. > * This function should not be used to determine regulator state." > > So, my logic was to enable the regulator only when requested, through > enable/disable callbacks to manage power. > > Please, let me know if I missed something here. Ah ok. I had a vague and it seems incorrect recollection that you had to turn the regs on to get voltage in some cases. Ah well. Clearly not :) What you have is fine. Add a comment though just so no one replaces this code with the helper. Jonathan
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á > > >
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 3d91015af6ea..f3dfdaa80678 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1155,6 +1155,16 @@ config SPEAR_ADC To compile this driver as a module, choose M here: the module will be called spear_adc. +config SD_ADC_BACKEND + tristate "Generic sigma delta modulator IIO backend" + select IIO_BACKEND + help + Select this option to enables sigma delta modulator. This driver can + support generic sigma delta modulators, as IIO backend devices. + + This driver can also be built as a module. If so, the module + will be called sd_adc_backend. + config SD_ADC_MODULATOR tristate "Generic sigma delta modulator" select IIO_BUFFER diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 37ac689a0209..9aee2e4307d7 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -139,3 +139,4 @@ obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o +obj-$(CONFIG_SD_ADC_BACKEND) += sd_adc_backend.o 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) +{ + struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend); + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + *val = priv->vref_mv; + *val2 = 0; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_OFFSET: + *val = 0; + *val2 = 0; + return IIO_VAL_INT; + } + + return -EINVAL; +}; + +static const struct iio_backend_ops sd_backend_ops = { + .enable = sd_backend_enable, + .disable = sd_backend_disable, + .read_raw = sd_backend_read, +}; + +static int iio_sd_backend_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct regulator *vref; + struct iio_sd_backend_priv *priv; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + vref = devm_regulator_get_optional(dev, "vref"); + if (IS_ERR(vref)) { + if (PTR_ERR(vref) != -ENODEV) + return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n"); + } else { + ret = regulator_get_voltage(vref); + if (ret < 0) + return ret; + + priv->vref = vref; + priv->vref_mv = ret / 1000; + } + + ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv); + if (ret) + return ret; + + return 0; +}; + +static const struct of_device_id sd_backend_of_match[] = { + { .compatible = "sd-backend" }, + { .compatible = "ads1201" }, + { } +}; +MODULE_DEVICE_TABLE(of, sd_backend_of_match); + +static struct platform_driver iio_sd_backend_adc = { + .driver = { + .name = "iio_sd_adc_backend", + .of_match_table = sd_backend_of_match, + }, + .probe = iio_sd_backend_probe, +}; + +module_platform_driver(iio_sd_backend_adc); + +MODULE_DESCRIPTION("Basic sigma delta modulator IIO backend"); +MODULE_AUTHOR("Olivier Moysan <olivier.moysan@foss.st.com>"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_BACKEND);
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> --- drivers/iio/adc/Kconfig | 10 +++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/sd_adc_backend.c | 110 +++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) create mode 100644 drivers/iio/adc/sd_adc_backend.c