From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Wells Lu <wellslutw@gmail.com>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
devicetree <devicetree@vger.kernel.org>,
"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
"Wells Lu 呂芳騰" <wells.lu@sunplus.com>,
dvorkin@tibbo.com
Subject: Re: [PATCH v5 2/2] pinctrl: Add driver for Sunplus SP7021
Date: Sat, 25 Dec 2021 17:32:37 +0200 [thread overview]
Message-ID: <CAHp75Vd3iMM+NteJXP_mMAyw5momk3xzp1Y2GX-YJZfFSAwo9A@mail.gmail.com> (raw)
In-Reply-To: <1640331779-18277-3-git-send-email-wellslutw@gmail.com>
On Sat, Dec 25, 2021 at 3:44 AM Wells Lu <wellslutw@gmail.com> wrote:
>
> Add driver for Sunplus SP7021 SoC.
Thanks for an update, my comments below.
...
> +config PINCTRL_SPPCTL
> + bool "Sunplus SP7021 PinMux and GPIO driver"
Why bool and not tristate?
> + depends on SOC_SP7021
> + depends on OF && HAS_IOMEM
...
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>
Can you move this group...
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
...to be somewhere here?
> +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
...
> +/* inline functions */
Useless.
...
> + mask = GENMASK(bit_sz - 1, 0) << (bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);
> + reg = mask | (val << bit_off);
Now you may do one step forward:
mask = GENMASK(bit_sz - 1, 0) << SPPCTL_GROUP_PINMUX_MASK_SHIFT;
reg = (val | mask) << bit_off;
...
> +static void sppctl_first_master_set(struct gpio_chip *chip, unsigned int offset,
> + enum mux_first_reg first, enum mux_master_reg master)
> +{
> + struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> + u32 reg_off, bit_off, reg;
> + int val;
> +
> + /* FIRST register */
> + if (first != mux_f_keep) {
> + /*
> + * Refer to descriptions of function sppctl_first_get()
> + * for usage of FIRST registers.
> + */
> + reg_off = (offset / 32) * 4;
> + bit_off = offset % 32;
> +
> + reg = sppctl_first_readl(spp_gchip, reg_off);
> + val = (reg & BIT(bit_off)) ? 1 : 0;
> + if (first != val) {
first is enum, val is int, are you sure it's good to compare like this?
> + if (first == mux_f_gpio)
> + reg |= BIT(bit_off);
> + else
> + reg &= ~BIT(bit_off);
Since you operate against enums it's better to use switch-case.
> + sppctl_first_writel(spp_gchip, reg, reg_off);
> + }
> + }
> +
> + /* MASTER register */
> + if (master != mux_m_keep) {
> + /*
> + * Refer to descriptions of function sppctl_master_get()
> + * for usage of MASTER registers.
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> +
> + reg = BIT(bit_off) << SPPCTL_MASTER_MASK_SHIFT;
> + if (master == mux_m_gpio)
> + reg |= BIT(bit_off);
> + sppctl_gpio_master_writel(spp_gchip, reg, reg_off);
> + }
> +}
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Perhaps a macro with definitive name?
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
You can use it even here:
if (val)
reg = MY_MACRO(bit_off)
else
reg = BIT(...) // perhaps another macro
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
And looking into repetition, you may even have a helper which does
this conditional
static inline u32 sppctl_...()
{
...
return reg;
}
...
> + int ret = 0;
Redudant variable, return directly.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + /*
> + * Upper 16-bit word is mask. Lower 16-bit word is value.
> + * Refer to descriptions of function sppctl_master_get().
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
As I commented above use helper function which takes offset as input
and returns you reg and reg_off.
> + sppctl_gpio_od_writel(spp_gchip, reg, reg_off);
> + break;
> +
> + case PIN_CONFIG_INPUT_ENABLE:
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + ret = sppctl_gpio_direction_output(chip, offset, 0);
> + break;
> +
> + case PIN_CONFIG_PERSIST_STATE:
> + ret = -ENOTSUPP;
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
...
> + if (!of_find_property(pdev->dev.of_node, "gpio-controller", NULL))
> + return dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");
Why do you need this check for?
...
> + gchip->can_sleep = 0;
Besides that it's already cleared, the type here is boolean.
...
> +/* pinconf operations */
Any value of this comment?
> +static int sppctl_pin_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int param = pinconf_to_config_param(*config);
> + unsigned int arg = 0;
Move assignment to where it actually makes sense.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + if (!sppctl_gpio_output_od_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + if (!sppctl_first_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (!sppctl_master_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (sppctl_gpio_get_direction(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + arg = sppctl_gpio_get(&pctl->spp_gchip->chip, pin);
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
...
> + switch (f->type) {
> + case pinmux_type_fpmx: /* fully-pinmux */
Why do you need these comments?
Shouldn't you rather to kernel doc your enum entries?
> + *num_groups = sppctl_pmux_list_sz;
> + *groups = sppctl_pmux_list_s;
> + break;
> +
> + case pinmux_type_grp: /* group-pinmux */
> + if (!f->grps)
> + break;
> +
> + *num_groups = f->gnum;
> + for (i = 0; i < pctl->unq_grps_sz; i++)
> + if (pctl->g2fp_maps[i].f_idx == selector)
> + break;
> + *groups = &pctl->unq_grps[i];
> + break;
> + }
> +/** sppctl_fully_pinmux_conv - Convert GPIO# to fully-pinmux control-field setting
> + *
> + * Each fully-pinmux function can be mapped to any of GPIO 8 ~ 71 by
> + * settings its control-field. Refer to following table:
> + *
> + * control-field | GPIO
> + * --------------+--------
> + * 0 | No map
> + * 1 | 8
> + * 2 | 9
> + * 3 | 10
> + * : | :
> + * 65 | 71
> + */
> +static inline int sppctl_fully_pinmux_conv(unsigned int offset)
> +{
> + return (offset < 8) ? 0 : offset - 7;
> +}
...
> +static const struct pinmux_ops sppctl_pinmux_ops = {
> + .get_functions_count = sppctl_get_functions_count,
> + .get_function_name = sppctl_get_function_name,
> + .get_function_groups = sppctl_get_function_groups,
> + .set_mux = sppctl_set_mux,
> + .gpio_request_enable = sppctl_gpio_request_enable,
> + .strict = true
+ Comma.
> +};
...
> +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
> + struct pinctrl_map **map, unsigned int *num_maps)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + int nmG = of_property_count_strings(np_config, "groups");
> + const struct sppctl_func *f = NULL;
> + u8 pin_num, pin_type, pin_func;
> + struct device_node *parent;
> + unsigned long *configs;
> + struct property *prop;
> + const char *s_f, *s_g;
> +
> + const __be32 *list;
> + u32 dt_pin, dt_fun;
> + int i, size = 0;
> +
> + list = of_get_property(np_config, "sunplus,pins", &size);
> +
> + if (nmG <= 0)
> + nmG = 0;
> +
> + parent = of_get_parent(np_config);
> + *num_maps = size / sizeof(*list);
> +
> + /*
> + * Process property:
> + * sunplus,pins = < u32 u32 u32 ... >;
> + *
> + * Each 32-bit integer defines a individual pin in which:
> + *
> + * Bit 32~24: defines GPIO pin number. Its range is 0 ~ 98.
> + * Bit 23~16: defines types: (1) fully-pinmux pins
> + * (2) IO processor pins
> + * (3) digital GPIO pins
> + * Bit 15~8: defines pins of peripherals (which are defined in
> + * 'include/dt-binging/pinctrl/sppctl.h').
> + * Bit 7~0: defines types or initial-state of digital GPIO pins.
> + */
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> +
> + /* Check if out of range? */
> + if (pin_num >= sppctl_pins_all_sz) {
> + dev_err(pctldev->dev, "Invalid pin property at index %d (0x%08x)\n",
> + i, dt_pin);
> + return -EINVAL;
> + }
> + }
> +
> + *map = kcalloc(*num_maps + nmG, sizeof(**map), GFP_KERNEL);
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> + pin_type = FIELD_GET(GENMASK(23, 16), dt_pin);
> + pin_func = FIELD_GET(GENMASK(15, 8), dt_pin);
> + (*map)[i].name = parent->name;
> +
> + if (pin_type == SPPCTL_PCTL_G_GPIO) {
> + /* A digital GPIO pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = FIELD_GET(GENMASK(7, 0), dt_pin);
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: GPIO (%s)\n",
> + (*map)[i].data.configs.group_or_pin,
> + (*configs & (SPPCTL_PCTL_L_OUT | SPPCTL_PCTL_L_OU1)) ?
> + "OUT" : "IN");
> + } else if (pin_type == SPPCTL_PCTL_G_IOPP) {
> + /* A IO Processor (IOP) pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = SPPCTL_IOP_CONFIGS;
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: IOP\n",
> + (*map)[i].data.configs.group_or_pin);
> + } else {
> + /* A fully-pinmux pin */
> + (*map)[i].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[i].data.mux.function = sppctl_list_funcs[pin_func].name;
> + (*map)[i].data.mux.group = pin_get_name(pctldev, pin_num);
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", (*map)[i].data.mux.group,
> + (*map)[i].data.mux.function);
> + }
> + }
> +
> + /*
> + * Process properties:
> + * function = "xxx";
> + * groups = "yyy";
> + */
> + if (nmG > 0 && of_property_read_string(np_config, "function", &s_f) == 0) {
> + of_property_for_each_string(np_config, "groups", prop, s_g) {
> + (*map)[*num_maps].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[*num_maps].data.mux.function = s_f;
> + (*map)[*num_maps].data.mux.group = s_g;
> + (*num_maps)++;
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", s_f, s_g);
> + }
> + }
> +
> + /*
> + * Process property:
> + * sunplus,zero_func = < u32 u32 u32 ...>
> + */
> + list = of_get_property(np_config, "sunplus,zero_func", &size);
> + if (list) {
> + for (i = 0; i < (size / sizeof(*list)); i++) {
> + dt_fun = be32_to_cpu(list[i]);
> + if (dt_fun >= sppctl_list_funcs_sz) {
> + dev_err(pctldev->dev, "Zero-func %d out of range!\n",
> + dt_fun);
> + continue;
> + }
> +
> + f = &sppctl_list_funcs[dt_fun];
> + switch (f->type) {
> + case pinmux_type_fpmx:
> + sppctl_func_set(pctl, dt_fun, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + case pinmux_type_grp:
> + sppctl_gmx_set(pctl, f->roff, f->boff, f->blen, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + default:
> + dev_err(pctldev->dev, "Wrong zero-group: %d (%s)\n",
> + dt_fun, f->name);
> + break;
> + }
> + }
> + }
> +
> + of_node_put(parent);
> + dev_dbg(pctldev->dev, "%d pins mapped\n", *num_maps);
> + return 0;
> +}
...
> + sppctl->g2fp_maps = devm_kcalloc(&pdev->dev, sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), GFP_KERNEL);
> + if (!sppctl->g2fp_maps)
> + return -ENOMEM;
> + /*
> + * Check only product of n and size of the second devm_kcalloc()
> + * because its size is the largest of the two.
> + */
> + if (unlikely(check_mul_overflow(sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), &prod)))
> + return -EINVAL;
What the point to check it after? What the point to use it with
kcalloc()? Please, do your homework, i.e. read the code which
implements that.
...
> + struct device_node *np = of_node_get(pdev->dev.of_node);
What's the role of of_node_get()?
...
> + /* Initialize pctl_desc */
Useless. Drop all useless comments like this from the code.
...
> + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech.");
Is it useful?
...
> +#ifndef __SPPCTL_H__
> +#define __SPPCTL_H__
> +
> +#include <linux/bits.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/spinlock.h>
types.h is missed.
...
> +/** enum mux_first_reg - define modes of FIRST register accesses
Fix the multi-line comment style. You mentioned you fixed, but seems
not (not in all places).
> + * - mux_f_mux: Select the pin to a fully-pinmux pin
> + * - mux_f_gpio: Select the pin to a GPIO or IOP pin
> + * - mux_f_keep: Don't change (keep intact)
> + */
> +enum mux_first_reg {
> + mux_f_mux = 0, /* select fully-pinmux */
> + mux_f_gpio = 1, /* select GPIO or IOP pinmux */
> + mux_f_keep = 2, /* keep no change */
> +};
> +struct sppctl_gpio_chip {
> + void __iomem *gpioxt_base; /* MASTER, OE, OUT, IN, I_INV, O_INV, OD */
> + void __iomem *first_base; /* GPIO_FIRST */
> +
> + struct gpio_chip chip;
> + spinlock_t lock; /* lock for accessing OE register */
> +};
Why is this in the header?
...
> +/* SP7021 Pin Controller Driver.
> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> + */
Multi-line comments.
I stopped here, please read my comments for previous versions and here
and try your best.
--
With Best Regards,
Andy Shevchenko
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Wells Lu <wellslutw@gmail.com>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
devicetree <devicetree@vger.kernel.org>,
"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
"Wells Lu 呂芳騰" <wells.lu@sunplus.com>,
dvorkin@tibbo.com
Subject: Re: [PATCH v5 2/2] pinctrl: Add driver for Sunplus SP7021
Date: Sat, 25 Dec 2021 17:32:37 +0200 [thread overview]
Message-ID: <CAHp75Vd3iMM+NteJXP_mMAyw5momk3xzp1Y2GX-YJZfFSAwo9A@mail.gmail.com> (raw)
In-Reply-To: <1640331779-18277-3-git-send-email-wellslutw@gmail.com>
On Sat, Dec 25, 2021 at 3:44 AM Wells Lu <wellslutw@gmail.com> wrote:
>
> Add driver for Sunplus SP7021 SoC.
Thanks for an update, my comments below.
...
> +config PINCTRL_SPPCTL
> + bool "Sunplus SP7021 PinMux and GPIO driver"
Why bool and not tristate?
> + depends on SOC_SP7021
> + depends on OF && HAS_IOMEM
...
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinmux.h>
Can you move this group...
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
...to be somewhere here?
> +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
...
> +/* inline functions */
Useless.
...
> + mask = GENMASK(bit_sz - 1, 0) << (bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);
> + reg = mask | (val << bit_off);
Now you may do one step forward:
mask = GENMASK(bit_sz - 1, 0) << SPPCTL_GROUP_PINMUX_MASK_SHIFT;
reg = (val | mask) << bit_off;
...
> +static void sppctl_first_master_set(struct gpio_chip *chip, unsigned int offset,
> + enum mux_first_reg first, enum mux_master_reg master)
> +{
> + struct sppctl_gpio_chip *spp_gchip = gpiochip_get_data(chip);
> + u32 reg_off, bit_off, reg;
> + int val;
> +
> + /* FIRST register */
> + if (first != mux_f_keep) {
> + /*
> + * Refer to descriptions of function sppctl_first_get()
> + * for usage of FIRST registers.
> + */
> + reg_off = (offset / 32) * 4;
> + bit_off = offset % 32;
> +
> + reg = sppctl_first_readl(spp_gchip, reg_off);
> + val = (reg & BIT(bit_off)) ? 1 : 0;
> + if (first != val) {
first is enum, val is int, are you sure it's good to compare like this?
> + if (first == mux_f_gpio)
> + reg |= BIT(bit_off);
> + else
> + reg &= ~BIT(bit_off);
Since you operate against enums it's better to use switch-case.
> + sppctl_first_writel(spp_gchip, reg, reg_off);
> + }
> + }
> +
> + /* MASTER register */
> + if (master != mux_m_keep) {
> + /*
> + * Refer to descriptions of function sppctl_master_get()
> + * for usage of MASTER registers.
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> +
> + reg = BIT(bit_off) << SPPCTL_MASTER_MASK_SHIFT;
> + if (master == mux_m_gpio)
> + reg |= BIT(bit_off);
> + sppctl_gpio_master_writel(spp_gchip, reg, reg_off);
> + }
> +}
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Perhaps a macro with definitive name?
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
You can use it even here:
if (val)
reg = MY_MACRO(bit_off)
else
reg = BIT(...) // perhaps another macro
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
...
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> + if (val)
> + reg |= BIT(bit_off);
Ditto.
And looking into repetition, you may even have a helper which does
this conditional
static inline u32 sppctl_...()
{
...
return reg;
}
...
> + int ret = 0;
Redudant variable, return directly.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + /*
> + * Upper 16-bit word is mask. Lower 16-bit word is value.
> + * Refer to descriptions of function sppctl_master_get().
> + */
> + reg_off = (offset / 16) * 4;
> + bit_off = offset % 16;
> + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
As I commented above use helper function which takes offset as input
and returns you reg and reg_off.
> + sppctl_gpio_od_writel(spp_gchip, reg, reg_off);
> + break;
> +
> + case PIN_CONFIG_INPUT_ENABLE:
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + ret = sppctl_gpio_direction_output(chip, offset, 0);
> + break;
> +
> + case PIN_CONFIG_PERSIST_STATE:
> + ret = -ENOTSUPP;
> + break;
> +
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
...
> + if (!of_find_property(pdev->dev.of_node, "gpio-controller", NULL))
> + return dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");
Why do you need this check for?
...
> + gchip->can_sleep = 0;
Besides that it's already cleared, the type here is boolean.
...
> +/* pinconf operations */
Any value of this comment?
> +static int sppctl_pin_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> + unsigned long *config)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + unsigned int param = pinconf_to_config_param(*config);
> + unsigned int arg = 0;
Move assignment to where it actually makes sense.
> + switch (param) {
> + case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> + if (!sppctl_gpio_output_od_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + break;
> +
> + case PIN_CONFIG_OUTPUT:
> + if (!sppctl_first_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (!sppctl_master_get(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + if (sppctl_gpio_get_direction(&pctl->spp_gchip->chip, pin))
> + return -EINVAL;
> + arg = sppctl_gpio_get(&pctl->spp_gchip->chip, pin);
> + break;
> +
> + default:
> + return -EOPNOTSUPP;
> + }
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
...
> + switch (f->type) {
> + case pinmux_type_fpmx: /* fully-pinmux */
Why do you need these comments?
Shouldn't you rather to kernel doc your enum entries?
> + *num_groups = sppctl_pmux_list_sz;
> + *groups = sppctl_pmux_list_s;
> + break;
> +
> + case pinmux_type_grp: /* group-pinmux */
> + if (!f->grps)
> + break;
> +
> + *num_groups = f->gnum;
> + for (i = 0; i < pctl->unq_grps_sz; i++)
> + if (pctl->g2fp_maps[i].f_idx == selector)
> + break;
> + *groups = &pctl->unq_grps[i];
> + break;
> + }
> +/** sppctl_fully_pinmux_conv - Convert GPIO# to fully-pinmux control-field setting
> + *
> + * Each fully-pinmux function can be mapped to any of GPIO 8 ~ 71 by
> + * settings its control-field. Refer to following table:
> + *
> + * control-field | GPIO
> + * --------------+--------
> + * 0 | No map
> + * 1 | 8
> + * 2 | 9
> + * 3 | 10
> + * : | :
> + * 65 | 71
> + */
> +static inline int sppctl_fully_pinmux_conv(unsigned int offset)
> +{
> + return (offset < 8) ? 0 : offset - 7;
> +}
...
> +static const struct pinmux_ops sppctl_pinmux_ops = {
> + .get_functions_count = sppctl_get_functions_count,
> + .get_function_name = sppctl_get_function_name,
> + .get_function_groups = sppctl_get_function_groups,
> + .set_mux = sppctl_set_mux,
> + .gpio_request_enable = sppctl_gpio_request_enable,
> + .strict = true
+ Comma.
> +};
...
> +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
> + struct pinctrl_map **map, unsigned int *num_maps)
> +{
> + struct sppctl_pdata *pctl = pinctrl_dev_get_drvdata(pctldev);
> + int nmG = of_property_count_strings(np_config, "groups");
> + const struct sppctl_func *f = NULL;
> + u8 pin_num, pin_type, pin_func;
> + struct device_node *parent;
> + unsigned long *configs;
> + struct property *prop;
> + const char *s_f, *s_g;
> +
> + const __be32 *list;
> + u32 dt_pin, dt_fun;
> + int i, size = 0;
> +
> + list = of_get_property(np_config, "sunplus,pins", &size);
> +
> + if (nmG <= 0)
> + nmG = 0;
> +
> + parent = of_get_parent(np_config);
> + *num_maps = size / sizeof(*list);
> +
> + /*
> + * Process property:
> + * sunplus,pins = < u32 u32 u32 ... >;
> + *
> + * Each 32-bit integer defines a individual pin in which:
> + *
> + * Bit 32~24: defines GPIO pin number. Its range is 0 ~ 98.
> + * Bit 23~16: defines types: (1) fully-pinmux pins
> + * (2) IO processor pins
> + * (3) digital GPIO pins
> + * Bit 15~8: defines pins of peripherals (which are defined in
> + * 'include/dt-binging/pinctrl/sppctl.h').
> + * Bit 7~0: defines types or initial-state of digital GPIO pins.
> + */
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> +
> + /* Check if out of range? */
> + if (pin_num >= sppctl_pins_all_sz) {
> + dev_err(pctldev->dev, "Invalid pin property at index %d (0x%08x)\n",
> + i, dt_pin);
> + return -EINVAL;
> + }
> + }
> +
> + *map = kcalloc(*num_maps + nmG, sizeof(**map), GFP_KERNEL);
> + for (i = 0; i < (*num_maps); i++) {
> + dt_pin = be32_to_cpu(list[i]);
> + pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> + pin_type = FIELD_GET(GENMASK(23, 16), dt_pin);
> + pin_func = FIELD_GET(GENMASK(15, 8), dt_pin);
> + (*map)[i].name = parent->name;
> +
> + if (pin_type == SPPCTL_PCTL_G_GPIO) {
> + /* A digital GPIO pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = FIELD_GET(GENMASK(7, 0), dt_pin);
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: GPIO (%s)\n",
> + (*map)[i].data.configs.group_or_pin,
> + (*configs & (SPPCTL_PCTL_L_OUT | SPPCTL_PCTL_L_OU1)) ?
> + "OUT" : "IN");
> + } else if (pin_type == SPPCTL_PCTL_G_IOPP) {
> + /* A IO Processor (IOP) pin */
> + (*map)[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> + (*map)[i].data.configs.num_configs = 1;
> + (*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
> + configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> + *configs = SPPCTL_IOP_CONFIGS;
> + (*map)[i].data.configs.configs = configs;
> +
> + dev_dbg(pctldev->dev, "%s: IOP\n",
> + (*map)[i].data.configs.group_or_pin);
> + } else {
> + /* A fully-pinmux pin */
> + (*map)[i].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[i].data.mux.function = sppctl_list_funcs[pin_func].name;
> + (*map)[i].data.mux.group = pin_get_name(pctldev, pin_num);
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", (*map)[i].data.mux.group,
> + (*map)[i].data.mux.function);
> + }
> + }
> +
> + /*
> + * Process properties:
> + * function = "xxx";
> + * groups = "yyy";
> + */
> + if (nmG > 0 && of_property_read_string(np_config, "function", &s_f) == 0) {
> + of_property_for_each_string(np_config, "groups", prop, s_g) {
> + (*map)[*num_maps].type = PIN_MAP_TYPE_MUX_GROUP;
> + (*map)[*num_maps].data.mux.function = s_f;
> + (*map)[*num_maps].data.mux.group = s_g;
> + (*num_maps)++;
> +
> + dev_dbg(pctldev->dev, "%s: %s\n", s_f, s_g);
> + }
> + }
> +
> + /*
> + * Process property:
> + * sunplus,zero_func = < u32 u32 u32 ...>
> + */
> + list = of_get_property(np_config, "sunplus,zero_func", &size);
> + if (list) {
> + for (i = 0; i < (size / sizeof(*list)); i++) {
> + dt_fun = be32_to_cpu(list[i]);
> + if (dt_fun >= sppctl_list_funcs_sz) {
> + dev_err(pctldev->dev, "Zero-func %d out of range!\n",
> + dt_fun);
> + continue;
> + }
> +
> + f = &sppctl_list_funcs[dt_fun];
> + switch (f->type) {
> + case pinmux_type_fpmx:
> + sppctl_func_set(pctl, dt_fun, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + case pinmux_type_grp:
> + sppctl_gmx_set(pctl, f->roff, f->boff, f->blen, 0);
> + dev_dbg(pctldev->dev, "%s: No map\n", f->name);
> + break;
> +
> + default:
> + dev_err(pctldev->dev, "Wrong zero-group: %d (%s)\n",
> + dt_fun, f->name);
> + break;
> + }
> + }
> + }
> +
> + of_node_put(parent);
> + dev_dbg(pctldev->dev, "%d pins mapped\n", *num_maps);
> + return 0;
> +}
...
> + sppctl->g2fp_maps = devm_kcalloc(&pdev->dev, sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), GFP_KERNEL);
> + if (!sppctl->g2fp_maps)
> + return -ENOMEM;
> + /*
> + * Check only product of n and size of the second devm_kcalloc()
> + * because its size is the largest of the two.
> + */
> + if (unlikely(check_mul_overflow(sppctl->unq_grps_sz + 1,
> + sizeof(*sppctl->g2fp_maps), &prod)))
> + return -EINVAL;
What the point to check it after? What the point to use it with
kcalloc()? Please, do your homework, i.e. read the code which
implements that.
...
> + struct device_node *np = of_node_get(pdev->dev.of_node);
What's the role of of_node_get()?
...
> + /* Initialize pctl_desc */
Useless. Drop all useless comments like this from the code.
...
> + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech.");
Is it useful?
...
> +#ifndef __SPPCTL_H__
> +#define __SPPCTL_H__
> +
> +#include <linux/bits.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/kernel.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/spinlock.h>
types.h is missed.
...
> +/** enum mux_first_reg - define modes of FIRST register accesses
Fix the multi-line comment style. You mentioned you fixed, but seems
not (not in all places).
> + * - mux_f_mux: Select the pin to a fully-pinmux pin
> + * - mux_f_gpio: Select the pin to a GPIO or IOP pin
> + * - mux_f_keep: Don't change (keep intact)
> + */
> +enum mux_first_reg {
> + mux_f_mux = 0, /* select fully-pinmux */
> + mux_f_gpio = 1, /* select GPIO or IOP pinmux */
> + mux_f_keep = 2, /* keep no change */
> +};
> +struct sppctl_gpio_chip {
> + void __iomem *gpioxt_base; /* MASTER, OE, OUT, IN, I_INV, O_INV, OD */
> + void __iomem *first_base; /* GPIO_FIRST */
> +
> + struct gpio_chip chip;
> + spinlock_t lock; /* lock for accessing OE register */
> +};
Why is this in the header?
...
> +/* SP7021 Pin Controller Driver.
> + * Copyright (C) Sunplus Tech / Tibbo Tech.
> + */
Multi-line comments.
I stopped here, please read my comments for previous versions and here
and try your best.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-12-25 15:33 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-24 7:42 [PATCH v5 0/2] This is a patch series for pinctrl driver of Sunplus SP7021 SoC Wells Lu
2021-12-24 7:42 ` Wells Lu
2021-12-24 7:42 ` [PATCH v5 1/2] dt-bindings: pinctrl: Add dt-bindings for Sunplus SP7021 Wells Lu
2021-12-24 7:42 ` Wells Lu
2021-12-25 14:48 ` Rob Herring
2021-12-25 14:48 ` Rob Herring
2021-12-26 12:44 ` Wells Lu 呂芳騰
2021-12-26 12:44 ` Wells Lu 呂芳騰
2021-12-27 16:02 ` Rob Herring
2021-12-27 16:02 ` Rob Herring
2021-12-27 16:10 ` Rob Herring
2021-12-27 16:10 ` Rob Herring
2021-12-27 17:36 ` Wells Lu 呂芳騰
2021-12-27 17:36 ` Wells Lu 呂芳騰
2021-12-24 7:42 ` [PATCH v5 2/2] pinctrl: Add driver " Wells Lu
2021-12-24 7:42 ` Wells Lu
2021-12-25 15:32 ` Andy Shevchenko [this message]
2021-12-25 15:32 ` Andy Shevchenko
2021-12-28 15:38 ` Wells Lu 呂芳騰
2021-12-28 15:38 ` Wells Lu 呂芳騰
2022-01-10 18:41 ` Andy Shevchenko
2022-01-10 18:41 ` Andy Shevchenko
2022-01-13 17:04 ` Wells Lu 呂芳騰
2022-01-13 17:04 ` Wells Lu 呂芳騰
2022-01-13 18:04 ` Andy Shevchenko
2022-01-13 18:04 ` Andy Shevchenko
2022-01-14 2:51 ` Wells Lu 呂芳騰
2022-01-14 2:51 ` Wells Lu 呂芳騰
2022-01-14 9:46 ` Andy Shevchenko
2022-01-14 9:46 ` Andy Shevchenko
2022-01-14 10:56 ` Wells Lu 呂芳騰
2022-01-14 10:56 ` Wells Lu 呂芳騰
2022-01-14 11:16 ` Andy Shevchenko
2022-01-14 11:16 ` Andy Shevchenko
2022-01-14 11:36 ` Wells Lu 呂芳騰
2022-01-14 11:36 ` Wells Lu 呂芳騰
2022-01-10 18:17 ` Wells Lu 呂芳騰
2022-01-10 18:17 ` Wells Lu 呂芳騰
2022-01-10 18:42 ` Andy Shevchenko
2022-01-10 18:42 ` Andy Shevchenko
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=CAHp75Vd3iMM+NteJXP_mMAyw5momk3xzp1Y2GX-YJZfFSAwo9A@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dvorkin@tibbo.com \
--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=robh+dt@kernel.org \
--cc=wells.lu@sunplus.com \
--cc=wellslutw@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.