Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
Hello Bjorn,
Thanks for the review comments.
On Tue, 12 May 2020 at 10:55, Bjorn Andersson
wrote:
>
> On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote:
>
> > From: Nisha Kumari
> >
> > 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
> > Signed-off-by: Sumit Semwal
> >
> > --
> > 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.
> > ---
> > drivers/regulator/qcom-labibb-regulator.c | 103 +-
> > 1 file changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/regulator/qcom-labibb-regulator.c
> > b/drivers/regulator/qcom-labibb-regulator.c
> > index a9dc7c060375..3539631c9f96 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
> > // Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -18,11 +19,15 @@
> > #define REG_LABIBB_ENABLE_CTL0x46
> > #define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
> > #define LABIBB_CONTROL_ENABLEBIT(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))
> >
> > #define POWER_DELAY 8000
> > +#define POLLING_SCP_DONE_INTERVAL_US 5000
> > +#define POLLING_SCP_TIMEOUT 16000
> > +
> >
> > struct labibb_regulator {
> > struct regulator_desc desc;
> > @@ -30,6 +35,8 @@ struct labibb_regulator {
> > struct regmap *regmap;
> > struct regulator_dev*rdev;
> > u16 base;
> > + int sc_irq;
> > + int vreg_enabled;
> > u8 type;
> > };
> >
> > @@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct
> > regulator_dev *rdev)
> > return ret;
> > }
> >
> > - if (ret)
> > + if (ret) {
> > + reg->vreg_enabled = 1;
> > return 0;
> > -
> > + }
> >
> > dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> > return -EINVAL;
> > @@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct
> > regulator_dev *rdev)
> > return ret;
> > }
> >
> > - if (!ret)
> > + if (!ret) {
> > + reg->vreg_enabled = 0;
> > return 0;
> > + }
> >
> > dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> > return -EINVAL;
> > @@ -153,6 +163,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, count;
> > + u16 reg;
> > + u8 sc_err_mask;
> > + unsigned int val;
> > + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
>
> No need to explicitly typecast a void *.
ok.
>
> > + bool in_sc_err, reg_en, scp_done = false;
>
> reg_en is unused.
>
Yes, will remove.
> > +
> > + if (irq == labibb_reg->sc_irq)
>
> When is this false?
Shouldn't be; will remove the check.
>
> > + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > + else
> > + return IRQ_HANDLED;
> > +
> > + sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
> > +
> > + ret = regmap_bulk_read(labibb_reg->regmap, reg, , 1);
>
> Just inline reg->base + REG_LABIBB_STATUS1 in this call.
Ok.
>
> > + if (ret < 0) {
> > + dev_err(labibb_reg->dev, "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 & sc_err_mask);
> > +
> > + /*
> > + * 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
Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
Hello Mark,
Thanks for the review comments.
On Mon, 11 May 2020 at 16:19, Mark Brown wrote:
>
> On Sat, May 09, 2020 at 02:12:00AM +0530, Sumit Semwal wrote:
>
> > +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> > +{
> > + int ret, count;
> > + u16 reg;
> > + u8 sc_err_mask;
> > + unsigned int val;
> > + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
> > + bool in_sc_err, reg_en, scp_done = false;
> > +
> > + if (irq == labibb_reg->sc_irq)
> > + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> > + else
> > + return IRQ_HANDLED;
>
> Why would we be registering the interrupt handler when it's not valid?
Agreed, will correct.
Best,
Sumit.
Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote:
> From: Nisha Kumari
>
> 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
> Signed-off-by: Sumit Semwal
>
> --
> 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.
> ---
> drivers/regulator/qcom-labibb-regulator.c | 103 +-
> 1 file changed, 100 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/qcom-labibb-regulator.c
> b/drivers/regulator/qcom-labibb-regulator.c
> index a9dc7c060375..3539631c9f96 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
> // Copyright (c) 2019, The Linux Foundation. All rights reserved.
>
> +#include
> #include
> #include
> #include
> @@ -18,11 +19,15 @@
> #define REG_LABIBB_ENABLE_CTL0x46
> #define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
> #define LABIBB_CONTROL_ENABLEBIT(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))
>
> #define POWER_DELAY 8000
> +#define POLLING_SCP_DONE_INTERVAL_US 5000
> +#define POLLING_SCP_TIMEOUT 16000
> +
>
> struct labibb_regulator {
> struct regulator_desc desc;
> @@ -30,6 +35,8 @@ struct labibb_regulator {
> struct regmap *regmap;
> struct regulator_dev*rdev;
> u16 base;
> + int sc_irq;
> + int vreg_enabled;
> u8 type;
> };
>
> @@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct
> regulator_dev *rdev)
> return ret;
> }
>
> - if (ret)
> + if (ret) {
> + reg->vreg_enabled = 1;
> return 0;
> -
> + }
>
> dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
> return -EINVAL;
> @@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct
> regulator_dev *rdev)
> return ret;
> }
>
> - if (!ret)
> + if (!ret) {
> + reg->vreg_enabled = 0;
> return 0;
> + }
>
> dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
> return -EINVAL;
> @@ -153,6 +163,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, count;
> + u16 reg;
> + u8 sc_err_mask;
> + unsigned int val;
> + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
No need to explicitly typecast a void *.
> + bool in_sc_err, reg_en, scp_done = false;
reg_en is unused.
> +
> + if (irq == labibb_reg->sc_irq)
When is this false?
> + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> + else
> + return IRQ_HANDLED;
> +
> + sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
> +
> + ret = regmap_bulk_read(labibb_reg->regmap, reg, , 1);
Just inline reg->base + REG_LABIBB_STATUS1 in this call.
> + if (ret < 0) {
> + dev_err(labibb_reg->dev, "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 & sc_err_mask);
> +
> + /*
> + * 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) {
if (!(val & LABIBB_STATUS1_SC_DETECT_BIT)) {
> +
> + 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,
> +
Re: [v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
On Sat, May 09, 2020 at 02:12:00AM +0530, Sumit Semwal wrote:
> +static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
> +{
> + int ret, count;
> + u16 reg;
> + u8 sc_err_mask;
> + unsigned int val;
> + struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
> + bool in_sc_err, reg_en, scp_done = false;
> +
> + if (irq == labibb_reg->sc_irq)
> + reg = labibb_reg->base + REG_LABIBB_STATUS1;
> + else
> + return IRQ_HANDLED;
Why would we be registering the interrupt handler when it's not valid?
signature.asc
Description: PGP signature
[v2 4/4] regulator: qcom: labibb: Add SC interrupt handling
From: Nisha Kumari
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
Signed-off-by: Sumit Semwal
--
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.
---
drivers/regulator/qcom-labibb-regulator.c | 103 +-
1 file changed, 100 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/qcom-labibb-regulator.c
b/drivers/regulator/qcom-labibb-regulator.c
index a9dc7c060375..3539631c9f96 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
// Copyright (c) 2019, The Linux Foundation. All rights reserved.
+#include
#include
#include
#include
@@ -18,11 +19,15 @@
#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_MASKBIT(7)
#define IBB_ENABLE_CTL_MASK(BIT(7) | BIT(6))
#define POWER_DELAY8000
+#define POLLING_SCP_DONE_INTERVAL_US 5000
+#define POLLING_SCP_TIMEOUT16000
+
struct labibb_regulator {
struct regulator_desc desc;
@@ -30,6 +35,8 @@ struct labibb_regulator {
struct regmap *regmap;
struct regulator_dev*rdev;
u16 base;
+ int sc_irq;
+ int vreg_enabled;
u8 type;
};
@@ -112,9 +119,10 @@ static int qcom_labibb_regulator_enable(struct
regulator_dev *rdev)
return ret;
}
- if (ret)
+ if (ret) {
+ reg->vreg_enabled = 1;
return 0;
-
+ }
dev_err(reg->dev, "Can't enable %s\n", reg->desc.name);
return -EINVAL;
@@ -140,8 +148,10 @@ static int qcom_labibb_regulator_disable(struct
regulator_dev *rdev)
return ret;
}
- if (!ret)
+ if (!ret) {
+ reg->vreg_enabled = 0;
return 0;
+ }
dev_err(reg->dev, "Can't disable %s\n", reg->desc.name);
return -EINVAL;
@@ -153,6 +163,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, count;
+ u16 reg;
+ u8 sc_err_mask;
+ unsigned int val;
+ struct labibb_regulator *labibb_reg = (struct labibb_regulator *)_reg;
+ bool in_sc_err, reg_en, scp_done = false;
+
+ if (irq == labibb_reg->sc_irq)
+ reg = labibb_reg->base + REG_LABIBB_STATUS1;
+ else
+ return IRQ_HANDLED;
+
+ sc_err_mask = LABIBB_STATUS1_SC_DETECT_BIT;
+
+ ret = regmap_bulk_read(labibb_reg->regmap, reg, , 1);
+ if (ret < 0) {
+ dev_err(labibb_reg->dev, "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 & sc_err_mask);
+
+ /*
+* 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->vreg_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) {
+
