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
