lkml.org 
[lkml]   [2012]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] toshiba_acpi: Add support for transflective LCD
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
>




\
 
 \ /
  Last update: 2012-04-03 15:59    [W:0.066 / U:0.500 seconds]
©2003-2014 Jasper Spaans. Advertise on this site