[PATCH 0/4] iio: adc: ad7124: proper clock support

[PATCH 0/4] iio: adc: ad7124: proper clock support

[David Lechner]

I started looking at adding some new features to the AD7124 driver and
noticed that the clock support was not quite right. The devicetree
bindings had a required "mclk" clock. MCLK is actually the name of an
internal counter in the ADC and also the name of the external clock
connection on the evaluation boards, so I guess it came from one or the
other of those. However, what the hardware actually has is a CLK pin
that can be wired up in one of three ways: not connected, input or
output. So the existing bindings making the clock required don't make
sense.

Furthermore, when looking at how this clock was being used in the
driver, I found that essentially this was being used as a way to
select the power mode of the ADC which is not at all how devicetree
bindings are supposed to work. The clock rate is fixed and the power
mode can change no matter what type of clock is being used. Again,
this just doesn't make sense.

So here is a series to fix the devicetree bindings and actually
implement proper clock support in the driver.

---
David Lechner (4):
      dt-bindings: iio: adc: adi,ad7124: fix clocks properties
      iio: adc: ad7124: do not require mclk
      iio: adc: ad7124: add external clock support
      iio: adc: ad7124: add clock output support

 .../devicetree/bindings/iio/adc/adi,ad7124.yaml    |  21 +++-
 drivers/iio/adc/ad7124.c                           | 134 +++++++++++++++++----
 2 files changed, 129 insertions(+), 26 deletions(-)
---
base-commit: ad59285dadaa0c0e54f5c30c7d0fb282b06e14a7
change-id: 20250723-iio-adc-ad7124-proper-clock-support-7249022a7349

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/4] dt-bindings: iio: adc: adi,ad7124: fix clocks properties

[David Lechner]

Use correct clocks properties for the AD7124 family of ADCs.

These ADCs have an internal clock along with an optional external clock
that can be connected to the CLK pin. This pin can be wired up 3 ways:
1. Not connected - the internal clock is used.
2. Connected to an external clock (input) - the external clock is used.
3. Connected to the CLK pin on another ADC (output) - the internal clock
   is used on one and the other is configured for an external clock.

The new bindings describe these 3 cases by picking one of the following:
1. Omit both clocks and #clock-cells properties.
2. Include only the clocks property with a phandle to the external clock.
3. Include only the #clock-cells property on the ADC providing the output.

The clock-names property is now deprecated and should not be used. The
MCLK signal that it refers to is an internal counter in the ADC and
therefore does not make sense as a devicetree property as it can't be
connected to anything external to the ADC. Since there is only one
possible external clock, the clock-names property is not needed anyway.
Based on the implementation of the Linux driver, it looks like the
"mclk" clock was basically being used as a control to select the power
mode of the ADC, which is not something that should be done in the
devicetree.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
index 7146a654ae382bac7410ba525dfc98662f0f674a..c4b5e29730d6df58d0c29ed6dc20d250a9af67e6 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
@@ -27,12 +27,21 @@ properties:
 
   clocks:
     maxItems: 1
-    description: phandle to the master clock (mclk)
+    description: Optional external clock connected to the CLK pin.
 
   clock-names:
+    deprecated: true
+    description:
+      MCLK is an internal counter in the ADC. Do not use this property.
     items:
       - const: mclk
 
+  '#clock-cells':
+    description:
+      The CLK pin can be used as an output. When that is the case, include
+      this property.
+    const: 0
+
   interrupts:
     description: IRQ line for the ADC
     maxItems: 1
@@ -66,10 +75,14 @@ properties:
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - interrupts
 
+# Can't have both clock input and output at the same time.
+not:
+  required:
+    - '#clock-cells'
+    - clocks
+
 patternProperties:
   "^channel@([0-9]|1[0-5])$":
     $ref: adc.yaml
@@ -135,8 +148,6 @@ examples:
         interrupt-parent = <&gpio>;
         rdy-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
         refin1-supply = <&adc_vref>;
-        clocks = <&ad7124_mclk>;
-        clock-names = "mclk";
 
         #address-cells = <1>;
         #size-cells = <0>;

-- 
2.43.0

Re: [PATCH 1/4] dt-bindings: iio: adc: adi,ad7124: fix clocks properties

[Rob Herring (Arm)]

