Update driver xdpe152c4.c

Update driver xdpe152c4.c

[Shirley.Lin]

[-- Attachment #1.1: Type: text/plain, Size: 1156 bytes --]

Dear Linux Kernel administrators, Good Day.

We have urgent requirement to update driver link, https://github.com/torvalds/linux/blob/master/drivers/hwmon/pmbus/xdpe152c4.c<https://github.com/torvalds/linux/blob/master/drivers/hwmon/pmbus/xdpe152c4.c#L72>

Please help to review the attached Linux Kernel patch for xdpe152xx driver.

PEC Retry Functionality added for both read_word_data() & read_byte_data() cases of PMBUS.

Updated by : Yadav Ashish (PSS PCS RD FW HD) Ashish.Yadav@infineon.com<mailto:Ashish.Yadav@infineon.com>

Kindly leave your comment or give us approval for upstream.

Thanks a lot.
Best Regards,
Shirley Lin

Infineon Technologies Taiwan Co. Ltd.
Field Application Engineer
IFTW PSS SMD GC TM DCO
Office: +886 2 2652 6866
Mobile: +886 9 7822 9671
Shirley.Lin@infineon.com<mailto:Shirley.Lin@infineon.com>

17F, No. 335, Ruiguang Road
Neihu District, Taipei 114063
Taiwan

www.infineon.com<http://www.infineon.com/>  Discoveries<http://www.infineon.com/discoveries>  Facebook<http://www.facebook.com/infineon>  X<https://x.com/Infineon>  LinkedIn<http://www.linkedin.com/company/infineon-technologies>


[-- Attachment #1.2: Type: text/html, Size: 7549 bytes --]

[-- Attachment #2: XDPE152xx-PMBUS-Driver-PEC-RETRY.patch --]
[-- Type: application/octet-stream, Size: 2329 bytes --]

From 16fd1cd36bc890604355ddee0a5753559292f932 Mon Sep 17 00:00:00 2001
From: Ashish Yadav <ashish.yadav@infineon.com>
Date: Sat, 29 Mar 2025 12:25:39 +0530
Subject: [PATCH] XDPE152xx PMBUS Driver: PEC RETRY

xdpe152xx driver retry to read data in case of PEC Error or Nack on PMBUS.

Signed-off-by: Ashish Yadav <ashish.yadav@infineon.com>
---
 drivers/hwmon/pmbus/xdpe152c4.c | 63 ++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/xdpe152c4.c b/drivers/hwmon/pmbus/xdpe152c4.c
index 235e6b41a..a61ba4a56 100644
--- a/drivers/hwmon/pmbus/xdpe152c4.c
+++ b/drivers/hwmon/pmbus/xdpe152c4.c
@@ -10,12 +10,73 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 #include "pmbus.h"
 
-#define XDPE152_PAGE_NUM 2
+#define XDPE152_PAGE_NUM	2
+#define XDPE152_RETRY_BUS_READ	4
+
+static int xdpe152_read_word_data(struct i2c_client *client, int page,
+					int phase, int reg)
+{
+	int ret = 0;
+	int retry = 0;
+
+	/* Virtual PMBUS Command not supported */
+	if (reg >= PMBUS_VIRT_BASE) {
+		ret = -ENXIO;
+		return ret;
+	}
+
+	do {
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret == -EBADMSG || ret == -ENXIO) {
+			/* PEC Error or NACK: try again */
+			retry++;
+			/* Sleep for an approximate time */
+			usleep_range(25, 50);
+			continue;
+		} else {
+			break;
+		}
+
+	} while (retry < XDPE152_RETRY_BUS_READ);
+
+	if (ret < 0)
+		pr_warn("PMBUS READ ERROR:%d\n", ret);
+
+	return ret;
+}
+
+static int xdpe152_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	int ret = 0;
+	int retry = 0;
+
+	do {
+		ret = pmbus_read_byte_data(client, page, reg);
+		if (ret == -EBADMSG || ret == -ENXIO) {
+			/* PEC Error or NACK: try again */
+			retry++;
+			/* Sleep for an approximate time */
+			usleep_range(25, 50);
+			continue;
+		} else {
+			break;
+		}
+
+	} while (retry < XDPE152_RETRY_BUS_READ);
+
+	if (ret < 0)
+		pr_warn("PMBUS READ ERROR:%d\n", ret);
+
+	return ret;
+}
 
 static struct pmbus_driver_info xdpe152_info = {
 	.pages = XDPE152_PAGE_NUM,
+	.read_word_data = xdpe152_read_word_data,
+	.read_byte_data = xdpe152_read_byte_data,
 	.format[PSC_VOLTAGE_IN] = linear,
 	.format[PSC_VOLTAGE_OUT] = linear,
 	.format[PSC_TEMPERATURE] = linear,
-- 
2.39.5


[-- Attachment #3: PEC Retries Proposal_202503255.pdf --]
[-- Type: application/pdf, Size: 545466 bytes --]

Re: Update driver xdpe152c4.c

[Mukesh Kumar Savaliya]

On 4/7/2025 9:22 AM, Shirley.Lin@infineon.com wrote:
> Dear Linux Kernel administrators, Good Day.
> 
> We have urgent requirement to update driver link, https://github.com/ 
> torvalds/linux/blob/master/drivers/hwmon/pmbus/xdpe152c4.c <https:// 
> github.com/torvalds/linux/blob/master/drivers/hwmon/pmbus/xdpe152c4.c#L72>
> 
> Please help to reviewthe attached Linux Kernel patch for xdpe152xx driver.
> 
This is not the way to upstream from your side and get it reviewed by 
others.

please follow standard upstream process. Review others drivers the way 
it's uploaded first time and then raise new patch.

https://www.kernel.org/doc/html/v4.10/process/submitting-drivers.html
https://www.kernel.org/doc/html/v4.10/process/howto.html
> PEC Retry Functionality added for both read_word_data() 
> & read_byte_data() cases of PMBUS.
> 
> Updated by : Yadav Ashish (PSS PCS RD FW HD) Ashish.Yadav@infineon.com 
> <mailto:Ashish.Yadav@infineon.com>
> 
> Kindly leave your comment or give us approval for upstream.
> 
> Thanks a lot.
> 
> Best Regards,
> 
> *Shirley Lin*
> 
> *Infineon Technologies Taiwan Co. Ltd.*
> 
> Field Application Engineer
> 
> IFTW PSS SMD GC TM DCO
> 
> Office: +886 2 2652 6866
> 
> Mobile: +886 9 7822 9671
> 
> Shirley.Lin@infineon.com <mailto:Shirley.Lin@infineon.com>
> 
> 17F, No. 335, Ruiguang Road
> 
> Neihu District, Taipei 114063
> 
> Taiwan
> 
> www.infineon.com <http://www.infineon.com/> Discoveries <http:// 
> www.infineon.com/discoveries> Facebook <http://www.facebook.com/ 
> infineon> X <https://x.com/Infineon> LinkedIn <http://www.linkedin.com/ 
> company/infineon-technologies>
> 

Re: Update driver xdpe152c4.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]


> https://www.kernel.org/doc/html/v4.10/process/submitting-drivers.html
> https://www.kernel.org/doc/html/v4.10/process/howto.html

Basically correct, but v4.10 is quite old. May I recommend the latest
documentation?

https://www.kernel.org/doc/html/latest/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/howto.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Update driver xdpe152c4.c

[Mukesh Kumar Savaliya]

On 4/7/2025 11:42 AM, Wolfram Sang wrote:
> 
>> https://www.kernel.org/doc/html/v4.10/process/submitting-drivers.html
>> https://www.kernel.org/doc/html/v4.10/process/howto.html
> 
> Basically correct, but v4.10 is quite old. May I recommend the latest
> documentation?
> 
Thanks for sharing the latest.
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> https://www.kernel.org/doc/html/latest/process/howto.html
> 

RE: Update driver xdpe152c4.c

[Shirley.Lin]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]


Thanks a lot for prompt reviewing and kind comments from Mukesh Kumar Savaliya 
<quic_msavaliy@quicinc.com> and Wolfram Sang 
<wsa+renesas@sang-engineering.com>. We will include/amend with patch and 
resend. Sincerely appreciate your kind help.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4214 bytes --]

