diff mbox series

[v3,1/3] hwmon: tmp513: Add max_channels variable to struct tmp51x_data

Message ID 20230825205345.632792-2-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Enhancements for tmp51x driver | expand

Commit Message

Biju Das Aug. 25, 2023, 8:53 p.m. UTC 
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().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
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(-)

Comments

Guenter Roeck Aug. 26, 2023, 2:05 a.m. UTC | #1 
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.

Guenter

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> 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
>
Biju Das Aug. 26, 2023, 6:29 a.m. UTC | #2 
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@bp.renesas.com>
> > ---
> > 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
> >
diff mbox series

Patch

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) {