Date: Tue, 25 Jun 2024 08:30:47 +0300
From: Alexandru Ardelean <aardelean@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: 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>, Jonathan Cameron <jic23@...nel.org>, 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 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.

>
> 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

> +};
> +
> +static int ad7192_clock_setup(struct ad7192_state *st)
>  {
>         struct device *dev = &st->sd.spi->dev;
> -       unsigned int clock_sel;
> -
> -       clock_sel = AD7192_CLK_INT;
> +       int ret;
>
> -       /* use internal clock */
> -       if (!st->mclk) {
> -               if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
> -                       clock_sel = AD7192_CLK_INT_CO;
> +       ret = device_property_match_property_string(dev, "clock-names",
> +                                                   ad7192_clock_names,
> +                                                   ARRAY_SIZE(ad7192_clock_names));
> +       if (ret < 0) {
> +               st->clock_sel = AD7192_CLK_INT;
> +               st->fclk = AD7192_INT_FREQ_MHZ;

Since this is a new function, an early return can be added here.
This would make the else statement redundant, and the function would
have less indentation.

So, something like:
if (ret < 0) {
         st->clock_sel = AD7192_CLK_INT;
         st->fclk = AD7192_INT_FREQ_MHZ;
         return 0;
}

st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;

st->mclk = devm_clk_get_enabled(dev, ad7192_clock_names[ret]);
if (IS_ERR(st->mclk))
...................


>         } else {
> -               if (device_property_read_bool(dev, "adi,clock-xtal"))
> -                       clock_sel = AD7192_CLK_EXT_MCLK1_2;
> -               else
> -                       clock_sel = AD7192_CLK_EXT_MCLK2;
> +               st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
> +
> +               st->mclk = devm_clk_get_enabled(dev, ad7192_clock_names[ret]);
> +               if (IS_ERR(st->mclk))
> +                       return dev_err_probe(dev, PTR_ERR(st->mclk),
> +                                            "Failed to get mclk\n");
> +
> +               st->fclk = clk_get_rate(st->mclk);
> +               if (!ad7192_valid_external_frequency(st->fclk))
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "External clock frequency out of bounds\n");
>         }
>
> -       return clock_sel;
> +       return 0;
>  }
>
>  static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
> @@ -1275,21 +1287,9 @@ static int ad7192_probe(struct spi_device *spi)
>         if (ret)
>                 return ret;
>
> -       st->fclk = AD7192_INT_FREQ_MHZ;
> -
> -       st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
> -       if (IS_ERR(st->mclk))
> -               return PTR_ERR(st->mclk);
> -
> -       st->clock_sel = ad7192_clock_select(st);
> -
> -       if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
> -           st->clock_sel == AD7192_CLK_EXT_MCLK2) {
> -               st->fclk = clk_get_rate(st->mclk);
> -               if (!ad7192_valid_external_frequency(st->fclk))
> -                       return dev_err_probe(dev, -EINVAL,
> -                                            "External clock frequency out of bounds\n");
> -       }
> +       ret = ad7192_clock_setup(st);
> +       if (ret)
> +               return ret;
>
>         ret = ad7192_setup(indio_dev, dev);
>         if (ret)
> --
> 2.34.1
>
>

Powered by blists - more mailing lists