From: kgunda@codeaurora.org To: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: linux-arm-msm@vger.kernel.org, Lee Jones <lee.jones@linaro.org>, Daniel Thompson <daniel.thompson@linaro.org>, Jingoo Han <jingoohan1@gmail.com>, Richard Purdie <rpurdie@rpsys.net>, Jacek Anaszewski <jacek.anaszewski@gmail.com>, Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling Date: Mon, 11 Dec 2017 14:58:24 +0530 [thread overview] Message-ID: <f1295a358459da5ad0e680657f3d7457@codeaurora.org> (raw) In-Reply-To: <20171205043515.GE28761@minitux> 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
WARNING: multiple messages have this Message-ID (diff)
From: kgunda@codeaurora.org To: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: linux-arm-msm@vger.kernel.org, Lee Jones <lee.jones@linaro.org>, Daniel Thompson <daniel.thompson@linaro.org>, Jingoo Han <jingoohan1@gmail.com>, Richard Purdie <rpurdie@rpsys.net>, Jacek Anaszewski <jacek.anaszewski@gmail.com>, Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling Date: Mon, 11 Dec 2017 09:40:24 +0000 [thread overview] Message-ID: <f1295a358459da5ad0e680657f3d7457@codeaurora.org> (raw) In-Reply-To: <20171205043515.GE28761@minitux> 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
next prev parent reply other threads:[~2017-12-11 9:28 UTC|newest] Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-16 12:18 [PATCH V1 0/4] qcom: spmi-wled: Support for QCOM wled driver Kiran Gunda 2017-11-16 12:18 ` [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling Kiran Gunda 2017-11-16 12:30 ` Kiran Gunda [not found] ` <1510834717-21765-3-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-11-17 20:30 ` Rob Herring 2017-11-17 20:30 ` Rob Herring 2017-11-17 20:30 ` Rob Herring 2017-11-20 11:42 ` kgunda 2017-11-20 11:54 ` kgunda 2017-12-05 4:35 ` Bjorn Andersson 2017-12-05 4:35 ` Bjorn Andersson 2017-12-05 4:35 ` Bjorn Andersson 2017-12-11 9:28 ` kgunda [this message] 2017-12-11 9:40 ` kgunda 2017-11-16 12:18 ` [PATCH V1 3/4] qcom: spmi-wled: Add support for OVP interrupt handling Kiran Gunda 2017-11-16 12:30 ` Kiran Gunda [not found] ` <1510834717-21765-4-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-12-05 4:45 ` Bjorn Andersson 2017-12-05 4:45 ` Bjorn Andersson 2017-12-05 4:45 ` Bjorn Andersson 2017-12-11 9:31 ` kgunda 2017-12-11 9:43 ` kgunda [not found] ` <1510834717-21765-1-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-11-16 12:18 ` [PATCH V1 1/4] qcom: spmi-wled: Add support for qcom wled driver Kiran Gunda 2017-11-16 12:30 ` Kiran Gunda 2017-11-16 12:18 ` Kiran Gunda 2017-11-16 16:55 ` Bjorn Andersson 2017-11-16 16:55 ` Bjorn Andersson 2017-11-17 6:36 ` kgunda 2017-11-17 6:48 ` kgunda 2017-11-17 6:56 ` Bjorn Andersson 2017-11-17 6:56 ` Bjorn Andersson 2017-11-17 8:33 ` Lee Jones 2017-11-17 8:33 ` Lee Jones 2017-11-17 11:01 ` kgunda 2017-11-17 11:13 ` kgunda 2017-11-17 9:52 ` kgunda-sgV2jX0FEOL9JmXXK+q4OQ 2017-11-17 9:52 ` kgunda 2017-11-17 9:52 ` kgunda 2017-11-17 20:28 ` Rob Herring 2017-11-17 20:28 ` Rob Herring 2017-12-05 2:01 ` Bjorn Andersson 2017-12-05 2:01 ` Bjorn Andersson 2017-12-11 9:11 ` kgunda 2017-12-11 9:23 ` kgunda [not found] ` <1510834717-21765-2-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2017-12-15 20:30 ` Pavel Machek 2017-12-15 20:30 ` Pavel Machek 2017-12-15 20:30 ` Pavel Machek 2017-11-16 12:18 ` [PATCH V1 4/4] qcom: spmi-wled: Add auto-calibration logic support Kiran Gunda 2017-11-16 12:30 ` Kiran Gunda 2017-11-16 12:18 ` Kiran Gunda 2017-12-05 5:40 ` Bjorn Andersson 2017-12-05 5:40 ` Bjorn Andersson 2018-04-19 10:45 ` kgunda 2018-04-19 10:57 ` kgunda 2018-04-19 15:58 ` Bjorn Andersson 2018-04-19 15:58 ` Bjorn Andersson 2018-04-20 5:43 ` kgunda 2018-04-20 5:55 ` kgunda 2018-04-20 16:03 ` Bjorn Andersson 2018-04-20 16:03 ` Bjorn Andersson 2018-04-23 11:26 ` kgunda 2018-04-23 11:38 ` kgunda 2018-04-23 10:35 ` kgunda 2018-04-23 10:47 ` kgunda
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=f1295a358459da5ad0e680657f3d7457@codeaurora.org \ --to=kgunda@codeaurora.org \ --cc=b.zolnierkie@samsung.com \ --cc=bjorn.andersson@linaro.org \ --cc=daniel.thompson@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=jacek.anaszewski@gmail.com \ --cc=jingoohan1@gmail.com \ --cc=lee.jones@linaro.org \ --cc=linux-arm-msm-owner@vger.kernel.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-leds@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=pavel@ucw.cz \ --cc=robh+dt@kernel.org \ --cc=rpurdie@rpsys.net \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.