[PATCH 0/3] iio: adc: add LTC2309 support

[PATCH 0/3] iio: adc: add LTC2309 support

[Liam Beguin]

The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface.

This implements support for all single-ended and differential channels,
in unipolar mode only.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
Liam Beguin (3):
      iio: adc: add ltc2309 support
      iio: adc: ltc2309: switch to new .probe()
      dt-bindings: iio: adc: add lltc,ltc2309 bindings

 .../devicetree/bindings/iio/adc/lltc,ltc2309.yaml  |  52 +++++
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ltc2309.c                          | 231 +++++++++++++++++++++
 4 files changed, 294 insertions(+)
---
base-commit: a5e505a99ca748583dbe558b691be1b26f05d678
change-id: 20230823-ltc2309-1945e1e94931

Best regards,
-- 
Liam Beguin <liambeguin@gmail.com>

[PATCH 1/3] iio: adc: add ltc2309 support

[Liam Beguin]

The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface.

This implements support for all single-ended and differential channels,
in unipolar mode only.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 drivers/iio/adc/Kconfig   |  10 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/ltc2309.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dc14bde31ac1..6ec18e02faf9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -607,6 +607,16 @@ config LPC32XX_ADC
 	  activate only one via device tree selection.  Provides direct access
 	  via sysfs.
 
+config LTC2309
+	tristate "Linear Technology LTC2309 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Linear Technology LTC2309, a low
+	  noise, low power, 8-channel, 12-bit SAR ADC
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2309.
+
 config LTC2471
 	tristate "Linear Technology LTC2471 and LTC2473 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index eb6e891790fb..fbd86184ec94 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
+obj-$(CONFIG_LTC2309) += ltc2309.o
 obj-$(CONFIG_LTC2471) += ltc2471.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
 obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c
