[PATCH v5 0/5] Add support for LTC6373

[PATCH v5 0/5] Add support 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.
The programmable interface consists of 3 digitally controled inputs.

V4->V5
 Move converion logic:
 - Modify to use callbacks
 Use pointers in match table:
 - Drop enum type from state
 Driver:
 - Use DIV_ROUND_CLOSEST for conversion
 - Remove comma from ltc6373_ext_info[]
V3->V4
 - Fix git commit message typo and remove newline between tags
V2->V3
 - Use return instead of break in *_gain_dB_to_code()
 - Add new line before return in *_code_to_gain_dB()
 - Match parameter alignment for added _powerdown functions
 - Add precursor patch for using pointers in the match table
 - Add chip_info attributes: has_powerdown and powerdown_val
 - Change probe logic to use has_powerdown for default powerdown state
 - Added 'Fixes' tag to commit message of 'add conditional GPIO array...' 
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 (5):
  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: use pointers in match table
  iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation
    Amplifier

 .../bindings/iio/amplifiers/adi,hmc425a.yaml  |  47 ++-
 drivers/iio/amplifiers/hmc425a.c              | 273 ++++++++++++++----
 2 files changed, 259 insertions(+), 61 deletions(-)

-- 
2.42.0

[PATCH v5 3/5] iio: amplifiers: hmc425a: move conversion logic

[Dumitru Ceclan]

Move gain-dB<->code conversion logic from read_raw and write_raw to
chip_info callbacks.

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

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index ed4d72922696..13e018a59637 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -34,6 +34,9 @@ struct hmc425a_chip_info {
 	int				gain_min;
 	int				gain_max;
 	int				default_gain;
+
+	int				(*gain_dB_to_code)(int gain, int *code);
+	int				(*code_to_gain_dB)(int code, int *val, int *val2);
 };
 
 struct hmc425a_state {
@@ -44,6 +47,74 @@ struct hmc425a_state {
 	u32	gain;
 };
 
+static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
+{
+	struct hmc425a_chip_info *inf = st->chip_info;
+	int gain;
+
+	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;
+
+	return st->chip_info->gain_dB_to_code(gain, code);
+}
+
+static int hmc425a_gain_dB_to_code(int gain, int *code)
+{
+	*code = ~((abs(gain) / 500) & 0x3F);
+	return 0;
+}
+
+static int hmc540s_gain_dB_to_code(int gain, int *code)
+{
+	*code = ~((abs(gain) / 1000) & 0xF);
+	return 0;
+}
+
+static int adrf5740_gain_dB_to_code(int gain, int *code)
+{
+	int temp = (abs(gain) / 2000) & 0xF;
+
+	/* Bit [0-3]: 2dB 4dB 8dB 8dB */
+	*code = temp & BIT(3) ? temp | BIT(2) : temp;
+	return 0;
+}
+
+static int code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2)
+{
+	return st->chip_info->code_to_gain_dB(st->gain, val, val2);
+}
+
+static int hmc425a_code_to_gain_dB(int code, int *val, int *val2)
+{
+	*val = (~code * -500) / 1000;
+	*val2 = ((~code * -500) % 1000) * 1000;
+	return 0;
+}
+
+static int hmc540s_code_to_gain_dB(int code, int *val, int *val2)
+{
+	*val = (~code * -1000) / 1000;
+	*val2 = ((~code * -1000) % 1000) * 1000;
+	return 0;
+}
+
+static int adrf5740_code_to_gain_dB(int code, int *val, int *val2)
+{
+	/*
+	 * Bit [0-3]: 2dB 4dB 8dB 8dB
+	 * When BIT(3) is set, unset BIT(2) and use 3 as double the place value
+	 */
+	code = code & BIT(3) ? code & ~BIT(2) : code;
+	*val = (code * -2000) / 1000;
+	*val2 = ((code * -2000) % 1000) * 1000;
+	return 0;
+}
+
 static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
 {
 	struct hmc425a_state *st = iio_priv(indio_dev);
@@ -61,30 +132,14 @@ static int hmc425a_read_raw(struct iio_dev *indio_dev,
 			    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;
+		ret = code_to_gain_dB(st, val, val2);
+		if (ret)
 			break;
-		case ID_ADRF5740:
-			code = code & BIT(3) ? code & ~BIT(2) : code;
-			gain = code * -2000;
-			break;
-		}
-
-		*val = gain / 1000;
-		*val2 = (gain % 1000) * 1000;
-
 		ret = IIO_VAL_INT_PLUS_MICRO_DB;
 		break;
 	default:
@@ -100,36 +155,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 = gain_dB_to_code(st, val, val2, &code);
+		if (ret)
+			break;
 		st->gain = code;
-
 		ret = hmc425a_write(indio_dev, st->gain);
 		break;
 	default:
@@ -189,6 +223,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 		.gain_min = -31500,
 		.gain_max = 0,
 		.default_gain = -0x40, /* set default gain -31.5db*/
+		.gain_dB_to_code = hmc425a_gain_dB_to_code,
+		.code_to_gain_dB = hmc425a_code_to_gain_dB,
 	},
 	[ID_HMC540S] = {
 		.name = "hmc540s",
@@ -198,6 +234,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 		.gain_min = -15000,
 		.gain_max = 0,
 		.default_gain = -0x10, /* set default gain -15.0db*/
+		.gain_dB_to_code = hmc540s_gain_dB_to_code,
+		.code_to_gain_dB = hmc540s_code_to_gain_dB,
 	},
 	[ID_ADRF5740] = {
 		.name = "adrf5740",
@@ -207,6 +245,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 		.gain_min = -22000,
 		.gain_max = 0,
 		.default_gain = 0xF, /* set default gain -22.0db*/
+		.gain_dB_to_code = adrf5740_gain_dB_to_code,
+		.code_to_gain_dB = adrf5740_code_to_gain_dB,
 	},
 };
 
