diff mbox series

[v5,1/2] dt-bindings: regulator: add support for MT6392

Message ID 20201024200304.1427864-1-fparent@baylibre.com
State Not Applicable
Headers show
Series [v5,1/2] dt-bindings: regulator: add support for MT6392 | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 220 lines checked

Commit Message

Fabien Parent Oct. 24, 2020, 8:03 p.m. UTC
Add binding documentation of the regulator for MT6392 SoCs.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

v5:
	* No change
v4:
	* No change
v3:
	* No change
v2:
	* Use 'pmic' as node name for the pmic.
	* Use 'regulators' as node name for the regulators
	* use dash instead of underscore for regulator's node names.

 .../bindings/regulator/mt6392-regulator.txt   | 220 ++++++++++++++++++
 1 file changed, 220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mt6392-regulator.txt

Comments

Mark Brown Oct. 26, 2020, 12:13 p.m. UTC | #1
On Sat, Oct 24, 2020 at 10:03:03PM +0200, Fabien Parent wrote:

> +Required properties:
> +- compatible: "mediatek,mt6392-regulator"

This is no longer used by the driver, should be unneeded and therefore
should be removed.

> +- mt6392regulator: List of regulators provided by this controller. It is named

This property doesn't seem to appear anywhere - there's regulators, the
collection of subnodes for each individual regulator which I think is
what is referenced here, but nothing called mt6392regulator.
Fabien Parent Oct. 26, 2020, 5:18 p.m. UTC | #2
Hi Mark,

On Mon, Oct 26, 2020 at 1:13 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, Oct 24, 2020 at 10:03:03PM +0200, Fabien Parent wrote:
>
> > +Required properties:
> > +- compatible: "mediatek,mt6392-regulator"
>
> This is no longer used by the driver, should be unneeded and therefore
> should be removed.

It is not used by the driver but it will be used by the MFD driver [0]
like this:
static const struct mfd_cell mt6392_devs[] = {
    {
        [snip]
    }, {
        [snip]
    }, {
        .name = "mt6392-regulator",
        .of_compatible = "mediatek,mt6392-regulator"
    }, {
        [snip]
    },
};

[0] drivers/mfd/mt6397-core.c

>
> > +- mt6392regulator: List of regulators provided by this controller. It is named
>
> This property doesn't seem to appear anywhere - there's regulators, the
> collection of subnodes for each individual regulator which I think is
> what is referenced here, but nothing called mt6392regulator.

Indeed, I will fix it in the next rev.
Mark Brown Oct. 26, 2020, 5:24 p.m. UTC | #3
On Mon, Oct 26, 2020 at 06:18:35PM +0100, Fabien Parent wrote:
> On Mon, Oct 26, 2020 at 1:13 PM Mark Brown <broonie@kernel.org> wrote:

> > On Sat, Oct 24, 2020 at 10:03:03PM +0200, Fabien Parent wrote:

> > > +Required properties:
> > > +- compatible: "mediatek,mt6392-regulator"

> > This is no longer used by the driver, should be unneeded and therefore
> > should be removed.

