[RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Guenter Roeck]

Hide register size differences in regmap access functions to simplify
runtime code and to simplify adding support for I3C devices. Also
use regmap API functions for bit operations where possible.

For this to work, the 16-bit and 8-bit configuration register has to be
mapped to a 16-bit value. Unlike other registers, this register is a
low-byte-first register, presumably for compatibility with chips with
8-bit wide configuration registers. Hide the differences in the regmap
access code.

While at it, enable alarm attribute support for TMP112.

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fix accesses to 16-bit configuration register.
    Use regmap access functions for all operations on config register.
    Declare regmap bus and use devm_regmap_init().
    Drop local configuration register copy; let regmap handle bit updates.

    Note: The driver could use additional cleanup, such as using bit macros
    and using devm_regulator_get_enable(). That is left for another day.

 drivers/hwmon/lm75.c | 131 ++++++++++++++++++++-----------------------
 1 file changed, 62 insertions(+), 69 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 2c2205aec7d4..d3eb5c9c25b1 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -110,7 +110,6 @@ struct lm75_data {
 	struct regmap			*regmap;
 	struct regulator		*vs;
 	u16				orig_conf;
-	u16				current_conf;
 	u8				resolution;	/* In bits, 9 to 16 */
 	unsigned int			sample_time;	/* In ms */
 	enum lm75_type			kind;
@@ -276,6 +275,7 @@ static const struct lm75_params device_params[] = {
 		.default_sample_time = 125,
 		.num_sample_times = 4,
 		.sample_times = (unsigned int []){ 125, 250, 1000, 4000 },
+		.alarm = true,
 	},
 	[tmp175] = {
 		.set_mask = 3 << 5,	/* 12-bit mode */
@@ -335,40 +335,16 @@ static inline long lm75_reg_to_mc(s16 temp, u8 resolution)
 static int lm75_write_config(struct lm75_data *data, u16 set_mask,
 			     u16 clr_mask)
 {
-	unsigned int value;
+	int err;
 
-	clr_mask |= LM75_SHUTDOWN << (8 * data->params->config_reg_16bits);
-	value = data->current_conf & ~clr_mask;
-	value |= set_mask;
+	err = regmap_update_bits(data->regmap, LM75_REG_CONF,
+				 clr_mask | LM75_SHUTDOWN, set_mask);
+	if (err)
+		return err;
 
-	if (data->current_conf != value) {
-		s32 err;
-		if (data->params->config_reg_16bits)
-			err = regmap_write(data->regmap, LM75_REG_CONF, value);
-		else
-			err = i2c_smbus_write_byte_data(data->client,
-							LM75_REG_CONF,
-							value);
-		if (err)
-			return err;
-		data->current_conf = value;
-	}
 	return 0;
 }
 
-static int lm75_read_config(struct lm75_data *data)
-{
-	int ret;
-	unsigned int status;
-
-	if (data->params->config_reg_16bits) {
-		ret = regmap_read(data->regmap, LM75_REG_CONF, &status);
-		return ret ? ret : status;
-	}
-
-	return i2c_smbus_read_byte_data(data->client, LM75_REG_CONF);
-}
-
 static irqreturn_t lm75_alarm_handler(int irq, void *private)
 {
 	struct device *hwmon_dev = private;
@@ -418,7 +394,8 @@ static int lm75_read(struct device *dev, enum hwmon_sensor_types type,
 		if (attr == hwmon_temp_alarm) {
 			switch (data->kind) {
 			case as6200:
-				*val = (regval >> 5) & 0x1;
+			case tmp112:
+				*val = (regval >> 13) & 0x1;
 				break;
 			default:
 				return -EINVAL;
@@ -469,7 +446,6 @@ static int lm75_write_temp(struct device *dev, u32 attr, long temp)
 static int lm75_update_interval(struct device *dev, long val)
 {
 	struct lm75_data *data = dev_get_drvdata(dev);
-	unsigned int reg;
 	u8 index;
 	s32 err;
 
@@ -489,19 +465,14 @@ static int lm75_update_interval(struct device *dev, long val)
 		break;
 	case tmp112:
 	case as6200:
-		err = regmap_read(data->regmap, LM75_REG_CONF, &reg);
-		if (err < 0)
-			return err;
-		reg &= ~0x00c0;
-		reg |= (3 - index) << 6;
-		err = regmap_write(data->regmap, LM75_REG_CONF, reg);
+		err = regmap_update_bits(data->regmap, LM75_REG_CONF,
+					 0xc000, (3 - index) << 14);
 		if (err < 0)
 			return err;
 		data->sample_time = data->params->sample_times[index];
 		break;
 	case pct2075:
-		err = i2c_smbus_write_byte_data(data->client, PCT2075_REG_IDLE,
-						index + 1);
+		err = regmap_write(data->regmap, PCT2075_REG_IDLE, index + 1);
 		if (err)
 			return err;
 		data->sample_time = data->params->sample_times[index];
@@ -598,6 +569,39 @@ static bool lm75_is_volatile_reg(struct device *dev, unsigned int reg)
 	return reg == LM75_REG_TEMP || reg == LM75_REG_CONF;
 }
 
+static int lm75_i2c_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct lm75_data *data = context;
+	struct i2c_client *client = data->client;
+	int ret;
+
+	if (reg == LM75_REG_CONF) {
+		if (!data->params->config_reg_16bits)
+			ret = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
+		else
+			ret = i2c_smbus_read_word_data(client, LM75_REG_CONF);
+	} else {
+		ret = i2c_smbus_read_word_swapped(client, reg);
+	}
+	if (ret < 0)
+		return ret;
+	*val = ret;
+	return 0;
+}
+
+static int lm75_i2c_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct lm75_data *data = context;
+	struct i2c_client *client = data->client;
+
+	if (reg == PCT2075_REG_IDLE ||
+	    (reg == LM75_REG_CONF && !data->params->config_reg_16bits))
+		return i2c_smbus_write_byte_data(client, reg, val);
+	else if (reg == LM75_REG_CONF)
+		return i2c_smbus_write_word_data(client, reg, val);
+	return i2c_smbus_write_word_swapped(client, reg, val);
+}
+
 static const struct regmap_config lm75_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -610,6 +614,11 @@ static const struct regmap_config lm75_regmap_config = {
 	.use_single_write = true,
 };
 
+static const struct regmap_bus lm75_i2c_regmap_bus = {
+	.reg_read = lm75_i2c_reg_read,
+	.reg_write = lm75_i2c_reg_write,
+};
+
 static void lm75_disable_regulator(void *data)
 {
 	struct lm75_data *lm75 = data;
@@ -620,9 +629,8 @@ static void lm75_disable_regulator(void *data)
 static void lm75_remove(void *data)
 {
 	struct lm75_data *lm75 = data;
-	struct i2c_client *client = lm75->client;
 
-	i2c_smbus_write_byte_data(client, LM75_REG_CONF, lm75->orig_conf);
+	regmap_write(lm75->regmap, LM75_REG_CONF, lm75->orig_conf);
 }
 
 static int lm75_probe(struct i2c_client *client)
@@ -640,6 +648,9 @@ static int lm75_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	/* needed by custom regmap callbacks */
+	dev_set_drvdata(dev, data);
+
 	data->client = client;
 	data->kind = (uintptr_t)i2c_get_match_data(client);
 
@@ -647,7 +658,8 @@ static int lm75_probe(struct i2c_client *client)
 	if (IS_ERR(data->vs))
 		return PTR_ERR(data->vs);
 
-	data->regmap = devm_regmap_init_i2c(client, &lm75_regmap_config);
+	data->regmap = devm_regmap_init(dev, &lm75_i2c_regmap_bus, data,
+					&lm75_regmap_config);
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
@@ -673,13 +685,10 @@ static int lm75_probe(struct i2c_client *client)
 		return err;
 
 	/* Cache original configuration */
-	status = lm75_read_config(data);
-	if (status < 0) {
-		dev_dbg(dev, "Can't read config? %d\n", status);
-		return status;
-	}
+	err = regmap_read(data->regmap, LM75_REG_CONF, &status);
+	if (err)
+		return err;
 	data->orig_conf = status;
-	data->current_conf = status;
 
 	err = lm75_write_config(data, data->params->set_mask,
 				data->params->clr_mask);
@@ -972,32 +981,16 @@ static int lm75_detect(struct i2c_client *new_client,
 #ifdef CONFIG_PM
 static int lm75_suspend(struct device *dev)
 {
-	int status;
-	struct i2c_client *client = to_i2c_client(dev);
+	struct lm75_data *data = dev_get_drvdata(dev);
 
-	status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
-	if (status < 0) {
-		dev_dbg(&client->dev, "Can't read config? %d\n", status);
-		return status;
-	}
-	status = status | LM75_SHUTDOWN;
-	i2c_smbus_write_byte_data(client, LM75_REG_CONF, status);
-	return 0;
+	return regmap_update_bits(data->regmap, LM75_REG_CONF, LM75_SHUTDOWN, LM75_SHUTDOWN);
 }
 
 static int lm75_resume(struct device *dev)
 {
-	int status;
-	struct i2c_client *client = to_i2c_client(dev);
+	struct lm75_data *data = dev_get_drvdata(dev);
 
-	status = i2c_smbus_read_byte_data(client, LM75_REG_CONF);
-	if (status < 0) {
-		dev_dbg(&client->dev, "Can't read config? %d\n", status);
-		return status;
-	}
-	status = status & ~LM75_SHUTDOWN;
-	i2c_smbus_write_byte_data(client, LM75_REG_CONF, status);
-	return 0;
+	return regmap_update_bits(data->regmap, LM75_REG_CONF, LM75_SHUTDOWN, 0);
 }
 
 static const struct dev_pm_ops lm75_dev_pm_ops = {
-- 
2.45.2

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

Hi Guenter,

quick response, will test your new patch later.

> ---
> v2: Fix accesses to 16-bit configuration register.

Great, thanks, I missed that more fixing is needed.

>     Use regmap access functions for all operations on config register.
>     Declare regmap bus and use devm_regmap_init().

The regmap_bus solution is really nice! Didn't know about it.

>     Drop local configuration register copy; let regmap handle bit updates.

And one patch is gone from my queue. I had this as well :)

>     Note: The driver could use additional cleanup, such as using bit macros
>     and using devm_regulator_get_enable(). That is left for another day.

I didn't do BIT yet (although tempted), but I have the regulator cleanup
already. Also, 'client' can go from the priv struct with just a little
bit of reordering. I hope I can send all the stuff tomorrow.

I have the proof-of-concept running on I3C already, need to remove some
FIXMEs, though.

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Guenter Roeck]

Hi Wolfram,

On 12/18/24 08:07, Wolfram Sang wrote:
> Hi Guenter,
> 
> quick response, will test your new patch later.
> 
>> ---
>> v2: Fix accesses to 16-bit configuration register.
> 
> Great, thanks, I missed that more fixing is needed.
> 
>>      Use regmap access functions for all operations on config register.
>>      Declare regmap bus and use devm_regmap_init().
> 
> The regmap_bus solution is really nice! Didn't know about it.
> 

Yes, it comes quite handy for hiding register access oddities (such as a mix
of 16-bit and 8-bit registers, or different read/write register addresses)
from the actual driver.

>>      Drop local configuration register copy; let regmap handle bit updates.
> 
> And one patch is gone from my queue. I had this as well :)
> 
>>      Note: The driver could use additional cleanup, such as using bit macros
>>      and using devm_regulator_get_enable(). That is left for another day.
> 
> I didn't do BIT yet (although tempted), but I have the regulator cleanup
> already. Also, 'client' can go from the priv struct with just a little
> bit of reordering. I hope I can send all the stuff tomorrow.
> 

Ah yes, 'client' can be passed as bus context, and isn't used elsewhere.
The access functions need lm75_data, but they can get the pointer to it
from drvdata.

Maybe that should be part of this patch. I assume you'll take it over - do you
want to make that change or should I send another version ? Handling it in a
separate patch is fine with me as well if you prefer to do that.

Thanks,
Guenter

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]


> Ah yes, 'client' can be passed as bus context, and isn't used elsewhere.
> The access functions need lm75_data, but they can get the pointer to it
> from drvdata.

Yup, that's exactly what I did.

> Maybe that should be part of this patch. I assume you'll take it over - do you
> want to make that change or should I send another version ? Handling it in a
> separate patch is fine with me as well if you prefer to do that.

Since I already have that patch on top, I suggest to keep it
incremental. Also easier to debug if something goes wrong etc...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Guenter Roeck]

On 12/18/24 08:25, Wolfram Sang wrote:
> 
>> Ah yes, 'client' can be passed as bus context, and isn't used elsewhere.
>> The access functions need lm75_data, but they can get the pointer to it
>> from drvdata.
> 
> Yup, that's exactly what I did.
> 
>> Maybe that should be part of this patch. I assume you'll take it over - do you
>> want to make that change or should I send another version ? Handling it in a
>> separate patch is fine with me as well if you prefer to do that.
> 
> Since I already have that patch on top, I suggest to keep it
> incremental. Also easier to debug if something goes wrong etc...
> 

SGTM. I'll let you handle the series from here.

Thanks,
Guenter

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Guenter Roeck]

On 12/18/24 08:25, Wolfram Sang wrote:
> 
>> Ah yes, 'client' can be passed as bus context, and isn't used elsewhere.
>> The access functions need lm75_data, but they can get the pointer to it
>> from drvdata.
> 
> Yup, that's exactly what I did.
> 
>> Maybe that should be part of this patch. I assume you'll take it over - do you
>> want to make that change or should I send another version ? Handling it in a
>> separate patch is fine with me as well if you prefer to do that.
> 
> Since I already have that patch on top, I suggest to keep it
> incremental. Also easier to debug if something goes wrong etc...
> 

All that makes me wonder: Do you by any chance have a means to test I3C support
on SPD5118 chips ? I have a patch series for that, but I can't test it so I
did not bother submitting it.

Thanks,
Guenter

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]


