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

[Yinbo Zhu <zhuyinbo@loongson.cn>]

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
>