linux-kernel - Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 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>] [day] [month] [year] [list]

Message-ID: <20141128153008.GS7712@sirena.org.uk>
Date:	Fri, 28 Nov 2014 15:30:08 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Flora Fu <flora.fu@...iatek.com>
Cc:	Rob Herring <robh+dt@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	linux-arm-kernel@...ts.infradead.org,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Russell King <linux@....linux.org.uk>,
	Grant Likely <grant.likely@...aro.org>,
	Santosh Shilimkar <santosh.shilimkar@...com>,
	Sandeep Nair <sandeep_n@...com>,
	Andy Gross <agross@...eaurora.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Stephen Warren <swarren@...dia.com>,
	Thierry Reding <treding@...dia.com>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Vladimir Murzin <vladimir.murzin@....com>,
	Ashwin Chaugule <ashwin.chaugule@...aro.org>,
	"Joe.C" <yingjoe.chen@...iatek.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, srv_heupstream@...iatek.com,
	Sascha Hauer <kernel@...gutronix.de>,
	Eddie Huang <eddie.huang@...iatek.com>,
	Dongdong Cheng <dongdong.cheng@...iatek.com>
Subject: Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 regulator

On Fri, Nov 28, 2014 at 11:54:34AM +0800, Flora Fu wrote:

> Add device tree for MT6397 regulators in mt8135.dtsi.
> 
> Signed-off-by: Flora Fu <flora.fu@...iatek.com>
> ---
>  arch/arm/boot/dts/mt8135.dtsi | 192 ++++++++++++++++++++++++++++++++++++++++++

This appears to be the DT fragment for a SoC but you are defining the
system integration for the PMIC.  That's bad, the PMIC is a separate
device so should be hooked up by the board using it.  If there's common
elements from a reference design they should be in their own .dtsi.

> +					mt6397_vsramca15_reg: buck_vsramca15 {
> +						regulator-name = "vsramca15";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;
> +						regulator-ramp-delay = <12500>;
> +						regulator-enable-ramp-delay = <115>;
> +						regulator-always-on;
> +						regulator-boot-on;

Why do these regulators have both a board specific ramp delay specified
and -always-on?  If they are always on presumably they never ramp at
runtime; if this is a generic parameter for the device it should be in
the driver.

Similarly why is boot_on being specified - we can read the startup state
for all these regulators?

> +					};
> +
> +					mt6397_vsramca7_reg: buck_vsramca7 {
> +						regulator-name = "vsramca7";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;

All these regulators seem to have exactly the same range specified which
looks awfully like it might be the maximum variability the regulator has
rather than the board specific range that's supported; the fact that
they appear to have no consumers that might vary the voltage is another
warning sign.  This is probably wrong, the constraints should be
whatever is verified good for the board.

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