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

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

On Thu, 2023-04-13 at 10:36 +0200, Fabrizio Lamarque wrote:
> Added undocumented properties:
> 
> - adi,clock-xtal
> - adi,int-clock-output-enable
> 
> Removed clocks from required properties.
> Renamed avdd-supply to vreg-supply, while keeping backward compatibility
> (deprecated avdd-supply).
> 
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----
>  drivers/iio/adc/ad7192.c                      | 18 ++++++------
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index d521d516088b..5dc7a7eea5f9 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -32,7 +32,9 @@ properties:
> 
>    clocks:
>      maxItems: 1
> -    description: phandle to the master clock (mclk)
> +    description: |
> +      phandle to the external master clock (mclk). If not defined, internal
> +      clock is selected.
> 
>    clock-names:
>      items:
> @@ -45,7 +47,23 @@ properties:
>      description: DVdd voltage supply
> 
>    avdd-supply:
> -    description: AVdd voltage supply
> +    description: Phandle to reference voltage regulator. Use
> vref-supply instead.
> +    deprecated: true
> +
> +  vref-supply:
> +    description: Phandle to reference voltage regulator
> +
> +  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.

> +  adi,int-clock-output-enable:
> +    description: |
> +      When internal clock is selected, this bit enables clock out pin.
> +    type: boolean
> 
>    adi,rejection-60-Hz-enable:
>      description: |
> @@ -84,11 +102,9 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - clocks
> -  - clock-names
>    - interrupts
>    - dvdd-supply
> -  - avdd-supply
> +  - vref-supply
>    - spi-cpol
>    - spi-cpha
> 
> @@ -114,7 +130,7 @@ examples:
>              interrupts = <25 0x2>;
>              interrupt-parent = <&gpio>;
>              dvdd-supply = <&dvdd>;
> -            avdd-supply = <&avdd>;
> +            vref-supply = <&vref>;
> 
>              adi,refin2-pins-enable;
>              adi,rejection-60-Hz-enable;
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c

You should not mix bindings changes with driver changes... They should go in
separate patches.

> index 5a9c8898f8af..a0cac9b60ea8 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -176,7 +176,7 @@ struct ad7192_chip_info {
> 
>  struct ad7192_state {
>      const struct ad7192_chip_info    *chip_info;
> -    struct regulator        *avdd;
> +    struct regulator        *vref;
>      struct clk            *mclk;
>      u16                int_vref_mv;
>      u32                fclk;
> @@ -1000,17 +1000,19 @@ static int ad7192_probe(struct spi_device *spi)
> 
>      mutex_init(&st->lock);
> 
> -    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á