[PATCH v2] toshiba_acpi: Add support for transflective LCD

[PATCH v2] toshiba_acpi: Add support for transflective LCD

[Akio Idehara]

Some Toshiba laptops have the transflective LCD and toshset
can control its backlight state.  I brought this feature to the
mainline.  To support transflective LCD, it's implemented by
adding an extra level to the backlight and having 0 change to
transflective mode.  It was tested on a Toshiba Portege R500.

Signed-off-by: Akio Idehara <zbe64533@gmail.com>
---
 drivers/platform/x86/toshiba_acpi.c |   59 ++++++++++++++++++++++++++++++++--
 1 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index eef3b53..f4188b0 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -95,6 +95,7 @@ MODULE_LICENSE("GPL");
 
 /* registers */
 #define HCI_FAN				0x0004
+#define HCI_TR_BACKLIGHT		0x0005
 #define HCI_SYSTEM_EVENT		0x0016
 #define HCI_VIDEO_OUT			0x001c
 #define HCI_HOTKEY_EVENT		0x001e
@@ -134,6 +135,7 @@ struct toshiba_acpi_dev {
 	unsigned int system_event_supported:1;
 	unsigned int ntfy_supported:1;
 	unsigned int info_supported:1;
+	unsigned int tr_backlight_supported:1;
 
 	struct mutex mutex;
 };
@@ -478,6 +480,22 @@ static const struct rfkill_ops toshiba_rfk_ops = {
 	.poll = bt_rfkill_poll,
 };
 
+static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 *status)
+{
+	u32 hci_result;
+
+	hci_read1(dev, HCI_TR_BACKLIGHT, status, &hci_result);
+	return hci_result == HCI_SUCCESS ? 0 : -EIO;
+}
+
+static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, int value)
+{
+	u32 hci_result;
+
+	hci_write1(dev, HCI_TR_BACKLIGHT, value, &hci_result);
+	return hci_result == HCI_SUCCESS ? 0 : -EIO;
+}
+
 static struct proc_dir_entry *toshiba_proc_dir /*= 0*/ ;
 
 static int get_lcd(struct backlight_device *bd)
@@ -485,10 +503,22 @@ static int get_lcd(struct backlight_device *bd)
 	struct toshiba_acpi_dev *dev = bl_get_data(bd);
 	u32 hci_result;
 	u32 value;
+	int tmp = 0;
+
+	if (dev->tr_backlight_supported) {
+		if (!get_tr_backlight_status(dev, &value)) {
+			if (value)
+				tmp = 1;
+			else
+				return tmp;
+		} else {
+			return -EIO;
+		}
+	}
 
 	hci_read1(dev, HCI_LCD_BRIGHTNESS, &value, &hci_result);
 	if (hci_result == HCI_SUCCESS)
-		return (value >> HCI_LCD_BRIGHTNESS_SHIFT);
+		return (value >> HCI_LCD_BRIGHTNESS_SHIFT) + tmp;
 
 	return -EIO;
 }
@@ -497,15 +527,18 @@ static int lcd_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
 	int value;
+	int levels = HCI_LCD_BRIGHTNESS_LEVELS;
 
 	if (!dev->backlight_dev)
 		return -ENODEV;
 
+	if (dev->tr_backlight_supported)
+		levels++;
+
 	value = get_lcd(dev->backlight_dev);
 	if (value >= 0) {
 		seq_printf(m, "brightness:              %d\n", value);
-		seq_printf(m, "brightness_levels:       %d\n",
-			     HCI_LCD_BRIGHTNESS_LEVELS);
+		seq_printf(m, "brightness_levels:       %d\n", levels);
 		return 0;
 	}
 
@@ -521,6 +554,13 @@ static int lcd_proc_open(struct inode *inode, struct file *file)
 static int set_lcd(struct toshiba_acpi_dev *dev, int value)
 {
 	u32 hci_result;
+	if (dev->tr_backlight_supported) {
+		int ret = set_tr_backlight_status(dev, !!value);
+		if (ret)
+			return ret;
+		if (value)
+			value--;
+	}
 
 	value = value << HCI_LCD_BRIGHTNESS_SHIFT;
 	hci_write1(dev, HCI_LCD_BRIGHTNESS, value, &hci_result);
@@ -541,6 +581,10 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 	size_t len;
 	int value;
 	int ret;
+	int levels = HCI_LCD_BRIGHTNESS_LEVELS;
+
+	if (dev->tr_backlight_supported)
+		levels++;
 
 	len = min(count, sizeof(cmd) - 1);
 	if (copy_from_user(cmd, buf, len))
@@ -548,7 +592,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 	cmd[len] = '\0';
 
 	if (sscanf(cmd, " brightness : %i", &value) == 1 &&
-	    value >= 0 && value < HCI_LCD_BRIGHTNESS_LEVELS) {
+	    value >= 0 && value < levels) {
 		ret = set_lcd(dev, value);
 		if (ret == 0)
 			ret = count;
@@ -1104,8 +1148,15 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 
 	mutex_init(&dev->mutex);
 
+	/* Determine whether or not BIOS supports transflective backlight */
+	ret = get_tr_backlight_status(dev, &dummy) ? false : true;
+	dev->tr_backlight_supported = ret;
+
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
+	/* adding an extra level and having 0 change to transflective mode */
+	if (dev->tr_backlight_supported)
+		props.max_brightness++;
 	dev->backlight_dev = backlight_device_register("toshiba",
 						       &acpi_dev->dev,
 						       dev,
-- 
1.7.7.6

Re: [PATCH v2] toshiba_acpi: Add support for transflective LCD

[Seth Forshee]

On Sat, Mar 31, 2012 at 12:16:30AM +0900, Akio Idehara wrote:
> +static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 *status)
> +{
> +	u32 hci_result;
> +
> +	hci_read1(dev, HCI_TR_BACKLIGHT, status, &hci_result);
> +	return hci_result == HCI_SUCCESS ? 0 : -EIO;
> +}
> +
> +static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, int value)
> +{
> +	u32 hci_result;
> +
> +	hci_write1(dev, HCI_TR_BACKLIGHT, value, &hci_result);
> +	return hci_result == HCI_SUCCESS ? 0 : -EIO;
> +}

I think the code will be easier to read if you change both of these to
use boolean arguments, since that's essentially how they're being used
anyway. I.e.

static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, boolean *enabled);
static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, boolean enable);

