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

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 2159 bytes --]

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 
logic...

> 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
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]