Date:	Wed, 19 Nov 2014 21:41:20 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Darren Hart <dvhart@...radead.org>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	libsmbios-devel@...ts.us.dell.com, Srinivas_G_Gowda@...l.com,
	Michael_E_Brown@...l.com,
	Gabriele Mazzotta <gabriele.mzt@...il.com>
Subject: Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

Hello,

I removed other lines so mail is not too long.

On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> > +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 == 0) {
> 
> Generally speaking, check for error to keep the main logic
> from getting indented.
> 
> 	if (buffer->output[0]) {
> 		ret = -EINVAL;
> 		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:
> > +	release_buffer();
> > +
> > +	if (ret)
> > +		return -EINVAL;
> 
> Drop this.
> 
> > +	return 0;
> 
> return ret;
> 
> In this particular case, it might be shorter by a line or two
> to drop the ret variable and simply release_buffer() and
> return -EINVAL in the error check above and just return 0
> here.
> 

Ok. But somewhere it is not possible.

> > +}
> > +
> > +static unsigned int kbd_get_max_level(void)
> > +{
> > +	if (kbd_info.levels != 0)
> > +		return kbd_info.levels;
> 
> This test.... does nothing? if it is 0, you still return 0
> below :-)
> 
> > +	if (kbd_mode_levels_count > 0)
> > +		return kbd_mode_levels_count - 1;
> > +	return 0;
> 
> So the function is:
> 
> return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 :
> kbd_info.levels;
> 
> The if blocks are more legible, so that's fine, but the first
> one doesn't seem to do anything and you can replace the final
> return with return kbd_info.levels.
> 

There are two main way for setting keyboard backlight level. One 
is setting level register and other one is setting special 
keyboard mode. And some dell laptops support only first and some 
only second. So this code choose first available/valid method and 
then return correct data. I'm not sure if those two methods are 
only one and also do not know if in future there will be 
something other. Because of this I use code pattern:

if (check_method_1) return value_method_1;
if (check_method_2) return value_method_2;
...
return unsupported;

Same pattern logic is used in all functions which doing something 
with keyboard backlight level. (I will not other functions).

> > +static int kbd_set_state(struct kbd_state *state)
> > +{
> > +	int ret;
> > +
> > +	get_buffer();
> > +	buffer->input[0] = 0x2;
> > +	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
> > +	buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > +	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> > +	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> > +	buffer->input[2] = state->als_setting & 0xFF;
> > +	buffer->input[2] |= (state->level & 0xFF) << 16;
> > +	dell_send_request(buffer, 4, 11);
> > +	ret = buffer->output[0];
> > +	release_buffer();
> > +
> > +	if (ret)
> > +		return -EINVAL;
> 
> Also, is EINVAL right here and elsewhere? Or did something
> fail? EXIO?
> 

According to Dell documentation, return value is stored in 
buffer->output[0] and can be:

0   Completed successfully
-1  Completed with error
-2  Function not supported

So we can return something other too (not always -EINVAL). Do you 
have any idea which errno should we return for -1 and -2?

> > +
> > +	return 0;
> > +}
> > +
> > +static int kbd_set_state_safe(struct kbd_state *state,
> > struct kbd_state *old) +{
> > +	int ret;
> > +
> > +	ret = kbd_set_state(state);
> > +	if (ret == 0)
> > +		return 0;
> > +
> > +	if (kbd_set_state(old))
> > +		pr_err("Setting old previous keyboard state 
failed\n");
> 
> And if we got an error and kbd_set_state(old) doesn't return
> !0, what's the state of things? Still a failure a presume?
> 

In some cases some laptops do not have to support everything. And 
when I (and Gabriele too) tried to set something unsupported Dell 
BIOS just resetted all settings to some default values. So this 
function try to set new state and if it fails then it try to 
restore previous settings.

> > +
> > +	return ret;
> > +}

> > +static void kbd_init(void)
> > +{
> > +	struct kbd_state state;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = kbd_get_info(&kbd_info);
> > +
> > +	if (ret == 0) {
> > +
> 
> Checking for success, let's invert and avoid the indentation
> here too.
> 

Again this is hard and not possible. This function first process 
data from kbd_get_info (if does not fail), then process data for 
kbd_tokens (via function find_token_id) and then set 
kbd_led_present to true if at least kbd_get_info or kbd_tokens 
succed.

> > +		....
> > +
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
> > +		if (find_token_id(kbd_tokens[i]) != -1)
> > +			kbd_token_bits |= BIT(i);
> > +
> > +	if (kbd_token_bits != 0 || ret == 0)
> > +		kbd_led_present = true;
> > +}
> > +
> > +static ssize_t kbd_led_timeout_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct kbd_state state;
> > +	struct kbd_state new_state;
> > +	int ret;
> > +	bool convert;
> > +	char ch;
> > +	u8 unit;
> > +	int value;
> > +	int i;
> 
> Decreasing line lenth please.
> 

What do you mean?

> > +	if (convert) {
> > +		/* NOTE: this switch fall down */
> 
> "fall down" ? As in, it intentionally doesn't have breaks?
> 

This code convert "value" in "units" to new value in minutes 
units. So for unit == days it is: 24*60*60... So no breaks.

> > +		switch (unit) {
> > +		case KBD_TIMEOUT_DAYS:
> > +			value *= 24;
> > +		case KBD_TIMEOUT_HOURS:
> > +			value *= 60;
> > +		case KBD_TIMEOUT_MINUTES:
> > +			value *= 60;
> > +			unit = KBD_TIMEOUT_SECONDS;
> > +	 	}
> > +
> > +		if (value <= kbd_info.seconds && kbd_info.seconds) {
> > +			unit = KBD_TIMEOUT_SECONDS;
> > +		} else if (value/60 <= kbd_info.minutes &&
> > kbd_info.minutes) {
> 
> One space around operators like / and * please, applies
> throughout.
> 

Ok.

> > +	if (kbd_als_supported)
> > +		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
> > +	else
> > +		als_enabled = false;
> > +
> > +	if (kbd_triggers_supported)
> > +		triggers_enabled =
> > kbd_is_trigger_mode_bit(state.mode_bit); +	else
> > +		triggers_enabled = false;
> 
> Could skip the else blocks with initial assignments.
> 

Ok.

> > +
> > +	enable_als = false;
> > +	disable_als = false;
> 
> Can skip these too.
> 

Ok.

> > +			if (triggers_enabled) {
> > +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> > +				kbd_set_level(&new_state, kbd_previous_level);
> > +			} else
> > +				new_state.mode_bit = KBD_MODE_BIT_ON;
> 
> if one block needs braces, use them throughout.
> Apply throughout.
> 

Ok.

> > +static enum led_brightness kbd_led_level_get(struct
> > led_classdev *led_cdev) +{
> > +	int ret;
> > +	u16 num;
> > +	struct kbd_state state;
> > +
> > +	if (kbd_get_max_level()) {
> > +		ret = kbd_get_state(&state);
> > +		if (ret)
> > +			return 0;
> > +		ret = kbd_get_level(&state);
> > +		if (ret < 0)
> > +			return 0;
> > +		return ret;
> > +	} else if (kbd_get_valid_token_counts()) {
> > +		ret = kbd_get_first_active_token_bit();
> > +		if (ret < 0)
> > +			return 0;
> > +		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
> > +			num &= num - 1; /* clear the first bit set */
> > +		if (num == 0)
> > +			return 0;
> > +		return ffs(num) - 1;
> > +	} else {
> > +		pr_warn("Keyboard brightness level control not
> > supported\n"); +		return 0;
> > +	}
> 
> You can drop the else blocks from the above as every case
> returns explicitly.
> 
> if (condA)
> 	return retA;
> if (condB)
> 	return retB
> return ret
> 
> (checkpatch.pl catches this)
> 

Ok, this is possible. I will change it.

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists