[PATCH 00/11] ALS and other new functions

[PATCH 00/11] ALS and other new functions

[Mattia Dongili]

Matthew,

This is partially a resend of the previous patch-set (first 7  patches) plus
additional changes to queue up for 3.15.

The largest change is support for ALS on recent Vaios. The work is a respin of
what Javier Achirica and Marco Chiappero had done almost 3 years ago but ported
to IIO (Javier did that).
The downside is that when ALS control is enabled, changes to the backlight via
_BCM become notifications to SNC that should instead act. Unfortunately there
is no obvious way to get the brightness level that was requested. To workaround
that I have added a notification callback to the backlight driver. It seems a
generally useful mechanism and is definitely useful in this case.
The ALS patch has still some rough edges but it has been used by many people
for years now.

Javier Achirica (9):
  sony-laptop: add support as Fn+1 as a hot key
  sony-laptop: Add support for lid resume settings on Vaio Pro
  sony-laptop: add panel_id function
  sony-laptop: add usb charge function
  sony-laptop: add fan speed regulation function
  sony-laptop: add hibernate on low battery function
  sony-laptop: adjust keyboard backlight values for off/auto/on
  sony-laptop: add smart connect control function
  sony-laptop: als support

Mattia Dongili (2):
  backlight: introduce inter-driver notification of changes
  sony-laptop: remove useless sony-laptop versioning

 Documentation/laptops/sony-laptop.txt |   31 +
 drivers/platform/x86/Kconfig          |    1 +
 drivers/platform/x86/sony-laptop.c    | 1668 +++++++++++++++++++++++++++++++--
 drivers/video/backlight/backlight.c   |   10 +
 include/linux/backlight.h             |    3 +
 5 files changed, 1631 insertions(+), 82 deletions(-)

-- 
1.9.0

[PATCH 01/11] sony-laptop: add support as Fn+1 as a hot key

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 8f8551a..fe5424c 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -1122,6 +1122,8 @@ static struct sony_nc_event sony_100_events[] = {
 	{ 0x25, SONYPI_EVENT_ANYBUTTON_RELEASED },
 	{ 0xa6, SONYPI_EVENT_HELP_PRESSED },
 	{ 0x26, SONYPI_EVENT_ANYBUTTON_RELEASED },
+	{ 0xa8, SONYPI_EVENT_FNKEY_1 },
+	{ 0x28, SONYPI_EVENT_ANYBUTTON_RELEASED },
 	{ 0, 0 },
 };
 
-- 
1.9.0

[PATCH 02/11] sony-laptop: Add support for lid resume settings on Vaio Pro

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Vaio Pro uses a different handle and doesn't support all the options as
other models (only S5 setting v/s S3/4/5).

Minor code rework to generalize functions by Mattia Dongili.

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 110 +++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 48 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index fe5424c..de17390 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -152,7 +152,8 @@ static void sony_nc_battery_care_cleanup(struct platform_device *pd);
 static int sony_nc_thermal_setup(struct platform_device *pd);
 static void sony_nc_thermal_cleanup(struct platform_device *pd);
 
-static int sony_nc_lid_resume_setup(struct platform_device *pd);
+static int sony_nc_lid_resume_setup(struct platform_device *pd,
+				    unsigned int handle);
 static void sony_nc_lid_resume_cleanup(struct platform_device *pd);
 
 static int sony_nc_gfx_switch_setup(struct platform_device *pd,
@@ -1341,7 +1342,8 @@ static void sony_nc_function_setup(struct acpi_device *device,
 						result);
 			break;
 		case 0x0119:
-			result = sony_nc_lid_resume_setup(pf_device);
+		case 0x015D:
+			result = sony_nc_lid_resume_setup(pf_device, handle);
 			if (result)
 				pr_err("couldn't set up lid resume function (%d)\n",
 						result);
@@ -1422,6 +1424,7 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 			sony_nc_battery_care_cleanup(pd);
 			break;
 		case 0x0119:
+		case 0x015D:
 			sony_nc_lid_resume_cleanup(pd);
 			break;
 		case 0x0122:
@@ -2223,9 +2226,14 @@ static void sony_nc_thermal_resume(void)
 #endif
 
 /* resume on LID open */
+#define LID_RESUME_S5	0
+#define LID_RESUME_S4	1
+#define LID_RESUME_S3	2
+#define LID_RESUME_MAX	3
 struct snc_lid_resume_control {
-	struct device_attribute attrs[3];
+	struct device_attribute attrs[LID_RESUME_MAX];
 	unsigned int status;
+	int handle;
 };
 static struct snc_lid_resume_control *lid_ctl;
 
@@ -2233,8 +2241,9 @@ static ssize_t sony_nc_lid_resume_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buffer, size_t count)
 {
-	unsigned int result, pos;
+	unsigned int result;
 	unsigned long value;
+	unsigned int pos = LID_RESUME_S5;
 	if (count > 31)
 		return -EINVAL;
 
@@ -2247,21 +2256,21 @@ static ssize_t sony_nc_lid_resume_store(struct device *dev,
 	 * +--------------+
 	 *   2    1    0
 	 */
-	if (strcmp(attr->attr.name, "lid_resume_S3") == 0)
-		pos = 2;
-	else if (strcmp(attr->attr.name, "lid_resume_S4") == 0)
-		pos = 1;
-	else if (strcmp(attr->attr.name, "lid_resume_S5") == 0)
-		pos = 0;
-	else
-               return -EINVAL;
+	while (pos < LID_RESUME_MAX) {
+		if (&lid_ctl->attrs[pos].attr == &attr->attr)
+			break;
+		pos++;
+	}
+	if (pos == LID_RESUME_MAX)
+		return -EINVAL;
 
 	if (value)
 		value = lid_ctl->status | (1 << pos);
 	else
 		value = lid_ctl->status & ~(1 << pos);
 
-	if (sony_call_snc_handle(0x0119, value << 0x10 | 0x0100, &result))
+	if (sony_call_snc_handle(lid_ctl->handle, value << 0x10 | 0x0100,
+				&result))
 		return -EIO;
 
 	lid_ctl->status = value;
@@ -2270,29 +2279,27 @@ static ssize_t sony_nc_lid_resume_store(struct device *dev,
 }
 
 static ssize_t sony_nc_lid_resume_show(struct device *dev,
-				       struct device_attribute *attr, char *buffer)
+					struct device_attribute *attr,
+					char *buffer)
 {
-	unsigned int pos;
-
-	if (strcmp(attr->attr.name, "lid_resume_S3") == 0)
-		pos = 2;
-	else if (strcmp(attr->attr.name, "lid_resume_S4") == 0)
-		pos = 1;
-	else if (strcmp(attr->attr.name, "lid_resume_S5") == 0)
-		pos = 0;
-	else
-		return -EINVAL;
-	       
-	return snprintf(buffer, PAGE_SIZE, "%d\n",
-			(lid_ctl->status >> pos) & 0x01);
+	unsigned int pos = LID_RESUME_S5;
+
+	while (pos < LID_RESUME_MAX) {
+		if (&lid_ctl->attrs[pos].attr == &attr->attr)
+			return snprintf(buffer, PAGE_SIZE, "%d\n",
+					(lid_ctl->status >> pos) & 0x01);
+		pos++;
+	}
+	return -EINVAL;
 }
 
-static int sony_nc_lid_resume_setup(struct platform_device *pd)
+static int sony_nc_lid_resume_setup(struct platform_device *pd,
+					unsigned int handle)
 {
 	unsigned int result;
 	int i;
 
-	if (sony_call_snc_handle(0x0119, 0x0000, &result))
+	if (sony_call_snc_handle(handle, 0x0000, &result))
 		return -EIO;
 
 	lid_ctl = kzalloc(sizeof(struct snc_lid_resume_control), GFP_KERNEL);
@@ -2300,26 +2307,29 @@ static int sony_nc_lid_resume_setup(struct platform_device *pd)
 		return -ENOMEM;
 
 	lid_ctl->status = result & 0x7;
+	lid_ctl->handle = handle;
 
 	sysfs_attr_init(&lid_ctl->attrs[0].attr);
-	lid_ctl->attrs[0].attr.name = "lid_resume_S3";
-	lid_ctl->attrs[0].attr.mode = S_IRUGO | S_IWUSR;
-	lid_ctl->attrs[0].show = sony_nc_lid_resume_show;
-	lid_ctl->attrs[0].store = sony_nc_lid_resume_store;
-
-	sysfs_attr_init(&lid_ctl->attrs[1].attr);
-	lid_ctl->attrs[1].attr.name = "lid_resume_S4";
-	lid_ctl->attrs[1].attr.mode = S_IRUGO | S_IWUSR;
-	lid_ctl->attrs[1].show = sony_nc_lid_resume_show;
-	lid_ctl->attrs[1].store = sony_nc_lid_resume_store;
-
-	sysfs_attr_init(&lid_ctl->attrs[2].attr);
-	lid_ctl->attrs[2].attr.name = "lid_resume_S5";
-	lid_ctl->attrs[2].attr.mode = S_IRUGO | S_IWUSR;
-	lid_ctl->attrs[2].show = sony_nc_lid_resume_show;
-	lid_ctl->attrs[2].store = sony_nc_lid_resume_store;
-
-	for (i = 0; i < 3; i++) {
+	lid_ctl->attrs[LID_RESUME_S5].attr.name = "lid_resume_S5";
+	lid_ctl->attrs[LID_RESUME_S5].attr.mode = S_IRUGO | S_IWUSR;
+	lid_ctl->attrs[LID_RESUME_S5].show = sony_nc_lid_resume_show;
+	lid_ctl->attrs[LID_RESUME_S5].store = sony_nc_lid_resume_store;
+
+	if (handle == 0x0119) {
+		sysfs_attr_init(&lid_ctl->attrs[1].attr);
+		lid_ctl->attrs[LID_RESUME_S4].attr.name = "lid_resume_S4";
+		lid_ctl->attrs[LID_RESUME_S4].attr.mode = S_IRUGO | S_IWUSR;
+		lid_ctl->attrs[LID_RESUME_S4].show = sony_nc_lid_resume_show;
+		lid_ctl->attrs[LID_RESUME_S4].store = sony_nc_lid_resume_store;
+
+		sysfs_attr_init(&lid_ctl->attrs[2].attr);
+		lid_ctl->attrs[LID_RESUME_S3].attr.name = "lid_resume_S3";
+		lid_ctl->attrs[LID_RESUME_S3].attr.mode = S_IRUGO | S_IWUSR;
+		lid_ctl->attrs[LID_RESUME_S3].show = sony_nc_lid_resume_show;
+		lid_ctl->attrs[LID_RESUME_S3].store = sony_nc_lid_resume_store;
+	}
+	for (i = 0; i < LID_RESUME_MAX &&
+			lid_ctl->attrs[LID_RESUME_S3].attr.name; i++) {
 		result = device_create_file(&pd->dev, &lid_ctl->attrs[i]);
 		if (result)
 			goto liderror;
@@ -2342,8 +2352,12 @@ static void sony_nc_lid_resume_cleanup(struct platform_device *pd)
 	int i;
 
 	if (lid_ctl) {
-		for (i = 0; i < 3; i++)
+		for (i = 0; i < LID_RESUME_MAX; i++) {
+			if (!lid_ctl->attrs[i].attr.name)
+				break;
+
 			device_remove_file(&pd->dev, &lid_ctl->attrs[i]);
+		}
 
 		kfree(lid_ctl);
 		lid_ctl = NULL;
-- 
1.9.0

[PATCH 03/11] sony-laptop: add panel_id function

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 59 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index de17390..d1f712e 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -164,6 +164,9 @@ static int __sony_nc_gfx_switch_status_get(void);
 static int sony_nc_highspeed_charging_setup(struct platform_device *pd);
 static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd);
 
+static int sony_nc_panelid_setup(struct platform_device *pd);
+static void sony_nc_panelid_cleanup(struct platform_device *pd);
+
 static int sony_nc_touchpad_setup(struct platform_device *pd,
 				  unsigned int handle);
 static void sony_nc_touchpad_cleanup(struct platform_device *pd);
@@ -1385,6 +1388,12 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up keyboard backlight function (%d)\n",
 						result);
 			break;
+		case 0x011D:
+			result = sony_nc_panelid_setup(pf_device);
+			if (result)
+				pr_err("couldn't set up panel ID function (%d)\n",
+				       result);
+			break;
 		default:
 			continue;
 		}
@@ -1449,6 +1458,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
 			break;
+		case 0x011D:
+			sony_nc_panelid_cleanup(pd);
+			break;
 		default:
 			continue;
 		}
@@ -2540,6 +2552,53 @@ static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd)
 	}
 }
 
+/* Panel ID function */
+static struct device_attribute *panel_handle;
+
+static ssize_t sony_nc_panelid_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x011D, 0x0000, &result))
+		return -EIO;
+
+	return snprintf(buffer, PAGE_SIZE, "%d\n", result);
+}
+
+static int sony_nc_panelid_setup(struct platform_device *pd)
+{
+	unsigned int result;
+
+	panel_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!panel_handle)
+		return -ENOMEM;
+
+	sysfs_attr_init(&panel_handle->attr);
+	panel_handle->attr.name = "panel_id";
+	panel_handle->attr.mode = S_IRUGO;
+	panel_handle->show = sony_nc_panelid_show;
+	panel_handle->store = NULL;
+
+	result = device_create_file(&pd->dev, panel_handle);
+	if (result) {
+		kfree(panel_handle);
+		panel_handle = NULL;
+		return result;
+	}
+
+	return 0;
+}
+
+static void sony_nc_panelid_cleanup(struct platform_device *pd)
+{
+	if (panel_handle) {
+		device_remove_file(&pd->dev, panel_handle);
+		kfree(panel_handle);
+		panel_handle = NULL;
+	}
+}
+
 /* Touchpad enable/disable */
 struct touchpad_control {
 	struct device_attribute attr;
-- 
1.9.0

[PATCH 04/11] sony-laptop: add usb charge function

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Allows to specify if the USB socket should charge attached devices while
the laptop is suspended to ram.

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 86 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index d1f712e..9620285 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -164,6 +164,9 @@ static int __sony_nc_gfx_switch_status_get(void);
 static int sony_nc_highspeed_charging_setup(struct platform_device *pd);
 static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd);
 
+static int sony_nc_usb_charge_setup(struct platform_device *pd);
+static void sony_nc_usb_charge_cleanup(struct platform_device *pd);
+
 static int sony_nc_panelid_setup(struct platform_device *pd);
 static void sony_nc_panelid_cleanup(struct platform_device *pd);
 
@@ -1388,6 +1391,12 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up keyboard backlight function (%d)\n",
 						result);
 			break;
+		case 0x0155:
+			result = sony_nc_usb_charge_setup(pf_device);
+			if (result)
+				pr_err("couldn't set up USB charge support (%d)\n",
+						result);
+			break;
 		case 0x011D:
 			result = sony_nc_panelid_setup(pf_device);
 			if (result)
@@ -1458,6 +1467,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
 			break;
+		case 0x0155:
+			sony_nc_usb_charge_cleanup(pd);
+			break;
 		case 0x011D:
 			sony_nc_panelid_cleanup(pd);
 			break;
@@ -2552,6 +2564,80 @@ static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd)
 	}
 }
 
+/* USB charge function */
+static struct device_attribute *uc_handle;
+
+static ssize_t sony_nc_usb_charge_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buffer, size_t count)
+{
+	unsigned int result;
+	unsigned long value;
+
+	if (count > 31)
+		return -EINVAL;
+
+	if (kstrtoul(buffer, 10, &value) || value > 1)
+		return -EINVAL;
+
+	if (sony_call_snc_handle(0x0155, value << 0x10 | 0x0100, &result))
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t sony_nc_usb_charge_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x0155, 0x0000, &result))
+		return -EIO;
+
+	return snprintf(buffer, PAGE_SIZE, "%d\n", result & 0x01);
+}
+
+static int sony_nc_usb_charge_setup(struct platform_device *pd)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x0155, 0x0000, &result) || !(result & 0x01)) {
+		/* some models advertise the handle but have no implementation
+		 * for it
+		 */
+		pr_info("No USB Charge capability found\n");
+		return 0;
+	}
+
+	uc_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!uc_handle)
+		return -ENOMEM;
+
+	sysfs_attr_init(&uc_handle->attr);
+	uc_handle->attr.name = "usb_charge";
+	uc_handle->attr.mode = S_IRUGO | S_IWUSR;
+	uc_handle->show = sony_nc_usb_charge_show;
+	uc_handle->store = sony_nc_usb_charge_store;
+
+	result = device_create_file(&pd->dev, uc_handle);
+	if (result) {
+		kfree(uc_handle);
+		uc_handle = NULL;
+		return result;
+	}
+
+	return 0;
+}
+
+static void sony_nc_usb_charge_cleanup(struct platform_device *pd)
+{
+	if (uc_handle) {
+		device_remove_file(&pd->dev, uc_handle);
+		kfree(uc_handle);
+		uc_handle = NULL;
+	}
+}
+
 /* Panel ID function */
 static struct device_attribute *panel_handle;
 
-- 
1.9.0

[PATCH 05/11] sony-laptop: add fan speed regulation function

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Rework error exit logic by Mattia Dongili.

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 119 +++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 9620285..7b5a56d 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -164,6 +164,9 @@ static int __sony_nc_gfx_switch_status_get(void);
 static int sony_nc_highspeed_charging_setup(struct platform_device *pd);
 static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd);
 
+static int sony_nc_fanspeed_setup(struct platform_device *pd);
+static void sony_nc_fanspeed_cleanup(struct platform_device *pd);
+
 static int sony_nc_usb_charge_setup(struct platform_device *pd);
 static void sony_nc_usb_charge_cleanup(struct platform_device *pd);
 
@@ -1391,6 +1394,12 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up keyboard backlight function (%d)\n",
 						result);
 			break;
+		case 0x0149:
+			result = sony_nc_fanspeed_setup(pf_device);
+			if (result)
+				pr_err("couldn't set up fan speed function (%d)\n",
+				       result);
+			break;
 		case 0x0155:
 			result = sony_nc_usb_charge_setup(pf_device);
 			if (result)
@@ -1467,6 +1476,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
 			break;
+		case 0x0149:
+			sony_nc_fanspeed_cleanup(pd);
+			break;
 		case 0x0155:
 			sony_nc_usb_charge_cleanup(pd);
 			break;
@@ -2564,6 +2576,113 @@ static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd)
 	}
 }
 
+/* fan speed function */
+static struct device_attribute *fan_handle, *hsf_handle;
+
+static ssize_t sony_nc_hsfan_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buffer, size_t count)
+{
+	unsigned int result;
+	unsigned long value;
+
+	if (count > 31)
+		return -EINVAL;
+
+	if (kstrtoul(buffer, 10, &value) || value > 1)
+		return -EINVAL;
+
+	if (sony_call_snc_handle(0x0149, value << 0x10 | 0x0200, &result))
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t sony_nc_hsfan_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x0149, 0x0100, &result))
+		return -EIO;
+
+	return snprintf(buffer, PAGE_SIZE, "%d\n", result & 0x01);
+}
+
+static ssize_t sony_nc_fanspeed_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x0149, 0x0300, &result))
+		return -EIO;
+
+	return snprintf(buffer, PAGE_SIZE, "%d\n", result & 0xff);
+}
+
+static int sony_nc_fanspeed_setup(struct platform_device *pd)
+{
+	unsigned int result;
+
+	fan_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!fan_handle)
+		return -ENOMEM;
+
+	hsf_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!hsf_handle) {
+		result = -ENOMEM;
+		goto out_hsf_handle_alloc;
+	}
+
+	sysfs_attr_init(&fan_handle->attr);
+	fan_handle->attr.name = "fanspeed";
+	fan_handle->attr.mode = S_IRUGO;
+	fan_handle->show = sony_nc_fanspeed_show;
+	fan_handle->store = NULL;
+
+	sysfs_attr_init(&hsf_handle->attr);
+	hsf_handle->attr.name = "fan_forced";
+	hsf_handle->attr.mode = S_IRUGO | S_IWUSR;
+	hsf_handle->show = sony_nc_hsfan_show;
+	hsf_handle->store = sony_nc_hsfan_store;
+
+	result = device_create_file(&pd->dev, fan_handle);
+	if (result)
+		goto out_fan_handle;
+
+	result = device_create_file(&pd->dev, hsf_handle);
+	if (result)
+		goto out_hsf_handle;
+
+	return 0;
+
+out_hsf_handle:
+	device_remove_file(&pd->dev, fan_handle);
+
+out_fan_handle:
+	kfree(hsf_handle);
+	hsf_handle = NULL;
+
+out_hsf_handle_alloc:
+	kfree(fan_handle);
+	fan_handle = NULL;
+	return result;
+}
+
+static void sony_nc_fanspeed_cleanup(struct platform_device *pd)
+{
+	if (fan_handle) {
+		device_remove_file(&pd->dev, fan_handle);
+		kfree(fan_handle);
+		fan_handle = NULL;
+	}
+	if (hsf_handle) {
+		device_remove_file(&pd->dev, hsf_handle);
+		kfree(hsf_handle);
+		hsf_handle = NULL;
+	}
+}
+
 /* USB charge function */
 static struct device_attribute *uc_handle;
 
-- 
1.9.0

[PATCH 06/11] sony-laptop: add hibernate on low battery function

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 78 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 7b5a56d..ba39a29 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -164,6 +164,9 @@ static int __sony_nc_gfx_switch_status_get(void);
 static int sony_nc_highspeed_charging_setup(struct platform_device *pd);
 static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd);
 
+static int sony_nc_lowbatt_setup(struct platform_device *pd);
+static void sony_nc_lowbatt_cleanup(struct platform_device *pd);
+
 static int sony_nc_fanspeed_setup(struct platform_device *pd);
 static void sony_nc_fanspeed_cleanup(struct platform_device *pd);
 
@@ -1394,6 +1397,12 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up keyboard backlight function (%d)\n",
 						result);
 			break;
+		case 0x0121:
+			result = sony_nc_lowbatt_setup(pf_device);
+			if (result)
+				pr_err("couldn't set up low battery function (%d)\n",
+				       result);
+			break;
 		case 0x0149:
 			result = sony_nc_fanspeed_setup(pf_device);
 			if (result)
@@ -1476,6 +1485,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
 			break;
+		case 0x0121:
+			sony_nc_lowbatt_cleanup(pd);
+			break;
 		case 0x0149:
 			sony_nc_fanspeed_cleanup(pd);
 			break;
@@ -2576,6 +2588,72 @@ static void sony_nc_highspeed_charging_cleanup(struct platform_device *pd)
 	}
 }
 
