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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2914963.e9J7NaK4W3@radijator>
Date:   Mon, 09 Oct 2023 17:08:59 +0200
From:   Duje Mihanović <duje.mihanovic@...le.hr>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Lee Jones <lee@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
        Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Helge Deller <deller@....de>, Karel Balej <balejk@...fyz.cz>,
        dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fbdev@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 2/2] backlight: Add Kinetic KTD2801 driver

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