hwmon:(adm1275) Enable adm1278 ADM1278_TEMP1_EN

PATCH Superseded Manikandan Elumalai 4 participants 10 messages May 29, 2020 – Jun 6, 2020 Hardware monitoring

Share & tools

Manikandan Elumalai

author

2020-05-29 12:46 UTC #0 lore raw

PATCH v2 1/1 hwmon:(adm1275) Enable adm1278 ADM1278_TEMP1_EN

The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.

Signed-off-by Manikandan Elumalai <[email protected]>

v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.

---
 drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

drivers/hwmon/pmbus/adm1275.c +17 -4

index 5caa37fb..ab5fceb 100644
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		if (config < 0)
+			return config;
+
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
 
 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Signature · 1 line

2.7.4

Guenter Roeck

maintainer

2020-05-29 17:30 UTC #1 lore raw

Quoted 4 lines · Manikandan Elumalai

The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
Signed-off-by Manikandan Elumalai <[email protected]>

This is not valid.

Quoted 4 lines

v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.

checkpatch reports:

Quoted 1 line

---WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)

#14:
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .

CHECK: Alignment should match open parenthesis
#45: FILE: drivers/hwmon/pmbus/adm1275.c:679:
+ ret = i2c_smbus_write_byte_data(client,
+ ADM1275_PMON_CONFIG,

WARNING: suspect code indent for conditional statements (16, 16) #47: FILE: drivers/hwmon/pmbus/adm1275.c:681:
+ if (ret < 0) {
+ dev_err(&client->dev,

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 2 warnings, 1 checks, 33 lines checked

Please follow published guidelines when submitting patches.

Quoted 19 lines

 drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

drivers/hwmon/pmbus/adm1275.c +7 -1

index 5caa37fb..ab5fceb 100644
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		if (config < 0)
+			return config;
+

drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685

Quoted 10 lines

drivers/hwmon/pmbus/adm1275.c +10 -0

+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}

This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.

Guenter

Quoted 14 lines

drivers/hwmon/pmbus/adm1275.c +0 -3

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Patrick Williams

2020-05-29 17:42 UTC #2 lore raw

Hi Guenter,

Thanks for the initial look at this.

One question for you below...

Quoted 14 lines · Guenter Roeck

drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685
Quoted 10 lines · Manikandan Elumalai
drivers/hwmon/pmbus/adm1275.c +10 -0
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.

I don't know if you noticed here but the change ends up enabling TEMP1_EN in all cases. Is this acceptable? If not, do you have any preference on how it is selected for enablement?

Quoted 14 lines

Quoted 13 lines

drivers/hwmon/pmbus/adm1275.c +0 -3

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Signature · 8 lines

Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20200529/9450a289/attachment-0001.sig>

Guenter Roeck

maintainer

2020-05-29 18:52 UTC #3 lore raw

Quoted 22 lines · Patrick Williams

Hi Guenter,
Thanks for the initial look at this.
One question for you below...
Quoted 14 lines · Guenter Roeck
drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685
Quoted 10 lines · Manikandan Elumalai
drivers/hwmon/pmbus/adm1275.c +10 -0
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.

Quoted 4 lines

I don't know if you noticed here but the change ends up enabling TEMP1_EN in all cases. Is this acceptable? If not, do you have any preference on how it is selected for enablement?

I did. We are doing the same for output voltage already, so I am not that much concerned about it. If it is, we might consider adding _enable attribute support (see Documentation/hwmon/sysfs-interface.rst) to the PMBus core (presumably as virtual PMBus commands) and let the user enable/disable individual attributes as needed.

What _should_ really be done, of course, is that the BIOS/ROMMON configures the chip as desired. Obviously that is not happening here.

Guenter

Quoted 6 lines

drivers/hwmon/pmbus/adm1275.c +0 -0

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}

Quoted 9 lines

Quoted 8 lines
drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 702
Quoted 7 lines
drivers/hwmon/pmbus/adm1275.c +0 -3
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20200529/64248c86/attachment.sig>

Manikandan

author

2020-06-05 16:48 UTC #4 lore raw

In reply to Guenter Roeck

Quoted 59 lines · Guenter Roeck

Quoted 4 lines · Manikandan Elumalai
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
Signed-off-by Manikandan Elumalai <[email protected]>
This is not valid.
Quoted 4 lines
v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.
checkpatch reports:
Quoted 1 line
---WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14:
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
CHECK: Alignment should match open parenthesis
#45: FILE: drivers/hwmon/pmbus/adm1275.c:679:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
WARNING: suspect code indent for conditional statements (16, 16) #47: FILE: drivers/hwmon/pmbus/adm1275.c:681:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 2 warnings, 1 checks, 33 lines checked
Please follow published guidelines when submitting patches.
Quoted 24 lines
 drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/hwmon/pmbus/adm1275.c +12 -1
index 5caa37fb..ab5fceb 100644
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		if (config < 0)
+			return config;
+
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);

