Re: [PATCH 2/3] regulator: add driver for hi6421 voltage regulator

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5220 bytes --]

On Tue, Jun 04, 2013 at 10:28:42PM +0800, Guodong Xu wrote:

> +Required properties:
> +- compatible: three types of regulator are defined,
> +	- "hisilicon,hi6421-ldo"
> +	- "hisilicon,hi6421-buck012"
> +	- "hisilicon,hi6421-buck345"

What do these mean?

> +- hisilicon,hi6421-ctrl: <ctrl_reg enable_mask eco_mode_mask>
> +- hisilicon,hi6421-vset: <vset_reg vset_mask>
> +- hisilicon,hi6421-n-voltages: <n>
> +- hisilicon,hi6421-vset-table: array of voltages selectable in this regulator
> +- hisilicon,hi6421-uv-step: <uV_step>
> +- hisilicon,hi6421-off-on-delay-us: <off_on_delay>
> +- hisilicon,hi6421-enable-time-us: <enable_time>

This lot are really not at all driver specific, they would apply to any
regulator which is able to use helpers like the regmap ones.  I have to
say I'm not a fan of dumping this much stuff into device tree (since it
means that the driver isn't able to figure out much of the hardware by
itself) but if we are going to do this then we should just define this
as a generic binding which any regulator can reuse rather than as one
that is specific to a particular driver.

> +Properties defined by the standard binding for regulators: (See regulator.txt)
> +- regulator-name:
> +- regulator-min-microvolt:
> +- regulator-max-microvolt:
> +- regulator-boot-on:
> +- regulator-always-on:

No need to list the properties, any new ones added in the core ought to
be supported.

> +Optional properties:
> +- hisilicon,hi6421-eco-microamp: maximum current allowed in ECO mode (in uA)

It's *possible* that whatever "ECO mode" is might be device specific but
I rather suspect it's just a low power mode in which case it's fairly
generic (the code certainly looks that way).

> +++ b/drivers/regulator/Kconfig
> @@ -514,5 +514,11 @@ config REGULATOR_AS3711
>  	  This driver provides support for the voltage regulators on the
>  	  AS3711 PMIC
>  
> -endif
> +config REGULATOR_HI6421
> +	tristate "HiSilicon Hi6421 PMIC voltage regulator support"
> +	depends on MFD_HI6421_PMIC && OF
> +	help
> +	  This driver provides support for the voltage regulators on the
> +	  HiSilicon Hi6421 PMU / Codec IC.

Keep these files sorted as much as possible.

> +static DEFINE_MUTEX(enable_mutex);
> +struct timeval last_enabled;

No globals.  Coordinate through the MFD if you have to.

> +/* helper function to ensure when it returns it is at least 'delay_us'
> + * microseconds after 'since'.
> + */
> +static void ensured_time_after(struct timeval since, u32 delay_us)
> +{

This should obviously be in generic code if it's needed.  Should also be
"ensure" not "ensured".

> +	}
> +	return;
> +}

The return is pointless.

> +static int hi6421_regulator_is_enabled(struct regulator_dev *dev)
> +{

If you convert the core to regmap almost all of the code in this driver
can be replaced with regmap helpers.

> +static int hi6421_regulator_enable(struct regulator_dev *dev)
> +{
> +	struct hi6421_regulator *sreg = rdev_get_drvdata(dev);
> +	struct hi6421_pmic *pmic = rdev_to_pmic(dev);
> +
> +	/* keep a distance of off_on_delay from last time disabled */
> +	ensured_time_after(sreg->last_off_time, sreg->off_on_delay);
> +
> +	/* cannot enable more than one regulator at one time */
> +	mutex_lock(&enable_mutex);
> +	ensured_time_after(last_enabled, HI6421_REGS_ENA_PROTECT_TIME);
> +
> +	/* set enable register */
> +	hi6421_pmic_rmw(pmic, sreg->register_info.ctrl_reg,
> +				sreg->register_info.enable_mask,
> +				sreg->register_info.enable_mask);
> +
> +	do_gettimeofday(&last_enabled);
> +	mutex_unlock(&enable_mutex);

What exactly is the constraint here?  It's very unusual for a regulator
to have a constraint requiring a delay between power off and power on,
or for there to be restrictions on powering up multiple regulators
simulataneously.  If there are such constraints why don't they also
affect voltage changes?

> +static int hi6421_regulator_ldo_set_voltage(struct regulator_dev *dev,
> +				int min_uV, int max_uV, unsigned *selector)
> +{

Implement map_voltage() and set_voltage_sel().

> +unsigned int hi6421_regulator_get_optimum_mode(struct regulator_dev *dev,
> +			int input_uV, int output_uV, int load_uA)
> +{
> +	struct hi6421_regulator *sreg = rdev_get_drvdata(dev);
> +
> +	if ((load_uA == 0) || (load_uA > sreg->eco_uA))
> +		return REGULATOR_MODE_NORMAL;
> +	else
> +		return REGULATOR_MODE_IDLE;

Why normal mode if the load is zero?

> +static const struct hi6421_regulator hi6421_regulator_ldo = {
> +	.rdesc = {
> +	.ops = &hi6421_ldo_rops,
> +		.type = REGULATOR_VOLTAGE,
> +		.owner = THIS_MODULE,
> +		},

Coding style.

> +	sreg->name = initdata->constraints.name;
> +	rdesc = &sreg->rdesc;
> +	rdesc->name = sreg->name;

You're abusing the name property here.  The descriptor name should
reflect the name the device has in silicon but you're using the
constraints name which should reflect the name of the supply on the
board.

> +	rdesc->min_uV = initdata->constraints.min_uV;

Similarly this should reflect the silicon not the board.

> +hi6421_probe_end:
> +	if (ret)
> +		kfree(sreg);

Use devm.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]