[PATCH 0/2] Hwmon driver for MAX31827 temperature switch

[PATCH 0/2] Hwmon driver for MAX31827 temperature switch

[Daniel Matyas]

The driver was tested on Raspberry Pi 4 Model B.

The implemented functionalities of it are: reading a 1shot input value
and changing and reading the overtemperature and undertemperature alarm
and hysteresis values. The read values are raw: signed 16 bit int
format. To obtain the real value one must multiply the value with 0.0625.

Daniel Matyas (2):
  dt-bindings: hwmon: add MAX31827
  hwmon: max31827: add MAX31827 driver

 .../bindings/hwmon/adi,max31827.yaml          |  48 ++++
 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 MAINTAINERS                                   |   8 +
 drivers/hwmon/Kconfig                         |  11 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/max31827.c                      | 240 ++++++++++++++++++
 6 files changed, 310 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
 create mode 100644 drivers/hwmon/max31827.c

-- 
2.34.1

[PATCH 1/2] dt-bindings: hwmon: add MAX31827

[Daniel Matyas]

MAX31827 is a low-power temperature switch with I2C interface.

The device is a ±1°C accuracy from -40°C to +125°C
(12 bits) local temperature switch and sensor with I2C/SM-
Bus interface. The combination of small 6-bump wafer-lev-
el package (WLP) and high accuracy makes this temper-
ature sensor/switch ideal for a wide range of applications.

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 .../bindings/hwmon/adi,max31827.yaml          | 48 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 +
 MAINTAINERS                                   |  7 +++
 3 files changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31827.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
new file mode 100644
index 000000000000..b6ed2d46a35d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/adi,max31827.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX31827, MAX31828, MAX31829 Low-Power Temperature Switch
+
+maintainers:
+  - Daniel Matyas <daniel.matyas@analog.com>
+
+description: |
+  Analog Devices MAX31827, MAX31828, MAX31829 Low-Power Temperature Switch with
+  I2C Interface
+  https://www.analog.com/media/en/technical-documentation/data-sheets/MAX31827-MAX31829.pdf
+properties:
+  compatible:
+    enum:
+      - adi,max31827
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      Must have values in the interval (1.6V; 3.6V) in order for the device to
+      function correctly.
+
+required:
+  - compatible
+  - reg
+  - vref-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        status = "okay";
+        max31827: max31827@42 {
+            compatible = "adi,max31827";
+            reg = <0x42>;
+            vref-supply = <&reg_vdd>;
+        };
+    };
+...
\ No newline at end of file
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 6f482a254a1d..7763610b97bc 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -43,6 +43,8 @@ properties:
           - adi,adp5589
             # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
           - adi,lt7182s
+            # MAX31827 Low-Power Temperature Switch with I2C interface
+          - adi,max31827
             # AMS iAQ-Core VOC Sensor
           - ams,iaq-core
             # i2c serial eeprom (24cxx)
diff --git a/MAINTAINERS b/MAINTAINERS
index 90abe83c02f3..549cea6bc340 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12535,6 +12535,13 @@ F:	Documentation/userspace-api/media/drivers/max2175.rst
 F:	drivers/media/i2c/max2175*
 F:	include/uapi/linux/max2175.h
 
+MAX31827 TEMPERATURE SWITCH DRIVER
+M:	Daniel Matyas <daniel.matyas@analog.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
+
 MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
 L:	linux-hwmon@vger.kernel.org
 S:	Orphan
-- 
2.34.1

[PATCH 2/2] hwmon: max31827: add MAX31827 driver

[Daniel Matyas]

MAX31827 is a low-power temperature switch with I2C interface.

The device is a ±1°C accuracy from -40°C to +125°C
(12 bits) local temperature switch and sensor with I2C/SM-
Bus interface. The combination of small 6-bump wafer-lev-
el package (WLP) and high accuracy makes this temper-
ature sensor/switch ideal for a wide range of applications.

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 MAINTAINERS              |   1 +
 drivers/hwmon/Kconfig    |  11 ++
 drivers/hwmon/Makefile   |   1 +
 drivers/hwmon/max31827.c | 240 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 253 insertions(+)
 create mode 100644 drivers/hwmon/max31827.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 549cea6bc340..63c17195a99b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12541,6 +12541,7 @@ L:	linux-hwmon@vger.kernel.org
 S:	Supported
 W:	http://ez.analog.com/community/linux-device-drivers
 F:	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
