From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Sumit Semwal <sumit.semwal@linaro.org>
Cc: agross@kernel.org, lgirdwood@gmail.com, broonie@kernel.org,
robh+dt@kernel.org, nishakumari@codeaurora.org,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, kgunda@codeaurora.org,
rnayak@codeaurora.org
Subject: Re: [PATCH v3 5/5] regulator: qcom: labibb: Add SC interrupt handling
Date: Thu, 28 May 2020 19:28:47 -0700 [thread overview]
Message-ID: <20200529022847.GO279327@builder.lan> (raw)
In-Reply-To: <20200528154625.17742-6-sumit.semwal@linaro.org>
On Thu 28 May 08:46 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>
It would be nice to see a short summary of your changes from the
original patch here, like:
[sumit: Changed this and that]
> 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.
> 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.
>
> ---
> drivers/regulator/qcom-labibb-regulator.c | 92 +++++++++++++++++++++++
> 1 file changed, 92 insertions(+)
>
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index 634d08461c6e..695ffac71e81 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,17 @@
> #define IBB_POLL_ENABLED_TIME (LABIBB_ENABLE_TIME * 10)
> #define LABIBB_OFF_ON_DELAY (8200)
>
> +#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;
> + int sc_irq;
This is now a local variable in register_labibb_regulator().
> + int vreg_enabled;
bool is a better representation of this (and the vreg_ prefix doesn't
really add value).
> u8 type;
> };
>
> @@ -65,6 +72,8 @@ static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> if (ret < 0)
> dev_err(reg->dev, "Write failed: enable %s regulator\n",
> reg->desc.name);
> + else
> + reg->vreg_enabled = 1;
No I see why you add these wrappers around the regmap in the previous
patch.
My request to not print an error message on enable/disable errors
remains.
>
> return ret;
> }
> @@ -78,6 +87,8 @@ static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
> if (ret < 0)
> dev_err(reg->dev, "Disable failed: disable %s\n",
> reg->desc.name);
> + else
> + reg->vreg_enabled = 0;
>
> return ret;
> }
> @@ -88,11 +99,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) {
> +
Empty line
Regards,
Bjorn
> + 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 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;
> @@ -108,6 +178,28 @@ static int register_labibb_regulator(struct labibb_regulator *reg,
> reg->desc.poll_enabled_time = reg_data->poll_enabled_time;
> reg->desc.off_on_delay = LABIBB_OFF_ON_DELAY;
>
> + reg->sc_irq = -EINVAL;
> + ret = of_irq_get_byname(of_node, "sc-err");
> + if (ret < 0) {
> + dev_err(reg->dev, "Unable to get sc-err, ret = %d\n",
> + ret);
> + return ret;
> + } else
> + reg->sc_irq = ret;
> +
> + if (reg->sc_irq > 0) {
> + ret = devm_request_threaded_irq(reg->dev,
> + reg->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 ret;
> + }
> + }
> +
> cfg.dev = reg->dev;
> cfg.driver_data = reg;
> cfg.regmap = reg->regmap;
> --
> 2.26.2
>
prev parent reply other threads:[~2020-05-29 2:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-28 15:46 [PATCH v3 0/5] Qualcomm labibb regulator driver Sumit Semwal
2020-05-28 15:46 ` [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable() Sumit Semwal
2020-05-29 1:37 ` Bjorn Andersson
2020-05-29 10:50 ` Mark Brown
2020-05-29 10:54 ` Mark Brown
2020-05-28 15:46 ` [PATCH v3 2/5] dt-bindings: regulator: Add labibb regulator Sumit Semwal
2020-05-29 3:01 ` Rob Herring
2020-05-28 15:46 ` [PATCH v3 3/5] arm64: dts: qcom: pmi8998: Add nodes for LAB and IBB regulators Sumit Semwal
2020-05-29 1:43 ` Bjorn Andersson
2020-05-28 15:46 ` [PATCH v3 4/5] regulator: qcom: Add labibb driver Sumit Semwal
2020-05-29 2:04 ` Bjorn Andersson
2020-05-28 15:46 ` [PATCH v3 5/5] regulator: qcom: labibb: Add SC interrupt handling Sumit Semwal
2020-05-29 2:28 ` Bjorn Andersson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200529022847.GO279327@builder.lan \
--to=bjorn.andersson@linaro.org \
--cc=agross@kernel.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=kgunda@codeaurora.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nishakumari@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=robh+dt@kernel.org \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).