[PATCH v2 0/7] hwmon: (max1619) Modernize driver

[PATCH v2 0/7] hwmon: (max1619) Modernize driver

[Guenter Roeck]

Fix limit overflows, improve chip detection code, convert to use
regmap, convert to use with_info hwmon API, and add support for
update_interval sysfs attribute.

v2: Initialize chip as in original code
    Separate 'alarms' attribute masking into its own patch
    Added Reviewed-by: tags

----------------------------------------------------------------
Guenter Roeck (7):
      hwmon: (max1619) Clamp temperature range when writing limits
      hwmon: (max1619) Reorder include files to alphabetic order
      hwmon: (max1619) Mask valid alarm bits
      hwmon: (max1619) Convert to use regmap
      hwmon: (max1619) Convert to with_info API
      hwmon: (max1619) Add support for update_interval attribute
      hwmon: (max1619) Improve chip detection code

 Documentation/hwmon/max1619.rst |   4 -
 drivers/hwmon/max1619.c         | 495 +++++++++++++++++++++++-----------------
 2 files changed, 288 insertions(+), 211 deletions(-)

[PATCH v2 1/7] hwmon: (max1619) Clamp temperature range when writing limits

[Guenter Roeck]

Module test code reports underflows when writing sensor limits.

temp2_min: Suspected underflow: [min=-77000, read 101000, written -2147483648]
temp2_max: Suspected underflow: [min=-77000, read 101000, written -2147483648]
temp2_crit: Suspected underflow: [min=-77000, read 101000, written -2147483648]

