Re: Re: Including empty regulator nodes in axp209.dtsi is a BAD idea

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3627 bytes --]

On Fri, Jan 16, 2015 at 12:48:53AM +0800, Chen-Yu Tsai wrote:
> On Wed, Jan 14, 2015 at 6:18 PM, Lars Doelle <lars.doelle-RMqqONVpFt+ELgA04lAiVw@public.gmane.org> wrote:
> > On Tuesday, January 13, 2015 17:46:06 Maxime Ripard wrote:
> >> On Tue, Jan 13, 2015 at 10:39:01AM +0100, Hans de Goede wrote:
> >> > Hi ChenYu, Maxime,
> >> >
> >> > During the review of a few dts files for new boards Maxime asked me to use
> >> > axp209.dtsi to avoid the standard axp209 "boilerplate" present in most
> >> > boards using the axp209 pmic.
> >> >
> >> > But axp209.dtsi includes empty regulator nodes, e.g. :
> >> >
> >> >                 reg_dcdc3: dcdc3 {
> >> >                         regulator-name = "dcdc3";
> >> >                 };
> >> >
> >> > This is a BAD idea, the presence of these empty nodes causes the
> >> > axp20x-regulator driver to actually register regulators for them,
> >> > and then on late_init the regulator subsys turns them off, since
> >> > they have absolutely no constraints set (nor users registered)
> >> > and the regulator subsys assumes that when devicetree is used their
> >> > is always a compete set of constraints and that thus turning them
> >> > off is safe.
> >> >
> >> > So when I switched to using axp209.dtsi for the bananapro.dts,
> >> > and booted the bananapro this is the last message I got from the
> >> > kernel while booting:
> >> >
> >> > [    2.314014] dcdc3: disabling
> >> >
> >> > And away went our DRAM power-supply, oops.
> >> >
> >> > So for dcdc2 (CPU) and dcdc3 (DRAM), the boilerplate
> >> > should contain reasonable constraints (eg the operating range
> >> > from the datasheet)
> >>
> >> Indeed.
> >
> > It is very unclear to me, what to with this dtsi as it is.
> >
> > Wrt. axp209, I have a specification like in sun7i-a20-olinuxino-lime2.dts.
> > If I include this dtsi, what does it help or how is it supposed to be of any
> > use?
> >
> > In particular, the following section
> >
> > |                       axp209: pmic@34 {
> > |                               compatible = "x-powers,axp209";
> > |                               reg = <0x34>;
> > |                               interrupt-parent = <&nmi_intc>;
> > |                               interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> >
> > |                               interrupt-controller;
> > |                               #interrupt-cells = <1>;
> >
> > |                               acin-supply = <&reg_axp_ipsout>;
> > |                               vin2-supply = <&reg_axp_ipsout>;
> > |                               vin3-supply = <&reg_axp_ipsout>;
> > |                               ldo24in-supply = <&reg_axp_ipsout>;
> > |                               ldo3in-supply = <&reg_axp_ipsout>;
> 
> We should not be using reg_axp_ipsout, as it is an unregulated supply,
> and can be left out.

Can it? Doesn't the regulator driver need to know the input voltage?

> > would be needed anyway as it is not in sun7i-a20.dtsi, followed by the
> > regulators, to switch them on. Now a dtsi would make sense, if it shortens
> > the device specific information.
> >
> > Should not the section above together with the full regulators be in axp209.dtsi
> > and only the regulators be mentioned in the boards dts, to switch them on? Isn't
> > this stuff that should be included in sun7i-a20.dtsi anyway, somehow?
> 
> It is entirely possible to make a board without using the PMIC, as Olimex
> has demonstrated. sun7i-a20 only describes the common SoC bits.

Or even with a different PMIC.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com