Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support

[Yinbo Zhu]

在 2022/11/15 下午5:05, Bartosz Golaszewski 写道:
> On Mon, Nov 14, 2022 at 10:53 AM Yinbo Zhu <zhuyinbo@xxxxxxxxxxx> wrote:
> > The latest Loongson series platform use dts or acpi framework to
> > register gpio device resources, such as the Loongson-2 series
> > SoC of LOONGARCH architecture. In order to support dts, acpi and
> > compatibility with previous platform device resources in driver,
> > this patch was added.
> >
> > Signed-off-by: lvjianmin <lvjianmin@xxxxxxxxxxx>
> > Signed-off-by: zhanghongchen <zhanghongchen@xxxxxxxxxxx>
> > Signed-off-by: Liu Peibao <liupeibao@xxxxxxxxxxx>
> > Signed-off-by: Juxin Gao <gaojuxin@xxxxxxxxxxx>
> > Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> > ---
> > Change in v2:
> >                  1. Fixup of_loongson_gpio_get_props and remove the parse logic about
> >                     "loongson,conf_offset", "loongson,out_offset", "loongson,in_offset",
> >                     "loongson,gpio_base", "loongson,support_irq" then kernel driver will
> >                     initial them that depend compatible except "loongson,gpio_base".
> >
> >   arch/loongarch/include/asm/loongson.h         |  13 +
> >   .../include/asm/mach-loongson2ef/loongson.h   |  12 +
> >   .../include/asm/mach-loongson64/loongson.h    |  13 +
> >   drivers/gpio/Kconfig                          |   6 +-
> >   drivers/gpio/gpio-loongson.c                  | 422 +++++++++++++++---
> >   5 files changed, 391 insertions(+), 75 deletions(-)
> >
> > diff --git a/arch/loongarch/include/asm/loongson.h b/arch/loongarch/include/asm/loongson.h
> > index 00db93edae1b..383fdda155f0 100644
> > --- a/arch/loongarch/include/asm/loongson.h
> > +++ b/arch/loongarch/include/asm/loongson.h
> > @@ -60,6 +60,19 @@ static inline void xconf_writeq(u64 val64, volatile void __iomem *addr)
> >          );
> >   }
> >
> > +/* ============== Data structrues =============== */
> > +
> > +/* gpio data */
> > +struct platform_gpio_data {
> > +       u32 gpio_conf;
> > +       u32 gpio_out;
> > +       u32 gpio_in;
> > +       u32 support_irq;
> > +       char *label;
> > +       int gpio_base;
> > +       int ngpio;
> > +};
>
> This is a terrible name for an exported structure. You would at least
> need some kind of a namespace prefix. But even then the need to add a
> platform data structure is very questionable. We've moved past the
> need for platform data in the kernel. I don't see anyone setting it up
> in this series either. Could you provide more explanation on why you
> would need it and who would use it?
okay, I will add a namespace prefix, about this platform data was added
that was to compatible with legacy platforms that do not support dts or
acpi, then, the mips loongson platform or loongarch loongson platform
can implement the gpio device driver to initialize the
platform_gpio_data structure as needed after exporting the structure.
> > +
> >   /* ============== LS7A registers =============== */
> >   #define LS7A_PCH_REG_BASE              0x10000000UL
> >   /* LPC regs */
> > diff --git a/arch/mips/include/asm/mach-loongson2ef/loongson.h b/arch/mips/include/asm/mach-loongson2ef/loongson.h
> > index ca039b8dcde3..b261cea4fee1 100644
> > --- a/arch/mips/include/asm/mach-loongson2ef/loongson.h
> > +++ b/arch/mips/include/asm/mach-loongson2ef/loongson.h
> > @@ -315,4 +315,16 @@ extern unsigned long _loongson_addrwincfg_base;
> >
> >   #endif /* ! CONFIG_CPU_SUPPORTS_ADDRWINCFG */
> >
> > +/* ============== Data structrues =============== */
> > +
> > +/* gpio data */
> > +struct platform_gpio_data {
> > +       u32 gpio_conf;
> > +       u32 gpio_out;
> > +       u32 gpio_in;
> > +       u32 support_irq;
> > +       char *label;
> > +       int gpio_base;
> > +       int ngpio;
> > +};
>
> No idea why you would need to duplicate it like this either. And why
> put it in arch/.
because loongson platform include mips and loongarch, and the gpio 
device data was defined in arch/ in leagcy loongson gpio driver.  so the
latest loongson gpio drvier add platform_gpio_data in same dir.
> [snip]
>
> I will hold off reviewing the rest of the patch until we get that clarified.
>
> Bartosz