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

[Sai Krishna Potthuri]

Hi Andy Shevchenko,

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Monday, April 26, 2021 7:35 PM
> To: Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Greg Kroah-
> Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-arm Mailing List <linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>; open
> list:GPIO SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; git <git@xxxxxxxxxx>;
> saikrishna12468@xxxxxxxxx
> Subject: Re: [PATCH v6 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
> 
> On Mon, Apr 26, 2021 at 4:20 PM Sai Krishna Potthuri
> <lakshmis@xxxxxxxxxx> wrote:
> > > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > Sent: Friday, April 23, 2021 9:24 PM
> > > On Thu, Apr 22, 2021 at 11:31 AM Sai Krishna Potthuri
> > > <lakshmi.sai.krishna.potthuri@xxxxxxxxxx> wrote:
> 
> ...
> 
> > > > +config PINCTRL_ZYNQMP
> > > > +       tristate "Pinctrl driver for Xilinx ZynqMP"
> > > > +       depends on ZYNQMP_FIRMWARE
> > > > +       select PINMUX
> > > > +       select GENERIC_PINCONF
> > > > +       default ZYNQMP_FIRMWARE
> > > > +       help
> > > > +         This selects the pinctrl driver for Xilinx ZynqMP platform.
> > > > +         This driver will query the pin information from the firmware
> > > > +         and allow configuring the pins.
> > > > +         Configuration can include the mux function to select on those
> > > > +         pin(s)/group(s), and various pin configuration parameters
> > > > +         such as pull-up, slew rate, etc.
> > >
> > > Missed module name.
> > Is this (module name) a configuration option in Kconfig?
> 
> It's a text in a free form that sheds light on how the module will be
> named in case the user will choose "m".
Ok, I will add.
> 
> ...
> 
> > > > + * Copyright (C) 2020 Xilinx, Inc.
> > >
> > > 2021?
> > Couple of versions for this patch series sent in 2020, hence maintaining
> > the same.
> > Is it like we maintain the year when this patch series is applied, which is
> > 2021?
> 
> 2020, 2021 sounds okay as well.
Ok, I will update.
> 
> ...
> 
> > > > +       if (pin >= zynqmp_desc.npins)
> > > > +               return -EOPNOTSUPP;
> > >
> > > Is it possible?
> > This is a safe check.
> 
> I.o.w. dead code, right?
> 
> > Pin information will get from dt files/Xilinx firmware (query pin information
> > for a group)/user application and there are chances of getting wrong pin.
> 
> I'm not sure I understand this. How comes that pin control core will
> ask for a pin higher than npins?
Ok, I got your point.
It is duplicate and will remove this check.
> 
> ...
> 
> > > > +               ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > > > +               if (arg != PM_PINCTRL_BIAS_PULL_UP)
> > > > +                       return -EINVAL;
> > >
> > > Error code being shadowed. Instead check it here properly.
> 
> > Are you mentioning the case where ret is also a non-zero?
> > If yes, then I will update this check to
> > if (!ret && arg != PM_PINCTRL_BIAS_PULL_UP)
> >         return -EINVAL;
> 
> No, this is wrong in the same way.
> 
> > ret non-zero case, we are handling at the end of switch case.
> 
> I meant that you need to pass the real return code to the (upper) caller.
Here we are checking for valid argument and not the return value of the API.
If the read value(argument) is not valid and return value of the API is
zero (SUCCESS) then framework expects driver to be returned with
'-EINVAL' and it is a legal error code in this case.
> Ditto for all other cases (mentioned and not mentioned)
> 
> ...
> 
> > > > +                       ret = -EOPNOTSUPP;
> > >
> > > Isn't it ENOTSUP for all cases here?
> > Giving 'Operation Not Supported (EOPNOTSUPP)' error, when
> > driver gets a request for unsupported pin or configuration.
> > Can you please elaborate your question if I didn't answer properly.
> 
> The pin control subsystem along with the GPIO library are using
> -ENOTSUPP error code for internal operations.
> EOPNOTSUPP is the one that should be returned to user space. Is it the
> case here?
Got your point, I will update error code with ENOTSUPP.
> 
> ...
> 
> > > > +};
> > >
> > > > +
> > >
> > > Ditto.
> > I see some drivers are maintaining the extra line in above two cases.
> > We shouldn't maintain extra line after struct declaration?
> 
> What's the point to add more blank lines where they won't add any value?
I will remove.

Regards
Sai Krishna
> 
> > > > +module_platform_driver(zynqmp_pinctrl_driver);
> 
> --
> With Best Regards,
> Andy Shevchenko