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