+/* low battery function */
+static struct device_attribute *lowbatt_handle;
+
+static ssize_t sony_nc_lowbatt_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buffer, size_t count)
+{
+	unsigned int result;
+	unsigned long value;
+
+	if (count > 31)
+		return -EINVAL;
+
+	if (kstrtoul(buffer, 10, &value) || value > 1)
+		return -EINVAL;
+
+	if (sony_call_snc_handle(0x0121, value << 8, &result))
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t sony_nc_lowbatt_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	unsigned int result;
+
+	if (sony_call_snc_handle(0x0121, 0x0200, &result))
+		return -EIO;
+
+	return snprintf(buffer, PAGE_SIZE, "%d\n", result & 1);
+}
+
+static int sony_nc_lowbatt_setup(struct platform_device *pd)
+{
+	unsigned int result;
+
+	lowbatt_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!lowbatt_handle)
+		return -ENOMEM;
+
+	sysfs_attr_init(&lowbatt_handle->attr);
+	lowbatt_handle->attr.name = "lowbatt_hibernate";
+	lowbatt_handle->attr.mode = S_IRUGO | S_IWUSR;
+	lowbatt_handle->show = sony_nc_lowbatt_show;
+	lowbatt_handle->store = sony_nc_lowbatt_store;
+
+	result = device_create_file(&pd->dev, lowbatt_handle);
+	if (result) {
+		kfree(lowbatt_handle);
+		lowbatt_handle = NULL;
+		return result;
+	}
+
+	return 0;
+}
+
+static void sony_nc_lowbatt_cleanup(struct platform_device *pd)
+{
+	if (lowbatt_handle) {
+		device_remove_file(&pd->dev, lowbatt_handle);
+		kfree(lowbatt_handle);
+		lowbatt_handle = NULL;
+	}
+}
+
 /* fan speed function */
 static struct device_attribute *fan_handle, *hsf_handle;
 
-- 
1.9.0

[PATCH 07/11] sony-laptop: adjust keyboard backlight values for off/auto/on

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

Keyboard backlight can be always off, use some automatic trigger
(activity and light sensor), always on.
The behaviour of the driver changes whereby previously when passed 1 it
tried to turn on backlight immediately now it does nothing. This is
however a bug fix since (a) it makes little sense to turn on the
backlight when control is automatic and (b) this behaviour is
consistent with what the windows driver does.

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index ba39a29..48e7e5b 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -129,7 +129,8 @@ static int kbd_backlight = -1;
 module_param(kbd_backlight, int, 0444);
 MODULE_PARM_DESC(kbd_backlight,
 		 "set this to 0 to disable keyboard backlight, "
-		 "1 to enable it (default: no change from current value)");
+		 "1 to enable it with automatic control and 2 to have it always "
+		 "on (default: no change from current value)");
 
 static int kbd_backlight_timeout = -1;
 module_param(kbd_backlight_timeout, int, 0444);
@@ -1772,7 +1773,7 @@ static ssize_t __sony_nc_kbd_backlight_mode_set(u8 value)
 {
 	int result;
 
-	if (value > 1)
+	if (value > 2)
 		return -EINVAL;
 
 	if (sony_call_snc_handle(kbdbl_ctl->handle,
@@ -1780,8 +1781,10 @@ static ssize_t __sony_nc_kbd_backlight_mode_set(u8 value)
 		return -EIO;
 
 	/* Try to turn the light on/off immediately */
-	sony_call_snc_handle(kbdbl_ctl->handle,
-			(value << 0x10) | (kbdbl_ctl->base + 0x100), &result);
+	if (value != 1)
+		sony_call_snc_handle(kbdbl_ctl->handle,
+				(value << 0x0f) | (kbdbl_ctl->base + 0x100),
+				&result);
 
 	kbdbl_ctl->mode = value;
 
-- 
1.9.0

[PATCH 08/11] sony-laptop: add smart connect control function

[Mattia Dongili]

From: Javier Achirica <jachirica@gmail.com>

The current value is not available through the SNC device and therefore
the attribute is writable only.

Signed-off-by: Javier Achirica <jachirica@gmail.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 67 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 48e7e5b..19d769e 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -177,6 +177,9 @@ static void sony_nc_usb_charge_cleanup(struct platform_device *pd);
 static int sony_nc_panelid_setup(struct platform_device *pd);
 static void sony_nc_panelid_cleanup(struct platform_device *pd);
 
+static int sony_nc_smart_conn_setup(struct platform_device *pd);
+static void sony_nc_smart_conn_cleanup(struct platform_device *pd);
+
 static int sony_nc_touchpad_setup(struct platform_device *pd,
 				  unsigned int handle);
 static void sony_nc_touchpad_cleanup(struct platform_device *pd);
@@ -1422,6 +1425,12 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up panel ID function (%d)\n",
 				       result);
 			break;
+		case 0x0168:
+			result = sony_nc_smart_conn_setup(pf_device);
+			if (result)
+				pr_err("couldn't set up smart connect support (%d)\n",
+						result);
+			break;
 		default:
 			continue;
 		}
@@ -1498,6 +1507,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x011D:
 			sony_nc_panelid_cleanup(pd);
 			break;
+		case 0x0168:
+			sony_nc_smart_conn_cleanup(pd);
+			break;
 		default:
 			continue;
 		}
@@ -2885,6 +2897,61 @@ static void sony_nc_panelid_cleanup(struct platform_device *pd)
 	}
 }
 
+/* smart connect function */
+static struct device_attribute *sc_handle;
+
+static ssize_t sony_nc_smart_conn_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buffer, size_t count)
+{
+	unsigned int result;
+	unsigned long value;
+
+	if (count > 31)
+		return -EINVAL;
+
+	if (kstrtoul(buffer, 10, &value) || value > 1)
+		return -EINVAL;
+
+	if (sony_call_snc_handle(0x0168, value << 0x10, &result))
+		return -EIO;
+
+	return count;
+}
+
+static int sony_nc_smart_conn_setup(struct platform_device *pd)
+{
+	unsigned int result;
+
+	sc_handle = kzalloc(sizeof(struct device_attribute), GFP_KERNEL);
+	if (!sc_handle)
+		return -ENOMEM;
+
+	sysfs_attr_init(&sc_handle->attr);
+	sc_handle->attr.name = "smart_connect";
+	sc_handle->attr.mode = S_IWUSR;
+	sc_handle->show = NULL;
+	sc_handle->store = sony_nc_smart_conn_store;
+
+	result = device_create_file(&pd->dev, sc_handle);
+	if (result) {
+		kfree(sc_handle);
+		sc_handle = NULL;
+		return result;
+	}
+
+	return 0;
+}
+
+static void sony_nc_smart_conn_cleanup(struct platform_device *pd)
+{
+	if (sc_handle) {
+		device_remove_file(&pd->dev, sc_handle);
+		kfree(sc_handle);
+		sc_handle = NULL;
+	}
+}
+
 /* Touchpad enable/disable */
 struct touchpad_control {
 	struct device_attribute attr;
-- 
1.9.0

[PATCH 09/11] backlight: introduce inter-driver notification of changes

[Mattia Dongili]

Allow registered backlight drivers to receive a notification when a
backlight change is triggered via one of the siblings.
sony-laptop needs this notification to be able to set backlight when ALS
is enabled and setting brightness via _BCM becomes a no-op and only
notifies SNC that "a change to the brightness level" was requested
without giving any actual value.

Cc: Jingoo Han <jg1.han@samsung.com>
Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/video/backlight/backlight.c | 10 ++++++++++
 include/linux/backlight.h           |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 5d05555..0f9d6ce 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -88,6 +88,7 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 static void backlight_generate_event(struct backlight_device *bd,
 				     enum backlight_update_reason reason)
 {
+	struct backlight_device *obd;
 	char *envp[2];
 
 	switch (reason) {
@@ -104,6 +105,15 @@ static void backlight_generate_event(struct backlight_device *bd,
 	envp[1] = NULL;
 	kobject_uevent_env(&bd->dev.kobj, KOBJ_CHANGE, envp);
 	sysfs_notify(&bd->dev.kobj, NULL, "actual_brightness");
+
+	/* notify other bl devices */
+	list_for_each_entry(obd, &backlight_dev_list, entry) {
+		if (bd == obd || !obd->ops || !obd->ops->brightness_changed)
+			continue;
+
+		obd->ops->brightness_changed(obd, bd->props.brightness,
+				bd->props.max_brightness);
+	}
 }
 
 static ssize_t bl_power_show(struct device *dev, struct device_attribute *attr,
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5f9cd96..974ae3c 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -55,6 +55,9 @@ struct backlight_ops {
 	/* Check if given framebuffer device is the one bound to this backlight;
 	   return 0 if not, !=0 if it is. If NULL, backlight always matches the fb. */
 	int (*check_fb)(struct backlight_device *, struct fb_info *);
+	/* Callback to receive notification of a backlight change triggered by
+	   a different backlight driver */
+	void (*brightness_changed)(struct backlight_device *, int, int);
 };
 
 /* This structure defines all the properties of a backlight */
-- 
1.9.0

[PATCH 10/11] sony-laptop: als support

[Mattia Dongili]

From: Javier Achirica <jachirica-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

vaios come with two different type of ACPI based ALS controls. The
oldest incarnation exposes and requires a much lower level control over
the ALS device (tsl256x) and raw data, the most recent one is much
simpler and only allows fetching lux readings.
Additionally, the DSDT exposes data tables that can be used apply
corrections and appropriate scaling to the sensor's readings.
The device is registered via the IIO subsystem and the tables exposed as
attributes of the IIO device.

As a bonus, enabling the sensor makes backlight Fn keys behave
differently than default. Instead of changing backlight, _BCM will
generate events to the SNC device (with no indication of the new
backlight value) expecting that to do the legwork instead.
To make this work in Linux we force registering platform backlight
control and use the backlight subsystem to get notified about changes
and applying the change using SNC specific methods.

The device is enabled by default but allows disabling passing
enable_als=0 as a module parameter.

Mattia's changes:
* remove unused event read functions
* rework backlight device registration precedence
* add callback for brightness change notifications
* re-arm ALS events after the event
* Kconfig: select IIO
* remove unused function and redundant allocation
* reformat code
* add module parameter to disable ALS
* do not force backlight device to be created when no ALS is setup.
* remove redundant tsl driver setup calls
* change calculate_lux function to return a value
* simplify tsl256x_get_id function

Marco Chiappero worked on early revisions of this code, see
http://www.absence.it/vaio-acpi/

Cc: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Marco Chiappero <marco-YK5v5TwyeXionA0d6jMUrA@public.gmane.org>
Signed-off-by: Javier Achirica <jachirica-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mattia Dongili <malattia-k2GhghHVRtY@public.gmane.org>
---
 Documentation/laptops/sony-laptop.txt |   31 +
 drivers/platform/x86/Kconfig          |    1 +
 drivers/platform/x86/sony-laptop.c    | 1111 ++++++++++++++++++++++++++++++++-
 3 files changed, 1128 insertions(+), 15 deletions(-)

diff --git a/Documentation/laptops/sony-laptop.txt b/Documentation/laptops/sony-laptop.txt
index 978b1e6..014ebdf 100644
--- a/Documentation/laptops/sony-laptop.txt
+++ b/Documentation/laptops/sony-laptop.txt
@@ -79,6 +79,37 @@ such a laptop you will find the necessary rfkill devices under
 /sys/class/rfkill. Check those starting with sony-* in
 	# grep . /sys/class/rfkill/*/{state,name}
 
+Ambient Light Sensor:
+---------------------
+On models equipped with ALS, sony-laptop will create an IIO device available
+under:
+	/sys/bus/iio/devices/
+In addition to the sensor's readings, a number of other attributes are exposed
+for userspace to consume:
+	typical_illuminance	Typical illuminance level for brightness
+				control in lux.
+	backlight_illuminance	Threshold in lux for keyboard backlight.
+	lux_correction_percent	Correction factor to apply to brightness in
+				percentage.
+	low_threshold_percent	Percentage from current lux level to trigger
+				event (low).
+	high_threshold_percent	Percentage from current lux level to trigger
+				event (high).
+	als_event_rate		Time in milliseconds between brightness changes
+				when triggered by light change.
+	als_event_delay		Percentage of brightness changes when triggered
+				by light change.
+	key_event_rate		Time in milliseconds between brightness changes
+				when triggered by keypress.
+	key_event_delay		Percentage of brightness changes when triggered
+				by keypress.
+	interrupt_rate		Light sensor interrupt rate in milliseconds.
+	light_compensation_x10	Light compensation needed due to sensor cover.
+	minimum_illuminance	Minimum expected illuminance level in lux.
+	maximum_illuminance	Maximum expected illuminance level in lux.
+	algorithm		Brightness calculation algorithm to be used.
+Depending on the sensor and laptop model, some of these attributes may not be
+available.
 
 Development:
 ------------
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5ae65c1..761086f 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -279,6 +279,7 @@ config SONY_LAPTOP
 	select BACKLIGHT_CLASS_DEVICE
 	depends on INPUT
 	depends on RFKILL
+	select IIO
 	  ---help---
 	  This mini-driver drives the SNC and SPIC devices present in the ACPI
 	  BIOS of the Sony Vaio laptops.
diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 19d769e..b0a311f 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -61,6 +61,9 @@
 #include <linux/workqueue.h>
 #include <linux/acpi.h>
 #include <linux/slab.h>
+#include <acpi/video.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
 #include <linux/sonypi.h>
 #include <linux/sony-laptop.h>
 #include <linux/rfkill.h>
@@ -138,6 +141,12 @@ MODULE_PARM_DESC(kbd_backlight_timeout,
 		 "meaningful values vary from 0 to 3 and their meaning depends "
 		 "on the model (default: no change from current value)");
 
+static int enable_als = 1;
+module_param(enable_als, int, 0444);
+MODULE_PARM_DESC(enable_als,
+		 "set this to 0 to disable the SNC controlled ALS sensor "
+		 "(default: 1)");
+
 #ifdef CONFIG_PM_SLEEP
 static void sony_nc_thermal_resume(void);
 #endif
@@ -200,6 +209,138 @@ static int sony_nc_rfkill_setup(struct acpi_device *device,
 static void sony_nc_rfkill_cleanup(void);
 static void sony_nc_rfkill_update(void);
 
+#define ALS_TABLE_SIZE	126
+
+struct als_device_ops {
+	int (*init)(const u8 defaults[]);
+	int (*exit)(void);
+	int (*event_handler)(void);
+	int (*set_power)(unsigned int);
+	int (*get_lux)(unsigned int *);
+	int (*get_kelvin)(unsigned int *);
+};
+
+struct als_device {
+	int handle;
+
+	unsigned int levels_num;
+	u8 *levels;
+	u8 *defaults;
+	u8 parameters[ALS_TABLE_SIZE];
+	struct als_param_list *als_params;
+
+	/* common device operations */
+	const struct als_device_ops *ops;
+
+	/* IIO device */
+	struct iio_dev *indio_dev;
+};
+static struct als_device *als_handle;
+
+struct als_param_list {
+	int offset;
+	char *name;
+};
+
+static struct als_param_list als_params[] = {
+	{ 1, "typical_illuminance" },
+	{ 11, "backlight_illuminance" },
+	{ 0, "lux_correction_percent" },
+	{ 3, "low_threshold_percent" },
+	{ 4, "high_threshold_percent" },
+	{ 5, "als_event_rate" },
+	{ 6, "als_event_delay" },
+	{ 7, "key_event_rate" },
+	{ 8, "key_event_delay" },
+	{ 9, "interrupt_rate" },
+	{ 10, "light_compensation_x10" },
+	{ 0, NULL }
+};
+
+static struct als_param_list ngals_params[] = {
+	/* Should these two should go to a "range" file in IIO? */
+	{ 4, "minimum_illuminance" },
+	{ 6, "maximum_illuminance" },
+	{ 0, "als_event_rate" },
+	{ 1, "als_event_delay" },
+	{ 2, "key_event_rate" },
+	{ 3, "key_event_delay" },
+	{ 8, "algorithm" },
+	{ 0, NULL }
+};
+
+static ssize_t sony_nc_als_parameters_show(struct device *dev,
+		struct device_attribute *attr, char *buffer);
+static ssize_t sony_nc_als_param_show(struct device *dev,
+		struct device_attribute *attr, char *buffer);
+
+static struct device_attribute als_attr_backlight_sensibility_table =
+	__ATTR(backlight_sensibility_table, 0444,
+		sony_nc_als_parameters_show, 0);
+
+static ssize_t sony_nc_als_kelvin_show(struct device *dev,
+		struct device_attribute *attr, char *buffer);
+
+static struct device_attribute als_attr_kelvin =
+	__ATTR(kelvin, 0444, sony_nc_als_kelvin_show, 0);
+
+#define ALS_ATTR(_name, _mode)					\
+struct device_attribute als_attr_##_name = __ATTR(_name,	\
+		_mode, sony_nc_als_param_show, 0)
+
+static ALS_ATTR(typical_illuminance, 0444);
+static ALS_ATTR(backlight_illuminance, 0444);
+static ALS_ATTR(lux_correction_percent, 0444);
+static ALS_ATTR(low_threshold_percent, 0444);
+static ALS_ATTR(high_threshold_percent, 0444);
+static ALS_ATTR(als_event_rate, 0444);
+static ALS_ATTR(als_event_delay, 0444);
+static ALS_ATTR(key_event_rate, 0444);
+static ALS_ATTR(key_event_delay, 0444);
+static ALS_ATTR(interrupt_rate, 0444);
+static ALS_ATTR(light_compensation_x10, 0444);
+
+static ALS_ATTR(minimum_illuminance, 0444);
+static ALS_ATTR(maximum_illuminance, 0444);
+static ALS_ATTR(algorithm, 0444);
+
+static struct attribute *als_attributes[] = {
+	&als_attr_backlight_sensibility_table.attr,
+	&als_attr_typical_illuminance.attr,
+	&als_attr_backlight_illuminance.attr,
+	&als_attr_lux_correction_percent.attr,
+	&als_attr_low_threshold_percent.attr,
+	&als_attr_high_threshold_percent.attr,
+	&als_attr_als_event_rate.attr,
+	&als_attr_als_event_delay.attr,
+	&als_attr_key_event_rate.attr,
+	&als_attr_key_event_delay.attr,
+	&als_attr_interrupt_rate.attr,
+	&als_attr_light_compensation_x10.attr,
+	&als_attr_kelvin.attr,
+	NULL
+};
+
+static struct attribute *ngals_attributes[] = {
+	&als_attr_backlight_sensibility_table.attr,
+	&als_attr_minimum_illuminance.attr,
+	&als_attr_maximum_illuminance.attr,
+	&als_attr_als_event_rate.attr,
+	&als_attr_als_event_delay.attr,
+	&als_attr_key_event_rate.attr,
+	&als_attr_key_event_delay.attr,
+	&als_attr_algorithm.attr,
+	NULL
+};
+
+struct attribute_group als_attr_group = {
+	.name = "als_parameters"
+};
+
+static int sony_nc_als_setup(struct acpi_device *dev, unsigned int handle);
+static void sony_nc_als_cleanup(struct platform_device *pd);
+static void sony_nc_als_resume(void);
+
 /*********** Input Devices ***********/
 
 #define SONY_LAPTOP_BUF_SIZE	128
@@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct backlight_device *bd)
 	return (result & 0xff) - sdev->offset;
 }
 
-static int sony_nc_update_status_ng(struct backlight_device *bd)
+static int __sony_nc_set_brightness_ng(struct sony_backlight_props *bl,
+		int brightness)
 {
-	int value, result;
-	struct sony_backlight_props *sdev =
-		(struct sony_backlight_props *)bl_get_data(bd);
+	int result = 0;
+	int value = brightness + bl->offset;
 
-	value = bd->props.brightness + sdev->offset;
-	if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value << 0x10),
+	if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value << 0x10),
 				&result))
 		return -EIO;
 
 	return value;
 }
 
