Re: [PATCH v1 1/4] dt-bindings: gpio: rockchip,gpio-bank: add compatible string per SoC

On 1/18/23 16:32, Rob Herring wrote:

On Wed, Jan 18, 2023 at 01:13:23PM +0100, Johan Jonker wrote:

quoted

Currently all Rockchip gpio nodes have the same compatible.
Replace all the compatibles in gpio nodes to be able to
give them a consistent ID independent from probe order or alias.

I fail to see how the compatible change affects probe order or aliases. 
It is also an ABI break if there is not the existing compatible as a 
fallback. State the problem you are trying to solve with this change, 
not just what your solution is.

Hi Rob,

Since the yaml conversion of rockchip,gpio-bank.yaml we have generic "gpio" names instead of "gpio-1".
For both Linux and U-boot there's a need for a consisted ID order between the nodes.

The kernel has no logic to decide between the first compatible and the fallback.
A fallback doesn't have ability to add/select "data" with probe, but have to use
of_device_is_compatible(np, "rockchip,rk3188-gpio-bank") for  "15"  SoCs instead.

I can produce a serie with fall back.
Let us know how to move forward here.

Kind regards,

Johan Jonker


===
Linux driver behavior:

	id = of_alias_get_id(np, "gpio");
	if (id < 0)
		id = gpio++;

Problems:
Alias not always available in existing DT files(not part of the binding)
Probe order is not guarantied and possible number gap for rk3066a between gpio4 and gpio6.
(id counter gives not consistent result)

===
U-boot rk_gpio.c current behavior:

	end = strrchr(dev->name, '@');
	priv->bank = trailing_strtoln(dev->name, end);
	priv->name[0] = 'A' + priv->bank;
	uc_priv->bank_name = priv->name;

Problems:
Crash when node name has no "gpio-1" format

U-boot rk-gpio proposed: 

	priv->name[0] = 'A' + dev_seq(dev);
	uc_priv->bank_name = priv->name;

Problems:
Reduced FDT's and rk3066a gives number gaps.

===

My proposal:

struct lookup_table rk_gpio_rk3188_data[] = {
	{0x2000a000, "A"},
	{0x2003c000, "B"},
	{0x2003e000, "C"},
	{0x20080000, "D"},
};

	{ .compatible = "rockchip,rk3188-gpio-bank", .data = &rk_gpio_rk3188_data },

===


See links for previous discussions:

[PATCH v1] arm: dts: rockchip: sync rk3066/rk3188 DT files from linux-next v6.2-rc4 Johan Jonker
https://lore.kernel.org/linux-rockchip/6ec4427e-f56f-7545-2296-bc75d74f4364@theobroma-systems.com/T/#t (local)

[PATCH 1/3] arm64: dts: rk3399: sync rk3399.dtsi from 6.1-rc1 Peter Robinson
https://lore.kernel.org/u-boot/0ab9a600-b517-0ce5-d189-99fc8eddfa60@theobroma-systems.com/ (local)

[PATCH/RFC] rockchip: derive GPIO bank from alias if available John Keeping
https://lore.kernel.org/u-boot/CAPnjgZ3j7YVJt+B-uEKS_rsSRBSR-kmyLujV3RozL7kbFNrqFw@mail.gmail.com/ (local)

[PATCH] rockchip: derive GPIO bank from alias if available John Keeping
https://lore.kernel.org/u-boot/Y8gaqasu9ho0vl8X@donbot/T/#m00e3823cf74d35a7c8536a1193ed1a77c7698135 (local)

Broken U-boot gpio driver:
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/gpio/rk_gpio.c#L154-L157

GPIO shouldn't really have an alias either IMO.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel