[PATCH v2 0/4] Add support for LTC6373

[PATCH v2 0/4] Add support for LTC6373

[Dumitru Ceclan]

This patch series adds support for the LTC6373 Instrumentation Amplifier within
the existing HMC425A driver.

The LTC6373 is a silicon, 3-bit Fully-Differential digital instrumentation
amplifier that supports the following programmable gains (Vout/Vin):
 G = 0.25, 0.5, 1, 2, 4, 8, 16 + Shutdown.
The programmable interface consists of 3 digitally controled inputs.

V1->V2
 Driver:
 - Fix chip info table indent
 - Remove enable attribute
 - Add ext_info powerdown attribute
 - Enable by default powerdown attribute
 - Set default gain after disabling powerdown to min value
 Binding:
 - Fix conditional checking of GPIO array size for LTC6373
 - Add precursor commit for correctly checking gpio size depending upon compatible

Dumitru Ceclan (4):
  dt-bindings: iio: hmc425a: add conditional GPIO array size constraints
  dt-bindings: iio: hmc425a: add entry for LTC6373
  iio: amplifiers: hmc425a: move conversion logic
  iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation
    Amplifier

 .../bindings/iio/amplifiers/adi,hmc425a.yaml  |  47 +++-
 drivers/iio/amplifiers/hmc425a.c              | 204 ++++++++++++++----
 2 files changed, 203 insertions(+), 48 deletions(-)

-- 
2.42.0

[PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Dumitru Ceclan]

ADRF5740 and HMC540S have a 4 bit parallel interface.
Update ctr-gpios description and min/maxItems values depending on the
matched compatible to correctly reflect the hardware properties.

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 .../bindings/iio/amplifiers/adi,hmc425a.yaml  | 33 +++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
index 67de9d4e3a1d..a434cb8ddcc9 100644
--- a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
+++ b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
@@ -33,11 +33,38 @@ properties:
 
   ctrl-gpios:
     description:
-      Must contain an array of 6 GPIO specifiers, referring to the GPIO pins
-      connected to the control pins V1-V6.
-    minItems: 6
+      Must contain an array of GPIO specifiers, referring to the GPIO pins
+      connected to the control pins.
+        ADRF5740  - 4 GPIO connected to D2-D5
+        HMC540S   - 4 GPIO connected to V1-V4
+        HMC425A   - 6 GPIO connected to V1-V6
+    minItems: 1
     maxItems: 6
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,hmc425a
+    then:
+      properties:
+        ctrl-gpios:
+          minItems: 6
+          maxItems: 6
+  - if:
+      properties:
+        compatible:
+          contains:
+            anyOf:
+              - const: adi,adrf5740
+              - const: adi,hmc540s
+    then:
+      properties:
+        ctrl-gpios:
+          minItems: 4
+          maxItems: 4
+
 required:
   - compatible
   - ctrl-gpios
-- 
2.42.0

[PATCH v2 2/4] dt-bindings: iio: hmc425a: add entry for LTC6373

[Dumitru Ceclan]

The LTC6373 is a silicon, 3-bit Fully-Differential digital instrumentation
amplifier that supports the following programmable gains (Vout/Vin):
 G = 0.25, 0.5, 1, 2, 4, 8, 16 + Shutdown.

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 .../bindings/iio/amplifiers/adi,hmc425a.yaml       | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
index a434cb8ddcc9..3a470459b965 100644
--- a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
+++ b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
@@ -21,6 +21,8 @@ description: |
   HMC540S 1 dB LSB Silicon MMIC 4-Bit Digital Positive Control Attenuator, 0.1 - 8 GHz
     https://www.analog.com/media/en/technical-documentation/data-sheets/hmc540s.pdf
 
+  LTC6373 is a 3-Bit precision instrumentation amplifier with fully differential outputs
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ltc6373.pdf
 
 properties:
   compatible:
@@ -28,6 +30,7 @@ properties:
       - adi,adrf5740
       - adi,hmc425a
       - adi,hmc540s
+      - adi,ltc6373
 
   vcc-supply: true
 
@@ -38,6 +41,7 @@ properties:
         ADRF5740  - 4 GPIO connected to D2-D5
         HMC540S   - 4 GPIO connected to V1-V4
         HMC425A   - 6 GPIO connected to V1-V6
+        LTC6373   - 3 GPIO connected to A0-A2
     minItems: 1
     maxItems: 6
 
@@ -64,6 +68,16 @@ allOf:
         ctrl-gpios:
           minItems: 4
           maxItems: 4
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: adi,ltc6373
+    then:
+      properties:
+        ctrl-gpios:
+          minItems: 3
+          maxItems: 3
 
 required:
   - compatible
-- 
2.42.0

[PATCH v2 3/4] iio: amplifiers: hmc425a: move conversion logic

