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

[Linus Walleij]

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.

> +#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.

> +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.

> +       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.

> +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.

> +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.

> +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

> +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.

> +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!

> +               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.

Yours,
Linus Walleij