Hi Günter,
On Fri, Aug 25, 2023 at 9:36 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On Fri, Aug 25, 2023 at 06:44:56AM +0000, Biju Das wrote:
> > > 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".
> > > > - 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 ?
The actual TMP51*TEMP_CONFIG_DEFAULT definitions are now unused.
Alternatively, rename them to TEMP_CONFIG_CH3 and TEMP_CONFIG_CH4,
and switch (data->max_channels) { ... }?
Sounds a bit silly, though, as we do have the formula to
generate the temp_config from the number of channels...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
