[PATCH 0/5] iio: addac: ad74413r: various fixups

[PATCH 0/5] iio: addac: ad74413r: various fixups

[Rasmus Villemoes]

These patches

- fix a run-time warning
- make the refin-supply optional in the binding
- add a reset-gpios binding

and update the driver to support the latter two.

Rasmus Villemoes (5):
  iio: addac: ad74413r: add spi_device_id table
  dt-bindings: iio: ad74413r: make refin-supply optional
  iio: addac: ad74413r: implement support for optional refin-supply
  dt-bindings: iio: ad74413r: add optional reset-gpios
  iio: addac: ad74413r: add support for reset-gpio

 .../bindings/iio/addac/adi,ad74413r.yaml      |  4 +-
 drivers/iio/addac/ad74413r.c                  | 51 +++++++++++++++----
 2 files changed, 43 insertions(+), 12 deletions(-)

-- 
2.37.2

[PATCH 1/5] iio: addac: ad74413r: add spi_device_id table

[Rasmus Villemoes]

Silence the run-time warning

  SPI driver ad74413r has no spi_device_id for adi,ad74412r

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 899bcd83f40b..37485be88a63 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
 };
 MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
 
+static const struct spi_device_id ad74413r_spi_id[] = {
+	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
+	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ad74413r_spi_id);
+
 static struct spi_driver ad74413r_driver = {
 	.driver = {
 		   .name = "ad74413r",
 		   .of_match_table = ad74413r_dt_id,
 	},
 	.probe = ad74413r_probe,
+	.id_table = ad74413r_spi_id,
 };
 
 module_driver(ad74413r_driver,
-- 
2.37.2

[PATCH 2/5] dt-bindings: iio: ad74413r: make refin-supply optional

[Rasmus Villemoes]

The ad74412r/ad74413r has an internal 2.5V reference output, which (by
tying the REFOUT pin to the REFIN pin) can be used in lieu of an
external 2.5V input reference. So stop marking refin-supply as
required.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index 03bb90a7f4f8..e954d5ae4f4f 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -56,7 +56,6 @@ required:
   - reg
   - spi-max-frequency
   - spi-cpol
-  - refin-supply
 
 additionalProperties: false
 
-- 
2.37.2

[PATCH 3/5] iio: addac: ad74413r: implement support for optional refin-supply

[Rasmus Villemoes]

The ad74412r/ad74413r has an internal 2.5V reference output, which (by
tying the REFOUT pin to the REFIN pin) can be used in lieu of an
external 2.5V input reference.

Support that case by using devm_regulator_get_optional(), and simply
hardcode the 2500000 uV in ad74413r_get_output_current_scale().

I'm not sure this is completely correct, but it's certainly better
than the current behaviour, where when refin-supply is not defined in
device tree, the regulator framework helpfully does its

  supply refin not found, using dummy regulator

thing. When we then do the regulator_get_voltage(), that dummy
regulator of course doesn't support that operation and thus returns
-22 (-EINVAL) which is used without being checked.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 37485be88a63..9f77d2f514de 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -608,7 +608,10 @@ static int ad74413r_get_output_voltage_scale(struct ad74413r_state *st,
 static int ad74413r_get_output_current_scale(struct ad74413r_state *st,
 					     int *val, int *val2)
 {
-	*val = regulator_get_voltage(st->refin_reg);
+	if (st->refin_reg)
+		*val = regulator_get_voltage(st->refin_reg);
+	else
+		*val = 2500000;
 	*val2 = st->sense_resistor_ohms * AD74413R_DAC_CODE_MAX * 1000;
 
 	return IIO_VAL_FRACTIONAL;
@@ -1313,19 +1316,25 @@ static int ad74413r_probe(struct spi_device *spi)
 	if (IS_ERR(st->regmap))
 		return PTR_ERR(st->regmap);
 
-	st->refin_reg = devm_regulator_get(st->dev, "refin");
-	if (IS_ERR(st->refin_reg))
-		return dev_err_probe(st->dev, PTR_ERR(st->refin_reg),
-				     "Failed to get refin regulator\n");
+	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
+	if (IS_ERR(st->refin_reg)) {
+		ret = PTR_ERR(st->refin_reg);
+		if (ret != -ENODEV)
+			return dev_err_probe(st->dev, ret,
+					     "Failed to get refin regulator\n");
+		st->refin_reg = NULL;
+	}
 
-	ret = regulator_enable(st->refin_reg);
-	if (ret)
-		return ret;
+	if (st->refin_reg) {
+		ret = regulator_enable(st->refin_reg);
+		if (ret)
+			return ret;
 
-	ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable,
+		ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable,
 				       st->refin_reg);
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 
 	st->sense_resistor_ohms = 100000000;
 	device_property_read_u32(st->dev, "shunt-resistor-micro-ohms",
-- 
2.37.2

[PATCH 4/5] dt-bindings: iio: ad74413r: add optional reset-gpios

[Rasmus Villemoes]

The ad74412 and ad74413 devices have an active-low reset pin. Add a
binding allowing one to specify a gpio tied to that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index e954d5ae4f4f..70f82cc716ae 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -51,6 +51,9 @@ properties:
       Shunt (sense) resistor value in micro-Ohms.
     default: 100000000
 
+  reset-gpios:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.37.2

[PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Rasmus Villemoes]

We have a board where the reset pin of the ad74412 is connected to a
gpio, but also pulled low by default. Hence to get the chip out of
reset, the driver needs to know about that gpio and set it high before
attempting to communicate with it.

When a reset-gpio is given in device tree, use that instead of the
software reset. According to the data sheet, the two methods are
functionally equivalent.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 9f77d2f514de..af09d43f921c 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -71,6 +71,7 @@ struct ad74413r_state {
 	struct regmap			*regmap;
 	struct device			*dev;
 	struct iio_trigger		*trig;
+	struct gpio_desc		*reset_gpio;
 
 	size_t			adc_active_channels;
 	struct spi_message	adc_samples_msg;
@@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state *st)
 {
 	int ret;
 
+	if (st->reset_gpio) {
+		gpiod_set_value_cansleep(st->reset_gpio, 1);
+		fsleep(50);
+		gpiod_set_value_cansleep(st->reset_gpio, 0);
+		return 0;
+	}
+
 	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
 			   AD74413R_CMD_KEY_RESET1);
 	if (ret)
@@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device *spi)
 	if (IS_ERR(st->regmap))
 		return PTR_ERR(st->regmap);
 
+	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(st->reset_gpio))
+		return PTR_ERR(st->reset_gpio);
+
 	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
 	if (IS_ERR(st->refin_reg)) {
 		ret = PTR_ERR(st->refin_reg);
-- 
2.37.2

Re: [PATCH 1/5] iio: addac: ad74413r: add spi_device_id table

[Jonathan Cameron]

On Fri, 11 Nov 2022 15:39:17 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Silence the run-time warning
> 
>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 899bcd83f40b..37485be88a63 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
>  
> +static const struct spi_device_id ad74413r_spi_id[] = {
> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
> +	{},
Trivial, but prefer not to have a comma after a "NULL" terminator like this.
It would never make sense to add anything after it in the array.
Now you are matching existing driver style, but I'd still rather not see more
instances of this added.

Also, driver_data is not currently used. It should be because adding this
spi_id table means the driver can be probed via various routes where
device_get_match_data() == NULL. 

Hence, alongside this change you need to have a fallback to cover that case.
Something along the lines of...

	st->chip_info = device_get_match_data(..);
	if (!st->chip_info) {
		struct spi_device_id *id = spi_get_device_id();
		if (!id)
			return -EINVAL;

		st->chip_info = (void *)id->driver_data;
		//or better yet cast to the correct type I'm just too lazy to look it up ;)
		if (!st->chip_info)
			return -EINVAL;

	}
> +};
> +MODULE_DEVICE_TABLE(spi, ad74413r_spi_id);
> +
>  static struct spi_driver ad74413r_driver = {
>  	.driver = {
>  		   .name = "ad74413r",
>  		   .of_match_table = ad74413r_dt_id,
>  	},
>  	.probe = ad74413r_probe,
> +	.id_table = ad74413r_spi_id,
>  };
>  
>  module_driver(ad74413r_driver,

Re: [PATCH 2/5] dt-bindings: iio: ad74413r: make refin-supply optional

[Jonathan Cameron]

On Fri, 11 Nov 2022 15:39:18 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> The ad74412r/ad74413r has an internal 2.5V reference output, which (by
> tying the REFOUT pin to the REFIN pin) can be used in lieu of an
> external 2.5V input reference. So stop marking refin-supply as
> required.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>


Interesting corner case.  Given we have no way of knowing if the
wiring has REFOUT connected to REFIN I see two ways we should fix this.

1) Just have any DT doing this provide a fixed regulator.
2) Have the REFOUT supported as an actual regulator - in theory it might be
   wired to other devices.  This might get a little interesting ordering
   wise as we'll want to register the regulator before we try to consume
   it in the same driver.  I'm also not 100% sure there are no other issues
   in a driver providing and consuming the same regulator.

I'm not keen to just assume lack of regulator means the chip is wired
like this.  Would be a different question if this was just setting
an internal mux to use the regulator without external loop back being
needed.

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> index 03bb90a7f4f8..e954d5ae4f4f 100644
> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> @@ -56,7 +56,6 @@ required:
>    - reg
>    - spi-max-frequency
>    - spi-cpol
> -  - refin-supply
>  
>  additionalProperties: false
>  

Re: [PATCH 3/5] iio: addac: ad74413r: implement support for optional refin-supply

[Jonathan Cameron]

On Fri, 11 Nov 2022 15:39:19 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> The ad74412r/ad74413r has an internal 2.5V reference output, which (by
> tying the REFOUT pin to the REFIN pin) can be used in lieu of an
> external 2.5V input reference.
> 
> Support that case by using devm_regulator_get_optional(), and simply
> hardcode the 2500000 uV in ad74413r_get_output_current_scale().
> 
> I'm not sure this is completely correct, but it's certainly better
> than the current behaviour, where when refin-supply is not defined in
> device tree, the regulator framework helpfully does its
> 
>   supply refin not found, using dummy regulator

You could reasonably assume that's a bug in the firmware.. See suggestions
in reply to patch 2.  Given external wiring is involved, I don't think
we can assume absence of a regulator means that loop back is in place.
We need to indicate that explicitly in the binding in some way.

Jonathan


> 
> thing. When we then do the regulator_get_voltage(), that dummy
> regulator of course doesn't support that operation and thus returns
> -22 (-EINVAL) which is used without being checked.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 37485be88a63..9f77d2f514de 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -608,7 +608,10 @@ static int ad74413r_get_output_voltage_scale(struct ad74413r_state *st,
>  static int ad74413r_get_output_current_scale(struct ad74413r_state *st,
>  					     int *val, int *val2)
>  {
> -	*val = regulator_get_voltage(st->refin_reg);
> +	if (st->refin_reg)
> +		*val = regulator_get_voltage(st->refin_reg);
> +	else
> +		*val = 2500000;
>  	*val2 = st->sense_resistor_ohms * AD74413R_DAC_CODE_MAX * 1000;
>  
>  	return IIO_VAL_FRACTIONAL;
> @@ -1313,19 +1316,25 @@ static int ad74413r_probe(struct spi_device *spi)
>  	if (IS_ERR(st->regmap))
>  		return PTR_ERR(st->regmap);
>  
> -	st->refin_reg = devm_regulator_get(st->dev, "refin");
> -	if (IS_ERR(st->refin_reg))
> -		return dev_err_probe(st->dev, PTR_ERR(st->refin_reg),
> -				     "Failed to get refin regulator\n");
> +	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
> +	if (IS_ERR(st->refin_reg)) {
> +		ret = PTR_ERR(st->refin_reg);
> +		if (ret != -ENODEV)
> +			return dev_err_probe(st->dev, ret,
> +					     "Failed to get refin regulator\n");
> +		st->refin_reg = NULL;
> +	}
>  
> -	ret = regulator_enable(st->refin_reg);
> -	if (ret)
> -		return ret;
> +	if (st->refin_reg) {
> +		ret = regulator_enable(st->refin_reg);
> +		if (ret)
> +			return ret;
>  
> -	ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable,
> +		ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable,
>  				       st->refin_reg);
> -	if (ret)
> -		return ret;
> +		if (ret)
> +			return ret;
> +	}
>  
>  	st->sense_resistor_ohms = 100000000;
>  	device_property_read_u32(st->dev, "shunt-resistor-micro-ohms",

Re: [PATCH 4/5] dt-bindings: iio: ad74413r: add optional reset-gpios

[Jonathan Cameron]

On Fri, 11 Nov 2022 15:39:20 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> The ad74412 and ad74413 devices have an active-low reset pin. Add a
> binding allowing one to specify a gpio tied to that.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> index e954d5ae4f4f..70f82cc716ae 100644
> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> @@ -51,6 +51,9 @@ properties:
>        Shunt (sense) resistor value in micro-Ohms.
>      default: 100000000
>  
> +  reset-gpios:
> +    maxItems: 1
> +

Probably good to add to the example as well.

>  required:
>    - compatible
>    - reg

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Jonathan Cameron]

On Fri, 11 Nov 2022 15:39:21 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> We have a board where the reset pin of the ad74412 is connected to a
> gpio, but also pulled low by default. Hence to get the chip out of
> reset, the driver needs to know about that gpio and set it high before
> attempting to communicate with it.

I'm a little confused on polarity here.  The pin is a !reset so
we need to drive it low briefly to trigger a reset.
I'm guessing for your board the pin is set to active low? (an example
in the dt would have made that clearer) Hence the pulse
in here to 1 is actually briefly driving it low before restoring to high?

For a pin documented as !reset that seems backwards though you have
called it reset so that is fine, but this description doesn't make that
celar.

Perhaps just add some more description here to make it clear the GPIO
is active low, and then refer to setting it to true and false to avoid
the confusing high / low terminology which are inverted...

> 
> When a reset-gpio is given in device tree, use that instead of the
> software reset. According to the data sheet, the two methods are
> functionally equivalent.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index 9f77d2f514de..af09d43f921c 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -71,6 +71,7 @@ struct ad74413r_state {
>  	struct regmap			*regmap;
>  	struct device			*dev;
>  	struct iio_trigger		*trig;
> +	struct gpio_desc		*reset_gpio;
>  
>  	size_t			adc_active_channels;
>  	struct spi_message	adc_samples_msg;
> @@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state *st)
>  {
>  	int ret;
>  
> +	if (st->reset_gpio) {
> +		gpiod_set_value_cansleep(st->reset_gpio, 1);
> +		fsleep(50);
> +		gpiod_set_value_cansleep(st->reset_gpio, 0);
> +		return 0;
> +	}
> +
>  	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
>  			   AD74413R_CMD_KEY_RESET1);
>  	if (ret)
> @@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device *spi)
>  	if (IS_ERR(st->regmap))
>  		return PTR_ERR(st->regmap);
>  
> +	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->reset_gpio))
> +		return PTR_ERR(st->reset_gpio);
> +
>  	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
>  	if (IS_ERR(st->refin_reg)) {
>  		ret = PTR_ERR(st->refin_reg);

Re: [PATCH 1/5] iio: addac: ad74413r: add spi_device_id table

[Rasmus Villemoes]

On 12/11/2022 17.50, Jonathan Cameron wrote:
> On Fri, 11 Nov 2022 15:39:17 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> Silence the run-time warning
>>
>>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  drivers/iio/addac/ad74413r.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
>> index 899bcd83f40b..37485be88a63 100644
>> --- a/drivers/iio/addac/ad74413r.c
>> +++ b/drivers/iio/addac/ad74413r.c
>> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
>>  
>> +static const struct spi_device_id ad74413r_spi_id[] = {
>> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
>> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
>> +	{},
> Trivial, but prefer not to have a comma after a "NULL" terminator like this.
> It would never make sense to add anything after it in the array.

I agree and wouldn't have added it if it weren't for the existing case
in the other table.

> Now you are matching existing driver style, but I'd still rather not see more
> instances of this added.

Sure.

> Also, driver_data is not currently used. It should be because adding this
> spi_id table means the driver can be probed via various routes where
> device_get_match_data() == NULL. 

That makes sense, I think I thought that that would somehow happen
automatically. Looking through the history of similar fixes, I see that
for example 3f8dd0a7dc does indeed add code as you suggest, but
855fe49984 does not (and also doesn't add the corresponding .driver_data
initializers in the spi table). They may very well both be correct, but
looping in Oleksij for good measure.

> Hence, alongside this change you need to have a fallback to cover that case.
> Something along the lines of...
> 
> 	st->chip_info = device_get_match_data(..);
> 	if (!st->chip_info) {
> 		struct spi_device_id *id = spi_get_device_id();
> 		if (!id)
> 			return -EINVAL;
> 
> 		st->chip_info = (void *)id->driver_data;
> 		//or better yet cast to the correct type I'm just too lazy to look it up ;)
> 		if (!st->chip_info)
> 			return -EINVAL;
> 
> 	}

Thanks,
Rasmus

Re: [PATCH 2/5] dt-bindings: iio: ad74413r: make refin-supply optional

[Rasmus Villemoes]

On 12/11/2022 17.54, Jonathan Cameron wrote:
> On Fri, 11 Nov 2022 15:39:18 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> The ad74412r/ad74413r has an internal 2.5V reference output, which (by
>> tying the REFOUT pin to the REFIN pin) can be used in lieu of an
>> external 2.5V input reference. So stop marking refin-supply as
>> required.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> 
> Interesting corner case.  Given we have no way of knowing if the
> wiring has REFOUT connected to REFIN I see two ways we should fix this.
> 
> 1) Just have any DT doing this provide a fixed regulator.
> 2) Have the REFOUT supported as an actual regulator - in theory it might be
>    wired to other devices.  This might get a little interesting ordering
>    wise as we'll want to register the regulator before we try to consume
>    it in the same driver.  I'm also not 100% sure there are no other issues
>    in a driver providing and consuming the same regulator.

