Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

From: Pali RohÃr
Date: Thu Dec 04 2014 - 21:03:58 EST

On Wednesday 03 December 2014 09:43:21 Darren Hart wrote:
> On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali RohÃr wrote:
> > This patch adds support for configuring keyboard backlight
> > settings on supported Dell laptops. It exports kernel leds
> > interface and uses Dell SMBIOS tokens or keyboard class
> > interface.
> >
> > With this patch it is possible to set:
> > * keyboard backlight level
> > * timeout after which will be backlight automatically turned
> > off * input activity triggers (keyboard, touchpad, mouse)
> > which enable backlight * ambient light settings
> >
> > Settings are exported via sysfs:
> > /sys/class/leds/dell::kbd_backlight/
> >
> > Code is based on newly released documentation by Dell in
> > libsmbios project.
> Hi Pali,
> I would really like to see this broken up. Possibly adding
> levels and timeouts as separate patches for example. It is
> quite difficult to review this all at once in a reasonable
> amount of time.

It is really hard to split this patch into more parts which every
one will compile and ideally also work. In my opinion every
single git commit should be possible to compile and should also
work (it helps also other developers to use git bisect).

And because we need to pass all (previous unchanged) values in
smbios call we need to parse all of them.

I understand that it could be hard to review long patch, but
there are more parts which interacts and do not work without
other. Also some of settings (e.g keyboard backlight level) could
be changed via different ways (and on some machines only one is
working) and also that smbios keyboard interface has complicated

> During this review I caught a few more CodingStyle violations,
> and raised some questions about the timeout and levels
> mechanisms.

I will fix style problems in next v3 patch.

What to do with Dan Carpenter patch which fixing kbd_timeout
handling? Should I integrate it into v3? It does not apply on top
of next v3 patch, because there will be new comment about quirk
plus style fixes...

Pali RohÃr

Attachment: signature.asc
Description: This is a digitally signed message part.