Re: [PATCH v2] leds: Mark GPIO LED trigger broken

From: Linus Walleij <linus.walleij@linaro.org>
To: "Jan Kundrát" <jan.kundrat@cesnet.cz>
Cc: Lee Jones <lee@kernel.org>, Pavel Machek <pavel@ucw.cz>,
	linux-leds@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Tony Lindgren <tony@atomide.com>, Felipe Balbi <balbi@kernel.org>,
	linux-omap@vger.kernel.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH v2] leds: Mark GPIO LED trigger broken
Date: Mon, 11 Sep 2023 11:17:55 +0200	[thread overview]
Message-ID: <CACRpkdZAvE5ZwVN=G57t_+aMMZ25OBKTgsf9=rRnk3-LnNEqUg@mail.gmail.com> (raw)
In-Reply-To: <1d11e190-df52-4941-946e-209238dd3e99@cesnet.cz>

On Thu, Aug 24, 2023 at 8:32 PM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> > I want to know that this trigger has active users that
> > cannot live without it if we are to continue to support it.
>
> We're using this feature. Our use case is a LED at the front panel which
> shows whether a signal is present at an input LC optical connector (DWDM
> network stuff). Here's how we're setting it up:
>
> https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/package/czechlight-clearfog-leds/init-leds-edfa.sh

Interesting!

> I understand that the GPIO numeric namespace is racy, but it's never been a
> problem for us in the past 5 years since this script runs much later during
> boot than any driver probing.

Hm OK that does make sequential sense in a way.

> > Option if this is really needed: I can develop a new trigger
> > that can associate GPIOs with LEDs as triggers using device
> > tree, which should also remove the use of userspace custom
> > scripts to achieve this and be much more trustworthy, if
> > someone with the Nokia n810 or a device with a similar need
> > is willing to test it.
>
> I'll be happy to test a patch like that.

OK let's just do it. I'll cook something up.

> However, the GPIO in question on our board is connected to a MCP23S18, and
> we have a pair of these. When used in this configuration (two chips at the
> same SPI CS, differing by a chip-specific "HW address" on a HW level),
> there are some impedance mismatches because it's essentially two
> independent pinctrl instances on the same SPI address. This causes
> problems, e.g. the debugfs pinctrl instance won't be created for the second
> chip because of a naming conflict. We also carry this out-of-tree patch to
> make the GPIO labels work when set from DTS:
>
> https://patchwork.ozlabs.org/project/linux-gpio/patch/517dcdda21ea0b0df884bc6adcba1dadb78b66b1.1551966077.git.jan.kundrat@cesnet.cz/
>
> (Feedback on how to solve that problem is welcome, btw.)

Instead of using the of_* prefixed device tree traversal
functions, use the fwnode API because we have Intel ACPI
users of this driver, so we need to use the right abstraction.
Just use the similar functions from include/linux/property.h
and grep around for some example that does this already.

Use the bits in spi-present-mask = <0x05>; to iterate perhaps?
Like use for_each_set_bit() from <linux/bitmap.h> and
then we should in theory make the YAML description warn
if that bitmask and the nodes don't match up (yeah...)
since the kernel is not a device tree (or other HW description)
validator.

Then I think you should just split up that patch in one DT binding patch
and one patch for the driver and send it out in a series
"support line names on MCP23S18" or somthing, the MCP
expander is really popular so this is highly desired.

I hope the DT people don't randomly ask you to rewrite the
bindings in YAML. It could happen that they do.

> Since I am not that much familiar with pinctrl/gpio stuff in kernel, I
> wanted to bring this up to make sure that your future trigger works even on
> a setup like ours. Here's how it's used via DTS in case it's relevant:
>
> https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/board/czechlight/clearfog/sdn-roadm-clearfog.dtsi#151

Looks reasonable to me.

Yours,
Linus Walleij