* [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable()
2020-06-02 10:09 [PATCH v4 0/5] Qualcomm labibb regulator driver Sumit Semwal
@ 2020-06-02 10:09 ` Sumit Semwal
2020-06-02 11:24 ` Mark Brown
2020-06-18 23:44 ` Bjorn Andersson
2020-06-02 10:09 ` [PATCH v4 2/5] dt-bindings: regulator: Add labibb regulator Sumit Semwal
` (3 subsequent siblings)
4 siblings, 2 replies; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 10:09 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt
Cc: nishakumari, linux-arm-msm, linux-kernel, devicetree, kgunda,
rnayak, Sumit Semwal
Some regulators might need to verify that they have indeed been enabled
after the enable() call is made and enable_time delay has passed.
This is implemented by repeatedly checking is_enabled() upto
poll_enabled_time, waiting for the already calculated enable delay in
each iteration.
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
--
v2: Address review comments, including swapping enable_time and poll_enabled_time.
---
drivers/regulator/core.c | 58 +++++++++++++++++++++++++++++++-
include/linux/regulator/driver.h | 5 +++
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7486f6e4e613..d9ab888da95f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2347,6 +2347,32 @@ static void _regulator_enable_delay(unsigned int delay)
udelay(us);
}
+/* _regulator_check_status_enabled
+ *
+ * returns:
+ * 1 if status shows regulator is in enabled state
+ * 0 if not enabled state
+ * else, error value as received from ops->get_status()
+ */
+static inline int _regulator_check_status_enabled(struct regulator_dev *rdev)
+{
+ int ret = rdev->desc->ops->get_status(rdev);
+
+ if (ret < 0) {
+ rdev_info(rdev, "get_status returned error: %d\n", ret);
+ return ret;
+ }
+
+ switch (ret) {
+ case REGULATOR_STATUS_OFF:
+ case REGULATOR_STATUS_ERROR:
+ case REGULATOR_STATUS_UNDEFINED:
+ return 0;
+ default:
+ return 1;
+ }
+}
+
static int _regulator_do_enable(struct regulator_dev *rdev)
{
int ret, delay;
@@ -2407,7 +2433,37 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
* together. */
trace_regulator_enable_delay(rdev_get_name(rdev));
- _regulator_enable_delay(delay);
+ /* If poll_enabled_time is set, poll upto the delay calculated
+ * above, delaying poll_enabled_time uS to check if the regulator
+ * actually got enabled.
+ * If the regulator isn't enabled after enable_delay has
+ * expired, return -ETIMEDOUT.
+ */
+ if (rdev->desc->poll_enabled_time) {
+ unsigned int time_remaining = delay;
+
+ while (time_remaining > 0) {
+ _regulator_enable_delay(rdev->desc->poll_enabled_time);
+
+ if (rdev->desc->ops->get_status) {
+ ret = _regulator_check_status_enabled(rdev);
+ if (ret < 0)
+ return ret;
+ else if (ret)
+ break;
+ } else if (rdev->desc->ops->is_enabled(rdev))
+ break;
+
+ time_remaining -= rdev->desc->poll_enabled_time;
+ }
+
+ if (time_remaining <= 0) {
+ rdev_err(rdev, "Enabled check failed.\n");
+ return -ETIMEDOUT;
+ }
+ } else {
+ _regulator_enable_delay(delay);
+ }
trace_regulator_enable_complete(rdev_get_name(rdev));
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 29d920516e0b..bb50e943010f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -322,6 +322,9 @@ enum regulator_type {
* @enable_time: Time taken for initial enable of regulator (in uS).
* @off_on_delay: guard time (in uS), before re-enabling a regulator
*
+ * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
+ * actually enabled, after enable() call
+ *
* @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
*/
struct regulator_desc {
@@ -389,6 +392,8 @@ struct regulator_desc {
unsigned int off_on_delay;
+ unsigned int poll_enabled_time;
+
unsigned int (*of_map_mode)(unsigned int mode);
};
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable()
2020-06-02 10:09 ` [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable() Sumit Semwal
@ 2020-06-02 11:24 ` Mark Brown
2020-06-02 11:57 ` Sumit Semwal
2020-06-18 23:44 ` Bjorn Andersson
1 sibling, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-02 11:24 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, bjorn.andersson, lgirdwood, robh+dt, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
[-- Attachment #1: Type: text/plain, Size: 409 bytes --]
On Tue, Jun 02, 2020 at 03:39:20PM +0530, Sumit Semwal wrote:
> +
> + if (time_remaining <= 0) {
> + rdev_err(rdev, "Enabled check failed.\n");
> + return -ETIMEDOUT;
s/failed/timed out/
> + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
> + * actually enabled, after enable() call
> + *
This comment needs updating to reflect the new implementation.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable()
2020-06-02 11:24 ` Mark Brown
@ 2020-06-02 11:57 ` Sumit Semwal
0 siblings, 0 replies; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 11:57 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
Hi Mark,
Thanks for the review!
On Tue, 2 Jun 2020 at 16:54, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 03:39:20PM +0530, Sumit Semwal wrote:
>
> > +
> > + if (time_remaining <= 0) {
> > + rdev_err(rdev, "Enabled check failed.\n");
> > + return -ETIMEDOUT;
>
> s/failed/timed out/
Ack
>
> > + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
> > + * actually enabled, after enable() call
> > + *
>
> This comment needs updating to reflect the new implementation.
Yes, I will update.
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable()
2020-06-02 10:09 ` [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable() Sumit Semwal
2020-06-02 11:24 ` Mark Brown
@ 2020-06-18 23:44 ` Bjorn Andersson
1 sibling, 0 replies; 22+ messages in thread
From: Bjorn Andersson @ 2020-06-18 23:44 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, lgirdwood, broonie, robh+dt, nishakumari, linux-arm-msm,
linux-kernel, devicetree, kgunda, rnayak
On Tue 02 Jun 03:09 PDT 2020, Sumit Semwal wrote:
> Some regulators might need to verify that they have indeed been enabled
> after the enable() call is made and enable_time delay has passed.
>
> This is implemented by repeatedly checking is_enabled() upto
> poll_enabled_time, waiting for the already calculated enable delay in
> each iteration.
>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> --
> v2: Address review comments, including swapping enable_time and poll_enabled_time.
> ---
> drivers/regulator/core.c | 58 +++++++++++++++++++++++++++++++-
> include/linux/regulator/driver.h | 5 +++
> 2 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 7486f6e4e613..d9ab888da95f 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2347,6 +2347,32 @@ static void _regulator_enable_delay(unsigned int delay)
> udelay(us);
> }
>
> +/* _regulator_check_status_enabled
Please make all your kerneldoc follow:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> + *
> + * returns:
> + * 1 if status shows regulator is in enabled state
> + * 0 if not enabled state
> + * else, error value as received from ops->get_status()
> + */
> +static inline int _regulator_check_status_enabled(struct regulator_dev *rdev)
> +{
> + int ret = rdev->desc->ops->get_status(rdev);
> +
> + if (ret < 0) {
> + rdev_info(rdev, "get_status returned error: %d\n", ret);
> + return ret;
> + }
> +
> + switch (ret) {
> + case REGULATOR_STATUS_OFF:
> + case REGULATOR_STATUS_ERROR:
> + case REGULATOR_STATUS_UNDEFINED:
> + return 0;
> + default:
> + return 1;
> + }
> +}
> +
> static int _regulator_do_enable(struct regulator_dev *rdev)
> {
> int ret, delay;
> @@ -2407,7 +2433,37 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
> * together. */
> trace_regulator_enable_delay(rdev_get_name(rdev));
>
> - _regulator_enable_delay(delay);
> + /* If poll_enabled_time is set, poll upto the delay calculated
> + * above, delaying poll_enabled_time uS to check if the regulator
> + * actually got enabled.
> + * If the regulator isn't enabled after enable_delay has
> + * expired, return -ETIMEDOUT.
> + */
> + if (rdev->desc->poll_enabled_time) {
> + unsigned int time_remaining = delay;
> +
> + while (time_remaining > 0) {
> + _regulator_enable_delay(rdev->desc->poll_enabled_time);
> +
> + if (rdev->desc->ops->get_status) {
> + ret = _regulator_check_status_enabled(rdev);
> + if (ret < 0)
> + return ret;
> + else if (ret)
> + break;
> + } else if (rdev->desc->ops->is_enabled(rdev))
> + break;
> +
> + time_remaining -= rdev->desc->poll_enabled_time;
> + }
> +
> + if (time_remaining <= 0) {
> + rdev_err(rdev, "Enabled check failed.\n");
> + return -ETIMEDOUT;
> + }
> + } else {
> + _regulator_enable_delay(delay);
> + }
>
> trace_regulator_enable_complete(rdev_get_name(rdev));
>
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index 29d920516e0b..bb50e943010f 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -322,6 +322,9 @@ enum regulator_type {
> * @enable_time: Time taken for initial enable of regulator (in uS).
> * @off_on_delay: guard time (in uS), before re-enabling a regulator
> *
> + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is
> + * actually enabled, after enable() call
I read this as "how long should we stay in the poll loop". I think it
would be better describes as something like "The polling interval to use
while checking that the regulator was actually enabled".
Regards,
Bjorn
> + *
> * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode
> */
> struct regulator_desc {
> @@ -389,6 +392,8 @@ struct regulator_desc {
>
> unsigned int off_on_delay;
>
> + unsigned int poll_enabled_time;
> +
> unsigned int (*of_map_mode)(unsigned int mode);
> };
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 2/5] dt-bindings: regulator: Add labibb regulator
2020-06-02 10:09 [PATCH v4 0/5] Qualcomm labibb regulator driver Sumit Semwal
2020-06-02 10:09 ` [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable() Sumit Semwal
@ 2020-06-02 10:09 ` Sumit Semwal
2020-06-09 22:52 ` Rob Herring
2020-06-02 10:09 ` [PATCH v4 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators Sumit Semwal
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 10:09 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt
Cc: nishakumari, linux-arm-msm, linux-kernel, devicetree, kgunda,
rnayak, Sumit Semwal
From: Nisha Kumari <nishakumari@codeaurora.org>
Adding the devicetree binding for labibb regulator.
Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[sumits: cleanup as per review comments and update to yaml]
--
v2: updated for better compatible string and names.
v3: moved to yaml
v4: fixed dt_binding_check issues
---
.../regulator/qcom-labibb-regulator.yaml | 79 +++++++++++++++++++
1 file changed, 79 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
new file mode 100644
index 000000000000..178820ec04c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/qcom-labibb-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
+
+maintainers:
+ - Sumit Semwal <sumit.semwal@linaro.org>
+
+description:
+ LAB can be used as a positive boost power supply and IBB can be used as a
+ negative boost power supply for display panels. Currently implemented for
+ pmi8998.
+
+allOf:
+ - $ref: "regulator.yaml#"
+
+properties:
+ compatible:
+ const: qcom,pmi8998-lab-ibb
+
+ lab:
+ type: object
+
+ properties:
+
+ interrupts:
+ maxItems: 1
+ description:
+ Short-circuit interrupt for lab.
+
+ interrupt-names:
+ const: sc-err
+
+ required:
+ - interrupts
+ - interrupt-names
+
+ ibb:
+ type: object
+
+ properties:
+
+ interrupts:
+ maxItems: 1
+ description:
+ Short-circuit interrupt for lab.
+
+ interrupt-names:
+ const: sc-err
+
+ required:
+ - interrupts
+ - interrupt-names
+
+required:
+ - compatible
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ labibb {
+ compatible = "qcom,pmi8998-lab-ibb";
+
+ lab {
+ interrupts = <0x3 0x0 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "sc-err";
+ };
+
+ ibb {
+ interrupts = <0x3 0x2 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "sc-err";
+ };
+ };
+
+...
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 2/5] dt-bindings: regulator: Add labibb regulator
2020-06-02 10:09 ` [PATCH v4 2/5] dt-bindings: regulator: Add labibb regulator Sumit Semwal
@ 2020-06-09 22:52 ` Rob Herring
0 siblings, 0 replies; 22+ messages in thread
From: Rob Herring @ 2020-06-09 22:52 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, bjorn.andersson, lgirdwood, broonie, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
On Tue, Jun 02, 2020 at 03:39:21PM +0530, Sumit Semwal wrote:
> From: Nisha Kumari <nishakumari@codeaurora.org>
>
> Adding the devicetree binding for labibb regulator.
>
> Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> [sumits: cleanup as per review comments and update to yaml]
> --
> v2: updated for better compatible string and names.
> v3: moved to yaml
> v4: fixed dt_binding_check issues
> ---
> .../regulator/qcom-labibb-regulator.yaml | 79 +++++++++++++++++++
> 1 file changed, 79 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> new file mode 100644
> index 000000000000..178820ec04c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/qcom-labibb-regulator.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: GPL-2.0-only
Dual license new bindings:
(GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/qcom-labibb-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm's LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost) Regulator
> +
> +maintainers:
> + - Sumit Semwal <sumit.semwal@linaro.org>
> +
> +description:
> + LAB can be used as a positive boost power supply and IBB can be used as a
> + negative boost power supply for display panels. Currently implemented for
> + pmi8998.
> +
> +allOf:
> + - $ref: "regulator.yaml#"
I think you want this under each child as this schema applies to each
regulator. But you aren't using any of the regulator properties, so not
even needed? Or the example is not complete?
> +
> +properties:
> + compatible:
> + const: qcom,pmi8998-lab-ibb
> +
> + lab:
> + type: object
> +
> + properties:
> +
> + interrupts:
> + maxItems: 1
> + description:
> + Short-circuit interrupt for lab.
> +
> + interrupt-names:
> + const: sc-err
You don't really need a name if there's only 1.
> +
> + required:
> + - interrupts
> + - interrupt-names
> +
> + ibb:
> + type: object
> +
> + properties:
> +
> + interrupts:
> + maxItems: 1
> + description:
> + Short-circuit interrupt for lab.
> +
> + interrupt-names:
> + const: sc-err
> +
> + required:
> + - interrupts
> + - interrupt-names
> +
> +required:
> + - compatible
unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + labibb {
> + compatible = "qcom,pmi8998-lab-ibb";
> +
> + lab {
> + interrupts = <0x3 0x0 IRQ_TYPE_EDGE_RISING>;
> + interrupt-names = "sc-err";
> + };
> +
> + ibb {
> + interrupts = <0x3 0x2 IRQ_TYPE_EDGE_RISING>;
> + interrupt-names = "sc-err";
> + };
> + };
> +
> +...
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators
2020-06-02 10:09 [PATCH v4 0/5] Qualcomm labibb regulator driver Sumit Semwal
2020-06-02 10:09 ` [PATCH v4 1/5] regulator: Allow regulators to verify enabled during enable() Sumit Semwal
2020-06-02 10:09 ` [PATCH v4 2/5] dt-bindings: regulator: Add labibb regulator Sumit Semwal
@ 2020-06-02 10:09 ` Sumit Semwal
2020-06-02 10:09 ` [PATCH v4 4/5] regulator: qcom: Add labibb driver Sumit Semwal
2020-06-02 10:09 ` [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling Sumit Semwal
4 siblings, 0 replies; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 10:09 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt
Cc: nishakumari, linux-arm-msm, linux-kernel, devicetree, kgunda,
rnayak, Sumit Semwal
From: Nisha Kumari <nishakumari@codeaurora.org>
This patch adds devicetree nodes for LAB and IBB regulators.
Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[sumits: Updated for better compatible strings and names]
--
v2: sumits: updated for better compatible string and names
v3: sumits: updated interrupt-names as per review comments
v4: sumits: removed labibb label
---
arch/arm64/boot/dts/qcom/pmi8998.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index 23f9146a161e..3230b78b8048 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -25,5 +25,19 @@ pmi8998_lsid1: pmic@3 {
reg = <0x3 SPMI_USID>;
#address-cells = <1>;
#size-cells = <0>;
+
+ labibb {
+ compatible = "qcom,pmi8998-lab-ibb";
+
+ ibb: ibb {
+ interrupts = <0x3 0xdc 0x2 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "sc-err";
+ };
+
+ lab: lab {
+ interrupts = <0x3 0xde 0x0 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "sc-err";
+ };
+ };
};
};
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-02 10:09 [PATCH v4 0/5] Qualcomm labibb regulator driver Sumit Semwal
` (2 preceding siblings ...)
2020-06-02 10:09 ` [PATCH v4 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators Sumit Semwal
@ 2020-06-02 10:09 ` Sumit Semwal
2020-06-02 11:32 ` Mark Brown
2020-06-02 10:09 ` [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling Sumit Semwal
4 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 10:09 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt
Cc: nishakumari, linux-arm-msm, linux-kernel, devicetree, kgunda,
rnayak, Sumit Semwal
From: Nisha Kumari <nishakumari@codeaurora.org>
Qualcomm platforms have LAB(LCD AMOLED Boost)/IBB(Inverting Buck Boost)
regulators, labibb for short, which are used as power supply for
LCD Mode displays.
This patch adds labibb regulator driver for pmi8998 PMIC, found on
SDM845 platforms.
Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[sumits: reworked to driver for more common code, using core regulator
features, and using newly-added poll_enabled_time functionality
from core]
--
v2: sumits: reworked the driver for more common code, and addressed
review comments from v1
v3: sumits: addressed review comments from v2; moved to use core
regulator features like enable_time, off_on_delay, and the newly
added poll_enabled_time. Moved the check_enabled functionality
to core framework via poll_enabled_time.
v4: sumits: address review comments from v3, including cleaning up
register_labibb_regulator(), and adapted to updated meaning of
poll_enabled_time.
---
drivers/regulator/Kconfig | 10 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/qcom-labibb-regulator.c | 193 ++++++++++++++++++++++
3 files changed, 204 insertions(+)
create mode 100644 drivers/regulator/qcom-labibb-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f4b72cb098ef..58704a9fd05d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1167,5 +1167,15 @@ config REGULATOR_WM8994
This driver provides support for the voltage regulators on the
WM8994 CODEC.
+config REGULATOR_QCOM_LABIBB
+ tristate "QCOM LAB/IBB regulator support"
+ depends on SPMI || COMPILE_TEST
+ help
+ This driver supports Qualcomm's LAB/IBB regulators present on the
+ Qualcomm's PMIC chip pmi8998. QCOM LAB and IBB are SPMI
+ based PMIC implementations. LAB can be used as positive
+ boost regulator and IBB can be used as a negative boost regulator
+ for LCD display panel.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6610ee001d9a..5b313786c0e8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_REGULATOR_MT6323) += mt6323-regulator.o
obj-$(CONFIG_REGULATOR_MT6358) += mt6358-regulator.o
obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
new file mode 100644
index 000000000000..33b764ac69d1
--- /dev/null
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (c) 2020, The Linux Foundation. All rights reserved.
+
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define REG_PERPH_TYPE 0x04
+#define QCOM_LAB_TYPE 0x24
+#define QCOM_IBB_TYPE 0x20
+
+#define REG_LABIBB_STATUS1 0x08
+#define REG_LABIBB_ENABLE_CTL 0x46
+#define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
+#define LABIBB_CONTROL_ENABLE BIT(7)
+
+#define LAB_ENABLE_CTL_MASK BIT(7)
+#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6))
+
+#define LABIBB_OFF_ON_DELAY 1000
+#define LAB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 2)
+#define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10)
+#define LABIBB_POLL_ENABLED_TIME 1000
+
+struct labibb_regulator {
+ struct regulator_desc desc;
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator_dev *rdev;
+ u16 base;
+ u8 type;
+};
+
+struct labibb_regulator_data {
+ u16 base;
+ const char *name;
+ u8 type;
+ unsigned int enable_time;
+ unsigned int enable_mask;
+};
+
+static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ int ret;
+ unsigned int val;
+ struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+ ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val);
+ if (ret < 0) {
+ dev_err(reg->dev, "Read register failed ret = %d\n", ret);
+ return ret;
+ }
+ return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
+}
+
+static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
+{
+ return regulator_enable_regmap(rdev);
+}
+
+static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
+{
+ return regulator_disable_regmap(rdev);
+}
+
+static struct regulator_ops qcom_labibb_ops = {
+ .enable = qcom_labibb_regulator_enable,
+ .disable = qcom_labibb_regulator_disable,
+ .is_enabled = qcom_labibb_regulator_is_enabled,
+};
+
+static struct regulator_dev *register_labibb_regulator(struct labibb_regulator *reg,
+ const struct labibb_regulator_data *reg_data,
+ struct device_node *of_node)
+{
+ struct regulator_config cfg = {};
+ int ret;
+
+ reg->base = reg_data->base;
+ reg->type = reg_data->type;
+ reg->desc.enable_mask = reg_data->enable_mask;
+ reg->desc.enable_reg = reg->base + REG_LABIBB_ENABLE_CTL;
+ reg->desc.enable_val = LABIBB_CONTROL_ENABLE;
+ reg->desc.of_match = reg_data->name;
+ reg->desc.name = reg_data->name;
+ reg->desc.owner = THIS_MODULE;
+ reg->desc.type = REGULATOR_VOLTAGE;
+ reg->desc.ops = &qcom_labibb_ops;
+
+ reg->desc.enable_time = reg_data->enable_time;
+ reg->desc.poll_enabled_time = LABIBB_POLL_ENABLED_TIME;
+ reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
+
+ cfg.dev = reg->dev;
+ cfg.driver_data = reg;
+ cfg.regmap = reg->regmap;
+ cfg.of_node = of_node;
+
+ return devm_regulator_register(reg->dev, ®->desc, &cfg);
+}
+
+static const struct labibb_regulator_data pmi8998_labibb_data[] = {
+ {0xde00, "lab", QCOM_LAB_TYPE, LAB_ENABLE_TIME, LAB_ENABLE_CTL_MASK},
+ {0xdc00, "ibb", QCOM_IBB_TYPE, IBB_ENABLE_TIME, IBB_ENABLE_CTL_MASK},
+ { },
+};
+
+static const struct of_device_id qcom_labibb_match[] = {
+ { .compatible = "qcom,pmi8998-lab-ibb", .data = &pmi8998_labibb_data},
+ { },
+};
+MODULE_DEVICE_TABLE(of, qcom_labibb_match);
+
+static int qcom_labibb_regulator_probe(struct platform_device *pdev)
+{
+ struct labibb_regulator *labibb_reg;
+ struct device *dev = &pdev->dev;
+ struct device_node *child;
+ const struct of_device_id *match;
+ const struct labibb_regulator_data *reg_data;
+ struct regmap *reg_regmap;
+ unsigned int type;
+ int ret;
+
+ reg_regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!reg_regmap) {
+ dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
+ return -ENODEV;
+ }
+
+ match = of_match_device(qcom_labibb_match, &pdev->dev);
+ if (!match)
+ return -ENODEV;
+
+ for (reg_data = match->data; reg_data->name; reg_data++) {
+ child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
+
+ if (WARN_ON(child == NULL))
+ return -EINVAL;
+
+ /* Validate if the type of regulator is indeed
+ * what's mentioned in DT.
+ */
+ ret = regmap_read(reg_regmap, reg_data->base + REG_PERPH_TYPE,
+ &type);
+ if (ret < 0) {
+ dev_err(dev,
+ "Peripheral type read failed ret=%d\n",
+ ret);
+ return -EINVAL;
+ }
+
+ if (WARN_ON((type != QCOM_LAB_TYPE) && (type != QCOM_IBB_TYPE)) ||
+ WARN_ON(type != reg_data->type))
+ return -EINVAL;
+
+ labibb_reg = devm_kzalloc(&pdev->dev, sizeof(*labibb_reg),
+ GFP_KERNEL);
+ if (!labibb_reg)
+ return -ENOMEM;
+
+ labibb_reg->regmap = reg_regmap;
+ labibb_reg->dev = dev;
+
+ dev_info(dev, "Registering %s regulator\n", child->full_name);
+
+ labibb_reg->rdev = register_labibb_regulator(labibb_reg, reg_data, child);
+ if (IS_ERR(labibb_reg->rdev)) {
+ dev_err(dev,
+ "qcom_labibb: error registering %s : %d\n",
+ child->full_name, ret);
+ return PTR_ERR(labibb_reg->rdev);
+ }
+ }
+
+ return 0;
+}
+
+static struct platform_driver qcom_labibb_regulator_driver = {
+ .driver = {
+ .name = "qcom-lab-ibb-regulator",
+ .of_match_table = qcom_labibb_match,
+ },
+ .probe = qcom_labibb_regulator_probe,
+};
+module_platform_driver(qcom_labibb_regulator_driver);
+
+MODULE_DESCRIPTION("Qualcomm labibb driver");
+MODULE_LICENSE("GPL v2");
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-02 10:09 ` [PATCH v4 4/5] regulator: qcom: Add labibb driver Sumit Semwal
@ 2020-06-02 11:32 ` Mark Brown
2020-06-02 12:10 ` Sumit Semwal
0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-02 11:32 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, bjorn.andersson, lgirdwood, robh+dt, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]
On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote:
> +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + int ret;
> + unsigned int val;
> + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> + ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val);
> + if (ret < 0) {
> + dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> + return ret;
> + }
> + return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
> +}
This should be a get_status() callback...
> +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> +{
> + return regulator_enable_regmap(rdev);
> +}
> +
> +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> +{
> + return regulator_disable_regmap(rdev);
> +}
...is_enabled() should just be regulator_is_enabled_regmap() and these
functions should just be removed entirely, you can use the regmap
operations directly as the ops without the wrapper.
> + match = of_match_device(qcom_labibb_match, &pdev->dev);
> + if (!match)
> + return -ENODEV;
> +
> + for (reg_data = match->data; reg_data->name; reg_data++) {
> + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> +
> + if (WARN_ON(child == NULL))
> + return -EINVAL;
This feels like the DT bindings are confused - why do we need to search
like this?
> + dev_info(dev, "Registering %s regulator\n", child->full_name);
This is noise, remove it. The regulator framework will announce new
regulators anyway.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-02 11:32 ` Mark Brown
@ 2020-06-02 12:10 ` Sumit Semwal
2020-06-02 12:25 ` Mark Brown
0 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 12:10 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
Hi Mark,
Thank you very much for reviewing.
On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote:
>
> > +static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > + int ret;
> > + unsigned int val;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regmap_read(reg->regmap, reg->base + REG_LABIBB_STATUS1, &val);
> > + if (ret < 0) {
> > + dev_err(reg->dev, "Read register failed ret = %d\n", ret);
> > + return ret;
> > + }
> > + return !!(val & LABIBB_STATUS1_VREG_OK_BIT);
> > +}
>
> This should be a get_status() callback...
>
From my (limited) understanding of downstream code, it seemed like for
this set of regulators, the 'enabled' check is done via the
'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg
/ enable_mask ones. That's why I used it as is_enabled() callback.
I will try and check with the QC folks to clarify this point about
their hardware.
> > +static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + return regulator_enable_regmap(rdev);
> > +}
> > +
> > +static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> > +{
> > + return regulator_disable_regmap(rdev);
> > +}
>
> ...is_enabled() should just be regulator_is_enabled_regmap() and these
> functions should just be removed entirely, you can use the regmap
> operations directly as the ops without the wrapper.
The 2 wrappers are a precursor to the next patch, where we keep track
of regulator's enable status to check during SC handling.
>
> > + match = of_match_device(qcom_labibb_match, &pdev->dev);
> > + if (!match)
> > + return -ENODEV;
> > +
> > + for (reg_data = match->data; reg_data->name; reg_data++) {
> > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> > +
> > + if (WARN_ON(child == NULL))
> > + return -EINVAL;
>
> This feels like the DT bindings are confused - why do we need to search
> like this?
The WARN_ON? This was suggested by Bjorn to catch the case where the
DT binding for a PMIC instantiates only one of the regulators.
>
> > + dev_info(dev, "Registering %s regulator\n", child->full_name);
>
> This is noise, remove it. The regulator framework will announce new
> regulators anyway.
Agreed. will remove in the next iteration.
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-02 12:10 ` Sumit Semwal
@ 2020-06-02 12:25 ` Mark Brown
2020-06-17 11:42 ` Sumit Semwal
0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-02 12:25 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]
On Tue, Jun 02, 2020 at 05:40:45PM +0530, Sumit Semwal wrote:
> On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote:
> > This should be a get_status() callback...
> From my (limited) understanding of downstream code, it seemed like for
> this set of regulators, the 'enabled' check is done via the
> 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg
> / enable_mask ones. That's why I used it as is_enabled() callback.
> I will try and check with the QC folks to clarify this point about
> their hardware.
The way this is functioning at the minute the downstream code is just
buggy.
> > ...is_enabled() should just be regulator_is_enabled_regmap() and these
> > functions should just be removed entirely, you can use the regmap
> > operations directly as the ops without the wrapper.
> The 2 wrappers are a precursor to the next patch, where we keep track
> of regulator's enable status to check during SC handling.
Add the functions when they're useful, not before. TBH if the register
is write only you're probably better off adding a register cache.
> > > + match = of_match_device(qcom_labibb_match, &pdev->dev);
> > > + if (!match)
> > > + return -ENODEV;
> > > +
> > > + for (reg_data = match->data; reg_data->name; reg_data++) {
> > > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> > > +
> > > + if (WARN_ON(child == NULL))
> > > + return -EINVAL;
> >
> > This feels like the DT bindings are confused - why do we need to search
> > like this?
> The WARN_ON? This was suggested by Bjorn to catch the case where the
> DT binding for a PMIC instantiates only one of the regulators.
No, this whole loop - why this whole match and get child stuff?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-02 12:25 ` Mark Brown
@ 2020-06-17 11:42 ` Sumit Semwal
2020-06-17 11:47 ` Mark Brown
0 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-17 11:42 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
Hello Mark,
On Tue, 2 Jun 2020 at 17:55, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 05:40:45PM +0530, Sumit Semwal wrote:
> > On Tue, 2 Jun 2020 at 17:02, Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Jun 02, 2020 at 03:39:23PM +0530, Sumit Semwal wrote:
>
> > > This should be a get_status() callback...
>
> > From my (limited) understanding of downstream code, it seemed like for
> > this set of regulators, the 'enabled' check is done via the
> > 'REG_LABIBB_STATUS1 reg; for some reason, not via the same enable_reg
> > / enable_mask ones. That's why I used it as is_enabled() callback.
> > I will try and check with the QC folks to clarify this point about
> > their hardware.
>
> The way this is functioning at the minute the downstream code is just
> buggy.
Apologies for the delay in responding - I pinged the QC folks, and was
waiting for their reply but haven't got any response so far.
I tried your suggestion to use the ENABLE_CTL register for checking if
the regulator is actually enabled. In my limited testing on the Poco,
it seems like the STATUS1 register updates faster than the ENABLE_CTL
register, so on the device, I see noticeable lag when I use ENABLE_CTL
for is_enabled() check. [This is especially true for the IBB, which
takes longer to become usable than the LAB regulator.]
I understand from a pure regulators' correctness point of view,
ENABLE_CTL should be the one checked there, so I can change the patch
as you suggested, but there seems to be some performance penalty
there.
>
> > > ...is_enabled() should just be regulator_is_enabled_regmap() and these
> > > functions should just be removed entirely, you can use the regmap
> > > operations directly as the ops without the wrapper.
>
> > The 2 wrappers are a precursor to the next patch, where we keep track
> > of regulator's enable status to check during SC handling.
>
> Add the functions when they're useful, not before. TBH if the register
> is write only you're probably better off adding a register cache.
Agreed, I will remove the wrappers from here, using the regmap
functions, and add the wrappers with the SC handling patch.
>
> > > > + match = of_match_device(qcom_labibb_match, &pdev->dev);
> > > > + if (!match)
> > > > + return -ENODEV;
> > > > +
> > > > + for (reg_data = match->data; reg_data->name; reg_data++) {
> > > > + child = of_get_child_by_name(pdev->dev.of_node, reg_data->name);
> > > > +
> > > > + if (WARN_ON(child == NULL))
> > > > + return -EINVAL;
> > >
> > > This feels like the DT bindings are confused - why do we need to search
> > > like this?
>
> > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > DT binding for a PMIC instantiates only one of the regulators.
>
> No, this whole loop - why this whole match and get child stuff?
This loop mechanism is what I saw in the other qcom regulators
upstream, so thought it was an acceptable way.
For the two children nodes, do you recommend another mechanism to get
and validate both nodes?
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-17 11:42 ` Sumit Semwal
@ 2020-06-17 11:47 ` Mark Brown
2020-06-17 11:57 ` Sumit Semwal
0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-17 11:47 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
[-- Attachment #1: Type: text/plain, Size: 876 bytes --]
On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote:
> I understand from a pure regulators' correctness point of view,
> ENABLE_CTL should be the one checked there, so I can change the patch
> as you suggested, but there seems to be some performance penalty
> there.
I thought the goal was to have the performance penalty to ensure that
the regulator had actually started?
> > > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > > DT binding for a PMIC instantiates only one of the regulators.
> > No, this whole loop - why this whole match and get child stuff?
> This loop mechanism is what I saw in the other qcom regulators
> upstream, so thought it was an acceptable way.
> For the two children nodes, do you recommend another mechanism to get
> and validate both nodes?
I don't understand what you mean by "two children nodes" here?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-17 11:47 ` Mark Brown
@ 2020-06-17 11:57 ` Sumit Semwal
2020-06-17 12:06 ` Mark Brown
0 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-17 11:57 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote:
>
> > I understand from a pure regulators' correctness point of view,
> > ENABLE_CTL should be the one checked there, so I can change the patch
> > as you suggested, but there seems to be some performance penalty
> > there.
>
> I thought the goal was to have the performance penalty to ensure that
> the regulator had actually started?
IMHO, with the poll_enabled_time mechanism added, we would not need to
wait for the full enabled_time time for the regulator to get enabled,
but we could poll (and potentially know earlier) if the regulator is
enabled.
The performance penalty I was talking, is about how should we check if
the regulator is really enabled or not - via reading the STATUS1
register, which seems to tell the status a bit faster, or via reading
the ENABLE_CTL register which we also use to enable/disable the
regulator, but which seems to be slower in updating the status.
>
> > > > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > > > DT binding for a PMIC instantiates only one of the regulators.
>
> > > No, this whole loop - why this whole match and get child stuff?
>
> > This loop mechanism is what I saw in the other qcom regulators
> > upstream, so thought it was an acceptable way.
> > For the two children nodes, do you recommend another mechanism to get
> > and validate both nodes?
>
> I don't understand what you mean by "two children nodes" here?
The two 'lab' and 'ibb' regulator nodes that are part of the labibb node.
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-17 11:57 ` Sumit Semwal
@ 2020-06-17 12:06 ` Mark Brown
2020-06-17 12:09 ` Sumit Semwal
0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-17 12:06 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]
On Wed, Jun 17, 2020 at 05:27:12PM +0530, Sumit Semwal wrote:
> On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote:
> > > I understand from a pure regulators' correctness point of view,
> > > ENABLE_CTL should be the one checked there, so I can change the patch
> > > as you suggested, but there seems to be some performance penalty
> > > there.
> > I thought the goal was to have the performance penalty to ensure that
> > the regulator had actually started?
> IMHO, with the poll_enabled_time mechanism added, we would not need to
> wait for the full enabled_time time for the regulator to get enabled,
> but we could poll (and potentially know earlier) if the regulator is
> enabled.
> The performance penalty I was talking, is about how should we check if
> the regulator is really enabled or not - via reading the STATUS1
> register, which seems to tell the status a bit faster, or via reading
> the ENABLE_CTL register which we also use to enable/disable the
> regulator, but which seems to be slower in updating the status.
That seems... interesting. Are you sure the regulator has fully ramped
when STATUS1 starts flagging?
> > > > > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > > > > DT binding for a PMIC instantiates only one of the regulators.
> > > > No, this whole loop - why this whole match and get child stuff?
> > > This loop mechanism is what I saw in the other qcom regulators
> > > upstream, so thought it was an acceptable way.
> > > For the two children nodes, do you recommend another mechanism to get
> > > and validate both nodes?
> > I don't understand what you mean by "two children nodes" here?
> The two 'lab' and 'ibb' regulator nodes that are part of the labibb node.
Use of_match and regulators_node like other regulator drivers.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-17 12:06 ` Mark Brown
@ 2020-06-17 12:09 ` Sumit Semwal
2020-06-17 12:40 ` Mark Brown
0 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-17 12:09 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
On Wed, 17 Jun 2020 at 17:36, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 17, 2020 at 05:27:12PM +0530, Sumit Semwal wrote:
> > On Wed, 17 Jun 2020 at 17:17, Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, Jun 17, 2020 at 05:12:35PM +0530, Sumit Semwal wrote:
>
> > > > I understand from a pure regulators' correctness point of view,
> > > > ENABLE_CTL should be the one checked there, so I can change the patch
> > > > as you suggested, but there seems to be some performance penalty
> > > > there.
>
> > > I thought the goal was to have the performance penalty to ensure that
> > > the regulator had actually started?
>
> > IMHO, with the poll_enabled_time mechanism added, we would not need to
> > wait for the full enabled_time time for the regulator to get enabled,
> > but we could poll (and potentially know earlier) if the regulator is
> > enabled.
> > The performance penalty I was talking, is about how should we check if
> > the regulator is really enabled or not - via reading the STATUS1
> > register, which seems to tell the status a bit faster, or via reading
> > the ENABLE_CTL register which we also use to enable/disable the
> > regulator, but which seems to be slower in updating the status.
>
> That seems... interesting. Are you sure the regulator has fully ramped
> when STATUS1 starts flagging?
On a consumer device, I am not sure I have any way of checking that,
but if there's some way you'd like me to validate it, I'll be happy
to.
>
> > > > > > The WARN_ON? This was suggested by Bjorn to catch the case where the
> > > > > > DT binding for a PMIC instantiates only one of the regulators.
>
> > > > > No, this whole loop - why this whole match and get child stuff?
>
> > > > This loop mechanism is what I saw in the other qcom regulators
> > > > upstream, so thought it was an acceptable way.
> > > > For the two children nodes, do you recommend another mechanism to get
> > > > and validate both nodes?
>
> > > I don't understand what you mean by "two children nodes" here?
>
> > The two 'lab' and 'ibb' regulator nodes that are part of the labibb node.
>
> Use of_match and regulators_node like other regulator drivers.
Ok, let me see what I can do with those; we still need to flag if some
platform only instantiates one of the two lab/ibb regulators - I was
given the impression they're 'both or none' case.
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] regulator: qcom: Add labibb driver
2020-06-17 12:09 ` Sumit Semwal
@ 2020-06-17 12:40 ` Mark Brown
0 siblings, 0 replies; 22+ messages in thread
From: Mark Brown @ 2020-06-17 12:40 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
[-- Attachment #1: Type: text/plain, Size: 539 bytes --]
On Wed, Jun 17, 2020 at 05:39:26PM +0530, Sumit Semwal wrote:
> On Wed, 17 Jun 2020 at 17:36, Mark Brown <broonie@kernel.org> wrote:
> > That seems... interesting. Are you sure the regulator has fully ramped
> > when STATUS1 starts flagging?
> On a consumer device, I am not sure I have any way of checking that,
> but if there's some way you'd like me to validate it, I'll be happy
> to.
Without any way of validating what's going on or information on the
hardware I'd be inclined to go with whatever reports more slowly for
safety.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
2020-06-02 10:09 [PATCH v4 0/5] Qualcomm labibb regulator driver Sumit Semwal
` (3 preceding siblings ...)
2020-06-02 10:09 ` [PATCH v4 4/5] regulator: qcom: Add labibb driver Sumit Semwal
@ 2020-06-02 10:09 ` Sumit Semwal
2020-06-02 12:22 ` Mark Brown
4 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-02 10:09 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt
Cc: nishakumari, linux-arm-msm, linux-kernel, devicetree, kgunda,
rnayak, Sumit Semwal
From: Nisha Kumari <nishakumari@codeaurora.org>
Add Short circuit interrupt handling and recovery for the lab and ibb
regulators on qcom platforms.
The client panel drivers need to register for REGULATOR_EVENT_OVER_CURRENT
notification which will be triggered on short circuit. They should
try to enable the regulator once, and if it doesn't get enabled,
handle shutting down the panel accordingly.
Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
[sumits: updated to rework to use regmap_read_poll_timeout, handle it per
regulator, add REGULATOR_EVENT_OVER_CURRENT handling and
notification to clients and other cleanup]
--
v2: sumits: reworked handling to user regmap_read_poll_timeout, and handle it
per-regulator instead of clearing both lab and ibb errors on either irq
triggering. Also added REGULATOR_EVENT_OVER_CURRENT handling and
notification to clients.
v3: sumits: updated as per review comments of v2: removed spurious check for
irq in handler and some unused variables; inlined some of the code,
omitted IRQF_TRIGGER_RISING as it's coming from DT.
v4: sumits: updated 'int vreg_enabled' to 'boot enabled', made sc_irq a local var
and other review comments from v3.
---
drivers/regulator/qcom-labibb-regulator.c | 102 +++++++++++++++++++++-
1 file changed, 99 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 33b764ac69d1..bca0308b26dd 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
// Copyright (c) 2020, The Linux Foundation. All rights reserved.
+#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/of_irq.h>
#include <linux/of.h>
@@ -18,6 +19,7 @@
#define REG_LABIBB_ENABLE_CTL 0x46
#define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
#define LABIBB_CONTROL_ENABLE BIT(7)
+#define LABIBB_STATUS1_SC_DETECT_BIT BIT(6)
#define LAB_ENABLE_CTL_MASK BIT(7)
#define IBB_ENABLE_CTL_MASK (BIT(7) | BIT(6))
@@ -27,12 +29,16 @@
#define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10)
#define LABIBB_POLL_ENABLED_TIME 1000
+#define POLLING_SCP_DONE_INTERVAL_US 5000
+#define POLLING_SCP_TIMEOUT 16000
+
struct labibb_regulator {
struct regulator_desc desc;
struct device *dev;
struct regmap *regmap;
struct regulator_dev *rdev;
u16 base;
+ bool enabled;
u8 type;
};
@@ -59,12 +65,26 @@ static int qcom_labibb_regulator_is_enabled(struct regulator_dev *rdev)
static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
{
- return regulator_enable_regmap(rdev);
+ int ret;
+ struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+ ret = regulator_enable_regmap(rdev);
+ if (ret >= 0)
+ reg->enabled = true;
+
+ return ret;
}
static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
{
- return regulator_disable_regmap(rdev);
+ int ret = 0;
+ struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+ ret = regulator_disable_regmap(rdev);
+ if (ret >= 0)
+ reg->enabled = false;
+
+ return ret;
}
static struct regulator_ops qcom_labibb_ops = {
@@ -73,12 +93,70 @@ static struct regulator_ops qcom_labibb_ops = {
.is_enabled = qcom_labibb_regulator_is_enabled,
};
+static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
+{
+ int ret;
+ u16 reg;
+ unsigned int val;
+ struct labibb_regulator *labibb_reg = _reg;
+ bool in_sc_err, scp_done = false;
+
+ ret = regmap_read(labibb_reg->regmap,
+ labibb_reg->base + REG_LABIBB_STATUS1, &val);
+ if (ret < 0) {
+ dev_err(labibb_reg->dev, "sc_err_irq: Read failed, ret=%d\n",
+ ret);
+ return IRQ_HANDLED;
+ }
+
+ dev_dbg(labibb_reg->dev, "%s SC error triggered! STATUS1 = %d\n",
+ labibb_reg->desc.name, val);
+
+ in_sc_err = !!(val & LABIBB_STATUS1_SC_DETECT_BIT);
+
+ /*
+ * The SC(short circuit) fault would trigger PBS(Portable Batch
+ * System) to disable regulators for protection. This would
+ * cause the SC_DETECT status being cleared so that it's not
+ * able to get the SC fault status.
+ * Check if the regulator is enabled in the driver but
+ * disabled in hardware, this means a SC fault had happened
+ * and SCP handling is completed by PBS.
+ */
+ if (!in_sc_err) {
+
+ reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
+
+ ret = regmap_read_poll_timeout(labibb_reg->regmap,
+ reg, val,
+ !(val & LABIBB_CONTROL_ENABLE),
+ POLLING_SCP_DONE_INTERVAL_US,
+ POLLING_SCP_TIMEOUT);
+
+ if (!ret && labibb_reg->enabled) {
+ dev_dbg(labibb_reg->dev,
+ "%s has been disabled by SCP\n",
+ labibb_reg->desc.name);
+ scp_done = true;
+ }
+ }
+
+ if (in_sc_err || scp_done) {
+ regulator_lock(labibb_reg->rdev);
+ regulator_notifier_call_chain(labibb_reg->rdev,
+ REGULATOR_EVENT_OVER_CURRENT,
+ NULL);
+ regulator_unlock(labibb_reg->rdev);
+ }
+ return IRQ_HANDLED;
+}
+
static struct regulator_dev *register_labibb_regulator(struct labibb_regulator *reg,
const struct labibb_regulator_data *reg_data,
struct device_node *of_node)
{
struct regulator_config cfg = {};
- int ret;
+ int ret, sc_irq;
reg->base = reg_data->base;
reg->type = reg_data->type;
@@ -95,6 +173,24 @@ static struct regulator_dev *register_labibb_regulator(struct labibb_regulator *
reg->desc.poll_enabled_time = LABIBB_POLL_ENABLED_TIME;
reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
+ sc_irq = of_irq_get_byname(of_node, "sc-err");
+ if (sc_irq < 0) {
+ dev_err(reg->dev, "Unable to get sc-err, ret = %d\n",
+ sc_irq);
+ return ERR_PTR(sc_irq);
+ } else {
+ ret = devm_request_threaded_irq(reg->dev,
+ sc_irq,
+ NULL, labibb_sc_err_handler,
+ IRQF_ONESHOT,
+ "sc-err", reg);
+ if (ret) {
+ dev_err(reg->dev, "Failed to register sc-err irq ret=%d\n",
+ ret);
+ return ERR_PTR(ret);
+ }
+ }
+
cfg.dev = reg->dev;
cfg.driver_data = reg;
cfg.regmap = reg->regmap;
--
2.26.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
2020-06-02 10:09 ` [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling Sumit Semwal
@ 2020-06-02 12:22 ` Mark Brown
2020-06-17 12:06 ` Sumit Semwal
0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2020-06-02 12:22 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, bjorn.andersson, lgirdwood, robh+dt, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]
On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
> static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> {
> - return regulator_enable_regmap(rdev);
> + int ret;
> + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> + ret = regulator_enable_regmap(rdev);
> + if (ret >= 0)
> + reg->enabled = true;
Can we not read the register we just wrote to here?
> + /*
> + * The SC(short circuit) fault would trigger PBS(Portable Batch
> + * System) to disable regulators for protection. This would
> + * cause the SC_DETECT status being cleared so that it's not
> + * able to get the SC fault status.
> + * Check if the regulator is enabled in the driver but
> + * disabled in hardware, this means a SC fault had happened
> + * and SCP handling is completed by PBS.
> + */
> + if (!in_sc_err) {
> +
> + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> +
> + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> + reg, val,
> + !(val & LABIBB_CONTROL_ENABLE),
> + POLLING_SCP_DONE_INTERVAL_US,
> + POLLING_SCP_TIMEOUT);
Why do we need a timeout here?
> + NULL);
> + regulator_unlock(labibb_reg->rdev);
> + }
> + return IRQ_HANDLED;
This returns IRQ_HANDLED even if we didn't detect an interrupt source...
Especially given the need to check to see if the regulator was turned
off by the hardware it seems like there must be some false positives.
> + } else {
> + ret = devm_request_threaded_irq(reg->dev,
> + sc_irq,
> + NULL, labibb_sc_err_handler,
> + IRQF_ONESHOT,
> + "sc-err", reg);
This looks like we're requesting the interrupt before we register the
regulator which means the interrupt might fire without the regulator
being there. The order of registration should be reversed.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
2020-06-02 12:22 ` Mark Brown
@ 2020-06-17 12:06 ` Sumit Semwal
2020-06-17 12:38 ` Mark Brown
0 siblings, 1 reply; 22+ messages in thread
From: Sumit Semwal @ 2020-06-17 12:06 UTC (permalink / raw)
To: Mark Brown
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
Hi Mark,
On Tue, 2 Jun 2020 at 17:52, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
>
> > static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > {
> > - return regulator_enable_regmap(rdev);
> > + int ret;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regulator_enable_regmap(rdev);
> > + if (ret >= 0)
> > + reg->enabled = true;
>
> Can we not read the register we just wrote to here?
As I mentioned in the other patch, it seems there is a (noticeable)
delay in getting the value to reflect in this register for IBB.
Also, from the notes from the downstream driver (also copied below),
it seems like during short circuit there is another protection system
that can cause the registers to be cleared, hence the need to track
the current state in software.
>
> > + /*
> > + * The SC(short circuit) fault would trigger PBS(Portable Batch
> > + * System) to disable regulators for protection. This would
> > + * cause the SC_DETECT status being cleared so that it's not
> > + * able to get the SC fault status.
> > + * Check if the regulator is enabled in the driver but
> > + * disabled in hardware, this means a SC fault had happened
> > + * and SCP handling is completed by PBS.
> > + */
> > + if (!in_sc_err) {
> > +
> > + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> > +
> > + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> > + reg, val,
> > + !(val & LABIBB_CONTROL_ENABLE),
> > + POLLING_SCP_DONE_INTERVAL_US,
> > + POLLING_SCP_TIMEOUT);
>
> Why do we need a timeout here?
IMHO, This seems to be the time required by the PBS to actually
disable the regulator? If the PBS is not able to disable the
regulator, then it points to a more serious problem?
I'm sorry, that's just my understanding based on the downstream driver
:/ - not much input is available from the QC teams about it.
>
> > + NULL);
> > + regulator_unlock(labibb_reg->rdev);
> > + }
> > + return IRQ_HANDLED;
>
> This returns IRQ_HANDLED even if we didn't detect an interrupt source...
> Especially given the need to check to see if the regulator was turned
> off by the hardware it seems like there must be some false positives.
Right - I'm not sure what else can I do here.
>
> > + } else {
> > + ret = devm_request_threaded_irq(reg->dev,
> > + sc_irq,
> > + NULL, labibb_sc_err_handler,
> > + IRQF_ONESHOT,
> > + "sc-err", reg);
>
> This looks like we're requesting the interrupt before we register the
> regulator which means the interrupt might fire without the regulator
> being there. The order of registration should be reversed.
Agreed, and will update in the next version.
Best,
Sumit.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
2020-06-17 12:06 ` Sumit Semwal
@ 2020-06-17 12:38 ` Mark Brown
0 siblings, 0 replies; 22+ messages in thread
From: Mark Brown @ 2020-06-17 12:38 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, Bjorn Andersson, lgirdwood, robh+dt, Nisha Kumari,
linux-arm-msm, LKML, devicetree, kgunda, Rajendra Nayak
[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]
On Wed, Jun 17, 2020 at 05:36:43PM +0530, Sumit Semwal wrote:
> On Tue, 2 Jun 2020 at 17:52, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
> > > +
> > > + ret = regulator_enable_regmap(rdev);
> > > + if (ret >= 0)
> > > + reg->enabled = true;
> > Can we not read the register we just wrote to here?
> As I mentioned in the other patch, it seems there is a (noticeable)
> delay in getting the value to reflect in this register for IBB.
This sounds like it may not actually have finished enabling fully?
> Also, from the notes from the downstream driver (also copied below),
> it seems like during short circuit there is another protection system
> that can cause the registers to be cleared, hence the need to track
> the current state in software.
If the regulator has been disabled underneath us in a way that means it
won't come back the driver should be reflecting that in the status it
reports.
> > > + * Check if the regulator is enabled in the driver but
> > > + * disabled in hardware, this means a SC fault had happened
> > > + * and SCP handling is completed by PBS.
> > > + */
> > > + if (!in_sc_err) {
> > > +
> > > + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> > > +
> > > + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> > > + reg, val,
> > > + !(val & LABIBB_CONTROL_ENABLE),
> > > + POLLING_SCP_DONE_INTERVAL_US,
> > > + POLLING_SCP_TIMEOUT);
> > Why do we need a timeout here?
> IMHO, This seems to be the time required by the PBS to actually
> disable the regulator? If the PBS is not able to disable the
> regulator, then it points to a more serious problem?
> I'm sorry, that's just my understanding based on the downstream driver
> :/ - not much input is available from the QC teams about it.
So it might generate an interrupt but then take a long time to take the
actions associated with the interrupt that allow us to tell what the
interrupt was about? That doesn't seem great. Do you know if this code
has ever been exercised, the error handling code appears unusually
involved here? Normally errors don't routinely occur in production.
> > > + NULL);
> > > + regulator_unlock(labibb_reg->rdev);
> > > + }
> > > + return IRQ_HANDLED;
> > This returns IRQ_HANDLED even if we didn't detect an interrupt source...
> > Especially given the need to check to see if the regulator was turned
> > off by the hardware it seems like there must be some false positives.
> Right - I'm not sure what else can I do here.
Only return IRQ_HANDLED if we actually managed to figure out an error to
report?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread