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

[RFC/RFT PATCH] 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.

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
This should help with adding I3C support.
Module tested only.

 drivers/hwmon/lm75.c | 99 ++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 55 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 2c2205aec7d4..23b41e5a5b47 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -343,12 +343,8 @@ static int lm75_write_config(struct lm75_data *data, u16 set_mask,
 
 	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);
+
+		err = regmap_write(data->regmap, LM75_REG_CONF, value);
 		if (err)
 			return err;
 		data->current_conf = value;
@@ -356,19 +352,6 @@ static int lm75_write_config(struct lm75_data *data, u16 set_mask,
 	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;
@@ -469,7 +452,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 +471,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,
+					 0x00c0, (3 - index) << 6);
 		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 +575,33 @@ 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 && !data->params->config_reg_16bits)
+		ret = i2c_smbus_read_byte_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);
+	return i2c_smbus_write_word_swapped(client, reg, val);
+}
+
 static const struct regmap_config lm75_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -608,6 +612,8 @@ static const struct regmap_config lm75_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 	.use_single_read = true,
 	.use_single_write = true,
+	.reg_read = lm75_i2c_reg_read,
+	.reg_write = lm75_i2c_reg_write,
 };
 
 static void lm75_disable_regulator(void *data)
@@ -620,9 +626,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 +645,8 @@ static int lm75_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	dev_set_drvdata(dev, data);
+
 	data->client = client;
 	data->kind = (uintptr_t)i2c_get_match_data(client);
 
@@ -673,11 +680,9 @@ 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;
 
@@ -972,32 +977,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] hwmon: (lm75) Hide register size differences in regmap access functions

[Wolfram Sang]

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

On Tue, Dec 17, 2024 at 02:52:10PM -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.
> 
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> This should help with adding I3C support.
> Module tested only.

Does that mean 'build tested as a module only'?

With the following small patch on top, it works \o/ I suggest that I
will include your patch in my series for adding I3C support. I have a
few patches on top already. I think this makes dependency handling a bit
easier?


diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index f2550f623bee..1ef47ba6b458 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -653,6 +653,7 @@ static int lm75_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	/* Set this early. Our custom regmap callbacks need it */
 	dev_set_drvdata(dev, data);
 
 	data->client = client;
@@ -662,7 +663,7 @@ 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(&client->dev, NULL, data, &lm75_regmap_config);
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);



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

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

[Guenter Roeck]

On 12/18/24 04:46, Wolfram Sang wrote:
> On Tue, Dec 17, 2024 at 02:52:10PM -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.
>>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> This should help with adding I3C support.
>> Module tested only.
> 
> Does that mean 'build tested as a module only'?
> 

No, it means tested by loading as module, simulating a LM75 using the SMBus
stub driver, and running a script on it testing the various attributes.
The scripts I use are at git@github.com:groeck/module-tests.git.

> With the following small patch on top, it works \o/ I suggest that I
> will include your patch in my series for adding I3C support. I have a
> few patches on top already. I think this makes dependency handling a bit
> easier?
> 

Makes sense. It needs more changes for chips with 16-bit addresses, though.
I'll send another version.

> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f2550f623bee..1ef47ba6b458 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -653,6 +653,7 @@ static int lm75_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	/* Set this early. Our custom regmap callbacks need it */
>   	dev_set_drvdata(dev, data);
>   
>   	data->client = client;
> @@ -662,7 +663,7 @@ 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(&client->dev, NULL, data, &lm75_regmap_config);

If  devm_regmap_init_i2c() doesn't work, it would be better to define a
regmap_bus and use it to access the registers. This way the actual regmap
configuration would be the same for i2c and i3c, and only the regmap bus
would be different (or at least I hope so). I'll do that in the next version
of the patch.

Thanks,
Guenter