| Message ID | 20180524055752.GE4249@localhost.localdomain (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, May 24, 2018 at 08:57:52AM +0300, Matti Vaittinen wrote: > +Required properties: > + - compatible: should be "rohm,bd71837-pmic". > + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7" The MFD is for a single device, there should be no need for compatibles on subfunctions.
Hello Mark, First of all, thank you for taking your time to check the patches. I do appreciate it. I find reading patches hard myself. > From: Mark Brown [broonie@kernel.org] > Sent: Thursday, May 24, 2018 5:01 PM > > > On Thu, May 24, 2018 at 08:57:52AM +0300, Matti Vaittinen wrote: > > > +Required properties: > > + - compatible: should be "rohm,bd71837-pmic". > > + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7" > > The MFD is for a single device, there should be no need for compatibles > on subfunctions. I will check this. I must admit I am not sure what is the de-facto mechanism for assigning the correct device-tree nodes to sub devices if compatibles are not used? I think I saw device-tree node name being used for regulators but how is it done for example with clk? I would be grateful if anyone could point me to right direction with this. Also, another thing I was wondering is how supply regulators should be handled? In this case the LDO5 is supplied by BUCK6 and LDO6 by BUCK7. From generic regulator bindings /Documentation/devicetree/bindings/regulator/regulator.txt I found statement: > - <name>-supply: phandle to the parent supply/regulator node and > Regulator Consumers: > Consumer nodes can reference one or more of its supplies/ > regulators using the below bindings. > > - <name>-supply: phandle to the regulator node > > These are the same bindings that a regulator in the above > example used to reference its own supply, in which case > ts just seen as a special case of a regulator being a > consumer itself. but I did not find handling for the supply properties from regulator core. Thus I ended up hard coding the supply relation in driver. This means that buck6 name must be fixed. Br, Matti Vaittinen-- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 24, 2018 at 05:30:57PM +0000, Vaittinen, Matti wrote: > > > On Thu, May 24, 2018 at 08:57:52AM +0300, Matti Vaittinen wrote: > > > > > +Required properties: > > > + - compatible: should be "rohm,bd71837-pmic". > > > + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7" > > > > The MFD is for a single device, there should be no need for compatibles > > on subfunctions. > > I will check this. I must admit I am not sure what is the de-facto mechanism > for assigning the correct device-tree nodes to sub devices if compatibles > are not used? I think I saw device-tree node name being used for regulators You can look at the regulators node within the parent device, you know that in Linux the parent device will be the MFD. Having a compatible string within the device makes no difference here. There's quite a few in tree examples of this. > Also, another thing I was wondering is how supply regulators should be > handled? In this case the LDO5 is supplied by BUCK6 and LDO6 by > BUCK7. > From generic regulator bindings > /Documentation/devicetree/bindings/regulator/regulator.txt > I found statement: > > - <name>-supply: phandle to the parent supply/regulator node None of that stuff uses compatible strings, just handle it as covered in the bindings.
On Thu, May 24, 2018 at 06:57:21PM +0100, Mark Brown wrote: > On Thu, May 24, 2018 at 05:30:57PM +0000, Vaittinen, Matti wrote: > > > > On Thu, May 24, 2018 at 08:57:52AM +0300, Matti Vaittinen wrote: > > > > > > > +Required properties: > > > > + - compatible: should be "rohm,bd71837-pmic". > > > > + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7" > > > > > > The MFD is for a single device, there should be no need for compatibles > > > on subfunctions. > > > > I will check this. I must admit I am not sure what is the de-facto mechanism > > for assigning the correct device-tree nodes to sub devices if compatibles > > are not used? I think I saw device-tree node name being used for regulators > > You can look at the regulators node within the parent device, you know > that in Linux the parent device will be the MFD. So I should parse the device-tree in MFD my driver in order to locate the regulators node? Isn't that somewhat like code dublication? If we rely on compatibles we can avoid device-tree parsing in MFD driver, right? An in-tree example of this is: Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt >Required properties: >- compatible: should be "sprd,sc27xx-regulator". //snip >Example: > regulators { > compatible = "sprd,sc27xx-regulator"; > > vddarm0: BUCK_CPU0 { > regulator-name = "vddarm0"; > regulator-min-microvolt = <400000>; drivers/mfd/sprd-sc27xx-spi.c > static const struct mfd_cell sprd_pmic_devs[] = { //snip > }, { > .name = "sc27xx-regulator", > .of_compatible = "sprd,sc27xx-regulator", > }, { //snip > }; and in probe just: > ret = devm_mfd_add_devices(&spi->dev, PLATFORM_DEVID_AUTO, > sprd_pmic_devs, ARRAY_SIZE(sprd_pmic_devs), > NULL, 0, > regmap_irq_get_domain(ddata->irq_data)); this looks clean to me and offloads the device-tree parsing completely to generic code. Wouldn't that be simpler approach that looking up the regulator node in MFD driver code? (I can do as you suggested but to me the approach used in sprd-sc27xx-spi.c makes sense) > > Also, another thing I was wondering is how supply regulators should be > > handled? In this case the LDO5 is supplied by BUCK6 and LDO6 by > > BUCK7. > > > From generic regulator bindings > > /Documentation/devicetree/bindings/regulator/regulator.txt > > I found statement: > > > > - <name>-supply: phandle to the parent supply/regulator node > > None of that stuff uses compatible strings, just handle it as covered in > the bindings. Sorry. I have not been clear with my question. This part was unrelated to compatible properties - I should have stated it in my previous mail. What I meant is that I tried out adding xxx-supply = <&buck6>; in LDO5 device tree node and expected that the regulator core code would take care of parsing this from device-tree and adding the supply information to LDO5. This was not done and I did not fing parsing for *-supply from drivers/regulator/of_regulator.c. So I was wondering if I am missing something? I guess the *-supply properties in device-tree for BD71837 regulators are now ignored. Should the supply parsing be added in drivers/regulator/of_regulator.c - or have I simply misunderstood something? Anyways, I ended up hard coding: .supply_name = "buck6" in LDO5 regulator_desc before passing the desc to regulator_register(). This works but it means the buck6 name must be fixed to "buck6". Br, Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 25, 2018 at 08:54:30AM +0300, Matti Vaittinen wrote: > On Thu, May 24, 2018 at 06:57:21PM +0100, Mark Brown wrote: > > You can look at the regulators node within the parent device, you know > > that in Linux the parent device will be the MFD. > So I should parse the device-tree in MFD my driver in order to locate > the regulators node? Isn't that somewhat like code dublication? If we > rely on compatibles we can avoid device-tree parsing in MFD driver, No, there's no need to do this - the child can just look at the of_node of the parent since it can never be instantiated otherwise. > right? An in-tree example of this is: There are some bad examples (and some where the same regulators can get used with multiple different parents) but that's no reason not to follow good practice.
On Fri, May 25, 2018 at 11:24:58AM +0100, Mark Brown wrote: > On Fri, May 25, 2018 at 08:54:30AM +0300, Matti Vaittinen wrote: > > On Thu, May 24, 2018 at 06:57:21PM +0100, Mark Brown wrote: > > > > You can look at the regulators node within the parent device, you know > > > that in Linux the parent device will be the MFD. > > > So I should parse the device-tree in MFD my driver in order to locate > > the regulators node? Isn't that somewhat like code dublication? If we > > rely on compatibles we can avoid device-tree parsing in MFD driver, > > No, there's no need to do this - the child can just look at the of_node > of the parent since it can never be instantiated otherwise. > > > right? An in-tree example of this is: > > There are some bad examples (and some where the same regulators can get > used with multiple different parents) but that's no reason not to follow > good practice. Fair enough. I guess you may still know regulator subsystem better than I do with my one month of experience ;) I'll follow your suggestion and cook-up new patches. Br, Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt new file mode 100644 index 000000000000..a43230143b66 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt @@ -0,0 +1,124 @@ +ROHM BD71837 Power Management Integrated Circuit (PMIC) regulator bindings + +BD71837MWV is a programmable Power Management +IC (PMIC) for powering single-core, dual-core, and +quad-core SoC’s such as NXP-i.MX 8M. It is optimized +for low BOM cost and compact solution footprint. It +integrates 8 Buck regulators and 7 LDO’s to provide all +the power rails required by the SoC and the commonly +used peripherals. + +Required properties: + - compatible: should be "rohm,bd71837-pmic". + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7" + +List of regulators provided by this controller. BD71837 regulators node +should be sub node of the BD71837 MFD node. See BD71837 MFD bindings at +Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt +Regulator nodes should be named to BUCK_<number> and LDO_<number>. 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: +BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, BUCK7, BUCK8 +LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7 + +Optional properties: +- Any optional property defined in bindings/regulator/regulator.txt + +Example: +regulators { + compatible = "rohm,bd71837-pmic"; + #address-cells = <1>; + #size-cells = <0>; + buck1: BUCK1 { + pmic-supply = <&vmmcsd_fixed>; + regulator-name = "buck1"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + regulator-ramp-delay = <1250>; + }; + buck2: BUCK2 { + regulator-name = "buck2"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + regulator-always-on; + regulator-ramp-delay = <1250>; + }; + buck3: BUCK3 { + regulator-name = "buck3"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + }; + buck4: BUCK4 { + regulator-name = "buck4"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + }; + buck5: BUCK5 { + regulator-name = "buck5"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1350000>; + regulator-boot-on; + }; + buck6: BUCK6 { + regulator-name = "buck6"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3300000>; + }; + buck7: BUCK7 { + regulator-name = "buck7"; + regulator-min-microvolt = <1605000>; + regulator-max-microvolt = <1995000>; + }; + buck8: BUCK8 { + regulator-name = "buck8"; + regulator-min-microvolt = <800000>; + regulator-max-microvolt = <1400000>; + }; + + ldo1: LDO1 { + regulator-name = "ldo1"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + }; + ldo2: LDO2 { + regulator-name = "ldo2"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <900000>; + regulator-boot-on; + }; + ldo3: LDO3 { + regulator-name = "ldo3"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; + ldo4: LDO4 { + regulator-name = "ldo4"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1800000>; + }; + ldo5: LDO5 { + regulator-name = "ldo5"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; + ldo6: LDO6 { + regulator-name = "ldo6"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1800000>; + }; + ldo7_reg: LDO7 { + regulator-name = "ldo7"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; +}; + +
Document devicetree bindings for ROHM BD71837 PMIC regulators. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- .../bindings/regulator/rohm,bd71837-regulator.txt | 124 +++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt