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
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/