new file mode 100644
index 000000000000..ee1fd9b82e2a
--- /dev/null
+++ b/drivers/iio/adc/ltc2309.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface.
+ *
+ * Datasheet:
+ * https://www.analog.com/media/en/technical-documentation/data-sheets/2309fd.pdf
+ *
+ * Copyright (c) 2023, Liam Beguin <liambeguin@gmail.com>
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+
+#define DRIVER_NAME		"ltc2309"
+#define LTC2309_ADC_RESOLUTION	12
+
+#define LTC2309_DIN_CH_MASK	GENMASK(7, 4)
+#define LTC2309_DIN_SDN		BIT(7)
+#define LTC2309_DIN_OSN		BIT(6)
+#define LTC2309_DIN_S1		BIT(5)
+#define LTC2309_DIN_S0		BIT(4)
+#define LTC2309_DIN_UNI		BIT(3)
+#define LTC2309_DIN_SLEEP	BIT(2)
+
+/* struct ltc2309 - internal device data structure
+ *
+ * @dev:	Device reference
+ * @client:	I2C reference
+ * @refcomp:	External reference source
+ * @lock:	Lock to protect against multiple access to the device
+ * @vref_mv	Internal voltage reference
+ */
+struct ltc2309 {
+	struct device		*dev;
+	struct i2c_client	*client;
+	struct regulator	*refcomp;
+	struct mutex		lock;
+	int			vref_mv;
+};
+
+/* Order matches expected channel address, See datasheet Table 1. */
+enum ltc2309_channels {
+	LTC2309_CH0_CH1 = 0,
+	LTC2309_CH2_CH3,
+	LTC2309_CH4_CH5,
+	LTC2309_CH6_CH7,
+	LTC2309_CH1_CH0,
+	LTC2309_CH3_CH2,
+	LTC2309_CH5_CH4,
+	LTC2309_CH7_CH6,
+	LTC2309_CH0,
+	LTC2309_CH2,
+	LTC2309_CH4,
+	LTC2309_CH6,
+	LTC2309_CH1,
+	LTC2309_CH3,
+	LTC2309_CH5,
+	LTC2309_CH7,
+};
+
+#define LTC2309_CHAN(_chan, _addr) {				\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.address = _addr,					\
+	.channel = _chan,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = "CH"#_chan,				\
+}
+
+#define LTC2309_DIFF_CHAN(_chan, _chan2, _addr) {		\
+	.type = IIO_VOLTAGE,					\
+	.differential = 1,					\
+	.indexed = 1,						\
+	.address = _addr,					\
+	.channel = _chan,					\
+	.channel2 = _chan2,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = "CH"#_chan"-CH"#_chan2,		\
+}
+
+static const struct iio_chan_spec ltc2309_channels[] = {
+	LTC2309_CHAN(0, LTC2309_CH0),
+	LTC2309_CHAN(1, LTC2309_CH1),
+	LTC2309_CHAN(2, LTC2309_CH2),
+	LTC2309_CHAN(3, LTC2309_CH3),
+	LTC2309_CHAN(4, LTC2309_CH4),
+	LTC2309_CHAN(5, LTC2309_CH5),
+	LTC2309_CHAN(6, LTC2309_CH6),
+	LTC2309_CHAN(7, LTC2309_CH7),
+	LTC2309_DIFF_CHAN(0, 1, LTC2309_CH0_CH1),
+	LTC2309_DIFF_CHAN(2, 3, LTC2309_CH2_CH3),
+	LTC2309_DIFF_CHAN(4, 5, LTC2309_CH4_CH5),
+	LTC2309_DIFF_CHAN(6, 7, LTC2309_CH6_CH7),
+	LTC2309_DIFF_CHAN(1, 0, LTC2309_CH1_CH0),
+	LTC2309_DIFF_CHAN(3, 2, LTC2309_CH3_CH2),
+	LTC2309_DIFF_CHAN(5, 4, LTC2309_CH5_CH4),
+	LTC2309_DIFF_CHAN(7, 6, LTC2309_CH7_CH6),
+};
+
+static int ltc2309_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct ltc2309 *ltc2309 = iio_priv(indio_dev);
+	u16 buf;
+	int ret;
+	u8 din;
+
+	mutex_lock(&ltc2309->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) |
+			FIELD_PREP(LTC2309_DIN_UNI, 1) |
+			FIELD_PREP(LTC2309_DIN_SLEEP, 0);
+
+		ret = i2c_smbus_write_byte(ltc2309->client, din);
+		if (ret < 0) {
+			dev_err(ltc2309->dev, "i2c command failed: %pe\n",
+				ERR_PTR(ret));
+			goto out;
+		}
+
+		ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2);
+		if (ret < 0) {
+			dev_err(ltc2309->dev, "i2c read failed: %pe\n",
+				ERR_PTR(ret));
+			goto out;
+		}
+
+		*val = be16_to_cpu(buf) >> 4;
+
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		*val = ltc2309->vref_mv;
+		*val2 = LTC2309_ADC_RESOLUTION;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+out:
+	mutex_unlock(&ltc2309->lock);
+	return ret;
+}
+
+static const struct iio_info ltc2309_info = {
+	.read_raw = ltc2309_read_raw,
+};
+
+static int ltc2309_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct ltc2309 *ltc2309;
+	int ret = 0;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, indio_dev);
+
+	ltc2309 = iio_priv(indio_dev);
+	ltc2309->dev = &indio_dev->dev;
+	ltc2309->client = client;
+	ltc2309->vref_mv = 4096; /* Default to the internal ref */
+
+	indio_dev->name = DRIVER_NAME;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ltc2309_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels);
+	indio_dev->info = &ltc2309_info;
+
+	ltc2309->refcomp = devm_regulator_get_optional(&client->dev, "refcomp");
+	if (!IS_ERR_OR_NULL(ltc2309->refcomp)) {
+		ret = regulator_enable(ltc2309->refcomp);
+		if (ret) {
+			dev_err(ltc2309->dev, "failed to enable REFCOMP\n");
+			return ret;
+		}
+
+		ret = regulator_get_voltage(ltc2309->refcomp);
+		if (ret < 0)
+			return ret;
+
+		ltc2309->vref_mv = ret / 1000;
+		if (ret)
+			return ret;
+	}
+
+	mutex_init(&ltc2309->lock);
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id ltc2309_of_match[] = {
+	{ .compatible = "lltc,ltc2309" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ltc2309_of_match);
+
+static const struct i2c_device_id ltc2309_id[] = {
+	{"ltc2309", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ltc2309_id);
+
+static struct i2c_driver ltc2309_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = ltc2309_of_match,
+	},
+	.probe		= ltc2309_probe,
+	.id_table	= ltc2309_id,
+};
+module_i2c_driver(ltc2309_driver);
+
+MODULE_AUTHOR("Liam Beguin <liambeguin@gmail.com>");
+MODULE_DESCRIPTION("Linear Technology LTC2309 ADC");
+MODULE_LICENSE("GPL v2");

