[PATCH v4 0/5] iio: adc: ad7192: Improvements

[PATCH v4 0/5] iio: adc: ad7192: Improvements

[Alisa-Dariana Roman]

Dear everyone,

Thank you very much for your feedback!

Here is the updated series. Please consider applying the commits in
order!

Kind regards,
Alisa-Dariana Roman.

v3 -> v4
  add clock provider patches
  add clean up dev patch
  as Nuno Sa pointed out, the old implementation of clock config was not
buggy, so the update clock config patches are no longer fixes

[PATCH v4 1/5] iio: adc: ad7192: Clean up dev

[Alisa-Dariana Roman]

Clean up by using a local variable struct device *dev. Also use
dev_err_probe where possible.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 65 +++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 0789121236d6..c7fb51a90e87 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1196,17 +1196,16 @@ static void ad7192_reg_disable(void *reg)
 
 static int ad7192_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	struct ad7192_state *st;
 	struct iio_dev *indio_dev;
 	struct regulator *aincom;
 	int ret;
 
-	if (!spi->irq) {
-		dev_err(&spi->dev, "no IRQ?\n");
-		return -ENODEV;
-	}
+	if (!spi->irq)
+		return dev_err_probe(dev, -ENODEV, "Failed to get IRQ\n");
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1219,71 +1218,69 @@ static int ad7192_probe(struct spi_device *spi)
 	 * Newer firmware should provide a zero volt fixed supply if wired to
 	 * ground.
 	 */
-	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
+	aincom = devm_regulator_get_optional(dev, "aincom");
 	if (IS_ERR(aincom)) {
 		if (PTR_ERR(aincom) != -ENODEV)
-			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
+			return dev_err_probe(dev, PTR_ERR(aincom),
 					     "Failed to get AINCOM supply\n");
 
 		st->aincom_mv = 0;
 	} else {
 		ret = regulator_enable(aincom);
 		if (ret)
-			return dev_err_probe(&spi->dev, ret,
+			return dev_err_probe(dev, ret,
 					     "Failed to enable specified AINCOM supply\n");
 
-		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
+		ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
 		if (ret)
 			return ret;
 
 		ret = regulator_get_voltage(aincom);
 		if (ret < 0)
-			return dev_err_probe(&spi->dev, ret,
+			return dev_err_probe(dev, ret,
 					     "Device tree error, AINCOM voltage undefined\n");
 		st->aincom_mv = ret / MILLI;
 	}
 
-	st->avdd = devm_regulator_get(&spi->dev, "avdd");
+	st->avdd = devm_regulator_get(dev, "avdd");
 	if (IS_ERR(st->avdd))
 		return PTR_ERR(st->avdd);
 
 	ret = regulator_enable(st->avdd);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable specified AVdd supply\n");
 
-	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
+	ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
 	if (ret)
 		return ret;
 
-	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
+	ret = devm_regulator_get_enable(dev, "dvdd");
 	if (ret)
-		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
+		return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
 
-	st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+	st->vref = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(st->vref)) {
 		if (PTR_ERR(st->vref) != -ENODEV)
 			return PTR_ERR(st->vref);
 
 		ret = regulator_get_voltage(st->avdd);
 		if (ret < 0)
-			return dev_err_probe(&spi->dev, ret,
+			return dev_err_probe(dev, ret,
 					     "Device tree error, AVdd voltage undefined\n");
 	} else {
 		ret = regulator_enable(st->vref);
-		if (ret) {
-			dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to enable specified Vref supply\n");
 
-		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
+		ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->vref);
 		if (ret)
 			return ret;
 
 		ret = regulator_get_voltage(st->vref);
 		if (ret < 0)
-			return dev_err_probe(&spi->dev, ret,
+			return dev_err_probe(dev, ret,
 					     "Device tree error, Vref voltage undefined\n");
 	}
 	st->int_vref_mv = ret / 1000;
@@ -1305,13 +1302,13 @@ static int ad7192_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
+	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
 	if (ret)
 		return ret;
 
 	st->fclk = AD7192_INT_FREQ_MHZ;
 
-	st->mclk = devm_clk_get_optional_enabled(&spi->dev, "mclk");
+	st->mclk = devm_clk_get_optional_enabled(dev, "mclk");
 	if (IS_ERR(st->mclk))
 		return PTR_ERR(st->mclk);
 
@@ -1320,18 +1317,16 @@ static int ad7192_probe(struct spi_device *spi)
 	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)) {
-			dev_err(&spi->dev,
-				"External clock frequency out of bounds\n");
-			return -EINVAL;
-		}
+		if (!ad7192_valid_external_frequency(st->fclk))
+			return dev_err_probe(dev, -EINVAL,
+					     "External clock frequency out of bounds\n");
 	}
 