Hm, I don't like the idea of exposing REFOUT as a real regulator. As you
write, there's gonna be interesting chicken-and-egg problems, and I also
don't think it can actually deliver any meaningful current, i.e. it
can't really (and shouldn't) be used for supplying other peripherals.

A third option is to have a boolean property to explicitly indicate that
"yes, we're using refout as refin", and then make the requirement in the
schema be "refin-supply XOR refout-as-refin".

But I think the simplest is (1), I will just add a fixed-regulator with
a suitable comment in my .dts, and patches 2,3 can be ignored.

Thanks,
Rasmus

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Rasmus Villemoes]

On 12/11/2022 18.07, Jonathan Cameron wrote:
> On Fri, 11 Nov 2022 15:39:21 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> We have a board where the reset pin of the ad74412 is connected to a
>> gpio, but also pulled low by default. Hence to get the chip out of
>> reset, the driver needs to know about that gpio and set it high before
>> attempting to communicate with it.
> 
> I'm a little confused on polarity here.  The pin is a !reset so
> we need to drive it low briefly to trigger a reset.
> I'm guessing for your board the pin is set to active low? (an example
> in the dt would have made that clearer) Hence the pulse
> in here to 1 is actually briefly driving it low before restoring to high?

Yes. I actually thought that was pretty standard. I do indeed have
something like

  reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;

