Hi Andy Shevchenko,
Thanks for the feedback.
> Subject: Re: [PATCH v2 3/3] hwmon: tmp513: Replace tmp51x_ids->max_channels
> in struct tmp51x_data
>
> 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.
You are correct "id->max_channels"
>
> > 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))
>
> ?
Looks good to me.
>
> Also add a comment to explain "a" and other bits.
I don't have access to user manual to explain these bits.
May be Guenter/Eric can help on this.
>
> ...
>
> > 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.
Logic wise it checks for invalid channels. But newer logic
check for invalid channels for both tmp512 and tmp513.
compared to only for tmp512 in previous case. For me it looks
like an improvement.
You mean split this into 2. First patch with just this logic (channel >= data->max_channels) and keep data->id in remaining
Places and Second patch is to replace the id and use max_channels instead.
>
> Btw, avoid Yoda style
>
> if (channel == data->max_channels)
OK, it should be (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);
Yes, I think you are correct. The below code doesn't makes sense. I guess this should be another patch.
if (ret >= 0)
memcpy(data->nfactor, 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?
Nope. Will remove it.
Cheers,
Biju
>
> > + data->temp_config = TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels);
>
> --
> With Best Regards,
> Andy Shevchenko
>
