Hello Bjorn,
Thanks for the review comments.
On Tue, 12 May 2020 at 10:55, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote:
>
> > From: Nisha Kumari <nishakumari@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> >
> > --
> > 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 <linux/interrupt.h>
> > #include <linux/module.h>
> > #include <linux/of_irq.h>
> > #include <linux/of.h>
> > @@ -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_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, &val, 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 (!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,
> > + POLLING_SCP_TIMEOUT);
> > +
> > + if (!ret && labibb_reg->vreg_enabled) {
>
> Wouldn't be fine to assume that if you get a short circuit IRQ the
> regulator is enabled?
So this is from my understanding of the previous patchset by the
author, as documented in the comment: I understood it to mean that if
we aren't able to read back the 'ENABLE' bit from the hardware (so
it's disabled in hw?) but the driver has it's vreg->enabled set, that
means the SC fault had happened but was handled by the PBS?
>
> If you are worried about racing with a disable this won't work anyways,
> and you better enable_irq()/disable_irq() in regulator enable/disable,
> respectively.
>
It didn't seem to be about the disable race, but being handled by the PBS.
> > + dev_dbg(labibb_reg->dev,
> > + "%s has been disabled by SCP\n",
> > + labibb_reg->desc.name);
> > + scp_done = true;
> > + }
>
> If you flip the poll check around you will get here by not being in an
> short-circuit condition and you conclude that the regulator is still on;
> in which case you can just return here.
>
> That way you can drop in_sc_err and scp_done and flatten below
> conditional section.
Yeah I think that's related to the PBS v/s regulator's handling of the
short-circuit, afaiu.
>
> > + }
> > +
> > + 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 int register_labibb_regulator(struct qcom_labibb *labibb,
> > const struct labibb_regulator_data *reg_data,
> > struct device_node *of_node)
> > @@ -181,6 +255,29 @@ static int register_labibb_regulator(struct qcom_labibb *labibb,
> > reg->desc.type = REGULATOR_VOLTAGE;
> > reg->desc.ops = &qcom_labibb_ops;
> >
> > + reg->sc_irq = -EINVAL;
> > + ret = of_irq_get_byname(of_node, reg_data->irq_name);
> > + if (ret < 0)
> > + dev_dbg(labibb->dev,
>
> Isn't this an error?
yes, will change.
>
> > + "Unable to get %s, ret = %d\n",
> > + reg_data->irq_name, ret);
> > + else
> > + reg->sc_irq = ret;
> > +
> > + if (reg->sc_irq > 0) {
> > + ret = devm_request_threaded_irq(labibb->dev,
> > + reg->sc_irq,
> > + NULL, labibb_sc_err_handler,
> > + IRQF_ONESHOT |
> > + IRQF_TRIGGER_RISING,
>
> Omit IRQF_TRIGGER_RISING and let that come from DT.
Ok. That's anyways coming from the DT.
>
> > + reg_data->irq_name, labibb);
> > + if (ret) {
> > + dev_err(labibb->dev, "Failed to register '%s' irq ret=%d\n",
> > + reg_data->irq_name, ret);
> > + return ret;
> > + }
> > + }
> > +
>
> Regards,
> Bjorn
>
> > cfg.dev = labibb->dev;
> > cfg.driver_data = reg;
> > cfg.regmap = labibb->regmap;
> > --
> > 2.26.2
> >
Best,
Sumit.
