[PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Fabrizio Lamarque]

Allow the use of external clock when mclk clock is defined.
When defining a mclk clock source in device tree with adi,clock-xtal
property, the external crystal oscillator is not turned on.
Without the change, the driver always uses the internal clock even
when mclk clock is defined.

Current implementation seems to contain a typo, since it expected
st->mclk to be NULL within ad7192_of_clock_select() in order to select
the external clock, but, if null, external clock cannot loaded
correctly (out of bounds due to invalid mclk) in ad7192_probe().

I believe this patch follows the author's intended behavior.
 After applying this patch, the external oscillator is started as expected.

I kindly ask your feedback, I may adjust the patch according to your
suggestions. I could also follow up with another patch on
documentation, containing the following (related) issues:

- adi,int-clock-output-enable is undocumented
- adi,clock-xtal is undocumented
- regulator name avdd and its description is quite misleading, since
this is unrelated to the AVdd pin (#20) of AD7192; it is used instead
as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See
int_vref_mv variable within driver implementation.

Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
--- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506 +0100
+++ linux/drivers/iio/adc/ad7192.c  2023-03-14 10:19:32.361924242 +0100
@@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct
      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 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Nuno Sá]

On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:
> Allow the use of external clock when mclk clock is defined.
> When defining a mclk clock source in device tree with adi,clock-xtal
> property, the external crystal oscillator is not turned on.
> Without the change, the driver always uses the internal clock even
> when mclk clock is defined.
> 
> Current implementation seems to contain a typo, since it expected
> st->mclk to be NULL within ad7192_of_clock_select() in order to
> select
> the external clock, but, if null, external clock cannot loaded
> correctly (out of bounds due to invalid mclk) in ad7192_probe().
> 
> I believe this patch follows the author's intended behavior.
>  After applying this patch, the external oscillator is started as
> expected.
> 

Yes, looks like a valid fix... Just missing a Fixes tag.
> I kindly ask your feedback, I may adjust the patch according to your
> suggestions. I could also follow up with another patch on
> documentation, containing the following (related) issues:
> 
> - adi,int-clock-output-enable is undocumented
> - adi,clock-xtal is undocumented
> - regulator name avdd and its description is quite misleading, since
> this is unrelated to the AVdd pin (#20) of AD7192; it is used instead
> as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See
> int_vref_mv variable within driver implementation.
> 

Don't think the above text belongs to this commit message. That said,
it would be great if you could follow up with a couple of patches to
document the undocumented properties. As for the regular name, I think
it's not so trivial to change it's name because there could be already
users using the property like this. So, I guess we have two ways:

1) Just add some description in the property to state what's this
regulator really is about.
2) This one is more complex but might be the right one... Deprecrate
the current property (you can mark a property as deprecated in the
bindings) and add the new one with a proper name. Then, you need to
change the driver accordingly keeping in mind that it must still work
with the old, deprecrated property.

Was lazy to ckeck but I'm assuming the bindings are already in yaml...

> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> --- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506

Anyways, for this patch and with a proper fixes tag (and with the
unrelated text removed from the commit message):

Reviewed-by: nuno.sa@analog.com

Re: [PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Fabrizio Lamarque]

On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:
> > Allow the use of external clock when mclk clock is defined.
> > When defining a mclk clock source in device tree with adi,clock-xtal
> > property, the external crystal oscillator is not turned on.
> > Without the change, the driver always uses the internal clock even
> > when mclk clock is defined.
> >
> > Current implementation seems to contain a typo, since it expected
> > st->mclk to be NULL within ad7192_of_clock_select() in order to
> > select
> > the external clock, but, if null, external clock cannot loaded
> > correctly (out of bounds due to invalid mclk) in ad7192_probe().
> >
> > I believe this patch follows the author's intended behavior.
> >  After applying this patch, the external oscillator is started as
> > expected.
> >
>
> Yes, looks like a valid fix... Just missing a Fixes tag.

Thank you for having this patch reviewed.
Here is a backwards compatibility note I believe you should be aware of.

Without this patch, the DT node shall contain (any) clock property
for the driver to be loaded properly.
The clock frequency is ignored, as it is ignored adi,clock-xtal property.
In case clock property is not set, the initialization always fails.
There is no way to use an external clock source.
As you already verified from source code, this was not intentional.

While the proposed patch should make the DT config load as intended,
there is a possible side effect on existing implementations relying on
internal clock source only, that would be deactivated when clock property
is defined in DT. In addition, bindings documentation states clock
property is mandatory.

I tend to dislike any solution that keeps the existing flow (and do not
break compatibility). Nothing that comes to my mind would be as simple
as this logical flow correction.
In addition, I checked the driver history and it looks like the minor bugs we
found in corrent implementation (see previous patch about NULL pointer
dereference) have been there for several years.

