From: Sai Krishna Potthuri <lakshmis@xilinx.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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" <saikrishna12468@gmail.com>
Subject: RE: [PATCH v6 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
Date: Mon, 26 Apr 2021 13:20:54 +0000 [thread overview]
Message-ID: <DM5PR02MB3877B234F85F3B4887DF3A95BD429@DM5PR02MB3877.namprd02.prod.outlook.com> (raw)
In-Reply-To: <CAHp75VfCbbnN-TBJiYFb=6Rhf30jA-Hz1p1UORsubF7UG6-ATw@mail.gmail.com>
Hi Andy Shevchenko,
Thanks for the review.
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, April 23, 2021 9:24 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 v6 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
>
> On Thu, Apr 22, 2021 at 11:31 AM Sai Krishna Potthuri
> <lakshmi.sai.krishna.potthuri@xilinx.com> wrote:
> >
> > Adding pinctrl driver for Xilinx ZynqMP platform.
> > This driver queries pin information from firmware and registers pin
> > control accordingly.
> >
> > Signed-off-by: Sai Krishna Potthuri
> > <lakshmi.sai.krishna.potthuri@xilinx.com>
>
> You may reduce the number of LOCs by joining some lines. See below.
>
> ...
>
> > +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?
>
> ...
>
> > +/*
> > + * ZynqMP pin controller
> > + *
> > + * 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?
>
> > + *
> > + * Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
> > + * Rajan Vaja <rajan.vaja@xilinx.com> */
>
> ...
>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/firmware/xlnx-zynqmp.h>
>
> ...
>
> > +static int zynqmp_pinconf_cfg_get(struct pinctrl_dev *pctldev,
> > + unsigned int pin,
> > + unsigned long *config)
> > +{
> > + unsigned int arg, param = pinconf_to_config_param(*config);
> > + int ret;
>
> > + if (pin >= zynqmp_desc.npins)
> > + return -EOPNOTSUPP;
>
> Is it possible?
This is a safe check.
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.
>
> > + switch (param) {
> > + case PIN_CONFIG_SLEW_RATE:
> > + param = PM_PINCTRL_CONFIG_SLEW_RATE;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + param = PM_PINCTRL_CONFIG_PULL_CTRL;
>
> > + 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;
ret non-zero case, we are handling at the end of switch case.
>
> > + arg = 1;
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + param = PM_PINCTRL_CONFIG_PULL_CTRL;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + if (arg != PM_PINCTRL_BIAS_PULL_DOWN)
> > + return -EINVAL;
>
> Ditto.
Same as above.
>
> > + arg = 1;
> > + break;
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + param = PM_PINCTRL_CONFIG_BIAS_STATUS;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + if (arg != PM_PINCTRL_BIAS_DISABLE)
> > + return -EINVAL;
>
> Ditto.
Same as above.
>
> > + arg = 1;
> > + break;
> > + case PIN_CONFIG_POWER_SOURCE:
> > + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + break;
> > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg);
> > + switch (arg) {
> > + case PM_PINCTRL_DRIVE_STRENGTH_2MA:
> > + arg = DRIVE_STRENGTH_2MA;
> > + break;
> > + case PM_PINCTRL_DRIVE_STRENGTH_4MA:
> > + arg = DRIVE_STRENGTH_4MA;
> > + break;
> > + case PM_PINCTRL_DRIVE_STRENGTH_8MA:
> > + arg = DRIVE_STRENGTH_8MA;
> > + break;
> > + case PM_PINCTRL_DRIVE_STRENGTH_12MA:
> > + arg = DRIVE_STRENGTH_12MA;
> > + break;
> > + default:
> > + /* Invalid drive strength */
> > + dev_warn(pctldev->dev,
> > + "Invalid drive strength for pin %d\n",
> > + pin);
> > + return -EINVAL;
> > + }
> > + break;
> > + default:
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + if (ret)
> > + return ret;
> > +
> > + param = pinconf_to_config_param(*config);
> > + *config = pinconf_to_config_packed(param, arg);
> > +
> > + return 0;
> > +}
>
> ...
>
> > + 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.
>
> ...
>
> > + ret = zynqmp_pm_query_data(qdata, payload);
> > + if (ret)
> > + return ret;
> > +
> > + *ngroups = payload[1];
> > +
>
> > + return ret;
>
> return 0;
I will fix.
>
> ...
>
> > + * Query firmware to get group IDs for each function. Firmware returns
> > + * group IDs. Based on group index for the function, group names in
>
> on the group
>
> > + * the function are stored. For example, the first group in "eth0" function
> > + * is named as "eth0_0" and second group as "eth0_1" and so on.
>
> and the second
>
> > + *
> > + * Based on the group ID received from the firmware, function stores
> name of
> > + * the group for that group ID. For example, if "eth0" first group ID
> > + * is x, groups[x] name will be stored as "eth0_0".
> > + *
> > + * Once done for each function, each function would have its group names
> > + * and each groups would also have their names.
>
> each group
I will fix all the above.
>
> ...
>
> > +done:
> > + func->groups = fgroups;
> > +
> > + return ret;
>
> return 0; ?
>
> ...
>
> > + *nfuncs = payload[1];
> > +
> > + return ret;
>
> Ditto.
>
> ...
>
> > + ret = zynqmp_pm_query_data(qdata, payload);
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(groups, &payload[1],
> PINCTRL_GET_PIN_GROUPS_RESP_LEN);
> > +
> > + return ret;
>
> Ditto.
>
> ...
>
> > + * Query firmware to get groups available for the given pin.
> > + * Based on the firmware response(group IDs for the pin), add
> > + * pin number to the respective group's pin array.
> > + *
> > + * Once all pins are queries, each groups would have its number
>
> each group
>
> > + * of pins and pin numbers data.
>
> ...
>
> > + return ret;
>
> return 0;
>
> ...
>
> > + * Query number of functions and number of function groups (number
> > + * of groups in given function) to allocate required memory buffers
>
> in the given
>
> > + * for functions and groups. Once buffers are allocated to store
> > + * functions and groups data, query and store required information
> > + * (number of groups and group names for each function, number of
> > + * pins and pin numbers for each group).
>
> ...
>
> > + pctrl->funcs = funcs;
> > + pctrl->groups = groups;
> > +
> > + return ret;
>
> return 0;
>
> ...
>
> > + *npins = payload[1];
> > +
> > + return ret;
>
> Ditto.
I will fix all the above similar comments.
>
> ...
>
> > + dev_err(&pdev->dev, "pin desc prepare fail with %d\n",
> > + ret);
>
> One line.
>
> ...
>
> > + dev_err(&pdev->dev, "function info prepare fail with %d\n",
> > + ret);
>
> Ditto.
I will fix all.
>
> ...
>
> > + pctrl->pctrl = pinctrl_register(&zynqmp_desc, &pdev->dev, pctrl);
>
> devm_pinctrl_register()
I will update.
>
> > + if (IS_ERR(pctrl->pctrl))
> > + return PTR_ERR(pctrl->pctrl);
>
> ...
>
> > +};
>
> > +
>
> Extra blank line.
>
> > +MODULE_DEVICE_TABLE(of, zynqmp_pinctrl_of_match);
>
> ...
>
> > +};
>
> > +
>
> Ditto.
I see some drivers are maintaining the extra line in above two cases.
We shouldn't maintain extra line after struct declaration?
Regards
Sai Krishna
>
> > +module_platform_driver(zynqmp_pinctrl_driver);
>
> --
> With Best Regards,
> Andy Shevchenko
next prev parent reply other threads:[~2021-04-26 13:20 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 8:29 [PATCH v6 0/3] Add ZynqMP pinctrl driver Sai Krishna Potthuri
2021-04-22 8:30 ` [PATCH v6 1/3] firmware: xilinx: Add pinctrl support Sai Krishna Potthuri
2021-04-22 8:30 ` [PATCH v6 2/3] dt-bindings: pinctrl: Add binding for ZynqMP pinctrl driver Sai Krishna Potthuri
2021-04-22 8:30 ` [PATCH v6 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support Sai Krishna Potthuri
2021-04-23 15:53 ` Andy Shevchenko
2021-04-26 13:20 ` Sai Krishna Potthuri [this message]
2021-04-26 14:04 ` Andy Shevchenko
2021-04-27 7:23 ` Michal Simek
2021-04-27 7:31 ` Andy Shevchenko
2021-04-27 7:38 ` Michal Simek
2021-04-27 8:39 ` Andy Shevchenko
2021-04-27 9:59 ` Michal Simek
2021-04-27 14:04 ` Andy Shevchenko
2021-04-28 5:33 ` Sai Krishna Potthuri
2021-05-11 12:38 ` Sai Krishna Potthuri
2021-06-17 6:37 ` Sai Krishna Potthuri
2021-06-17 7:18 ` Andy Shevchenko
2021-06-17 7:31 ` Greg Kroah-Hartman
2021-04-22 9:13 ` [PATCH v6 0/3] Add ZynqMP pinctrl driver Linus Walleij
2021-04-23 15:54 ` Andy Shevchenko
2021-04-29 14:21 ` Linus Walleij
2021-04-29 14:32 ` Sai Krishna Potthuri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM5PR02MB3877B234F85F3B4887DF3A95BD429@DM5PR02MB3877.namprd02.prod.outlook.com \
--to=lakshmis@xilinx.com \
--cc=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=git@xilinx.com \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michals@xilinx.com \
--cc=robh+dt@kernel.org \
--cc=saikrishna12468@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).