Skip to contentSkip to conversation

Keyboard backlight control for some Vaio Fit models

PATCH In review Mattia Dongili 2 participants 3 messages Dec 15, 2015 – Dec 16, 2015

Subsystems (2)
x86 platform drivers Sony vaio control device driver
lore
Share & tools
lore

Mattia Dongili

author maintainer
#0 lore raw

PATCH 1/1 Keyboard backlight control for some Vaio Fit models

SVF1521P6EW, SVF1521DCXW, SVF13N1L2ES and likely most SVF*. do not expose separate timeout controls in auto mode.

Signed-off-by Dominik Matta <[email protected]>
Signed-off-by Mattia Dongili <[email protected]>
---
 drivers/platform/x86/sony-laptop.c | 65 ++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 20 deletions(-)
drivers/platform/x86/sony-laptop.c +45 -20
index f73c295..e9caa34 100644
@@ -1393,6 +1393,7 @@ static void sony_nc_function_setup(struct acpi_device *device,
 		case 0x0143:
 		case 0x014b:
 		case 0x014c:
+		case 0x0153:
 		case 0x0163:
 			result = sony_nc_kbd_backlight_setup(pf_device, handle);
 			if (result)

@@ -1490,6 +1491,7 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x0143:
 		case 0x014b:
 		case 0x014c:
+		case 0x0153:
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
 			break;

@@ -1773,6 +1775,7 @@ struct kbd_backlight {
 	unsigned int base;
 	unsigned int mode;
 	unsigned int timeout;
+	unsigned int has_timeout;
 	struct device_attribute mode_attr;
 	struct device_attribute timeout_attr;
 };

@@ -1877,6 +1880,8 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 		unsigned int handle)
 {
 	int result;
+	int probe_base = 0;
+	int ctl_base = 0;
 	int ret = 0;
 
 	if (kbdbl_ctl) {

@@ -1885,11 +1890,25 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 		return -EBUSY;
 	}
 
-	/* verify the kbd backlight presence, these handles are not used for
-	 * keyboard backlight only
+	/* verify the kbd backlight presence, some of these handles are not used
+	 * for keyboard backlight only
 	 */
-	ret = sony_call_snc_handle(handle, handle == 0x0137 ? 0x0B00 : 0x0100,
-			&result);
+	switch (handle) {
+	case 0x0153:
+		probe_base = 0x0;
+		ctl_base = 0x0;
+		break;
+	case 0x0137:
+		probe_base = 0x0B00;
+		ctl_base = 0x0C00;
+		break;
+	default:
+		probe_base = 0x0100;
+		ctl_base = 0x4000;
+		break;
+	}
+
+	ret = sony_call_snc_handle(handle, probe_base, &result);
 	if (ret)
 		return ret;
 

@@ -1906,10 +1925,9 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 	kbdbl_ctl->mode = kbd_backlight;
 	kbdbl_ctl->timeout = kbd_backlight_timeout;
 	kbdbl_ctl->handle = handle;
-	if (handle == 0x0137)
-		kbdbl_ctl->base = 0x0C00;
-	else
-		kbdbl_ctl->base = 0x4000;
+	kbdbl_ctl->base = ctl_base;
+	/* Some models do not allow timeout control */
+	kbdbl_ctl->has_timeout = handle != 0x0153;
 
 	sysfs_attr_init(&kbdbl_ctl->mode_attr.attr);
 	kbdbl_ctl->mode_attr.attr.name = "kbd_backlight";

@@ -1917,22 +1935,28 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 	kbdbl_ctl->mode_attr.show = sony_nc_kbd_backlight_mode_show;
 	kbdbl_ctl->mode_attr.store = sony_nc_kbd_backlight_mode_store;
 
-	sysfs_attr_init(&kbdbl_ctl->timeout_attr.attr);
-	kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout";
-	kbdbl_ctl->timeout_attr.attr.mode = S_IRUGO | S_IWUSR;
-	kbdbl_ctl->timeout_attr.show = sony_nc_kbd_backlight_timeout_show;
-	kbdbl_ctl->timeout_attr.store = sony_nc_kbd_backlight_timeout_store;
-
 	ret = device_create_file(&pd->dev, &kbdbl_ctl->mode_attr);
 	if (ret)
 		goto outkzalloc;
 
-	ret = device_create_file(&pd->dev, &kbdbl_ctl->timeout_attr);
-	if (ret)
-		goto outmode;
-
 	__sony_nc_kbd_backlight_mode_set(kbdbl_ctl->mode);
-	__sony_nc_kbd_backlight_timeout_set(kbdbl_ctl->timeout);
+
+	if (kbdbl_ctl->has_timeout) {
+		sysfs_attr_init(&kbdbl_ctl->timeout_attr.attr);
+		kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout";
+		kbdbl_ctl->timeout_attr.attr.mode = S_IRUGO | S_IWUSR;
+		kbdbl_ctl->timeout_attr.show =
+			sony_nc_kbd_backlight_timeout_show;
+		kbdbl_ctl->timeout_attr.store =
+			sony_nc_kbd_backlight_timeout_store;
+
+		ret = device_create_file(&pd->dev, &kbdbl_ctl->timeout_attr);
+		if (ret)
+			goto outmode;
+
+		__sony_nc_kbd_backlight_timeout_set(kbdbl_ctl->timeout);
+	}
+
 
 	return 0;
 

@@ -1949,7 +1973,8 @@ static void sony_nc_kbd_backlight_cleanup(struct platform_device *pd,
 {
 	if (kbdbl_ctl && handle == kbdbl_ctl->handle) {
 		device_remove_file(&pd->dev, &kbdbl_ctl->mode_attr);
-		device_remove_file(&pd->dev, &kbdbl_ctl->timeout_attr);
+		if (kbdbl_ctl->has_timeout)
+			device_remove_file(&pd->dev, &kbdbl_ctl->timeout_attr);
 		kfree(kbdbl_ctl);
 		kbdbl_ctl = NULL;
 	}
Signature · 1 line
2.6.4
#1 lore raw
Quoted 9 lines · Mattia Dongili

SVF1521P6EW, SVF1521DCXW, SVF13N1L2ES and likely most SVF*. do not expose separate timeout controls in auto mode.

Signed-off-by Dominik Matta <[email protected]>
Signed-off-by Mattia Dongili <[email protected]>
---
 drivers/platform/x86/sony-laptop.c | 65 ++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 20 deletions(-)

Thanks Mattia,

The changes look fine.

At some point we should address some of the style inconsistencies with this driver, but to nitpic on them for this patch wouldn't be productive. I've merged this patch - please consider the following for future work with the goal toward making the kernel code as consistent as possible to aid in legibility and maintainability.

drivers/platform/x86/sony-laptop.c static int sony_nc_kbd_backlight_setup(struct platform_device *pd, · line 1885

Quoted 7 lines
drivers/platform/x86/sony-laptop.c +2 -0
@@ -1877,6 +1880,8 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 		unsigned int handle)
 {
 	int result;
+	int probe_base = 0;
+	int ctl_base = 0;
 	int ret = 0;

This isn't in CodingStyle, but is enforced by several maintainers and something I'm trying to standardize on within platform/drivers/x86.

Please declare variables in decreasing line length in the absence of dependencies. Also called "Reverse Christmas Tree Order".

So the above would become:

int probe_base = 0;
int ctl_base = 0;
int ret = 0;
int result;

Some maintainers prefer merging like types, especially with similar purpose. I'm ambivalent on this.

drivers/platform/x86/sony-laptop.c static int sony_nc_kbd_backlight_setup(struct platform_device *pd, · line 1895

Quoted 11 lines
drivers/platform/x86/sony-laptop.c +2 -2
 	if (kbdbl_ctl) {

@@ -1885,11 +1890,25 @@ static int sony_nc_kbd_backlight_setup(struct platform_device *pd,
 		return -EBUSY;
 	}
 
-	/* verify the kbd backlight presence, these handles are not used for
-	 * keyboard backlight only
+	/* verify the kbd backlight presence, some of these handles are not used
+	 * for keyboard backlight only
 	 */

Comment blocks should start with a blank line:

/*
* This is the first line.
* This is the second.
*/

And should use standard sentence capitalization.

Thanks,

Signature · 2 lines
Darren Hart
Intel Open Source Technology Center
author maintainer
#2 lore raw
Quoted 13 lines · Darren Hart
Quoted 9 lines · Mattia Dongili

SVF1521P6EW, SVF1521DCXW, SVF13N1L2ES and likely most SVF*. do not expose separate timeout controls in auto mode.

Signed-off-by Dominik Matta <[email protected]>
Signed-off-by Mattia Dongili <[email protected]>
---
 drivers/platform/x86/sony-laptop.c | 65 ++++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 20 deletions(-)

Thanks Mattia,

Quoted 7 lines

The changes look fine.

At some point we should address some of the style inconsistencies with this driver, but to nitpic on them for this patch wouldn't be productive. I've merged this patch - please consider the following for future work with the goal toward making the kernel code as consistent as possible to aid in legibility and maintainability.

Thanks Darren.

This driver needs many things, but yes, I'll keep in mind these other guidelines for any future change.

Best,

Signature · 2 lines
mattia
:wq!