-- 
2.42.0

Re: [PATCH v5 3/5] iio: amplifiers: hmc425a: move conversion logic

[Nuno Sá]

On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> Move gain-dB<->code conversion logic from read_raw and write_raw to
> chip_info callbacks.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/amplifiers/hmc425a.c | 126 ++++++++++++++++++++-----------
>  1 file changed, 83 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index ed4d72922696..13e018a59637 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -34,6 +34,9 @@ struct hmc425a_chip_info {
>  	int				gain_min;
>  	int				gain_max;
>  	int				default_gain;
> +
> +	int				(*gain_dB_to_code)(int gain, int *code);
> +	int				(*code_to_gain_dB)(int code, int *val, int
> *val2);
>  };
>  
>  struct hmc425a_state {
> @@ -44,6 +47,74 @@ struct hmc425a_state {
>  	u32	gain;
>  };
>  
> +static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
> +{
> +	struct hmc425a_chip_info *inf = st->chip_info;
> +	int gain;
> +
> +	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;
> +
> +	return st->chip_info->gain_dB_to_code(gain, code);
> +}
> +
> +static int hmc425a_gain_dB_to_code(int gain, int *code)
> +{
> +	*code = ~((abs(gain) / 500) & 0x3F);
> +	return 0;
> +}
> +
> +static int hmc540s_gain_dB_to_code(int gain, int *code)
> +{
> +	*code = ~((abs(gain) / 1000) & 0xF);
> +	return 0;
> +}
> +
> +static int adrf5740_gain_dB_to_code(int gain, int *code)
> +{
> +	int temp = (abs(gain) / 2000) & 0xF;
> +
> +	/* Bit [0-3]: 2dB 4dB 8dB 8dB */
> +	*code = temp & BIT(3) ? temp | BIT(2) : temp;
> +	return 0;
> +}
> +
> +static int code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2)
> +{
> +	return st->chip_info->code_to_gain_dB(st->gain, val, val2);
> +}
> +
> +static int hmc425a_code_to_gain_dB(int code, int *val, int *val2)
> +{
> +	*val = (~code * -500) / 1000;
> +	*val2 = ((~code * -500) % 1000) * 1000;
> +	return 0;
> +}
> +
> +static int hmc540s_code_to_gain_dB(int code, int *val, int *val2)
> +{
> +	*val = (~code * -1000) / 1000;
> +	*val2 = ((~code * -1000) % 1000) * 1000;
> +	return 0;
> +}
> +
> +static int adrf5740_code_to_gain_dB(int code, int *val, int *val2)
> +{
> +	/*
> +	 * Bit [0-3]: 2dB 4dB 8dB 8dB
> +	 * When BIT(3) is set, unset BIT(2) and use 3 as double the place value
> +	 */
> +	code = code & BIT(3) ? code & ~BIT(2) : code;
> +	*val = (code * -2000) / 1000;
> +	*val2 = ((code * -2000) % 1000) * 1000;
> +	return 0;
> +}
> +
>  static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
>  {
>  	struct hmc425a_state *st = iio_priv(indio_dev);
> @@ -61,30 +132,14 @@ static int hmc425a_read_raw(struct iio_dev *indio_dev,
>  			    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;
> +		ret = code_to_gain_dB(st, val, val2);
> +		if (ret)
>  			break;
> -		case ID_ADRF5740:
> -			code = code & BIT(3) ? code & ~BIT(2) : code;
> -			gain = code * -2000;
> -			break;
> -		}
> -
> -		*val = gain / 1000;
> -		*val2 = (gain % 1000) * 1000;
> -
>  		ret = IIO_VAL_INT_PLUS_MICRO_DB;
>  		break;
>  	default:
> @@ -100,36 +155,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 = gain_dB_to_code(st, val, val2, &code);
> +		if (ret)
> +			break;
>  		st->gain = code;
> -
>  		ret = hmc425a_write(indio_dev, st->gain);
>  		break;
>  	default:
> @@ -189,6 +223,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
>  		.gain_min = -31500,
>  		.gain_max = 0,
>  		.default_gain = -0x40, /* set default gain -31.5db*/
> +		.gain_dB_to_code = hmc425a_gain_dB_to_code,
> +		.code_to_gain_dB = hmc425a_code_to_gain_dB,
>  	},
>  	[ID_HMC540S] = {
>  		.name = "hmc540s",
> @@ -198,6 +234,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
>  		.gain_min = -15000,
>  		.gain_max = 0,
>  		.default_gain = -0x10, /* set default gain -15.0db*/
> +		.gain_dB_to_code = hmc540s_gain_dB_to_code,
> +		.code_to_gain_dB = hmc540s_code_to_gain_dB,
>  	},
>  	[ID_ADRF5740] = {
>  		.name = "adrf5740",
> @@ -207,6 +245,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
>  		.gain_min = -22000,
>  		.gain_max = 0,
>  		.default_gain = 0xF, /* set default gain -22.0db*/
> +		.gain_dB_to_code = adrf5740_gain_dB_to_code,
> +		.code_to_gain_dB = adrf5740_code_to_gain_dB,
>  	},
>  };
>  

