Messages in this thread | | | Subject | Re: [PATCH v4 1/2] gpio: loongson: add dts and acpi support | From | Yinbo Zhu <> | Date | Mon, 21 Nov 2022 20:43:07 +0800 |
| |
Hi Arnd,
I had adop your advice and as v5 series patch. and about move the legacy gpio driver to other deposition that I have internal talk in loongson team and think it should be okay.
BRs, Yinbo. 在 2022/11/17 下午5:55, Arnd Bergmann 写道: > On Thu, Nov 17, 2022, at 04:59, Yinbo Zhu wrote: >> >> config GPIO_LOONGSON >> - bool "Loongson-2/3 GPIO support" >> - depends on CPU_LOONGSON2EF || CPU_LOONGSON64 >> + bool "Loongson series GPIO support" >> + depends on LOONGARCH || COMPILE_TEST > > This looks like it will introduce a regression for users of the > older machines CPU_LOONGSON2EF and CPU_LOONGSON64 machines. > > While the driver previously called 'platform_device_register_simple' > to create the platform device itself, this call is no longer > done anywhere, so it also cannot work here, but whatever was > working should not be broken. I can see two possible ways to do > this: > > a) create the platform_device in the mips code in a way that > the driver can handle it as before > > b) duplicate the entire driver and leave the old code untouched. > > The second one is probably easier here, but the first one would > be nicer in the end, depending on how much of the original > code remains. > >> help >> - Driver for GPIO functionality on Loongson-2F/3A/3B processors. >> + Driver for GPIO functionality on Loongson seires processors. > > s/seires/series/ > >> +static void of_loongson_gpio_get_props(struct device_node *np, >> + struct loongson_gpio_chip *lgpio) >> +{ >> + const char *name; >> + >> + of_property_read_u32(np, "ngpios", (u32 *)&lgpio->chip.ngpio); > > This does not work: chip.ngpio is a 16-bit field, so you > cannot overwrite it using a 32-bit pointer dereference. Just > use a local variable as an intermediate > >> + of_property_read_string(np, "compatible", &name); >> + lgpio->chip.label = kstrdup(name, GFP_KERNEL); >> + if (!strcmp(name, "loongson,ls2k-gpio")) { >> + lgpio->conf_offset = 0x0; > > This probably works, but is not reliable since "compatible" > is an enumeration rather than a single string. Using > of_device_is_compatible() would work here, or even better > you can have a configuration that is referenced from > the 'data' field of the 'of_device_id' > >> +static void acpi_loongson_gpio_get_props(struct platform_device *pdev, >> + struct loongson_gpio_chip *lgpio) >> +{ >> + >> + struct device *dev = &pdev->dev; >> + int rval; >> + >> + device_property_read_u32(dev, "ngpios", (u32 *)&lgpio->chip.ngpio); >> + device_property_read_u32(dev, "gpio_base", (u32 *)&lgpio->chip.base); >> + device_property_read_u32(dev, "conf_offset", >> + (u32 *)&lgpio->conf_offset); >> + device_property_read_u32(dev, "out_offset", >> + (u32 *)&lgpio->out_offset); >> + device_property_read_u32(dev, "in_offset", (u32 *)&lgpio->in_offset); > > This looks worrying: While you addressed the feedback in the > DT binding, the ACPI version still uses the old format, which > the binding is different depending on the firmware. > > A modern driver should not set the "gpio_base" any more, and > the firmware should not care either. > > The other fields appear to correspond to the ones that the DT > version decides based on the device identifier. There isn't > really a point in doing this differently, so pick one version > or the other and then use the same method for both DT and ACPI. > >> +static void platform_loongson_gpio_get_props(struct platform_device *pdev, >> + struct loongson_gpio_chip *lgpio) >> +{ >> +} > >> + if (np) >> + of_loongson_gpio_get_props(np, lgpio); >> + else if (ACPI_COMPANION(&pdev->dev)) >> + acpi_loongson_gpio_get_props(pdev, lgpio); >> + else >> + platform_loongson_gpio_get_props(pdev, lgpio); > > The third branch is clearly broken now as it fails to assign > anything. Using device_property_read_u32() etc should really > work in all three cases, so if you fold the > of_loongson_gpio_get_props and acpi_loongson_gpio_get_props > functions into one, that will solve the third case as well. > >> +static const struct of_device_id loongson_gpio_dt_ids[] = { >> + { .compatible = "loongson,ls2k-gpio"}, >> + { .compatible = "loongson,ls7a-gpio"}, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, loongson_gpio_dt_ids); >> + >> +static const struct acpi_device_id loongson_gpio_acpi_match[] = { >> + {"LOON0002"}, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match); >> + >> static struct platform_driver loongson_gpio_driver = { >> .driver = { >> .name = "loongson-gpio", >> + .owner = THIS_MODULE, >> + .of_match_table = loongson_gpio_dt_ids, >> + .acpi_match_table = ACPI_PTR(loongson_gpio_acpi_match), >> }, > > The ACPI_PTR() macro here means that you get an "unused variable" > warning when the driver is build with CONFIG_ACPI disabled. > I think you should just reference the variable directly. If you > want to save a few bytes, you can keep the ACPI_PTR() here > and enclose the struct definition in #ifdef CONFIG_ACPI. > > Arnd >
| |