Re: [PATCH 6/6] ARM: dts: aspeed: rainier: Add leds that are on optional PCI cable cards

On Mon, 15 Feb 2021, at 17:14, vishwanatha subbanna wrote:

quoted

On 15-Feb-2021, at 4:38 AM, Andrew Jeffery [off-list ref] wrote:



On Wed, 10 Feb 2021, at 21:46, vishwanatha subbanna wrote:

quoted

quoted

On 16-Nov-2020, at 11:43 AM, Joel Stanley [off-list ref] wrote:

On Fri, 13 Nov 2020 at 05:59, Vishwanatha Subbanna
[off-list ref] wrote:

quoted

These are LEDs on the cable cards that plug into PCIE slots.
The LEDs are controlled by PCA9552 I2C expander

Signed-off-by: Vishwanatha Subbanna <redacted>
---
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 288 +++++++++++++++++++++++++++
1 file changed, 288 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 67c8c40..7de5f76 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts

@@ -696,6 +696,70 @@
                     gpios = <&pca4 7 GPIO_ACTIVE_LOW>;
             };
     };
+
+       leds-optional-cablecard0 {

Is it necessary to have separate nodes for each of the different GPIO devices?

Would it make sense to combine them, or is it better to be separate?

Andrew, Eddie, Brad: please review this one before I merge it.

I answered this in previous patch set.  If I express ‘em all in one 
node that is “leds {", then if any of the GPIO is not seen because of 
not having the card, then the current leds-gpio driver knocks off all 
the ones on which it successfully acquired the GPIOs also, leaving 
nothing.

I'm struggling to follow this sentence. Can you please explain what you're 
trying to say in a less colloquial way?

Okay.. So, let me give a bit of background. We have some cards that are 
optional. What that means is, if the cards are to be inserted, then the 
system needs to be brought down to put ‘em. Now, there can be N such 
cards in our system and any of ‘em can be populated or none of ‘em can 
be populated depending on what user wants.

Now, let us assume I put global “leds { , compatible = "gpio-leds"; ” 
section and I describe LEDs of all these N cards along with all the 
other LEDs on the planar, then leds-gpio driver would populate all the 
entries in /sys/class/leds/ __if__ all of the N cards that I mentioned 
are plugged in and their GPIOs can be detected.

However, take for instance where 1 or more of those cards are not 
plugged in, then, because there is a failure in detecting those GPIOs, 
leds-gpio driver will
discard all other LEDs on which it could successfully acquire the 
GPIOs. So, there will not be anything in “/sys/class/leds”. So, the way 
it works is : either -all- or -none-.

Right.

So what's happening is we're using the static description of the hardware in 
the devicetree to describe non-static data. That makes the patch a bit of a 
hack. I'm not terribly enthused about it. Reliance on a specific probe 
behaviour of the leds-gpio driver is not ideal.

The alternative is runtime device creation and driver binding, but then we have 
issues describing the device configuration to the kernel. For LED configuration 
I don't think that's a solved problem, but I'm keen for others to weigh in with 
ideas here.

I think where I land is the devicetree approach is quite distasteful, but 
pragmatic?

I'm torn so I'm going to punt to the maintainer.

Andrew