[PATCH v2 0/3] iio: adc: ad7192: Functional fixes

[PATCH v2 0/3] iio: adc: ad7192: Functional fixes

[Fabrizio Lamarque]

This patch series fixes two bugs introduced in ad7192 ADC driver in
the last commits and adds related bindings documentation.

Changes in v2:
 - Obtained ad7192_state from iio_dev pointer
 - Added patch on bindings documentation

Backported patches have been tested on a custom board (ARM v7, NXP)
with kernel 5.15.

There is still an unresolved issue with the IRQ management on the
shared SPI-SDO/RDY pin causing single samples to be discarded. More
information and possible solutions here:
https://lore.kernel.org/all/CAPJMGm4GaSjD6bdqMwCr2EVZGenWzT-nCCf3BMRaD1TSfAabpA@mail.gmail.com/

In addition, this patch is required to make libiio start up correctly:
https://lore.kernel.org/all/20230330102100.17590-1-paul@crapouillou.net/

Links to v1:
- https://lore.kernel.org/linux-iio/CAPJMGm4GDVdAmwB4sHVkg78UhtVpmbCL6KT8-KbEY7cRSD5UZg@mail.gmail.com/
- https://lore.kernel.org/linux-iio/CAPJMGm4StRvJ4zTyrOb7ebo47LrR9bBuZ46p7VOxkDfwWSG=PA@mail.gmail.com/

Fabrizio Lamarque (3):
  iio: adc: ad7192: Fix null ad7192_state pointer access
  iio: adc: ad7192: Fix internal/external clock selection
  iio: adc: ad7192: Clarify binding documentation

 .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----
 drivers/iio/adc/ad7192.c                      | 26 +++++++++--------
 2 files changed, 36 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] iio: adc: ad7192: Fix null pointer dereference on probe

[Fabrizio Lamarque]

Pointer to indio_dev structure is obtained via spi_get_drvdata() at
the beginning of function ad7192_setup(), but the spi->dev->driver_data
member is not initialized, hence a NULL pointer is returned.

Fixed by changing ad7192_setup() signature to take pointer to struct
iio_dev, and get ad7192_state pointer via st = iio_priv(indio_dev);

Fixes: bd5dcdeb3fd0 iio: adc: ad7192: convert to device-managed functions
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
---
Changes in v2: obtained ad7192_state from iio_dev pointer as suggested
by Jonathan, removed Reviewed-by since the entire patch changed its
content.

 drivers/iio/adc/ad7192.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 55a6ab591016..94a9cf34a255 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
  return clock_sel;
 }

-static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
+static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
 {
- struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
+ struct ad7192_state *st = iio_priv(indio_dev);
  bool rej60_en, refin2_en;
  bool buf_en, bipolar, burnout_curr_en;
  unsigned long long scale_uv;
@@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
  }
  }

- ret = ad7192_setup(st, spi->dev.of_node);
+ ret = ad7192_setup(indio_dev, spi->dev.of_node);
  if (ret)
  return ret;

--
2.34.1

[PATCH 2/3] iio: adc: ad7192: Fix internal/external clock selection

[Fabrizio Lamarque]

Fixed wrong selection of internal clock when mclk is defined.

Resolved a logical inversion introduced in c9ec2cb328e3.

Fixes: c9ec2cb328e3 iio: adc: ad7192: use devm_clk_get_optional() for mclk
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
---
 drivers/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 94a9cf34a255..5a9c8898f8af 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
     clock_sel = AD7192_CLK_INT;

     /* use internal clock */
-    if (st->mclk) {
+    if (!st->mclk) {
         if (of_property_read_bool(np, "adi,int-clock-output-enable"))
             clock_sel = AD7192_CLK_INT_CO;
     } else {
-- 
2.34.1

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

[Fabrizio Lamarque]

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
+
+  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
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);

-    ret = regulator_enable(st->avdd);
+    ret = regulator_enable(st->vref);
     if (ret) {
-        dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
+        dev_err(&spi->dev, "Failed to enable specified VRef supply\n");
         return ret;
     }

-    ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
+    ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
     if (ret)
         return ret;

@@ -1018,7 +1020,7 @@ static int ad7192_probe(struct spi_device *spi)
     if (ret)
         return dev_err_probe(&spi->dev, ret, "Failed to enable
specified DVdd supply\n");

-    ret = regulator_get_voltage(st->avdd);
+    ret = regulator_get_voltage(st->vref);
     if (ret < 0) {
         dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
         return ret;
-- 
2.34.1

Re: [PATCH v2 1/3] iio: adc: ad7192: Fix null pointer dereference on probe

[Nuno Sá]

On Thu, 2023-04-13 at 10:28 +0200, Fabrizio Lamarque wrote:
> Pointer to indio_dev structure is obtained via spi_get_drvdata() at
> the beginning of function ad7192_setup(), but the spi->dev->driver_data
> member is not initialized, hence a NULL pointer is returned.
> 
> Fixed by changing ad7192_setup() signature to take pointer to struct
> iio_dev, and get ad7192_state pointer via st = iio_priv(indio_dev);
> 
> Fixes: bd5dcdeb3fd0 iio: adc: ad7192: convert to device-managed functions

This should be:
Fixes: bd5dcdeb3fd0 ("iio: adc: ad7192: convert to device-managed functions")

(applies to the other patches in the series).
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---

With the above fixed:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

> Changes in v2: obtained ad7192_state from iio_dev pointer as suggested
> by Jonathan, removed Reviewed-by since the entire patch changed its
> content.
> 
>  drivers/iio/adc/ad7192.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 55a6ab591016..94a9cf34a255 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>   return clock_sel;
>  }
> 
> -static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> +static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
>  {
> - struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
> + struct ad7192_state *st = iio_priv(indio_dev);
>   bool rej60_en, refin2_en;
>   bool buf_en, bipolar, burnout_curr_en;
>   unsigned long long scale_uv;
> @@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
>   }
>   }
> 
> - ret = ad7192_setup(st, spi->dev.of_node);
> + ret = ad7192_setup(indio_dev, spi->dev.of_node);
>   if (ret)
>   return ret;
> 
> --
> 2.34.1

Re: [PATCH 2/3] iio: adc: ad7192: Fix internal/external clock selection

[Nuno Sá]

On Thu, 2023-04-13 at 10:33 +0200, Fabrizio Lamarque wrote:
> Fixed wrong selection of internal clock when mclk is defined.
> 
> Resolved a logical inversion introduced in c9ec2cb328e3.
> 
> Fixes: c9ec2cb328e3 iio: adc: ad7192: use devm_clk_get_optional() for mclk

ditto...

> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
>  drivers/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 94a9cf34a255..5a9c8898f8af 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>      clock_sel = AD7192_CLK_INT;
> 
>      /* use internal clock */
> -    if (st->mclk) {
> +    if (!st->mclk) {
>          if (of_property_read_bool(np, "adi,int-clock-output-enable"))
>              clock_sel = AD7192_CLK_INT_CO;
>      } else {

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

[Nuno Sá]

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á

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

[Fabrizio Lamarque]

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

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

Fabrizio Lamarque

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

[Nuno Sá]

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á

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

[Krzysztof Kozlowski]

On 13/04/2023 10:36, Fabrizio Lamarque wrote:
> Added undocumented properties:

Use imperative.
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> - adi,clock-xtal
> - adi,int-clock-output-enable
> 
> Removed clocks from required properties.

Why?

> Renamed avdd-supply to vreg-supply, while keeping backward compatibility
> (deprecated avdd-supply).

Why?

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You decided to ignore quite a lot of entries, but most important - also
lists, so it won't even be tested.


> 
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----

Bindings are always separate patches.

>  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.

Corrupted patch.

Run checkpatch, test your patches with dt_binding_check. This really
needs a lot of work.

Best regards,
Krzysztof

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

[Fabrizio Lamarque]

On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
> > Added undocumented properties:
>
> Use imperative.
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> >
> > - adi,clock-xtal
> > - adi,int-clock-output-enable
> >
> > Removed clocks from required properties.
>
> Why?

Current documentation does not follow existing source code implementation.
Patch 2/3 fixes a commit that caused an unwanted logical inversion and
thus prevented the use of  external clock/crystal.
The driver has been originally designed to operate with the internal
clock when clocks property is omitted.

I thought the reason is clear from patch 2, but, as Nuno Sá already
suggested, I will describe the reasons in full again, each time I post
a revised patch set, even if it is quite verbose.

>
> > Renamed avdd-supply to vreg-supply, while keeping backward compatibility
> > (deprecated avdd-supply).
>
> Why?

From AD7192 datasheet, you may see AVDD pin/voltage has no
relationship with VREF pin/voltage.
avdd-supply name is misleading, since it is treated in code as AVDD
pin and iio reference voltage instead.
The option to change the regulator name or add a third regulator would
have broken compatibility.
Other ADI drivers already have the vref-supply property in place.

Here again I partially left the reasons in the first thread, sorry.

In any case I will remove this change on a revised patch set.
I will leave the avdd-supply name but I'll change the description in
documentation.

>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>

The single change on documentation will be prepended by
dt-bindings: iio: ad7192:
The invalid change I suggested intended to change avdd to vref name in
the driver too.
I misinterpreted the meaning of a single "logical change", sorry.

> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
>
> You decided to ignore quite a lot of entries, but most important - also
> lists, so it won't even be tested.

The patch is indeed based on the latest version; however the driver
maintainer does not work in ADI any more; the first time I sent a
message I got the email bounced back.
I see I omitted a necessary step though.

You are right in saying that I did not follow carefully the
instructions provided, but it was not deliberate. It's the first time
I am trying to send back the changes.
I appreciate the feedback and corrections; in the next patch set I
will try to remedy everything you indicated.

>
> > ---
> >  .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----
>
> Bindings are always separate patches.
>
> Corrupted patch.
>
> Run checkpatch, test your patches with dt_binding_check. This really
> needs a lot of work.

I kindly ask you whether the entire (corrected) change on the
documentation file only (without any change on the driver source code)
could be accepted as a single patch.

Unless I was wrong in doing copy/paste, the only feedback I got from
the tests is a warning message telling that the changes to
documentation should be isolated from source code changes.
I will make sure these tests pass without any warning.

Thank you and best regards,
Fabrizio Lamarque

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

[Krzysztof Kozlowski]

On 13/04/2023 18:07, Fabrizio Lamarque wrote:
> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
>>> Added undocumented properties:
>>
>> Use imperative.
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>>
>>> - adi,clock-xtal
>>> - adi,int-clock-output-enable
>>>
>>> Removed clocks from required properties.
>>
>> Why?
> 
> Current documentation does not follow existing source code implementation.

Bindings describe hardware, not current implementation.

> Patch 2/3 fixes a commit that caused an unwanted logical inversion and
> thus prevented the use of  external clock/crystal.

OK

> The driver has been originally designed to operate with the internal
> clock when clocks property is omitted.

Not really a reason to do it. Reason could be - hardware does not always
need clock input.

> 
> I thought the reason is clear from patch 2, but, as Nuno Sá already
> suggested, I will describe the reasons in full again, each time I post
> a revised patch set, even if it is quite verbose.

Your commit must answer to why you are doing it. What you are doing is
easily visible from the diff. Rephrase commit msg to explain it and add
proper rationale (hardware related, not driver).

> 
>>
>>> Renamed avdd-supply to vreg-supply, while keeping backward compatibility
>>> (deprecated avdd-supply).
>>
>> Why?
> 
> From AD7192 datasheet, you may see AVDD pin/voltage has no
> relationship with VREF pin/voltage.
> avdd-supply name is misleading, since it is treated in code as AVDD
> pin and iio reference voltage instead.

Then why removing AVDD? It is a supply, as you said, thus bindings
should describe it. I don't understand why AVDD is being deprecated.

> The option to change the regulator name or add a third regulator would
> have broken compatibility.
> Other ADI drivers already have the vref-supply property in place.
> 
> Here again I partially left the reasons in the first thread, sorry.
> 
> In any case I will remove this change on a revised patch set.
> I will leave the avdd-supply name but I'll change the description in
> documentation.
> 
>>
>> Use subject prefixes matching the subsystem (which you can get for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching).
>>
> 
> The single change on documentation will be prepended by
> dt-bindings: iio: ad7192:
> The invalid change I suggested intended to change avdd to vref name in
> the driver too.
> I misinterpreted the meaning of a single "logical change", sorry.
> 
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC.  It might happen, that command when run on an older
>> kernel, gives you outdated entries.  Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>> You decided to ignore quite a lot of entries, but most important - also
>> lists, so it won't even be tested.
> 
> The patch is indeed based on the latest version; however the driver
> maintainer does not work in ADI any more; the first time I sent a
> message I got the email bounced back.
> I see I omitted a necessary step though.

Just use get_maintainers.pl.

> 
> You are right in saying that I did not follow carefully the
> instructions provided, but it was not deliberate. It's the first time
> I am trying to send back the changes.
> I appreciate the feedback and corrections; in the next patch set I
> will try to remedy everything you indicated.
> 
>>
>>> ---
>>>  .../bindings/iio/adc/adi,ad7192.yaml          | 28 +++++++++++++++----
>>
>> Bindings are always separate patches.
>>
>> Corrupted patch.
>>
>> Run checkpatch, test your patches with dt_binding_check. This really
>> needs a lot of work.
> 
> I kindly ask you whether the entire (corrected) change on the
> documentation file only (without any change on the driver source code)
> could be accepted as a single patch.

I don't understand the question. Each change should be one logical
change, but bindings are not related to the driver.

> 
> Unless I was wrong in doing copy/paste, the only feedback I got from
> the tests is a warning message telling that the changes to
> documentation should be isolated from source code changes.

Yep... and this should stop you from sending it.

> I will make sure these tests pass without any warning.

The patch is corrupted. Try to apply it from mailing lists...

Best regards,
Krzysztof

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

[Fabrizio Lamarque]

On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/04/2023 18:07, Fabrizio Lamarque wrote:
> > On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
> >
> > Current documentation does not follow existing source code implementation.
>
> Bindings describe hardware, not current implementation.

OK, I'll keep in mind the perspective while describing the change.
The bug corrected by 2/2 was found because the external crystal
oscillator did not work and the internal, fixed clock was always used.
Before the correction, the  driver did not load correctly without the
clocks property. At the same time, the specified frequency was ignored
and the fixed, internal frequency was used instead.
After the correction, I found the documentation telling that clock
property is mandatory, while omitting it is the way to use the
internal, fixed frequency, clock.
The documentation did not explain how to choose between the internal
and external oscillator, but the driver was already designed to
implement the choice.

> > The driver has been originally designed to operate with the internal
> > clock when clocks property is omitted.
>
> Not really a reason to do it. Reason could be - hardware does not always
> need clock input.

I hope the change in perspective will be enough. The external clock is
mandatory for some applications.
The internal clock might be required for others.

>
> >
> > I thought the reason is clear from patch 2, but, as Nuno Sá already
> > suggested, I will describe the reasons in full again, each time I post
> > a revised patch set, even if it is quite verbose.
>
> Your commit must answer to why you are doing it. What you are doing is
> easily visible from the diff. Rephrase commit msg to explain it and add
> proper rationale (hardware related, not driver).

I am really just suggesting to align the documentation with the
driver, since the driver operates the hardware as expected (after the
two regressions fixes).
Without this change, one should read the source code to understand
which clock is used and when, which bindings have to be applied, and
find that documentation mandates an (already) optional property.

>
> >
> >>
> >>> Renamed avdd-supply to vreg-supply, while keeping backward compatibility
> >>> (deprecated avdd-supply).
> >>
> >> Why?
> >
> > From AD7192 datasheet, you may see AVDD pin/voltage has no
> > relationship with VREF pin/voltage.
> > avdd-supply name is misleading, since it is treated in code as AVDD
> > pin and iio reference voltage instead.
>
> Then why removing AVDD? It is a supply, as you said, thus bindings
> should describe it. I don't understand why AVDD is being deprecated.

Please ignore the deprecation and the additional avdd-supply property.
It will be removed from the next patch version.

I won't be able to provide any patch here without breaking compatibility.
BTW, avdd-supply has nothing to do with vref on the hardware.
In the driver avdd-supply voltage is used as if it were the reference
voltage (VREF pin).
This change will be removed from this patch set, maybe the driver
author could provide something acceptable.
The idea to deprecate it has been suggested in the thread related to
the previous patch version.

> >
> > I kindly ask you whether the entire (corrected) change on the
> > documentation file only (without any change on the driver source code)
> > could be accepted as a single patch.
>
> I don't understand the question. Each change should be one logical
> change, but bindings are not related to the driver.

The question came after this:

On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 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.

I really need to send a proper, complete and acceptable v3 patch set,
or drop patch 3/3 from the set.

Would you accept the change to adi,ad7192.yaml file alone with both
the change in description of avdd-supply and the additional missing
properties?
Is "Phandle to reference voltage regulator" as a description for
avdd-supply acceptable on your side?

In case you feel the proposed adjustments are not enough or changes in
documentation are unworthy, I feel it would be better to leave them to
the driver maintainers.
Otherwise, I'll do my best (according to my limited expertise, to the
available time and the acceptable costs for the company I work for) to
provide something acceptable.

Thank you again.

Best regards,
Fabrizio Lamarque

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

[Krzysztof Kozlowski]

On 13/04/2023 20:19, Fabrizio Lamarque wrote:
> On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 13/04/2023 18:07, Fabrizio Lamarque wrote:
>>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
>>>
>>> Current documentation does not follow existing source code implementation.
>>
>> Bindings describe hardware, not current implementation.
> 
> OK, I'll keep in mind the perspective while describing the change.
> The bug corrected by 2/2 was found because the external crystal
> oscillator did not work and the internal, fixed clock was always used.
> Before the correction, the  driver did not load correctly without the
> clocks property. At the same time, the specified frequency was ignored
> and the fixed, internal frequency was used instead.
> After the correction, I found the documentation telling that clock
> property is mandatory, while omitting it is the way to use the
> internal, fixed frequency, clock.
> The documentation did not explain how to choose between the internal
> and external oscillator, but the driver was already designed to
> implement the choice.
> 
>>> The driver has been originally designed to operate with the internal
>>> clock when clocks property is omitted.
>>
>> Not really a reason to do it. Reason could be - hardware does not always
>> need clock input.
> 
> I hope the change in perspective will be enough. The external clock is
> mandatory for some applications.
> The internal clock might be required for others.

I told you that reason you wrote is not enough and you answer "in
perspective will be enough". Wait, what? I don't understand it at all. I
gave you example reason. If you do not like it, find other reasons which
refer to the actual device, not to the specific implementation.

>>>
>>> I thought the reason is clear from patch 2, but, as Nuno Sá already
>>> suggested, I will describe the reasons in full again, each time I post
>>> a revised patch set, even if it is quite verbose.
>>
>> Your commit must answer to why you are doing it. What you are doing is
>> easily visible from the diff. Rephrase commit msg to explain it and add
>> proper rationale (hardware related, not driver).
> 
> I am really just suggesting to align the documentation with the
> driver, since the driver operates the hardware as expected (after the
> two regressions fixes).

Does it mean you are not going to answer to "why?"? I cannot accept such
commits. That's the basics of software development and versioning. It's
not even Linux kernel related...


> Without this change, one should read the source code to understand
> which clock is used and when, which bindings have to be applied, and
> find that documentation mandates an (already) optional property.

How is it related? I did not refer whether change is reasonable itself
or not. I said you commit msg is very poor and you must answer to "why".
Not to "what".

(...)

> 
>>>
>>> I kindly ask you whether the entire (corrected) change on the
>>> documentation file only (without any change on the driver source code)
>>> could be accepted as a single patch.
>>
>> I don't understand the question. Each change should be one logical
>> change, but bindings are not related to the driver.
> 
> The question came after this:
> 
> On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>>> 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.
> 
> I really need to send a proper, complete and acceptable v3 patch set,
> or drop patch 3/3 from the set.
> 
> Would you accept the change to adi,ad7192.yaml file alone with both
> the change in description of avdd-supply and the additional missing
> properties?

Do you mean by this changing bindings without changing driver? Then
depends on the context. The driver must implement bindings, so you
should not send patches which break the implementation of interface.


> Is "Phandle to reference voltage regulator" as a description for
> avdd-supply acceptable on your side?

Sorry, how is this related to our topic? Anyway, drop "phandle to",
because you should describe hardware, not syntax of DTS.

> In case you feel the proposed adjustments are not enough or changes in
> documentation are unworthy, I feel it would be better to leave them to
> the driver maintainers.

I don't understand. I wrote nowhere that changes are unworthy. I pointed
out specific issues which need to be fixed. Can we focus on specific
issues, not on some other unrelated parts?

> Otherwise, I'll do my best (according to my limited expertise, to the
> available time and the acceptable costs for the company I work for) to
> provide something acceptable.
Best regards,
Krzysztof

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

[Fabrizio Lamarque]

On Fri, Apr 14, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/04/2023 20:19, Fabrizio Lamarque wrote:
> > On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 13/04/2023 18:07, Fabrizio Lamarque wrote:
> >>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>>
> >>>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
> >>>
> >>> Current documentation does not follow existing source code implementation.
> >>
> >> Bindings describe hardware, not current implementation.
> >
> >>> The driver has been originally designed to operate with the internal
> >>> clock when clocks property is omitted.
> >>
> >> Not really a reason to do it. Reason could be - hardware does not always
> >> need clock input.
> >
> > I hope the change in perspective will be enough. The external clock is
> > mandatory for some applications.
> > The internal clock might be required for others.
>
> I told you that reason you wrote is not enough and you answer "in
> perspective will be enough". Wait, what? I don't understand it at all. I
> gave you example reason. If you do not like it, find other reasons which
> refer to the actual device, not to the specific implementation.
>
> >>>
> >>> I thought the reason is clear from patch 2, but, as Nuno Sá already
> >>> suggested, I will describe the reasons in full again, each time I post
> >>> a revised patch set, even if it is quite verbose.
> >>
> >> Your commit must answer to why you are doing it. What you are doing is
> >> easily visible from the diff. Rephrase commit msg to explain it and add
> >> proper rationale (hardware related, not driver).
> >
> > I am really just suggesting to align the documentation with the
> > driver, since the driver operates the hardware as expected (after the
> > two regressions fixes).
>
> Does it mean you are not going to answer to "why?"? I cannot accept such
> commits. That's the basics of software development and versioning. It's
> not even Linux kernel related...
>

I already wrote that the hardware clock might be generated internally
and hence not required, but also that an external crystal/oscillator
is suggested in the datasheet for better performance.
There shall be a method to choose between the crystal, the external
oscillator and the internal oscillator.
This is what I was expecting to write on revised commit information,
and this was the hopefully accepted "perspective change".

I am really sorry but I do not understand what more I should say here.
I appreciate your explanations and the fact you are taking your time
to give directions to an obviously inexperienced developer, but my
lack of understanding is perhaps related to what I called
"perspective" on which comes first.
If the above sentence is not enough, I will drop this last patch.
I did not change the driver at all. The documentation is wrong (where
it states the clocks property is mandatory) and it was missing
information on how to choose between the clocks from the beginning.

>
> > Without this change, one should read the source code to understand
> > which clock is used and when, which bindings have to be applied, and
> > find that documentation mandates an (already) optional property.
>
> How is it related? I did not refer whether change is reasonable itself
> or not. I said you commit msg is very poor and you must answer to "why".
> Not to "what".
>
> (...)
>
> >
> >>>
> >>> I kindly ask you whether the entire (corrected) change on the
> >>> documentation file only (without any change on the driver source code)
> >>> could be accepted as a single patch.
> >>
> >> I don't understand the question. Each change should be one logical
> >> change, but bindings are not related to the driver.
> >
> > The question came after this:
> >
> > On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> >>> 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.
> >
> > I really need to send a proper, complete and acceptable v3 patch set,
> > or drop patch 3/3 from the set.
> >
> > Would you accept the change to adi,ad7192.yaml file alone with both
> > the change in description of avdd-supply and the additional missing
> > properties?
>
> Do you mean by this changing bindings without changing driver? Then
> depends on the context. The driver must implement bindings, so you
> should not send patches which break the implementation of interface.
>

Perhaps I failed to explain it from the beginning.
I said "the documentation does not follow the driver" but I understand
I should have seen it the other way round.

Actual documentation does not allow the use of the internal oscillator
or the external crystal.
By strictly following the documentation I could only use the external
oscillator option (neither internal nor external crystal oscillator).
The implementation already had in place more binding options than the
documented ones, so I am not suggesting to break anything.

>
> > Is "Phandle to reference voltage regulator" as a description for
> > avdd-supply acceptable on your side?
>
> Sorry, how is this related to our topic? Anyway, drop "phandle to",
> because you should describe hardware, not syntax of DTS.

Ok.
 - Is it acceptable to change the description of property
"avdd-supply" from "AVdd voltage supply" to "VRef voltage supply"?
 - It is clear that AVDD pin (almost always tied with DVDD pin with a
passive filter in the middle) on hardware is not VREF pin, but
property avdd-supply is treated by the implementation as if it were
VREF (it's just a matter of misleading name)? I am asking this because
any (perhaps) formally correct change would break compatibility with
existing implementations and, according to previous discussions with
driver maintainers, I am not going to suggest binding name changes any
more. In case of doubt, I will skip this change in description until a
better solution is found.
 - In the hope to submit a patch that does not require additional
corrections, is a single patch acceptable with both a change in
description and with the added missing properties (already existing in
implementation), or should it be split in two?

Best regards,
Fabrizio Lamarque

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

[Jonathan Cameron]

On Fri, 14 Apr 2023 12:42:08 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> On Fri, Apr 14, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 13/04/2023 20:19, Fabrizio Lamarque wrote:  
> > > On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> > >>
> > >> On 13/04/2023 18:07, Fabrizio Lamarque wrote:  
> > >>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> > >>>>
> > >>>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:  
> > >>>
> > >>> Current documentation does not follow existing source code implementation.  
> > >>
> > >> Bindings describe hardware, not current implementation.  
> > >  
> > >>> The driver has been originally designed to operate with the internal
> > >>> clock when clocks property is omitted.  
> > >>
> > >> Not really a reason to do it. Reason could be - hardware does not always
> > >> need clock input.  
> > >
> > > I hope the change in perspective will be enough. The external clock is
> > > mandatory for some applications.
> > > The internal clock might be required for others.  
> >
> > I told you that reason you wrote is not enough and you answer "in
> > perspective will be enough". Wait, what? I don't understand it at all. I
> > gave you example reason. If you do not like it, find other reasons which
> > refer to the actual device, not to the specific implementation.

Just leave it vague - we don't need to talk about specific apps. If someone
wired a clock, they probably want that clock an will describe it in their
DTS.

An internal clock is available and may be used if no external clock is provided.

> >  
> > >>>
> > >>> I thought the reason is clear from patch 2, but, as Nuno Sá already
> > >>> suggested, I will describe the reasons in full again, each time I post
> > >>> a revised patch set, even if it is quite verbose.  
> > >>
> > >> Your commit must answer to why you are doing it. What you are doing is
> > >> easily visible from the diff. Rephrase commit msg to explain it and add
> > >> proper rationale (hardware related, not driver).  
> > >
> > > I am really just suggesting to align the documentation with the
> > > driver, since the driver operates the hardware as expected (after the
> > > two regressions fixes).  
> >
> > Does it mean you are not going to answer to "why?"? I cannot accept such
> > commits. That's the basics of software development and versioning. It's
> > not even Linux kernel related...
> >  
> 
> I already wrote that the hardware clock might be generated internally
> and hence not required, but also that an external crystal/oscillator
> is suggested in the datasheet for better performance.
> There shall be a method to choose between the crystal, the external
> oscillator and the internal oscillator.
> This is what I was expecting to write on revised commit information,
> and this was the hopefully accepted "perspective change".
> 
> I am really sorry but I do not understand what more I should say here.
> I appreciate your explanations and the fact you are taking your time
> to give directions to an obviously inexperienced developer, but my
> lack of understanding is perhaps related to what I called
> "perspective" on which comes first.
> If the above sentence is not enough, I will drop this last patch.
> I did not change the driver at all. The documentation is wrong (where
> it states the clocks property is mandatory) and it was missing
> information on how to choose between the clocks from the beginning.

A simple statement that there is an internal clock that may be used if
no external clock is wired up should be sufficient.  I don't think we
need to talk about precision of clocks etc vs cost of board built and
now that drives a design choice.

> 
> >  
> > > Without this change, one should read the source code to understand
> > > which clock is used and when, which bindings have to be applied, and
> > > find that documentation mandates an (already) optional property.  
> >
> > How is it related? I did not refer whether change is reasonable itself
> > or not. I said you commit msg is very poor and you must answer to "why".
> > Not to "what".
> >
> > (...)
> >  
> > >  
> > >>>
> > >>> I kindly ask you whether the entire (corrected) change on the
> > >>> documentation file only (without any change on the driver source code)
> > >>> could be accepted as a single patch.  
> > >>
> > >> I don't understand the question. Each change should be one logical
> > >> change, but bindings are not related to the driver.  
> > >
> > > The question came after this:
> > >
> > > On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > >>> 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.  
> > >
> > > I really need to send a proper, complete and acceptable v3 patch set,
> > > or drop patch 3/3 from the set.
> > >
> > > Would you accept the change to adi,ad7192.yaml file alone with both
> > > the change in description of avdd-supply and the additional missing
> > > properties?  
> >
> > Do you mean by this changing bindings without changing driver? Then
> > depends on the context. The driver must implement bindings, so you
> > should not send patches which break the implementation of interface.
> >  
> 
> Perhaps I failed to explain it from the beginning.
> I said "the documentation does not follow the driver" but I understand
> I should have seen it the other way round.

No need to mention the driver at all for the DT binding patch.
You are documenting what could be wired.  We shouldn't care for DT about
what he driver happens to implement today.

> 
> Actual documentation does not allow the use of the internal oscillator
> or the external crystal.
> By strictly following the documentation I could only use the external
> oscillator option (neither internal nor external crystal oscillator).
> The implementation already had in place more binding options than the
> documented ones, so I am not suggesting to break anything.
> 
> >  
> > > Is "Phandle to reference voltage regulator" as a description for
> > > avdd-supply acceptable on your side?  
> >
> > Sorry, how is this related to our topic? Anyway, drop "phandle to",
> > because you should describe hardware, not syntax of DTS.  
> 
> Ok.
>  - Is it acceptable to change the description of property
> "avdd-supply" from "AVdd voltage supply" to "VRef voltage supply"?
No.

We have a slightly complex case of not wanting to break old drivers, but
the binding should be correct none the less.

Binding wise:
* Leave avdd-supply alone.
* Add a new vref-supply entry and list it as required. This is the Binding
  'bug fix' and there should be an appropriate fixes tag.

No need at all for the binding to describe some old buggy driver behaviour.

Driver wise. - with comments on why we are doing this as we are papering
over a bug that will be seen if someone has a DT that worked with the old
code.  That's fine as long as we document that we are doing it.

* Always get vadd-supply (not optional) - we may get a dummy regulator.
  That is fine here.
* Try to use vref-supply first as an optional supply.
* If that fails, then 'use' avdd as if it it were vref. with nice obvious
  comment that this is happening an perhaps even a warning print
  "Warning: Using avdd in place of vref. Likely an old DTS".

Jonathan


>  - It is clear that AVDD pin (almost always tied with DVDD pin with a
> passive filter in the middle) on hardware is not VREF pin, but
> property avdd-supply is treated by the implementation as if it were
> VREF (it's just a matter of misleading name)? I am asking this because
> any (perhaps) formally correct change would break compatibility with
> existing implementations and, according to previous discussions with
> driver maintainers, I am not going to suggest binding name changes any
> more. In case of doubt, I will skip this change in description until a
> better solution is found.
>  - In the hope to submit a patch that does not require additional
> corrections, is a single patch acceptable with both a change in
> description and with the added missing properties (already existing in
> implementation), or should it be split in two?
> 
> Best regards,
> Fabrizio Lamarque

Re: [PATCH 2/3] iio: adc: ad7192: Fix internal/external clock selection

[Jonathan Cameron]

On Thu, 13 Apr 2023 10:33:32 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> Fixed wrong selection of internal clock when mclk is defined.
> 
> Resolved a logical inversion introduced in c9ec2cb328e3.
> 
> Fixes: c9ec2cb328e3 iio: adc: ad7192: use devm_clk_get_optional() for mclk
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>

Version should apply to whole series.

git format-patch -v2 
is a good way to get this right automatically.

Patch is fine subject to the fixes tag fix that Nuno pointed out.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 94a9cf34a255..5a9c8898f8af 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>      clock_sel = AD7192_CLK_INT;
> 
>      /* use internal clock */
> -    if (st->mclk) {
> +    if (!st->mclk) {
>          if (of_property_read_bool(np, "adi,int-clock-output-enable"))
>              clock_sel = AD7192_CLK_INT_CO;
>      } else {

Re: [PATCH v2 1/3] iio: adc: ad7192: Fix null pointer dereference on probe

[Jonathan Cameron]

On Thu, 13 Apr 2023 10:28:11 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> Pointer to indio_dev structure is obtained via spi_get_drvdata() at
> the beginning of function ad7192_setup(), but the spi->dev->driver_data
> member is not initialized, hence a NULL pointer is returned.
> 
> Fixed by changing ad7192_setup() signature to take pointer to struct
> iio_dev, and get ad7192_state pointer via st = iio_priv(indio_dev);
> 
> Fixes: bd5dcdeb3fd0 iio: adc: ad7192: convert to device-managed functions
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---
> Changes in v2: obtained ad7192_state from iio_dev pointer as suggested
> by Jonathan, removed Reviewed-by since the entire patch changed its
> content.

Great. Please just tidy up the Fixes tag formatting and send a v3.

Note that we are very late in current cycle, so I'll only queue these
up now for after 6.4-rc1 is available.  They'll get pulled back into stable
trees after that (in about 3-4 weeks time).

Thanks,

Jonathan

> 
>  drivers/iio/adc/ad7192.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 55a6ab591016..94a9cf34a255 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>   return clock_sel;
>  }
> 
> -static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> +static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
>  {
> - struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
> + struct ad7192_state *st = iio_priv(indio_dev);
>   bool rej60_en, refin2_en;
>   bool buf_en, bipolar, burnout_curr_en;
>   unsigned long long scale_uv;
> @@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
>   }
>   }
> 
> - ret = ad7192_setup(st, spi->dev.of_node);
> + ret = ad7192_setup(indio_dev, spi->dev.of_node);
>   if (ret)
>   return ret;
> 
> --
> 2.34.1

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

[Fabrizio Lamarque]

On Sat, Apr 15, 2023 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 14 Apr 2023 12:42:08 +0200
> Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:
>
> > On Fri, Apr 14, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On 13/04/2023 20:19, Fabrizio Lamarque wrote:
> > > > On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >>
> > > >> On 13/04/2023 18:07, Fabrizio Lamarque wrote:
> > > >>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >>>>
> > > >>>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:
> > > >>>
> > > >>> Current documentation does not follow existing source code implementation.
> > > >>
> > > >> Bindings describe hardware, not current implementation.
> > > >
> > > >>> The driver has been originally designed to operate with the internal
> > > >>> clock when clocks property is omitted.
> > > >>
> > > >> Not really a reason to do it. Reason could be - hardware does not always
> > > >> need clock input.
> > > >
> > > > I hope the change in perspective will be enough. The external clock is
> > > > mandatory for some applications.
> > > > The internal clock might be required for others.
> > >
> > > I told you that reason you wrote is not enough and you answer "in
> > > perspective will be enough". Wait, what? I don't understand it at all. I
> > > gave you example reason. If you do not like it, find other reasons which
> > > refer to the actual device, not to the specific implementation.
>
> Just leave it vague - we don't need to talk about specific apps. If someone
> wired a clock, they probably want that clock an will describe it in their
> DTS.
>
> An internal clock is available and may be used if no external clock is provided.
>
> > >
> > > >>>
> > > >>> I thought the reason is clear from patch 2, but, as Nuno Sá already
> > > >>> suggested, I will describe the reasons in full again, each time I post
> > > >>> a revised patch set, even if it is quite verbose.
> > > >>
> > > >> Your commit must answer to why you are doing it. What you are doing is
> > > >> easily visible from the diff. Rephrase commit msg to explain it and add
> > > >> proper rationale (hardware related, not driver).
> > > >
> > > > I am really just suggesting to align the documentation with the
> > > > driver, since the driver operates the hardware as expected (after the
> > > > two regressions fixes).
> > >
> > > Does it mean you are not going to answer to "why?"? I cannot accept such
> > > commits. That's the basics of software development and versioning. It's
> > > not even Linux kernel related...
> > >
> >
> > I already wrote that the hardware clock might be generated internally
> > and hence not required, but also that an external crystal/oscillator
> > is suggested in the datasheet for better performance.
> > There shall be a method to choose between the crystal, the external
> > oscillator and the internal oscillator.
> > This is what I was expecting to write on revised commit information,
> > and this was the hopefully accepted "perspective change".
> >
> > I am really sorry but I do not understand what more I should say here.
> > I appreciate your explanations and the fact you are taking your time
> > to give directions to an obviously inexperienced developer, but my
> > lack of understanding is perhaps related to what I called
> > "perspective" on which comes first.
> > If the above sentence is not enough, I will drop this last patch.
> > I did not change the driver at all. The documentation is wrong (where
> > it states the clocks property is mandatory) and it was missing
> > information on how to choose between the clocks from the beginning.
>
> A simple statement that there is an internal clock that may be used if
> no external clock is wired up should be sufficient.  I don't think we
> need to talk about precision of clocks etc vs cost of board built and
> now that drives a design choice.
>
> >
> > >
> > > > Without this change, one should read the source code to understand
> > > > which clock is used and when, which bindings have to be applied, and
> > > > find that documentation mandates an (already) optional property.
> > >
> > > How is it related? I did not refer whether change is reasonable itself
> > > or not. I said you commit msg is very poor and you must answer to "why".
> > > Not to "what".
> > >
> > > (...)
> > >
> > > >
> > > >>>
> > > >>> I kindly ask you whether the entire (corrected) change on the
> > > >>> documentation file only (without any change on the driver source code)
> > > >>> could be accepted as a single patch.
> > > >>
> > > >> I don't understand the question. Each change should be one logical
> > > >> change, but bindings are not related to the driver.
> > > >
> > > > The question came after this:
> > > >
> > > > On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >>> 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.
> > > >
> > > > I really need to send a proper, complete and acceptable v3 patch set,
> > > > or drop patch 3/3 from the set.
> > > >
> > > > Would you accept the change to adi,ad7192.yaml file alone with both
> > > > the change in description of avdd-supply and the additional missing
> > > > properties?
> > >
> > > Do you mean by this changing bindings without changing driver? Then
> > > depends on the context. The driver must implement bindings, so you
> > > should not send patches which break the implementation of interface.
> > >
> >
> > Perhaps I failed to explain it from the beginning.
> > I said "the documentation does not follow the driver" but I understand
> > I should have seen it the other way round.
>
> No need to mention the driver at all for the DT binding patch.
> You are documenting what could be wired.  We shouldn't care for DT about
> what he driver happens to implement today.

Thank you once again for your corrections, suggestions and patience.
I hope to be able to provide a formally corrected patch set. Before
sending v3, would you please have a look at this and check if it
correctly responds to your expectations or if there is still something
to adjust?
(in case it would be cleaner to send a v3 patch and wait for feedback
instead, feel free to ask)

The patch set will be then split in 5 total commits.
 - iio: adc: ad7192: Fix null ad7192_state pointer access
 - iio: adc: ad7192: Fix internal/external clock selection
 - iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source
 - dt-bindings: iio: ad7192: Add mandatory reference voltage source
 - dt-bindings: iio: ad7192: Allow selection of clock modes

Patches "iio: adc: ad7192: Use VRef instead of AVdd as reference
voltage source" and "iio: adc: ad7192: Use VRef instead of AVdd as
reference voltage source" and will be a fix against b581f748cce0
("staging: iio: adc: ad7192: move out of staging").

First commit message on bindings will be:

Add required reference voltage (VRef) supply regulator.
AD7192 requires three independent voltage sources: DVdd, AVdd and VRef
(on REFINx pin pairs).

Some questions:
 - Is the FIxes tag referenced commit acceptable, or should it be
explained? If so, what should I write?
 - Is the commit message acceptable?

>
> >
> > Actual documentation does not allow the use of the internal oscillator
> > or the external crystal.
> > By strictly following the documentation I could only use the external
> > oscillator option (neither internal nor external crystal oscillator).
> > The implementation already had in place more binding options than the
> > documented ones, so I am not suggesting to break anything.
> >
> > >
> > > > Is "Phandle to reference voltage regulator" as a description for
> > > > avdd-supply acceptable on your side?
> > >
> > > Sorry, how is this related to our topic? Anyway, drop "phandle to",
> > > because you should describe hardware, not syntax of DTS.
> >
> > Ok.
> >  - Is it acceptable to change the description of property
> > "avdd-supply" from "AVdd voltage supply" to "VRef voltage supply"?
> No.
>
> We have a slightly complex case of not wanting to break old drivers, but
> the binding should be correct none the less.
>
> Binding wise:
> * Leave avdd-supply alone.
> * Add a new vref-supply entry and list it as required. This is the Binding
>   'bug fix' and there should be an appropriate fixes tag.
>
> No need at all for the binding to describe some old buggy driver behaviour.
>
> Driver wise. - with comments on why we are doing this as we are papering
> over a bug that will be seen if someone has a DT that worked with the old
> code.  That's fine as long as we document that we are doing it.
>
> * Always get vadd-supply (not optional) - we may get a dummy regulator.
>   That is fine here.
> * Try to use vref-supply first as an optional supply.
> * If that fails, then 'use' avdd as if it it were vref. with nice obvious
>   comment that this is happening an perhaps even a warning print
>   "Warning: Using avdd in place of vref. Likely an old DTS".

Here is the change I am backporting and testing on my platform:

Subject: [PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as
 reference voltage source

Add missing vref-supply and fix avdd-supply used as if it were vref.

AD7192 requires three independent voltage sources, digital supply (on
pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
alternate pin pair REFIN1 or REFIN2).

Emit a warning message when AVdd is used in place of VRef for backwards
compatibility.

Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
---
 drivers/iio/adc/ad7192.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 5a9c8898f8af..4ac6843b7c23 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -177,6 +177,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;
@@ -1014,11 +1015,30 @@ static int ad7192_probe(struct spi_device *spi)
     if (ret)
         return ret;

+    st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+    if (!IS_ERR(st->vref)) {
+        ret = regulator_enable(st->vref);
+        if (ret) {
+            dev_err(&spi->dev, "Failed to enable specified VRef supply\n");
+            return ret;
+        }
+
+        ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable,
st->vref);
+        if (ret)
+            return ret;
+    }
+
     ret = devm_regulator_get_enable(&spi->dev, "dvdd");
     if (ret)
         return dev_err_probe(&spi->dev, ret, "Failed to enable
specified DVdd supply\n");

-    ret = regulator_get_voltage(st->avdd);
+
+    if (!IS_ERR(st->vref)) {
+        ret = regulator_get_voltage(st->vref);
+    } else {
+        dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an
old DTS\n");
+        ret = regulator_get_voltage(st->avdd);
+    }
     if (ret < 0) {
         dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
         return ret;
-- 
2.34.1

I kindly ask you this advice:
 - when reference voltage is read from AVdd, should I set the vref
pointer to NULL, or is IS_ERR check acceptable as proposed in the
patch?

Best regards,
Fabrizio Lamarque

>
> Jonathan
>
>
> >  - It is clear that AVDD pin (almost always tied with DVDD pin with a
> > passive filter in the middle) on hardware is not VREF pin, but
> > property avdd-supply is treated by the implementation as if it were
> > VREF (it's just a matter of misleading name)? I am asking this because
> > any (perhaps) formally correct change would break compatibility with
> > existing implementations and, according to previous discussions with
> > driver maintainers, I am not going to suggest binding name changes any
> > more. In case of doubt, I will skip this change in description until a
> > better solution is found.
> >  - In the hope to submit a patch that does not require additional
> > corrections, is a single patch acceptable with both a change in
> > description and with the added missing properties (already existing in
> > implementation), or should it be split in two?
> >
> > Best regards,
> > Fabrizio Lamarque
>

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

[Jonathan Cameron]

On Wed, 26 Apr 2023 12:45:31 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> On Sat, Apr 15, 2023 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 14 Apr 2023 12:42:08 +0200
> > Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:
> >  
> > > On Fri, Apr 14, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> > > >
> > > > On 13/04/2023 20:19, Fabrizio Lamarque wrote:  
> > > > > On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> > > > >>
> > > > >> On 13/04/2023 18:07, Fabrizio Lamarque wrote:  
> > > > >>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> > > > >>>>
> > > > >>>> On 13/04/2023 10:36, Fabrizio Lamarque wrote:  
> > > > >>>
> > > > >>> Current documentation does not follow existing source code implementation.  
> > > > >>
> > > > >> Bindings describe hardware, not current implementation.  
> > > > >  
> > > > >>> The driver has been originally designed to operate with the internal
> > > > >>> clock when clocks property is omitted.  
> > > > >>
> > > > >> Not really a reason to do it. Reason could be - hardware does not always
> > > > >> need clock input.  
> > > > >
> > > > > I hope the change in perspective will be enough. The external clock is
> > > > > mandatory for some applications.
> > > > > The internal clock might be required for others.  
> > > >
> > > > I told you that reason you wrote is not enough and you answer "in
> > > > perspective will be enough". Wait, what? I don't understand it at all. I
> > > > gave you example reason. If you do not like it, find other reasons which
> > > > refer to the actual device, not to the specific implementation.  
> >
> > Just leave it vague - we don't need to talk about specific apps. If someone
> > wired a clock, they probably want that clock an will describe it in their
> > DTS.
> >
> > An internal clock is available and may be used if no external clock is provided.
> >  
> > > >  
> > > > >>>
> > > > >>> I thought the reason is clear from patch 2, but, as Nuno Sá already
> > > > >>> suggested, I will describe the reasons in full again, each time I post
> > > > >>> a revised patch set, even if it is quite verbose.  
> > > > >>
> > > > >> Your commit must answer to why you are doing it. What you are doing is
> > > > >> easily visible from the diff. Rephrase commit msg to explain it and add
> > > > >> proper rationale (hardware related, not driver).  
> > > > >
> > > > > I am really just suggesting to align the documentation with the
> > > > > driver, since the driver operates the hardware as expected (after the
> > > > > two regressions fixes).  
> > > >
> > > > Does it mean you are not going to answer to "why?"? I cannot accept such
> > > > commits. That's the basics of software development and versioning. It's
> > > > not even Linux kernel related...
> > > >  
> > >
> > > I already wrote that the hardware clock might be generated internally
> > > and hence not required, but also that an external crystal/oscillator
> > > is suggested in the datasheet for better performance.
> > > There shall be a method to choose between the crystal, the external
> > > oscillator and the internal oscillator.
> > > This is what I was expecting to write on revised commit information,
> > > and this was the hopefully accepted "perspective change".
> > >
> > > I am really sorry but I do not understand what more I should say here.
> > > I appreciate your explanations and the fact you are taking your time
> > > to give directions to an obviously inexperienced developer, but my
> > > lack of understanding is perhaps related to what I called
> > > "perspective" on which comes first.
> > > If the above sentence is not enough, I will drop this last patch.
> > > I did not change the driver at all. The documentation is wrong (where
> > > it states the clocks property is mandatory) and it was missing
> > > information on how to choose between the clocks from the beginning.  
> >
> > A simple statement that there is an internal clock that may be used if
> > no external clock is wired up should be sufficient.  I don't think we
> > need to talk about precision of clocks etc vs cost of board built and
> > now that drives a design choice.
> >  
> > >  
> > > >  
> > > > > Without this change, one should read the source code to understand
> > > > > which clock is used and when, which bindings have to be applied, and
> > > > > find that documentation mandates an (already) optional property.  
> > > >
> > > > How is it related? I did not refer whether change is reasonable itself
> > > > or not. I said you commit msg is very poor and you must answer to "why".
> > > > Not to "what".
> > > >
> > > > (...)
> > > >  
> > > > >  
> > > > >>>
> > > > >>> I kindly ask you whether the entire (corrected) change on the
> > > > >>> documentation file only (without any change on the driver source code)
> > > > >>> could be accepted as a single patch.  
> > > > >>
> > > > >> I don't understand the question. Each change should be one logical
> > > > >> change, but bindings are not related to the driver.  
> > > > >
> > > > > The question came after this:
> > > > >
> > > > > On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > >>> 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.  
> > > > >
> > > > > I really need to send a proper, complete and acceptable v3 patch set,
> > > > > or drop patch 3/3 from the set.
> > > > >
> > > > > Would you accept the change to adi,ad7192.yaml file alone with both
> > > > > the change in description of avdd-supply and the additional missing
> > > > > properties?  
> > > >
> > > > Do you mean by this changing bindings without changing driver? Then
> > > > depends on the context. The driver must implement bindings, so you
> > > > should not send patches which break the implementation of interface.
> > > >  
> > >
> > > Perhaps I failed to explain it from the beginning.
> > > I said "the documentation does not follow the driver" but I understand
> > > I should have seen it the other way round.  
> >
> > No need to mention the driver at all for the DT binding patch.
> > You are documenting what could be wired.  We shouldn't care for DT about
> > what he driver happens to implement today.  
> 
> Thank you once again for your corrections, suggestions and patience.
> I hope to be able to provide a formally corrected patch set. Before
> sending v3, would you please have a look at this and check if it
> correctly responds to your expectations or if there is still something
> to adjust?
> (in case it would be cleaner to send a v3 patch and wait for feedback
> instead, feel free to ask)

I tend to only get back to IIO discussions about once a week (this week
was particularly crazy as was at a conference all week) so if you are
fairly sure what I mean and want to move quickly I'd suggest just
sending a new version based on 'best guess'.

> 
> The patch set will be then split in 5 total commits.
>  - iio: adc: ad7192: Fix null ad7192_state pointer access
>  - iio: adc: ad7192: Fix internal/external clock selection
>  - iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source
>  - dt-bindings: iio: ad7192: Add mandatory reference voltage source
>  - dt-bindings: iio: ad7192: Allow selection of clock modes
> 
> Patches "iio: adc: ad7192: Use VRef instead of AVdd as reference
> voltage source" and "iio: adc: ad7192: Use VRef instead of AVdd as
> reference voltage source" and will be a fix against b581f748cce0
> ("staging: iio: adc: ad7192: move out of staging").
> 
> First commit message on bindings will be:
> 
> Add required reference voltage (VRef) supply regulator.
> AD7192 requires three independent voltage sources: DVdd, AVdd and VRef
> (on REFINx pin pairs).
> 
> Some questions:
>  - Is the FIxes tag referenced commit acceptable, or should it be
> explained? If so, what should I write?

Given commit says they are 'required' I think ti's obvious that adding them
is what you are fixing wrt to the earlier patch that didn't have them.
So I don't think you need to state more.

>  - Is the commit message acceptable?

Looks good to me.

> 
> >  
> > >
> > > Actual documentation does not allow the use of the internal oscillator
> > > or the external crystal.
> > > By strictly following the documentation I could only use the external
> > > oscillator option (neither internal nor external crystal oscillator).
> > > The implementation already had in place more binding options than the
> > > documented ones, so I am not suggesting to break anything.
> > >  
> > > >  
> > > > > Is "Phandle to reference voltage regulator" as a description for
> > > > > avdd-supply acceptable on your side?  
> > > >
> > > > Sorry, how is this related to our topic? Anyway, drop "phandle to",
> > > > because you should describe hardware, not syntax of DTS.  
> > >
> > > Ok.
> > >  - Is it acceptable to change the description of property
> > > "avdd-supply" from "AVdd voltage supply" to "VRef voltage supply"?  
> > No.
> >
> > We have a slightly complex case of not wanting to break old drivers, but
> > the binding should be correct none the less.
> >
> > Binding wise:
> > * Leave avdd-supply alone.
> > * Add a new vref-supply entry and list it as required. This is the Binding
> >   'bug fix' and there should be an appropriate fixes tag.
> >
> > No need at all for the binding to describe some old buggy driver behaviour.
> >
> > Driver wise. - with comments on why we are doing this as we are papering
> > over a bug that will be seen if someone has a DT that worked with the old
> > code.  That's fine as long as we document that we are doing it.
> >
> > * Always get vadd-supply (not optional) - we may get a dummy regulator.
> >   That is fine here.
> > * Try to use vref-supply first as an optional supply.
> > * If that fails, then 'use' avdd as if it it were vref. with nice obvious
> >   comment that this is happening an perhaps even a warning print
> >   "Warning: Using avdd in place of vref. Likely an old DTS".  
> 
> Here is the change I am backporting and testing on my platform:
> 
> Subject: [PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as
>  reference voltage source
> 
> Add missing vref-supply and fix avdd-supply used as if it were vref.
> 
> AD7192 requires three independent voltage sources, digital supply (on
> pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
> alternate pin pair REFIN1 or REFIN2).
> 
> Emit a warning message when AVdd is used in place of VRef for backwards
> compatibility.
> 
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> ---
>  drivers/iio/adc/ad7192.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 5a9c8898f8af..4ac6843b7c23 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -177,6 +177,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;
> @@ -1014,11 +1015,30 @@ static int ad7192_probe(struct spi_device *spi)
>      if (ret)
>          return ret;
> 
> +    st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> +    if (!IS_ERR(st->vref)) {

You need to specifically handle only -ENODEV (I think - check what is
returned if get_optional doesn't get it because it's not supplied).

You might get a deferral that needs to be treated as an error (so we go
around again once the supply driver is bound).  So add an

	} else if (PTR_ERR(st->vref) != -ENODEV) {
		return PTR_ERR(st->vref);
	}

> +        ret = regulator_enable(st->vref);
> +        if (ret) {
> +            dev_err(&spi->dev, "Failed to enable specified VRef supply\n");

return dev_err_probe() is fine for all calls in probe() as it's neater
and means we don't need to think if something could defer or not.

> +            return ret;
> +        }
> +
> +        ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable,
> st->vref);
> +        if (ret)
> +            return ret;
> +    }
> +
>      ret = devm_regulator_get_enable(&spi->dev, "dvdd");
>      if (ret)
>          return dev_err_probe(&spi->dev, ret, "Failed to enable
> specified DVdd supply\n");
> 
> -    ret = regulator_get_voltage(st->avdd);
> +
> +    if (!IS_ERR(st->vref)) {
> +        ret = regulator_get_voltage(st->vref);
> +    } else {
> +        dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an
> old DTS\n");
> +        ret = regulator_get_voltage(st->avdd);
> +    }

This bit is good.

>      if (ret < 0) {
>          dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
>          return ret;