Hi Andy Shevchenko,

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Monday, March 22, 2021 10:47 PM
To: Sai Krishna Potthuri <redacted>
Cc: Linus Walleij <redacted>; Rob Herring
[off-list ref]; Michal Simek [off-list ref]; Greg Kroah-
Hartman [off-list ref]; linux-arm Mailing List <linux-arm-
kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
kernel@vger.kernel.org>; devicetree [off-list ref]; open
list:GPIO SUBSYSTEM [off-list ref]; git [off-list ref];
saikrishna12468@gmail.com
Subject: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support

On Mon, Mar 22, 2021 at 5:25 PM Sai Krishna Potthuri
[off-list ref] wrote:

quoted

quoted

From: Andy Shevchenko <redacted>
Sent: Friday, March 19, 2021 3:53 PM On Thu, Mar 18, 2021 at 4:42 PM
Sai Krishna Potthuri [off-list ref]
wrote:

quoted

quoted

From: Andy Shevchenko <redacted>
Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at
10:27 AM Sai Krishna Potthuri
[off-list ref] wrote:

...

quoted

quoted

quoted

#include <dt-bindings/pinctrl/pinctrl-zynqmp.h>

Looking into other drivers with similar includes, shouldn't it go
first in the file before any other linux/* asm/* etc?

I see some drivers are including this header before linux/* and some
are using after linux/*.

The rule of thumb is that: more generic headers are going first.

I consider dt/* ones are more generic than linux/* ones because they are
covering more than just the Linux kernel.

Ok, I will reorder accordingly.

...

quoted

quoted

quoted

quoted

I'm lost here. What is IO standard exactly? Why it can't be in
generic headers?

It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts.
Since this is specific to Xilinx ZynqMP platform, considered to be
added in the driver file.

So, why can't we create a couple of bits to represent this voltages
in the generic header and gain usability for others as well?

I see some drivers are maintaining the configuration list in the
driver file, if the configuration is specific to the driver.

Yes, my point is that this case doesn't sound too specific to the driver. Many
pin control buffers (in hardware way of speaking) have properties to be
different voltage tolerant / produce.

Ok, you want me to proceed with this change or shall we wait for
Linus to comment?

quoted

We can move this to generic header if it is used by others as well.
Ok, will wait for Linus to comment.

quoted

Linus?

...

quoted

quoted

quoted

quoted

quoted

+       ret = zynqmp_pm_pinctrl_request(pin);
+       if (ret) {
+               dev_err(pctldev->dev, "request failed for pin
+ %u\n", pin);

quoted

+               return -EIO;

Why shadowing error code?

So, any comments on the initial Q?

Xilinx low level secure firmware error codes are different from Linux error

codes.

quoted

Secure firmware maintains list of error codes (positive values other than

zero).

quoted

Hence we return -EIO, if the return value from firmware is non-zero.

Why the zynqmp_pm_pinctrl_request() can't return codes in Linux error
code space?

I cross checked with the Xilinx firmware team, firmware driver is now returning
Linux error codes in the latest kernel but while developing the driver it was a
different approach. I will update the driver to simply return the values from
firmware calls.

quoted

quoted

quoted

quoted

 Since it's the only possible error, why is it not
reflected in the kernel doc?

I will update the kernel doc with the error value for such cases.

quoted

quoted

+       }

--
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel