On 6/22/20 3:12 PM, Vishwanatha Subbanna wrote:
Hi Jacek,
On 22-Jun-2020, at 5:06 PM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
On 6/22/20 1:07 PM, Vishwanatha Subbanna wrote:
Hi Jacek,
On 22-Jun-2020, at 4:24 PM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
Hi Vishwanatha,
On 6/22/20 8:58 AM, Vishwanatha Subbanna wrote:
Thank you very much Jacek.
On 22-Jun-2020, at 3:12 AM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
Hi Vishwanatha,
On 6/20/20 7:25 PM, Vishwanatha Subbanna wrote:
Hi Jacek,
Thank you very much for the quick response. Greatly appreciate that.
You're welcome.
On 20-Jun-2020, at 3:27 AM, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
Hi Vishwanatha,
Please refer to
Documentation/devicetree/bindings/leds/leds-pca955x.txt.
At first glance I don't get why you have gpio-leds node, which
is for
leds-gpio driver.
Not sure I understood it right.. But if you are asking me why I
have "leds {" and “gpio-leds” in there, then it is to get the
entries in /sys/class/leds.
The GPIOs from PCA9552 are connected to LED. Also, that is how we
have had in the past, and that worked.
Example:
https://github.com/openbmc/linux/blob/dev-5.4/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts#L115
Thanks. Yeah, that looks OK, I had to take closer look at the driver.
The problem I am running into is for :
https://github.com/openbmc/linux/blob/dev-5.4/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
On 6/19/20 3:34 PM, Vishwanatha Subbanna wrote:
Hello,
I am Vishwanath, working with IBM and looking for your help on
one of the issues that I am running into. Would really
appreciate help on this. I run Linux 5.4.38
I have 2 number of PCA9552 chips, one on the Planar and other
on the card that is optionally pluggable. The optional card
must be plugged prior to booting and is not hot pluggable. In
my experiment, I am running *without* the optional card plugged in.
In the device tree, I have a "leds {" section that looks like
below for the PCA9552 that is on the planar and everything
works fine and I can see /sys/class/leds/fan0
leds {
compatible = "gpio-leds”;
fan0 {
retain-state-shutdown;
default-state = "keep";
gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
};
};
&i2c7 {
status = "okay”;
pca0: pca9552@61 {
compatible = "nxp,pca9552";
reg = <0x61>;
#address-cells = <1>;
#size-cells = <0>;
gpio-controller;
#gpio-cells = <2>;
gpio@0 {
reg = <0>;
type = <PCA955X_TYPE_GPIO>;
};
};
};
Similarly, if I update the device tree entry for PCA9552 for
the card that is optionally pluggable, then I don’t see any
leds entries in /sys/class/leds.
Please share your DT node after the update.
Pasting the GPIO_0 entry only here for brevity.
leds {
compatible = "gpio-leds”;
fan0 {
retain-state-shutdown;
default-state = "keep";
gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
};
nvmeslot0 {
retain-state-shutdown;
default-state = "keep";
gpios = <&pca1 0 GPIO_ACTIVE_LOW>;
};
};
&i2c7 {
status = "okay”;
pca0: pca9552@61 {
compatible = "nxp,pca9552";
reg = <0x61>;
#address-cells = <1>;
#size-cells = <0>;
gpio-controller;
#gpio-cells = <2>;
gpio@0 {
reg = <0>;
type = <PCA955X_TYPE_GPIO>;
};
};
};
&i2c13
{
pca1: pca9552@60 {
compatible = "nxp,pca9552";
reg = <0x60>;
#address-cells = <1>;
#size-cells = <0>;
gpio-controller;
#gpio-cells = <2>;
gpio@0 {
reg = <0>;
type = <PCA955X_TYPE_GPIO>;
};
};
};
Thanks
!! Vishwa !!
I don’t even see “fan0” that is on the PCA9552 on planar also.
I was expecting that I should see “/sys/class/leds/fan0”.
However, I could see all the entries in “/proc/device-tree/leds”.
Data from the failure.
[ 7.895757] leds-pca955x 7-0061: leds-pca955x: Using pca9552
16-bit LED driver at slave address 0x61
[ 7.907659] leds-pca955x 7-0061: gpios 168...183
It is weird that you don't see "fan0" LED since this gpio seems to
have
been properly registered according to this log.
This is exactly what I don’t understand. I would expect “fan0” to
appear in /sys/class/leds. Is there any reason why this might not
be appearing ?..
OK, now the reason is clear to me. If leds-gpio driver fails to register
any of the LEDs found in DT node it returns with an error from the
probe(), which results in unregistering any of the LEDs registered in
the previous iteration steps.
Look at the function gpio_leds_create() in
drivers/leds/leds-gpio.c.
Probably it is devm_fwnode_get_gpiod_from_child() that fails
while parsing nvmeslot0 node.
Is this how it is designed or a bug ?.. From a system standpoint, not
having an optional card results in not seeing the ones that are
present on the system.
Would you think it is worthwhile to modify to not chuck off what is
existing because something optional is not plugged in ?.. I believe
the I2C driver handles this scenario by putting an error message but
still consumes what is present.
Well, this code is in mainline for some time and we cannot guarantee
the someone does not rely on this behavior.
Should someone assume those behaviours ?.. Would it be okay to put an
email to the gpio-leds community ?. Just in my opinion, I see a lot of
value in modifying it. Also, is there an IRC where we can discuss this
than the email ?
Changing this behavior could break someone's userspace, which is one of
the most vital no-nos in kernel development. Also, there is no such
entity like gpio-leds community - these are two separate kernel
subsystems, but leds-gpio driver is under LED maintainer's jurisdiction.
I don't think we need IRC discussion yet. Let's wait for the other
LED maintainer's opinion. Pavel?
You mentioned, that your card is not hot-pluggable so it is even more
justified to treat the two hardware setups as demanding a separate DT.
I mean, it is the same system that can either have a card on the slot or
don’t have it. So, it’s not really a different system needing different DT.
Also, it has 3 slots and we can have multiple combinations :)
Otherwise you could probably employ DT overlays mechanism.
Hmm.. this looks interesting in a quick glance. However, I feel the
current leds-gpio driver needs to be updated to not discard the valid ones.
Besides someone may be counting on the existing behaviour, is there any
reason why we want to maintain leds-gpio the way it is and not do what
I2C driver does for example.
This is perfectly sufficient reason to not accept such a modification.
--
Best regards,
Jacek Anaszewski
