linux-kernel - Re: [PATCH] mfd: add bq2415x charger driver

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, 6 Dec 2011 13:43:06 +0200
From:	Felipe Contreras <felipe.contreras@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Felipe Contreras <felipe.contreras@...ia.com>,
	linux-main <linux-kernel@...r.kernel.org>,
	linux-omap <linux-omap@...r.kernel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	"Pali Roh??r" <pali.rohar@...il.com>,
	Aliaksei Katovich <aliaksei.katovich@...ia.com>,
	Vladimir Zapolskiy <vz@...ia.com>
Subject: Re: [PATCH] mfd: add bq2415x charger driver

On Tue, Dec 6, 2011 at 1:31 PM, Mark Brown
<broonie@...nsource.wolfsonmicro.com> wrote:
> On Tue, Dec 06, 2011 at 12:35:41AM +0200, Felipe Contreras wrote:
>
>> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
>> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
>> @@ -925,6 +925,9 @@ static struct i2c_board_info __initdata rx51_peripherals_i2c_board_info_2[] = {
>>               I2C_BOARD_INFO("bq27200", 0x55),
>>       },
>>  #endif
>> +     {
>> +             I2C_BOARD_INFO("bq24150", 0x6b),
>> +     },
>
> Clearly this is orthogonal.

Yes, I added it just to demonstrate how to activate it.

>> +static inline int bq2415x_i2c_read(struct i2c_client *cli, u8 reg)
>> +{
>> +     return i2c_smbus_read_byte_data(cli, reg);
>> +}
>> +
>> +static inline int bq2415x_i2c_write(struct i2c_client *cli, u8 reg, u8 val)
>> +{
>> +     return i2c_smbus_write_byte_data(cli, reg, val);
>> +}
>
> regmap might be useful here (it's got an update bits operation and
> cache).

Will take a look.

>> +static int bq2415x_set_current_limit(struct i2c_client *cli,
>> +             int min_uA, int max_uA)
>> +{
>> +     int res;
>> +
>> +     res = bq2415x_i2c_read(cli, BQ2415X_GEN_CTL);
>> +     if (res < 0)
>> +             return res;
>> +
>> +     res &= ~BQ2415X_IIN_LIMIT_UNLIM_MASK;
>> +
>> +     if (min_uA >= BQ2415X_IIN_LIMIT_100 && max_uA < BQ2415X_IIN_LIMIT_500)
>> +             ;
>> +     else if (min_uA >= BQ2415X_IIN_LIMIT_500 && max_uA < BQ2415X_IIN_LIMIT_800)
>> +             res |= BQ2415X_IIN_LIMIT_500_MASK;
>> +     else if (min_uA >= BQ2415X_IIN_LIMIT_800 && max_uA < BQ2415X_IIN_LIMIT_UNLIM)
>> +             res |= BQ2415X_IIN_LIMIT_800_MASK;
>> +     else if (min_uA >= BQ2415X_IIN_LIMIT_UNLIM)
>> +             res |= BQ2415X_IIN_LIMIT_UNLIM_MASK;
>> +     else
>> +             return -EINVAL;
>> +
>> +     return bq2415x_i2c_write(cli, BQ2415X_GEN_CTL, res);
>> +}
>
> This is the sort of stuff that people were pushing through the regulator
> API (and you have cloned the interface...) in order to allow a separate
> bit of code to pick the current limits.  At the minute it looks like
> you're hard coding the settings, the regulator API would at least let
> you punt those to machines with fixed configurations and provides a hook
> for anything which does want to play with the configuration at runtime.
> Don't know if there's a better API, but it does seem like this is a
> general thing for chargers and should therefore go through a generic
> API.

Yes, for this particular case the regulator API might be useful, but I
don't see how external code will use this. Will they have to search
for the name of this regulator, and then try to change the
current_limit?

Anyway, there are other levels I'm not sure the regulator interface is
good for, like a "weak battery" voltage threshold, that most likely
would only make sense to handle internally in the driver.

> On the other hand if you just set limits and let the charger get on with
> its thing and run autonomously starting, stopping and fast charging by
> itself then a power supply driver seems like a good fit - just provide
> the upper limits as platform data or something and watch it go.

It would have to change its behavior depending on external events,
like charger plugged/unplugged, different types of chargers, and so
on. I'm thinking the rx51 board code could join some hooks from
isp1704 (which detects the events) into this driver.

Cheers.

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