Re: Update driver xdpe152c4.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]


> Please help to review the attached Linux Kernel patch for xdpe152xx driver.

To shorten things, you could return -EAGAIN as an error code, then the
I2C core will retry the message for you. Within the configured limits
for the controller.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Update driver xdpe152c4.c

[Guenter Roeck]

On 4/6/25 23:15, Wolfram Sang wrote:
> 
>> Please help to review the attached Linux Kernel patch for xdpe152xx driver.
> 
> To shorten things, you could return -EAGAIN as an error code, then the
> I2C core will retry the message for you. Within the configured limits
> for the controller.
> 

The patch neglects to state the _reason_ for the change.

I'd like to know what causes the problem before applying a change like this.
I suspect that the chip needs either a delay between accesses or a delay
after a write. Both are supported by the PMBus core.

Guenter

Re: Update driver xdpe152c4.c

[Guenter Roeck]

On 4/7/25 07:01, Guenter Roeck wrote:
> On 4/6/25 23:15, Wolfram Sang wrote:
>>
>>> Please help to review the attached Linux Kernel patch for xdpe152xx driver.
>>
>> To shorten things, you could return -EAGAIN as an error code, then the
>> I2C core will retry the message for you. Within the configured limits
>> for the controller.
>>
> 
> The patch neglects to state the _reason_ for the change.
> 
> I'd like to know what causes the problem before applying a change like this.
> I suspect that the chip needs either a delay between accesses or a delay
> after a write. Both are supported by the PMBus core.
> 

