[PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[Kiran Gunda]

pm8941-wled.c driver is supporting the WLED peripheral
on pm8941. Rename it to qcom-wled.c so that it can support
WLED on multiple PMICs.

Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
---
 .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 +-
 drivers/video/backlight/Kconfig                                   | 8 ++++----
 drivers/video/backlight/Makefile                                  | 2 +-
 drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
 4 files changed, 6 insertions(+), 6 deletions(-)
 rename Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt} (95%)
 rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
similarity index 95%
rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
index e5b294d..fb39e32 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
@@ -1,4 +1,4 @@
-Binding for Qualcomm PM8941 WLED driver
+Binding for Qualcomm Technologies, Inc. WLED driver
 
 Required properties:
 - compatible: should be "qcom,pm8941-wled"
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 4e1d2ad..8c095e3 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -299,12 +299,12 @@ config BACKLIGHT_TOSA
 	  If you have an Sharp SL-6000 Zaurus say Y to enable a driver
 	  for its backlight
 
-config BACKLIGHT_PM8941_WLED
-	tristate "Qualcomm PM8941 WLED Driver"
+config BACKLIGHT_QCOM_WLED
+	tristate "Qualcomm PMIC WLED Driver"
 	select REGMAP
 	help
-	  If you have the Qualcomm PM8941, say Y to enable a driver for the
-	  WLED block.
+	  If you have the Qualcomm PMIC, say Y to enable a driver for the
+	  WLED block. Currently it supports PM8941 and PMI8998.
 
 config BACKLIGHT_SAHARA
 	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 5e28f01..6fd76ef 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_OT200)		+= ot200_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
-obj-$(CONFIG_BACKLIGHT_PM8941_WLED)	+= pm8941-wled.o
 obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
 obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
 obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c
similarity index 100%
rename from drivers/video/backlight/pm8941-wled.c
rename to drivers/video/backlight/qcom-wled.c
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project

Re: [PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[Bjorn Andersson]

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:

> pm8941-wled.c driver is supporting the WLED peripheral
> on pm8941. Rename it to qcom-wled.c so that it can support
> WLED on multiple PMICs.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>

Please carry any tags acquired when reposting patches without changes
and please add a "Changes since v???:" list below the --- to expedite
the review process.


This is still 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 +-
>  drivers/video/backlight/Kconfig                                   | 8 ++++----
>  drivers/video/backlight/Makefile                                  | 2 +-
>  drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
>  4 files changed, 6 insertions(+), 6 deletions(-)
>  rename Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt} (95%)
>  rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> similarity index 95%
> rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> index e5b294d..fb39e32 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> @@ -1,4 +1,4 @@
> -Binding for Qualcomm PM8941 WLED driver
> +Binding for Qualcomm Technologies, Inc. WLED driver
>  
>  Required properties:
>  - compatible: should be "qcom,pm8941-wled"
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 4e1d2ad..8c095e3 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA
>  	  If you have an Sharp SL-6000 Zaurus say Y to enable a driver
>  	  for its backlight
>  
> -config BACKLIGHT_PM8941_WLED
> -	tristate "Qualcomm PM8941 WLED Driver"
> +config BACKLIGHT_QCOM_WLED
> +	tristate "Qualcomm PMIC WLED Driver"
>  	select REGMAP
>  	help
> -	  If you have the Qualcomm PM8941, say Y to enable a driver for the
> -	  WLED block.
> +	  If you have the Qualcomm PMIC, say Y to enable a driver for the
> +	  WLED block. Currently it supports PM8941 and PMI8998.
>  
>  config BACKLIGHT_SAHARA
>  	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 5e28f01..6fd76ef 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
>  obj-$(CONFIG_BACKLIGHT_OT200)		+= ot200_bl.o
>  obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
>  obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
> -obj-$(CONFIG_BACKLIGHT_PM8941_WLED)	+= pm8941-wled.o
>  obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
> +obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
>  obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
>  obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
> diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c
> similarity index 100%
> rename from drivers/video/backlight/pm8941-wled.c
> rename to drivers/video/backlight/qcom-wled.c
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>  a Linux Foundation Collaborative Project
> 

Re: [PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[kgunda]

On 2018-06-20 04:24, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> 
>> pm8941-wled.c driver is supporting the WLED peripheral
>> on pm8941. Rename it to qcom-wled.c so that it can support
>> WLED on multiple PMICs.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> 
> Please carry any tags acquired when reposting patches without changes
> and please add a "Changes since v???:" list below the --- to expedite
> the review process.
> 
> 
> This is still
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Regards,
> Bjorn
> 
Thanks Bjorn for reviewing. Sure I will keep the tags when re-posting.
I have mentioned the consolidated changes in the 0th patch. Anyways,
I will keep it for individual patches from next time.
>> ---
>>  .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 
>> +-
>>  drivers/video/backlight/Kconfig                                   | 8 
>> ++++----
>>  drivers/video/backlight/Makefile                                  | 2 
>> +-
>>  drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
>>  4 files changed, 6 insertions(+), 6 deletions(-)
>>  rename 
>> Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => 
>> qcom-wled.txt} (95%)
>>  rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt 
>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> similarity index 95%
>> rename from 
>> Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
>> rename to 
>> Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> index e5b294d..fb39e32 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> @@ -1,4 +1,4 @@
>> -Binding for Qualcomm PM8941 WLED driver
>> +Binding for Qualcomm Technologies, Inc. WLED driver
>> 
>>  Required properties:
>>  - compatible: should be "qcom,pm8941-wled"
>> diff --git a/drivers/video/backlight/Kconfig 
>> b/drivers/video/backlight/Kconfig
>> index 4e1d2ad..8c095e3 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
>> @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA
>>  	  If you have an Sharp SL-6000 Zaurus say Y to enable a driver
>>  	  for its backlight
>> 
>> -config BACKLIGHT_PM8941_WLED
>> -	tristate "Qualcomm PM8941 WLED Driver"
>> +config BACKLIGHT_QCOM_WLED
>> +	tristate "Qualcomm PMIC WLED Driver"
>>  	select REGMAP
>>  	help
>> -	  If you have the Qualcomm PM8941, say Y to enable a driver for the
>> -	  WLED block.
>> +	  If you have the Qualcomm PMIC, say Y to enable a driver for the
>> +	  WLED block. Currently it supports PM8941 and PMI8998.
>> 
>>  config BACKLIGHT_SAHARA
>>  	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
>> diff --git a/drivers/video/backlight/Makefile 
>> b/drivers/video/backlight/Makefile
>> index 5e28f01..6fd76ef 100644
>> --- a/drivers/video/backlight/Makefile
>> +++ b/drivers/video/backlight/Makefile
>> @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
>>  obj-$(CONFIG_BACKLIGHT_OT200)		+= ot200_bl.o
>>  obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
>>  obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
>> -obj-$(CONFIG_BACKLIGHT_PM8941_WLED)	+= pm8941-wled.o
>>  obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
>> +obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
>>  obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
>>  obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
>>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
>> diff --git a/drivers/video/backlight/pm8941-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> similarity index 100%
>> rename from drivers/video/backlight/pm8941-wled.c
>> rename to drivers/video/backlight/qcom-wled.c
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>>  a Linux Foundation Collaborative Project
>> 

Re: [PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[Rob Herring]

On Tue, Jun 19, 2018 at 04:43:36PM +0530, Kiran Gunda wrote:
> pm8941-wled.c driver is supporting the WLED peripheral
> on pm8941. Rename it to qcom-wled.c so that it can support
> WLED on multiple PMICs.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/video/backlight/Kconfig                                   | 8 ++++----
>  drivers/video/backlight/Makefile                                  | 2 +-
>  drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
>  4 files changed, 6 insertions(+), 6 deletions(-)
>  rename Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt} (95%)
>  rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)

Re: [PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[Daniel Thompson]

On Tue, Jun 19, 2018 at 04:43:36PM +0530, Kiran Gunda wrote:
> pm8941-wled.c driver is supporting the WLED peripheral
> on pm8941. Rename it to qcom-wled.c so that it can support
> WLED on multiple PMICs.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>

With changes proposed for v4:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

> ---
>  .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 +-
>  drivers/video/backlight/Kconfig                                   | 8 ++++----
>  drivers/video/backlight/Makefile                                  | 2 +-
>  drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
>  4 files changed, 6 insertions(+), 6 deletions(-)
>  rename Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt} (95%)
>  rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> similarity index 95%
> rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> index e5b294d..fb39e32 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> @@ -1,4 +1,4 @@
> -Binding for Qualcomm PM8941 WLED driver
> +Binding for Qualcomm Technologies, Inc. WLED driver
>  
>  Required properties:
>  - compatible: should be "qcom,pm8941-wled"
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 4e1d2ad..8c095e3 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA
>  	  If you have an Sharp SL-6000 Zaurus say Y to enable a driver
>  	  for its backlight
>  
> -config BACKLIGHT_PM8941_WLED
> -	tristate "Qualcomm PM8941 WLED Driver"
> +config BACKLIGHT_QCOM_WLED
> +	tristate "Qualcomm PMIC WLED Driver"
>  	select REGMAP
>  	help
> -	  If you have the Qualcomm PM8941, say Y to enable a driver for the
> -	  WLED block.
> +	  If you have the Qualcomm PMIC, say Y to enable a driver for the
> +	  WLED block. Currently it supports PM8941 and PMI8998.
>  
>  config BACKLIGHT_SAHARA
>  	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 5e28f01..6fd76ef 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
>  obj-$(CONFIG_BACKLIGHT_OT200)		+= ot200_bl.o
>  obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
>  obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
> -obj-$(CONFIG_BACKLIGHT_PM8941_WLED)	+= pm8941-wled.o
>  obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
> +obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
>  obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
>  obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
> diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c
> similarity index 100%
> rename from drivers/video/backlight/pm8941-wled.c
> rename to drivers/video/backlight/qcom-wled.c
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>  a Linux Foundation Collaborative Project
> 

Re: [PATCH V3 1/7] backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

[Daniel Thompson]

On Thu, Jun 21, 2018 at 02:05:59PM +0100, Daniel Thompson wrote:
> On Tue, Jun 19, 2018 at 04:43:36PM +0530, Kiran Gunda wrote:
> > pm8941-wled.c driver is supporting the WLED peripheral
> > on pm8941. Rename it to qcom-wled.c so that it can support
> > WLED on multiple PMICs.
> > 
> > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> 
> With changes proposed for v4:

Ignore that bit... misremembered the thread to date.

It's just a plain (no caveat):
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.


> 
> > ---
> >  .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}    | 2 +-
> >  drivers/video/backlight/Kconfig                                   | 8 ++++----
> >  drivers/video/backlight/Makefile                                  | 2 +-
> >  drivers/video/backlight/{pm8941-wled.c => qcom-wled.c}            | 0
> >  4 files changed, 6 insertions(+), 6 deletions(-)
> >  rename Documentation/devicetree/bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt} (95%)
> >  rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> > similarity index 95%
> > rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> > rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> > index e5b294d..fb39e32 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
> > +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> > @@ -1,4 +1,4 @@
> > -Binding for Qualcomm PM8941 WLED driver
> > +Binding for Qualcomm Technologies, Inc. WLED driver
> >  
> >  Required properties:
> >  - compatible: should be "qcom,pm8941-wled"
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index 4e1d2ad..8c095e3 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA
> >  	  If you have an Sharp SL-6000 Zaurus say Y to enable a driver
> >  	  for its backlight
> >  
> > -config BACKLIGHT_PM8941_WLED
> > -	tristate "Qualcomm PM8941 WLED Driver"
> > +config BACKLIGHT_QCOM_WLED
> > +	tristate "Qualcomm PMIC WLED Driver"
> >  	select REGMAP
> >  	help
> > -	  If you have the Qualcomm PM8941, say Y to enable a driver for the
> > -	  WLED block.
> > +	  If you have the Qualcomm PMIC, say Y to enable a driver for the
> > +	  WLED block. Currently it supports PM8941 and PMI8998.
> >  
> >  config BACKLIGHT_SAHARA
> >  	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
> > diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> > index 5e28f01..6fd76ef 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1)		+= omap1_bl.o
> >  obj-$(CONFIG_BACKLIGHT_OT200)		+= ot200_bl.o
> >  obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
> >  obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
> > -obj-$(CONFIG_BACKLIGHT_PM8941_WLED)	+= pm8941-wled.o
> >  obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
> > +obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
> >  obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
> >  obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
> >  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
> > diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c
> > similarity index 100%
> > rename from drivers/video/backlight/pm8941-wled.c
> > rename to drivers/video/backlight/qcom-wled.c
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >  a Linux Foundation Collaborative Project
> > 

[PATCH V3 4/7] backlight: qcom-wled: Rename PM8941* to WLED3

[Kiran Gunda]

Rename the PM8941* references as WLED3 to make the
driver generic and have WLED support for other PMICs.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 0b6d219..812f3cb 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -18,77 +18,79 @@
 #include <linux/regmap.h>
 
 /* From DT binding */
-#define PM8941_WLED_DEFAULT_BRIGHTNESS		2048
+#define WLED_DEFAULT_BRIGHTNESS				2048
 
-#define PM8941_WLED_REG_VAL_BASE		0x40
-#define  PM8941_WLED_REG_VAL_MAX		0xFFF
+#define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
+#define WLED3_CTRL_REG_VAL_BASE				0x40
 
-#define PM8941_WLED_REG_MOD_EN			0x46
-#define  PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
-#define  PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
+/* WLED3 control registers */
+#define WLED3_CTRL_REG_MOD_EN				0x46
+#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
+#define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
 
-#define PM8941_WLED_REG_SYNC			0x47
-#define  PM8941_WLED_REG_SYNC_MASK		0x07
-#define  PM8941_WLED_REG_SYNC_LED1		BIT(0)
-#define  PM8941_WLED_REG_SYNC_LED2		BIT(1)
-#define  PM8941_WLED_REG_SYNC_LED3		BIT(2)
-#define  PM8941_WLED_REG_SYNC_ALL		0x07
-#define  PM8941_WLED_REG_SYNC_CLEAR		0x00
+#define WLED3_CTRL_REG_FREQ				0x4c
+#define  WLED3_CTRL_REG_FREQ_MASK			0x0f
 
-#define PM8941_WLED_REG_FREQ			0x4c
-#define  PM8941_WLED_REG_FREQ_MASK		0x0f
+#define WLED3_CTRL_REG_OVP				0x4d
+#define  WLED3_CTRL_REG_OVP_MASK			0x03
 
-#define PM8941_WLED_REG_OVP			0x4d
-#define  PM8941_WLED_REG_OVP_MASK		0x03
+#define WLED3_CTRL_REG_ILIMIT				0x4e
+#define  WLED3_CTRL_REG_ILIMIT_MASK			0x07
 
-#define PM8941_WLED_REG_BOOST			0x4e
-#define  PM8941_WLED_REG_BOOST_MASK		0x07
+/* WLED3 sink registers */
+#define WLED3_SINK_REG_SYNC				0x47
+#define  WLED3_SINK_REG_SYNC_MASK			0x07
+#define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
+#define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
+#define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
+#define  WLED3_SINK_REG_SYNC_ALL			0x07
+#define  WLED3_SINK_REG_SYNC_CLEAR			0x00
 
-#define PM8941_WLED_REG_SINK			0x4f
-#define  PM8941_WLED_REG_SINK_MASK		0xe0
-#define  PM8941_WLED_REG_SINK_SHFT		0x05
+#define WLED3_SINK_REG_CURR_SINK			0x4f
+#define  WLED3_SINK_REG_CURR_SINK_MASK			0xe0
+#define  WLED3_SINK_REG_CURR_SINK_SHFT			0x05
 
-/* Per-'string' registers below */
-#define PM8941_WLED_REG_STR_OFFSET		0x10
+/* WLED3 per-'string' registers below */
+#define WLED3_SINK_REG_STR_OFFSET			0x10
 
-#define PM8941_WLED_REG_STR_MOD_EN_BASE		0x60
-#define  PM8941_WLED_REG_STR_MOD_MASK		BIT(7)
-#define  PM8941_WLED_REG_STR_MOD_EN		BIT(7)
+#define WLED3_SINK_REG_STR_MOD_EN_BASE			0x60
+#define  WLED3_SINK_REG_STR_MOD_MASK			BIT(7)
+#define  WLED3_SINK_REG_STR_MOD_EN			BIT(7)
 
-#define PM8941_WLED_REG_STR_SCALE_BASE		0x62
-#define  PM8941_WLED_REG_STR_SCALE_MASK		0x1f
+#define WLED3_SINK_REG_STR_FULL_SCALE_CURR		0x62
+#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK	0x1f
 
