| Message ID | 20200508204200.13481-5-sumit.semwal@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Qualcomm labibb regulator driver | expand |
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?
On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote: > 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> > > -- > 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 *. > + 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, &val, 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, > + 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? 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. > + 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. > + } > + > + 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? > + "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. > + 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 >
Hello Mark, Thanks for the review comments. On Mon, 11 May 2020 at 16:19, Mark Brown <broonie@kernel.org> 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.
Hello Bjorn, Thanks for the review comments. On Tue, 12 May 2020 at 10:55, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 08 May 13:42 PDT 2020, Sumit Semwal wrote: > > > 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> > > > > -- > > 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.
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; + 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, &val, 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) { + 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, + "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, + 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; + } + } + cfg.dev = labibb->dev; cfg.driver_data = reg; cfg.regmap = labibb->regmap;