Keyboard backlight control for some Vaio Fit models

Commit Message

Mattia Dongili Dec. 15, 2015, 5:12 a.m. UTC

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

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

Comments

Darren Hart Dec. 15, 2015, 6:01 p.m. UTC | #1

On Mon, Dec 14, 2015 at 09:12:32PM -0800, Mattia Dongili wrote:
> SVF1521P6EW, SVF1521DCXW, SVF13N1L2ES and likely most SVF*.
> do not expose separate timeout controls in auto mode.
> 
> Signed-off-by: Dominik Matta <[e-mail]>
> Signed-off-by: Mattia Dongili <[e-mail]>
> ---
>  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.

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

>  
>  	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,

Mattia Dongili Dec. 16, 2015, 3:04 p.m. UTC | #2

On Tue, Dec 15, 2015 at 10:01:28AM -0800, Darren Hart wrote:
> On Mon, Dec 14, 2015 at 09:12:32PM -0800, Mattia Dongili wrote:
> > SVF1521P6EW, SVF1521DCXW, SVF13N1L2ES and likely most SVF*.
> > do not expose separate timeout controls in auto mode.
> > 
> > Signed-off-by: Dominik Matta <[e-mail]>
> > Signed-off-by: Mattia Dongili <[e-mail]>
> > ---
> >  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.

Thanks Darren.

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

Best,

Patch

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index f73c295..e9caa34 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -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;
 	}