[PATCH 0/2] iio: ad74413r: allow setting sink current for digital input

[PATCH 0/2] iio: ad74413r: allow setting sink current for digital input

[Rasmus Villemoes]

Depending on the actual hardware wired up to a digital input channel,
it may be necessary to configure the ad74413r to sink a small
current. For example, in the case of a simple mechanical switch, the
charge on the external 68 nF capacitor (cf. the data sheet's Figure
34) will keep the channel as reading high even after the switch is
turned off again.

Add a DT binding and driver support for setting the desired sink current.

I have chosen the term "drive strength" because it matches existing
practice, even if this is only a sink. E.g. there's

 * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
 *      passed as argument. The argument is in uA.

and indeed it would be trivial to hook up that
PIN_CONFIG_DRIVE_STRENGTH_UA in ad74413r_gpio_set_comp_config().

However, unlike the debounce time, there does not appear to be any way
to actually tweak the drive strength from userspace, nor do I know if
that would actually be a good idea. For our application(s), the
current sink needed is a property of the attached hardware, and thus
can and should be defined in DT.

Rasmus Villemoes (2):
  dt-bindings: iio: ad74413r: allow setting sink current for digital
    input
  iio: ad74413r: wire up support for drive-strength-microamp property

 .../bindings/iio/addac/adi,ad74413r.yaml      | 10 ++++++++
 drivers/iio/addac/ad74413r.c                  | 24 +++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
2.37.2

[PATCH 1/2] dt-bindings: iio: ad74413r: allow setting sink current for digital input

[Rasmus Villemoes]

Depending on the actual hardware wired up to a digital input channel,
it may be necessary to configure the ad74413r to sink a small
current. For example, in the case of a simple mechanical switch, the
charge on the external 68 nF capacitor (cf. the data sheet's Figure
34) will keep the channel as reading high even after the switch is
turned off again.

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

diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index 9eb3ecc8bbc8..fcae300182f7 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -101,6 +101,16 @@ patternProperties:
           When not configured as a comparator, the GPO will be treated as an
           output-only GPIO.
 
+      drive-strength-microamp:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          For channels configured as digital input, this configures the sink
+          current.
+        minimum: 0
+        maximum: 1800
+        default: 0
+        multipleOf: 120
+
     required:
       - reg
 
-- 
2.37.2

Re: [PATCH 1/2] dt-bindings: iio: ad74413r: allow setting sink current for digital input

[Rob Herring]

On Thu, 02 Mar 2023 14:49:20 +0100, Rasmus Villemoes wrote:
> Depending on the actual hardware wired up to a digital input channel,
> it may be necessary to configure the ad74413r to sink a small
> current. For example, in the case of a simple mechanical switch, the
> charge on the external 68 nF capacitor (cf. the data sheet's Figure
> 34) will keep the channel as reading high even after the switch is
> turned off again.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/iio/addac/adi,ad74413r.yaml    | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml: patternProperties:^channel@[0-3]$:properties:drive-strength-microamp: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230302134922.1120217-2-linux@rasmusvillemoes.dk

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 2/2] iio: ad74413r: wire up support for drive-strength-microamp property

[Rasmus Villemoes]

