[PATCH v4 0/2] Enhancements for tmp51x driver

[PATCH v4 0/2] Enhancements for tmp51x driver

[Biju Das]

This patch series aims to add enhancements for tmp51x driver.
 * patch#1 Adds max_channels variable to struct tmp51x_data for HW
           differences.
 * Patch#2 Simplify tmp51x_read_properties()

This patch series is only compile tested.

v3->v4:
 * Updated commit description for patch#1
 * Updated macros TMP51X_TEMP_CONFIG_DEFAULT and TMP51X_TEMP_CHANNEL_MASK.
 * Removed trailing comma in the terminator entry for OF table.
 * Added Rb tag from Geert and Andy for patch#2.
v2->v3:
 * Dropped patch#1 and patch#2 as it is accepted
 * Split patch#3 into 3.
 * Avoided Yoda style logic in tmp51x_is_visible().
 * Replaced OF/ID data from tmp51x_ids->max_channels
 * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions
   for Configuration register 2.
 * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT.
v1->v2:
 * Created 2 new patches. Patch#1 for fixing channel number in
   tmp51x_is_visible() and patch#3 avoids using enum chip id for HW
   differences and improves the code by avoiding checks.

Biju Das (2):
  hwmon: tmp513: Add max_channels variable to struct tmp51x_data
  hwmon: tmp513: Simplify tmp51x_read_properties()

 drivers/hwmon/tmp513.c | 54 +++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

[Biju Das]

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.

Replace id->max_channels variable from struct tmp51x_data and drop the
macros TMP51{2,3}_TEMP_CONFIG_DEFAULT as it can be derived from the macro
TMP51X_TEMP_CONFIG_DEFAULT and update the logic in tmp51x_is_visible(),
tmp51x_read_properties() and tmp51x_init() using max_channels.

While at it, drop enum tmp51x_ids as there is no user and remove
trailing comma in the terminator entry for OF table.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v3->v4:
 * Updated commit description
 * Updated macros TMP51X_TEMP_CONFIG_DEFAULT and TMP51X_TEMP_CHANNEL_MASK.
 * Removed trailing comma in the terminator entry for OF table.
v2->v3:
 * Updated the macro TMP51X_TEMP_CONFIG_DEFAULT by adding bit definitions.
 * Dropped unused macros TMP51{2,3}_TEMP_CONFIG_DEFAULT.
 * Avoided Yoda style logic.
 * Replaced OF/ID data from tmp51x_ids->max_channels
v2:
 * New patch.
---
 drivers/hwmon/tmp513.c | 51 +++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c
index 9a180b1030c9..7c531fa4453e 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
@@ -113,6 +111,18 @@
 
 #define MAX_TEMP_HYST			127500
 
+#define TMP512_MAX_CHANNELS		3
+#define TMP513_MAX_CHANNELS		4
+
+#define TMP51X_TEMP_CONFIG_CONV_RATE	FIELD_PREP(GENMASK(9, 7), 0x7)
+#define TMP51X_TEMP_CONFIG_RC		BIT(10)
+#define TMP51X_TEMP_CHANNEL_MASK(n)	FIELD_PREP(GENMASK(14, 11), \
+						   GENMASK((n) - 1, 0))
+#define TMP51X_TEMP_CONFIG_CONT		BIT(15)
+#define TMP51X_TEMP_CONFIG_DEFAULT(n)	(TMP51X_TEMP_CONFIG_CONT | \
+			TMP51X_TEMP_CHANNEL_MASK(n) | \
+			TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
+
 static const u8 TMP51X_TEMP_INPUT[4] = {
 	TMP51X_LOCAL_TEMP_RESULT,
 	TMP51X_REMOTE_TEMP_RESULT_1,
@@ -152,10 +162,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 +175,7 @@ struct tmp51x_data {
 	u32 curr_lsb_ua;
 	u32 pwr_lsb_uw;
 
-	enum tmp51x_ids id;
+	u8 max_channels;
 	struct regmap *regmap;
 };
 
@@ -434,7 +440,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:
@@ -585,7 +591,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,22 +607,16 @@ 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);
 