> It is not used by the driver but it will be used by the MFD driver [0]
> like this:
> static const struct mfd_cell mt6392_devs[] = {
>     {
>         [snip]
>     }, {
>         [snip]
>     }, {
>         .name = "mt6392-regulator",
>         .of_compatible = "mediatek,mt6392-regulator"

This is still unneeded, it's just a reflection of Linux implementation
details and should be removed.   The MFD can just register the child
without supplying a compatible and things will continue to work just as
well.
Fabien Parent Oct. 26, 2020, 6:38 p.m. UTC | #4
Hi Mark,

On Mon, Oct 26, 2020 at 6:24 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Oct 26, 2020 at 06:18:35PM +0100, Fabien Parent wrote:
> > On Mon, Oct 26, 2020 at 1:13 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > On Sat, Oct 24, 2020 at 10:03:03PM +0200, Fabien Parent wrote:
>
> > > > +Required properties:
> > > > +- compatible: "mediatek,mt6392-regulator"
>
> > > This is no longer used by the driver, should be unneeded and therefore
> > > should be removed.
>
> > It is not used by the driver but it will be used by the MFD driver [0]
> > like this:
> > static const struct mfd_cell mt6392_devs[] = {
> >     {
> >         [snip]
> >     }, {
> >         [snip]
> >     }, {
> >         .name = "mt6392-regulator",
> >         .of_compatible = "mediatek,mt6392-regulator"
>
> This is still unneeded, it's just a reflection of Linux implementation
> details and should be removed.   The MFD can just register the child
> without supplying a compatible and things will continue to work just as
> well.

I'm not exactly sure how it is supposed to work. mfd_add_devices seems
to register devices based on of_compatible or acpi_match from the
mfd_cell. This platform does not have ACPI so I don't understand how
the regulator driver would probe without this line. Anyway I tried to
remove the lines below in the MFD driver and the device tree and the
boot of the board failed because the regulator driver didn't probe.
Any help to get me understand how it should work without this line
would be helpful, thanks.


                regulators {
-                       compatible = "mediatek,mt6392-regulator";
-
                        mt6392_vproc_reg: buck-vproc {


@@ -135,7 +135,6 @@ static const struct mfd_cell mt6392_devs[] = {
                .of_compatible = "mediatek,mt6392-keys"
        }, {
                .name = "mt6392-regulator",
-               .of_compatible = "mediatek,mt6392-regulator"
        }, {
Mark Brown Oct. 26, 2020, 8:36 p.m. UTC | #5
On Mon, Oct 26, 2020 at 07:38:14PM +0100, Fabien Parent wrote:
> On Mon, Oct 26, 2020 at 6:24 PM Mark Brown <broonie@kernel.org> wrote:

> > >         .name = "mt6392-regulator",
> > >         .of_compatible = "mediatek,mt6392-regulator"

> > This is still unneeded, it's just a reflection of Linux implementation
> > details and should be removed.   The MFD can just register the child
> > without supplying a compatible and things will continue to work just as
> > well.

> I'm not exactly sure how it is supposed to work. mfd_add_devices seems
> to register devices based on of_compatible or acpi_match from the
> mfd_cell. This platform does not have ACPI so I don't understand how

It should also support unconditionally registering devices, if it no
longer does so that's a regression in the framework which should be
fixed.  Looking at mfd_add_devices() I can't see an issue though, both
ACPI and DT information is optional - the entire DT section in
mfd_add_device() will be skipped if no of_compatible is specified in the
cell.  Are you *sure* that the regulator driver isn't running?
Fabien Parent Oct. 27, 2020, 9:16 p.m. UTC | #6
Hi Mark,

On Mon, Oct 26, 2020 at 9:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Oct 26, 2020 at 07:38:14PM +0100, Fabien Parent wrote:
> > On Mon, Oct 26, 2020 at 6:24 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > >         .name = "mt6392-regulator",
> > > >         .of_compatible = "mediatek,mt6392-regulator"
>
> > > This is still unneeded, it's just a reflection of Linux implementation
> > > details and should be removed.   The MFD can just register the child
> > > without supplying a compatible and things will continue to work just as
> > > well.
>
> > I'm not exactly sure how it is supposed to work. mfd_add_devices seems
> > to register devices based on of_compatible or acpi_match from the
> > mfd_cell. This platform does not have ACPI so I don't understand how
>
> It should also support unconditionally registering devices, if it no
> longer does so that's a regression in the framework which should be
> fixed.  Looking at mfd_add_devices() I can't see an issue though, both
> ACPI and DT information is optional - the entire DT section in
> mfd_add_device() will be skipped if no of_compatible is specified in the
> cell.  Are you *sure* that the regulator driver isn't running?

You are correct, the regulator driver is running and probes
successfully. From my investigation it seems the failure when removing
the compatible string from the MFD and the DTS is because the
regulator driver does not have a of_node matched since the compatible
is gone. Because of that all the regulators registered by the driver
are not linked to the regulator definitions in the device tree. And
all the drivers that tries to acquire a regulator get -EPROBE_DEFER
because of it.
Mark Brown Oct. 28, 2020, 12:32 p.m. UTC | #7
On Tue, Oct 27, 2020 at 10:16:22PM +0100, Fabien Parent wrote:

> You are correct, the regulator driver is running and probes
> successfully. From my investigation it seems the failure when removing
> the compatible string from the MFD and the DTS is because the
> regulator driver does not have a of_node matched since the compatible
> is gone. Because of that all the regulators registered by the driver
> are not linked to the regulator definitions in the device tree. And
> all the drivers that tries to acquire a regulator get -EPROBE_DEFER
> because of it.

You should be using the of_node from the parent device to find the
regulators set, look at how other drivers do this.
Fabien Parent Oct. 28, 2020, 1:14 p.m. UTC | #8
On Wed, Oct 28, 2020 at 1:33 PM Mark Brown <broonie@kernel.org> wrote:
>
> You should be using the of_node from the parent device to find the
> regulators set, look at how other drivers do this.

Thanks for the help. I got it to work. I will send a new revision of
the patches.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
new file mode 100644
index 000000000000..d03c0707fabc
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mt6392-regulator.txt
@@ -0,0 +1,220 @@ 
+Mediatek MT6392 Regulator
+
+Required properties:
+- compatible: "mediatek,mt6392-regulator"
+- mt6392regulator: List of regulators provided by this controller. It is named
+  according to its regulator type, buck_<name> and ldo_<name>.
+  The definition for each of these nodes is defined using the standard binding
+  for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
+
+The valid names for regulators are::
+BUCK:
+  buck_vproc, buck_vsys, buck_vcore
+LDO:
+  ldo_vxo22, ldo_vaud22, ldo_vcama, ldo_vaud28, ldo_vadc18, ldo_vcn35,
+  ldo_vio28. ldo_vusb, ldo_vmc, ldo_vmch, ldo_vemc3v3, ldo_vgp1, ldo_vgp2,
+  ldo_vcn18, ldo_vcamaf, ldo_vm, ldo_vio18, ldo_vcamd, ldo_vcamio, ldo_vm25,
+  ldo_vefuse
+
+Example:
+	pmic {
+		compatible = "mediatek,mt6392", "mediatek,mt6323";
+		mediatek,system-power-controller;
+
+		regulator {
+			compatible = "mediatek,mt6392-regulator";
+
+			mt6392_vproc_reg: buck-vproc {
+				regulator-name = "buck_vproc";
+				regulator-min-microvolt = < 700000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <12500>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vsys_reg: buck-vsys {
+				regulator-name = "buck_vsys";
+				regulator-min-microvolt = <1400000>;
+				regulator-max-microvolt = <2987500>;
+				regulator-ramp-delay = <25000>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vcore_reg: buck-vcore {
+				regulator-name = "buck_vcore";
+				regulator-min-microvolt = < 700000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-ramp-delay = <12500>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vxo22_reg: ldo-vxo22 {
+				regulator-name = "ldo_vxo22";
+				regulator-min-microvolt = <2200000>;
+				regulator-max-microvolt = <2200000>;
+				regulator-enable-ramp-delay = <110>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vaud22_reg: ldo-vaud22 {
+				regulator-name = "ldo_vaud22";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <2200000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vcama_reg: ldo-vcama {
+				regulator-name = "ldo_vcama";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vaud28_reg: ldo-vaud28 {
+				regulator-name = "ldo_vaud28";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vadc18_reg: ldo-vadc18 {
+				regulator-name = "ldo_vadc18";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vcn35_reg: ldo-vcn35 {
+				regulator-name = "ldo_vcn35";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3600000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vio28_reg: ldo-vio28 {
+				regulator-name = "ldo_vio28";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vusb_reg: ldo-vusb {
+				regulator-name = "ldo_vusb";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vmc_reg: ldo-vmc {
+				regulator-name = "ldo_vmc";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-boot-on;
+			};
+
+			mt6392_vmch_reg: ldo-vmch {
+				regulator-name = "ldo_vmch";
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-boot-on;
+			};
+
+			mt6392_vemc3v3_reg: ldo-vemc3v3 {
+				regulator-name = "ldo_vemc3v3";
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-boot-on;
+			};
+
+			mt6392_vgp1_reg: ldo-vgp1 {
+				regulator-name = "ldo_vgp1";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vgp2_reg: ldo-vgp2 {
+				regulator-name = "ldo_vgp2";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vcn18_reg: ldo-vcn18 {
+				regulator-name = "ldo_vcn18";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vcamaf_reg: ldo-vcamaf {
+				regulator-name = "ldo_vcamaf";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vm_reg: ldo-vm {
+				regulator-name = "ldo_vm";
+				regulator-min-microvolt = <1240000>;
+				regulator-max-microvolt = <1390000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vio18_reg: ldo-vio18 {
+				regulator-name = "ldo_vio18";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-enable-ramp-delay = <264>;
+				regulator-always-on;
+				regulator-boot-on;
+			};
+
+			mt6392_vcamd_reg: ldo-vcamd {
+				regulator-name = "ldo_vcamd";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vcamio_reg: ldo-vcamio {
+				regulator-name = "ldo_vcamio";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vm25_reg: ldo-vm25 {
+				regulator-name = "ldo_vm25";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+
+			mt6392_vefuse_reg: ldo-vefuse {
+				regulator-name = "ldo_vefuse";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <2000000>;
+				regulator-enable-ramp-delay = <264>;
+			};
+		};
+	};