[Dumitru Ceclan]

Move gain-dB<->code conversion logic from read_raw and write_raw to
hmc425a_gain_dB_to_code() and hmc425a_code_to_gain_dB().

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 drivers/iio/amplifiers/hmc425a.c | 100 ++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index ed4d72922696..b5fd19403d15 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -56,35 +56,70 @@ static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
 	return 0;
 }
 
+static int hmc425a_gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
+{
+	struct hmc425a_chip_info *inf = st->chip_info;
+	int gain, temp;
+
+	if (val < 0)
+		gain = (val * 1000) - (val2 / 1000);
+	else
+		gain = (val * 1000) + (val2 / 1000);
+
+	if (gain > inf->gain_max || gain < inf->gain_min)
+		return -EINVAL;
+
+	switch (st->type) {
+	case ID_HMC425A:
+		*code = ~((abs(gain) / 500) & 0x3F);
+		break;
+	case ID_HMC540S:
+		*code = ~((abs(gain) / 1000) & 0xF);
+		break;
+	case ID_ADRF5740:
+		temp = (abs(gain) / 2000) & 0xF;
+		*code = temp & BIT(3) ? temp | BIT(2) : temp;
+		break;
+	}
+	return 0;
+}
+
+static int hmc425a_code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2)
+{
+	int code, gain;
+
+	code = st->gain;
+	switch (st->type) {
+	case ID_HMC425A:
+		gain = ~code * -500;
+		break;
+	case ID_HMC540S:
+		gain = ~code * -1000;
+		break;
+	case ID_ADRF5740:
+		code = code & BIT(3) ? code & ~BIT(2) : code;
+		gain = code * -2000;
+		break;
+	}
+
+	*val = gain / 1000;
+	*val2 = (gain % 1000) * 1000;
+	return 0;
+}
+
 static int hmc425a_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan, int *val,
 			    int *val2, long m)
 {
 	struct hmc425a_state *st = iio_priv(indio_dev);
-	int code, gain = 0;
 	int ret;
 
 	mutex_lock(&st->lock);
 	switch (m) {
 	case IIO_CHAN_INFO_HARDWAREGAIN:
-		code = st->gain;
-
-		switch (st->type) {
-		case ID_HMC425A:
-			gain = ~code * -500;
-			break;
-		case ID_HMC540S:
-			gain = ~code * -1000;
-			break;
-		case ID_ADRF5740:
-			code = code & BIT(3) ? code & ~BIT(2) : code;
-			gain = code * -2000;
+		ret = hmc425a_code_to_gain_dB(st, val, val2);
+		if (ret)
 			break;
-		}
-
-		*val = gain / 1000;
-		*val2 = (gain % 1000) * 1000;
-
 		ret = IIO_VAL_INT_PLUS_MICRO_DB;
 		break;
 	default:
@@ -100,36 +135,15 @@ static int hmc425a_write_raw(struct iio_dev *indio_dev,
 			     int val2, long mask)
 {
 	struct hmc425a_state *st = iio_priv(indio_dev);
-	struct hmc425a_chip_info *inf = st->chip_info;
-	int code = 0, gain;
-	int ret;
-
-	if (val < 0)
-		gain = (val * 1000) - (val2 / 1000);
-	else
-		gain = (val * 1000) + (val2 / 1000);
-
-	if (gain > inf->gain_max || gain < inf->gain_min)
-		return -EINVAL;
-
-	switch (st->type) {
-	case ID_HMC425A:
-		code = ~((abs(gain) / 500) & 0x3F);
-		break;
-	case ID_HMC540S:
-		code = ~((abs(gain) / 1000) & 0xF);
-		break;
-	case ID_ADRF5740:
-		code = (abs(gain) / 2000) & 0xF;
-		code = code & BIT(3) ? code | BIT(2) : code;
-		break;
-	}
+	int code = 0, ret;
 
 	mutex_lock(&st->lock);
 	switch (mask) {
 	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = hmc425a_gain_dB_to_code(st, val, val2, &code);
+		if (ret)
+			break;
 		st->gain = code;
-
 		ret = hmc425a_write(indio_dev, st->gain);
 		break;
 	default:
-- 
2.42.0

[PATCH v2 4/4] iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation Amplifier

[Dumitru Ceclan]

This adds support for LTC6373 36 V Fully-Differential Programmable-Gain
Instrumentation Amplifier with 25 pA Input Bias Current.
The user can program the gain to one of seven available settings through
a 3-bit parallel interface (A2 to A0).

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 drivers/iio/amplifiers/hmc425a.c | 104 ++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index b5fd19403d15..2048bf9e0343 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -2,9 +2,10 @@
 /*
  * HMC425A and similar Gain Amplifiers
  *
- * Copyright 2020 Analog Devices Inc.
+ * Copyright 2020, 2023 Analog Devices Inc.
  */
 
+#include <linux/bits.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -20,10 +21,24 @@
 #include <linux/regulator/consumer.h>
 #include <linux/sysfs.h>
 
+/*
+ * The LTC6373 amplifier supports configuring gain using GPIO's with the following
+ *  values (OUTPUT_V / INPUT_V): 0(shutdown), 0.25, 0.5, 1, 2, 4, 8, 16
+ *
+ * Except for the shutdown value, all can be converted to dB using 20 * log10(x)
+ * From here, it is observed that all values are multiples of the '2' gain setting,
+ *  with the correspondent of 6.020dB.
+ */
+#define LTC6373_CONVERSION_CONSTANT	6020
+#define LTC6373_MIN_GAIN_CODE		0x6
+#define LTC6373_CONVERSION_MASK		GENMASK(2, 0)
+#define LTC6373_SHUTDOWN		GENMASK(2, 0)
+
 enum hmc425a_type {
 	ID_HMC425A,
 	ID_HMC540S,
-	ID_ADRF5740
+	ID_ADRF5740,
+	ID_LTC6373,
 };
 
 struct hmc425a_chip_info {
@@ -42,6 +57,7 @@ struct hmc425a_state {
 	struct	gpio_descs *gpios;
 	enum	hmc425a_type type;
 	u32	gain;
+	bool	powerdown;
 };
 
 static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
@@ -80,6 +96,17 @@ static int hmc425a_gain_dB_to_code(struct hmc425a_state *st, int val, int val2,
 		temp = (abs(gain) / 2000) & 0xF;
 		*code = temp & BIT(3) ? temp | BIT(2) : temp;
 		break;
+	case ID_LTC6373:
+		if (st->powerdown)
+			return -EPERM;
+
+		/* add half of the value for rounding */
+		temp = LTC6373_CONVERSION_CONSTANT / 2;
+		if (val < 0)
+			temp *= -1;
+		*code = ~((gain + temp) / LTC6373_CONVERSION_CONSTANT + 3)
+			& LTC6373_CONVERSION_MASK;
+		break;
 	}
 	return 0;
 }
@@ -100,6 +127,12 @@ static int hmc425a_code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2
 		code = code & BIT(3) ? code & ~BIT(2) : code;
 		gain = code * -2000;
 		break;
+	case ID_LTC6373:
+		if (st->powerdown)
+			return -EPERM;
+		gain = ((~code & LTC6373_CONVERSION_MASK) - 3) *
+		       LTC6373_CONVERSION_CONSTANT;
+		break;
 	}
 
 	*val = gain / 1000;
@@ -172,6 +205,44 @@ static const struct iio_info hmc425a_info = {
 	.write_raw_get_fmt = &hmc425a_write_raw_get_fmt,
 };
 
+static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev,
+	uintptr_t private, const struct iio_chan_spec *chan, char *buf)
+{
+	struct hmc425a_state *st = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%d\n", st->powerdown);
+}
+
+static ssize_t ltc6373_write_powerdown(struct iio_dev *indio_dev,
+	 uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
+	 size_t len)
+{
+	struct hmc425a_state *st = iio_priv(indio_dev);
+	bool powerdown;
+	int code, ret;
+
+	ret = kstrtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	mutex_lock(&st->lock);
+	st->powerdown = powerdown;
+	code = (powerdown) ? LTC6373_SHUTDOWN : st->gain;
+	ret = hmc425a_write(indio_dev, code);
+	mutex_unlock(&st->lock);
+	return len;
+}
+
+static const struct iio_chan_spec_ext_info ltc6373_ext_info[] = {
+	{
+		.name = "powerdown",
+		.read = ltc6373_read_powerdown,
+		.write = ltc6373_write_powerdown,
+		.shared = IIO_SEPARATE,
+	},
+	{},
+};
+
 #define HMC425A_CHAN(_channel)						\
 {									\
 	.type = IIO_VOLTAGE,						\
@@ -181,15 +252,30 @@ static const struct iio_info hmc425a_info = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
 }
 
+#define LTC6373_CHAN(_channel)						\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.output = 1,							\
+	.indexed = 1,							\
+	.channel = _channel,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
+	.ext_info = ltc6373_ext_info,					\
+}
+
 static const struct iio_chan_spec hmc425a_channels[] = {
 	HMC425A_CHAN(0),
 };
 
+static const struct iio_chan_spec ltc6373_channels[] = {
+	LTC6373_CHAN(0),
+};
+
 /* Match table for of_platform binding */
 static const struct of_device_id hmc425a_of_match[] = {
 	{ .compatible = "adi,hmc425a", .data = (void *)ID_HMC425A },
 	{ .compatible = "adi,hmc540s", .data = (void *)ID_HMC540S },
 	{ .compatible = "adi,adrf5740", .data = (void *)ID_ADRF5740 },
+	{ .compatible = "adi,ltc6373", .data = (void *)ID_LTC6373 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, hmc425a_of_match);
@@ -222,6 +308,15 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 		.gain_max = 0,
 		.default_gain = 0xF, /* set default gain -22.0db*/
 	},
+	[ID_LTC6373] = {
+		.name = "ltc6373",
+		.channels = ltc6373_channels,
+		.num_channels = ARRAY_SIZE(ltc6373_channels),
+		.num_gpios = 3,
+		.gain_min = -12041, /* gain setting x0.25*/
+		.gain_max = 24082,  /* gain setting x16  */
+		.default_gain = LTC6373_SHUTDOWN,
+	},
 };
 
 static int hmc425a_probe(struct platform_device *pdev)
@@ -266,6 +361,11 @@ static int hmc425a_probe(struct platform_device *pdev)
 	/* Set default gain */
 	hmc425a_write(indio_dev, st->gain);
 
+	if (st->type == ID_LTC6373) {
+		st->powerdown = true;
+		st->gain = LTC6373_MIN_GAIN_CODE;
+	}
+
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
-- 
2.42.0

Re: [PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Conor Dooley]

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

On Wed, Jan 10, 2024 at 05:37:09PM +0200, Dumitru Ceclan wrote:
> ADRF5740 and HMC540S have a 4 bit parallel interface.
> Update ctr-gpios description and min/maxItems values depending on the
> matched compatible to correctly reflect the hardware properties.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

Seems like you need a Fixes: tag, since the original binding was wrong?

> ---
>  .../bindings/iio/amplifiers/adi,hmc425a.yaml  | 33 +++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> index 67de9d4e3a1d..a434cb8ddcc9 100644
> --- a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> +++ b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> @@ -33,11 +33,38 @@ properties:
>  
>    ctrl-gpios:
>      description:
> -      Must contain an array of 6 GPIO specifiers, referring to the GPIO pins
> -      connected to the control pins V1-V6.
> -    minItems: 6
> +      Must contain an array of GPIO specifiers, referring to the GPIO pins
> +      connected to the control pins.
> +        ADRF5740  - 4 GPIO connected to D2-D5
> +        HMC540S   - 4 GPIO connected to V1-V4
> +        HMC425A   - 6 GPIO connected to V1-V6
> +    minItems: 1
>      maxItems: 6
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,hmc425a
> +    then:
> +      properties:
> +        ctrl-gpios:
> +          minItems: 6

