lkml.org 
[lkml]   [2018]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V1 3/5] backlight: qcom-wled: Add support for short circuit handling
    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

    \
     
     \ /
      Last update: 2018-05-08 12:36    [W:3.573 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site