[PATCH v6 0/6] iio: adc: ad7192: Improvements

[PATCH v6 0/6] iio: adc: ad7192: Improvements

[Alisa-Dariana Roman]

Dear everyone,

Thank you very much for your feedback!

The series is based on testing branch, since the initial first commit
was applied there.

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

Conor, please note that I didn't add your Reviewed-by tag, since I
modified that commit in this series.

Kind regards,
Alisa-Dariana Roman.

v5 -> v6
	add my SoB to David's commit
	add restrictions to the bindings patches and also modify their
commit messages

v5: https://lore.kernel.org/all/20240618142138.520192-1-alisa.roman@analog.com/

[PATCH v6 1/6] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[Alisa-Dariana Roman]

From: David Lechner <dlechner@baylibre.com>

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Error messages have changed slightly since there are now fewer places
where we print an error. The rest of the logic of selecting which
supply to use as the reference voltage remains the same.

Also 1000 is replaced by MILLI in a few places for consistency.

Signed-off-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/ad7192.c | 103 ++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index c7fb51a90e87..334ab90991d4 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -200,8 +200,6 @@ struct ad7192_chip_info {
 
 struct ad7192_state {
 	const struct ad7192_chip_info	*chip_info;
-	struct regulator		*avdd;
-	struct regulator		*vref;
 	struct clk			*mclk;
 	u16				int_vref_mv;
 	u32				aincom_mv;
@@ -1189,18 +1187,12 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	},
 };
 
-static void ad7192_reg_disable(void *reg)
-{
-	regulator_disable(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;
+	int ret, avdd_mv;
 
 	if (!spi->irq)
 		return dev_err_probe(dev, -ENODEV, "Failed to get IRQ\n");
@@ -1218,72 +1210,49 @@ 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(dev, "aincom");
-	if (IS_ERR(aincom)) {
-		if (PTR_ERR(aincom) != -ENODEV)
-			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(dev, ret,
-					     "Failed to enable specified AINCOM supply\n");
-
-		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(dev, ret,
-					     "Device tree error, AINCOM voltage undefined\n");
-		st->aincom_mv = ret / MILLI;
+	ret = devm_regulator_get_enable_read_voltage(dev, "aincom");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get AINCOM voltage\n");
+
+	st->aincom_mv = ret == -ENODEV ? 0 : ret / MILLI;
+
+	/* AVDD can optionally be used as reference voltage */
+	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
+	if (ret == -ENODEV || ret == -EINVAL) {
+		int ret2;
+
+		/*
+		 * We get -EINVAL if avdd is a supply with unknown voltage. We
+		 * still need to enable it since it is also a power supply.
+		 */
+		ret2 = devm_regulator_get_enable(dev, "avdd");
+		if (ret2)
+			return dev_err_probe(dev, ret2,
+					     "Failed to enable AVDD supply\n");
+	} else if (ret < 0) {
+		return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
 	}
 
-	st->avdd = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(st->avdd))
-		return PTR_ERR(st->avdd);
-
-	ret = regulator_enable(st->avdd);
-	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to enable specified AVdd supply\n");
-
-	ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
-	if (ret)
-		return ret;
+	avdd_mv = ret == -ENODEV || ret == -EINVAL ? 0 : ret / MILLI;
 
 	ret = devm_regulator_get_enable(dev, "dvdd");
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
 
-	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(dev, ret,
-					     "Device tree error, AVdd voltage undefined\n");
-	} else {
-		ret = regulator_enable(st->vref);
-		if (ret)
-			return dev_err_probe(dev, ret,
-					     "Failed to enable specified Vref supply\n");
-
-		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(dev, ret,
-					     "Device tree error, Vref voltage undefined\n");
+	/*
+	 * This is either REFIN1 or REFIN2 depending on adi,refin2-pins-enable.
+	 * If this supply is not present, fall back to AVDD as reference.
+	 */
+	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (ret == -ENODEV) {
+		if (avdd_mv == 0)
+			return dev_err_probe(dev, -ENODEV,
+					     "No reference voltage available\n");
+	} else if (ret < 0) {
+		return ret;
 	}
-	st->int_vref_mv = ret / 1000;
+
+	st->int_vref_mv = ret == -ENODEV ? avdd_mv : ret / MILLI;
 
 	st->chip_info = spi_get_device_match_data(spi);
 	indio_dev->name = st->chip_info->name;
-- 
2.34.1

Re: [PATCH v6 1/6] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[Jonathan Cameron]

On Mon, 24 Jun 2024 15:49:36 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> From: David Lechner <dlechner@baylibre.com>
> 
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
> 
> Error messages have changed slightly since there are now fewer places
> where we print an error. The rest of the logic of selecting which
> supply to use as the reference voltage remains the same.
> 
> Also 1000 is replaced by MILLI in a few places for consistency.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

On basis of nibbling away at outstanding patch sets to reduce the size
of v7 (assuming there is one) I'm going to pick up what I can now.
Basically we have a lot in flight and it's coming to the end of this
cycle so I want to be able to focus on the bits that need more eyes!

Applied this patch to the togreg branch of iio.git and pushed out as
testing for 0-day to poke at it.

Thanks,

Jonathan

[PATCH v6 2/6] 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>
---
 .../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

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

[Conor Dooley]

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

On Mon, Jun 24, 2024 at 03:49:37PM +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.
> 
> 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>

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

[PATCH v6 3/6] 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 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"
+};
+
+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)
@@ -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

