diff mbox

[V1,4/5] backlight: qcom-wled: Add support for OVP interrupt handling

Message ID 1525341432-15818-5-git-send-email-kgunda@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kiran Gunda May 3, 2018, 9:57 a.m. UTC
WLED peripheral has over voltage protection(OVP) circuitry and the OVP
fault is notified through an interrupt. Though this fault condition rising
is due to an incorrect hardware configuration is mitigated in the hardware,
it still needs to be detected and handled. Add support for it.

When WLED module is enabled, keep OVP fault interrupt disabled for 10 ms to
account for soft start delay.

Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
---
 drivers/video/backlight/qcom-wled.c | 118 +++++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 2 deletions(-)

Comments

Bjorn Andersson May 7, 2018, 5:21 p.m. UTC | #1
On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:

> WLED peripheral has over voltage protection(OVP) circuitry and the OVP
> fault is notified through an interrupt. Though this fault condition rising
> is due to an incorrect hardware configuration is mitigated in the hardware,
> it still needs to be detected and handled. Add support for it.
> 
> When WLED module is enabled, keep OVP fault interrupt disabled for 10 ms to
> account for soft start delay.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 118 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 2cfba77..80ae084 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -23,14 +23,20 @@
>  
>  /* From DT binding */
>  #define WLED_DEFAULT_BRIGHTNESS				2048
> -
> +#define WLED_SOFT_START_DLY_US				10000
>  #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>  
>  /* WLED3 Control registers */
>  #define WLED3_CTRL_REG_FAULT_STATUS			0x08
> +#define  WLED3_CTRL_REG_ILIM_FAULT_BIT			BIT(0)
> +#define  WLED3_CTRL_REG_OVP_FAULT_BIT			BIT(1)
> +#define  WLED4_CTRL_REG_SC_FAULT_BIT			BIT(2)
> +
> +#define WLED3_CTRL_REG_INT_RT_STS			0x10
>  
>  #define WLED3_CTRL_REG_MOD_EN				0x46
>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
> +#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)

"BIT(7)" is not a "bit" it's a "mask", so perhaps you could use the
define with that name...which has the same value?

>  
>  #define WLED3_CTRL_REG_FREQ				0x4c
>  #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
> @@ -161,9 +167,12 @@ struct wled {
>  	u32 short_count;
>  	const int *version;
>  	int short_irq;
> +	int ovp_irq;
>  	bool force_mod_disable;
> +	bool ovp_irq_disabled;
>  
>  	struct wled_config cfg;
> +	struct delayed_work ovp_work;
>  	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
>  	int (*wled_sync_toggle)(struct wled *wled);
>  };
> @@ -209,6 +218,32 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness)
>  	return 0;
>  }
>  
> +static void wled_ovp_work(struct work_struct *work)
> +{
> +	u32 val;
> +	int rc;
> +
> +	struct wled *wled = container_of(work,
> +					 struct wled, ovp_work.work);
> +
> +	rc = regmap_read(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
> +			 &val);
> +	if (rc < 0)
> +		return;
> +
> +	if (val & WLED3_CTRL_REG_MOD_EN_BIT) {
> +		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
> +			enable_irq(wled->ovp_irq);
> +			wled->ovp_irq_disabled = false;
> +		}
> +	} else {
> +		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
> +			disable_irq(wled->ovp_irq);
> +			wled->ovp_irq_disabled = true;
> +		}
> +	}
> +}
> +
>  static int wled_module_enable(struct wled *wled, int val)
>  {
>  	int rc;
> @@ -220,7 +255,12 @@ static int wled_module_enable(struct wled *wled, int val)
>  				WLED3_CTRL_REG_MOD_EN,
>  				WLED3_CTRL_REG_MOD_EN_MASK,
>  				WLED3_CTRL_REG_MOD_EN_MASK);
> -	return rc;
> +	if (rc < 0)
> +		return rc;
> +
> +	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);

Do you really want to delay the work on disable?

Wouldn't it be better to use a delay worker for the enablement and in
the disable case you cancel the work and just disable_irq() directly
here.

But more importantly, if this is only related to auto detection, do you
really want to enable/disable the ovp_irq after you have detected the
string configuration?

