Re: [RESEND PATCH v6 2/3] leds: Add driver for the TLC5925 LED controller

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Jean-Jacques Hiblot <jjhiblot@traphandler.com>,
	Linus Walleij <linusw@kernel.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Linux LED Subsystem <linux-leds@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH v6 2/3] leds: Add driver for the TLC5925 LED controller
Date: Wed, 24 Aug 2022 13:18:32 +0300	[thread overview]
Message-ID: <CAHp75Vc5g0OL6YUY2WsUZA6bovB+sdJE3Bv3SWp-1pRh3kyiow@mail.gmail.com> (raw)
In-Reply-To: <5bb9955e-4c2f-ca55-0e77-c082a868371a@traphandler.com>

+Cc: GPIO maintainers

On Wed, Aug 24, 2022 at 12:58 PM Jean-Jacques Hiblot
<jjhiblot@traphandler.com> wrote:
> On 24/08/2022 10:55, Andy Shevchenko wrote:
> > On Wed, Aug 24, 2022 at 11:39 AM Jean-Jacques Hiblot
> > <jjhiblot@traphandler.com> wrote:

...

> >> - with this approach, every time a LED status is changed the whole
> >> register has to be sent on the SPI bus. In other words, changes cannot
> >> be coalesced.
> > But isn't it the same as what you do in your driver? To me it looks
> > like you send the entire range of the values each time you change one
> > LED's brightness. I don't see any differences with the GPIO driver.
> No. The TLC5925 driver updates the register asynchronously: the cached
> value of the register is updated synchronously and then it is
> transferred over SPI using a workqueue. This way if multiple LED are set
> in a short time, the changes are coalesced into a single SPI transfer.
> This is however probably not a must-have feature.

Ah, thanks for elaboration. But GPIO supports this type of ops.

And the more I think about this feature I find it more harmful than
useful. The problem is that delayed operation may take an
unpredictable amount of time and on the heavily loaded machine the
event might be lost (imagine the blinking LED informing about some
critical issue and it blinks only once and then, for example, machine
reboots or so). I believe we both understand that for the GPIO is a
no-go feature for sure, because sequence of the GPIO signals is highly
important (imagine bit-banging of any of the protocols).

> >> I don't know if this is enough to make a dedicated TLC5925 driver
> >> desirable in the kernel.
> > I don't think you have enough justification for a new driver.

After this message I first must withdraw my Rb tag, and turn my voice
against this driver because of the above. On the contrary we might ask
the GPIO library for a specific API to have what you do with the
user's consent of side effects. Linus, Bart, I'm talking of the
delayed (async) version of gpio_set_multiple(). But personally I think
it's not so easy to implement in a bugless manner (because we need to
synchronize it forcibly at any time we call another GPIO API against
the same chip).

Summarize this:
 - you may add a compatible string to the GPIO driver and DT schema,
and we are done.

-- 
With Best Regards,
Andy Shevchenko