in my .dts, so setting the gpio value to 1 (logically asserting its
function) will end up driving the signal low, and setting it to 0
(de-asserting reset) will set the signal high. I will add that line to
the example in the binding.

> For a pin documented as !reset that seems backwards 

Well, it depends on where the knowledge of the pin being active low
belongs. In this case, the driver itself handles the gpio so it could be
done both ways.

But if, for example, the iio framework would handle an optional
reset-gpio for each device, it couldn't possibly know whether to set it
to 1 or 0 for a given device, it could only set it logic 1 to assert
reset and then rely on DT gpio descriptor to include the active low/high
info.

Also, see the "The active low and open drain semantics" section in
Documentation/driver-api/gpio/consumer.rst.

Rasmus

RE: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Tanislav, Cosmin]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, November 12, 2022 7:07 PM
> To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio
> 
> [External]
> 
> On Fri, 11 Nov 2022 15:39:21 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > We have a board where the reset pin of the ad74412 is connected to a
> > gpio, but also pulled low by default. Hence to get the chip out of
> > reset, the driver needs to know about that gpio and set it high before
> > attempting to communicate with it.
> 
> I'm a little confused on polarity here.  The pin is a !reset so
> we need to drive it low briefly to trigger a reset.
> I'm guessing for your board the pin is set to active low? (an example
> in the dt would have made that clearer) Hence the pulse
> in here to 1 is actually briefly driving it low before restoring to high?
> 
> For a pin documented as !reset that seems backwards though you have
> called it reset so that is fine, but this description doesn't make that
> celar.

