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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F7B01C8.9050109@gmail.com>
Date:	Tue, 03 Apr 2012 22:57:28 +0900
From:	Akio Idehara <zbe64533@...il.com>
To:	seth.forshee@...onical.com
CC:	mjg59@...f.ucam.org, platform-driver-x86@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [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
> 


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