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 11:31:31 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Felipe Contreras <felipe.contreras@...ia.com>
Cc:	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>,
	Felipe Contreras <felipe.contreras@...il.com>
Subject: Re: [PATCH] mfd: add bq2415x charger driver

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.

> +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).

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

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.

Either way it doesn't really seem like this device has multiple
functions...
--
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/