Re: [PATCH 3/3] iio: adc: ad7192: Clarify binding documentation

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

On Thu, 2023-04-13 at 12:47 +0200, Fabrizio Lamarque wrote:
> On Thu, Apr 13, 2023 at 12:13 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2023-04-13 at 10:36 +0200, Fabrizio Lamarque wrote:
> > > ---
> > >  .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----
> > >  drivers/iio/adc/ad7192.c                      | 18 ++++++------
> > >  2 files changed, 32 insertions(+), 14 deletions(-)
> > > 
> 
> > You should not mix bindings changes with driver changes... They should go in
> > separate patches.
> 
> > +  adi,clock-xtal:
> > +    description: |
> > +      This bit selects whether an external crystal oscillator or an
> > external
> > +      clock is applied as master (mclk) clock. It is ignored when clocks
> > +      property is not set.
> > +    type: boolean
> > +
> 
> It looks like you could use a dependency... Grep for "dependencies:" in the
> bindings folder.
> 
> > > -    st->avdd = devm_regulator_get(&spi->dev, "avdd");
> > > -    if (IS_ERR(st->avdd))
> > > -        return PTR_ERR(st->avdd);
> > > +    st->vref = devm_regulator_get(&spi->dev, "vref");
> > > +    if (IS_ERR(st->vref))
> > > +        st->vref = devm_regulator_get(&spi->dev, "avdd");
> > > +    if (IS_ERR(st->vref))
> > > +        return PTR_ERR(st->vref);
> > > 
> > I'm also not sure this will work as you expect. If no regulator is found,
> > you'll
> > still get a dummy one which means you won't ever reach the point to get
> > "avdd".
> > Look here:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2137
> > 
> > So I guess you could devm_regulator_get_optional() for "vref" and then move
> > forward to look for "avdd" if you get -ENODEV from the first call. But using
> > devm_regulator_get_optional() like this is really an __hack__ and not how
> > it's
> > supposed to be used. So maybe this is cumbersome enough to just let it be as
> > before? You can just update the description in the bindings.
> > 
> > - Nuno Sá
> 
> You are right. I missed it.
> I kindly ask you to confirm if, as per your suggestion, I should send
> a v3 patch series with the proper "fixes" tag and this last one
> changed as follows:
> 
>  - No changes on driver side (keep avdd-supply instead of vref-supply)
>  - Indicate in bindings documentation that avdd-supply is vref instead
> (with the "Phandle to reference voltage regulator")
>  - Add dependencies to yaml bindings
> 

Yeps, but note that for the bindings patch you are making distinct changes (
adding missing properties and changing one) so someone might complain. But for
me, personally, they are simple enough that can go in one patch. Just properly
document it in the commit description.

> Thank you for your patience and for having this one reviewed again.
> 

Sure, I also like to have my patches reviewed :)

- Nuno Sá