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

On 4/29/26 11:35, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
> 

This is most definitely wrong. Please fix. Just remove that check.
It is unnecessary.

> 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.
> 

I agree with Sashiko; I don't immediately see why it would make sense to check
for bit 2..5 instead of bit 1 .. 4.

FWIW, it might be easier to program this in a loop and declare a static array
for the registers, similar to how it is done for the other registers.

> [ ... ]
> 
>> +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?
> 

The code is a bit inconsistent in this regard: Sometimes it uses int,
sometimes unsigned int. Please use unsigned int consistently.

Thanks,
Guenter