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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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