| Message ID | 20241108-p3t1085-v2-2-6a8990a59efd@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: temperature: Add support for P3T1085 | expand |
On 11/8/24 14:26, Frank Li wrote: > Add help function tmp108_common_probe() to pave road to support i3c for help -> helper > P3T1085(NXP) chip. > > Using dev_err_probe() simple code. Use dev_err_probe() to simplify the code. > > Add compatible string "nxp,p3t1085". > This is borderline and problematic. First, it is the one functional change, and second, that functional change is not mentioned in the subject. At the very least it needs to be mentioned in the subject. I would, however, prefer two separate patches, even if that is just a one-liner. Also, the key change is preparation for i3c support, not that a helper function is added. The subject should be something like "Prepare for adding I3C support", and the description should then mention the added helper function. > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > dev_err_probe() have not involve addition diff change. The difference > always list these code block change regardless use dev_err_probe(). > --- > drivers/hwmon/tmp108.c | 40 ++++++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c > index a82bbc959eb15..bfbea6349a95f 100644 > --- a/drivers/hwmon/tmp108.c > +++ b/drivers/hwmon/tmp108.c > @@ -323,33 +323,19 @@ static const struct regmap_config tmp108_regmap_config = { > .use_single_write = true, > }; > > -static int tmp108_probe(struct i2c_client *client) > +static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name) > { > - struct device *dev = &client->dev; > struct device *hwmon_dev; > struct tmp108 *tmp108; > - int err; > u32 config; > - > - if (!i2c_check_functionality(client->adapter, > - I2C_FUNC_SMBUS_WORD_DATA)) { > - dev_err(dev, > - "adapter doesn't support SMBus word transactions\n"); > - return -ENODEV; > - } > + int err; > > tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL); > if (!tmp108) > return -ENOMEM; > > dev_set_drvdata(dev, tmp108); > - > - tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config); > - if (IS_ERR(tmp108->regmap)) { > - err = PTR_ERR(tmp108->regmap); > - dev_err(dev, "regmap init failed: %d", err); > - return err; > - } > + tmp108->regmap = regmap; > > err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config); > if (err < 0) { > @@ -383,13 +369,30 @@ static int tmp108_probe(struct i2c_client *client) > return err; > } > > - hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, > + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, > tmp108, > &tmp108_chip_info, > NULL); > return PTR_ERR_OR_ZERO(hwmon_dev); > } > > +static int tmp108_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct regmap *regmap; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WORD_DATA)) > + return dev_err_probe(dev, -ENODEV, > + "adapter doesn't support SMBus word transactions\n"); > + > + regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config); > + if (IS_ERR(regmap)) > + return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed"); > + > + return tmp108_common_probe(dev, regmap, client->name); > +} > + > static int tmp108_suspend(struct device *dev) > { > struct tmp108 *tmp108 = dev_get_drvdata(dev); > @@ -420,6 +423,7 @@ MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids); > > #ifdef CONFIG_OF It might also make sense to get rid of this conditional and of the of_match_ptr() below to enable instantiation through ACPI. That should be a separate patch, though. Thanks, Guenter > static const struct of_device_id tmp108_of_ids[] = { > + { .compatible = "nxp,p3t1085", }, > { .compatible = "ti,tmp108", }, > {} > }; >
On 11/8/24 15:55, Guenter Roeck wrote: > On 11/8/24 14:26, Frank Li wrote: >> Add help function tmp108_common_probe() to pave road to support i3c for > > help -> helper > >> P3T1085(NXP) chip. >> >> Using dev_err_probe() simple code. > > Use dev_err_probe() to simplify the code. > >> >> Add compatible string "nxp,p3t1085". >> > > This is borderline and problematic. First, it is the one functional change, > and second, that functional change is not mentioned in the subject. At the very > least it needs to be mentioned in the subject. I would, however, prefer two > separate patches, even if that is just a one-liner. > I forgot: The additional chip support should also be mentioned in Kconfig and in Documentation/hwmon/tmp108.rst. Thanks, Guenter
diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c index a82bbc959eb15..bfbea6349a95f 100644 --- a/drivers/hwmon/tmp108.c +++ b/drivers/hwmon/tmp108.c @@ -323,33 +323,19 @@ static const struct regmap_config tmp108_regmap_config = { .use_single_write = true, }; -static int tmp108_probe(struct i2c_client *client) +static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name) { - struct device *dev = &client->dev; struct device *hwmon_dev; struct tmp108 *tmp108; - int err; u32 config; - - if (!i2c_check_functionality(client->adapter, - I2C_FUNC_SMBUS_WORD_DATA)) { - dev_err(dev, - "adapter doesn't support SMBus word transactions\n"); - return -ENODEV; - } + int err; tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL); if (!tmp108) return -ENOMEM; dev_set_drvdata(dev, tmp108); - - tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config); - if (IS_ERR(tmp108->regmap)) { - err = PTR_ERR(tmp108->regmap); - dev_err(dev, "regmap init failed: %d", err); - return err; - } + tmp108->regmap = regmap; err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config); if (err < 0) { @@ -383,13 +369,30 @@ static int tmp108_probe(struct i2c_client *client) return err; } - hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, + hwmon_dev = devm_hwmon_device_register_with_info(dev, name, tmp108, &tmp108_chip_info, NULL); return PTR_ERR_OR_ZERO(hwmon_dev); } +static int tmp108_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct regmap *regmap; + + if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_WORD_DATA)) + return dev_err_probe(dev, -ENODEV, + "adapter doesn't support SMBus word transactions\n"); + + regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config); + if (IS_ERR(regmap)) + return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed"); + + return tmp108_common_probe(dev, regmap, client->name); +} + static int tmp108_suspend(struct device *dev) { struct tmp108 *tmp108 = dev_get_drvdata(dev); @@ -420,6 +423,7 @@ MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids); #ifdef CONFIG_OF static const struct of_device_id tmp108_of_ids[] = { + { .compatible = "nxp,p3t1085", }, { .compatible = "ti,tmp108", }, {} };
Add help function tmp108_common_probe() to pave road to support i3c for P3T1085(NXP) chip. Using dev_err_probe() simple code. Add compatible string "nxp,p3t1085". Signed-off-by: Frank Li <Frank.Li@nxp.com> --- dev_err_probe() have not involve addition diff change. The difference always list these code block change regardless use dev_err_probe(). --- drivers/hwmon/tmp108.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)