Hi Andy Shevchenko,

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Friday, March 19, 2021 3:53 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 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

+config PINCTRL_ZYNQMP
+       bool "Pinctrl driver for Xilinx ZynqMP"

Why not module?

As most of the Xilinx drivers depending on the pin controller driver
for configuring the MIO pins, we are not supporting to build this
driver as a module.

OK.

quoted

quoted

quoted

+       depends on ARCH_ZYNQMP
+       select PINMUX
+       select GENERIC_PINCONF

...

quoted

quoted

quoted

+#include <linux/init.h>
+#include <linux/of_address.h>

quoted

+#include <linux/pinctrl/pinmux.h> #include
+<linux/pinctrl/pinconf-generic.h>

Can you move this group of headers after linux/* ?

quoted

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

Can it be moved outside of these headers

quoted

+#include <linux/platform_device.h> #include
+<linux/firmware/xlnx-zynqmp.h>

Ordering (for all groups of headers)?

Ok, I will order the headers in the below order #include <linux/*>
#include <linux/firmware/xlnx-zynqmp.h>

+ blank line

quoted

#include <linux/pinctrl/*>

+ blank line

Ok, I will add above two blank lines.

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/*.

quoted

quoted

quoted

+#include "core.h"
+#include "pinctrl-utils.h"

...

quoted

quoted

quoted

+       PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, };

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.
We can move this to generic header if it is used by others as well.
Ok, will wait for Linus to comment.

Linus?

...

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.
Secure firmware maintains list of error codes (positive values other than zero).
Hence we return -EIO, if the return value from firmware is non-zero. 

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

+       }

...

quoted

quoted

quoted

+               default:
+                       /* Invalid drive strength */
+                       dev_warn(pctldev->dev,
+                                "Invalid drive strength for pin %d\n",
+                                pin);
+                       return -EINVAL;
+               }
+               break;
+       default:
+               ret = -EOPNOTSUPP;
+               break;
+       }
+
+       param = pinconf_to_config_param(*config);
+       *config = pinconf_to_config_packed(param, arg);
+
+       return ret;

This is wrong. You have to return the error codes directly and do
not touch *config as long as error happens.

I will update the *config and param under if (!ret) condition.

Simpler and better just to return errors immediately from the switch-case
entries.

Ok, I will update.

...

quoted

quoted

quoted

+       fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups,
+                              GFP_KERNEL);

One line

With single line it is crossing 80 line bar and getting the checkpatch
warning, hence split into two lines.

No, you may not get a checkpatch warning. Are you working on v5.4 kernels
or earlier?

Ok, got it.
Driver is developed on top of the older kernel (<=5.4) and we see this warning at that
time and fixed by splitting into two lines.
Yes, with the latest kernel I am not seeing the warning by keeping it in one
line, I will update this.

quoted

quoted

quoted

+       if (!fgroups)
+               return -ENOMEM;

...

quoted

quoted

quoted

+static int zynqmp_pinctrl_prepare_group_pins(struct device *dev,
+                                            struct zynqmp_pctrl_group *groups,
+                                            unsigned int ngroups) {
+       unsigned int pin;
+       int ret = 0;

Redundant assignment.

Static analyzer tool will throw warning as it expects the variable to
be Initialized in all possible paths.

Because you need to explicitly return 0 at the end of the function.
Don't follow static analyzers or other tools blindly. Think about all aspects.

Ok, I will update this.

quoted

I will cross check on this and remove if it is not the case.

quoted

quoted

+       for (pin = 0; pin < zynqmp_desc.npins; pin++) {
+               ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin);
+               if (ret)
+                       return ret;
+       }
+
+       return ret;
+}

...

quoted

quoted

quoted

+       groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups,
+                             GFP_KERNEL);

One line.

It will cross 80 line mark if we make it to a single line.

I don't think it's a problem in this case.

I will update this.

quoted

quoted

quoted

+       if (!groups)
+               return -ENOMEM;

--
With Best Regards,
Andy Shevchenko