+
+static int sony_nc_update_status_ng(struct backlight_device *bd)
+{
+	struct sony_backlight_props *sdev =
+		(struct sony_backlight_props *)bl_get_data(bd);
+
+	return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
+}
+
+static void sony_nc_brightness_changed_ng(struct backlight_device *bd,
+		int brightness, int max_brightness)
+{
+	int new_brightness =
+		brightness * bd->props.max_brightness / max_brightness;
+	struct sony_backlight_props *sdev =
+		(struct sony_backlight_props *)bl_get_data(bd);
+
+	dprintk("brightness changed cur: %d, max: %d -> new: %d\n", brightness,
+			max_brightness, new_brightness);
+
+	__sony_nc_set_brightness_ng(sdev, new_brightness);
+	backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
+}
+
 static const struct backlight_ops sony_backlight_ops = {
 	.options = BL_CORE_SUSPENDRESUME,
 	.update_status = sony_backlight_update_status,
@@ -1088,6 +1252,7 @@ static const struct backlight_ops sony_backlight_ng_ops = {
 	.options = BL_CORE_SUSPENDRESUME,
 	.update_status = sony_nc_update_status_ng,
 	.get_brightness = sony_nc_get_brightness_ng,
+	.brightness_changed = &sony_nc_brightness_changed_ng,
 };
 
 /*
@@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event, unsigned int handle)
 enum event_types {
 	HOTKEY = 1,
 	KILLSWITCH,
-	GFX_SWITCH
+	GFX_SWITCH,
+	ALS
 };
 static void sony_nc_notify(struct acpi_device *device, u32 event)
 {
@@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
 			ev_type = GFX_SWITCH;
 			real_ev = __sony_nc_gfx_switch_status_get();
 			break;
+
+		case 0x0143:
+		case 0x014b:
+		case 0x014c:
+		case 0x0163:
+			sony_call_snc_handle(handle, 0x2000, &result);
+			/* event reasons are inverted ? */
+			real_ev = (result & 0x03) == 1 ? 2 : 1;
+			dprintk("ALS event received (%s change): 0x%x\n",
+					real_ev == 1 ?
+					"light" : "backlight", result);
+			if (real_ev == 1 &&
+					als_handle->ops->event_handler)
+				als_handle->ops->event_handler();
+
+			ev_type = ALS;
+			break;
+
+		case 0x012f:
+		case 0x0137:
+			sony_call_snc_handle(handle, 0x0800, &result);
+			real_ev = result & 0x03;
+			dprintk("ALS event received (%s change): 0x%x\n",
+					real_ev == 1 ?
+					"light" : "backlight", result);
+			if (real_ev == 1 &&
+					als_handle->ops->event_handler)
+				als_handle->ops->event_handler();
+
+			ev_type = ALS;
+			break;
+
 		default:
 			dprintk("Unknown event 0x%x for handle 0x%x\n",
 					event, handle);
@@ -1378,6 +1576,11 @@ static void sony_nc_function_setup(struct acpi_device *device,
 				pr_err("couldn't set up GFX Switch status (%d)\n",
 						result);
 			break;
+		case 0x012f:
+			result = sony_nc_als_setup(device, handle);
+			if (result)
+				pr_err("couldn't set up ALS (%d)\n", result);
+			break;
 		case 0x0131:
 			result = sony_nc_highspeed_charging_setup(pf_device);
 			if (result)
@@ -1400,6 +1603,10 @@ static void sony_nc_function_setup(struct acpi_device *device,
 			if (result)
 				pr_err("couldn't set up keyboard backlight function (%d)\n",
 						result);
+			result = sony_nc_als_setup(device, handle);
+			if (result)
+				pr_err("couldn't set up ALS (%d)\n", result);
+
 			break;
 		case 0x0121:
 			result = sony_nc_lowbatt_setup(pf_device);
@@ -1481,6 +1688,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x015B:
 			sony_nc_gfx_switch_cleanup(pd);
 			break;
+		case 0x012f:
+			sony_nc_als_cleanup(pd);
+			break;
 		case 0x0131:
 			sony_nc_highspeed_charging_cleanup(pd);
 			break;
@@ -1494,6 +1704,7 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
 		case 0x014c:
 		case 0x0163:
 			sony_nc_kbd_backlight_cleanup(pd, handle);
+			sony_nc_als_cleanup(pd);
 			break;
 		case 0x0121:
 			sony_nc_lowbatt_cleanup(pd);
@@ -1550,6 +1761,14 @@ static void sony_nc_function_resume(void)
 		case 0x0135:
 			sony_nc_rfkill_update();
 			break;
+		case 0x012f:
+		case 0x0137:
+		case 0x0143:
+		case 0x014b:
+		case 0x014c:
+		case 0x0163:
+			sony_nc_als_resume();
+			break;
 		default:
 			continue;
 		}
@@ -1769,6 +1988,845 @@ static int sony_nc_rfkill_setup(struct acpi_device *device,
 	return 0;
 }
 
+/* ALS */
+
+/* model specific ALS data and controls
+ * TAOS TSL256x device data
+ */
+#define LUX_SHIFT_BITS		16	/* for non-floating point math */
+/* scale 100000 multiplied fractional coefficients rounding the values */
+#define SCALE(u)	((((((u64) u) << LUX_SHIFT_BITS) / 10000) + 5) / 10)
+
+#define TSL256X_REG_CTRL	0x00
+#define TSL256X_REG_TIMING	0x01
+#define TSL256X_REG_TLOW	0x02
+#define TSL256X_REG_THIGH	0x04
+#define TSL256X_REG_INT		0x06
+#define TSL256X_REG_ID		0x0a
+#define TSL256X_REG_DATA0	0x0c
+#define TSL256X_REG_DATA1	0x0e
+
+#define TSL256X_POWER_ON	0x03
+#define TSL256X_POWER_OFF	0x00
+
+#define TSL256X_POWER_MASK	0x03
+#define TSL256X_INT_MASK	0x10
+
+struct tsl256x_coeff {
+	u32 ratio;
+	u32 ch0;
+	u32 ch1;
+	u32 ka;
+	s32 kb;
+};
+
+struct tsl256x_gain_coeff {
+	u32 time;
+	u16 min;
+	u16 max;
+	u16 scale;
+};
+
+struct tsl256x_gain_coeff tsl256x_gain[] = {
+	{
+		.time   = 0x12, /* 402 ms, gain x16 */
+		.min    = 0,
+		.max    = 65534,
+		.scale  = 1,
+	}, {
+		.time   = 0x02, /* 402 ms, gain x1 */
+		.min    = 2048,
+		.max    = 65534,
+		.scale  = 16,
+	}, {
+		.time   = 0x01, /* 101 ms, gain x1 */
+		.min    = 4095,
+		.max    = 37176,
+		.scale  = 64,
+	}, {
+		.time   = 0x00, /* 13.7 ms, gain x1 */
+		.min    = 3000,
+		.max    = 65535,
+		.scale  = 468,
+	},
+};
+
+struct tsl256x_data {
+	unsigned int periods;
+	struct tsl256x_gain_coeff const *gain;
+	u8 defaults[4];
+	struct tsl256x_coeff const *coeff_table;
+};
+static struct tsl256x_data *tsl256x_handle;
+
+static const struct tsl256x_coeff tsl256x_coeff_fn[] = {
+	{
+		.ratio	= SCALE(12500),	/* 0.125 * 2^LUX_SHIFT_BITS  */
+		.ch0	= SCALE(3040),	/* 0.0304 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(2720),	/* 0.0272 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(313550000),
+		.kb	= -10651,
+	}, {
+		.ratio	= SCALE(25000),	/* 0.250 * 2^LUX_SHIFT_BITS  */
+		.ch0	= SCALE(3250),	/* 0.0325 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(4400),	/* 0.0440 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(203390000),
+		.kb	= -2341,
+	}, {
+		.ratio	= SCALE(37500),	/* 0.375 * 2^LUX_SHIFT_BITS  */
+		.ch0	= SCALE(3510),	/* 0.0351 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(5440),	/* 0.0544 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(152180000),
+		.kb	= 157,
+	}, {
+		.ratio	= SCALE(50000),	/* 0.50 * 2^LUX_SHIFT_BITS   */
+		.ch0	= SCALE(3810),	/* 0.0381 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(6240),	/* 0.0624 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(163580000),
+		.kb	= -145,
+	}, {
+		.ratio	= SCALE(61000),	/* 0.61 * 2^LUX_SHIFT_BITS   */
+		.ch0	= SCALE(2240),	/* 0.0224 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(3100),	/* 0.0310 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(180800000),
+		.kb	= -495,
+	}, {
+		.ratio	= SCALE(80000),	/* 0.80 * 2^LUX_SHIFT_BITS   */
+		.ch0	= SCALE(1280),	/* 0.0128 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(1530),	/* 0.0153 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(197340000),
+		.kb	= -765
+	}, {
+		.ratio	= SCALE(130000),/* 1.3 * 2^LUX_SHIFT_BITS     */
+		.ch0	= SCALE(146),	/* 0.00146 * 2^LUX_SHIFT_BITS */
+		.ch1	= SCALE(112),	/* 0.00112 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(182900000),
+		.kb	= -608,
+	}, {
+		.ratio	= UINT_MAX,	/* for higher ratios */
+		.ch0	= 0,
+		.ch1	= 0,
+		.ka	= 0,
+		.kb	= 830,
+	}
+};
+
+static const struct tsl256x_coeff tsl256x_coeff_cs[] = {
+	{
+		.ratio  = SCALE(13000),	/* 0.130 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(3150),	/* 0.0315 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(2620),	/* 0.0262 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(300370000),
+		.kb	= -9587,
+	}, {
+		.ratio  = SCALE(26000),	/* 0.260 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(3370),	/* 0.0337 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(4300),	/* 0.0430 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(194270000),
+		.kb	= -1824,
+	}, {
+		.ratio  = SCALE(39000),	/* 0.390 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(3630),	/* 0.0363 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(5290),	/* 0.0529 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(152520000),
+		.kb	= 145,
+	}, {
+		.ratio  = SCALE(52000),	/* 0.520 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(3920),	/* 0.0392 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(6050),	/* 0.0605 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(165960000),
+		.kb	= -200,
+	}, {
+		.ratio  = SCALE(65000),	/* 0.650 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(2290),	/* 0.0229 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(2910),	/* 0.0291 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(184800000),
+		.kb	= -566,
+	}, {
+		.ratio  = SCALE(80000),	/* 0.800 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(1570),	/* 0.0157 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(1800),	/* 0.0180 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(199220000),
+		.kb	= -791,
+	}, {
+		.ratio  = SCALE(130000),/* 0.130 * 2^LUX_SHIFT_BITS  */
+		.ch0    = SCALE(338),	/* 0.00338 * 2^LUX_SHIFT_BITS */
+		.ch1    = SCALE(260),	/* 0.00260 * 2^LUX_SHIFT_BITS */
+		.ka	= SCALE(182900000),
+		.kb	= -608,
+	}, {
+		.ratio  = UINT_MAX,	/* for higher ratios */
+		.ch0    = 0,
+		.ch1    = 0,
+		.ka	= 0,
+		.kb	= 830,
+	}
+};
+
+/* TAOS helper & control functions */
+static inline int tsl256x_exec_writebyte(unsigned int reg,
+						unsigned int const *value)
+{
+	unsigned int result;
+
+	return (sony_call_snc_handle(als_handle->handle, (*value << 0x18) |
+		(reg << 0x10) | 0x800500, &result) || !(result & 0x01))
+		? -EIO : 0;
+}
+
+static inline int tsl256x_exec_writeword(unsigned int reg,
+						unsigned int const *value)
+{
+	u8 result[1];
+	int offset = sony_find_snc_handle(als_handle->handle);
+	u64 arg = (*value << 0x18) | (reg << 0x10) | 0xA00700 | offset;
+
+	/* using sony_call_snc_handle_buffer due to possible input overflows */
+	if (sony_nc_buffer_call(sony_nc_acpi_handle, "SN06", &arg,
+				result, 1) < 0)
+		return -EIO;
+
+	return result[0] & 0x01;
+}
+
+static inline int tsl256x_exec_readbyte(unsigned int reg, unsigned int *result)
+{
+	int arg = (reg << 0x10) | 0x800400;
+
+	if (sony_call_snc_handle(als_handle->handle, arg, result) < 0)
+		return -EIO;
+
+	if (!(*result & 0x01))
+		return -EINVAL;
+
+	*result = (*result >> 0x08) & 0xFF;
+
+	return 0;
+}
+
+static inline int tsl256x_exec_readword(unsigned int reg, unsigned int *result)
+{
+	int arg = (reg << 0x10) | 0xA00600;
+
+	if (sony_call_snc_handle(als_handle->handle, arg, result) < 0)
+		return -EIO;
+
+	if (!(*result & 0x01))
+		return -EINVAL;
+
+	*result = (*result >> 0x08) & 0xFFFF;
+
+	return 0;
+}
+
+static int tsl256x_interrupt_ctrls(unsigned int *interrupt,
+					unsigned int *periods)
+{
+	unsigned int value, result;
+
+	/* if no interrupt parameter, retrieve interrupt status */
+	if (!interrupt) {
+		if (tsl256x_exec_readbyte(TSL256X_REG_INT, &result))
+			return -EIO;
+
+		value = (result & TSL256X_INT_MASK);
+	} else {
+		value = *interrupt << 0x04;
+	}
+
+	/* if no periods provided use the last one set */
+	value |= (periods ? *periods : tsl256x_handle->periods);
+
+	if (tsl256x_exec_writebyte(TSL256X_REG_INT, &value))
+		return -EIO;
+
+	if (periods)
+		tsl256x_handle->periods = *periods;
+
+	return 0;
+}
+
+static int tsl256x_setup(void)
+{
+	unsigned int interr = 1, zero = 0;
+
+	/* reset the threshold settings to trigger an event as soon as the event
+	 * goes on, forcing a backlight adaptation to the current lighting
+	 * conditions
+	 */
+	tsl256x_exec_writeword(TSL256X_REG_TLOW, &zero);
+	tsl256x_exec_writeword(TSL256X_REG_THIGH, &zero);
+
+	/* set gain and time */
+	if (tsl256x_exec_writebyte(TSL256X_REG_TIMING,
+				&tsl256x_handle->gain->time))
+		return -EIO;
+
+	/* restore persistence value and enable the interrupt generation */
+	if (tsl256x_interrupt_ctrls(&interr, &tsl256x_handle->periods))
+		return -EIO;
+
+	return 0;
+}
+
+static int tsl256x_set_power(unsigned int status)
+{
+	int ret;
+
+	if (status) {
+		ret = tsl256x_setup();
+		if (ret)
+			return ret;
+	}
+
+	status = status ? TSL256X_POWER_ON : TSL256X_POWER_OFF;
+	ret = tsl256x_exec_writebyte(TSL256X_REG_CTRL, &status);
+
+	return ret;
+}
+
+static int tsl256x_get_raw_data(unsigned int *ch0, unsigned int *ch1)
+{
+	if (!ch0)
+		return -EINVAL;
+
+	if (tsl256x_exec_readword(TSL256X_REG_DATA0, ch0))
+		return -EIO;
+
+	if (ch1) {
+		if (tsl256x_exec_readword(TSL256X_REG_DATA1, ch1))
+			return -EIO;
+	}
+
+	return 0;
+}
+
+static int tsl256x_set_thresholds(const unsigned int *ch0)
+{
+	unsigned int tlow, thigh;
+
+	tlow = (*ch0 * tsl256x_handle->defaults[0]) / 100;
+	thigh = ((*ch0 * tsl256x_handle->defaults[1]) / 100) + 1;
+
+	if (thigh > 0xffff)
+		thigh = 0xffff;
+
+	if (tsl256x_exec_writeword(TSL256X_REG_TLOW, &tlow) ||
+		tsl256x_exec_writeword(TSL256X_REG_THIGH, &thigh))
+		return -EIO;
+
+	return 0;
+}
+
+#define MAX_LUX 700000
+static unsigned int tsl256x_calculate_lux(const u32 ch0, const u32 ch1)
+{
+	/* the raw output from the sensor is just a "count" value, as it is the
+	 * result of the integration of the analog sensor signal, the
+	 * counts-to-lux curve (and its approximation can be found on the
+	 * datasheet).
+	 */
+	const struct tsl256x_coeff *coeff = tsl256x_handle->coeff_table;
+	u32 ratio, temp, integer;
+
+	if (ch0 >= 65535 || ch1 >= 65535)
+		return MAX_LUX;
+
+	/* STEP 1: ratio calculation, for ch0 & ch1 coeff selection */
+
+	/* protect against division by 0 */
+	ratio = ch0 ? ((ch1 << (LUX_SHIFT_BITS + 1)) / ch0) : UINT_MAX - 1;
+	/* round the ratio value */
+	ratio = (ratio + 1) >> 1;
+
+	/* coeff selection rule */
+	while (coeff->ratio < ratio)
+		coeff++;
+
+	/* STEP 2: lux calculation formula using the right coeffcients */
+	temp = (ch0 * coeff->ch0) - (ch1 * coeff->ch1);
+	temp *= tsl256x_handle->gain->scale;
+	/* the sensor is placed under a plastic or glass cover which filters
+	 * a certain ammount of light (depending on that particular material).
+	 * To have an accurate reading, we need to compensate for this loss,
+	 * multiplying for compensation parameter, taken from the DSDT.
+	 */
+	temp *= tsl256x_handle->defaults[3] / 10;
+
+	/* round fractional portion to obtain the integer part */
+	integer = (temp + (1 << (LUX_SHIFT_BITS - 1))) >> LUX_SHIFT_BITS;
+
+	if (integer > MAX_LUX)
+		return MAX_LUX;
+
+	return integer;
+}
+
+static void tsl256x_calculate_kelvin(const u32 *ch0, const u32 *ch1,
+					unsigned int *temperature)
+{
+	const struct tsl256x_coeff *coeff = tsl256x_handle->coeff_table;
+	u32 ratio;
+
+	/* protect against division by 0 */
+	ratio = *ch0 ? ((*ch1 << (LUX_SHIFT_BITS + 1)) / *ch0) : UINT_MAX - 1;
+	/* round the ratio value */
+	ratio = (ratio + 1) >> 1;
+
+	/* coeff selection rule */
+	while (coeff->ratio < ratio)
+		coeff++;
+
+	*temperature = ratio ? coeff->ka / ratio + coeff->kb : 0;
+}
+
+static int tsl256x_get_lux(unsigned int *integ)
+{
+	int ret = 0;
+	unsigned int ch0, ch1;
+
+	if (!integ)
+		return -1;
+
+	ret = tsl256x_get_raw_data(&ch0, &ch1);
+	if (!ret)
+		*integ = tsl256x_calculate_lux(ch0, ch1);
+
+	return ret;
+}
+
+static int tsl256x_get_kelvin(unsigned int *temperature)
+{
+	int ret = -1;
+	unsigned int ch0, ch1;
+
+	if (!temperature)
+		return ret;
+
+	ret = tsl256x_get_raw_data(&ch0, &ch1);
+	if (!ret)
+		tsl256x_calculate_kelvin(&ch0, &ch1, temperature);
+
+	return ret;
+}
+
+static int tsl256x_get_id(void)
+{
+	int id, ret;
+	unsigned int result;
+	char *name = NULL;
+
+	ret = tsl256x_exec_readbyte(TSL256X_REG_ID, &result);
+	if (ret)
+		return ret;
+
+	id = (result >> 0x04) & 0x0F;
+	switch (id) {
+	case 11:
+		name = "TAOS TSL2569";
+		break;
+	case 5:
+		name = "TAOS TSL2561";
+		break;
+	case 3:
+		name = "TAOS TSL2563";
+		break;
+	case 0:
+		name = "TAOS TSL2560CS";
+		break;
+	default:
+		pr_warn("unsupported ALS model %u rev%u\n", id, result & 0x0F);
+		return -ENXIO;
+	}
+
+	pr_info("ALS model %u rev%u (%s)\n", id, result & 0x0F, name);
+	return id;
+}
+
+static int tsl256x_event_handler(void)
+{
+	unsigned int ch0, scale, interr = 1;
+
+	/* clear the notification */
+	sony_call_snc_handle(als_handle->handle, 0x04C60500, &interr);
+
+	/* read the raw data */
+	tsl256x_get_raw_data(&ch0, NULL);
+
+	scale = tsl256x_handle->gain->scale;
+	if (ch0 < tsl256x_handle->gain->min)
+		tsl256x_handle->gain--;
+	else if (ch0 > tsl256x_handle->gain->max)
+		tsl256x_handle->gain++;
+
+	if (tsl256x_handle->gain->scale != scale) {
+		ch0 *= scale;
+		ch0 /= tsl256x_handle->gain->scale;
+		tsl256x_exec_writebyte(TSL256X_REG_TIMING,
+				&tsl256x_handle->gain->time);
+	}
+
+	/* set the thresholds */
+	tsl256x_set_thresholds(&ch0);
+
+	/* enable interrupt */
+	tsl256x_interrupt_ctrls(&interr, NULL);
+
+	iio_push_event(als_handle->indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+					    0,
+					    IIO_EV_TYPE_THRESH,
+					    IIO_EV_DIR_EITHER),
+		       iio_get_time_ns());
+	return 0;
+}
+
+static int tsl256x_init(const u8 defaults[])
+{
+	int id = tsl256x_get_id();
+	if (id < 0)
+		return id;
+
+	tsl256x_handle = kzalloc(sizeof(struct tsl256x_data), GFP_KERNEL);
+	if (!tsl256x_handle)
+		return -ENOMEM;
+
+	/* populate the device data */
+	tsl256x_handle->defaults[0] = defaults[3];  /* low threshold % */
+	tsl256x_handle->defaults[1] = defaults[4];  /* high threshold % */
+	tsl256x_handle->defaults[2] = defaults[9];  /* sensor interrupt rate */
+	tsl256x_handle->defaults[3] = defaults[10]; /* light compensat. rate */
+	tsl256x_handle->gain = tsl256x_gain;
+	tsl256x_handle->periods = defaults[9];
+	tsl256x_handle->coeff_table = id == 0 ?
+		tsl256x_coeff_cs : tsl256x_coeff_fn;
+
+	return 0;
+}
+
+static int tsl256x_exit(void)
+{
+	unsigned int interr = 0, periods = tsl256x_handle->defaults[2];
+
+	/* disable the interrupt generation, restore defaults */
+	tsl256x_interrupt_ctrls(&interr, &periods);
+
+	tsl256x_handle->coeff_table = NULL;
+	kfree(tsl256x_handle);
+
+	return 0;
+}
+
+/* TAOS TSL256x specific ops */
+static const struct als_device_ops tsl256x_ops = {
+	.init = tsl256x_init,
+	.exit = tsl256x_exit,
+	.event_handler = tsl256x_event_handler,
+	.set_power = tsl256x_set_power,
+	.get_lux = tsl256x_get_lux,
+	.get_kelvin = tsl256x_get_kelvin,
+};
+
+/* unknown ALS sensors controlled by the EC present on newer Vaios */
+static int ngals_get_lux(unsigned int *integ)
+{
+	unsigned int data;
+
+	/* Event needs to be rearmed */
+	if (sony_call_snc_handle(als_handle->handle, 0x12200, &data))
+		return -EIO;
+	/* read raw data */
+	if (sony_call_snc_handle(als_handle->handle, 0x1000, &data))
+		return -EIO;
+
+	/* if we have a valid lux data */
+	if (!!(data & 0xff0000) == 0x01)
+		*integ = 0xffff & data;
+	else
+		return -1;
+
+	return 0;
+}
+
+static int ngals_event_handler(void)
+{
+	unsigned int unused;
+
+	iio_push_event(als_handle->indio_dev,
+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+					    0,
+					    IIO_EV_TYPE_THRESH,
+					    IIO_EV_DIR_EITHER),
+		       iio_get_time_ns());
+
+	/* Event needs to be rearmed */
+	if (sony_call_snc_handle(als_handle->handle, 0x12200, &unused))
+		return -EIO;
+	if (sony_call_snc_handle(als_handle->handle, 0x1000, &unused))
+		return -EIO;
+
+
+	return 0;
+}
+
+static const struct als_device_ops ngals_ops = {
+	.init = NULL,
+	.exit = NULL,
+	.event_handler = ngals_event_handler,
+	.set_power = NULL,
+	.get_lux = ngals_get_lux,
+	.get_kelvin = NULL,
+};
+
+static int __sony_nc_als_power_set(unsigned int status)
+{
+	unsigned int cmd, result;
+
+	if (als_handle->ops->set_power) {
+		if (als_handle->ops->set_power(status))
+			return -EIO;
+	}
+
+	/*  turn on/off the event notification */
+	cmd = als_handle->handle < 0x0143 ? 0x0901 : 0x2200;
+
+	if (sony_call_snc_handle(als_handle->handle,
+				(status << 0x10) | cmd, &result))
+		return -EIO;
+
+	if (sony_call_snc_handle(als_handle->handle, 0x1000, &result))
+		return -EIO;
+
+	return 0;
+}
+
+/* ALS sys interface */
+static ssize_t sony_nc_als_parameters_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	ssize_t count = 0;
+	unsigned int i, num;
+	u8 *list;
+
+	/* backlight_sensibility_table */
+	list = als_handle->levels;
+	num = als_handle->levels_num;
+
+	for (i = 0; i < num; i++)
+		count += snprintf(buffer + count, PAGE_SIZE - count,
+				"%u ", list[i]);
+
+	count += snprintf(buffer + count, PAGE_SIZE - count, "\n");
+
+	return count;
+}
+
+static ssize_t sony_nc_als_param_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	ssize_t count = 0;
+	unsigned int i;
+	int offset;
+
+	for (i = 0; als_handle->als_params[i].name; i++)
+		if (!strcmp(attr->attr.name, als_handle->als_params[i].name))
+			break;
+
+	/* first 2 elements of the array are 2 bytes values */
+	offset = als_handle->als_params[i].offset;
+	if (i < 2)
+		count = snprintf(buffer, PAGE_SIZE, "%u\n",
+				als_handle->defaults[offset]
+				+ 256 * als_handle->defaults[offset + 1]);
+	else if (als_handle->als_params[i].name)
+		count = snprintf(buffer, PAGE_SIZE, "%u\n",
+				als_handle->defaults[offset]);
+
+	return count;
+}
+
+static ssize_t sony_nc_als_kelvin_show(struct device *dev,
+		struct device_attribute *attr, char *buffer)
+{
+	ssize_t count = 0;
+	unsigned int kelvin = 0;
+
+	if (als_handle->ops->get_kelvin)
+		als_handle->ops->get_kelvin(&kelvin);
+
+	count = snprintf(buffer, PAGE_SIZE, "%d\n", kelvin);
+
+	return count;
+}
+
+
+/* ALS attach/detach functions */
+static void sony_nc_als_resume(void)
+{
+	__sony_nc_als_power_set(1);
+}
+
+static int sony_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan, int *val, int *val2,
+		long mask)
+{
+	struct als_device *als_handle = iio_priv(indio_dev);
+
+	if (als_handle->ops->get_lux(val))
+		return -EINVAL;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info sony_info = {
+	.attrs			= &als_attr_group,
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &sony_read_raw,
+};
+
+static const struct iio_event_spec sony_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER
+	}
+};
+
+static const struct iio_chan_spec sony_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.indexed = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.channel = 0,
+		.event_spec = sony_events,
+		.num_event_specs = ARRAY_SIZE(sony_events)
+	}
+};
+
+static int sony_nc_als_setup(struct acpi_device *dev, unsigned int handle)
+{
+	struct iio_dev *indio_dev;
+	int result = 0;
+	u64 arg;
+
+	if (!enable_als)
+		return 0;
+
+	indio_dev = devm_iio_device_alloc(&dev->dev, sizeof(struct als_device));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	als_handle = iio_priv(indio_dev);
+	als_handle->indio_dev = indio_dev;
+	als_handle->handle = handle;
+
+	/* model specific data */
+	switch (handle) {
+	case 0x0143:
+	case 0x014b:
+	case 0x014c:
+	case 0x0163:
+		/* check the device presence */
+		if (sony_call_snc_handle(handle, 0x100, &result)) {
+			result = -EIO;
+			goto nosensor;
+		}
+
+		if (!(result & 0x02))
+			/* no ALS controls */
+			goto nosensor;
+
+		als_handle->ops = &ngals_ops;
+		als_handle->levels_num = 16;
+		/* There're 101 additional values. What do we do with them? */
+		als_handle->als_params = ngals_params;
+		als_attr_group.attrs = ngals_attributes;
+		break;
+
+	case 0x0137:
+		/* check the device presence */
+		if (sony_call_snc_handle(handle, 0xB00, &result)) {
+			result = -EIO;
+			goto nosensor;
+		}
+
+		if (!(result & 0x01))
+			/* no ALS controls */
+			goto nosensor;
+		/* fall through */
+
+	case 0x012f:
+		als_handle->ops = &tsl256x_ops;
+		als_handle->levels_num = 9;
+		als_handle->als_params = als_params;
+		als_attr_group.attrs = als_attributes;
+		break;
+
+	default:
+		result = -EINVAL;
+		goto nosensor;
+	}
+
+
+	/* backlight levels are the first levels_num values, the remaining
+	 * values are default settings for als regulation
+	 */
+	als_handle->levels = als_handle->parameters;
+	als_handle->defaults = als_handle->parameters + als_handle->levels_num;
+
+	/* get ALS parameters */
+	arg = sony_find_snc_handle(handle);
+	if (sony_nc_buffer_call(sony_nc_acpi_handle, "SN06", &arg,
+		als_handle->parameters, ALS_TABLE_SIZE) < 0)
+		goto nosensor;
+
+	/* initial device configuration */
+	if (als_handle->ops->init) {
+		result = als_handle->ops->init(als_handle->defaults);
+		if (result)
+			goto nosensor;
+	}
+
+	indio_dev->dev.parent = &dev->dev;
+	indio_dev->channels = sony_channels;
+	indio_dev->num_channels = ARRAY_SIZE(sony_channels);
+	indio_dev->name = "sony-laptop";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &sony_info;
+
+	result = iio_device_register(indio_dev);
+	if (result < 0)
+		goto nosensor;
+
+	/* set power state */
+	if (__sony_nc_als_power_set(1))
+		pr_warn("unable to set the power status\n");
+
+	return 0;
+
+nosensor:
+	iio_device_free(indio_dev);
+	als_handle = NULL;
+
+	return result;
+}
+
+static void sony_nc_als_cleanup(struct platform_device *pd)
+{
+	if (als_handle) {
+		if (__sony_nc_als_power_set(0))
+			pr_info("ALS power off failed\n");
+
+		if (als_handle->ops->exit)
+			if (als_handle->ops->exit())
+				pr_info("ALS device clean-up failed\n");
+
+		iio_device_unregister(als_handle->indio_dev);
+		iio_device_free(als_handle->indio_dev);
+		als_handle = NULL;
+	}
+}
+/* end ALS code */
+
 /* Keyboard backlight feature */
 struct kbd_backlight {
 	unsigned int handle;
@@ -3058,6 +4116,7 @@ static void sony_nc_backlight_ng_read_limits(int handle,
 	case 0x143:
 	case 0x14b:
 	case 0x14c:
+	case 0x163:
 		lvl_table_len = 16;
 		break;
 	}
@@ -3086,11 +4145,30 @@ static void sony_nc_backlight_ng_read_limits(int handle,
 
 static void sony_nc_backlight_setup(void)
 {
+	acpi_handle unused;
 	int max_brightness = 0;
 	const struct backlight_ops *ops = NULL;
 	struct backlight_properties props;
+	int acpi_video_support = acpi_video_backlight_support();
+
+	/* backlight control behaves differently when ALS is enabled.
+	 * 1. if the acpi backlight device is not registered we lose brightness
+	 *    key press events (Fn+F[56])
+	 * 2. when the acpi backlight device is registered, changing brightness
+	 *    through that doesn't have any effect other than sending a
+	 *    notification to SNC: Notify (\_SB.PCI0.LPCB.SNC, 0x93)
+	 *    In this case we SNC is supposed to react and set the new
+	 *    brightness but the brightness value set via _BCM is stored outside
+	 *    SNC scope.
+	 * We allow registering our "sony" backlight device even if acpi is
+	 * preferred and we set it up to receive notifications upon which we act
+	 * scaling _BCM's values to SNC's.
+	 */
+	if (!als_handle && acpi_video_support) {
+		pr_info("brightness ignored, must be controlled by ACPI video driver\n");
+		return;
 
-	if (sony_find_snc_handle(0x12f) >= 0) {
+	} else if (sony_find_snc_handle(0x12f) >= 0) {
 		ops = &sony_backlight_ng_ops;
 		sony_bl_props.cmd_base = 0x0100;
 		sony_nc_backlight_ng_read_limits(0x12f, &sony_bl_props);
@@ -3120,7 +4198,15 @@ static void sony_nc_backlight_setup(void)
 		sony_nc_backlight_ng_read_limits(0x14c, &sony_bl_props);
 		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
 
-	} else if (acpi_has_method(sony_nc_acpi_handle, "GBRT")) {
+	} else if (sony_find_snc_handle(0x163) >= 0) {
+		ops = &sony_backlight_ng_ops;
+		sony_bl_props.cmd_base = 0x3000;
+		sony_nc_backlight_ng_read_limits(0x163, &sony_bl_props);
+		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
+
+	/* legacy */
+	} else if (ACPI_SUCCESS(acpi_get_handle(sony_nc_acpi_handle, "GBRT",
+						&unused))) {
 		ops = &sony_backlight_ops;
 		max_brightness = SONY_MAX_BRIGHTNESS - 1;
 
@@ -3205,12 +4291,7 @@ static int sony_nc_add(struct acpi_device *device)
 			sony_nc_function_setup(device, sony_pf_device);
 	}
 
-	/* setup input devices and helper fifo */
-	if (acpi_video_backlight_support()) {
-		pr_info("brightness ignored, must be controlled by ACPI video driver\n");
-	} else {
-		sony_nc_backlight_setup();
-	}
+	sony_nc_backlight_setup();
 
 	/* create sony_pf sysfs attributes related to the SNC device */
 	for (item = sony_nc_values; item->name; ++item) {
-- 
1.9.0

[PATCH 11/11] sony-laptop: remove useless sony-laptop versioning

[Mattia Dongili]

Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index b0a311f..6612c4a 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -79,8 +79,6 @@ do {						\
 		pr_warn(fmt, ##__VA_ARGS__);	\
 } while (0)
 
-#define SONY_LAPTOP_DRIVER_VERSION	"0.6"
-
 #define SONY_NC_CLASS		"sony-nc"
 #define SONY_NC_HID		"SNY5001"
 #define SONY_NC_DRIVER_NAME	"Sony Notebook Control Driver"
@@ -92,7 +90,6 @@ do {						\
 MODULE_AUTHOR("Stelian Pop, Mattia Dongili");
 MODULE_DESCRIPTION("Sony laptop extras driver (SPIC and SNC ACPI device)");
 MODULE_LICENSE("GPL");
-MODULE_VERSION(SONY_LAPTOP_DRIVER_VERSION);
 
 static int debug;
 module_param(debug, int, 0);
@@ -4240,8 +4237,6 @@ static int sony_nc_add(struct acpi_device *device)
 	int result = 0;
 	struct sony_nc_value *item;
 
-	pr_info("%s v%s\n", SONY_NC_DRIVER_NAME, SONY_LAPTOP_DRIVER_VERSION);
-
 	sony_nc_acpi_device = device;
 	strcpy(acpi_device_class(device), "sony/hotkey");
 
@@ -4330,6 +4325,7 @@ static int sony_nc_add(struct acpi_device *device)
 		}
 	}
 
+	pr_info("SNC setup done.\n");
 	return 0;
 
 out_sysfs:
@@ -5768,8 +5764,6 @@ static int sony_pic_add(struct acpi_device *device)
 	struct sony_pic_ioport *io, *tmp_io;
 	struct sony_pic_irq *irq, *tmp_irq;
 
-	pr_info("%s v%s\n", SONY_PIC_DRIVER_NAME, SONY_LAPTOP_DRIVER_VERSION);
-
 	spic_dev.acpi_dev = device;
 	strcpy(acpi_device_class(device), "sony/hotkey");
 	sony_pic_detect_device_type(&spic_dev);
@@ -5869,6 +5863,7 @@ static int sony_pic_add(struct acpi_device *device)
 	if (result)
 		goto err_remove_pf;
 
+	pr_info("SPIC setup done.\n");
 	return 0;
 
 err_remove_pf:
-- 
1.9.0

Re: [PATCH 10/11] sony-laptop: als support

[Jonathan Cameron]

On 20/03/14 23:01, Mattia Dongili wrote:
> From: Javier Achirica <jachirica@gmail.com>
>
> vaios come with two different type of ACPI based ALS controls. The
> oldest incarnation exposes and requires a much lower level control over
> the ALS device (tsl256x) and raw data, the most recent one is much
> simpler and only allows fetching lux readings.
> Additionally, the DSDT exposes data tables that can be used apply
> corrections and appropriate scaling to the sensor's readings.
> The device is registered via the IIO subsystem and the tables exposed as
> attributes of the IIO device.
>
> As a bonus, enabling the sensor makes backlight Fn keys behave
> differently than default. Instead of changing backlight, _BCM will
> generate events to the SNC device (with no indication of the new
> backlight value) expecting that to do the legwork instead.
> To make this work in Linux we force registering platform backlight
> control and use the backlight subsystem to get notified about changes
> and applying the change using SNC specific methods.
>
> The device is enabled by default but allows disabling passing
> enable_als=0 as a module parameter.
>
> Mattia's changes:
> * remove unused event read functions
> * rework backlight device registration precedence
> * add callback for brightness change notifications
> * re-arm ALS events after the event
> * Kconfig: select IIO
> * remove unused function and redundant allocation
> * reformat code
> * add module parameter to disable ALS
> * do not force backlight device to be created when no ALS is setup.
> * remove redundant tsl driver setup calls
> * change calculate_lux function to return a value
> * simplify tsl256x_get_id function
>
> Marco Chiappero worked on early revisions of this code, see
> http://www.absence.it/vaio-acpi/
>
Hi,

I hate to say it but my first reaction to this was, 'What are so
many functions doing in one file in the first place?'

The embedded world has been busy splitting up and reparcelling large
files like this so the various devices end up within the correct
subsystems. I won't even go into all the reasons for this as they
are listed everywhere else?  Is this just a case of modifying old
code no one wants to touch on a larger scale or is there actually
resistance to breaking new functionality out of this file and
into nice clean small chunks?
   
So my first request is can we at least split this new functionality
out?

As I look through what we have here, it looks like we ought to have
this functionality alone split into the ALS sensor and a backlight
driver (which can by all means use the standard interfaces to
talk to the ALS)... (event support for in kernel users isn't there
yet, but this might motivate us to hurry up with it).

The bulk of this driver is actually just a set of nasty access routines
and then functionality that already exists in the existing tsl2563 driver.
Hence, unless there are very good reasons why not I would suggest the
following.

1) Convert the existing tsl2563 driver over to using regmap.
2) Add regmap support for the access routines we have here then add the
    little bit of code needed to make the tsl2563 register appopriately.  This
    may require a small amount of magic mfd like code in here.
(plan b on this is to perhaps create an i2c bus driver using the sony
specific calls).
3) Figure out what extra bits are that are needed by the ALS driver we have
    here and add them on (kelvin support and perhaps a few other bits).
4) Then and only then take on any non ALS elements in here (such as the
    back light).

The other als driver (one where all the clever stuff is hidden) should ideally
be a completely separate als driver (be it a very simple one.

Anyhow, your code is reasonably clean but I think some of these questions about
the fundamental approach need addressing to avoid getting ourselves into a
nasty mess in the long term.   Sorry if this review seems a little negative.
I appreciate that you've only taken the approach taken throughout this file!

Just as an aside, is there any interest in cleaning this whole file up?
The sonypi device looks like it should registering a whole host of standard
interfaces...

Jonathan
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Marco Chiappero <marco@absence.it>
> Signed-off-by: Javier Achirica <jachirica@gmail.com>
> Signed-off-by: Mattia Dongili <malattia@linux.it>
> ---
>   Documentation/laptops/sony-laptop.txt |   31 +
>   drivers/platform/x86/Kconfig          |    1 +
>   drivers/platform/x86/sony-laptop.c    | 1111 ++++++++++++++++++++++++++++++++-
>   3 files changed, 1128 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/laptops/sony-laptop.txt b/Documentation/laptops/sony-laptop.txt
> index 978b1e6..014ebdf 100644
> --- a/Documentation/laptops/sony-laptop.txt
> +++ b/Documentation/laptops/sony-laptop.txt
> @@ -79,6 +79,37 @@ such a laptop you will find the necessary rfkill devices under
>   /sys/class/rfkill. Check those starting with sony-* in
>   	# grep . /sys/class/rfkill/*/{state,name}
>
> +Ambient Light Sensor:
> +---------------------
> +On models equipped with ALS, sony-laptop will create an IIO device available
> +under:
> +	/sys/bus/iio/devices/
> +In addition to the sensor's readings, a number of other attributes are exposed
> +for userspace to consume:
> +	typical_illuminance	Typical illuminance level for brightness
> +				control in lux.
Is this a target value?  Need a better description for this as it's not immediately
obvious what it is.

> +	backlight_illuminance	Threshold in lux for keyboard backlight.
> +	lux_correction_percent	Correction factor to apply to brightness in
> +				percentage.
If applied to the measurement before exposed to userspace then it should
be handled using the iio info_mask element calibscale. If it's a value that
should be applied in userspace then the scale infomask element.
  
> +	low_threshold_percent	Percentage from current lux level to trigger
> +				event (low).
Interesting.  These are a new one :)
Anyhow the should be handled through additions to iio's iio_event_info
enum and all the stuff that goes with that.
Perhaps
IIO_EV_INFO_PERCENT, with sysfs attributes
events/in_illuminance_thresh_rising_percent
events/in_illuminance_thresh_falling_percent

Actually you don't seem to have any IIO_EV_INFO stuff in here yet you
are still pushing out events via IIO. Interesting...

> +	high_threshold_percent	Percentage from current lux level to trigger
> +				event (high).
> +	als_event_rate		Time in milliseconds between brightness changes
> +				when triggered by light change.
The naming is not nearly descriptive enough as I'd read that as being how
often an event (e.g. it going dark) is checked from the als.

> +	als_event_delay		Percentage of brightness changes when triggered
> +				by light change.
> +	key_event_rate		Time in milliseconds between brightness changes
> +				when triggered by keypress.
> +	key_event_delay		Percentage of brightness changes when triggered
> +				by keypress.
These are so device dependent we'll probably just let them go.  Might be
something that's worth checking with the input guys to discover if they
have an interface already defined for this sort of thing.

> +	interrupt_rate		Light sensor interrupt rate in milliseconds.
> +	light_compensation_x10	Light compensation needed due to sensor cover.
Offset, or a scale factor?  Either way there are well defined abi elements for
this already.

> +	minimum_illuminance	Minimum expected illuminance level in lux.
Expected in what sense?
> +	maximum_illuminance	Maximum expected illuminance level in lux.
> +	algorithm		Brightness calculation algorithm to be used.
That's a lot of new abi as far as IIO is concerned.  It will all need documenting in
Documentation/ABI/testing/sysfs-bus-iio-sony-laptop or the generic files if appropriate.
Also, no units for some of the above.

> +Depending on the sensor and laptop model, some of these attributes may not be
> +available.
>
>   Development:
>   ------------
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 5ae65c1..761086f 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -279,6 +279,7 @@ config SONY_LAPTOP
>   	select BACKLIGHT_CLASS_DEVICE
>   	depends on INPUT
>   	depends on RFKILL
> +	select IIO
>   	  ---help---
>   	  This mini-driver drives the SNC and SPIC devices present in the ACPI
>   	  BIOS of the Sony Vaio laptops.
> diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
> index 19d769e..b0a311f 100644
> --- a/drivers/platform/x86/sony-laptop.c
> +++ b/drivers/platform/x86/sony-laptop.c
> @@ -61,6 +61,9 @@
>   #include <linux/workqueue.h>
>   #include <linux/acpi.h>
>   #include <linux/slab.h>
> +#include <acpi/video.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
>   #include <linux/sonypi.h>
>   #include <linux/sony-laptop.h>
>   #include <linux/rfkill.h>
> @@ -138,6 +141,12 @@ MODULE_PARM_DESC(kbd_backlight_timeout,
>   		 "meaningful values vary from 0 to 3 and their meaning depends "
>   		 "on the model (default: no change from current value)");
>
> +static int enable_als = 1;
> +module_param(enable_als, int, 0444);
> +MODULE_PARM_DESC(enable_als,
> +		 "set this to 0 to disable the SNC controlled ALS sensor "
> +		 "(default: 1)");
> +
>   #ifdef CONFIG_PM_SLEEP
>   static void sony_nc_thermal_resume(void);
>   #endif
> @@ -200,6 +209,138 @@ static int sony_nc_rfkill_setup(struct acpi_device *device,
>   static void sony_nc_rfkill_cleanup(void);
>   static void sony_nc_rfkill_update(void);
>
> +#define ALS_TABLE_SIZE	126
> +
> +struct als_device_ops {
> +	int (*init)(const u8 defaults[]);
> +	int (*exit)(void);
> +	int (*event_handler)(void);
> +	int (*set_power)(unsigned int);
> +	int (*get_lux)(unsigned int *);
> +	int (*get_kelvin)(unsigned int *);
> +};
> +
> +struct als_device {
> +	int handle;
> +
> +	unsigned int levels_num;
> +	u8 *levels;
> +	u8 *defaults;
> +	u8 parameters[ALS_TABLE_SIZE];
> +	struct als_param_list *als_params;
> +
> +	/* common device operations */
> +	const struct als_device_ops *ops;
> +
> +	/* IIO device */
> +	struct iio_dev *indio_dev;
> +};
> +static struct als_device *als_handle;
> +
> +struct als_param_list {
> +	int offset;
> +	char *name;
> +};
> +
const?
> +static struct als_param_list als_params[] = {
> +	{ 1, "typical_illuminance" },
> +	{ 11, "backlight_illuminance" },
> +	{ 0, "lux_correction_percent" },
> +	{ 3, "low_threshold_percent" },
> +	{ 4, "high_threshold_percent" },
> +	{ 5, "als_event_rate" },
> +	{ 6, "als_event_delay" },
> +	{ 7, "key_event_rate" },
> +	{ 8, "key_event_delay" },
> +	{ 9, "interrupt_rate" },
> +	{ 10, "light_compensation_x10" },
> +	{ 0, NULL }
> +};
> +
const?
> +static struct als_param_list ngals_params[] = {
> +	/* Should these two should go to a "range" file in IIO? */
An excellent question.   Ranges aren't currently well defined.
There is a patch set I posted a while ago introducing _available
type sysfs attributes which does include the ability to specify
the limits on any iio info_mask element (including *_input)
which would do the job.  That series has gotten somewhat stalled
by me being busy though... Hence I'm not totally against this
but it would be in_illuminance_maximum to fit with the normal
ABI form.  We'd just leave this in place once that new support
is there as well. Doesn't do any realy harm.
> +	{ 4, "minimum_illuminance" },
> +	{ 6, "maximum_illuminance" },
> +	{ 0, "als_event_rate" },
> +	{ 1, "als_event_delay" },
> +	{ 2, "key_event_rate" },
> +	{ 3, "key_event_delay" },
> +	{ 8, "algorithm" },
> +	{ 0, NULL }
> +};
> +
Why not just reorder the code so these show functions are
before they are used?  Looks straight forward and much neater.
I guess this is part of this being in a monolithic file.

The more I read, the more I think this definitely needs breaking
up.  Taking this driver outside of it is just the start.
> +static ssize_t sony_nc_als_parameters_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer);
> +static ssize_t sony_nc_als_param_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer);
> +
This one isn't documented above.  Every attr MUST be documented.
It needs to be in the Documentation/ABI/testing directory, but
having it above would have been a good start ;)
> +static struct device_attribute als_attr_backlight_sensibility_table =
> +	__ATTR(backlight_sensibility_table, 0444,
> +		sony_nc_als_parameters_show, 0);
> +
> +static ssize_t sony_nc_als_kelvin_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer);
Are we talking actual temperature or colour temperature here?
Either way, I missed the documentation...
> +
> +static struct device_attribute als_attr_kelvin =
> +	__ATTR(kelvin, 0444, sony_nc_als_kelvin_show, 0);
> +
> +#define ALS_ATTR(_name, _mode)					\
> +struct device_attribute als_attr_##_name = __ATTR(_name,	\
> +		_mode, sony_nc_als_param_show, 0)
> +
> +static ALS_ATTR(typical_illuminance, 0444);
> +static ALS_ATTR(backlight_illuminance, 0444);
> +static ALS_ATTR(lux_correction_percent, 0444);
> +static ALS_ATTR(low_threshold_percent, 0444);
> +static ALS_ATTR(high_threshold_percent, 0444);
> +static ALS_ATTR(als_event_rate, 0444);
> +static ALS_ATTR(als_event_delay, 0444);
> +static ALS_ATTR(key_event_rate, 0444);
> +static ALS_ATTR(key_event_delay, 0444);
> +static ALS_ATTR(interrupt_rate, 0444);
> +static ALS_ATTR(light_compensation_x10, 0444);
> +
> +static ALS_ATTR(minimum_illuminance, 0444);
> +static ALS_ATTR(maximum_illuminance, 0444);
> +static ALS_ATTR(algorithm, 0444);
> +
> +static struct attribute *als_attributes[] = {
> +	&als_attr_backlight_sensibility_table.attr,
> +	&als_attr_typical_illuminance.attr,
> +	&als_attr_backlight_illuminance.attr,
> +	&als_attr_lux_correction_percent.attr,
> +	&als_attr_low_threshold_percent.attr,
> +	&als_attr_high_threshold_percent.attr,
> +	&als_attr_als_event_rate.attr,
> +	&als_attr_als_event_delay.attr,
> +	&als_attr_key_event_rate.attr,
> +	&als_attr_key_event_delay.attr,
> +	&als_attr_interrupt_rate.attr,
> +	&als_attr_light_compensation_x10.attr,
> +	&als_attr_kelvin.attr,
> +	NULL
> +};
> +
> +static struct attribute *ngals_attributes[] = {
> +	&als_attr_backlight_sensibility_table.attr,
> +	&als_attr_minimum_illuminance.attr,
> +	&als_attr_maximum_illuminance.attr,
> +	&als_attr_als_event_rate.attr,
> +	&als_attr_als_event_delay.attr,
> +	&als_attr_key_event_rate.attr,
> +	&als_attr_key_event_delay.attr,
> +	&als_attr_algorithm.attr,
So is algorithm going to give me a magic number?  Please convert this
to something human readable if so.
> +	NULL
> +};
> +
> +struct attribute_group als_attr_group = {
> +	.name = "als_parameters"
> +};
> +
> +static int sony_nc_als_setup(struct acpi_device *dev, unsigned int handle);
> +static void sony_nc_als_cleanup(struct platform_device *pd);
> +static void sony_nc_als_resume(void);
> +
>   /*********** Input Devices ***********/
>
>   #define SONY_LAPTOP_BUF_SIZE	128
> @@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct backlight_device *bd)
>   	return (result & 0xff) - sdev->offset;
>   }
>
> -static int sony_nc_update_status_ng(struct backlight_device *bd)
> +static int __sony_nc_set_brightness_ng(struct sony_backlight_props *bl,
> +		int brightness)
>   {
> -	int value, result;
> -	struct sony_backlight_props *sdev =
> -		(struct sony_backlight_props *)bl_get_data(bd);
> +	int result = 0;
> +	int value = brightness + bl->offset;
>
> -	value = bd->props.brightness + sdev->offset;
> -	if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value << 0x10),
> +	if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value << 0x10),
>   				&result))
>   		return -EIO;
>
>   	return value;
>   }
Whilst this stuff is presumably being driver from the ALS it is decidedly
back light specific. Perhaps I'm idealistic in thinking it should be
a different driver.
>
> +
> +static int sony_nc_update_status_ng(struct backlight_device *bd)
> +{
> +	struct sony_backlight_props *sdev =
> +		(struct sony_backlight_props *)bl_get_data(bd);
> +
> +	return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
> +}
> +
> +static void sony_nc_brightness_changed_ng(struct backlight_device *bd,
> +		int brightness, int max_brightness)
> +{
> +	int new_brightness =
> +		brightness * bd->props.max_brightness / max_brightness;
> +	struct sony_backlight_props *sdev =
> +		(struct sony_backlight_props *)bl_get_data(bd);
> +
> +	dprintk("brightness changed cur: %d, max: %d -> new: %d\n", brightness,
> +			max_brightness, new_brightness);
> +
> +	__sony_nc_set_brightness_ng(sdev, new_brightness);
> +	backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
> +}
> +
>   static const struct backlight_ops sony_backlight_ops = {
>   	.options = BL_CORE_SUSPENDRESUME,
>   	.update_status = sony_backlight_update_status,
> @@ -1088,6 +1252,7 @@ static const struct backlight_ops sony_backlight_ng_ops = {
>   	.options = BL_CORE_SUSPENDRESUME,
>   	.update_status = sony_nc_update_status_ng,
>   	.get_brightness = sony_nc_get_brightness_ng,
> +	.brightness_changed = &sony_nc_brightness_changed_ng,
>   };
>
>   /*
> @@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event, unsigned int handle)
>   enum event_types {
>   	HOTKEY = 1,
>   	KILLSWITCH,
> -	GFX_SWITCH
> +	GFX_SWITCH,
> +	ALS
>   };
Hmm. I don't suppose suggesting this should be an mfd with a nice clean
event hook up interface will go down to well?
>   static void sony_nc_notify(struct acpi_device *device, u32 event)
>   {
> @@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
>   			ev_type = GFX_SWITCH;
>   			real_ev = __sony_nc_gfx_switch_status_get();
>   			break;
> +
> +		case 0x0143:
> +		case 0x014b:
> +		case 0x014c:
> +		case 0x0163:
Could you document what these different events are here?  Would make
it easier to review.
> +			sony_call_snc_handle(handle, 0x2000, &result);
> +			/* event reasons are inverted ? */
> +			real_ev = (result & 0x03) == 1 ? 2 : 1;
> +			dprintk("ALS event received (%s change): 0x%x\n",
> +					real_ev == 1 ?
> +					"light" : "backlight", result);
> +			if (real_ev == 1 &&
> +					als_handle->ops->event_handler)
> +				als_handle->ops->event_handler();
> +
> +			ev_type = ALS;
> +			break;
> +
> +		case 0x012f:
> +		case 0x0137:
> +			sony_call_snc_handle(handle, 0x0800, &result);
> +			real_ev = result & 0x03;
> +			dprintk("ALS event received (%s change): 0x%x\n",
> +					real_ev == 1 ?
> +					"light" : "backlight", result);
> +			if (real_ev == 1 &&
> +					als_handle->ops->event_handler)
> +				als_handle->ops->event_handler();
> +
> +			ev_type = ALS;
> +			break;
> +
>   		default:
>   			dprintk("Unknown event 0x%x for handle 0x%x\n",
>   					event, handle);
> @@ -1378,6 +1576,11 @@ static void sony_nc_function_setup(struct acpi_device *device,
>   				pr_err("couldn't set up GFX Switch status (%d)\n",
>   						result);
>   			break;
> +		case 0x012f:
> +			result = sony_nc_als_setup(device, handle);
> +			if (result)
> +				pr_err("couldn't set up ALS (%d)\n", result);
> +			break;
>   		case 0x0131:
>   			result = sony_nc_highspeed_charging_setup(pf_device);
>   			if (result)
> @@ -1400,6 +1603,10 @@ static void sony_nc_function_setup(struct acpi_device *device,
>   			if (result)
>   				pr_err("couldn't set up keyboard backlight function (%d)\n",
>   						result);
> +			result = sony_nc_als_setup(device, handle);
> +			if (result)
> +				pr_err("couldn't set up ALS (%d)\n", result);
> +
>   			break;
>   		case 0x0121:
>   			result = sony_nc_lowbatt_setup(pf_device);
> @@ -1481,6 +1688,9 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
>   		case 0x015B:
>   			sony_nc_gfx_switch_cleanup(pd);
>   			break;
> +		case 0x012f:
> +			sony_nc_als_cleanup(pd);
> +			break;
>   		case 0x0131:
>   			sony_nc_highspeed_charging_cleanup(pd);
>   			break;
> @@ -1494,6 +1704,7 @@ static void sony_nc_function_cleanup(struct platform_device *pd)
>   		case 0x014c:
>   		case 0x0163:
>   			sony_nc_kbd_backlight_cleanup(pd, handle);
> +			sony_nc_als_cleanup(pd);
>   			break;
>   		case 0x0121:
>   			sony_nc_lowbatt_cleanup(pd);
> @@ -1550,6 +1761,14 @@ static void sony_nc_function_resume(void)
>   		case 0x0135:
>   			sony_nc_rfkill_update();
>   			break;
> +		case 0x012f:
> +		case 0x0137:
> +		case 0x0143:
> +		case 0x014b:
> +		case 0x014c:
> +		case 0x0163:
> +			sony_nc_als_resume();
> +			break;
>   		default:
>   			continue;
>   		}
> @@ -1769,6 +1988,845 @@ static int sony_nc_rfkill_setup(struct acpi_device *device,
>   	return 0;
>   }
>
> +/* ALS */
> +
coding style please :)
/*
  * model
> +/* model specific ALS data and controls
> + * TAOS TSL256x device data
> + */
> +#define LUX_SHIFT_BITS		16	/* for non-floating point math */
> +/* scale 100000 multiplied fractional coefficients rounding the values */
> +#define SCALE(u)	((((((u64) u) << LUX_SHIFT_BITS) / 10000) + 5) / 10)
> +
These are the same register defines as in the tsl2563 driver. Please factor
them out of there into a header then use it. We don't want this repeated in
two different drivers.

> +#define TSL256X_REG_CTRL	0x00
> +#define TSL256X_REG_TIMING	0x01
> +#define TSL256X_REG_TLOW	0x02
> +#define TSL256X_REG_THIGH	0x04
> +#define TSL256X_REG_INT		0x06
> +#define TSL256X_REG_ID		0x0a
> +#define TSL256X_REG_DATA0	0x0c
> +#define TSL256X_REG_DATA1	0x0e
> +
> +#define TSL256X_POWER_ON	0x03
> +#define TSL256X_POWER_OFF	0x00
> +
> +#define TSL256X_POWER_MASK	0x03
> +#define TSL256X_INT_MASK	0x10
> +
> +struct tsl256x_coeff {
> +	u32 ratio;
> +	u32 ch0;
> +	u32 ch1;
> +	u32 ka;
> +	s32 kb;
> +};
> +
> +struct tsl256x_gain_coeff {
> +	u32 time;
> +	u16 min;
> +	u16 max;
> +	u16 scale;
> +};
> +
very similar stuff to this is in the tsl2563 driver.
Any chance of sharing code?
> +struct tsl256x_gain_coeff tsl256x_gain[] = {
> +	{
> +		.time   = 0x12, /* 402 ms, gain x16 */
> +		.min    = 0,
> +		.max    = 65534,
> +		.scale  = 1,
> +	}, {
> +		.time   = 0x02, /* 402 ms, gain x1 */
> +		.min    = 2048,
> +		.max    = 65534,
> +		.scale  = 16,
> +	}, {
> +		.time   = 0x01, /* 101 ms, gain x1 */
> +		.min    = 4095,
> +		.max    = 37176,
> +		.scale  = 64,
> +	}, {
> +		.time   = 0x00, /* 13.7 ms, gain x1 */
> +		.min    = 3000,
> +		.max    = 65535,
> +		.scale  = 468,
> +	},
> +};
> +
> +struct tsl256x_data {
> +	unsigned int periods;
> +	struct tsl256x_gain_coeff const *gain;
> +	u8 defaults[4];
> +	struct tsl256x_coeff const *coeff_table;
> +};
> +static struct tsl256x_data *tsl256x_handle;
I'm against limiting a driver to a single instance on principle
but I guess it doesn't matter here.
> +
> +static const struct tsl256x_coeff tsl256x_coeff_fn[] = {
> +	{
> +		.ratio	= SCALE(12500),	/* 0.125 * 2^LUX_SHIFT_BITS  */
> +		.ch0	= SCALE(3040),	/* 0.0304 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(2720),	/* 0.0272 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(313550000),
> +		.kb	= -10651,
> +	}, {
> +		.ratio	= SCALE(25000),	/* 0.250 * 2^LUX_SHIFT_BITS  */
> +		.ch0	= SCALE(3250),	/* 0.0325 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(4400),	/* 0.0440 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(203390000),
> +		.kb	= -2341,
> +	}, {
> +		.ratio	= SCALE(37500),	/* 0.375 * 2^LUX_SHIFT_BITS  */
> +		.ch0	= SCALE(3510),	/* 0.0351 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(5440),	/* 0.0544 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(152180000),
> +		.kb	= 157,
> +	}, {
> +		.ratio	= SCALE(50000),	/* 0.50 * 2^LUX_SHIFT_BITS   */
> +		.ch0	= SCALE(3810),	/* 0.0381 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(6240),	/* 0.0624 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(163580000),
> +		.kb	= -145,
> +	}, {
> +		.ratio	= SCALE(61000),	/* 0.61 * 2^LUX_SHIFT_BITS   */
> +		.ch0	= SCALE(2240),	/* 0.0224 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(3100),	/* 0.0310 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(180800000),
> +		.kb	= -495,
> +	}, {
> +		.ratio	= SCALE(80000),	/* 0.80 * 2^LUX_SHIFT_BITS   */
> +		.ch0	= SCALE(1280),	/* 0.0128 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(1530),	/* 0.0153 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(197340000),
> +		.kb	= -765
> +	}, {
> +		.ratio	= SCALE(130000),/* 1.3 * 2^LUX_SHIFT_BITS     */
> +		.ch0	= SCALE(146),	/* 0.00146 * 2^LUX_SHIFT_BITS */
> +		.ch1	= SCALE(112),	/* 0.00112 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(182900000),
> +		.kb	= -608,
> +	}, {
> +		.ratio	= UINT_MAX,	/* for higher ratios */
> +		.ch0	= 0,
> +		.ch1	= 0,
> +		.ka	= 0,
> +		.kb	= 830,
> +	}
> +};
>
So what are _fn and _cs?
+
> +static const struct tsl256x_coeff tsl256x_coeff_cs[] = {
> +	{
> +		.ratio  = SCALE(13000),	/* 0.130 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(3150),	/* 0.0315 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(2620),	/* 0.0262 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(300370000),
> +		.kb	= -9587,
> +	}, {
> +		.ratio  = SCALE(26000),	/* 0.260 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(3370),	/* 0.0337 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(4300),	/* 0.0430 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(194270000),
> +		.kb	= -1824,
> +	}, {
> +		.ratio  = SCALE(39000),	/* 0.390 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(3630),	/* 0.0363 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(5290),	/* 0.0529 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(152520000),
> +		.kb	= 145,
> +	}, {
> +		.ratio  = SCALE(52000),	/* 0.520 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(3920),	/* 0.0392 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(6050),	/* 0.0605 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(165960000),
> +		.kb	= -200,
> +	}, {
> +		.ratio  = SCALE(65000),	/* 0.650 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(2290),	/* 0.0229 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(2910),	/* 0.0291 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(184800000),
> +		.kb	= -566,
> +	}, {
> +		.ratio  = SCALE(80000),	/* 0.800 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(1570),	/* 0.0157 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(1800),	/* 0.0180 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(199220000),
> +		.kb	= -791,
> +	}, {
> +		.ratio  = SCALE(130000),/* 0.130 * 2^LUX_SHIFT_BITS  */
> +		.ch0    = SCALE(338),	/* 0.00338 * 2^LUX_SHIFT_BITS */
> +		.ch1    = SCALE(260),	/* 0.00260 * 2^LUX_SHIFT_BITS */
> +		.ka	= SCALE(182900000),
> +		.kb	= -608,
> +	}, {
> +		.ratio  = UINT_MAX,	/* for higher ratios */
> +		.ch0    = 0,
> +		.ch1    = 0,
> +		.ka	= 0,
> +		.kb	= 830,
> +	}
> +};
> +
Hmm. So in this code you are talking directly the underling taos chip?
Interesting.  I wonder if it would be possible with the insertion
of a few callbacks to utilize som of the tsl2563 code directly..

> +/* TAOS helper & control functions */
> +static inline int tsl256x_exec_writebyte(unsigned int reg,
> +						unsigned int const *value)
Why _exec_  Also these are totaly sony specific as far as I can see.
Hence call it.
sony_tsl256x_write_byte or similar.
> +{
> +	unsigned int result;
> +
> +	return (sony_call_snc_handle(als_handle->handle, (*value << 0x18) |
> +		(reg << 0x10) | 0x800500, &result) || !(result & 0x01))
> +		? -EIO : 0;
> +}
> +
Why pass the value as a pointer?  If you are writing it that makes no real
sense.  Just pass a u16. Same for the writebyte above. Will get rid
of quite a few unnecessary lines of code below.
> +static inline int tsl256x_exec_writeword(unsigned int reg,
> +						unsigned int const *value)
> +{
> +	u8 result[1];
> +	int offset = sony_find_snc_handle(als_handle->handle);
> +	u64 arg = (*value << 0x18) | (reg << 0x10) | 0xA00700 | offset;
> +
> +	/* using sony_call_snc_handle_buffer due to possible input overflows */
> +	if (sony_nc_buffer_call(sony_nc_acpi_handle, "SN06", &arg,
> +				result, 1) < 0)
> +		return -EIO;
> +
> +	return result[0] & 0x01;
> +}
> +
> +static inline int tsl256x_exec_readbyte(unsigned int reg, unsigned int *result)
> +{
> +	int arg = (reg << 0x10) | 0x800400;
> +
> +	if (sony_call_snc_handle(als_handle->handle, arg, result) < 0)
> +		return -EIO;
> +
> +	if (!(*result & 0x01))
> +		return -EINVAL;
> +
> +	*result = (*result >> 0x08) & 0xFF;
> +
> +	return 0;
> +}
> +
As you are reading a word, don't use a 32bit + integer.
u16 *result

Then you can avoid the masking below as well.
> +static inline int tsl256x_exec_readword(unsigned int reg, unsigned int *result)
> +{
> +	int arg = (reg << 0x10) | 0xA00600;
> +
> +	if (sony_call_snc_handle(als_handle->handle, arg, result) < 0)
> +		return -EIO;
> +
> +	if (!(*result & 0x01))
> +		return -EINVAL;
> +
> +	*result = (*result >> 0x08) & 0xFFFF;
> +
> +	return 0;
> +}
> +
> +static int tsl256x_interrupt_ctrls(unsigned int *interrupt,
> +					unsigned int *periods)
> +{
> +	unsigned int value, result;
> +
> +	/* if no interrupt parameter, retrieve interrupt status */
> +	if (!interrupt) {
> +		if (tsl256x_exec_readbyte(TSL256X_REG_INT, &result))
> +			return -EIO;
> +
> +		value = (result & TSL256X_INT_MASK);
> +	} else {
> +		value = *interrupt << 0x04;
> +	}
> +
> +	/* if no periods provided use the last one set */
If none provided, do you need to set it at all? If you do then the comment
should make this clear.
> +	value |= (periods ? *periods : tsl256x_handle->periods);
> +
> +	if (tsl256x_exec_writebyte(TSL256X_REG_INT, &value))
Read the return values of the exec_writebyte and return that. I know
here it is the same, but it will leave use with a more obviously correct
driver which will be easier to maintain in the long run.
> +		return -EIO;
> +
> +	if (periods)
> +		tsl256x_handle->periods = *periods;
> +
> +	return 0;
> +}
> +
> +static int tsl256x_setup(void)
> +{
> +	unsigned int interr = 1, zero = 0;
> +
Style again.
> +	/* reset the threshold settings to trigger an event as soon as the event
> +	 * goes on, forcing a backlight adaptation to the current lighting
> +	 * conditions
> +	 */
> +	tsl256x_exec_writeword(TSL256X_REG_TLOW, &zero);
> +	tsl256x_exec_writeword(TSL256X_REG_THIGH, &zero);
Here is a fine illustration of why passing a pointer into a write function
for the value is not a clean way of doing it.  Also, two possible errors
here that aren't handled.  Check for all such cases and make sure you have
handled them.
> +
> +	/* set gain and time */
> +	if (tsl256x_exec_writebyte(TSL256X_REG_TIMING,
> +				&tsl256x_handle->gain->time))
> +		return -EIO;
> +
> +	/* restore persistence value and enable the interrupt generation */
> +	if (tsl256x_interrupt_ctrls(&interr, &tsl256x_handle->periods))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int tsl256x_set_power(unsigned int status)
> +{
> +	int ret;
> +
> +	if (status) {
> +		ret = tsl256x_setup();
> +		if (ret)
> +			return ret;
> +	}
> +
> +	status = status ? TSL256X_POWER_ON : TSL256X_POWER_OFF;
> +	ret = tsl256x_exec_writebyte(TSL256X_REG_CTRL, &status);
return directly
e.g.
if (status)
	return sony_tsl256x_write_byte(TSL2563_REG_CTRL, TSL256X_POWER_ON);
else
	return sony_tsl256x_write_byte(TSL2563_REG_CTRL, TSL256X_POWER_OFF);
Or even better reorganise the function to only check status once.
> +
> +	return ret;
> +}
> +
Cleaner not to bother with this function and just read the channels
directly where they are wanted via the readword calls.  It doesn't
add much to readability etc.
> +static int tsl256x_get_raw_data(unsigned int *ch0, unsigned int *ch1)
> +{
> +	if (!ch0)
> +		return -EINVAL;
> +
> +	if (tsl256x_exec_readword(TSL256X_REG_DATA0, ch0))
> +		return -EIO;
> +
> +	if (ch1) {
> +		if (tsl256x_exec_readword(TSL256X_REG_DATA1, ch1))
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tsl256x_set_thresholds(const unsigned int *ch0)
> +{
> +	unsigned int tlow, thigh;
> +
> +	tlow = (*ch0 * tsl256x_handle->defaults[0]) / 100;
> +	thigh = ((*ch0 * tsl256x_handle->defaults[1]) / 100) + 1;
> +
> +	if (thigh > 0xffff)
> +		thigh = 0xffff;
> +
> +	if (tsl256x_exec_writeword(TSL256X_REG_TLOW, &tlow) ||
> +		tsl256x_exec_writeword(TSL256X_REG_THIGH, &thigh))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
Prefix this appropriately. SONY_LAPTOP_TSL256X_MAX_LUX or similar.
> +#define MAX_LUX 700000
I'd like to know how this differs form the function in tsl2563.c...

This takes a few more things into account perhaps, but they are fundamentally
the same and we should only have one...
> +static unsigned int tsl256x_calculate_lux(const u32 ch0, const u32 ch1)
> +{
> +	/* the raw output from the sensor is just a "count" value, as it is the
> +	 * result of the integration of the analog sensor signal, the
> +	 * counts-to-lux curve (and its approximation can be found on the
> +	 * datasheet).
> +	 */
> +	const struct tsl256x_coeff *coeff = tsl256x_handle->coeff_table;
> +	u32 ratio, temp, integer;
> +
> +	if (ch0 >= 65535 || ch1 >= 65535)
> +		return MAX_LUX;
> +
> +	/* STEP 1: ratio calculation, for ch0 & ch1 coeff selection */
> +
> +	/* protect against division by 0 */
> +	ratio = ch0 ? ((ch1 << (LUX_SHIFT_BITS + 1)) / ch0) : UINT_MAX - 1;
> +	/* round the ratio value */
> +	ratio = (ratio + 1) >> 1;
> +
> +	/* coeff selection rule */
> +	while (coeff->ratio < ratio)
> +		coeff++;
> +
> +	/* STEP 2: lux calculation formula using the right coeffcients */
> +	temp = (ch0 * coeff->ch0) - (ch1 * coeff->ch1);
> +	temp *= tsl256x_handle->gain->scale;
> +	/* the sensor is placed under a plastic or glass cover which filters
> +	 * a certain ammount of light (depending on that particular material).
> +	 * To have an accurate reading, we need to compensate for this loss,
> +	 * multiplying for compensation parameter, taken from the DSDT.
> +	 */
> +	temp *= tsl256x_handle->defaults[3] / 10;
> +
> +	/* round fractional portion to obtain the integer part */
> +	integer = (temp + (1 << (LUX_SHIFT_BITS - 1))) >> LUX_SHIFT_BITS;
> +
> +	if (integer > MAX_LUX)
> +		return MAX_LUX;
> +
> +	return integer;
> +}
> +
This is new for a tsl256x chip..  Hmm. approximate temperature form the
infrared channel?  How accurate is it? (just currious as I wouldn't have
through these were really suitable for use as thermopiles)

Why pass in ch1 if you aren't going to use it?
> +static void tsl256x_calculate_kelvin(const u32 *ch0, const u32 *ch1,
> +					unsigned int *temperature)
> +{
> +	const struct tsl256x_coeff *coeff = tsl256x_handle->coeff_table;
> +	u32 ratio;
> +
> +	/* protect against division by 0 */
> +	ratio = *ch0 ? ((*ch1 << (LUX_SHIFT_BITS + 1)) / *ch0) : UINT_MAX - 1;
> +	/* round the ratio value */
> +	ratio = (ratio + 1) >> 1;
> +
> +	/* coeff selection rule */
> +	while (coeff->ratio < ratio)
> +		coeff++;
> +
> +	*temperature = ratio ? coeff->ka / ratio + coeff->kb : 0;
> +}
> +
integ?  Why that variable name?
> +static int tsl256x_get_lux(unsigned int *integ)
> +{
> +	int ret = 0;
> +	unsigned int ch0, ch1;
> +
> +	if (!integ)
> +		return -1;
> +
> +	ret = tsl256x_get_raw_data(&ch0, &ch1);
> +	if (!ret)
Flip this and exit out wiht an error.  Thats a far more common
kernel code ideom and when writing this sort of code you want
to do what people expect...
e.g.
if (ret)
    return ret;
*integ =
> +		*integ = tsl256x_calculate_lux(ch0, ch1);
> +
> +	return ret;
> +}
> +
> +static int tsl256x_get_kelvin(unsigned int *temperature)
> +{
> +	int ret = -1;
> +	unsigned int ch0, ch1;
> +
> +	if (!temperature)
> +		return ret;
> +
> +	ret = tsl256x_get_raw_data(&ch0, &ch1);
> +	if (!ret)
> +		tsl256x_calculate_kelvin(&ch0, &ch1, temperature);
> +
> +	return ret;
> +}
> +
> +static int tsl256x_get_id(void)
> +{
> +	int id, ret;
> +	unsigned int result;
> +	char *name = NULL;
> +
> +	ret = tsl256x_exec_readbyte(TSL256X_REG_ID, &result);
> +	if (ret)
> +		return ret;
> +
> +	id = (result >> 0x04) & 0x0F;
> +	switch (id) {
> +	case 11:
> +		name = "TAOS TSL2569";
Hmm. Didn't know this part existed.  I wonder if the existing tsl2563 will
work with it out of the box.  Guess I'll have to read the datasheet at
some point.
> +		break;
> +	case 5:
> +		name = "TAOS TSL2561";
> +		break;
> +	case 3:
> +		name = "TAOS TSL2563";
> +		break;
> +	case 0:
> +		name = "TAOS TSL2560CS";
> +		break;
> +	default:
> +		pr_warn("unsupported ALS model %u rev%u\n", id, result & 0x0F);
> +		return -ENXIO;
> +	}
> +
> +	pr_info("ALS model %u rev%u (%s)\n", id, result & 0x0F, name);
> +	return id;
> +}
> +
> +static int tsl256x_event_handler(void)
> +{
> +	unsigned int ch0, scale, interr = 1;
> +
> +	/* clear the notification */
> +	sony_call_snc_handle(als_handle->handle, 0x04C60500, &interr);
> +
> +	/* read the raw data */
> +	tsl256x_get_raw_data(&ch0, NULL);
> +
> +	scale = tsl256x_handle->gain->scale;
> +	if (ch0 < tsl256x_handle->gain->min)
> +		tsl256x_handle->gain--;
> +	else if (ch0 > tsl256x_handle->gain->max)
> +		tsl256x_handle->gain++;
> +
> +	if (tsl256x_handle->gain->scale != scale) {
> +		ch0 *= scale;
> +		ch0 /= tsl256x_handle->gain->scale;
> +		tsl256x_exec_writebyte(TSL256X_REG_TIMING,
> +				&tsl256x_handle->gain->time);
> +	}
> +
> +	/* set the thresholds */
> +	tsl256x_set_thresholds(&ch0);
> +
> +	/* enable interrupt */
> +	tsl256x_interrupt_ctrls(&interr, NULL);
> +
You are pushing events without any direct control interface for them...
> +	iio_push_event(als_handle->indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> +					    0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns());
> +	return 0;
> +}
> +
> +static int tsl256x_init(const u8 defaults[])
> +{
> +	int id = tsl256x_get_id();
> +	if (id < 0)
> +		return id;
> +
> +	tsl256x_handle = kzalloc(sizeof(struct tsl256x_data), GFP_KERNEL);
> +	if (!tsl256x_handle)
> +		return -ENOMEM;
> +
> +	/* populate the device data */
*laughs*  Not remotely inituitive.  This needs an enum for each of the
two defaults registers..
> +	tsl256x_handle->defaults[0] = defaults[3];  /* low threshold % */
> +	tsl256x_handle->defaults[1] = defaults[4];  /* high threshold % */
> +	tsl256x_handle->defaults[2] = defaults[9];  /* sensor interrupt rate */
> +	tsl256x_handle->defaults[3] = defaults[10]; /* light compensat. rate */
> +	tsl256x_handle->gain = tsl256x_gain;
> +	tsl256x_handle->periods = defaults[9];
> +	tsl256x_handle->coeff_table = id == 0 ?
> +		tsl256x_coeff_cs : tsl256x_coeff_fn;
> +
> +	return 0;
> +}
> +
> +static int tsl256x_exit(void)
> +{
> +	unsigned int interr = 0, periods = tsl256x_handle->defaults[2];
> +
> +	/* disable the interrupt generation, restore defaults */
> +	tsl256x_interrupt_ctrls(&interr, &periods);
> +
> +	tsl256x_handle->coeff_table = NULL;
> +	kfree(tsl256x_handle);
> +
> +	return 0;
> +}
> +
> +/* TAOS TSL256x specific ops */
> +static const struct als_device_ops tsl256x_ops = {
> +	.init = tsl256x_init,
> +	.exit = tsl256x_exit,
> +	.event_handler = tsl256x_event_handler,
> +	.set_power = tsl256x_set_power,
> +	.get_lux = tsl256x_get_lux,
> +	.get_kelvin = tsl256x_get_kelvin,
> +};
> +
So down to here you have some sony specific read access to a device
we already have support for.... See introduction to this review.

Now this looks more interesting...

> +/* unknown ALS sensors controlled by the EC present on newer Vaios */
> +static int ngals_get_lux(unsigned int *integ)
> +{
> +	unsigned int data;
> +
> +	/* Event needs to be rearmed */
> +	if (sony_call_snc_handle(als_handle->handle, 0x12200, &data))
> +		return -EIO;
> +	/* read raw data */
> +	if (sony_call_snc_handle(als_handle->handle, 0x1000, &data))
> +		return -EIO;
> +
> +	/* if we have a valid lux data */
> +	if (!!(data & 0xff0000) == 0x01)
Its late and I need more coffee but...
if (data & 0ff00000) gives the same result...
> +		*integ = 0xffff & data;
> +	else
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int ngals_event_handler(void)
> +{
> +	unsigned int unused;
> +
Again, you are pushing without there being any sysfs elements to control them..
With those missing there is no way for userspace to know that the device
might spit out events, or what they might be...

> +	iio_push_event(als_handle->indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> +					    0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns());
> +
> +	/* Event needs to be rearmed */
> +	if (sony_call_snc_handle(als_handle->handle, 0x12200, &unused))
> +		return -EIO;
> +	if (sony_call_snc_handle(als_handle->handle, 0x1000, &unused))
> +		return -EIO;
> +
> +
> +	return 0;
> +}
> +
> +static const struct als_device_ops ngals_ops = {
> +	.init = NULL,
> +	.exit = NULL,
> +	.event_handler = ngals_event_handler,
> +	.set_power = NULL,
> +	.get_lux = ngals_get_lux,
> +	.get_kelvin = NULL,
> +};
> +
> +static int __sony_nc_als_power_set(unsigned int status)
> +{
> +	unsigned int cmd, result;
> +
> +	if (als_handle->ops->set_power) {
> +		if (als_handle->ops->set_power(status))
> +			return -EIO;
> +	}
> +

> +	/*  turn on/off the event notification */
If so then then this is what should be controllable from userspace...
via the event_info elements in iio.
There are a lot of magic addresses in here. If they are known then
they should be replaced by appropriately informative #defined names.
> +	cmd = als_handle->handle < 0x0143 ? 0x0901 : 0x2200;
> +
> +	if (sony_call_snc_handle(als_handle->handle,
> +				(status << 0x10) | cmd, &result))
> +		return -EIO;
> +
> +	if (sony_call_snc_handle(als_handle->handle, 0x1000, &result))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +/* ALS sys interface */
Naming of this vs next function is confusing.. Call it
sony_nc_als_array_show or similar (or just name it after the backlight
and be done with it)
> +static ssize_t sony_nc_als_parameters_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer)
> +{
> +	ssize_t count = 0;
> +	unsigned int i, num;
> +	u8 *list;
> +
This definitely needs documenting..
> +	/* backlight_sensibility_table */
> +	list = als_handle->levels;
> +	num = als_handle->levels_num;
> +
> +	for (i = 0; i < num; i++)
> +		count += snprintf(buffer + count, PAGE_SIZE - count,
> +				"%u ", list[i]);
> +
> +	count += snprintf(buffer + count, PAGE_SIZE - count, "\n");
> +
> +	return count;
> +}
> +
> +static ssize_t sony_nc_als_param_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer)
> +{
> +	ssize_t count = 0;
> +	unsigned int i;
> +	int offset;
> +
> +	for (i = 0; als_handle->als_params[i].name; i++)
> +		if (!strcmp(attr->attr.name, als_handle->als_params[i].name))
> +			break;
> +
> +	/* first 2 elements of the array are 2 bytes values */
> +	offset = als_handle->als_params[i].offset;
> +	if (i < 2)
> +		count = snprintf(buffer, PAGE_SIZE, "%u\n",
use a shift instead of *256 (usual thing about what people expect rather than
anything else. Might also need a typecast to avoid ti occuring entirely
in a u8 value (which obviously won't work).
> +				als_handle->defaults[offset]
> +				+ 256 * als_handle->defaults[offset + 1]);
> +	else if (als_handle->als_params[i].name)
> +		count = snprintf(buffer, PAGE_SIZE, "%u\n",
> +				als_handle->defaults[offset]);
> +
> +	return count;
> +}
> +
I'd argue this is just a temperature reading and hence should be done
via a temperature channel. Irritatingly they are in celcius, but
the conversion is at least nice and easy ;)
> +static ssize_t sony_nc_als_kelvin_show(struct device *dev,
> +		struct device_attribute *attr, char *buffer)
> +{
> +	ssize_t count = 0;
> +	unsigned int kelvin = 0;
> +
> +	if (als_handle->ops->get_kelvin)
> +		als_handle->ops->get_kelvin(&kelvin);
> +
> +	count = snprintf(buffer, PAGE_SIZE, "%d\n", kelvin);
> +
> +	return count;
> +}
> +
> +
> +/* ALS attach/detach functions */
> +static void sony_nc_als_resume(void)
> +{
> +	__sony_nc_als_power_set(1);
> +}
> +
> +static int sony_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan, int *val, int *val2,
> +		long mask)
> +{
> +	struct als_device *als_handle = iio_priv(indio_dev);
> +
> +	if (als_handle->ops->get_lux(val))
> +		return -EINVAL;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info sony_info = {
> +	.attrs			= &als_attr_group,
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= &sony_read_raw,
> +};
> +
> +static const struct iio_event_spec sony_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER
Would expect some control stuff in here as well...
> +	}
> +};
> +
> +static const struct iio_chan_spec sony_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.indexed = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.channel = 0,
> +		.event_spec = sony_events,
> +		.num_event_specs = ARRAY_SIZE(sony_events)
> +	}
> +};
> +
> +static int sony_nc_als_setup(struct acpi_device *dev, unsigned int handle)
> +{
> +	struct iio_dev *indio_dev;
> +	int result = 0;
> +	u64 arg;
> +
> +	if (!enable_als)
> +		return 0;
> +
> +	indio_dev = devm_iio_device_alloc(&dev->dev, sizeof(struct als_device));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	als_handle = iio_priv(indio_dev);
> +	als_handle->indio_dev = indio_dev;
> +	als_handle->handle = handle;
> +
> +	/* model specific data */
> +	switch (handle) {
> +	case 0x0143:
> +	case 0x014b:
> +	case 0x014c:
> +	case 0x0163:
> +		/* check the device presence */
> +		if (sony_call_snc_handle(handle, 0x100, &result)) {
> +			result = -EIO;
> +			goto nosensor;
> +		}
> +
> +		if (!(result & 0x02))
> +			/* no ALS controls */
> +			goto nosensor;
> +
> +		als_handle->ops = &ngals_ops;
> +		als_handle->levels_num = 16;
> +		/* There're 101 additional values. What do we do with them? */
> +		als_handle->als_params = ngals_params;
> +		als_attr_group.attrs = ngals_attributes;
> +		break;
> +
> +	case 0x0137:
> +		/* check the device presence */
> +		if (sony_call_snc_handle(handle, 0xB00, &result)) {
> +			result = -EIO;
> +			goto nosensor;
> +		}
> +
> +		if (!(result & 0x01))
> +			/* no ALS controls */
> +			goto nosensor;
> +		/* fall through */
> +
> +	case 0x012f:
> +		als_handle->ops = &tsl256x_ops;
> +		als_handle->levels_num = 9;
> +		als_handle->als_params = als_params;
> +		als_attr_group.attrs = als_attributes;
> +		break;
> +
> +	default:
> +		result = -EINVAL;
> +		goto nosensor;
> +	}
> +
> +
> +	/* backlight levels are the first levels_num values, the remaining
> +	 * values are default settings for als regulation
> +	 */
> +	als_handle->levels = als_handle->parameters;
> +	als_handle->defaults = als_handle->parameters + als_handle->levels_num;
> +
> +	/* get ALS parameters */
> +	arg = sony_find_snc_handle(handle);
> +	if (sony_nc_buffer_call(sony_nc_acpi_handle, "SN06", &arg,
> +		als_handle->parameters, ALS_TABLE_SIZE) < 0)
> +		goto nosensor;
> +
> +	/* initial device configuration */
> +	if (als_handle->ops->init) {
> +		result = als_handle->ops->init(als_handle->defaults);
> +		if (result)
> +			goto nosensor;
> +	}
> +
> +	indio_dev->dev.parent = &dev->dev;
> +	indio_dev->channels = sony_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sony_channels);
> +	indio_dev->name = "sony-laptop";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &sony_info;
> +
> +	result = iio_device_register(indio_dev);
> +	if (result < 0)
> +		goto nosensor;
> +
> +	/* set power state */
> +	if (__sony_nc_als_power_set(1))
> +		pr_warn("unable to set the power status\n");
> +
> +	return 0;
> +
> +nosensor:
> +	iio_device_free(indio_dev);
> +	als_handle = NULL;
> +
> +	return result;
> +}
> +
> +static void sony_nc_als_cleanup(struct platform_device *pd)
> +{
> +	if (als_handle) {
> +		if (__sony_nc_als_power_set(0))
> +			pr_info("ALS power off failed\n");
> +
> +		if (als_handle->ops->exit)
> +			if (als_handle->ops->exit())
> +				pr_info("ALS device clean-up failed\n");
> +
> +		iio_device_unregister(als_handle->indio_dev);
Either drop the free or don't use a devm allocation.  You don't
want one in the probe error path either.

> +		iio_device_free(als_handle->indio_dev);
> +		als_handle = NULL;
> +	}
> +}
> +/* end ALS code */
> +
>   /* Keyboard backlight feature */
>   struct kbd_backlight {
>   	unsigned int handle;
> @@ -3058,6 +4116,7 @@ static void sony_nc_backlight_ng_read_limits(int handle,
>   	case 0x143:
>   	case 0x14b:
>   	case 0x14c:
> +	case 0x163:
>   		lvl_table_len = 16;
>   		break;
>   	}
> @@ -3086,11 +4145,30 @@ static void sony_nc_backlight_ng_read_limits(int handle,
>
>   static void sony_nc_backlight_setup(void)
>   {
> +	acpi_handle unused;
>   	int max_brightness = 0;
>   	const struct backlight_ops *ops = NULL;
>   	struct backlight_properties props;
> +	int acpi_video_support = acpi_video_backlight_support();
> +
> +	/* backlight control behaves differently when ALS is enabled.
> +	 * 1. if the acpi backlight device is not registered we lose brightness
> +	 *    key press events (Fn+F[56])
> +	 * 2. when the acpi backlight device is registered, changing brightness
> +	 *    through that doesn't have any effect other than sending a
> +	 *    notification to SNC: Notify (\_SB.PCI0.LPCB.SNC, 0x93)
> +	 *    In this case we SNC is supposed to react and set the new
> +	 *    brightness but the brightness value set via _BCM is stored outside
> +	 *    SNC scope.
> +	 * We allow registering our "sony" backlight device even if acpi is
> +	 * preferred and we set it up to receive notifications upon which we act
> +	 * scaling _BCM's values to SNC's.
> +	 */
> +	if (!als_handle && acpi_video_support) {
> +		pr_info("brightness ignored, must be controlled by ACPI video driver\n");
> +		return;
>
> -	if (sony_find_snc_handle(0x12f) >= 0) {
> +	} else if (sony_find_snc_handle(0x12f) >= 0) {
>   		ops = &sony_backlight_ng_ops;
>   		sony_bl_props.cmd_base = 0x0100;
>   		sony_nc_backlight_ng_read_limits(0x12f, &sony_bl_props);
> @@ -3120,7 +4198,15 @@ static void sony_nc_backlight_setup(void)
>   		sony_nc_backlight_ng_read_limits(0x14c, &sony_bl_props);
>   		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
>
> -	} else if (acpi_has_method(sony_nc_acpi_handle, "GBRT")) {
> +	} else if (sony_find_snc_handle(0x163) >= 0) {
> +		ops = &sony_backlight_ng_ops;
> +		sony_bl_props.cmd_base = 0x3000;
> +		sony_nc_backlight_ng_read_limits(0x163, &sony_bl_props);
> +		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
> +
> +	/* legacy */
> +	} else if (ACPI_SUCCESS(acpi_get_handle(sony_nc_acpi_handle, "GBRT",
> +						&unused))) {
>   		ops = &sony_backlight_ops;
>   		max_brightness = SONY_MAX_BRIGHTNESS - 1;
>
> @@ -3205,12 +4291,7 @@ static int sony_nc_add(struct acpi_device *device)
>   			sony_nc_function_setup(device, sony_pf_device);
>   	}
>
> -	/* setup input devices and helper fifo */
> -	if (acpi_video_backlight_support()) {
> -		pr_info("brightness ignored, must be controlled by ACPI video driver\n");
> -	} else {
> -		sony_nc_backlight_setup();
> -	}
> +	sony_nc_backlight_setup();
>
>   	/* create sony_pf sysfs attributes related to the SNC device */
>   	for (item = sony_nc_values; item->name; ++item) {
>

Re: [PATCH 10/11] sony-laptop: als support

[Mattia Dongili]

On Sun, Mar 23, 2014 at 07:32:34PM +0000, Jonathan Cameron wrote:
> On 20/03/14 23:01, Mattia Dongili wrote:
> >From: Javier Achirica <jachirica@gmail.com>
> >
> >vaios come with two different type of ACPI based ALS controls. The
...

Hi Jonathan,
thanks for the extensive review.
I can answer some of your questions, those related to the taos driver in
particular need to wait for Javier as large part of the work was plain
reverse engineering the windows code that he did.

> I hate to say it but my first reaction to this was, 'What are so
> many functions doing in one file in the first place?'
> 
> The embedded world has been busy splitting up and reparcelling large
> files like this so the various devices end up within the correct
> subsystems. I won't even go into all the reasons for this as they
> are listed everywhere else?  Is this just a case of modifying old
> code no one wants to touch on a larger scale or is there actually
> resistance to breaking new functionality out of this file and
> into nice clean small chunks?

I don't think there is any resistance per-se and to some extent it has
been done in the past (see meye.ko).
On the other hand there is no documentation on the devices this driver
is about and any abstraction you put in place to generalize access and
to allow splitting functionalities, will break. I have to admit that
things have somehow settled recently (and the Vaio brand has just been
buried, we'll see what happens) so making an attempt may make sense
now.
But then again, I'm not sure that making a mini-subsystem out of an
undocumented device/interface is going to pay out.

> So my first request is can we at least split this new functionality
> out?

we can certainly try. :)
Though some functionalities are so interdependent from each other that
I'm not sure it makes much sense (backlight Fn-keys, panel backlight and
als are the worst).

> As I look through what we have here, it looks like we ought to have
> this functionality alone split into the ALS sensor and a backlight
> driver (which can by all means use the standard interfaces to
> talk to the ALS)... (event support for in kernel users isn't there
> yet, but this might motivate us to hurry up with it).
> 
> The bulk of this driver is actually just a set of nasty access routines
> and then functionality that already exists in the existing tsl2563 driver.
> Hence, unless there are very good reasons why not I would suggest the
> following.
> 
> 1) Convert the existing tsl2563 driver over to using regmap.
> 2) Add regmap support for the access routines we have here then add the
>    little bit of code needed to make the tsl2563 register appopriately.  This
>    may require a small amount of magic mfd like code in here.
> (plan b on this is to perhaps create an i2c bus driver using the sony
> specific calls).

I need to figure out what regmaps are about (a quick look in
include/linux/regmap.h reveals a lot), this is going to be a piece of
work.

> 3) Figure out what extra bits are that are needed by the ALS driver we have
>    here and add them on (kelvin support and perhaps a few other bits).
> 4) Then and only then take on any non ALS elements in here (such as the
>    back light).

most of the backlight code here is really a workaround to a sick
behaviour that vaios have once you enable the als device. It's generally
unrelated to the als portion of the driver except that once you
introduce the latter, the former will break. The code is folded together
to avoid being broken at any stage but it can be split to different
changes if it makes more sense.

> The other als driver (one where all the clever stuff is hidden) should ideally
> be a completely separate als driver (be it a very simple one.
> 
> Anyhow, your code is reasonably clean but I think some of these questions about
> the fundamental approach need addressing to avoid getting ourselves into a
> nasty mess in the long term.   Sorry if this review seems a little negative.

it does indeed but I wasn't expecting this enourmous blob to go through
unnoticed and this is the type of feedback I wanted to get this code
unstuck and finally ready for inclusion.

I'll go through your comments in a few iterations to clean up the
obvious first and evaluate the larger rework (i.e. merging with the
current taos driver and splitting the als portion of sony-laptop) with
more stable code at hand.

Most of the questions you have about the driver itself will need to be
answered by Javier though, I have only a few comments below about
generic code in sony-laptop.c.

> I appreciate that you've only taken the approach taken throughout this file!
> 
> Just as an aside, is there any interest in cleaning this whole file up?
> The sonypi device looks like it should registering a whole host of standard
> interfaces...

iirc, sonypi doesn't have enough known functionality to cover the
standard interfaces, it largely predates most of them. It's quite
some time I don't look into it and it's actually a legacy device that
is likely better be deprecated than worked on. If anybody wants to take
it on, I'm not opposed though.

> Jonathan
> >Cc: Jonathan Cameron <jic23@kernel.org>
> >Cc: linux-iio@vger.kernel.org
> >Cc: Jingoo Han <jg1.han@samsung.com>
> >Cc: Marco Chiappero <marco@absence.it>
> >Signed-off-by: Javier Achirica <jachirica@gmail.com>
> >Signed-off-by: Mattia Dongili <malattia@linux.it>
> >---
> >  Documentation/laptops/sony-laptop.txt |   31 +
> >  drivers/platform/x86/Kconfig          |    1 +
> >  drivers/platform/x86/sony-laptop.c    | 1111 ++++++++++++++++++++++++++++++++-
> >  3 files changed, 1128 insertions(+), 15 deletions(-)
> >
> >diff --git a/Documentation/laptops/sony-laptop.txt b/Documentation/laptops/sony-laptop.txt
...
> >diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
...
> >diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
...

[large chop, I'll let Javier comment on those questions]

> >@@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct backlight_device *bd)
> >  	return (result & 0xff) - sdev->offset;
> >  }
> >
> >-static int sony_nc_update_status_ng(struct backlight_device *bd)
> >+static int __sony_nc_set_brightness_ng(struct sony_backlight_props *bl,
> >+		int brightness)
> >  {
> >-	int value, result;
> >-	struct sony_backlight_props *sdev =
> >-		(struct sony_backlight_props *)bl_get_data(bd);
> >+	int result = 0;
> >+	int value = brightness + bl->offset;
> >
> >-	value = bd->props.brightness + sdev->offset;
> >-	if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value << 0x10),
> >+	if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value << 0x10),
> >  				&result))
> >  		return -EIO;
> >
> >  	return value;
> >  }
> Whilst this stuff is presumably being driver from the ALS it is decidedly
> back light specific. Perhaps I'm idealistic in thinking it should be
> a different driver.

