All of lore.kernel.org
 help / color / mirror / Atom feed
* Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
@ 2020-06-19 13:34 Vishwanatha Subbanna
  2020-06-19 21:57 ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-19 13:34 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, dmurphy, linux-leds

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. 
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
[    7.913012] leds-pca955x 13-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
[    7.923486] leds-pca955x 13-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
[    7.931695] leds-pca955x 14-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
[    7.942138] leds-pca955x 14-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
[    7.950320] leds-pca955x 15-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60

root@bmc:/sys/class/gpio/# ls -l
lrwxrwxrwx    1 root     root             0 Jan  1  1970 gpiochip168 -> ../../devices/platform/ahb/ahb:apb/ahb:apb:bus@1e78a000/1e78a400.i2c-bus/i2c-7/7-0061/gpio/gpiochip168

root@bmc:/sys/class/gpio/gpiochip168/device# ls -l
lrwxrwxrwx    1 root     root             0 Jun  3 22:03 driver -> ../../../../../../../../bus/i2c/drivers/leds-pca955x
drwxr-xr-x    3 root     root             0 Jan  1  1970 gpio
drwxr-xr-x    3 root     root             0 Jan  1  1970 gpiochip3
-r--r--r--    1 root     root          4096 Jun  3 22:03 modalias
-r--r--r--    1 root     root          4096 Jan  1  1970 name
lrwxrwxrwx    1 root     root             0 Jun  3 22:03 of_node -> ../../../../../../../../firmware/devicetree/base/ahb/apb/bus@1e78a000/i2c-bus@400/pca9552@61
drwxr-xr-x    2 root     root             0 Jun  3 22:03 power
lrwxrwxrwx    1 root     root             0 Jan  1  1970 subsystem -> ../../../../../../../../bus/i2c
-rw-r--r--    1 root     root          4096 Jan  1  1970 uevent
root@bmc:/sys/class/gpio/gpiochip168/device# ls -l gpio

Thank you,
!! Vishwa !!


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-19 13:34 Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38 Vishwanatha Subbanna
@ 2020-06-19 21:57 ` Jacek Anaszewski
  2020-06-20 17:25   ` Vishwanatha Subbanna
  0 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-19 21:57 UTC (permalink / raw)
  To: Vishwanatha Subbanna, pavel, dmurphy, linux-leds

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.

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.

> 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
> [    7.913012] leds-pca955x 13-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
> [    7.923486] leds-pca955x 13-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
> [    7.931695] leds-pca955x 14-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
> [    7.942138] leds-pca955x 14-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
> [    7.950320] leds-pca955x 15-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
> 
> root@bmc:/sys/class/gpio/# ls -l
> lrwxrwxrwx    1 root     root             0 Jan  1  1970 gpiochip168 -> ../../devices/platform/ahb/ahb:apb/ahb:apb:bus@1e78a000/1e78a400.i2c-bus/i2c-7/7-0061/gpio/gpiochip168
> 
> root@bmc:/sys/class/gpio/gpiochip168/device# ls -l
> lrwxrwxrwx    1 root     root             0 Jun  3 22:03 driver -> ../../../../../../../../bus/i2c/drivers/leds-pca955x
> drwxr-xr-x    3 root     root             0 Jan  1  1970 gpio
> drwxr-xr-x    3 root     root             0 Jan  1  1970 gpiochip3
> -r--r--r--    1 root     root          4096 Jun  3 22:03 modalias
> -r--r--r--    1 root     root          4096 Jan  1  1970 name
> lrwxrwxrwx    1 root     root             0 Jun  3 22:03 of_node -> ../../../../../../../../firmware/devicetree/base/ahb/apb/bus@1e78a000/i2c-bus@400/pca9552@61
> drwxr-xr-x    2 root     root             0 Jun  3 22:03 power
> lrwxrwxrwx    1 root     root             0 Jan  1  1970 subsystem -> ../../../../../../../../bus/i2c
> -rw-r--r--    1 root     root          4096 Jan  1  1970 uevent
> root@bmc:/sys/class/gpio/gpiochip168/device# ls -l gpio
> 
> Thank you,
> !! Vishwa !!
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-19 21:57 ` Jacek Anaszewski
@ 2020-06-20 17:25   ` Vishwanatha Subbanna
  2020-06-21 21:42     ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-20 17:25 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: pavel, dmurphy, linux-leds, Vishwanatha Subbanna

Hi Jacek,

Thank you very much for the quick response. Greatly appreciate that.

