lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 1/2] gpio: loongson: add dts and acpi support
From
Date

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
>

\
 
 \ /
  Last update: 2022-11-21 13:44    [W:0.201 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site