[PATCH v7 0/4] iio: adc: adc7192: Improvements

[PATCH v7 0/4] iio: adc: adc7192: Improvements

[Alisa-Dariana Roman]

Dear everyone,

Thank you very much for your feedback!

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

Alexandru and Conor, please note that I didn't add your Reviewed-by tags
since I modified the commits. Please consider checking them again!

Kind regards,
Alisa-Dariana Roman.

v6 -> v7
	add comment explaining clock names
	use early return
	keep backward compatibility for undocumented properties
	apply Nuno's suggestion, instead of failing when clock cells
property is missing, just don't register clock provider
	update bindings accordingly

v6: https://lore.kernel.org/all/20240624124941.113010-1-alisa.roman@analog.com/

[PATCH v7 1/4] 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.

To configure external clock as either a crystal or a CMOS-compatible
clock, changing the register settings is necessary. Therefore, add clock
name xtal alongside mclk. By selecting one or the other, the register is
configured.

The presence of an external clock source is optional, not required. When
both clocks and clock-names properties are present, an external clock
source is used. If the intention is to use the internal clock, both
properties should be absent. Modify required properties accordingly.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/iio/adc/adi,ad7192.yaml          | 22 ++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index a03da9489ed9..c3adc32684cf 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
@@ -157,6 +159,16 @@ allOf:
     then:
       patternProperties:
         "^channel@[0-9a-f]+$": false
+  - if:
+      anyOf:
+        - required:
+            - clocks
+        - required:
+            - clock-names
+    then:
+      required:
+        - clocks
+        - clock-names
 
 unevaluatedProperties: false
 
-- 
2.34.1

[PATCH v7 2/4] 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.

Undocumented properties adi,int-clock-output-enable and adi,clock-xtal
still supported for backward compatibility, but their use is highly
discouraged. Use cleaner alternative of configuring external clock by
using clock names mclk and xtal.

Functionality of AD7192_CLK_INT_CO will be implemented in complementary
patch by adding clock provider.

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

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 334ab90991d4..042319f0c641 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -396,25 +396,72 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
 		freq <= AD7192_EXT_FREQ_MHZ_MAX);
 }
 
-static int ad7192_clock_select(struct ad7192_state *st)
+/*
+ * Position 0 of ad7192_clock_names, xtal, corresponds to clock source
+ * configuration AD7192_CLK_EXT_MCLK1_2 and position 1, mclk, corresponds to
+ * AD7192_CLK_EXT_MCLK2
+ */
+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;
+	int ret;
 
-	clock_sel = AD7192_CLK_INT;
+	/*
+	 * The following two if branches are kept for backward compatibility but
+	 * the use of the two devicetree properties is highly discouraged. Clock
+	 * configuration should be done according to the bindings.
+	 */
 
-	/* use internal clock */
-	if (!st->mclk) {
-		if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
-			clock_sel = AD7192_CLK_INT_CO;
-	} else {
-		if (device_property_read_bool(dev, "adi,clock-xtal"))
-			clock_sel = AD7192_CLK_EXT_MCLK1_2;
-		else
-			clock_sel = AD7192_CLK_EXT_MCLK2;
+	if (device_property_read_bool(dev, "adi,int-clock-output-enable")) {
+		st->clock_sel = AD7192_CLK_INT_CO;
+		st->fclk = AD7192_INT_FREQ_MHZ;
+		dev_warn(dev, "Property adi,int-clock-output-enable is deprecated! Check bindings!\n");
+		return 0;
 	}
 
-	return clock_sel;
+	if (device_property_read_bool(dev, "adi,clock-xtal")) {
+		st->clock_sel = AD7192_CLK_EXT_MCLK1_2;
+		st->mclk = devm_clk_get_enabled(dev, "mclk");
+		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");
+
+		dev_warn(dev, "Property adi,clock-xtal is deprecated! Check bindings!\n");
+		return 0;
+	}
+
+	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;
+		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))
+		return dev_err_probe(dev, PTR_ERR(st->mclk),
+				     "Failed to get clock source\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 0;
 }
 
 static int ad7192_setup(struct iio_dev *indio_dev, struct device *dev)
@@ -1275,21 +1322,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 v7 3/4] 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 when clock cells property
is present.

The clock source can be either provided externally or the internal clock
is used. Pair of clocks and clock-names property is mutally exclusive
with #clock-cells property.

Modify second example to showcase the mode where internal clock is used.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index c3adc32684cf..edfa4378e838 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -42,13 +42,19 @@ 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
+    description:
+      If present when internal clock is used, configured as clock provider.
+
   interrupts:
     maxItems: 1
 
