diff mbox series

[RFC,5/5] hwmon: (lm75) add I3C support for P3T1755

Message ID 20241219225522.3490-12-wsa+renesas@sang-engineering.com (mailing list archive)
State Superseded
Headers show
Series hwmon: (lm75) add I3C support | expand

Commit Message

Wolfram Sang Dec. 19, 2024, 10:55 p.m. UTC
Introduce I3C support by defining I3C accessors for regmap and
implementing an I3C driver. Enable I3C for the NXP P3T1755.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Two things I would like to discuss:

- Shall we reuse i2c_device_id's to get a proper name for the chip?
  Looks strange, but adding all the info again for I3C looks strange as
  well

- are there some suitable kernel helpers for dealing with big/little
  endianess in lm75_i3c_regmap_bus? Note: I also tried to use the
  non-*_reg callbacks for the regmap bus. But the constified
  void-pointers make it ugly as we need to change buffers because some
  registers are big endian and some little endian.

 drivers/hwmon/Kconfig |   2 +
 drivers/hwmon/lm75.c  | 114 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 1 deletion(-)

Comments

Guenter Roeck Dec. 20, 2024, 6:10 a.m. UTC | #1
Hi Wolfram,

On 12/19/24 14:55, Wolfram Sang wrote:
> Introduce I3C support by defining I3C accessors for regmap and
> implementing an I3C driver. Enable I3C for the NXP P3T1755.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Two things I would like to discuss:
> 
> - Shall we reuse i2c_device_id's to get a proper name for the chip?
>    Looks strange, but adding all the info again for I3C looks strange as
>    well
> 

I don't think there will be many i3c devices supporting LM75 compatible chips.
For the few i3c chips which do happen do be lm75 compatible, we should have
something like

struct lm75_i3c_devices {
	enum lm75_type type;
	const char *name;
};

with an instance for each i3c device, and then point i3c_device_id->data to it.
And I think "lm75compatible" is really a terrible hwmon device name ;-).

FWIW, it is too bad that i3c_device_id doesn't have a "name" field. That would
really come handy here.

An alternative would be to just use dev_name(i3cdev_to_dev(i3cdev)),
but that would not reflect the chip name and thus be less than perfect.

> - are there some suitable kernel helpers for dealing with big/little
>    endianess in lm75_i3c_regmap_bus? Note: I also tried to use the
>    non-*_reg callbacks for the regmap bus. But the constified
>    void-pointers make it ugly as we need to change buffers because some
>    registers are big endian and some little endian.
> 

i3c doesn't seem to have any access functions (kernel helpers) similar to i2c,
other than i3c_device_do_priv_xfers(), so unless those are made available
I think we'll have to bite the bullet and use local access functions.

The other patches look good to me. If you send me a Reviewed-by: and/or
Tested-by: to my patch, I'll queue it all up (except for this patch)
for 6.14.

Thanks,
Guenter
Wolfram Sang Dec. 20, 2024, 8:20 a.m. UTC | #2
Hi Guenter,

> For the few i3c chips which do happen do be lm75 compatible, we should have
> something like
> 
> struct lm75_i3c_devices {
> 	enum lm75_type type;
> 	const char *name;
> };

Sigh, OK, let's have that info a third time.

> And I think "lm75compatible" is really a terrible hwmon device name ;-).

The series is RFC for a reason :D

> i3c doesn't seem to have any access functions (kernel helpers) similar to i2c,
> other than i3c_device_do_priv_xfers(), so unless those are made available
> I think we'll have to bite the bullet and use local access functions.

Well, yes, there are not many I3C target drivers, so there is no
critical mass for I3C helpers yet. With helpers I meant more convesion
helpers like cpu_to_be16 and the likes. I see them rarely used in hwmon,
so I wondered about this.

> The other patches look good to me. If you send me a Reviewed-by: and/or
> Tested-by: to my patch, I'll queue it all up (except for this patch)
> for 6.14.

Done. Thank you!

All the best,

   Wolfram
diff mbox series

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index dd376602f3f1..86897b4d105f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1412,7 +1412,9 @@  config SENSORS_LM73
 config SENSORS_LM75
 	tristate "National Semiconductor LM75 and compatibles"
 	depends on I2C
+	depends on I3C || !I3C
 	select REGMAP_I2C
+	select REGMAP_I3C if I3C
 	help
 	  If you say yes here you get support for one common type of
 	  temperature sensor chip, with models including:
diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 8b4f324524da..2979f1746585 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -11,6 +11,7 @@ 
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 #include <linux/i2c.h>
+#include <linux/i3c/device.h>
 #include <linux/hwmon.h>
 #include <linux/err.h>
 #include <linux/of.h>
