linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver
@ 2019-12-03 13:57 Beniamin Bia
  2019-12-03 13:57 ` [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177 Beniamin Bia
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Beniamin Bia @ 2019-12-03 13:57 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Michael.Hennerich, linux-kernel, jdelvare, linux, mark.rutland,
	lgirdwood, broonie, devicetree, biabeniamin, Beniamin Bia

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: Beniamin Bia <beniamin.bia@analog.com>
---
 drivers/hwmon/Kconfig   |  10 ++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/adm1177.c | 274 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/hwmon/adm1177.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5308c59d7001..3db8f5752675 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -164,6 +164,16 @@ config SENSORS_ADM1031
 	  This driver can also be built as a module. If so, the module
 	  will be called adm1031.
 
+config SENSORS_ADM1177
+	tristate "Analog Devices ADM1177 and compatibles"
+	depends on I2C
+	help
+	  If you say yes here you get support for Analog Devices ADM1177
+	  sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called adm1177.
+
 config SENSORS_ADM9240
 	tristate "Analog Devices ADM9240 and compatibles"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 40c036ea45e6..27d04eab1be4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ADM1177)	+= adm1177.o
 obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
 obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
new file mode 100644
index 000000000000..08950cecc9f9
--- /dev/null
+++ b/drivers/hwmon/adm1177.c
@@ -0,0 +1,274 @@
+// 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/module.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/regulator/consumer.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_raw(struct adm1177_state *st, u8 num, u8 *data)
+{
+	struct i2c_client *client = st->client;
+
+	return i2c_master_recv(client, data, num);
+}
+
+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_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct adm1177_state *st = dev_get_drvdata(dev);
+	u8 data[3];
+	long dummy;
+	int ret;
+
+	switch (type) {
+	case hwmon_curr:
+		ret = adm1177_read_raw(st, 3, data);
+		if (ret < 0)
+			return ret;
+		dummy = (data[1] << 4) | (data[2] & 0xF);
+		/*
+		 * convert in milliamperes
+		 * ((105.84mV / 4096) x raw) / senseResistor(ohm)
+		 */
+		*val = div_u64((105840000ull * dummy), 4096 * st->r_sense_uohm);
+		return 0;
+	case hwmon_in:
+		ret = adm1177_read_raw(st, 3, data);
+		if (ret < 0)
+			return ret;
+		dummy = (data[0] << 4) | (data[2] >> 4);
+		/*
+		 * convert in millivolts based on resistor devision
+		 * (V_fullscale / 4096) * raw
+		 */
+		if (st->command & ADM1177_CMD_VRANGE)
+			*val = 6650;
+		else
+			*val = 26350;
+
+		*val = ((*val * dummy) / 4096);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t adm1177_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	const struct adm1177_state *st = data;
+
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			if (st->r_sense_uohm)
+				return 0444;
+			return 0;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const u32 adm1177_curr_config[] = {
+	HWMON_C_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info adm1177_curr = {
+	.type = hwmon_curr,
+	.config = adm1177_curr_config,
+};
+
+static const u32 adm1177_in_config[] = {
+	HWMON_I_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info adm1177_in = {
+	.type = hwmon_in,
+	.config = adm1177_in_config,
+};
+
+static const struct hwmon_channel_info *adm1177_info[] = {
+	&adm1177_curr,
+	&adm1177_in,
+	NULL
+};
+
+static const struct hwmon_ops adm1177_hwmon_ops = {
+	.is_visible = adm1177_is_visible,
+	.read = adm1177_read,
+};
+
+static const struct hwmon_chip_info adm1177_chip_info = {
+	.ops = &adm1177_hwmon_ops,
+	.info = adm1177_info,
+};
+
+static void adm1177_remove(void *data)
+{
+	struct adm1177_state *st = data;
+
+	regulator_disable(st->reg);
+}
+
+static int adm1177_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct adm1177_state *st;
+	int ret;
+
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	st->client = client;
+
+	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;
+	}
+
+	if (device_property_read_u32(dev, "adi,r-sense-micro-ohms",
+				     &st->r_sense_uohm))
+		st->r_sense_uohm = 1;
+	if (device_property_read_u32(dev, "adi,shutdown-threshold-microamp",
+				     &st->alert_threshold_ua))
+		st->alert_threshold_ua = 0;
+	st->vrange_high = device_property_read_bool(dev,
+						    "adi,vrange-high-enable");
+	if (st->alert_threshold_ua) {
+		u64 val;
+
+		val = (0xFFUL * st->alert_threshold_ua * st->r_sense_uohm);
+		val = div_u64(val, 105840000U);
+		if (val > 0xFF) {
+			dev_warn(&client->dev,
+				 "Requested shutdown current %d uA above limit\n",
+				 st->alert_threshold_ua);
+
+			val = 0xFF;
+		}
+		i2c_smbus_write_byte_data(st->client, ADM1177_REG_ALERT_TH,
+					  val);
+	}
+
+	ret = adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
+				    ADM1177_CMD_I_CONT |
+				    (st->vrange_high ? 0 : ADM1177_CMD_VRANGE));
+	if (ret)
+		return ret;
+
+	hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, client->name, st,
+						     &adm1177_chip_info, NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id adm1177_id[] = {
+	{"adm1177", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, adm1177_id);
+
+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 = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name = "adm1177",
+		.of_match_table = adm1177_dt_ids,
+	},
+	.probe = adm1177_probe,
+	.id_table = adm1177_id,
+};
+module_i2c_driver(adm1177_driver);
+
+MODULE_AUTHOR("Beniamin Bia <beniamin.bia@analog.com>");
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177
  2019-12-03 13:57 [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Beniamin Bia
@ 2019-12-03 13:57 ` Beniamin Bia
  2019-12-03 15:06   ` Guenter Roeck
  2019-12-03 13:57 ` [PATCH 3/3] MAINTAINERS: add entry for ADM1177 driver Beniamin Bia
  2019-12-03 19:28 ` [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Guenter Roeck
  2 siblings, 1 reply; 5+ messages in thread
From: Beniamin Bia @ 2019-12-03 13:57 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Michael.Hennerich, linux-kernel, jdelvare, linux, mark.rutland,
	lgirdwood, broonie, devicetree, biabeniamin, Beniamin Bia

Documentation for ADM1177 was added.

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

diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
new file mode 100644
index 000000000000..abd9354217ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/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.
+
+  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
+  - adi,r-sense-micro-ohms
+  - adi,shutdown-threshold-microamp
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pwmon@5a {
+                compatible = "adi,adm1177";
+                reg = <0x5a>;
+                adi,r-sense-micro-ohms = <50000>; /* 50 mOhm */
+                adi,shutdown-threshold-microamp = <1059000>; /* 1.059 A */
+                adi,vrange-high-enable;
+        };
+    };
+...
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/3] MAINTAINERS: add entry for ADM1177 driver
  2019-12-03 13:57 [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Beniamin Bia
  2019-12-03 13:57 ` [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177 Beniamin Bia
@ 2019-12-03 13:57 ` Beniamin Bia
  2019-12-03 19:28 ` [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Guenter Roeck
  2 siblings, 0 replies; 5+ messages in thread
From: Beniamin Bia @ 2019-12-03 13:57 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Michael.Hennerich, linux-kernel, jdelvare, linux, mark.rutland,
	lgirdwood, broonie, devicetree, biabeniamin, 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 3ef731fc753b..bc19b624fcd5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -978,6 +978,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-hwmon@vger.kernel.org
+W:	http://ez.analog.com/community/linux-device-drivers
+S:	Supported
+F:	drivers/hwmon/adm1177.c
+F:	Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
+
 ANALOG DEVICES INC ADP5061 DRIVER
 M:	Stefan Popa <stefan.popa@analog.com>
 L:	linux-pm@vger.kernel.org
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177
  2019-12-03 13:57 ` [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177 Beniamin Bia
@ 2019-12-03 15:06   ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-12-03 15:06 UTC (permalink / raw)
  To: Beniamin Bia, linux-hwmon
  Cc: Michael.Hennerich, linux-kernel, jdelvare, mark.rutland,
	lgirdwood, broonie, devicetree, biabeniamin

On 12/3/19 5:57 AM, Beniamin Bia wrote:
> Documentation for ADM1177 was added.
> 

Subject still points to iio.

> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> ---
>   .../bindings/hwmon/adi,adm1177.yaml           | 65 +++++++++++++++++++
>   1 file changed, 65 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml b/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
> new file mode 100644
> index 000000000000..abd9354217ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/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.

There is a standard property for that (shunt-resistor-micro-ohms).

> +
> +  adi,shutdown-threshold-microamp:
> +    description:
> +      Specifies the current level at which an over current alert occurs.
> +
> +  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
> +  - adi,r-sense-micro-ohms
> +  - adi,shutdown-threshold-microamp
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pwmon@5a {
> +                compatible = "adi,adm1177";
> +                reg = <0x5a>;
> +                adi,r-sense-micro-ohms = <50000>; /* 50 mOhm */
> +                adi,shutdown-threshold-microamp = <1059000>; /* 1.059 A */
> +                adi,vrange-high-enable;
> +        };
> +    };
> +...
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver
  2019-12-03 13:57 [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Beniamin Bia
  2019-12-03 13:57 ` [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177 Beniamin Bia
  2019-12-03 13:57 ` [PATCH 3/3] MAINTAINERS: add entry for ADM1177 driver Beniamin Bia
@ 2019-12-03 19:28 ` Guenter Roeck
  2 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-12-03 19:28 UTC (permalink / raw)
  To: Beniamin Bia
  Cc: linux-hwmon, Michael.Hennerich, linux-kernel, jdelvare,
	mark.rutland, lgirdwood, broonie, devicetree, biabeniamin

On Tue, Dec 03, 2019 at 03:57:09PM +0200, Beniamin Bia wrote:
> 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
> 

High level question:

The chip supports setting the current limit, and it reports
the overcurrent status. Any reason for not providing the respective
sysfs entries ?

> Signed-off-by: Beniamin Bia <beniamin.bia@analog.com>
> ---
>  drivers/hwmon/Kconfig   |  10 ++
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/adm1177.c | 274 ++++++++++++++++++++++++++++++++++++++++

Please also provide Documentation/hwmon/adm1177.rst.

>  3 files changed, 285 insertions(+)
>  create mode 100644 drivers/hwmon/adm1177.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5308c59d7001..3db8f5752675 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -164,6 +164,16 @@ config SENSORS_ADM1031
>  	  This driver can also be built as a module. If so, the module
>  	  will be called adm1031.
>  
> +config SENSORS_ADM1177
> +	tristate "Analog Devices ADM1177 and compatibles"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Analog Devices ADM1177
> +	  sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called adm1177.
> +
>  config SENSORS_ADM9240
>  	tristate "Analog Devices ADM9240 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 40c036ea45e6..27d04eab1be4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
>  obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
>  obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
>  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
> +obj-$(CONFIG_SENSORS_ADM1177)	+= adm1177.o
>  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
>  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
>  obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> new file mode 100644
> index 000000000000..08950cecc9f9
> --- /dev/null
> +++ b/drivers/hwmon/adm1177.c
> @@ -0,0 +1,274 @@
> +// 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/module.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/regulator/consumer.h>

Alphabetic include file order, please.

> +
> +/*  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)
> +

Requires #include <linux/bits.h>. ADM1177_CMD_STATUS_RD is not used.

> +/* Extended Register */
> +#define ADM1177_REG_ALERT_EN	1

ADM1177_REG_ALERT_EN is not used.

> +#define ADM1177_REG_ALERT_TH	2
> +#define ADM1177_REG_CONTROL	3

ADM1177_REG_CONTROL is not used.

> +
> +/* 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

The above defines are not used.

> +
> +/**
> + * 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_raw(struct adm1177_state *st, u8 num, u8 *data)
> +{
> +	struct i2c_client *client = st->client;
> +

That variable seems unncessary. Just use st->client directly, like below.

> +	return i2c_master_recv(client, data, num);
> +}
> +
> +static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd)
> +{
> +	st->command = cmd;

This is an odd way to record and check if vrange is high or low,
especially since there is already a flag indicating that/if it is high.

Why not just use "if (st->vrange_high) instead of "if (st->command &
ADM1177_CMD_VRANGE") ?

> +	return i2c_smbus_write_byte(st->client, cmd);
> +}
> +
> +static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct adm1177_state *st = dev_get_drvdata(dev);
> +	u8 data[3];
> +	long dummy;
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_curr:
> +		ret = adm1177_read_raw(st, 3, data);
> +		if (ret < 0)
> +			return ret;
> +		dummy = (data[1] << 4) | (data[2] & 0xF);
> +		/*
> +		 * convert in milliamperes

convert to

> +		 * ((105.84mV / 4096) x raw) / senseResistor(ohm)
> +		 */
> +		*val = div_u64((105840000ull * dummy), 4096 * st->r_sense_uohm);
> +		return 0;
> +	case hwmon_in:
> +		ret = adm1177_read_raw(st, 3, data);
> +		if (ret < 0)
> +			return ret;
> +		dummy = (data[0] << 4) | (data[2] >> 4);
> +		/*
> +		 * convert in millivolts based on resistor devision
> +		 * (V_fullscale / 4096) * raw
> +		 */
> +		if (st->command & ADM1177_CMD_VRANGE)
> +			*val = 6650;
> +		else
> +			*val = 26350;
> +
Dereferencing val several times isn't really very efficient.
How about the following ?

		if (st->vrange_high)
			dummy *= 26350;
		else
			dummy *= 6650;
		*val = DIV_ROUND_CLOSEST(dummy, 4096);

> +		*val = ((*val * dummy) / 4096);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t adm1177_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	const struct adm1177_state *st = data;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			if (st->r_sense_uohm)
> +				return 0444;
> +			return 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const u32 adm1177_curr_config[] = {
> +	HWMON_C_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info adm1177_curr = {
> +	.type = hwmon_curr,
> +	.config = adm1177_curr_config,
> +};
> +
> +static const u32 adm1177_in_config[] = {
> +	HWMON_I_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info adm1177_in = {
> +	.type = hwmon_in,
> +	.config = adm1177_in_config,
> +};
> +
> +static const struct hwmon_channel_info *adm1177_info[] = {
> +	&adm1177_curr,
> +	&adm1177_in,
> +	NULL
> +};

Please use the HWMON_CHANNEL_INFO  macro to declare the above.

> +
> +static const struct hwmon_ops adm1177_hwmon_ops = {
> +	.is_visible = adm1177_is_visible,
> +	.read = adm1177_read,
> +};
> +
> +static const struct hwmon_chip_info adm1177_chip_info = {
> +	.ops = &adm1177_hwmon_ops,
> +	.info = adm1177_info,
> +};
> +
> +static void adm1177_remove(void *data)
> +{
> +	struct adm1177_state *st = data;
> +
> +	regulator_disable(st->reg);
> +}
> +
> +static int adm1177_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct adm1177_state *st;
> +	int ret;
> +
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	st->client = client;
> +
> +	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;
> +	}
> +
> +	if (device_property_read_u32(dev, "adi,r-sense-micro-ohms",
> +				     &st->r_sense_uohm))
> +		st->r_sense_uohm = 1;

Does the default make sense ? A default of 1 mOhm seems to make more sense.

I assume that you explicitly want to be able to disable current sensing
by setting r_sense_uohm to 0. If so, that needs to be documented in the
devicetree file. Also, as written, the property is optional. I think this
also needs to be documented.

> +	if (device_property_read_u32(dev, "adi,shutdown-threshold-microamp",
> +				     &st->alert_threshold_ua))
> +		st->alert_threshold_ua = 0;
> +	st->vrange_high = device_property_read_bool(dev,
> +						    "adi,vrange-high-enable");
> +	if (st->alert_threshold_ua) {
> +		u64 val;
> +
> +		val = (0xFFUL * st->alert_threshold_ua * st->r_sense_uohm);

Unnecessary ( )

The above sets val to 0 if st->r_sense_uohm is 0. That means the register
value is set to 0. Does that mean "disable" for the chip ?

> +		val = div_u64(val, 105840000U);
> +		if (val > 0xFF) {
> +			dev_warn(&client->dev,
> +				 "Requested shutdown current %d uA above limit\n",
> +				 st->alert_threshold_ua);
> +
> +			val = 0xFF;
> +		}
> +		i2c_smbus_write_byte_data(st->client, ADM1177_REG_ALERT_TH,
> +					  val);
> +	}

Does the chip have a default value ?

> +
> +	ret = adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
> +				    ADM1177_CMD_I_CONT |
> +				    (st->vrange_high ? 0 : ADM1177_CMD_VRANGE));
> +	if (ret)
> +		return ret;
> +
> +	hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, client->name, st,
> +						     &adm1177_chip_info, NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id adm1177_id[] = {
> +	{"adm1177", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, adm1177_id);
> +
> +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 = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "adm1177",
> +		.of_match_table = adm1177_dt_ids,
> +	},
> +	.probe = adm1177_probe,
> +	.id_table = adm1177_id,
> +};
> +module_i2c_driver(adm1177_driver);
> +
> +MODULE_AUTHOR("Beniamin Bia <beniamin.bia@analog.com>");
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices ADM1177 ADC driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-12-03 19:29 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 13:57 [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Beniamin Bia
2019-12-03 13:57 ` [PATCH 2/3] dt-binding: iio: Add documentation for ADM1177 Beniamin Bia
2019-12-03 15:06   ` Guenter Roeck
2019-12-03 13:57 ` [PATCH 3/3] MAINTAINERS: add entry for ADM1177 driver Beniamin Bia
2019-12-03 19:28 ` [PATCH 1/3] hwmon: adm1177: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).