> +          maxItems: 6

This one should not be needed, it's already set by constraints on the
property above.

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

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            anyOf:
> +              - const: adi,adrf5740
> +              - const: adi,hmc540s
> +    then:
> +      properties:
> +        ctrl-gpios:
> +          minItems: 4
> +          maxItems: 4

I'd say this should be an else, but that clearly would just be churn
since your next patch adds something with a different set of
constraints.

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

Cheers,
Conor.

> +
>  required:
>    - compatible
>    - ctrl-gpios
> -- 
> 2.42.0
> 

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

Re: [PATCH v2 2/4] dt-bindings: iio: hmc425a: add entry for LTC6373

[Conor Dooley]

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

On Wed, Jan 10, 2024 at 05:37:10PM +0200, Dumitru Ceclan wrote:
> The LTC6373 is a silicon, 3-bit Fully-Differential digital instrumentation
> amplifier that supports the following programmable gains (Vout/Vin):
>  G = 0.25, 0.5, 1, 2, 4, 8, 16 + Shutdown.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

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

Cheers,
Conor.

> ---
>  .../bindings/iio/amplifiers/adi,hmc425a.yaml       | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> index a434cb8ddcc9..3a470459b965 100644
> --- a/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> +++ b/Documentation/devicetree/bindings/iio/amplifiers/adi,hmc425a.yaml
> @@ -21,6 +21,8 @@ description: |
>    HMC540S 1 dB LSB Silicon MMIC 4-Bit Digital Positive Control Attenuator, 0.1 - 8 GHz
>      https://www.analog.com/media/en/technical-documentation/data-sheets/hmc540s.pdf
>  
> +  LTC6373 is a 3-Bit precision instrumentation amplifier with fully differential outputs
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ltc6373.pdf
>  
>  properties:
>    compatible:
> @@ -28,6 +30,7 @@ properties:
>        - adi,adrf5740
>        - adi,hmc425a
>        - adi,hmc540s
> +      - adi,ltc6373
>  
>    vcc-supply: true
>  
> @@ -38,6 +41,7 @@ properties:
>          ADRF5740  - 4 GPIO connected to D2-D5
>          HMC540S   - 4 GPIO connected to V1-V4
>          HMC425A   - 6 GPIO connected to V1-V6
> +        LTC6373   - 3 GPIO connected to A0-A2
>      minItems: 1
>      maxItems: 6
>  
> @@ -64,6 +68,16 @@ allOf:
>          ctrl-gpios:
>            minItems: 4
>            maxItems: 4
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: adi,ltc6373
> +    then:
> +      properties:
> +        ctrl-gpios:
> +          minItems: 3
> +          maxItems: 3
>  
>  required:
>    - compatible
> -- 
> 2.42.0
> 

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

