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

[Pali RohÃr]

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.
> 
> During this review I caught a few more CodingStyle violations,
> and raised some questions about the timeout and levels
> mechanisms.
> 
> > @@ -62,6 +71,10 @@ struct calling_interface_structure {
> > 
> >  struct quirk_entry {
> >  
> >  	u8 touchpad_led;
> > 
> > +
> > +	/* Ordered list of timeouts expressed in seconds.
> > +	 * The list must end with -1 */
> 
> Despite other instances in this file, block comments are
> documented in CodingStyle as:
> 
> /*
>  * Comment text.
>  */
> 
> The old ones should be cleaned up eventually, but new ones
> need to be done according to CodingStyle. Please correct
> throughout.
> 
> > +	int kbd_timeouts[];
> > 
> >  };
> >  
> >  static struct quirk_entry *quirks;
> > 
> > @@ -76,6 +89,10 @@ static int __init dmi_matched(const
> > struct dmi_system_id *dmi)
> > 
> >  	return 1;
> >  
> >  }
> > 
> > +static struct quirk_entry quirk_dell_xps13_9333 = {
> > +	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> 
> Where did these values come from? Were they documented in the
> libsmbios project? Can you provide a URL to that? These
> really should be described by the firmware, but if they
> aren't, nothing we can do about it.
> 

They comes from some Windows Dell GUI utility. It is not 
documented in libsmbios project and people behind libsmbios do 
not know why only those values are accepted by BIOS for XPS 
laptop...

> > @@ -790,6 +842,964 @@ static void touchpad_led_exit(void)
> > 
> >  	led_classdev_unregister(&touchpad_led);
> >  
> >  }
> > 
> > +/* Derived from information in smbios-keyboard-ctl:
> See block comment above.
> 
> > +
> > + cbClass 4
> > + cbSelect 11
> > + Keyboar illumination
> 
> Keyboard
> 
> > + cbArg1 determines the function to be performed
> > +
> > + cbArg1 0x0 = Get Feature Information
> > +  cbRES1         Standard return codes (0, -1, -2)
> > +  cbRES2, word0  Bitmap of user-selectable modes
> > +     bit 0     Always off (All systems)
> > +     bit 1     Always on (Travis ATG, Siberia)
> > +     bit 2     Auto: ALS-based On; ALS-based Off (Travis
> > ATG) +     bit 3     Auto: ALS- and input-activity-based
> > On; input-activity based Off +     bit 4     Auto:
> > Input-activity-based On; input-activity based Off +     bit
> > 5     Auto: Input-activity-based On (illumination level
> > 25%); input-activity based Off +     bit 6     Auto:
> > Input-activity-based On (illumination level 50%);
> > input-activity based Off +     bit 7     Auto:
> > Input-activity-based On (illumination level 75%);
> > input-activity based Off +     bit 8     Auto:
> > Input-activity-based On (illumination level 100%);
> > input-activity based Off +     bits 9-15 Reserved for
> > future use
> > +  cbRES2, byte2  Reserved for future use
> > +  cbRES2, byte3  Keyboard illumination type
> > +     0         Reserved
> > +     1         Tasklight
> > +     2         Backlight
> > +     3-255     Reserved for future use
> > +  cbRES3, byte0  Supported auto keyboard illumination
> > trigger bitmap. +     bit 0     Any keystroke
> > +     bit 1     Touchpad activity
> > +     bit 2     Pointing stick
> > +     bit 3     Any mouse
> > +     bits 4-7  Reserved for future use
> > +  cbRES3, byte1  Supported timeout unit bitmap
> > +     bit 0     Seconds
> > +     bit 1     Minutes
> > +     bit 2     Hours
> > +     bit 3     Days
> > +     bits 4-7  Reserved for future use
> > +  cbRES3, byte2  Number of keyboard light brightness levels
> > +  cbRES4, byte0  Maximum acceptable seconds value (0 if
> > seconds not supported). +  cbRES4, byte1  Maximum
> > acceptable minutes value (0 if minutes not supported). + 
> > cbRES4, byte2  Maximum acceptable hours value (0 if hours
> > not supported). +  cbRES4, byte3  Maxiomum acceptable days
> > value (0 if days not supported)
> 
> Maximum             ^
> 
> This interface appears to define all possible values for the
> timeout between cbRES3[1] and cbRES4[*]. Why is the
> kbd_timeouts[] array a quirk with fixed values? It seems it
> could indeed be dynamically determined.
> 

BIOS bug (or BIOS feature?). No idea. For invalid value on XPS 
BIOS just reset keyboard backlight to someting default...

> > +
> > +struct kbd_info {
> > +	u16 modes;
> > +	u8 type;
> > +	u8 triggers;
> > +	u8 levels;
> > +	u8 seconds;
> > +	u8 minutes;
> > +	u8 hours;
> > +	u8 days;
> > +};
> > +
> > +
> > +static u8 kbd_mode_levels[16];
> > +static int kbd_mode_levels_count;
> 
> I'm confused by these. How are they different from
> kbd_info.levels?
> 

There are two interfaces how to set keyboard backlight. Both are 
documented above. One via kbd mode and one via kbd level. Some 
laptops (e.g my E6440) support only kbd mode and some (e.g XPS) 
support only kbd level. So we need to implement both interfaces 
in kernel if we want to support as more machines as possible.

> We seem to check the latter first and fall back to these if
> that is 0.... why?
> 

Because if maximum possible kbd level is 0 then we cannot use it.

> > +static int kbd_get_info(struct kbd_info *info)
> > +{
> > +	u8 units;
> > +	int ret;
> > +
> > +	get_buffer();
> > +
> > +	buffer->input[0] = 0x0;
> > +	dell_send_request(buffer, 4, 11);
> > +	ret = buffer->output[0];
> > +
> > +	if (ret) {
> > +		ret = dell_smi_error(ret);
> > +		goto out;
> > +	}
> > +
> > +	info->modes = buffer->output[1] & 0xFFFF;
> > +	info->type = (buffer->output[1] >> 24) & 0xFF;
> > +	info->triggers = buffer->output[2] & 0xFF;
> > +	units = (buffer->output[2] >> 8) & 0xFF;
> > +	info->levels = (buffer->output[2] >> 16) & 0xFF;
> > +
> > +	if (units & BIT(0))
> > +		info->seconds = (buffer->output[3] >> 0) & 0xFF;
> > +	if (units & BIT(1))
> > +		info->minutes = (buffer->output[3] >> 8) & 0xFF;
> > +	if (units & BIT(2))
> > +		info->hours = (buffer->output[3] >> 16) & 0xFF;
> > +	if (units & BIT(3))
> > +		info->days = (buffer->output[3] >> 24) & 0xFF;
> > +
> 
> > +out:
> Please indent gotos by one space. This improves diffs context
> by not treating the goto label like a function.
> 
> > +	release_buffer();
> > +	return ret;
> > +}
> > +
> > +static unsigned int kbd_get_max_level(void)
> > +{
> > +	if (kbd_info.levels != 0)
> > +		return kbd_info.levels;
> > +	if (kbd_mode_levels_count > 0)
> > +		return kbd_mode_levels_count - 1;
> 
> Here for example. In what scenario does this happen?
> 

When laptop does not support configuring keyboard backlight 
level. You can see that in documentation are more properties 
which can be configured, so if BIOS tell us that this one (level) 
does not support we cannot change it.

> > +	return 0;
> > +}
> > +
> > 
> > 
> > 
> > +static inline int kbd_init_info(void)
> > +{
> > +	struct kbd_state state;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = kbd_get_info(&kbd_info);
> > +	if (ret)
> > +		return ret;
> > +
> > +	kbd_get_state(&state);
> > +
> > +	/* NOTE: timeout value is stored in 6 bits so max value is
> > 63 */ +	if (kbd_info.seconds > 63)
> > +		kbd_info.seconds = 63;
> > +	if (kbd_info.minutes > 63)
> > +		kbd_info.minutes = 63;
> > +	if (kbd_info.hours > 63)
> > +		kbd_info.hours = 63;
> > +	if (kbd_info.days > 63)
> > +		kbd_info.days = 63;
> > +
> > +	/* NOTE: On tested machines ON mode did not work and
> > caused +	 *       problems (turned backlight off) so do not
> > use it +	 */
> > +	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
> > +
> > +	kbd_previous_level = kbd_get_level(&state);
> > +	kbd_previous_mode_bit = state.mode_bit;
> > +
> > +	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
> > +		kbd_previous_level = 1;
> > +
> > +	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
> > +		kbd_previous_mode_bit =
> > +			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
> > +		if (kbd_previous_mode_bit != 0)
> > +			kbd_previous_mode_bit--;
> > +	}
> > +
> > +	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
> > +			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
> > +		kbd_als_supported = true;
> > +
> > +	if (kbd_info.modes & (
> > +	    BIT(KBD_MODE_BIT_TRIGGER_ALS) |
> > BIT(KBD_MODE_BIT_TRIGGER) | +	   
> > BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50)
> > | +	    BIT(KBD_MODE_BIT_TRIGGER_75) |
> > BIT(KBD_MODE_BIT_TRIGGER_100) +	   ))
> > +		kbd_triggers_supported = true;
> > +
> > +	for (i = 0; i < 16; ++i)
> 
> Doesn't this only apply to bits 5-8? Why not just loop through
> those?
> 

