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

[Seth Forshee <seth.forshee@canonical.com>]

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