My opinion is that the driver shouldn't exactly know the polarity of the reset,
and just assume that setting the reset GPIO to 1 means putting it in reset,
and setting it to 0 means bringing out of reset.

> 
> Perhaps just add some more description here to make it clear the GPIO
> is active low, and then refer to setting it to true and false to avoid
> the confusing high / low terminology which are inverted...
> 
> >
> > When a reset-gpio is given in device tree, use that instead of the
> > software reset. According to the data sheet, the two methods are
> > functionally equivalent.
> >
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > ---
> >  drivers/iio/addac/ad74413r.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> > index 9f77d2f514de..af09d43f921c 100644
> > --- a/drivers/iio/addac/ad74413r.c
> > +++ b/drivers/iio/addac/ad74413r.c
> > @@ -71,6 +71,7 @@ struct ad74413r_state {
> >  	struct regmap			*regmap;
> >  	struct device			*dev;
> >  	struct iio_trigger		*trig;
> > +	struct gpio_desc		*reset_gpio;
> >
> >  	size_t			adc_active_channels;
> >  	struct spi_message	adc_samples_msg;
> > @@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state
> *st)
> >  {
> >  	int ret;
> >
> > +	if (st->reset_gpio) {
> > +		gpiod_set_value_cansleep(st->reset_gpio, 1);
> > +		fsleep(50);
> > +		gpiod_set_value_cansleep(st->reset_gpio, 0);
> > +		return 0;
> > +	}
> > +
> >  	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
> >  			   AD74413R_CMD_KEY_RESET1);
> >  	if (ret)
> > @@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device *spi)
> >  	if (IS_ERR(st->regmap))
> >  		return PTR_ERR(st->regmap);
> >
> > +	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset",
> GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->reset_gpio))
> > +		return PTR_ERR(st->reset_gpio);
> > +
> >  	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
> >  	if (IS_ERR(st->refin_reg)) {
> >  		ret = PTR_ERR(st->refin_reg);

Re: [PATCH 1/5] iio: addac: ad74413r: add spi_device_id table

[Jonathan Cameron]

On Mon, 14 Nov 2022 09:02:44 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 12/11/2022 17.50, Jonathan Cameron wrote:
> > On Fri, 11 Nov 2022 15:39:17 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> >> Silence the run-time warning
> >>
> >>   SPI driver ad74413r has no spi_device_id for adi,ad74412r
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  drivers/iio/addac/ad74413r.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> >> index 899bcd83f40b..37485be88a63 100644
> >> --- a/drivers/iio/addac/ad74413r.c
> >> +++ b/drivers/iio/addac/ad74413r.c
> >> @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
> >>  };
> >>  MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
> >>  
> >> +static const struct spi_device_id ad74413r_spi_id[] = {
> >> +	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
> >> +	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
> >> +	{},  
> > Trivial, but prefer not to have a comma after a "NULL" terminator like this.
> > It would never make sense to add anything after it in the array.  
> 
> I agree and wouldn't have added it if it weren't for the existing case
> in the other table.
> 
> > Now you are matching existing driver style, but I'd still rather not see more
> > instances of this added.  
> 
> Sure.
> 
> > Also, driver_data is not currently used. It should be because adding this
> > spi_id table means the driver can be probed via various routes where
> > device_get_match_data() == NULL.   
> 
> That makes sense, I think I thought that that would somehow happen
> automatically. Looking through the history of similar fixes, I see that
> for example 3f8dd0a7dc does indeed add code as you suggest, but
> 855fe49984 does not (and also doesn't add the corresponding .driver_data
> initializers in the spi table). They may very well both be correct, but
> looping in Oleksij for good measure.

It depends a bit on whether there is any plausible chance of anyone making
use of say greybus with the device (I think that still relies on the spi id
though not checked recently).  If not and no board files exist, chances are
that all is needed is the id table (to make autoprobing of modules work).
Still I'd not leave the opening for the unexpected to happen given users
have an annoying habit of finding the corner cases :)

Jonathan

> 
> > Hence, alongside this change you need to have a fallback to cover that case.
> > Something along the lines of...
> > 
> > 	st->chip_info = device_get_match_data(..);
> > 	if (!st->chip_info) {
> > 		struct spi_device_id *id = spi_get_device_id();
> > 		if (!id)
> > 			return -EINVAL;
> > 
> > 		st->chip_info = (void *)id->driver_data;
> > 		//or better yet cast to the correct type I'm just too lazy to look it up ;)
> > 		if (!st->chip_info)
> > 			return -EINVAL;
> > 
> > 	}  
> 
> Thanks,
> Rasmus
> 

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Jonathan Cameron]

On Mon, 14 Nov 2022 09:37:59 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 12/11/2022 18.07, Jonathan Cameron wrote:
> > On Fri, 11 Nov 2022 15:39:21 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> >> We have a board where the reset pin of the ad74412 is connected to a
> >> gpio, but also pulled low by default. Hence to get the chip out of
> >> reset, the driver needs to know about that gpio and set it high before
> >> attempting to communicate with it.  
> > 
> > I'm a little confused on polarity here.  The pin is a !reset so
> > we need to drive it low briefly to trigger a reset.
> > I'm guessing for your board the pin is set to active low? (an example
> > in the dt would have made that clearer) Hence the pulse
> > in here to 1 is actually briefly driving it low before restoring to high?  
> 
> Yes. I actually thought that was pretty standard. I do indeed have
> something like
> 
>   reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
> 
> in my .dts, so setting the gpio value to 1 (logically asserting its
> function) will end up driving the signal low, and setting it to 0
> (de-asserting reset) will set the signal high. I will add that line to
> the example in the binding.
> 
> > For a pin documented as !reset that seems backwards   
> 
> Well, it depends on where the knowledge of the pin being active low
> belongs. In this case, the driver itself handles the gpio so it could be
> done both ways.
> 
> But if, for example, the iio framework would handle an optional
> reset-gpio for each device, it couldn't possibly know whether to set it
> to 1 or 0 for a given device, it could only set it logic 1 to assert
> reset and then rely on DT gpio descriptor to include the active low/high
> info.
> 
> Also, see the "The active low and open drain semantics" section in
> Documentation/driver-api/gpio/consumer.rst.

Throw in an example in the dt-binding and I'm fine with this as it
stands.

Jonathan

> 
> Rasmus
> 

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Jonathan Cameron]

On Mon, 14 Nov 2022 13:52:26 +0000
"Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, November 12, 2022 7:07 PM
> > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio
> > 
> > [External]
> > 
> > On Fri, 11 Nov 2022 15:39:21 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> > > We have a board where the reset pin of the ad74412 is connected to a
> > > gpio, but also pulled low by default. Hence to get the chip out of
> > > reset, the driver needs to know about that gpio and set it high before
> > > attempting to communicate with it.  
> > 
> > I'm a little confused on polarity here.  The pin is a !reset so
> > we need to drive it low briefly to trigger a reset.
> > I'm guessing for your board the pin is set to active low? (an example
> > in the dt would have made that clearer) Hence the pulse
> > in here to 1 is actually briefly driving it low before restoring to high?
> > 
> > For a pin documented as !reset that seems backwards though you have
> > called it reset so that is fine, but this description doesn't make that
> > celar.  
> 
> My opinion is that the driver shouldn't exactly know the polarity of the reset,
> and just assume that setting the reset GPIO to 1 means putting it in reset,
> and setting it to 0 means bringing out of reset.