On Thu, 24 Jul 2025 18:25:22 -0500, David Lechner wrote:
> Use correct clocks properties for the AD7124 family of ADCs.
> 
> These ADCs have an internal clock along with an optional external clock
> that can be connected to the CLK pin. This pin can be wired up 3 ways:
> 1. Not connected - the internal clock is used.
> 2. Connected to an external clock (input) - the external clock is used.
> 3. Connected to the CLK pin on another ADC (output) - the internal clock
>    is used on one and the other is configured for an external clock.
> 
> The new bindings describe these 3 cases by picking one of the following:
> 1. Omit both clocks and #clock-cells properties.
> 2. Include only the clocks property with a phandle to the external clock.
> 3. Include only the #clock-cells property on the ADC providing the output.
> 
> The clock-names property is now deprecated and should not be used. The
> MCLK signal that it refers to is an internal counter in the ADC and
> therefore does not make sense as a devicetree property as it can't be
> connected to anything external to the ADC. Since there is only one
> possible external clock, the clock-names property is not needed anyway.
> Based on the implementation of the Linux driver, it looks like the
> "mclk" clock was basically being used as a control to select the power
> mode of the ADC, which is not something that should be done in the
> devicetree.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7124.yaml     | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH 2/4] iio: adc: ad7124: do not require mclk

[David Lechner]

Make the "mclk" clock optional in the ad7124 driver. The MCLK is an
internal counter on the ADC, so it is not something that should be
coming from the devicetree. However, existing users may be using this
to essentially select the power mode of the ADC from the devicetree.
In order to not break those users, we have to keep the existing "mclk"
handling, but now it is optional.

Now, when the "mclk" clock is omitted from the devicetree, the driver
will default to the full power mode. Support for an external clock
and dynamic power mode switching can be added later if needed.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 61 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9808df2e92424283a86e9c105492c7447d071e44..f587574e8a24040540a470e13fed412ec9c81971 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -174,7 +174,6 @@ struct ad7124_state {
 	struct ad_sigma_delta sd;
 	struct ad7124_channel *channels;
 	struct regulator *vref[4];
-	struct clk *mclk;
 	unsigned int adc_control;
 	unsigned int num_channels;
 	struct mutex cfgs_lock; /* lock for configs access */
@@ -254,7 +253,9 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 {
 	unsigned int fclk, odr_sel_bits;
 
-	fclk = clk_get_rate(st->mclk);
+	fclk = ad7124_master_clk_freq_hz[FIELD_GET(
+		AD7124_ADC_CONTROL_POWER_MODE, st->adc_control)];
+
 	/*
 	 * FS[10:0] = fCLK / (fADC x 32) where:
 	 * fADC is the output data rate
@@ -1111,21 +1112,49 @@ 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 fclk, power_mode;
+	unsigned int power_mode;
+	struct clk *mclk;
 	int i, ret;
 
-	fclk = clk_get_rate(st->mclk);
-	if (!fclk)
-		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
+	/*
+	 * Always use full power mode for max performance. If needed, the driver
+	 * could be adapted to use a dynamic power mode based on the requested
+	 * output data rate.
+	 */
+	power_mode = AD7124_ADC_CONTROL_POWER_MODE_FULL;
 
-	/* The power mode changes the master clock frequency */
-	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
-					ARRAY_SIZE(ad7124_master_clk_freq_hz),
-					fclk);
-	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
-		ret = clk_set_rate(st->mclk, fclk);
-		if (ret)
-			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
+	/*
+	 * HACK: This "mclk" business is needed for backwards compatibility with
+	 * old devicetrees that specified a fake clock named "mclk" to select
+	 * the power mode.
+	 */
+	mclk = devm_clk_get_optional_enabled(dev, "mclk");
+	if (IS_ERR(mclk))
+		return dev_err_probe(dev, PTR_ERR(mclk), "Failed to get mclk\n");
+
+	if (mclk) {
+		unsigned long mclk_hz;
+
+		mclk_hz = clk_get_rate(mclk);
+		if (!mclk_hz)
+			return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
+
+		/*
+		 * This logic is a bit backwards, which is why it is considered
+		 * a hack and is only here for backwards compatibility. The
+		 * driver should be able to set the power mode as it sees fit
+		 * and the f_clk/mclk rate should be dynamic accordingly. But
+		 * here, we are selecting a fixed power mode based on the given
+		 * "mclk" rate.
+		 */
+		power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
+			ARRAY_SIZE(ad7124_master_clk_freq_hz), mclk_hz);
+
+		if (mclk_hz != ad7124_master_clk_freq_hz[power_mode]) {
+			ret = clk_set_rate(mclk, mclk_hz);
+			if (ret)
+				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
+		}
 	}
 
 	/* Set the power mode */