-#define PM8941_WLED_REG_STR_MOD_SRC_BASE	0x63
-#define  PM8941_WLED_REG_STR_MOD_SRC_MASK	0x01
-#define  PM8941_WLED_REG_STR_MOD_SRC_INT	0x00
-#define  PM8941_WLED_REG_STR_MOD_SRC_EXT	0x01
+#define WLED3_SINK_REG_STR_MOD_SRC_BASE			0x63
+#define  WLED3_SINK_REG_STR_MOD_SRC_MASK		0x01
+#define  WLED3_SINK_REG_STR_MOD_SRC_INT			0x00
+#define  WLED3_SINK_REG_STR_MOD_SRC_EXT			0x01
 
-#define PM8941_WLED_REG_STR_CABC_BASE		0x66
-#define  PM8941_WLED_REG_STR_CABC_MASK		BIT(7)
-#define  PM8941_WLED_REG_STR_CABC_EN		BIT(7)
+#define WLED3_SINK_REG_STR_CABC_BASE			0x66
+#define  WLED3_SINK_REG_STR_CABC_MASK			BIT(7)
+#define  WLED3_SINK_REG_STR_CABC_EN			BIT(7)
 
-struct pm8941_wled_config {
-	u32 i_boost_limit;
+struct wled_config {
+	u32 boost_i_limit;
 	u32 ovp;
 	u32 switch_freq;
 	u32 num_strings;
-	u32 i_limit;
+	u32 string_i_limit;
 	bool cs_out_en;
 	bool ext_gen;
 	bool cabc_en;
 };
 
-struct pm8941_wled {
+struct wled {
 	const char *name;
 	struct regmap *regmap;
 	u16 addr;
 
-	struct pm8941_wled_config cfg;
+	struct wled_config cfg;
 };
 
-static int pm8941_wled_update_status(struct backlight_device *bl)
+static int wled_update_status(struct backlight_device *bl)
 {
-	struct pm8941_wled *wled = bl_get_data(bl);
+	struct wled *wled = bl_get_data(bl);
 	u16 val = bl->props.brightness;
 	u8 ctrl = 0;
 	int rc;
@@ -100,11 +102,11 @@ static int pm8941_wled_update_status(struct backlight_device *bl)
 		val = 0;
 
 	if (val != 0)
-		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
+		ctrl = WLED3_CTRL_REG_MOD_EN_BIT;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_MOD_EN,
-			PM8941_WLED_REG_MOD_EN_MASK, ctrl);
+			wled->addr + WLED3_CTRL_REG_MOD_EN,
+			WLED3_CTRL_REG_MOD_EN_MASK, ctrl);
 	if (rc)
 		return rc;
 
@@ -112,89 +114,89 @@ static int pm8941_wled_update_status(struct backlight_device *bl)
 		u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
 
 		rc = regmap_bulk_write(wled->regmap,
-				wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
+				wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
 				v, 2);
 		if (rc)
 			return rc;
 	}
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_SYNC,
-			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL);
+			wled->addr + WLED3_SINK_REG_SYNC,
+			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
 	if (rc)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_SYNC,
-			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
+			wled->addr + WLED3_SINK_REG_SYNC,
+			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
 	return rc;
 }
 
-static int pm8941_wled_setup(struct pm8941_wled *wled)
+static int wled_setup(struct wled *wled)
 {
 	int rc;
 	int i;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_OVP,
-			PM8941_WLED_REG_OVP_MASK, wled->cfg.ovp);
+			wled->addr + WLED3_CTRL_REG_OVP,
+			WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
 	if (rc)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_BOOST,
-			PM8941_WLED_REG_BOOST_MASK, wled->cfg.i_boost_limit);
+			wled->addr + WLED3_CTRL_REG_ILIMIT,
+			WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
 	if (rc)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + PM8941_WLED_REG_FREQ,
-			PM8941_WLED_REG_FREQ_MASK, wled->cfg.switch_freq);
+			wled->addr + WLED3_CTRL_REG_FREQ,
+			WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
 	if (rc)
 		return rc;
 
 	if (wled->cfg.cs_out_en) {
 		u8 all = (BIT(wled->cfg.num_strings) - 1)
-				<< PM8941_WLED_REG_SINK_SHFT;
+				<< WLED3_SINK_REG_CURR_SINK_SHFT;
 
 		rc = regmap_update_bits(wled->regmap,
-				wled->addr + PM8941_WLED_REG_SINK,
-				PM8941_WLED_REG_SINK_MASK, all);
+				wled->addr + WLED3_SINK_REG_CURR_SINK,
+				WLED3_SINK_REG_CURR_SINK_MASK, all);
 		if (rc)
 			return rc;
 	}
 
 	for (i = 0; i < wled->cfg.num_strings; ++i) {
-		u16 addr = wled->addr + PM8941_WLED_REG_STR_OFFSET * i;
+		u16 addr = wled->addr + WLED3_SINK_REG_STR_OFFSET * i;
 
 		rc = regmap_update_bits(wled->regmap,
-				addr + PM8941_WLED_REG_STR_MOD_EN_BASE,
-				PM8941_WLED_REG_STR_MOD_MASK,
-				PM8941_WLED_REG_STR_MOD_EN);
+				addr + WLED3_SINK_REG_STR_MOD_EN_BASE,
+				WLED3_SINK_REG_STR_MOD_MASK,
+				WLED3_SINK_REG_STR_MOD_EN);
 		if (rc)
 			return rc;
 
 		if (wled->cfg.ext_gen) {
 			rc = regmap_update_bits(wled->regmap,
-					addr + PM8941_WLED_REG_STR_MOD_SRC_BASE,
-					PM8941_WLED_REG_STR_MOD_SRC_MASK,
-					PM8941_WLED_REG_STR_MOD_SRC_EXT);
+					addr + WLED3_SINK_REG_STR_MOD_SRC_BASE,
+					WLED3_SINK_REG_STR_MOD_SRC_MASK,
+					WLED3_SINK_REG_STR_MOD_SRC_EXT);
 			if (rc)
 				return rc;
 		}
 
 		rc = regmap_update_bits(wled->regmap,
-				addr + PM8941_WLED_REG_STR_SCALE_BASE,
-				PM8941_WLED_REG_STR_SCALE_MASK,
-				wled->cfg.i_limit);
+				addr + WLED3_SINK_REG_STR_FULL_SCALE_CURR,
+				WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK,
+				wled->cfg.string_i_limit);
 		if (rc)
 			return rc;
 
 		rc = regmap_update_bits(wled->regmap,
-				addr + PM8941_WLED_REG_STR_CABC_BASE,
-				PM8941_WLED_REG_STR_CABC_MASK,
+				addr + WLED3_SINK_REG_STR_CABC_BASE,
+				WLED3_SINK_REG_STR_CABC_MASK,
 				wled->cfg.cabc_en ?
-					PM8941_WLED_REG_STR_CABC_EN : 0);
+					WLED3_SINK_REG_STR_CABC_EN : 0);
 		if (rc)
 			return rc;
 	}
@@ -202,9 +204,9 @@ static int pm8941_wled_setup(struct pm8941_wled *wled)
 	return 0;
 }
 
