Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator

From: Mark Brown <broonie@kernel.org>
To: Wen Su <Wen.Su@mediatek.com>
Cc: Lee Jones <lee.jones@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
	devicetree@vger.kernel.org, Liam Girdwood <lgirdwood@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-arm-kernel@lists.infradead.org, wsd_upstream@mediatek.com
Subject: Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator
Date: Mon, 20 Jan 2020 19:04:27 +0000	[thread overview]
Message-ID: <20200120190427.GO6852@sirena.org.uk> (raw)
In-Reply-To: <1579506450-21830-4-git-send-email-Wen.Su@mediatek.com>

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

On Mon, Jan 20, 2020 at 03:47:29PM +0800, Wen Su wrote:

This seems pretty good, a few comments below but they're fairly small
and should be easy to address:

> +static int mt6359_set_voltage_sel(struct regulator_dev *rdev,
> +				  unsigned int selector)
> +{
> +	int idx, ret;
> +	const u32 *pvol;
> +	struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	pvol = info->index_table;
> +
> +	idx = pvol[selector];
> +	ret = regmap_update_bits(rdev->regmap, info->desc.vsel_reg,
> +				 info->desc.vsel_mask,
> +				 idx << info->vsel_shift);
> +
> +	return ret;
> +}

This looks like you should be using regulator_list_voltage_table() and
associated functions, probably map_voltage_ascend() or _iterate() and
just a simple set_voltage_sel_regmap().

> +static int mt6359_get_status(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	u32 regval;
> +	struct mt6359_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	ret = regmap_read(rdev->regmap, info->status_reg, &regval);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev, "Failed to get enable reg: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return (regval & info->qi) ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF;

Please write normal conditionl statements rather than using the ternery
operator to improve legibility.

> +	switch (mode) {
> +	case REGULATOR_MODE_FAST:
> +		if (curr_mode == REGULATOR_MODE_IDLE) {
> +			WARN_ON(1);
> +			dev_notice(&rdev->dev,
> +				   "BUCK %s is LP mode, can't FPWM\n",
> +				   rdev->desc->name);
> +			return -EIO;

I'd expect the device to go out of low power mode then into force PWM
mode if it has to do that rather than reject the operation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]