Re: [PATCH v4 7/8] iio: adc: ad9467: convert to backend framework

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

On Thu, 2023-12-21 at 17:52 +0000, Jonathan Cameron wrote:
> On Wed, 20 Dec 2023 16:34:10 +0100
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Convert the driver to use the new IIO backend framework. The device
> > functionality is expected to be the same (meaning no added or removed
> > features).
> > 
> > Also note this patch effectively breaks ABI and that's needed so we can
> > properly support this device and add needed features making use of the
> > new IIO framework.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> A few minor comments. Looks good to me.
> 
> Jonathan
> 
> > +static int ad9467_update_scan_mode(struct iio_dev *indio_dev,
> > +                                  const unsigned long *scan_mask)
> > +{
> > +       struct ad9467_state *st = iio_priv(indio_dev);
> > +       unsigned int c;
> > +       int ret;
> > +
> > +       for (c = 0; c < st->info->num_channels; c++) {
> > +               if (test_bit(c, scan_mask))
> > +                       ret = iio_backend_chan_enable(st->back, c);
> > +               else
> > +                       ret = iio_backend_chan_disable(st->back, c);
> > +
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> >  static int ad9467_reset(struct device *dev)
> > @@ -443,25 +475,63 @@ static int ad9467_reset(struct device *dev)
> >         return 0;
> >  }
> >  
> > +static int ad9467_iio_backend_get(struct ad9467_state *st)
> > +{
> > +       struct device *dev = &st->spi->dev;
> > +       struct device_node *__back;
> > +
> > +       st->back = devm_iio_backend_get_optional(&st->spi->dev, NULL);
> > +       if (IS_ERR(st->back))
> > +               return PTR_ERR(st->back);
> 
> As per the comment on previous patch I'd just get it using the normal
> function and handle PTR_ERR(-ENOENT) here as meaning we need to
> try the old way.

That makes sense. So we can add a devm_iio_backend_get_optional() API when/if we
really need one and not just for handling legacy code.

> 
> > +       if (st->back)
> > +               return 0;
> > +       /*
> > +        * if we don't get the backend using the normal API's, use the legacy
> > +        * 'adi,adc-dev' property. So we get all nodes with that property, and
> > +        * look for the one pointing at us. Then we directly lookup that fwnode
> > +        * on the backend list of registered devices. This is done so we don't
> > +        * make io-backends mandatory which would break DT ABI.
> > +        */
> > +       for_each_node_with_property(__back, "adi,adc-dev") {
> > +               struct device_node *__me;
> > +
> > +               __me = of_parse_phandle(__back, "adi,adc-dev", 0);
> > +               if (!__me)
> > +                       continue;
> > +
> > +               if (!device_match_of_node(dev, __me)) {
> > +                       of_node_put(__me);
> > +                       continue;
> > +               }
> > +
> > +               of_node_put(__me);
> > +               st->back = devm_iio_backend_get_from_fwnode_lookup(dev,
> > +                                                                 
> > of_fwnode_handle(__back));
> > +               of_node_put(__back);
> 
> If it lands first the patch
> RFC PATCH 1/4] of: Add cleanup.h based autorelease via __free(device_node)
> markings.
> will get rid of this manual handling for you for both the continue and return.
> This will make a very nice example for that :)

Yeah, and I'll happily rebase on that...

- Nuno Sá
>