-static const struct pm8941_wled_config pm8941_wled_config_defaults = {
-	.i_boost_limit = 3,
-	.i_limit = 20,
+static const struct wled_config wled3_config_defaults = {
+	.boost_i_limit = 3,
+	.string_i_limit = 20,
 	.ovp = 2,
 	.switch_freq = 5,
 	.num_strings = 0,
@@ -213,55 +215,55 @@ static int pm8941_wled_setup(struct pm8941_wled *wled)
 	.cabc_en = false,
 };
 
-struct pm8941_wled_var_cfg {
+struct wled_var_cfg {
 	const u32 *values;
 	u32 (*fn)(u32);
 	int size;
 };
 
-static const u32 pm8941_wled_i_boost_limit_values[] = {
+static const u32 wled3_boost_i_limit_values[] = {
 	105, 385, 525, 805, 980, 1260, 1400, 1680,
 };
 
-static const struct pm8941_wled_var_cfg pm8941_wled_i_boost_limit_cfg = {
-	.values = pm8941_wled_i_boost_limit_values,
-	.size = ARRAY_SIZE(pm8941_wled_i_boost_limit_values),
+static const struct wled_var_cfg wled3_boost_i_limit_cfg = {
+	.values = wled3_boost_i_limit_values,
+	.size = ARRAY_SIZE(wled3_boost_i_limit_values),
 };
 
-static const u32 pm8941_wled_ovp_values[] = {
+static const u32 wled3_ovp_values[] = {
 	35, 32, 29, 27,
 };
 
-static const struct pm8941_wled_var_cfg pm8941_wled_ovp_cfg = {
-	.values = pm8941_wled_ovp_values,
-	.size = ARRAY_SIZE(pm8941_wled_ovp_values),
+static const struct wled_var_cfg wled3_ovp_cfg = {
+	.values = wled3_ovp_values,
+	.size = ARRAY_SIZE(wled3_ovp_values),
 };
 
-static u32 pm8941_wled_num_strings_values_fn(u32 idx)
+static u32 wled3_num_strings_values_fn(u32 idx)
 {
 	return idx + 1;
 }
 
-static const struct pm8941_wled_var_cfg pm8941_wled_num_strings_cfg = {
-	.fn = pm8941_wled_num_strings_values_fn,
+static const struct wled_var_cfg wled3_num_strings_cfg = {
+	.fn = wled3_num_strings_values_fn,
 	.size = 3,
 };
 
-static u32 pm8941_wled_switch_freq_values_fn(u32 idx)
+static u32 wled3_switch_freq_values_fn(u32 idx)
 {
 	return 19200 / (2 * (1 + idx));
 }
 
-static const struct pm8941_wled_var_cfg pm8941_wled_switch_freq_cfg = {
-	.fn = pm8941_wled_switch_freq_values_fn,
+static const struct wled_var_cfg wled3_switch_freq_cfg = {
+	.fn = wled3_switch_freq_values_fn,
 	.size = 16,
 };
 
-static const struct pm8941_wled_var_cfg pm8941_wled_i_limit_cfg = {
+static const struct wled_var_cfg wled3_string_i_limit_cfg = {
 	.size = 26,
 };
 
-static u32 pm8941_wled_values(const struct pm8941_wled_var_cfg *cfg, u32 idx)
+static u32 wled3_values(const struct wled_var_cfg *cfg, u32 idx)
 {
 	if (idx >= cfg->size)
 		return UINT_MAX;
@@ -272,9 +274,9 @@ static u32 pm8941_wled_values(const struct pm8941_wled_var_cfg *cfg, u32 idx)
 	return idx;
 }
 
-static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
+static int wled_configure(struct wled *wled, struct device *dev)
 {
-	struct pm8941_wled_config *cfg = &wled->cfg;
+	struct wled_config *cfg = &wled->cfg;
 	u32 val;
 	int rc;
 	u32 c;
@@ -284,32 +286,32 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
 	const struct {
 		const char *name;
 		u32 *val_ptr;
-		const struct pm8941_wled_var_cfg *cfg;
+		const struct wled_var_cfg *cfg;
 	} u32_opts[] = {
 		{
 			"qcom,current-boost-limit",
-			&cfg->i_boost_limit,
-			.cfg = &pm8941_wled_i_boost_limit_cfg,
+			&cfg->boost_i_limit,
+			.cfg = &wled3_boost_i_limit_cfg,
 		},
 		{
 			"qcom,current-limit",
-			&cfg->i_limit,
-			.cfg = &pm8941_wled_i_limit_cfg,
+			&cfg->string_i_limit,
+			.cfg = &wled3_string_i_limit_cfg,
 		},
 		{
 			"qcom,ovp",
 			&cfg->ovp,
-			.cfg = &pm8941_wled_ovp_cfg,
+			.cfg = &wled3_ovp_cfg,
 		},
 		{
 			"qcom,switching-freq",
 			&cfg->switch_freq,
-			.cfg = &pm8941_wled_switch_freq_cfg,
+			.cfg = &wled3_switch_freq_cfg,
 		},
 		{
 			"qcom,num-strings",
 			&cfg->num_strings,
-			.cfg = &pm8941_wled_num_strings_cfg,
+			.cfg = &wled3_num_strings_cfg,
 		},
 	};
 	const struct {
@@ -332,7 +334,7 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
 	if (rc)
 		wled->name = dev->of_node->name;
 
-	*cfg = pm8941_wled_config_defaults;
+	*cfg = wled3_config_defaults;
 	for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
 		rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);
 		if (rc = -EINVAL) {
@@ -344,7 +346,7 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
 
 		c = UINT_MAX;
 		for (j = 0; c != val; j++) {
-			c = pm8941_wled_values(u32_opts[i].cfg, j);
+			c = wled3_values(u32_opts[i].cfg, j);
 			if (c = UINT_MAX) {
 				dev_err(dev, "invalid value for '%s'\n",
 					u32_opts[i].name);
@@ -366,15 +368,15 @@ static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
 	return 0;
 }
 
-static const struct backlight_ops pm8941_wled_ops = {
-	.update_status = pm8941_wled_update_status,
+static const struct backlight_ops wled_ops = {
+	.update_status = wled_update_status,
 };
 
-static int pm8941_wled_probe(struct platform_device *pdev)
+static int wled_probe(struct platform_device *pdev)
 {
 	struct backlight_properties props;
 	struct backlight_device *bl;
-	struct pm8941_wled *wled;
+	struct wled *wled;
 	struct regmap *regmap;
 	u32 val;
 	int rc;
@@ -391,42 +393,42 @@ static int pm8941_wled_probe(struct platform_device *pdev)
 
 	wled->regmap = regmap;
 
-	rc = pm8941_wled_configure(wled, &pdev->dev);
+	rc = wled_configure(wled, &pdev->dev);
 	if (rc)
 		return rc;
 
-	rc = pm8941_wled_setup(wled);
+	rc = wled_setup(wled);
 	if (rc)
 		return rc;
 
-	val = PM8941_WLED_DEFAULT_BRIGHTNESS;
+	val = WLED_DEFAULT_BRIGHTNESS;
 	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_RAW;
 	props.brightness = val;
-	props.max_brightness = PM8941_WLED_REG_VAL_MAX;
+	props.max_brightness = WLED3_SINK_REG_BRIGHT_MAX;
 	bl = devm_backlight_device_register(&pdev->dev, wled->name,
 					    &pdev->dev, wled,
-					    &pm8941_wled_ops, &props);
+					    &wled_ops, &props);
 	return PTR_ERR_OR_ZERO(bl);
 };
 
-static const struct of_device_id pm8941_wled_match_table[] = {
+static const struct of_device_id wled_match_table[] = {
 	{ .compatible = "qcom,pm8941-wled" },
 	{}
 };
-MODULE_DEVICE_TABLE(of, pm8941_wled_match_table);
+MODULE_DEVICE_TABLE(of, wled_match_table);
 
-static struct platform_driver pm8941_wled_driver = {
-	.probe = pm8941_wled_probe,
+static struct platform_driver wled_driver = {
+	.probe = wled_probe,
 	.driver	= {
-		.name = "pm8941-wled",
-		.of_match_table	= pm8941_wled_match_table,
+		.name = "qcom,wled",
+		.of_match_table	= wled_match_table,
 	},
 };
 
-module_platform_driver(pm8941_wled_driver);
+module_platform_driver(wled_driver);
 
-MODULE_DESCRIPTION("pm8941 wled driver");
+MODULE_DESCRIPTION("qcom wled driver");
 MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project

Re: [PATCH V3 4/7] backlight: qcom-wled: Rename PM8941* to WLED3

[Bjorn Andersson]

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:

> Rename the PM8941* references as WLED3 to make the
> driver generic and have WLED support for other PMICs.
> 

Looks good, just three minor comments below.

> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 248 ++++++++++++++++++------------------
>  1 file changed, 125 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 0b6d219..812f3cb 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -18,77 +18,79 @@
>  #include <linux/regmap.h>
>  
>  /* From DT binding */
> -#define PM8941_WLED_DEFAULT_BRIGHTNESS		2048
> +#define WLED_DEFAULT_BRIGHTNESS				2048
>  
> -#define PM8941_WLED_REG_VAL_BASE		0x40
> -#define  PM8941_WLED_REG_VAL_MAX		0xFFF
> +#define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
> +#define WLED3_CTRL_REG_VAL_BASE				0x40
>  
> -#define PM8941_WLED_REG_MOD_EN			0x46
> -#define  PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
> -#define  PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
> +/* WLED3 control registers */
> +#define WLED3_CTRL_REG_MOD_EN				0x46
> +#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
> +#define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>  
> -#define PM8941_WLED_REG_SYNC			0x47

These are in address order, any reason why WLED3_SINK_REG_SYNC moved
down?

> -#define  PM8941_WLED_REG_SYNC_MASK		0x07
> -#define  PM8941_WLED_REG_SYNC_LED1		BIT(0)
> -#define  PM8941_WLED_REG_SYNC_LED2		BIT(1)
> -#define  PM8941_WLED_REG_SYNC_LED3		BIT(2)
> -#define  PM8941_WLED_REG_SYNC_ALL		0x07
> -#define  PM8941_WLED_REG_SYNC_CLEAR		0x00
> +#define WLED3_CTRL_REG_FREQ				0x4c
> +#define  WLED3_CTRL_REG_FREQ_MASK			0x0f
>  
> -#define PM8941_WLED_REG_FREQ			0x4c
> -#define  PM8941_WLED_REG_FREQ_MASK		0x0f
> +#define WLED3_CTRL_REG_OVP				0x4d
> +#define  WLED3_CTRL_REG_OVP_MASK			0x03
>  
> -#define PM8941_WLED_REG_OVP			0x4d
> -#define  PM8941_WLED_REG_OVP_MASK		0x03
> +#define WLED3_CTRL_REG_ILIMIT				0x4e
> +#define  WLED3_CTRL_REG_ILIMIT_MASK			0x07
>  
> -#define PM8941_WLED_REG_BOOST			0x4e
> -#define  PM8941_WLED_REG_BOOST_MASK		0x07
> +/* WLED3 sink registers */
> +#define WLED3_SINK_REG_SYNC				0x47
> +#define  WLED3_SINK_REG_SYNC_MASK			0x07
> +#define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
> +#define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
> +#define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
> +#define  WLED3_SINK_REG_SYNC_ALL			0x07
> +#define  WLED3_SINK_REG_SYNC_CLEAR			0x00
>  
[..]
> -struct pm8941_wled_config {
> -	u32 i_boost_limit;
> +struct wled_config {
> +	u32 boost_i_limit;
>  	u32 ovp;
>  	u32 switch_freq;
>  	u32 num_strings;
> -	u32 i_limit;
> +	u32 string_i_limit;

Changing the members in this struct seems unrelated.

>  	bool cs_out_en;
>  	bool ext_gen;
>  	bool cabc_en;
>  };
>  
[..]
> -MODULE_DESCRIPTION("pm8941 wled driver");
> +MODULE_DESCRIPTION("qcom wled driver");

I would prefer if this was "Qualcomm WLED driver".

>  MODULE_LICENSE("GPL v2");

Regards,
Bjorn

Re: [PATCH V3 4/7] backlight: qcom-wled: Rename PM8941* to WLED3

[kgunda]

On 2018-06-20 04:41, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> 
>> Rename the PM8941* references as WLED3 to make the
>> driver generic and have WLED support for other PMICs.
>> 
> 
> Looks good, just three minor comments below.
> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/video/backlight/qcom-wled.c | 248 
>> ++++++++++++++++++------------------
>>  1 file changed, 125 insertions(+), 123 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index 0b6d219..812f3cb 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -18,77 +18,79 @@
>>  #include <linux/regmap.h>
>> 
>>  /* From DT binding */
>> -#define PM8941_WLED_DEFAULT_BRIGHTNESS		2048
>> +#define WLED_DEFAULT_BRIGHTNESS				2048
>> 
>> -#define PM8941_WLED_REG_VAL_BASE		0x40
>> -#define  PM8941_WLED_REG_VAL_MAX		0xFFF
>> +#define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>> +#define WLED3_CTRL_REG_VAL_BASE				0x40
>> 
>> -#define PM8941_WLED_REG_MOD_EN			0x46
>> -#define  PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
>> -#define  PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
>> +/* WLED3 control registers */
>> +#define WLED3_CTRL_REG_MOD_EN				0x46
>> +#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
>> +#define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>> 
>> -#define PM8941_WLED_REG_SYNC			0x47
> 
> These are in address order, any reason why WLED3_SINK_REG_SYNC moved
> down?
> 
Actually, this is a sink specific register. That's why moved it to
under the /* WLED3 sink specific registers */
>> -#define  PM8941_WLED_REG_SYNC_MASK		0x07
>> -#define  PM8941_WLED_REG_SYNC_LED1		BIT(0)
>> -#define  PM8941_WLED_REG_SYNC_LED2		BIT(1)
>> -#define  PM8941_WLED_REG_SYNC_LED3		BIT(2)
>> -#define  PM8941_WLED_REG_SYNC_ALL		0x07
>> -#define  PM8941_WLED_REG_SYNC_CLEAR		0x00
>> +#define WLED3_CTRL_REG_FREQ				0x4c
>> +#define  WLED3_CTRL_REG_FREQ_MASK			0x0f
>> 
>> -#define PM8941_WLED_REG_FREQ			0x4c
>> -#define  PM8941_WLED_REG_FREQ_MASK		0x0f
>> +#define WLED3_CTRL_REG_OVP				0x4d
>> +#define  WLED3_CTRL_REG_OVP_MASK			0x03
>> 
>> -#define PM8941_WLED_REG_OVP			0x4d
>> -#define  PM8941_WLED_REG_OVP_MASK		0x03
>> +#define WLED3_CTRL_REG_ILIMIT				0x4e
>> +#define  WLED3_CTRL_REG_ILIMIT_MASK			0x07
>> 
>> -#define PM8941_WLED_REG_BOOST			0x4e
>> -#define  PM8941_WLED_REG_BOOST_MASK		0x07
>> +/* WLED3 sink registers */
>> +#define WLED3_SINK_REG_SYNC				0x47
>> +#define  WLED3_SINK_REG_SYNC_MASK			0x07
>> +#define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
>> +#define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
>> +#define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
>> +#define  WLED3_SINK_REG_SYNC_ALL			0x07
>> +#define  WLED3_SINK_REG_SYNC_CLEAR			0x00
>> 
> [..]
>> -struct pm8941_wled_config {
>> -	u32 i_boost_limit;
>> +struct wled_config {
>> +	u32 boost_i_limit;
>>  	u32 ovp;
>>  	u32 switch_freq;
>>  	u32 num_strings;
>> -	u32 i_limit;
>> +	u32 string_i_limit;
> 
> Changing the members in this struct seems unrelated.
> 
Changed the members, to have resemblance with the IPCAT and
better readability .
>>  	bool cs_out_en;
>>  	bool ext_gen;
>>  	bool cabc_en;
>>  };
>> 
> [..]
>> -MODULE_DESCRIPTION("pm8941 wled driver");
>> +MODULE_DESCRIPTION("qcom wled driver");
> 
> I would prefer if this was "Qualcomm WLED driver".
> 
Sure. I will change it in the next series.
>>  MODULE_LICENSE("GPL v2");
> 
> Regards,
> Bjorn

[PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral

[Kiran Gunda]

WLED4 peripheral is present on some PMICs like pmi8998 and
pm660l. It has a different register map and configurations
are also different. Add support for it.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 812f3cb..0bc7fcd 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -15,59 +15,112 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_address.h>
 #include <linux/regmap.h>
 
 /* From DT binding */
+#define WLED_MAX_STRINGS				4
+
 #define WLED_DEFAULT_BRIGHTNESS				2048
 
 #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
-#define WLED3_CTRL_REG_VAL_BASE				0x40
 
 /* WLED3 control registers */
 #define WLED3_CTRL_REG_MOD_EN				0x46
-#define  WLED3_CTRL_REG_MOD_EN_BIT			BIT(7)
 #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
 
 #define WLED3_CTRL_REG_FREQ				0x4c
-#define  WLED3_CTRL_REG_FREQ_MASK			0x0f
+#define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
 
 #define WLED3_CTRL_REG_OVP				0x4d
-#define  WLED3_CTRL_REG_OVP_MASK			0x03
+#define  WLED3_CTRL_REG_OVP_MASK			GENMASK(1, 0)
 
 #define WLED3_CTRL_REG_ILIMIT				0x4e
-#define  WLED3_CTRL_REG_ILIMIT_MASK			0x07
+#define  WLED3_CTRL_REG_ILIMIT_MASK			GENMASK(2, 0)
 
 /* WLED3 sink registers */
 #define WLED3_SINK_REG_SYNC				0x47
-#define  WLED3_SINK_REG_SYNC_MASK			0x07
+#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
+#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
 #define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
 #define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
 #define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
+#define  WLED4_SINK_REG_SYNC_LED4			BIT(3)
 #define  WLED3_SINK_REG_SYNC_ALL			0x07
+#define  WLED4_SINK_REG_SYNC_ALL			0x0f
 #define  WLED3_SINK_REG_SYNC_CLEAR			0x00
 
 #define WLED3_SINK_REG_CURR_SINK			0x4f
-#define  WLED3_SINK_REG_CURR_SINK_MASK			0xe0
-#define  WLED3_SINK_REG_CURR_SINK_SHFT			0x05
+#define  WLED3_SINK_REG_CURR_SINK_MASK			GENMASK(7, 5)
+#define  WLED3_SINK_REG_CURR_SINK_SHFT			5
 
 /* WLED3 per-'string' registers below */
-#define WLED3_SINK_REG_STR_OFFSET			0x10
+#define WLED3_SINK_REG_BRIGHT(n)			(0x40 + n)
 
-#define WLED3_SINK_REG_STR_MOD_EN_BASE			0x60
+#define WLED3_SINK_REG_STR_MOD_EN(n)			(0x60 + (n * 0x10))
 #define  WLED3_SINK_REG_STR_MOD_MASK			BIT(7)
-#define  WLED3_SINK_REG_STR_MOD_EN			BIT(7)
 
-#define WLED3_SINK_REG_STR_FULL_SCALE_CURR		0x62
-#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK	0x1f
+#define WLED3_SINK_REG_STR_FULL_SCALE_CURR(n)		(0x62 + (n * 0x10))
+#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK	GENMASK(4, 0)
 
-#define WLED3_SINK_REG_STR_MOD_SRC_BASE			0x63
-#define  WLED3_SINK_REG_STR_MOD_SRC_MASK		0x01
+#define WLED3_SINK_REG_STR_MOD_SRC(n)			(0x63 + (n * 0x10))
+#define  WLED3_SINK_REG_STR_MOD_SRC_MASK		BIT(0)
 #define  WLED3_SINK_REG_STR_MOD_SRC_INT			0x00
 #define  WLED3_SINK_REG_STR_MOD_SRC_EXT			0x01
 
-#define WLED3_SINK_REG_STR_CABC_BASE			0x66
+#define WLED3_SINK_REG_STR_CABC(n)			(0x66 + (n * 0x10))
 #define  WLED3_SINK_REG_STR_CABC_MASK			BIT(7)
-#define  WLED3_SINK_REG_STR_CABC_EN			BIT(7)
+
+/* WLED4 sink registers */
+#define WLED4_SINK_REG_CURR_SINK			0x46
+#define  WLED4_SINK_REG_CURR_SINK_MASK			GENMASK(7, 4)
+#define  WLED4_SINK_REG_CURR_SINK_SHFT			4
+
+/* WLED4 per-'string' registers below */
+#define WLED4_SINK_REG_STR_MOD_EN(n)			(0x50 + (n * 0x10))
+#define  WLED4_SINK_REG_STR_MOD_MASK			BIT(7)
+
+#define WLED4_SINK_REG_STR_FULL_SCALE_CURR(n)		(0x52 + (n * 0x10))
+#define  WLED4_SINK_REG_STR_FULL_SCALE_CURR_MASK	GENMASK(3, 0)
+
+#define WLED4_SINK_REG_STR_MOD_SRC(n)			(0x53 + (n * 0x10))
+#define  WLED4_SINK_REG_STR_MOD_SRC_MASK		BIT(0)
+#define  WLED4_SINK_REG_STR_MOD_SRC_INT			0x00
+#define  WLED4_SINK_REG_STR_MOD_SRC_EXT			0x01
+
+#define WLED4_SINK_REG_STR_CABC(n)			(0x56 + (n * 0x10))
+#define  WLED4_SINK_REG_STR_CABC_MASK			BIT(7)
+
+#define WLED4_SINK_REG_BRIGHT(n)			(0x57 + (n * 0x10))
+
+enum wled_version {
+	WLED_PM8941 = 3,
+	WLED_PMI8998,
+	WLED_PM660L,
+};
+
+static const int version_table[] = {
+	[0] = WLED_PM8941,
+	[1] = WLED_PMI8998,
+	[2] = WLED_PM660L,
+};
+
+struct wled_var_cfg {
+	const u32 *values;
+	u32 (*fn)(u32);
+	int size;
+};
+
+struct wled_u32_opts {
+	const char *name;
+	u32 *val_ptr;
+	const struct wled_var_cfg *cfg;
+};
+
+struct wled_bool_opts {
+	const char *name;
+	bool *val_ptr;
+};
 
 struct wled_config {
 	u32 boost_i_limit;
@@ -75,132 +128,224 @@ struct wled_config {
 	u32 switch_freq;
 	u32 num_strings;
 	u32 string_i_limit;
+	u32 enabled_strings[WLED_MAX_STRINGS];
 	bool cs_out_en;
 	bool ext_gen;
-	bool cabc_en;
+	bool cabc;
 };
 
 struct wled {
 	const char *name;
+	struct device *dev;
 	struct regmap *regmap;
-	u16 addr;
+	u16 ctrl_addr;
+	u16 sink_addr;
+	u32 brightness;
+	u32 max_brightness;
+	const int *version;
 
 	struct wled_config cfg;
+	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+	int (*wled_sync_toggle)(struct wled *wled);
 };
 
-static int wled_update_status(struct backlight_device *bl)
+static int wled3_set_brightness(struct wled *wled, u16 brightness)
 {
-	struct wled *wled = bl_get_data(bl);
-	u16 val = bl->props.brightness;
-	u8 ctrl = 0;
-	int rc;
-	int i;
+	int rc, i;
+	u8 v[2];
 
-	if (bl->props.power != FB_BLANK_UNBLANK ||
-	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
-	    bl->props.state & BL_CORE_FBBLANK)
-		val = 0;
+	v[0] = brightness & 0xff;
+	v[1] = (brightness >> 8) & 0xf;
 
-	if (val != 0)
-		ctrl = WLED3_CTRL_REG_MOD_EN_BIT;
+	for (i = 0;  i < wled->cfg.num_strings; ++i) {
+		rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr +
+				       WLED3_SINK_REG_BRIGHT(i), v, 2);
+		if (rc < 0)
+			return rc;
+	}
 
-	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_CTRL_REG_MOD_EN,
-			WLED3_CTRL_REG_MOD_EN_MASK, ctrl);
-	if (rc)
-		return rc;
+	return 0;
+}
 
-	for (i = 0; i < wled->cfg.num_strings; ++i) {
-		u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
+static int wled4_set_brightness(struct wled *wled, u16 brightness)
+{
+	int rc, i;
+	u16 low_limit = wled->max_brightness * 4 / 1000;
+	u8 v[2];
 
-		rc = regmap_bulk_write(wled->regmap,
-				wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
-				v, 2);
-		if (rc)
+	/* WLED4's lower limit of operation is 0.4% */
+	if (brightness > 0 && brightness < low_limit)
+		brightness = low_limit;
+
+	v[0] = brightness & 0xff;
+	v[1] = (brightness >> 8) & 0xf;
+
+	for (i = 0;  i < wled->cfg.num_strings; ++i) {
+		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
+				       WLED4_SINK_REG_BRIGHT(i), v, 2);
+		if (rc < 0)
 			return rc;
 	}
 
+	return 0;
+}
+
+static int wled_module_enable(struct wled *wled, int val)
+{
+	int rc;
+
+	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+				WLED3_CTRL_REG_MOD_EN,
+				WLED3_CTRL_REG_MOD_EN_MASK,
+				WLED3_CTRL_REG_MOD_EN_MASK);
+	return rc;
+}
+
+static int wled3_sync_toggle(struct wled *wled)
+{
+	int rc;
+
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_SINK_REG_SYNC,
-			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
-	if (rc)
+				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
+				WLED3_SINK_REG_SYNC_MASK,
+				WLED3_SINK_REG_SYNC_MASK);
+	if (rc < 0)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_SINK_REG_SYNC,
-			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
+				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
+				WLED3_SINK_REG_SYNC_MASK,
+				WLED3_SINK_REG_SYNC_CLEAR);
+
 	return rc;
 }
 
-static int wled_setup(struct wled *wled)
+static int wled4_sync_toggle(struct wled *wled)
 {
 	int rc;
-	int i;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_CTRL_REG_OVP,
-			WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
-	if (rc)
+				wled->sink_addr + WLED3_SINK_REG_SYNC,
+				WLED4_SINK_REG_SYNC_MASK,
+				WLED4_SINK_REG_SYNC_MASK);
+	if (rc < 0)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_CTRL_REG_ILIMIT,
-			WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
+				wled->sink_addr + WLED3_SINK_REG_SYNC,
+				WLED4_SINK_REG_SYNC_MASK,
+				WLED3_SINK_REG_SYNC_CLEAR);
+
+	return rc;
+}
+
+static int wled_update_status(struct backlight_device *bl)
+{
+	struct wled *wled = bl_get_data(bl);
+	u16 brightness = bl->props.brightness;
+	int rc = 0;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	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;
+		}
+
+		rc = wled->wled_sync_toggle(wled);
+		if (rc < 0) {
+			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
+			return rc;
+		}
+	}
+
+	if (!!brightness != !!wled->brightness) {
+		rc = wled_module_enable(wled, !!brightness);
+		if (rc < 0) {
+			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
+			return rc;
+		}
+	}
+
+	wled->brightness = brightness;
+
+	return rc;
+}
+
+static int wled3_setup(struct wled *wled)
+{
+	u16 addr;
+	u8 sink_en = 0;
+	int rc, i, j;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_OVP,
+				WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
 	if (rc)
 		return rc;
 
 	rc = regmap_update_bits(wled->regmap,
-			wled->addr + WLED3_CTRL_REG_FREQ,
-			WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
+				wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
+				WLED3_CTRL_REG_ILIMIT_MASK,
+				wled->cfg.boost_i_limit);
 	if (rc)
 		return rc;
 
-	if (wled->cfg.cs_out_en) {
-		u8 all = (BIT(wled->cfg.num_strings) - 1)
-				<< WLED3_SINK_REG_CURR_SINK_SHFT;
-
-		rc = regmap_update_bits(wled->regmap,
-				wled->addr + WLED3_SINK_REG_CURR_SINK,
-				WLED3_SINK_REG_CURR_SINK_MASK, all);
-		if (rc)
-			return rc;
-	}
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_FREQ,
+				WLED3_CTRL_REG_FREQ_MASK,
+				wled->cfg.switch_freq);
+	if (rc)
+		return rc;
 
 	for (i = 0; i < wled->cfg.num_strings; ++i) {
-		u16 addr = wled->addr + WLED3_SINK_REG_STR_OFFSET * i;
-
-		rc = regmap_update_bits(wled->regmap,
-				addr + WLED3_SINK_REG_STR_MOD_EN_BASE,
-				WLED3_SINK_REG_STR_MOD_MASK,
-				WLED3_SINK_REG_STR_MOD_EN);
+		j = wled->cfg.enabled_strings[i];
+		addr = wled->ctrl_addr + WLED3_SINK_REG_STR_MOD_EN(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED3_SINK_REG_STR_MOD_MASK,
+					WLED3_SINK_REG_STR_MOD_MASK);
 		if (rc)
 			return rc;
 
 		if (wled->cfg.ext_gen) {
-			rc = regmap_update_bits(wled->regmap,
-					addr + WLED3_SINK_REG_STR_MOD_SRC_BASE,
-					WLED3_SINK_REG_STR_MOD_SRC_MASK,
-					WLED3_SINK_REG_STR_MOD_SRC_EXT);
+			addr = wled->ctrl_addr + WLED3_SINK_REG_STR_MOD_SRC(j);
+			rc = regmap_update_bits(wled->regmap, addr,
+						WLED3_SINK_REG_STR_MOD_SRC_MASK,
+						WLED3_SINK_REG_STR_MOD_SRC_EXT);
 			if (rc)
 				return rc;
 		}
 
-		rc = regmap_update_bits(wled->regmap,
-				addr + WLED3_SINK_REG_STR_FULL_SCALE_CURR,
-				WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK,
-				wled->cfg.string_i_limit);
+		addr = wled->ctrl_addr + WLED3_SINK_REG_STR_FULL_SCALE_CURR(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK,
+					wled->cfg.string_i_limit);
 		if (rc)
 			return rc;
 
-		rc = regmap_update_bits(wled->regmap,
-				addr + WLED3_SINK_REG_STR_CABC_BASE,
-				WLED3_SINK_REG_STR_CABC_MASK,
-				wled->cfg.cabc_en ?
-					WLED3_SINK_REG_STR_CABC_EN : 0);
+		addr = wled->ctrl_addr + WLED3_SINK_REG_STR_CABC(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED3_SINK_REG_STR_CABC_MASK,
+					wled->cfg.cabc ?
+					WLED3_SINK_REG_STR_CABC_MASK : 0);
 		if (rc)
 			return rc;
+
+		sink_en |= BIT(j + WLED3_SINK_REG_CURR_SINK_SHFT);
 	}
 
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_SINK_REG_CURR_SINK,
+				WLED3_SINK_REG_CURR_SINK_MASK, sink_en);
+	if (rc)
+		return rc;
+
 	return 0;
 }
 
@@ -208,17 +353,125 @@ static int wled_setup(struct wled *wled)
 	.boost_i_limit = 3,
 	.string_i_limit = 20,
 	.ovp = 2,
+	.num_strings = 3,
 	.switch_freq = 5,
-	.num_strings = 0,
 	.cs_out_en = false,
 	.ext_gen = false,
-	.cabc_en = false,
+	.cabc = false,
 };
 
-struct wled_var_cfg {
-	const u32 *values;
-	u32 (*fn)(u32);
-	int size;
+static int wled4_setup(struct wled *wled)
+{
+	int rc, temp, i, j;
+	u16 addr;
+	u8 sink_en = 0;
+	u32 sink_cfg = 0;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_OVP,
+				WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
+				WLED3_CTRL_REG_ILIMIT_MASK,
+				wled->cfg.boost_i_limit);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_FREQ,
+				WLED3_CTRL_REG_FREQ_MASK,
+				wled->cfg.switch_freq);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_read(wled->regmap, wled->sink_addr +
+			 WLED4_SINK_REG_CURR_SINK, &sink_cfg);
+	if (rc < 0)
+		return rc;
+
+	for (i = 0; i < wled->cfg.num_strings; i++) {
+		j = wled->cfg.enabled_strings[i];
+		temp = j + WLED4_SINK_REG_CURR_SINK_SHFT;
+		sink_en |= 1 << temp;
+	}
+
+	if (sink_cfg = sink_en)
+		return 0;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
+				WLED4_SINK_REG_CURR_SINK_MASK, 0);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+				WLED3_CTRL_REG_MOD_EN,
+				WLED3_CTRL_REG_MOD_EN_MASK, 0);
+	if (rc < 0)
+		return rc;
+
+	/* Per sink/string configuration */
+	for (i = 0; i < wled->cfg.num_strings; i++) {
+		j = wled->cfg.enabled_strings[i];
+
+		addr = wled->sink_addr +
+				WLED4_SINK_REG_STR_MOD_EN(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED4_SINK_REG_STR_MOD_MASK,
+					WLED4_SINK_REG_STR_MOD_MASK);
+		if (rc < 0)
+			return rc;
+
+		addr = wled->sink_addr +
+				WLED4_SINK_REG_STR_FULL_SCALE_CURR(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED4_SINK_REG_STR_FULL_SCALE_CURR_MASK,
+					wled->cfg.string_i_limit);
+		if (rc < 0)
+			return rc;
+
+		addr = wled->sink_addr +
+				WLED4_SINK_REG_STR_CABC(j);
+		rc = regmap_update_bits(wled->regmap, addr,
+					WLED4_SINK_REG_STR_CABC_MASK,
+					wled->cfg.cabc ?
+					WLED4_SINK_REG_STR_CABC_MASK : 0);
+		if (rc < 0)
+			return rc;
+	}
+
+	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+				WLED3_CTRL_REG_MOD_EN,
+				WLED3_CTRL_REG_MOD_EN_MASK,
+				WLED3_CTRL_REG_MOD_EN_MASK);
+	if (rc < 0)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
+				WLED4_SINK_REG_CURR_SINK_MASK, sink_en);
+	if (rc < 0)
+		return rc;
+
+	rc = wled->wled_sync_toggle(wled);
+	if (rc < 0) {
+		dev_err(wled->dev, "Failed to toggle sync reg rc:%d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static const struct wled_config wled4_config_defaults = {
+	.boost_i_limit = 4,
+	.string_i_limit = 10,
+	.ovp = 1,
+	.num_strings = 4,
+	.switch_freq = 11,
+	.cabc = false,
 };
 
 static const u32 wled3_boost_i_limit_values[] = {
@@ -230,6 +483,15 @@ struct wled_var_cfg {
 	.size = ARRAY_SIZE(wled3_boost_i_limit_values),
 };
 
+static const u32 wled4_boost_i_limit_values[] = {
+	105, 280, 450, 620, 970, 1150, 1300, 1500,
+};
+
+static const struct wled_var_cfg wled4_boost_i_limit_cfg = {
+	.values = wled4_boost_i_limit_values,
+	.size = ARRAY_SIZE(wled4_boost_i_limit_values),
+};
+
 static const u32 wled3_ovp_values[] = {
 	35, 32, 29, 27,
 };
@@ -239,6 +501,15 @@ struct wled_var_cfg {
 	.size = ARRAY_SIZE(wled3_ovp_values),
 };
 
+static const u32 wled4_ovp_values[] = {
+	31100, 29600, 19600, 18100,
+};
+
+static const struct wled_var_cfg wled4_ovp_cfg = {
+	.values = wled4_ovp_values,
+	.size = ARRAY_SIZE(wled4_ovp_values),
+};
+
 static u32 wled3_num_strings_values_fn(u32 idx)
 {
 	return idx + 1;
@@ -249,6 +520,11 @@ static u32 wled3_num_strings_values_fn(u32 idx)
 	.size = 3,
 };
 
+static const struct wled_var_cfg wled4_num_strings_cfg = {
+	.fn = wled3_num_strings_values_fn,
+	.size = 4,
+};
+
 static u32 wled3_switch_freq_values_fn(u32 idx)
 {
 	return 19200 / (2 * (1 + idx));
@@ -263,7 +539,25 @@ static u32 wled3_switch_freq_values_fn(u32 idx)
 	.size = 26,
 };
 
-static u32 wled3_values(const struct wled_var_cfg *cfg, u32 idx)
+static const u32 wled4_string_i_limit_values[] = {
+	0, 2500, 5000, 7500, 10000, 12500, 15000, 17500, 20000,
+	22500, 25000, 27500, 30000,
+};
+
+static const struct wled_var_cfg wled4_string_i_limit_cfg = {
+	.values = wled4_string_i_limit_values,
+	.size = ARRAY_SIZE(wled4_string_i_limit_values),
+};
+
+static const struct wled_var_cfg wled3_string_cfg = {
+	.size = 8,
+};
+
+static const struct wled_var_cfg wled4_string_cfg = {
+	.size = 16,
+};
+
+static u32 wled_values(const struct wled_var_cfg *cfg, u32 idx)
 {
 	if (idx >= cfg->size)
 		return UINT_MAX;
@@ -274,68 +568,113 @@ static u32 wled3_values(const struct wled_var_cfg *cfg, u32 idx)
 	return idx;
 }
 
-static int wled_configure(struct wled *wled, struct device *dev)
+static int wled_configure(struct wled *wled)
 {
 	struct wled_config *cfg = &wled->cfg;
-	u32 val;
-	int rc;
-	u32 c;
-	int i;
-	int j;
-
-	const struct {
-		const char *name;
-		u32 *val_ptr;
-		const struct wled_var_cfg *cfg;
-	} u32_opts[] = {
+	struct device *dev = wled->dev;
+	const __be32 *prop_addr;
+	u32 size, val, c, string_len;
+	int rc, i, j;
+
+	const struct wled_u32_opts *u32_opts = NULL;
+	const struct wled_u32_opts wled3_opts[] = {
 		{
-			"qcom,current-boost-limit",
-			&cfg->boost_i_limit,
+			.name = "qcom,current-boost-limit",
+			.val_ptr = &cfg->boost_i_limit,
 			.cfg = &wled3_boost_i_limit_cfg,
 		},
 		{
-			"qcom,current-limit",
-			&cfg->string_i_limit,
+			.name = "qcom,current-limit",
+			.val_ptr = &cfg->string_i_limit,
 			.cfg = &wled3_string_i_limit_cfg,
 		},
 		{
-			"qcom,ovp",
-			&cfg->ovp,
+			.name = "qcom,ovp",
+			.val_ptr = &cfg->ovp,
 			.cfg = &wled3_ovp_cfg,
 		},
 		{
-			"qcom,switching-freq",
-			&cfg->switch_freq,
+			.name = "qcom,switching-freq",
+			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
 		{
-			"qcom,num-strings",
-			&cfg->num_strings,
+			.name = "qcom,num-strings",
+			.val_ptr = &cfg->num_strings,
 			.cfg = &wled3_num_strings_cfg,
 		},
 	};
-	const struct {
-		const char *name;
-		bool *val_ptr;
-	} bool_opts[] = {
+
+	const struct wled_u32_opts wled4_opts[] = {
+		{
+			.name = "qcom,current-boost-limit",
+			.val_ptr = &cfg->boost_i_limit,
+			.cfg = &wled4_boost_i_limit_cfg,
+		},
+		{
+			.name = "qcom,current-limit-microamp",
+			.val_ptr = &cfg->string_i_limit,
+			.cfg = &wled4_string_i_limit_cfg,
+		},
+		{
+			.name = "qcom,ovp-millivolt",
+			.val_ptr = &cfg->ovp,
+			.cfg = &wled4_ovp_cfg,
+		},
+		{
+			.name = "qcom,switching-freq",
+			.val_ptr = &cfg->switch_freq,
+			.cfg = &wled3_switch_freq_cfg,
+		},
+		{
+			.name = "qcom,num-strings",
+			.val_ptr = &cfg->num_strings,
+			.cfg = &wled4_num_strings_cfg,
+		},
+	};
+
+	const struct wled_bool_opts bool_opts[] = {
 		{ "qcom,cs-out", &cfg->cs_out_en, },
 		{ "qcom,ext-gen", &cfg->ext_gen, },
-		{ "qcom,cabc", &cfg->cabc_en, },
+		{ "qcom,cabc", &cfg->cabc, },
 	};
 
-	rc = of_property_read_u32(dev->of_node, "reg", &val);
-	if (rc || val > 0xffff) {
-		dev_err(dev, "invalid IO resources\n");
-		return rc ? rc : -EINVAL;
+	prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
+	if (!prop_addr) {
+		dev_err(wled->dev, "invalid IO resources\n");
+		return -EINVAL;
+	}
+	wled->ctrl_addr = be32_to_cpu(*prop_addr);
+
+	if (*wled->version != WLED_PM8941) {
+		prop_addr = of_get_address(dev->of_node, 1, NULL, NULL);
+		if (!prop_addr) {
+			dev_err(wled->dev, "invalid IO resources\n");
+			return -EINVAL;
+		}
+		wled->sink_addr = be32_to_cpu(*prop_addr);
 	}
-	wled->addr = val;
 
 	rc = of_property_read_string(dev->of_node, "label", &wled->name);
 	if (rc)
 		wled->name = dev->of_node->name;
 
-	*cfg = wled3_config_defaults;
-	for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
+	if (*wled->version = WLED_PM8941) {
+		u32_opts = wled3_opts;
+		size = ARRAY_SIZE(wled3_opts);
+		*cfg = wled3_config_defaults;
+		wled->wled_set_brightness = wled3_set_brightness;
+		wled->wled_sync_toggle = wled3_sync_toggle;
+	} else if (*wled->version = WLED_PMI8998 ||
+			*wled->version = WLED_PM660L) {
+		u32_opts = wled4_opts;
+		size = ARRAY_SIZE(wled4_opts);
+		*cfg = wled4_config_defaults;
+		wled->wled_set_brightness = wled4_set_brightness;
+		wled->wled_sync_toggle = wled4_sync_toggle;
+	}
+
+	for (i = 0; i < size; ++i) {
 		rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);
 		if (rc = -EINVAL) {
 			continue;
@@ -346,12 +685,15 @@ static int wled_configure(struct wled *wled, struct device *dev)
 
 		c = UINT_MAX;
 		for (j = 0; c != val; j++) {
-			c = wled3_values(u32_opts[i].cfg, j);
+			c = wled_values(u32_opts[i].cfg, j);
 			if (c = UINT_MAX) {
 				dev_err(dev, "invalid value for '%s'\n",
 					u32_opts[i].name);
 				return -EINVAL;
 			}
+
+			if (c = val)
+				break;
 		}
 
 		dev_dbg(dev, "'%s' = %u\n", u32_opts[i].name, c);
@@ -365,6 +707,15 @@ static int wled_configure(struct wled *wled, struct device *dev)
 
 	cfg->num_strings = cfg->num_strings + 1;
 
+	string_len = of_property_count_elems_of_size(dev->of_node,
+						     "qcom,enabled-strings",
+						     sizeof(u32));
+	if (string_len > 0)
+		rc = of_property_read_u32_array(dev->of_node,
+						"qcom,enabled-strings",
+						wled->cfg.enabled_strings,
+						sizeof(u32));
+
 	return 0;
 }
 
@@ -392,15 +743,33 @@ static int wled_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	wled->regmap = regmap;
+	wled->dev = &pdev->dev;
 
-	rc = wled_configure(wled, &pdev->dev);
-	if (rc)
-		return rc;
+	wled->version = of_device_get_match_data(&pdev->dev);
+	if (!wled->version) {
+		dev_err(&pdev->dev, "Unknown device version\n");
+		return -ENODEV;
+	}
 
-	rc = wled_setup(wled);
+	rc = wled_configure(wled);
 	if (rc)
 		return rc;
 
+	if (*wled->version = WLED_PM8941) {
+		rc = wled3_setup(wled);
+		if (rc) {
+			dev_err(&pdev->dev, "wled3_setup failed\n");
+			return rc;
+		}
+	} else if (*wled->version = WLED_PMI8998 ||
+			*wled->version = WLED_PM660L) {
+		rc = wled4_setup(wled);
+		if (rc) {
+			dev_err(&pdev->dev, "wled4_setup failed\n");
+			return rc;
+		}
+	}
+
 	val = WLED_DEFAULT_BRIGHTNESS;
 	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
 
@@ -415,7 +784,9 @@ static int wled_probe(struct platform_device *pdev)
 };
 
 static const struct of_device_id wled_match_table[] = {
-	{ .compatible = "qcom,pm8941-wled" },
+	{ .compatible = "qcom,pm8941-wled", .data = &version_table[0] },
+	{ .compatible = "qcom,pmi8998-wled", .data = &version_table[1] },
+	{ .compatible = "qcom,pm660l-wled", .data = &version_table[2] },
 	{}
 };
 MODULE_DEVICE_TABLE(of, wled_match_table);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project

Re: [PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral

[Bjorn Andersson]

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:

> WLED4 peripheral is present on some PMICs like pmi8998 and
> pm660l. It has a different register map and configurations
> are also different. Add support for it.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 635 ++++++++++++++++++++++++++++--------
>  1 file changed, 503 insertions(+), 132 deletions(-)

Split this further into a patch that does structural preparation of
WLED3 support and then an addition of WLED4, the mixture makes parts of
this patch almost impossible to review. Please find some comments below.

> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
[..]
>  
>  /* WLED3 sink registers */
>  #define WLED3_SINK_REG_SYNC				0x47

Drop the 3 from this if it's common between the two.

> -#define  WLED3_SINK_REG_SYNC_MASK			0x07
> +#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
> +#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
>  #define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
>  #define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
>  #define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
> +#define  WLED4_SINK_REG_SYNC_LED4			BIT(3)
>  #define  WLED3_SINK_REG_SYNC_ALL			0x07
> +#define  WLED4_SINK_REG_SYNC_ALL			0x0f
>  #define  WLED3_SINK_REG_SYNC_CLEAR			0x00
>  
[..]
> +static int wled4_set_brightness(struct wled *wled, u16 brightness)

Afaict this is identical to wled3_set_brightness() with the exception
that there's a minimum brightness and the base address for the
brightness registers are different.

I would suggest that you add a min_brightness to wled and that you
figure out a way to carry the brightness base register address; and by
that you squash these two functions.

> +{
> +	int rc, i;
> +	u16 low_limit = wled->max_brightness * 4 / 1000;
> +	u8 v[2];
>  
> -		rc = regmap_bulk_write(wled->regmap,
> -				wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
> -				v, 2);
> -		if (rc)
> +	/* WLED4's lower limit of operation is 0.4% */
> +	if (brightness > 0 && brightness < low_limit)
> +		brightness = low_limit;
> +
> +	v[0] = brightness & 0xff;
> +	v[1] = (brightness >> 8) & 0xf;
> +
> +	for (i = 0;  i < wled->cfg.num_strings; ++i) {
> +		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
> +				       WLED4_SINK_REG_BRIGHT(i), v, 2);
> +		if (rc < 0)
>  			return rc;
>  	}
>  
> +	return 0;
> +}
> +
> +static int wled_module_enable(struct wled *wled, int val)
> +{
> +	int rc;
> +
> +	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> +				WLED3_CTRL_REG_MOD_EN,
> +				WLED3_CTRL_REG_MOD_EN_MASK,
> +				WLED3_CTRL_REG_MOD_EN_MASK);

This should depend on val.

> +	return rc;
> +}
> +
> +static int wled3_sync_toggle(struct wled *wled)
> +{
> +	int rc;
> +
>  	rc = regmap_update_bits(wled->regmap,
> -			wled->addr + WLED3_SINK_REG_SYNC,
> -			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
> -	if (rc)
> +				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
> +				WLED3_SINK_REG_SYNC_MASK,
> +				WLED3_SINK_REG_SYNC_MASK);
> +	if (rc < 0)
>  		return rc;
>  
>  	rc = regmap_update_bits(wled->regmap,
> -			wled->addr + WLED3_SINK_REG_SYNC,
> -			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
> +				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
> +				WLED3_SINK_REG_SYNC_MASK,
> +				WLED3_SINK_REG_SYNC_CLEAR);
> +
>  	return rc;
>  }
>  
> -static int wled_setup(struct wled *wled)
> +static int wled4_sync_toggle(struct wled *wled)

This is identical to wled3_sync_toggle() if you express the SYNC_MASK as
GENMASK(max-string-count, 0);

>  {
>  	int rc;
> -	int i;
>  
>  	rc = regmap_update_bits(wled->regmap,
> -			wled->addr + WLED3_CTRL_REG_OVP,
> -			WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
> -	if (rc)
> +				wled->sink_addr + WLED3_SINK_REG_SYNC,
> +				WLED4_SINK_REG_SYNC_MASK,
> +				WLED4_SINK_REG_SYNC_MASK);
> +	if (rc < 0)
>  		return rc;
>  
>  	rc = regmap_update_bits(wled->regmap,
> -			wled->addr + WLED3_CTRL_REG_ILIMIT,
> -			WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
> +				wled->sink_addr + WLED3_SINK_REG_SYNC,
> +				WLED4_SINK_REG_SYNC_MASK,
> +				WLED3_SINK_REG_SYNC_CLEAR);
> +
> +	return rc;
> +}
> +
> +static int wled_update_status(struct backlight_device *bl)

You should be able to do the structural changes of this in one patch,
then follow up with the additions of WLED4 in a separate patch.

> +{
> +	struct wled *wled = bl_get_data(bl);
> +	u16 brightness = bl->props.brightness;
> +	int rc = 0;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||
> +	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> +	    bl->props.state & BL_CORE_FBBLANK)
> +		brightness = 0;
> +
> +	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;
> +		}
> +
> +		rc = wled->wled_sync_toggle(wled);
> +		if (rc < 0) {
> +			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> +			return rc;
> +		}
> +	}
> +
> +	if (!!brightness != !!wled->brightness) {
> +		rc = wled_module_enable(wled, !!brightness);
> +		if (rc < 0) {
> +			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
> +			return rc;
> +		}
> +	}
> +
> +	wled->brightness = brightness;
> +
> +	return rc;
> +}
> +
> +static int wled3_setup(struct wled *wled)

Changes to this function should be unrelated to the addition of WLED4
support.

> +{
> +	u16 addr;
> +	u8 sink_en = 0;
> +	int rc, i, j;
> +
> +	rc = regmap_update_bits(wled->regmap,
> +				wled->ctrl_addr + WLED3_CTRL_REG_OVP,
> +				WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
>  	if (rc)
>  		return rc;
>  
>  	rc = regmap_update_bits(wled->regmap,
> -			wled->addr + WLED3_CTRL_REG_FREQ,
> -			WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
> +				wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
> +				WLED3_CTRL_REG_ILIMIT_MASK,
> +				wled->cfg.boost_i_limit);
[..]
> @@ -392,15 +743,33 @@ static int wled_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	wled->regmap = regmap;
> +	wled->dev = &pdev->dev;
>  
> -	rc = wled_configure(wled, &pdev->dev);
> -	if (rc)
> -		return rc;
> +	wled->version = of_device_get_match_data(&pdev->dev);
> +	if (!wled->version) {
> +		dev_err(&pdev->dev, "Unknown device version\n");
> +		return -ENODEV;
> +	}
>  
> -	rc = wled_setup(wled);
> +	rc = wled_configure(wled);

Pass version as a parameter to wled_configure to make "version" a local
variable.

>  	if (rc)
>  		return rc;
>  
> +	if (*wled->version = WLED_PM8941) {

This would be better expressed with a select statement. And rather than
keeping track of every single version just stick to 3 and 4, if there
are further differences within version 4 let's try to encode these with
some sort of quirks or feature flags.

> +		rc = wled3_setup(wled);
> +		if (rc) {
> +			dev_err(&pdev->dev, "wled3_setup failed\n");
> +			return rc;
> +		}
> +	} else if (*wled->version = WLED_PMI8998 ||
> +			*wled->version = WLED_PM660L) {
> +		rc = wled4_setup(wled);
> +		if (rc) {
> +			dev_err(&pdev->dev, "wled4_setup failed\n");
> +			return rc;
> +		}
> +	}
> +
>  	val = WLED_DEFAULT_BRIGHTNESS;
>  	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
>  
> @@ -415,7 +784,9 @@ static int wled_probe(struct platform_device *pdev)
>  };
>  
>  static const struct of_device_id wled_match_table[] = {
> -	{ .compatible = "qcom,pm8941-wled" },
> +	{ .compatible = "qcom,pm8941-wled", .data = &version_table[0] },
> +	{ .compatible = "qcom,pmi8998-wled", .data = &version_table[1] },
> +	{ .compatible = "qcom,pm660l-wled", .data = &version_table[2] },

You can simply do .data = (void *)3 and .data = (void *)4 to pass the
WLED version to probe.

>  	{}
>  };

Regards,
Bjorn

Re: [PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral

[kgunda]

On 2018-06-20 10:44, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> 
>> WLED4 peripheral is present on some PMICs like pmi8998 and
>> pm660l. It has a different register map and configurations
>> are also different. Add support for it.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/video/backlight/qcom-wled.c | 635 
>> ++++++++++++++++++++++++++++--------
>>  1 file changed, 503 insertions(+), 132 deletions(-)
> 
> Split this further into a patch that does structural preparation of
> WLED3 support and then an addition of WLED4, the mixture makes parts of
> this patch almost impossible to review. Please find some comments 
> below.
> 
Sure. I will split it in the next series.
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
> [..]
>> 
>>  /* WLED3 sink registers */
>>  #define WLED3_SINK_REG_SYNC				0x47
> 
> Drop the 3 from this if it's common between the two.
> 
>> -#define  WLED3_SINK_REG_SYNC_MASK			0x07
>> +#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
>> +#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
>>  #define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
>>  #define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
>>  #define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
>> +#define  WLED4_SINK_REG_SYNC_LED4			BIT(3)
>>  #define  WLED3_SINK_REG_SYNC_ALL			0x07
>> +#define  WLED4_SINK_REG_SYNC_ALL			0x0f
>>  #define  WLED3_SINK_REG_SYNC_CLEAR			0x00
>> 
> [..]
>> +static int wled4_set_brightness(struct wled *wled, u16 brightness)
> 
> Afaict this is identical to wled3_set_brightness() with the exception
> that there's a minimum brightness and the base address for the
> brightness registers are different.
> 
> I would suggest that you add a min_brightness to wled and that you
> figure out a way to carry the brightness base register address; and by
> that you squash these two functions.
> 
There are four different parameters. 1) minimum brightness 2) WLED base 
addresses
3) Brightness base addresses 4) the brightness sink registers address 
offsets also
different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as in 
wled4 0x57, 0x67, 0x77, 0x87).

Irrelevant to this patch, but wled5 has some more extra registers to set 
the brightness.
Keeping this in mind, it is better to have separate functions? Otherwise 
we will have to use the
version checks in the wled_set_brightness function, if we have the 
common function.
>> +{
>> +	int rc, i;
>> +	u16 low_limit = wled->max_brightness * 4 / 1000;
>> +	u8 v[2];
>> 
>> -		rc = regmap_bulk_write(wled->regmap,
>> -				wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
>> -				v, 2);
>> -		if (rc)
>> +	/* WLED4's lower limit of operation is 0.4% */
>> +	if (brightness > 0 && brightness < low_limit)
>> +		brightness = low_limit;
>> +
>> +	v[0] = brightness & 0xff;
>> +	v[1] = (brightness >> 8) & 0xf;
>> +
>> +	for (i = 0;  i < wled->cfg.num_strings; ++i) {
>> +		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
>> +				       WLED4_SINK_REG_BRIGHT(i), v, 2);
>> +		if (rc < 0)
>>  			return rc;
>>  	}
>> 
>> +	return 0;
>> +}
>> +
>> +static int wled_module_enable(struct wled *wled, int val)
>> +{
>> +	int rc;
>> +
>> +	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>> +				WLED3_CTRL_REG_MOD_EN,
>> +				WLED3_CTRL_REG_MOD_EN_MASK,
>> +				WLED3_CTRL_REG_MOD_EN_MASK);
> 
> This should depend on val.
> 
Will fix it in the next series.
>> +	return rc;
>> +}
>> +
>> +static int wled3_sync_toggle(struct wled *wled)
>> +{
>> +	int rc;
>> +
>>  	rc = regmap_update_bits(wled->regmap,
>> -			wled->addr + WLED3_SINK_REG_SYNC,
>> -			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
>> -	if (rc)
>> +				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
>> +				WLED3_SINK_REG_SYNC_MASK,
>> +				WLED3_SINK_REG_SYNC_MASK);
>> +	if (rc < 0)
>>  		return rc;
>> 
>>  	rc = regmap_update_bits(wled->regmap,
>> -			wled->addr + WLED3_SINK_REG_SYNC,
>> -			WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
>> +				wled->ctrl_addr + WLED3_SINK_REG_SYNC,
>> +				WLED3_SINK_REG_SYNC_MASK,
>> +				WLED3_SINK_REG_SYNC_CLEAR);
>> +
>>  	return rc;
>>  }
>> 
>> -static int wled_setup(struct wled *wled)
>> +static int wled4_sync_toggle(struct wled *wled)
> 
> This is identical to wled3_sync_toggle() if you express the SYNC_MASK 
> as
> GENMASK(max-string-count, 0);
> 
Will change it in the next series.
>>  {
>>  	int rc;
>> -	int i;
>> 
>>  	rc = regmap_update_bits(wled->regmap,
>> -			wled->addr + WLED3_CTRL_REG_OVP,
>> -			WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
>> -	if (rc)
>> +				wled->sink_addr + WLED3_SINK_REG_SYNC,
>> +				WLED4_SINK_REG_SYNC_MASK,
>> +				WLED4_SINK_REG_SYNC_MASK);
>> +	if (rc < 0)
>>  		return rc;
>> 
>>  	rc = regmap_update_bits(wled->regmap,
>> -			wled->addr + WLED3_CTRL_REG_ILIMIT,
>> -			WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
>> +				wled->sink_addr + WLED3_SINK_REG_SYNC,
>> +				WLED4_SINK_REG_SYNC_MASK,
>> +				WLED3_SINK_REG_SYNC_CLEAR);
>> +
>> +	return rc;
>> +}
>> +
>> +static int wled_update_status(struct backlight_device *bl)
> 
> You should be able to do the structural changes of this in one patch,
> then follow up with the additions of WLED4 in a separate patch.
> 
Sure. Will do it in the next series.
>> +{
>> +	struct wled *wled = bl_get_data(bl);
>> +	u16 brightness = bl->props.brightness;
>> +	int rc = 0;
>> +
>> +	if (bl->props.power != FB_BLANK_UNBLANK ||
>> +	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
>> +	    bl->props.state & BL_CORE_FBBLANK)
>> +		brightness = 0;
>> +
>> +	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;
>> +		}
>> +
>> +		rc = wled->wled_sync_toggle(wled);
>> +		if (rc < 0) {
>> +			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	if (!!brightness != !!wled->brightness) {
>> +		rc = wled_module_enable(wled, !!brightness);
>> +		if (rc < 0) {
>> +			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	wled->brightness = brightness;
>> +
>> +	return rc;
>> +}
>> +
>> +static int wled3_setup(struct wled *wled)
> 
> Changes to this function should be unrelated to the addition of WLED4
> support.
> 
I will separate it out from this patch in next series.
>> +{
>> +	u16 addr;
>> +	u8 sink_en = 0;
>> +	int rc, i, j;
>> +
>> +	rc = regmap_update_bits(wled->regmap,
>> +				wled->ctrl_addr + WLED3_CTRL_REG_OVP,
>> +				WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
>>  	if (rc)
>>  		return rc;
>> 
>>  	rc = regmap_update_bits(wled->regmap,
>> -			wled->addr + WLED3_CTRL_REG_FREQ,
>> -			WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
>> +				wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
>> +				WLED3_CTRL_REG_ILIMIT_MASK,
>> +				wled->cfg.boost_i_limit);
> [..]
>> @@ -392,15 +743,33 @@ static int wled_probe(struct platform_device 
>> *pdev)
>>  		return -ENOMEM;
>> 
>>  	wled->regmap = regmap;
>> +	wled->dev = &pdev->dev;
>> 
>> -	rc = wled_configure(wled, &pdev->dev);
>> -	if (rc)
>> -		return rc;
>> +	wled->version = of_device_get_match_data(&pdev->dev);
>> +	if (!wled->version) {
>> +		dev_err(&pdev->dev, "Unknown device version\n");
>> +		return -ENODEV;
>> +	}
>> 
>> -	rc = wled_setup(wled);
>> +	rc = wled_configure(wled);
> 
> Pass version as a parameter to wled_configure to make "version" a local
> variable.
> 
Sure. I will do it in the next series.
>>  	if (rc)
>>  		return rc;
>> 
>> +	if (*wled->version = WLED_PM8941) {
> 
> This would be better expressed with a select statement. And rather than
> keeping track of every single version just stick to 3 and 4, if there
> are further differences within version 4 let's try to encode these with
> some sort of quirks or feature flags.
> 
I will modify it in the next series.
>> +		rc = wled3_setup(wled);
>> +		if (rc) {
>> +			dev_err(&pdev->dev, "wled3_setup failed\n");
>> +			return rc;
>> +		}
>> +	} else if (*wled->version = WLED_PMI8998 ||
>> +			*wled->version = WLED_PM660L) {
>> +		rc = wled4_setup(wled);
>> +		if (rc) {
>> +			dev_err(&pdev->dev, "wled4_setup failed\n");
>> +			return rc;
>> +		}
>> +	}
>> +
>>  	val = WLED_DEFAULT_BRIGHTNESS;
>>  	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
>> 
>> @@ -415,7 +784,9 @@ static int wled_probe(struct platform_device 
>> *pdev)
>>  };
>> 
>>  static const struct of_device_id wled_match_table[] = {
>> -	{ .compatible = "qcom,pm8941-wled" },
>> +	{ .compatible = "qcom,pm8941-wled", .data = &version_table[0] },
>> +	{ .compatible = "qcom,pmi8998-wled", .data = &version_table[1] },
>> +	{ .compatible = "qcom,pm660l-wled", .data = &version_table[2] },
> 
> You can simply do .data = (void *)3 and .data = (void *)4 to pass the
> WLED version to probe.
> 
I will modify it in the next series.
>>  	{}
>>  };
> 
> 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

Re: [PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral

[Bjorn Andersson]

On Wed 20 Jun 04:00 PDT 2018, kgunda@codeaurora.org wrote:

> On 2018-06-20 10:44, Bjorn Andersson wrote:
> > On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> > 
> > > WLED4 peripheral is present on some PMICs like pmi8998 and
> > > pm660l. It has a different register map and configurations
> > > are also different. Add support for it.
> > > 
> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> > > ---
> > >  drivers/video/backlight/qcom-wled.c | 635
> > > ++++++++++++++++++++++++++++--------
> > >  1 file changed, 503 insertions(+), 132 deletions(-)
> > 
> > Split this further into a patch that does structural preparation of
> > WLED3 support and then an addition of WLED4, the mixture makes parts of
> > this patch almost impossible to review. Please find some comments below.
> > 
> Sure. I will split it in the next series.

Thanks!

> > > 
> > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > b/drivers/video/backlight/qcom-wled.c
> > [..]
> > > 
> > >  /* WLED3 sink registers */
> > >  #define WLED3_SINK_REG_SYNC				0x47
> > 
> > Drop the 3 from this if it's common between the two.
> > 
> > > -#define  WLED3_SINK_REG_SYNC_MASK			0x07
> > > +#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
> > > +#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
> > >  #define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
> > >  #define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
> > >  #define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
> > > +#define  WLED4_SINK_REG_SYNC_LED4			BIT(3)
> > >  #define  WLED3_SINK_REG_SYNC_ALL			0x07
> > > +#define  WLED4_SINK_REG_SYNC_ALL			0x0f
> > >  #define  WLED3_SINK_REG_SYNC_CLEAR			0x00
> > > 
> > [..]
> > > +static int wled4_set_brightness(struct wled *wled, u16 brightness)
> > 
> > Afaict this is identical to wled3_set_brightness() with the exception
> > that there's a minimum brightness and the base address for the
> > brightness registers are different.
> > 
> > I would suggest that you add a min_brightness to wled and that you
> > figure out a way to carry the brightness base register address; and by
> > that you squash these two functions.
> > 
> There are four different parameters. 1) minimum brightness 2) WLED base
> addresses
> 3) Brightness base addresses 4) the brightness sink registers address
> offsets also
> different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as in
> wled4 0x57, 0x67, 0x77, 0x87).
> 