@@ -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) {
-- 
2.25.1

Re: [PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 07:42:51PM +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.
> 
> Replace id->max_channels variable from struct tmp51x_data and drop the
> macros TMP51{2,3}_TEMP_CONFIG_DEFAULT as it can be derived from the macro
> TMP51X_TEMP_CONFIG_DEFAULT and update the logic in tmp51x_is_visible(),
> tmp51x_read_properties() and tmp51x_init() using max_channels.
> 
> While at it, drop enum tmp51x_ids as there is no user and remove
> trailing comma in the terminator entry for OF table.

...

> +#define TMP51X_TEMP_CONFIG_CONV_RATE	FIELD_PREP(GENMASK(9, 7), 0x7)
> +#define TMP51X_TEMP_CONFIG_RC		BIT(10)
> +#define TMP51X_TEMP_CHANNEL_MASK(n)	FIELD_PREP(GENMASK(14, 11), \
> +						   GENMASK((n) - 1, 0))

Why do you need FIELD_PREP() for the pure constants here? Why can you simply
define the constants in the proper place?

...

> +#define TMP51X_TEMP_CONFIG_DEFAULT(n)	(TMP51X_TEMP_CONFIG_CONT | \
> +			TMP51X_TEMP_CHANNEL_MASK(n) | \
> +			TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)

This is better to read in a form of

