[PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Beniamin Bia]

From: Michael Hennerich <michael.hennerich@analog.com>

ADM1177 is a Hot Swap Controller and Digital Power Monitor with
Soft Start Pin.

Datasheet:
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Co-developed-by: Beniamin Bia <beniamin.bia@analog.com>
Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
---
 drivers/iio/adc/Kconfig   |  12 ++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/adm1177.c | 257 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/iio/adc/adm1177.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9554890a3200..4db8c6dcb595 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -228,6 +228,18 @@ config ASPEED_ADC
 	  To compile this driver as a module, choose M here: the module will be
 	  called aspeed_adc.
 
+config ADM1177
+	tristate "Analog Devices ADM1177 Digital Power Monitor driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Analog Devices:
+	  ADM1177 Hot Swap Controller and Digital Power Monitor
+	  with Soft Start Pin. Provides direct access
+	  via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adm1177.
+
 config AT91_ADC
 	tristate "Atmel AT91 ADC"
 	depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 5ecc481c2967..7899d6a158f3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD7949) += ad7949.o
 obj-$(CONFIG_AD799X) += ad799x.o
 obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
+obj-$(CONFIG_ADM1177) += adm1177.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
 obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o
diff --git a/drivers/iio/adc/adm1177.c b/drivers/iio/adc/adm1177.c
new file mode 100644
index 000000000000..daec34566a65
--- /dev/null
+++ b/drivers/iio/adc/adm1177.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADM1177 Hot Swap Controller and Digital Power Monitor with Soft Start Pin
+ *
+ * Copyright 2015-2019 Analog Devices Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+/*  Command Byte Operations */
+#define ADM1177_CMD_V_CONT	BIT(0)
+#define ADM1177_CMD_V_ONCE	BIT(1)
+#define ADM1177_CMD_I_CONT	BIT(2)
+#define ADM1177_CMD_I_ONCE	BIT(3)
+#define ADM1177_CMD_VRANGE	BIT(4)
+#define ADM1177_CMD_STATUS_RD	BIT(6)
+
+/* Extended Register */
+#define ADM1177_REG_ALERT_EN	1
+#define ADM1177_REG_ALERT_TH	2
+#define ADM1177_REG_CONTROL	3
+
+/* ADM1177_REG_ALERT_EN */
+#define ADM1177_EN_ADC_OC1	BIT(0)
+#define ADM1177_EN_ADC_OC4	BIT(1)
+#define ADM1177_EN_HS_ALERT	BIT(2)
+#define ADM1177_EN_OFF_ALERT	BIT(3)
+#define ADM1177_CLEAR		BIT(4)
+
+/* ADM1177_REG_CONTROL */
+#define ADM1177_SWOFF		BIT(0)
+
+#define ADM1177_BITS		12
+
+/**
+ * struct adm1177_state - driver instance specific data
+ * @client		pointer to i2c client
+ * @reg			regulator info for the the power supply of the device
+ * @command		internal control register
+ * @r_sense_uohm	current sense resistor value
+ * @alert_threshold_ua	current limit for shutdown
+ * @vrange_high		internal voltage divider
+ */
+struct adm1177_state {
+	struct i2c_client	*client;
+	struct regulator	*reg;
+	u8			command;
+	u32			r_sense_uohm;
+	u32			alert_threshold_ua;
+	bool			vrange_high;
+};
+
+static int adm1177_read(struct adm1177_state *st, u8 num, u8 *data)
+{
+	struct i2c_client *client = st->client;
+	int ret = 0;
+
+	ret = i2c_master_recv(client, data, num);
+	if (ret < 0) {
+		dev_err(&client->dev, "I2C read error\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd)
+{
+	st->command = cmd;
+	return i2c_smbus_write_byte(st->client, cmd);
+}
+
+static int adm1177_write_reg(struct adm1177_state *st, u8 reg, u8 val)
+{
+	return i2c_smbus_write_byte_data(st->client, reg, val);
+}
+
+static int adm1177_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long mask)
+{
+	struct adm1177_state *st = iio_priv(indio_dev);
+	u8 data[3];
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		adm1177_read(st, 3, data);
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = (data[0] << 4) | (data[2] >> 4);
+			return IIO_VAL_INT;
+		case IIO_CURRENT:
+			*val = (data[1] << 4) | (data[2] & 0xF);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (st->command & ADM1177_CMD_VRANGE)
+				*val = 6650;
+			else
+				*val = 26350;
+
+			*val2 = ADM1177_BITS;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_CURRENT:
+			*val = 105840 / st->r_sense_uohm;
+
+			*val2 = ADM1177_BITS;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec adm1177_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = 0,
+	},
+	{
+		.type = IIO_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = 0,
+	},
+};
+
+static const struct iio_info adm1177_info = {
+	.read_raw = &adm1177_read_raw,
+};
+static void adm1177_remove(void *data)
+{
+	struct adm1177_state *st = data;
+
+	if (st->reg)
+		regulator_disable(st->reg);
+}
+
+static int adm1177_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	int ret;
+	struct device_node *np;
+	struct adm1177_state *st;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	st->client = client;
+
+	np = client->dev.of_node;
+
+	st->reg = devm_regulator_get_optional(&client->dev, "vref");
+	if (IS_ERR(st->reg)) {
+		if (PTR_ERR(st->reg) == EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		st->reg = NULL;
+	} else {
+		ret = regulator_enable(st->reg);
+		if (ret)
+			return ret;
+		ret = devm_add_action_or_reset(&client->dev, adm1177_remove,
+					       st);
+		if (ret)
+			return ret;
+	}
+
+	of_property_read_u32(np, "adi,r-sense-micro-ohms", &st->r_sense_uohm);
+	of_property_read_u32(np, "adi,shutdown-threshold-microamp",
+			     &st->alert_threshold_ua);
+	st->vrange_high = of_property_read_bool(np,
+				"adi,vrange-high-enable");
+	if (st->alert_threshold_ua) {
+		unsigned int val;
+
+		val = (st->alert_threshold_ua * st->r_sense_uohm * 0xFF);
+		val /= 105840000U;
+		if (val > 0xFF) {
+			dev_err(&client->dev,
+				"Requested shutdown current %d uA above limit\n",
+				st->alert_threshold_ua);
+
+			val = 0xFF;
+		}
+		adm1177_write_reg(st, ADM1177_REG_ALERT_TH, val);
+	}
+
+	adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
+			      ADM1177_CMD_I_CONT |
+			      (st->vrange_high ? 0 : ADM1177_CMD_VRANGE));
+
+	indio_dev->name = id->name;
+	indio_dev->channels = adm1177_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adm1177_channels);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &adm1177_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id adm1177_ids[] = {
+	{ "adm1177", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, adm1177_ids);
+
+static const struct of_device_id adm1177_dt_ids[] = {
+	{ .compatible = "adi,adm1177" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, adm1177_dt_ids);
+
+static struct i2c_driver adm1177_driver = {
+	.driver = {
+		.name = "adm1177",
+		.of_match_table = adm1177_dt_ids,
+	},
+	.probe = adm1177_probe,
+	.id_table = adm1177_ids,
+};
+module_i2c_driver(adm1177_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH 2/3] dt-binding: iio: Add documentation for ADM1177

[Beniamin Bia]

Documentation for ADM1177 was added.

Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
---
 .../bindings/iio/adc/adi,adm1177.yaml         | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml b/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
new file mode 100644
index 000000000000..69a0230e59f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,adm1177.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADM1177 Hot Swap Controller and Digital Power Monitor
+
+maintainers:
+  - Michael Hennerich <michael.hennerich@analog.com>
+  - Beniamin Bia <beniamin.bia@analog.com>
+
+description: |
+  Analog Devices ADM1177 Hot Swap Controller and Digital Power Monitor
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,adm1177
+
+  reg:
+    maxItems: 1
+
+  avcc-supply:
+    description:
+      Phandle to the Avcc power supply
+
+  adi,r-sense-micro-ohms:
+    description:
+      The value of curent sense resistor in microohms.
+
+  adi,shutdown-threshold-microamp:
+    description:
+      Specifies the current level at which an over current alert occurs.
+    maximum: 255
+
+  adi,vrange-high-enable:
+    description:
+      Specifies which internal voltage divider to be used. A 1 selects
+      a 7:2 voltage divider while a 0 selects a 14:1 voltage divider.
+    type: boolean
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@b4 {
+                compatible = "adi,adm1177";
+                reg = <0xb4>;
+        };
+    };
+...
-- 
2.17.1

[PATCH 3/3] MAINTAINERS: add entry for ADM1177 driver

[Beniamin Bia]

Add Beniamin Bia and Michael Hennerich as a maintainer for ADM1177 ADC.

Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fca3b055985..41a34d7a802c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -962,6 +962,15 @@ W:	http://ez.analog.com/community/linux-device-drivers
 F:	drivers/iio/imu/adis16460.c
 F:	Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
 
+ANALOG DEVICES INC ADM1177 DRIVER
+M:	Beniamin Bia <beniamin.bia@analog.com>
+M:	Michael Hennerich <Michael.Hennerich@analog.com>
+L:	linux-iio@vger.kernel.org
+W:	http://ez.analog.com/community/linux-device-drivers
+S:	Supported
+F:	drivers/iio/adc/adm1177.c
+F:	Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
+
 ANALOG DEVICES INC ADP5061 DRIVER
 M:	Stefan Popa <stefan.popa@analog.com>
 L:	linux-pm@vger.kernel.org
-- 
2.17.1

Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Jonathan Cameron]