Sorry, I must have gotten lost in the defines, I see the difference
between the two register layouts now. If you retain the old mechanism of
doing the math openly in the function this would have been obvious.

> Irrelevant to this patch, but wled5 has some more extra registers to
> set the brightness.  Keeping this in mind, it is better to have
> separate functions? Otherwise we will have to use the version checks
> in the wled_set_brightness function, if we have the common function.

Okay, so it sounds reasonable to split this out to some degree.

Regards,
Bjorn

Re: [PATCH V3 5/7] backlight: qcom-wled: Add support for WLED4 peripheral

[kgunda]

On 2018-06-23 04:39, Bjorn Andersson wrote:
> On Wed 20 Jun 04:00 PDT 2018, kgunda@codeaurora.org wrote:
> 
>> On 2018-06-20 10:44, Bjorn Andersson wrote:
>> > On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
>> >
>> > > WLED4 peripheral is present on some PMICs like pmi8998 and
>> > > pm660l. It has a different register map and configurations
>> > > are also different. Add support for it.
>> > >
>> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> > > ---
>> > >  drivers/video/backlight/qcom-wled.c | 635
>> > > ++++++++++++++++++++++++++++--------
>> > >  1 file changed, 503 insertions(+), 132 deletions(-)
>> >
>> > Split this further into a patch that does structural preparation of
>> > WLED3 support and then an addition of WLED4, the mixture makes parts of
>> > this patch almost impossible to review. Please find some comments below.
>> >
>> Sure. I will split it in the next series.
> 
> Thanks!
> 
>> > >
>> > > diff --git a/drivers/video/backlight/qcom-wled.c
>> > > b/drivers/video/backlight/qcom-wled.c
>> > [..]
>> > >
>> > >  /* WLED3 sink registers */
>> > >  #define WLED3_SINK_REG_SYNC				0x47
>> >
>> > Drop the 3 from this if it's common between the two.
>> >
>> > > -#define  WLED3_SINK_REG_SYNC_MASK			0x07
>> > > +#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
>> > > +#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
>> > >  #define  WLED3_SINK_REG_SYNC_LED1			BIT(0)
>> > >  #define  WLED3_SINK_REG_SYNC_LED2			BIT(1)
>> > >  #define  WLED3_SINK_REG_SYNC_LED3			BIT(2)
>> > > +#define  WLED4_SINK_REG_SYNC_LED4			BIT(3)
>> > >  #define  WLED3_SINK_REG_SYNC_ALL			0x07
>> > > +#define  WLED4_SINK_REG_SYNC_ALL			0x0f
>> > >  #define  WLED3_SINK_REG_SYNC_CLEAR			0x00
>> > >
>> > [..]
>> > > +static int wled4_set_brightness(struct wled *wled, u16 brightness)
>> >
>> > Afaict this is identical to wled3_set_brightness() with the exception
>> > that there's a minimum brightness and the base address for the
>> > brightness registers are different.
>> >
>> > I would suggest that you add a min_brightness to wled and that you
>> > figure out a way to carry the brightness base register address; and by
>> > that you squash these two functions.
>> >
>> There are four different parameters. 1) minimum brightness 2) WLED 
>> base
>> addresses
>> 3) Brightness base addresses 4) the brightness sink registers address
>> offsets also
>> different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as 
>> in
>> wled4 0x57, 0x67, 0x77, 0x87).
>> 
> 
> Sorry, I must have gotten lost in the defines, I see the difference
> between the two register layouts now. If you retain the old mechanism 
> of
> doing the math openly in the function this would have been obvious.
> 
>> Irrelevant to this patch, but wled5 has some more extra registers to
>> set the brightness.  Keeping this in mind, it is better to have
>> separate functions? Otherwise we will have to use the version checks
>> in the wled_set_brightness function, if we have the common function.
> 
> Okay, so it sounds reasonable to split this out to some degree.
> 
> Regards,
> Bjorn
> --
Thanks for that !
> 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