-	ret = ad7192_setup(indio_dev, &spi->dev);
+	ret = ad7192_setup(indio_dev, dev);
 	if (ret)
 		return ret;
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static const struct of_device_id ad7192_of_match[] = {
-- 
2.34.1

[PATCH v4 2/5] dt-bindings: iio: adc: ad7192: Update clock config

[Alisa-Dariana Roman]

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.

Add clock name xtal alongside mclk. When an external crystal is
attached, xtal should be chosen. When an external clock is used, mclk
should be chosen.

The presence of an external clock source is optional, not required. When
absent, internal clock is used. Modify required property accordingly and
modify second example to showcase this.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7192.yaml    | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index a03da9489ed9..3ae2f860d24c 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -39,11 +39,15 @@ properties:
 
   clocks:
     maxItems: 1
-    description: phandle to the master clock (mclk)
+    description: |
+      Optionally, either a crystal can be attached externally between MCLK1 and
+      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
+      pin. If absent, internal 4.92MHz clock is used.
 
   clock-names:
-    items:
-      - const: mclk
+    enum:
+      - xtal
+      - mclk
 
   interrupts:
     maxItems: 1
@@ -135,8 +139,6 @@ patternProperties:
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - interrupts
   - dvdd-supply
   - avdd-supply
@@ -202,8 +204,6 @@ examples:
             spi-max-frequency = <1000000>;
             spi-cpol;
             spi-cpha;
-            clocks = <&ad7192_mclk>;
-            clock-names = "mclk";
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
             aincom-supply = <&aincom>;
-- 
2.34.1

[PATCH v4 3/5] iio: adc: ad7192: Update clock config

[Alisa-Dariana Roman]

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.

Removed functionality of AD7192_CLK_INT_CO restored in complementary
patch.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.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 c7fb51a90e87..c30ffe47cd70 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -398,25 +398,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"
+};
+
+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;
 	} 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)
@@ -1306,21 +1318,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

[PATCH v4 4/5] dt-bindings: iio: adc: ad7192: Add clock provider

[Alisa-Dariana Roman]

Internal clock of AD719X devices can be made available on MCLK2 pin. Add
clock provider to support this functionality.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7192.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 3ae2f860d24c..1434d89c2880 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -42,13 +42,20 @@ properties:
     description: |
       Optionally, either a crystal can be attached externally between MCLK1 and
       MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
-      pin. If absent, internal 4.92MHz clock is used.
+      pin. If absent, internal 4.92MHz clock is used, which can be made
+      available on MCLK2 pin.
 
   clock-names:
     enum:
       - xtal
       - mclk
 
+  "#clock-cells":
+    const: 0
+
+  clock-output-names:
+    maxItems: 1
+
   interrupts:
     maxItems: 1
 
@@ -204,6 +211,8 @@ examples:
             spi-max-frequency = <1000000>;
             spi-cpol;
             spi-cpha;
+            #clock-cells = <0>;
+            clock-output-names = "ad7194_mclk";
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
             aincom-supply = <&aincom>;
-- 
2.34.1

[PATCH v4 5/5] iio: adc: ad7192: Add clock provider

[Alisa-Dariana Roman]

