Re: [PATCH v6 3/6] iio: adc: ad7192: Update clock config

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 25 Jun 2024 08:30:47 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:

> On Mon, Jun 24, 2024 at 3:50 PM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> >  
> 
> Hello,
> 
> A few comments inline.
> 
> > There are actually 4 configuration modes of clock source for AD719X
> > devices. Either a crystal can be attached externally between MCLK1 and
> > MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> > pin. The other 2 modes make use of the 4.92MHz internal clock.
> >
> > Removed properties adi,int-clock-output-enable and adi,clock-xtal were
> > undocumented. Use cleaner alternative of configuring external clock by
> > using clock names mclk and xtal.  
> 
> Should we keep the old properties for backwards compatibility?
> While I like the new approach, the downside is that someone upgrades
> the kernel and may run into issues with their board, because one of
> these properties went away.
> 
It's indeed a gamble that no one was actually using them.  Whilst
the lack of documentation suggests that might be the case, who knows.

If it isn't a bit maintenance issue to keep the old property support
in the driver it is probably a good idea (with lots of comments to
say it's deprecated).

> >
> > Removed functionality of AD7192_CLK_INT_CO restored in complementary
> > patch.
> >
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  drivers/iio/adc/ad7192.c | 56 ++++++++++++++++++++--------------------
> >  1 file changed, 28 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 334ab90991d4..940517df5429 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -396,25 +396,37 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
> >                 freq <= AD7192_EXT_FREQ_MHZ_MAX);
> >  }
> >
> > -static int ad7192_clock_select(struct ad7192_state *st)
> > +static const char *const ad7192_clock_names[] = {
> > +       "xtal",
> > +       "mclk"  
> 
> Just a thought; no strong opinion.
> Maybe add a short comment about these being "clock_sel" values
> AD7192_CLK_EXT_MCLK1_2 & AD7192_CLK_EXT_MCLK2 ?
> This is mostly due to the "st->clock_sel = AD7192_CLK_EXT_MCLK1_2 +
> ret;" logic (which is fine)
> Before, this change, it would

Did we lose rest of sentence here?

J