Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 10:39:07 -0500
From:   Patrick Williams <patrick@stwcx.xyz>
To:     Eddie James <eajames@linux.ibm.com>
Cc:     linux-leds@vger.kernel.org, openbmc@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, pavel@ucw.cz, joel@jms.id.au
Subject: Re: [PATCH] leds: pca955x: Add HW blink support
Message-ID: <YkXLG++LWdQWCxQF@heinlein>
References: <20220330203318.19225-1-eajames@linux.ibm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="1Owk7bxpetCwLmTk"
Content-Disposition: inline
In-Reply-To: <20220330203318.19225-1-eajames@linux.ibm.com>
Precedence: bulk


--1Owk7bxpetCwLmTk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 hea=
lthy
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.

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?

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

--=20
Patrick Williams

--1Owk7bxpetCwLmTk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEBGD9ii4LE9cNbqJBqwNHzC0AwRkFAmJFyxkACgkQqwNHzC0A
wRkAaBAAjBfOY0GBJZfL3KuD59+0bFzyNMbkCbK0EmE4VU3YlONWgxXTLwTDir9/
Ctb2a86QdZglRQzoof7/jyEmAJ1/tFFA+Kps5Z1FGPczZbGxJ9Obel7J8SNo3aiy
sP3gBz2pbZtprpOo2w0Yc1ReZFiJe/kH7z3a1H/2Xl/lq9TPEDg22wVQnzkccNJp
4X+1p0rwAqOqxGjBeOjaGAj1y7PvqXUICZVw0rkkXaI80Gjg+IikrUB8/FDSaCDT
WHV3i4QfocIEUD62v5bSnzKijyaixo2e16fOUNqCTv1hWA5U2ZvoH+LhRm+91I0+
otF6Hf9TUhcLFQkAKcoBOTXcmOJ2IZ06bUZ/sklWhdLWHmc0jL0t0LmcJk8s4eJb
PJz+bP81sjTaB18sFkQu1KCXOaRkDf4/32csLZd2Tqq1HEXc24uFzhGdhirNgKpw
55Bxs8H4Bp0OyqvIW+TYWutWDbP96vULv6xkaGNtJM4Nc0gJECh82yp/QQEBd9bS
JjjwLbjl1W58SOfhtrzrwLd52RqKuBYUSzWSBoTP1RFOhHeTcCisE8XgL2hT/JZB
DKl4ER8AQJDuqmjK6PilTPEM0c4gSupmJFqLnMC61StQ75pmpoxDqlE3bvqJ2GSd
p1Xpy3S5LhJqY+TEo65UIst6nWhAF4XRsBjy+hf0uqzib9ykpxw=
=/hef
-----END PGP SIGNATURE-----

--1Owk7bxpetCwLmTk--