Re: [PATCH v5 3/5] iio: amplifiers: hmc425a: move conversion logic

[Jonathan Cameron]

On Tue, 20 Feb 2024 17:34:49 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> Move gain-dB<->code conversion logic from read_raw and write_raw to
> chip_info callbacks.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

I've made a small tweak whilst applying this one.

> @@ -100,36 +155,15 @@ static int hmc425a_write_raw(struct iio_dev *indio_dev,
>  			     int val2, long mask)
>  {

>  	mutex_lock(&st->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		ret = gain_dB_to_code(st, val, val2, &code);
> +		if (ret)
> +			break;
>  		st->gain = code;
> -
Check your patches for unrelated white space changes.
They make things a little less reviewable and they create
noise in the history etc.

If you want to tidy up whitespace, a single patch just doing that
is the way to go.

>  		ret = hmc425a_write(indio_dev, st->gain);
>  		break;
>  	default:
> @@ -189,6 +223,8 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
>  		.gain_min = -31500,
>  		.gain_max = 0,
>  		.default_gain = -0x40, /* set default gain -31.5db*/

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

[Dumitru Ceclan]

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

Fixes: 79f2ff6461e7 ("dt-bindings: iio: hmc425a: add entry for ADRF5740 Attenuator")
Fixes: 20f87a9a26be ("dt-bindings: iio: hmc425a: add entry for HMC540S")
Acked-by: Conor Dooley <conor.dooley@microchip.com>
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 v5 4/5] iio: amplifiers: hmc425a: use pointers in match table

[Dumitru Ceclan]

Change the match table to use pointers instead of device ids.
Remove type from state as it is not used anymore.

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

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index 13e018a59637..77872e2dfdfe 100644
--- a/drivers/iio/amplifiers/hmc425a.c
+++ b/drivers/iio/amplifiers/hmc425a.c
@@ -41,15 +41,14 @@ struct hmc425a_chip_info {
 
 struct hmc425a_state {
 	struct	mutex lock; /* protect sensor state */
-	struct	hmc425a_chip_info *chip_info;
+	const struct	hmc425a_chip_info *chip_info;
 	struct	gpio_descs *gpios;
-	enum	hmc425a_type type;
 	u32	gain;
 };
 
 static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
 {
-	struct hmc425a_chip_info *inf = st->chip_info;
+	const struct hmc425a_chip_info *inf = st->chip_info;
 	int gain;
 
 	if (val < 0)
@@ -205,15 +204,6 @@ static const struct iio_chan_spec hmc425a_channels[] = {
 	HMC425A_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 },
-	{},
-};
-MODULE_DEVICE_TABLE(of, hmc425a_of_match);
-
 static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 	[ID_HMC425A] = {
 		.name = "hmc425a",
@@ -261,9 +251,8 @@ static int hmc425a_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
-	st->type = (uintptr_t)device_get_match_data(&pdev->dev);
 
-	st->chip_info = &hmc425a_chip_info_tbl[st->type];
+	st->chip_info = device_get_match_data(&pdev->dev);
 	indio_dev->num_channels = st->chip_info->num_channels;
 	indio_dev->channels = st->chip_info->channels;
 	indio_dev->name = st->chip_info->name;
@@ -295,6 +284,18 @@ static int hmc425a_probe(struct platform_device *pdev)
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
 
+/* Match table for of_platform binding */
+static const struct of_device_id hmc425a_of_match[] = {
+	{ .compatible = "adi,hmc425a",
+	  .data = &hmc425a_chip_info_tbl[ID_HMC425A]},
+	{ .compatible = "adi,hmc540s",
+	  .data = &hmc425a_chip_info_tbl[ID_HMC540S]},
+	{ .compatible = "adi,adrf5740",
+	  .data = &hmc425a_chip_info_tbl[ID_ADRF5740]},
+	{}
+};
+MODULE_DEVICE_TABLE(of, hmc425a_of_match);
+
 static struct platform_driver hmc425a_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
-- 
2.42.0

Re: [PATCH v5 4/5] iio: amplifiers: hmc425a: use pointers in match table

[Nuno Sá]

On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> Change the match table to use pointers instead of device ids.
> Remove type from state as it is not used anymore.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---

One comment (Jonathan might be able to address that while applying)... With that:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/amplifiers/hmc425a.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index 13e018a59637..77872e2dfdfe 100644
> --- a/drivers/iio/amplifiers/hmc425a.c
> +++ b/drivers/iio/amplifiers/hmc425a.c
> @@ -41,15 +41,14 @@ struct hmc425a_chip_info {
>  
>  struct hmc425a_state {
>  	struct	mutex lock; /* protect sensor state */
> -	struct	hmc425a_chip_info *chip_info;
> +	const struct	hmc425a_chip_info *chip_info;

Since you're doing this, I believe you should also constify hmc425a_chip_info_tbl[]
and mention it in the commit message.

- Nuno Sá

Re: [PATCH v5 4/5] iio: amplifiers: hmc425a: use pointers in match table

[Jonathan Cameron]

On Wed, 21 Feb 2024 14:17:22 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan wrote:
> > Change the match table to use pointers instead of device ids.
> > Remove type from state as it is not used anymore.
> > 
> > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> > ---  
> 
> One comment (Jonathan might be able to address that while applying)... With that:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> >  drivers/iio/amplifiers/hmc425a.c | 29 +++++++++++++++--------------
> >  1 file changed, 15 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> > index 13e018a59637..77872e2dfdfe 100644
> > --- a/drivers/iio/amplifiers/hmc425a.c
> > +++ b/drivers/iio/amplifiers/hmc425a.c
> > @@ -41,15 +41,14 @@ struct hmc425a_chip_info {
> >  
> >  struct hmc425a_state {
> >  	struct	mutex lock; /* protect sensor state */
> > -	struct	hmc425a_chip_info *chip_info;
> > +	const struct	hmc425a_chip_info *chip_info;  
> 
> Since you're doing this, I believe you should also constify hmc425a_chip_info_tbl[]
> and mention it in the commit message.
> 
Absolutely.  I've made that change and added a note on it to commit message.

> - Nuno Sá
> 

[PATCH v5 2/5] 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.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
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 v5 5/5] 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 | 124 ++++++++++++++++++++++++++++++-
 1 file changed, 120 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
index 77872e2dfdfe..50c86c2d28d7 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, 2024 Analog Devices Inc.
  */
 
+#include <linux/bits.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -12,6 +13,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/kernel.h>
+#include <linux/math.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -20,10 +22,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 {
@@ -34,6 +50,8 @@ struct hmc425a_chip_info {
 	int				gain_min;
 	int				gain_max;
 	int				default_gain;
+	int				powerdown_val;
+	bool				has_powerdown;
 
 	int				(*gain_dB_to_code)(int gain, int *code);
 	int				(*code_to_gain_dB)(int code, int *val, int *val2);
@@ -44,6 +62,7 @@ struct hmc425a_state {
 	const struct	hmc425a_chip_info *chip_info;
 	struct	gpio_descs *gpios;
 	u32	gain;
+	bool	powerdown;
 };
 
 static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *code)
@@ -58,6 +77,8 @@ static int gain_dB_to_code(struct hmc425a_state *st, int val, int val2, int *cod
 
 	if (gain > inf->gain_max || gain < inf->gain_min)
 		return -EINVAL;
+	if (st->powerdown)
+		return -EPERM;
 
 	return st->chip_info->gain_dB_to_code(gain, code);
 }
@@ -83,8 +104,17 @@ static int adrf5740_gain_dB_to_code(int gain, int *code)
 	return 0;
 }
 
+static int ltc6373_gain_dB_to_code(int gain, int *code)
+{
+	*code = ~(DIV_ROUND_CLOSEST(gain, LTC6373_CONVERSION_CONSTANT) + 3)
+		& LTC6373_CONVERSION_MASK;
+	return 0;
+}
+
 static int code_to_gain_dB(struct hmc425a_state *st, int *val, int *val2)
 {
+	if (st->powerdown)
+		return -EPERM;
 	return st->chip_info->code_to_gain_dB(st->gain, val, val2);
 }
 
@@ -114,6 +144,16 @@ static int adrf5740_code_to_gain_dB(int code, int *val, int *val2)
 	return 0;
 }
 
+static int ltc6373_code_to_gain_dB(int code, int *val, int *val2)
+{
+	int gain = ((~code & LTC6373_CONVERSION_MASK) - 3) *
+		   LTC6373_CONVERSION_CONSTANT;
+
+	*val = gain / 1000;
+	*val2 = (gain % 1000) * 1000;
+	return 0;
+}
+
 static int hmc425a_write(struct iio_dev *indio_dev, u32 value)
 {
 	struct hmc425a_state *st = iio_priv(indio_dev);
@@ -191,6 +231,48 @@ 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;
+	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,						\
@@ -200,10 +282,24 @@ 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),
+};
+
 static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 	[ID_HMC425A] = {
 		.name = "hmc425a",
@@ -238,6 +334,19 @@ static struct hmc425a_chip_info hmc425a_chip_info_tbl[] = {
 		.gain_dB_to_code = adrf5740_gain_dB_to_code,
 		.code_to_gain_dB = adrf5740_code_to_gain_dB,
 	},
+	[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_MIN_GAIN_CODE,
+		.powerdown_val = LTC6373_SHUTDOWN,
+		.has_powerdown = true,
+		.gain_dB_to_code = ltc6373_gain_dB_to_code,
+		.code_to_gain_dB = ltc6373_code_to_gain_dB,
+	},
 };
 
 static int hmc425a_probe(struct platform_device *pdev)
@@ -278,8 +387,13 @@ static int hmc425a_probe(struct platform_device *pdev)
 	indio_dev->info = &hmc425a_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	/* Set default gain */
-	hmc425a_write(indio_dev, st->gain);
+	if (st->chip_info->has_powerdown) {
+		st->powerdown = true;
+		hmc425a_write(indio_dev, st->chip_info->powerdown_val);
+	} else {
+		/* Set default gain */
+		hmc425a_write(indio_dev, st->gain);
+	}
 
 	return devm_iio_device_register(&pdev->dev, indio_dev);
 }
@@ -292,6 +406,8 @@ static const struct of_device_id hmc425a_of_match[] = {
 	  .data = &hmc425a_chip_info_tbl[ID_HMC540S]},
 	{ .compatible = "adi,adrf5740",
 	  .data = &hmc425a_chip_info_tbl[ID_ADRF5740]},
+	{ .compatible = "adi,ltc6373",
+	  .data = &hmc425a_chip_info_tbl[ID_LTC6373]},
 	{}
 };
 MODULE_DEVICE_TABLE(of, hmc425a_of_match);
-- 
2.42.0

Re: [PATCH v5 5/5] iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation Amplifier

[Nuno Sá]

On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan 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>
> ---

Just one minor comment. With that:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
>  1 file changed, 120 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> index 77872e2dfdfe..50c86c2d28d7 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, 2024 Analog Devices Inc.
>   */

...

> 
>  
> +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);