@@ -1303,10 +1332,6 @@ static int ad7124_probe(struct spi_device *spi)
 			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
 	}
 
-	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
-	if (IS_ERR(st->mclk))
-		return dev_err_probe(dev, PTR_ERR(st->mclk), "Failed to get mclk\n");
-
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
 		return ret;

-- 
2.43.0

Re: [PATCH 2/4] iio: adc: ad7124: do not require mclk

[Jonathan Cameron]

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

> Make the "mclk" clock optional in the ad7124 driver. The MCLK is an
> internal counter on the ADC, so it is not something that should be
> coming from the devicetree. However, existing users may be using this
> to essentially select the power mode of the ADC from the devicetree.
> In order to not break those users, we have to keep the existing "mclk"
> handling, but now it is optional.
> 
> Now, when the "mclk" clock is omitted from the devicetree, the driver
> will default to the full power mode. Support for an external clock
> and dynamic power mode switching can be added later if needed.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 61 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9808df2e92424283a86e9c105492c7447d071e44..f587574e8a24040540a470e13fed412ec9c81971 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -174,7 +174,6 @@ struct ad7124_state {
>  	struct ad_sigma_delta sd;
>  	struct ad7124_channel *channels;
>  	struct regulator *vref[4];
> -	struct clk *mclk;
>  	unsigned int adc_control;
>  	unsigned int num_channels;
>  	struct mutex cfgs_lock; /* lock for configs access */
> @@ -254,7 +253,9 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
>  {
>  	unsigned int fclk, odr_sel_bits;
>  
> -	fclk = clk_get_rate(st->mclk);
> +	fclk = ad7124_master_clk_freq_hz[FIELD_GET(
> +		AD7124_ADC_CONTROL_POWER_MODE, st->adc_control)];
> +

I'm not keen on that wrap point.

	fclk = ad7124_master_clk_freq_hz[FIELD_GET(AD7124_ADC_CONTROL_POWER_MODE,
						   st->adc_control)];

maybe?
>  	/*
>  	 * FS[10:0] = fCLK / (fADC x 32) where:
>  	 * fADC is the output data rate
> @@ -1111,21 +1112,49 @@ 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 fclk, power_mode;
> +	unsigned int power_mode;
> +	struct clk *mclk;
>  	int i, ret;
>  
> -	fclk = clk_get_rate(st->mclk);
> -	if (!fclk)
> -		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> +	/*
> +	 * Always use full power mode for max performance. If needed, the driver
> +	 * could be adapted to use a dynamic power mode based on the requested
> +	 * output data rate.
> +	 */
> +	power_mode = AD7124_ADC_CONTROL_POWER_MODE_FULL;
>  
> -	/* The power mode changes the master clock frequency */
> -	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> -					ARRAY_SIZE(ad7124_master_clk_freq_hz),
> -					fclk);
> -	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
> -		ret = clk_set_rate(st->mclk, fclk);
> -		if (ret)
> -			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> +	/*
> +	 * HACK: This "mclk" business is needed for backwards compatibility with

I'd drop the HACK bit of this. Whilst I understand the spirit of the comment
that term tends to make people try to 'fix' things ;)

> +	 * old devicetrees that specified a fake clock named "mclk" to select
> +	 * the power mode.
> +	 */
> +	mclk = devm_clk_get_optional_enabled(dev, "mclk");
> +	if (IS_ERR(mclk))
> +		return dev_err_probe(dev, PTR_ERR(mclk), "Failed to get mclk\n");
> +
> +	if (mclk) {
> +		unsigned long mclk_hz;
> +
> +		mclk_hz = clk_get_rate(mclk);
> +		if (!mclk_hz)
> +			return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> +
> +		/*
> +		 * This logic is a bit backwards, which is why it is considered
> +		 * a hack and is only here for backwards compatibility. The
> +		 * driver should be able to set the power mode as it sees fit
> +		 * and the f_clk/mclk rate should be dynamic accordingly. But
> +		 * here, we are selecting a fixed power mode based on the given
> +		 * "mclk" rate.

My assumption is that someone had a board with a fixed rate clock on this pin.
So it might not be possible to have the driver do that adjustment.
If anyone ever adds that support, we'll have to be careful about handling fixed
clocks.

This looks fine though.

> +		 */
> +		power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> +			ARRAY_SIZE(ad7124_master_clk_freq_hz), mclk_hz);
> +
> +		if (mclk_hz != ad7124_master_clk_freq_hz[power_mode]) {
> +			ret = clk_set_rate(mclk, mclk_hz);
> +			if (ret)
> +				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> +		}
>  	}
>  
>  	/* Set the power mode */
> @@ -1303,10 +1332,6 @@ static int ad7124_probe(struct spi_device *spi)
>  			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
>  	}
>  
> -	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
> -	if (IS_ERR(st->mclk))
> -		return dev_err_probe(dev, PTR_ERR(st->mclk), "Failed to get mclk\n");
> -
>  	ret = ad7124_soft_reset(st);
>  	if (ret < 0)
>  		return ret;
> 