In case backwards compatibility is a real issue, I could rewrite the patch
so that an explicit external clock source property could be used instead.
If so, I kindly ask you to suggest an acceptable property name.

> > I kindly ask your feedback, I may adjust the patch according to your
> > suggestions. I could also follow up with another patch on
> > documentation, containing the following (related) issues:
> >
> > - adi,int-clock-output-enable is undocumented
> > - adi,clock-xtal is undocumented
> > - regulator name avdd and its description is quite misleading, since
> > this is unrelated to the AVdd pin (#20) of AD7192; it is used instead
> > as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See
> > int_vref_mv variable within driver implementation.
> >
>
> Don't think the above text belongs to this commit message. That said,
> it would be great if you could follow up with a couple of patches to
> document the undocumented properties. As for the regular name, I think
> it's not so trivial to change it's name because there could be already
> users using the property like this. So, I guess we have two ways:
>
> 1) Just add some description in the property to state what's this
> regulator really is about.
> 2) This one is more complex but might be the right one... Deprecrate
> the current property (you can mark a property as deprecated in the
> bindings) and add the new one with a proper name. Then, you need to
> change the driver accordingly keeping in mind that it must still work
> with the old, deprecrated property.
>
> Was lazy to ckeck but I'm assuming the bindings are already in yaml...

I can confirm bindings are already documented in yaml.
I will try to submit a patch with the missing documentation and the
proposed solution 2 in the next days.

>
> > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
> > --- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506
>
> Anyways, for this patch and with a proper fixes tag (and with the
> unrelated text removed from the commit message):
>
> Reviewed-by: nuno.sa@analog.com
>
>

Re: [PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Nuno Sá]

On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote:
> On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com>
> wrote:
> > 
> > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:
> > > Allow the use of external clock when mclk clock is defined.
> > > When defining a mclk clock source in device tree with adi,clock-
> > > xtal
> > > property, the external crystal oscillator is not turned on.
> > > Without the change, the driver always uses the internal clock
> > > even
> > > when mclk clock is defined.
> > > 
> > > Current implementation seems to contain a typo, since it expected
> > > st->mclk to be NULL within ad7192_of_clock_select() in order to
> > > select
> > > the external clock, but, if null, external clock cannot loaded
> > > correctly (out of bounds due to invalid mclk) in ad7192_probe().
> > > 
> > > I believe this patch follows the author's intended behavior.
> > >  After applying this patch, the external oscillator is started as
> > > expected.
> > > 
> > 
> > Yes, looks like a valid fix... Just missing a Fixes tag.
> 
> Thank you for having this patch reviewed.
> Here is a backwards compatibility note I believe you should be aware
> of.
> 
> Without this patch, the DT node shall contain (any) clock property
> for the driver to be loaded properly.
> The clock frequency is ignored, as it is ignored adi,clock-xtal
> property.
> In case clock property is not set, the initialization always fails.
> There is no way to use an external clock source.
> As you already verified from source code, this was not intentional.
> 
> While the proposed patch should make the DT config load as intended,
> there is a possible side effect on existing implementations relying
> on
> internal clock source only, that would be deactivated when clock
> property
> is defined in DT. 

Hmm, I see. I would argue anyone relying on that should have just
stepped forward and fix the real issue as you did :). So, maybe there's
no one actually relying on this but that's a big __maybe__... Not
really sure how to handle this. I'm not sure we want to keep
compatibility with something that's clearly wrong but often we need to
"live" with past messes.

Jonathan I guess your input will be valuable here :)?

> In addition, bindings documentation states clock
> property is mandatory.
> 

This looks at least suspicious to me given the fact we have an internal
alternative. Clearly looks wrong to me...

- Nuno Sá
> 

Re: [PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Jonathan Cameron]