+F:	drivers/hwmon/max31827.c
 
 MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..80c44a787d42 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2401,4 +2401,15 @@ config SENSORS_ASUS_EC
 
 endif # ACPI
 
+config MAX31827
+	tristate "MAX31827 low-power temperature switch"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for MAX31827
+	  low-power temperature switch and sensor connected with I2C.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max31827.
+
 endif # HWMON
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 88712b5031c8..d00f0a1e73f6 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -224,3 +224,4 @@ obj-$(CONFIG_PMBUS)		+= pmbus/
 
 ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
 
+obj-$(CONFIG_MAX31827) += max31827.o
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
new file mode 100644
index 000000000000..1c79bcf12d78
--- /dev/null
+++ b/drivers/hwmon/max31827.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * max31827.c - Support for Maxim Low-Power Switch
+ *
+ * Copyright (c) 2023 Daniel Matyas <daniel.matyas@analog.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#define MAX31827_T_REG				0x0
+#define MAX31827_CONFIGURATION_REG              0x2
+#define MAX31827_TH_REG                         0x4
+#define MAX31827_TL_REG                         0x6
+#define MAX31827_TH_HYST_REG                    0x8
+#define MAX31827_TL_HYST_REG                    0xA
+
+#define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
+#define MAX31827_CONFIGURATION_CNV_RATE_MASK    GENMASK(3, 1)
+#define MAX31827_CONFIGURATION_RESOL_MASK       GENMASK(7, 6)
+#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
+#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
+
+#define MAX31827_CNV_SHUTDOWN			0x0
+#define MAX31827_CNV_1_DIV_64_HZ		0x1
+#define MAX31827_CNV_1_DIV_32_HZ		0x2
+#define MAX31827_CNV_1_DIV_16_HZ		0x3
+#define MAX31827_CNV_1_DIV_4_HZ			0x4
+#define MAX31827_CNV_1_HZ			0x5
+#define MAX31827_CNV_4_HZ			0x6
+#define MAX31827_CNV_8_HZ			0x7
+
+#define MAX31827_1SHOT_EN(x)			((x) ? BIT(0) : 0)
+
+struct max31827_state {
+	struct regmap *regmap;
+	struct i2c_client *client;
+};
+
+static const struct regmap_config max31827_regmap = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xA,
+};
+
+static umode_t max31827_is_visible(const void *state,
+				   enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	if (type == hwmon_temp) {
+		switch (attr) {
+		case hwmon_temp_enable:
+		case hwmon_temp_max:
+		case hwmon_temp_min:
+		case hwmon_temp_max_hyst:
+		case hwmon_temp_min_hyst:
+			return 0644;
+		case hwmon_temp_input:
+			return 0444;
+		}
+	}
+
+	return 0;
+}
+
+static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct max31827_state *st;
+	unsigned int uval;
+	int ret;
+
+	st = dev_get_drvdata(dev);
+	if (IS_ERR(st))
+		return PTR_ERR(st);
+
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_enable:
+		ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &uval);
+		uval  = FIELD_GET(MAX31827_CONFIGURATION_1SHOT_MASK, uval);
+		break;
+
+	case hwmon_temp_input:
+		ret = regmap_read(st->regmap, MAX31827_T_REG, &uval);
+		break;
+
+	case hwmon_temp_max:
+		ret = regmap_read(st->regmap, MAX31827_TH_REG, &uval);
+		break;
+
+	case hwmon_temp_max_hyst:
+		ret = regmap_read(st->regmap, MAX31827_TH_HYST_REG, &uval);
+		break;
+	case hwmon_temp_min:
+		ret = regmap_read(st->regmap, MAX31827_TL_REG, &uval);
+		break;
+
+	case hwmon_temp_min_hyst:
+		ret = regmap_read(st->regmap, MAX31827_TL_HYST_REG, &uval);
+		break;
+
+	default:
+		ret = -EOPNOTSUPP;
+	}
+
+	if (ret)
+		return ret;
+
+	*val = uval;
+
+	return 0;
+}
+
+static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long val)
+{
+	struct max31827_state *st = dev_get_drvdata(dev);
+
+	if (IS_ERR(st))
+		return PTR_ERR(st);
+
+	switch (attr) {
+	case hwmon_temp_enable:
+		if (val >> 1)
+			return -EOPNOTSUPP;
+
+		return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
+					  MAX31827_CONFIGURATION_1SHOT_MASK,
+					  MAX31827_1SHOT_EN(val));
+
+	case hwmon_temp_max:
+		return regmap_write(st->regmap, MAX31827_TH_REG, val);
+
+	case hwmon_temp_max_hyst:
+		return regmap_write(st->regmap, MAX31827_TH_HYST_REG, val);
+
+	case hwmon_temp_min:
+		return regmap_write(st->regmap, MAX31827_TL_REG, val);
+
+	case hwmon_temp_min_hyst:
+		return regmap_write(st->regmap, MAX31827_TL_HYST_REG, val);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int max31827_init_client(struct max31827_state *st)
+{
+	return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
+				 MAX31827_CONFIGURATION_CNV_RATE_MASK |
+				 MAX31827_CONFIGURATION_1SHOT_MASK,
+				 MAX31827_1SHOT_EN(1));
+}
+
+static const struct hwmon_channel_info *max31827_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT | HWMON_T_MIN |
+			   HWMON_T_MIN_HYST | HWMON_T_MAX | HWMON_T_MAX_HYST),
+	NULL,
+};
+
+static const struct hwmon_ops max31827_hwmon_ops = {
+	.is_visible = max31827_is_visible,
+	.read = max31827_read,
+	.write = max31827_write,
+};
+
+static const struct hwmon_chip_info max31827_chip_info = {
+	.ops = &max31827_hwmon_ops,
+	.info = max31827_info,
+};
+
+static int max31827_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct max31827_state *st;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return -EOPNOTSUPP;
+
+	st = devm_kzalloc(dev, sizeof(struct max31827_state), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	st->client = client;
+
+	st->regmap = devm_regmap_init_i2c(client, &max31827_regmap);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to allocate regmap: %ld\n",
+				     PTR_ERR(st->regmap));
+
+	ret = max31827_init_client(st);
+	if (ret)
+		return ret;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, st,
+							 &max31827_chip_info,
+							 NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id max31827_i2c_ids[] = {
+	{ .name = "max31827" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
+
+static const struct of_device_id max31827_of_match[] = {
+	{ .compatible = "max31827" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max31827_of_match);
+
+static struct i2c_driver max31827_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name = "max31827",
+		.of_match_table = max31827_of_match,
+	},
+	.probe_new = max31827_probe,
+	.id_table = max31827_i2c_ids,
+};
+module_i2c_driver(max31827_driver);
+
+MODULE_AUTHOR("Daniel Matyas <daniel.matyas@analog.com>");
+MODULE_DESCRIPTION("Maxim MAX31827 low-power temperature switch driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] hwmon: max31827: add MAX31827 driver

[Guenter Roeck]

On Thu, Apr 06, 2023 at 07:43:27PM +0300, Daniel Matyas wrote:
> MAX31827 is a low-power temperature switch with I2C interface.
> 
> The device is a ±1°C accuracy from -40°C to +125°C
> (12 bits) local temperature switch and sensor with I2C/SM-
> Bus interface. The combination of small 6-bump wafer-lev-
> el package (WLP) and high accuracy makes this temper-
> ature sensor/switch ideal for a wide range of applications.
> 
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/hwmon/Kconfig    |  11 ++
>  drivers/hwmon/Makefile   |   1 +
>  drivers/hwmon/max31827.c | 240 +++++++++++++++++++++++++++++++++++++++

Documentation/hwmon/max31727.rst missing.

>  4 files changed, 253 insertions(+)
>  create mode 100644 drivers/hwmon/max31827.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 549cea6bc340..63c17195a99b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12541,6 +12541,7 @@ L:	linux-hwmon@vger.kernel.org
>  S:	Supported
>  W:	http://ez.analog.com/community/linux-device-drivers
>  F:	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> +F:	drivers/hwmon/max31827.c
>  
>  MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
>  L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..80c44a787d42 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2401,4 +2401,15 @@ config SENSORS_ASUS_EC
>  
>  endif # ACPI
>  
> +config MAX31827
> +	tristate "MAX31827 low-power temperature switch"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for MAX31827
> +	  low-power temperature switch and sensor connected with I2C.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max31827.
> +
>  endif # HWMON
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..d00f0a1e73f6 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -224,3 +224,4 @@ obj-$(CONFIG_PMBUS)		+= pmbus/
>  
>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>  
> +obj-$(CONFIG_MAX31827) += max31827.o
> diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> new file mode 100644
> index 000000000000..1c79bcf12d78
> --- /dev/null
> +++ b/drivers/hwmon/max31827.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * max31827.c - Support for Maxim Low-Power Switch
> + *
> + * Copyright (c) 2023 Daniel Matyas <daniel.matyas@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Not needed.

> +
> +#define MAX31827_T_REG				0x0
> +#define MAX31827_CONFIGURATION_REG              0x2
> +#define MAX31827_TH_REG                         0x4
> +#define MAX31827_TL_REG                         0x6
> +#define MAX31827_TH_HYST_REG                    0x8
> +#define MAX31827_TL_HYST_REG                    0xA
> +
> +#define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
> +#define MAX31827_CONFIGURATION_CNV_RATE_MASK    GENMASK(3, 1)
> +#define MAX31827_CONFIGURATION_RESOL_MASK       GENMASK(7, 6)
> +#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
> +#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
> +
> +#define MAX31827_CNV_SHUTDOWN			0x0
> +#define MAX31827_CNV_1_DIV_64_HZ		0x1
> +#define MAX31827_CNV_1_DIV_32_HZ		0x2
> +#define MAX31827_CNV_1_DIV_16_HZ		0x3
> +#define MAX31827_CNV_1_DIV_4_HZ			0x4
> +#define MAX31827_CNV_1_HZ			0x5
> +#define MAX31827_CNV_4_HZ			0x6
> +#define MAX31827_CNV_8_HZ			0x7
> +

Most of the above defines are not used.

> +#define MAX31827_1SHOT_EN(x)			((x) ? BIT(0) : 0)

x is either 0 or 1. This macro does not add any value.

> +
> +struct max31827_state {
> +	struct regmap *regmap;
> +	struct i2c_client *client;

Not used anywhere.

> +};
> +
> +static const struct regmap_config max31827_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xA,
> +};
> +
> +static umode_t max31827_is_visible(const void *state,
> +				   enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	if (type == hwmon_temp) {
> +		switch (attr) {
> +		case hwmon_temp_enable:
> +		case hwmon_temp_max:
> +		case hwmon_temp_min:
> +		case hwmon_temp_max_hyst:
> +		case hwmon_temp_min_hyst:
> +			return 0644;
> +		case hwmon_temp_input:
> +			return 0444;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct max31827_state *st;
> +	unsigned int uval;
> +	int ret;
> +
> +	st = dev_get_drvdata(dev);

Please be consistent: Either assign in the declaration, or later,
but don't mix the two.

> +	if (IS_ERR(st))
> +		return PTR_ERR(st);

Unnecessary check.

> +
> +	if (type != hwmon_temp)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_enable:
> +		ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &uval);
> +		uval  = FIELD_GET(MAX31827_CONFIGURATION_1SHOT_MASK, uval);

This is an ABI abuse. hwmon_temp_enable is expected to enable / disable the sensor
(here: set conversion rate to 0), not to set 1-shot mode or to trigger a single
conversion.

If your application mandates 1-shot mode, do it properly. Implement continuous mode
by default, have the _enable attribute disable the sensor as per ABI, and, if disabled,
trigger one-shot mode when reading the temperature and let it wait until a temperature
measurement is available. I'd strongly suggest to skip that and use a conversion rate
of 1 / 64 seconds instead to keep the code simple.

You might also want to consider implementing the update_interval attribute
to let the user configure the conversion rate.

> +		break;
> +
> +	case hwmon_temp_input:
> +		ret = regmap_read(st->regmap, MAX31827_T_REG, &uval);
> +		break;
> +
> +	case hwmon_temp_max:
> +		ret = regmap_read(st->regmap, MAX31827_TH_REG, &uval);
> +		break;
> +
> +	case hwmon_temp_max_hyst:
> +		ret = regmap_read(st->regmap, MAX31827_TH_HYST_REG, &uval);
> +		break;
> +	case hwmon_temp_min:
> +		ret = regmap_read(st->regmap, MAX31827_TL_REG, &uval);
> +		break;
> +
> +	case hwmon_temp_min_hyst:
> +		ret = regmap_read(st->regmap, MAX31827_TL_HYST_REG, &uval);
> +		break;
> +

Why no alarm attribute suppport ?

> +	default:
> +		ret = -EOPNOTSUPP;
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	*val = uval;
> +
> +	return 0;
> +}
> +
> +static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long val)
> +{
> +	struct max31827_state *st = dev_get_drvdata(dev);
> +
> +	if (IS_ERR(st))
> +		return PTR_ERR(st);

Unnecessary check.

> +
> +	switch (attr) {
> +	case hwmon_temp_enable:
> +		if (val >> 1)
> +			return -EOPNOTSUPP;
> +
> +		return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> +					  MAX31827_CONFIGURATION_1SHOT_MASK,
> +					  MAX31827_1SHOT_EN(val));
> +
> +	case hwmon_temp_max:
> +		return regmap_write(st->regmap, MAX31827_TH_REG, val);
> +
> +	case hwmon_temp_max_hyst:
> +		return regmap_write(st->regmap, MAX31827_TH_HYST_REG, val);

Datasheet:

"
Before the register values are changed over I2C, the part has to be placed in Shutdown mode. Refer to the Configuration/
Status Register Conversion Rate field for details. Operation in automatic mode can resume after the register update.
"

Yes, I understand, the driver currently always operates in shutdown/1-shot
mode, but as mentioned above that is unacceptable.

> +
> +	case hwmon_temp_min:
> +		return regmap_write(st->regmap, MAX31827_TL_REG, val);
> +
> +	case hwmon_temp_min_hyst:
> +		return regmap_write(st->regmap, MAX31827_TL_HYST_REG, val);
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int max31827_init_client(struct max31827_state *st)
> +{
> +	return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> +				 MAX31827_CONFIGURATION_CNV_RATE_MASK |
> +				 MAX31827_CONFIGURATION_1SHOT_MASK,
> +				 MAX31827_1SHOT_EN(1));

More ABI abuse. This configures the driver (hard) for 1-shot mode,
and assumes that a conversion is triggered by writing into the _enable
attribute. Sorry, this is unacceptable.

> +}
> +
> +static const struct hwmon_channel_info *max31827_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_ENABLE | HWMON_T_INPUT | HWMON_T_MIN |
> +			   HWMON_T_MIN_HYST | HWMON_T_MAX | HWMON_T_MAX_HYST),
> +	NULL,
> +};
> +
> +static const struct hwmon_ops max31827_hwmon_ops = {
> +	.is_visible = max31827_is_visible,
> +	.read = max31827_read,
> +	.write = max31827_write,
> +};
> +
> +static const struct hwmon_chip_info max31827_chip_info = {
> +	.ops = &max31827_hwmon_ops,
> +	.info = max31827_info,
> +};
> +
> +static int max31827_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct max31827_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EOPNOTSUPP;
> +
> +	st = devm_kzalloc(dev, sizeof(struct max31827_state), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	st->client = client;
> +
> +	st->regmap = devm_regmap_init_i2c(client, &max31827_regmap);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to allocate regmap: %ld\n",
> +				     PTR_ERR(st->regmap));

