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

[Yinbo Zhu]

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