Re: Fwd: [PATCH] bq27x00_battery: Add new properties

[Lars-Peter Clausen <lars@metafoo.de>]

On 01/31/2011 07:07 PM, Pali Rohár wrote:
> 2011/1/31 Anton Vorontsov <cbouatmailru@gmail.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@linux.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@gmail.com
>>
> 
> 
>