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

[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.

[...]
> > +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.

[...]
> >  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.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com