On Tue, 25 Jun 2024 08:30:47 +0300
Alexandru Ardelean <aardelean@xxxxxxxxxxxx> wrote:
> On Mon, Jun 24, 2024 at 3:50 PM Alisa-Dariana Roman
> <alisadariana@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
