| Message ID | 20201024200304.1427864-1-fparent@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: regulator: add support for MT6392 | expand |
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.
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.
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.
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" }, {
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?
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.
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.
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 --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>; + }; + }; + };