RE: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support

[Sai Krishna Potthuri <lakshmis@xilinx.com>]

Hi Andy Shevchenko,

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, March 19, 2021 3:53 PM
> To: Sai Krishna Potthuri <lakshmis@xilinx.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Rob Herring
> <robh+dt@kernel.org>; Michal Simek <michals@xilinx.com>; Greg Kroah-
> Hartman <gregkh@linuxfoundation.org>; linux-arm Mailing List <linux-arm-
> kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; open
> list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; git <git@xilinx.com>;
> 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 <lakshmis@xilinx.com>
> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at
> > > 10:27 AM Sai Krishna Potthuri
> > > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote:
> 
> ...
> 
> > > > +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.
> 
> > > > +       depends on ARCH_ZYNQMP
> > > > +       select PINMUX
> > > > +       select GENERIC_PINCONF
> 
> ...
> 
> > > > +#include <linux/init.h>
> > > > +#include <linux/of_address.h>
> > >
> > > > +#include <linux/pinctrl/pinmux.h> #include
> > > > +<linux/pinctrl/pinconf-generic.h>
> > >
> > > Can you move this group of headers after linux/* ?
> > >
> > > > +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h>
> > >
> > > Can it be moved outside of these headers
> > >
> > > > +#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
> 
> > #include <linux/pinctrl/*>
> 
> + blank line
Ok, I will add above two blank lines.
> 
> > #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/*.
> 
> > > > +#include "core.h"
> > > > +#include "pinctrl-utils.h"
> 
> ...
> 
> > > > +       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?
> 
> ...
> 
> > > > +       ret = zynqmp_pm_pinctrl_request(pin);
> > > > +       if (ret) {
> > > > +               dev_err(pctldev->dev, "request failed for pin
> > > > + %u\n", pin);
> > >
> > > > +               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. 
> 
> >>  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.
> > >
> > > > +       }
> 
> ...
> 
> > > > +               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.
> 
> ...
> 
> > > > +       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.
> 
> > > > +       if (!fgroups)
> > > > +               return -ENOMEM;
> 
> ...
> 
> > > > +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.
> 
> > I will cross check on this and remove if it is not the case.
> > >
> > > > +       for (pin = 0; pin < zynqmp_desc.npins; pin++) {
> > > > +               ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return ret;
> > > > +}
> 
> ...
> 
> > > > +       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.
> 
> > > > +       if (!groups)
> > > > +               return -ENOMEM;
> 
> --
> With Best Regards,
> Andy Shevchenko