Well, in theory the read should also be protected with the lock...

- Nuno Sá

Re: [PATCH v5 5/5] iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation Amplifier

[Jonathan Cameron]

On Wed, 21 Feb 2024 14:23:51 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan 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>
> > ---  
> 
> Just one minor comment. With that:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> >  drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
> >  1 file changed, 120 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/amplifiers/hmc425a.c b/drivers/iio/amplifiers/hmc425a.c
> > index 77872e2dfdfe..50c86c2d28d7 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, 2024 Analog Devices Inc.
> >   */  
> 
> ...
> 
> > 
> >  
> > +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);  
> 
> Well, in theory the read should also be protected with the lock...

Only reason I can think of for that is potential read tearing.
If that happens on a bool we are going to be in a mess so I think this
is in practice fine without, though paranoia might suggest locking.

It can race against it being powered down but that effectively happens
even if we do have a lock as we either see the value before or after
a racing power down event and we have no way of knowing which.

Applied rest series to iio.git togreg branch and pushed out as testing.

Duitru, if you can figure out what happened to your message thread before
sending more patches that would be great. The IDs for patches 0-5 go

20240220153553.2432-1-mitrutzceclan@gmail.com
20240220153553.2432-3-mitrutzceclan@gmail.com
20240220153553.2432-5-mitrutzceclan@gmail.com
20240220153553.2432-2-mitrutzceclan@gmail.com
20240220153553.2432-4-mitrutzceclan@gmail.com
20240220153553.2432-6-mitrutzceclan@gmail.com

