| Message ID | 1510834717-21765-3-git-send-email-kgunda@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 16, 2017 at 05:48:35PM +0530, Kiran Gunda wrote: > 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 <kgunda@codeaurora.org> > --- > .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++ ... and also put all the binding in one patch. I want to review the full view of the h/w, not piecemeal. > drivers/video/backlight/qcom-spmi-wled.c | 126 ++++++++++++++++++++- > 2 files changed, 142 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-11-18 02:00, Rob Herring wrote: > On Thu, Nov 16, 2017 at 05:48:35PM +0530, Kiran Gunda wrote: >> 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 <kgunda@codeaurora.org> >> --- >> .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++ > > ... and also put all the binding in one patch. I want to review the > full > view of the h/w, not piecemeal. > Sure. Will send the next series with the changes suggested. >> drivers/video/backlight/qcom-spmi-wled.c | 126 >> ++++++++++++++++++++- >> 2 files changed, 142 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote: > 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 <kgunda@codeaurora.org> > --- > .../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. > + 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". > + > +- 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. > + > 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. > + 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. > + > /* 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. > }; > > 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. > }; > > 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? > +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? > + 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"? > + 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. > + 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. > + 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. > + 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? > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017-12-05 10:05, Bjorn Andersson wrote: > On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote: > >> 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 <kgunda@codeaurora.org> >> --- >> .../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. > Ok. Will address it in next series. >> + 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". > Ok. Will address it in next series. >> + >> +- 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. > Ok. Will address it in next series. >> + >> 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. > Ok. Will address it in next series. >> + 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. > Ok. Will address it in next series. >> + >> /* 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. > Ok. Will address it in next series. >> 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. > Ok. Will address it in next series. >> }; >> >> 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. > Ok. Will address it in next series. >> }; >> >> 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? > Yes. It avoid the race between the upate_status and sc_irq as the module is enabled at one place and disabled at other place respectively. >> +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? > As this SC irq gets triggered in very rare conditions, i think it is okay to have a print for the information purpose. >> + 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) > ... > Ok. sure. >> + pr_err("SC trigged %d times, disabling WLED forever!\n", > > "forever" as in "until someone turns it on again"? > Yes. It is turned on for the next reboot or some one forcefully enables it form the sysfs. >> + 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. > Ok. sure. >> + 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. > Sure. Will remove it. >> + 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. > Sure. Will correct it. >> + 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? > External FET is controlled through the DTEST2 register. External FET is > not part of the WLED IP so it is controlled from the DTEST pins. >> + 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. > Ok. Sure. >> return 0; >> } >> > > Regards, > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + Usage: optional + Value type: <bool> + Definition: Specify if external PFET control for short circuit + protection is needed. + +- 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". + 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>; + 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 + /* 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; bool en_cabc; + bool ext_pfet_sc_pro_en; }; 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; }; 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); if (brightness) { rc = qcom_wled_set_brightness(wled, brightness); if (rc < 0) { pr_err("wled failed to set brightness rc:%d\n", rc); - return rc; + goto unlock_mutex; } if (!!brightness != wled->prev_state) { rc = qcom_wled_module_enable(wled, !!brightness); if (rc < 0) { pr_err("wled enable failed rc:%d\n", rc); - return rc; + goto unlock_mutex; } } } else { rc = qcom_wled_module_enable(wled, brightness); if (rc < 0) { pr_err("wled disable failed rc:%d\n", rc); - return rc; + goto unlock_mutex; } } @@ -184,19 +207,69 @@ static int qcom_wled_update_status(struct backlight_device *bl) rc = qcom_wled_sync_toggle(wled); if (rc < 0) { pr_err("wled sync failed rc:%d\n", rc); - return rc; + goto unlock_mutex; } wled->brightness = brightness; +unlock_mutex: + mutex_unlock(&wled->lock); return rc; } +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); + 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) { + pr_err("SC trigged %d times, disabling WLED forever!\n", + 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) { + 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); + 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 */ + 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); + 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"); + return 0; } @@ -469,6 +581,8 @@ static int qcom_wled_probe(struct platform_device *pdev) return rc; } + mutex_init(&wled->lock); + val = QCOM_WLED_DEFAULT_BRIGHTNESS; of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); wled->brightness = val;
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 <kgunda@codeaurora.org> --- .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++ drivers/video/backlight/qcom-spmi-wled.c | 126 ++++++++++++++++++++- 2 files changed, 142 insertions(+), 6 deletions(-)