| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <832e76a4-6881-4bc8-af17-4a449ebbea30@gmail.com>
Date: Wed, 29 Nov 2023 12:18:34 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, p.zabel@...gutronix.de, j.neuschaefer@....net,
linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH v2 4/4] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO
driver
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@...il.com> 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
Powered by blists - more mailing lists