-- 
2.39.0

[PATCH 2/3] iio: adc: ltc2309: switch to new .probe()

[Liam Beguin]

Recent changes to the I2C subsystem removed the id parameter of the
probe function. Update driver to use the new prototype, and keep this as
an independent commit to facilitate backporting.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 drivers/iio/adc/ltc2309.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c
index ee1fd9b82e2a..d26bbd70b0ff 100644
--- a/drivers/iio/adc/ltc2309.c
+++ b/drivers/iio/adc/ltc2309.c
@@ -158,8 +158,7 @@ static const struct iio_info ltc2309_info = {
 	.read_raw = ltc2309_read_raw,
 };
 
-static int ltc2309_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int ltc2309_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
 	struct ltc2309 *ltc2309;

-- 
2.39.0

[PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Liam Beguin]

Add devicetree bindings for the Linear Technology LTC2309 ADC driver.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 .../devicetree/bindings/iio/adc/lltc,ltc2309.yaml  | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
new file mode 100644
index 000000000000..7874290dad75
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/lltc,ltc2309.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Linear Technology / Analog Devices LTC2309 ADC
+
+maintainers:
+  - Liam Beguin <liambeguin@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - lltc,ltc2309
+
+  refcomp-supply:
+    description: Power supply for the reference voltage
+
+  reg:
+    enum:
+      - 0x08
+      - 0x09
+      - 0x0a
+      - 0x0b
+      - 0x18
+      - 0x19
+      - 0x1a
+      - 0x1b
+      - 0x28
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@28 {
+            #io-channel-cells = <1>;
+            compatible = "lltc,ltc2309";
+            reg = <0x28>;
+        };
+    };

-- 
2.39.0

Re: [PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Krzysztof Kozlowski]

On 24/08/2023 18:55, Liam Beguin wrote:
> Add devicetree bindings for the Linear Technology LTC2309 ADC driver.
> 
> Signed-off-by: Liam Beguin <liambeguin@gmail.com>

Thank you for your patch. There is something to discuss/improve.


> +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: GPL-2.0

Wrong license. Run checkpatch before sending patches.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/lltc,ltc2309.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Linear Technology / Analog Devices LTC2309 ADC
> +
> +maintainers:
> +  - Liam Beguin <liambeguin@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lltc,ltc2309
> +
> +  refcomp-supply:
> +    description: Power supply for the reference voltage

refcomp is not a supply. It is called "Reference Buffer Output.". You
probably wanted vref-supply, which suggests you should just add it to
ltc2497 bindings. I don't see any differences.

> +
> +  reg:
> +    enum:
> +      - 0x08
> +      - 0x09
> +      - 0x0a
> +      - 0x0b
> +      - 0x18
> +      - 0x19
> +      - 0x1a
> +      - 0x1b
> +      - 0x28
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@28 {
> +            #io-channel-cells = <1>;
> +            compatible = "lltc,ltc2309";
> +            reg = <0x28>;

If the example stays, then order is compatible first, then reg, then the
rest. Also add the supply to make example complete.

But I think this should be squashed with other binding so no need for
the example.

Best regards,
Krzysztof

Re: [PATCH 1/3] iio: adc: add ltc2309 support

[Krzysztof Kozlowski]

On 24/08/2023 18:55, Liam Beguin wrote:
> The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface.
> 
> This implements support for all single-ended and differential channels,
> in unipolar mode only.
> 
> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> ---
>  drivers/iio/adc/Kconfig   |  10 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/ltc2309.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 243 insertions(+)
> 



> +static int ltc2309_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct ltc2309 *ltc2309 = iio_priv(indio_dev);
> +	u16 buf;
> +	int ret;
> +	u8 din;
> +
> +	mutex_lock(&ltc2309->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) |
> +			FIELD_PREP(LTC2309_DIN_UNI, 1) |
> +			FIELD_PREP(LTC2309_DIN_SLEEP, 0);
> +
> +		ret = i2c_smbus_write_byte(ltc2309->client, din);
> +		if (ret < 0) {
> +			dev_err(ltc2309->dev, "i2c command failed: %pe\n",
> +				ERR_PTR(ret));
> +			goto out;
> +		}
> +
> +		ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2);
> +		if (ret < 0) {
> +			dev_err(ltc2309->dev, "i2c read failed: %pe\n",
> +				ERR_PTR(ret));
> +			goto out;
> +		}
> +
> +		*val = be16_to_cpu(buf) >> 4;
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = ltc2309->vref_mv;
> +		*val2 = LTC2309_ADC_RESOLUTION;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;

