linux-kernel - Re: [PATCH v2 2/4] hwmon: tmp108: Add help function tmp108_common

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c1b77ef0-f01b-4926-8259-e387166d0750@roeck-us.net>
Date: Fri, 8 Nov 2024 15:55:40 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Frank Li <Frank.Li@....com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Jean Delvare <jdelvare@...e.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, Krzysztof Kozlowski <krzk@...nel.org>,
 linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2 2/4] hwmon: tmp108: Add help function
 tmp108_common_probe()

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