linux-kernel - Re: [PATCH v4 2/2] gpio: loongson: add more gpio chip 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: <CACRpkdZJVjwOCGYhmzaPLWbcX0rgrDD-d2OX=TnmYs0F56fGug@mail.gmail.com>
Date:   Wed, 23 Aug 2023 14:29:25 +0200
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>,
        Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jianmin Lv <lvjianmin@...ngson.cn>, wanghongliang@...ngson.cn,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v4 2/2] gpio: loongson: add more gpio chip support

Hi Yinbo,

thanks for the new patch, it's starting to look really good!
The main point with offsets in the match data is very nice.

On Wed, Aug 23, 2023 at 5:34 AM Yinbo Zhu <zhuyinbo@...ngson.cn> wrote:

> This patch was to add loongson 2k0500, 2k2000 and 3a5000 gpio chip
> driver support.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
(...)


>  static int loongson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
>  {
> +       unsigned int u;
>         struct platform_device *pdev = to_platform_device(chip->parent);
> +       struct loongson_gpio_chip *lgpio = to_loongson_gpio_chip(chip);
> +
> +       if (lgpio->chip_data->mode == BIT_CTRL_MODE) {
> +               u = readl(lgpio->reg_base + lgpio->chip_data->inten_offset + offset / 32 * 4);
> +               u |= BIT(offset % 32);
> +               writel(u, lgpio->reg_base + lgpio->chip_data->inten_offset + offset / 32 * 4);

This offset / 32 * 4 is really hard to read.
What about

/* Get the register index from offset then multiply by bytes per register */
(offset / 32) * 4

>         lgpio->reg_base = reg_base;
> +       if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
> +               return -EINVAL;
> +
> +       ret = DIV_ROUND_UP(ngpios, 8);
> +       switch (ret) {
> +       case 1 ... 2:
> +               io_width = ret;
> +               break;
> +       case 3 ... 4:
> +               io_width = 0x4;
> +               break;
> +       case 5 ... 8:
> +               io_width = 0x8;
> +               break;
> +       default:
> +               dev_err(dev, "unsupported io width\n");
> +               return -EINVAL;
> +       }

Is it really a good idea to infer the register width from ngpios?

What about just putting this into the struct loongson_gpio_chip_data
as well? Certainly it will be fixed for a certain device.

Yours,
Linus Walleij