On Wed, 26 Apr 2023 12:45:31 +0200
Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx> wrote:
> On Sat, Apr 15, 2023 at 6:38 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Fri, 14 Apr 2023 12:42:08 +0200
> > Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx> wrote:
> >
> > > On Fri, Apr 14, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> > > >
> > > > On 13/04/2023 20:19, Fabrizio Lamarque wrote:
> > > > > On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> > > > >>
> > > > >> On 13/04/2023 18:07, Fabrizio Lamarque wrote:
> > > > >>> On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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;