Why this case is in critical section?

> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +out:
> +	mutex_unlock(&ltc2309->lock);
> +	return ret;
> +}
> +
> +static const struct iio_info ltc2309_info = {
> +	.read_raw = ltc2309_read_raw,
> +};
> +
> +static int ltc2309_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2309 *ltc2309;
> +	int ret = 0;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	ltc2309 = iio_priv(indio_dev);
> +	ltc2309->dev = &indio_dev->dev;
> +	ltc2309->client = client;
> +	ltc2309->vref_mv = 4096; /* Default to the internal ref */
> +
> +	indio_dev->name = DRIVER_NAME;
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2309_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels);
> +	indio_dev->info = &ltc2309_info;
> +
> +	ltc2309->refcomp = devm_regulator_get_optional(&client->dev, "refcomp");
> +	if (!IS_ERR_OR_NULL(ltc2309->refcomp)) {
> +		ret = regulator_enable(ltc2309->refcomp);
> +		if (ret) {
> +			dev_err(ltc2309->dev, "failed to enable REFCOMP\n");
> +			return ret;
> +		}
> +
> +		ret = regulator_get_voltage(ltc2309->refcomp);
> +		if (ret < 0)

You have unbalanced regulator. Same in all further error paths.

> +			return ret;
> +
> +		ltc2309->vref_mv = ret / 1000;
> +		if (ret)
> +			return ret;
> +	}
> +
> +	mutex_init(&ltc2309->lock);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +


Best regards,
Krzysztof

Re: [PATCH 2/3] iio: adc: ltc2309: switch to new .probe()

[Krzysztof Kozlowski]

On 24/08/2023 18:55, Liam Beguin wrote:
> Recent changes to the I2C subsystem removed the id parameter of the
> probe function. Update driver to use the new prototype, and keep this as
> an independent commit to facilitate backporting.
> 
> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> ---
>  drivers/iio/adc/ltc2309.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c
> index ee1fd9b82e2a..d26bbd70b0ff 100644
> --- a/drivers/iio/adc/ltc2309.c
> +++ b/drivers/iio/adc/ltc2309.c
> @@ -158,8 +158,7 @@ static const struct iio_info ltc2309_info = {
>  	.read_raw = ltc2309_read_raw,
>  };
>  
> -static int ltc2309_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id)
> +static int ltc2309_probe(struct i2c_client *client)

This patch does not make sense. Do not send patch (1/3) which does not
compile and is buggy, just to immediately fix it. We do not add known
wrong code.

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Liam Beguin]

On Thu, Aug 24, 2023 at 07:56:29PM +0200, Krzysztof Kozlowski wrote:
> On 24/08/2023 18:55, Liam Beguin wrote:
> > Add devicetree bindings for the Linear Technology LTC2309 ADC driver.
> > 
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Wrong license. Run checkpatch before sending patches.
> 

Sorry about that, I ran it through checkpatch but it didn't flag
anything.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/lltc,ltc2309.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Linear Technology / Analog Devices LTC2309 ADC
> > +
> > +maintainers:
> > +  - Liam Beguin <liambeguin@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - lltc,ltc2309
> > +
> > +  refcomp-supply:
> > +    description: Power supply for the reference voltage
> 
> refcomp is not a supply. It is called "Reference Buffer Output.". You

That makes sense, I was going for the PIN name from the datasheet.

> probably wanted vref-supply, which suggests you should just add it to
> ltc2497 bindings. I don't see any differences.
> 

I hadn't thought of reusing an existing bindings file for a different
driver. I'll update ltc2497.yaml instead since it avoids duplicating the
whole file.

