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

[WANG Xuerui <kernel@xen0n.name>]

Sorry for jumping in, but...

On 2022/11/15 17:53, Yinbo Zhu wrote:
> 
> 
> 在 2022/11/15 下午5:05, Bartosz Golaszewski 写道:
>> On Mon, Nov 14, 2022 at 10:53 AM Yinbo Zhu <zhuyinbo@loongson.cn> 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@loongson.cn>
>>> Signed-off-by: zhanghongchen <zhanghongchen@loongson.cn>
>>> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
>>> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>>> ---
>>> 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

Why are you trying to support "legacy" LoongArch platforms when the 
architecture was just upstreamed *this year*?

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

I think at this point it's hopefully clear, that the way forward to 
supporting Loongson IP blocks shared between MIPS/LoongArch SoCs is to 
start over and do things properly, making the code as platform-agnostic 
as possible. Just make sure the drivers can get initialized via DT and 
ACPI then you're all set -- the upstream kernel is guaranteed to use one 
of the two well-established boot flows for every Loongson chip it 
supports. Be it hard-coded DT in arch/mips/boot/dts/loongson, or the 
LoongArch ACPI/upcoming DT, no need for hard-coding things in arch/ in 
either case.

>>
>> [snip]
>>
>> I will hold off reviewing the rest of the patch until we get that 
>> clarified.
>>
>> Bartosz
>>
> 

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/