Re: [PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Ceclan Dumitru]

On 1/10/24 18:17, Conor Dooley wrote:
> On Wed, Jan 10, 2024 at 05:37:09PM +0200, Dumitru Ceclan wrote:...
>>    ctrl-gpios:
>>      description:
>> -      Must contain an array of 6 GPIO specifiers, referring to the GPIO pins
>> -      connected to the control pins V1-V6.
>> -    minItems: 6
>> +      Must contain an array of GPIO specifiers, referring to the GPIO pins
>> +      connected to the control pins.
>> +        ADRF5740  - 4 GPIO connected to D2-D5
>> +        HMC540S   - 4 GPIO connected to V1-V4
>> +        HMC425A   - 6 GPIO connected to V1-V6
>> +    minItems: 1
>>      maxItems: 6
>>  
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: adi,hmc425a
>> +    then:
>> +      properties:
>> +        ctrl-gpios:
>> +          minItems: 6
> 
>> +          maxItems: 6
> 
> This one should not be needed, it's already set by constraints on the
> property above.
> 

No, not needed, just inspired from:
 /bindings/clock/samsung,exynos7-clock.yaml

Specifically, the top constraints:
  clocks:

    minItems: 1

    maxItems: 13

One of the conditional constraints:
  clocks:

    minItems: 13

    maxItems: 13


I would only have two arguments for this staying here:
 - It stays consistent with other cases
 - In the case a new device with more than 6 GPIOs is added, this would
need to be put back in

Re: [PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Conor Dooley]

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

On Thu, Jan 11, 2024 at 10:17:58AM +0200, Ceclan Dumitru wrote:
> 
> 
> On 1/10/24 18:17, Conor Dooley wrote:
> > On Wed, Jan 10, 2024 at 05:37:09PM +0200, Dumitru Ceclan wrote:...
> >>    ctrl-gpios:
> >>      description:
> >> -      Must contain an array of 6 GPIO specifiers, referring to the GPIO pins
> >> -      connected to the control pins V1-V6.
> >> -    minItems: 6
> >> +      Must contain an array of GPIO specifiers, referring to the GPIO pins
> >> +      connected to the control pins.
> >> +        ADRF5740  - 4 GPIO connected to D2-D5
> >> +        HMC540S   - 4 GPIO connected to V1-V4
> >> +        HMC425A   - 6 GPIO connected to V1-V6
> >> +    minItems: 1
> >>      maxItems: 6
> >>  
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: adi,hmc425a
> >> +    then:
> >> +      properties:
> >> +        ctrl-gpios:
> >> +          minItems: 6
> > 
> >> +          maxItems: 6
> > 
> > This one should not be needed, it's already set by constraints on the
> > property above.
> > 
> 
> No, not needed, just inspired from:
>  /bindings/clock/samsung,exynos7-clock.yaml
> 
> Specifically, the top constraints:
>   clocks:
> 
>     minItems: 1
> 
>     maxItems: 13
> 
> One of the conditional constraints:
>   clocks:
> 
>     minItems: 13
> 
>     maxItems: 13
> 
> 
> I would only have two arguments for this staying here:
>  - It stays consistent with other cases
>  - In the case a new device with more than 6 GPIOs is added, this would
> need to be put back in

Okay.

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

Re: [PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Conor Dooley]

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

On Thu, Jan 11, 2024 at 04:31:40PM +0000, Conor Dooley wrote:
> On Thu, Jan 11, 2024 at 10:17:58AM +0200, Ceclan Dumitru wrote:
> > 
> > 
> > On 1/10/24 18:17, Conor Dooley wrote:
> > > On Wed, Jan 10, 2024 at 05:37:09PM +0200, Dumitru Ceclan wrote:...
> > >>    ctrl-gpios:
> > >>      description:
> > >> -      Must contain an array of 6 GPIO specifiers, referring to the GPIO pins
> > >> -      connected to the control pins V1-V6.
> > >> -    minItems: 6
> > >> +      Must contain an array of GPIO specifiers, referring to the GPIO pins
> > >> +      connected to the control pins.
> > >> +        ADRF5740  - 4 GPIO connected to D2-D5
> > >> +        HMC540S   - 4 GPIO connected to V1-V4
> > >> +        HMC425A   - 6 GPIO connected to V1-V6
> > >> +    minItems: 1
> > >>      maxItems: 6
> > >>  
> > >> +allOf:
> > >> +  - if:
> > >> +      properties:
> > >> +        compatible:
> > >> +          contains:
> > >> +            const: adi,hmc425a
> > >> +    then:
> > >> +      properties:
> > >> +        ctrl-gpios:
> > >> +          minItems: 6
> > > 
> > >> +          maxItems: 6
> > > 
> > > This one should not be needed, it's already set by constraints on the
> > > property above.
> > > 
> > 
> > No, not needed, just inspired from:
> >  /bindings/clock/samsung,exynos7-clock.yaml
> > 
> > Specifically, the top constraints:
> >   clocks:
> > 
> >     minItems: 1
> > 
> >     maxItems: 13
> > 
> > One of the conditional constraints:
> >   clocks:
> > 
> >     minItems: 13
> > 
> >     maxItems: 13
> > 
> > 
> > I would only have two arguments for this staying here:
> >  - It stays consistent with other cases
> >  - In the case a new device with more than 6 GPIOs is added, this would
> > need to be put back in
> 
> Okay.

