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]
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/

Powered by blists - more mailing lists