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: <AANLkTikr3LL-+4LObBaCTo7JGTFTRdVbpbbUH4nfJ28X@mail.gmail.com>
Date:	Mon, 31 Jan 2011 19:07:27 +0100
From:	Pali Rohár <pali.rohar@...il.com>
To:	Anton Vorontsov <cbouatmailru@...il.com>
Cc:	Grazvydas Ignotas <notasas@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: Fwd: [PATCH] bq27x00_battery: Add new properties

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.

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



-- 
Pali Rohár
pali.rohar@...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/