Quoted 9 lines

drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685
Quoted 5 lines
drivers/hwmon/pmbus/adm1275.c +5 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.

Thanks for review Guenter, Patrick and Vijay.
Sorry for delay response.
I have made changes to write ADM1278_VOUT_EN and ADM1278_TEMP1_EN in single operation and tested in platfrom .
The changes given for quick look and will help if any misunderstand.

--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -666,11 +666,11 @@ static int adm1275_probe(struct i2c_client *client,
tindex = 3;

info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |

	-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
	+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | 
        +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;

/* Enable VOUT if not enabled (it is disabled by default) */

 		if (!(config & ADM1278_VOUT_EN)) {
	-			config |= ADM1278_VOUT_EN;
	+			config |= (ADM1278_VOUT_EN | ADM1278_TEMP1_EN);
 			ret = i2c_smbus_write_byte_data(client,
 							ADM1275_PMON_CONFIG,
 							config);
	@@ -680,10 +680,6 @@ static int adm1275_probe(struct i2c_client *client,
 				return -ENODEV;
 			}
 		}
	-
	-		if (config & ADM1278_TEMP1_EN)
	-			info->func[0] |=
	-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;

if (config & ADM1278_VIN_EN)
info->func[0] |= PMBUS_HAVE_VIN;
break;

Quoted 17 lines

Guenter

Quoted 14 lines

drivers/hwmon/pmbus/adm1275.c +0 -3

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Guenter Roeck

maintainer

2020-06-05 18:04 UTC #5 lore raw

Quoted 85 lines · Manikandan

Quoted 59 lines · Guenter Roeck
Quoted 4 lines · Manikandan Elumalai
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
Signed-off-by Manikandan Elumalai <[email protected]>
This is not valid.
Quoted 4 lines
v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.
checkpatch reports:
Quoted 1 line
---WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14:
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
CHECK: Alignment should match open parenthesis
#45: FILE: drivers/hwmon/pmbus/adm1275.c:679:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
WARNING: suspect code indent for conditional statements (16, 16) #47: FILE: drivers/hwmon/pmbus/adm1275.c:681:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 2 warnings, 1 checks, 33 lines checked
Please follow published guidelines when submitting patches.
Quoted 24 lines
 drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/hwmon/pmbus/adm1275.c +12 -1
index 5caa37fb..ab5fceb 100644
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		if (config < 0)
+			return config;
+
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
Quoted 9 lines
drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685
Quoted 5 lines
drivers/hwmon/pmbus/adm1275.c +5 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.
Thanks for review Guenter, Patrick and Vijay.
Sorry for delay response.
I have made changes to write ADM1278_VOUT_EN and ADM1278_TEMP1_EN in single operation and tested in platfrom .
The changes given for quick look and will help if any misunderstand.
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -666,11 +666,11 @@ static int adm1275_probe(struct i2c_client *client,
drivers/hwmon/pmbus/adm1275.c +0 -0
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
	-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
	+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | 
        +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;

drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 688

Quoted 2 lines

drivers/hwmon/pmbus/adm1275.c +0 -0

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

		if (config & (ADM1278_VOUT_EN | ADM1278_TEMP1_EN) !=
						ADM1278_VOUT_EN | ADM1278_TEMP1_EN)

Quoted 2 lines

	-			config |= ADM1278_VOUT_EN;
	+			config |= (ADM1278_VOUT_EN | ADM1278_TEMP1_EN);

( ) is unnecessary here.

Quoted 31 lines

(quoted diff) +0 -0

 			ret = i2c_smbus_write_byte_data(client,
 							ADM1275_PMON_CONFIG,
 							config);

	@@ -680,10 +680,6 @@ static int adm1275_probe(struct i2c_client *client,

(quoted diff) +0 -0

 				return -ENODEV;
 			}
 		}

	-
	-		if (config & ADM1278_TEMP1_EN)
	-			info->func[0] |=
	-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;

drivers/hwmon/pmbus/adm1275.c +0 -0

 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Quoted 17 lines

Guenter

Quoted 14 lines

drivers/hwmon/pmbus/adm1275.c +0 -3

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Manikandan

author

2020-06-06 05:13 UTC #6 lore raw

Quoted 89 lines · Guenter Roeck

Quoted 85 lines · Manikandan
Quoted 59 lines · Guenter Roeck
Quoted 4 lines · Manikandan Elumalai
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
Signed-off-by Manikandan Elumalai <[email protected]>
This is not valid.
Quoted 4 lines
v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.
checkpatch reports:
Quoted 1 line
---WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#14:
The adm1278 temperature sysfs attribute need it for one of the openbmc platform .
CHECK: Alignment should match open parenthesis
#45: FILE: drivers/hwmon/pmbus/adm1275.c:679:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
WARNING: suspect code indent for conditional statements (16, 16) #47: FILE: drivers/hwmon/pmbus/adm1275.c:681:
drivers/hwmon/pmbus/adm1275.c +2 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 2 warnings, 1 checks, 33 lines checked
Please follow published guidelines when submitting patches.
Quoted 24 lines
 drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
drivers/hwmon/pmbus/adm1275.c +12 -1
index 5caa37fb..ab5fceb 100644
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+		if (config < 0)
+			return config;
+
+		/* Enable TEMP1 by default */
+		config |= ADM1278_TEMP1_EN;
+		ret = i2c_smbus_write_byte_data(client,
+					ADM1275_PMON_CONFIG,
+					config);
Quoted 9 lines
drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 685
Quoted 5 lines
drivers/hwmon/pmbus/adm1275.c +5 -0
+		if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to enable temperature config\n");
+		return -ENODEV;
+		}
This can be handled in a single operation, together with ADM1278_VOUT_EN below. There is no need for two separate write operations.
Thanks for review Guenter, Patrick and Vijay.
Sorry for delay response.
I have made changes to write ADM1278_VOUT_EN and ADM1278_TEMP1_EN in single operation and tested in platfrom .
The changes given for quick look and will help if any misunderstand.
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -666,11 +666,11 @@ static int adm1275_probe(struct i2c_client *client,
drivers/hwmon/pmbus/adm1275.c +0 -0
 		tindex = 3;
 
 		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
	-			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
	+			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | 
        +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
drivers/hwmon/pmbus/adm1275.c static int adm1275_probe(struct i2c_client *client, · line 688
Quoted 2 lines
drivers/hwmon/pmbus/adm1275.c +0 -0
 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

Quoted 7 lines

		if (config & (ADM1278_VOUT_EN | ADM1278_TEMP1_EN) !=
						ADM1278_VOUT_EN | ADM1278_TEMP1_EN)

Quoted 2 lines

	-			config |= ADM1278_VOUT_EN;
	+			config |= (ADM1278_VOUT_EN | ADM1278_TEMP1_EN);

( ) is unnecessary here.

Thanks for quick review Guenter. I will submit all changes in v3.

Quoted 32 lines

(quoted diff) +0 -0

 			ret = i2c_smbus_write_byte_data(client,
 							ADM1275_PMON_CONFIG,
 							config);

	@@ -680,10 +680,6 @@ static int adm1275_probe(struct i2c_client *client,

(quoted diff) +0 -0

 				return -ENODEV;
 			}
 		}

	-
	-		if (config & ADM1278_TEMP1_EN)
	-			info->func[0] |=
	-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;

drivers/hwmon/pmbus/adm1275.c +0 -0

 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Quoted 17 lines

Guenter

Quoted 14 lines

drivers/hwmon/pmbus/adm1275.c +0 -3

 		/* Enable VOUT if not enabled (it is disabled by default) */
 		if (!(config & ADM1278_VOUT_EN)) {

@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
 			}
 		}
 
