All the mail mirrored from lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.