Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 19 Mar 2021 12:23:11 +0200 | 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
> #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
| |