Messages in this thread | | | Subject | Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support | From | Yinbo Zhu <> | Date | Wed, 16 Nov 2022 11:31:30 +0800 |
| |
在 2022/11/15 下午9:35, Bartosz Golaszewski 写道: > On Tue, Nov 15, 2022 at 2:07 PM Yinbo Zhu <zhuyinbo@loongson.cn> wrote: >> >> >> >> 在 2022/11/15 下午6:17, 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*? >> the leagacy gpio driver had support LoongArch, and you can find some >> gpio register defined in arch/loongarch/include >> /asm/loongson.h in legacy gpio driver, such as LOONGSON_GPIODATA, >> The legacy gpio driver is the driver that doesn't include my gpio patch. >>> >>>> 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. >> Our old platforms are used by customers, but we will not maintain those >> platforms. Adding dts/acpi support to those old platforms not only makes >> no sense, but also affects their use. Because the configuration of >> dts/acpi requires the support of the firmware team, but in fact, we have >> no one to maintain those old platforms. >> >> in a words, My patch to upstream was supposed to consider dts/acpi in >> LoongArch platform But I have to consider the original legacy gpio >> driver and to compatible with it. > > Which legacy driver are you referring to? Neither gpio-loongson nor > gpio-loongson1 define any platform data. If it wasn't needed before > then it's sure we won't be adding it in 2022. If you have board-files > upstream that need to use this driver then you can do fine with > regular device properties. in fact, It is my gpio driver need to be compatible with gpio-loongson.c, because I need use this name gpio-loongson.c and doesn't drop this driver.
The legacy driver also uses a set of common definitions, but does not use the structure to encapsulate it. then use platform_device_register to register device, and my driver is to suppot dts/acpi gpio and need separate platform device register logic, so gpio device need a platform data to register device. It's confusing that as for some legacy platform device drvier to set loongson platform device properties that required by gpio driver, but if supply a platform data for device drvier it will clear.
in addition, I have a look about include/linux/platform_data/ I think it is appropriate for me to add gpio platform data here
Thanks
BRs, Yinbo > > Bartosz >
| |