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

Yinbo Zhu <zhuyinbo@xxxxxxxxxxx> · 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@xxxxxxxxxxx> 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@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

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








