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

["Arnd Bergmann" <arnd@xxxxxxxx>]

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