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

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

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