> All that makes me wonder: Do you by any chance have a means to test I3C support
> on SPD5118 chips ? I have a patch series for that, but I can't test it so I
> did not bother submitting it.

Sadly not. Despite Renesas being big with SPD5118, I don't have any
here and I don't see that I'll get one.

I do see, though, that Jarkko Nikula from Intel is active on the I3C
list these days. I could imagine he has some I3C controllers connected
to DDR5 modules?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Guenter Roeck]

On 12/18/24 09:39, Wolfram Sang wrote:
> 
>> All that makes me wonder: Do you by any chance have a means to test I3C support
>> on SPD5118 chips ? I have a patch series for that, but I can't test it so I
>> did not bother submitting it.
> 
> Sadly not. Despite Renesas being big with SPD5118, I don't have any
> here and I don't see that I'll get one.
> 

Renesas did send me a samples, actually, and I can build a test board with it,
but I don't have an I3C controller to connect them to. The ones I found are
either out of reach financially, or I'd have to find and program a microcontroller
myself.

> I do see, though, that Jarkko Nikula from Intel is active on the I3C
> list these days. I could imagine he has some I3C controllers connected
> to DDR5 modules?
> 

Good idea. I'll ask on the I3C list.

Thanks,
Guenter

Re: [RFC/RFT PATCH v2] hwmon: (lm75) Hide register size differences in regmap access functions

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

On Wed, Dec 18, 2024 at 07:04:04AM -0800, Guenter Roeck wrote:
> Hide register size differences in regmap access functions to simplify
> runtime code and to simplify adding support for I3C devices. Also
> use regmap API functions for bit operations where possible.
> 
> For this to work, the 16-bit and 8-bit configuration register has to be
> mapped to a 16-bit value. Unlike other registers, this register is a
> low-byte-first register, presumably for compatibility with chips with
> 8-bit wide configuration registers. Hide the differences in the regmap
> access code.
> 
> While at it, enable alarm attribute support for TMP112.
> 
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]