On Thu, 2024-09-12 at 12:13 +0200, Guillaume Stols wrote:
> On 9/5/24 10:40, Nuno Sá wrote:
> > On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote:
> > > - Basic support for iio backend.
> > > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
> > > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
> > > supported if iio-backend mode is selected.
> > >
> > > A small correction was added to the driver's file name in the Kconfig
> > > file's description.
> > >
> > > Signed-off-by: Guillaume Stols <gstols@xxxxxxxxxxxx>
> > > ---
> > Hi Guillaume,
> >
> > Some initial feedback from me...
> >
> > > drivers/iio/adc/Kconfig | 3 +-
> > > drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++-------
> > > -
> > > drivers/iio/adc/ad7606.h | 16 +++++++
> > > drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++-
> > > 4 files changed, 200 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 88e8ce2e78b3..01248b6df868 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
> > > help
> > > Say yes here to build parallel interface support for Analog
> > > Devices:
> > > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters
> > > (ADC).
> > > + It also support iio_backended devices for AD7606B.
> > >
> > > To compile this driver as a module, choose M here: the
> > > - module will be called ad7606_parallel.
> > > + module will be called ad7606_par.
> > >
> > > config AD7606_IFACE_SPI
> > > tristate "Analog Devices AD7606 ADC driver with spi interface
> > > support"
> > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > > index 99d5ca5c2348..a753d5caa9f8 100644
> > > --- a/drivers/iio/adc/ad7606.c
> > > +++ b/drivers/iio/adc/ad7606.c
> > > @@ -21,6 +21,7 @@
> > > #include <linux/util_macros.h>
> > > #include <linux/units.h>
> > > +
> > > + /* backend manages interruptions by itself.*/
> > missing space before closing the comment (also not sure the comments adds much)
>
>
> thx, will check again
>
>
> >
> > > + if (!st->back) {
> > > + ret = wait_for_completion_timeout(&st->completion,
> > > + msecs_to_jiffies(1000));
> > > + if (!ret) {
> > > + ret = -ETIMEDOUT;
> > > + goto error_ret;
> > > + }
> > > }
> > >
> > > ret = ad7606_read_samples(st);
> > > @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > > *val = st->oversampling;
> > > return IIO_VAL_INT;
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
> > > + /* If the PWM is swinging, return the real frequency,
> > > otherwise 0 */
> > > + *val = ad7606_pwm_is_swinging(st) ?
> > > + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> > > cnvst_pwm_state.period) : 0;
> > > + return IIO_VAL_INT;
> > > }
> > > return -EINVAL;
> > > }
> > > @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> > > return ret;
> > >
> > > return 0;
> > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > + return ad7606_set_sampling_freq(st, val);
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > > struct ad7606_state *st = iio_priv(indio_dev);
> > >
> > > gpiod_set_value(st->gpio_convst, 1);
> > > - ad7606_pwm_set_swing(st);
> > >
> > > return 0;
> > > }
> > > @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > > struct ad7606_state *st = iio_priv(indio_dev);
> > >
> > > gpiod_set_value(st->gpio_convst, 0);
> > > - ad7606_pwm_set_low(st);
> > >
> > > return 0;
> > > }
> > >
> > > +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev)
> > > +{
> > > + struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > + return ad7606_pwm_set_swing(st);
> > > +}
> > > +
> > > +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev)
> > > +{
> > > + struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > + return ad7606_pwm_set_low(st);
> > > +}
> > Maybe I'm missing something but are we removing the gpiod calls?
>
>
> Well actually the pwm is meant to be used only with backend. Though it
> could be used without it, I dont think it is a very good idea because
> interrupt handling + transmission init takes quite some time, and a new
> rising edge could happen before the current samples are effectively
> transferred. However, since PWM and backend are two separate things, I
> wanted to show an usage for the PWM when introducing it, and one way to
> do it was to use it to emulate a GPIO by setting the duty cycle 100% for
> having a 1 (set_high) and 0% for having a 0 (set_low). Then on this
> patch, since we introduce iio-backend, I removed this 'mock' usage of it.
>
> But I think that I should separate the removal into an additional patch
> to avoid confusions. Or shall I just remove the mock usage from the PWM
> patch ?
>
>
Yeah, probably better (with a proper commit message explaining the reasoning)
> > > +
> > > +static int ad7606_update_scan_mode(struct iio_dev *indio_dev,
> > > + const unsigned long *scan_mask)
> > > +{
> > > + struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > + /* The update scan mode is only for iio backend compatible drivers.
> > > + * If the specific update_scan_mode is not defined in the bus ops,
> > > + * just do nothing and return 0.
> > > + */
> > > + if (st->bops->update_scan_mode)
> > > + return st->bops->update_scan_mode(indio_dev, scan_mask);
> > > + else
> > > + return 0;
> > Redundant else
>
>
> ack
>
> > > - if (ret)
> > > - return ret;
> > >
> > > + if (st->bops->iio_backend_config) {
> > > + st->bops->iio_backend_config(dev, indio_dev);
> > > + indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
> > Ignoring error code
>
>
> will handle
>
>
> >
> > > + } else {
> > > + /* Reserve the PWM use only for backend (force gpio_convst
> > > definition)*/
> > > + if (!st->gpio_convst)
> > > + return dev_err_probe(dev, -EINVAL,
> > > + "Convst pin must be defined when
> > > not in backend mode");
> > > +
> > > + init_completion(&st->completion);
> > > + ret = devm_request_threaded_irq(dev, irq,
> > > + NULL,
> > > + &ad7606_interrupt,
> > > + IRQF_TRIGGER_FALLING |
> > > IRQF_ONESHOT,
> > > + chip_info->name, indio_dev);
> > > + if (ret)
> > > + return ret;
> > > + }
> > Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend
> > device?
>
>
> No, this portion of code is only executed if convst is defined
> (conversion trigger GPIO), which is not the case if there is a backend.
>
>
> >
> > > return devm_iio_device_register(dev, indio_dev);
> > > }
> > ...
> >
> > > +#ifdef CONFIG_IIO_BACKEND
> > Not a fan of this #ifef... It's not that much code so I would just select
> > IIO_BACKEND for this driver. In fact, I don't think we can separately enable
> > IIO_BACKEND in the menuconfig menu?
>
>
> OK I can do it that way.
>
> > > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev
> > > *indio_dev)
> > > +{
> > > + struct ad7606_state *st = iio_priv(indio_dev);
> > > + unsigned int ret, c;
> > > +
> > > + st->back = devm_iio_backend_get(dev, NULL);
> > > + if (IS_ERR(st->back))
> > > + return PTR_ERR(st->back);
> > > +
> > > + /* If the device is iio_backend powered the PWM is mandatory
> > > */
> > > + if (!st->cnvst_pwm)
> > > + return -EINVAL;
> > > +
> > > + ret = devm_iio_backend_request_buffer(dev, st->back,
> > > indio_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_iio_backend_enable(dev, st->back);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + struct iio_backend_data_fmt data = {
> > > + .sign_extend = true,
> > > + .enable = true,
> > > + };
> > I would follow typical kernel coding style and have this declared at the
> > beginning of the function.
>
>
> aouch, yes !
>
>
> > > -
> > > +#ifdef CONFIG_IIO_BACKEND
> > > + struct iio_backend *back;
> > > +
> > > + /*For now, only the AD7606B is backend compatible.*/
> > > + if (chip_info->has_backend) {
> > > + back = devm_iio_backend_get(&pdev->dev, NULL);
> > > + if (IS_ERR(back))
> > > + return PTR_ERR(back);
> > > +
> > > + return ad7606_probe(&pdev->dev, 0, NULL,
> > > + chip_info,
> > > + &ad7606_bi_bops);
> > > + }
> > > +#endif
> > Not sure I follow the above? You also get the backend in
> > ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is
> > not really needed?
>
>
> The first call to devm_iio_backend_get checks if there is a backend
> available, and if so the backend bops (ad7606_bi_bops)are passed to the
> generic probe function.
>
Why not checking for the presence of the DT property? We should only get the backend
when ready for that.
- Nuno Sá
>