On Tue, 12 Nov 2019 17:35:50 +0200
Beniamin Bia <beniamin.bia@analog.com> wrote:

> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> ADM1177 is a Hot Swap Controller and Digital Power Monitor with
> Soft Start Pin.
> 
> Datasheet:
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Co-developed-by: Beniamin Bia <beniamin.bia@analog.com>
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>

Hi Beniamin,

A couple immediate thoughts.

1. That cc list has some rather non obvious people on it.  Unless something
   fairly surprising is going on, probably better to cut it back a bit.

2. Why IIO?  Not entirely obvious to me.  From first glance this is definitely
   doing hardware monitoring.  If there is a reason there should be a clear
   statement here on why.

+CC Guenter and Jean as hwmon maintainers.

Whilst I'm here, a very quick review below.

> ---
>  drivers/iio/adc/Kconfig   |  12 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/adm1177.c | 257 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/iio/adc/adm1177.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9554890a3200..4db8c6dcb595 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -228,6 +228,18 @@ config ASPEED_ADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called aspeed_adc.
>  
> +config ADM1177
> +	tristate "Analog Devices ADM1177 Digital Power Monitor driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Analog Devices:
> +	  ADM1177 Hot Swap Controller and Digital Power Monitor
> +	  with Soft Start Pin. Provides direct access
> +	  via sysfs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called adm1177.
> +
>  config AT91_ADC
>  	tristate "Atmel AT91 ADC"
>  	depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 5ecc481c2967..7899d6a158f3 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> +obj-$(CONFIG_ADM1177) += adm1177.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
>  obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o
> diff --git a/drivers/iio/adc/adm1177.c b/drivers/iio/adc/adm1177.c
> new file mode 100644
> index 000000000000..daec34566a65
> --- /dev/null
> +++ b/drivers/iio/adc/adm1177.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADM1177 Hot Swap Controller and Digital Power Monitor with Soft Start Pin
> + *
> + * Copyright 2015-2019 Analog Devices Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +
> +/*  Command Byte Operations */
> +#define ADM1177_CMD_V_CONT	BIT(0)
> +#define ADM1177_CMD_V_ONCE	BIT(1)
> +#define ADM1177_CMD_I_CONT	BIT(2)
> +#define ADM1177_CMD_I_ONCE	BIT(3)
> +#define ADM1177_CMD_VRANGE	BIT(4)
> +#define ADM1177_CMD_STATUS_RD	BIT(6)
> +
> +/* Extended Register */
> +#define ADM1177_REG_ALERT_EN	1
> +#define ADM1177_REG_ALERT_TH	2
> +#define ADM1177_REG_CONTROL	3
> +
> +/* ADM1177_REG_ALERT_EN */
> +#define ADM1177_EN_ADC_OC1	BIT(0)
> +#define ADM1177_EN_ADC_OC4	BIT(1)
> +#define ADM1177_EN_HS_ALERT	BIT(2)
> +#define ADM1177_EN_OFF_ALERT	BIT(3)
> +#define ADM1177_CLEAR		BIT(4)
> +
> +/* ADM1177_REG_CONTROL */
> +#define ADM1177_SWOFF		BIT(0)
> +
> +#define ADM1177_BITS		12
> +
> +/**
> + * struct adm1177_state - driver instance specific data
> + * @client		pointer to i2c client
> + * @reg			regulator info for the the power supply of the device
> + * @command		internal control register
> + * @r_sense_uohm	current sense resistor value
> + * @alert_threshold_ua	current limit for shutdown
> + * @vrange_high		internal voltage divider
> + */
> +struct adm1177_state {
> +	struct i2c_client	*client;
> +	struct regulator	*reg;
> +	u8			command;
> +	u32			r_sense_uohm;
> +	u32			alert_threshold_ua;
> +	bool			vrange_high;
> +};
> +
> +static int adm1177_read(struct adm1177_state *st, u8 num, u8 *data)
> +{
> +	struct i2c_client *client = st->client;
> +	int ret = 0;
> +
> +	ret = i2c_master_recv(client, data, num);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "I2C read error\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd)
> +{
> +	st->command = cmd;
> +	return i2c_smbus_write_byte(st->client, cmd);
> +}
> +
> +static int adm1177_write_reg(struct adm1177_state *st, u8 reg, u8 val)
> +{
> +	return i2c_smbus_write_byte_data(st->client, reg, val);

These wrappers don't really add anything.  I'd just make the i2c
calls directly inline. They will be self explanatory.

> +}
> +
> +static int adm1177_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long mask)
> +{
> +	struct adm1177_state *st = iio_priv(indio_dev);
> +	u8 data[3];
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		adm1177_read(st, 3, data);
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = (data[0] << 4) | (data[2] >> 4);
> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = (data[1] << 4) | (data[2] & 0xF);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->command & ADM1177_CMD_VRANGE)
> +				*val = 6650;
> +			else
> +				*val = 26350;
> +
> +			*val2 = ADM1177_BITS;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_CURRENT:
> +			*val = 105840 / st->r_sense_uohm;
> +
> +			*val2 = ADM1177_BITS;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec adm1177_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = 0,
> +	},
> +	{
> +		.type = IIO_CURRENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = 0,
> +	},
> +};
> +
> +static const struct iio_info adm1177_info = {
> +	.read_raw = &adm1177_read_raw,
> +};