> +
> +	return 0;
>  }
>  
>  static int wled3_sync_toggle(struct wled *wled)
> @@ -346,6 +386,36 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
> +{
> +	struct wled *wled = _wled;
> +	int rc;
> +	u32 int_sts, fault_sts;
> +
> +	rc = regmap_read(wled->regmap,
> +			 wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS, &int_sts);
> +	if (rc < 0) {
> +		dev_err(wled->dev, "Error in reading WLED3_INT_RT_STS rc=%d\n",
> +			rc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	rc = regmap_read(wled->regmap, wled->ctrl_addr +
> +			 WLED3_CTRL_REG_FAULT_STATUS, &fault_sts);
> +	if (rc < 0) {
> +		dev_err(wled->dev, "Error in reading WLED_FAULT_STATUS rc=%d\n",
> +			rc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (fault_sts &
> +		(WLED3_CTRL_REG_OVP_FAULT_BIT | WLED3_CTRL_REG_ILIM_FAULT_BIT))
> +		dev_dbg(wled->dev, "WLED OVP fault detected, int_sts=%x fault_sts= %x\n",
> +			int_sts, fault_sts);
> +

It's hard to give a good review on this, as the actual logic comes in
the next patch. And as these two patches both implement auto detection
(without a clear separation) I think you should squash them.

> +	return IRQ_HANDLED;
> +}
> +
>  static int wled3_setup(struct wled *wled)
>  {
>  	u16 addr;
> @@ -821,6 +891,44 @@ static int wled_configure_short_irq(struct wled *wled,
>  	return rc;
>  }
>  
> +static int wled_configure_ovp_irq(struct wled *wled,
> +				  struct platform_device *pdev)
> +{
> +	int rc = 0;
> +	u32 val;
> +
> +	if (*wled->version == WLED_PM8941)
> +		return 0;
> +
> +	wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
> +	if (wled->ovp_irq < 0) {
> +		dev_dbg(&pdev->dev, "ovp irq is not used\n");
> +		return 0;
> +	}
> +
> +	rc = devm_request_threaded_irq(wled->dev, wled->ovp_irq, NULL,
> +				       wled_ovp_irq_handler, IRQF_ONESHOT,
> +				       "wled_ovp_irq", wled);
> +	if (rc < 0) {
> +		dev_err(wled->dev, "Unable to request ovp_irq (err:%d)\n",
> +			rc);

wled->ovp_irq might be > 0 here which means wled_ovp_work() will find it
valid and call enabled_irq()/disable_irq() on it.

> +		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
Kiran Gunda May 8, 2018, 12:26 p.m. UTC | #2
On 2018-05-07 22:51, Bjorn Andersson wrote:
> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> 
>> WLED peripheral has over voltage protection(OVP) circuitry and the OVP
>> fault is notified through an interrupt. Though this fault condition 
>> rising
>> is due to an incorrect hardware configuration is mitigated in the 
>> hardware,
>> it still needs to be detected and handled. Add support for it.
>> 
>> When WLED module is enabled, keep OVP fault interrupt disabled for 10 
>> ms to
>> account for soft start delay.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/video/backlight/qcom-wled.c | 118 
>> +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 116 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index 2cfba77..80ae084 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -23,14 +23,20 @@
>> 
>>  /* From DT binding */
>>  #define WLED_DEFAULT_BRIGHTNESS				2048
>> -
>> +#define WLED_SOFT_START_DLY_US				10000
>>  #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>> 
>>  /* WLED3 Control registers */
>>  #define WLED3_CTRL_REG_FAULT_STATUS			0x08
>> +#define  WLED3_CTRL_REG_ILIM_FAULT_BIT			BIT(0)
>> +#define  WLED3_CTRL_REG_OVP_FAULT_BIT			BIT(1)
>> +#define  WLED4_CTRL_REG_SC_FAULT_BIT			BIT(2)
>> +
>> +#define WLED3_CTRL_REG_INT_RT_STS			0x10
>> 
>>  #define WLED3_CTRL_REG_MOD_EN				0x46
>>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>> +#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
> 
> "BIT(7)" is not a "bit" it's a "mask", so perhaps you could use the
> define with that name...which has the same value?
> 
Sure. I will change it in next series.
>> 
>>  #define WLED3_CTRL_REG_FREQ				0x4c
>>  #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
>> @@ -161,9 +167,12 @@ struct wled {
>>  	u32 short_count;
>>  	const int *version;
>>  	int short_irq;
>> +	int ovp_irq;
>>  	bool force_mod_disable;
>> +	bool ovp_irq_disabled;
>> 
>>  	struct wled_config cfg;
>> +	struct delayed_work ovp_work;
>>  	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
>>  	int (*wled_sync_toggle)(struct wled *wled);
>>  };
>> @@ -209,6 +218,32 @@ static int wled4_set_brightness(struct wled 
>> *wled, u16 brightness)
>>  	return 0;
>>  }
>> 
>> +static void wled_ovp_work(struct work_struct *work)
>> +{
>> +	u32 val;
>> +	int rc;
>> +
>> +	struct wled *wled = container_of(work,
>> +					 struct wled, ovp_work.work);
>> +
>> +	rc = regmap_read(wled->regmap, wled->ctrl_addr + 
>> WLED3_CTRL_REG_MOD_EN,
>> +			 &val);
>> +	if (rc < 0)
>> +		return;
>> +
>> +	if (val & WLED3_CTRL_REG_MOD_EN_BIT) {
>> +		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
>> +			enable_irq(wled->ovp_irq);
>> +			wled->ovp_irq_disabled = false;
>> +		}
>> +	} else {
>> +		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
>> +			disable_irq(wled->ovp_irq);
>> +			wled->ovp_irq_disabled = true;
>> +		}
>> +	}
>> +}
>> +
>>  static int wled_module_enable(struct wled *wled, int val)
>>  {
>>  	int rc;
>> @@ -220,7 +255,12 @@ static int wled_module_enable(struct wled *wled, 
>> int val)
>>  				WLED3_CTRL_REG_MOD_EN,
>>  				WLED3_CTRL_REG_MOD_EN_MASK,
>>  				WLED3_CTRL_REG_MOD_EN_MASK);
>> -	return rc;
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
> 
> Do you really want to delay the work on disable?
> 
> Wouldn't it be better to use a delay worker for the enablement and in
> the disable case you cancel the work and just disable_irq() directly
> here.
> 
Sure. Will do it in the next series.
> But more importantly, if this is only related to auto detection, do you
> really want to enable/disable the ovp_irq after you have detected the
> string configuration?
> 
Ok. This is used for the genuine OVP detection and for the auto 
detection as well.
>> +
>> +	return 0;
>>  }
>> 
>>  static int wled3_sync_toggle(struct wled *wled)
>> @@ -346,6 +386,36 @@ static irqreturn_t wled_short_irq_handler(int 
>> irq, void *_wled)
>>  	return IRQ_HANDLED;
>>  }
>> 
>> +static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
>> +{
>> +	struct wled *wled = _wled;
>> +	int rc;
>> +	u32 int_sts, fault_sts;
>> +
>> +	rc = regmap_read(wled->regmap,
>> +			 wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS, &int_sts);
>> +	if (rc < 0) {
>> +		dev_err(wled->dev, "Error in reading WLED3_INT_RT_STS rc=%d\n",
>> +			rc);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	rc = regmap_read(wled->regmap, wled->ctrl_addr +
>> +			 WLED3_CTRL_REG_FAULT_STATUS, &fault_sts);
>> +	if (rc < 0) {
>> +		dev_err(wled->dev, "Error in reading WLED_FAULT_STATUS rc=%d\n",
>> +			rc);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	if (fault_sts &
>> +		(WLED3_CTRL_REG_OVP_FAULT_BIT | WLED3_CTRL_REG_ILIM_FAULT_BIT))
>> +		dev_dbg(wled->dev, "WLED OVP fault detected, int_sts=%x fault_sts= 
>> %x\n",
>> +			int_sts, fault_sts);
>> +
> 
> It's hard to give a good review on this, as the actual logic comes in
> the next patch. And as these two patches both implement auto detection
> (without a clear separation) I think you should squash them.
> Ok. I will squash them in the next series.
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int wled3_setup(struct wled *wled)
>>  {
>>  	u16 addr;
>> @@ -821,6 +891,44 @@ static int wled_configure_short_irq(struct wled 
>> *wled,
>>  	return rc;
>>  }
>> 
>> +static int wled_configure_ovp_irq(struct wled *wled,
>> +				  struct platform_device *pdev)
>> +{
>> +	int rc = 0;
>> +	u32 val;
>> +
>> +	if (*wled->version == WLED_PM8941)
>> +		return 0;
>> +
>> +	wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
>> +	if (wled->ovp_irq < 0) {
>> +		dev_dbg(&pdev->dev, "ovp irq is not used\n");
>> +		return 0;
>> +	}
>> +
>> +	rc = devm_request_threaded_irq(wled->dev, wled->ovp_irq, NULL,
>> +				       wled_ovp_irq_handler, IRQF_ONESHOT,
>> +				       "wled_ovp_irq", wled);
>> +	if (rc < 0) {
>> +		dev_err(wled->dev, "Unable to request ovp_irq (err:%d)\n",
>> +			rc);
> 
> wled->ovp_irq might be > 0 here which means wled_ovp_work() will find 
> it
> valid and call enabled_irq()/disable_irq() on it.
> 
Ok. I will make it '0' if the "request_threaded_irq" fails.
>> +		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
Bjorn Andersson May 8, 2018, 5:19 p.m. UTC | #3
On Tue 08 May 05:26 PDT 2018, kgunda@codeaurora.org wrote:

> On 2018-05-07 22:51, Bjorn Andersson wrote:
> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
[..]
> > > @@ -220,7 +255,12 @@ static int wled_module_enable(struct wled
> > > *wled, int val)
> > >  				WLED3_CTRL_REG_MOD_EN,
> > >  				WLED3_CTRL_REG_MOD_EN_MASK,
> > >  				WLED3_CTRL_REG_MOD_EN_MASK);
> > > -	return rc;
> > > +	if (rc < 0)
> > > +		return rc;
> > > +
> > > +	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
> > 
> > Do you really want to delay the work on disable?
> > 
> > Wouldn't it be better to use a delay worker for the enablement and in
> > the disable case you cancel the work and just disable_irq() directly
> > here.
> > 
> Sure. Will do it in the next series.
> > But more importantly, if this is only related to auto detection, do you
> > really want to enable/disable the ovp_irq after you have detected the
> > string configuration?
> > 
> Ok. This is used for the genuine OVP detection and for the auto detection as
> well.

What is the expected outcome of detecting an OVP condition, outside auto
detection?

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
Kiran Gunda May 9, 2018, 5:06 a.m. UTC | #4
On 2018-05-08 22:49, Bjorn Andersson wrote:
> On Tue 08 May 05:26 PDT 2018, kgunda@codeaurora.org wrote:
> 
>> On 2018-05-07 22:51, Bjorn Andersson wrote:
>> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> [..]
>> > > @@ -220,7 +255,12 @@ static int wled_module_enable(struct wled
>> > > *wled, int val)
>> > >  				WLED3_CTRL_REG_MOD_EN,
>> > >  				WLED3_CTRL_REG_MOD_EN_MASK,
>> > >  				WLED3_CTRL_REG_MOD_EN_MASK);
>> > > -	return rc;
>> > > +	if (rc < 0)
>> > > +		return rc;
>> > > +
>> > > +	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
>> >
>> > Do you really want to delay the work on disable?
>> >
>> > Wouldn't it be better to use a delay worker for the enablement and in
>> > the disable case you cancel the work and just disable_irq() directly
>> > here.
>> >
>> Sure. Will do it in the next series.
>> > But more importantly, if this is only related to auto detection, do you
>> > really want to enable/disable the ovp_irq after you have detected the
>> > string configuration?
>> >
>> Ok. This is used for the genuine OVP detection and for the auto 
>> detection as
>> well.
> 
> What is the expected outcome of detecting an OVP condition, outside 
> auto
> detection?
> 
Ok... Out side auto detection, it is used for information purpose. I 
think it is
okay to ignore enable/disable the ovp_irq after auto detection is done.
> 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
Kiran Gunda May 9, 2018, 6:16 a.m. UTC | #5
On 2018-05-09 10:36, kgunda@codeaurora.org wrote:
> On 2018-05-08 22:49, Bjorn Andersson wrote:
>> On Tue 08 May 05:26 PDT 2018, kgunda@codeaurora.org wrote:
>> 
>>> On 2018-05-07 22:51, Bjorn Andersson wrote:
>>> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>> [..]
>>> > > @@ -220,7 +255,12 @@ static int wled_module_enable(struct wled
>>> > > *wled, int val)
>>> > >  				WLED3_CTRL_REG_MOD_EN,
>>> > >  				WLED3_CTRL_REG_MOD_EN_MASK,
>>> > >  				WLED3_CTRL_REG_MOD_EN_MASK);
>>> > > -	return rc;
>>> > > +	if (rc < 0)
>>> > > +		return rc;
>>> > > +
>>> > > +	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
>>> >
>>> > Do you really want to delay the work on disable?
>>> >
>>> > Wouldn't it be better to use a delay worker for the enablement and in
>>> > the disable case you cancel the work and just disable_irq() directly
>>> > here.
>>> >
>>> Sure. Will do it in the next series.
>>> > But more importantly, if this is only related to auto detection, do you
>>> > really want to enable/disable the ovp_irq after you have detected the
>>> > string configuration?
>>> >
>>> Ok. This is used for the genuine OVP detection and for the auto 
>>> detection as
>>> well.
>> 
>> What is the expected outcome of detecting an OVP condition, outside 
>> auto
>> detection?
>> 
> Ok... Out side auto detection, it is used for information purpose. I 
> think it is
> okay to ignore enable/disable the ovp_irq after auto detection is done.

I am taking back the above comment. The OVP irq is needed even after the 
auto detection is done.
Because there are also cases where one/more of the connected LED string 
of the display-backlight
is malfunctioning (due to damage) and requires the damaged string to be 
turned off to prevent the
complete panel and/or board from being damaged by running the auto 
detection again.

currently we are not resetting "auto_detection_done" flag once it set to 
true. I will fix it in
the next series to run the auto detection again (If the OVP condition is 
met) because of the
above mentioned reason.

We are going to discuss the HW systems to check if the OVP keep on 
occurring due to some other
reason, other then the string issue, what needs to do (disable the 
module ?).

>> Regards,
>> Bjorn
> --
> 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
diff mbox

Patch

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 2cfba77..80ae084 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -23,14 +23,20 @@ 
 
 /* From DT binding */
 #define WLED_DEFAULT_BRIGHTNESS				2048
-
+#define WLED_SOFT_START_DLY_US				10000
 #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
 
 /* WLED3 Control registers */
 #define WLED3_CTRL_REG_FAULT_STATUS			0x08
+#define  WLED3_CTRL_REG_ILIM_FAULT_BIT			BIT(0)
+#define  WLED3_CTRL_REG_OVP_FAULT_BIT			BIT(1)
+#define  WLED4_CTRL_REG_SC_FAULT_BIT			BIT(2)
+
+#define WLED3_CTRL_REG_INT_RT_STS			0x10
 
 #define WLED3_CTRL_REG_MOD_EN				0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
+#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
 
 #define WLED3_CTRL_REG_FREQ				0x4c
 #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
@@ -161,9 +167,12 @@  struct wled {
 	u32 short_count;
 	const int *version;
 	int short_irq;
+	int ovp_irq;
 	bool force_mod_disable;
+	bool ovp_irq_disabled;
 
 	struct wled_config cfg;
+	struct delayed_work ovp_work;
 	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
 	int (*wled_sync_toggle)(struct wled *wled);
 };
@@ -209,6 +218,32 @@  static int wled4_set_brightness(struct wled *wled, u16 brightness)
 	return 0;
 }
 
+static void wled_ovp_work(struct work_struct *work)
+{
+	u32 val;
+	int rc;
+
+	struct wled *wled = container_of(work,
+					 struct wled, ovp_work.work);
+
+	rc = regmap_read(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
+			 &val);
+	if (rc < 0)
+		return;
+
+	if (val & WLED3_CTRL_REG_MOD_EN_BIT) {
+		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
+			enable_irq(wled->ovp_irq);
+			wled->ovp_irq_disabled = false;
+		}
+	} else {
+		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
+			disable_irq(wled->ovp_irq);
+			wled->ovp_irq_disabled = true;
+		}
+	}
+}
+
 static int wled_module_enable(struct wled *wled, int val)
 {
 	int rc;
@@ -220,7 +255,12 @@  static int wled_module_enable(struct wled *wled, int val)
 				WLED3_CTRL_REG_MOD_EN,
 				WLED3_CTRL_REG_MOD_EN_MASK,
 				WLED3_CTRL_REG_MOD_EN_MASK);
-	return rc;
+	if (rc < 0)
+		return rc;
+
+	schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
+
+	return 0;
 }
 
 static int wled3_sync_toggle(struct wled *wled)
@@ -346,6 +386,36 @@  static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
+{
+	struct wled *wled = _wled;
+	int rc;
+	u32 int_sts, fault_sts;
+
+	rc = regmap_read(wled->regmap,
+			 wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS, &int_sts);
+	if (rc < 0) {
+		dev_err(wled->dev, "Error in reading WLED3_INT_RT_STS rc=%d\n",
+			rc);
+		return IRQ_HANDLED;
+	}
+
+	rc = regmap_read(wled->regmap, wled->ctrl_addr +
+			 WLED3_CTRL_REG_FAULT_STATUS, &fault_sts);
+	if (rc < 0) {
+		dev_err(wled->dev, "Error in reading WLED_FAULT_STATUS rc=%d\n",
+			rc);
+		return IRQ_HANDLED;
+	}
+
+	if (fault_sts &
+		(WLED3_CTRL_REG_OVP_FAULT_BIT | WLED3_CTRL_REG_ILIM_FAULT_BIT))
+		dev_dbg(wled->dev, "WLED OVP fault detected, int_sts=%x fault_sts= %x\n",
+			int_sts, fault_sts);
+
+	return IRQ_HANDLED;
+}
+
 static int wled3_setup(struct wled *wled)
 {
 	u16 addr;
@@ -821,6 +891,44 @@  static int wled_configure_short_irq(struct wled *wled,
 	return rc;
 }
 
+static int wled_configure_ovp_irq(struct wled *wled,
+				  struct platform_device *pdev)
+{
+	int rc = 0;
+	u32 val;
+
+	if (*wled->version == WLED_PM8941)
+		return 0;
+
+	wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
+	if (wled->ovp_irq < 0) {
+		dev_dbg(&pdev->dev, "ovp irq is not used\n");
+		return 0;
+	}
+
+	rc = devm_request_threaded_irq(wled->dev, wled->ovp_irq, NULL,
+				       wled_ovp_irq_handler, IRQF_ONESHOT,
+				       "wled_ovp_irq", wled);
+	if (rc < 0) {
+		dev_err(wled->dev, "Unable to request ovp_irq (err:%d)\n",
+			rc);
+		return 0;
+	}
+
+	rc = regmap_read(wled->regmap, wled->ctrl_addr +
+			 WLED3_CTRL_REG_MOD_EN, &val);
+	if (rc < 0)
+		return rc;
+
+	/* Keep OVP irq disabled until module is enabled */
+	if (!rc && !(val & WLED3_CTRL_REG_MOD_EN_MASK)) {
+		disable_irq(wled->ovp_irq);
+		wled->ovp_irq_disabled = true;
+	}
+
+	return rc;
+}
+
 static const struct backlight_ops wled_ops = {
 	.update_status = wled_update_status,
 };
@@ -874,10 +982,16 @@  static int wled_probe(struct platform_device *pdev)
 		}
 	}
 
+	INIT_DELAYED_WORK(&wled->ovp_work, wled_ovp_work);
+
 	rc = wled_configure_short_irq(wled, pdev);
 	if (rc < 0)
 		return rc;
 
+	rc = wled_configure_ovp_irq(wled, pdev);
+	if (rc < 0)
+		return rc;
+
 	val = WLED_DEFAULT_BRIGHTNESS;
 	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);