Hi Andy Shevchenko,
Thanks for the review.
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Wednesday, March 17, 2021 6:26 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 v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
>
> On Wed, Mar 17, 2021 at 10:27 AM Sai Krishna Potthuri
> <lakshmi.sai.krishna.potthuri@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > drivers/pinctrl/Kconfig | 13 +
> > drivers/pinctrl/Makefile | 1 +
> > drivers/pinctrl/pinctrl-zynqmp.c | 1030
> > ++++++++++++++++++++++++++++++
> > 3 files changed, 1044 insertions(+)
> > create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c
> >
> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> > 815095326e2d..25d3c7208975 100644
> > --- a/drivers/pinctrl/Kconfig
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ
> > help
> > This selects the pinctrl driver for Xilinx Zynq.
> >
> > +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.
>
> > + depends on ARCH_ZYNQMP
> > + select PINMUX
> > + select GENERIC_PINCONF
> > + 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.
> > +
> > config PINCTRL_INGENIC
> > bool "Pinctrl driver for the Ingenic JZ47xx SoCs"
> > default MACH_INGENIC
> > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> > f53933b2ff02..7e058739f0d5 100644
> > --- a/drivers/pinctrl/Makefile
> > +++ b/drivers/pinctrl/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o
> > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o
> > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o
> > obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o
> > +obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o
> > obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o
> > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o
> > obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o
> > diff --git a/drivers/pinctrl/pinctrl-zynqmp.c
> > b/drivers/pinctrl/pinctrl-zynqmp.c
> > new file mode 100644
> > index 000000000000..63edde400e85
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinctrl-zynqmp.c
> > @@ -0,0 +1,1030 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ZynqMP pin controller
> > + *
> > + * Copyright (C) 2020 Xilinx, Inc.
> > + *
> > + * Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xxxxxxxxxx>
> > + * Rajan Vaja <rajanv@xxxxxxxxxx>
> > + */
> > +
> > +#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>
#include <linux/pinctrl/*>
#include <dt-bindings/pinctrl/pinctrl-zynqmp.h>
>
> > +#include "core.h"
> > +#include "pinctrl-utils.h"
> > +
> > +#define ZYNQMP_PIN_PREFIX "MIO"
> > +#define PINCTRL_GET_FUNC_NAME_RESP_LEN 16
> > +#define MAX_FUNC_NAME_LEN 16
> > +#define MAX_GROUP_PIN 50
> > +#define END_OF_FUNCTIONS "END_OF_FUNCTIONS"
> > +#define NUM_GROUPS_PER_RESP 6
> > +
> > +#define PINCTRL_GET_FUNC_GROUPS_RESP_LEN 12
> > +#define PINCTRL_GET_PIN_GROUPS_RESP_LEN 12
> > +#define NA_GROUP -1
> > +#define RESERVED_GROUP -2
> > +
> > +#define DRIVE_STRENGTH_2MA 2
> > +#define DRIVE_STRENGTH_4MA 4
> > +#define DRIVE_STRENGTH_8MA 8
> > +#define DRIVE_STRENGTH_12MA 12
> > +
> > +/**
> > + * struct zynqmp_pmux_function - a pinmux function
> > + * @name: Name of the pinmux function
> > + * @groups: List of pingroups for this function
> > + * @ngroups: Number of entries in @groups
>
> > + * @node:` Firmware node matching with for function
>
> Typo
>
> > + *
> > + * This structure holds information about pin control function
> > + * and function group names supporting that function.
> > + */
> > +struct zynqmp_pmux_function {
> > + char name[MAX_FUNC_NAME_LEN];
> > + const char * const *groups;
> > + unsigned int ngroups;
> > +};
> > +
> > +/**
> > + * struct zynqmp_pinctrl - driver data
> > + * @pctrl: Pinctrl device
>
> Pin control
>
> > + * @groups: Pingroups
>
> Pin groups
>
> > + * @ngroups: Number of @groups
> > + * @funcs: Pinmux functions
>
> Pin mux functions
I will fix all of them in the next version.
>
> > + * @nfuncs: Number of @funcs
> > + *
> > + * This struct is stored as driver data and used to retrieve
> > + * information regarding pin control functions, groups and
> > + * group pins.
> > + */
> > +struct zynqmp_pinctrl {
> > + struct pinctrl_dev *pctrl;
> > + const struct zynqmp_pctrl_group *groups;
> > + unsigned int ngroups;
> > + const struct zynqmp_pmux_function *funcs;
> > + unsigned int nfuncs;
> > +};
> > +
> > +/**
> > + * struct zynqmp_pctrl_group - Pin control group info
> > + * @name: Group name
> > + * @pins: Group pin numbers
> > + * @npins: Number of pins in group
> > + */
> > +struct zynqmp_pctrl_group {
> > + const char *name;
> > + unsigned int pins[MAX_GROUP_PIN];
> > + unsigned int npins;
> > +};
> > +
> > +/**
> > + * enum zynqmp_pin_config_param - possible pin configuration
> parameters
> > + * @PIN_CONFIG_IOSTANDARD: if the pin can select an IO standard,
> > + * the argument to this parameter (on a
> > + * custom format) tells the driver which
> > + * alternative IO standard to use
> > + */
> > +enum zynqmp_pin_config_param {
> > + 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.
>
> > +static const struct pinconf_generic_params zynqmp_dt_params[] = {
> > + {"io-standard", PIN_CONFIG_IOSTANDARD,
> IO_STANDARD_LVCMOS18},
> > +};
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +static const struct
> > +pin_config_item zynqmp_conf_items[ARRAY_SIZE(zynqmp_dt_params)] =
> {
> > + PCONFDUMP(PIN_CONFIG_IOSTANDARD, "io-standard", NULL, true),
> > +}; #endif
> > +
> > +static struct pinctrl_desc zynqmp_desc;
> > +
> > +/**
> > + * zynqmp_pctrl_get_groups_count() - get group count
> > + * @pctldev: Pincontrol device pointer.
> > + *
> > + * Get total groups count.
> > + *
> > + * Return: group count.
> > + */
> > +static int zynqmp_pctrl_get_groups_count(struct pinctrl_dev *pctldev)
> > +{
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + return pctrl->ngroups;
> > +}
> > +
> > +/**
> > + * zynqmp_pctrl_get_group_name() - get group name
> > + * @pctldev: Pincontrol device pointer.
> > + * @selector: Group ID.
>
> > + *
> > + * Get gorup's name.
> > + *
> > + * Return: group name.
>
> Isn't too much duplication without any added value for the reader?
I will remove the kernel doc for the obvious ones in the next series.
>
> > + */
> > +static const char *zynqmp_pctrl_get_group_name(struct pinctrl_dev
> *pctldev,
> > + unsigned int selector)
> > +{
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + return pctrl->groups[selector].name; }
> > +
> > +/**
> > + * zynqmp_pctrl_get_group_pins() - get group pins
> > + * @pctldev: Pincontrol device pointer.
> > + * @selector: Group ID.
> > + * @pins: Pin numbers.
> > + * @npins: Number of pins in group.
> > + *
> > + * Get gorup's pin count and pin number.
>
> > + * Return: Success.
>
> Clean up all your kernel docs from noise like this.
I will remove the documentation for the obvious ones in the next
series.
>
> > + */
> > +static int zynqmp_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
> > + unsigned int selector,
> > + const unsigned int **pins,
> > + unsigned int *npins) {
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + *pins = pctrl->groups[selector].pins;
> > + *npins = pctrl->groups[selector].npins;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pinctrl_ops zynqmp_pctrl_ops = {
> > + .get_groups_count = zynqmp_pctrl_get_groups_count,
> > + .get_group_name = zynqmp_pctrl_get_group_name,
> > + .get_group_pins = zynqmp_pctrl_get_group_pins,
> > + .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> > + .dt_free_map = pinctrl_utils_free_map, };
> > +
> > +/**
> > + * zynqmp_pinmux_request_pin() - Request a pin for muxing
> > + * @pctldev: Pincontrol device pointer.
> > + * @pin: Pin number.
> > + *
> > + * Request a pin from firmware for muxing.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinmux_request_pin(struct pinctrl_dev *pctldev,
> > + unsigned int pin) {
> > + int ret;
> > +
> > + 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? 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.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_pmux_get_functions_count() - get number of functions
> > + * @pctldev: Pincontrol device pointer.
> > + *
> > + * Get total function count.
> > + *
> > + * Return: function count.
> > + */
> > +static int zynqmp_pmux_get_functions_count(struct pinctrl_dev
> > +*pctldev) {
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + return pctrl->nfuncs;
> > +}
> > +
> > +/**
> > + * zynqmp_pmux_get_function_name() - get function name
> > + * @pctldev: Pincontrol device pointer.
> > + * @selector: Function ID.
> > + *
> > + * Get function's name.
> > + *
> > + * Return: function name.
> > + */
> > +static const char *zynqmp_pmux_get_function_name(struct pinctrl_dev
> *pctldev,
> > + unsigned int
> > +selector) {
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + return pctrl->funcs[selector].name; }
> > +
> > +/**
> > + * zynqmp_pmux_get_function_groups() - Get groups for the function
> > + * @pctldev: Pincontrol device pointer.
> > + * @selector: Function ID
> > + * @groups: Group names.
> > + * @num_groups: Number of function groups.
> > + *
> > + * Get function's group count and group names.
> > + *
> > + * Return: Success.
> > + */
> > +static int zynqmp_pmux_get_function_groups(struct pinctrl_dev *pctldev,
> > + unsigned int selector,
> > + const char * const **groups,
> > + unsigned * const
> > +num_groups) {
> > + struct zynqmp_pinctrl *pctrl =
> > +pinctrl_dev_get_drvdata(pctldev);
> > +
> > + *groups = pctrl->funcs[selector].groups;
> > + *num_groups = pctrl->funcs[selector].ngroups;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_pinmux_set_mux() - Set requested function for the group
> > + * @pctldev: Pincontrol device pointer.
> > + * @function: Function ID.
> > + * @group: Group ID.
> > + *
> > + * Loop though all pins of group and call firmware API
> > + * to set requested function for all pins in group.
>
> through
> of the group
> in the group
>
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > + unsigned int function,
> > + unsigned int group) {
> > + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[group];
> > + int ret, i;
> > +
> > + for (i = 0; i < pgrp->npins; i++) {
> > + unsigned int pin = pgrp->pins[i];
> > +
> > + ret = zynqmp_pm_pinctrl_set_function(pin, function);
> > + if (ret) {
> > + dev_err(pctldev->dev, "set mux failed for pin %u\n",
> > + pin);
>
> > + return -EIO;
>
> Same as above.
> And applies to all similar cases.
>
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_pinmux_release_pin() - Release a pin
> > + * @pctldev: Pincontrol device pointer.
> > + * @pin: Pin number.
> > + *
> > + * Release a pin from firmware.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinmux_release_pin(struct pinctrl_dev *pctldev,
> > + unsigned int pin) {
> > + int ret;
> > +
> > + ret = zynqmp_pm_pinctrl_release(pin);
> > + if (ret) {
> > + dev_err(pctldev->dev, "free pin failed for pin %u\n",
> > + pin);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pinmux_ops zynqmp_pinmux_ops = {
> > + .request = zynqmp_pinmux_request_pin,
> > + .get_functions_count = zynqmp_pmux_get_functions_count,
> > + .get_function_name = zynqmp_pmux_get_function_name,
> > + .get_function_groups = zynqmp_pmux_get_function_groups,
> > + .set_mux = zynqmp_pinmux_set_mux,
> > + .free = zynqmp_pinmux_release_pin, };
> > +
> > +/**
> > + * zynqmp_pinconf_cfg_get() - get config value for the pin
> > + * @pctldev: Pin control device pointer.
> > + * @pin: Pin number.
> > + * @config: Value of config param.
> > + *
> > + * Get value of the requested configuration parameter for the
> > + * given pin.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinconf_cfg_get(struct pinctrl_dev *pctldev,
> > + unsigned int pin,
> > + unsigned long *config) {
> > + int ret;
> > + unsigned int arg = 0, param =
> > +pinconf_to_config_param(*config);
>
> Reversed xmas tree order?
> Assignment of arg AFAICS is redundant.
I will update this in the next version.
>
> > + if (pin >= zynqmp_desc.npins)
> > + return -EOPNOTSUPP;
> > +
> > + 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;
> > +
> > + 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;
> > +
> > + 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;
> > +
> > + arg = 1;
> > + break;
> > + case PIN_CONFIG_IOSTANDARD:
> > + 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;
> > + }
> > +
> > + 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.
>
> > +}
> > +
> > +/**
> > + * zynqmp_pinconf_cfg_set() - Set requested config for the pin
> > + * @pctldev: Pincontrol device pointer.
> > + * @pin: Pin number.
> > + * @configs: Configuration to set.
> > + * @num_configs: Number of configurations.
> > + *
>
> > + * Loop though all configurations and call firmware API
>
> through
>
> > + * to set requested configurations for the pin.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinconf_cfg_set(struct pinctrl_dev *pctldev,
> > + unsigned int pin, unsigned long *configs,
> > + unsigned int num_configs) {
> > + int i, ret;
> > +
> > + if (pin >= zynqmp_desc.npins)
> > + return -EOPNOTSUPP;
> > +
> > + for (i = 0; i < num_configs; i++) {
> > + unsigned int param = pinconf_to_config_param(configs[i]);
> > + unsigned int arg = pinconf_to_config_argument(configs[i]);
> > + unsigned int value;
> > +
> > + switch (param) {
> > + case PIN_CONFIG_SLEW_RATE:
> > + param = PM_PINCTRL_CONFIG_SLEW_RATE;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + param = PM_PINCTRL_CONFIG_PULL_CTRL;
> > + arg = PM_PINCTRL_BIAS_PULL_UP;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg);
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + param = PM_PINCTRL_CONFIG_PULL_CTRL;
> > + arg = PM_PINCTRL_BIAS_PULL_DOWN;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg);
> > + break;
> > + case PIN_CONFIG_BIAS_DISABLE:
> > + param = PM_PINCTRL_CONFIG_BIAS_STATUS;
> > + arg = PM_PINCTRL_BIAS_DISABLE;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg);
> > + break;
> > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg);
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + switch (arg) {
> > + case DRIVE_STRENGTH_2MA:
> > + value = PM_PINCTRL_DRIVE_STRENGTH_2MA;
> > + break;
> > + case DRIVE_STRENGTH_4MA:
> > + value = PM_PINCTRL_DRIVE_STRENGTH_4MA;
> > + break;
> > + case DRIVE_STRENGTH_8MA:
> > + value = PM_PINCTRL_DRIVE_STRENGTH_8MA;
> > + break;
> > + case DRIVE_STRENGTH_12MA:
> > + value = PM_PINCTRL_DRIVE_STRENGTH_12MA;
> > + break;
> > + default:
> > + /* Invalid drive strength */
> > + dev_warn(pctldev->dev,
> > + "Invalid drive strength for pin %d\n",
> > + pin);
> > + return -EINVAL;
> > + }
> > +
> > + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH;
> > + ret = zynqmp_pm_pinctrl_set_config(pin, param, value);
> > + break;
> > + case PIN_CONFIG_IOSTANDARD:
> > + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS;
> > + ret = zynqmp_pm_pinctrl_get_config(pin, param,
> > + &value);
> > +
> > + if (arg != value)
> > + dev_warn(pctldev->dev,
> > + "Invalid IO Standard requested for pin %d\n",
> > + pin);
> > +
> > + break;
> > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> > + case PIN_CONFIG_LOW_POWER_MODE:
> > + /*
> > + * These cases are mentioned in dts but configurable
> > + * registers are unknown. So falling through to ignore
> > + * boot time warnings as of now.
> > + */
> > + ret = 0;
> > + break;
> > + default:
> > + dev_warn(pctldev->dev,
> > + "unsupported configuration parameter '%u'\n",
> > + param);
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + param = pinconf_to_config_param(configs[i]);
> > + arg = pinconf_to_config_argument(configs[i]);
> > + if (ret)
> > + dev_warn(pctldev->dev,
> > + "%s failed: pin %u param %u value %u\n",
> > + __func__, pin, param, arg);
> > + }
>
> Remove all these __func__. In a properly formed (unique) message base you
> don't need it. For the debugging Dynamic debug and other mechanisms allow
> to get it at run-time w/o code recompilation.
I will remove using '__func__' in the next series.
>
> > + return 0;
> > +}
> > +
> > +/**
> > + * zynqmp_pinconf_group_set() - Set requested config for the group
> > + * @pctldev: Pincontrol device pointer.
> > + * @selector: Group ID.
> > + * @configs: Configuration to set.
> > + * @num_configs: Number of configurations.
> > + *
> > + * Call function to set configs for each pin in group.
>
> in the group
I will update in next series.
>
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinconf_group_set(struct pinctrl_dev *pctldev,
> > + unsigned int selector,
> > + unsigned long *configs,
> > + unsigned int num_configs) {
> > + int i, ret;
> > + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > + const struct zynqmp_pctrl_group *pgrp =
> > +&pctrl->groups[selector];
> > +
> > + for (i = 0; i < pgrp->npins; i++) {
> > + ret = zynqmp_pinconf_cfg_set(pctldev, pgrp->pins[i], configs,
> > + num_configs);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pinconf_ops zynqmp_pinconf_ops = {
> > + .is_generic = true,
> > + .pin_config_get = zynqmp_pinconf_cfg_get,
> > + .pin_config_set = zynqmp_pinconf_cfg_set,
> > + .pin_config_group_set = zynqmp_pinconf_group_set, };
> > +
> > +static struct pinctrl_desc zynqmp_desc = {
> > + .name = "zynqmp_pinctrl",
> > + .owner = THIS_MODULE,
> > + .pctlops = &zynqmp_pctrl_ops,
> > + .pmxops = &zynqmp_pinmux_ops,
> > + .confops = &zynqmp_pinconf_ops, #ifdef CONFIG_DEBUG_FS
> > + .custom_conf_items = zynqmp_conf_items, #endif };
> > +
> > +/**
> > + * zynqmp_pinctrl_get_function_groups() - get groups for the function
> > + * @fid: Function ID.
> > + * @index: Group index.
> > + * @groups: Groups data.
> > + *
> > + * Call firmware API to get groups for the given function.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_get_function_groups(u32 fid, u32 index, u16
> > +*groups) {
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
>
> ret_ is confusing. Drop it.
Ok, will just drop 'ret_' prefix and simply use paylod in next version.
>
> > + int ret;
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_GROUPS;
> > + qdata.arg1 = fid;
> > + qdata.arg2 = index;
> > +
> > + ret = zynqmp_pm_query_data(qdata, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(groups, &ret_payload[1],
> > + PINCTRL_GET_FUNC_GROUPS_RESP_LEN);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_get_func_num_groups() - get number of groups in
> function
> > + * @fid: Function ID.
> > + * @ngroups: Number of groups in function.
> > + *
> > + * Call firmware API to get number of group in function.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_get_func_num_groups(u32 fid, unsigned int
> > +*ngroups) {
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > + int ret;
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTION_GROUPS;
> > + qdata.arg1 = fid;
> > +
> > + ret = zynqmp_pm_query_data(qdata, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + *ngroups = ret_payload[1];
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_prepare_func_groups() - prepare function and groups
> data
> > + * @dev: Device pointer.
> > + * @fid: Function ID.
> > + * @func: Function data.
> > + * @groups: Groups data.
> > + *
> > + * Query firmware to get group IDs for each function. Firmware
> > +returns
> > + * group IDs. Based on gorup index for the function, group names in
>
> group
>
> > + * function are stored. For example, first gorup in "eth0" function
>
> the function
> the first
> group
>
> > + * is named as "eth0_0", second as "eth0_1" and so on.
>
> Spell check your comments.
Will check and fix in the next version.
>
> > + *
> > + * Based on group ID received from firmware, function stores name of
> > + * group for that group ID. For an 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.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32
> fid,
> > + struct zynqmp_pmux_function *func,
> > + struct
> > +zynqmp_pctrl_group *groups) {
> > + u16 resp[NUM_GROUPS_PER_RESP] = {0};
> > + const char **fgroups;
> > + int ret = 0, index, i;
>
> > + 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.
>
> > + if (!fgroups)
> > + return -ENOMEM;
> > +
> > + for (index = 0; index < func->ngroups; index +=
> NUM_GROUPS_PER_RESP) {
> > + ret = zynqmp_pinctrl_get_function_groups(fid, index, resp);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) {
> > + if (resp[i] == (u16)NA_GROUP)
> > + goto done;
> > +
> > + if (resp[i] == (u16)RESERVED_GROUP)
> > + continue;
> > +
> > + fgroups[index + i] = devm_kasprintf(dev, GFP_KERNEL,
> > + "%s_%d_grp",
> > + func->name,
> > + index + i);
> > + groups[resp[i]].name = devm_kasprintf(dev, GFP_KERNEL,
> > + "%s_%d_grp",
> > + func->name,
> > + index +
> > + i);
>
> No NULL checks?!
I will update in the next series.
>
> > + }
> > + }
> > +done:
> > + func->groups = fgroups;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_get_function_name() - get function name
> > + * @fid: Function ID.
> > + * @name: Function name
> > + *
> > + * Call firmware API to get name of given function.
> > + */
> > +static void zynqmp_pinctrl_get_function_name(u32 fid, char *name) {
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_NAME;
> > + qdata.arg1 = fid;
> > +
> > + /*
> > + * Name of the function is maximum 16 bytes and cannot
> > + * accommodate the return value in SMC buffers, hence ignoring
> > + * the return value for this specific qid.
> > + */
> > + zynqmp_pm_query_data(qdata, ret_payload);
> > + memcpy(name, ret_payload, PINCTRL_GET_FUNC_NAME_RESP_LEN);
> }
> > +
> > +/**
> > + * zynqmp_pinctrl_get_num_functions() - get number of supported
> functions
> > + * @nfuncs: Number of functions.
> > + *
> > + * Call firmware API to get number of functions supported by
> system/board.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_get_num_functions(unsigned int *nfuncs) {
>
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
>
> Drop in all cases those redundant prefixes.
>
> > + int ret;
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTIONS;
> > +
> > + ret = zynqmp_pm_query_data(qdata, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + *nfuncs = ret_payload[1];
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_get_pin_groups() - get groups for the pin
> > + * @pin: Pin number.
> > + * @index: Group index.
> > + * @groups: Groups data.
> > + *
> > + * Call firmware API to get groups for the given pin.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_get_pin_groups(u32 pin, u32 index, u16
> > +*groups) {
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > + int ret;
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_PIN_GROUPS;
> > + qdata.arg1 = pin;
> > + qdata.arg2 = index;
> > +
> > + ret = zynqmp_pm_query_data(qdata, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + memcpy(groups, &ret_payload[1],
> > + PINCTRL_GET_PIN_GROUPS_RESP_LEN);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_group_add_pin() - add pin to given group
> > + * @group: Group data.
> > + * @pin: Pin number.
> > + *
> > + * Add pin number to respective group's pin array at end and
> > + * increment pin count for the group.
> > + */
> > +static void zynqmp_pinctrl_group_add_pin(struct zynqmp_pctrl_group
> *group,
> > + unsigned int pin) {
> > + group->pins[group->npins++] = pin; }
> > +
> > +/**
> > + * zynqmp_pinctrl_create_pin_groups() - assign pins to respective groups
> > + * @dev: Device pointer.
> > + * @groups: Groups data.
> > + * @pin: Pin number.
> > + *
> > + * Query firmware to get groups available for the given pin.
> > + * Based on firmware response(group IDs for the pin), add
> > + * pin number to respective group's pin array.
> > + *
> > + * Once all pins are queries, each groups would have its number
> > + * of pins and pin numbers data.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_create_pin_groups(struct device *dev,
> > + struct zynqmp_pctrl_group *groups,
> > + unsigned int pin) {
> > + int ret, i, index = 0;
> > + u16 resp[NUM_GROUPS_PER_RESP] = {0};
> > +
> > + do {
> > + ret = zynqmp_pinctrl_get_pin_groups(pin, index, resp);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) {
> > + if (resp[i] == (u16)NA_GROUP)
> > + return ret;
> > +
> > + if (resp[i] == (u16)RESERVED_GROUP)
> > + continue;
>
> In the entire driver remove all explicit castings where it's not needed, like
> above. If something is not okay with it, fix the root cause.
Ok, I will check this and fix in next version.
>
> > + zynqmp_pinctrl_group_add_pin(&groups[resp[i]], pin);
> > + }
> > + index += NUM_GROUPS_PER_RESP;
> > + } while (1);
>
> Try to refactor to avoid infinite loops.
I will check and update this in next version.
>
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_prepare_group_pins() - prepare each group's pin data
> > + * @dev: Device pointer.
> > + * @groups: Groups data.
> > + * @ngroups: Number of groups.
> > + *
> > + * Prepare pin number and number of pins data for each pins.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +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.
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;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_prepare_function_info() - prepare function info
> > + * @dev: Device pointer.
> > + * @pctrl: Pin control driver data.
> > + *
> > + * Query firmware for functions, groups and pin information and
> > + * prepare pin control driver data.
> > + *
> > + * Query number of functions and number of function groups (number
> > + * of groups in given function) to allocate required memory buffers
> > + * for functions and groups. Once buffers are allocated to store
> > + * functions and groups data, query and store required information
> > + * (numbe of groups and group names for each function, number of
>
> number
>
> > + * pins and pin numbers for each group).
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_prepare_function_info(struct device *dev,
> > + struct zynqmp_pinctrl
> > +*pctrl) {
> > + struct zynqmp_pmux_function *funcs;
> > + struct zynqmp_pctrl_group *groups;
> > + int ret, i;
> > +
> > + ret = zynqmp_pinctrl_get_num_functions(&pctrl->nfuncs);
> > + if (ret)
> > + return ret;
> > +
> > + funcs = devm_kzalloc(dev, sizeof(*funcs) * pctrl->nfuncs,
> GFP_KERNEL);
> > + if (!funcs)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < pctrl->nfuncs; i++) {
> > + zynqmp_pinctrl_get_function_name(i, funcs[i].name);
> > +
> > + ret = zynqmp_pinctrl_get_func_num_groups(i,
> &funcs[i].ngroups);
> > + if (ret)
> > + return ret;
> > +
> > + pctrl->ngroups += funcs[i].ngroups;
> > + }
>
> > + 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.
>
> > + if (!groups)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < pctrl->nfuncs; i++) {
> > + ret = zynqmp_pinctrl_prepare_func_groups(dev, i, &funcs[i],
> > + groups);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = zynqmp_pinctrl_prepare_group_pins(dev, groups, pctrl-
> >ngroups);
> > + if (ret)
> > + return ret;
> > +
> > + pctrl->funcs = funcs;
> > + pctrl->groups = groups;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_get_num_pins() - get number of pins in system
> > + * @npins: Number of pins in system/board.
> > + *
> > + * Call firmware API to get number of pins.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_get_num_pins(unsigned int *npins) {
> > + struct zynqmp_pm_query_data qdata = {0};
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > + int ret;
> > +
> > + qdata.qid = PM_QID_PINCTRL_GET_NUM_PINS;
> > +
> > + ret = zynqmp_pm_query_data(qdata, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + *npins = ret_payload[1];
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pinctrl_prepare_pin_desc() - prepare pin description info
> > + * @dev: Device pointer.
> > + * @zynqmp_pins: Pin information.
> > + * @npins: Number of pins.
> > + *
> > + * Query number of pins information from firmware and prepare pin
> > + * description containing pin number and pin name.
> > + *
> > + * Return: 0 on success else error code.
> > + */
> > +static int zynqmp_pinctrl_prepare_pin_desc(struct device *dev,
> > + const struct pinctrl_pin_desc
> > + **zynqmp_pins,
> > + unsigned int *npins) {
> > + struct pinctrl_pin_desc *pins, *pin;
> > + int ret;
> > + int i;
> > +
> > + ret = zynqmp_pinctrl_get_num_pins(npins);
> > + if (ret)
> > + return ret;
> > +
> > + pins = devm_kzalloc(dev, sizeof(*pins) * *npins, GFP_KERNEL);
> > + if (!pins)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < *npins; i++) {
> > + pin = &pins[i];
> > + pin->number = i;
> > + pin->name = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
> > + ZYNQMP_PIN_PREFIX, i);
>
> no NULL check?!
I will update in the next series.
> > + }
> > +
> > + *zynqmp_pins = pins;
> > +
> > + return 0;
> > +}
> > +
> > +static int zynqmp_pinctrl_probe(struct platform_device *pdev) {
> > + struct zynqmp_pinctrl *pctrl;
> > + int ret;
> > +
> > + pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> > + if (!pctrl)
> > + return -ENOMEM;
> > +
> > + ret = zynqmp_pinctrl_prepare_pin_desc(&pdev->dev,
> > + &zynqmp_desc.pins,
> > + &zynqmp_desc.npins);
> > + if (ret) {
> > + dev_err(&pdev->dev, "%s() pin desc prepare fail with %d\n",
> > + __func__, ret);
> > + return ret;
> > + }
> > +
> > + ret = zynqmp_pinctrl_prepare_function_info(&pdev->dev, pctrl);
> > + if (ret) {
> > + dev_err(&pdev->dev, "%s() function info prepare fail with %d\n",
> > + __func__, ret);
> > + return ret;
> > + }
> > +
> > + pctrl->pctrl = pinctrl_register(&zynqmp_desc, &pdev->dev, pctrl);
> > + if (IS_ERR(pctrl->pctrl)) {
>
> > + ret = PTR_ERR(pctrl->pctrl);
> > + return ret;
>
> Fold it and drop {}.
I will change this to 'return PTR_ERR(pctrl->pctrl);' in next version.
>
> > + }
> > +
> > + platform_set_drvdata(pdev, pctrl);
>
> > + dev_info(&pdev->dev, "zynqmp pinctrl initialized\n");
>
> Unneeded noise.
I will remove this in next version.
>
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id zynqmp_pinctrl_of_match[] = {
> > + { .compatible = "xlnx,zynqmp-pinctrl" },
> > + { }
> > +};
> > +
> > +static struct platform_driver zynqmp_pinctrl_driver = {
> > + .driver = {
> > + .name = "zynqmp-pinctrl",
> > + .of_match_table = zynqmp_pinctrl_of_match,
> > + },
> > + .probe = zynqmp_pinctrl_probe, };
> > +builtin_platform_driver(zynqmp_pinctrl_driver);
>
> I believe the code base can be easily shrinked by 10%. So, next version I
> would expect something like < 1000 LOCs in the stat.
By removing the kernel docs for the obvious ones will definitely reach.
Regards
Sai Krishna
>
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: RE: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- From: Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>
- Date: Thu, 18 Mar 2021 14:42:20 +0000
- Accept-language: en-US
- In-reply-to: <CAHp75VehkqzDPQp87JL=hnCi_b4y0d3htpxRKBJ2q73AAAbBXA@mail.gmail.com>
- References: <1615969516-87663-1-git-send-email-lakshmi.sai.krishna.potthuri@xilinx.com> <1615969516-87663-4-git-send-email-lakshmi.sai.krishna.potthuri@xilinx.com> <CAHp75VehkqzDPQp87JL=hnCi_b4y0d3htpxRKBJ2q73AAAbBXA@mail.gmail.com>
- Follow-Ups:
- Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- From: Andy Shevchenko
- Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- References:
- [PATCH v4 0/3] Add ZynqMP pinctrl driver
- From: Sai Krishna Potthuri
- [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- From: Sai Krishna Potthuri
- Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- From: Andy Shevchenko
- [PATCH v4 0/3] Add ZynqMP pinctrl driver
- Prev by Date: Re: setting gpio-line-names in dts for sama5d2 SoC
- Next by Date: Re: setting gpio-line-names in dts for sama5d2 SoC
- Previous by thread: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- Next by thread: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support
- Index(es):
 |