Re: [PATCH 2/4] iio: adc: ad7124: do not require mclk

[David Lechner]

On 7/27/25 7:21 AM, Jonathan Cameron wrote:
> On Thu, 24 Jul 2025 18:25:23 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 

...

>> @@ -1111,21 +1112,49 @@ 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 fclk, power_mode;
>> +	unsigned int power_mode;
>> +	struct clk *mclk;
>>  	int i, ret;
>>  
>> -	fclk = clk_get_rate(st->mclk);
>> -	if (!fclk)
>> -		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
>> +	/*
>> +	 * Always use full power mode for max performance. If needed, the driver
>> +	 * could be adapted to use a dynamic power mode based on the requested
>> +	 * output data rate.
>> +	 */
>> +	power_mode = AD7124_ADC_CONTROL_POWER_MODE_FULL;
>>  
>> -	/* The power mode changes the master clock frequency */
>> -	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
>> -					ARRAY_SIZE(ad7124_master_clk_freq_hz),
>> -					fclk);
>> -	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
>> -		ret = clk_set_rate(st->mclk, fclk);
>> -		if (ret)
>> -			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
>> +	/*
>> +	 * HACK: This "mclk" business is needed for backwards compatibility with
> 
> I'd drop the HACK bit of this. Whilst I understand the spirit of the comment
> that term tends to make people try to 'fix' things ;)
> 
>> +	 * old devicetrees that specified a fake clock named "mclk" to select
>> +	 * the power mode.
>> +	 */
>> +	mclk = devm_clk_get_optional_enabled(dev, "mclk");
>> +	if (IS_ERR(mclk))
>> +		return dev_err_probe(dev, PTR_ERR(mclk), "Failed to get mclk\n");
>> +
>> +	if (mclk) {
>> +		unsigned long mclk_hz;
>> +
>> +		mclk_hz = clk_get_rate(mclk);
>> +		if (!mclk_hz)
>> +			return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
>> +
>> +		/*
>> +		 * This logic is a bit backwards, which is why it is considered
>> +		 * a hack and is only here for backwards compatibility. The
>> +		 * driver should be able to set the power mode as it sees fit
>> +		 * and the f_clk/mclk rate should be dynamic accordingly. But
>> +		 * here, we are selecting a fixed power mode based on the given
>> +		 * "mclk" rate.
> 
> My assumption is that someone had a board with a fixed rate clock on this pin.
> So it might not be possible to have the driver do that adjustment.
> If anyone ever adds that support, we'll have to be careful about handling fixed
> clocks.

In order to use an external clock, you have to program a register field to
allow that. Since the driver isn't doing that, we can be sure that even if
someone had an external clock, the driver was still using the internal clock.

> 
> This looks fine though.
> 
>> +		 */
>> +		power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
>> +			ARRAY_SIZE(ad7124_master_clk_freq_hz), mclk_hz);
>> +
>> +		if (mclk_hz != ad7124_master_clk_freq_hz[power_mode]) {
>> +			ret = clk_set_rate(mclk, mclk_hz);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
>> +		}
>>  	}

Re: [PATCH 2/4] iio: adc: ad7124: do not require mclk

[Jonathan Cameron]

