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

[Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>]

在 2023/8/23 下午8:29, Linus Walleij 写道:
> 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


okay, I will add it.

> >          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.


Andy's suggestion seems to look better, We have bgpio_init() handle
this switch case. I will remove this switch case.
https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=55b2395e4e92adc492c6b30ac109eb78250dcd9d

Thanks,
Yinbo