> @@ -497,15 +527,18 @@ static int lcd_proc_show(struct seq_file *m, void *v)
>  {
>  	struct toshiba_acpi_dev *dev = m->private;
>  	int value;
> +	int levels = HCI_LCD_BRIGHTNESS_LEVELS;
>  
>  	if (!dev->backlight_dev)
>  		return -ENODEV;
>  
> +	if (dev->tr_backlight_supported)
> +		levels++;

dev->backlight_dev->props.max_brightness + 1? That seems nicer than
having to duplicate the "tr backlight gives me an additional brightness
level" logic throughout the file.

> @@ -1104,8 +1148,15 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
>  
>  	mutex_init(&dev->mutex);
>  
> +	/* Determine whether or not BIOS supports transflective backlight */
> +	ret = get_tr_backlight_status(dev, &dummy) ? false : true;
> +	dev->tr_backlight_supported = ret;

I'd personally prefer

	ret = get_tr_backlight_status(dev, &dummy);
	dev->tr_backlight_supported = !ret;

to be consistent with how this is done other places in the file.

Cheers,
Seth

Re: [PATCH v2] toshiba_acpi: Add support for transflective LCD

[Akio Idehara]

Hi, Seth.

Thank you for reviewing my code again and again.
All your comments make sense.
I'll try to make the v3 patch.

Best Regards,
Akio

Seth Forshee wrote:

> On Sat, Mar 31, 2012 at 12:16:30AM +0900, Akio Idehara wrote:
>> +static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 *status)
>> +{
>> +	u32 hci_result;
>> +
>> +	hci_read1(dev, HCI_TR_BACKLIGHT, status, &hci_result);
>> +	return hci_result == HCI_SUCCESS ? 0 : -EIO;
>> +}
>> +
>> +static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, int value)
>> +{
>> +	u32 hci_result;
>> +
>> +	hci_write1(dev, HCI_TR_BACKLIGHT, value, &hci_result);
>> +	return hci_result == HCI_SUCCESS ? 0 : -EIO;
>> +}
> 
> I think the code will be easier to read if you change both of these to
> use boolean arguments, since that's essentially how they're being used
> anyway. I.e.
> 
> static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, boolean *enabled);
> static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, boolean enable);
> 
>> @@ -497,15 +527,18 @@ static int lcd_proc_show(struct seq_file *m, void *v)
>>  {
>>  	struct toshiba_acpi_dev *dev = m->private;
>>  	int value;
>> +	int levels = HCI_LCD_BRIGHTNESS_LEVELS;
>>  
>>  	if (!dev->backlight_dev)
>>  		return -ENODEV;
>>  
>> +	if (dev->tr_backlight_supported)
>> +		levels++;
> 
> dev->backlight_dev->props.max_brightness + 1? That seems nicer than
> having to duplicate the "tr backlight gives me an additional brightness
> level" logic throughout the file.
> 
>> @@ -1104,8 +1148,15 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
>>  
>>  	mutex_init(&dev->mutex);
>>  
>> +	/* Determine whether or not BIOS supports transflective backlight */
>> +	ret = get_tr_backlight_status(dev, &dummy) ? false : true;
>> +	dev->tr_backlight_supported = ret;
> 
> I'd personally prefer
> 
> 	ret = get_tr_backlight_status(dev, &dummy);
> 	dev->tr_backlight_supported = !ret;
> 
> to be consistent with how this is done other places in the file.
> 
> Cheers,
> Seth
> 

Re: [PATCH v2] toshiba_acpi: Add support for transflective LCD

[Seth Forshee]

On Tue, Apr 03, 2012 at 10:57:28PM +0900, Akio Idehara wrote:
> Hi, Seth.
> 
> Thank you for reviewing my code again and again.
> All your comments make sense.
> I'll try to make the v3 patch.

Great!

I realized I also forgot to mention, it would be better to come up with
a more descriptive name than tmp for the local variable you added to
get_lcd() as well.

Thanks,
Seth