On Tue, 29 Jul 2025 11:08:29 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/27/25 7:21 AM, Jonathan Cameron wrote:
> > On Thu, 24 Jul 2025 18:25:23 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> 
> ...
> 
> >> @@ -1111,21 +1112,49 @@ 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 fclk, power_mode;
> >> +	unsigned int power_mode;
> >> +	struct clk *mclk;
> >>  	int i, ret;
> >>  
> >> -	fclk = clk_get_rate(st->mclk);
> >> -	if (!fclk)
> >> -		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> >> +	/*
> >> +	 * Always use full power mode for max performance. If needed, the driver
> >> +	 * could be adapted to use a dynamic power mode based on the requested
> >> +	 * output data rate.
> >> +	 */
> >> +	power_mode = AD7124_ADC_CONTROL_POWER_MODE_FULL;
> >>  
> >> -	/* The power mode changes the master clock frequency */
> >> -	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> >> -					ARRAY_SIZE(ad7124_master_clk_freq_hz),
> >> -					fclk);
> >> -	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
> >> -		ret = clk_set_rate(st->mclk, fclk);
> >> -		if (ret)
> >> -			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> >> +	/*
> >> +	 * HACK: This "mclk" business is needed for backwards compatibility with  
> > 
> > I'd drop the HACK bit of this. Whilst I understand the spirit of the comment
> > that term tends to make people try to 'fix' things ;)
> >   
> >> +	 * old devicetrees that specified a fake clock named "mclk" to select
> >> +	 * the power mode.
> >> +	 */
> >> +	mclk = devm_clk_get_optional_enabled(dev, "mclk");
> >> +	if (IS_ERR(mclk))
> >> +		return dev_err_probe(dev, PTR_ERR(mclk), "Failed to get mclk\n");
> >> +
> >> +	if (mclk) {
> >> +		unsigned long mclk_hz;
> >> +
> >> +		mclk_hz = clk_get_rate(mclk);
> >> +		if (!mclk_hz)
> >> +			return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
> >> +
> >> +		/*
> >> +		 * This logic is a bit backwards, which is why it is considered
> >> +		 * a hack and is only here for backwards compatibility. The
> >> +		 * driver should be able to set the power mode as it sees fit
> >> +		 * and the f_clk/mclk rate should be dynamic accordingly. But
> >> +		 * here, we are selecting a fixed power mode based on the given
> >> +		 * "mclk" rate.  
> > 
> > My assumption is that someone had a board with a fixed rate clock on this pin.
> > So it might not be possible to have the driver do that adjustment.
> > If anyone ever adds that support, we'll have to be careful about handling fixed
> > clocks.  
> 
> In order to use an external clock, you have to program a register field to
> allow that. Since the driver isn't doing that, we can be sure that even if
> someone had an external clock, the driver was still using the internal clock.
Ah. That indeed solves this!

> 
> > 
> > This looks fine though.
> >   
> >> +		 */
> >> +		power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
> >> +			ARRAY_SIZE(ad7124_master_clk_freq_hz), mclk_hz);
> >> +
> >> +		if (mclk_hz != ad7124_master_clk_freq_hz[power_mode]) {
> >> +			ret = clk_set_rate(mclk, mclk_hz);
> >> +			if (ret)
> >> +				return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
> >> +		}
> >>  	}  
> 

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

[David Lechner]

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>
---
 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");
