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