On Thu, 30 Mar 2023 09:53:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote:
> > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com>
> > wrote:  
> > > 
> > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:  
> > > > Allow the use of external clock when mclk clock is defined.
> > > > When defining a mclk clock source in device tree with adi,clock-
> > > > xtal
> > > > property, the external crystal oscillator is not turned on.
> > > > Without the change, the driver always uses the internal clock
> > > > even
> > > > when mclk clock is defined.
> > > > 
> > > > Current implementation seems to contain a typo, since it expected
> > > > st->mclk to be NULL within ad7192_of_clock_select() in order to
> > > > select
> > > > the external clock, but, if null, external clock cannot loaded
> > > > correctly (out of bounds due to invalid mclk) in ad7192_probe().
> > > > 
> > > > I believe this patch follows the author's intended behavior.
> > > >  After applying this patch, the external oscillator is started as
> > > > expected.
> > > >   
> > > 
> > > Yes, looks like a valid fix... Just missing a Fixes tag.  
> > 
> > Thank you for having this patch reviewed.
> > Here is a backwards compatibility note I believe you should be aware
> > of.
> > 
> > Without this patch, the DT node shall contain (any) clock property
> > for the driver to be loaded properly.
> > The clock frequency is ignored, as it is ignored adi,clock-xtal
> > property.
> > In case clock property is not set, the initialization always fails.
> > There is no way to use an external clock source.
> > As you already verified from source code, this was not intentional.
> > 
> > While the proposed patch should make the DT config load as intended,
> > there is a possible side effect on existing implementations relying
> > on
> > internal clock source only, that would be deactivated when clock
> > property
> > is defined in DT.   
> 
> Hmm, I see. I would argue anyone relying on that should have just
> stepped forward and fix the real issue as you did :). So, maybe there's
> no one actually relying on this but that's a big __maybe__... Not
> really sure how to handle this. I'm not sure we want to keep
> compatibility with something that's clearly wrong but often we need to
> "live" with past messes.
> 
> Jonathan I guess your input will be valuable here :)?

Rather odd if they have bindings that say there is a clock but 'rely'
on the internal clock as opposed to the current bug meaning they aren't
using the external clock (but everything would work fine if they were).

So I'd expect that we will only see a problem if someone has another
bug somewhere else that means that clock isn't working correctly.
At that point they will almost certainly want to fix that bug.

> 
> > In addition, bindings documentation states clock
> > property is mandatory.
> >   
> 
> This looks at least suspicious to me given the fact we have an internal
> alternative. Clearly looks wrong to me...

Agreed. I don't think it should be mandatory in the binding doc.

Thanks,

Jonathan


> 
> - Nuno Sá
> >   

Re: [PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Fabrizio Lamarque]

On Sat, Apr 1, 2023 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 30 Mar 2023 09:53:29 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote:
> > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com>
> > > wrote:
> > > >
> > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:
> > > > > Allow the use of external clock when mclk clock is defined.
> > > > > When defining a mclk clock source in device tree with adi,clock-
> > > > > xtal
> > > > > property, the external crystal oscillator is not turned on.
> > > > > Without the change, the driver always uses the internal clock
> > > > > even
> > > > > when mclk clock is defined.
> > > > >
> > > > > Current implementation seems to contain a typo, since it expected
> > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to
> > > > > select
> > > > > the external clock, but, if null, external clock cannot loaded
> > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe().
> > > > >
> > > > > I believe this patch follows the author's intended behavior.
> > > > >  After applying this patch, the external oscillator is started as
> > > > > expected.
> > > > >
> > > >
> > > > Yes, looks like a valid fix... Just missing a Fixes tag.
> > >
> > > Thank you for having this patch reviewed.
> > > Here is a backwards compatibility note I believe you should be aware
> > > of.
> > >
> > > Without this patch, the DT node shall contain (any) clock property
> > > for the driver to be loaded properly.
> > > The clock frequency is ignored, as it is ignored adi,clock-xtal
> > > property.
> > > In case clock property is not set, the initialization always fails.
> > > There is no way to use an external clock source.
> > > As you already verified from source code, this was not intentional.
> > >
> > > While the proposed patch should make the DT config load as intended,
> > > there is a possible side effect on existing implementations relying
> > > on
> > > internal clock source only, that would be deactivated when clock
> > > property
> > > is defined in DT.
> >
> > Hmm, I see. I would argue anyone relying on that should have just
> > stepped forward and fix the real issue as you did :). So, maybe there's
> > no one actually relying on this but that's a big __maybe__... Not
> > really sure how to handle this. I'm not sure we want to keep
> > compatibility with something that's clearly wrong but often we need to
> > "live" with past messes.
> >
> > Jonathan I guess your input will be valuable here :)?
>
> Rather odd if they have bindings that say there is a clock but 'rely'
> on the internal clock as opposed to the current bug meaning they aren't
> using the external clock (but everything would work fine if they were).
>
> So I'd expect that we will only see a problem if someone has another
> bug somewhere else that means that clock isn't working correctly.
> At that point they will almost certainly want to fix that bug.

The bug was introduced in June 2021 from c9ec2cb328e3 iio: adc:
ad7192: use devm_clk_get_optional() for mclk, where there is a clearly
mistaken logic inversion.

Link: https://lore.kernel.org/all/20210513120752.90074-10-aardelean@deviqon.com/#r

If you agree, I can send a revised patch set (perhaps within a new thread),
with the additional clarifications on clock usage in bindings documentation,
and one third patch for undocumented bindings.