Some bits are reserved for future use (now undocumented). So once 
we know what they means we can adjust global enum/macro and 
changing this for loop will not be needed. 

> for (i = KBD_MODE_BIT_TRIGGER_25; i <=
> KBD_MODE_BIT_TRIGGER_100)
> 
> The level_mode_bit check becomes unecessary.
> 

I tried to write general code which check all possible modes if 
they are of type which configure level. And because some of them 
are undocumented I used this approach, so in future global 
enum/macro could be extended.

> > +		if (kbd_is_level_mode_bit(i) && (BIT(i) &
> > kbd_info.modes))
> > +			kbd_mode_levels[1+kbd_mode_levels_count++] = i;
> 
> One space around operators like + please.
> 
> What is kbd_mode_levels[0] reserved for? The current level?
> Isn't that what kbd_state.level is for?
> 

Reserved for off mode -- keyboard backlight turned off.

kbd level is for laptops which support kbd level. kbd mode is for 
laptops which support setting level via kbd mode.

> > +
> > +	if (kbd_mode_levels_count > 0) {
> > +		for (i = 0; i < 16; ++i) {
> 
> If 0-4 don't apply here, why loop through them? Should we just
> set levels[0] to 0 if it isn't one of 5-8?
> 

We know that BIOSes are buggy, so I can imagine that off mode 
(which is enum = 0) does not have to be supported... So for 
kbd_mode_levels[0] we set first supported mode (if off is not 
supported we will choose something other, so kernel code will not 
call unsupported mode).

> If bits 9-16 are reserved, it seems it would be best to avoid
> checking for them as they might return a false positive, and
> we'd be setting kbd_mode_levels to an undefined value.
> 
> > +			if (BIT(i) & kbd_info.modes) {
> > +				kbd_mode_levels[0] = i;
> > +				break;
> > +			}
> > +		}
> > +		kbd_mode_levels_count++;
> > +	}
> > +
> > +	return 0;

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

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