Re: [PATCH v4 3/3] MFD MAX8998/LP3974: Support Charger

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Jan 04, 2011 at 02:17:41PM +0900, MyungJoo Ham wrote:
> With the new regulator, "CHARGER", users can control charging
> current and turn on and off the charger. Note that the charger
> specification of LP3974 is different from that of MAX8998.
> 
> driver/power/max8998.c supports power supply APIs for

It's probably better to split the power supply driver into a separate
patch as there should be no build time dependency between the two.  I've
added Anton to the CCs as he is the drivers/power maintainer.

One thing that jumps out here is that there's no regulator API usage at
all in the power driver - the power driver just jumps straight in
and updates registers when it needs to do anything.  Are you sure that
the regulator API driver is needed at all, if it is I'd expect to see
use of it in the power API.

> +static const char * const manufacturers[] = {
> +	[TYPE_MAX8998] = "Maxim",
> +	[TYPE_LP3974] = "National Semiconductor",
> +	[TYPE_UNKNOWN] = "Unknown",

Normally this refers to the battery rather than the chip.

> +static void _max8998_update_eoc(struct platform_device *pdev)
> +{
> +	struct max8998_battery_data *max8998 = platform_get_drvdata(pdev);
> +	struct i2c_client *i2c = max8998->iodev->i2c;
> +	int charge_current = 0;
> +	int target_eoc_ratio;
> +	u8 val;
> +
> +	/* Nothing to do. User set EOC with % */
> +	if (max8998->eoc_in_mA == 0)
> +		return;
> +
> +	/* Not initialized. */
> +	if (!charger_current_map_desc)
> +		return;

This should be be driver data rather than a global for neatness (though
in reality the chances of more than one of these chargers in a system
are pretty low).

> --- a/drivers/regulator/max8998.c
> +++ b/drivers/regulator/max8998.c
> @@ -86,6 +86,13 @@ static const struct voltage_map_desc buck3_voltage_map_desc = {
>  static const struct voltage_map_desc buck4_voltage_map_desc = {
>  	.min = 800,	.step = 100,	.max = 2300,
>  };
> +static const int charger_current_map_desc_max8998[] = {
> +	90, 380, 475, 550, 570, 600, 700, 800
> +};
> +static const int charger_current_map_desc_lp3974[] = {
> +	100, 400, 450, 500, 550, 600, 700, 800
> +};
> +const int *charger_current_map_desc;

Ah, this is exported from the regulator driver...  That's slightly odd.
For voltages we've an enumeration API for the supported settings, we
probably ought to add that for current regulators too.

> +	dev_info(&rdev->dev, "charger current limit = %dmA (%xh)\n",
> +			chosen_current, chosen);

dev_dbg() or remove it entirely please, otherwise it might get a bit
noisy.

> +static struct regulator_ops max8998_charger_ops = {
> +	.is_enabled		= max8998_ldo_is_enabled_negated,
> +	/* enable and disable are intentionally negated */
> +	.enable			= max8998_ldo_disable,
> +	.disable		= max8998_ldo_enable,

That's really confusing...  why?