Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Marco Chiappero]

Hello,

I just wanted to report possible issues related to the new backlight 
control implemented in commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8 
[sony-laptop: implement new backlight control method]. I and Javier 
Achirica have been using it for a while on different notebook models and 
agree that values close to 0 are far too dark to be useful, while 0 
means totally off for both. Moreover flickering has been reported with 
CCFL LCDs (still present on a few models, such as the Vaio F) by another 
tester.
In fact, with these Vaios, Windows never goes below a certain model 
specific backlight lower bound, provided along with the ALS capability. 
However, this requires further code not yet implemented (or the ALS 
support already available but not yet included). Please consider 
correcting or delaying that patch.
Thanks.

Marco Chiappero

Re: Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Matthew Garrett]

On Tue, Apr 05, 2011 at 06:17:24PM +0200, Marco Chiappero wrote:

> I just wanted to report possible issues related to the new backlight
> control implemented in commit
> 7751ab8e600f26e10c2ba12a92d48a4852a51da8 [sony-laptop: implement new
> backlight control method]. I and Javier Achirica have been using it
> for a while on different notebook models and agree that values close
> to 0 are far too dark to be useful, while 0 means totally off for
> both. Moreover flickering has been reported with CCFL LCDs (still
> present on a few models, such as the Vaio F) by another tester.
> In fact, with these Vaios, Windows never goes below a certain model
> specific backlight lower bound, provided along with the ALS
> capability. However, this requires further code not yet implemented
> (or the ALS support already available but not yet included). Please
> consider correcting or delaying that patch.

Hmm. How low is useful? There's no real problem with shifting everything 
by an offset.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Marco Chiappero]

Il 12/04/2011 17:20, Matthew Garrett ha scritto:

> Hmm. How low is useful? There's no real problem with shifting everything
> by an offset.

I agree. The DSDT of any ALS equipped Vaio provides us 9 backlight 
levels (that are LCD/model specific), so the first and the last ones 
define our useful operating range, which is always different from 0-255 
(eg. 13-255 for the Vaio S, 52-255 for the Vaio F, 7-182 for the Vaio Z, 
7-213 for the Vaio TT and so on). However the code to retrieve and use 
those lower and upper bounds is still lacking upstream (I, Javier 
Achirica and many other people are testing a heavily modified 
sony-laptop.c written by us which includes lots of features that we 
might include soon, but there is no patchset yet).
Moreover it is questionable whether to expose this "wire-range"/fine 
grain backlight device, which is intended to be used for an ambient 
light based regulation (that requires smooth transitions and sometimes 
minimal changes), while for normal backlight operations the 9 levels 
already mentioned should suffice.

Regards,
Marco Chiappero

Re: Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Matthew Garrett]

On Wed, Apr 13, 2011 at 06:50:34PM +0200, Marco Chiappero wrote:

> I agree. The DSDT of any ALS equipped Vaio provides us 9 backlight
> levels (that are LCD/model specific), so the first and the last ones
> define our useful operating range, which is always different from
> 0-255 (eg. 13-255 for the Vaio S, 52-255 for the Vaio F, 7-182 for
> the Vaio Z, 7-213 for the Vaio TT and so on). However the code to
> retrieve and use those lower and upper bounds is still lacking
> upstream (I, Javier Achirica and many other people are testing a
> heavily modified sony-laptop.c written by us which includes lots of
> features that we might include soon, but there is no patchset yet).
> Moreover it is questionable whether to expose this "wire-range"/fine
> grain backlight device, which is intended to be used for an ambient
> light based regulation (that requires smooth transitions and
> sometimes minimal changes), while for normal backlight operations
> the 9 levels already mentioned should suffice.

Can this code be merged within the next week? If not I agree that it'd 
be better to drop this patch for now. I think exposing the full range of 
(usable) brightness values makes sense, since ALS policy is likely to be 
determined in userspace - we'll need some way to expose the appropriate 
mapping, but that's something that needs to be handled generically in 
the long run.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Marco Chiappero]

Il 13/04/2011 18:53, Matthew Garrett ha scritto:
> Can this code be merged within the next week?

Uhm, I don't know exactly but I don't think so.

> If not I agree that it'd
> be better to drop this patch for now.

There is no hurry, such backlight device is useless as long as the ALS 
code isn't included too.

> I think exposing the full range of
> (usable) brightness values makes sense, since ALS policy is likely to be
> determined in userspace - we'll need some way to expose the appropriate
> mapping, but that's something that needs to be handled generically in
> the long run.

Well, Javier Achirica and another tester already produced a working 
daemon implementing the ALS driven backlight regulation (and that might 
also deal with a few more Vaio specific features in the near feature). 
However it's not that plain and easy, exposing a single backlight device 
prevents everything from working properly because of some issues 
(currently we are using a different solution, with its drawbacks as 
well, but that proved to work well). If you're interested I can provide 
further details.

Regards,
Marco Chiappero