Re: [PATCH v6 3/6] iio: adc: ad7192: Update clock config

[Alexandru Ardelean]

On Mon, Jun 24, 2024 at 3:50 PM Alisa-Dariana Roman
<alisadariana@gmail.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@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 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
>
>

Re: [PATCH v6 3/6] iio: adc: ad7192: Update clock config

[Jonathan Cameron]

On Tue, 25 Jun 2024 08:30:47 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:

> On Mon, Jun 24, 2024 at 3:50 PM Alisa-Dariana Roman
> <alisadariana@gmail.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.
> 
It's indeed a gamble that no one was actually using them.  Whilst
the lack of documentation suggests that might be the case, who knows.

If it isn't a bit maintenance issue to keep the old property support
in the driver it is probably a good idea (with lots of comments to
say it's deprecated).

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

Did we lose rest of sentence here?

J

[PATCH v6 4/6] 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.

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   | 15 ++++++++++++---
 1 file changed, 12 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..384bff7e9bb7 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -42,13 +42,17 @@ 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
+
   interrupts:
     maxItems: 1
 
@@ -169,6 +173,12 @@ allOf:
       required:
         - clocks
         - clock-names
+  - oneOf:
+      - required:
+          - clocks
+          - clock-names
+      - required:
+          - "#clock-cells"
 
 unevaluatedProperties: false
 
@@ -214,8 +224,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 v6 4/6] dt-bindings: iio: adc: ad7192: Add clock provider

[Conor Dooley]

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

On Mon, Jun 24, 2024 at 03:49:39PM +0300, Alisa-Dariana Roman wrote:
> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality.
> 
> 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.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

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

[Jonathan Cameron]

On Mon, 24 Jun 2024 15:49:39 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality.
> 
> 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   | 15 ++++++++++++---
>  1 file changed, 12 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..384bff7e9bb7 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -42,13 +42,17 @@ 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
> +
>    interrupts:
>      maxItems: 1
>  
> @@ -169,6 +173,12 @@ allOf:
>        required:
>          - clocks
>          - clock-names
> +  - oneOf:
> +      - required:
> +          - clocks
> +          - clock-names
> +      - required:
> +          - "#clock-cells"

Just a heads up that in the next patch discussion we are considering if the
driver should fail to probe or not if the clock provider stuff isn't here.

This is a bit like io-channels-cells where we have listed it as required
for some devices but not others and left that at discretion of the the
binding writer as it often reflects likely rather than possible usecases.

Here though I think this is possibly a backwards compatibility break we
don't need to make.

Jonathan

>  
>  unevaluatedProperties: false
>  
> @@ -214,8 +224,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>;

