Re: [PATCH v2 2/4] hwmon: tmp108: Add help function tmp108_common_probe()

[Guenter Roeck <linux@xxxxxxxxxxxx>]

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@xxxxxxx>
> ---
> 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", },
>   	{}
>   };