linux-kernel - Re: [PATCH 3/9] regulator: mt6380: Add support for MT6380

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]

Date:   Tue, 6 Jun 2017 19:22:24 +0100
From:   Mark Brown <broonie@...nel.org>
To:     sean.wang@...iatek.com
Cc:     robh+dt@...nel.org, matthias.bgg@...il.com, mark.rutland@....com,
        lgirdwood@...il.com, jamesjj.liao@...iatek.com,
        henryc.chen@...iatek.com, devicetree@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, chen.zhong@...iatek.com,
        chenglin.xu@...iatek.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/9] regulator: mt6380: Add support for MT6380

On Sat, Jun 03, 2017 at 01:55:44AM +0800, sean.wang@...iatek.com wrote:

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

This isn't really a get_status() operation - it's just showing the
status of the enable we set.  The get_status() operation is for hardware
that has a mechanism for reading back the current physical status of the
regulator, usually including things like if it's in regulation or not.

Also please write normal conditional statements, it helps people read
the code.

> +	ret = regmap_update_bits(rdev->regmap, info->modeset_reg,
> +				 info->modeset_mask, val);
> +
> +	if (regmap_read(rdev->regmap, info->modeset_reg, &reg_value) < 0) {
> +		dev_err(&rdev->dev, "Failed to read register value\n");
> +		return -EIO;
> +	}

Is I/O to the device unreliable for some reason?  If so this isn't great
handling for it...  also if there is an error from regmap_read() you
should return the error code.

> +	unsigned int mode;
> +	int ret;
> +	struct mt6380_regulator_info *info = rdev_get_drvdata(rdev);
> +
> +	if (!info->modeset_mask) {
> +		dev_err(&rdev->dev, "regulator %s doesn't support get_mode\n",
> +			info->desc.name);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(rdev->regmap, info->modeset_reg, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= info->modeset_mask;
> +	val >>= ffs(info->modeset_mask) - 1;
> +
> +	if (val & 0x1)
> +		mode = REGULATOR_MODE_STANDBY;
> +	else
> +		mode = REGULATOR_MODE_NORMAL;
> +
> +	return mode;
> +}

This won't initialize mode if the regulator is in force PWM mode.  It'd
be clearer and safer to just write a switch statement.

> +		/* Constrain board-specific capabilities according to what
> +		 * this driver and the chip itself can actually do.
> +		 */
> +		c = rdev->constraints;
> +		c->valid_modes_mask |= REGULATOR_MODE_NORMAL |
> +			REGULATOR_MODE_STANDBY | REGULATOR_MODE_FAST;

No, this is completely broken.  A regulator driver should *never* modify
constraints, if the constraints are broken the constraints are broken,
and the constraints will already have been applied when we return from
registering the regulator.

> +		c->valid_ops_mask |= REGULATOR_CHANGE_MODE;

> +static const struct of_device_id mt6380_of_match[] = {
> +	{ .compatible = "mediatek,mt6380-regulator", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mt6380_of_match);

Given that this driver is entirely specific to the parent PMIC there
should be no need for a separate compatible string, it's redundant.

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