Hi Guenter Roeck,
Thanks for the feedback.
> Subject: Re: [PATCH v2 3/3] hwmon: tmp513: Replace tmp51x_ids->max_channels
> in struct tmp51x_data
>
> 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.
Agreed.
>
> > >
> > > > 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".
OK, will 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.
Thanks for the info.
>
> > > ?
> >
> > 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.
> >
>
> Configuration register 2 is documented on page 35 and 36.
>
> If you change this, please consider using defines for the various bits.
OK, will do.
>
> > >
> > > ...
> > >
> > > > 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.
The plan was,
First patch will replace id->max_channels in the table
and will fill the id based on max_channels and fix the logic for invalid channels.
The Second patch is to remove id altogether.
I am ok with doing it in the same patch. Please let me know.
>
> > >
> > > 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.
My bad English. Sorry for that.
Cheers,
Biju
>
> > 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
> > >
> >
