* [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device @ 2015-03-03 4:25 Bjorn Andersson 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson ` (3 more replies) 0 siblings, 4 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 4:25 UTC (permalink / raw) To: Chanwoo Choi, Ian Campbell, Krzysztof Kozlowski, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross Cc: Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm Stephen Boyd pointed out that the current design of the Qualcomm RPM and regulator driver consumes 12-20kB of ram just for the platform_device structs. This series starts with a new revision of the dt binding documentation for the rpm regulators, it then extend the of_parse_cb callback to support changing the constraints flags during register of a regulator, refactor the qcom_rpm-regulator driver to move all custom parse code to a function suitable for usage as of_parse_cb. The final patch defines the tables of registers and change the probe function to register the appropriate regulators based on pmic. As Stephen pointed out in his PATCH/RFC/argument [1], this gives a more accurate representation of input supplies, as they are now named as in the specification. Note that for platforms with multiple pmics (e.g. 8660 and 8974) will have multiple regulator subnodes to the rpm node - something that will be clearer with this binding than the previously suggested. [1] https://lkml.org/lkml/2015/2/26/713 Bjorn Andersson (4): mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes regulator: core: Expose init_data to of_parse_cb regulator: qcom: Refactor of-parsing code regulator: qcom: Rework to single platform device Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 201 +++++++++++++- drivers/regulator/max77686.c | 3 +- drivers/regulator/of_regulator.c | 2 +- drivers/regulator/qcom_rpm-regulator.c | 290 ++++++++++++++------- include/linux/regulator/driver.h | 3 +- 5 files changed, 389 insertions(+), 110 deletions(-) -- 1.8.2.2 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 4:25 [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device Bjorn Andersson @ 2015-03-03 4:25 ` Bjorn Andersson 2015-03-03 12:47 ` Mark Brown 2015-03-03 18:53 ` Stephen Boyd 2015-03-03 4:25 ` [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb Bjorn Andersson ` (2 subsequent siblings) 3 siblings, 2 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 4:25 UTC (permalink / raw) To: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- As the previously accepted binding didn't cover any functionality but registering the mfd core there is no users, hence it's fine to drop the two required fields at this time. Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 201 +++++++++++++++++++-- 1 file changed, 189 insertions(+), 12 deletions(-) diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt index 85e3198..92bcd19 100644 --- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt +++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt @@ -31,16 +31,6 @@ frequencies. Value type: <string-array> Definition: must be the three strings "ack", "err" and "wakeup", in order -- #address-cells: - Usage: required - Value type: <u32> - Definition: must be 1 - -- #size-cells: - Usage: required - Value type: <u32> - Definition: must be 0 - - qcom,ipc: Usage: required Value type: <prop-encoded-array> @@ -52,6 +42,172 @@ frequencies. - u32 representing the ipc bit within the register += SUBNODES + +The RPM exposes resources to its subnodes. The below bindings specify the set +of valid subnodes that can operate on these resources. + +== Regulators + +Regulator notes are identified by their compatible: + +- compatible: + Usage: required + Value type: <string> + Definition: must be one of: + "qcom,rpm-pm8058-regulators" + "qcom,rpm-pm8901-regulators" + "qcom,rpm-pm8921-regulators" + +- vdd_l0_l1_lvs-supply: +- vdd_l10-supply: +- vdd_l13_l16-supply: +- vdd_l14_l15-supply: +- vdd_l17_l18-supply: +- vdd_l19_l20-supply: +- vdd_l21-supply: +- vdd_l22-supply: +- vdd_l23_l24_l25-supply: +- vdd_l2_l11_l12-supply: +- vdd_l3_l4_l5-supply: +- vdd_l6_l7-supply: +- vdd_l8-supply: +- vdd_l9-supply: +- vdd_ncp-supply: +- vdd_s0-supply: +- vdd_s1-supply: +- vdd_s2-supply: +- vdd_s3-supply: +- vdd_s4-supply: + Usage: optional (pm8058 only) + Value type: <phandle> + Definition: reference to regulator supplying the input pin, as + described in the data sheet + +- vdd_l0-supply: +- vdd_l1-supply: +- vdd_l2-supply: +- vdd_l3-supply: +- vdd_l4-supply: +- vdd_l5-supply: +- vdd_l6-supply: +- vdd_s0-supply: +- vdd_s1-supply: +- vdd_s2-supply: +- vdd_s3-supply: +- vdd_s4-supply: +- lvs0_in-supply: +- lvs1_in-supply: +- lvs2_in-supply: +- lvs3_in-supply: +- mvs_in-supply: + Usage: optional (pm8901 only) + Value type: <phandle> + Definition: reference to regulator supplying the input pin, as + described in the data sheet + +- vin_l1_l2_l12_l18-supply: +- vin_l24-supply: +- vin_l25-supply: +- vin_l27-supply: +- vin_l28-supply: +- vin_lvs_1_3_6-supply: +- vin_lvs2-supply: +- vin_lvs_4_5_7-supply: +- vin_ncp-supply: + Usage: optional (pm8921 only) + Value type: <phandle> + Definition: reference to regulator supplying the input pin, as + described in the data sheet + +The regulator node houses sub-nodes for each regulator within the device. Each +sub-node is identified using the node's name, with valid values listed for each +of the pmics below. + +pm8058: + l0, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15, + l16, l17, l18, l19, l20, l21, l22, l23, l24, l25, s0, s1, s2, s3, s4, + lvs0, lvs1, ncp + +pm8901: + l0, l1, l2, l3, l4, l5, l6, s0, s1, s2, s3, s4, lvs0, lvs1, lvs2, lvs3, + mvs + +pm8921: + s1, s2, s3, s4, s7, s8, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, + l12, l14, l15, l16, l17, l18, l21, l22, l23, l24, l25, l26, l27, l28, + l29, lvs1, lvs2, lvs3, lvs4, lvs5, lvs6, lvs7, usb-switch, hdmi-switch, + ncp + +The content of each sub-node is defined by the standard binding for regulators - +see regulator.txt - with additional custom properties described below: + +=== Switch-mode Power Supply regulator custom properties + +- bias-pull-down: + Usage: optional + Value type: <empty> + Definition: enable pull down of the regulator when inactive + +- qcom,switch-mode-frequency: + Usage: required + Value type: <u32> + Definition: Frequency (Hz) of the switch-mode power supply; + must be one of: + 19200000, 9600000, 6400000, 4800000, 3840000, 3200000, + 2740000, 2400000, 2130000, 1920000, 1750000, 1600000, + 1480000, 1370000, 1280000, 1200000 + +- qcom,force-mode: + Usage: optional (default if no other qcom,force-mode is specified) + Value type: <u32> + Defintion: indicates that the regulator should be forced to a + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_AUTO - allow regulator to automatically + select its own mode based on + realtime current draw, only for: + pm8921 smps and ftsmps + +- qcom,power-mode-hysteretic: + Usage: optional + Value type: <empty> + Definition: select that the power supply should operate in hysteretic + mode, instead of the default pwm mode + +=== Low-dropout regulator custom properties + +- bias-pull-down: + Usage: optional + Value type: <empty> + Definition: enable pull down of the regulator when inactive + +- qcom,force-mode: + Usage: optional + Value type: <u32> + Defintion: indicates that the regulator should not be forced to any + particular mode, valid values are: + QCOM_RPM_FORCE_MODE_NONE - do not force any mode + QCOM_RPM_FORCE_MODE_LPM - force into low power mode + QCOM_RPM_FORCE_MODE_HPM - force into high power mode + QCOM_RPM_FORCE_MODE_BYPASS - set regulator to use bypass + mode, i.e. to act as a switch + and not regulate, only for: + pm8921 pldo, nldo and nldo1200 + +=== Negative Charge Pump custom properties + +- qcom,switch-mode-frequency: + Usage: required + Value type: <u32> + Definition: Frequency (Hz) of the swith mode power supply; + must be one of: + 19200000, 9600000, 6400000, 4800000, 3840000, 3200000, + 2740000, 2400000, 2130000, 1920000, 1750000, 1600000, + 1480000, 1370000, 1280000, 1200000 + = EXAMPLE #include <dt-bindings/mfd/qcom-rpm.h> @@ -64,7 +220,28 @@ frequencies. interrupts = <0 19 0>, <0 21 0>, <0 22 0>; interrupt-names = "ack", "err", "wakeup"; - #address-cells = <1>; - #size-cells = <0>; + regulators { + compatible = "qcom,rpm-pm8921-regulators"; + vin_l1_l2_l12_l18-supply = <&pm8921_s4>; + + s1 { + regulator-min-microvolt = <1225000>; + regulator-max-microvolt = <1225000>; + + bias-pull-down; + + qcom,switch-mode-frequency = <3200000>; + }; + + pm8921_s4: s4 { + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + + qcom,switch-mode-frequency = <1600000>; + bias-pull-down; + + qcom,force-mode = <QCOM_RPM_FORCE_MODE_AUTO>; + }; + }; }; -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson @ 2015-03-03 12:47 ` Mark Brown 2015-03-03 16:02 ` Bjorn Andersson 2015-03-03 18:53 ` Stephen Boyd 1 sibling, 1 reply; 31+ messages in thread From: Mark Brown @ 2015-03-03 12:47 UTC (permalink / raw) To: Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 395 bytes --] On Mon, Mar 02, 2015 at 08:25:37PM -0800, Bjorn Andersson wrote: > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be one of: > + "qcom,rpm-pm8058-regulators" > + "qcom,rpm-pm8901-regulators" > + "qcom,rpm-pm8921-regulators" Why do these subnodes have a compatible - do they ever appear except as a child of a parent of the same type of device? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 12:47 ` Mark Brown @ 2015-03-03 16:02 ` Bjorn Andersson 2015-03-05 0:33 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 16:02 UTC (permalink / raw) To: Mark Brown Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 04:47 PST 2015, Mark Brown wrote: > On Mon, Mar 02, 2015 at 08:25:37PM -0800, Bjorn Andersson wrote: > > > +- compatible: > > + Usage: required > > + Value type: <string> > > + Definition: must be one of: > > + "qcom,rpm-pm8058-regulators" > > + "qcom,rpm-pm8901-regulators" > > + "qcom,rpm-pm8921-regulators" > > Why do these subnodes have a compatible - do they ever appear except as > a child of a parent of the same type of device? The relationship with the parent node is 1:M; in the case of 8960/8064 there is only one PMIC controlled by the RPM, hence the dt will look like: rpm { compatible = "qcom,rpm-apq8960"; regulators { compatible = "qcom,rpm-pm8921-regulators"; ... }; }; But for 8660, and later for e.g. 8974 we have something like: rpm { compatible = "qcom,rpm-msm8660"; pm8058-regulators { compatible = "qcom,rpm-pm8058-regulators"; vdd_xxx-supply = <&pm8058_s4>; ... }; pm8901-regulators { compatible = "qcom,rpm-pm8901-regulators"; ... }; }; I intended to match these by name, having one rpm-regulator device instance and using desc->regulators_node to match regulators in the right node - with of_node being the rpm node. But this doesn't really map to reality, as supplies are a property of pm8058 and pm8901 and not of the rpm. I ended up writing some custom device registering code to instantiate the right number of regulator children and give each of them the right of_node etc. But as several other of-based platforms have a compatible in their regulators node I consider that a viable and cleaner solution. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 16:02 ` Bjorn Andersson @ 2015-03-05 0:33 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2015-03-05 0:33 UTC (permalink / raw) To: Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 302 bytes --] On Tue, Mar 03, 2015 at 08:02:51AM -0800, Bjorn Andersson wrote: > rpm { > compatible = "qcom,rpm-apq8960"; > > regulators { > compatible = "qcom,rpm-pm8921-regulators"; Oh, so what you're saying is that the pm8921 is not actually a MFD at all? Why name it like a MFD component then? [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson 2015-03-03 12:47 ` Mark Brown @ 2015-03-03 18:53 ` Stephen Boyd 2015-03-03 21:54 ` Bjorn Andersson 1 sibling, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-03 18:53 UTC (permalink / raw) To: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/02/15 20:25, Bjorn Andersson wrote: > > += SUBNODES > + > +The RPM exposes resources to its subnodes. The below bindings specify the set > +of valid subnodes that can operate on these resources. > + > +== Regulators > + > +Regulator notes are identified by their compatible: > + > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must be one of: > + "qcom,rpm-pm8058-regulators" > + "qcom,rpm-pm8901-regulators" > + "qcom,rpm-pm8921-regulators" > + > +- vdd_l0_l1_lvs-supply: > +- vdd_l10-supply: > +- vdd_l13_l16-supply: > +- vdd_l14_l15-supply: > +- vdd_l17_l18-supply: > +- vdd_l19_l20-supply: > +- vdd_l21-supply: > +- vdd_l22-supply: > +- vdd_l23_l24_l25-supply: > +- vdd_l2_l11_l12-supply: > +- vdd_l3_l4_l5-supply: > +- vdd_l6_l7-supply: > +- vdd_l8-supply: > +- vdd_l9-supply: > +- vdd_ncp-supply: > +- vdd_s0-supply: > +- vdd_s1-supply: > +- vdd_s2-supply: > +- vdd_s3-supply: > +- vdd_s4-supply: > + Usage: optional (pm8058 only) > + Value type: <phandle> > + Definition: reference to regulator supplying the input pin, as > + described in the data sheet > + > +- vdd_l0-supply: > +- vdd_l1-supply: > +- vdd_l2-supply: > +- vdd_l3-supply: > +- vdd_l4-supply: > +- vdd_l5-supply: > +- vdd_l6-supply: > +- vdd_s0-supply: > +- vdd_s1-supply: > +- vdd_s2-supply: > +- vdd_s3-supply: > +- vdd_s4-supply: > +- lvs0_in-supply: > +- lvs1_in-supply: > +- lvs2_in-supply: > +- lvs3_in-supply: > +- mvs_in-supply: > + Usage: optional (pm8901 only) > + Value type: <phandle> > + Definition: reference to regulator supplying the input pin, as > + described in the data sheet > + > +- vin_l1_l2_l12_l18-supply: > +- vin_l24-supply: > +- vin_l25-supply: > +- vin_l27-supply: > +- vin_l28-supply: > +- vin_lvs_1_3_6-supply: > +- vin_lvs2-supply: > +- vin_lvs_4_5_7-supply: > +- vin_ncp-supply: Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I left out? It looks like you've covered all the inputs for the other pmics. > + Usage: optional (pm8921 only) > + Value type: <phandle> > + Definition: reference to regulator supplying the input pin, as > + described in the data sheet > + > +The regulator node houses sub-nodes for each regulator within the device. Each > +sub-node is identified using the node's name, with valid values listed for each > +of the pmics below. > + -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 18:53 ` Stephen Boyd @ 2015-03-03 21:54 ` Bjorn Andersson 2015-03-03 22:02 ` Stephen Boyd 0 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 21:54 UTC (permalink / raw) To: Stephen Boyd Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote: > On 03/02/15 20:25, Bjorn Andersson wrote: [..] > > + > > +- vin_l1_l2_l12_l18-supply: > > +- vin_l24-supply: > > +- vin_l25-supply: > > +- vin_l27-supply: > > +- vin_l28-supply: > > +- vin_lvs_1_3_6-supply: > > +- vin_lvs2-supply: > > +- vin_lvs_4_5_7-supply: > > +- vin_ncp-supply: > > Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I > left out? It looks like you've covered all the inputs for the other pmics. > Sorry, I had problems finding the information in the rather sparse documentation I have for the 8921, so I just assumed that I could steal your list. I finally managed to find the pinout diagram, so the correct list for pm8921 seems to be: - vdd_l1_2_12_18-supply - vdd_l3_15_17-supply - vdd_l5_8_16-supply - vdd_l6_7-supply - vdd_l9_11-supply - vdd_l10_22-supply - vdd_l21_23_29-supply - vdd_l24-supply - vdd_l25-supply - vdd_l26-supply - vdd_l27-supply - vdd_l28-supply - vdd_ncp-supply - vdd_s1-supply - vdd_s2-supply - vdd_s4-supply - vdd_s5-supply - vdd_s6-supply - vdd_s7-supply - vdd_s8-supply - vin_lvs1_3_6-supply - vin_lvs2-supply - vin_lvs4_5_7-supply I will send out an updated patch with these. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 21:54 ` Bjorn Andersson @ 2015-03-03 22:02 ` Stephen Boyd 2015-03-03 22:17 ` Bjorn Andersson 0 siblings, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-03 22:02 UTC (permalink / raw) To: Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/03/15 13:54, Bjorn Andersson wrote: > On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote: > >> On 03/02/15 20:25, Bjorn Andersson wrote: > [..] >>> + >>> +- vin_l1_l2_l12_l18-supply: >>> +- vin_l24-supply: >>> +- vin_l25-supply: >>> +- vin_l27-supply: >>> +- vin_l28-supply: >>> +- vin_lvs_1_3_6-supply: >>> +- vin_lvs2-supply: >>> +- vin_lvs_4_5_7-supply: >>> +- vin_ncp-supply: >> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I >> left out? It looks like you've covered all the inputs for the other pmics. >> > Sorry, I had problems finding the information in the rather sparse > documentation I have for the 8921, so I just assumed that I could steal > your list. > > I finally managed to find the pinout diagram, so the correct list for > pm8921 seems to be: > > - vdd_l1_2_12_18-supply > - vdd_l3_15_17-supply > - vdd_l5_8_16-supply vin_l4-supply? > - vdd_l6_7-supply > - vdd_l9_11-supply > - vdd_l10_22-supply > - vdd_l21_23_29-supply Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers (for all the LDOs). > - vdd_l24-supply > - vdd_l25-supply > - vdd_l26-supply > - vdd_l27-supply > - vdd_l28-supply > - vdd_ncp-supply > - vdd_s1-supply > - vdd_s2-supply > - vdd_s4-supply > - vdd_s5-supply > - vdd_s6-supply > - vdd_s7-supply > - vdd_s8-supply > - vin_lvs1_3_6-supply > - vin_lvs2-supply > - vin_lvs4_5_7-supply > > I will send out an updated patch with these. s/vdd/vin/ seems to match my datasheet. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 22:02 ` Stephen Boyd @ 2015-03-03 22:17 ` Bjorn Andersson 2015-03-03 23:25 ` Stephen Boyd 0 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 22:17 UTC (permalink / raw) To: Stephen Boyd Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 14:02 PST 2015, Stephen Boyd wrote: > On 03/03/15 13:54, Bjorn Andersson wrote: > > On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote: > > > >> On 03/02/15 20:25, Bjorn Andersson wrote: > > [..] > >>> + > >>> +- vin_l1_l2_l12_l18-supply: > >>> +- vin_l24-supply: > >>> +- vin_l25-supply: > >>> +- vin_l27-supply: > >>> +- vin_l28-supply: > >>> +- vin_lvs_1_3_6-supply: > >>> +- vin_lvs2-supply: > >>> +- vin_lvs_4_5_7-supply: > >>> +- vin_ncp-supply: > >> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I > >> left out? It looks like you've covered all the inputs for the other pmics. > >> > > Sorry, I had problems finding the information in the rather sparse > > documentation I have for the 8921, so I just assumed that I could steal > > your list. > > > > I finally managed to find the pinout diagram, so the correct list for > > pm8921 seems to be: > > > > - vdd_l1_2_12_18-supply > > - vdd_l3_15_17-supply > > - vdd_l5_8_16-supply > > vin_l4-supply? > Ahh, right, I missed the vdd_l4_l14 pad. > > - vdd_l6_7-supply > > - vdd_l9_11-supply > > - vdd_l10_22-supply > > - vdd_l21_23_29-supply > > Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers > (for all the LDOs). > The other pmics name their inputs like that; should we just pick that naming scheme for all these supplies and be happy? > > - vdd_l24-supply > > - vdd_l25-supply > > - vdd_l26-supply > > - vdd_l27-supply > > - vdd_l28-supply > > - vdd_ncp-supply > > - vdd_s1-supply > > - vdd_s2-supply > > - vdd_s4-supply > > - vdd_s5-supply > > - vdd_s6-supply > > - vdd_s7-supply > > - vdd_s8-supply > > - vin_lvs1_3_6-supply > > - vin_lvs2-supply > > - vin_lvs4_5_7-supply > > > > I will send out an updated patch with these. > > s/vdd/vin/ seems to match my datasheet. > For all of these? Same with the other pmics? These are the documented pad names that I have, nice to see that they are named differently in different documents. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes 2015-03-03 22:17 ` Bjorn Andersson @ 2015-03-03 23:25 ` Stephen Boyd 0 siblings, 0 replies; 31+ messages in thread From: Stephen Boyd @ 2015-03-03 23:25 UTC (permalink / raw) To: Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/03/15 14:17, Bjorn Andersson wrote: > On Tue 03 Mar 14:02 PST 2015, Stephen Boyd wrote: > >> On 03/03/15 13:54, Bjorn Andersson wrote: >>> On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote: >>> >>>> On 03/02/15 20:25, Bjorn Andersson wrote: >>> [..] >>>>> + >>>>> +- vin_l1_l2_l12_l18-supply: >>>>> +- vin_l24-supply: >>>>> +- vin_l25-supply: >>>>> +- vin_l27-supply: >>>>> +- vin_l28-supply: >>>>> +- vin_lvs_1_3_6-supply: >>>>> +- vin_lvs2-supply: >>>>> +- vin_lvs_4_5_7-supply: >>>>> +- vin_ncp-supply: >>>> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I >>>> left out? It looks like you've covered all the inputs for the other pmics. >>>> >>> Sorry, I had problems finding the information in the rather sparse >>> documentation I have for the 8921, so I just assumed that I could steal >>> your list. >>> >>> I finally managed to find the pinout diagram, so the correct list for >>> pm8921 seems to be: >>> >>> - vdd_l1_2_12_18-supply >>> - vdd_l3_15_17-supply >>> - vdd_l5_8_16-supply >> vin_l4-supply? >> > Ahh, right, I missed the vdd_l4_l14 pad. > >>> - vdd_l6_7-supply >>> - vdd_l9_11-supply >>> - vdd_l10_22-supply >>> - vdd_l21_23_29-supply >> Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers >> (for all the LDOs). >> > The other pmics name their inputs like that; should we just pick that > naming scheme for all these supplies and be happy? Ok. >>> - vdd_l24-supply >>> - vdd_l25-supply >>> - vdd_l26-supply >>> - vdd_l27-supply >>> - vdd_l28-supply >>> - vdd_ncp-supply >>> - vdd_s1-supply >>> - vdd_s2-supply >>> - vdd_s4-supply >>> - vdd_s5-supply >>> - vdd_s6-supply >>> - vdd_s7-supply >>> - vdd_s8-supply >>> - vin_lvs1_3_6-supply >>> - vin_lvs2-supply >>> - vin_lvs4_5_7-supply >>> >>> I will send out an updated patch with these. >> s/vdd/vin/ seems to match my datasheet. >> > For all of these? Same with the other pmics? > > These are the documented pad names that I have, nice to see that they > are named differently in different documents. Oh. I seem to have been looking at some pre-published doc that was still laying around. vdd seems to match the official documentation so that sounds good to me. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb 2015-03-03 4:25 [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device Bjorn Andersson 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson @ 2015-03-03 4:25 ` Bjorn Andersson 2015-03-03 12:50 ` Mark Brown 2015-03-03 4:25 ` [PATCH 3/4] regulator: qcom: Refactor of-parsing code Bjorn Andersson 2015-03-03 4:25 ` [PATCH 4/4] regulator: qcom: Rework to single platform device Bjorn Andersson 3 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 4:25 UTC (permalink / raw) To: Chanwoo Choi, Ian Campbell, Krzysztof Kozlowski, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross Cc: Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm Expose the newly created init_data to the driver's parse callback so that it can futher enhance it with e.g. constraints of the regulator. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- This is needed because calling regulator_register() with a of_node and of_match will overwrite the passed config->init_data. An alternative way would be to merge the parsed init_data with the driver provided one. drivers/regulator/max77686.c | 3 ++- drivers/regulator/of_regulator.c | 2 +- include/linux/regulator/driver.h | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c index 15fb141..87cbef2 100644 --- a/drivers/regulator/max77686.c +++ b/drivers/regulator/max77686.c @@ -259,7 +259,8 @@ static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay) static int max77686_of_parse_cb(struct device_node *np, const struct regulator_desc *desc, - struct regulator_config *config) + struct regulator_config *config, + struct regulator_init_data *init_data) { struct max77686_data *max77686 = config->driver_data; diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 24e812c..f65934f 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -309,7 +309,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, } if (desc->of_parse_cb) { - if (desc->of_parse_cb(child, desc, config)) { + if (desc->of_parse_cb(child, desc, config, init_data)) { dev_err(dev, "driver callback failed to parse DT for regulator %s\n", child->name); diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 8a0165f..0f86a182 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -266,7 +266,8 @@ struct regulator_desc { const char *regulators_node; int (*of_parse_cb)(struct device_node *, const struct regulator_desc *, - struct regulator_config *); + struct regulator_config *, + struct regulator_init_data *); int id; bool continuous_voltage_range; unsigned n_voltages; -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb 2015-03-03 4:25 ` [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb Bjorn Andersson @ 2015-03-03 12:50 ` Mark Brown 2015-03-03 16:15 ` Bjorn Andersson 0 siblings, 1 reply; 31+ messages in thread From: Mark Brown @ 2015-03-03 12:50 UTC (permalink / raw) To: Bjorn Andersson Cc: Chanwoo Choi, Ian Campbell, Krzysztof Kozlowski, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 765 bytes --] On Mon, Mar 02, 2015 at 08:25:38PM -0800, Bjorn Andersson wrote: > Expose the newly created init_data to the driver's parse callback so > that it can futher enhance it with e.g. constraints of the regulator. Why would the driver need to do that? I guess I'll see later on in the series but the changelog should make that clear. Drivers aren't supposed to ever need to look at their init data, modifying the init data from what the machine provied is usually a red flag. > This is needed because calling regulator_register() with a of_node and > of_match will overwrite the passed config->init_data. An alternative way > would be to merge the parsed init_data with the driver provided one. Put any explanation as to why the feature is needed in the changelog. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb 2015-03-03 12:50 ` Mark Brown @ 2015-03-03 16:15 ` Bjorn Andersson 2015-03-05 0:42 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 16:15 UTC (permalink / raw) To: Mark Brown Cc: Chanwoo Choi, Ian Campbell, Krzysztof Kozlowski, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 04:50 PST 2015, Mark Brown wrote: > On Mon, Mar 02, 2015 at 08:25:38PM -0800, Bjorn Andersson wrote: > > > Expose the newly created init_data to the driver's parse callback so > > that it can futher enhance it with e.g. constraints of the regulator. > > Why would the driver need to do that? I guess I'll see later on in the > series but the changelog should make that clear. Drivers aren't > supposed to ever need to look at their init data, modifying the init > data from what the machine provied is usually a red flag. > I think what you're getting at is that the init_data should come from a board file or device tree. With the reworkings done in patch 4 this makes more sense than it did before (e.g. I no longer call of_get_regulator_init_data()). However, by calling register_regulator() with dev->of_node being non-NULL and desc->of_match being something that will match a DT entry all init_data is now coming from device tree, through: regulator_register() regulator_of_get_init_data() of_get_regulator_init_data() of_get_regulation_constraints() As this matches a regulator, there is no way for the driver (or anyone else to affect the init_data). The problem at hand is that there is nothing in this code path telling the core that we can do DRMS - something we can figure out in the driver by basically looking at the ops for the registering regulator. > > This is needed because calling regulator_register() with a of_node and > > of_match will overwrite the passed config->init_data. An alternative way > > would be to merge the parsed init_data with the driver provided one. > > Put any explanation as to why the feature is needed in the changelog. Point taken. Let me know if you think I'm on a sane path with the above reasoning and I can resend the patch with a better description of the problem at hand. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb 2015-03-03 16:15 ` Bjorn Andersson @ 2015-03-05 0:42 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2015-03-05 0:42 UTC (permalink / raw) To: Bjorn Andersson Cc: Chanwoo Choi, Ian Campbell, Krzysztof Kozlowski, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 1993 bytes --] On Tue, Mar 03, 2015 at 08:15:41AM -0800, Bjorn Andersson wrote: > On Tue 03 Mar 04:50 PST 2015, Mark Brown wrote: > > Why would the driver need to do that? I guess I'll see later on in the > > series but the changelog should make that clear. Drivers aren't > > supposed to ever need to look at their init data, modifying the init > > data from what the machine provied is usually a red flag. > I think what you're getting at is that the init_data should come from a > board file or device tree. With the reworkings done in patch 4 this Yes, that's the entire purpose of the init data. > As this matches a regulator, there is no way for the driver (or anyone > else to affect the init_data). That's a goal not a problem. There is an interface for the machine description to configure the system integration which neither the regulator driver nor the consumer driver should be a part of. > The problem at hand is that there is nothing in this code path telling > the core that we can do DRMS - something we can figure out in the driver > by basically looking at the ops for the registering regulator. No, that way lies people doing all the crap they usually do with putting the default constraints for their reference design or the maximum limits for their PMIC into the constraints and then getting upset when drivers then go and use this to make their CPU catch fire or whatever. You need to provide a way for the machine description to say DRMS (or really setting the load which is what's actually happening here now we have that op) is safe on a given system. I think that means adding a new permission to DT and then using that; it's more sensible to want to use this feature in the context where we're working with an external processor which also has a chance to have system tuning than when we're working with the PMIC directly so a permission that enable load setting seems good. We could even kill the existing DRMS permission, there's one user in a legacy STE platform. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/4] regulator: qcom: Refactor of-parsing code 2015-03-03 4:25 [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device Bjorn Andersson 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson 2015-03-03 4:25 ` [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb Bjorn Andersson @ 2015-03-03 4:25 ` Bjorn Andersson 2015-03-03 14:13 ` Mark Brown 2015-03-03 18:56 ` Stephen Boyd 2015-03-03 4:25 ` [PATCH 4/4] regulator: qcom: Rework to single platform device Bjorn Andersson 3 siblings, 2 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 4:25 UTC (permalink / raw) To: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm Refactor out all custom property parsing code from the probe function into a function suitable for regulator_desc->of_parse_cb usage. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- This simply moves the parsing code out of probe(), no functional change. drivers/regulator/qcom_rpm-regulator.c | 147 +++++++++++++++++++-------------- 1 file changed, 85 insertions(+), 62 deletions(-) diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c index 15e07c2..1ec4b98 100644 --- a/drivers/regulator/qcom_rpm-regulator.c +++ b/drivers/regulator/qcom_rpm-regulator.c @@ -645,7 +645,9 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg, return 0; } -static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg) +static int rpm_reg_of_parse_freq(struct device *dev, + struct device_node *node, + struct qcom_rpm_reg *vreg) { static const int freq_table[] = { 19200000, 9600000, 6400000, 4800000, 3840000, 3200000, 2740000, @@ -659,7 +661,7 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg) int i; key = "qcom,switch-mode-frequency"; - ret = of_property_read_u32(dev->of_node, key, &freq); + ret = of_property_read_u32(node, key, &freq); if (ret) { dev_err(dev, "regulator requires %s property\n", key); return -EINVAL; @@ -676,88 +678,45 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg) return -EINVAL; } -static int rpm_reg_probe(struct platform_device *pdev) +static int rpm_reg_of_parse(struct device_node *node, + const struct regulator_desc *desc, + struct regulator_config *config, + struct regulator_init_data *init_data) { - struct regulator_init_data *initdata; - const struct qcom_rpm_reg *template; - const struct of_device_id *match; - struct regulator_config config = { }; - struct regulator_dev *rdev; - struct qcom_rpm_reg *vreg; + struct qcom_rpm_reg *vreg = config->driver_data; + struct device *dev = config->dev; const char *key; u32 force_mode; bool pwm; - u32 val; int ret; - - match = of_match_device(rpm_of_match, &pdev->dev); - template = match->data; - - vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); - if (!vreg) { - dev_err(&pdev->dev, "failed to allocate vreg\n"); - return -ENOMEM; - } - memcpy(vreg, template, sizeof(*vreg)); - mutex_init(&vreg->lock); - vreg->dev = &pdev->dev; - vreg->desc.id = -1; - vreg->desc.owner = THIS_MODULE; - vreg->desc.type = REGULATOR_VOLTAGE; - vreg->desc.name = pdev->dev.of_node->name; - vreg->desc.supply_name = "vin"; - - vreg->rpm = dev_get_drvdata(pdev->dev.parent); - if (!vreg->rpm) { - dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); - return -ENODEV; - } - - initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node, - &vreg->desc); - if (!initdata) - return -EINVAL; - - key = "reg"; - ret = of_property_read_u32(pdev->dev.of_node, key, &val); - if (ret) { - dev_err(&pdev->dev, "failed to read %s\n", key); - return ret; - } - vreg->resource = val; - - if ((vreg->parts->uV.mask || vreg->parts->mV.mask) && - (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) { - dev_err(&pdev->dev, "no voltage specified for regulator\n"); - return -EINVAL; - } + u32 val; /* Regulators with ia property suppports drms */ if (vreg->parts->ia.mask) - initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS; + init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS; key = "bias-pull-down"; - if (of_property_read_bool(pdev->dev.of_node, key)) { + if (of_property_read_bool(node, key)) { ret = rpm_reg_set(vreg, &vreg->parts->pd, 1); if (ret) { - dev_err(&pdev->dev, "%s is invalid", key); + dev_err(dev, "%s is invalid", key); return ret; } } if (vreg->parts->freq.mask) { - ret = rpm_reg_of_parse_freq(&pdev->dev, vreg); + ret = rpm_reg_of_parse_freq(dev, node, vreg); if (ret < 0) return ret; } if (vreg->parts->pm.mask) { key = "qcom,power-mode-hysteretic"; - pwm = !of_property_read_bool(pdev->dev.of_node, key); + pwm = !of_property_read_bool(node, key); ret = rpm_reg_set(vreg, &vreg->parts->pm, pwm); if (ret) { - dev_err(&pdev->dev, "failed to set power mode\n"); + dev_err(dev, "failed to set power mode\n"); return ret; } } @@ -766,11 +725,11 @@ static int rpm_reg_probe(struct platform_device *pdev) force_mode = -1; key = "qcom,force-mode"; - ret = of_property_read_u32(pdev->dev.of_node, key, &val); + ret = of_property_read_u32(node, key, &val); if (ret == -EINVAL) { val = QCOM_RPM_FORCE_MODE_NONE; } else if (ret < 0) { - dev_err(&pdev->dev, "failed to read %s\n", key); + dev_err(dev, "failed to read %s\n", key); return ret; } @@ -805,21 +764,85 @@ static int rpm_reg_probe(struct platform_device *pdev) } if (force_mode == -1) { - dev_err(&pdev->dev, "invalid force mode\n"); + dev_err(dev, "invalid force mode\n"); return -EINVAL; } ret = rpm_reg_set(vreg, &vreg->parts->fm, force_mode); if (ret) { - dev_err(&pdev->dev, "failed to set force mode\n"); + dev_err(dev, "failed to set force mode\n"); return ret; } } + return 0; +} + +static int rpm_reg_probe(struct platform_device *pdev) +{ + struct regulator_init_data *initdata; + const struct qcom_rpm_reg *template; + const struct of_device_id *match; + struct regulator_config config = { }; + struct regulator_dev *rdev; + struct qcom_rpm_reg *vreg; + const char *key; + u32 val; + int ret; + + match = of_match_device(rpm_of_match, &pdev->dev); + template = match->data; + + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); + if (!vreg) { + dev_err(&pdev->dev, "failed to allocate vreg\n"); + return -ENOMEM; + } + memcpy(vreg, template, sizeof(*vreg)); + mutex_init(&vreg->lock); + vreg->dev = &pdev->dev; + vreg->desc.id = -1; + vreg->desc.owner = THIS_MODULE; + vreg->desc.type = REGULATOR_VOLTAGE; + vreg->desc.name = pdev->dev.of_node->name; + vreg->desc.supply_name = "vin"; + + vreg->rpm = dev_get_drvdata(pdev->dev.parent); + if (!vreg->rpm) { + dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); + return -ENODEV; + } + + initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node, + &vreg->desc); + if (!initdata) + return -EINVAL; + + key = "reg"; + ret = of_property_read_u32(pdev->dev.of_node, key, &val); + if (ret) { + dev_err(&pdev->dev, "failed to read %s\n", key); + return ret; + } + vreg->resource = val; + + if ((vreg->parts->uV.mask || vreg->parts->mV.mask) && + (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) { + dev_err(&pdev->dev, "no voltage specified for regulator\n"); + return -EINVAL; + } + + config.dev = &pdev->dev; config.init_data = initdata; config.driver_data = vreg; config.of_node = pdev->dev.of_node; + + ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc, + &config, initdata); + if (ret) + return ret; + rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); if (IS_ERR(rdev)) { dev_err(&pdev->dev, "can't register regulator\n"); -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code 2015-03-03 4:25 ` [PATCH 3/4] regulator: qcom: Refactor of-parsing code Bjorn Andersson @ 2015-03-03 14:13 ` Mark Brown 2015-03-03 16:26 ` Bjorn Andersson 2015-03-03 18:56 ` Stephen Boyd 1 sibling, 1 reply; 31+ messages in thread From: Mark Brown @ 2015-03-03 14:13 UTC (permalink / raw) To: Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 702 bytes --] On Mon, Mar 02, 2015 at 08:25:39PM -0800, Bjorn Andersson wrote: > + if ((vreg->parts->uV.mask || vreg->parts->mV.mask) && > + (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) { > + dev_err(&pdev->dev, "no voltage specified for regulator\n"); > + return -EINVAL; > + } OK, so it looks like we're using it because we can't read the initial hardware state - can we introduce a specific interface for that perhaps? A call to query the current constraints might do it. Or possibly just change your earlier patch to make sure the constraints are marked const would do it, reading them isn't the problem that modifying them is. I might've missed some other usage somewhere though. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code 2015-03-03 14:13 ` Mark Brown @ 2015-03-03 16:26 ` Bjorn Andersson 0 siblings, 0 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 16:26 UTC (permalink / raw) To: Mark Brown Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 06:13 PST 2015, Mark Brown wrote: > On Mon, Mar 02, 2015 at 08:25:39PM -0800, Bjorn Andersson wrote: > > > + if ((vreg->parts->uV.mask || vreg->parts->mV.mask) && > > + (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) { > > + dev_err(&pdev->dev, "no voltage specified for regulator\n"); > > + return -EINVAL; > > + } > > OK, so it looks like we're using it because we can't read the initial > hardware state - can we introduce a specific interface for that perhaps? > A call to query the current constraints might do it. Or possibly just > change your earlier patch to make sure the constraints are marked const > would do it, reading them isn't the problem that modifying them is. I > might've missed some other usage somewhere though. No, the reason for init_data is this snippet: /* Regulators with ia property suppports drms */ if (vreg->parts->ia.mask) init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS; This patch just moves the code around to make it possible to implement patch 4 as cleanly as possible. The quoted code checks to see that for regulators with voltage-nobs (could have checked for set_voltage ops) the of-parser did find voltage limits. So it's a sanity check that discards regulators with missing dt parameters. I did drop this in patch 4, as it's not strictly needed. But the result is that we will successfully register the regulator and the consumers will be faced with errors upon trying to set voltage or enable it. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code 2015-03-03 4:25 ` [PATCH 3/4] regulator: qcom: Refactor of-parsing code Bjorn Andersson 2015-03-03 14:13 ` Mark Brown @ 2015-03-03 18:56 ` Stephen Boyd 2015-03-03 22:07 ` Bjorn Andersson 1 sibling, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-03 18:56 UTC (permalink / raw) To: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/02/15 20:25, Bjorn Andersson wrote: > + > +static int rpm_reg_probe(struct platform_device *pdev) > +{ > + struct regulator_init_data *initdata; > + const struct qcom_rpm_reg *template; > + const struct of_device_id *match; > + struct regulator_config config = { }; > + struct regulator_dev *rdev; > + struct qcom_rpm_reg *vreg; > + const char *key; > + u32 val; > + int ret; > + > + match = of_match_device(rpm_of_match, &pdev->dev); > + template = match->data; > + > + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); > + if (!vreg) { > + dev_err(&pdev->dev, "failed to allocate vreg\n"); We don't need error messages on allocation failures. > + return -ENOMEM; > + } -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code 2015-03-03 18:56 ` Stephen Boyd @ 2015-03-03 22:07 ` Bjorn Andersson 0 siblings, 0 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 22:07 UTC (permalink / raw) To: Stephen Boyd Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 10:56 PST 2015, Stephen Boyd wrote: > On 03/02/15 20:25, Bjorn Andersson wrote: > > + > > +static int rpm_reg_probe(struct platform_device *pdev) > > +{ > > + struct regulator_init_data *initdata; > > + const struct qcom_rpm_reg *template; > > + const struct of_device_id *match; > > + struct regulator_config config = { }; > > + struct regulator_dev *rdev; > > + struct qcom_rpm_reg *vreg; > > + const char *key; > > + u32 val; > > + int ret; > > + > > + match = of_match_device(rpm_of_match, &pdev->dev); > > + template = match->data; > > + > > + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); > > + if (!vreg) { > > + dev_err(&pdev->dev, "failed to allocate vreg\n"); > > We don't need error messages on allocation failures. > Right, it's just that I wanted to keep these patches free from any unrelated changes. I can add an extra patch at the end removing this and moving the retrieval of rpm out of the for loop. > > + return -ENOMEM; > > + } Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 4:25 [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device Bjorn Andersson ` (2 preceding siblings ...) 2015-03-03 4:25 ` [PATCH 3/4] regulator: qcom: Refactor of-parsing code Bjorn Andersson @ 2015-03-03 4:25 ` Bjorn Andersson 2015-03-03 22:09 ` Stephen Boyd 2015-03-04 19:35 ` Stephen Boyd 3 siblings, 2 replies; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 4:25 UTC (permalink / raw) To: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Stephen Boyd, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm Modeling the individual RPM resources as platform devices consumes at least 12-15kb of RAM, just to hold the platform_device structs. Rework this to instead have one device per pmic exposed by the RPM. With this representation we can more accurately define the input pins on the pmic and have the supply description match the data sheet. Suggested-by: Stephen Boyd <sboyd@codeaurora.org> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- drivers/regulator/qcom_rpm-regulator.c | 245 ++++++++++++++++++++++----------- 1 file changed, 161 insertions(+), 84 deletions(-) diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c index 1ec4b98..5cdd568 100644 --- a/drivers/regulator/qcom_rpm-regulator.c +++ b/drivers/regulator/qcom_rpm-regulator.c @@ -607,31 +607,6 @@ static const struct qcom_rpm_reg smb208_smps = { .supports_force_mode_bypass = false, }; -static const struct of_device_id rpm_of_match[] = { - { .compatible = "qcom,rpm-pm8058-pldo", .data = &pm8058_pldo }, - { .compatible = "qcom,rpm-pm8058-nldo", .data = &pm8058_nldo }, - { .compatible = "qcom,rpm-pm8058-smps", .data = &pm8058_smps }, - { .compatible = "qcom,rpm-pm8058-ncp", .data = &pm8058_ncp }, - { .compatible = "qcom,rpm-pm8058-switch", .data = &pm8058_switch }, - - { .compatible = "qcom,rpm-pm8901-pldo", .data = &pm8901_pldo }, - { .compatible = "qcom,rpm-pm8901-nldo", .data = &pm8901_nldo }, - { .compatible = "qcom,rpm-pm8901-ftsmps", .data = &pm8901_ftsmps }, - { .compatible = "qcom,rpm-pm8901-switch", .data = &pm8901_switch }, - - { .compatible = "qcom,rpm-pm8921-pldo", .data = &pm8921_pldo }, - { .compatible = "qcom,rpm-pm8921-nldo", .data = &pm8921_nldo }, - { .compatible = "qcom,rpm-pm8921-nldo1200", .data = &pm8921_nldo1200 }, - { .compatible = "qcom,rpm-pm8921-smps", .data = &pm8921_smps }, - { .compatible = "qcom,rpm-pm8921-ftsmps", .data = &pm8921_ftsmps }, - { .compatible = "qcom,rpm-pm8921-ncp", .data = &pm8921_ncp }, - { .compatible = "qcom,rpm-pm8921-switch", .data = &pm8921_switch }, - - { .compatible = "qcom,rpm-smb208", .data = &smb208_smps }, - { } -}; -MODULE_DEVICE_TABLE(of, rpm_of_match); - static int rpm_reg_set(struct qcom_rpm_reg *vreg, const struct request_member *req, const int value) @@ -778,75 +753,177 @@ static int rpm_reg_of_parse(struct device_node *node, return 0; } -static int rpm_reg_probe(struct platform_device *pdev) -{ - struct regulator_init_data *initdata; +struct rpm_regulator_data { + const char *name; + int resource; const struct qcom_rpm_reg *template; - const struct of_device_id *match; - struct regulator_config config = { }; - struct regulator_dev *rdev; - struct qcom_rpm_reg *vreg; - const char *key; - u32 val; - int ret; + const char *supply; +}; + +static const struct rpm_regulator_data rpm_pm8058_regulators[] = { + { "l0", QCOM_RPM_PM8058_LDO0, &pm8058_nldo, "vdd_l0_l1_lvs" }, + { "l1", QCOM_RPM_PM8058_LDO1, &pm8058_nldo, "vdd_l0_l1_lvs" }, + { "l2", QCOM_RPM_PM8058_LDO2, &pm8058_pldo, "vdd_l2_l11_l12" }, + { "l3", QCOM_RPM_PM8058_LDO3, &pm8058_pldo, "vdd_l3_l4_l5" }, + { "l4", QCOM_RPM_PM8058_LDO4, &pm8058_pldo, "vdd_l3_l4_l5" }, + { "l5", QCOM_RPM_PM8058_LDO5, &pm8058_pldo, "vdd_l3_l4_l5" }, + { "l6", QCOM_RPM_PM8058_LDO6, &pm8058_pldo, "vdd_l6_l7" }, + { "l7", QCOM_RPM_PM8058_LDO7, &pm8058_pldo, "vdd_l6_l7" }, + { "l8", QCOM_RPM_PM8058_LDO8, &pm8058_pldo, "vdd_l8" }, + { "l9", QCOM_RPM_PM8058_LDO9, &pm8058_pldo, "vdd_l9" }, + { "l10", QCOM_RPM_PM8058_LDO10, &pm8058_pldo, "vdd_l10" }, + { "l11", QCOM_RPM_PM8058_LDO11, &pm8058_pldo, "vdd_l2_l11_l12" }, + { "l12", QCOM_RPM_PM8058_LDO12, &pm8058_pldo, "vdd_l2_l11_l12" }, + { "l13", QCOM_RPM_PM8058_LDO13, &pm8058_pldo, "vdd_l13_l16" }, + { "l14", QCOM_RPM_PM8058_LDO14, &pm8058_pldo, "vdd_l14_l15" }, + { "l15", QCOM_RPM_PM8058_LDO15, &pm8058_pldo, "vdd_l14_l15" }, + { "l16", QCOM_RPM_PM8058_LDO16, &pm8058_pldo, "vdd_l13_l16" }, + { "l17", QCOM_RPM_PM8058_LDO17, &pm8058_pldo, "vdd_l17_l18" }, + { "l18", QCOM_RPM_PM8058_LDO18, &pm8058_pldo, "vdd_l17_l18" }, + { "l19", QCOM_RPM_PM8058_LDO19, &pm8058_pldo, "vdd_l19_l20" }, + { "l20", QCOM_RPM_PM8058_LDO20, &pm8058_pldo, "vdd_l19_l20" }, + { "l21", QCOM_RPM_PM8058_LDO21, &pm8058_nldo, "vdd_l21" }, + { "l22", QCOM_RPM_PM8058_LDO22, &pm8058_nldo, "vdd_l22" }, + { "l23", QCOM_RPM_PM8058_LDO23, &pm8058_nldo, "vdd_l23_l24_l25" }, + { "l24", QCOM_RPM_PM8058_LDO24, &pm8058_nldo, "vdd_l23_l24_l25" }, + { "l25", QCOM_RPM_PM8058_LDO25, &pm8058_nldo, "vdd_l23_l24_l25" }, + + { "s0", QCOM_RPM_PM8058_SMPS0, &pm8058_smps, "vdd_s0" }, + { "s1", QCOM_RPM_PM8058_SMPS1, &pm8058_smps, "vdd_s1" }, + { "s2", QCOM_RPM_PM8058_SMPS2, &pm8058_smps, "vdd_s2" }, + { "s3", QCOM_RPM_PM8058_SMPS3, &pm8058_smps, "vdd_s3" }, + { "s4", QCOM_RPM_PM8058_SMPS4, &pm8058_smps, "vdd_s4" }, + + { "lvs0", QCOM_RPM_PM8058_LVS0, &pm8058_switch, "vdd_l0_l1_lvs" }, + { "lvs1", QCOM_RPM_PM8058_LVS1, &pm8058_switch, "vdd_l0_l1_lvs" }, + + { "ncp", QCOM_RPM_PM8058_NCP, &pm8058_ncp, "vdd_ncp" }, + { } +}; - match = of_match_device(rpm_of_match, &pdev->dev); - template = match->data; +static const struct rpm_regulator_data rpm_pm8901_regulators[] = { + { "l0", QCOM_RPM_PM8901_LDO0, &pm8901_nldo, "vdd_l0" }, + { "l1", QCOM_RPM_PM8901_LDO1, &pm8901_pldo, "vdd_l1" }, + { "l2", QCOM_RPM_PM8901_LDO2, &pm8901_pldo, "vdd_l2" }, + { "l3", QCOM_RPM_PM8901_LDO3, &pm8901_pldo, "vdd_l3" }, + { "l4", QCOM_RPM_PM8901_LDO4, &pm8901_pldo, "vdd_l4" }, + { "l5", QCOM_RPM_PM8901_LDO5, &pm8901_pldo, "vdd_l5" }, + { "l6", QCOM_RPM_PM8901_LDO6, &pm8901_pldo, "vdd_l6" }, - vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); - if (!vreg) { - dev_err(&pdev->dev, "failed to allocate vreg\n"); - return -ENOMEM; - } - memcpy(vreg, template, sizeof(*vreg)); - mutex_init(&vreg->lock); - vreg->dev = &pdev->dev; - vreg->desc.id = -1; - vreg->desc.owner = THIS_MODULE; - vreg->desc.type = REGULATOR_VOLTAGE; - vreg->desc.name = pdev->dev.of_node->name; - vreg->desc.supply_name = "vin"; - - vreg->rpm = dev_get_drvdata(pdev->dev.parent); - if (!vreg->rpm) { - dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); - return -ENODEV; - } + { "s0", QCOM_RPM_PM8901_SMPS0, &pm8901_ftsmps, "vdd_s0" }, + { "s1", QCOM_RPM_PM8901_SMPS1, &pm8901_ftsmps, "vdd_s1" }, + { "s2", QCOM_RPM_PM8901_SMPS2, &pm8901_ftsmps, "vdd_s2" }, + { "s3", QCOM_RPM_PM8901_SMPS3, &pm8901_ftsmps, "vdd_s3" }, + { "s4", QCOM_RPM_PM8901_SMPS4, &pm8901_ftsmps, "vdd_s4" }, - initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node, - &vreg->desc); - if (!initdata) - return -EINVAL; + { "lvs0", QCOM_RPM_PM8901_LVS0, &pm8901_switch, "lvs0_in" }, + { "lvs1", QCOM_RPM_PM8901_LVS1, &pm8901_switch, "lvs1_in" }, + { "lvs2", QCOM_RPM_PM8901_LVS2, &pm8901_switch, "lvs2_in" }, + { "lvs3", QCOM_RPM_PM8901_LVS3, &pm8901_switch, "lvs3_in" }, - key = "reg"; - ret = of_property_read_u32(pdev->dev.of_node, key, &val); - if (ret) { - dev_err(&pdev->dev, "failed to read %s\n", key); - return ret; - } - vreg->resource = val; + { "mvs", QCOM_RPM_PM8901_MVS, &pm8901_switch, "mvs_in" }, + { } +}; - if ((vreg->parts->uV.mask || vreg->parts->mV.mask) && - (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) { - dev_err(&pdev->dev, "no voltage specified for regulator\n"); - return -EINVAL; - } +static const struct rpm_regulator_data rpm_pm8921_regulators[] = { + { "s1", QCOM_RPM_PM8921_SMPS1, &pm8921_smps }, + { "s2", QCOM_RPM_PM8921_SMPS2, &pm8921_smps }, + { "s3", QCOM_RPM_PM8921_SMPS3, &pm8921_smps }, + { "s4", QCOM_RPM_PM8921_SMPS4, &pm8921_smps }, + { "s7", QCOM_RPM_PM8921_SMPS7, &pm8921_smps }, + { "s8", QCOM_RPM_PM8921_SMPS8, &pm8921_smps }, + + { "l1", QCOM_RPM_PM8921_LDO1, &pm8921_nldo, "vin_l1_l2_l12_l18" }, + { "l2", QCOM_RPM_PM8921_LDO2, &pm8921_nldo, "vin_l1_l2_l12_l18" }, + { "l3", QCOM_RPM_PM8921_LDO3, &pm8921_pldo }, + { "l4", QCOM_RPM_PM8921_LDO4, &pm8921_pldo }, + { "l5", QCOM_RPM_PM8921_LDO5, &pm8921_pldo }, + { "l6", QCOM_RPM_PM8921_LDO6, &pm8921_pldo }, + { "l7", QCOM_RPM_PM8921_LDO7, &pm8921_pldo }, + { "l8", QCOM_RPM_PM8921_LDO8, &pm8921_pldo }, + { "l9", QCOM_RPM_PM8921_LDO9, &pm8921_pldo }, + { "l10", QCOM_RPM_PM8921_LDO10, &pm8921_pldo }, + { "l11", QCOM_RPM_PM8921_LDO11, &pm8921_pldo }, + { "l12", QCOM_RPM_PM8921_LDO12, &pm8921_nldo }, + { "l14", QCOM_RPM_PM8921_LDO14, &pm8921_pldo }, + { "l15", QCOM_RPM_PM8921_LDO15, &pm8921_pldo }, + { "l16", QCOM_RPM_PM8921_LDO16, &pm8921_pldo }, + { "l17", QCOM_RPM_PM8921_LDO17, &pm8921_pldo }, + { "l18", QCOM_RPM_PM8921_LDO18, &pm8921_nldo, "vin_l1_l2_l12_l18" }, + { "l21", QCOM_RPM_PM8921_LDO21, &pm8921_pldo }, + { "l22", QCOM_RPM_PM8921_LDO22, &pm8921_pldo }, + { "l23", QCOM_RPM_PM8921_LDO23, &pm8921_pldo }, + { "l24", QCOM_RPM_PM8921_LDO24, &pm8921_nldo1200, "vin_l24" }, + { "l25", QCOM_RPM_PM8921_LDO25, &pm8921_nldo1200, "vin_l25" }, + { "l26", QCOM_RPM_PM8921_LDO26, &pm8921_nldo1200 }, + { "l27", QCOM_RPM_PM8921_LDO27, &pm8921_nldo1200, "vin_l27" }, + { "l28", QCOM_RPM_PM8921_LDO28, &pm8921_nldo1200, "vin_l28" }, + { "l29", QCOM_RPM_PM8921_LDO29, &pm8921_pldo }, + + { "lvs1", QCOM_RPM_PM8921_LVS1, &pm8921_switch, "vin_lvs_1_3_6" }, + { "lvs2", QCOM_RPM_PM8921_LVS2, &pm8921_switch, "vin_lvs2" }, + { "lvs3", QCOM_RPM_PM8921_LVS3, &pm8921_switch, "vin_lvs_1_3_6" }, + { "lvs4", QCOM_RPM_PM8921_LVS4, &pm8921_switch, "vin_lvs_4_5_7" }, + { "lvs5", QCOM_RPM_PM8921_LVS5, &pm8921_switch, "vin_lvs_4_5_7" }, + { "lvs6", QCOM_RPM_PM8921_LVS6, &pm8921_switch, "vin_lvs_1_3_6" }, + { "lvs7", QCOM_RPM_PM8921_LVS7, &pm8921_switch, "vin_lvs_4_5_7" }, + + { "usb-switch", QCOM_RPM_USB_OTG_SWITCH, &pm8921_switch }, + { "hdmi-switch", QCOM_RPM_HDMI_SWITCH, &pm8921_switch }, + { "ncp", QCOM_RPM_PM8921_NCP, &pm8921_ncp, "vin_ncp" }, + { } +}; +static const struct of_device_id rpm_of_match[] = { + { .compatible = "qcom,rpm-pm8058-regulators", .data = &rpm_pm8058_regulators }, + { .compatible = "qcom,rpm-pm8901-regulators", .data = &rpm_pm8901_regulators }, + { .compatible = "qcom,rpm-pm8921-regulators", .data = &rpm_pm8921_regulators }, + { } +}; +MODULE_DEVICE_TABLE(of, rpm_of_match); - config.dev = &pdev->dev; - config.init_data = initdata; - config.driver_data = vreg; - config.of_node = pdev->dev.of_node; +static int rpm_reg_probe(struct platform_device *pdev) +{ + const struct rpm_regulator_data *reg; + const struct of_device_id *match; + struct regulator_config config = { }; + struct regulator_dev *rdev; + struct qcom_rpm_reg *vreg; - ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc, - &config, initdata); - if (ret) - return ret; + match = of_match_device(rpm_of_match, &pdev->dev); + for (reg = match->data; reg->name; reg++) { + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); + if (!vreg) { + dev_err(&pdev->dev, "failed to allocate vreg\n"); + return -ENOMEM; + } + memcpy(vreg, reg->template, sizeof(*vreg)); + mutex_init(&vreg->lock); + + vreg->dev = &pdev->dev; + vreg->resource = reg->resource; + + vreg->desc.id = -1; + vreg->desc.owner = THIS_MODULE; + vreg->desc.type = REGULATOR_VOLTAGE; + vreg->desc.name = reg->name; + vreg->desc.supply_name = reg->supply; + vreg->desc.of_match = reg->name; + vreg->desc.of_parse_cb = rpm_reg_of_parse; + + vreg->rpm = dev_get_drvdata(pdev->dev.parent); + if (!vreg->rpm) { + dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); + return -ENODEV; + } - rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); - if (IS_ERR(rdev)) { - dev_err(&pdev->dev, "can't register regulator\n"); - return PTR_ERR(rdev); + config.dev = &pdev->dev; + config.driver_data = vreg; + rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); + if (IS_ERR(rdev)) { + dev_err(&pdev->dev, "can't register regulator\n"); + return PTR_ERR(rdev); + } } return 0; -- 1.8.2.2 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 4:25 ` [PATCH 4/4] regulator: qcom: Rework to single platform device Bjorn Andersson @ 2015-03-03 22:09 ` Stephen Boyd 2015-03-03 22:32 ` Bjorn Andersson 2015-03-04 19:35 ` Stephen Boyd 1 sibling, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-03 22:09 UTC (permalink / raw) To: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/02/15 20:25, Bjorn Andersson wrote: > - config.of_node = pdev->dev.of_node; > +static int rpm_reg_probe(struct platform_device *pdev) > +{ > + const struct rpm_regulator_data *reg; > + const struct of_device_id *match; > + struct regulator_config config = { }; > + struct regulator_dev *rdev; > + struct qcom_rpm_reg *vreg; > > - ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc, > - &config, initdata); > - if (ret) > - return ret; > + match = of_match_device(rpm_of_match, &pdev->dev); > + for (reg = match->data; reg->name; reg++) { How does this work for the case where we may not want to add all the regulators that a PMIC supports. I'm mostly thinking about the case where we want to use the pm8xxx-regulator driver for a few regulators and so we omit them from the DT for the RPM regulators. -Stephen > + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); > + if (!vreg) { > + dev_err(&pdev->dev, "failed to allocate vreg\n"); > + return -ENOMEM; > + } > + memcpy(vreg, reg->template, sizeof(*vreg)); > + mutex_init(&vreg->lock); > + > + vreg->dev = &pdev->dev; > + vreg->resource = reg->resource; > + > + vreg->desc.id = -1; > + vreg->desc.owner = THIS_MODULE; > + vreg->desc.type = REGULATOR_VOLTAGE; > + vreg->desc.name = reg->name; > + vreg->desc.supply_name = reg->supply; > + vreg->desc.of_match = reg->name; > + vreg->desc.of_parse_cb = rpm_reg_of_parse; > + > + vreg->rpm = dev_get_drvdata(pdev->dev.parent); > + if (!vreg->rpm) { > + dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); > + return -ENODEV; > + } > > - rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > - if (IS_ERR(rdev)) { > - dev_err(&pdev->dev, "can't register regulator\n"); > - return PTR_ERR(rdev); > + config.dev = &pdev->dev; > + config.driver_data = vreg; > + rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > + if (IS_ERR(rdev)) { > + dev_err(&pdev->dev, "can't register regulator\n"); > + return PTR_ERR(rdev); > + } > } > > return 0; -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 22:09 ` Stephen Boyd @ 2015-03-03 22:32 ` Bjorn Andersson 2015-03-03 23:52 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-03 22:32 UTC (permalink / raw) To: Stephen Boyd Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote: > On 03/02/15 20:25, Bjorn Andersson wrote: > > - config.of_node = pdev->dev.of_node; > > +static int rpm_reg_probe(struct platform_device *pdev) > > +{ > > + const struct rpm_regulator_data *reg; > > + const struct of_device_id *match; > > + struct regulator_config config = { }; > > + struct regulator_dev *rdev; > > + struct qcom_rpm_reg *vreg; > > > > - ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc, > > - &config, initdata); > > - if (ret) > > - return ret; > > + match = of_match_device(rpm_of_match, &pdev->dev); > > + for (reg = match->data; reg->name; reg++) { > > How does this work for the case where we may not want to add all the > regulators that a PMIC supports. I'm mostly thinking about the case > where we want to use the pm8xxx-regulator driver for a few regulators > and so we omit them from the DT for the RPM regulators. > An empty or non-existing regulator of_node will still be registered, but without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any operation on this regulator will fail with an -EPERM. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 22:32 ` Bjorn Andersson @ 2015-03-03 23:52 ` Mark Brown 2015-03-04 0:01 ` Stephen Boyd 0 siblings, 1 reply; 31+ messages in thread From: Mark Brown @ 2015-03-03 23:52 UTC (permalink / raw) To: Bjorn Andersson Cc: Stephen Boyd, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 693 bytes --] On Tue, Mar 03, 2015 at 02:32:35PM -0800, Bjorn Andersson wrote: > On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote: > > How does this work for the case where we may not want to add all the > > regulators that a PMIC supports. I'm mostly thinking about the case > > where we want to use the pm8xxx-regulator driver for a few regulators > > and so we omit them from the DT for the RPM regulators. > An empty or non-existing regulator of_node will still be registered, but > without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any > operation on this regulator will fail with an -EPERM. ...but of course we'd never try any operations on it anyway as there would be no consumers. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 23:52 ` Mark Brown @ 2015-03-04 0:01 ` Stephen Boyd 2015-03-04 0:09 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-04 0:01 UTC (permalink / raw) To: Mark Brown, Bjorn Andersson Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/03/15 15:52, Mark Brown wrote: > On Tue, Mar 03, 2015 at 02:32:35PM -0800, Bjorn Andersson wrote: >> On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote: >>> How does this work for the case where we may not want to add all the >>> regulators that a PMIC supports. I'm mostly thinking about the case >>> where we want to use the pm8xxx-regulator driver for a few regulators >>> and so we omit them from the DT for the RPM regulators. >> An empty or non-existing regulator of_node will still be registered, but >> without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any >> operation on this regulator will fail with an -EPERM. > ...but of course we'd never try any operations on it anyway as there > would be no consumers. Yes sounds fine. The only concern is that we're probably wasting memory with things that won't ever "match" something in DT. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-04 0:01 ` Stephen Boyd @ 2015-03-04 0:09 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2015-03-04 0:09 UTC (permalink / raw) To: Stephen Boyd Cc: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 526 bytes --] On Tue, Mar 03, 2015 at 04:01:43PM -0800, Stephen Boyd wrote: > On 03/03/15 15:52, Mark Brown wrote: > > ...but of course we'd never try any operations on it anyway as there > > would be no consumers. > Yes sounds fine. The only concern is that we're probably wasting memory > with things that won't ever "match" something in DT. You still get diagnostic output about what the regulator state is which is useful in itself and the amount of memory involved is really not enough to worry about in the grand scheme of things. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-03 4:25 ` [PATCH 4/4] regulator: qcom: Rework to single platform device Bjorn Andersson 2015-03-03 22:09 ` Stephen Boyd @ 2015-03-04 19:35 ` Stephen Boyd 2015-03-04 23:51 ` Bjorn Andersson 2015-03-05 0:30 ` Mark Brown 1 sibling, 2 replies; 31+ messages in thread From: Stephen Boyd @ 2015-03-04 19:35 UTC (permalink / raw) To: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross Cc: Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/02/15 20:25, Bjorn Andersson wrote: > + match = of_match_device(rpm_of_match, &pdev->dev); > + for (reg = match->data; reg->name; reg++) { > + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); > + if (!vreg) { > + dev_err(&pdev->dev, "failed to allocate vreg\n"); > + return -ENOMEM; > + } > + memcpy(vreg, reg->template, sizeof(*vreg)); > + mutex_init(&vreg->lock); > + > + vreg->dev = &pdev->dev; > + vreg->resource = reg->resource; > + > + vreg->desc.id = -1; > + vreg->desc.owner = THIS_MODULE; > + vreg->desc.type = REGULATOR_VOLTAGE; > + vreg->desc.name = reg->name; > + vreg->desc.supply_name = reg->supply; > + vreg->desc.of_match = reg->name; > + vreg->desc.of_parse_cb = rpm_reg_of_parse; > + > + vreg->rpm = dev_get_drvdata(pdev->dev.parent); > + if (!vreg->rpm) { > + dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); > + return -ENODEV; > + } > > - rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > - if (IS_ERR(rdev)) { > - dev_err(&pdev->dev, "can't register regulator\n"); > - return PTR_ERR(rdev); > + config.dev = &pdev->dev; > + config.driver_data = vreg; > + rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > + if (IS_ERR(rdev)) { > + dev_err(&pdev->dev, "can't register regulator\n"); > + return PTR_ERR(rdev); > + } > } > > return 0; There's another problem with this of_parse_cb design. The regulator framework requires supplies to be registered before consumers of the supplies are registered. So when we register L23 we need to make sure it's supply is already registered (S8 for example). If we used of_regulator_match() we could sort the array by hand so that we always register the supplies first. Or we could modify the regulator framework to have a concept of "orphaned" supplies like the clock framework has so that when a regulator is registered it waits until its supply is registered. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-04 19:35 ` Stephen Boyd @ 2015-03-04 23:51 ` Bjorn Andersson 2015-03-05 0:56 ` Mark Brown 2015-03-05 0:30 ` Mark Brown 1 sibling, 1 reply; 31+ messages in thread From: Bjorn Andersson @ 2015-03-04 23:51 UTC (permalink / raw) To: Stephen Boyd Cc: Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Brown, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On Wed 04 Mar 11:35 PST 2015, Stephen Boyd wrote: > On 03/02/15 20:25, Bjorn Andersson wrote: > > + match = of_match_device(rpm_of_match, &pdev->dev); > > + for (reg = match->data; reg->name; reg++) { > > + vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL); > > + if (!vreg) { > > + dev_err(&pdev->dev, "failed to allocate vreg\n"); > > + return -ENOMEM; > > + } > > + memcpy(vreg, reg->template, sizeof(*vreg)); > > + mutex_init(&vreg->lock); > > + > > + vreg->dev = &pdev->dev; > > + vreg->resource = reg->resource; > > + > > + vreg->desc.id = -1; > > + vreg->desc.owner = THIS_MODULE; > > + vreg->desc.type = REGULATOR_VOLTAGE; > > + vreg->desc.name = reg->name; > > + vreg->desc.supply_name = reg->supply; > > + vreg->desc.of_match = reg->name; > > + vreg->desc.of_parse_cb = rpm_reg_of_parse; > > + > > + vreg->rpm = dev_get_drvdata(pdev->dev.parent); > > + if (!vreg->rpm) { > > + dev_err(&pdev->dev, "unable to retrieve handle to rpm\n"); > > + return -ENODEV; > > + } > > > > - rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > > - if (IS_ERR(rdev)) { > > - dev_err(&pdev->dev, "can't register regulator\n"); > > - return PTR_ERR(rdev); > > + config.dev = &pdev->dev; > > + config.driver_data = vreg; > > + rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config); > > + if (IS_ERR(rdev)) { > > + dev_err(&pdev->dev, "can't register regulator\n"); > > + return PTR_ERR(rdev); > > + } > > } > > > > return 0; > > There's another problem with this of_parse_cb design. The regulator > framework requires supplies to be registered before consumers of the > supplies are registered. You're right, didn't think of that. The way I've ordered pm8921 it happens to work, but neither 8058 nor 8901 will pass probe by filling in the dependencies according to what we have in the caf branch. > So when we register L23 we need to make sure > it's supply is already registered (S8 for example). If we used > of_regulator_match() we could sort the array by hand so that we always > register the supplies first. Right, but that would mean that any block of regulators with internal dependencies would miss out on the "standard" code paths through regulator_register() and have to implement this on their own. Just looking at the Qualcomm case, we would have to implement this sorting mechanism in the rpm-regulator, pm8xxx-regulator, smd-regulator and qpnp-regulator drivers. I took a stab at implementing EPROBE_DEFER within qcom_rpm-regulator, but as it's a mixture of internal and external dependencies (e.g. <&pm8921_s4> vs <&pm8921_mpp 7>) this became quite messy. I started looking at using the dt dependencies sort and iterate over the entries in a way that adheres to their dependencies, but that's also a lot of code. But... > Or we could modify the regulator framework > to have a concept of "orphaned" supplies like the clock framework has so > that when a regulator is registered it waits until its supply is registered. > ...as we're grouping all regulators into one device per PMIC we create artificial dependencies that the hardware guys does not have. Further more the fact that we can have some parts of the pmic exposed through the rpm driver and others through the direct driver we have yet another level of possible just adds to this. It's perfectly fine to route the dependencies between two of our PMICs in a way that on a device level we have a circular dependency. So I think you're right, we should be able to alter the supply lookup code to defer the EPROBE_DEFER until we actually need the supply to be there; e.g. attempt to map supplies when an external consumer request the regulator. Some care needs to be taken with regards to e.g. always-on regulators. Regards, Bjorn ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-04 23:51 ` Bjorn Andersson @ 2015-03-05 0:56 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2015-03-05 0:56 UTC (permalink / raw) To: Bjorn Andersson Cc: Stephen Boyd, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 1622 bytes --] On Wed, Mar 04, 2015 at 03:51:46PM -0800, Bjorn Andersson wrote: > I took a stab at implementing EPROBE_DEFER within qcom_rpm-regulator, > but as it's a mixture of internal and external dependencies (e.g. > <&pm8921_s4> vs <&pm8921_mpp 7>) this became quite messy. I started > looking at using the dt dependencies sort and iterate over the entries > in a way that adheres to their dependencies, but that's also a lot of > code. This is why I don't like trying to open code in subsystems, it's too much work. > So I think you're right, we should be able to alter the supply lookup > code to defer the EPROBE_DEFER until we actually need the supply to be > there; e.g. attempt to map supplies when an external consumer request > the regulator. > Some care needs to be taken with regards to e.g. always-on regulators. I'm not sure why always on regulators would need special casing here? Enabling is orthogonal to supply mapping. Like I said in reply to Stephen's mail I'm more worried about discoverability of problems with this approach and with interactions with dependencies on other subsystems (mainly GPIOs). Thinking about it some more the other subsystems will probably sort themselves out but the diagnosics are an issue. I do like the idea of a general mechanism for registering dependency resources and deferring completion until they're ready more - I'm not sure it's even that much more work, especially if the first cut only handles regulators as a dependency... that feels like attacking the right problem, there's other things people are raising with deferred probe like your complaint about logging. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-04 19:35 ` Stephen Boyd 2015-03-04 23:51 ` Bjorn Andersson @ 2015-03-05 0:30 ` Mark Brown 2015-03-05 1:46 ` Stephen Boyd 1 sibling, 1 reply; 31+ messages in thread From: Mark Brown @ 2015-03-05 0:30 UTC (permalink / raw) To: Stephen Boyd Cc: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 2020 bytes --] On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote: > There's another problem with this of_parse_cb design. The regulator > framework requires supplies to be registered before consumers of the > supplies are registered. So when we register L23 we need to make sure > it's supply is already registered (S8 for example). If we used > of_regulator_match() we could sort the array by hand so that we always > register the supplies first. Or we could modify the regulator framework > to have a concept of "orphaned" supplies like the clock framework has so > that when a regulator is registered it waits until its supply is registered. Dependency resolution isn't anything new, I'm not sure why you think this is related to of_parse_cb()? Open coding does exactly the same thing and the ability to have device specific properties on is not obviously related to dependency resolution so perhaps I'm misunderstanding what you're talking about... If you *are* talking about dependency resolution then just bulk registering the regulators (which we should be doing anyway) and then iterating the array until we stop making progress should do the trick for most cases, normally there's only one PMIC in play, or have people who care do a single struct device per regulator and then let the probe deferral stuff sort it out. I'm a bit nervous of the idea of having orphaning without core support, it's not just inter regulator dependencies we need to worry about (they use GPIOs and so on) and there's concerns about debuggability but it is the kind of thing probe deferral was meant to be a cheap implementation of. There was a proposal quite recently from someone at Samsung Poland to do something more coreish and basically split registrations in two, one half registering the things needed for each resource and then a second half which runs once the required resources are registered. Pushing that along might be best, it's a more general approach. The component stuff Russell did has some similarities here. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-05 0:30 ` Mark Brown @ 2015-03-05 1:46 ` Stephen Boyd 2015-03-05 10:38 ` Mark Brown 0 siblings, 1 reply; 31+ messages in thread From: Stephen Boyd @ 2015-03-05 1:46 UTC (permalink / raw) To: Mark Brown Cc: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm On 03/04/15 16:30, Mark Brown wrote: > On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote: > >> There's another problem with this of_parse_cb design. The regulator >> framework requires supplies to be registered before consumers of the >> supplies are registered. So when we register L23 we need to make sure >> it's supply is already registered (S8 for example). If we used >> of_regulator_match() we could sort the array by hand so that we always >> register the supplies first. Or we could modify the regulator framework >> to have a concept of "orphaned" supplies like the clock framework has so >> that when a regulator is registered it waits until its supply is registered. > Dependency resolution isn't anything new, I'm not sure why you think > this is related to of_parse_cb()? Open coding does exactly the same > thing and the ability to have device specific properties on is not > obviously related to dependency resolution so perhaps I'm > misunderstanding what you're talking about... I was just using of_parse_cb to indicate the difference from of_regulator_match(). I could have said "between your design and my design". > > If you *are* talking about dependency resolution then just bulk > registering the regulators (which we should be doing anyway) and then > iterating the array until we stop making progress should do the trick > for most cases, normally there's only one PMIC in play, or have people > who care do a single struct device per regulator and then let the probe > deferral stuff sort it out. Yeah I think the brute force, keep trying approach will work for now if we can't add orphan support to regulator core. I'm not aware of any circular dependencies for these platforms. > > I'm a bit nervous of the idea of having orphaning without core support, > it's not just inter regulator dependencies we need to worry about (they > use GPIOs and so on) and there's concerns about debuggability but it is > the kind of thing probe deferral was meant to be a cheap implementation > of. There was a proposal quite recently from someone at Samsung Poland > to do something more coreish and basically split registrations in two, > one half registering the things needed for each resource and then a > second half which runs once the required resources are registered. > Pushing that along might be best, it's a more general approach. The > component stuff Russell did has some similarities here. Ah you're talking about the res track stuff[1]? That patchset seemed to be doing a *lot* of different stuff where probe defer was just a part of it. Frmo what I recall that still operates on the device level, where here we want to be able to say that a particular regulator needs another resource, but it's ok to register it's sibling regulator within this device because that regulator doesn't need anything. Are you saying you want the restrack stuff to work at the regulator level? If we went that way we could do the same thing in the clock framework and get rid of the orphan list and rely on the notifications from restrack to figure out when a clock resource becomes fully available. [1] https://lkml.org/lkml/2014/12/10/342 -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] regulator: qcom: Rework to single platform device 2015-03-05 1:46 ` Stephen Boyd @ 2015-03-05 10:38 ` Mark Brown 0 siblings, 0 replies; 31+ messages in thread From: Mark Brown @ 2015-03-05 10:38 UTC (permalink / raw) To: Stephen Boyd Cc: Bjorn Andersson, Ian Campbell, Kumar Gala, Lee Jones, Liam Girdwood, Mark Rutland, Pawel Moll, Rob Herring, Andy Gross, Chanwoo Choi, Krzysztof Kozlowski, Srinivas Kandagatla, devicetree, linux-kernel, linux-arm-msm [-- Attachment #1: Type: text/plain, Size: 1947 bytes --] On Wed, Mar 04, 2015 at 05:46:25PM -0800, Stephen Boyd wrote: > On 03/04/15 16:30, Mark Brown wrote: > > On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote: > > Dependency resolution isn't anything new, I'm not sure why you think > > this is related to of_parse_cb()? Open coding does exactly the same > I was just using of_parse_cb to indicate the difference from > of_regulator_match(). I could have said "between your design and my design". Please do things like that - it makes things harder to follow if people throw in random unrelated terms for things. > > of. There was a proposal quite recently from someone at Samsung Poland > > to do something more coreish and basically split registrations in two, > > one half registering the things needed for each resource and then a > > second half which runs once the required resources are registered. > > Pushing that along might be best, it's a more general approach. The > > component stuff Russell did has some similarities here. > Ah you're talking about the res track stuff[1]? That patchset seemed to > be doing a *lot* of different stuff where probe defer was just a part of > it. Frmo what I recall that still operates on the device level, where > here we want to be able to say that a particular regulator needs another > resource, but it's ok to register it's sibling regulator within this > device because that regulator doesn't need anything. Are you saying you > want the restrack stuff to work at the regulator level? If we went that > way we could do the same thing in the clock framework and get rid of the > orphan list and rely on the notifications from restrack to figure out > when a clock resource becomes fully available. Yes, that's it. It's not quite the same thing as registering individual resources and there are definite issues to be addressed but I think it's a promising approach for addressing the deferred probe problem in a more elegant way. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2015-03-05 10:38 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-03 4:25 [PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device Bjorn Andersson 2015-03-03 4:25 ` [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes Bjorn Andersson 2015-03-03 12:47 ` Mark Brown 2015-03-03 16:02 ` Bjorn Andersson 2015-03-05 0:33 ` Mark Brown 2015-03-03 18:53 ` Stephen Boyd 2015-03-03 21:54 ` Bjorn Andersson 2015-03-03 22:02 ` Stephen Boyd 2015-03-03 22:17 ` Bjorn Andersson 2015-03-03 23:25 ` Stephen Boyd 2015-03-03 4:25 ` [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb Bjorn Andersson 2015-03-03 12:50 ` Mark Brown 2015-03-03 16:15 ` Bjorn Andersson 2015-03-05 0:42 ` Mark Brown 2015-03-03 4:25 ` [PATCH 3/4] regulator: qcom: Refactor of-parsing code Bjorn Andersson 2015-03-03 14:13 ` Mark Brown 2015-03-03 16:26 ` Bjorn Andersson 2015-03-03 18:56 ` Stephen Boyd 2015-03-03 22:07 ` Bjorn Andersson 2015-03-03 4:25 ` [PATCH 4/4] regulator: qcom: Rework to single platform device Bjorn Andersson 2015-03-03 22:09 ` Stephen Boyd 2015-03-03 22:32 ` Bjorn Andersson 2015-03-03 23:52 ` Mark Brown 2015-03-04 0:01 ` Stephen Boyd 2015-03-04 0:09 ` Mark Brown 2015-03-04 19:35 ` Stephen Boyd 2015-03-04 23:51 ` Bjorn Andersson 2015-03-05 0:56 ` Mark Brown 2015-03-05 0:30 ` Mark Brown 2015-03-05 1:46 ` Stephen Boyd 2015-03-05 10:38 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).