> > +
> > +  reg:
> > +    enum:
> > +      - 0x08
> > +      - 0x09
> > +      - 0x0a
> > +      - 0x0b
> > +      - 0x18
> > +      - 0x19
> > +      - 0x1a
> > +      - 0x1b
> > +      - 0x28
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        adc@28 {
> > +            #io-channel-cells = <1>;
> > +            compatible = "lltc,ltc2309";
> > +            reg = <0x28>;
> 
> If the example stays, then order is compatible first, then reg, then the
> rest. Also add the supply to make example complete.

Thanks for pointing out the proper order.

> But I think this should be squashed with other binding so no need for
> the example.
> 
> Best regards,
> Krzysztof
> 

Thanks for your time!
Liam

Re: [PATCH 1/3] iio: adc: add ltc2309 support

[Liam Beguin]

On Thu, Aug 24, 2023 at 08:00:11PM +0200, Krzysztof Kozlowski wrote:
> On 24/08/2023 18:55, Liam Beguin wrote:
> > The LTC2309 is an 8-Channel, 12-Bit SAR ADC with an I2C Interface.
> > 
> > This implements support for all single-ended and differential channels,
> > in unipolar mode only.
> > 
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> > ---
> >  drivers/iio/adc/Kconfig   |  10 ++
> >  drivers/iio/adc/Makefile  |   1 +
> >  drivers/iio/adc/ltc2309.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 243 insertions(+)
> > 
> 
> 
> 
> > +static int ltc2309_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	struct ltc2309 *ltc2309 = iio_priv(indio_dev);
> > +	u16 buf;
> > +	int ret;
> > +	u8 din;
> > +
> > +	mutex_lock(&ltc2309->lock);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) |
> > +			FIELD_PREP(LTC2309_DIN_UNI, 1) |
> > +			FIELD_PREP(LTC2309_DIN_SLEEP, 0);
> > +
> > +		ret = i2c_smbus_write_byte(ltc2309->client, din);
> > +		if (ret < 0) {
> > +			dev_err(ltc2309->dev, "i2c command failed: %pe\n",
> > +				ERR_PTR(ret));
> > +			goto out;
> > +		}
> > +
> > +		ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2);
> > +		if (ret < 0) {
> > +			dev_err(ltc2309->dev, "i2c read failed: %pe\n",
> > +				ERR_PTR(ret));
> > +			goto out;
> > +		}
> > +
> > +		*val = be16_to_cpu(buf) >> 4;
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = ltc2309->vref_mv;
> > +		*val2 = LTC2309_ADC_RESOLUTION;
> > +		ret = IIO_VAL_FRACTIONAL_LOG2;
> 
> Why this case is in critical section?
> 

my bad, I'll reduce it to INFO_RAW.

> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&ltc2309->lock);
> > +	return ret;
> > +}
> > +
> > +static const struct iio_info ltc2309_info = {
> > +	.read_raw = ltc2309_read_raw,
> > +};
> > +
> > +static int ltc2309_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct ltc2309 *ltc2309;
> > +	int ret = 0;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ltc2309));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, indio_dev);
> > +
> > +	ltc2309 = iio_priv(indio_dev);
> > +	ltc2309->dev = &indio_dev->dev;
> > +	ltc2309->client = client;
> > +	ltc2309->vref_mv = 4096; /* Default to the internal ref */
> > +
> > +	indio_dev->name = DRIVER_NAME;
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = ltc2309_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ltc2309_channels);
> > +	indio_dev->info = &ltc2309_info;
> > +
> > +	ltc2309->refcomp = devm_regulator_get_optional(&client->dev, "refcomp");
> > +	if (!IS_ERR_OR_NULL(ltc2309->refcomp)) {
> > +		ret = regulator_enable(ltc2309->refcomp);
> > +		if (ret) {
> > +			dev_err(ltc2309->dev, "failed to enable REFCOMP\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = regulator_get_voltage(ltc2309->refcomp);
> > +		if (ret < 0)
> 
> You have unbalanced regulator. Same in all further error paths.
> 

Right, will fix.

I was going to add an action with devm_add_action_or_reset(), and
noticed a lot of duplicate code adding a custom disable action. Does
adding something like this make sense?

-- >8 --

diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 90bb0d178885..ff94f35fad87 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -70,12 +70,17 @@ struct regulator *devm_regulator_get_exclusive(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);

-static void regulator_action_disable(void *d)
+/**
+ * regulator_action_disable - Generic disable action for managed resource
+ * @d: regulator to disable
+ */
+void regulator_action_disable(void *d)
 {
 	struct regulator *r = (struct regulator *)d;

 	regulator_disable(r);
 }
+EXPORT_SYMBOL_GPL(regulator_action_disable);

 static int _devm_regulator_get_enable(struct device *dev, const char *id,
 				      int get_type)
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 39b666b40ea6..4c018af5d008 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -207,6 +207,8 @@ struct regulator *__must_check regulator_get_optional(struct device *dev,
 						      const char *id);
 struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
 							   const char *id);
+
+void regulator_action_disable(void *d);
 int devm_regulator_get_enable(struct device *dev, const char *id);
 int devm_regulator_get_enable_optional(struct device *dev, const char *id);
 void regulator_put(struct regulator *regulator);

-- >8 --

This would let consumers reuse it directly with something like:

	devm_add_action_or_reset(ltc2309->dev,
				 regulator_action_disable,
				 ltc2309->vref);


Maybe it should be a separate series, including the cleanup?

> > +			return ret;
> > +
> > +		ltc2309->vref_mv = ret / 1000;
> > +		if (ret)
> > +			return ret;

I just noticed this extra if. will remove too.

> > +	}
> > +
> > +	mutex_init(&ltc2309->lock);
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> 
> 
> Best regards,
> Krzysztof
> 

Thanks,
Liam

Re: [PATCH 2/3] iio: adc: ltc2309: switch to new .probe()

[Liam Beguin]

On Thu, Aug 24, 2023 at 08:01:01PM +0200, Krzysztof Kozlowski wrote:
> On 24/08/2023 18:55, Liam Beguin wrote:
> > Recent changes to the I2C subsystem removed the id parameter of the
> > probe function. Update driver to use the new prototype, and keep this as
> > an independent commit to facilitate backporting.
> > 
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> > ---
> >  drivers/iio/adc/ltc2309.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ltc2309.c b/drivers/iio/adc/ltc2309.c
> > index ee1fd9b82e2a..d26bbd70b0ff 100644
> > --- a/drivers/iio/adc/ltc2309.c
> > +++ b/drivers/iio/adc/ltc2309.c
> > @@ -158,8 +158,7 @@ static const struct iio_info ltc2309_info = {
> >  	.read_raw = ltc2309_read_raw,
> >  };
> >  
> > -static int ltc2309_probe(struct i2c_client *client,
> > -			 const struct i2c_device_id *id)
> > +static int ltc2309_probe(struct i2c_client *client)
> 
> This patch does not make sense. Do not send patch (1/3) which does not
> compile and is buggy, just to immediately fix it. We do not add known
> wrong code.

Sorry about that! You're right, I missed that 1/3 doesn't build without
this change. I might've pressed send too quickly here..

> Best regards,
> Krzysztof
> 

Thanks,
Liam

Re: [PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Krzysztof Kozlowski]

On 24/08/2023 20:50, Liam Beguin wrote:
> On Thu, Aug 24, 2023 at 07:56:29PM +0200, Krzysztof Kozlowski wrote:
>> On 24/08/2023 18:55, Liam Beguin wrote:
>>> Add devicetree bindings for the Linear Technology LTC2309 ADC driver.
>>>
>>> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>
>> Wrong license. Run checkpatch before sending patches.
>>
> 
> Sorry about that, I ran it through checkpatch but it didn't flag
> anything.

No, you didn't, because checkpatch flags it easily:

WARNING: DT binding documents should be licensed (GPL-2.0-only OR
BSD-2-Clause)
#21: FILE: Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml:1:
+# SPDX-License-Identifier: GPL-2.0

total: 0 errors, 2 warnings, 52 lines checked


Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Liam Beguin]

Hi Krzysztof,