@@ -169,6 +175,8 @@ allOf:
       required:
         - clocks
         - clock-names
+      properties:
+        "#clock-cells": false
 
 unevaluatedProperties: false
 
@@ -214,8 +222,7 @@ examples:
             spi-max-frequency = <1000000>;
             spi-cpol;
             spi-cpha;
-            clocks = <&ad7192_mclk>;
-            clock-names = "mclk";
+            #clock-cells = <0>;
             interrupts = <25 0x2>;
             interrupt-parent = <&gpio>;
             aincom-supply = <&aincom>;
-- 
2.34.1

[PATCH v7 4/4] 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 when clock cells property
is present.

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

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 042319f0c641..3f803b1eefcc 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>
@@ -201,6 +202,7 @@ struct ad7192_chip_info {
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
 	struct clk			*mclk;
+	struct clk_hw			int_clk_hw;
 	u16				int_vref_mv;
 	u32				aincom_mv;
 	u32				fclk;
@@ -406,6 +408,91 @@ 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 = {};
+	int ret;
+
+	if (!device_property_present(dev, "#clock-cells"))
+		return 0;
+
+	if (!IS_ENABLED(CONFIG_COMMON_CLK))
+		return 0;
+
+	init.name = devm_kasprintf(dev, GFP_KERNEL, "%s-clk",
+				   fwnode_get_name(dev_fwnode(dev)));
+	if (!init.name)
+		return -ENOMEM;
+
+	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;
@@ -446,6 +533,11 @@ 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 dev_err_probe(dev, ret,
+					     "Failed to register clock provider\n");
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH v7 0/4] iio: adc: adc7192: Improvements

[Alisa-Dariana Roman]

cc: Alexandru Ardelean <aardelean@baylibre.com>

Re: [PATCH v7 2/4] iio: adc: ad7192: Update clock config

[Nuno Sá]

On Thu, 2024-07-18 at 00:25 +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.
> 
> Undocumented properties adi,int-clock-output-enable and adi,clock-xtal
> still supported for backward compatibility, but their use is highly
> discouraged. Use cleaner alternative of configuring external clock by
> using clock names mclk and xtal.
> 
> Functionality of AD7192_CLK_INT_CO will be implemented in complementary
> patch by adding clock provider.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

LGTM,

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

Re: [PATCH v7 4/4] iio: adc: ad7192: Add clock provider

[Nuno Sá]

On Thu, 2024-07-18 at 00:25 +0300, Alisa-Dariana Roman wrote:
> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality when clock cells property
> is present.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