err, this hunk should not have made it here at all. Part of the other
backlight work is in this patch for the reason I explained above but
this one is really extraneous.

> >
> >+
> >+static int sony_nc_update_status_ng(struct backlight_device *bd)
> >+{
> >+	struct sony_backlight_props *sdev =
> >+		(struct sony_backlight_props *)bl_get_data(bd);
> >+
> >+	return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
> >+}
> >+
> >+static void sony_nc_brightness_changed_ng(struct backlight_device *bd,
> >+		int brightness, int max_brightness)
> >+{
> >+	int new_brightness =
> >+		brightness * bd->props.max_brightness / max_brightness;
> >+	struct sony_backlight_props *sdev =
> >+		(struct sony_backlight_props *)bl_get_data(bd);
> >+
> >+	dprintk("brightness changed cur: %d, max: %d -> new: %d\n", brightness,
> >+			max_brightness, new_brightness);
> >+
> >+	__sony_nc_set_brightness_ng(sdev, new_brightness);
> >+	backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
> >+}
> >+
> >  static const struct backlight_ops sony_backlight_ops = {
> >  	.options = BL_CORE_SUSPENDRESUME,
> >  	.update_status = sony_backlight_update_status,
> >@@ -1088,6 +1252,7 @@ static const struct backlight_ops sony_backlight_ng_ops = {
> >  	.options = BL_CORE_SUSPENDRESUME,
> >  	.update_status = sony_nc_update_status_ng,
> >  	.get_brightness = sony_nc_get_brightness_ng,
> >+	.brightness_changed = &sony_nc_brightness_changed_ng,
> >  };
> >
> >  /*
> >@@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event, unsigned int handle)
> >  enum event_types {
> >  	HOTKEY = 1,
> >  	KILLSWITCH,
> >-	GFX_SWITCH
> >+	GFX_SWITCH,
> >+	ALS
> >  };
> Hmm. I don't suppose suggesting this should be an mfd with a nice clean
> event hook up interface will go down to well?

ah, a multi function device. Again I'd need to look how it would look
like adding that to sony-laptop but these are essentially for triggering
acpi events.
Would it make it that much more clean than decoding the event from the
GPE notification callback and sending it via ACPI's netlink interface
(which is all we are doing here)?

> >  static void sony_nc_notify(struct acpi_device *device, u32 event)
> >  {
> >@@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device *device, u32 event)
> >  			ev_type = GFX_SWITCH;
> >  			real_ev = __sony_nc_gfx_switch_status_get();
> >  			break;
> >+
> >+		case 0x0143:
> >+		case 0x014b:
> >+		case 0x014c:
> >+		case 0x0163:
> Could you document what these different events are here?  Would make
> it easier to review.

yes, I can add documentation but this is not als specific.
In general SNC exposes a number of "handles" (those you see above) in an
indexed table (see find_snc_handle). All event notifications and calls
into the device methods (SN06 and SN07 are the entry points) specify the
offset at which the desired handle is.
Each handle cover one or more functionality based on the arguments provided.
Different handle may cover similar functionalities and are different
from model to model. 

The functionalities are easier to see in sony_nc_function_setup where
each handle has it's corresponding setup function called.

The event decoding here (sony_nc_notify) have to look at what handle is
associated to it and decide accordingly (map a hotkey, read a switch
position, read additional "light" event information).

...

> You are pushing events without any direct control interface for them...
> >+	iio_push_event(als_handle->indio_dev,
> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> >+					    0,
> >+					    IIO_EV_TYPE_THRESH,
> >+					    IIO_EV_DIR_EITHER),
> >+		       iio_get_time_ns());
...
> Again, you are pushing without there being any sysfs elements to control them..
> With those missing there is no way for userspace to know that the device
> might spit out events, or what they might be...
> 
> >+	iio_push_event(als_handle->indio_dev,
> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> >+					    0,
> >+					    IIO_EV_TYPE_THRESH,
> >+					    IIO_EV_DIR_EITHER),
> >+		       iio_get_time_ns());
...
> >+static const struct iio_event_spec sony_events[] = {
> >+	{
> >+		.type = IIO_EV_TYPE_THRESH,
> >+		.dir = IIO_EV_DIR_EITHER
> Would expect some control stuff in here as well...
> >+	}

what do you mean? do you have an example I can look at?

Thanks
-- 
mattia
:wq!

Re: [PATCH 10/11] sony-laptop: als support

[Jonathan Cameron]

On March 24, 2014 11:06:07 PM GMT+00:00, Mattia Dongili <malattia-k2GhghHVRtY@public.gmane.org> wrote:
>On Sun, Mar 23, 2014 at 07:32:34PM +0000, Jonathan Cameron wrote:
>> On 20/03/14 23:01, Mattia Dongili wrote:
>> >From: Javier Achirica <jachirica-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >
>> >vaios come with two different type of ACPI based ALS controls. The
>...
>
>Hi Jonathan,
>thanks for the extensive review.
>I can answer some of your questions, those related to the taos driver
>in
>particular need to wait for Javier as large part of the work was plain
>reverse engineering the windows code that he did.
>
>> I hate to say it but my first reaction to this was, 'What are so
>> many functions doing in one file in the first place?'
>> 
>> The embedded world has been busy splitting up and reparcelling large
>> files like this so the various devices end up within the correct
>> subsystems. I won't even go into all the reasons for this as they
>> are listed everywhere else?  Is this just a case of modifying old
>> code no one wants to touch on a larger scale or is there actually
>> resistance to breaking new functionality out of this file and
>> into nice clean small chunks?
>
>I don't think there is any resistance per-se and to some extent it has
>been done in the past (see meye.ko).
>On the other hand there is no documentation on the devices this driver
>is about and any abstraction you put in place to generalize access and
>to allow splitting functionalities, will break. I have to admit that
>things have somehow settled recently (and the Vaio brand has just been
>buried, we'll see what happens) so making an attempt may make sense
>now.
>But then again, I'm not sure that making a mini-subsystem out of an
>undocumented device/interface is going to pay out.
>
>> So my first request is can we at least split this new functionality
>> out?
>
>we can certainly try. :)
>Though some functionalities are so interdependent from each other that
>I'm not sure it makes much sense (backlight Fn-keys, panel backlight
>and
>als are the worst).
>
>> As I look through what we have here, it looks like we ought to have
>> this functionality alone split into the ALS sensor and a backlight
>> driver (which can by all means use the standard interfaces to
>> talk to the ALS)... (event support for in kernel users isn't there
>> yet, but this might motivate us to hurry up with it).
>> 
>> The bulk of this driver is actually just a set of nasty access
>routines
>> and then functionality that already exists in the existing tsl2563
>driver.
>> Hence, unless there are very good reasons why not I would suggest the
>> following.
>> 
>> 1) Convert the existing tsl2563 driver over to using regmap.
>> 2) Add regmap support for the access routines we have here then add
>the
>>    little bit of code needed to make the tsl2563 register
>appopriately.  This
>>    may require a small amount of magic mfd like code in here.
>> (plan b on this is to perhaps create an i2c bus driver using the sony
>> specific calls).
>
>I need to figure out what regmaps are about (a quick look in
>include/linux/regmap.h reveals a lot), this is going to be a piece of
>work.
>
>> 3) Figure out what extra bits are that are needed by the ALS driver
>we have
>>    here and add them on (kelvin support and perhaps a few other
>bits).
>> 4) Then and only then take on any non ALS elements in here (such as
>the
>>    back light).
>
>most of the backlight code here is really a workaround to a sick
>behaviour that vaios have once you enable the als device. It's
>generally
>unrelated to the als portion of the driver except that once you
>introduce the latter, the former will break. The code is folded
>together
>to avoid being broken at any stage but it can be split to different
>changes if it makes more sense.
>
>> The other als driver (one where all the clever stuff is hidden)
>should ideally
>> be a completely separate als driver (be it a very simple one.
>> 
>> Anyhow, your code is reasonably clean but I think some of these
>questions about
>> the fundamental approach need addressing to avoid getting ourselves
>into a
>> nasty mess in the long term.   Sorry if this review seems a little
>negative.
>
>it does indeed but I wasn't expecting this enourmous blob to go through
>unnoticed and this is the type of feedback I wanted to get this code
>unstuck and finally ready for inclusion.
>
>I'll go through your comments in a few iterations to clean up the
>obvious first and evaluate the larger rework (i.e. merging with the
>current taos driver and splitting the als portion of sony-laptop) with
>more stable code at hand.
>
>Most of the questions you have about the driver itself will need to be
>answered by Javier though, I have only a few comments below about
>generic code in sony-laptop.c.
>
>> I appreciate that you've only taken the approach taken throughout
>this file!
>> 
>> Just as an aside, is there any interest in cleaning this whole file
>up?
>> The sonypi device looks like it should registering a whole host of
>standard
>> interfaces...
>
>iirc, sonypi doesn't have enough known functionality to cover the
>standard interfaces, it largely predates most of them. It's quite
>some time I don't look into it and it's actually a legacy device that
>is likely better be deprecated than worked on. If anybody wants to take
>it on, I'm not opposed though.
>
>> Jonathan
>> >Cc: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> >Cc: Marco Chiappero <marco-YK5v5TwyeXionA0d6jMUrA@public.gmane.org>
>> >Signed-off-by: Javier Achirica <jachirica-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >Signed-off-by: Mattia Dongili <malattia-k2GhghHVRtY@public.gmane.org>
>> >---
>> >  Documentation/laptops/sony-laptop.txt |   31 +
>> >  drivers/platform/x86/Kconfig          |    1 +
>> >  drivers/platform/x86/sony-laptop.c    | 1111
>++++++++++++++++++++++++++++++++-
>> >  3 files changed, 1128 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/Documentation/laptops/sony-laptop.txt
>b/Documentation/laptops/sony-laptop.txt
>...
>> >diff --git a/drivers/platform/x86/Kconfig
>b/drivers/platform/x86/Kconfig
>...
>> >diff --git a/drivers/platform/x86/sony-laptop.c
>b/drivers/platform/x86/sony-laptop.c
>...
>
>[large chop, I'll let Javier comment on those questions]
>
>> >@@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct
>backlight_device *bd)
>> >  	return (result & 0xff) - sdev->offset;
>> >  }
>> >
>> >-static int sony_nc_update_status_ng(struct backlight_device *bd)
>> >+static int __sony_nc_set_brightness_ng(struct sony_backlight_props
>*bl,
>> >+		int brightness)
>> >  {
>> >-	int value, result;
>> >-	struct sony_backlight_props *sdev =
>> >-		(struct sony_backlight_props *)bl_get_data(bd);
>> >+	int result = 0;
>> >+	int value = brightness + bl->offset;
>> >
>> >-	value = bd->props.brightness + sdev->offset;
>> >-	if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value <<
>0x10),
>> >+	if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value <<
>0x10),
>> >  				&result))
>> >  		return -EIO;
>> >
>> >  	return value;
>> >  }
>> Whilst this stuff is presumably being driver from the ALS it is
>decidedly
>> back light specific. Perhaps I'm idealistic in thinking it should be
>> a different driver.
>
>err, this hunk should not have made it here at all. Part of the other
>backlight work is in this patch for the reason I explained above but
>this one is really extraneous.
>
>> >
>> >+
>> >+static int sony_nc_update_status_ng(struct backlight_device *bd)
>> >+{
>> >+	struct sony_backlight_props *sdev =
>> >+		(struct sony_backlight_props *)bl_get_data(bd);
>> >+
>> >+	return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
>> >+}
>> >+
>> >+static void sony_nc_brightness_changed_ng(struct backlight_device
>*bd,
>> >+		int brightness, int max_brightness)
>> >+{
>> >+	int new_brightness =
>> >+		brightness * bd->props.max_brightness / max_brightness;
>> >+	struct sony_backlight_props *sdev =
>> >+		(struct sony_backlight_props *)bl_get_data(bd);
>> >+
>> >+	dprintk("brightness changed cur: %d, max: %d -> new: %d\n",
>brightness,
>> >+			max_brightness, new_brightness);
>> >+
>> >+	__sony_nc_set_brightness_ng(sdev, new_brightness);
>> >+	backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
>> >+}
>> >+
>> >  static const struct backlight_ops sony_backlight_ops = {
>> >  	.options = BL_CORE_SUSPENDRESUME,
>> >  	.update_status = sony_backlight_update_status,
>> >@@ -1088,6 +1252,7 @@ static const struct backlight_ops
>sony_backlight_ng_ops = {
>> >  	.options = BL_CORE_SUSPENDRESUME,
>> >  	.update_status = sony_nc_update_status_ng,
>> >  	.get_brightness = sony_nc_get_brightness_ng,
>> >+	.brightness_changed = &sony_nc_brightness_changed_ng,
>> >  };
>> >
>> >  /*
>> >@@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event,
>unsigned int handle)
>> >  enum event_types {
>> >  	HOTKEY = 1,
>> >  	KILLSWITCH,
>> >-	GFX_SWITCH
>> >+	GFX_SWITCH,
>> >+	ALS
>> >  };
>> Hmm. I don't suppose suggesting this should be an mfd with a nice
>clean
>> event hook up interface will go down to well?
>
>ah, a multi function device. Again I'd need to look how it would look
>like adding that to sony-laptop but these are essentially for
>triggering
>acpi events.
>Would it make it that much more clean than decoding the event from the
>GPE notification callback and sending it via ACPI's netlink interface
>(which is all we are doing here)?
>
>> >  static void sony_nc_notify(struct acpi_device *device, u32 event)
>> >  {
>> >@@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device
>*device, u32 event)
>> >  			ev_type = GFX_SWITCH;
>> >  			real_ev = __sony_nc_gfx_switch_status_get();
>> >  			break;
>> >+
>> >+		case 0x0143:
>> >+		case 0x014b:
>> >+		case 0x014c:
>> >+		case 0x0163:
>> Could you document what these different events are here?  Would make
>> it easier to review.
>
>yes, I can add documentation but this is not als specific.
>In general SNC exposes a number of "handles" (those you see above) in
>an
>indexed table (see find_snc_handle). All event notifications and calls
>into the device methods (SN06 and SN07 are the entry points) specify
>the
>offset at which the desired handle is.
>Each handle cover one or more functionality based on the arguments
>provided.
>Different handle may cover similar functionalities and are different
>from model to model. 
>
>The functionalities are easier to see in sony_nc_function_setup where
>each handle has it's corresponding setup function called.
>
>The event decoding here (sony_nc_notify) have to look at what handle is
>associated to it and decide accordingly (map a hotkey, read a switch
>position, read additional "light" event information).
>
>...
>
>> You are pushing events without any direct control interface for
>them...
>> >+	iio_push_event(als_handle->indio_dev,
>> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
>> >+					    0,
>> >+					    IIO_EV_TYPE_THRESH,
>> >+					    IIO_EV_DIR_EITHER),
>> >+		       iio_get_time_ns());
>...
>> Again, you are pushing without there being any sysfs elements to
>control them..
>> With those missing there is no way for userspace to know that the
>device
>> might spit out events, or what they might be...
>> 
>> >+	iio_push_event(als_handle->indio_dev,
>> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
>> >+					    0,
>> >+					    IIO_EV_TYPE_THRESH,
>> >+					    IIO_EV_DIR_EITHER),
>> >+		       iio_get_time_ns());
>...
>> >+static const struct iio_event_spec sony_events[] = {
>> >+	{
>> >+		.type = IIO_EV_TYPE_THRESH,
>> >+		.dir = IIO_EV_DIR_EITHER
>> Would expect some control stuff in here as well...
>> >+	}
>
>what do you mean? do you have an example I can look at?
As it is somewhat relevant how about the tsl2563 driver...

https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/light/tsl2563.c?h=togreg&id=239670ef48dfff9cf07675acdb3bb7deee4853e1

See the mask separate elements and event related callbacks.
>
>Thanks

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 10/11] sony-laptop: als support

[Jonathan Cameron]

On March 24, 2014 11:06:07 PM GMT+00:00, Mattia Dongili <malattia@linux.it> wrote:
>On Sun, Mar 23, 2014 at 07:32:34PM +0000, Jonathan Cameron wrote:
>> On 20/03/14 23:01, Mattia Dongili wrote:
>> >From: Javier Achirica <jachirica@gmail.com>
>> >
>> >vaios come with two different type of ACPI based ALS controls. The
>...
>
>Hi Jonathan,
>thanks for the extensive review.
>I can answer some of your questions, those related to the taos driver
>in
>particular need to wait for Javier as large part of the work was plain
>reverse engineering the windows code that he did.
>
>> I hate to say it but my first reaction to this was, 'What are so
>> many functions doing in one file in the first place?'
>> 
>> The embedded world has been busy splitting up and reparcelling large
>> files like this so the various devices end up within the correct
>> subsystems. I won't even go into all the reasons for this as they
>> are listed everywhere else?  Is this just a case of modifying old
>> code no one wants to touch on a larger scale or is there actually
>> resistance to breaking new functionality out of this file and
>> into nice clean small chunks?
>
>I don't think there is any resistance per-se and to some extent it has
>been done in the past (see meye.ko).
>On the other hand there is no documentation on the devices this driver
>is about and any abstraction you put in place to generalize access and
>to allow splitting functionalities, will break. I have to admit that
>things have somehow settled recently (and the Vaio brand has just been
>buried, we'll see what happens) so making an attempt may make sense
>now.
>But then again, I'm not sure that making a mini-subsystem out of an
>undocumented device/interface is going to pay out.
>
>> So my first request is can we at least split this new functionality
>> out?
>
>we can certainly try. :)
>Though some functionalities are so interdependent from each other that
>I'm not sure it makes much sense (backlight Fn-keys, panel backlight
>and
>als are the worst).
>
>> As I look through what we have here, it looks like we ought to have
>> this functionality alone split into the ALS sensor and a backlight
>> driver (which can by all means use the standard interfaces to
>> talk to the ALS)... (event support for in kernel users isn't there
>> yet, but this might motivate us to hurry up with it).
>> 
>> The bulk of this driver is actually just a set of nasty access
>routines
>> and then functionality that already exists in the existing tsl2563
>driver.
>> Hence, unless there are very good reasons why not I would suggest the
>> following.
>> 
>> 1) Convert the existing tsl2563 driver over to using regmap.
>> 2) Add regmap support for the access routines we have here then add
>the
>>    little bit of code needed to make the tsl2563 register
>appopriately.  This
>>    may require a small amount of magic mfd like code in here.
>> (plan b on this is to perhaps create an i2c bus driver using the sony
>> specific calls).
>
>I need to figure out what regmaps are about (a quick look in
>include/linux/regmap.h reveals a lot), this is going to be a piece of
>work.
>
>> 3) Figure out what extra bits are that are needed by the ALS driver
>we have
>>    here and add them on (kelvin support and perhaps a few other
>bits).
>> 4) Then and only then take on any non ALS elements in here (such as
>the
>>    back light).
>
>most of the backlight code here is really a workaround to a sick
>behaviour that vaios have once you enable the als device. It's
>generally
>unrelated to the als portion of the driver except that once you
>introduce the latter, the former will break. The code is folded
>together
>to avoid being broken at any stage but it can be split to different
>changes if it makes more sense.
>
>> The other als driver (one where all the clever stuff is hidden)
>should ideally
>> be a completely separate als driver (be it a very simple one.
>> 
>> Anyhow, your code is reasonably clean but I think some of these
>questions about
>> the fundamental approach need addressing to avoid getting ourselves
>into a
>> nasty mess in the long term.   Sorry if this review seems a little
>negative.
>
>it does indeed but I wasn't expecting this enourmous blob to go through
>unnoticed and this is the type of feedback I wanted to get this code
>unstuck and finally ready for inclusion.
>
>I'll go through your comments in a few iterations to clean up the
>obvious first and evaluate the larger rework (i.e. merging with the
>current taos driver and splitting the als portion of sony-laptop) with
>more stable code at hand.
>
>Most of the questions you have about the driver itself will need to be
>answered by Javier though, I have only a few comments below about
>generic code in sony-laptop.c.
>
>> I appreciate that you've only taken the approach taken throughout
>this file!
>> 
>> Just as an aside, is there any interest in cleaning this whole file
>up?
>> The sonypi device looks like it should registering a whole host of
>standard
>> interfaces...
>
>iirc, sonypi doesn't have enough known functionality to cover the
>standard interfaces, it largely predates most of them. It's quite
>some time I don't look into it and it's actually a legacy device that
>is likely better be deprecated than worked on. If anybody wants to take
>it on, I'm not opposed though.
>
>> Jonathan
>> >Cc: Jonathan Cameron <jic23@kernel.org>
>> >Cc: linux-iio@vger.kernel.org
>> >Cc: Jingoo Han <jg1.han@samsung.com>
>> >Cc: Marco Chiappero <marco@absence.it>
>> >Signed-off-by: Javier Achirica <jachirica@gmail.com>
>> >Signed-off-by: Mattia Dongili <malattia@linux.it>
>> >---
>> >  Documentation/laptops/sony-laptop.txt |   31 +
>> >  drivers/platform/x86/Kconfig          |    1 +
>> >  drivers/platform/x86/sony-laptop.c    | 1111
>++++++++++++++++++++++++++++++++-
>> >  3 files changed, 1128 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/Documentation/laptops/sony-laptop.txt
>b/Documentation/laptops/sony-laptop.txt
>...
>> >diff --git a/drivers/platform/x86/Kconfig
>b/drivers/platform/x86/Kconfig
>...
>> >diff --git a/drivers/platform/x86/sony-laptop.c
>b/drivers/platform/x86/sony-laptop.c
>...
>
>[large chop, I'll let Javier comment on those questions]
>
>> >@@ -1065,20 +1206,43 @@ static int sony_nc_get_brightness_ng(struct
>backlight_device *bd)
>> >  	return (result & 0xff) - sdev->offset;
>> >  }
>> >
>> >-static int sony_nc_update_status_ng(struct backlight_device *bd)
>> >+static int __sony_nc_set_brightness_ng(struct sony_backlight_props
>*bl,
>> >+		int brightness)
>> >  {
>> >-	int value, result;
>> >-	struct sony_backlight_props *sdev =
>> >-		(struct sony_backlight_props *)bl_get_data(bd);
>> >+	int result = 0;
>> >+	int value = brightness + bl->offset;
>> >
>> >-	value = bd->props.brightness + sdev->offset;
>> >-	if (sony_call_snc_handle(sdev->handle, sdev->cmd_base | (value <<
>0x10),
>> >+	if (sony_call_snc_handle(bl->handle, bl->cmd_base | (value <<
>0x10),
>> >  				&result))
>> >  		return -EIO;
>> >
>> >  	return value;
>> >  }
>> Whilst this stuff is presumably being driver from the ALS it is
>decidedly
>> back light specific. Perhaps I'm idealistic in thinking it should be
>> a different driver.
>
>err, this hunk should not have made it here at all. Part of the other
>backlight work is in this patch for the reason I explained above but
>this one is really extraneous.
>
>> >
>> >+
>> >+static int sony_nc_update_status_ng(struct backlight_device *bd)
>> >+{
>> >+	struct sony_backlight_props *sdev =
>> >+		(struct sony_backlight_props *)bl_get_data(bd);
>> >+
>> >+	return __sony_nc_set_brightness_ng(sdev, bd->props.brightness);
>> >+}
>> >+
>> >+static void sony_nc_brightness_changed_ng(struct backlight_device
>*bd,
>> >+		int brightness, int max_brightness)
>> >+{
>> >+	int new_brightness =
>> >+		brightness * bd->props.max_brightness / max_brightness;
>> >+	struct sony_backlight_props *sdev =
>> >+		(struct sony_backlight_props *)bl_get_data(bd);
>> >+
>> >+	dprintk("brightness changed cur: %d, max: %d -> new: %d\n",
>brightness,
>> >+			max_brightness, new_brightness);
>> >+
>> >+	__sony_nc_set_brightness_ng(sdev, new_brightness);
>> >+	backlight_force_update(bd, BACKLIGHT_UPDATE_HOTKEY);
>> >+}
>> >+
>> >  static const struct backlight_ops sony_backlight_ops = {
>> >  	.options = BL_CORE_SUSPENDRESUME,
>> >  	.update_status = sony_backlight_update_status,
>> >@@ -1088,6 +1252,7 @@ static const struct backlight_ops
>sony_backlight_ng_ops = {
>> >  	.options = BL_CORE_SUSPENDRESUME,
>> >  	.update_status = sony_nc_update_status_ng,
>> >  	.get_brightness = sony_nc_get_brightness_ng,
>> >+	.brightness_changed = &sony_nc_brightness_changed_ng,
>> >  };
>> >
>> >  /*
>> >@@ -1201,7 +1366,8 @@ static int sony_nc_hotkeys_decode(u32 event,
>unsigned int handle)
>> >  enum event_types {
>> >  	HOTKEY = 1,
>> >  	KILLSWITCH,
>> >-	GFX_SWITCH
>> >+	GFX_SWITCH,
>> >+	ALS
>> >  };
>> Hmm. I don't suppose suggesting this should be an mfd with a nice
>clean
>> event hook up interface will go down to well?
>
>ah, a multi function device. Again I'd need to look how it would look
>like adding that to sony-laptop but these are essentially for
>triggering
>acpi events.
>Would it make it that much more clean than decoding the event from the
>GPE notification callback and sending it via ACPI's netlink interface
>(which is all we are doing here)?
>
>> >  static void sony_nc_notify(struct acpi_device *device, u32 event)
>> >  {
>> >@@ -1276,6 +1442,38 @@ static void sony_nc_notify(struct acpi_device
>*device, u32 event)
>> >  			ev_type = GFX_SWITCH;
>> >  			real_ev = __sony_nc_gfx_switch_status_get();
>> >  			break;
>> >+
>> >+		case 0x0143:
>> >+		case 0x014b:
>> >+		case 0x014c:
>> >+		case 0x0163:
>> Could you document what these different events are here?  Would make
>> it easier to review.
>
>yes, I can add documentation but this is not als specific.
>In general SNC exposes a number of "handles" (those you see above) in
>an
>indexed table (see find_snc_handle). All event notifications and calls
>into the device methods (SN06 and SN07 are the entry points) specify
>the
>offset at which the desired handle is.
>Each handle cover one or more functionality based on the arguments
>provided.
>Different handle may cover similar functionalities and are different
>from model to model. 
>
>The functionalities are easier to see in sony_nc_function_setup where
>each handle has it's corresponding setup function called.
>
>The event decoding here (sony_nc_notify) have to look at what handle is
>associated to it and decide accordingly (map a hotkey, read a switch
>position, read additional "light" event information).
>
>...
>
>> You are pushing events without any direct control interface for
>them...
>> >+	iio_push_event(als_handle->indio_dev,
>> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
>> >+					    0,
>> >+					    IIO_EV_TYPE_THRESH,
>> >+					    IIO_EV_DIR_EITHER),
>> >+		       iio_get_time_ns());
>...
>> Again, you are pushing without there being any sysfs elements to
>control them..
>> With those missing there is no way for userspace to know that the
>device
>> might spit out events, or what they might be...
>> 
>> >+	iio_push_event(als_handle->indio_dev,
>> >+		       IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
>> >+					    0,
>> >+					    IIO_EV_TYPE_THRESH,
>> >+					    IIO_EV_DIR_EITHER),
>> >+		       iio_get_time_ns());
>...
>> >+static const struct iio_event_spec sony_events[] = {
>> >+	{
>> >+		.type = IIO_EV_TYPE_THRESH,
>> >+		.dir = IIO_EV_DIR_EITHER
>> Would expect some control stuff in here as well...
>> >+	}
>
>what do you mean? do you have an example I can look at?
As it is somewhat relevant how about the tsl2563 driver...

https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/tree/drivers/iio/light/tsl2563.c?h=togreg&id=239670ef48dfff9cf07675acdb3bb7deee4853e1

See the mask separate elements and event related callbacks.
>
>Thanks

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 10/11] sony-laptop: als support

[Javier Achirica]

Hello,

Jumping in the skipped part...

On Sun, Mar 23, 2014 at 8:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> +Ambient Light Sensor:
>> +---------------------
>> +On models equipped with ALS, sony-laptop will create an IIO device
>> available
>> +under:
>> +       /sys/bus/iio/devices/
>> +In addition to the sensor's readings, a number of other attributes are
>> exposed
>> +for userspace to consume:
>> +       typical_illuminance     Typical illuminance level for brightness
>> +                               control in lux.
>
> Is this a target value?  Need a better description for this as it's not
> immediately
> obvious what it is.

All these names come from the names that Sony used in Windows, but in
some cases we don't really fully understand its meaning. I
reverse-engineered Windows control module, and Sony uses these values
in quite sophisticated algorithms to calculate brightness, which I've
been able to partially replicate, but I'm not covering all cases.

In this specific case, this is the illuminance value used as a "middle
point" from where all brightness variations are calculated.

>> +       backlight_illuminance   Threshold in lux for keyboard backlight.
>> +       lux_correction_percent  Correction factor to apply to brightness
>> in
>> +                               percentage.
>
> If applied to the measurement before exposed to userspace then it should
> be handled using the iio info_mask element calibscale. If it's a value that
> should be applied in userspace then the scale infomask element.

It should be applied in userspace.

>> +       low_threshold_percent   Percentage from current lux level to
>> trigger
>> +                               event (low).
>
> Interesting.  These are a new one :)
> Anyhow the should be handled through additions to iio's iio_event_info
> enum and all the stuff that goes with that.
> Perhaps
> IIO_EV_INFO_PERCENT, with sysfs attributes
> events/in_illuminance_thresh_rising_percent
> events/in_illuminance_thresh_falling_percent
>
> Actually you don't seem to have any IIO_EV_INFO stuff in here yet you
> are still pushing out events via IIO. Interesting...

In fact, I implemented it, but only for the older Vaios (the
TAOS-based ones), but we couldn't figure out how to get this
information in the newer ones, as everything is hidden, so we ended up
removing it. Of course we could add it back, but what should we do
with the hidden ones?

>> +       high_threshold_percent  Percentage from current lux level to
>> trigger
>> +                               event (high).
>> +       als_event_rate          Time in milliseconds between brightness
>> changes
>> +                               when triggered by light change.
>
> The naming is not nearly descriptive enough as I'd read that as being how
> often an event (e.g. it going dark) is checked from the als.

As I said above, we took Sony names. Of course we can change them to anything.

>> +       als_event_delay         Percentage of brightness changes when
>> triggered
>> +                               by light change.
>> +       key_event_rate          Time in milliseconds between brightness
>> changes
>> +                               when triggered by keypress.
>> +       key_event_delay         Percentage of brightness changes when
>> triggered
>> +                               by keypress.
>
> These are so device dependent we'll probably just let them go.  Might be
> something that's worth checking with the input guys to discover if they
> have an interface already defined for this sort of thing.
>
>> +       interrupt_rate          Light sensor interrupt rate in
>> milliseconds.
>> +       light_compensation_x10  Light compensation needed due to sensor
>> cover.
>
> Offset, or a scale factor?  Either way there are well defined abi elements
> for
> this already.

Scale factor.

>> +       minimum_illuminance     Minimum expected illuminance level in lux.
>
> Expected in what sense?

Once again, is something that it's used in Sony code. This range is
the illuminance range where the backlight change is meaningful
(outside of it, backlight change is almost unnoticeable).

>> +       maximum_illuminance     Maximum expected illuminance level in lux.
>> +       algorithm               Brightness calculation algorithm to be
>> used.
>
> That's a lot of new abi as far as IIO is concerned.  It will all need
> documenting in
> Documentation/ABI/testing/sysfs-bus-iio-sony-laptop or the generic files if
> appropriate.
> Also, no units for some of the above.

I think we documented units every we know about them, but surely we'll recheck.

>> +static ssize_t sony_nc_als_kelvin_show(struct device *dev,
>> +               struct device_attribute *attr, char *buffer);
>
> Are we talking actual temperature or colour temperature here?
> Either way, I missed the documentation...

Colour temperature.

>> +static struct attribute *ngals_attributes[] = {
>> +       &als_attr_backlight_sensibility_table.attr,
>> +       &als_attr_minimum_illuminance.attr,
>> +       &als_attr_maximum_illuminance.attr,
>> +       &als_attr_als_event_rate.attr,
>> +       &als_attr_als_event_delay.attr,
>> +       &als_attr_key_event_rate.attr,
>> +       &als_attr_key_event_delay.attr,
>> +       &als_attr_algorithm.attr,
>
> So is algorithm going to give me a magic number?  Please convert this
> to something human readable if so.

Yes. From the code looks like right now the magic number can go from 0
to 3, but we don't really know how to name them (apart from
"first_algorithm", "new_algorithm", "nice_algorithm" or something
similar). Any suggestion?

>> +static const struct tsl256x_coeff tsl256x_coeff_fn[] = {
>> +       {
>> +               .ratio  = SCALE(12500), /* 0.125 * 2^LUX_SHIFT_BITS  */
>> +               .ch0    = SCALE(3040),  /* 0.0304 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(2720),  /* 0.0272 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(313550000),
>> +               .kb     = -10651,
>> +       }, {
>> +               .ratio  = SCALE(25000), /* 0.250 * 2^LUX_SHIFT_BITS  */
>> +               .ch0    = SCALE(3250),  /* 0.0325 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(4400),  /* 0.0440 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(203390000),
>> +               .kb     = -2341,
>> +       }, {
>> +               .ratio  = SCALE(37500), /* 0.375 * 2^LUX_SHIFT_BITS  */
>> +               .ch0    = SCALE(3510),  /* 0.0351 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(5440),  /* 0.0544 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(152180000),
>> +               .kb     = 157,
>> +       }, {
>> +               .ratio  = SCALE(50000), /* 0.50 * 2^LUX_SHIFT_BITS   */
>> +               .ch0    = SCALE(3810),  /* 0.0381 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(6240),  /* 0.0624 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(163580000),
>> +               .kb     = -145,
>> +       }, {
>> +               .ratio  = SCALE(61000), /* 0.61 * 2^LUX_SHIFT_BITS   */
>> +               .ch0    = SCALE(2240),  /* 0.0224 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(3100),  /* 0.0310 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(180800000),
>> +               .kb     = -495,
>> +       }, {
>> +               .ratio  = SCALE(80000), /* 0.80 * 2^LUX_SHIFT_BITS   */
>> +               .ch0    = SCALE(1280),  /* 0.0128 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(1530),  /* 0.0153 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(197340000),
>> +               .kb     = -765
>> +       }, {
>> +               .ratio  = SCALE(130000),/* 1.3 * 2^LUX_SHIFT_BITS     */
>> +               .ch0    = SCALE(146),   /* 0.00146 * 2^LUX_SHIFT_BITS */
>> +               .ch1    = SCALE(112),   /* 0.00112 * 2^LUX_SHIFT_BITS */
>> +               .ka     = SCALE(182900000),
>> +               .kb     = -608,
>> +       }, {
>> +               .ratio  = UINT_MAX,     /* for higher ratios */
>> +               .ch0    = 0,
>> +               .ch1    = 0,
>> +               .ka     = 0,
>> +               .kb     = 830,
>> +       }
>> +};
>>
> So what are _fn and _cs?

