On Fri, Aug 25, 2023 at 06:44:56AM +0000, Biju Das wrote:
> 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"
>
"replacing tmp51x_ids->max_channels" is a bit difficult to read.
> >
> > > 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
Not really, because it is the number of channels and well known.
We should not optimize the code for something that won't ever happen.
The number of channels is 3 or 4, and the generated mask needs to be
0x3800 or 0x7800 depending on the chip. We could maybe have something
like
#define CHANNEL_MASK(channels) (...)
and or in the other bits separately.
Anyway, maybe "a" is not the best variable name. Maybe use "channels"
or "n".
> > 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))
> >
Probably better to split out resistance correction (bit 10)
and conversion rate (bit 7..9) instead of a single mask.
Just for completeness - bit 15 is "continuous conversion".
Bit 3..6 are unused, and bit 0..2 are used to configure
the gpio pin which is currently not supported by the driver.
> > ?
>
> 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.
>
https://www.ti.com/lit/gpn/tmp513
Configuration register 2 is documented on page 35 and 36.
If you change this, please consider using defines for the
various 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.
>
> 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.
>
There is only one field available in struct i2c_device_id.
Splitting this patch in 2 seems overkill because the first patch
would have to introduce code (set max_channels based on
enum tmp51x_ids) only to remove it in the second patch.
> >
> > 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.
>
"doesn't make sense" is a bit strong, but I agree that the code
is unnecessarily complex. And, yes, that would be a separate 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.
>
Not sure I understand. The above lines _are_ being removed
(- in 1st column). What else is there to remove ?
Thanks,
Guenter
> Cheers,
> Biju
>
> >
> > > + data->temp_config = TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels);
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
>
