| Message ID | 20230825205345.632792-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Enhancements for tmp51x driver | expand |
On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote: > Drop variable id from struct tmp51x_data and enum tmp51x_ids as all the > hw differences can be handled by max_channels. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions. > * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT. > --- > drivers/hwmon/tmp513.c | 38 +++++++++++++++++++++----------------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c > index 99f66f9d5f19..6bbae4735a4b 100644 > --- a/drivers/hwmon/tmp513.c > +++ b/drivers/hwmon/tmp513.c > @@ -19,6 +19,7 @@ > * the Free Software Foundation; version 2 of the License. > */ > > +#include <linux/bitfield.h> > #include <linux/err.h> > #include <linux/hwmon.h> > #include <linux/i2c.h> > @@ -73,9 +74,6 @@ > #define TMP51X_PGA_DEFAULT 8 > #define TMP51X_MAX_REGISTER_ADDR 0xFF > > -#define TMP512_TEMP_CONFIG_DEFAULT 0xBF80 > -#define TMP513_TEMP_CONFIG_DEFAULT 0xFF80 > - > // Mask and shift > #define CURRENT_SENSE_VOLTAGE_320_MASK 0x1800 > #define CURRENT_SENSE_VOLTAGE_160_MASK 0x1000 > @@ -116,6 +114,22 @@ > #define TMP512_MAX_CHANNELS 3 > #define TMP513_MAX_CHANNELS 4 > > +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2) > +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10) > +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15) > + > +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) > +#define TMP51X_TEMP_CONFIG_GP FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) > +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) > +#define TMP51X_TEMP_CONFIG_RC FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) > +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), GENMASK(n, 0) > 1) > +#define TMP51X_TEMP_CONFIG_CONT FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) > + > +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ > + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ > + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | \ > + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) > + > static const u8 TMP51X_TEMP_INPUT[4] = { > TMP51X_LOCAL_TEMP_RESULT, > TMP51X_REMOTE_TEMP_RESULT_1, > @@ -155,10 +169,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; > @@ -172,7 +182,6 @@ struct tmp51x_data { > u32 curr_lsb_ua; > u32 pwr_lsb_uw; > > - enum tmp51x_ids id; > u8 max_channels; > struct regmap *regmap; > }; > @@ -589,7 +598,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) > @@ -672,9 +681,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) { > @@ -696,8 +705,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); > @@ -719,10 +727,6 @@ static int tmp51x_probe(struct i2c_client *client) > return -ENOMEM; > > data->max_channels = (uintptr_t)i2c_get_match_data(client); > - if (data->max_channels == TMP513_MAX_CHANNELS) > - data->id = tmp513; > - else > - data->id = tmp512; > See, hat is exactly what I wanted to avoid: The code above was introduced with the previous patch for the sole reason to be removed with this patch. On top of that, you introduced a bogus "fix" in the previous patch which doesn't fix anything and is at the very least misleading. So, if I accept this series, I'll combine those two patches back together. I just do not see the point of making things more complicated than necessary. Sure, the rule is "one patch per logical change", but the logical change here is to replace the chip id with the number of channels, not the introduction of some variable. Guenter > ret = tmp51x_configure(dev, data); > if (ret < 0) { > -- > 2.25.1 >
Hi Guenter Roeck, Thanks for the feedback. > Subject: Re: [PATCH v3 2/3] hwmon: tmp513: Drop enum tmp51x_ids and > variable id from struct tmp51x_data > > On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote: > > Drop variable id from struct tmp51x_data and enum tmp51x_ids as all > > the hw differences can be handled by max_channels. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit > definitions. > > * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT. > > --- > > drivers/hwmon/tmp513.c | 38 +++++++++++++++++++++----------------- > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c index > > 99f66f9d5f19..6bbae4735a4b 100644 > > --- a/drivers/hwmon/tmp513.c > > +++ b/drivers/hwmon/tmp513.c > > @@ -19,6 +19,7 @@ > > * the Free Software Foundation; version 2 of the License. > > */ > > > > +#include <linux/bitfield.h> > > #include <linux/err.h> > > #include <linux/hwmon.h> > > #include <linux/i2c.h> > > @@ -73,9 +74,6 @@ > > #define TMP51X_PGA_DEFAULT 8 > > #define TMP51X_MAX_REGISTER_ADDR 0xFF > > > > -#define TMP512_TEMP_CONFIG_DEFAULT 0xBF80 > > -#define TMP513_TEMP_CONFIG_DEFAULT 0xFF80 > > - > > // Mask and shift > > #define CURRENT_SENSE_VOLTAGE_320_MASK 0x1800 > > #define CURRENT_SENSE_VOLTAGE_160_MASK 0x1000 > > @@ -116,6 +114,22 @@ > > #define TMP512_MAX_CHANNELS 3 > > #define TMP513_MAX_CHANNELS 4 > > > > +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2) > > +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10) > > +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15) > > + > > +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) > > +#define TMP51X_TEMP_CONFIG_GP > FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) > > +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) > > +#define TMP51X_TEMP_CONFIG_RC > FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) > > +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), > GENMASK(n, 0) > 1) > > +#define TMP51X_TEMP_CONFIG_CONT > FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) > > + > > +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ > > + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ > > + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | > \ > > + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) > > + > > static const u8 TMP51X_TEMP_INPUT[4] = { > > TMP51X_LOCAL_TEMP_RESULT, > > TMP51X_REMOTE_TEMP_RESULT_1, > > @@ -155,10 +169,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; > > @@ -172,7 +182,6 @@ struct tmp51x_data { > > u32 curr_lsb_ua; > > u32 pwr_lsb_uw; > > > > - enum tmp51x_ids id; > > u8 max_channels; > > struct regmap *regmap; > > }; > > @@ -589,7 +598,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) > > @@ -672,9 +681,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) { @@ > > -696,8 +705,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); @@ -719,10 +727,6 @@ > > static int tmp51x_probe(struct i2c_client *client) > > return -ENOMEM; > > > > data->max_channels = (uintptr_t)i2c_get_match_data(client); > > - if (data->max_channels == TMP513_MAX_CHANNELS) > > - data->id = tmp513; > > - else > > - data->id = tmp512; > > > > See, hat is exactly what I wanted to avoid: The code above was introduced > with the previous patch for the sole reason to be removed with this patch. > On top of that, you introduced a bogus "fix" in the previous patch which > doesn't fix anything and is at the very least misleading. > > So, if I accept this series, I'll combine those two patches back together. OK, Thank you for merging those two patches together. Cheers, Biju
On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote: ... > +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) > +#define TMP51X_TEMP_CONFIG_GP FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) > +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) How is this different from (GENMASK(2, 0) << 7)? > +#define TMP51X_TEMP_CONFIG_RC FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) > +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), GENMASK(n, 0) > 1) > +#define TMP51X_TEMP_CONFIG_CONT FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) Looking at these I believe the FIELD_PREP() is overkill. ... > +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ > + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ > + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | \ > + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) Too many TABs
On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote: > Drop variable id from struct tmp51x_data and enum tmp51x_ids as all the > hw differences can be handled by max_channels. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions. > * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT. > --- [ ... ] I had another look at those. First of all, using MASK and FIELD_PREP for single-bit fields doesn't add value. Just drop the _MASK from all those and just use BIT(). > +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2) GPM is bit 0 and 1, so this is wrong. This should probably be TMP51X_TEMP_CONFIG_GP which is bit 2. It is also a read-only value, so setting it is never warranted. > +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10) > +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15) Drop _MASK. > + > +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) Technically, using a 2-bit field here is misleading, since bit 1 defines if the gpio bit is an input or output, and bit 0 defines the state of the pin if it is an output. This would have to change if and when gpio support is added to the driver, so it is best to not define anything GP related in the first place. > +#define TMP51X_TEMP_CONFIG_GP FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) > +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) > +#define TMP51X_TEMP_CONFIG_RC FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) Those are really the values to be put into the configuration register, which I find is just confusing. But define the register bits, set the bit if needed, and otherwise keep it alone. > +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), GENMASK(n, 0) > 1) This is wrong. Either s/>/>>/ or GENMASK((n) - 1, 0). I personally prefer the latter since I find it easier to understand. > +#define TMP51X_TEMP_CONFIG_CONT FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) > + > +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ > + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ > + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | \ > + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) > + I would very much prefer something like #define TMP51X_TEMP_CONFIG_DEFAULT(n) (TMP51X_TEMP_CONFIG_CONT | \ TMP51X_TEMP_CHANNEL_MASK(n) \ TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC) and drop the unnecessary complexity of defining single bit values with FIELD_PREP(). Guenter
Hi Guenter Roeck, Thanks for the feedback. > Subject: Re: [PATCH v3 2/3] hwmon: tmp513: Drop enum tmp51x_ids and > variable id from struct tmp51x_data > > On Fri, Aug 25, 2023 at 09:53:44PM +0100, Biju Das wrote: > > Drop variable id from struct tmp51x_data and enum tmp51x_ids as all > > the hw differences can be handled by max_channels. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit > definitions. > > * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT. > > --- > > [ ... ] > > I had another look at those. First of all, using MASK and FIELD_PREP for > single-bit fields doesn't add value. Just drop the _MASK from all those and > just use BIT(). OK. > > > +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2) > > GPM is bit 0 and 1, so this is wrong. This should probably be > TMP51X_TEMP_CONFIG_GP which is bit 2. It is also a read-only value, so > setting it is never warranted. OK, will drop this. > > > +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10) > > +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15) > > Drop _MASK. OK. > > > + > > +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) > > Technically, using a 2-bit field here is misleading, since bit 1 defines if > the gpio bit is an input or output, and bit 0 defines the state of the pin > if it is an output. This would have to change if and when gpio support is > added to the driver, so it is best to not define anything GP related in the > first place. OK. > > > +#define TMP51X_TEMP_CONFIG_GP > FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) > > +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) > > +#define TMP51X_TEMP_CONFIG_RC > FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) > > Those are really the values to be put into the configuration register, > which I find is just confusing. But define the register bits, set the bit > if needed, and otherwise keep it alone. OK. > > > +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), > GENMASK(n, 0) > 1) > > This is wrong. Either s/>/>>/ or GENMASK((n) - 1, 0). I personally prefer > the latter since I find it easier to understand. Agreed, there is a mistake, your suggestion is good. > > > +#define TMP51X_TEMP_CONFIG_CONT > FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) > > + > > +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ > > + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ > > + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | > \ > > + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) > > + > > I would very much prefer something like > > #define TMP51X_TEMP_CONFIG_DEFAULT(n) (TMP51X_TEMP_CONFIG_CONT | \ > TMP51X_TEMP_CHANNEL_MASK(n) \ > TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC) > > and drop the unnecessary complexity of defining single bit values with > FIELD_PREP(). OK. Cheers, Biju
diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c index 99f66f9d5f19..6bbae4735a4b 100644 --- a/drivers/hwmon/tmp513.c +++ b/drivers/hwmon/tmp513.c @@ -19,6 +19,7 @@ * the Free Software Foundation; version 2 of the License. */ +#include <linux/bitfield.h> #include <linux/err.h> #include <linux/hwmon.h> #include <linux/i2c.h> @@ -73,9 +74,6 @@ #define TMP51X_PGA_DEFAULT 8 #define TMP51X_MAX_REGISTER_ADDR 0xFF -#define TMP512_TEMP_CONFIG_DEFAULT 0xBF80 -#define TMP513_TEMP_CONFIG_DEFAULT 0xFF80 - // Mask and shift #define CURRENT_SENSE_VOLTAGE_320_MASK 0x1800 #define CURRENT_SENSE_VOLTAGE_160_MASK 0x1000 @@ -116,6 +114,22 @@ #define TMP512_MAX_CHANNELS 3 #define TMP513_MAX_CHANNELS 4 +#define TMP51X_TEMP_CONFIG_GPM_MASK BIT(2) +#define TMP51X_TEMP_CONFIG_RC_MASK BIT(10) +#define TMP51X_TEMP_CONFIG_CONT_MASK BIT(15) + +#define TMP51X_TEMP_CONFIG_GPM FIELD_PREP(GENMASK(1, 0), 0) +#define TMP51X_TEMP_CONFIG_GP FIELD_PREP(TMP51X_TEMP_CONFIG_GPM_MASK, 0) +#define TMP51X_TEMP_CONFIG_CONV_RATE FIELD_PREP(GENMASK(9, 7), 0x7) +#define TMP51X_TEMP_CONFIG_RC FIELD_PREP(TMP51X_TEMP_CONFIG_RC_MASK, 1) +#define TMP51X_TEMP_CHANNEL_MASK(n) FIELD_PREP(GENMASK(14, 11), GENMASK(n, 0) > 1) +#define TMP51X_TEMP_CONFIG_CONT FIELD_PREP(TMP51X_TEMP_CONFIG_CONT_MASK, 1) + +#define TMP51X_TEMP_CONFIG_DEFAULT(n) \ + (TMP51X_TEMP_CONFIG_GPM | TMP51X_TEMP_CONFIG_GP | \ + TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC | \ + TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT) + static const u8 TMP51X_TEMP_INPUT[4] = { TMP51X_LOCAL_TEMP_RESULT, TMP51X_REMOTE_TEMP_RESULT_1, @@ -155,10 +169,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; @@ -172,7 +182,6 @@ struct tmp51x_data { u32 curr_lsb_ua; u32 pwr_lsb_uw; - enum tmp51x_ids id; u8 max_channels; struct regmap *regmap; }; @@ -589,7 +598,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) @@ -672,9 +681,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) { @@ -696,8 +705,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); @@ -719,10 +727,6 @@ static int tmp51x_probe(struct i2c_client *client) return -ENOMEM; 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) {
Drop variable id from struct tmp51x_data and enum tmp51x_ids as all the hw differences can be handled by max_channels. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions. * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT. --- drivers/hwmon/tmp513.c | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-)