On Fri, Aug 25, 2023 at 08:15:57AM +0200, Krzysztof Kozlowski wrote:
> On 24/08/2023 20:50, Liam Beguin wrote:
> > On Thu, Aug 24, 2023 at 07:56:29PM +0200, Krzysztof Kozlowski wrote:
> >> On 24/08/2023 18:55, Liam Beguin wrote:
> >>> Add devicetree bindings for the Linear Technology LTC2309 ADC driver.
> >>>
> >>> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >>> +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
> >>> @@ -0,0 +1,52 @@
> >>> +# SPDX-License-Identifier: GPL-2.0
> >>
> >> Wrong license. Run checkpatch before sending patches.
> >>
> > 
> > Sorry about that, I ran it through checkpatch but it didn't flag
> > anything.
> 
> No, you didn't, because checkpatch flags it easily:
> 
> WARNING: DT binding documents should be licensed (GPL-2.0-only OR
> BSD-2-Clause)
> #21: FILE: Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml:1:
> +# SPDX-License-Identifier: GPL-2.0
> 
> total: 0 errors, 2 warnings, 52 lines checked

It seems like I wasn't running checkpatch in strict mode.
Thanks for pointing that out.

> 
> Best regards,
> Krzysztof

Cheers,
Liam

Re: [PATCH 3/3] dt-bindings: iio: adc: add lltc,ltc2309 bindings

[Krzysztof Kozlowski]

On 25/08/2023 17:53, Liam Beguin wrote:
> Hi Krzysztof,
> 
> On Fri, Aug 25, 2023 at 08:15:57AM +0200, Krzysztof Kozlowski wrote:
>> On 24/08/2023 20:50, Liam Beguin wrote:
>>> On Thu, Aug 24, 2023 at 07:56:29PM +0200, Krzysztof Kozlowski wrote:
>>>> On 24/08/2023 18:55, Liam Beguin wrote:
>>>>> Add devicetree bindings for the Linear Technology LTC2309 ADC driver.
>>>>>
>>>>> Signed-off-by: Liam Beguin <liambeguin@gmail.com>
>>>>
>>>> Thank you for your patch. There is something to discuss/improve.
>>>>
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml
>>>>> @@ -0,0 +1,52 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>
>>>> Wrong license. Run checkpatch before sending patches.
>>>>
>>>
>>> Sorry about that, I ran it through checkpatch but it didn't flag
>>> anything.
>>
>> No, you didn't, because checkpatch flags it easily:
>>
>> WARNING: DT binding documents should be licensed (GPL-2.0-only OR
>> BSD-2-Clause)
>> #21: FILE: Documentation/devicetree/bindings/iio/adc/lltc,ltc2309.yaml:1:
>> +# SPDX-License-Identifier: GPL-2.0
>>
>> total: 0 errors, 2 warnings, 52 lines checked
> 
> It seems like I wasn't running checkpatch in strict mode.

No. This was not a strict mode but a regular run. I don't understand why
you are turning this all the time around... You can easily check by
yourself and reproduce the warning any time.


Best regards,
Krzysztof

Re: [PATCH 2/3] iio: adc: ltc2309: switch to new .probe()

[kernel test robot]

Hi Liam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a5e505a99ca748583dbe558b691be1b26f05d678]

