Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <82528887-a6e9-f7cc-26f3-6a0932d967c9@linux.ibm.com>
Date:   Fri, 1 Apr 2022 09:17:24 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] leds: pca955x: Add HW blink support
Content-Language: en-US
To:     Patrick Williams <patrick@stwcx.xyz>
Cc:     pavel@ucw.cz, openbmc@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
        joel@jms.id.au
References: <20220330203318.19225-1-eajames@linux.ibm.com>
 <YkXLG++LWdQWCxQF@heinlein>
From:   Eddie James <eajames@linux.ibm.com>
In-Reply-To: <YkXLG++LWdQWCxQF@heinlein>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/31/22 10:39, Patrick Williams wrote:
> On Wed, Mar 30, 2022 at 03:33:18PM -0500, Eddie James wrote:
>> Support blinking using the PCA955x chip. Use PWM0 for blinking
>> instead of LED_HALF brightness. Since there is only one frequency
>> and brightness register for any blinking LED, all blinked LEDs on
>> the chip will have the same frequency and brightness.
> The current implementation uses the PWM to control the "brightness"
> with PWM0 being assigned 50% and PWM1 being configured as a single value
> that isn't ON, OFF, or 50%.  I suspect that most users of these 955x
> chips care either about brightness or blinking but not both, but it is
> possible I am wrong.  It would be nice if we could use PWM1 as another
> hardware blink control when it hasn't been used for brightness, but that
> would require some additional state tracking I think.
>
> I like that we can now use the hardware to control blink rate, rather
> than doing it in software, and, I really like that in theory if N LEDs on
> the device are all blinking at the same rate they will actually turn on and
> off at the same exact moment because it is done in hardware.  I am really
> concerned about this proposed change and the way it will change current
> behavior though.
>
> It is not uncommon in a BMC design to use one of these 955x chips to control
> 8 or 16 different LEDs reflecting the state of the system and at
> different blink rates.  An example LED policy might be that you have 1 LED
> for "power status" and another LED for "system identify + health status".
> When the system is powered off the "power status" LED flashes at a slow rate
> and when the system is powered on it goes on solid.  When the system is healthy
> the "health status" is on, when it is unhealthy it blinks slowly, and when the
> system is "identified" it blinks fast.
>
> My point of the above is that there are certainly system policies where
> you'd want to flash two different LEDs at two different rates.  In
> today's implementation of this driver those both turn into
> software-emulated blinking by the kernel.  With your proposal we lose
> this ability and instead whichever LED is configured second will affect
> all other blinking LEDs.


Yep. I see your point, it could be problematic.


>
> It looks like in led-core.c led_blink_setup that if the device
> `blink_set` returns an error then software blinking is the fallback.  Is
> it possible for us to have this driver keep track of how many LEDs are
> in blink state (and which speeds are allocated) and get led-core to
> fallback to software blinking if we are unable to satisfy the new blink
> rate without affecting an existing LED blink rate?


OK, I like this idea, I'll go ahead and implement it. Thanks for the 
suggestion!


Eddie


>
> Looking at the tree it seems bcm6328 does what I am suggesting already
> but I don't see any other drivers that obviously do.  The PCA955x is
> pretty widely used in BMC implementations:
>
>      $ git grep -l pca955 arch/arm/boot/dts/aspeed* | wc -l
>      13
>