There is one last issue we are investigating, we have a work-around but
we have not found the root cause yet. I will post about it later, in
the hope the
patch set would include all the changes needed to have the driver operating
as expected.

Thank you once again for this review.

Fabrizio Lamarque

>
> >
> > > In addition, bindings documentation states clock
> > > property is mandatory.
> > >
> >
> > This looks at least suspicious to me given the fact we have an internal
> > alternative. Clearly looks wrong to me...
>
> Agreed. I don't think it should be mandatory in the binding doc.
>
> Thanks,
>
> Jonathan
>
>
> >
> > - Nuno Sá
> > >
>

Re: [PATCH 2/2] ad7192 driver: fixed unexpected internal clock fallback when no mclk clock is defined.

[Jonathan Cameron]

On Tue, 4 Apr 2023 09:20:24 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> On Sat, Apr 1, 2023 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 30 Mar 2023 09:53:29 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >  
> > > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote:  
> > > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com>
> > > > wrote:  
> > > > >
> > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote:  
> > > > > > Allow the use of external clock when mclk clock is defined.
> > > > > > When defining a mclk clock source in device tree with adi,clock-
> > > > > > xtal
> > > > > > property, the external crystal oscillator is not turned on.
> > > > > > Without the change, the driver always uses the internal clock
> > > > > > even
> > > > > > when mclk clock is defined.
> > > > > >
> > > > > > Current implementation seems to contain a typo, since it expected
> > > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to
> > > > > > select
> > > > > > the external clock, but, if null, external clock cannot loaded
> > > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe().
> > > > > >
> > > > > > I believe this patch follows the author's intended behavior.
> > > > > >  After applying this patch, the external oscillator is started as
> > > > > > expected.
> > > > > >  
> > > > >
> > > > > Yes, looks like a valid fix... Just missing a Fixes tag.  
> > > >
> > > > Thank you for having this patch reviewed.
> > > > Here is a backwards compatibility note I believe you should be aware
> > > > of.
> > > >
> > > > Without this patch, the DT node shall contain (any) clock property
> > > > for the driver to be loaded properly.
> > > > The clock frequency is ignored, as it is ignored adi,clock-xtal
> > > > property.
> > > > In case clock property is not set, the initialization always fails.
> > > > There is no way to use an external clock source.
> > > > As you already verified from source code, this was not intentional.
> > > >
> > > > While the proposed patch should make the DT config load as intended,
> > > > there is a possible side effect on existing implementations relying
> > > > on
> > > > internal clock source only, that would be deactivated when clock
> > > > property
> > > > is defined in DT.  
> > >
> > > Hmm, I see. I would argue anyone relying on that should have just
> > > stepped forward and fix the real issue as you did :). So, maybe there's
> > > no one actually relying on this but that's a big __maybe__... Not
> > > really sure how to handle this. I'm not sure we want to keep
> > > compatibility with something that's clearly wrong but often we need to
> > > "live" with past messes.
> > >
> > > Jonathan I guess your input will be valuable here :)?  
> >
> > Rather odd if they have bindings that say there is a clock but 'rely'
> > on the internal clock as opposed to the current bug meaning they aren't
> > using the external clock (but everything would work fine if they were).
> >
> > So I'd expect that we will only see a problem if someone has another
> > bug somewhere else that means that clock isn't working correctly.
> > At that point they will almost certainly want to fix that bug.  
> 
> The bug was introduced in June 2021 from c9ec2cb328e3 iio: adc:
> ad7192: use devm_clk_get_optional() for mclk, where there is a clearly
> mistaken logic inversion.
> 
> Link: https://lore.kernel.org/all/20210513120752.90074-10-aardelean@deviqon.com/#r
> 
> If you agree, I can send a revised patch set (perhaps within a new thread),
> with the additional clarifications on clock usage in bindings documentation,
> and one third patch for undocumented bindings.

Sounds good to me.

> 
> There is one last issue we are investigating, we have a work-around but
> we have not found the root cause yet. I will post about it later, in
> the hope the
> patch set would include all the changes needed to have the driver operating
> as expected.

Sounds interesting :(

> 
> Thank you once again for this review.

NP.

Thanks for sending your fixes upstream.

Jonathan

> 
> Fabrizio Lamarque
> 
> >  
> > >  
> > > > In addition, bindings documentation states clock
> > > > property is mandatory.
> > > >  
> > >
> > > This looks at least suspicious to me given the fact we have an internal
> > > alternative. Clearly looks wrong to me...  
> >
> > Agreed. I don't think it should be mandatory in the binding doc.
> >
> > Thanks,
> >
> > Jonathan
> >
> >  
> > >
> > > - Nuno Sá  
> > > >  
> >