Hi Andy,
Thanks a lot for your review.
Please see my answers below:
> >
> > 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?
Pinctrl driver is selected by many drivers in SP7021 platform.
We never build it as a module, but build-in to kernel.
So we use "bool".
Should we set it to 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?
Yes, I'll move it next patch.
> > +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
>
> ...
>
> > +/* inline functions */
>
> Useless.
I'll remove all this kind of comments next patch.
> ...
>
> > + 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;
I'll revise these statements as code you've shared.
> ...
>
> > +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?
No, it is always not good to compare different type of things.
I'll modify code as shown below:
enum mux_first_reg val;
:
:
val = (reg & BIT(bit_off)) ? mux_f_gpio : mux_f_mux;
> > + 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.
I'll modify code as shown below:
if (first != val)
switch (first) {
case mux_f_gpio:
reg |= BIT(bit_off);
sppctl_first_writel(spp_gchip, reg, reg_off);
break;
case mux_f_mux:
reg &= ~BIT(bit_off);
sppctl_first_writel(spp_gchip, reg, reg_off);
break;
case mux_f_keep:
break;
}
> > + 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?
I'll define two new macros:
#define SPPCTL_MOON_REG_MASK_SHIFT 16
#define SPPCTL_SET_MOON_REG_BIT(bit) (BIT(bit + SPPCTL_MOON_REG_MASK_SHIFT) | BIT(bit))
#define SPPCTL_CLR_MOON_REG_BIT(bit) BIT(bit + SPPCTL_MOON_REG_MASK_SHIFT)
And modify code as shown below:
reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
> ...
>
> > + 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
I'll modify code as shown below:
if (val)
reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
else
reg = SPPCTL_CLR_MOON_REG_BIT(bit_off);
> ...
>
> > + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
>
> Ditto.
I'll modify code as shown below:
reg = SPPCTL_CLR_MOON_REG_BIT(bit_off);
> ...
>
> > + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) | BIT(bit_off);
>
> Ditto.
I'll modify code as shown shown below:
reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
> ...
>
> > + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> > + if (val)
> > + reg |= BIT(bit_off);
>
> Ditto.
I'll modify code as shown below:
if (val)
reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
else
reg = SPPCTL_CLR_MOON_REG_BIT(bit_off);
> ...
>
> > + reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT);
> > + if (val)
> > + reg |= BIT(bit_off);
>
> Ditto.
I'll modify code as shown in above case.
> And looking into repetition, you may even have a helper which does this conditional
>
> static inline u32 sppctl_...()
> {
> ...
> return reg;
> }
I'll add an inline function as shown below:
static inline u32 sppctl_prep_moon_reg(u32 bit_off, int val)
{
if (val)
return SPPCTL_SET_MOON_REG_BIT(bit_off);
else
return SPPCTL_CLR_MOON_REG_BIT(bit_off);
}
And replace all "if (val) ... " with the inline function.
> ...
>
> > + int ret = 0;
>
> Redundant variable, return directly.
I'll remove the redundant variable.
> > + 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.
I'll modify code as shown below:
reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
Sorry, I don't understand your meaning "returns you reg and reg_off".
The helper macro will return reg but not reg_off, right?
> > + 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?
By referring to other pinctrl driver, we check if property "gpio-controller" exists?
Will core help us check this?
Is this redundant?
> ...
>
> > + gchip->can_sleep = 0;
>
> Besides that it's already cleared, the type here is boolean.
I'll remove the unnecessary assignment as the whole structure is initialized to 0.
Should I also remove the assignment:
gchip->base = 0;
> ...
>
> > +/* pinconf operations */
>
> Any value of this comment?
I'll remove it.
> > +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.
I'll move it as shown below:
case PIN_CONFIG_DRIVE_OPEN_DRAIN:
if (!sppctl_gpio_output_od_get(&pctl->spp_gchip->chip, pin))
return -EINVAL;
arg = 0;
break;
> > + 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?
I'll remove the comments.
Could you please tell me where I should write and put my kernel doc?
Is there any examples I can refer to?
> > + *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.
I'll add a comma right after 'true'.
> > +};
>
> ...
>
> > +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.
I'll remove the "if (unlikely(check_mul_overflow()...) return -EINVAL" statement next patch.
I think I mis-understood your previous comment.
I thought I was asked to add check_mul_overflow() function for devm_kcalloc(...).
Sorry for strange codes.
I should study devm_kcalloc() furthermore. Now I know devm_kcalloc(...) does
multiplication overflow check for us. That's why we need to devm_kzalloc() with
devm_kcalloc().
One question left in my mind is, in this case, even we have 10,000 pins,
we will never get overflow. It looks not so necessary.
> ...
>
> > + struct device_node *np = of_node_get(pdev->dev.of_node);
>
> What's the role of of_node_get()?
I'll remove the unused codes.
I think it was used to check if OF node exists.
> ...
>
> > + /* Initialize pctl_desc */
>
> Useless. Drop all useless comments like this from the code.
I'll remove all this kind of comments.
> ...
>
> > + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech.");
>
> Is it useful?
I think yes. It tells users that Pinctrl driver has probed successfully.
If no this message, users don't know if Pinctrl driver has probed
successfully or not. For example, because that dts node of pinctrl is
"disabled" or Pinctrl driver is even not enabled.
Can I keep this?
> ...
>
> > +#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.
I'll add the header file next patch.
> ...
>
> > +/** 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).
I'll fix it.
Sorry for my mistake.
I modified multi-line comments for all.
Later, I added more comments. But I
forgot to switch back from network mode.
> > + * - 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?
Do you mean I need to move this "struct sppctl_gpio_chip { ... }" declaration
to c file because it is only used by the c file?
> ...
>
> > +/* SP7021 Pin Controller Driver.
> > + * Copyright (C) Sunplus Tech / Tibbo Tech.
> > + */
>
> Multi-line comments.
I'll fix it.
Sorry for mistake. I overlooked the file header.
> I stopped here, please read my comments for previous versions and here and try your best.
I read all your comments many times.
I'll do my best to address all comments.
Sorry for mis-understand some comments.
Your previous comments:
> > > > +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config,
> > > > + struct pinctrl_map **map, unsigned
> > > > +int *num_maps) {
> > >
> > > Looking into this rather quite big function why you can't use what pin control core provides?
> >
> > No, we cannot use functions pin-control core provides.
> > Please refer to dt-bindings document, "pinctrl/sunplus,sp7021-pinctrl.yaml".
> > We have more explanation there.
>
> Fine, can you reuse some library functions, etc? Please, consider refactoring to make it more readable.
Could you please share me your idea about "refactoring"?
Or could you give me some hints?
I think many times, but have no idea about refactoring.
> --
> With Best Regards,
> Andy Shevchenko