Internal clock of AD719X devices can be made available on MCLK2 pin. Add
clock provider to support this functionality.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 90 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index c30ffe47cd70..36e3fe50c455 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -8,6 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -203,6 +204,7 @@ struct ad7192_state {
 	struct regulator		*avdd;
 	struct regulator		*vref;
 	struct clk			*mclk;
+	struct clk_hw			int_clk_hw;
 	u16				int_vref_mv;
 	u32				aincom_mv;
 	u32				fclk;
@@ -403,6 +405,90 @@ static const char *const ad7192_clock_names[] = {
 	"mclk"
 };
 
+static struct ad7192_state *clk_hw_to_ad7192(struct clk_hw *hw)
+{
+	return container_of(hw, struct ad7192_state, int_clk_hw);
+}
+
+static unsigned long ad7192_clk_recalc_rate(struct clk_hw *hw,
+					    unsigned long parent_rate)
+{
+	return AD7192_INT_FREQ_MHZ;
+}
+
+static int ad7192_clk_output_is_enabled(struct clk_hw *hw)
+{
+	struct ad7192_state *st = clk_hw_to_ad7192(hw);
+
+	return st->clock_sel == AD7192_CLK_INT_CO;
+}
+
+static int ad7192_clk_prepare(struct clk_hw *hw)
+{
+	struct ad7192_state *st = clk_hw_to_ad7192(hw);
+	int ret;
+
+	st->mode &= ~AD7192_MODE_CLKSRC_MASK;
+	st->mode |= AD7192_CLK_INT_CO;
+
+	ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+	if (ret)
+		return ret;
+
+	st->clock_sel = AD7192_CLK_INT_CO;
+
+	return 0;
+}
+
+static void ad7192_clk_unprepare(struct clk_hw *hw)
+{
+	struct ad7192_state *st = clk_hw_to_ad7192(hw);
+	int ret;
+
+	st->mode &= ~AD7192_MODE_CLKSRC_MASK;
+	st->mode |= AD7192_CLK_INT;
+
+	ret = ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+	if (ret)
+		return;
+
+	st->clock_sel = AD7192_CLK_INT;
+}
+
+static const struct clk_ops ad7192_int_clk_ops = {
+	.recalc_rate = ad7192_clk_recalc_rate,
+	.is_enabled = ad7192_clk_output_is_enabled,
+	.prepare = ad7192_clk_prepare,
+	.unprepare = ad7192_clk_unprepare,
+};
+
+static int ad7192_register_clk_provider(struct ad7192_state *st)
+{
+	struct device *dev = &st->sd.spi->dev;
+	struct clk_init_data init = {};
+	const char *clk_name;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK))
+		return 0;
+
+	ret = device_property_read_string(dev, "clock-output-names", &clk_name);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get clock-output-names\n");
+
+	init.name = clk_name;
+	init.ops = &ad7192_int_clk_ops;
+
+	st->int_clk_hw.init = &init;
+	ret = devm_clk_hw_register(dev, &st->int_clk_hw);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					   &st->int_clk_hw);
+}
+
 static int ad7192_clock_setup(struct ad7192_state *st)
 {
 	struct device *dev = &st->sd.spi->dev;
@@ -414,6 +500,10 @@ static int ad7192_clock_setup(struct ad7192_state *st)
 	if (ret < 0) {
 		st->clock_sel = AD7192_CLK_INT;
 		st->fclk = AD7192_INT_FREQ_MHZ;
+
+		ret = ad7192_register_clk_provider(st);
+		if (ret)
+			return ret;
 	} else {
 		st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
 
-- 
2.34.1

Re: [PATCH v4 2/5] dt-bindings: iio: adc: ad7192: Update clock config

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

On Thu, Jun 13, 2024 at 02:39:58PM +0300, Alisa-Dariana Roman wrote:
> 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.
> 
> Add clock name xtal alongside mclk. When an external crystal is
> attached, xtal should be chosen. When an external clock is used, mclk
> should be chosen.

This is still missing an explanation of why a new name is needed.
Hint: do you need to change register settings to use one versus the
other?

> 
> The presence of an external clock source is optional, not required. When
> absent, internal clock is used. Modify required property accordingly and
> modify second example to showcase this.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml    | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index a03da9489ed9..3ae2f860d24c 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -39,11 +39,15 @@ properties:
>  
>    clocks:
>      maxItems: 1
> -    description: phandle to the master clock (mclk)
> +    description: |
> +      Optionally, either a crystal can be attached externally between MCLK1 and
> +      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> +      pin. If absent, internal 4.92MHz clock is used.
>  
>    clock-names:
> -    items:
> -      - const: mclk
> +    enum:
> +      - xtal
> +      - mclk
>  
>    interrupts:
>      maxItems: 1
> @@ -135,8 +139,6 @@ patternProperties:
>  required:
>    - compatible
>    - reg
> -  - clocks
> -  - clock-names
>    - interrupts
>    - dvdd-supply
>    - avdd-supply
> @@ -202,8 +204,6 @@ examples:
>              spi-max-frequency = <1000000>;
>              spi-cpol;
>              spi-cpha;
> -            clocks = <&ad7192_mclk>;
> -            clock-names = "mclk";
>              interrupts = <25 0x2>;
>              interrupt-parent = <&gpio>;
>              aincom-supply = <&aincom>;
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 4/5] dt-bindings: iio: adc: ad7192: Add clock provider

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Thu, Jun 13, 2024 at 02:40:00PM +0300, Alisa-Dariana Roman wrote:
> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml       | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 3ae2f860d24c..1434d89c2880 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -42,13 +42,20 @@ properties:
>      description: |
>        Optionally, either a crystal can be attached externally between MCLK1 and
>        MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> -      pin. If absent, internal 4.92MHz clock is used.
> +      pin. If absent, internal 4.92MHz clock is used, which can be made
> +      available on MCLK2 pin.
>  
>    clock-names:
>      enum:
>        - xtal
>        - mclk
>  
> +  "#clock-cells":
> +    const: 0
> +
> +  clock-output-names:
> +    maxItems: 1

Why do you need an output name when you have exactly one clock?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/5] iio: adc: ad7192: Clean up dev