Re: Commit 7751ab8e600f26e10c2ba12a92d48a4852a51da8

[Mattia Dongili]

On Wed, Apr 13, 2011 at 05:53:44PM +0100, Matthew Garrett wrote:
> On Wed, Apr 13, 2011 at 06:50:34PM +0200, Marco Chiappero wrote:
> 
> > I agree. The DSDT of any ALS equipped Vaio provides us 9 backlight
> > levels (that are LCD/model specific), so the first and the last ones
> > define our useful operating range, which is always different from
> > 0-255 (eg. 13-255 for the Vaio S, 52-255 for the Vaio F, 7-182 for
> > the Vaio Z, 7-213 for the Vaio TT and so on). However the code to
> > retrieve and use those lower and upper bounds is still lacking
> > upstream (I, Javier Achirica and many other people are testing a
...
> Can this code be merged within the next week? If not I agree that it'd 

yes, I'll try to get it ready over the weekend. The patch itself should
be relatively innocuous.

-- 
mattia
:wq!

[PATCH] sony-laptop: limit brightness range to DSDT provided ones

[Mattia Dongili]

The new style brightness control provides an operating range of 9 values
(seems consistent over a large number of models sharing the same
brightness control methods).
Read and use the minimum and maximum values to limit the backlight
interface between those boundaries.

Signed-off-by: Mattia Dongili <malattia@linux.it>
---
 drivers/platform/x86/sony-laptop.c |  126 +++++++++++++++++++++++++++++-------
 1 files changed, 102 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/sony-laptop.c b/drivers/platform/x86/sony-laptop.c
index 8f709ae..cd327cf 100644
--- a/drivers/platform/x86/sony-laptop.c
+++ b/drivers/platform/x86/sony-laptop.c
@@ -934,6 +934,14 @@ static ssize_t sony_nc_sysfs_store(struct device *dev,
 /*
  * Backlight device
  */
+struct sony_backlight_props {
+	struct backlight_device *dev;
+	int			handle;
+	u8			offset;
+	u8			maxlvl;
+};
+struct sony_backlight_props sony_bl_props;
+
 static int sony_backlight_update_status(struct backlight_device *bd)
 {
 	return acpi_callsetfunc(sony_nc_acpi_handle, "SBRT",
@@ -953,20 +961,23 @@ static int sony_backlight_get_brightness(struct backlight_device *bd)
 static int sony_nc_get_brightness_ng(struct backlight_device *bd)
 {
 	int result;
-	int *handle = (int *)bl_get_data(bd);
+	struct sony_backlight_props *sdev =
+		(struct sony_backlight_props *)bl_get_data(bd);
 
-	sony_call_snc_handle(*handle, 0x0200, &result);
+	sony_call_snc_handle(sdev->handle, 0x0200, &result);
 
-	return result & 0xff;
+	return (result & 0xff) - sdev->offset;
 }
 
 static int sony_nc_update_status_ng(struct backlight_device *bd)
 {
 	int value, result;
-	int *handle = (int *)bl_get_data(bd);
+	struct sony_backlight_props *sdev =
+		(struct sony_backlight_props *)bl_get_data(bd);
 
-	value = bd->props.brightness;
-	sony_call_snc_handle(*handle, 0x0100 | (value << 16), &result);
+	value = bd->props.brightness + sdev->offset;
+	if (sony_call_snc_handle(sdev->handle, 0x0100 | (value << 16), &result))
+		return -EIO;
 
 	return sony_nc_get_brightness_ng(bd);
 }
@@ -981,8 +992,6 @@ static const struct backlight_ops sony_backlight_ng_ops = {
 	.update_status = sony_nc_update_status_ng,
 	.get_brightness = sony_nc_get_brightness_ng,
 };
-static int backlight_ng_handle;
-static struct backlight_device *sony_backlight_device;
 
 /*
  * New SNC-only Vaios event mapping to driver known keys
@@ -1549,6 +1558,75 @@ static void sony_nc_kbd_backlight_resume(void)
 				&ignore);
 }
 
+static void sony_nc_backlight_ng_read_limits(int handle,
+		struct sony_backlight_props *props)
+{
+	int offset;
+	acpi_status status;
+	u8 brlvl, i;
+	u8 min = 0xff, max = 0x00;
+	struct acpi_object_list params;
+	union acpi_object in_obj;
+	union acpi_object *lvl_enum;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	props->handle = handle;
+	props->offset = 0;
+	props->maxlvl = 0xff;
+
+	offset = sony_find_snc_handle(handle);
+	if (offset < 0)
+		return;
+
+	/* try to read the boundaries from ACPI tables, if we fail the above
+	 * defaults should be reasonable
+	 */
+	params.count = 1;
+	params.pointer = &in_obj;
+	in_obj.type = ACPI_TYPE_INTEGER;
+	in_obj.integer.value = offset;
+	status = acpi_evaluate_object(sony_nc_acpi_handle, "SN06", &params,
+			&buffer);
+	if (ACPI_FAILURE(status))
+		return;
+
+	lvl_enum = (union acpi_object *) buffer.pointer;
+	if (!lvl_enum) {
+		pr_err(DRV_PFX "No SN06 return object.");
+		return;
+	}
+	if (lvl_enum->type != ACPI_TYPE_BUFFER) {
+		pr_err(DRV_PFX "Invalid SN06 return object 0x%.2x\n",
+				lvl_enum->type);
+		goto out_invalid;
+	}
+
+	/* the buffer lists brightness levels available, brightness levels are
+	 * from 0 to 8 in the array, other values are used by ALS control.
+	 */
+	for (i = 0; i < 9 && i < lvl_enum->buffer.length; i++) {
+
+		brlvl = *(lvl_enum->buffer.pointer + i);
+		dprintk("Brightness level: %d\n", brlvl);
+
+		if (!brlvl)
+			break;
+
+		if (brlvl > max)
+			max = brlvl;
+		if (brlvl < min)
+			min = brlvl;
+	}
+	props->offset = min;
+	props->maxlvl = max;
+	dprintk("Brightness levels: min=%d max=%d\n", props->offset,
+			props->maxlvl);
+
+out_invalid:
+	kfree(buffer.pointer);
+	return;
+}
+
 static void sony_nc_backlight_setup(void)
 {
 	acpi_handle unused;
@@ -1557,14 +1635,14 @@ static void sony_nc_backlight_setup(void)
 	struct backlight_properties props;
 
 	if (sony_find_snc_handle(0x12f) != -1) {
-		backlight_ng_handle = 0x12f;
 		ops = &sony_backlight_ng_ops;
-		max_brightness = 0xff;
+		sony_nc_backlight_ng_read_limits(0x12f, &sony_bl_props);
+		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
 
 	} else if (sony_find_snc_handle(0x137) != -1) {
-		backlight_ng_handle = 0x137;
 		ops = &sony_backlight_ng_ops;
-		max_brightness = 0xff;
+		sony_nc_backlight_ng_read_limits(0x137, &sony_bl_props);
+		max_brightness = sony_bl_props.maxlvl - sony_bl_props.offset;
 
 	} else if (ACPI_SUCCESS(acpi_get_handle(sony_nc_acpi_handle, "GBRT",
 						&unused))) {
@@ -1577,22 +1655,22 @@ static void sony_nc_backlight_setup(void)
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = max_brightness;
-	sony_backlight_device = backlight_device_register("sony", NULL,
-							  &backlight_ng_handle,
+	sony_bl_props.dev = backlight_device_register("sony", NULL,
+							  &sony_bl_props,
 							  ops, &props);
 
-	if (IS_ERR(sony_backlight_device)) {
+	if (IS_ERR(sony_bl_props.dev)) {
 		pr_warning(DRV_PFX "unable to register backlight device\n");
-		sony_backlight_device = NULL;
+		sony_bl_props.dev = NULL;
 	} else
-		sony_backlight_device->props.brightness =
-		    ops->get_brightness(sony_backlight_device);
+		sony_bl_props.dev->props.brightness =
+		    ops->get_brightness(sony_bl_props.dev);
 }
 
 static void sony_nc_backlight_cleanup(void)
 {
-	if (sony_backlight_device)
-		backlight_device_unregister(sony_backlight_device);
+	if (sony_bl_props.dev)
+		backlight_device_unregister(sony_bl_props.dev);
 }
 
 static int sony_nc_add(struct acpi_device *device)
@@ -2590,7 +2668,7 @@ static long sonypi_misc_ioctl(struct file *fp, unsigned int cmd,
 	mutex_lock(&spic_dev.lock);
 	switch (cmd) {
 	case SONYPI_IOCGBRT:
-		if (sony_backlight_device == NULL) {
+		if (sony_bl_props.dev == NULL) {
 			ret = -EIO;
 			break;
 		}
@@ -2603,7 +2681,7 @@ static long sonypi_misc_ioctl(struct file *fp, unsigned int cmd,
 				ret = -EFAULT;
 		break;
 	case SONYPI_IOCSBRT:
-		if (sony_backlight_device == NULL) {
+		if (sony_bl_props.dev == NULL) {
 			ret = -EIO;
 			break;
 		}
@@ -2617,8 +2695,8 @@ static long sonypi_misc_ioctl(struct file *fp, unsigned int cmd,
 			break;
 		}
 		/* sync the backlight device status */
-		sony_backlight_device->props.brightness =
-		    sony_backlight_get_brightness(sony_backlight_device);
+		sony_bl_props.dev->props.brightness =
+		    sony_backlight_get_brightness(sony_bl_props.dev);
 		break;
 	case SONYPI_IOCGBAT1CAP:
 		if (ec_read16(SONYPI_BAT1_FULL, &val16)) {
-- 
1.7.4.1