Re: [PATCH v2 1/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/01/2026 07:57, Mayank Mahajan wrote:

> @@ -369,7 +486,7 @@ static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *
> 
>  	err = devm_add_action_or_reset(dev, tmp108_restore_config, tmp108);
>  	if (err) {
> -		dev_err(dev, "add action or reset failed: %d", err);
> +		dev_err_probe(dev, err, "Add action or reset failed");

How is this relevant to new device? Do not mix independent changes into
one commit. Please carefully read submitting patches, although this is
generic programming practice and nothing specific to kernel.

>  		return err;
>  	}
> 
> @@ -384,17 +501,34 @@ static int tmp108_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct regmap *regmap;
> +	enum tmp108_hw_id hw_id;
> +	const void *of_data;
> 
>  	if (!i2c_check_functionality(client->adapter,
> -				     I2C_FUNC_SMBUS_WORD_DATA))
> -		return dev_err_probe(dev, -ENODEV,
> +				     I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> +		return dev_err_probe(dev, -EOPNOTSUPP,
>  				     "adapter doesn't support SMBus word transactions\n");

I do not see how changing error code is related/relevant to new device
support...

> 
> -	regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
> +	regmap = devm_regmap_init(dev, &tmp108_i2c_regmap_bus, 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);
> +	/* Prefer OF match data (DT-first systems) */
> +	of_data = device_get_match_data(&client->dev);
> +	if (of_data) {
> +		hw_id = (enum tmp108_hw_id)(uintptr_t)of_data;

Completely mixed up cases. First, do not use uintptr_t. Second, last
enum cast is not needed. You only need cast via unsigned long.

> +	} else {
> +		/* Fall back to legacy I2C ID table */
> +		const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +
> +		if (!id) {
> +			dev_err_probe(dev, -ENODEV, "No matching device ID for i2c device\n");
> +			return -ENODEV;

Syntax is return dev_err_probe. Look above - just few lines above you
have it right, so you are you introducing different syntax?

> +		}
> +		hw_id = (enum tmp108_hw_id)id->driver_data;

And this should cause build warnings on W=1.

Are you sure you build tested it with different compilers, with W=1, for
32 and 64 bit platforms?

> +	}
> +
> +	return tmp108_common_probe(dev, regmap, client->name, hw_id);
>  }
> 


> 
>  static const struct of_device_id tmp108_of_ids[] = {
> -	{ .compatible = "nxp,p3t1085", },
> -	{ .compatible = "ti,tmp108", },
> -	{}
> +	{ .compatible = "nxp,p3t1035", .data = (void *)(uintptr_t)P3T1035_ID },
> +	{ .compatible = "nxp,p3t1085", .data = (void *)(uintptr_t)P3T1085_ID },
> +	{ .compatible = "nxp,p3t2030", .data = (void *)(uintptr_t)P3T1035_ID },

So devices are compatible? If so, drop this and express it in the bindings.

> +	{ .compatible = "ti,tmp108", .data = (void *)(uintptr_t)TMP108_ID },
> +	{ /* sentinel */ },

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46



Best regards,
Krzysztof