[PATCH v6 5/6] 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 | 89 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 940517df5429..90763c14679d 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;
@@ -401,6 +403,88 @@ 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 (!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;
@@ -412,6 +496,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");
 	} else {
 		st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
 
-- 
2.34.1

Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider

[Alexandru Ardelean]

On Mon, Jun 24, 2024 at 3:51 PM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> clock provider to support this functionality.
>

Just a question that should be addressed by the failing of the
clock-provider registration.
With that addressed:

Reviewed-by: Alexandru Ardelean <aardelean@baylibre.com>

> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  drivers/iio/adc/ad7192.c | 89 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 940517df5429..90763c14679d 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;
> @@ -401,6 +403,88 @@ 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 (!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;
> @@ -412,6 +496,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");

A question here: do we want to fail the probe of this driver when it
cannot register a clock provider?
Or should we ignore it?
No preference from my side.

>         } else {
>                 st->clock_sel = AD7192_CLK_EXT_MCLK1_2 + ret;
>
> --
> 2.34.1
>
>

Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider

[Nuno Sá]

On Tue, 2024-06-25 at 08:48 +0300, Alexandru Ardelean wrote:
> On Mon, Jun 24, 2024 at 3:51 PM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
> > 
> > Internal clock of AD719X devices can be made available on MCLK2 pin. Add
> > clock provider to support this functionality.
> > 
> 
> Just a question that should be addressed by the failing of the
> clock-provider registration.
> With that addressed:
> 
> Reviewed-by: Alexandru Ardelean <aardelean@baylibre.com>
> 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  drivers/iio/adc/ad7192.c | 89 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 940517df5429..90763c14679d 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;
> > @@ -401,6 +403,88 @@ 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 (!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;
> > @@ -412,6 +496,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");
> 
> A question here: do we want to fail the probe of this driver when it
> cannot register a clock provider?
> Or should we ignore it?
> No preference from my side.

Sensible question... I would say it depends. On one side this is an optional
feature so we should not (arguably) error out. OTOH, someone may really want
(and relies on) this feature so failing makes sense.

Maybe we should have

if (!device_property_present(&spi->dev, "#clock-cells"))
	return 0;

in ad7192_register_clk_provider(). So that if we fail the function, then yes, we
should fail probing as FW wants this to be a provider. Also, not providing
#clock-cells means we don't register the clock.

Having said the above I think that failing devm_clk_hw_register() means that
something is already really wrong (or we have a bug in the driver) so likely we
should keep it simple and just always provide the clock and return an error if
we fail to do so.

my 2 cents...

- Nuno Sá

Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider

[Jonathan Cameron]

> > > +
> > >  static int ad7192_clock_setup(struct ad7192_state *st)
> > >  {
> > >         struct device *dev = &st->sd.spi->dev;
> > > @@ -412,6 +496,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");  
> > 
> > A question here: do we want to fail the probe of this driver when it
> > cannot register a clock provider?
> > Or should we ignore it?
> > No preference from my side.  
> 
> Sensible question... I would say it depends. On one side this is an optional
> feature so we should not (arguably) error out. OTOH, someone may really want
> (and relies on) this feature so failing makes sense.
> 
> Maybe we should have
> 
> if (!device_property_present(&spi->dev, "#clock-cells"))
> 	return 0;

I'm not 100% sure from looking at the code, but if the absence of this property
(because the DT writer doesn't care about this) is sufficient to make the
calls in ad7192_register_clk_provider() fail then we should check this.
I don't think we need the complexity of get_provider_clk_node() as there is
no reason to look in a parent of this device (it's not an mfd or similar) so
this check should be sufficient.

Does this also mean the binding should not require this?  I suspect it shouldn't.
 
> 
> in ad7192_register_clk_provider(). So that if we fail the function, then yes, we
> should fail probing as FW wants this to be a provider. Also, not providing
> #clock-cells means we don't register the clock.
> 
> Having said the above I think that failing devm_clk_hw_register() means that
> something is already really wrong (or we have a bug in the driver) so likely we
> should keep it simple and just always provide the clock and return an error if
> we fail to do so.
> 
> my 2 cents...
> 
> - Nuno Sá
> 
> 

Re: [PATCH v6 5/6] iio: adc: ad7192: Add clock provider

[Conor Dooley]

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

On Sun, Jun 30, 2024 at 10:54:48AM +0100, Jonathan Cameron wrote:
> 
> > > > +
> > > >  static int ad7192_clock_setup(struct ad7192_state *st)
> > > >  {
> > > >         struct device *dev = &st->sd.spi->dev;
> > > > @@ -412,6 +496,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");  
> > > 
> > > A question here: do we want to fail the probe of this driver when it
> > > cannot register a clock provider?
> > > Or should we ignore it?
> > > No preference from my side.  
> > 
> > Sensible question... I would say it depends. On one side this is an optional
> > feature so we should not (arguably) error out. OTOH, someone may really want
> > (and relies on) this feature so failing makes sense.
> > 
> > Maybe we should have
> > 
> > if (!device_property_present(&spi->dev, "#clock-cells"))
> > 	return 0;
> 
> I'm not 100% sure from looking at the code, but if the absence of this property
> (because the DT writer doesn't care about this) is sufficient to make the
> calls in ad7192_register_clk_provider() fail then we should check this.
> I don't think we need the complexity of get_provider_clk_node() as there is
> no reason to look in a parent of this device (it's not an mfd or similar) so
> this check should be sufficient.
> 
> Does this also mean the binding should not require this?  I suspect it shouldn't.

Per the binding (proposed and current) I think the code here is fine
w.r.t. probe failures. Before the series, it looks like clocks/clock-names
were required by the binding and the driver would fail to probe if they were
not provided. The current code only fails to probe if neither clocks
or clock-names and #clock-cells are not provided, so it is a weaker
restriction than before. The binding doesn't require #clock-cells at all
times, only if the clock consumer properties are not present, so it is
both fine backwards compatibility wise and seems to match how the driver
is behaving. I'm biased, but I don't buy "the DT writer doesn't care" as
an argument cos if they didn't care about adding the required clock
consumer properties now then they'd not have probed before either...

Cheers,
Conor.

> > in ad7192_register_clk_provider(). So that if we fail the function, then yes, we
> > should fail probing as FW wants this to be a provider. Also, not providing
> > #clock-cells means we don't register the clock.
> > 
> > Having said the above I think that failing devm_clk_hw_register() means that
> > something is already really wrong (or we have a bug in the driver) so likely we
> > should keep it simple and just always provide the clock and return an error if
> > we fail to do so.
> > 
> > my 2 cents...
> > 
> > - Nuno Sá
> > 
> > 
> 

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

[PATCH v6 6/6] MAINTAINERS: Update AD7192 driver maintainer

[Alisa-Dariana Roman]

Alexandru Tachici has not been active. Also the email address included
is not reachable anymore. I was assigned to work on the driver instead.

Remove Alexandru Tachici and add myself as maintainer of AD7192 driver.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9517093d889d..ab1e82fd3b76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1217,7 +1217,7 @@ F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r*
 F:	drivers/iio/adc/ad7091r*
 
 ANALOG DEVICES INC AD7192 DRIVER
-M:	Alexandru Tachici <alexandru.tachici@analog.com>
+M:	Alisa-Dariana Roman <alisa.roman@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
-- 
2.34.1

Re: [PATCH v6 6/6] MAINTAINERS: Update AD7192 driver maintainer

[Jonathan Cameron]

On Mon, 24 Jun 2024 15:49:41 +0300
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Alexandru Tachici has not been active. Also the email address included
> is not reachable anymore. I was assigned to work on the driver instead.
> 
> Remove Alexandru Tachici and add myself as maintainer of AD7192 driver.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
I'll pick this one up as well.

Applied to the togreg branch of iio.git and pushed out as testing because
other stuff on the tree needs build tests.  

Jonathan

> ---
>  MAINTAINERS | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9517093d889d..ab1e82fd3b76 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1217,7 +1217,7 @@ F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r*
>  F:	drivers/iio/adc/ad7091r*
>  
>  ANALOG DEVICES INC AD7192 DRIVER
> -M:	Alexandru Tachici <alexandru.tachici@analog.com>
> +M:	Alisa-Dariana Roman <alisa.roman@analog.com>
>  L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers