[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240630104516.59505c39@jic23-huawei>
Date: Sun, 30 Jun 2024 10:45:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: Alisa-Dariana Roman <alisadariana@...il.com>, Alisa-Dariana Roman
<alisa.roman@...log.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Michael Hennerich <michael.hennerich@...log.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, Mark
Brown <broonie@...nel.org>
Subject: Re: [PATCH v6 3/6] iio: adc: ad7192: Update clock config
On Tue, 25 Jun 2024 08:30:47 +0300
Alexandru Ardelean <aardelean@...libre.com> wrote:
> On Mon, Jun 24, 2024 at 3:50 PM Alisa-Dariana Roman
> <alisadariana@...il.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@...log.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
Powered by blists - more mailing lists