And this is a complete no-go:

+	if (ret < 0)
+		pr_warn("PMBUS READ ERROR:%d\n", ret);

It isn't even dev_warn(), and I am not going to accept patches which will
clog the kernel log for every single error return. Just imagine how the
kernel log would look like if every driver would log errors like this.

Guenter

RE: Update driver xdpe152c4.c

[Ashish.Yadav]

Hi Guenter, Wolfram and Mukesh, 

I hope you are doing well.
Thanks for your suggestion/feedback. 

Sorry for the inconvenience.
This patch is still in the testing phase and not the final one, that is why  " pr_warn()" is used for debug purposes. 

We will get back to you with the final patch with proper process as suggested by you. 

With Best Regards
   Ashish Yadav


-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: 07 April 2025 20:06
To: Wolfram Sang <wsa+renesas@sang-engineering.com>; Lin Shirley (SMD C3 GC TM DCO) <Shirley.Lin@infineon.com>; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; jdelvare@suse.com; corbet@lwn.net; patrick.rudolph@9elements.com; bhelgaas@google.com; ninad@linux.ibm.com; festevam@denx.de; devicetree@vger.kernel.org; linux-hwmon@vger.kernel.org; linux-i2c@vger.kernel.org; Liu Mills (SMD C3 GC TM DCO) <Mills.Liu@infineon.com>; Yadav Ashish (PSS PCS RD FW HD) <Ashish.Yadav@infineon.com>; Fang Ian (SMD C3 GC TM DCO) <Ian.Fang@infineon.com>
Subject: Re: Update driver xdpe152c4.c

Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.



On 4/7/25 07:01, Guenter Roeck wrote:
> On 4/6/25 23:15, Wolfram Sang wrote:
>>
>>> Please help to review the attached Linux Kernel patch for xdpe152xx driver.
>>
>> To shorten things, you could return -EAGAIN as an error code, then 
>> the I2C core will retry the message for you. Within the configured 
>> limits for the controller.
>>
>
> The patch neglects to state the _reason_ for the change.
>
> I'd like to know what causes the problem before applying a change like this.
> I suspect that the chip needs either a delay between accesses or a 
> delay after a write. Both are supported by the PMBus core.
>