minor thing below you may consider if a re-spin is needed...

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/ad7192.c | 92 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 042319f0c641..3f803b1eefcc 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>
> @@ -201,6 +202,7 @@ struct ad7192_chip_info {
>  struct ad7192_state {
>  	const struct ad7192_chip_info	*chip_info;
>  	struct clk			*mclk;
> +	struct clk_hw			int_clk_hw;
>  	u16				int_vref_mv;
>  	u32				aincom_mv;
>  	u32				fclk;
> @@ -406,6 +408,91 @@ 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 = {};
> +	int ret;
> +
> +	if (!device_property_present(dev, "#clock-cells"))
> +		return 0;
> +
> +	if (!IS_ENABLED(CONFIG_COMMON_CLK))
> +		return 0;
> 

nit: This could be the first test to do. No point in calling
device_property_present() if CONFIG_COMMON_CLK is disabled. FWIW, the compiler should
be smart enough to sort things out but it would still be better (for readability) to
have this first.

- Nuno Sá

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

[Conor Dooley]

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

On Thu, Jul 18, 2024 at 12:25:34AM +0300, Alisa-Dariana Roman wrote:
> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality when clock cells property
> is present.
> 
> The clock source can be either provided externally or the internal clock
> is used. Pair of clocks and clock-names property is mutally exclusive
> with #clock-cells property.
> 
> Modify second example to showcase the mode where internal clock is used.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml     | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index c3adc32684cf..edfa4378e838 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -42,13 +42,19 @@ 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
> +    description:
> +      If present when internal clock is used, configured as clock provider.
> +
>    interrupts:
>      maxItems: 1
>  
> @@ -169,6 +175,8 @@ allOf:
>        required:
>          - clocks
>          - clock-names
> +      properties:
> +        "#clock-cells": false

Properties before required, otherwise
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

>  
>  unevaluatedProperties: false
>  
> @@ -214,8 +222,7 @@ examples:
>              spi-max-frequency = <1000000>;
>              spi-cpol;
>              spi-cpha;
> -            clocks = <&ad7192_mclk>;
> -            clock-names = "mclk";
> +            #clock-cells = <0>;
>              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 v7 3/4] dt-bindings: iio: adc: ad7192: Add clock provider

[Jonathan Cameron]

On Thu, 18 Jul 2024 16:14:17 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Thu, Jul 18, 2024 at 12:25:34AM +0300, Alisa-Dariana Roman wrote:
> > Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> > clock provider to support this functionality when clock cells property
> > is present.
> > 
> > The clock source can be either provided externally or the internal clock
> > is used. Pair of clocks and clock-names property is mutally exclusive
> > with #clock-cells property.
> > 
> > Modify second example to showcase the mode where internal clock is used.
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7192.yaml     | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > index c3adc32684cf..edfa4378e838 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > @@ -42,13 +42,19 @@ 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
> > +    description:
> > +      If present when internal clock is used, configured as clock provider.
> > +
> >    interrupts:
> >      maxItems: 1
> >  
> > @@ -169,6 +175,8 @@ allOf:
> >        required:
> >          - clocks
> >          - clock-names
> > +      properties:
> > +        "#clock-cells": false  
> 
> Properties before required, otherwise
Tweaked whilst applying.

Thanks,

Jonathan

> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Cheers,
> Conor.
> 
> >  
> >  unevaluatedProperties: false
> >  
> > @@ -214,8 +222,7 @@ examples:
> >              spi-max-frequency = <1000000>;
> >              spi-cpol;
> >              spi-cpha;
> > -            clocks = <&ad7192_mclk>;
> > -            clock-names = "mclk";
> > +            #clock-cells = <0>;
> >              interrupts = <25 0x2>;
> >              interrupt-parent = <&gpio>;
> >              aincom-supply = <&aincom>;
> > -- 
> > 2.34.1
> >   

Re: [PATCH v7 4/4] iio: adc: ad7192: Add clock provider

[Jonathan Cameron]

On Thu, 18 Jul 2024 16:11:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2024-07-18 at 00:25 +0300, Alisa-Dariana Roman wrote:
> > Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> > clock provider to support this functionality when clock cells property
> > is present.
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---  
> 
> minor thing below you may consider if a re-spin is needed...
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>

> > +static int ad7192_register_clk_provider(struct ad7192_state *st)
> > +{
> > +	struct device *dev = &st->sd.spi->dev;
> > +	struct clk_init_data init = {};
> > +	int ret;
> > +
> > +	if (!device_property_present(dev, "#clock-cells"))
> > +		return 0;
> > +
> > +	if (!IS_ENABLED(CONFIG_COMMON_CLK))
> > +		return 0;
> >   
> 
> nit: This could be the first test to do. No point in calling
> device_property_present() if CONFIG_COMMON_CLK is disabled. FWIW, the compiler should
> be smart enough to sort things out but it would still be better (for readability) to
> have this first.
> 
Tweaked whilst applying.

Compiler probably can't figure it out as won't have enough visibility of the
implementation of device_property_present() to know it doesn't have side effects
deep in one of the indirect function calls that can generate.

Jonathan


> - Nuno Sá
> 
> 

Re: [PATCH v7 0/4] iio: adc: adc7192: Improvements

[Jonathan Cameron]

On Thu, 18 Jul 2024 00:36:23 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> cc: Alexandru Ardelean <aardelean@baylibre.com>
> 

I've queued this up, but it'll only be in my testing branch until rc1 is available
so plenty of time for additional review etc.

Applied to the testing branch of iio.git so 0-day can start looking for things
we missed.

Thanks,

Jonathan

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

[Alisa-Dariana Roman]

Thank you for suggesting and taking care of the tweaks!

I just wanted to point out that some little stray changes found their way into this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=42991c882f7f264ff8533c289edd015bbc0bc5a7

Kind regards,
Alisa-Dariana Roman.

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

[Jonathan Cameron]

On Sun, 21 Jul 2024 16:35:14 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Thank you for suggesting and taking care of the tweaks!
> 
> I just wanted to point out that some little stray changes found their way into this commit:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=42991c882f7f264ff8533c289edd015bbc0bc5a7

oops.  Messed up rebase mess.  Anyhow, should now be fixed.
Thanks!

Jonathan

> 
> Kind regards,
> Alisa-Dariana Roman.
>