-		if (config & ADM1278_TEMP1_EN)
-			info->func[0] |=
-				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
 		if (config & ADM1278_VIN_EN)
 			info->func[0] |= PMBUS_HAVE_VIN;
 		break;

Vijay Khemka

2020-05-29 17:57 UTC #7 lore raw

In reply to Manikandan Elumalai

?On 5/29/20, 5:47 AM, "Manikandan Elumalai" <[email protected]> wrote:

    The adm1278 temperature sysfs attribute need it for one of the openbmc platform . 
    This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
    
    Signed-off-by   : Manikandan Elumalai <[email protected]>
    
    v2:
       - Add Signed-off-by.
       - Removed ADM1278_TEMP1_EN check.
    ---
     drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
     1 file changed, 17 insertions(+), 4 deletions(-)
    
    diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
    index 5caa37fb..ab5fceb 100644
    --- a/drivers/hwmon/pmbus/adm1275.c
    +++ b/drivers/hwmon/pmbus/adm1275.c
    @@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
     		tindex = 3;
     
     		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
    -			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
    +			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
    +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
    +
    +		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
    +		if (config < 0)
    +			return config;
    +
    +		/* Enable TEMP1 by default */
    +		config |= ADM1278_TEMP1_EN;
    +		ret = i2c_smbus_write_byte_data(client,
    +					ADM1275_PMON_CONFIG,
    +					config);
    +		if (ret < 0) {
    +		dev_err(&client->dev,
    +			"Failed to enable temperature config\n");
    +		return -ENODEV;
    +		}