Agreed. I'd just like a comment + example in the dt-binding to make the point
that the pin is !reset.

Preferably with an example in the dt binding of the common case of it being wired
up to an active low pin.

The main oddity here is the need to pulse it rather than request it directly as
in the reset state and then just set that to off.

Jonathan
> 
> > 
> > Perhaps just add some more description here to make it clear the GPIO
> > is active low, and then refer to setting it to true and false to avoid
> > the confusing high / low terminology which are inverted...


> >   
> > >
> > > When a reset-gpio is given in device tree, use that instead of the
> > > software reset. According to the data sheet, the two methods are
> > > functionally equivalent.
> > >
> > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > ---
> > >  drivers/iio/addac/ad74413r.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> > > index 9f77d2f514de..af09d43f921c 100644
> > > --- a/drivers/iio/addac/ad74413r.c
> > > +++ b/drivers/iio/addac/ad74413r.c
> > > @@ -71,6 +71,7 @@ struct ad74413r_state {
> > >  	struct regmap			*regmap;
> > >  	struct device			*dev;
> > >  	struct iio_trigger		*trig;
> > > +	struct gpio_desc		*reset_gpio;
> > >
> > >  	size_t			adc_active_channels;
> > >  	struct spi_message	adc_samples_msg;
> > > @@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state  
> > *st)  
> > >  {
> > >  	int ret;
> > >
> > > +	if (st->reset_gpio) {
> > > +		gpiod_set_value_cansleep(st->reset_gpio, 1);
> > > +		fsleep(50);
> > > +		gpiod_set_value_cansleep(st->reset_gpio, 0);
> > > +		return 0;
> > > +	}
> > > +
> > >  	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
> > >  			   AD74413R_CMD_KEY_RESET1);
> > >  	if (ret)
> > > @@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device *spi)
> > >  	if (IS_ERR(st->regmap))
> > >  		return PTR_ERR(st->regmap);
> > >
> > > +	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset",  
> > GPIOD_OUT_LOW);  
> > > +	if (IS_ERR(st->reset_gpio))
> > > +		return PTR_ERR(st->reset_gpio);
> > > +
> > >  	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
> > >  	if (IS_ERR(st->refin_reg)) {
> > >  		ret = PTR_ERR(st->refin_reg);  
> 

[PATCH v2 0/3] iio: addac: ad74413r: add spi device id table, support reset gpio

[Rasmus Villemoes]

Fix a run-time warning from the SPI core by providing a spi_device_id
table, and add binding and driver support for a reset gpio.

v2:
- drop the two patches related to refin-supply
- fall back to getting device data from the spi_device_id entry
- update the example in the binding with a reset-gpios entry
- fix a style nit

Rasmus Villemoes (3):
  iio: addac: ad74413r: add spi_device_id table
  dt-bindings: iio: ad74413r: add optional reset-gpios
  iio: addac: ad74413r: add support for reset-gpio

 .../bindings/iio/addac/adi,ad74413r.yaml      |  4 +++
 drivers/iio/addac/ad74413r.c                  | 29 +++++++++++++++++++
 2 files changed, 33 insertions(+)

-- 
2.37.2

[PATCH v2 1/3] iio: addac: ad74413r: add spi_device_id table

[Rasmus Villemoes]

Silence the run-time warning

  SPI driver ad74413r has no spi_device_id for adi,ad74412r

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index 899bcd83f40b..a157f2247a50 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -1305,6 +1305,15 @@ static int ad74413r_probe(struct spi_device *spi)
 	st->spi = spi;
 	st->dev = &spi->dev;
 	st->chip_info = device_get_match_data(&spi->dev);
+	if (!st->chip_info) {
+		const struct spi_device_id *id = spi_get_device_id(spi);
+		if (id)
+			st->chip_info =
+				(struct ad74413r_chip_info *)id->driver_data;
+		if (!st->chip_info)
+			return -EINVAL;
+	}
+
 	mutex_init(&st->lock);
 	init_completion(&st->adc_data_completion);
 
@@ -1457,12 +1466,20 @@ static const struct of_device_id ad74413r_dt_id[] = {
 };
 MODULE_DEVICE_TABLE(of, ad74413r_dt_id);
 
+static const struct spi_device_id ad74413r_spi_id[] = {
+	{ .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data },
+	{ .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad74413r_spi_id);
+
 static struct spi_driver ad74413r_driver = {
 	.driver = {
 		   .name = "ad74413r",
 		   .of_match_table = ad74413r_dt_id,
 	},
 	.probe = ad74413r_probe,
+	.id_table = ad74413r_spi_id,
 };
 
 module_driver(ad74413r_driver,
-- 
2.37.2

[PATCH v2 2/3] dt-bindings: iio: ad74413r: add optional reset-gpios

[Rasmus Villemoes]

The ad74412 and ad74413 devices have an active-low reset pin. Add a
binding allowing one to specify a gpio tied to that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index 03bb90a7f4f8..62f446dbc3d8 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -51,6 +51,9 @@ properties:
       Shunt (sense) resistor value in micro-Ohms.
     default: 100000000
 
+  reset-gpios:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -129,6 +132,7 @@ examples:
         interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
 
         refin-supply = <&ad74413r_refin>;
+        reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
 
         channel@0 {
           reg = <0>;
-- 
2.37.2

[PATCH v2 3/3] iio: addac: ad74413r: add support for reset-gpio

[Rasmus Villemoes]

We have a board where the reset pin of the ad74412 is connected to a
gpio, but also pulled low (i.e., asserted) by default. Hence to get
the chip out of reset, the driver needs to know about that gpio and
deassert the reset signal before attempting to communicate with the
chip.

When a reset-gpio is given in device tree, use that instead of the
software reset. According to the data sheet, the two methods are
functionally equivalent.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index a157f2247a50..313c279173f2 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -71,6 +71,7 @@ struct ad74413r_state {
 	struct regmap			*regmap;
 	struct device			*dev;
 	struct iio_trigger		*trig;
+	struct gpio_desc		*reset_gpio;
 
 	size_t			adc_active_channels;
 	struct spi_message	adc_samples_msg;
@@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state *st)
 {
 	int ret;
 
+	if (st->reset_gpio) {
+		gpiod_set_value_cansleep(st->reset_gpio, 1);
+		fsleep(50);
+		gpiod_set_value_cansleep(st->reset_gpio, 0);
+		return 0;
+	}
+
 	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
 			   AD74413R_CMD_KEY_RESET1);
 	if (ret)
@@ -1322,6 +1330,10 @@ static int ad74413r_probe(struct spi_device *spi)
 	if (IS_ERR(st->regmap))
 		return PTR_ERR(st->regmap);
 
+	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(st->reset_gpio))
+		return PTR_ERR(st->reset_gpio);
+
 	st->refin_reg = devm_regulator_get(st->dev, "refin");
 	if (IS_ERR(st->refin_reg))
 		return dev_err_probe(st->dev, PTR_ERR(st->refin_reg),
-- 
2.37.2

Re: [PATCH v2 2/3] dt-bindings: iio: ad74413r: add optional reset-gpios

[Krzysztof Kozlowski]

On 15/11/2022 10:55, Rasmus Villemoes wrote:
> The ad74412 and ad74413 devices have an active-low reset pin. Add a
> binding allowing one to specify a gpio tied to that.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Nuno Sá]

On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:
> On Mon, 14 Nov 2022 13:52:26 +0000
> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Saturday, November 12, 2022 7:07 PM
> > > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter
> > > Clausen
> > > <lars@metafoo.de>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>;
> > > devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>;
> > > linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for
> > > reset-gpio
> > > 
> > > [External]
> > > 
> > > On Fri, 11 Nov 2022 15:39:21 +0100
> > > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > >   
> > > > We have a board where the reset pin of the ad74412 is connected
> > > > to a
> > > > gpio, but also pulled low by default. Hence to get the chip out
> > > > of
> > > > reset, the driver needs to know about that gpio and set it high
> > > > before
> > > > attempting to communicate with it.  
> > > 
> > > I'm a little confused on polarity here.  The pin is a !reset so
> > > we need to drive it low briefly to trigger a reset.
> > > I'm guessing for your board the pin is set to active low? (an
> > > example
> > > in the dt would have made that clearer) Hence the pulse
> > > in here to 1 is actually briefly driving it low before restoring
> > > to high?
> > > 
> > > For a pin documented as !reset that seems backwards though you
> > > have
> > > called it reset so that is fine, but this description doesn't
> > > make that
> > > celar.  
> > 
> > My opinion is that the driver shouldn't exactly know the polarity
> > of the reset,
> > and just assume that setting the reset GPIO to 1 means putting it
> > in reset,
> > and setting it to 0 means bringing out of reset.
> 
> Agreed. I'd just like a comment + example in the dt-binding to make
> the point
> that the pin is !reset.
> 
> Preferably with an example in the dt binding of the common case of it
> being wired
> up to an active low pin.
> 
> The main oddity here is the need to pulse it rather than request it
> directly as
> in the reset state and then just set that to off.
> 
> 

Agreed... In theory we should be able to request the gpio with
GPIOD_OUT_HIGH and then just bring the device out of reset

- Nuno Sá

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Nuno Sá]

On Fri, 2022-11-11 at 15:39 +0100, Rasmus Villemoes wrote:
> We have a board where the reset pin of the ad74412 is connected to a
> gpio, but also pulled low by default. Hence to get the chip out of
> reset, the driver needs to know about that gpio and set it high
> before
> attempting to communicate with it.
> 
> When a reset-gpio is given in device tree, use that instead of the
> software reset. According to the data sheet, the two methods are
> functionally equivalent.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/iio/addac/ad74413r.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c
> b/drivers/iio/addac/ad74413r.c
> index 9f77d2f514de..af09d43f921c 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -71,6 +71,7 @@ struct ad74413r_state {
>         struct regmap                   *regmap;
>         struct device                   *dev;
>         struct iio_trigger              *trig;
> +       struct gpio_desc                *reset_gpio;
>  
>         size_t                  adc_active_channels;
>         struct spi_message      adc_samples_msg;
> @@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state
> *st)
>  {
>         int ret;
>  
> +       if (st->reset_gpio) {
> +               gpiod_set_value_cansleep(st->reset_gpio, 1);
> +               fsleep(50);
> +               gpiod_set_value_cansleep(st->reset_gpio, 0);
> +               return 0;
> +       }
> +
>         ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
>                            AD74413R_CMD_KEY_RESET1);
>         if (ret)
> @@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device
> *spi)
>         if (IS_ERR(st->regmap))
>                 return PTR_ERR(st->regmap);
>  
> +       st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset",
> GPIOD_OUT_LOW);
> +       if (IS_ERR(st->reset_gpio))
> +               return PTR_ERR(st->reset_gpio);
> +

