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

[Jonathan Cameron <jic23@kernel.org>]

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;