Different chips with different tables. Chip model names end in FN and CS.

[Lots of nice code cleaning suggestions removed...]

> Prefix this appropriately. SONY_LAPTOP_TSL256X_MAX_LUX or similar.
>>
>> +#define MAX_LUX 700000
>
> I'd like to know how this differs form the function in tsl2563.c...
>
> This takes a few more things into account perhaps, but they are
> fundamentally
> the same and we should only have one...

It uses multiple ranges available in the chip, it surely can be merged
in the tsl2563.c code.

>> +static unsigned int tsl256x_calculate_lux(const u32 ch0, const u32 ch1)
>> +{
>> +       /* the raw output from the sensor is just a "count" value, as it
>> is the
>> +        * result of the integration of the analog sensor signal, the
>> +        * counts-to-lux curve (and its approximation can be found on the
>> +        * datasheet).
>> +        */
>> +       const struct tsl256x_coeff *coeff = tsl256x_handle->coeff_table;
>> +       u32 ratio, temp, integer;
>> +
>> +       if (ch0 >= 65535 || ch1 >= 65535)
>> +               return MAX_LUX;
>> +
>> +       /* STEP 1: ratio calculation, for ch0 & ch1 coeff selection */
>> +
>> +       /* protect against division by 0 */
>> +       ratio = ch0 ? ((ch1 << (LUX_SHIFT_BITS + 1)) / ch0) : UINT_MAX -
>> 1;
>> +       /* round the ratio value */
>> +       ratio = (ratio + 1) >> 1;
>> +
>> +       /* coeff selection rule */
>> +       while (coeff->ratio < ratio)
>> +               coeff++;
>> +
>> +       /* STEP 2: lux calculation formula using the right coeffcients */
>> +       temp = (ch0 * coeff->ch0) - (ch1 * coeff->ch1);
>> +       temp *= tsl256x_handle->gain->scale;
>> +       /* the sensor is placed under a plastic or glass cover which
>> filters
>> +        * a certain ammount of light (depending on that particular
>> material).
>> +        * To have an accurate reading, we need to compensate for this
>> loss,
>> +        * multiplying for compensation parameter, taken from the DSDT.
>> +        */
>> +       temp *= tsl256x_handle->defaults[3] / 10;
>> +
>> +       /* round fractional portion to obtain the integer part */
>> +       integer = (temp + (1 << (LUX_SHIFT_BITS - 1))) >> LUX_SHIFT_BITS;
>> +
>> +       if (integer > MAX_LUX)
>> +               return MAX_LUX;
>> +
>> +       return integer;
>> +}
>> +
>
> This is new for a tsl256x chip..  Hmm. approximate temperature form the
> infrared channel?  How accurate is it? (just currious as I wouldn't have
> through these were really suitable for use as thermopiles)