Clamp temperature ranges when writing limits to fix the problem.
While at it, use sign_extend32() when reading temperatures to make
the code easier to understand.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max1619.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index a89a519cf5d9..464f4c838394 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -52,16 +52,6 @@ static const unsigned short normal_i2c[] = {
  * Conversions
  */
 
-static int temp_from_reg(int val)
-{
-	return (val & 0x80 ? val-0x100 : val) * 1000;
-}
-
-static int temp_to_reg(int val)
-{
-	return (val < 0 ? val+0x100*1000 : val) / 1000;
-}
-
 enum temp_index {
 	t_input1 = 0,
 	t_input2,
@@ -142,7 +132,7 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct max1619_data *data = max1619_update_device(dev);
 
-	return sprintf(buf, "%d\n", temp_from_reg(data->temp[attr->index]));
+	return sprintf(buf, "%d\n", sign_extend(data->temp[attr->index], 7) * 1000);
 }
 
 static ssize_t temp_store(struct device *dev,
@@ -158,7 +148,7 @@ static ssize_t temp_store(struct device *dev,
 		return err;
 
 	mutex_lock(&data->update_lock);
-	data->temp[attr->index] = temp_to_reg(val);
+	data->temp[attr->index] = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
 	i2c_smbus_write_byte_data(client, regs_write[attr->index],
 				  data->temp[attr->index]);
 	mutex_unlock(&data->update_lock);
-- 
2.39.2

[PATCH v2 2/7] hwmon: (max1619) Reorder include files to alphabetic order

[Guenter Roeck]

Simplify maintenance by reordering include files to alphabetic order.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max1619.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 464f4c838394..8eb7d04bd2f5 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -12,15 +12,15 @@
  *   http://pdfserv.maxim-ic.com/en/ds/MAX1619.pdf
  */
 
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/jiffies.h>
-#include <linux/i2c.h>
+#include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/slab.h>
 #include <linux/sysfs.h>
 
 static const unsigned short normal_i2c[] = {
-- 
2.39.2

[PATCH v2 3/7] hwmon: (max1619) Mask valid alarm bits

[Guenter Roeck]

Bit 0, 5, and 6 in the status register are reserved and, if set, do not
indicate an alarm. Bit 7 is the 'busy' bit and also does not indicate
an alarm. Mask the non-alarm bits to avoid reporting them to userspace.

Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

 drivers/hwmon/max1619.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 8eb7d04bd2f5..5edc9bbbe299 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -112,6 +112,7 @@ static struct max1619_data *max1619_update_device(struct device *dev)
 		config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
 		if (!(config & 0x20))
 			data->alarms ^= 0x02;
+		data->alarms &= 0x1e;
 
 		data->last_updated = jiffies;
 		data->valid = true;
-- 
2.39.2

Re: [PATCH v2 3/7] hwmon: (max1619) Mask valid alarm bits

[Tzung-Bi Shih]

On Sun, Jul 28, 2024 at 07:37:11AM -0700, Guenter Roeck wrote:
> Bit 0, 5, and 6 in the status register are reserved and, if set, do not
> indicate an alarm. Bit 7 is the 'busy' bit and also does not indicate
> an alarm. Mask the non-alarm bits to avoid reporting them to userspace.
> 
> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH v2 4/7] hwmon: (max1619) Convert to use regmap

[Guenter Roeck]

Use regmap for local caching, to hide register read/write address
differences, and for multi-byte operations. With this change,
the driver specific lock is no longer necessary.

While at it, check errors seen when initializing the chip and bail out
if chip initialization fails.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Split masking of 'alarms' attribute into separate patch
    Initialize chip to match original code

 Documentation/hwmon/max1619.rst |   4 -
 drivers/hwmon/max1619.c         | 259 +++++++++++++++++---------------
 2 files changed, 137 insertions(+), 126 deletions(-)

diff --git a/Documentation/hwmon/max1619.rst b/Documentation/hwmon/max1619.rst
index e25956e70f73..b5fc175ae18d 100644
--- a/Documentation/hwmon/max1619.rst
+++ b/Documentation/hwmon/max1619.rst
@@ -27,7 +27,3 @@ All temperature values are given in degrees Celsius. Resolution
 is 1.0 degree for the local temperature and for the remote temperature.
 
 Only the external sensor has high and low limits.
-
-The max1619 driver will not update its values more frequently than every
-other second; reading them more often will do no harm, but will return
-'old' values.
diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 5edc9bbbe299..3ecad847e597 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -17,10 +17,8 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/jiffies.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/slab.h>
+#include <linux/regmap.h>
 #include <linux/sysfs.h>
 
 static const unsigned short normal_i2c[] = {
@@ -30,27 +28,17 @@ static const unsigned short normal_i2c[] = {
  * The MAX1619 registers
  */
 
-#define MAX1619_REG_R_MAN_ID		0xFE
-#define MAX1619_REG_R_CHIP_ID		0xFF
-#define MAX1619_REG_R_CONFIG		0x03
-#define MAX1619_REG_W_CONFIG		0x09
-#define MAX1619_REG_R_CONVRATE		0x04
-#define MAX1619_REG_W_CONVRATE		0x0A
-#define MAX1619_REG_R_STATUS		0x02
-#define MAX1619_REG_R_LOCAL_TEMP	0x00
-#define MAX1619_REG_R_REMOTE_TEMP	0x01
-#define MAX1619_REG_R_REMOTE_HIGH	0x07
-#define MAX1619_REG_W_REMOTE_HIGH	0x0D
-#define MAX1619_REG_R_REMOTE_LOW	0x08
-#define MAX1619_REG_W_REMOTE_LOW	0x0E
-#define MAX1619_REG_R_REMOTE_CRIT	0x10
-#define MAX1619_REG_W_REMOTE_CRIT	0x12
-#define MAX1619_REG_R_TCRIT_HYST	0x11
-#define MAX1619_REG_W_TCRIT_HYST	0x13
-
-/*
- * Conversions
- */
+#define MAX1619_REG_LOCAL_TEMP		0x00
+#define MAX1619_REG_REMOTE_TEMP		0x01
+#define MAX1619_REG_STATUS		0x02
+#define MAX1619_REG_CONFIG		0x03
+#define MAX1619_REG_CONVRATE		0x04
+#define MAX1619_REG_REMOTE_HIGH		0x07
+#define MAX1619_REG_REMOTE_LOW		0x08
+#define MAX1619_REG_REMOTE_CRIT		0x10
+#define MAX1619_REG_REMOTE_CRIT_HYST	0x11
+#define MAX1619_REG_MAN_ID		0xFE
+#define MAX1619_REG_CHIP_ID		0xFF
 
 enum temp_index {
 	t_input1 = 0,
@@ -66,63 +54,15 @@ enum temp_index {
  * Client data (each client gets its own)
  */
 
-struct max1619_data {
-	struct i2c_client *client;
-	struct mutex update_lock;
-	bool valid; /* false until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
-
-	/* registers values */
-	u8 temp[t_num_regs];	/* index with enum temp_index */
-	u8 alarms;
+static const u8 regs[t_num_regs] = {
+	[t_input1] = MAX1619_REG_LOCAL_TEMP,
+	[t_input2] = MAX1619_REG_REMOTE_TEMP,
+	[t_low2] = MAX1619_REG_REMOTE_LOW,
+	[t_high2] = MAX1619_REG_REMOTE_HIGH,
+	[t_crit2] = MAX1619_REG_REMOTE_CRIT,
+	[t_hyst2] = MAX1619_REG_REMOTE_CRIT_HYST,
 };
 
-static const u8 regs_read[t_num_regs] = {
-	[t_input1] = MAX1619_REG_R_LOCAL_TEMP,
-	[t_input2] = MAX1619_REG_R_REMOTE_TEMP,
-	[t_low2] = MAX1619_REG_R_REMOTE_LOW,
-	[t_high2] = MAX1619_REG_R_REMOTE_HIGH,
-	[t_crit2] = MAX1619_REG_R_REMOTE_CRIT,
-	[t_hyst2] = MAX1619_REG_R_TCRIT_HYST,
-};
-
-static const u8 regs_write[t_num_regs] = {
-	[t_low2] = MAX1619_REG_W_REMOTE_LOW,
-	[t_high2] = MAX1619_REG_W_REMOTE_HIGH,
-	[t_crit2] = MAX1619_REG_W_REMOTE_CRIT,
-	[t_hyst2] = MAX1619_REG_W_TCRIT_HYST,
-};
-
-static struct max1619_data *max1619_update_device(struct device *dev)
-{
-	struct max1619_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int config, i;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ * 2) || !data->valid) {
-		dev_dbg(&client->dev, "Updating max1619 data.\n");
-		for (i = 0; i < t_num_regs; i++)
-			data->temp[i] = i2c_smbus_read_byte_data(client,
-					regs_read[i]);
-		data->alarms = i2c_smbus_read_byte_data(client,
-					MAX1619_REG_R_STATUS);
-		/* If OVERT polarity is low, reverse alarm bit */
-		config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
-		if (!(config & 0x20))
-			data->alarms ^= 0x02;
-		data->alarms &= 0x1e;
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-
-	mutex_unlock(&data->update_lock);
-
-	return data;
-}
-
 /*
  * Sysfs stuff
  */
@@ -131,9 +71,15 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max1619_data *data = max1619_update_device(dev);
+	struct regmap *regmap = dev_get_drvdata(dev);
+	u32 temp;
+	int ret;
 
-	return sprintf(buf, "%d\n", sign_extend(data->temp[attr->index], 7) * 1000);
+	ret = regmap_read(regmap, regs[attr->index], &temp);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", sign_extend32(temp, 7) * 1000);
 }
 
 static ssize_t temp_store(struct device *dev,
@@ -141,34 +87,61 @@ static ssize_t temp_store(struct device *dev,
 			  size_t count)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max1619_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = dev_get_drvdata(dev);
 	long val;
 	int err = kstrtol(buf, 10, &val);
 	if (err)
 		return err;
 
-	mutex_lock(&data->update_lock);
-	data->temp[attr->index] = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
-	i2c_smbus_write_byte_data(client, regs_write[attr->index],
-				  data->temp[attr->index]);
-	mutex_unlock(&data->update_lock);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
+	err = regmap_write(regmap, regs[attr->index], val);
+	if (err < 0)
+		return err;
 	return count;
 }
 
+static int get_alarms(struct regmap *regmap)
+{
+	static u32 regs[2] = { MAX1619_REG_STATUS, MAX1619_REG_CONFIG };
+	u8 regdata[2];
+	int ret;
+
+	ret = regmap_multi_reg_read(regmap, regs, regdata, 2);
+	if (ret)
+		return ret;
+
+	/* OVERT status bit may be reversed */
+	if (!(regdata[1] & 0x20))
+		regdata[0] ^= 0x02;
+
+	return regdata[0] & 0x1e;
+}
+
 static ssize_t alarms_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
-	struct max1619_data *data = max1619_update_device(dev);
-	return sprintf(buf, "%d\n", data->alarms);
+	struct regmap *regmap = dev_get_drvdata(dev);
+	int alarms;
+
+	alarms = get_alarms(regmap);
+	if (alarms < 0)
+		return alarms;
+
+	return sprintf(buf, "%d\n", alarms);
 }
 
 static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
 	int bitnr = to_sensor_dev_attr(attr)->index;
-	struct max1619_data *data = max1619_update_device(dev);
-	return sprintf(buf, "%d\n", (data->alarms >> bitnr) & 1);
+	struct regmap *regmap = dev_get_drvdata(dev);
+	int alarms;
+
+	alarms = get_alarms(regmap);
+	if (alarms < 0)
+		return alarms;
+
+	return sprintf(buf, "%d\n", (alarms >> bitnr) & 1);
 }
 
 static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, t_input1);
@@ -212,9 +185,9 @@ static int max1619_detect(struct i2c_client *client,
 		return -ENODEV;
 
 	/* detection */
-	reg_config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
-	reg_convrate = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONVRATE);
-	reg_status = i2c_smbus_read_byte_data(client, MAX1619_REG_R_STATUS);
+	reg_config = i2c_smbus_read_byte_data(client, MAX1619_REG_CONFIG);
+	reg_convrate = i2c_smbus_read_byte_data(client, MAX1619_REG_CONVRATE);
+	reg_status = i2c_smbus_read_byte_data(client, MAX1619_REG_STATUS);
 	if ((reg_config & 0x03) != 0x00
 	 || reg_convrate > 0x07 || (reg_status & 0x61) != 0x00) {
 		dev_dbg(&adapter->dev, "MAX1619 detection failed at 0x%02x\n",
@@ -223,8 +196,8 @@ static int max1619_detect(struct i2c_client *client,
 	}
 
 	/* identification */
-	man_id = i2c_smbus_read_byte_data(client, MAX1619_REG_R_MAN_ID);
-	chip_id = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CHIP_ID);
+	man_id = i2c_smbus_read_byte_data(client, MAX1619_REG_MAN_ID);
+	chip_id = i2c_smbus_read_byte_data(client, MAX1619_REG_CHIP_ID);
 	if (man_id != 0x4D || chip_id != 0x04) {
 		dev_info(&adapter->dev,
 			 "Unsupported chip (man_id=0x%02X, chip_id=0x%02X).\n",
@@ -237,40 +210,82 @@ static int max1619_detect(struct i2c_client *client,
 	return 0;
 }
 
-static void max1619_init_client(struct i2c_client *client)
+static int max1619_init_chip(struct regmap *regmap)
 {
-	u8 config;
+	int ret;
 
-	/*
-	 * Start the conversions.
-	 */
-	i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONVRATE,
-				  5); /* 2 Hz */
-	config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
-	if (config & 0x40)
-		i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONFIG,
-					  config & 0xBF); /* run */
+	ret = regmap_write(regmap, MAX1619_REG_CONVRATE, 5);	/* 2 Hz */
+	if (ret)
+		return ret;
+
+	/* Start conversions */
+	return regmap_set_bits(regmap, MAX1619_REG_CONFIG, 0x40);
 }
 
-static int max1619_probe(struct i2c_client *new_client)
+/* regmap */
+
+static int max1619_reg_read(void *context, unsigned int reg, unsigned int *val)
 {
-	struct max1619_data *data;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(context, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+	return 0;
+}
+
+static int max1619_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	int offset = reg < MAX1619_REG_REMOTE_CRIT ? 6 : 2;
+
+	return i2c_smbus_write_byte_data(context, reg + offset, val);
+}
+
+static bool max1619_regmap_is_volatile(struct device *dev, unsigned int reg)
+{
+	return reg <= MAX1619_REG_STATUS;
+}
+
+static bool max1619_regmap_is_writeable(struct device *dev, unsigned int reg)
+{
+	return reg > MAX1619_REG_STATUS && reg <= MAX1619_REG_REMOTE_CRIT_HYST;
+}
+
+static const struct regmap_config max1619_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = MAX1619_REG_REMOTE_CRIT_HYST,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = max1619_regmap_is_volatile,
+	.writeable_reg = max1619_regmap_is_writeable,
+};
+
+static const struct regmap_bus max1619_regmap_bus = {
+	.reg_write = max1619_reg_write,
+	.reg_read = max1619_reg_read,
+};
+
+static int max1619_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
+	int ret;
 
-	data = devm_kzalloc(&new_client->dev, sizeof(struct max1619_data),
-			    GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
+	regmap = devm_regmap_init(dev, &max1619_regmap_bus, client,
+				  &max1619_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
 
-	data->client = new_client;
-	mutex_init(&data->update_lock);
+	ret = max1619_init_chip(regmap);
+	if (ret)
+		return ret;
 
-	/* Initialize the MAX1619 chip */
-	max1619_init_client(new_client);
-
-	hwmon_dev = devm_hwmon_device_register_with_groups(&new_client->dev,
-							   new_client->name,
-							   data,
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+							   client->name,
+							   regmap,
 							   max1619_groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
-- 
2.39.2

Re: [PATCH v2 4/7] hwmon: (max1619) Convert to use regmap

[Tzung-Bi Shih]

On Sun, Jul 28, 2024 at 07:37:12AM -0700, Guenter Roeck wrote:
> -static void max1619_init_client(struct i2c_client *client)
> +static int max1619_init_chip(struct regmap *regmap)
>  {
> -	u8 config;
> +	int ret;
>  
> -	/*
> -	 * Start the conversions.
> -	 */
> -	i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONVRATE,
> -				  5); /* 2 Hz */
> -	config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
> -	if (config & 0x40)
> -		i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONFIG,
> -					  config & 0xBF); /* run */
> +	ret = regmap_write(regmap, MAX1619_REG_CONVRATE, 5);	/* 2 Hz */
> +	if (ret)
> +		return ret;
> +
> +	/* Start conversions */
> +	return regmap_set_bits(regmap, MAX1619_REG_CONFIG, 0x40);

Should be regmap_clear_bits()?

Re: [PATCH v2 4/7] hwmon: (max1619) Convert to use regmap

[Guenter Roeck]

On 7/28/24 19:18, Tzung-Bi Shih wrote:
> On Sun, Jul 28, 2024 at 07:37:12AM -0700, Guenter Roeck wrote:
>> -static void max1619_init_client(struct i2c_client *client)
>> +static int max1619_init_chip(struct regmap *regmap)
>>   {
>> -	u8 config;
>> +	int ret;
>>   
>> -	/*
>> -	 * Start the conversions.
>> -	 */
>> -	i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONVRATE,
>> -				  5); /* 2 Hz */
>> -	config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
>> -	if (config & 0x40)
>> -		i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONFIG,
>> -					  config & 0xBF); /* run */
>> +	ret = regmap_write(regmap, MAX1619_REG_CONVRATE, 5);	/* 2 Hz */
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Start conversions */
>> +	return regmap_set_bits(regmap, MAX1619_REG_CONFIG, 0x40);
> 
> Should be regmap_clear_bits()?


Sigh. yes, of course.

Thanks,
Guenter

Re: [PATCH v2 4/7] hwmon: (max1619) Convert to use regmap

[Tzung-Bi Shih]

On Sun, Jul 28, 2024 at 08:16:35PM -0700, Guenter Roeck wrote:
> On 7/28/24 19:18, Tzung-Bi Shih wrote:
> > On Sun, Jul 28, 2024 at 07:37:12AM -0700, Guenter Roeck wrote:
> > > -static void max1619_init_client(struct i2c_client *client)
> > > +static int max1619_init_chip(struct regmap *regmap)
> > >   {
> > > -	u8 config;
> > > +	int ret;
> > > -	/*
> > > -	 * Start the conversions.
> > > -	 */
> > > -	i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONVRATE,
> > > -				  5); /* 2 Hz */
> > > -	config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
> > > -	if (config & 0x40)
> > > -		i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONFIG,
> > > -					  config & 0xBF); /* run */
> > > +	ret = regmap_write(regmap, MAX1619_REG_CONVRATE, 5);	/* 2 Hz */
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Start conversions */
> > > +	return regmap_set_bits(regmap, MAX1619_REG_CONFIG, 0x40);
> > 
> > Should be regmap_clear_bits()?
> 
> 
> Sigh. yes, of course.

With that,
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Re: [PATCH v2 4/7] hwmon: (max1619) Convert to use regmap

[Guenter Roeck]

On 7/28/24 20:24, Tzung-Bi Shih wrote:
> On Sun, Jul 28, 2024 at 08:16:35PM -0700, Guenter Roeck wrote:
>> On 7/28/24 19:18, Tzung-Bi Shih wrote:
>>> On Sun, Jul 28, 2024 at 07:37:12AM -0700, Guenter Roeck wrote:
>>>> -static void max1619_init_client(struct i2c_client *client)
>>>> +static int max1619_init_chip(struct regmap *regmap)
>>>>    {
>>>> -	u8 config;
>>>> +	int ret;
>>>> -	/*
>>>> -	 * Start the conversions.
>>>> -	 */
>>>> -	i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONVRATE,
>>>> -				  5); /* 2 Hz */
>>>> -	config = i2c_smbus_read_byte_data(client, MAX1619_REG_R_CONFIG);
>>>> -	if (config & 0x40)
>>>> -		i2c_smbus_write_byte_data(client, MAX1619_REG_W_CONFIG,
>>>> -					  config & 0xBF); /* run */
>>>> +	ret = regmap_write(regmap, MAX1619_REG_CONVRATE, 5);	/* 2 Hz */
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Start conversions */
>>>> +	return regmap_set_bits(regmap, MAX1619_REG_CONFIG, 0x40);
>>>
>>> Should be regmap_clear_bits()?
>>
>>
>> Sigh. yes, of course.
> 
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 

Thanks!

Guenter

[PATCH v2 5/7] hwmon: (max1619) Convert to with_info API

[Guenter Roeck]

Convert driver to with_info hwmon API to simplify the code and
with it its maintainability.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max1619.c | 269 +++++++++++++++++++++++-----------------
 1 file changed, 157 insertions(+), 112 deletions(-)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 3ecad847e597..fd84bcc789fa 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -14,20 +14,14 @@
 
 #include <linux/err.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
-#include <linux/sysfs.h>
 
 static const unsigned short normal_i2c[] = {
 	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
 
-/*
- * The MAX1619 registers
- */
-
 #define MAX1619_REG_LOCAL_TEMP		0x00
 #define MAX1619_REG_REMOTE_TEMP		0x01
 #define MAX1619_REG_STATUS		0x02
@@ -40,66 +34,6 @@ static const unsigned short normal_i2c[] = {
 #define MAX1619_REG_MAN_ID		0xFE
 #define MAX1619_REG_CHIP_ID		0xFF
 
-enum temp_index {
-	t_input1 = 0,
-	t_input2,
-	t_low2,
-	t_high2,
-	t_crit2,
-	t_hyst2,
-	t_num_regs
-};
-
-/*
- * Client data (each client gets its own)
- */
-
-static const u8 regs[t_num_regs] = {
-	[t_input1] = MAX1619_REG_LOCAL_TEMP,
-	[t_input2] = MAX1619_REG_REMOTE_TEMP,
-	[t_low2] = MAX1619_REG_REMOTE_LOW,
-	[t_high2] = MAX1619_REG_REMOTE_HIGH,
-	[t_crit2] = MAX1619_REG_REMOTE_CRIT,
-	[t_hyst2] = MAX1619_REG_REMOTE_CRIT_HYST,
-};
-
-/*
- * Sysfs stuff
- */
-
-static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct regmap *regmap = dev_get_drvdata(dev);
-	u32 temp;
-	int ret;
-
-	ret = regmap_read(regmap, regs[attr->index], &temp);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%d\n", sign_extend32(temp, 7) * 1000);
-}
-
-static ssize_t temp_store(struct device *dev,
-			  struct device_attribute *devattr, const char *buf,
-			  size_t count)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct regmap *regmap = dev_get_drvdata(dev);
-	long val;
-	int err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
-	err = regmap_write(regmap, regs[attr->index], val);
-	if (err < 0)
-		return err;
-	return count;
-}
-
 static int get_alarms(struct regmap *regmap)
 {
 	static u32 regs[2] = { MAX1619_REG_STATUS, MAX1619_REG_CONFIG };
@@ -117,62 +51,175 @@ static int get_alarms(struct regmap *regmap)
 	return regdata[0] & 0x1e;
 }
 
-static ssize_t alarms_show(struct device *dev, struct device_attribute *attr,
-			   char *buf)
+static int max1619_temp_read(struct regmap *regmap, u32 attr, int channel, long *val)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
-	int alarms;
+	int reg = -1, alarm_bit = 0;
+	u32 temp;
+	int ret;
 
-	alarms = get_alarms(regmap);
-	if (alarms < 0)
-		return alarms;
-
-	return sprintf(buf, "%d\n", alarms);
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? MAX1619_REG_REMOTE_TEMP : MAX1619_REG_LOCAL_TEMP;
+		break;
+	case hwmon_temp_min:
+		reg = MAX1619_REG_REMOTE_LOW;
+		break;
+	case hwmon_temp_max:
+		reg = MAX1619_REG_REMOTE_HIGH;
+		break;
+	case hwmon_temp_crit:
+		reg = MAX1619_REG_REMOTE_CRIT;
+		break;
+	case hwmon_temp_crit_hyst:
+		reg = MAX1619_REG_REMOTE_CRIT_HYST;
+		break;
+	case hwmon_temp_min_alarm:
+		alarm_bit = 3;
+		break;
+	case hwmon_temp_max_alarm:
+		alarm_bit = 4;
+		break;
+	case hwmon_temp_crit_alarm:
+		alarm_bit = 1;
+		break;
+	case hwmon_temp_fault:
+		alarm_bit = 2;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	if (reg >= 0) {
+		ret = regmap_read(regmap, reg, &temp);
+		if (ret < 0)
+			return ret;
+		*val = sign_extend32(temp, 7) * 1000;
+	} else {
+		ret = get_alarms(regmap);
+		if (ret < 0)
+			return ret;
+		*val = !!(ret & BIT(alarm_bit));
+	}
+	return 0;
 }
 
-static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
-			  char *buf)
+static int max1619_chip_read(struct regmap *regmap, u32 attr, long *val)
 {
-	int bitnr = to_sensor_dev_attr(attr)->index;
-	struct regmap *regmap = dev_get_drvdata(dev);
 	int alarms;
 
-	alarms = get_alarms(regmap);
-	if (alarms < 0)
-		return alarms;
-
-	return sprintf(buf, "%d\n", (alarms >> bitnr) & 1);
+	switch (attr) {
+	case hwmon_chip_alarms:
+		alarms = get_alarms(regmap);
+		if (alarms < 0)
+			return alarms;
+		*val = alarms;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
 }
 
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, t_input1);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, t_input2);
-static SENSOR_DEVICE_ATTR_RW(temp2_min, temp, t_low2);
-static SENSOR_DEVICE_ATTR_RW(temp2_max, temp, t_high2);
-static SENSOR_DEVICE_ATTR_RW(temp2_crit, temp, t_crit2);
-static SENSOR_DEVICE_ATTR_RW(temp2_crit_hyst, temp, t_hyst2);
+static int max1619_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
 