[Jonathan Cameron]

On Thu, 13 Jun 2024 14:39:57 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Clean up by using a local variable struct device *dev. Also use
> dev_err_probe where possible.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing

Thanks,

Jonathan

Re: [PATCH v4 2/5] dt-bindings: iio: adc: ad7192: Update clock config

[Jonathan Cameron]

On Thu, 13 Jun 2024 17:41:52 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Thu, Jun 13, 2024 at 02:39:58PM +0300, Alisa-Dariana Roman wrote:
> > 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.
> > 
> > Add clock name xtal alongside mclk. When an external crystal is
> > attached, xtal should be chosen. When an external clock is used, mclk
> > should be chosen.  
> 
> This is still missing an explanation of why a new name is needed.
> Hint: do you need to change register settings to use one versus the
> other?

Absolutely - dt reviewer shouldn't need to look at the code to find this
out as it wastes their over subscribed time!

> 
> > 
> > The presence of an external clock source is optional, not required. When
> > absent, internal clock is used. Modify required property accordingly and
> > modify second example to showcase this.
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7192.yaml    | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > index a03da9489ed9..3ae2f860d24c 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > @@ -39,11 +39,15 @@ properties:
> >  
> >    clocks:
> >      maxItems: 1
> > -    description: phandle to the master clock (mclk)
> > +    description: |
> > +      Optionally, either a crystal can be attached externally between MCLK1 and
> > +      MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2
> > +      pin. If absent, internal 4.92MHz clock is used.

Trivial but doesn't need to be formatted, so don't think the | matters.

> >  
> >    clock-names:
> > -    items:
> > -      - const: mclk
> > +    enum:
> > +      - xtal
> > +      - mclk
> >  
> >    interrupts:
> >      maxItems: 1
> > @@ -135,8 +139,6 @@ patternProperties:
> >  required:
> >    - compatible
> >    - reg
> > -  - clocks
> > -  - clock-names
> >    - interrupts
> >    - dvdd-supply
> >    - avdd-supply
> > @@ -202,8 +204,6 @@ examples:
> >              spi-max-frequency = <1000000>;
> >              spi-cpol;
> >              spi-cpha;
> > -            clocks = <&ad7192_mclk>;
> > -            clock-names = "mclk";

Why drop it from the example?  It's a valid setting to have one after all.

> >              interrupts = <25 0x2>;
> >              interrupt-parent = <&gpio>;
> >              aincom-supply = <&aincom>;
> > -- 
> > 2.34.1
> >