[PATCH V3 6/7] backlight: qcom-wled: add support for short circuit handling

[Kiran Gunda]

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 | 130 +++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 3 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 0bc7fcd..e87ba70 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>
@@ -38,6 +41,16 @@
 #define WLED3_CTRL_REG_ILIMIT				0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASK			GENMASK(2, 0)
 
+/* 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)
@@ -132,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;
+	bool disabled_by_short;
+	bool has_short_detect;
 
 	struct wled_config cfg;
 	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
@@ -194,6 +213,9 @@ static int wled_module_enable(struct wled *wled, int val)
 {
 	int rc;
 
+	if (wled->disabled_by_short)
+		return -EFAULT;
+
 	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
 				WLED3_CTRL_REG_MOD_EN,
 				WLED3_CTRL_REG_MOD_EN_MASK,
@@ -250,18 +272,19 @@ 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);
 		if (rc < 0) {
 			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
-			return rc;
+			goto unlock_mutex;
 		}
 	}
 
@@ -269,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		USEC_PER_SEC
+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->disabled_by_short = 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;
@@ -387,6 +455,21 @@ static int wled4_setup(struct wled *wled)
 	if (rc < 0)
 		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;
+	}
+
 	rc = regmap_read(wled->regmap, wled->sink_addr +
 			 WLED4_SINK_REG_CURR_SINK, &sink_cfg);
 	if (rc < 0)
@@ -472,6 +555,7 @@ static int wled4_setup(struct wled *wled)
 	.num_strings = 4,
 	.switch_freq = 11,
 	.cabc = false,
+	.external_pfet = false,
 };
 
 static const u32 wled3_boost_i_limit_values[] = {
@@ -637,6 +721,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);
@@ -719,6 +804,38 @@ 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, short_irq;
+
+	if (!wled->has_short_detect)
+		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;
+
+	short_irq = platform_get_irq_byname(pdev, "short");
+	if (short_irq < 0) {
+		dev_dbg(&pdev->dev, "short irq is not used\n");
+		return 0;
+	}
+
+	rc = devm_request_threaded_irq(wled->dev, 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,
 };
@@ -751,6 +868,8 @@ static int wled_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	mutex_init(&wled->lock);
+
 	rc = wled_configure(wled);
 	if (rc)
 		return rc;
@@ -763,6 +882,7 @@ static int wled_probe(struct platform_device *pdev)
 		}
 	} else if (*wled->version = WLED_PMI8998 ||
 			*wled->version = WLED_PM660L) {
+		wled->has_short_detect = true;
 		rc = wled4_setup(wled);
 		if (rc) {
 			dev_err(&pdev->dev, "wled4_setup failed\n");
@@ -770,6 +890,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);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project

Re: [PATCH V3 6/7] backlight: qcom-wled: add support for short circuit handling

[Bjorn Andersson]

On Tue 19 Jun 04:13 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 | 130 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 127 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
[..]
>  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;
> +	bool disabled_by_short;
> +	bool has_short_detect;

The use of feature flags, instead of checking the version, like this is
good!

>  
>  	struct wled_config cfg;
>  	int (*wled_set_brightness)(struct wled *wled, u16 brightness);

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

Re: [PATCH V3 6/7] backlight: qcom-wled: add support for short circuit handling

[Vinod]

On 19-06-18, 16:43, Kiran Gunda wrote:

>  struct wled {
>  	const char *name;
>  	struct device *dev;
>  	struct regmap *regmap;
> +	struct mutex lock;	/* Lock to avoid race from ISR */