-static DEVICE_ATTR_RO(alarms);
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, alarm, 1);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, alarm, 2);
-static SENSOR_DEVICE_ATTR_RO(temp2_min_alarm, alarm, 3);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, alarm, 4);
+	switch (type) {
+	case hwmon_chip:
+		return max1619_chip_read(regmap, attr, val);
+	case hwmon_temp:
+		return max1619_temp_read(regmap, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
 
-static struct attribute *max1619_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_min.dev_attr.attr,
-	&sensor_dev_attr_temp2_max.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+static int max1619_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	int reg;
 
-	&dev_attr_alarms.attr,
-	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
+	if (type != hwmon_temp)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = MAX1619_REG_REMOTE_LOW;
+		break;
+	case hwmon_temp_max:
+		reg = MAX1619_REG_REMOTE_HIGH;
+		break;
+	case hwmon_temp_crit:
+		reg = MAX1619_REG_REMOTE_CRIT;
+		break;
+	case hwmon_temp_crit_hyst:
+		reg = MAX1619_REG_REMOTE_CRIT_HYST;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
+	return regmap_write(regmap, reg, val);
+}
+
+static umode_t max1619_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_alarms:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+		case hwmon_temp_crit:
+		case hwmon_temp_crit_hyst:
+			return 0644;
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+		case hwmon_temp_crit_alarm:
+		case hwmon_temp_fault:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const struct hwmon_channel_info * const max1619_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT),
 	NULL
 };