@@ -112,6 +113,8 @@  struct lm75_data {
 	unsigned int			sample_time;	/* In ms */
 	enum lm75_type			kind;
 	const struct lm75_params	*params;
+	u8				reg_buf[1];
+	u8				val_buf[3];
 };
 
 /*-----------------------------------------------------------------------*/
@@ -606,6 +609,77 @@  static const struct regmap_bus lm75_i2c_regmap_bus = {
 	.reg_write = lm75_i2c_reg_write,
 };
 
+static int lm75_i3c_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct i3c_device *i3cdev = context;
+	struct lm75_data *data = i3cdev_get_drvdata(i3cdev);
+	struct i3c_priv_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = 1,
+			.data.out = data->reg_buf,
+		},
+		{
+			.rnw = true,
+			.len = 2,
+			.data.out = data->val_buf,
+		},
+	};
+	int ret;
+
+	data->reg_buf[0] = reg;
+
+	if (reg == LM75_REG_CONF && !data->params->config_reg_16bits)
+		xfers[1].len--;
+
+	ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
+	if (ret < 0)
+		return ret;
+
+	if (reg == LM75_REG_CONF && !data->params->config_reg_16bits)
+		*val = data->val_buf[0];
+	else if (reg == LM75_REG_CONF)
+		*val = data->val_buf[0] | (data->val_buf[1] << 8);
+	else
+		*val = data->val_buf[1] | (data->val_buf[0] << 8);
+
+	return 0;
+}
+
+static int lm75_i3c_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct i3c_device *i3cdev = context;
+	struct lm75_data *data = i3cdev_get_drvdata(i3cdev);
+	struct i3c_priv_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = 3,
+			.data.out = data->val_buf,
+		},
+	};
+
+	data->val_buf[0] = reg;
+
+	if (reg == PCT2075_REG_IDLE ||
+	    (reg == LM75_REG_CONF && !data->params->config_reg_16bits)) {
+		xfers[0].len--;
+		data->val_buf[1] = val & 0xff;
+	} else if (reg == LM75_REG_CONF) {
+		data->val_buf[1] = val & 0xff;
+		data->val_buf[2] = (val >> 8) & 0xff;
+	} else {
+		data->val_buf[2] = val & 0xff;
+		data->val_buf[1] = (val >> 8) & 0xff;
+	}
+
+	return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
+}
+
+static const struct regmap_bus lm75_i3c_regmap_bus = {
+	.reg_read = lm75_i3c_reg_read,
+	.reg_write = lm75_i3c_reg_write,
+};
+
 static const struct regmap_config lm75_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -750,6 +824,36 @@  static const struct i2c_device_id lm75_i2c_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, lm75_i2c_ids);
 
+static const struct i3c_device_id lm75_i3c_ids[] = {
+	I3C_DEVICE(0x011b, 0x152a, (void *)p3t1755),
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i3c, lm75_i3c_ids);
+
+static int lm75_i3c_probe(struct i3c_device *i3cdev)
+{
+	struct device *dev = i3cdev_to_dev(i3cdev);
+	const struct i3c_device_id *id;
+	struct regmap *regmap;
+	const char *name;
+	unsigned int i;
+
+	regmap = devm_regmap_init(dev, &lm75_i3c_regmap_bus, i3cdev, &lm75_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	id = i3c_device_match_id(i3cdev, lm75_i3c_ids);
+
+	/* Reuse i2c_device_ids is OK? Or do we want a third copy of this data in lm75_i3c_ids? */
+	for (i = 0; lm75_i2c_ids[i].name[0]; i++)
+		if ((kernel_ulong_t)id->data == lm75_i2c_ids[i].driver_data)
+			break;
+
+	name = lm75_i2c_ids[i].name[0] ? lm75_i2c_ids[i].name : "lm75compatible";
+
+	return lm75_generic_probe(dev, name, id->data, 0, regmap);
+}
+
 static const struct of_device_id __maybe_unused lm75_of_match[] = {
 	{
 		.compatible = "adi,adt75",
@@ -1008,7 +1112,15 @@  static struct i2c_driver lm75_i2c_driver = {
 	.address_list	= normal_i2c,
 };
 
-module_i2c_driver(lm75_i2c_driver);
+static struct i3c_driver lm75_i3c_driver = {
+	.driver = {
+		.name = "lm75_i3c",
+	},
+	.probe = lm75_i3c_probe,
+	.id_table = lm75_i3c_ids,
+};
+
+module_i3c_i2c_driver(lm75_i3c_driver, &lm75_i2c_driver)
 
 MODULE_AUTHOR("Frodo Looijaard <frodol@dds.nl>");
 MODULE_DESCRIPTION("LM75 driver");