Re: [PATCH v2 4/4] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver

[Jacky Huang]

Dear Linus,

Thanks for your review.

On 2023/11/28 下午 06:14, Linus Walleij wrote:
> Hi Jacky,
>
> thanks for your patch!
>
> This is an interesting new driver. The initial review pass will be
> along the lines "utilize helpers and library functions please".
> You will see that this will shrink the core driver and make it
> rely on core code helpers making it much easier to maintain
> in the long run (I think).
>
> On Tue, Nov 28, 2023 at 7:11 AM Jacky Huang <ychuang570808@xxxxxxxxx> wrote:
>
> > +if ARCH_MA35 || COMPILE_TEST
> Isn't it cleaner to put the depends on inside the Kconfig entries?
> This looks a bit convoluted.
>
> > +config PINCTRL_MA35
> > +       bool
> > +       depends on OF
> So
> depends on ARCH_MA35 || COMPILE_TEST here
>
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> > +       select GENERIC_PINCONF
> > +       select GPIOLIB
> > +       select GPIO_GENERIC
> > +       select GPIOLIB_IRQCHIP
> > +       select MFD_SYSCON
> > +
> > +config PINCTRL_MA35D1
> > +       bool "Pinctrl and GPIO driver for Nuvoton MA35D1"
> > +       depends on OF
> Now depends on OF gets listed twice, which is confusing
>
> > +       select PINCTRL_MA35
> So use
> depends on PINCTRL_MA35
>
> instead, and this becomes a sub-choice.

OK, I will fix the above issues. It will be:

config PINCTRL_MA35
    bool "Pinctrl and GPIO driver for Nuvoton MA35 series"
    depends on (ARCH_MA35 || COMPILE_TEST) && OF
    select GENERIC_PINCTRL_GROUPS
    select GENERIC_PINMUX_FUNCTIONS
    select GENERIC_PINCONF
    select GPIOLIB
    select GPIO_GENERIC
    select GPIOLIB_IRQCHIP
    select MFD_SYSCON

config PINCTRL_MA35D1
    bool "Pinctrl and GPIO driver for Nuvoton MA35D1"
    depends on PINCTRL_MA35
    help
      Say Y here to enable pin controller and GPIO support
      for Nuvoton MA35D1 SoC.

> > +#include <linux/clk.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_address.h>
> Do you really need them all?
>
> Then I think you need <linux/platform_device.h> because
> ma35d1_pinctrl_probe(struct platform_device *pdev)
> passes a platform_device into this file.

I will have a compile test and remove unnecessary ones and
add <linux/platform_device.h>.

> > +struct ma35_pin_bank {
> > +       void __iomem            *reg_base;
> > +       struct clk              *clk;
> > +       int                     irq;
> > +       u8                      nr_pins;
> > +       const char              *name;
> > +       u8                      bank_num;
> > +       bool                    valid;
> > +       struct device_node      *of_node;
> Just call the variable *np  ("noide pointer")
> this is the most usual practice despite struct device
> using thus long "of_node" name.

OK, I will use 'struct device_node *np'.

> > +       struct gpio_chip        chip;
> > +       struct irq_chip         irqc;
> Please do not use dynamic irq_chips anymore, use an immutable
> irq_chip, look in other drivers how to do this because we changed
> almost all of them.

I will use immutable instead.
> > +static int ma35_get_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
> > +                              const unsigned int **pins, unsigned int *npins)
> > +{
> > +       struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +       if (selector >= npctl->ngroups)
> > +               return -EINVAL;
> > +
> > +       *pins = npctl->groups[selector].pins;
> > +       *npins = npctl->groups[selector].npins;
> > +
> > +       return 0;
> > +}
> Hm it looks simple.
>
> Have you looked into using CONFIG_GENERIC_PINCTRL_GROUPS
> and then you get a bunch of these functions such as
> pinctrl_generic_get_group_count
> pinctrl_generic_get_group_name
> pinctrl_generic_get_group_name(this function)
> pinctrl_generic_get_group
> pinctrl_generic_group_name_to_selector
> (etc)
>
> for FREE, also using a radix tree which is neat.

Thank you for your reminder. I will look into how to use 
CONFIG_GENERIC_PINCTRL_GROUPS.

