On Thu, Aug 24, 2023 at 09:44:56PM +0100, Biju Das wrote:
> The tmp512 chip has 3 channels whereas tmp513 has 4 channels. Avoid
> using tmp51x_ids for this HW difference by replacing
> tmp51x_ids->max_channels in struct tmp51x_data and drop
You don't replace it, you replaced "id" by it.
> enum tmp51x_ids as there is no user.
...
> +#define TMP51X_TEMP_CONFIG_DEFAULT(a) (0x8780 | GENMASK(11 + (a) - 1, 11))
This seems fragile ("a" can't be 0, and can't be > 4) and will give not good
code generation (for GENMASK() use), besides it has 0x8780 magic. What is that?
Two bit field masks?
(BIT(15) | (GENMASK((a) - 1, 0) << 11) | GENMASK(10, 7))
?
Also add a comment to explain "a" and other bits.
...
> case hwmon_temp:
> - if (data->id == tmp512 && channel == 3)
> + if (data->max_channels == channel)
This is not the same as it was previously. And looks like this kind of fix
(if I understood the previous patch correctly) should be done there.
Btw, avoid Yoda style
if (channel == data->max_channels)
> return 0;
...
> ret = device_property_read_u32_array(dev, "ti,nfactor", nfactor,
> - (data->id == tmp513) ? 3 : 2);
> + data->max_channels - 1);
> if (ret >= 0)
> - memcpy(data->nfactor, nfactor, (data->id == tmp513) ? 3 : 2);
> + memcpy(data->nfactor, nfactor, data->max_channels - 1);
It looks like data->nfactor is of the same type as nfactor, right?
Why this can't be simplified to just
device_property_read_u32_array(dev, "ti,nfactor",
data->nfactor, data->max_channels - 1);
...
> - data->temp_config = (data->id == tmp513) ?
> - TMP513_TEMP_CONFIG_DEFAULT : TMP512_TEMP_CONFIG_DEFAULT;
Are those still being in use?
> + data->temp_config = TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels);
--
With Best Regards,
Andy Shevchenko