Sorry, that's probably super unclear. I meant it's okay to leave it.


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

Re: [PATCH v2 3/4] iio: amplifiers: hmc425a: move conversion logic

[Jonathan Cameron]

On Wed, 10 Jan 2024 17:37:11 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> Move gain-dB<->code conversion logic from read_raw and write_raw to
> hmc425a_gain_dB_to_code() and hmc425a_code_to_gain_dB().
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
A couple of trivial formatting suggestions if you end up doing a v3.
If not I might make the tweaks whilst applying,

Jonathan

> ---
>  drivers/iio/amplifiers/hmc425a.c | 100 ++++++++++++++++++-------------
>  1 file changed, 57 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index ed4d72922696..b5fd19403d15 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -56,35 +56,70 @@ static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
>  	return 0;
>  }
>  
> +static int hmc425a_gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
> +{
> +	struct hmc425a_chip_info *inf = st->chip_info;
> +	int gain, temp;
> +
> +	if (val < 0)
> +		gain = (val * 1000) - (val2 / 1000);
> +	else
> +		gain = (val * 1000) + (val2 / 1000);
> +
> +	if (gain > inf->gain_max || gain < inf->gain_min)
> +		return -EINVAL;
> +
> +	switch (st->type) {
> +	case ID_HMC425A:
> +		*code = ~((abs(gain) / 500) & 0x3F);
> +		break;
> +	case ID_HMC540S:
> +		*code = ~((abs(gain) / 1000) & 0xF);
> +		break;
> +	case ID_ADRF5740:
> +		temp = (abs(gain) / 2000) & 0xF;
> +		*code = temp & BIT(3) ? temp | BIT(2) : temp;
> +		break;
> +	}
trivial preference for blank line here.

> +	return 0;
> +}
> +
> +static int hmc425a_code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2)
> +{
> +	int code, gain;
> +
> +	code = st->gain;
> +	switch (st->type) {
> +	case ID_HMC425A:
> +		gain = ~code * -500;
> +		break;
> +	case ID_HMC540S:
> +		gain = ~code * -1000;
> +		break;
> +	case ID_ADRF5740:
> +		code = code & BIT(3) ? code & ~BIT(2) : code;
> +		gain = code * -2000;
> +		break;
> +	}
> +
> +	*val = gain / 1000;
> +	*val2 = (gain % 1000) * 1000;

As above. It's godo to have a blank line before a 'bare' return like this.

> +	return 0;
> +}
> +
>  static int hmc425a_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan, int *val,
>  			    int *val2, long m)
>  {
>  	struct hmc425a_state *st = iio_priv(indio_dev);
> -	int code, gain = 0;
>  	int ret;
>  
>  	mutex_lock(&st->lock);
>  	switch (m) {
>  	case IIO_CHAN_INFO_HARDWAREGAIN:
> -		code = st->gain;
> -
> -		switch (st->type) {
> -		case ID_HMC425A:
> -			gain = ~code * -500;
> -			break;
> -		case ID_HMC540S:
> -			gain = ~code * -1000;
> -			break;
> -		case ID_ADRF5740:
> -			code = code & BIT(3) ? code & ~BIT(2) : code;
> -			gain = code * -2000;
> +		ret = hmc425a_code_to_gain_dB(st, val, val2);
> +		if (ret)
>  			break;
> -		}
> -
> -		*val = gain / 1000;
> -		*val2 = (gain % 1000) * 1000;
> -
>  		ret = IIO_VAL_INT_PLUS_MICRO_DB;
>  		break;
>  	default:
> @@ -100,36 +135,15 @@ static int hmc425a_write_raw(struct iio_dev *indio_dev,
>  			     int val2, long mask)
>  {
>  	struct hmc425a_state *st = iio_priv(indio_dev);
> -	struct hmc425a_chip_info *inf = st->chip_info;
> -	int code = 0, gain;
> -	int ret;
> -
> -	if (val < 0)
> -		gain = (val * 1000) - (val2 / 1000);
> -	else
> -		gain = (val * 1000) + (val2 / 1000);
> -
> -	if (gain > inf->gain_max || gain < inf->gain_min)
> -		return -EINVAL;
> -
> -	switch (st->type) {
> -	case ID_HMC425A:
> -		code = ~((abs(gain) / 500) & 0x3F);
> -		break;
> -	case ID_HMC540S:
> -		code = ~((abs(gain) / 1000) & 0xF);
> -		break;
> -	case ID_ADRF5740:
> -		code = (abs(gain) / 2000) & 0xF;
> -		code = code & BIT(3) ? code | BIT(2) : code;
> -		break;
> -	}
> +	int code = 0, ret;
>  
>  	mutex_lock(&st->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		ret = hmc425a_gain_dB_to_code(st, val, val2, &code);
> +		if (ret)
> +			break;
>  		st->gain = code;
> -
>  		ret = hmc425a_write(indio_dev, st->gain);
>  		break;
>  	default:

Re: [PATCH v2 1/4] dt-bindings: iio: hmc425a: add conditional GPIO array size constraints

[Jonathan Cameron]

On Wed, 10 Jan 2024 16:17:45 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Wed, Jan 10, 2024 at 05:37:09PM +0200, Dumitru Ceclan wrote:
> > ADRF5740 and HMC540S have a 4 bit parallel interface.
> > Update ctr-gpios description and min/maxItems values depending on the
> > matched compatible to correctly reflect the hardware properties.
> > 
> > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>  
> 
> Seems like you need a Fixes: tag, since the original binding was wrong?
If you aren't doing a v3 as nothing else to change then just replying with
an appropriate fixes tag to this email is fine as b4 will work it's magic
when I pull the patches off the archive.

Thanks,

Jonathan

Re: [PATCH v2 4/4] iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation Amplifier

[Jonathan Cameron]

On Wed, 10 Jan 2024 17:37:12 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> This adds support for LTC6373 36 V Fully-Differential Programmable-Gain
> Instrumentation Amplifier with 25 pA Input Bias Current.
> The user can program the gain to one of seven available settings through
> a 3-bit parallel interface (A2 to A0).
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
Some formatting comments inline.

Ideally I'd like this driver to move to not using device ids in the match
table but instead use pointers to the chip_info structures + all the
remaining chip specific stuff being moved into those.
Given that's a non trivial change I'm not insisting on it for this series
but might get grumpy if you add more devices to this driver without tidying
that up.  It always ends up being easier to extend in the long run!

Jonathan


> ---
>  drivers/iio/amplifiers/hmc425a.c | 104 ++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index b5fd19403d15..2048bf9e0343 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -2,9 +2,10 @@
>  /*
>   * HMC425A and similar Gain Amplifiers
>   *
> - * Copyright 2020 Analog Devices Inc.
> + * Copyright 2020, 2023 Analog Devices Inc.
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/bitops.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -20,10 +21,24 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/sysfs.h>
>  
> +/*
> + * The LTC6373 amplifier supports configuring gain using GPIO's with the following
> + *  values (OUTPUT_V / INPUT_V): 0(shutdown), 0.25, 0.5, 1, 2, 4, 8, 16
> + *
> + * Except for the shutdown value, all can be converted to dB using 20 * log10(x)
> + * From here, it is observed that all values are multiples of the '2' gain setting,
> + *  with the correspondent of 6.020dB.
> + */
> +#define LTC6373_CONVERSION_CONSTANT	6020
> +#define LTC6373_MIN_GAIN_CODE		0x6
> +#define LTC6373_CONVERSION_MASK		GENMASK(2, 0)
> +#define LTC6373_SHUTDOWN		GENMASK(2, 0)
> +
>  enum hmc425a_type {
>  	ID_HMC425A,
>  	ID_HMC540S,
> -	ID_ADRF5740
> +	ID_ADRF5740,
> +	ID_LTC6373,
>  };
>  
>  struct hmc425a_chip_info {
> @@ -42,6 +57,7 @@ struct hmc425a_state {
>  	struct	gpio_descs *gpios;
>  	enum	hmc425a_type type;
>  	u32	gain;
> +	bool	powerdown;
>  };
>  
>  static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
> @@ -80,6 +96,17 @@ static int hmc425a_gain_dB_to_code(struct hmc425a_state *st, int val, int val2,
>  		temp = (abs(gain) / 2000) & 0xF;
>  		*code = temp & BIT(3) ? temp | BIT(2) : temp;
>  		break;
> +	case ID_LTC6373:
> +		if (st->powerdown)
> +			return -EPERM;
> +
> +		/* add half of the value for rounding */
> +		temp = LTC6373_CONVERSION_CONSTANT / 2;
> +		if (val < 0)
> +			temp *= -1;
> +		*code = ~((gain + temp) / LTC6373_CONVERSION_CONSTANT + 3)
> +			& LTC6373_CONVERSION_MASK;
> +		break;
>  	}
>  	return 0;

Didn't notice this in previous patch but direct returns instead of breaks in the switch case
statements would simplify this function a little.


>  }
> @@ -100,6 +127,12 @@ static int hmc425a_code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2
>  		code = code & BIT(3) ? code & ~BIT(2) : code;
>  		gain = code * -2000;
>  		break;
> +	case ID_LTC6373:
> +		if (st->powerdown)
> +			return -EPERM;
> +		gain = ((~code & LTC6373_CONVERSION_MASK) - 3) *
> +		       LTC6373_CONVERSION_CONSTANT;
> +		break;
>  	}
>  
>  	*val = gain / 1000;
> @@ -172,6 +205,44 @@ static const struct iio_info hmc425a_info = {
>  	.write_raw_get_fmt = &hmc425a_write_raw_get_fmt,
>  };
>  
> +static ssize_t ltc6373_read_powerdown(struct iio_dev *indio_dev,
> +	uintptr_t private, const struct iio_chan_spec *chan, char *buf)

The existing code looks to stick to kernel style conventions with respect
to aligning parameters just after the (

Please do that for new functions as well.


> +{
> +	struct hmc425a_state *st = iio_priv(indio_dev);
> +
> +	return sysfs_emit(buf, "%d\n", st->powerdown);
> +}
> +
> +static ssize_t ltc6373_write_powerdown(struct iio_dev *indio_dev,
> +	 uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
> +	 size_t len)
> +{
> +	struct hmc425a_state *st = iio_priv(indio_dev);
> +	bool powerdown;
> +	int code, ret;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&st->lock);
> +	st->powerdown = powerdown;
> +	code = (powerdown) ? LTC6373_SHUTDOWN : st->gain;
> +	ret = hmc425a_write(indio_dev, code);
> +	mutex_unlock(&st->lock);
> +	return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info ltc6373_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.read = ltc6373_read_powerdown,
> +		.write = ltc6373_write_powerdown,
> +		.shared = IIO_SEPARATE,
> +	},
> +	{},
> +};
> +
>  #define HMC425A_CHAN(_channel)						\
>  {									\
>  	.type = IIO_VOLTAGE,						\
> @@ -181,15 +252,30 @@ static const struct iio_info hmc425a_info = {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
>  }
>  
> +#define LTC6373_CHAN(_channel)						\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.output = 1,							\
> +	.indexed = 1,							\
> +	.channel = _channel,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
> +	.ext_info = ltc6373_ext_info,					\
> +}
> +
>  static const struct iio_chan_spec hmc425a_channels[] = {
>  	HMC425A_CHAN(0),
>  };
>  
> +static const struct iio_chan_spec ltc6373_channels[] = {
> +	LTC6373_CHAN(0),
> +};
> +
>  /* Match table for of_platform binding */
>  static const struct of_device_id hmc425a_of_match[] = {
>  	{ .compatible = "adi,hmc425a", .data = (void *)ID_HMC425A },
>  	{ .compatible = "adi,hmc540s", .data = (void *)ID_HMC540S },
>  	{ .compatible = "adi,adrf5740", .data = (void *)ID_ADRF5740 },
> +	{ .compatible = "adi,ltc6373", .data = (void *)ID_LTC6373 },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, hmc425a_of_match);
> @@ -222,6 +308,15 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
>  		.gain_max = 0,
>  		.default_gain = 0xF, /* set default gain -22.0db*/
>  	},
> +	[ID_LTC6373] = {
> +		.name = "ltc6373",
> +		.channels = ltc6373_channels,
> +		.num_channels = ARRAY_SIZE(ltc6373_channels),
> +		.num_gpios = 3,
> +		.gain_min = -12041, /* gain setting x0.25*/
> +		.gain_max = 24082,  /* gain setting x16  */
> +		.default_gain = LTC6373_SHUTDOWN,
> +	},
>  };
>  
>  static int hmc425a_probe(struct platform_device *pdev)
> @@ -266,6 +361,11 @@ static int hmc425a_probe(struct platform_device *pdev)
>  	/* Set default gain */
>  	hmc425a_write(indio_dev, st->gain);
>  
> +	if (st->type == ID_LTC6373) {
> +		st->powerdown = true;
> +		st->gain = LTC6373_MIN_GAIN_CODE;

Better to put this lot in chip info so all the device specific
settings are in one place.

> +	}
> +
>  	return devm_iio_device_register(&pdev->dev, indio_dev);
>  }
>