#define TMP51X_TEMP_CONFIG_DEFAULT(n)					\
	(TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT |	\
	 TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

[Biju Das]

Hi Andy,

> Subject: Re: [PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to
> struct tmp51x_data
> 
> On Tue, Sep 05, 2023 at 07:42:51PM +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.
> >
> > Replace id->max_channels variable from struct tmp51x_data and drop the
> > macros TMP51{2,3}_TEMP_CONFIG_DEFAULT as it can be derived from the
> > macro TMP51X_TEMP_CONFIG_DEFAULT and update the logic in
> > tmp51x_is_visible(),
> > tmp51x_read_properties() and tmp51x_init() using max_channels.
> >
> > While at it, drop enum tmp51x_ids as there is no user and remove
> > trailing comma in the terminator entry for OF table.
> 
> ...
> 
> > +#define TMP51X_TEMP_CONFIG_CONV_RATE	FIELD_PREP(GENMASK(9, 7), 0x7)
> > +#define TMP51X_TEMP_CONFIG_RC		BIT(10)
> > +#define TMP51X_TEMP_CHANNEL_MASK(n)	FIELD_PREP(GENMASK(14, 11), \
> > +						   GENMASK((n) - 1, 0))
> 
> Why do you need FIELD_PREP() for the pure constants here? Why can you
> simply define the constants in the proper place?

I think one can clearly understand the bit definitions and value from FIELD_PREP.

For eg:
GENMASK(14, 11)--> bits 11..14
GENMASK((n) - 1, 0)--> value 15 for 4 channels and 7 for 3 channels.

> 
> ...
> 
> > +#define TMP51X_TEMP_CONFIG_DEFAULT(n)	(TMP51X_TEMP_CONFIG_CONT | \
> > +			TMP51X_TEMP_CHANNEL_MASK(n) | \
> > +			TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> 
> This is better to read in a form of
> 
> #define TMP51X_TEMP_CONFIG_DEFAULT(n)					\
> 	(TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT |	\
> 	 TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> 

I just used the indentation suggested by Guenter.

Cheers,
Biju

Re: [PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

[Andy Shevchenko]

On Wed, Sep 06, 2023 at 02:46:37PM +0000, Biju Das wrote:
> > On Tue, Sep 05, 2023 at 07:42:51PM +0100, Biju Das wrote:

...

> > > +#define TMP51X_TEMP_CONFIG_CONV_RATE	FIELD_PREP(GENMASK(9, 7), 0x7)
> > > +#define TMP51X_TEMP_CONFIG_RC		BIT(10)
> > > +#define TMP51X_TEMP_CHANNEL_MASK(n)	FIELD_PREP(GENMASK(14, 11), \
> > > +						   GENMASK((n) - 1, 0))
> > 
> > Why do you need FIELD_PREP() for the pure constants here? Why can you
> > simply define the constants in the proper place?
> 
> I think one can clearly understand the bit definitions and value from FIELD_PREP.

I think it is clear over engineering. In the first one the constant can't be
more than the field, in the second the defensive programming that is discouraged
in the kernel.

#define TMP51X_TEMP_CHANNEL_MASK(n)	(GENMASK((n) - 1, 0) << 11)

would suffice and much easier to understand.

...

> > > +#define TMP51X_TEMP_CONFIG_DEFAULT(n)	(TMP51X_TEMP_CONFIG_CONT | \
> > > +			TMP51X_TEMP_CHANNEL_MASK(n) | \
> > > +			TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> > 
> > This is better to read in a form of
> > 
> > #define TMP51X_TEMP_CONFIG_DEFAULT(n)					\
> > 	(TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT |	\
> > 	 TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> > 
> 
> I just used the indentation suggested by Guenter.

But wouldn't my proposal be sightly better to read?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

[Guenter Roeck]

On Wed, Sep 06, 2023 at 05:59:55PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 06, 2023 at 02:46:37PM +0000, Biju Das wrote:
> > > On Tue, Sep 05, 2023 at 07:42:51PM +0100, Biju Das wrote:
> 
> ...
> 
> > > > +#define TMP51X_TEMP_CONFIG_CONV_RATE	FIELD_PREP(GENMASK(9, 7), 0x7)
> > > > +#define TMP51X_TEMP_CONFIG_RC		BIT(10)
> > > > +#define TMP51X_TEMP_CHANNEL_MASK(n)	FIELD_PREP(GENMASK(14, 11), \
> > > > +						   GENMASK((n) - 1, 0))
> > > 
> > > Why do you need FIELD_PREP() for the pure constants here? Why can you
> > > simply define the constants in the proper place?
> > 
> > I think one can clearly understand the bit definitions and value from FIELD_PREP.
> 
> I think it is clear over engineering. In the first one the constant can't be
> more than the field, in the second the defensive programming that is discouraged
> in the kernel.
> 
> #define TMP51X_TEMP_CHANNEL_MASK(n)	(GENMASK((n) - 1, 0) << 11)
> 
> would suffice and much easier to understand.
> 

I agree.

> ...
> 
> > > > +#define TMP51X_TEMP_CONFIG_DEFAULT(n)	(TMP51X_TEMP_CONFIG_CONT | \
> > > > +			TMP51X_TEMP_CHANNEL_MASK(n) | \
> > > > +			TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> > > 
> > > This is better to read in a form of
> > > 
> > > #define TMP51X_TEMP_CONFIG_DEFAULT(n)					\
> > > 	(TMP51X_TEMP_CHANNEL_MASK(n) | TMP51X_TEMP_CONFIG_CONT |	\
> > > 	 TMP51X_TEMP_CONFIG_CONV_RATE | TMP51X_TEMP_CONFIG_RC)
> > > 
> > 
> > I just used the indentation suggested by Guenter.
> 
> But wouldn't my proposal be sightly better to read?

I'd have to look up what specifically I had asked for, but it wasn't meant
to exclude better formatting. I agree that the formatting you suggested
is better.

Guenter

[PATCH v4 2/2] hwmon: tmp513: Simplify tmp51x_read_properties()

[Biju Das]

Simplify tmp51x_read_properties() by replacing 'nfactor' ->'data->nfactor'
in device_property_read_u32_array() and drop the local variable as it is
unused.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3->v4:
 * Added Rb tag from Geert and Andy.
v3:
 * New patch.
---
 drivers/hwmon/tmp513.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/tmp513.c b/drivers/hwmon/tmp513.c
index 7c531fa4453e..43f4c96ecf3d 100644
--- a/drivers/hwmon/tmp513.c
+++ b/drivers/hwmon/tmp513.c
@@ -655,7 +655,6 @@ static int tmp51x_pga_gain_to_reg(struct device *dev, struct tmp51x_data *data)
 static int tmp51x_read_properties(struct device *dev, struct tmp51x_data *data)
 {
 	int ret;
-	u32 nfactor[3];
 	u32 val;
 
 	ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms", &val);
@@ -673,10 +672,8 @@ static int tmp51x_read_properties(struct device *dev, struct tmp51x_data *data)
 	if (ret < 0)
 		return ret;
 
-	ret = device_property_read_u32_array(dev, "ti,nfactor", nfactor,
-					    data->max_channels - 1);
-	if (ret >= 0)
-		memcpy(data->nfactor, nfactor, data->max_channels - 1);
+	device_property_read_u32_array(dev, "ti,nfactor", data->nfactor,
+				       data->max_channels - 1);
 
 	// Check if shunt value is compatible with pga-gain
 	if (data->shunt_uohms > data->pga_gain * 40 * 1000 * 1000) {
-- 
2.25.1