On Thu, Apr 13, 2023 at 12:13 PM Nuno Sá <noname.nuno@xxxxxxxxx> 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 Thank you for your patience and for having this one reviewed again. Fabrizio Lamarque