url:    https://github.com/intel-lab-lkp/linux/commits/Liam-Beguin/iio-adc-add-ltc2309-support/20230825-005720
base:   a5e505a99ca748583dbe558b691be1b26f05d678
patch link:    https://lore.kernel.org/r/20230824-ltc2309-v1-2-b87b4eb8030c%40gmail.com
patch subject: [PATCH 2/3] iio: adc: ltc2309: switch to new .probe()
config: i386-randconfig-061-20230906 (https://download.01.org/0day-ci/archive/20230906/202309060456.UYGqTIBd-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230906/202309060456.UYGqTIBd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309060456.UYGqTIBd-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/adc/ltc2309.c:138:24: sparse: sparse: cast to restricted __be16

vim +138 drivers/iio/adc/ltc2309.c

a967828958b014 Liam Beguin 2023-08-24  106  
a967828958b014 Liam Beguin 2023-08-24  107  static int ltc2309_read_raw(struct iio_dev *indio_dev,
a967828958b014 Liam Beguin 2023-08-24  108  			    struct iio_chan_spec const *chan, int *val,
a967828958b014 Liam Beguin 2023-08-24  109  			    int *val2, long mask)
a967828958b014 Liam Beguin 2023-08-24  110  {
a967828958b014 Liam Beguin 2023-08-24  111  	struct ltc2309 *ltc2309 = iio_priv(indio_dev);
a967828958b014 Liam Beguin 2023-08-24  112  	u16 buf;
a967828958b014 Liam Beguin 2023-08-24  113  	int ret;
a967828958b014 Liam Beguin 2023-08-24  114  	u8 din;
a967828958b014 Liam Beguin 2023-08-24  115  
a967828958b014 Liam Beguin 2023-08-24  116  	mutex_lock(&ltc2309->lock);
a967828958b014 Liam Beguin 2023-08-24  117  
a967828958b014 Liam Beguin 2023-08-24  118  	switch (mask) {
a967828958b014 Liam Beguin 2023-08-24  119  	case IIO_CHAN_INFO_RAW:
a967828958b014 Liam Beguin 2023-08-24  120  		din = FIELD_PREP(LTC2309_DIN_CH_MASK, chan->address & 0x0f) |
a967828958b014 Liam Beguin 2023-08-24  121  			FIELD_PREP(LTC2309_DIN_UNI, 1) |
a967828958b014 Liam Beguin 2023-08-24  122  			FIELD_PREP(LTC2309_DIN_SLEEP, 0);
a967828958b014 Liam Beguin 2023-08-24  123  
a967828958b014 Liam Beguin 2023-08-24  124  		ret = i2c_smbus_write_byte(ltc2309->client, din);
a967828958b014 Liam Beguin 2023-08-24  125  		if (ret < 0) {
a967828958b014 Liam Beguin 2023-08-24  126  			dev_err(ltc2309->dev, "i2c command failed: %pe\n",
a967828958b014 Liam Beguin 2023-08-24  127  				ERR_PTR(ret));
a967828958b014 Liam Beguin 2023-08-24  128  			goto out;
a967828958b014 Liam Beguin 2023-08-24  129  		}
a967828958b014 Liam Beguin 2023-08-24  130  
a967828958b014 Liam Beguin 2023-08-24  131  		ret = i2c_master_recv(ltc2309->client, (char *)&buf, 2);
a967828958b014 Liam Beguin 2023-08-24  132  		if (ret < 0) {
a967828958b014 Liam Beguin 2023-08-24  133  			dev_err(ltc2309->dev, "i2c read failed: %pe\n",
a967828958b014 Liam Beguin 2023-08-24  134  				ERR_PTR(ret));
a967828958b014 Liam Beguin 2023-08-24  135  			goto out;
a967828958b014 Liam Beguin 2023-08-24  136  		}
a967828958b014 Liam Beguin 2023-08-24  137  
a967828958b014 Liam Beguin 2023-08-24 @138  		*val = be16_to_cpu(buf) >> 4;
a967828958b014 Liam Beguin 2023-08-24  139  
a967828958b014 Liam Beguin 2023-08-24  140  		ret = IIO_VAL_INT;
a967828958b014 Liam Beguin 2023-08-24  141  		break;
a967828958b014 Liam Beguin 2023-08-24  142  	case IIO_CHAN_INFO_SCALE:
a967828958b014 Liam Beguin 2023-08-24  143  		*val = ltc2309->vref_mv;
a967828958b014 Liam Beguin 2023-08-24  144  		*val2 = LTC2309_ADC_RESOLUTION;
a967828958b014 Liam Beguin 2023-08-24  145  		ret = IIO_VAL_FRACTIONAL_LOG2;
a967828958b014 Liam Beguin 2023-08-24  146  		break;
a967828958b014 Liam Beguin 2023-08-24  147  	default:
a967828958b014 Liam Beguin 2023-08-24  148  		ret = -EINVAL;
a967828958b014 Liam Beguin 2023-08-24  149  		break;
a967828958b014 Liam Beguin 2023-08-24  150  	}
a967828958b014 Liam Beguin 2023-08-24  151  
a967828958b014 Liam Beguin 2023-08-24  152  out:
a967828958b014 Liam Beguin 2023-08-24  153  	mutex_unlock(&ltc2309->lock);
a967828958b014 Liam Beguin 2023-08-24  154  	return ret;
a967828958b014 Liam Beguin 2023-08-24  155  }
a967828958b014 Liam Beguin 2023-08-24  156  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki