Re: [PATCH 2/2] backlight: Add Kinetic KTD2801 driver

[Duje Mihanović]

On Monday, October 9, 2023 11:21:03 AM CEST Daniel Thompson wrote:
> On Thu, Oct 05, 2023 at 08:49:09PM +0200, Duje Mihanović wrote:
> > +#define EW_DELAY	150
> > +#define EW_DET		270
> > +#define LOW_BIT_HIGH	5
> > +#define LOW_BIT_LOW	(4 * HIGH_BIT_LOW)
> > +#define HIGH_BIT_LOW	5
> > +#define HIGH_BIT_HIGH	(4 * HIGH_BIT_LOW)
> 
> These names are pretty cryptic (they don't even mention that they
> are time values and that the unit is microseconds). They also look
> like they were derived by tuning so comments would be nice explaining
> where they come from (or, failing that, why they are correct).

These values are taken from Samsung's driver, which themselves are datasheet 
values with a couple of us added for I presume safety. The datasheet is 
publicly available [1] and I can add a link to it to the binding patch in v2.

> > +	if (!ktd2801->was_on) {
> > +		gpiod_set_value(ktd2801->desc, 0);
> > +		udelay(EW_DELAY);
> > +		gpiod_set_value(ktd2801->desc, 1);
> > +		udelay(EW_DET);
> > +		gpiod_set_value(ktd2801->desc, 0);
> > +		ktd2801->was_on = true;
> > +	}
> 
> Isn't this implementing the same single GPIO line protocol used by
> drivers/leds/flash/leds-ktd2692.c?
> 
> If so, it would be good to pull the expresswire handling into a library
> so it can be shared between drivers. leds-ktd2692.c does a pretty
> good job of decomposing the expresswire management into functions (e.g.
> separating data framing from setting of control values). Expresswire is
> a data framing protocol rather than a bus so I think just implementing
> it as library code is probably sufficient.

KTD2801 and 2692 indeed seem to share the protocol and I can write a library 
just for this. I believe such a library could go in drivers/leds/leds-
expresswire.c and include/linux/leds-expresswire.h (or perhaps even just in a 
header), do you have any better ideas?

> Also, can the expresswire code have protocol-violation watchdogs that
> trigger a re-transmit of the message if we get pre-empted in the middle
> of sending a message to the backlight (see calls to ktime_get_ns() in
> ktd253-backlight.c ).

The Samsung driver does not have such watchdogs, but if you think they are 
needed I can add them.

[1] https://www.mouser.com/datasheet/2/936/KTD2801-04b-1391831.pdf

Regards,
Duje