+
+		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");
+
+			/*
+			 * 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);
 

-- 
2.43.0

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

[Jonathan Cameron]

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

[PATCH 4/4] iio: adc: ad7124: add clock output support

[David Lechner]

Add support for the AD7124's internal clock output. If the #clock-cells
property is present, turn on the internal clock output during probe.

If both the clocks and #clock-names properties are present (not allowed
by devicetree bindings), assume that an external clock is being used so
that we don't accidentally have two outputs fighting each other.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

We could make this fancier and only turn on the output on demand of a
clock consumer, but then we have to deal with locking of the SPI bus
to be able to write to the register. So I opted for the simpler
solution of always turning it on during probe. This would only be used
for synchronizing with other similar ADCs, so implementing the functions
for a more general-purpose clock seems a bit overkill.
---
 drivers/iio/adc/ad7124.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b0b03f838eed730347a3afcd759be7c1a8ab201e..b18229ff037596c6e98e12dc22b1552bf13fdc4e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -7,6 +7,7 @@
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -125,10 +126,12 @@ static const unsigned int ad7124_reg_size[] = {
 	3, 3, 3, 3, 3
 };
 
+#define AD7124_INT_CLK_HZ 614400
+
 static const int ad7124_master_clk_freq_hz[3] = {
-	[AD7124_LOW_POWER] = 76800,
-	[AD7124_MID_POWER] = 153600,
-	[AD7124_FULL_POWER] = 614400,
+	[AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
+	[AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
+	[AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
 };
 
 static const char * const ad7124_ref_names[] = {
@@ -1163,6 +1166,32 @@ static int ad7124_setup(struct ad7124_state *st)
 		}
 
 		clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
+	} else if (!device_property_present(dev, "clocks") &&
+		   device_property_present(dev, "clock-names")) {
+		struct clk_hw *clk_hw;
+		char *name;
+
+		name = devm_kasprintf(dev, GFP_KERNEL, "%s-clk",
+				      fwnode_get_name(dev_fwnode(dev)));
+		if (!name)
+			return -ENOMEM;
+
+		clk_hw = devm_clk_hw_register_fixed_rate(dev, name, NULL, 0,
+							 AD7124_INT_CLK_HZ);
+		if (IS_ERR(clk_hw))
+			return dev_err_probe(dev, PTR_ERR(clk_hw), "Failed to register clock provider\n");
+
+		ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+						  clk_hw);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to add clock provider\n");
+
+		/*
+		 * Treat the clock as always on. This way we don't have to deal
+		 * with someone trying to enable/disable the clock while we are
+		 * reading samples.
+		 */
+		clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT_OUT;
 	} else {
 		struct clk *clk;
 

-- 
2.43.0

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

[Jonathan Cameron]

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

> Add support for the AD7124's internal clock output. If the #clock-cells
> property is present, turn on the internal clock output during probe.
> 
> If both the clocks and #clock-names properties are present (not allowed
> by devicetree bindings), assume that an external clock is being used so
> that we don't accidentally have two outputs fighting each other.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> We could make this fancier and only turn on the output on demand of a
> clock consumer, but then we have to deal with locking of the SPI bus
> to be able to write to the register. So I opted for the simpler
> solution of always turning it on during probe. This would only be used
> for synchronizing with other similar ADCs, so implementing the functions
> for a more general-purpose clock seems a bit overkill.
Seems reasonable.  One comment inline.
> ---
>  drivers/iio/adc/ad7124.c | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index b0b03f838eed730347a3afcd759be7c1a8ab201e..b18229ff037596c6e98e12dc22b1552bf13fdc4e 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -7,6 +7,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -125,10 +126,12 @@ static const unsigned int ad7124_reg_size[] = {
>  	3, 3, 3, 3, 3
>  };
>  
> +#define AD7124_INT_CLK_HZ 614400
> +
>  static const int ad7124_master_clk_freq_hz[3] = {
> -	[AD7124_LOW_POWER] = 76800,
> -	[AD7124_MID_POWER] = 153600,
> -	[AD7124_FULL_POWER] = 614400,
> +	[AD7124_LOW_POWER] = AD7124_INT_CLK_HZ / 8,
> +	[AD7124_MID_POWER] = AD7124_INT_CLK_HZ / 4,
> +	[AD7124_FULL_POWER] = AD7124_INT_CLK_HZ,
>  };
>  
>  static const char * const ad7124_ref_names[] = {
> @@ -1163,6 +1166,32 @@ static int ad7124_setup(struct ad7124_state *st)
>  		}
>  
>  		clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT;
> +	} else if (!device_property_present(dev, "clocks") &&
> +		   device_property_present(dev, "clock-names")) {
> +		struct clk_hw *clk_hw;
> +		char *name;
> +
> +		name = devm_kasprintf(dev, GFP_KERNEL, "%s-clk",
> +				      fwnode_get_name(dev_fwnode(dev)));

I think for anything that isn't const the clock core will copy the name
during registration.  Ultimately __clk_register()
https://elixir.bootlin.com/linux/v6.15.8/source/drivers/clk/clk.c#L4342

As such tying this to devm lifespans is excessive.  Should be fine
using a __free(kfree) to clean it up at the end of this scope.

> +		if (!name)
> +			return -ENOMEM;
> +
> +		clk_hw = devm_clk_hw_register_fixed_rate(dev, name, NULL, 0,
> +							 AD7124_INT_CLK_HZ);
> +		if (IS_ERR(clk_hw))
> +			return dev_err_probe(dev, PTR_ERR(clk_hw), "Failed to register clock provider\n");
> +
> +		ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +						  clk_hw);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to add clock provider\n");
> +
> +		/*
> +		 * Treat the clock as always on. This way we don't have to deal
> +		 * with someone trying to enable/disable the clock while we are
> +		 * reading samples.
> +		 */
> +		clk_sel = AD7124_ADC_CONTROL_CLK_SEL_INT_OUT;
>  	} else {
>  		struct clk *clk;
>  
>