linux-kernel - Re: [PATCH v6 1/2] gpio: loongson: add gpio driver support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ5OJpMFH1Wi31TKQZskQtCmNGyySdkOpouiNW2t_jV6Q@mail.gmail.com>
Date:   Wed, 23 Nov 2022 23:14:31 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Yinbo Zhu <zhuyinbo@...ngson.cn>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        WANG Xuerui <kernel@...0n.name>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Juxin Gao <gaojuxin@...ngson.cn>,
        Bibo Mao <maobibo@...ngson.cn>,
        Yanteng Si <siyanteng@...ngson.cn>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        loongarch@...ts.linux.dev, linux-mips@...r.kernel.org,
        Arnaud Patard <apatard@...driva.com>,
        Huacai Chen <chenhuacai@...nel.org>,
        Jianmin Lv <lvjianmin@...ngson.cn>,
        Hongchen Zhang <zhanghongchen@...ngson.cn>,
        Liu Peibao <liupeibao@...ngson.cn>
Subject: Re: [PATCH v6 1/2] gpio: loongson: add gpio driver support

On Wed, Nov 23, 2022 at 9:04 AM Yinbo Zhu <zhuyinbo@...ngson.cn> wrote:

> The Loongson platforms GPIO controller contains 60 GPIO pins in total,
> 4 of which are dedicated GPIO pins, and the remaining 56 are reused
> with other functions. Each GPIO can set input/output and has the
> interrupt capability.
>
> This driver added support for Loongson GPIO controller and support to
> use DTS or ACPI to descibe GPIO device resources.
>
> Signed-off-by: Jianmin Lv <lvjianmin@...ngson.cn>
> Signed-off-by: Hongchen Zhang <zhanghongchen@...ngson.cn>
> Signed-off-by: Liu Peibao <liupeibao@...ngson.cn>
> Signed-off-by: Juxin Gao <gaojuxin@...ngson.cn>
> Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
> ---
> Change in v6:

This is way better :)

I guess you notice how the driver gets smaller and smaller.
This is a good sign!

> +static int loongson_gpio_request(
> +                       struct gpio_chip *chip, unsigned int pin)
> +{
> +       if (pin >= chip->ngpio)
> +               return -EINVAL;
> +
> +       return 0;
> +}

Drop this altogether as discussed in my other reply.

> +static inline void __set_direction(struct loongson_gpio_chip *lgpio,
> +                       unsigned int pin, int input)
> +static void __set_level(struct loongson_gpio_chip *lgpio, unsigned int pin,
> +                       int high)

I missed this before. Also the use of __underscore for inner functions
is a bad habit IMO (because __underscore is also used for compiler
primitives such as __init which is confusing) The signature of these
functions is too generic. Name them loongson_commit_direction() or
loongson_commit_level() or something.

> +static int loongson_gpio_get_direction(
> +                               struct gpio_chip *chip, unsigned int pin)

thanks for implementing this!

> +       if (lgpio->p_data->mode == BIT_CTRL_MODE) {
> +               ret = bgpio_init(&lgpio->chip, dev, 8,
> +                               LOONGSON_GPIO_IN(lgpio),
> +                               LOONGSON_GPIO_OUT(lgpio), 0,
> +                               LOONGSON_GPIO_OEN(lgpio), NULL, 0);
> +               if (ret) {
> +                       dev_err(dev, "unable to init generic GPIO\n");
> +                       return ret;
> +               }
> +               lgpio->chip.ngpio = ngpios;

Neat!

> +               lgpio->chip.base = 0;

Drop this. It is good that the base is unpredictable so
people don't start to rely on it. (drivers/gpio/TODO)

> +       rval = device_property_read_u16_array(dev, "gsi_idx_map", NULL, 0);

But this gsi_idx_map is missing from your device tree bindings,
is it not?

Or what am I missing here? Sorry I might overlook something...

> +static int loongson_gpio_probe(struct platform_device *pdev)
> +{
> +       void __iomem *reg_base;
> +       struct loongson_gpio_chip *lgpio;
> +       struct device_node *np = pdev->dev.of_node;
> +       struct device *dev = &pdev->dev;
> +
> +       lgpio = devm_kzalloc(dev, sizeof(*lgpio), GFP_KERNEL);
> +       if (!lgpio)
> +               return -ENOMEM;
> +
> +       loongson_gpio_get_props(pdev, lgpio);
> +
> +       lgpio->p_data = device_get_match_data(&pdev->dev);

lgpio->p_data = device_get_match_data(dev);


> +static int __init loongson_gpio_setup(void)
> +{
> +       return platform_driver_register(&loongson_gpio_driver);
> +}
> +postcore_initcall(loongson_gpio_setup);

Why does this have to be postcore_initcall()?

Yours,
Linus Walleij