Re: [PATCH] mfd: add bq2415x charger driver

[Felipe Contreras]

On Tue, Dec 6, 2011 at 1:31 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/