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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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/[email protected]
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.
On Thu, 2 Mar 2023 14:49:21 +0100
Rasmus Villemoes <[email protected]> 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 <[email protected]>
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;
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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 <[email protected]>
Best regards,
Krzysztof
On Mon, 6 Mar 2023 10:42:59 +0100
Rasmus Villemoes <[email protected]> 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