Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling
flat view
From: Bjorn Andersson <hidden>
Date: 2017-12-05 04:35:25
Also in:
linux-arm-msm, linux-devicetree, linux-leds, lkml
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:
quoted hunk ↗ jump to hunk
Handle the short circuit(SC) interrupt and check if the SC interrupt is valid. Re-enable the module to check if it goes away. Disable the module altogether if the SC event persists. Signed-off-by: Kiran Gunda <redacted> --- .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++ drivers/video/backlight/qcom-spmi-wled.c | 126 ++++++++++++++++++++- 2 files changed, 142 insertions(+), 6 deletions(-)diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt index f1ea25b..768608c 100644 --- a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt@@ -74,6 +74,26 @@ The PMIC is connected to the host processor via SPMI bus. Definition: Specify if cabc (content adaptive backlight control) is needed. +- qcom,ext-pfet-sc-pro-en
Please use readable names, rather than a bunch of abbreviations.
quoted hunk ↗ jump to hunk
+ Usage: optional + Value type: <bool> + Definition: Specify if external PFET control for short circuit + protection is needed.
What does this mean? At least change the wording to "...protection is used".
quoted hunk ↗ jump to hunk
+ +- interrupts + Usage: optional + Value type: <prop encoded array> + Definition: Interrupts associated with WLED. Interrupts can be + specified as per the encoding listed under + Documentation/devicetree/bindings/spmi/ + qcom,spmi-pmic-arb.txt. + +- interrupt-names + Usage: optional + Value type: <string> + Definition: Interrupt names associated with the interrupts. + Must be "sc-irq".
This is obviously an irq, so no need to include that in the name. I would also prefer if you use the name "short" to make this easier to read.
quoted hunk ↗ jump to hunk
+ Example: qcom-wled@d800 {@@ -82,6 +102,8 @@ qcom-wled@d800 { reg-names = "qcom-wled-ctrl-base", "qcom-wled-sink-base"; label = "backlight"; + interrupts = <0x3 0xd8 0x2 IRQ_TYPE_EDGE_RISING>;
We tend to write these on the form <decimal, hex, decimal, enum>, please follow this.
quoted hunk ↗ jump to hunk
+ interrupt-names = "sc-irq"; qcom,fs-current-limit = <25000>; qcom,current-boost-limit = <970>; qcom,switching-freq = <800>;diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c index 14c3adc..7dbaaa7 100644 --- a/drivers/video/backlight/qcom-spmi-wled.c +++ b/drivers/video/backlight/qcom-spmi-wled.c@@ -11,6 +11,9 @@ * GNU General Public License for more details. */ +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/ktime.h> #include <linux/kernel.h> #include <linux/backlight.h> #include <linux/module.h>@@ -23,7 +26,13 @@ #define QCOM_WLED_DEFAULT_BRIGHTNESS 2048 #define QCOM_WLED_MAX_BRIGHTNESS 4095 +#define QCOM_WLED_SC_DLY_MS 20 +#define QCOM_WLED_SC_CNT_MAX 5 +#define QCOM_WLED_SC_RESET_CNT_DLY_US 1000000
With times of this ballpark you can just use jiffies, with this just being HZ.
quoted hunk ↗ jump to hunk
+ /* WLED control registers */ +#define QCOM_WLED_CTRL_FAULT_STATUS 0x08 + #define QCOM_WLED_CTRL_MOD_ENABLE 0x46 #define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7) #define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7@@ -37,6 +46,15 @@ #define QCOM_WLED_CTRL_ILIM 0x4e #define QCOM_WLED_CTRL_ILIM_MASK GENMASK(2, 0) +#define QCOM_WLED_CTRL_SHORT_PROTECT 0x5e +#define QCOM_WLED_CTRL_SHORT_EN_MASK BIT(7) + +#define QCOM_WLED_CTRL_SEC_ACCESS 0xd0 +#define QCOM_WLED_CTRL_SEC_UNLOCK 0xa5 + +#define QCOM_WLED_CTRL_TEST1 0xe2 +#define QCOM_WLED_EXT_FET_DTEST2 0x09 + /* WLED sink registers */ #define QCOM_WLED_SINK_CURR_SINK_EN 0x46 #define QCOM_WLED_SINK_CURR_SINK_MASK GENMASK(7, 4)@@ -71,19 +89,23 @@ struct qcom_wled_config { u32 switch_freq; u32 fs_current; u32 string_cfg; + int sc_irq;
Keep data parsed directly from DT in the config and move this to qcom_wled.
bool en_cabc; + bool ext_pfet_sc_pro_en;
This name is long and hard to parse. "external_pfet" would be much easier to read.
quoted hunk ↗ jump to hunk
}; struct qcom_wled { const char *name; struct platform_device *pdev; struct regmap *regmap; + struct mutex lock; + struct qcom_wled_config cfg; + ktime_t last_sc_event_time; u16 sink_addr; u16 ctrl_addr; u32 brightness; + u32 sc_count; bool prev_state; - - struct qcom_wled_config cfg;
Moving this seems unnecessary.
quoted hunk ↗ jump to hunk
}; static int qcom_wled_module_enable(struct qcom_wled *wled, int val)@@ -157,25 +179,26 @@ static int qcom_wled_update_status(struct backlight_device *bl) bl->props.state & BL_CORE_FBBLANK) brightness = 0; + mutex_lock(&wled->lock);
Is this lock necessary?
quoted hunk ↗ jump to hunk
+static irqreturn_t qcom_wled_sc_irq_handler(int irq, void *_wled) +{ + struct qcom_wled *wled = _wled; + int rc; + u32 val; + s64 elapsed_time; + + rc = regmap_read(wled->regmap, + wled->ctrl_addr + QCOM_WLED_CTRL_FAULT_STATUS, &val); + if (rc < 0) { + pr_err("Error in reading WLED_FAULT_STATUS rc=%d\n", rc); + return IRQ_HANDLED; + } + + wled->sc_count++; + pr_err("WLED short circuit detected %d times fault_status=%x\n", + wled->sc_count, val);
Who will read this and is it worth the extra read of FAULT_STATUS just to produce this print?
quoted hunk ↗ jump to hunk
+ mutex_lock(&wled->lock); + rc = qcom_wled_module_enable(wled, false); + if (rc < 0) { + pr_err("wled disable failed rc:%d\n", rc); + goto unlock_mutex; + } + + elapsed_time = ktime_us_delta(ktime_get(), + wled->last_sc_event_time); + if (elapsed_time > QCOM_WLED_SC_RESET_CNT_DLY_US) { + wled->sc_count = 0; + } else if (wled->sc_count > QCOM_WLED_SC_CNT_MAX) {
This isn't really "else elapsed_time was more than DLY_US". Split this into: if (elapsed_time > xyz) wled->sc_count = 0; if (wled->sc_count > QCOM_WLED_SC_CNT_MAX) ...
+ pr_err("SC trigged %d times, disabling WLED forever!\n","forever" as in "until someone turns it on again"?
quoted hunk ↗ jump to hunk
+ wled->sc_count); + goto unlock_mutex; + } + + wled->last_sc_event_time = ktime_get(); + + msleep(QCOM_WLED_SC_DLY_MS); + rc = qcom_wled_module_enable(wled, true); + if (rc < 0) + pr_err("wled enable failed rc:%d\n", rc); + +unlock_mutex: + mutex_unlock(&wled->lock); + + return IRQ_HANDLED; +} + static int qcom_wled_setup(struct qcom_wled *wled) { int rc, temp, i; u8 sink_en = 0; u8 string_cfg = wled->cfg.string_cfg; + int sc_irq = wled->cfg.sc_irq; rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + QCOM_WLED_CTRL_OVP,@@ -261,6 +334,39 @@ static int qcom_wled_setup(struct qcom_wled *wled) return rc; } + if (sc_irq >= 0) {
I think things will be cleaner if you let qcom_wled_setup() configure the hardware based on the wled->cfg (as is done to this point) and then deal with the interrupts in a separate code path from the probe function.
quoted hunk ↗ jump to hunk
+ rc = devm_request_threaded_irq(&wled->pdev->dev, sc_irq, + NULL, qcom_wled_sc_irq_handler, IRQF_ONESHOT, + "qcom_wled_sc_irq", wled); + if (rc < 0) { + pr_err("Unable to request sc(%d) IRQ(err:%d)\n", + sc_irq, rc);
sc_irq is just a number without meaning, no need to print it.
quoted hunk ↗ jump to hunk
+ return rc; + } + + rc = regmap_update_bits(wled->regmap, + wled->ctrl_addr + QCOM_WLED_CTRL_SHORT_PROTECT, + QCOM_WLED_CTRL_SHORT_EN_MASK, + QCOM_WLED_CTRL_SHORT_EN_MASK); + if (rc < 0) + return rc; + + if (wled->cfg.ext_pfet_sc_pro_en) { + /* unlock the secure access regisetr */
Spelling of register, and this operation does "Unlock the secure register access" it doesn't unlock the secure access register.
quoted hunk ↗ jump to hunk
+ rc = regmap_write(wled->regmap, wled->ctrl_addr + + QCOM_WLED_CTRL_SEC_ACCESS, + QCOM_WLED_CTRL_SEC_UNLOCK); + if (rc < 0) + return rc; + + rc = regmap_write(wled->regmap, + wled->ctrl_addr + QCOM_WLED_CTRL_TEST1, + QCOM_WLED_EXT_FET_DTEST2);
What is the relationship between DTEST2 and the external FET?
quoted hunk ↗ jump to hunk
+ if (rc < 0) + return rc; + } + } + return 0; }@@ -271,6 +377,7 @@ static int qcom_wled_setup(struct qcom_wled *wled) .switch_freq = 11, .string_cfg = 0xf, .en_cabc = 0, + .ext_pfet_sc_pro_en = 1, }; struct qcom_wled_var_cfg {@@ -376,6 +483,7 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev) bool *val_ptr; } bool_opts[] = { { "qcom,en-cabc", &cfg->en_cabc, }, + { "qcom,ext-pfet-sc-pro", &cfg->ext_pfet_sc_pro_en, }, }; prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);@@ -427,6 +535,10 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev) *bool_opts[i].val_ptr = true; } + wled->cfg.sc_irq = platform_get_irq_byname(wled->pdev, "sc-irq"); + if (wled->cfg.sc_irq < 0) + dev_dbg(&wled->pdev->dev, "sc irq is not used\n"); +
Move this to qcom_wled_probe() or into its own code path, together with the rest of the sc_irq initialization. And as you're not enabling or disabling it you can store it in a local variable.
return 0; }
Regards, Bjorn