Minor thing but,

I would move this into ad74413r_reset() as there's no real need to have
the gpio_desc in struct ad74413r_state.

- Nuno Sá 

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Jonathan Cameron]

On Tue, 15 Nov 2022 15:49:46 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:
> > On Mon, 14 Nov 2022 13:52:26 +0000
> > "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Saturday, November 12, 2022 7:07 PM
> > > > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter
> > > > Clausen
> > > > <lars@metafoo.de>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>;
> > > > devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>;
> > > > linux-
> > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for
> > > > reset-gpio
> > > > 
> > > > [External]
> > > > 
> > > > On Fri, 11 Nov 2022 15:39:21 +0100
> > > > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > > >     
> > > > > We have a board where the reset pin of the ad74412 is connected
> > > > > to a
> > > > > gpio, but also pulled low by default. Hence to get the chip out
> > > > > of
> > > > > reset, the driver needs to know about that gpio and set it high
> > > > > before
> > > > > attempting to communicate with it.    
> > > > 
> > > > I'm a little confused on polarity here.  The pin is a !reset so
> > > > we need to drive it low briefly to trigger a reset.
> > > > I'm guessing for your board the pin is set to active low? (an
> > > > example
> > > > in the dt would have made that clearer) Hence the pulse
> > > > in here to 1 is actually briefly driving it low before restoring
> > > > to high?
> > > > 
> > > > For a pin documented as !reset that seems backwards though you
> > > > have
> > > > called it reset so that is fine, but this description doesn't
> > > > make that
> > > > celar.    
> > > 
> > > My opinion is that the driver shouldn't exactly know the polarity
> > > of the reset,
> > > and just assume that setting the reset GPIO to 1 means putting it
> > > in reset,
> > > and setting it to 0 means bringing out of reset.  
> > 
> > Agreed. I'd just like a comment + example in the dt-binding to make
> > the point
> > that the pin is !reset.
> > 
> > Preferably with an example in the dt binding of the common case of it
> > being wired
> > up to an active low pin.
> > 
> > The main oddity here is the need to pulse it rather than request it
> > directly as
> > in the reset state and then just set that to off.
> > 
> >   
> 
> Agreed... In theory we should be able to request the gpio with
> GPIOD_OUT_HIGH and then just bring the device out of reset

If I recall correctly the datasheet specifically calls out that a pulse
should be used.  No idea if that's actually true, or if it was meant
to be there just to say it needs to be set for X nsecs.

Jonathan

> 
> - Nuno Sá
> 

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Rasmus Villemoes]