Use the value specified in the channel configuration node to populate
the DIN_SINK field of the DIN_CONFIGx register.

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

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index f32c8c2fb26d..cbf0f66fdc74 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -39,6 +39,7 @@ struct ad74413r_chip_info {
 
 struct ad74413r_channel_config {
 	u32		func;
+	u32		drive_strength;
 	bool		gpo_comparator;
 	bool		initialized;
 };
@@ -111,6 +112,7 @@ struct ad74413r_state {
 #define AD74413R_REG_DIN_CONFIG_X(x)	(0x09 + (x))
 #define AD74413R_DIN_DEBOUNCE_MASK	GENMASK(4, 0)
 #define AD74413R_DIN_DEBOUNCE_LEN	BIT(5)
+#define AD74413R_DIN_SINK_MASK		GENMASK(9, 6)
 
 #define AD74413R_REG_DAC_CODE_X(x)	(0x16 + (x))
 #define AD74413R_DAC_CODE_MAX		GENMASK(12, 0)
@@ -261,6 +263,19 @@ static int ad74413r_set_comp_debounce(struct ad74413r_state *st,
 				  val);
 }
 
+static int ad74413r_set_comp_drive_strength(struct ad74413r_state *st,
+					    unsigned int offset,
+					    unsigned int strength)
+{
+	if (strength > 1800)
+		strength = 1800;
+
+	return regmap_update_bits(st->regmap, AD74413R_REG_DIN_CONFIG_X(offset),
+				  AD74413R_DIN_SINK_MASK,
+				  FIELD_PREP(AD74413R_DIN_SINK_MASK, strength / 120));
+}
+
+
 static void ad74413r_gpio_set(struct gpio_chip *chip,
 			      unsigned int offset, int val)
 {
@@ -1190,6 +1205,9 @@ static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
 	config->gpo_comparator = fwnode_property_read_bool(channel_node,
 		"adi,gpo-comparator");
 
+	fwnode_property_read_u32(channel_node, "drive-strength-microamp",
+				 &config->drive_strength);
+
 	if (!config->gpo_comparator)
 		st->num_gpo_gpios++;
 
@@ -1269,6 +1287,7 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
 	unsigned int gpo_gpio_i = 0;
 	unsigned int i;
 	u8 gpo_config;
+	u32 strength;
 	int ret;
 
 	for (i = 0; i < AD74413R_CHANNEL_MAX; i++) {
@@ -1285,6 +1304,11 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
 		    config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER)
 			st->comp_gpio_offsets[comp_gpio_i++] = i;
 
+		strength = config->drive_strength;
+		ret = ad74413r_set_comp_drive_strength(st, i, strength);
+		if (ret)
+			return ret;
+
 		ret = ad74413r_set_gpo_config(st, i, gpo_config);
 		if (ret)
 			return ret;
-- 
2.37.2

Re: [PATCH 2/2] iio: ad74413r: wire up support for drive-strength-microamp property

[Jonathan Cameron]

On Thu,  2 Mar 2023 14:49:21 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Use the value specified in the channel configuration node to populate
> the DIN_SINK field of the DIN_CONFIGx register.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Hi Rasmus,

 Given you are doing a v2 for the binding issue
Rob's bot noted. One trivial comment inline.
(I'd have ignored it if everything else was fine!)

Thanks,

Jonathan

> ---
>  drivers/iio/addac/ad74413r.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> index f32c8c2fb26d..cbf0f66fdc74 100644
> --- a/drivers/iio/addac/ad74413r.c
> +++ b/drivers/iio/addac/ad74413r.c
> @@ -39,6 +39,7 @@ struct ad74413r_chip_info {
>  
>  struct ad74413r_channel_config {
>  	u32		func;
> +	u32		drive_strength;
>  	bool		gpo_comparator;
>  	bool		initialized;
>  };
> @@ -111,6 +112,7 @@ struct ad74413r_state {
>  #define AD74413R_REG_DIN_CONFIG_X(x)	(0x09 + (x))
>  #define AD74413R_DIN_DEBOUNCE_MASK	GENMASK(4, 0)
>  #define AD74413R_DIN_DEBOUNCE_LEN	BIT(5)
> +#define AD74413R_DIN_SINK_MASK		GENMASK(9, 6)
>  
>  #define AD74413R_REG_DAC_CODE_X(x)	(0x16 + (x))
>  #define AD74413R_DAC_CODE_MAX		GENMASK(12, 0)
> @@ -261,6 +263,19 @@ static int ad74413r_set_comp_debounce(struct ad74413r_state *st,
>  				  val);
>  }
>  
> +static int ad74413r_set_comp_drive_strength(struct ad74413r_state *st,
> +					    unsigned int offset,
> +					    unsigned int strength)
> +{
> +	if (strength > 1800)
> +		strength = 1800;

trivial but I think
	strength = min(strength, 1800);
is perhaps a little more readable?

> +
> +	return regmap_update_bits(st->regmap, AD74413R_REG_DIN_CONFIG_X(offset),
> +				  AD74413R_DIN_SINK_MASK,
> +				  FIELD_PREP(AD74413R_DIN_SINK_MASK, strength / 120));
> +}
> +
> +
>  static void ad74413r_gpio_set(struct gpio_chip *chip,
>  			      unsigned int offset, int val)
>  {
> @@ -1190,6 +1205,9 @@ static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
>  	config->gpo_comparator = fwnode_property_read_bool(channel_node,
>  		"adi,gpo-comparator");
>  
> +	fwnode_property_read_u32(channel_node, "drive-strength-microamp",
> +				 &config->drive_strength);
> +
>  	if (!config->gpo_comparator)
>  		st->num_gpo_gpios++;
>  
> @@ -1269,6 +1287,7 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
>  	unsigned int gpo_gpio_i = 0;
>  	unsigned int i;
>  	u8 gpo_config;
> +	u32 strength;
>  	int ret;
>  
>  	for (i = 0; i < AD74413R_CHANNEL_MAX; i++) {
> @@ -1285,6 +1304,11 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
>  		    config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER)
>  			st->comp_gpio_offsets[comp_gpio_i++] = i;
>  
> +		strength = config->drive_strength;
> +		ret = ad74413r_set_comp_drive_strength(st, i, strength);
> +		if (ret)
> +			return ret;
> +
>  		ret = ad74413r_set_gpo_config(st, i, gpo_config);
>  		if (ret)
>  			return ret;

[PATCH v2 0/2] iio: ad74413r: allow setting sink current for digital input

[Rasmus Villemoes]

Depending on the actual hardware wired up to a digital input channel,
it may be necessary to configure the ad74413r to sink a small
current. For example, in the case of a simple mechanical switch, the
charge on the external 68 nF capacitor (cf. the data sheet's Figure
34) will keep the channel as reading high even after the switch is
turned off again.

Add a DT binding and driver support for setting the desired sink current.

I have chosen the term "drive strength" because it matches existing
practice, even if this is only a sink. E.g. there's

 * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
 *      passed as argument. The argument is in uA.

and indeed it would be trivial to hook up that
PIN_CONFIG_DRIVE_STRENGTH_UA in ad74413r_gpio_set_comp_config().

However, unlike the debounce time, there does not appear to be any way
to actually tweak the drive strength from userspace, nor do I know if
that would actually be a good idea. For our application(s), the
current sink needed is a property of the attached hardware, and thus
can and should be defined in DT.

v2:
- remove redundant type info in binding per Rob's bot
- use min() instead of if() in ad74413r_set_comp_drive_strength() per Jonathan

Rasmus Villemoes (2):
  dt-bindings: iio: ad74413r: allow setting sink current for digital
    input
  iio: ad74413r: wire up support for drive-strength-microamp property

 .../bindings/iio/addac/adi,ad74413r.yaml      |  9 ++++++++
 drivers/iio/addac/ad74413r.c                  | 23 +++++++++++++++++++
 2 files changed, 32 insertions(+)

-- 
2.37.2

[PATCH v2 1/2] dt-bindings: iio: ad74413r: allow setting sink current for digital input

[Rasmus Villemoes]

Depending on the actual hardware wired up to a digital input channel,
it may be necessary to configure the ad74413r to sink a small
current. For example, in the case of a simple mechanical switch, the
charge on the external 68 nF capacitor (cf. the data sheet's Figure
34) will keep the channel as reading high even after the switch is
turned off again.

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

diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index 9eb3ecc8bbc8..590ea7936ad7 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -101,6 +101,15 @@ patternProperties:
           When not configured as a comparator, the GPO will be treated as an
           output-only GPIO.
 
+      drive-strength-microamp:
+        description: |
+          For channels configured as digital input, this configures the sink
+          current.
+        minimum: 0
+        maximum: 1800
+        default: 0
+        multipleOf: 120
+
     required:
       - reg
 
-- 
2.37.2

Re: [PATCH v2 1/2] dt-bindings: iio: ad74413r: allow setting sink current for digital input

[Krzysztof Kozlowski]

On 06/03/2023 10:43, Rasmus Villemoes wrote:
> Depending on the actual hardware wired up to a digital input channel,
> it may be necessary to configure the ad74413r to sink a small
> current. For example, in the case of a simple mechanical switch, the
> charge on the external 68 nF capacitor (cf. the data sheet's Figure
> 34) will keep the channel as reading high even after the switch is
> turned off again.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  .../devicetree/bindings/iio/addac/adi,ad74413r.yaml      | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> index 9eb3ecc8bbc8..590ea7936ad7 100644
> --- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> +++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
> @@ -101,6 +101,15 @@ patternProperties:
>            When not configured as a comparator, the GPO will be treated as an
>            output-only GPIO.
>  
> +      drive-strength-microamp:
> +        description: |
> +          For channels configured as digital input, this configures the sink
> +          current.
> +        minimum: 0
> +        maximum: 1800
> +        default: 0
> +        multipleOf: 120

You could add it also to the example, but it's fine:

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

Best regards,
Krzysztof

[PATCH v2 2/2] iio: ad74413r: wire up support for drive-strength-microamp property

[Rasmus Villemoes]

Use the value specified in the channel configuration node to populate
the DIN_SINK field of the DIN_CONFIGx register.

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

diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index f32c8c2fb26d..4395758dbaa6 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -39,6 +39,7 @@ struct ad74413r_chip_info {
 
 struct ad74413r_channel_config {
 	u32		func;
+	u32		drive_strength;
 	bool		gpo_comparator;
 	bool		initialized;
 };
@@ -111,6 +112,7 @@ struct ad74413r_state {
 #define AD74413R_REG_DIN_CONFIG_X(x)	(0x09 + (x))
 #define AD74413R_DIN_DEBOUNCE_MASK	GENMASK(4, 0)
 #define AD74413R_DIN_DEBOUNCE_LEN	BIT(5)
+#define AD74413R_DIN_SINK_MASK		GENMASK(9, 6)
 
 #define AD74413R_REG_DAC_CODE_X(x)	(0x16 + (x))
 #define AD74413R_DAC_CODE_MAX		GENMASK(12, 0)
@@ -261,6 +263,18 @@ static int ad74413r_set_comp_debounce(struct ad74413r_state *st,
 				  val);
 }
 
+static int ad74413r_set_comp_drive_strength(struct ad74413r_state *st,
+					    unsigned int offset,
+					    unsigned int strength)
+{
+	strength = min(strength, 1800U);
+
+	return regmap_update_bits(st->regmap, AD74413R_REG_DIN_CONFIG_X(offset),
+				  AD74413R_DIN_SINK_MASK,
+				  FIELD_PREP(AD74413R_DIN_SINK_MASK, strength / 120));
+}
+
+
 static void ad74413r_gpio_set(struct gpio_chip *chip,
 			      unsigned int offset, int val)
 {
@@ -1190,6 +1204,9 @@ static int ad74413r_parse_channel_config(struct iio_dev *indio_dev,
 	config->gpo_comparator = fwnode_property_read_bool(channel_node,
 		"adi,gpo-comparator");
 
+	fwnode_property_read_u32(channel_node, "drive-strength-microamp",
+				 &config->drive_strength);
+
 	if (!config->gpo_comparator)
 		st->num_gpo_gpios++;
 
@@ -1269,6 +1286,7 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
 	unsigned int gpo_gpio_i = 0;
 	unsigned int i;
 	u8 gpo_config;
+	u32 strength;
 	int ret;
 
 	for (i = 0; i < AD74413R_CHANNEL_MAX; i++) {
@@ -1285,6 +1303,11 @@ static int ad74413r_setup_gpios(struct ad74413r_state *st)
 		    config->func == CH_FUNC_DIGITAL_INPUT_LOOP_POWER)
 			st->comp_gpio_offsets[comp_gpio_i++] = i;
 
+		strength = config->drive_strength;
+		ret = ad74413r_set_comp_drive_strength(st, i, strength);
+		if (ret)
+			return ret;
+
 		ret = ad74413r_set_gpo_config(st, i, gpo_config);
 		if (ret)
 			return ret;
-- 
2.37.2

Re: [PATCH v2 0/2] iio: ad74413r: allow setting sink current for digital input

[Jonathan Cameron]

On Mon,  6 Mar 2023 10:42:59 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Depending on the actual hardware wired up to a digital input channel,
> it may be necessary to configure the ad74413r to sink a small
> current. For example, in the case of a simple mechanical switch, the
> charge on the external 68 nF capacitor (cf. the data sheet's Figure
> 34) will keep the channel as reading high even after the switch is
> turned off again.
> 
> Add a DT binding and driver support for setting the desired sink current.
> 
> I have chosen the term "drive strength" because it matches existing
> practice, even if this is only a sink. E.g. there's
> 
>  * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
>  *      passed as argument. The argument is in uA.
> 
> and indeed it would be trivial to hook up that
> PIN_CONFIG_DRIVE_STRENGTH_UA in ad74413r_gpio_set_comp_config().
> 
> However, unlike the debounce time, there does not appear to be any way
> to actually tweak the drive strength from userspace, nor do I know if
> that would actually be a good idea. For our application(s), the
> current sink needed is a property of the attached hardware, and thus
> can and should be defined in DT.
> 
> v2:
> - remove redundant type info in binding per Rob's bot
> - use min() instead of if() in ad74413r_set_comp_drive_strength() per Jonathan
> 
> Rasmus Villemoes (2):
>   dt-bindings: iio: ad74413r: allow setting sink current for digital
>     input
>   iio: ad74413r: wire up support for drive-strength-microamp property
> 
>  .../bindings/iio/addac/adi,ad74413r.yaml      |  9 ++++++++
>  drivers/iio/addac/ad74413r.c                  | 23 +++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
Series applied to the togreg branch of iio.git and pushed out as testing
for 0-day to take a first look at it.

Thanks,

Jonathan