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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Mar 18, 2021 at 4:42 PM Sai Krishna Potthuri
<lakshmis@xxxxxxxxxx> wrote:
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Sent: Wednesday, March 17, 2021 6:26 PM
> > On Wed, Mar 17, 2021 at 10:27 AM Sai Krishna Potthuri
> > <lakshmi.sai.krishna.potthuri@xxxxxxxxxx> 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

> #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?

> > > +#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?

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?

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

...

> > > +       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?

> > > +       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.

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

> > > +       if (!groups)
> > > +               return -ENOMEM;

--
With Best Regards,
Andy Shevchenko