the comment is wrong as you avoid race with thread handler and not the
main ISR. The ISR runs in atomic context so you cant use a mutex but you
may do so with a thread handler

> +#define WLED_SHORT_DLY_MS			20
> +#define WLED_SHORT_CNT_MAX			5
> +#define WLED_SHORT_RESET_CNT_DLY_US		USEC_PER_SEC

an empty line after defines would be better

> +static int wled_configure_short_irq(struct wled *wled,
> +				    struct platform_device *pdev)
> +{
> +	int rc = 0, short_irq;

superfluous initialization of rc

-- 
~Vinod

Re: [PATCH V3 6/7] backlight: qcom-wled: add support for short circuit handling

[kgunda]

On 2018-06-20 11:17, Vinod wrote:
> On 19-06-18, 16:43, Kiran Gunda wrote:
> 
>>  struct wled {
>>  	const char *name;
>>  	struct device *dev;
>>  	struct regmap *regmap;
>> +	struct mutex lock;	/* Lock to avoid race from ISR */
> 
> the comment is wrong as you avoid race with thread handler and not the
> main ISR. The ISR runs in atomic context so you cant use a mutex but 
> you
> may do so with a thread handler
> 
Will fix the comment in the next series.
>> +#define WLED_SHORT_DLY_MS			20
>> +#define WLED_SHORT_CNT_MAX			5
>> +#define WLED_SHORT_RESET_CNT_DLY_US		USEC_PER_SEC
> 
> an empty line after defines would be better
> 
Will add it in the next series.
>> +static int wled_configure_short_irq(struct wled *wled,
>> +				    struct platform_device *pdev)
>> +{
>> +	int rc = 0, short_irq;
> 
> superfluous initialization of rc
Will remove the initialization in next series.

[PATCH V3 7/7] backlight: qcom-wled: Add auto string detection logic

[Kiran Gunda]

The auto string detection algorithm checks if the current WLED
sink configuration is valid. It tries enabling every sink and
checks if the OVP fault is observed. Based on this information
it detects and enables the valid sink configuration.
Auto calibration will be triggered when the OVP fault interrupts
are seen frequently thereby it tries to fix the sink configuration.

The auto-detection also kicks in when the connected LED string
of the display-backlight malfunctions (because of damage) and
requires the damaged string to be turned off to prevent the
complete panel and/or board from being damaged.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index e87ba70..6bc627c 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -25,13 +25,23 @@
 #define WLED_MAX_STRINGS				4
 
 #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_OVP_FAULT_STATUS		BIT(1)
+
 #define WLED3_CTRL_REG_MOD_EN				0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
 
+#define WLED3_CTRL_REG_FEEDBACK_CONTROL			0x48
+
 #define WLED3_CTRL_REG_FREQ				0x4c
 #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
 
@@ -146,6 +156,7 @@ struct wled_config {
 	bool ext_gen;
 	bool cabc;
 	bool external_pfet;
+	bool auto_detection_enabled;
 };
 
 struct wled {
@@ -154,16 +165,22 @@ struct wled {
 	struct regmap *regmap;
 	struct mutex lock;	/* Lock to avoid race from ISR */
 	ktime_t last_short_event;
+	ktime_t start_ovp_fault_time;
 	u16 ctrl_addr;
 	u16 sink_addr;
+	u16 auto_detection_ovp_count;
 	u32 brightness;
 	u32 max_brightness;
 	u32 short_count;
+	u32 auto_detect_count;
 	const int *version;
 	bool disabled_by_short;
 	bool has_short_detect;
+	int ovp_irq;
+	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 +226,27 @@ 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_MASK) {
+		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
+			enable_irq(wled->ovp_irq);
+			wled->ovp_irq_disabled = false;
+		}
+	}
+}
+
 static int wled_module_enable(struct wled *wled, int val)
 {
 	int rc;
@@ -220,7 +258,20 @@ 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;
+
+	if (val) {
+		schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
+	} else {
+		cancel_delayed_work(&wled->ovp_work);
+		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
+			disable_irq(wled->ovp_irq);
+			wled->ovp_irq_disabled = true;
+		}
+	}
+
+	return 0;
 }
 
 static int wled3_sync_toggle(struct wled *wled)
@@ -346,6 +397,305 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
 	return IRQ_HANDLED;
 }
 
