Re: [PATCH v2 1/2] regulator: pm8921-regulator: Add regulator driver for PM8921

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

On Tue, May 03, 2011 at 09:05:56AM -0700, David Collins wrote:
> Create a regulator driver to control all regulators on a Qualcomm
> PM8921 PMIC chip.  This chip contains many different types of
> regulators with a wide range of abilities and voltage ranges.

This is basically OK but a few comments below.

> Eight different regulator types are available on the PM8921.  These
> are managed via 7 different type values in the driver:
> 
> LDO      - low drop out regulator (supports both NMOS and PMOS LDOs)
> NLDO1200 - 1.2A NMOS LDO (different control structure than other LDOs)
> SMPS     - switched-mode power supply
> FTSMPS   - fast transient SMPS
> VS       - voltage switch
> VS300    - 300mA voltage switch (different control structure than
>              other switches)
> NCP      - negative charge pump

Given that I'm not seeing much code sharing except is_enabled() it might
be nice to split the driver up by regulator, it's very large.

> +		for (i = 0; i < pdata->num_regulators; i++) {
> +			mfd_regulators[i].name = PM8921_REGULATOR_DEV_NAME;
> +			mfd_regulators[i].id = pdata->regulator_pdatas[i].id;
> +			mfd_regulators[i].platform_data =
> +				&(pdata->regulator_pdatas[i]);
> +			mfd_regulators[i].pdata_size =
> +				sizeof(struct pm8921_regulator_platform_data);
> +		}
> +		ret = mfd_add_devices(pmic->dev, 0, mfd_regulators,
> +				pdata->num_regulators, NULL, irq_base);

I'm having a hard time liking this.

> +static int pm8921_vreg_masked_write(struct pm8921_vreg *vreg, u16 addr, u8 val,
> +		u8 mask, u8 *reg_save)
> +{
> +	int rc = 0;
> +	u8 reg;
> +
> +	reg = (*reg_save & ~mask) | (val & mask);
> +	if (reg != *reg_save)
> +		rc = pm8xxx_writeb(vreg->dev->parent, addr, reg);
> +
> +	if (rc)
> +		pr_err("pm8xxx_writeb failed; addr=0x%03X, rc=%d\n", addr, rc);

dev_err or one of your custom error macros.

> +static int _pm8921_vreg_is_enabled(struct pm8921_vreg *vreg)
> +{
> +	int rc = 0;
> +
> +	/*
> +	 * All regulator types except advanced mode SMPS, FTSMPS, and VS300 have
> +	 * enable bit in bit 7 of the control register.
> +	 */
> +	switch (vreg->type) {

If they're all checking bit 7 the switch statement feels a bit odd...

> +static int pm8921_nldo_list_voltage(struct regulator_dev *rdev,
> +				    unsigned selector)
> +{
> +	if (selector >= NLDO_SET_POINTS)
> +		return 0;

That looks like it should be returning an error.  0 is for things that
are in range but can't be set for some reason (it's more intended for
values knocked out by constraints or similar).

> +static int _pm8921_nldo1200_get_voltage(struct pm8921_vreg *vreg)
> +{
> +	int uV = 0;
> +	int vprog;
> +
> +	if (!NLDO1200_IN_ADVANCED_MODE(vreg)) {
> +		pr_warn("%s: currently in legacy mode; voltage unknown.\n",
> +			vreg->name);
> +		return vreg->save_uV;
> +	}
> +
> +	vprog = vreg->ctrl_reg & NLDO1200_CTRL_VPROG_MASK;
> +
> +	if ((vreg->ctrl_reg & NLDO1200_CTRL_RANGE_MASK)
> +	    == NLDO1200_CTRL_RANGE_LOW)
> +		uV = vprog * NLDO1200_LOW_UV_STEP + NLDO1200_LOW_UV_MIN;
> +	else
> +		uV = vprog * NLDO1200_HIGH_UV_STEP + NLDO1200_HIGH_UV_MIN;

Just implement get_voltage_sel() - the same thing applies to most of the
other regulators that have meaningful selectors.

> +		/* Advanced mode */
> +		if ((vreg->test_reg[2] & NLDO1200_ADVANCED_PM_MASK)
> +		    == NLDO1200_ADVANCED_PM_LPM)

Do we need #defines for the indexes into these arrays?  It's a bit magic
and the code is complicated enough.

> +	if (mode != REGULATOR_MODE_NORMAL && mode != REGULATOR_MODE_IDLE) {
> +		vreg_err(vreg, "invalid mode: %u\n", mode);
> +		return -EINVAL;
> +	}

switch would be clearer.

> +/**
> + * struct pm8921_regulator_platform_data - PMIC 8921 regulator platform data
> + * @init_data:		regulator constraints
> + * @id:			regulator id; from enum pm8921_vreg_id
> + * @pull_down_enable:	0 = no pulldown, 1 = pulldown when regulator disabled
> + * @pin_ctrl:		pin control inputs to use for the regulator; should be
> + *			a combination of PM8921_VREG_PIN_CTRL_* values
> + * @pin_fn:		action to perform when pin control pin is active
> + * @system_uA:		current drawn from regulator not accounted for by any
> + *			regulator framework consumer

Having system_uA here seems wrong, this is hardly something that is
specific to this chip.