blank line here.

> +static void adm1177_remove(void *data)
> +{
> +	struct adm1177_state *st = data;
> +
> +	if (st->reg)
> +		regulator_disable(st->reg);

Probe and remove should be mirror images of each other.
As you have a mixture of managed and non managed calls that
isn't the case here.

devm_add_action_or_reset will sort this out for you if
used to turn off the regulator at the right point on removal.

> +}
> +
> +static int adm1177_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct device_node *np;
> +	struct adm1177_state *st;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	st->client = client;
> +
> +	np = client->dev.of_node;
> +
> +	st->reg = devm_regulator_get_optional(&client->dev, "vref");
> +	if (IS_ERR(st->reg)) {
> +		if (PTR_ERR(st->reg) == EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		st->reg = NULL;
> +	} else {
> +		ret = regulator_enable(st->reg);
> +		if (ret)
> +			return ret;
> +		ret = devm_add_action_or_reset(&client->dev, adm1177_remove,
> +					       st);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	of_property_read_u32(np, "adi,r-sense-micro-ohms", &st->r_sense_uohm);
> +	of_property_read_u32(np, "adi,shutdown-threshold-microamp",
> +			     &st->alert_threshold_ua);
> +	st->vrange_high = of_property_read_bool(np,
> +				"adi,vrange-high-enable");
> +	if (st->alert_threshold_ua) {
> +		unsigned int val;
> +
> +		val = (st->alert_threshold_ua * st->r_sense_uohm * 0xFF);
> +		val /= 105840000U;
> +		if (val > 0xFF) {
> +			dev_err(&client->dev,
> +				"Requested shutdown current %d uA above limit\n",
> +				st->alert_threshold_ua);
> +
> +			val = 0xFF;
> +		}
> +		adm1177_write_reg(st, ADM1177_REG_ALERT_TH, val);
> +	}
> +
> +	adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
> +			      ADM1177_CMD_I_CONT |
> +			      (st->vrange_high ? 0 : ADM1177_CMD_VRANGE));
> +
> +	indio_dev->name = id->name;
> +	indio_dev->channels = adm1177_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adm1177_channels);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &adm1177_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id adm1177_ids[] = {
> +	{ "adm1177", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, adm1177_ids);
> +
> +static const struct of_device_id adm1177_dt_ids[] = {
> +	{ .compatible = "adi,adm1177" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, adm1177_dt_ids);
> +
> +static struct i2c_driver adm1177_driver = {
> +	.driver = {
> +		.name = "adm1177",
> +		.of_match_table = adm1177_dt_ids,
> +	},
> +	.probe = adm1177_probe,
> +	.id_table = adm1177_ids,
> +};
> +module_i2c_driver(adm1177_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Guenter Roeck]

On Tue, Nov 12, 2019 at 05:37:57PM +0000, Jonathan Cameron wrote:
> On Tue, 12 Nov 2019 17:35:50 +0200
> Beniamin Bia <beniamin.bia@analog.com> wrote:
> 
> > From: Michael Hennerich <michael.hennerich@analog.com>
> > 
> > ADM1177 is a Hot Swap Controller and Digital Power Monitor with
> > Soft Start Pin.
> > 
> > Datasheet:
> > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf
> > 
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > Co-developed-by: Beniamin Bia <beniamin.bia@analog.com>
> > Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> 
> Hi Beniamin,
> 
> A couple immediate thoughts.
> 
> 1. That cc list has some rather non obvious people on it.  Unless something
>    fairly surprising is going on, probably better to cut it back a bit.
> 
> 2. Why IIO?  Not entirely obvious to me.  From first glance this is definitely
>    doing hardware monitoring.  If there is a reason there should be a clear
>    statement here on why.
> 

I don't see why this is implemented as iio driver. I think it should be implemented
as hardware monitoring driver.

Some more comments below.

Guenter

> +CC Guenter and Jean as hwmon maintainers.
> 
> Whilst I'm here, a very quick review below.
> 
> > ---
> >  drivers/iio/adc/Kconfig   |  12 ++
> >  drivers/iio/adc/Makefile  |   1 +
> >  drivers/iio/adc/adm1177.c | 257 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 270 insertions(+)
> >  create mode 100644 drivers/iio/adc/adm1177.c
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 9554890a3200..4db8c6dcb595 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -228,6 +228,18 @@ config ASPEED_ADC
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called aspeed_adc.
> >  
> > +config ADM1177
> > +	tristate "Analog Devices ADM1177 Digital Power Monitor driver"
> > +	depends on I2C
> > +	help
> > +	  Say yes here to build support for Analog Devices:
> > +	  ADM1177 Hot Swap Controller and Digital Power Monitor
> > +	  with Soft Start Pin. Provides direct access
> > +	  via sysfs.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called adm1177.
> > +
> >  config AT91_ADC
> >  	tristate "Atmel AT91 ADC"
> >  	depends on ARCH_AT91
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 5ecc481c2967..7899d6a158f3 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
> >  obj-$(CONFIG_AD7949) += ad7949.o
> >  obj-$(CONFIG_AD799X) += ad799x.o
> >  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> > +obj-$(CONFIG_ADM1177) += adm1177.o
> >  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> >  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> >  obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o
> > diff --git a/drivers/iio/adc/adm1177.c b/drivers/iio/adc/adm1177.c
> > new file mode 100644
> > index 000000000000..daec34566a65
> > --- /dev/null
> > +++ b/drivers/iio/adc/adm1177.c
> > @@ -0,0 +1,257 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ADM1177 Hot Swap Controller and Digital Power Monitor with Soft Start Pin
> > + *
> > + * Copyright 2015-2019 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/sysfs.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +
> > +/*  Command Byte Operations */
> > +#define ADM1177_CMD_V_CONT	BIT(0)
> > +#define ADM1177_CMD_V_ONCE	BIT(1)
> > +#define ADM1177_CMD_I_CONT	BIT(2)
> > +#define ADM1177_CMD_I_ONCE	BIT(3)
> > +#define ADM1177_CMD_VRANGE	BIT(4)
> > +#define ADM1177_CMD_STATUS_RD	BIT(6)
> > +
> > +/* Extended Register */
> > +#define ADM1177_REG_ALERT_EN	1
> > +#define ADM1177_REG_ALERT_TH	2
> > +#define ADM1177_REG_CONTROL	3
> > +
> > +/* ADM1177_REG_ALERT_EN */
> > +#define ADM1177_EN_ADC_OC1	BIT(0)
> > +#define ADM1177_EN_ADC_OC4	BIT(1)
> > +#define ADM1177_EN_HS_ALERT	BIT(2)
> > +#define ADM1177_EN_OFF_ALERT	BIT(3)
> > +#define ADM1177_CLEAR		BIT(4)
> > +
> > +/* ADM1177_REG_CONTROL */
> > +#define ADM1177_SWOFF		BIT(0)
> > +
> > +#define ADM1177_BITS		12
> > +
> > +/**
> > + * struct adm1177_state - driver instance specific data
> > + * @client		pointer to i2c client
> > + * @reg			regulator info for the the power supply of the device
> > + * @command		internal control register
> > + * @r_sense_uohm	current sense resistor value
> > + * @alert_threshold_ua	current limit for shutdown
> > + * @vrange_high		internal voltage divider
> > + */
> > +struct adm1177_state {
> > +	struct i2c_client	*client;
> > +	struct regulator	*reg;
> > +	u8			command;
> > +	u32			r_sense_uohm;
> > +	u32			alert_threshold_ua;
> > +	bool			vrange_high;
> > +};
> > +
> > +static int adm1177_read(struct adm1177_state *st, u8 num, u8 *data)
> > +{
> > +	struct i2c_client *client = st->client;
> > +	int ret = 0;
> > +
> > +	ret = i2c_master_recv(client, data, num);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "I2C read error\n");

Seems a bit noisy (and inconsistent). It would make much more sense to
report the error to userspace instead of ignoring it and reporting a random
value (ie the stack content).

> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd)
> > +{
> > +	st->command = cmd;
> > +	return i2c_smbus_write_byte(st->client, cmd);
> > +}
> > +
> > +static int adm1177_write_reg(struct adm1177_state *st, u8 reg, u8 val)
> > +{
> > +	return i2c_smbus_write_byte_data(st->client, reg, val);
> 
> These wrappers don't really add anything.  I'd just make the i2c
> calls directly inline. They will be self explanatory.
> 

Plus, their return value is never checked.

> > +}
> > +
> > +static int adm1177_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val,
> > +			   int *val2,
> > +			   long mask)
> > +{
> > +	struct adm1177_state *st = iio_priv(indio_dev);
> > +	u8 data[3];
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		adm1177_read(st, 3, data);
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			*val = (data[0] << 4) | (data[2] >> 4);
> > +			return IIO_VAL_INT;
> > +		case IIO_CURRENT:
> > +			*val = (data[1] << 4) | (data[2] & 0xF);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			if (st->command & ADM1177_CMD_VRANGE)
> > +				*val = 6650;
> > +			else
> > +				*val = 26350;
> > +
> > +			*val2 = ADM1177_BITS;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> > +		case IIO_CURRENT:
> > +			*val = 105840 / st->r_sense_uohm;
> > +
> > +			*val2 = ADM1177_BITS;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_chan_spec adm1177_channels[] = {
> > +	{
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = 0,
> > +	},
> > +	{
> > +		.type = IIO_CURRENT,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = 0,
> > +	},
> > +};
> > +
> > +static const struct iio_info adm1177_info = {
> > +	.read_raw = &adm1177_read_raw,
> > +};
> 
> blank line here.
> 
> > +static void adm1177_remove(void *data)
> > +{
> > +	struct adm1177_state *st = data;
> > +
> > +	if (st->reg)
> > +		regulator_disable(st->reg);
> 
> Probe and remove should be mirror images of each other.
> As you have a mixture of managed and non managed calls that
> isn't the case here.
> 
> devm_add_action_or_reset will sort this out for you if
> used to turn off the regulator at the right point on removal.
> 

On top of that, this function is added with devm_add_action_or_reset(),
and it is only ever called if st->reg is not NULL.

> > +}
> > +
> > +static int adm1177_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct device_node *np;
> > +	struct adm1177_state *st;
> > +	struct iio_dev *indio_dev;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	st->client = client;
> > +
> > +	np = client->dev.of_node;
> > +
> > +	st->reg = devm_regulator_get_optional(&client->dev, "vref");
> > +	if (IS_ERR(st->reg)) {
> > +		if (PTR_ERR(st->reg) == EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		st->reg = NULL;
> > +	} else {
> > +		ret = regulator_enable(st->reg);
> > +		if (ret)
> > +			return ret;
> > +		ret = devm_add_action_or_reset(&client->dev, adm1177_remove,
> > +					       st);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	of_property_read_u32(np, "adi,r-sense-micro-ohms", &st->r_sense_uohm);

shunt-resistor-micro-ohms

> > +	of_property_read_u32(np, "adi,shutdown-threshold-microamp",
> > +			     &st->alert_threshold_ua);
> > +	st->vrange_high = of_property_read_bool(np,
> > +				"adi,vrange-high-enable");
> > +	if (st->alert_threshold_ua) {
> > +		unsigned int val;
> > +
> > +		val = (st->alert_threshold_ua * st->r_sense_uohm * 0xFF);
> > +		val /= 105840000U;
> > +		if (val > 0xFF) {
> > +			dev_err(&client->dev,
> > +				"Requested shutdown current %d uA above limit\n",
> > +				st->alert_threshold_ua);
> > +
> > +			val = 0xFF;
> > +		}
> > +		adm1177_write_reg(st, ADM1177_REG_ALERT_TH, val);
> > +	}
> > +
> > +	adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
> > +			      ADM1177_CMD_I_CONT |
> > +			      (st->vrange_high ? 0 : ADM1177_CMD_VRANGE));
> > +
> > +	indio_dev->name = id->name;
> > +	indio_dev->channels = adm1177_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(adm1177_channels);
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &adm1177_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct i2c_device_id adm1177_ids[] = {
> > +	{ "adm1177", 0 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, adm1177_ids);
> > +
> > +static const struct of_device_id adm1177_dt_ids[] = {
> > +	{ .compatible = "adi,adm1177" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, adm1177_dt_ids);
> > +
> > +static struct i2c_driver adm1177_driver = {
> > +	.driver = {
> > +		.name = "adm1177",
> > +		.of_match_table = adm1177_dt_ids,
> > +	},
> > +	.probe = adm1177_probe,
> > +	.id_table = adm1177_ids,
> > +};
> > +module_i2c_driver(adm1177_driver);
> > +
> > +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver");
> > +MODULE_LICENSE("GPL v2");
> 
> 

RE: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Hennerich, Michael]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Dienstag, 12. November 2019 20:18
> To: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Bia, Beniamin <Beniamin.Bia@analog.com>; jic23@kernel.org;
> lars@metafoo.de; Hennerich, Michael <Michael.Hennerich@analog.com>;
> pmeerw@pmeerw.net; gregkh@linuxfoundation.org; linux-
> iio@vger.kernel.org; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; mark.rutland@arm.com; robh+dt@kernel.org;
> devicetree@vger.kernel.org; paulmck@linux.ibm.com;
> mchehab+samsung@kernel.org; linus.walleij@linaro.org;
> nicolas.ferre@microchip.com; biabeniamin@outlook.com; Jean Delvare
> <jdelvare@suse.com>
> Subject: Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital
> Power Monitor driver
> 
> On Tue, Nov 12, 2019 at 05:37:57PM +0000, Jonathan Cameron wrote:
> > On Tue, 12 Nov 2019 17:35:50 +0200
> > Beniamin Bia <beniamin.bia@analog.com> wrote:
> >
> > > From: Michael Hennerich <michael.hennerich@analog.com>
> > >
> > > ADM1177 is a Hot Swap Controller and Digital Power Monitor with Soft
> > > Start Pin.
> > >
> > > Datasheet:
> > > Link:
> > > https://www.analog.com/media/en/technical-documentation/data-
> sheets/
> > > ADM1177.pdf
> > >
> > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > Co-developed-by: Beniamin Bia <beniamin.bia@analog.com>
> > > Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> >
> > Hi Beniamin,
> >
> > A couple immediate thoughts.
> >
> > 1. That cc list has some rather non obvious people on it.  Unless something
> >    fairly surprising is going on, probably better to cut it back a bit.
> >
> > 2. Why IIO?  Not entirely obvious to me.  From first glance this is definitely
> >    doing hardware monitoring.  If there is a reason there should be a clear
> >    statement here on why.
> >
> 
> I don't see why this is implemented as iio driver. I think it should be
> implemented as hardware monitoring driver.

Totally agree that this driver could have been implemented as HWMON driver.
Well we use this device as USB supply monitor on our embedded portably kits, to detect low VBUS or excess current draw. 

ADALM-PLUTO and ADALM2000:
https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/adalm-pluto.html

https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html

The only connectivity to the host PC is via IIO/libiio USB Gadget FS and Ethernet backends.

We recommend people to read the IIO attributes whenever they report an issue.
Unless libiio supports directly HWMON or HWMON adds an IIO bridge we would prefer this driver being exposed as IIO device, since HWMON users still can use the IIO/HWMON bridge.

-Michael

Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Guenter Roeck]

On Wed, Nov 13, 2019 at 08:10:50AM +0000, Hennerich, Michael wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Dienstag, 12. November 2019 20:18
> > To: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Bia, Beniamin <Beniamin.Bia@analog.com>; jic23@kernel.org;
> > lars@metafoo.de; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > pmeerw@pmeerw.net; gregkh@linuxfoundation.org; linux-
> > iio@vger.kernel.org; devel@driverdev.osuosl.org; linux-
> > kernel@vger.kernel.org; mark.rutland@arm.com; robh+dt@kernel.org;
> > devicetree@vger.kernel.org; paulmck@linux.ibm.com;
> > mchehab+samsung@kernel.org; linus.walleij@linaro.org;
> > nicolas.ferre@microchip.com; biabeniamin@outlook.com; Jean Delvare
> > <jdelvare@suse.com>
> > Subject: Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital
> > Power Monitor driver
> > 
> > On Tue, Nov 12, 2019 at 05:37:57PM +0000, Jonathan Cameron wrote:
> > > On Tue, 12 Nov 2019 17:35:50 +0200
> > > Beniamin Bia <beniamin.bia@analog.com> wrote:
> > >
> > > > From: Michael Hennerich <michael.hennerich@analog.com>
> > > >
> > > > ADM1177 is a Hot Swap Controller and Digital Power Monitor with Soft
> > > > Start Pin.
> > > >
> > > > Datasheet:
> > > > Link:
> > > > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/
> > > > ADM1177.pdf
> > > >
> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > > Co-developed-by: Beniamin Bia <beniamin.bia@analog.com>
> > > > Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> > >
> > > Hi Beniamin,
> > >
> > > A couple immediate thoughts.
> > >
> > > 1. That cc list has some rather non obvious people on it.  Unless something
> > >    fairly surprising is going on, probably better to cut it back a bit.
> > >
> > > 2. Why IIO?  Not entirely obvious to me.  From first glance this is definitely
> > >    doing hardware monitoring.  If there is a reason there should be a clear
> > >    statement here on why.
> > >
> > 
> > I don't see why this is implemented as iio driver. I think it should be
> > implemented as hardware monitoring driver.
> 
> Totally agree that this driver could have been implemented as HWMON driver.
> Well we use this device as USB supply monitor on our embedded portably kits, to detect low VBUS or excess current draw. 
> 
> ADALM-PLUTO and ADALM2000:
> https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/adalm-pluto.html
> 
> https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html
> 
> The only connectivity to the host PC is via IIO/libiio USB Gadget FS and Ethernet backends.
> 
> We recommend people to read the IIO attributes whenever they report an issue.
> Unless libiio supports directly HWMON or HWMON adds an IIO bridge we would prefer this driver being exposed as IIO device, since HWMON users still can use the IIO/HWMON bridge.
> 

I do not think this is a valid argument.

- This is a hardware monitoring chip.
- Implementing kernel support as IIO driver, keeping it out of tree for years,
  establishing an iio based use case, and then pressuring kernel maintainers
  to accept an iio driver seems inappropriate.
- The argument of "we need libiio support for this chip" could effectively
  be used to re-implement pretty much all hwmon drivers as iio drivers.
- The iio->hwmon bridge would add complexity for the majority of potential
  users of this chip. Focus should be on the majority, not on one use case.
- Userspace may as well use libsensors and/or sensors to do the necessary
  access - or implement it if neded. Or add a libsensors based backend to
  libiio (or to iiod).
- Last but not least, it would be more appropriate to implement a generic
  hwmon->iio bridge for iio use cases for chips supported by hwmon drivers,
  similar to the hwmon->thermal bridge.

Guenter

Re: [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177

[Rob Herring]

On Tue, Nov 12, 2019 at 05:35:51PM +0200, Beniamin Bia wrote:
> Documentation for ADM1177 was added.
> 
> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> ---
>  .../bindings/iio/adc/adi,adm1177.yaml         | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml b/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
> new file mode 100644
> index 000000000000..69a0230e59f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,adm1177.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,adm1177.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADM1177 Hot Swap Controller and Digital Power Monitor
> +
> +maintainers:
> +  - Michael Hennerich <michael.hennerich@analog.com>
> +  - Beniamin Bia <beniamin.bia@analog.com>
> +
> +description: |
> +  Analog Devices ADM1177 Hot Swap Controller and Digital Power Monitor
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ADM1177.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adm1177
> +
> +  reg:
> +    maxItems: 1
> +
> +  avcc-supply:
> +    description:
> +      Phandle to the Avcc power supply
> +
> +  adi,r-sense-micro-ohms:
> +    description:
> +      The value of curent sense resistor in microohms.

s/curent/current/

Is there a range of values allowed?

> +
> +  adi,shutdown-threshold-microamp:
> +    description:
> +      Specifies the current level at which an over current alert occurs.
> +    maximum: 255
> +
> +  adi,vrange-high-enable:
> +    description:
> +      Specifies which internal voltage divider to be used. A 1 selects
> +      a 7:2 voltage divider while a 0 selects a 14:1 voltage divider.
> +    type: boolean
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@b4 {
> +                compatible = "adi,adm1177";
> +                reg = <0xb4>;
> +        };
> +    };
> +...
> -- 
> 2.17.1
>