+#define AUTO_DETECT_BRIGHTNESS		200
+static int wled_auto_string_detection(struct wled *wled)
+{
+	int rc = 0, i;
+	u32 sink_config = 0, int_sts;
+	u8 sink_test = 0, sink_valid = 0, val;
+
+	/* read configured sink configuration */
+	rc = regmap_read(wled->regmap, wled->sink_addr +
+			 WLED4_SINK_REG_CURR_SINK, &sink_config);
+	if (rc < 0) {
+		pr_err("Failed to read SINK configuration rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+	/* disable the module before starting detection */
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
+				WLED3_CTRL_REG_MOD_EN_MASK, 0);
+	if (rc < 0) {
+		pr_err("Failed to disable WLED module rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+	/* set low brightness across all sinks */
+	rc = wled4_set_brightness(wled, AUTO_DETECT_BRIGHTNESS);
+	if (rc < 0) {
+		pr_err("Failed to set brightness for auto detection rc=%d\n",
+		       rc);
+		goto failed_detect;
+	}
+
+	if (wled->cfg.cabc) {
+		for (i = 0; i < wled->cfg.num_strings; i++) {
+			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
+						WLED4_SINK_REG_STR_CABC(i),
+						WLED4_SINK_REG_STR_CABC_MASK,
+						0);
+			if (rc < 0)
+				goto failed_detect;
+		}
+	}
+
+	/* disable all sinks */
+	rc = regmap_write(wled->regmap,
+			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK, 0);
+	if (rc < 0) {
+		pr_err("Failed to disable all sinks rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+	/* iterate through the strings one by one */
+	for (i = 0; i < wled->cfg.num_strings; i++) {
+		sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i));
+
+		/* Enable feedback control */
+		rc = regmap_write(wled->regmap, wled->ctrl_addr +
+				  WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1);
+		if (rc < 0) {
+			pr_err("Failed to enable feedback for SINK %d rc = %d\n",
+			       i + 1, rc);
+			goto failed_detect;
+		}
+
+		/* enable the sink */
+		rc = regmap_write(wled->regmap, wled->sink_addr +
+				  WLED4_SINK_REG_CURR_SINK, sink_test);
+		if (rc < 0) {
+			pr_err("Failed to configure SINK %d rc=%d\n", i + 1,
+			       rc);
+			goto failed_detect;
+		}
+
+		/* Enable the module */
+		rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+					WLED3_CTRL_REG_MOD_EN,
+					WLED3_CTRL_REG_MOD_EN_MASK,
+					WLED3_CTRL_REG_MOD_EN_MASK);
+		if (rc < 0) {
+			pr_err("Failed to enable WLED module rc=%d\n", rc);
+			goto failed_detect;
+		}
+
+		usleep_range(WLED_SOFT_START_DLY_US,
+			     WLED_SOFT_START_DLY_US + 1000);
+
+		rc = regmap_read(wled->regmap, wled->ctrl_addr +
+				 WLED3_CTRL_REG_INT_RT_STS, &int_sts);
+		if (rc < 0) {
+			pr_err("Error in reading WLED3_CTRL_INT_RT_STS rc=%d\n",
+			       rc);
+			goto failed_detect;
+		}
+
+		if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
+			pr_debug("WLED OVP fault detected with SINK %d\n",
+				 i + 1);
+		else
+			sink_valid |= sink_test;
+
+		/* Disable the module */
+		rc = regmap_update_bits(wled->regmap,
+					wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
+					WLED3_CTRL_REG_MOD_EN_MASK, 0);
+		if (rc < 0) {
+			pr_err("Failed to disable WLED module rc=%d\n", rc);
+			goto failed_detect;
+		}
+	}
+
+	if (sink_valid = sink_config) {
+		pr_debug("WLED auto-detection complete, default sink-config=%x OK!\n",
+			 sink_config);
+	} else {
+		pr_warn("Invalid WLED default sink config=%x changing it to=%x\n",
+			sink_config, sink_valid);
+		sink_config = sink_valid;
+	}
+
+	if (!sink_config) {
+		pr_err("No valid WLED sinks found\n");
+		wled->disabled_by_short = true;
+		goto failed_detect;
+	}
+
+	/* write the new sink configuration */
+	rc = regmap_write(wled->regmap,
+			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
+			  sink_config);
+	if (rc < 0) {
+		pr_err("Failed to reconfigure the default sink rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+	/* Enable valid sinks */
+	for (i = 0; i < wled->cfg.num_strings; i++) {
+		if (wled->cfg.cabc) {
+			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
+						WLED4_SINK_REG_STR_CABC(i),
+						WLED4_SINK_REG_STR_CABC_MASK,
+						WLED4_SINK_REG_STR_CABC_MASK);
+			if (rc < 0)
+				goto failed_detect;
+		}
+
+		if (sink_config & BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
+			val = WLED4_SINK_REG_STR_MOD_MASK;
+		else
+			val = 0x0; /* disable modulator_en for unused sink */
+
+		rc = regmap_write(wled->regmap, wled->sink_addr +
+				  WLED4_SINK_REG_STR_MOD_EN(i), val);
+		if (rc < 0) {
+			pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc);
+			goto failed_detect;
+		}
+	}
+
+	/* restore the feedback setting */
+	rc = regmap_write(wled->regmap,
+			  wled->ctrl_addr + WLED3_CTRL_REG_FEEDBACK_CONTROL, 0);
+	if (rc < 0) {
+		pr_err("Failed to restore feedback setting rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+	/* restore  brightness */
+	rc = wled4_set_brightness(wled, wled->brightness);
+	if (rc < 0) {
+		pr_err("Failed to set brightness after auto detection rc=%d\n",
+		       rc);
+		goto failed_detect;
+	}
+
+	rc = regmap_update_bits(wled->regmap,
+				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
+				WLED3_CTRL_REG_MOD_EN_MASK,
+				WLED3_CTRL_REG_MOD_EN_MASK);
+	if (rc < 0) {
+		pr_err("Failed to enable WLED module rc=%d\n", rc);
+		goto failed_detect;
+	}
+
+failed_detect:
+	return rc;
+}
+
+#define WLED_AUTO_DETECT_OVP_COUNT		5
+#define WLED_AUTO_DETECT_CNT_DLY_US		USEC_PER_SEC
+static bool wled_auto_detection_required(struct wled *wled)
+{
+	s64 elapsed_time_us;
+
+	if (*wled->version = WLED_PM8941 || !wled->cfg.auto_detection_enabled)
+		return false;
+
+	/*
+	 * Check if the OVP fault was an occasional one
+	 * or if it's firing continuously, the latter qualifies
+	 * for an auto-detection check.
+	 */
+	if (!wled->auto_detection_ovp_count) {
+		wled->start_ovp_fault_time = ktime_get();
+		wled->auto_detection_ovp_count++;
+	} else {
+		elapsed_time_us = ktime_us_delta(ktime_get(),
+						 wled->start_ovp_fault_time);
+		if (elapsed_time_us > WLED_AUTO_DETECT_CNT_DLY_US)
+			wled->auto_detection_ovp_count = 0;
+		else
+			wled->auto_detection_ovp_count++;
+
+		if (wled->auto_detection_ovp_count >+				WLED_AUTO_DETECT_OVP_COUNT) {
+			wled->auto_detection_ovp_count = 0;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static int wled_auto_detection_at_init(struct wled *wled)
+{
+	int rc;
+	u32 fault_status = 0, rt_status = 0;
+
+	if (!wled->cfg.auto_detection_enabled)
+		return 0;
+
+	rc = regmap_read(wled->regmap,
+			 wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
+			 &rt_status);
+	if (rc < 0) {
+		pr_err("Failed to read RT status rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = regmap_read(wled->regmap,
+			 wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
+			 &fault_status);
+	if (rc < 0) {
+		pr_err("Failed to read fault status rc=%d\n", rc);
+		return rc;
+	}
+
+	if ((rt_status & WLED3_CTRL_REG_OVP_FAULT_STATUS) ||
+	    (fault_status & WLED3_CTRL_REG_OVP_FAULT_BIT)) {
+		mutex_lock(&wled->lock);
+		wled_auto_string_detection(wled);
+		mutex_unlock(&wled->lock);
+	}
+
+	return rc;
+}
+
+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);
+
+	if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT) {
+		mutex_lock(&wled->lock);
+		disable_irq_nosync(wled->ovp_irq);
+
+		if (wled_auto_detection_required(wled))
+			wled_auto_string_detection(wled);
+
+		enable_irq(wled->ovp_irq);
+		wled->ovp_irq_disabled = false;
+
+		mutex_unlock(&wled->lock);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int wled3_setup(struct wled *wled)
 {
 	u16 addr;
@@ -481,8 +831,10 @@ static int wled4_setup(struct wled *wled)
 		sink_en |= 1 << temp;
 	}
 
-	if (sink_cfg = sink_en)
-		return 0;
+	if (sink_cfg = sink_en) {
+		rc = wled_auto_detection_at_init(wled);
+		return rc;
+	}
 
 	rc = regmap_update_bits(wled->regmap,
 				wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
@@ -545,7 +897,9 @@ static int wled4_setup(struct wled *wled)
 		return rc;
 	}
 
-	return 0;
+	rc = wled_auto_detection_at_init(wled);
+
+	return rc;
 }
 
 static const struct wled_config wled4_config_defaults = {
@@ -556,6 +910,7 @@ static int wled4_setup(struct wled *wled)
 	.switch_freq = 11,
 	.cabc = false,
 	.external_pfet = false,
+	.auto_detection_enabled = false,
 };
 
 static const u32 wled3_boost_i_limit_values[] = {
@@ -722,6 +1077,7 @@ static int wled_configure(struct wled *wled)
 		{ "qcom,ext-gen", &cfg->ext_gen, },
 		{ "qcom,cabc", &cfg->cabc, },
 		{ "qcom,external-pfet", &cfg->external_pfet, },
+		{ "qcom,auto-string-detection", &cfg->auto_detection_enabled, },
 	};
 
 	prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
@@ -836,6 +1192,42 @@ 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;
+
+	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 = 0;
+		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,
 };
@@ -890,10 +1282,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);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project

Re: [PATCH V3 7/7] backlight: qcom-wled: Add auto string detection logic

[Bjorn Andersson]

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:

> The auto string detection algorithm checks if the current WLED
> sink configuration is valid. It tries enabling every sink and
> checks if the OVP fault is observed. Based on this information
> it detects and enables the valid sink configuration.
> Auto calibration will be triggered when the OVP fault interrupts
> are seen frequently thereby it tries to fix the sink configuration.
> 
> The auto-detection also kicks in when the connected LED string
> of the display-backlight malfunctions (because of damage) and
> requires the damaged string to be turned off to prevent the
> complete panel and/or board from being damaged.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 408 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 403 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index e87ba70..6bc627c 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -25,13 +25,23 @@
>  #define WLED_MAX_STRINGS				4
>  
>  #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_OVP_FAULT_STATUS		BIT(1)
> +

These seems to be used in both WLED 3 and 4, so please drop the 3 from
the name.

>  #define WLED3_CTRL_REG_MOD_EN				0x46
>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>  
> +#define WLED3_CTRL_REG_FEEDBACK_CONTROL			0x48
> +
>  #define WLED3_CTRL_REG_FREQ				0x4c
>  #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
>  
> @@ -146,6 +156,7 @@ struct wled_config {
>  	bool ext_gen;
>  	bool cabc;
>  	bool external_pfet;
> +	bool auto_detection_enabled;
>  };
>  
>  struct wled {
> @@ -154,16 +165,22 @@ struct wled {
>  	struct regmap *regmap;
>  	struct mutex lock;	/* Lock to avoid race from ISR */
>  	ktime_t last_short_event;
> +	ktime_t start_ovp_fault_time;
>  	u16 ctrl_addr;
>  	u16 sink_addr;
> +	u16 auto_detection_ovp_count;
>  	u32 brightness;
>  	u32 max_brightness;
>  	u32 short_count;
> +	u32 auto_detect_count;
>  	const int *version;
>  	bool disabled_by_short;
>  	bool has_short_detect;
> +	int ovp_irq;
> +	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 +226,27 @@ 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_MASK) {

The way you implement this now makes this a "delayed enable_irq" worker,
as such you shouldn't need to check if the hardware is enabled here but
just blindly enable_irq() and then in module_enable() you can check the
return value of cancel_delayed_work() to know if enable_irq() has been
called.

> +		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
> +			enable_irq(wled->ovp_irq);
> +			wled->ovp_irq_disabled = false;
> +		}
> +	}
> +}
> +
>  static int wled_module_enable(struct wled *wled, int val)
>  {
>  	int rc;
> @@ -220,7 +258,20 @@ 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;
> +
> +	if (val) {
> +		schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
> +	} else {
> +		cancel_delayed_work(&wled->ovp_work);

I recommend using cancel_delayed_work_sync() to ensure that htis isn't
racing with the worker.

> +		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
> +			disable_irq(wled->ovp_irq);
> +			wled->ovp_irq_disabled = true;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static int wled3_sync_toggle(struct wled *wled)
> @@ -346,6 +397,305 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
>  	return IRQ_HANDLED;
>  }
>  
> +#define AUTO_DETECT_BRIGHTNESS		200
> +static int wled_auto_string_detection(struct wled *wled)

You don't care about the return value of this function in any of the
callers.

> +{
> +	int rc = 0, i;
> +	u32 sink_config = 0, int_sts;
> +	u8 sink_test = 0, sink_valid = 0, val;
> +
> +	/* read configured sink configuration */
> +	rc = regmap_read(wled->regmap, wled->sink_addr +
> +			 WLED4_SINK_REG_CURR_SINK, &sink_config);
> +	if (rc < 0) {
> +		pr_err("Failed to read SINK configuration rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +	/* disable the module before starting detection */
> +	rc = regmap_update_bits(wled->regmap,
> +				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
> +				WLED3_CTRL_REG_MOD_EN_MASK, 0);
> +	if (rc < 0) {
> +		pr_err("Failed to disable WLED module rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +	/* set low brightness across all sinks */
> +	rc = wled4_set_brightness(wled, AUTO_DETECT_BRIGHTNESS);
> +	if (rc < 0) {
> +		pr_err("Failed to set brightness for auto detection rc=%d\n",
> +		       rc);
> +		goto failed_detect;
> +	}
> +
> +	if (wled->cfg.cabc) {
> +		for (i = 0; i < wled->cfg.num_strings; i++) {
> +			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
> +						WLED4_SINK_REG_STR_CABC(i),
> +						WLED4_SINK_REG_STR_CABC_MASK,
> +						0);
> +			if (rc < 0)
> +				goto failed_detect;
> +		}
> +	}
> +
> +	/* disable all sinks */
> +	rc = regmap_write(wled->regmap,
> +			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK, 0);
> +	if (rc < 0) {
> +		pr_err("Failed to disable all sinks rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +	/* iterate through the strings one by one */
> +	for (i = 0; i < wled->cfg.num_strings; i++) {
> +		sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i));
> +
> +		/* Enable feedback control */
> +		rc = regmap_write(wled->regmap, wled->ctrl_addr +
> +				  WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1);
> +		if (rc < 0) {
> +			pr_err("Failed to enable feedback for SINK %d rc = %d\n",
> +			       i + 1, rc);
> +			goto failed_detect;
> +		}
> +
> +		/* enable the sink */
> +		rc = regmap_write(wled->regmap, wled->sink_addr +
> +				  WLED4_SINK_REG_CURR_SINK, sink_test);
> +		if (rc < 0) {
> +			pr_err("Failed to configure SINK %d rc=%d\n", i + 1,
> +			       rc);
> +			goto failed_detect;
> +		}
> +
> +		/* Enable the module */
> +		rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> +					WLED3_CTRL_REG_MOD_EN,
> +					WLED3_CTRL_REG_MOD_EN_MASK,
> +					WLED3_CTRL_REG_MOD_EN_MASK);
> +		if (rc < 0) {
> +			pr_err("Failed to enable WLED module rc=%d\n", rc);
> +			goto failed_detect;
> +		}
> +
> +		usleep_range(WLED_SOFT_START_DLY_US,
> +			     WLED_SOFT_START_DLY_US + 1000);
> +
> +		rc = regmap_read(wled->regmap, wled->ctrl_addr +
> +				 WLED3_CTRL_REG_INT_RT_STS, &int_sts);
> +		if (rc < 0) {
> +			pr_err("Error in reading WLED3_CTRL_INT_RT_STS rc=%d\n",
> +			       rc);

You should probably do something about the state of the hardware if it
fail here.

> +			goto failed_detect;
> +		}
> +
> +		if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
> +			pr_debug("WLED OVP fault detected with SINK %d\n",
> +				 i + 1);
> +		else
> +			sink_valid |= sink_test;
> +
> +		/* Disable the module */
> +		rc = regmap_update_bits(wled->regmap,
> +					wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
> +					WLED3_CTRL_REG_MOD_EN_MASK, 0);
> +		if (rc < 0) {
> +			pr_err("Failed to disable WLED module rc=%d\n", rc);
> +			goto failed_detect;
> +		}
> +	}
> +
> +	if (sink_valid = sink_config) {
> +		pr_debug("WLED auto-detection complete, default sink-config=%x OK!\n",
> +			 sink_config);

It's not the "default" sink config we're using, it's whatever was
configured before this function was called.

> +	} else {
> +		pr_warn("Invalid WLED default sink config=%x changing it to=%x\n",

Use dev_warn(), and as it's not the "default sink config" that's invalid
write something like "New WLED string configuration found: %x". Perhaps
you can print it using %*pbl ? 

> +			sink_config, sink_valid);
> +		sink_config = sink_valid;
> +	}
> +
> +	if (!sink_config) {
> +		pr_err("No valid WLED sinks found\n");

dev_err() and move this above the other prints, there's no reason to
first say that you found an "invalid WLED default" and then say that you
didn't findd a valid config.

> +		wled->disabled_by_short = true;
> +		goto failed_detect;
> +	}
> +
> +	/* write the new sink configuration */
> +	rc = regmap_write(wled->regmap,
> +			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
> +			  sink_config);
> +	if (rc < 0) {
> +		pr_err("Failed to reconfigure the default sink rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +	/* Enable valid sinks */
> +	for (i = 0; i < wled->cfg.num_strings; i++) {
> +		if (wled->cfg.cabc) {
> +			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
> +						WLED4_SINK_REG_STR_CABC(i),
> +						WLED4_SINK_REG_STR_CABC_MASK,
> +						WLED4_SINK_REG_STR_CABC_MASK);
> +			if (rc < 0)
> +				goto failed_detect;
> +		}
> +
> +		if (sink_config & BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
> +			val = WLED4_SINK_REG_STR_MOD_MASK;
> +		else
> +			val = 0x0; /* disable modulator_en for unused sink */
> +
> +		rc = regmap_write(wled->regmap, wled->sink_addr +
> +				  WLED4_SINK_REG_STR_MOD_EN(i), val);
> +		if (rc < 0) {
> +			pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc);
> +			goto failed_detect;
> +		}
> +	}
> +
> +	/* restore the feedback setting */
> +	rc = regmap_write(wled->regmap,
> +			  wled->ctrl_addr + WLED3_CTRL_REG_FEEDBACK_CONTROL, 0);
> +	if (rc < 0) {
> +		pr_err("Failed to restore feedback setting rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +	/* restore  brightness */

Extra space

> +	rc = wled4_set_brightness(wled, wled->brightness);
> +	if (rc < 0) {
> +		pr_err("Failed to set brightness after auto detection rc=%d\n",
> +		       rc);
> +		goto failed_detect;
> +	}
> +
> +	rc = regmap_update_bits(wled->regmap,
> +				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
> +				WLED3_CTRL_REG_MOD_EN_MASK,
> +				WLED3_CTRL_REG_MOD_EN_MASK);
> +	if (rc < 0) {
> +		pr_err("Failed to enable WLED module rc=%d\n", rc);
> +		goto failed_detect;
> +	}
> +
> +failed_detect:
> +	return rc;
> +}
> +
> +#define WLED_AUTO_DETECT_OVP_COUNT		5
> +#define WLED_AUTO_DETECT_CNT_DLY_US		USEC_PER_SEC
> +static bool wled_auto_detection_required(struct wled *wled)
> +{
> +	s64 elapsed_time_us;
> +
> +	if (*wled->version = WLED_PM8941 || !wled->cfg.auto_detection_enabled)

Ensure that auto_detection_enabled can't be enabled for pm8941, so that
you don't need to check the first part.

> +		return false;
> +
> +	/*
> +	 * Check if the OVP fault was an occasional one
> +	 * or if it's firing continuously, the latter qualifies
> +	 * for an auto-detection check.
> +	 */
> +	if (!wled->auto_detection_ovp_count) {
> +		wled->start_ovp_fault_time = ktime_get();
> +		wled->auto_detection_ovp_count++;
> +	} else {
> +		elapsed_time_us = ktime_us_delta(ktime_get(),
> +						 wled->start_ovp_fault_time);
> +		if (elapsed_time_us > WLED_AUTO_DETECT_CNT_DLY_US)
> +			wled->auto_detection_ovp_count = 0;
> +		else
> +			wled->auto_detection_ovp_count++;
> +
> +		if (wled->auto_detection_ovp_count >> +				WLED_AUTO_DETECT_OVP_COUNT) {
> +			wled->auto_detection_ovp_count = 0;
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +static int wled_auto_detection_at_init(struct wled *wled)
> +{
> +	int rc;
> +	u32 fault_status = 0, rt_status = 0;

No need to initialize these, they will be filled out if regmap_read
return successfully.

> +
> +	if (!wled->cfg.auto_detection_enabled)
> +		return 0;
> +
[..]
> @@ -836,6 +1192,42 @@ 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;

No need to initialize rc here.

> +	u32 val;
> +
> +	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 = 0;
> +		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)) {

If rc isn't 0 we returned above.

> +		disable_irq(wled->ovp_irq);
> +		wled->ovp_irq_disabled = true;
> +	}
> +
> +	return rc;

As such, it is always 0 here.

> +}
> +

Regards,
Bjorn

Re: [PATCH V3 7/7] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2018-06-20 11:52, Bjorn Andersson wrote:
> On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
> 
>> The auto string detection algorithm checks if the current WLED
>> sink configuration is valid. It tries enabling every sink and
>> checks if the OVP fault is observed. Based on this information
>> it detects and enables the valid sink configuration.
>> Auto calibration will be triggered when the OVP fault interrupts
>> are seen frequently thereby it tries to fix the sink configuration.
>> 
>> The auto-detection also kicks in when the connected LED string
>> of the display-backlight malfunctions (because of damage) and
>> requires the damaged string to be turned off to prevent the
>> complete panel and/or board from being damaged.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/video/backlight/qcom-wled.c | 408 
>> +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 403 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index e87ba70..6bc627c 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -25,13 +25,23 @@
>>  #define WLED_MAX_STRINGS				4
>> 
>>  #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_OVP_FAULT_STATUS		BIT(1)
>> +
> 
> These seems to be used in both WLED 3 and 4, so please drop the 3 from
> the name.
> 
Sure. I will change it in the next series.
>>  #define WLED3_CTRL_REG_MOD_EN				0x46
>>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>> 
>> +#define WLED3_CTRL_REG_FEEDBACK_CONTROL			0x48
>> +
>>  #define WLED3_CTRL_REG_FREQ				0x4c
>>  #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
>> 
>> @@ -146,6 +156,7 @@ struct wled_config {
>>  	bool ext_gen;
>>  	bool cabc;
>>  	bool external_pfet;
>> +	bool auto_detection_enabled;
>>  };
>> 
>>  struct wled {
>> @@ -154,16 +165,22 @@ struct wled {
>>  	struct regmap *regmap;
>>  	struct mutex lock;	/* Lock to avoid race from ISR */
>>  	ktime_t last_short_event;
>> +	ktime_t start_ovp_fault_time;
>>  	u16 ctrl_addr;
>>  	u16 sink_addr;
>> +	u16 auto_detection_ovp_count;
>>  	u32 brightness;
>>  	u32 max_brightness;
>>  	u32 short_count;
>> +	u32 auto_detect_count;
>>  	const int *version;
>>  	bool disabled_by_short;
>>  	bool has_short_detect;
>> +	int ovp_irq;
>> +	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 +226,27 @@ 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_MASK) {
> 
> The way you implement this now makes this a "delayed enable_irq" 
> worker,
> as such you shouldn't need to check if the hardware is enabled here but
> just blindly enable_irq() and then in module_enable() you can check the
> return value of cancel_delayed_work() to know if enable_irq() has been
> called.
> 
Ok. I will modify as per your suggestion in the next series.
>> +		if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
>> +			enable_irq(wled->ovp_irq);
>> +			wled->ovp_irq_disabled = false;
>> +		}
>> +	}
>> +}
>> +
>>  static int wled_module_enable(struct wled *wled, int val)
>>  {
>>  	int rc;
>> @@ -220,7 +258,20 @@ 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;
>> +
>> +	if (val) {
>> +		schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
>> +	} else {
>> +		cancel_delayed_work(&wled->ovp_work);
> 
> I recommend using cancel_delayed_work_sync() to ensure that htis isn't
> racing with the worker.
> 
Sure. I will modify it in next series.
>> +		if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
>> +			disable_irq(wled->ovp_irq);
>> +			wled->ovp_irq_disabled = true;
>> +		}
>> +	}
>> +
>> +	return 0;
>>  }
>> 
>>  static int wled3_sync_toggle(struct wled *wled)
>> @@ -346,6 +397,305 @@ static irqreturn_t wled_short_irq_handler(int 
>> irq, void *_wled)
>>  	return IRQ_HANDLED;
>>  }
>> 
>> +#define AUTO_DETECT_BRIGHTNESS		200
>> +static int wled_auto_string_detection(struct wled *wled)
> 
> You don't care about the return value of this function in any of the
> callers.
> 
I will make it void in the next series.
>> +{
>> +	int rc = 0, i;
>> +	u32 sink_config = 0, int_sts;
>> +	u8 sink_test = 0, sink_valid = 0, val;
>> +
>> +	/* read configured sink configuration */
>> +	rc = regmap_read(wled->regmap, wled->sink_addr +
>> +			 WLED4_SINK_REG_CURR_SINK, &sink_config);
>> +	if (rc < 0) {
>> +		pr_err("Failed to read SINK configuration rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* disable the module before starting detection */
>> +	rc = regmap_update_bits(wled->regmap,
>> +				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
>> +				WLED3_CTRL_REG_MOD_EN_MASK, 0);
>> +	if (rc < 0) {
>> +		pr_err("Failed to disable WLED module rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* set low brightness across all sinks */
>> +	rc = wled4_set_brightness(wled, AUTO_DETECT_BRIGHTNESS);
>> +	if (rc < 0) {
>> +		pr_err("Failed to set brightness for auto detection rc=%d\n",
>> +		       rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	if (wled->cfg.cabc) {
>> +		for (i = 0; i < wled->cfg.num_strings; i++) {
>> +			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
>> +						WLED4_SINK_REG_STR_CABC(i),
>> +						WLED4_SINK_REG_STR_CABC_MASK,
>> +						0);
>> +			if (rc < 0)
>> +				goto failed_detect;
>> +		}
>> +	}
>> +
>> +	/* disable all sinks */
>> +	rc = regmap_write(wled->regmap,
>> +			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK, 0);
>> +	if (rc < 0) {
>> +		pr_err("Failed to disable all sinks rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* iterate through the strings one by one */
>> +	for (i = 0; i < wled->cfg.num_strings; i++) {
>> +		sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i));
>> +
>> +		/* Enable feedback control */
>> +		rc = regmap_write(wled->regmap, wled->ctrl_addr +
>> +				  WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1);
>> +		if (rc < 0) {
>> +			pr_err("Failed to enable feedback for SINK %d rc = %d\n",
>> +			       i + 1, rc);
>> +			goto failed_detect;
>> +		}
>> +
>> +		/* enable the sink */
>> +		rc = regmap_write(wled->regmap, wled->sink_addr +
>> +				  WLED4_SINK_REG_CURR_SINK, sink_test);
>> +		if (rc < 0) {
>> +			pr_err("Failed to configure SINK %d rc=%d\n", i + 1,
>> +			       rc);
>> +			goto failed_detect;
>> +		}
>> +
>> +		/* Enable the module */
>> +		rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>> +					WLED3_CTRL_REG_MOD_EN,
>> +					WLED3_CTRL_REG_MOD_EN_MASK,
>> +					WLED3_CTRL_REG_MOD_EN_MASK);
>> +		if (rc < 0) {
>> +			pr_err("Failed to enable WLED module rc=%d\n", rc);
>> +			goto failed_detect;
>> +		}
>> +
>> +		usleep_range(WLED_SOFT_START_DLY_US,
>> +			     WLED_SOFT_START_DLY_US + 1000);
>> +
>> +		rc = regmap_read(wled->regmap, wled->ctrl_addr +
>> +				 WLED3_CTRL_REG_INT_RT_STS, &int_sts);
>> +		if (rc < 0) {
>> +			pr_err("Error in reading WLED3_CTRL_INT_RT_STS rc=%d\n",
>> +			       rc);
> 
> You should probably do something about the state of the hardware if it
> fail here.
> 
This is a spmi read and it is very unlikely to fail. If it fails it 
leads the complete
system to go in to bad state.
>> +			goto failed_detect;
>> +		}
>> +
>> +		if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
>> +			pr_debug("WLED OVP fault detected with SINK %d\n",
>> +				 i + 1);
>> +		else
>> +			sink_valid |= sink_test;
>> +
>> +		/* Disable the module */
>> +		rc = regmap_update_bits(wled->regmap,
>> +					wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
>> +					WLED3_CTRL_REG_MOD_EN_MASK, 0);
>> +		if (rc < 0) {
>> +			pr_err("Failed to disable WLED module rc=%d\n", rc);
>> +			goto failed_detect;
>> +		}
>> +	}
>> +
>> +	if (sink_valid = sink_config) {
>> +		pr_debug("WLED auto-detection complete, default sink-config=%x 
>> OK!\n",
>> +			 sink_config);
> 
> It's not the "default" sink config we're using, it's whatever was
> configured before this function was called.
> 
I will correct it in the next series.
>> +	} else {
>> +		pr_warn("Invalid WLED default sink config=%x changing it to=%x\n",
> 
> Use dev_warn(), and as it's not the "default sink config" that's 
> invalid
> write something like "New WLED string configuration found: %x". Perhaps
> you can print it using %*pbl ?
> 
I will modify it in the next series. I missed to modify here.
>> +			sink_config, sink_valid);
>> +		sink_config = sink_valid;
>> +	}
>> +
>> +	if (!sink_config) {
>> +		pr_err("No valid WLED sinks found\n");
> 
> dev_err() and move this above the other prints, there's no reason to
> first say that you found an "invalid WLED default" and then say that 
> you
> didn't findd a valid config.
> 
Sure. I will move it in the next series.
>> +		wled->disabled_by_short = true;
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* write the new sink configuration */
>> +	rc = regmap_write(wled->regmap,
>> +			  wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
>> +			  sink_config);
>> +	if (rc < 0) {
>> +		pr_err("Failed to reconfigure the default sink rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* Enable valid sinks */
>> +	for (i = 0; i < wled->cfg.num_strings; i++) {
>> +		if (wled->cfg.cabc) {
>> +			rc = regmap_update_bits(wled->regmap, wled->sink_addr +
>> +						WLED4_SINK_REG_STR_CABC(i),
>> +						WLED4_SINK_REG_STR_CABC_MASK,
>> +						WLED4_SINK_REG_STR_CABC_MASK);
>> +			if (rc < 0)
>> +				goto failed_detect;
>> +		}
>> +
>> +		if (sink_config & BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
>> +			val = WLED4_SINK_REG_STR_MOD_MASK;
>> +		else
>> +			val = 0x0; /* disable modulator_en for unused sink */
>> +
>> +		rc = regmap_write(wled->regmap, wled->sink_addr +
>> +				  WLED4_SINK_REG_STR_MOD_EN(i), val);
>> +		if (rc < 0) {
>> +			pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc);
>> +			goto failed_detect;
>> +		}
>> +	}
>> +
>> +	/* restore the feedback setting */
>> +	rc = regmap_write(wled->regmap,
>> +			  wled->ctrl_addr + WLED3_CTRL_REG_FEEDBACK_CONTROL, 0);
>> +	if (rc < 0) {
>> +		pr_err("Failed to restore feedback setting rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	/* restore  brightness */
> 
> Extra space
> 
Will remove it in the next series.
>> +	rc = wled4_set_brightness(wled, wled->brightness);
>> +	if (rc < 0) {
>> +		pr_err("Failed to set brightness after auto detection rc=%d\n",
>> +		       rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +	rc = regmap_update_bits(wled->regmap,
>> +				wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
>> +				WLED3_CTRL_REG_MOD_EN_MASK,
>> +				WLED3_CTRL_REG_MOD_EN_MASK);
>> +	if (rc < 0) {
>> +		pr_err("Failed to enable WLED module rc=%d\n", rc);
>> +		goto failed_detect;
>> +	}
>> +
>> +failed_detect:
>> +	return rc;
>> +}
>> +
>> +#define WLED_AUTO_DETECT_OVP_COUNT		5
>> +#define WLED_AUTO_DETECT_CNT_DLY_US		USEC_PER_SEC
>> +static bool wled_auto_detection_required(struct wled *wled)
>> +{
>> +	s64 elapsed_time_us;
>> +
>> +	if (*wled->version = WLED_PM8941 || 
>> !wled->cfg.auto_detection_enabled)
> 
> Ensure that auto_detection_enabled can't be enabled for pm8941, so that
> you don't need to check the first part.
> 
I will change it in the next series.
>> +		return false;
>> +
>> +	/*
>> +	 * Check if the OVP fault was an occasional one
>> +	 * or if it's firing continuously, the latter qualifies
>> +	 * for an auto-detection check.
>> +	 */
>> +	if (!wled->auto_detection_ovp_count) {
>> +		wled->start_ovp_fault_time = ktime_get();
>> +		wled->auto_detection_ovp_count++;
>> +	} else {
>> +		elapsed_time_us = ktime_us_delta(ktime_get(),
>> +						 wled->start_ovp_fault_time);
>> +		if (elapsed_time_us > WLED_AUTO_DETECT_CNT_DLY_US)
>> +			wled->auto_detection_ovp_count = 0;
>> +		else
>> +			wled->auto_detection_ovp_count++;
>> +
>> +		if (wled->auto_detection_ovp_count >>> +				WLED_AUTO_DETECT_OVP_COUNT) {
>> +			wled->auto_detection_ovp_count = 0;
>> +			return true;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static int wled_auto_detection_at_init(struct wled *wled)
>> +{
>> +	int rc;
>> +	u32 fault_status = 0, rt_status = 0;
> 
> No need to initialize these, they will be filled out if regmap_read
> return successfully.
> 
I will remove it in the next series.
>> +
>> +	if (!wled->cfg.auto_detection_enabled)
>> +		return 0;
>> +
> [..]
>> @@ -836,6 +1192,42 @@ 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;
> 
> No need to initialize rc here.
> 
I will remove it in the next series.
>> +	u32 val;
>> +
>> +	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 = 0;
>> +		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)) {
> 
> If rc isn't 0 we returned above.
> 
I will remove it in the next series.
>> +		disable_irq(wled->ovp_irq);
>> +		wled->ovp_irq_disabled = true;
>> +	}
>> +
>> +	return rc;
> 
> As such, it is always 0 here.
> 
I will modify it in the next series.
>> +}
>> +
> 
> 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