linux-kernel - Re: [PATCH V1] regulator: pv88060: new regulator driver

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] [day] [month] [year] [list]

Message-ID: <20151117185030.GD31303@sirena.org.uk>
Date:	Tue, 17 Nov 2015 18:50:30 +0000
From:	Mark Brown <broonie@...nel.org>
To:	James Ban <James.Ban.opensource@...semi.com>
Cc:	LINUXKERNEL <linux-kernel@...r.kernel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	David Dajun Chen <david.chen@...semi.com>,
	Support Opensource <support.opensource@...semi.com>
Subject: Re: [PATCH V1] regulator: pv88060: new regulator driver

On Tue, Nov 17, 2015 at 11:25:21AM +0900, James Ban wrote:

A couple of minor points but overall this looks good:

> +	node = of_get_child_by_name(dev->of_node, "regulators");
> +	if (!node) {
> +		dev_err(dev, "regulators node not found\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	num = of_regulator_match(dev, node, pv88060_matches,
> +				 ARRAY_SIZE(pv88060_matches));

Don't manually parse the DT, use the core support and set of_match and
regulators_node in the regulator_desc.

> +		if (i == 0) {
> +			pv88060_matches[i].init_data->constraints
> +				.valid_modes_mask
> +				|= REGULATOR_MODE_FAST
> +				| REGULATOR_MODE_NORMAL
> +				| REGULATOR_MODE_STANDBY;
> +			pv88060_matches[i].init_data->constraints.valid_ops_mask
> +				|=	REGULATOR_CHANGE_MODE;
> +		}

The driver should never modify the constraints that are passed in - the
whole point with constraints is to allow the system to set up what is
safe on that particular system.

> +	if (!chip->pdata)
> +		chip->pdata = pv88060_parse_regulators_dt(chip->dev);
> +
> +	if (IS_ERR(chip->pdata)) {
> +		dev_err(chip->dev, "No regulators defined for the platform\n");
> +		return PTR_ERR(chip->pdata);
> +	}

This should not be an error, the driver should just instantiate all
regulators the device has and at least let people read back the current
state.

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