And this is a complete no-go:

+       if (ret < 0)
+               pr_warn("PMBUS READ ERROR:%d\n", ret);

It isn't even dev_warn(), and I am not going to accept patches which will clog the kernel log for every single error return. Just imagine how the kernel log would look like if every driver would log errors like this.

Guenter

Re: Update driver xdpe152c4.c

[Guenter Roeck]

On 4/7/25 20:28, Ashish.Yadav@infineon.com wrote:
> Hi Guenter, Wolfram and Mukesh,
> 
> I hope you are doing well.
> Thanks for your suggestion/feedback.
> 
> Sorry for the inconvenience.
> This patch is still in the testing phase and not the final one, that is why  " pr_warn()" is used for debug purposes.
> 
> We will get back to you with the final patch with proper process as suggested by you.
> 

Please provide an answer for:

 > I'd like to know what causes the problem before applying a change like this.
 > I suspect that the chip needs either a delay between accesses or a
 > delay after a write. Both are supported by the PMBus core.

There are various microcontroller based PMBus chips. Those microcontrollers
sometimes need a delay between accesses to work properly. That is why
access_delay, write_delay, and now page_change_delay are supported by the
PMBus core. It is at least somewhat likely that the xdpe152c4 has this
problem. The delay between retries in your code is an indication that some
delay between accesses may indeed be needed.

Another concern is the need for retry due to PEC errors. That would normally
be due to a bad board design, which is not chip dependent.

Long story short, so far not a single PMBus chip needs retries. It is very unlikely
that this chip is different. You'll have to provide substantially more details
for me to accept retries instead of access/write delays in the driver.

Thanks,
Guenter

> With Best Regards
>     Ashish Yadav
> 
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: 07 April 2025 20:06
> To: Wolfram Sang <wsa+renesas@sang-engineering.com>; Lin Shirley (SMD C3 GC TM DCO) <Shirley.Lin@infineon.com>; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; jdelvare@suse.com; corbet@lwn.net; patrick.rudolph@9elements.com; bhelgaas@google.com; ninad@linux.ibm.com; festevam@denx.de; devicetree@vger.kernel.org; linux-hwmon@vger.kernel.org; linux-i2c@vger.kernel.org; Liu Mills (SMD C3 GC TM DCO) <Mills.Liu@infineon.com>; Yadav Ashish (PSS PCS RD FW HD) <Ashish.Yadav@infineon.com>; Fang Ian (SMD C3 GC TM DCO) <Ian.Fang@infineon.com>
> Subject: Re: Update driver xdpe152c4.c
> 
> Caution: This e-mail originated outside Infineon Technologies. Please be cautious when sharing information or opening attachments especially from unknown senders. Refer to our intranet guide<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx> to help you identify Phishing email.
> 
> 
> 
> On 4/7/25 07:01, Guenter Roeck wrote:
>> On 4/6/25 23:15, Wolfram Sang wrote:
>>>
>>>> Please help to review the attached Linux Kernel patch for xdpe152xx driver.
>>>
>>> To shorten things, you could return -EAGAIN as an error code, then
>>> the I2C core will retry the message for you. Within the configured
>>> limits for the controller.
>>>
>>
>> The patch neglects to state the _reason_ for the change.
>>
>> I'd like to know what causes the problem before applying a change like this.
>> I suspect that the chip needs either a delay between accesses or a
>> delay after a write. Both are supported by the PMBus core.
>>
> 
> And this is a complete no-go:
> 
> +       if (ret < 0)
> +               pr_warn("PMBUS READ ERROR:%d\n", ret);
> 
> It isn't even dev_warn(), and I am not going to accept patches which will clog the kernel log for every single error return. Just imagine how the kernel log would look like if every driver would log errors like this.
> 
> Guenter
> 
>