Messages in this thread | | | Date | Tue, 15 Nov 2022 18:17:23 +0800 | Subject | Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support | From | WANG Xuerui <> |
| |
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/
| |