-ATTRIBUTE_GROUPS(max1619);
+
+static const struct hwmon_ops max1619_hwmon_ops = {
+	.is_visible = max1619_is_visible,
+	.read = max1619_read,
+	.write = max1619_write,
+};
+
+static const struct hwmon_chip_info max1619_chip_info = {
+	.ops = &max1619_hwmon_ops,
+	.info = max1619_info,
+};
 
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int max1619_detect(struct i2c_client *client,
@@ -283,10 +330,8 @@ static int max1619_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
-							   client->name,
-							   regmap,
-							   max1619_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 regmap, &max1619_chip_info, NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.2

[PATCH v2 6/7] hwmon: (max1619) Add support for update_interval attribute

[Guenter Roeck]

The chip supports reading and writing the conversion rate.
Add support for the update_interval sysfs attribute.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max1619.c | 50 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index fd84bcc789fa..69d3156fa2ba 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/util_macros.h>
 
 static const unsigned short normal_i2c[] = {
 	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
@@ -102,11 +103,20 @@ static int max1619_temp_read(struct regmap *regmap, u32 attr, int channel, long
 	return 0;
 }
 
+static u16 update_intervals[] = { 16000, 8000, 4000, 2000, 1000, 500, 250, 125 };
+
 static int max1619_chip_read(struct regmap *regmap, u32 attr, long *val)
 {
-	int alarms;
+	int alarms, ret;
+	u32 regval;
 
 	switch (attr) {
+	case hwmon_chip_update_interval:
+		ret = regmap_read(regmap, MAX1619_REG_CONVRATE, &regval);
+		if (ret < 0)
+			return ret;
+		*val = update_intervals[regval & 7];
+		break;
 	case hwmon_chip_alarms:
 		alarms = get_alarms(regmap);
 		if (alarms < 0)
@@ -134,14 +144,21 @@ static int max1619_read(struct device *dev, enum hwmon_sensor_types type,
 	}
 }
 
-static int max1619_write(struct device *dev, enum hwmon_sensor_types type,
-			 u32 attr, int channel, long val)
+static int max1619_chip_write(struct regmap *regmap, u32 attr, long val)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
-	int reg;
-
-	if (type != hwmon_temp)
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		val = find_closest_descending(val, update_intervals, ARRAY_SIZE(update_intervals));
+		return regmap_write(regmap, MAX1619_REG_CONVRATE, val);
+	default:
 		return -EOPNOTSUPP;
+	}
+}
+
+static int max1619_temp_write(struct regmap *regmap,
+			      u32 attr, int channel, long val)
+{
+	int reg;
 
 	switch (attr) {
 	case hwmon_temp_min:
@@ -163,12 +180,29 @@ static int max1619_write(struct device *dev, enum hwmon_sensor_types type,
 	return regmap_write(regmap, reg, val);
 }
 
+static int max1619_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_chip:
+		return max1619_chip_write(regmap, attr, val);
+	case hwmon_temp:
+		return max1619_temp_write(regmap, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t max1619_is_visible(const void *_data, enum hwmon_sensor_types type,
 				  u32 attr, int channel)
 {
 	switch (type) {
 	case hwmon_chip:
 		switch (attr) {
+		case hwmon_chip_update_interval:
+			return 0644;
 		case hwmon_chip_alarms:
 			return 0444;
 		default:
@@ -200,7 +234,7 @@ static umode_t max1619_is_visible(const void *_data, enum hwmon_sensor_types typ
 }
 
 static const struct hwmon_channel_info * const max1619_info[] = {
-	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS),
+	HWMON_CHANNEL_INFO(chip, HWMON_C_ALARMS | HWMON_C_UPDATE_INTERVAL),
 	HWMON_CHANNEL_INFO(temp,
 			   HWMON_T_INPUT,
 			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
-- 
2.39.2

[PATCH v2 7/7] hwmon: (max1619) Improve chip detection code

[Guenter Roeck]

Bail out immediately if reading any of the registers used for chip
detection fails, or if it returns an unexpected value. Drop all log
messages from detection code.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max1619.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 69d3156fa2ba..26ffb21daa3d 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -260,31 +260,27 @@ static int max1619_detect(struct i2c_client *client,
 			  struct i2c_board_info *info)
 {
 	struct i2c_adapter *adapter = client->adapter;
-	u8 reg_config, reg_convrate, reg_status, man_id, chip_id;
+	int regval;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
 
-	/* detection */
-	reg_config = i2c_smbus_read_byte_data(client, MAX1619_REG_CONFIG);
-	reg_convrate = i2c_smbus_read_byte_data(client, MAX1619_REG_CONVRATE);
-	reg_status = i2c_smbus_read_byte_data(client, MAX1619_REG_STATUS);
-	if ((reg_config & 0x03) != 0x00
-	 || reg_convrate > 0x07 || (reg_status & 0x61) != 0x00) {
-		dev_dbg(&adapter->dev, "MAX1619 detection failed at 0x%02x\n",
-			client->addr);
+	regval = i2c_smbus_read_byte_data(client, MAX1619_REG_CONFIG);
+	if (regval < 0 || (regval & 0x03))
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, MAX1619_REG_CONVRATE);
+	if (regval < 0 || regval > 0x07)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, MAX1619_REG_STATUS);
+	if (regval < 0 || (regval & 0x61))
 		return -ENODEV;
-	}
 
-	/* identification */
-	man_id = i2c_smbus_read_byte_data(client, MAX1619_REG_MAN_ID);
-	chip_id = i2c_smbus_read_byte_data(client, MAX1619_REG_CHIP_ID);
-	if (man_id != 0x4D || chip_id != 0x04) {
-		dev_info(&adapter->dev,
-			 "Unsupported chip (man_id=0x%02X, chip_id=0x%02X).\n",
-			 man_id, chip_id);
+	regval = i2c_smbus_read_byte_data(client, MAX1619_REG_MAN_ID);
+	if (regval != 0x4d)
+		return -ENODEV;
+	regval = i2c_smbus_read_byte_data(client, MAX1619_REG_CHIP_ID);
+	if (regval != 0x04)
 		return -ENODEV;
-	}
 
 	strscpy(info->type, "max1619", I2C_NAME_SIZE);
 
-- 
2.39.2