| Message ID | 20230824204456.401580-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Enhancements for tmp51x driver | expand |
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. > 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)) ? Also add a comment to explain "a" and other 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. Btw, avoid Yoda style if (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); ... > - data->temp_config = (data->id == tmp513) ? > - TMP513_TEMP_CONFIG_DEFAULT : TMP512_TEMP_CONFIG_DEFAULT; Are those still being in use? > + data->temp_config = TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels);
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 >
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 > > >
Hi Günter, On Fri, Aug 25, 2023 at 9:36 AM Guenter Roeck <linux@roeck-us.net> 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
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 > > > > >
On Fri, Aug 25, 2023 at 08:01:49AM +0000, Biju Das wrote: > > > > > > 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. > Same here. You can try with your two-step approach, but please do not introduce code in step 1 only to remove it in step 2. Thanks, Guenter
On Fri, Aug 25, 2023 at 09:43:24AM +0200, Geert Uytterhoeven wrote: > > > > > > - 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. > Ah yes, sure. Guess I had trouble parsing "those". Thanks, Guenter
diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c index 9a180b1030c9..d45d358087be 100644 --- a/drivers/hwmon/tmp513.c +++ b/drivers/hwmon/tmp513.c @@ -113,6 +113,10 @@ #define MAX_TEMP_HYST 127500 +#define TMP512_MAX_CHANNELS 3 +#define TMP513_MAX_CHANNELS 4 +#define TMP51X_TEMP_CONFIG_DEFAULT(a) (0x8780 | GENMASK(11 + (a) - 1, 11)) + static const u8 TMP51X_TEMP_INPUT[4] = { TMP51X_LOCAL_TEMP_RESULT, TMP51X_REMOTE_TEMP_RESULT_1, @@ -152,10 +156,6 @@ static struct regmap_config tmp51x_regmap_config = { .max_register = TMP51X_MAX_REGISTER_ADDR, }; -enum tmp51x_ids { - tmp512, tmp513 -}; - struct tmp51x_data { u16 shunt_config; u16 pga_gain; @@ -169,7 +169,7 @@ struct tmp51x_data { u32 curr_lsb_ua; u32 pwr_lsb_uw; - enum tmp51x_ids id; + u8 max_channels; struct regmap *regmap; }; @@ -434,7 +434,7 @@ static umode_t tmp51x_is_visible(const void *_data, switch (type) { case hwmon_temp: - if (data->id == tmp512 && channel == 3) + if (data->max_channels == channel) return 0; switch (attr) { case hwmon_temp_input: @@ -585,7 +585,7 @@ static int tmp51x_init(struct tmp51x_data *data) if (ret < 0) return ret; - if (data->id == tmp513) { + if (data->max_channels == TMP513_MAX_CHANNELS) { ret = regmap_write(data->regmap, TMP513_N_FACTOR_3, data->nfactor[2] << 8); if (ret < 0) @@ -601,8 +601,8 @@ 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); @@ -610,11 +610,11 @@ MODULE_DEVICE_TABLE(i2c, tmp51x_id); static const struct of_device_id tmp51x_of_match[] = { { .compatible = "ti,tmp512", - .data = (void *)tmp512 + .data = (void *)TMP512_MAX_CHANNELS }, { .compatible = "ti,tmp513", - .data = (void *)tmp513 + .data = (void *)TMP513_MAX_CHANNELS }, { }, }; @@ -674,9 +674,9 @@ static int tmp51x_read_properties(struct device *dev, struct tmp51x_data *data) return ret; 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); // Check if shunt value is compatible with pga-gain if (data->shunt_uohms > data->pga_gain * 40 * 1000 * 1000) { @@ -698,8 +698,7 @@ static void tmp51x_use_default(struct tmp51x_data *data) static int tmp51x_configure(struct device *dev, struct tmp51x_data *data) { data->shunt_config = TMP51X_SHUNT_CONFIG_DEFAULT; - data->temp_config = (data->id == tmp513) ? - TMP513_TEMP_CONFIG_DEFAULT : TMP512_TEMP_CONFIG_DEFAULT; + data->temp_config = TMP51X_TEMP_CONFIG_DEFAULT(data->max_channels); if (dev->of_node) return tmp51x_read_properties(dev, data); @@ -720,7 +719,7 @@ 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); ret = tmp51x_configure(dev, data); if (ret < 0) {
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 enum tmp51x_ids as there is no user. Suggested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2: * New patch. --- drivers/hwmon/tmp513.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)