Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>, pavel@ucw.cz
Cc: dmurphy@ti.com, linux-leds@vger.kernel.org
Subject: Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
Date: Mon, 22 Jun 2020 22:08:45 +0200
Message-ID: <b75787b3-87ae-3a75-14dd-628a10313150@gmail.com> (raw)
In-Reply-To: <F6E7B2CD-3871-4C41-A7F0-8A77E824D155@linux.vnet.ibm.com>

On 6/22/20 3:12 PM, Vishwanatha Subbanna wrote:
> Hi Jacek,
> 
>> On 22-Jun-2020, at 5:06 PM, Jacek Anaszewski 
>> <jacek.anaszewski@gmail.com> 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@gmail.com> 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@gmail.com> 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@gmail.com> 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

      parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-19 13:34 Vishwanatha Subbanna
2020-06-19 21:57 ` Jacek Anaszewski
2020-06-20 17:25   ` Vishwanatha Subbanna
2020-06-21 21:42     ` Jacek Anaszewski
2020-06-22  6:58       ` Vishwanatha Subbanna
2020-06-22 10:54         ` Jacek Anaszewski
2020-06-22 11:01           ` Jacek Anaszewski
2020-06-26 13:04             ` Vishwanatha Subbanna
2020-06-22 11:07           ` Vishwanatha Subbanna
2020-06-22 11:36             ` Jacek Anaszewski
     [not found]               ` <F6E7B2CD-3871-4C41-A7F0-8A77E824D155@linux.vnet.ibm.com>
2020-06-22 18:13                 ` Vishwanatha Subbanna
2020-06-22 20:08                 ` Jacek Anaszewski [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b75787b3-87ae-3a75-14dd-628a10313150@gmail.com \
    --to=jacek.anaszewski@gmail.com \
    --cc=dmurphy@ti.com \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=vishwa@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git