Re: [PATCH v4 2/2] gpio: loongson: add more gpio chip support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

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@xxxxxxxxxxx> wrote:

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


>  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