> > +static int ma35_pinctrl_dt_node_to_map_func(struct pinctrl_dev *pctldev,
> > +                                           struct device_node *np,
> > +                                           struct pinctrl_map **map,
> > +                                           unsigned int *num_maps)
> > +{
> > +       struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> > +       struct ma35_pin_group *grp;
> > +       struct pinctrl_map *new_map;
> > +       struct device_node *parent;
> > +       int map_num = 1;
> > +       int i;
> > +
> > +       /*
> > +        * first find the group of this node and check if we need create
> > +        * config maps for pins
> > +        */
> > +       grp = ma35_pinctrl_find_group_by_name(npctl, np->name);
> > +       if (!grp) {
> > +               dev_err(npctl->dev, "unable to find group for node %s\n", np->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       map_num += grp->npins;
> > +       new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num, GFP_KERNEL);
> > +       if (!new_map)
> > +               return -ENOMEM;
> > +
> > +       *map = new_map;
> > +       *num_maps = map_num;
> > +       /* create mux map */
> > +       parent = of_get_parent(np);
> > +       if (!parent) {
> > +               devm_kfree(pctldev->dev, new_map);
> > +               return -EINVAL;
> > +       }
> > +
> > +       new_map[0].type = PIN_MAP_TYPE_MUX_GROUP;
> > +       new_map[0].data.mux.function = parent->name;
> > +       new_map[0].data.mux.group = np->name;
> > +       of_node_put(parent);
> > +
> > +       new_map++;
> > +       for (i = 0; i < grp->npins; i++) {
> > +               new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
> > +               new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, grp->pins[i]);
> > +               new_map[i].data.configs.configs = grp->settings[i].configs;
> > +               new_map[i].data.configs.num_configs = grp->settings[i].nconfigs;
> > +       }
> > +       dev_dbg(pctldev->dev, "maps: function %s group %s num %d\n",
> > +               (*map)->data.mux.function, (*map)->data.mux.group, map_num);
> > +
> > +       return 0;
> > +}
> This looks like it could be replaced with:
> pinconf_generic_dt_node_to_map_group
> pinconf_generic_dt_node_to_map_all
>
> please check the generic helpers closely.

Alright, I will try using these helpers to simplify the code.

> > +static void ma35_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
> > +                            unsigned int num_maps)
> > +{
> > +       devm_kfree(pctldev->dev, map);
> > +}
> pinconf_generic_dt_free_map

I will fix it.

> > +static int ma35_pinmux_get_func_count(struct pinctrl_dev *pctldev)
> > +{
> > +       struct ma35_pinctrl *npctl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +       return npctl->nfunctions;
> > +}
> pinmux_generic_get_function_count
> pinmux_generic_get_function_name
> pinmux_generic_get_function_groups
> (etc)
>
> Please check the CONFIG_GENERIC_PINMUX_FUNCTIONS
> option because these are again all very generic.

I will try to use these helpers and have a test.

> > +static int ma35_gpio_core_direction_in(struct gpio_chip *gc, unsigned int gpio)
> > +{
> > +       struct ma35_pin_bank *bank = gpiochip_get_data(gc);
> > +       void __iomem *reg_mode = bank->reg_base + MA35_GP_REG_MODE;
> > +       unsigned long flags;
> > +       unsigned int regval;
> > +
> > +       spin_lock_irqsave(&bank->lock, flags);
> > +
> > +       regval = readl(reg_mode);
> > +
> > +       regval &= ~GENMASK(gpio * 2 + 1, gpio * 2);
> > +       regval |= MA35_GP_MODE_INPUT << gpio * 2;
> > +
> > +       writel(regval, reg_mode);
> > +
> > +       spin_unlock_irqrestore(&bank->lock, flags);
> > +
> > +       return 0;
> > +}
> The pinctrl set_mux is using a regmap but not the GPIO which is a bit
> of a weird mix.
>
> Further, if you were using regmap-mmio for GPIO, you could probably
> utilize CONFIG_GPIO_REGMAP to simplify also this part of the
> code with a library. Look at other drivers using this!

Sorry, I'm not quite clear on the issue. This function is just setting 
the input/output
direction for a GPIO pin. The code here is only configuring the GPIO 
control register.
I've observed similar implementations in other drivers, such as in 
pinctrl-amd.c.


> > +               if (bank->irq > 0) {
> > +                       struct gpio_irq_chip *girq;
> > +
> > +                       girq = &bank->chip.irq;
> > +                       girq->chip = &bank->irqc;
> > +                       girq->chip->name = bank->name;
> > +                       girq->chip->irq_disable = ma35_irq_gpio_mask;
> > +                       girq->chip->irq_enable = ma35_irq_gpio_unmask;
> > +                       girq->chip->irq_set_type = ma35_irq_irqtype;
> > +                       girq->chip->irq_mask = ma35_irq_gpio_mask;
> > +                       girq->chip->irq_unmask = ma35_irq_gpio_unmask;
> > +                       girq->chip->flags = IRQCHIP_MASK_ON_SUSPEND |
> > +                       IRQCHIP_SKIP_SET_WAKE | IRQCHIP_IMMUTABLE;
> > +                       girq->parent_handler = ma35_irq_demux_intgroup;
> > +                       girq->num_parents = 1;
> > +
> > +                       girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents),
> > +                                                    GFP_KERNEL);
> > +                       if (!girq->parents)
> > +                               return -ENOMEM;
> > +
> > +                       girq->parents[0] = bank->irq;
> > +                       girq->default_type = IRQ_TYPE_NONE;
> > +                       girq->handler = handle_level_irq;
> > +               }
> As menioned, replace this with an immutable irq_chip.

OK, I will fix it.

> Yours,
> Linus Walleij

Best Regards,
Jacky Huang