Which really confuses my email client and patchwork.
https://patchwork.kernel.org/project/linux-iio/list/?series=827901



> 
> - Nuno Sá
> 

Re: [PATCH v5 5/5] iio: amplifiers: hmc425a: add support for LTC6373 Instrumentation Amplifier

[Nuno Sá]

On Sat, 2024-02-24 at 17:54 +0000, Jonathan Cameron wrote:
> On Wed, 21 Feb 2024 14:23:51 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Tue, 2024-02-20 at 17:34 +0200, Dumitru Ceclan 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>
> > > ---  
> > 
> > Just one minor comment. With that:
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > 
> > >  drivers/iio/amplifiers/hmc425a.c | 124 ++++++++++++++++++++++++++++++-
> > >  1 file changed, 120 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/iio/amplifiers/hmc425a.c
> > > b/drivers/iio/amplifiers/hmc425a.c
> > > index 77872e2dfdfe..50c86c2d28d7 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, 2024 Analog Devices Inc.
> > >   */  
> > 
> > ...
> > 
> > > 
> > >  
> > > +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);  
> > 
> > Well, in theory the read should also be protected with the lock...
> 
> Only reason I can think of for that is potential read tearing.
> If that happens on a bool we are going to be in a mess so I think this
> is in practice fine without, though paranoia might suggest locking.

Yeah, also mentioned it for correctness. I mean, in theory, read_once() should be
more that enough in here but I often find that too much for using in "simple" drivers
where a lock is surely easier to understand for someone reading the code.

Now, about your bool comment, I used to think like that until I saw the LF rcu
mentorship video from Paul. I'm fairly sure he comes up with some "crazy" possibility
about the CPU/compiler screwing you even on a char (IIRC, he was also arguing about
not using read_once() on a bool).

Now, practically speaking, I tend to agree that for the archs we care this will
likely never be an issue but yeah, not 100% correct kernel code IMO.

- Nuno Sá