Re: [PATCH 3/4] iio: adc: ad7124: add external clock support

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 24 Jul 2025 18:25:24 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Add support for an external clock source to the AD7124 ADC driver.
> 
> Previously, the driver only supported using the internal clock and had
> bad devicetree bindings that used a fake clock to essentially select
> the power mode. This is preserved for backwards compatibility.
> 
> If the clock is not named "mclk", then we know that the devicetree is
> using the correct bindings and we can configure the chip to use an
> external clock source rather than internal.
> 
> Also drop a redundant comment when configuring the register fields
> instead of adding more.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Hi David,

A few trivial things inline.

> ---
>  drivers/iio/adc/ad7124.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index f587574e8a24040540a470e13fed412ec9c81971..b0b03f838eed730347a3afcd759be7c1a8ab201e 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -18,6 +18,7 @@
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/units.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/adc/ad_sigma_delta.h>
> @@ -44,6 +45,11 @@
>  #define AD7124_STATUS_POR_FLAG			BIT(4)
>  
>  /* AD7124_ADC_CONTROL */
> +#define AD7124_ADC_CONTROL_CLK_SEL		GENMASK(1, 0)
> +#define AD7124_ADC_CONTROL_CLK_SEL_INT			0
> +#define AD7124_ADC_CONTROL_CLK_SEL_INT_OUT		1
> +#define AD7124_ADC_CONTROL_CLK_SEL_EXT			2
> +#define AD7124_ADC_CONTROL_CLK_SEL_EXT_DIV4		3
>  #define AD7124_ADC_CONTROL_MODE			GENMASK(5, 2)
>  #define AD7124_ADC_CONTROL_MODE_CONTINUOUS		0
>  #define AD7124_ADC_CONTROL_MODE_SINGLE			1
> @@ -1112,7 +1118,7 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
>  static int ad7124_setup(struct ad7124_state *st)
>  {
>  	struct device *dev = &st->sd.spi->dev;
> -	unsigned int power_mode;
> +	unsigned int power_mode, clk_sel;
>  	struct clk *mclk;
>  	int i, ret;
>  
> @@ -1155,9 +1161,41 @@ static int ad7124_setup(struct ad7124_state *st)
>  			if (ret)
>  				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
>  		}
> +
> +		clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
> +	} else {
> +		struct clk *clk;
> +
> +		clk = devm_clk_get_optional_enabled(dev, NULL);
> +		if (IS_ERR(clk))
> +			return dev_err_probe(dev, PTR_ERR(clk), "Failed to get external clock\n");

Somme very long lines here where breaks won't hurt readability.

> +			return dev_err_probe(dev, PTR_ERR(clk),
					     "Failed to get external clock\n");

> +
> +		if (clk) {
> +			unsigned long clk_hz;
> +
> +			clk_hz = clk_get_rate(clk);
> +			if (!clk_hz)
> +				return dev_err_probe(dev, -EINVAL, "Failed to get external clock rate\n");

Add a break.

> +
> +			/*
> +			 * The external clock may be 4x the nominal clock rate,
> +			 * in which case the ADC needs to be configured to
> +			 * divide it by 4. Using MEGA is a bit arbitrary, but
> +			 * the expected clock rates are either 614.4 kHz or
> +			 * 2.4576 MHz, so this should work.
> +			 */
> +			if (clk_hz > MEGA)
> +				clk_sel = AD7124_ADC_CONTROL_CLK_SEL_EXT_DIV4;
> +			else
> +				clk_sel = AD7124_ADC_CONTROL_CLK_SEL_EXT;
> +		} else {
> +			clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
> +		}
>  	}
>  
> -	/* Set the power mode */
> +	st->adc_control &= ~AD7124_ADC_CONTROL_CLK_SEL;
> +	st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_CLK_SEL, clk_sel);
> +
>  	st->adc_control &= ~AD7124_ADC_CONTROL_POWER_MODE;
>  	st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_POWER_MODE, power_mode);
>  
>