| Message ID | 1525341432-15818-4-git-send-email-kgunda@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kiran,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on backlight/for-backlight-next]
[also build test WARNING on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kiran-Gunda/backlight-qcom-wled-Support-for-QCOM-wled-driver/20180504-011042
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
smatch warnings:
drivers/video/backlight/qcom-wled.c:304 wled_update_status() warn: inconsistent returns 'mutex:&wled->lock'.
Locked on: line 287
Unlocked on: line 304
# https://github.com/0day-ci/linux/commit/3856199804123df89ef9a712f0740978ec71ddf6
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 3856199804123df89ef9a712f0740978ec71ddf6
vim +304 drivers/video/backlight/qcom-wled.c
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 263
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 264 static int wled_update_status(struct backlight_device *bl)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 265 {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 266 struct wled *wled = bl_get_data(bl);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 267 u16 brightness = bl->props.brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 268 int rc = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 269
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 270 if (bl->props.power != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 271 bl->props.fb_blank != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 272 bl->props.state & BL_CORE_FBBLANK)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 273 brightness = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 274
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 275 mutex_lock(&wled->lock);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 276 if (brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 277 rc = wled->wled_set_brightness(wled, brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 278 if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 279 dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 280 rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 281 goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 282 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 283
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 284 rc = wled->wled_sync_toggle(wled);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 285 if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 286 dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
93c64f1e drivers/leds/leds-pm8941-wled.c Courtney Cavin 2015-03-12 287 return rc;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 288 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 289 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 290
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 291 if (!!brightness != !!wled->brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 292 rc = wled_module_enable(wled, !!brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 293 if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 294 dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 295 goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 296 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 297 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 298
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 299 wled->brightness = brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 300
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 301 unlock_mutex:
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 302 mutex_unlock(&wled->lock);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 303
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 @304 return rc;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 305 }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 306
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
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 2018-05-07 13:36, Dan Carpenter wrote: > Hi Kiran, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on backlight/for-backlight-next] > [also build test WARNING on v4.17-rc3 next-20180504] > [if your patch is applied to the wrong git tree, please drop us a note > to help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Kiran-Gunda/backlight-qcom-wled-Support-for-QCOM-wled-driver/20180504-011042 > base: > https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git > for-backlight-next > > smatch warnings: > drivers/video/backlight/qcom-wled.c:304 wled_update_status() warn: > inconsistent returns 'mutex:&wled->lock'. > Locked on: line 287 > Unlocked on: line 304 > Will fix it in the next series. > # > https://github.com/0day-ci/linux/commit/3856199804123df89ef9a712f0740978ec71ddf6 > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout 3856199804123df89ef9a712f0740978ec71ddf6 > vim +304 drivers/video/backlight/qcom-wled.c > > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 263 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 264 static int wled_update_status(struct backlight_device *bl) > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 265 { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 266 struct wled *wled = bl_get_data(bl); > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 267 u16 brightness = bl->props.brightness; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 268 int rc = 0; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 269 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 270 if (bl->props.power != FB_BLANK_UNBLANK || > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 271 bl->props.fb_blank != FB_BLANK_UNBLANK || > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 272 bl->props.state & BL_CORE_FBBLANK) > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 273 brightness = 0; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 274 > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 275 mutex_lock(&wled->lock); > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 276 if (brightness) { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 277 rc = wled->wled_set_brightness(wled, brightness); > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 278 if (rc < 0) { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 279 dev_err(wled->dev, "wled failed to set brightness rc:%d\n", > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 280 rc); > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 281 goto unlock_mutex; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 282 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 283 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 284 rc = wled->wled_sync_toggle(wled); > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 285 if (rc < 0) { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 286 dev_err(wled->dev, "wled sync failed rc:%d\n", rc); > 93c64f1e drivers/leds/leds-pm8941-wled.c Courtney Cavin 2015-03-12 > 287 return rc; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 288 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 289 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 290 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 291 if (!!brightness != !!wled->brightness) { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 292 rc = wled_module_enable(wled, !!brightness); > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 293 if (rc < 0) { > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 294 dev_err(wled->dev, "wled enable failed rc:%d\n", rc); > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 295 goto unlock_mutex; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 296 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 297 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 298 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 299 wled->brightness = brightness; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 300 > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 301 unlock_mutex: > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 302 mutex_unlock(&wled->lock); > 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 303 > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > @304 return rc; > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 305 } > 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda 2018-05-03 > 306 > > --- > 0-DAY kernel test infrastructure Open Source Technology > Center > https://lists.01.org/pipermail/kbuild-all Intel > Corporation > -- > To unsubscribe from this list: send the line "unsubscribe > linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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 03 May 02:57 PDT 2018, Kiran Gunda wrote: > Handle the short circuit interrupt and check if the short circuit > interrupt is valid. Re-enable the module to check if it goes > away. Disable the module altogether if the short circuit event > persists. > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> > --- > drivers/video/backlight/qcom-wled.c | 134 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 130 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c > index be8b8d3..2cfba77 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -10,6 +10,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,9 @@ > > #define WLED3_SINK_REG_BRIGHT_MAX 0xFFF > > -/* Control registers */ > +/* WLED3 Control registers */ Minor nit, please move the change of this comment to the previous patch. > +#define WLED3_CTRL_REG_FAULT_STATUS 0x08 Unused. > + > #define WLED3_CTRL_REG_MOD_EN 0x46 > #define WLED3_CTRL_REG_MOD_EN_MASK BIT(7) > > @@ -36,7 +41,17 @@ > #define WLED3_CTRL_REG_ILIMIT 0x4e > #define WLED3_CTRL_REG_ILIMIT_MASK GENMASK(2, 0) > > -/* sink registers */ Please move comment change to previous commit. > +/* WLED4 control registers */ > +#define WLED4_CTRL_REG_SHORT_PROTECT 0x5e > +#define WLED4_CTRL_REG_SHORT_EN_MASK BIT(7) > + > +#define WLED4_CTRL_REG_SEC_ACCESS 0xd0 > +#define WLED4_CTRL_REG_SEC_UNLOCK 0xa5 > + > +#define WLED4_CTRL_REG_TEST1 0xe2 > +#define WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2 0x09 > + > +/* WLED3 sink registers */ > #define WLED3_SINK_REG_SYNC 0x47 > #define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0) > #define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0) > @@ -130,17 +145,23 @@ struct wled_config { > bool cs_out_en; > bool ext_gen; > bool cabc; > + bool external_pfet; > }; > > struct wled { > const char *name; > struct device *dev; > struct regmap *regmap; > + struct mutex lock; /* Lock to avoid race from ISR */ > + ktime_t last_short_event; > u16 ctrl_addr; > u16 sink_addr; > u32 brightness; > u32 max_brightness; > + u32 short_count; > const int *version; > + int short_irq; > + bool force_mod_disable; "bool disabled_by_short" would describe what's going on better. > > struct wled_config cfg; > int (*wled_set_brightness)(struct wled *wled, u16 brightness); > @@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, int val) > { > int rc; > > + if (wled->force_mod_disable) > + return 0; > + I don't fancy the idea of not telling user space that it's request to turn on the backlight is ignored. Can we return some error here so that it's possible for something to do something about this problem? > rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + > WLED3_CTRL_REG_MOD_EN, > WLED3_CTRL_REG_MOD_EN_MASK, > @@ -248,12 +272,13 @@ static int wled_update_status(struct backlight_device *bl) > bl->props.state & BL_CORE_FBBLANK) > brightness = 0; > > + mutex_lock(&wled->lock); > if (brightness) { > rc = wled->wled_set_brightness(wled, brightness); > if (rc < 0) { > dev_err(wled->dev, "wled failed to set brightness rc:%d\n", > rc); > - return rc; > + goto unlock_mutex; > } > > rc = wled->wled_sync_toggle(wled); > @@ -267,15 +292,60 @@ static int wled_update_status(struct backlight_device *bl) > rc = wled_module_enable(wled, !!brightness); > if (rc < 0) { > dev_err(wled->dev, "wled enable failed rc:%d\n", rc); > - return rc; > + goto unlock_mutex; > } > } > > wled->brightness = brightness; > > +unlock_mutex: > + mutex_unlock(&wled->lock); > + > return rc; > } > > +#define WLED_SHORT_DLY_MS 20 > +#define WLED_SHORT_CNT_MAX 5 > +#define WLED_SHORT_RESET_CNT_DLY_US HZ HZ is in the unit of jiffies, not micro seconds. > +static irqreturn_t wled_short_irq_handler(int irq, void *_wled) > +{ > + struct wled *wled = _wled; > + int rc; > + s64 elapsed_time; > + > + wled->short_count++; > + mutex_lock(&wled->lock); > + rc = wled_module_enable(wled, false); > + if (rc < 0) { > + dev_err(wled->dev, "wled disable failed rc:%d\n", rc); > + goto unlock_mutex; > + } > + > + elapsed_time = ktime_us_delta(ktime_get(), > + wled->last_short_event); > + if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US) > + wled->short_count = 0; > + > + if (wled->short_count > WLED_SHORT_CNT_MAX) { > + dev_err(wled->dev, "Short trigged %d times, disabling WLED forever!\n", > + wled->short_count); > + wled->force_mod_disable = true; > + goto unlock_mutex; > + } > + > + wled->last_short_event = ktime_get(); > + > + msleep(WLED_SHORT_DLY_MS); > + rc = wled_module_enable(wled, true); > + if (rc < 0) > + dev_err(wled->dev, "wled enable failed rc:%d\n", rc); > + > +unlock_mutex: > + mutex_unlock(&wled->lock); > + > + return IRQ_HANDLED; > +} > + > static int wled3_setup(struct wled *wled) > { > u16 addr; > @@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled) > return rc; > } > > + if (wled->cfg.external_pfet) { > + /* Unlock the secure register access */ > + rc = regmap_write(wled->regmap, wled->ctrl_addr + > + WLED4_CTRL_REG_SEC_ACCESS, > + WLED4_CTRL_REG_SEC_UNLOCK); > + if (rc < 0) > + return rc; > + > + rc = regmap_write(wled->regmap, > + wled->ctrl_addr + WLED4_CTRL_REG_TEST1, > + WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2); > + if (rc < 0) > + return rc; > + } > + > return 0; > } > > @@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled) > .switch_freq = 11, > .enabled_strings = 0xf, > .cabc = false, > + .external_pfet = true, You added the "qcom,external-pfet" boolean to dt, but this forces it to always be set - i.e. this is either wrong or you can omit the new dt property. > }; > > static const u32 wled3_boost_i_limit_values[] = { > @@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled) > { "qcom,cs-out", &cfg->cs_out_en, }, > { "qcom,ext-gen", &cfg->ext_gen, }, > { "qcom,cabc", &cfg->cabc, }, > + { "qcom,external-pfet", &cfg->external_pfet, }, > }; > > prop_addr = of_get_address(dev->of_node, 0, NULL, NULL); > @@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled) > return 0; > } > > +static int wled_configure_short_irq(struct wled *wled, > + struct platform_device *pdev) > +{ > + int rc = 0; > + > + /* PM8941 doesn't have the short detection support */ > + if (*wled->version == WLED_PM8941) > + return 0; If you attempt to acquire the "short" irq first in this function you don't need this check - as "short" won't exist on pm8941. Otherwise, it would be better if you add a "bool has_short_detect" to the wled struct and check that, rather than sprinkling version checks throughout. Or...is cfg->external_pfet already denoting this? > + > + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + > + WLED4_CTRL_REG_SHORT_PROTECT, > + WLED4_CTRL_REG_SHORT_EN_MASK, > + WLED4_CTRL_REG_SHORT_EN_MASK); Do you really want to enable the short protect thing before figuring out if you have a "short" irq. > + if (rc < 0) > + return rc; > + > + wled->short_irq = platform_get_irq_byname(pdev, "short"); short_irq isn't used outside this function, so preferably you turn it into a local variable. > + if (wled->short_irq < 0) { > + dev_dbg(&pdev->dev, "short irq is not used\n"); > + return 0; > + } > + > + rc = devm_request_threaded_irq(wled->dev, wled->short_irq, > + NULL, wled_short_irq_handler, > + IRQF_ONESHOT, > + "wled_short_irq", wled); > + if (rc < 0) > + dev_err(wled->dev, "Unable to request short_irq (err:%d)\n", > + rc); > + > + return rc; > +} > + > static const struct backlight_ops wled_ops = { > .update_status = wled_update_status, > }; > @@ -733,6 +853,8 @@ static int wled_probe(struct platform_device *pdev) > return -ENODEV; > } > > + mutex_init(&wled->lock); > + > rc = wled_configure(wled); > if (rc) > return rc; > @@ -752,6 +874,10 @@ static int wled_probe(struct platform_device *pdev) > } > } > > + rc = wled_configure_short_irq(wled, pdev); > + if (rc < 0) > + return rc; > + > val = WLED_DEFAULT_BRIGHTNESS; > of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); 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 2018-05-07 22:36, Bjorn Andersson wrote: > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote: > >> Handle the short circuit interrupt and check if the short circuit >> interrupt is valid. Re-enable the module to check if it goes >> away. Disable the module altogether if the short circuit event >> persists. >> >> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> >> --- >> drivers/video/backlight/qcom-wled.c | 134 >> ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 130 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/video/backlight/qcom-wled.c >> b/drivers/video/backlight/qcom-wled.c >> index be8b8d3..2cfba77 100644 >> --- a/drivers/video/backlight/qcom-wled.c >> +++ b/drivers/video/backlight/qcom-wled.c >> @@ -10,6 +10,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,9 @@ >> >> #define WLED3_SINK_REG_BRIGHT_MAX 0xFFF >> >> -/* Control registers */ >> +/* WLED3 Control registers */ > > Minor nit, please move the change of this comment to the previous > patch. > Ok. Will do it the next series. >> +#define WLED3_CTRL_REG_FAULT_STATUS 0x08 > > Unused. > Will remove it in the next series. >> + >> #define WLED3_CTRL_REG_MOD_EN 0x46 >> #define WLED3_CTRL_REG_MOD_EN_MASK BIT(7) >> >> @@ -36,7 +41,17 @@ >> #define WLED3_CTRL_REG_ILIMIT 0x4e >> #define WLED3_CTRL_REG_ILIMIT_MASK GENMASK(2, 0) >> >> -/* sink registers */ > > Please move comment change to previous commit. > Will remove it in the next series. >> +/* WLED4 control registers */ >> +#define WLED4_CTRL_REG_SHORT_PROTECT 0x5e >> +#define WLED4_CTRL_REG_SHORT_EN_MASK BIT(7) >> + >> +#define WLED4_CTRL_REG_SEC_ACCESS 0xd0 >> +#define WLED4_CTRL_REG_SEC_UNLOCK 0xa5 >> + >> +#define WLED4_CTRL_REG_TEST1 0xe2 >> +#define WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2 0x09 >> + >> +/* WLED3 sink registers */ >> #define WLED3_SINK_REG_SYNC 0x47 >> #define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0) >> #define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0) >> @@ -130,17 +145,23 @@ struct wled_config { >> bool cs_out_en; >> bool ext_gen; >> bool cabc; >> + bool external_pfet; >> }; >> >> struct wled { >> const char *name; >> struct device *dev; >> struct regmap *regmap; >> + struct mutex lock; /* Lock to avoid race from ISR */ >> + ktime_t last_short_event; >> u16 ctrl_addr; >> u16 sink_addr; >> u32 brightness; >> u32 max_brightness; >> + u32 short_count; >> const int *version; >> + int short_irq; >> + bool force_mod_disable; > > "bool disabled_by_short" would describe what's going on better. > Will rename it in the next series. >> >> struct wled_config cfg; >> int (*wled_set_brightness)(struct wled *wled, u16 brightness); >> @@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, >> int val) >> { >> int rc; >> >> + if (wled->force_mod_disable) >> + return 0; >> + > > I don't fancy the idea of not telling user space that it's request to > turn on the backlight is ignored. Can we return some error here so that > it's possible for something to do something about this problem? > Sure. Will do it in the next series. >> rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + >> WLED3_CTRL_REG_MOD_EN, >> WLED3_CTRL_REG_MOD_EN_MASK, >> @@ -248,12 +272,13 @@ static int wled_update_status(struct >> backlight_device *bl) >> bl->props.state & BL_CORE_FBBLANK) >> brightness = 0; >> >> + mutex_lock(&wled->lock); >> if (brightness) { >> rc = wled->wled_set_brightness(wled, brightness); >> if (rc < 0) { >> dev_err(wled->dev, "wled failed to set brightness rc:%d\n", >> rc); >> - return rc; >> + goto unlock_mutex; >> } >> >> rc = wled->wled_sync_toggle(wled); >> @@ -267,15 +292,60 @@ static int wled_update_status(struct >> backlight_device *bl) >> rc = wled_module_enable(wled, !!brightness); >> if (rc < 0) { >> dev_err(wled->dev, "wled enable failed rc:%d\n", rc); >> - return rc; >> + goto unlock_mutex; >> } >> } >> >> wled->brightness = brightness; >> >> +unlock_mutex: >> + mutex_unlock(&wled->lock); >> + >> return rc; >> } >> >> +#define WLED_SHORT_DLY_MS 20 >> +#define WLED_SHORT_CNT_MAX 5 >> +#define WLED_SHORT_RESET_CNT_DLY_US HZ > > HZ is in the unit of jiffies, not micro seconds. > Will address in next series. >> +static irqreturn_t wled_short_irq_handler(int irq, void *_wled) >> +{ >> + struct wled *wled = _wled; >> + int rc; >> + s64 elapsed_time; >> + >> + wled->short_count++; >> + mutex_lock(&wled->lock); >> + rc = wled_module_enable(wled, false); >> + if (rc < 0) { >> + dev_err(wled->dev, "wled disable failed rc:%d\n", rc); >> + goto unlock_mutex; >> + } >> + >> + elapsed_time = ktime_us_delta(ktime_get(), >> + wled->last_short_event); >> + if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US) >> + wled->short_count = 0; >> + >> + if (wled->short_count > WLED_SHORT_CNT_MAX) { >> + dev_err(wled->dev, "Short trigged %d times, disabling WLED >> forever!\n", >> + wled->short_count); >> + wled->force_mod_disable = true; >> + goto unlock_mutex; >> + } >> + >> + wled->last_short_event = ktime_get(); >> + >> + msleep(WLED_SHORT_DLY_MS); >> + rc = wled_module_enable(wled, true); >> + if (rc < 0) >> + dev_err(wled->dev, "wled enable failed rc:%d\n", rc); >> + >> +unlock_mutex: >> + mutex_unlock(&wled->lock); >> + >> + return IRQ_HANDLED; >> +} >> + >> static int wled3_setup(struct wled *wled) >> { >> u16 addr; >> @@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled) >> return rc; >> } >> >> + if (wled->cfg.external_pfet) { >> + /* Unlock the secure register access */ >> + rc = regmap_write(wled->regmap, wled->ctrl_addr + >> + WLED4_CTRL_REG_SEC_ACCESS, >> + WLED4_CTRL_REG_SEC_UNLOCK); >> + if (rc < 0) >> + return rc; >> + >> + rc = regmap_write(wled->regmap, >> + wled->ctrl_addr + WLED4_CTRL_REG_TEST1, >> + WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2); >> + if (rc < 0) >> + return rc; >> + } >> + >> return 0; >> } >> >> @@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled) >> .switch_freq = 11, >> .enabled_strings = 0xf, >> .cabc = false, >> + .external_pfet = true, > > You added the "qcom,external-pfet" boolean to dt, but this forces it to > always be set - i.e. this is either wrong or you can omit the new dt > property. > I will make it to false in the next series. >> }; >> >> static const u32 wled3_boost_i_limit_values[] = { >> @@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled) >> { "qcom,cs-out", &cfg->cs_out_en, }, >> { "qcom,ext-gen", &cfg->ext_gen, }, >> { "qcom,cabc", &cfg->cabc, }, >> + { "qcom,external-pfet", &cfg->external_pfet, }, >> }; >> >> prop_addr = of_get_address(dev->of_node, 0, NULL, NULL); >> @@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled) >> return 0; >> } >> >> +static int wled_configure_short_irq(struct wled *wled, >> + struct platform_device *pdev) >> +{ >> + int rc = 0; >> + >> + /* PM8941 doesn't have the short detection support */ >> + if (*wled->version == WLED_PM8941) >> + return 0; > > If you attempt to acquire the "short" irq first in this function you > don't need this check - as "short" won't exist on pm8941. > > Otherwise, it would be better if you add a "bool has_short_detect" to > the wled struct and check that, rather than sprinkling version checks > throughout. > > > Or...is cfg->external_pfet already denoting this? > I think it is better to add the has_short_detect variable, as the external_pfet is only specific to PMI8998. >> + >> + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + >> + WLED4_CTRL_REG_SHORT_PROTECT, >> + WLED4_CTRL_REG_SHORT_EN_MASK, >> + WLED4_CTRL_REG_SHORT_EN_MASK); > > Do you really want to enable the short protect thing before figuring > out > if you have a "short" irq. > Yes. Irrespective of the interrupt, the short circuit detection should be enabled to protect the HW from the short circuit. >> + if (rc < 0) >> + return rc; >> + >> + wled->short_irq = platform_get_irq_byname(pdev, "short"); > > short_irq isn't used outside this function, so preferably you turn it > into a local variable. > Ok. Will do that in the next series. >> + if (wled->short_irq < 0) { >> + dev_dbg(&pdev->dev, "short irq is not used\n"); >> + return 0; >> + } >> + >> + rc = devm_request_threaded_irq(wled->dev, wled->short_irq, >> + NULL, wled_short_irq_handler, >> + IRQF_ONESHOT, >> + "wled_short_irq", wled); >> + if (rc < 0) >> + dev_err(wled->dev, "Unable to request short_irq (err:%d)\n", >> + rc); >> + >> + return rc; >> +} >> + >> static const struct backlight_ops wled_ops = { >> .update_status = wled_update_status, >> }; >> @@ -733,6 +853,8 @@ static int wled_probe(struct platform_device >> *pdev) >> return -ENODEV; >> } >> >> + mutex_init(&wled->lock); >> + >> rc = wled_configure(wled); >> if (rc) >> return rc; >> @@ -752,6 +874,10 @@ static int wled_probe(struct platform_device >> *pdev) >> } >> } >> >> + rc = wled_configure_short_irq(wled, pdev); >> + if (rc < 0) >> + return rc; >> + >> val = WLED_DEFAULT_BRIGHTNESS; >> of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); > > 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/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index be8b8d3..2cfba77 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -10,6 +10,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,9 @@ #define WLED3_SINK_REG_BRIGHT_MAX 0xFFF -/* Control registers */ +/* WLED3 Control registers */ +#define WLED3_CTRL_REG_FAULT_STATUS 0x08 + #define WLED3_CTRL_REG_MOD_EN 0x46 #define WLED3_CTRL_REG_MOD_EN_MASK BIT(7) @@ -36,7 +41,17 @@ #define WLED3_CTRL_REG_ILIMIT 0x4e #define WLED3_CTRL_REG_ILIMIT_MASK GENMASK(2, 0) -/* sink registers */ +/* WLED4 control registers */ +#define WLED4_CTRL_REG_SHORT_PROTECT 0x5e +#define WLED4_CTRL_REG_SHORT_EN_MASK BIT(7) + +#define WLED4_CTRL_REG_SEC_ACCESS 0xd0 +#define WLED4_CTRL_REG_SEC_UNLOCK 0xa5 + +#define WLED4_CTRL_REG_TEST1 0xe2 +#define WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2 0x09 + +/* WLED3 sink registers */ #define WLED3_SINK_REG_SYNC 0x47 #define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0) #define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0) @@ -130,17 +145,23 @@ struct wled_config { bool cs_out_en; bool ext_gen; bool cabc; + bool external_pfet; }; struct wled { const char *name; struct device *dev; struct regmap *regmap; + struct mutex lock; /* Lock to avoid race from ISR */ + ktime_t last_short_event; u16 ctrl_addr; u16 sink_addr; u32 brightness; u32 max_brightness; + u32 short_count; const int *version; + int short_irq; + bool force_mod_disable; struct wled_config cfg; int (*wled_set_brightness)(struct wled *wled, u16 brightness); @@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, int val) { int rc; + if (wled->force_mod_disable) + return 0; + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN, WLED3_CTRL_REG_MOD_EN_MASK, @@ -248,12 +272,13 @@ static int wled_update_status(struct backlight_device *bl) bl->props.state & BL_CORE_FBBLANK) brightness = 0; + mutex_lock(&wled->lock); if (brightness) { rc = wled->wled_set_brightness(wled, brightness); if (rc < 0) { dev_err(wled->dev, "wled failed to set brightness rc:%d\n", rc); - return rc; + goto unlock_mutex; } rc = wled->wled_sync_toggle(wled); @@ -267,15 +292,60 @@ static int wled_update_status(struct backlight_device *bl) rc = wled_module_enable(wled, !!brightness); if (rc < 0) { dev_err(wled->dev, "wled enable failed rc:%d\n", rc); - return rc; + goto unlock_mutex; } } wled->brightness = brightness; +unlock_mutex: + mutex_unlock(&wled->lock); + return rc; } +#define WLED_SHORT_DLY_MS 20 +#define WLED_SHORT_CNT_MAX 5 +#define WLED_SHORT_RESET_CNT_DLY_US HZ +static irqreturn_t wled_short_irq_handler(int irq, void *_wled) +{ + struct wled *wled = _wled; + int rc; + s64 elapsed_time; + + wled->short_count++; + mutex_lock(&wled->lock); + rc = wled_module_enable(wled, false); + if (rc < 0) { + dev_err(wled->dev, "wled disable failed rc:%d\n", rc); + goto unlock_mutex; + } + + elapsed_time = ktime_us_delta(ktime_get(), + wled->last_short_event); + if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US) + wled->short_count = 0; + + if (wled->short_count > WLED_SHORT_CNT_MAX) { + dev_err(wled->dev, "Short trigged %d times, disabling WLED forever!\n", + wled->short_count); + wled->force_mod_disable = true; + goto unlock_mutex; + } + + wled->last_short_event = ktime_get(); + + msleep(WLED_SHORT_DLY_MS); + rc = wled_module_enable(wled, true); + if (rc < 0) + dev_err(wled->dev, "wled enable failed rc:%d\n", rc); + +unlock_mutex: + mutex_unlock(&wled->lock); + + return IRQ_HANDLED; +} + static int wled3_setup(struct wled *wled) { u16 addr; @@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled) return rc; } + if (wled->cfg.external_pfet) { + /* Unlock the secure register access */ + rc = regmap_write(wled->regmap, wled->ctrl_addr + + WLED4_CTRL_REG_SEC_ACCESS, + WLED4_CTRL_REG_SEC_UNLOCK); + if (rc < 0) + return rc; + + rc = regmap_write(wled->regmap, + wled->ctrl_addr + WLED4_CTRL_REG_TEST1, + WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2); + if (rc < 0) + return rc; + } + return 0; } @@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled) .switch_freq = 11, .enabled_strings = 0xf, .cabc = false, + .external_pfet = true, }; static const u32 wled3_boost_i_limit_values[] = { @@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled) { "qcom,cs-out", &cfg->cs_out_en, }, { "qcom,ext-gen", &cfg->ext_gen, }, { "qcom,cabc", &cfg->cabc, }, + { "qcom,external-pfet", &cfg->external_pfet, }, }; prop_addr = of_get_address(dev->of_node, 0, NULL, NULL); @@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled) return 0; } +static int wled_configure_short_irq(struct wled *wled, + struct platform_device *pdev) +{ + int rc = 0; + + /* PM8941 doesn't have the short detection support */ + if (*wled->version == WLED_PM8941) + return 0; + + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + + WLED4_CTRL_REG_SHORT_PROTECT, + WLED4_CTRL_REG_SHORT_EN_MASK, + WLED4_CTRL_REG_SHORT_EN_MASK); + if (rc < 0) + return rc; + + wled->short_irq = platform_get_irq_byname(pdev, "short"); + if (wled->short_irq < 0) { + dev_dbg(&pdev->dev, "short irq is not used\n"); + return 0; + } + + rc = devm_request_threaded_irq(wled->dev, wled->short_irq, + NULL, wled_short_irq_handler, + IRQF_ONESHOT, + "wled_short_irq", wled); + if (rc < 0) + dev_err(wled->dev, "Unable to request short_irq (err:%d)\n", + rc); + + return rc; +} + static const struct backlight_ops wled_ops = { .update_status = wled_update_status, }; @@ -733,6 +853,8 @@ static int wled_probe(struct platform_device *pdev) return -ENODEV; } + mutex_init(&wled->lock); + rc = wled_configure(wled); if (rc) return rc; @@ -752,6 +874,10 @@ static int wled_probe(struct platform_device *pdev) } } + rc = wled_configure_short_irq(wled, pdev); + if (rc < 0) + return rc; + val = WLED_DEFAULT_BRIGHTNESS; of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
Handle the short circuit interrupt and check if the short circuit interrupt is valid. Re-enable the module to check if it goes away. Disable the module altogether if the short circuit event persists. Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> --- drivers/video/backlight/qcom-wled.c | 134 ++++++++++++++++++++++++++++++++++-- 1 file changed, 130 insertions(+), 4 deletions(-)