You don't need this above code removing check as below should be enough to populate sysfs entry you need.

     		/* Enable VOUT if not enabled (it is disabled by default) */
     		if (!(config & ADM1278_VOUT_EN)) {
    @@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
     			}
     		}
     
    -		if (config & ADM1278_TEMP1_EN)
    -			info->func[0] |=
    -				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
     		if (config & ADM1278_VIN_EN)
     			info->func[0] |= PMBUS_HAVE_VIN;
     		break;
    -- 
    2.7.4

Guenter Roeck

maintainer

2020-05-29 18:55 UTC #8 lore raw

Quoted 42 lines · Vijay Khemka

?On 5/29/20, 5:47 AM, "Manikandan Elumalai" <[email protected]> wrote:

    The adm1278 temperature sysfs attribute need it for one of the openbmc platform . 
    This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
    
    Signed-off-by   : Manikandan Elumalai <[email protected]>
    
    v2:
       - Add Signed-off-by.
       - Removed ADM1278_TEMP1_EN check.
    ---
     drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
     1 file changed, 17 insertions(+), 4 deletions(-)
    
    diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
    index 5caa37fb..ab5fceb 100644
    --- a/drivers/hwmon/pmbus/adm1275.c
    +++ b/drivers/hwmon/pmbus/adm1275.c
    @@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
     		tindex = 3;
     
     		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
    -			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
    +			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
    +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
    +
    +		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
    +		if (config < 0)
    +			return config;
    +
    +		/* Enable TEMP1 by default */
    +		config |= ADM1278_TEMP1_EN;
    +		ret = i2c_smbus_write_byte_data(client,
    +					ADM1275_PMON_CONFIG,
    +					config);
    +		if (ret < 0) {
    +		dev_err(&client->dev,
    +			"Failed to enable temperature config\n");
    +		return -ENODEV;
    +		}

