[2/2] pinctrl: starfive: jh7100: ignore disabled device tree nodes

The driver always registers pin configurations in device tree. This can
cause some inconvenience to users, as pin configurations in the base
device tree cannot be disabled in the device tree overlay, even when the
relevant devices are not used.

Ignore disabled pin configuration nodes in device tree.

Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs")
Cc: stable@vger.kernel.org
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Emil,

On Fri, Dec 01, 2023 at 03:28:27PM +0100, Emil Renner Berthing wrote:
> Nam Cao wrote:
> > diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > index 530fe340a9a1..561fd0c6b9b0 100644
> > --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7100.c
> > @@ -492,7 +492,7 @@ static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev,
> >
> > nmaps = 0;
> > ngroups = 0;
> > - for_each_child_of_node(np, child) {
> > + for_each_available_child_of_node(np, child) {
>
> Is this safe to do? I mean will the children considered "available" not change
> as drivers are loaded during boot so this is racy?

I think if node removal like this causes race condition, we would
already have race condition with node addition too: "what if the nodes
are added while the drivers are being loaded?"

At least with U-Boot, the device tree overlay is "merged" into the base
device tree, before the kernel even runs, so no race there. I don't know
if there are any cases where the device tree overlay is not guaranteed
to be applied before driver loading, but those cases do not sound sane
to me: they would cause race condition, regardless of whether nodes are
added or removed.

> Also arguably this is not a bugfix, but a new feature.

I'm not sure myself, I haven't seen official documentation/rules about
this. But many people do consider this to be a bug:

"Though you can add/override 'status' with 'status = "disabled";' which
should be treated very similar to a node not being present. I say
similar because it's a source of bugs for the OS to fail to pay
attention to status property." - Rob Herring [1].

"Linux has widespread use of the "status" property to indicate that a
node does not exist. (...). Expect efforts to fix the kernel code to
respect the "status" property." - elinux.org [2].

And I do agree with them. When someone write a device tree with some
nodes with "status = disabled" for whatever reasons, clearly they intend
to exclude these nodes.

Though I must admit that I am still quite new, so please correct me if
my reasoning/understanding is flawed.

Best regards,
Nam

[1] https://lore.kernel.org/lkml/CAL_JsqLV5d5cL3o3Dx=--zGD37c5O09rL9AXyRFmceTfBHt3Zg@mail.gmail.com/
[2] https://elinux.org/Device_Tree_Linux#status_property

On Fri, Dec 1, 2023 at 10:23 AM Nam Cao <namcao@linutronix.de> wrote:

> The driver always registers pin configurations in device tree. This can
> cause some inconvenience to users, as pin configurations in the base
> device tree cannot be disabled in the device tree overlay, even when the
> relevant devices are not used.
>
> Ignore disabled pin configuration nodes in device tree.
>
> Fixes: ec648f6b7686 ("pinctrl: starfive: Add pinctrl driver for StarFive SoCs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nam Cao <namcao@linutronix.de>

Patch applied for fixes.

If there is some doubts about the saneness of this patch, seek input
from the devicetree maintainers.

Yours,
Linus Walleij