linux-kernel - Re: Fwd: [PATCH] bq27x00_battery: Add new properties

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: <4D470B42.7050301@metafoo.de>
Date:	Mon, 31 Jan 2011 20:19:30 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Pali Rohár <pali.rohar@...il.com>
CC:	Anton Vorontsov <cbouatmailru@...il.com>,
	Grazvydas Ignotas <notasas@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Fwd: [PATCH] bq27x00_battery: Add new properties

On 01/31/2011 07:07 PM, Pali Rohár wrote:
> 2011/1/31 Anton Vorontsov <cbouatmailru@...il.com>:
>> On Mon, Jan 31, 2011 at 04:43:56PM +0100, Rodolfo Giometti wrote:
>>> On Mon, Jan 31, 2011 at 04:51:47PM +0300, Anton Vorontsov wrote:
>> [...]
>>> Why not using:
>>>
>>>    !!(gpio & 0x40)
>>>
>>> or
>>>
>>>    (gpio & 0x40) ? 1 : 0
>>>
>>> ? I think it could be more readable... :)
>>>
>>> However, I don't understand why you fix gpio=0... it's your machine
>>> depending settings? In these case you should use the platform_data
>>> mechanism to properly setup the GPIO.
>>
>> I agree. It looks like a machine-specific configuration, and
>> exporting this GPIO as 'ONLINE' property may break other users.
>>
>> Instead, I would suggest adding gpiolib support into this driver,
>> and passing 'int online_gpio' via platform data. That way you can
>> pass any GPIOs to this driver, not only bq's.
>>
>> Pali, you can just split GPIO and 'ONLINE' property additions
>> into a separate patch for now, so we can apply the rest of the
>> changes, and hold on the GPIO stuff.
> 
> Ok, I delete GPIO and ONLINE additions from this patch.
> 
>>
>> [...]
>>>> +static int bq27x00_battery_nac(struct bq27x00_device_info *di)
>>>
>>> Let me suggest using bq27x00_battery_nac_uAh as function name...
>>
>> Regarding these suffixes. I'm not sure. This seems a bit too
>> verbose. So I'd prefer to not introduce these.
>>
>> Though, whatever you guys like... I'm fine either way.
> 
> It think it is not needed to rename functions. In
> Documentation/power/power_supply_class.txt is written that default
> units are µA, µV, µAh, ... and functions in my patch return values in
> these correct units.
> 
>>
>> [...]
>>>>  MODULE_AUTHOR("Rodolfo Giometti <giometti@...ux.it>");
>>>>  MODULE_DESCRIPTION("BQ27x00 battery monitor driver");
>>>> +MODULE_ALIAS("i2c:bq27000");
>>>> +MODULE_ALIAS("i2c:bq27200");
>>>> +MODULE_ALIAS("i2c:bq27500");
>>
>> Btw, this is actually another bugfix. Pali, you might want to separate
>> it into yet another patch.
> 
> Ok, I separate aliases to new patch.
> 

The bq27000 is not an I2C device.

- Lars

>>
>> Thanks!
>>
>> --
>> Anton Vorontsov
>> Email: cbouatmailru@...il.com
>>
> 
> 
> 

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