Re: [PATCH v10 2/2] hwmon: temperature: add support for EMC1812

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] `emc1812_is_visible()` returns `-EOPNOTSUPP` instead of `0` to hide unsupported channels, exposing them with garbage sysfs permissions.
- [Medium] Incorrect bit indices are used when checking the active channels to configure the ideality factors, leading to an off-by-one error.
- [Low] Variables of type `int` are passed to `regmap_read()`, which expects an `unsigned int *`, causing `-Wpointer-sign` compiler warnings.
--

commit 334894f6ed5a53fe9f4a36ce3c196060e9967729
Author: Marius Cristea <marius.cristea@microchip.com>

hwmon: temperature: add support for EMC1812

This commit introduces the hardware monitoring driver for the Microchip
EMC1812/13/14/15/33 family of remote diode temperature sensors.

> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> index 000000000000..94e2709a566f
> --- /dev/null
> +++ b/drivers/hwmon/emc1812.c

[ ... ]

> +static umode_t emc1812_is_visible(const void *_data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	const struct emc1812_data *data = _data;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		/* Don't show channels which are not physically available */
> +		if (channel >= data->chip->phys_channels)
> +			return -EOPNOTSUPP;

Does this correctly hide the unsupported channels?

The hwmon core interprets the returned umode_t value as file permissions.
Returning a negative error code like -EOPNOTSUPP results in a large
positive non-zero value, which unintentionally creates sysfs attributes
with malformed permissions instead of hiding them. It seems returning 0
would properly hide the attributes.

[ ... ]

> +static int emc1812_read_reg(struct device *dev, struct emc1812_data *data, u32 attr,
> +			    int channel, long *val)
> +{
> +	int hyst, ret;
> +
> +	switch (attr) {

[ ... ]

> +	case hwmon_temp_max_hyst:
> +		ret = emc1812_get_limit_temp(data, channel, temp_max, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read(data->regmap, EMC1812_THRM_HYS_ADDR, &hyst);

Does this trigger a -Wpointer-sign compiler warning? The regmap_read API
expects an unsigned int pointer, but the hyst variable is declared as a signed
int.

[ ... ]

> +static int emc1812_set_hyst(struct emc1812_data *data, int channel, int val)
> +{
> +	int hyst, ret;
> +	int limit;
> +
> +	/* Critical register is 8bits long and keeps only integer part of temperature */
> +	ret = regmap_read(data->regmap, emc1812_temp_crit_regs[channel], &limit);

Similar to the above, limit is declared as a signed int. Does passing it
here cause a pointer signedness mismatch warning?

[ ... ]

> +static int emc1812_init(struct emc1812_data *priv)
> +{
> +	int i, ret;
> +	u8 val;

[ ... ]

> +	/* Set the ideality factor only for the channels enabled on the chip */
> +	if (test_bit(2, &priv->active_ch_mask)) {
> +		ret = regmap_write(priv->regmap, EMC1812_EXT1_IDEALITY_FACTOR_ADDR,
> +				   EMC1812_DEFAULT_IDEALITY_FACTOR);

Since channel 0 corresponds to the internal diode, shouldn't EXT1 correspond
to bit 1, EXT2 to bit 2, and so on?

If so, checking bit 2 for EXT1 (and continuing up to bit 5 for EXT4) will result
in an off-by-one misalignment. This means EXT1 won't be configured if only
channel 1 is active, and the out-of-bounds bit 5 will be checked for EXT4 since
the bitmask has a maximum of 5 channels.

[ ... ]

> +static int emc1812_chip_identify(struct emc1812_data *data, struct i2c_client *client)
> +{
> +	const struct emc1812_features *chip;
> +	struct device *dev = &client->dev;
> +	int ret, tmp;
> +
> +	ret = regmap_read(data->regmap, EMC1812_PRODUCT_ID_ADDR, &tmp);

This also passes a signed int pointer to regmap_read. Can tmp be declared
as an unsigned int to avoid compiler warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429-hw_mon-emc1812-v10-0-a8ca1d779502@microchip.com?part=2