On 15/11/2022 17.10, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 15:49:46 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
>> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:
>>> On Mon, 14 Nov 2022 13:52:26 +0000
>>> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
>>>   
>>>>>
>>>>> I'm a little confused on polarity here.  The pin is a !reset so
>>>>> we need to drive it low briefly to trigger a reset.
>>>>> I'm guessing for your board the pin is set to active low? (an
>>>>> example
>>>>> in the dt would have made that clearer) Hence the pulse
>>>>> in here to 1 is actually briefly driving it low before restoring
>>>>> to high?
>>>>>
>>>>> For a pin documented as !reset that seems backwards though you
>>>>> have
>>>>> called it reset so that is fine, but this description doesn't
>>>>> make that
>>>>> celar.    
>>>>
>>>> My opinion is that the driver shouldn't exactly know the polarity
>>>> of the reset,
>>>> and just assume that setting the reset GPIO to 1 means putting it
>>>> in reset,
>>>> and setting it to 0 means bringing out of reset.  
>>>
>>> Agreed. I'd just like a comment + example in the dt-binding to make
>>> the point
>>> that the pin is !reset.
>>>
>>> Preferably with an example in the dt binding of the common case of it
>>> being wired
>>> up to an active low pin.
>>>
>>> The main oddity here is the need to pulse it rather than request it
>>> directly as
>>> in the reset state and then just set that to off.
>>>
>>>   
>>
>> Agreed... In theory we should be able to request the gpio with
>> GPIOD_OUT_HIGH and then just bring the device out of reset
> 
> If I recall correctly the datasheet specifically calls out that a pulse
> should be used.  No idea if that's actually true, or if it was meant
> to be there just to say it needs to be set for X nsecs.

So the data sheet says

  The hardware reset is initiated by pulsing the RESET pin low. The
RESET pulse width must comply with the specifications in Table 11.

and table 11 says that the pulse must be min 50us, max 1ms. We don't
really have any way whatsoever to ensure that we're not rescheduled
right before pulling the gpio high again (deasserting the reset), so the
pulse could effectively be much more than 1ms. But I have a hard time
believing that that actually matters (i.e., what state would the chip be
in if we happen to make a pulse 1234us wide?). But what might be
relevant, and maybe where that 1ms figure really comes from, can perhaps
be read in table 10, which lists a "device reset time" of 1ms, with the
description

  Time taken for device reset and calibration memory upload to complete
hardware or software reset events after the device is powered up

so perhaps we should ensure a 1ms delay after the reset (whether we used
the software or gpio method). But that would be a separate fix IMO (and
I'm not sure we actually need it).

I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still keep
the gpiod_set_value(, 1) in the reset function, otherwise it's a bit too
magic for my taste.

Rasmus

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Jonathan Cameron]

On Tue, 15 Nov 2022 20:10:53 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 15/11/2022 17.10, Jonathan Cameron wrote:
> > On Tue, 15 Nov 2022 15:49:46 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> >> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
> >>> On Mon, 14 Nov 2022 13:52:26 +0000
> >>> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> >>>     
> >>>>>
> >>>>> I'm a little confused on polarity here.  The pin is a !reset so
> >>>>> we need to drive it low briefly to trigger a reset.
> >>>>> I'm guessing for your board the pin is set to active low? (an
> >>>>> example
> >>>>> in the dt would have made that clearer) Hence the pulse
> >>>>> in here to 1 is actually briefly driving it low before restoring
> >>>>> to high?
> >>>>>
> >>>>> For a pin documented as !reset that seems backwards though you
> >>>>> have
> >>>>> called it reset so that is fine, but this description doesn't
> >>>>> make that
> >>>>> celar.      
> >>>>
> >>>> My opinion is that the driver shouldn't exactly know the polarity
> >>>> of the reset,
> >>>> and just assume that setting the reset GPIO to 1 means putting it
> >>>> in reset,
> >>>> and setting it to 0 means bringing out of reset.    
> >>>
> >>> Agreed. I'd just like a comment + example in the dt-binding to make
> >>> the point
> >>> that the pin is !reset.
> >>>
> >>> Preferably with an example in the dt binding of the common case of it
> >>> being wired
> >>> up to an active low pin.
> >>>
> >>> The main oddity here is the need to pulse it rather than request it
> >>> directly as
> >>> in the reset state and then just set that to off.
> >>>
> >>>     
> >>
> >> Agreed... In theory we should be able to request the gpio with
> >> GPIOD_OUT_HIGH and then just bring the device out of reset  
> > 
> > If I recall correctly the datasheet specifically calls out that a pulse
> > should be used.  No idea if that's actually true, or if it was meant
> > to be there just to say it needs to be set for X nsecs.  
> 
> So the data sheet says
> 
>   The hardware reset is initiated by pulsing the RESET pin low. The
> RESET pulse width must comply with the specifications in Table 11.
> 
> and table 11 says that the pulse must be min 50us, max 1ms. We don't
> really have any way whatsoever to ensure that we're not rescheduled
> right before pulling the gpio high again (deasserting the reset), so the
> pulse could effectively be much more than 1ms. But I have a hard time
> believing that that actually matters (i.e., what state would the chip be
> in if we happen to make a pulse 1234us wide?).

Test it maybe?  Otherwise we'd have to play games to do it again if the
timing was too long to ensure after a couple of goes we do get a suitable
width pulse.

> But what might be
> relevant, and maybe where that 1ms figure really comes from, can perhaps
> be read in table 10, which lists a "device reset time" of 1ms, with the
> description
> 
>   Time taken for device reset and calibration memory upload to complete
> hardware or software reset events after the device is powered up
> 
> so perhaps we should ensure a 1ms delay after the reset (whether we used
> the software or gpio method). But that would be a separate fix IMO (and
> I'm not sure we actually need it).
> 
> I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still keep
> the gpiod_set_value(, 1) in the reset function, otherwise it's a bit too
> magic for my taste.

Without testing I'd worry that it really does need a pulse so probably better
to leave it doing so. 

Jonathan

> 
> Rasmus
> 

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Nuno Sá]