It's reasonable accurate while using black-body light sources
(incandescent, halogen, sunlight), not so good with sodium, mercury,
fluorescent or LEDs.

The temperature is obtained comparing the relative level of visible
and infrared channels. In fact this is a correction that has to be
than to determine the visible illuminance level, so I just mapped it
to temperature in kelvins.

> integ?  Why that variable name?
>
>> +static int tsl256x_get_lux(unsigned int *integ)
>> +{
>> +       int ret = 0;
>> +       unsigned int ch0, ch1;
>> +
>> +       if (!integ)
>> +               return -1;
>> +
>> +       ret = tsl256x_get_raw_data(&ch0, &ch1);
>> +       if (!ret)

Used to be integer and fractional part, but as the fractional part
wasn't available in the newer versions, we just removed it.

> I'd argue this is just a temperature reading and hence should be done
> via a temperature channel. Irritatingly they are in celcius, but
> the conversion is at least nice and easy ;)
>
>> +static ssize_t sony_nc_als_kelvin_show(struct device *dev,
>> +               struct device_attribute *attr, char *buffer)
>> +{
>> +       ssize_t count = 0;
>> +       unsigned int kelvin = 0;
>> +
>> +       if (als_handle->ops->get_kelvin)
>> +               als_handle->ops->get_kelvin(&kelvin);
>> +
>> +       count = snprintf(buffer, PAGE_SIZE, "%d\n", kelvin);
>> +
>> +       return count;
>> +}

In kelvin, not exactly celsius. That's what happens when you're
european and have a physics background :-)

Anyway, thank you very much for all the work you've put into reviewing
all this code and all useful comments.

Javier

Re: [PATCH 00/11] ALS and other new functions

[Mattia Dongili]

On Fri, Mar 21, 2014 at 08:01:11AM +0900, Mattia Dongili wrote:
> Matthew,
> 
> This is partially a resend of the previous patch-set (first 7  patches) plus
> additional changes to queue up for 3.15.

could you pick up everything but patch 9 and 10? I'll resend those when
ready with the changes that Jonathan suggested.

Essentially these two should be skipped for now:
>   sony-laptop: als support
>   backlight: introduce inter-driver notification of changes

the latter could actually go by itself but without any real use it's a
bit gratuitous.

Thanks
-- 
mattia
:wq!