Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM

From: Mark Brown <broonie@kernel.org>
To: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>,
	Lee Jones <lee.jones@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Josh Cartwright <joshc@codeaurora.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM
Date: Wed, 28 May 2014 17:55:29 +0100	[thread overview]
Message-ID: <20140528165529.GC5099@sirena.org.uk> (raw)
In-Reply-To: <1401211721-19712-4-git-send-email-bjorn.andersson@sonymobile.com>

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

On Tue, May 27, 2014 at 10:28:41AM -0700, Bjorn Andersson wrote:

> +static int rpm_reg_set_voltage(struct regulator_dev *rdev,
> +			       int min_uV, int max_uV,
> +			       unsigned *selector)
> +{
> +	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> +	const struct rpm_reg_parts *parts = vreg->parts;
> +	const struct request_member *req;
> +	int ret = 0;
> +	int sel;
> +	int val;
> +	int uV;
> +
> +	dev_dbg(vreg->dev, "set_voltage(%d, %d)\n", min_uV, max_uV);
> +
> +	if (parts->uV.mask == 0 && parts->mV.mask == 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Snap to the voltage to a supported level.
> +	 */

What is "snapping" a voltage?

> +	sel = regulator_map_voltage_linear_range(rdev, min_uV, max_uV);

Don't open code mapping the voltage, just implement set_voltage_sel().

> +	mutex_lock(&vreg->lock);
> +	if (parts->uV.mask)
> +		req = &parts->uV;
> +	else if (parts->mV.mask)
> +		req = &parts->mV;
> +	else
> +		req = &parts->enable_state;

Just implement separate operations for the regulators with different
register definitions.  It's going to simplify the code.

> +static int rpm_reg_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> +	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> +	const struct rpm_reg_parts *parts = vreg->parts;
> +	int max_mA = parts->ip.mask >> parts->ip.shift;
> +	int load_mA;
> +	int ret;
> +
> +	if (mode == REGULATOR_MODE_IDLE)
> +		load_mA = vreg->idle_uA / 1000;
> +	else
> +		load_mA = vreg->normal_uA / 1000;
> +
> +	if (load_mA > max_mA)
> +		load_mA = max_mA;
> +
> +	mutex_lock(&vreg->lock);
> +	ret = rpm_reg_write(vreg, &parts->ip, load_mA);
> +	if (!ret)
> +		vreg->mode = mode;
> +	mutex_unlock(&vreg->lock);

I'm not entirely sure what this is intended to do.  It looks like it's
doing something to do with current limits but it's a set_mode().  Some
documentation might help.  It should also be implementing only specific
modes and rejecting unsupported modes, if we do the same operation to
the device for two different modes that seems wrong.

> +static unsigned int rpm_reg_get_optimum_mode(struct regulator_dev *rdev,
> +					     int input_uV,
> +					     int output_uV,
> +					     int load_uA)
> +{
> +	struct qcom_rpm_reg *vreg = rdev_get_drvdata(rdev);
> +
> +	load_uA += vreg->load_bias_uA;
> +
> +	if (load_uA < vreg->hpm_threshold_uA) {
> +		vreg->idle_uA = load_uA;
> +		return REGULATOR_MODE_IDLE;
> +	} else {
> +		vreg->normal_uA = load_uA;
> +		return REGULATOR_MODE_NORMAL;
> +	}
> +}

This looks very broken - why are we storing anything here?  What is the
stored value supposed to do?

> +	if (vreg->parts->ip.mask) {
> +		initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
> +		initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> +		initdata->constraints.valid_modes_mask |= REGULATOR_MODE_NORMAL;
> +		initdata->constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;

No, this is just plain broken.  Constraints are set by the *board*, you
don't know if these settings are safe on any given board.

> +static int __init rpm_reg_init(void)
> +{
> +	return platform_driver_register(&rpm_reg_driver);
> +}
> +arch_initcall(rpm_reg_init);
> +
> +static void __exit rpm_reg_exit(void)
> +{
> +	platform_driver_unregister(&rpm_reg_driver);
> +}
> +module_exit(rpm_reg_exit)

module_platform_driver() or if you must bodge the init order at least
use subsys_initcall() like everyone else.

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