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

From: "Andrew Jeffery" <andrew@aj.id.au>
To: vishwa <vishwa@linux.vnet.ibm.com>, "Joel Stanley" <joel@jms.id.au>
Cc: "Eddie James" <eajames@linux.ibm.com>,
	"Brad Bishop" <bradleyb@fuzziesquirrel.com>,
	linux-aspeed <linux-aspeed@lists.ozlabs.org>,
	devicetree <devicetree@vger.kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>
Subject: Re: [PATCH 6/6] ARM: dts: aspeed: rainier: Add leds that are on optional PCI cable cards
Date: Mon, 15 Feb 2021 09:38:13 +1030	[thread overview]
Message-ID: <30e6c20b-1f00-4209-960c-8de1d70c8a4e@www.fastmail.com> (raw)
In-Reply-To: <6CFB3D8D-CF5A-4E33-8D57-6A4034DDC49E@linux.vnet.ibm.com>

On Wed, 10 Feb 2021, at 21:46, vishwanatha subbanna wrote:
> 
> 
> > On 16-Nov-2020, at 11:43 AM, Joel Stanley <joel@jms.id.au> wrote:
> > 
> > On Fri, 13 Nov 2020 at 05:59, Vishwanatha Subbanna
> > <vishwa@linux.vnet.ibm.com> wrote:
> >> 
> >> 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 <vishwa@linux.vnet.ibm.com>
> >> ---
> >> 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?

> I did speak to the maintainer and it looked like the behaviour 
> was existing since long time and changing it would break old code.
> 

Break how?

Andrew