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

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

[Darren Hart]

On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> Hello,

Hi Pali,

> 
> I removed other lines so mail is not too long.
> 
> On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
...
> > > +}
> > > +
> > > +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).

Fair enough. Clear, legible, consistent coding is preferable to a few micro
optimizations that the compiler is likely to catch anyway. I withdraw the
comment :-)

> 
> > > +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?

For -1, I should think  -EIO (I/O Error)
For -2, I'd expect -ENXIO (No such device or address)

Cc Rafael, Mika, linux-acpi in case they have a better idea.

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

OK, that deserves a comment then as the rationale isn't obvious.

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

The goal here is to avoid more than 3 levels of indentations for improved
legibility. Often there are logical inversions and such we can make to
accomplish this. When that fails, we break things up into functions, static
inlines, etc.

For reference:
https://lkml.org/lkml/2007/6/15/449

Which elaborates on CodingStyle Chapter 1: Indentation a bit.

...
> > > +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?

This is a nit, but one other maintainers have imposed on me, as a means to
improve legibility. The preference is to declare variables in decreasing line
length, longest to shortest:

struct kbd_state new_state;
struct kbd_state state;
bool convert;
int value;
u8 unit;
char ch;
int ret;
int i;

This is a generalization and sometimes there are good reasons for doing
something else, such as logical groupings for commenting groups, etc. But since
this list appeared mostly random, defaulting to decreasing line length is
preferred.

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

Right, so the language of the comment just wasn't clear, try:

/* Convert value from seconds to minutes */

> > > +		switch (unit) {
> > > +		case KBD_TIMEOUT_DAYS:
> > > +			value *= 24;
> > > +		case KBD_TIMEOUT_HOURS:
> > > +			value *= 60;
> > > +		case KBD_TIMEOUT_MINUTES:
> > > +			value *= 60;
> > > +			unit = KBD_TIMEOUT_SECONDS;
> > > +	 	}
> > > +

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pali Rohár]

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

On Friday 21 November 2014 21:39:30 Darren Hart wrote:
> On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> > Hello,
> 
> Hi Pali,
> 
> > I removed other lines so mail is not too long.
> 
> > On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> ...
> 
> > > > +}
> > > > +
> > > > +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).
> 
> Fair enough. Clear, legible, consistent coding is preferable
> to a few micro optimizations that the compiler is likely to
> catch anyway. I withdraw the comment :-)
> 

Ok.

> > > > +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?
> 
> For -1, I should think  -EIO (I/O Error)
> For -2, I'd expect -ENXIO (No such device or address)
> 

What about -ENOSYS for -2?

> Cc Rafael, Mika, linux-acpi in case they have a better idea.
> 
> > > > +
> > > > +	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.
> 
> OK, that deserves a comment then as the rationale isn't
> obvious.
> 

Ok, I will add comment.

> > > > +
> > > > +	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.
> 
> The goal here is to avoid more than 3 levels of indentations
> for improved legibility. Often there are logical inversions
> and such we can make to accomplish this. When that fails, we
> break things up into functions, static inlines, etc.
> 
> For reference:
> https://lkml.org/lkml/2007/6/15/449
> 
> Which elaborates on CodingStyle Chapter 1: Indentation a bit.
> 
> ...
> 

Ok, I will move it into two static inline functions (one for 
kbd_get_info and second for kbd_tokens).

> > > > +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?
> 
> This is a nit, but one other maintainers have imposed on me,
> as a means to improve legibility. The preference is to
> declare variables in decreasing line length, longest to
> shortest:
> 
> struct kbd_state new_state;
> struct kbd_state state;
> bool convert;
> int value;
> u8 unit;
> char ch;
> int ret;
> int i;
> 
> This is a generalization and sometimes there are good reasons
> for doing something else, such as logical groupings for
> commenting groups, etc. But since this list appeared mostly
> random, defaulting to decreasing line length is preferred.
> 

Ok. I'm not native English speaker and I did not understand what 
"Decreasing line lenth" means...

> > > > +	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.
> 
> Right, so the language of the comment just wasn't clear, try:
> 
> /* Convert value from seconds to minutes */
> 

Err... to seconds :-) But OK, I will change comment.

> > > > +		switch (unit) {
> > > > +		case KBD_TIMEOUT_DAYS:
> > > > +			value *= 24;
> > > > +		case KBD_TIMEOUT_HOURS:
> > > > +			value *= 60;
> > > > +		case KBD_TIMEOUT_MINUTES:
> > > > +			value *= 60;
> > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > +	 	}
> > > > +

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

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

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

[Darren Hart]

On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:

> > > 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?
> > 
> > For -1, I should think  -EIO (I/O Error)
> > For -2, I'd expect -ENXIO (No such device or address)
> > 
> 
> What about -ENOSYS for -2?

No. This specific topic came up at kernel summit this year. ENOSYS is
specifically for not implemented system calls.

> 
> > > > > +	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.
> > 
> > Right, so the language of the comment just wasn't clear, try:
> > 
> > /* Convert value from seconds to minutes */
> > 
> 
> Err... to seconds :-) But OK, I will change comment.

Oops, duh.

/* Convert value from current units to seconds. */

> 
> > > > > +		switch (unit) {
> > > > > +		case KBD_TIMEOUT_DAYS:
> > > > > +			value *= 24;
> > > > > +		case KBD_TIMEOUT_HOURS:
> > > > > +			value *= 60;
> > > > > +		case KBD_TIMEOUT_MINUTES:
> > > > > +			value *= 60;
> > > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > > +	 	}

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pali Rohár]

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

On Friday 21 November 2014 23:09:40 Darren Hart wrote:
> On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:
> > > > 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?
> > > 
> > > For -1, I should think  -EIO (I/O Error)
> > > For -2, I'd expect -ENXIO (No such device or address)
> > 
> > What about -ENOSYS for -2?
> 
> No. This specific topic came up at kernel summit this year.
> ENOSYS is specifically for not implemented system calls.
> 

Ok, I will use -ENXIO.

> > > > > > +	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.
> > > 
> > > Right, so the language of the comment just wasn't clear,
> > > try:
> > > 
> > > /* Convert value from seconds to minutes */
> > 
> > Err... to seconds :-) But OK, I will change comment.
> 
> Oops, duh.
> 
> /* Convert value from current units to seconds. */
> 
> > > > > > +		switch (unit) {
> > > > > > +		case KBD_TIMEOUT_DAYS:
> > > > > > +			value *= 24;
> > > > > > +		case KBD_TIMEOUT_HOURS:
> > > > > > +			value *= 60;
> > > > > > +		case KBD_TIMEOUT_MINUTES:
> > > > > > +			value *= 60;
> > > > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > > > +	 	}

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

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