dev_err_probe() already displays an error text.

> +
> +	ret = max31827_init_client(st);
> +	if (ret)
> +		return ret;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, st,
> +							 &max31827_chip_info,
> +							 NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id max31827_i2c_ids[] = {
> +	{ .name = "max31827" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> +
> +static const struct of_device_id max31827_of_match[] = {
> +	{ .compatible = "max31827" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max31827_of_match);
> +
> +static struct i2c_driver max31827_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "max31827",
> +		.of_match_table = max31827_of_match,
> +	},
> +	.probe_new = max31827_probe,
> +	.id_table = max31827_i2c_ids,
> +};
> +module_i2c_driver(max31827_driver);
> +
> +MODULE_AUTHOR("Daniel Matyas <daniel.matyas@analog.com>");
> +MODULE_DESCRIPTION("Maxim MAX31827 low-power temperature switch driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] hwmon: max31827: add MAX31827 driver

[Krzysztof Kozlowski]

On 06/04/2023 18:43, Daniel Matyas wrote:
> MAX31827 is a low-power temperature switch with I2C interface.
> 

(...)

> +
> +static const struct hwmon_ops max31827_hwmon_ops = {
> +	.is_visible = max31827_is_visible,
> +	.read = max31827_read,
> +	.write = max31827_write,
> +};
> +
> +static const struct hwmon_chip_info max31827_chip_info = {
> +	.ops = &max31827_hwmon_ops,
> +	.info = max31827_info,
> +};
> +
> +static int max31827_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct max31827_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EOPNOTSUPP;
> +
> +	st = devm_kzalloc(dev, sizeof(struct max31827_state), GFP_KERNEL);

sizeof(*...)

> +	if (!st)
> +		return -ENOMEM;
> +


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: hwmon: add MAX31827

[Rob Herring]

On Thu, 06 Apr 2023 19:43:26 +0300, Daniel Matyas wrote:
> MAX31827 is a low-power temperature switch with I2C interface.
> 
> The device is a ±1°C accuracy from -40°C to +125°C
> (12 bits) local temperature switch and sensor with I2C/SM-
> Bus interface. The combination of small 6-bump wafer-lev-
> el package (WLP) and high accuracy makes this temper-
> ature sensor/switch ideal for a wide range of applications.
> 
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
>  .../bindings/hwmon/adi,max31827.yaml          | 48 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 +
>  MAINTAINERS                                   |  7 +++
>  3 files changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/hwmon/adi,max31827.yaml:48:4: [error] no new line character at the end of file (new-line-at-end-of-file)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/adi,max31827.example.dtb: max31827@42: 'vref-supply' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/trivial-devices.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230406164331.6557-2-daniel.matyas@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: hwmon: add MAX31827

[Krzysztof Kozlowski]

On 06/04/2023 18:43, Daniel Matyas wrote:
> MAX31827 is a low-power temperature switch with I2C interface.
> 
> The device is a ±1°C accuracy from -40°C to +125°C
> (12 bits) local temperature switch and sensor with I2C/SM-
> Bus interface. The combination of small 6-bump wafer-lev-
> el package (WLP) and high accuracy makes this temper-
> ature sensor/switch ideal for a wide range of applications.
> 
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
>  .../bindings/hwmon/adi,max31827.yaml          | 48 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 +
>  MAINTAINERS                                   |  7 +++

>  3 files changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> new file mode 100644
> index 000000000000..b6ed2d46a35d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/adi,max31827.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX31827, MAX31828, MAX31829 Low-Power Temperature Switch

Where are the compatibles? I expect them in such case to be compatible
with this 31827.

> +
> +maintainers:
> +  - Daniel Matyas <daniel.matyas@analog.com>
> +
> +description: |
> +  Analog Devices MAX31827, MAX31828, MAX31829 Low-Power Temperature Switch with
> +  I2C Interface
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/MAX31827-MAX31829.pdf

Missing blank line

> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31827
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      Must have values in the interval (1.6V; 3.6V) in order for the device to
> +      function correctly.
> +
> +required:
> +  - compatible
> +  - reg
> +  - vref-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        status = "okay";

Drop status

> +        max31827: max31827@42 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Drop also label. You do not use it.


> +            compatible = "adi,max31827";
> +            reg = <0x42>;
> +            vref-supply = <&reg_vdd>;
> +        };
> +    };
> +...
> \ No newline at end of file

Patch error to fix.

> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 6f482a254a1d..7763610b97bc 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -43,6 +43,8 @@ properties:
>            - adi,adp5589
>              # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
>            - adi,lt7182s
> +            # MAX31827 Low-Power Temperature Switch with I2C interface
> +          - adi,max31827

No need for this.

Best regards,
Krzysztof