Hi Guenter Roeck,
Thanks for the feedback.
> Subject: Re: [PATCH v3 1/3] hwmon: tmp513: Add max_channels variable to
> struct tmp51x_data
>
> On Fri, Aug 25, 2023 at 09:53:43PM +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 OF/ID table data
> > with maximum channels supported by the device.
> >
> > Add max_channels variable to struct tmp51x_data and fix the logic for
> > invalid channel in tmp51x_is_visible().
>
> This patch does not fix anything. The existing logic in tmp51x_is_visible()
> is perfectly fine.
I agree. It is not a fix, but it is an improvement.
Earlier logic checks invalid channels for only tmp512,
whereas the new logic checks invalid channels for both
tmp512 and tmp513.
Cheers,
Biju
>
> Guenter
>
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v3:
> > * New patch split from patch #3
> > * Avoided Yoda style logic.
> > * Replaced OF/ID data from tmp51x_ids->max_channels
> > ---
> > drivers/hwmon/tmp513.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c index
> > 9a180b1030c9..99f66f9d5f19 100644
> > --- a/drivers/hwmon/tmp513.c
> > +++ b/drivers/hwmon/tmp513.c
> > @@ -113,6 +113,9 @@
> >
> > #define MAX_TEMP_HYST 127500
> >
> > +#define TMP512_MAX_CHANNELS 3
> > +#define TMP513_MAX_CHANNELS 4
> > +
> > static const u8 TMP51X_TEMP_INPUT[4] = {
> > TMP51X_LOCAL_TEMP_RESULT,
> > TMP51X_REMOTE_TEMP_RESULT_1,
> > @@ -170,6 +173,7 @@ struct tmp51x_data {
> > u32 pwr_lsb_uw;
> >
> > enum tmp51x_ids id;
> > + u8 max_channels;
> > struct regmap *regmap;
> > };
> >
> > @@ -434,7 +438,7 @@ static umode_t tmp51x_is_visible(const void
> > *_data,
> >
> > switch (type) {
> > case hwmon_temp:
> > - if (data->id == tmp512 && channel == 3)
> > + if (channel >= data->max_channels)
> > return 0;
> > switch (attr) {
> > case hwmon_temp_input:
> > @@ -601,21 +605,15 @@ static int tmp51x_init(struct tmp51x_data *data)
> > }
> >
> > static const struct i2c_device_id tmp51x_id[] = {
> > - { "tmp512", tmp512 },
> > - { "tmp513", tmp513 },
> > + { "tmp512", TMP512_MAX_CHANNELS },
> > + { "tmp513", TMP513_MAX_CHANNELS },
> > { }
> > };
> > MODULE_DEVICE_TABLE(i2c, tmp51x_id);
> >
> > static const struct of_device_id tmp51x_of_match[] = {
> > - {
> > - .compatible = "ti,tmp512",
> > - .data = (void *)tmp512
> > - },
> > - {
> > - .compatible = "ti,tmp513",
> > - .data = (void *)tmp513
> > - },
> > + { .compatible = "ti,tmp512", .data = (void *)TMP512_MAX_CHANNELS },
> > + { .compatible = "ti,tmp513", .data = (void *)TMP513_MAX_CHANNELS },
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, tmp51x_of_match); @@ -720,7 +718,11 @@ static
> > int tmp51x_probe(struct i2c_client *client)
> > if (!data)
> > return -ENOMEM;
> >
> > - data->id = (uintptr_t)i2c_get_match_data(client);
> > + data->max_channels = (uintptr_t)i2c_get_match_data(client);
> > + if (data->max_channels == TMP513_MAX_CHANNELS)
> > + data->id = tmp513;
> > + else
> > + data->id = tmp512;
> >
> > ret = tmp51x_configure(dev, data);
> > if (ret < 0) {
> > --
> > 2.25.1
> >
