linux-kernel - Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

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, 20 Oct 2015 12:31:28 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	"Andrew F. Davis" <afd@...com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Brown <broonie@...nel.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Grygorii Strashko <grygorii.strashko@...com>,
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the
 TPS65086 PMIC

On Mon, 19 Oct 2015, Andrew F. Davis wrote:

> On 10/19/2015 10:21 AM, Lee Jones wrote:
> >On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >
> >>On 10/19/2015 04:13 AM, Lee Jones wrote:
> >>>On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >>>
> >>>>The TPS65086 PMIC contains several regulators and a GPO controller.
> >>>>Add bindings for the TPS65086 PMIC.
> >>>>
> >>>>Signed-off-by: Andrew F. Davis <afd@...com>
> >>>>---
> >>>>  .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
> >>>>  Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
> >>>>  .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
> >>>
> >>>Please split these up into separate patches.
> >>>
> >>>There is no functional reason to bundle them up.
> >>>
> >>
> >>ACK
> >>
> >>>>  3 files changed, 99 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
> >>>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
> >>>
> >>>[...]
> >>>
> >>>>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>new file mode 100644
> >>>>index 0000000..4b6aeb4
> >>>>--- /dev/null
> >>>>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>@@ -0,0 +1,46 @@
> >>>>+* TPS65086 Power Management Integrated Circuit bindings
> >>>>+
> >>>>+Required properties:
> >>>>+ - compatible		: Should be "ti,tps65086".
> >>>
> >>>Any indication that it's a PMIC?
> >>>
> >>
> >>In the compatible string?
> >
> >Ya.
> >
> 
> Not sure what you mean then?, no one else seems to be doing that,
> "xx,xxxxxxx-pmic" is usually used for matching the regulator node,
> not the device itself.

Either the driver is MFD is the PMIC or it's not.

If it is, the compatible should reflect that, if isn't not then the
description in the header comment and the one above is not correct.

IMO, 'pmic' should not be used in the regulator compatible strings, as
it's a general description of the overall device.  The regulators are
just a component of that device.

> >>>>+ - reg			: Slave address.
> >>>
> >>>I2C/SPI?
> >>>
> >>
> >>ACK
> >>
> >>>>+ - interrupt-parent	: The parent interrupt controller.
> >>>
> >>>Phandled to ...
> >>>
> >>
> >>ACK
> >>
> >>>>+ - interrupts		: The interrupt line the device is connected to.
> >>>>+ - interrupt-controller	: Marks the device node as an interrupt controller.
> >>>>+ - #interrupt-cells	: The number of cells to describe an IRQ, this
> >>>>+			  should be 2. The first cell is the IRQ number.
> >>>>+			  The second cell is the flags, encoded as the trigger
> >>>>+			  masks from ../interrupt-controller/interrupts.txt.
> >>>
> >>>Masks?  What masks?
> >>>
> >>>Best to make a link to the header where the flags are defined here.
> >>>
> >>
> >>ACK
> >>
> >>>>+Additional nodes defined in:
> >>>>+ - Regulators	: ../regulator/tps65086-regulator.txt.
> >>>>+ - GPIO		: ../gpio/gpio-tps65086.txt.
> >>>
> >>>I'd suggest removing the full stops from all of the lines above.
> >>>
> >>>Just treat them as bullet points like we normally do.
> >>>
> >>
> >>ACK
> >>
> >>>>+Example:
> >>>>+
> >>>>+	pmic: tps65086@5e {
> >>>>+		compatible = "ti,tps65086";
> >>>>+		reg = <0x5e>;
> >>>>+		interrupt-parent = <&gpio1>;
> >>>>+		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> >>>>+		interrupt-controller;
> >>>>+		#interrupt-cells = <2>;
> >>>>+
> >>>>+		regulators {
> >>>>+			compatible = "ti,tps65086-regulator";
> >>>>+
> >>>>+			buck1 {
> >>>>+				regulator-name = "vcc1";
> >>>>+				regulator-min-microvolt = <1600000>;
> >>>>+				regulator-max-microvolt = <1600000>;
> >>>>+				regulator-boot-on;
> >>>>+				ti,regulator-decay;
> >>>>+				ti,regulator-step-size-25mv;
> >>>>+			};
> >>>>+		};
> >>>>+
> >>>>+		gpio4: tps65086_gpio {
> >>>>+			compatible = "ti,tps65086-gpio";
> >>>>+			gpio-controller;
> >>>>+			#gpio-cells = <2>;
> >>>>+		};
> >>>>+	};
> >>>
> >>>[...]
> >>>
> >>
> >
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/