On Wed, 2022-11-16 at 10:22 +0000, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > On 15/11/2022 17.10, Jonathan Cameron wrote:
> > > On Tue, 15 Nov 2022 15:49:46 +0100
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
> > > > > On Mon, 14 Nov 2022 13:52:26 +0000
> > > > > "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> > > > >     
> > > > > > > 
> > > > > > > I'm a little confused on polarity here.  The pin is a
> > > > > > > !reset so
> > > > > > > we need to drive it low briefly to trigger a reset.
> > > > > > > I'm guessing for your board the pin is set to active low?
> > > > > > > (an
> > > > > > > example
> > > > > > > in the dt would have made that clearer) Hence the pulse
> > > > > > > in here to 1 is actually briefly driving it low before
> > > > > > > restoring
> > > > > > > to high?
> > > > > > > 
> > > > > > > For a pin documented as !reset that seems backwards
> > > > > > > though you
> > > > > > > have
> > > > > > > called it reset so that is fine, but this description
> > > > > > > doesn't
> > > > > > > make that
> > > > > > > celar.      
> > > > > > 
> > > > > > My opinion is that the driver shouldn't exactly know the
> > > > > > polarity
> > > > > > of the reset,
> > > > > > and just assume that setting the reset GPIO to 1 means
> > > > > > putting it
> > > > > > in reset,
> > > > > > and setting it to 0 means bringing out of reset.    
> > > > > 
> > > > > Agreed. I'd just like a comment + example in the dt-binding
> > > > > to make
> > > > > the point
> > > > > that the pin is !reset.
> > > > > 
> > > > > Preferably with an example in the dt binding of the common
> > > > > case of it
> > > > > being wired
> > > > > up to an active low pin.
> > > > > 
> > > > > The main oddity here is the need to pulse it rather than
> > > > > request it
> > > > > directly as
> > > > > in the reset state and then just set that to off.
> > > > > 
> > > > >     
> > > > 
> > > > Agreed... In theory we should be able to request the gpio with
> > > > GPIOD_OUT_HIGH and then just bring the device out of reset  
> > > 
> > > If I recall correctly the datasheet specifically calls out that a
> > > pulse
> > > should be used.  No idea if that's actually true, or if it was
> > > meant
> > > to be there just to say it needs to be set for X nsecs.  
> > 
> > So the data sheet says
> > 
> >   The hardware reset is initiated by pulsing the RESET pin low. The
> > RESET pulse width must comply with the specifications in Table 11.
> > 
> > and table 11 says that the pulse must be min 50us, max 1ms. We
> > don't
> > really have any way whatsoever to ensure that we're not rescheduled
> > right before pulling the gpio high again (deasserting the reset),
> > so the
> > pulse could effectively be much more than 1ms. But I have a hard
> > time
> > believing that that actually matters (i.e., what state would the
> > chip be
> > in if we happen to make a pulse 1234us wide?).
> 
> Test it maybe?  Otherwise we'd have to play games to do it again if
> the
> timing was too long to ensure after a couple of goes we do get a
> suitable
> width pulse.
> 
> > But what might be
> > relevant, and maybe where that 1ms figure really comes from, can
> > perhaps
> > be read in table 10, which lists a "device reset time" of 1ms, with
> > the
> > description
> > 
> >   Time taken for device reset and calibration memory upload to
> > complete
> > hardware or software reset events after the device is powered up
> > 
> > so perhaps we should ensure a 1ms delay after the reset (whether we
> > used
> > the software or gpio method). But that would be a separate fix IMO
> > (and
> > I'm not sure we actually need it).
> > 
> > I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still
> > keep
> > the gpiod_set_value(, 1) in the reset function, otherwise it's a
> > bit too
> > magic for my taste.
> 
> Without testing I'd worry that it really does need a pulse so
> probably better
> to leave it doing so. 
> 

Yeah, without testing maybe better to play safe. But, FWIW, what I read
from the datasheet is just another typical reset gpio usage with more
(fancy/confusing) description.

- Nuno Sá

Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Rasmus Villemoes]

On 16/11/2022 11.22, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> On 15/11/2022 17.10, Jonathan Cameron wrote:
>>> On Tue, 15 Nov 2022 15:49:46 +0100
>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>   
>>>> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
>>>>> On Mon, 14 Nov 2022 13:52:26 +0000
>>>>> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
>>>>>     
>>>>>>>
>>>>>>> I'm a little confused on polarity here.  The pin is a !reset so
>>>>>>> we need to drive it low briefly to trigger a reset.
>>>>>>> I'm guessing for your board the pin is set to active low? (an
>>>>>>> example
>>>>>>> in the dt would have made that clearer) Hence the pulse
>>>>>>> in here to 1 is actually briefly driving it low before restoring
>>>>>>> to high?
>>>>>>>
>>>>>>> For a pin documented as !reset that seems backwards though you
>>>>>>> have
>>>>>>> called it reset so that is fine, but this description doesn't
>>>>>>> make that
>>>>>>> celar.      
>>>>>>
>>>>>> My opinion is that the driver shouldn't exactly know the polarity
>>>>>> of the reset,
>>>>>> and just assume that setting the reset GPIO to 1 means putting it
>>>>>> in reset,
>>>>>> and setting it to 0 means bringing out of reset.    
>>>>>
>>>>> Agreed. I'd just like a comment + example in the dt-binding to make
>>>>> the point
>>>>> that the pin is !reset.
>>>>>
>>>>> Preferably with an example in the dt binding of the common case of it
>>>>> being wired
>>>>> up to an active low pin.
>>>>>
>>>>> The main oddity here is the need to pulse it rather than request it
>>>>> directly as
>>>>> in the reset state and then just set that to off.
>>>>>
>>>>>     
>>>>
>>>> Agreed... In theory we should be able to request the gpio with
>>>> GPIOD_OUT_HIGH and then just bring the device out of reset  
>>>
>>> If I recall correctly the datasheet specifically calls out that a pulse
>>> should be used.  No idea if that's actually true, or if it was meant
>>> to be there just to say it needs to be set for X nsecs.  
>>
>> So the data sheet says
>>
>>   The hardware reset is initiated by pulsing the RESET pin low. The
>> RESET pulse width must comply with the specifications in Table 11.
>>
>> and table 11 says that the pulse must be min 50us, max 1ms. We don't
>> really have any way whatsoever to ensure that we're not rescheduled
>> right before pulling the gpio high again (deasserting the reset), so the
>> pulse could effectively be much more than 1ms. But I have a hard time
>> believing that that actually matters (i.e., what state would the chip be
>> in if we happen to make a pulse 1234us wide?).
> 
> Test it maybe?  Otherwise we'd have to play games to do it again if the
> timing was too long to ensure after a couple of goes we do get a suitable
> width pulse.

So I've booted quite a number of times with various large sleep values
(between 1 and 10ms), and never seen a problem. Our hardware guys also
confirm that there should be no such thing as a "too long" pulse.

So do you want me to respin, moving the gpio request into the reset
function (i.e. not storing the descriptor in the ad74413r_state as Nuno
pointed out), requesting it in asserted state, and then, if the gpio was
found, just do the fsleep(50) and then deassert it?

Rasmus

Re: [PATCH v2 0/3] iio: addac: ad74413r: add spi device id table, support reset gpio

[Jonathan Cameron]

On Tue, 15 Nov 2022 10:55:14 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Fix a run-time warning from the SPI core by providing a spi_device_id
> table, and add binding and driver support for a reset gpio.
> 
> v2:
> - drop the two patches related to refin-supply
> - fall back to getting device data from the spi_device_id entry
> - update the example in the binding with a reset-gpios entry
> - fix a style nit

Series applied to the togreg branch of iio.git.  They might get one
day soaking as testing before I push them out to get some linux-next
coverage or I might skip that bit.

Thanks,

Jonathan

> 
> Rasmus Villemoes (3):
>   iio: addac: ad74413r: add spi_device_id table
>   dt-bindings: iio: ad74413r: add optional reset-gpios
>   iio: addac: ad74413r: add support for reset-gpio
> 
>  .../bindings/iio/addac/adi,ad74413r.yaml      |  4 +++
>  drivers/iio/addac/ad74413r.c                  | 29 +++++++++++++++++++
>  2 files changed, 33 insertions(+)
>