> 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

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
>> [    7.913012] leds-pca955x 13-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
>> [    7.923486] leds-pca955x 13-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
>> [    7.931695] leds-pca955x 14-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
>> [    7.942138] leds-pca955x 14-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
>> [    7.950320] leds-pca955x 15-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
>> root@bmc:/sys/class/gpio/# ls -l
>> lrwxrwxrwx    1 root     root             0 Jan  1  1970 gpiochip168 -> ../../devices/platform/ahb/ahb:apb/ahb:apb:bus@1e78a000/1e78a400.i2c-bus/i2c-7/7-0061/gpio/gpiochip168
>> root@bmc:/sys/class/gpio/gpiochip168/device# ls -l
>> lrwxrwxrwx    1 root     root             0 Jun  3 22:03 driver -> ../../../../../../../../bus/i2c/drivers/leds-pca955x
>> drwxr-xr-x    3 root     root             0 Jan  1  1970 gpio
>> drwxr-xr-x    3 root     root             0 Jan  1  1970 gpiochip3
>> -r--r--r--    1 root     root          4096 Jun  3 22:03 modalias
>> -r--r--r--    1 root     root          4096 Jan  1  1970 name
>> lrwxrwxrwx    1 root     root             0 Jun  3 22:03 of_node -> ../../../../../../../../firmware/devicetree/base/ahb/apb/bus@1e78a000/i2c-bus@400/pca9552@61
>> drwxr-xr-x    2 root     root             0 Jun  3 22:03 power
>> lrwxrwxrwx    1 root     root             0 Jan  1  1970 subsystem -> ../../../../../../../../bus/i2c
>> -rw-r--r--    1 root     root          4096 Jan  1  1970 uevent
>> root@bmc:/sys/class/gpio/gpiochip168/device# ls -l gpio
>> Thank you,
>> !! Vishwa !!
> 
> -- 
> Best regards,
> Jacek Anaszewski


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-20 17:25   ` Vishwanatha Subbanna
@ 2020-06-21 21:42     ` Jacek Anaszewski
  2020-06-22  6:58       ` Vishwanatha Subbanna
  0 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-21 21:42 UTC (permalink / raw)
  To: Vishwanatha Subbanna; +Cc: pavel, dmurphy, linux-leds

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.

>>> [    7.913012] leds-pca955x 13-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
>>> [    7.923486] leds-pca955x 13-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6

This indicates the problem with finding pca955x device at address 0x60
on I2C bus connected to the I2C controller labelled as i2c13.

There are two options:
1) There is a problem with a physical connection with the chip on the
    card
2) You mismatched the I2C controller with the bus that controls the card

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-21 21:42     ` Jacek Anaszewski
@ 2020-06-22  6:58       ` Vishwanatha Subbanna
  2020-06-22 10:54         ` Jacek Anaszewski
  0 siblings, 1 reply; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-22  6:58 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: pavel, dmurphy, linux-leds, Vishwanatha Subbanna

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 ?.. 


>>>> [    7.913012] leds-pca955x 13-0060: leds-pca955x: Using pca9552 16-bit LED driver at slave address 0x60
>>>> [    7.923486] leds-pca955x 13-0060: pca955x_write_pwm: reg 0x0, val 0x80, err -6
> 
> This indicates the problem with finding pca955x device at address 0x60
> on I2C bus connected to the I2C controller labelled as i2c13.
> 
> There are two options:
> 1) There is a problem with a physical connection with the chip on the
>   card
> 2) You mismatched the I2C controller with the bus that controls the card
> 
> -- 

The PCA at address 0x60 is on the optional card that is not connected in my experiment. This is expected. However, what is not expected is the “fan0” missing even through it is on the planar. Looks like the leds driver is chucking off even the good ones even if there is at least one GPIO that can not be acquired. 

Thank you,
!! Vishwa !!

> Best regards,
> Jacek Anaszewski


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-22  6:58       ` Vishwanatha Subbanna
@ 2020-06-22 10:54         ` Jacek Anaszewski
  2020-06-22 11:01           ` Jacek Anaszewski
  2020-06-22 11:07           ` Vishwanatha Subbanna
  0 siblings, 2 replies; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-22 10:54 UTC (permalink / raw)
  To: Vishwanatha Subbanna; +Cc: pavel, dmurphy, linux-leds

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.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  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
  1 sibling, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-22 11:01 UTC (permalink / raw)
  To: Vishwanatha Subbanna; +Cc: pavel, dmurphy, linux-leds

On 6/22/20 12:54 PM, Jacek Anaszewski 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.

IOW you have to have two separate dts files for the arrangement
with the card and without it.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-22 10:54         ` Jacek Anaszewski
  2020-06-22 11:01           ` Jacek Anaszewski
@ 2020-06-22 11:07           ` Vishwanatha Subbanna
  2020-06-22 11:36             ` Jacek Anaszewski
  1 sibling, 1 reply; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-22 11:07 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: pavel, dmurphy, linux-leds

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.

> 
> -- 
> Best regards,
> Jacek Anaszewski


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  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>
  0 siblings, 1 reply; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-22 11:36 UTC (permalink / raw)
  To: Vishwanatha Subbanna; +Cc: pavel, dmurphy, linux-leds

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.

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.

Otherwise you could probably employ DT overlays mechanism.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
       [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
  1 sibling, 0 replies; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-22 18:13 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: pavel, dmurphy, linux-leds, Vishwanatha Subbanna

Apologies if you already received. I got a mailer Damon error. Resending it

Thanks,
!! Vishwa !!

> On 22-Jun-2020, at 6:42 PM, Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com> 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 ?
> 
>> 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.
> 
> Thank you,
> !! Vishwa !!
> 
>> -- 
>> Best regards,
>> Jacek Anaszewski


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
       [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
  1 sibling, 0 replies; 12+ messages in thread
From: Jacek Anaszewski @ 2020-06-22 20:08 UTC (permalink / raw)
  To: Vishwanatha Subbanna, pavel; +Cc: dmurphy, linux-leds

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38
  2020-06-22 11:01           ` Jacek Anaszewski
@ 2020-06-26 13:04             ` Vishwanatha Subbanna
  0 siblings, 0 replies; 12+ messages in thread
From: Vishwanatha Subbanna @ 2020-06-26 13:04 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: pavel, dmurphy, linux-leds, Vishwanatha Subbanna

I will do some study here. Thank you for the prompt response Jacek.

Thanks,
!! Vishwa !!

> On 22-Jun-2020, at 4:31 PM, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
> On 6/22/20 12:54 PM, Jacek Anaszewski 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.
> 
> IOW you have to have two separate dts files for the arrangement
> with the card and without it.
> 
> -- 
> Best regards,
> Jacek Anaszewski


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-06-26 13:05 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 13:34 Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38 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 is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.