Quoted 3 lines

You don't need this above code removing check as below should be enough to populate sysfs entry you need.

You mean you are only interested in having the attribute, even if it doesn't report anything useful because monitoring is disabled in the chip ? Sorry, I don't understand. Can you elaborate ?

Thanks,
Guenter

Quoted 17 lines

     		/* Enable VOUT if not enabled (it is disabled by default) */
     		if (!(config & ADM1278_VOUT_EN)) {
    @@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
     			}
     		}
     
    -		if (config & ADM1278_TEMP1_EN)
    -			info->func[0] |=
    -				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
     		if (config & ADM1278_VIN_EN)
     			info->func[0] |= PMBUS_HAVE_VIN;
     		break;
    -- 
    2.7.4

Vijay Khemka

2020-05-29 23:47 UTC #9 lore raw

?On 5/29/20, 11:56 AM, "Guenter Roeck" <groeck7 at gmail.com on behalf of [email protected]> wrote:

    On 5/29/20 10:57 AM, Vijay Khemka wrote:
    > 
    > 
    > On 5/29/20, 5:47 AM, "Manikandan Elumalai" <[email protected]> wrote:
    > 
    >     The adm1278 temperature sysfs attribute need it for one of the openbmc platform . 
    >     This functionality is not enabled by default, so PMON_CONFIG needs to be modified in order to enable it.
    >     
    >     Signed-off-by   : Manikandan Elumalai <[email protected]>
    >     
    >     v2:
    >        - Add Signed-off-by.
    >        - Removed ADM1278_TEMP1_EN check.
    >     ---
    >      drivers/hwmon/pmbus/adm1275.c | 21 +++++++++++++++++----
    >      1 file changed, 17 insertions(+), 4 deletions(-)
    >     
    >     diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
    >     index 5caa37fb..ab5fceb 100644
    >     --- a/drivers/hwmon/pmbus/adm1275.c
    >     +++ b/drivers/hwmon/pmbus/adm1275.c
    >     @@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
    >      		tindex = 3;
    >      
    >      		info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
    >     -			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
    >     +			PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
    >     +			PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
    >     +
    >     +		config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
    >     +		if (config < 0)
    >     +			return config;
    >     +
    >     +		/* Enable TEMP1 by default */
    >     +		config |= ADM1278_TEMP1_EN;
    >     +		ret = i2c_smbus_write_byte_data(client,
    >     +					ADM1275_PMON_CONFIG,
    >     +					config);
    >     +		if (ret < 0) {
    >     +		dev_err(&client->dev,
    >     +			"Failed to enable temperature config\n");
    >     +		return -ENODEV;
    >     +		}
    > You don't need this above code removing check as below should be enough  to
    > populate sysfs entry you need.
    > 

    You mean you are only interested in having the attribute, even if it doesn't
    report anything useful because monitoring is disabled in the chip ?
    Sorry, I don't understand. Can you elaborate ?

Sorry for misinterpretation, No I don't mean that. This should be fine.

    Thanks,
    Guenter

    >      		/* Enable VOUT if not enabled (it is disabled by default) */
    >      		if (!(config & ADM1278_VOUT_EN)) {
    >     @@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
    >      			}
    >      		}
    >      
    >     -		if (config & ADM1278_TEMP1_EN)
    >     -			info->func[0] |=
    >     -				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
    >      		if (config & ADM1278_VIN_EN)
    >      			info->func[0] |= PMBUS_HAVE_VIN;
    >      		break;
    >     -- 
    >     2.7.4
    >     
    >     
    >