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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200120190427.GO6852@sirena.org.uk>
Date:   Mon, 20 Jan 2020 19:04:27 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Wen Su <Wen.Su@...iatek.com>
Cc:     Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        devicetree@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-kernel@...ts.infradead.org, wsd_upstream@...iatek.com
Subject: Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)