[PATCH V1 1/5] qcom: wled: Rename pm8941-wled.c to qcom-wled.c

[PATCH V1 1/5] 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 V1 1/5] qcom: wled: Rename pm8941-wled.c to qcom-wled.c

[Bjorn Andersson]

On Thu 03 May 02:57 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>

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 V1 1/5] qcom: wled: Rename pm8941-wled.c to qcom-wled.c

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Thu 2018-05-03 15:27:08, 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>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH V1 2/5] 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 also
configurations are different. Add support for it.

Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
---
 .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
 drivers/video/backlight/qcom-wled.c                | 749 +++++++++++++++------
 2 files changed, 696 insertions(+), 225 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
index fb39e32..0ceffa1 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
@@ -1,30 +1,129 @@
 Binding for Qualcomm Technologies, Inc. WLED driver
 
-Required properties:
-- compatible: should be "qcom,pm8941-wled"
-- reg: slave address
-
-Optional properties:
-- default-brightness: brightness value on boot, value from: 0-4095
-	default: 2048
-- label: The name of the backlight device
-- qcom,cs-out: bool; enable current sink output
-- qcom,cabc: bool; enable content adaptive backlight control
-- qcom,ext-gen: bool; use externally generated modulator signal to dim
-- qcom,current-limit: mA; per-string current limit; value from 0 to 25
-	default: 20mA
-- qcom,current-boost-limit: mA; boost current limit; one of:
-	105, 385, 525, 805, 980, 1260, 1400, 1680
-	default: 805mA
-- qcom,switching-freq: kHz; switching frequency; one of:
-	600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
-	1600, 1920, 2400, 3200, 4800, 9600,
-	default: 1600kHz
-- qcom,ovp: V; Over-voltage protection limit; one of:
-	27, 29, 32, 35
-	default: 29V
-- qcom,num-strings: #; number of led strings attached; value from 1 to 3
-	default: 2
+WLED (White Light Emitting Diode) driver is used for controlling display
+backlight that is part of PMIC on Qualcomm Technologies, Inc. reference
+platforms. The PMIC is connected to the host processor via SPMI bus.
+
+- compatible
+	Usage:        required
+	Value type:   <string>
+	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
+		      or "qcom,pm660l-wled".
+
+- reg
+	Usage:        required
+	Value type:   <prop encoded array>
+	Definition:   Base address of the WLED modules.
+
+- interrupts
+	Usage:        optional
+	Value type:   <prop encoded array>
+	Definition:   Interrupts associated with WLED. Interrupts can be
+		      specified as per the encoding listed under
+		      Documentation/devicetree/bindings/spmi/
+		      qcom,spmi-pmic-arb.txt.
+
+- interrupt-names
+	Usage:        optional
+	Value type:   <string>
+	Definition:   Interrupt names associated with the interrupts.
+		      Must be "short" and "ovp". The short circuit detection
+		      is not supported for PM8941.
+
+- label
+	Usage:        required
+	Value type:   <string>
+	Definition:   The name of the backlight device
+
+- default-brightness
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   brightness value on boot, value from: 0-4095
+		      Default: 2048
+
+- qcom,current-limit
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   uA; per-string current limit
+		      value:
+		      For pm8941: from 0 to 25000 with 5000 ua step
+				  Default 20000 uA
+		      For pmi8998: from 0 to 30000 with 5000 ua step
+				   Default 25000 uA.
+
+- qcom,current-boost-limit
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   mA; boost current limit.
+		      For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
+				 1680. Default: 805 mA
+		      For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
+				  1500. Default: 970 mA
+
+- qcom,switching-freq
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   kHz; switching frequency; one of: 600, 640, 685, 738,
+		      800, 872, 960, 1066, 1200, 1371, 1600, 1920, 2400, 3200,
+		      4800, 9600.
+		      Default: for pm8941: 1600 kHz
+			      for pmi8998: 800 kHz
+
+- qcom,ovp
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   mV; Over-voltage protection limit;
+		      For pm8941:  one of 27000, 29000, 32000, 35000
+				  Default: 29000 mV
+		      For pmi8998: one of 18100, 19600, 29600, 31100
+				  Default: 29600 mV
+
+- qcom,num-strings
+	Usage:        optional
+	Value type:   <u32>
+	Definition:   #; number of led strings attached;
+		      for pm8941: value 3.
+		      for pmi8998: value 4.
+
+- qcom,enabled-strings
+	Usage:        optional
+	Value tyoe:   <u32>
+	Definition:   Bit mask of the WLED strings. Bit 0 to 3 indicates strings
+		      0 to 3 respectively. Each string of leds are operated
+		      individually. Specify the strings using the bit mask. Any
+		      combination of led strings can be used.
+		      for pm8941: Default value is 7 (b111).
+		      for pmi8998: Default value is 15 (b1111).
+
+- qcom,cs-out
+	Usage:        optional
+	Value type:   <bool>
+	Definition:   enable current sink output.
+		      This property is supported only for PM8941.
+
+- qcom,cabc
+	Usage:        optional
+	Value type:   <bool>
+	Definition:   enable content adaptive backlight control.
+
+- qcom,ext-gen
+	Usage:        optional
+	Value type:   <bool>
+	Definition:   use externally generated modulator signal to dim.
+		      This property is supported only for PM8941.
+
+- qcom,external-pfet
+	Usage:        optional
+	Value type:   <bool>
+	Definition:   Specify if external PFET control for short circuit
+		      protection is used. This property is supported only
+		      for PMI8998.
+
+- qcom,auto-string-detection
+	Usage:        optional
+	Value type:   <bool>
+	Definition:   Enables auto-detection of the WLED string configuration.
+		      This feature is not supported for PM8941.
 
 Example:
 
@@ -34,9 +133,26 @@ pm8941-wled@d800 {
 	label = "backlight";
 
 	qcom,cs-out;
-	qcom,current-limit = <20>;
+	qcom,current-limit = <20000>;
+	qcom,current-boost-limit = <805>;
+	qcom,switching-freq = <1600>;
+	qcom,ovp = <29000>;
+	qcom,num-strings = <3>;
+	qcom,enabled-strings = <7>;
+};
+
+pmi8998-wled@d800 {
+	compatible = "qcom,pmi8998-wled";
+	reg = <0xd800 0xd900>;
+	label = "backlight";
+
+	interrupts = <3 0xd8 2 IRQ_TYPE_EDGE_RISING>,
+		     <3 0xd8 1 IRQ_TYPE_EDGE_RISING>;
+	interrupt-names = "short", "ovp";
+	qcom,current-limit = <25000>;
 	qcom,current-boost-limit = <805>;
 	qcom,switching-freq = <1600>;
-	qcom,ovp = <29>;
-	qcom,num-strings = <2>;
+	qcom,ovp = <29600>;
+	qcom,num-strings = <4>;
+	qcom,enabled-strings = <15>;
 };
diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 0b6d219..be8b8d3 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -15,253 +15,529 @@
 #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 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 PM8941_WLED_REG_MOD_EN			0x46
-#define  PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
-#define  PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
+/* Control registers */
+#define WLED3_CTRL_REG_MOD_EN				0x46
+#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			GENMASK(3, 0)
 
-#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			GENMASK(1, 0)
 
-#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			GENMASK(2, 0)
 
-#define PM8941_WLED_REG_BOOST			0x4e
-#define  PM8941_WLED_REG_BOOST_MASK		0x07
+/* sink registers */
+#define WLED3_SINK_REG_SYNC				0x47
+#define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
+#define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
+#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 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			GENMASK(7, 5)
+#define  WLED3_SINK_REG_CURR_SINK_SHFT			5
 
-/* Per-'string' registers below */
-#define PM8941_WLED_REG_STR_OFFSET		0x10
+/* WLED3 Per-'string' registers below */
+#define WLED3_SINK_REG_BRIGHT(n)			(0x40 + n)
 
-#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(n)			(0x60 + (n * 0x10))
+#define  WLED3_SINK_REG_STR_MOD_MASK			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(n)		(0x62 + (n * 0x10))
+#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK	GENMASK(4, 0)
 
-#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(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 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(n)			(0x66 + (n * 0x10))
+#define  WLED3_SINK_REG_STR_CABC_MASK			BIT(7)
 
-struct pm8941_wled_config {
-	u32 i_boost_limit;
+/* WLED4 sink specific 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;
 	u32 ovp;
 	u32 switch_freq;
 	u32 num_strings;
-	u32 i_limit;
+	u32 string_i_limit;
+	u32 enabled_strings;
 	bool cs_out_en;
 	bool ext_gen;
-	bool cabc_en;
+	bool cabc;
 };
 
-struct pm8941_wled {
+struct wled {
 	const char *name;
+	struct device *dev;
 	struct regmap *regmap;
-	u16 addr;
-
-	struct pm8941_wled_config cfg;
+	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 pm8941_wled_update_status(struct backlight_device *bl)
+static int wled3_set_brightness(struct wled *wled, u16 brightness)
 {
-	struct pm8941_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 = PM8941_WLED_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 + PM8941_WLED_REG_MOD_EN,
-			PM8941_WLED_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 + PM8941_WLED_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 + PM8941_WLED_REG_SYNC,
-			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_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 + PM8941_WLED_REG_SYNC,
-			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
+				wled->ctrl_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 wled4_sync_toggle(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);
-	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 + PM8941_WLED_REG_BOOST,
-			PM8941_WLED_REG_BOOST_MASK, wled->cfg.i_boost_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 enabled_strings = wled->cfg.enabled_strings;
+	u8 sink_en = 0;
+	int rc, i;
+
+	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 + PM8941_WLED_REG_FREQ,
-			PM8941_WLED_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)
-				<< PM8941_WLED_REG_SINK_SHFT;
-
-		rc = regmap_update_bits(wled->regmap,
-				wled->addr + PM8941_WLED_REG_SINK,
-				PM8941_WLED_REG_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 + PM8941_WLED_REG_STR_OFFSET * i;
+		if (!(enabled_strings & BIT(i)))
+			continue;
 
-		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 = wled->ctrl_addr + WLED3_SINK_REG_STR_MOD_EN(i);
+		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 + PM8941_WLED_REG_STR_MOD_SRC_BASE,
-					PM8941_WLED_REG_STR_MOD_SRC_MASK,
-					PM8941_WLED_REG_STR_MOD_SRC_EXT);
+			addr = wled->ctrl_addr + WLED3_SINK_REG_STR_MOD_SRC(i);
+			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 + PM8941_WLED_REG_STR_SCALE_BASE,
-				PM8941_WLED_REG_STR_SCALE_MASK,
-				wled->cfg.i_limit);
+		addr = wled->ctrl_addr + WLED3_SINK_REG_STR_FULL_SCALE_CURR(i);
+		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 + PM8941_WLED_REG_STR_CABC_BASE,
-				PM8941_WLED_REG_STR_CABC_MASK,
-				wled->cfg.cabc_en ?
-					PM8941_WLED_REG_STR_CABC_EN : 0);
+		addr = wled->ctrl_addr + WLED3_SINK_REG_STR_CABC(i);
+		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(i + 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;
 }
 
-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,
+	.num_strings = 3,
 	.switch_freq = 5,
-	.num_strings = 0,
+	.enabled_strings = 7,
 	.cs_out_en = false,
 	.ext_gen = false,
-	.cabc_en = false,
+	.cabc = false,
 };
 
-struct pm8941_wled_var_cfg {
-	const u32 *values;
-	u32 (*fn)(u32);
-	int size;
+static int wled4_setup(struct wled *wled)
+{
+	int rc, temp, i;
+	u16 addr;
+	u8 sink_en = 0;
+	u8 enabled_strings = wled->cfg.enabled_strings;
+
+	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;
+
+	/* Per sink/string configuration */
+	for (i = 0; i < wled->cfg.num_strings; i++) {
+		if (!(enabled_strings & BIT(i)))
+			continue;
+
+		addr = wled->sink_addr +
+				WLED4_SINK_REG_STR_MOD_EN(i);
+		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(i);
+		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(i);
+		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;
+
+		temp = i + WLED4_SINK_REG_CURR_SINK_SHFT;
+		sink_en |= 1 << temp;
+	}
+
+	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,
+	.enabled_strings = 0xf,
+	.cabc = false,
 };
 
-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[] = {
-	35, 32, 29, 27,
+static const u32 wled4_boost_i_limit_values[] = {
+	105, 280, 450, 620, 970, 1150, 1300, 1500,
 };
 
-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 wled4_boost_i_limit_cfg = {
+	.values = wled4_boost_i_limit_values,
+	.size = ARRAY_SIZE(wled4_boost_i_limit_values),
 };
 
-static u32 pm8941_wled_num_strings_values_fn(u32 idx)
+static const u32 wled3_ovp_values[] = {
+	35000, 32000, 29000, 27000,
+};
+
+static const struct wled_var_cfg wled3_ovp_cfg = {
+	.values = wled3_ovp_values,
+	.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;
 }
 
-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 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));
 }
 
-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 = {
-	.size = 26,
+static const u32 wled3_string_i_limit_values[] = {
+	0, 2500, 5000, 7500, 10000, 12500, 15000, 17500, 20000,
+	22500, 25000,
 };
 
-static u32 pm8941_wled_values(const struct pm8941_wled_var_cfg *cfg, u32 idx)
+static const struct wled_var_cfg wled3_string_i_limit_cfg = {
+	.values = wled3_string_i_limit_values,
+	.size = ARRAY_SIZE(wled3_string_i_limit_values),
+};
+
+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;
@@ -272,68 +548,124 @@ 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 pm8941_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 pm8941_wled_var_cfg *cfg;
-	} u32_opts[] = {
+	struct wled_config *cfg = &wled->cfg;
+	struct device *dev = wled->dev;
+	const __be32 *prop_addr;
+	u32 size, val, c;
+	int rc, i, j;
+
+	const struct wled_u32_opts *u32_opts = NULL;
+	const struct wled_u32_opts wled3_opts[] = {
+		{
+			.name = "qcom,current-boost-limit",
+			.val_ptr = &cfg->boost_i_limit,
+			.cfg = &wled3_boost_i_limit_cfg,
+		},
+		{
+			.name = "qcom,current-limit",
+			.val_ptr = &cfg->string_i_limit,
+			.cfg = &wled3_string_i_limit_cfg,
+		},
+		{
+			.name = "qcom,ovp",
+			.val_ptr = &cfg->ovp,
+			.cfg = &wled3_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 = &wled3_num_strings_cfg,
+		},
+		{
+			.name = "qcom,enabled-strings",
+			.val_ptr = &cfg->enabled_strings,
+			.cfg = &wled3_string_cfg,
+		},
+	};
+
+	const struct wled_u32_opts wled4_opts[] = {
 		{
-			"qcom,current-boost-limit",
-			&cfg->i_boost_limit,
-			.cfg = &pm8941_wled_i_boost_limit_cfg,
+			.name = "qcom,current-boost-limit",
+			.val_ptr = &cfg->boost_i_limit,
+			.cfg = &wled4_boost_i_limit_cfg,
 		},
 		{
-			"qcom,current-limit",
-			&cfg->i_limit,
-			.cfg = &pm8941_wled_i_limit_cfg,
+			.name = "qcom,current-limit",
+			.val_ptr = &cfg->string_i_limit,
+			.cfg = &wled4_string_i_limit_cfg,
 		},
 		{
-			"qcom,ovp",
-			&cfg->ovp,
-			.cfg = &pm8941_wled_ovp_cfg,
+			.name = "qcom,ovp",
+			.val_ptr = &cfg->ovp,
+			.cfg = &wled4_ovp_cfg,
 		},
 		{
-			"qcom,switching-freq",
-			&cfg->switch_freq,
-			.cfg = &pm8941_wled_switch_freq_cfg,
+			.name = "qcom,switching-freq",
+			.val_ptr = &cfg->switch_freq,
+			.cfg = &wled3_switch_freq_cfg,
 		},
 		{
-			"qcom,num-strings",
-			&cfg->num_strings,
-			.cfg = &pm8941_wled_num_strings_cfg,
+			.name = "qcom,num-strings",
+			.val_ptr = &cfg->num_strings,
+			.cfg = &wled4_num_strings_cfg,
 		},
+		{
+			.name = "qcom,enabled-strings",
+			.val_ptr = &cfg->enabled_strings,
+			.cfg = &wled4_string_cfg,
+		},
+
 	};
-	const struct {
-		const char *name;
-		bool *val_ptr;
-	} bool_opts[] = {
+
+	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 = pm8941_wled_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;
@@ -344,12 +676,15 @@ 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 = 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);
@@ -366,15 +701,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;
@@ -390,43 +725,63 @@ static int pm8941_wled_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	wled->regmap = regmap;
+	wled->dev = &pdev->dev;
 
-	rc = pm8941_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 = pm8941_wled_setup(wled);
+	rc = wled_configure(wled);
 	if (rc)
 		return rc;
 
-	val = PM8941_WLED_DEFAULT_BRIGHTNESS;
+	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);
 
 	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[] = {
-	{ .compatible = "qcom,pm8941-wled" },
+static const struct of_device_id wled_match_table[] = {
+	{ .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, 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 V1 2/5] backlight: qcom-wled: Add support for WLED4 peripheral

[Bjorn Andersson]

On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:

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

Several things are going on in this patch, it needs to be split to
not hide the functional changes from the structural/renames.

> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>  drivers/video/backlight/qcom-wled.c                | 749 +++++++++++++++------
>  2 files changed, 696 insertions(+), 225 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> index fb39e32..0ceffa1 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> @@ -1,30 +1,129 @@
>  Binding for Qualcomm Technologies, Inc. WLED driver
>  
> -Required properties:
> -- compatible: should be "qcom,pm8941-wled"
> -- reg: slave address
> -
> -Optional properties:
> -- default-brightness: brightness value on boot, value from: 0-4095
> -	default: 2048
> -- label: The name of the backlight device
> -- qcom,cs-out: bool; enable current sink output
> -- qcom,cabc: bool; enable content adaptive backlight control
> -- qcom,ext-gen: bool; use externally generated modulator signal to dim
> -- qcom,current-limit: mA; per-string current limit; value from 0 to 25
> -	default: 20mA
> -- qcom,current-boost-limit: mA; boost current limit; one of:
> -	105, 385, 525, 805, 980, 1260, 1400, 1680
> -	default: 805mA
> -- qcom,switching-freq: kHz; switching frequency; one of:
> -	600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
> -	1600, 1920, 2400, 3200, 4800, 9600,
> -	default: 1600kHz
> -- qcom,ovp: V; Over-voltage protection limit; one of:
> -	27, 29, 32, 35
> -	default: 29V
> -- qcom,num-strings: #; number of led strings attached; value from 1 to 3
> -	default: 2
> +WLED (White Light Emitting Diode) driver is used for controlling display
> +backlight that is part of PMIC on Qualcomm Technologies, Inc. reference
> +platforms. The PMIC is connected to the host processor via SPMI bus.
> +
> +- compatible
> +	Usage:        required
> +	Value type:   <string>
> +	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
> +		      or "qcom,pm660l-wled".

Better written as

		should be one of:
		      	X
		      	Y
		      	Z

> +
> +- reg
> +	Usage:        required
> +	Value type:   <prop encoded array>
> +	Definition:   Base address of the WLED modules.
> +
> +- interrupts
> +	Usage:        optional
> +	Value type:   <prop encoded array>
> +	Definition:   Interrupts associated with WLED. Interrupts can be
> +		      specified as per the encoding listed under
> +		      Documentation/devicetree/bindings/spmi/
> +		      qcom,spmi-pmic-arb.txt.

Better to describe that this should be the "short" and "ovp" interrupts
in this property than in the interrupt-names.

> +
> +- interrupt-names
> +	Usage:        optional
> +	Value type:   <string>
> +	Definition:   Interrupt names associated with the interrupts.
> +		      Must be "short" and "ovp". The short circuit detection
> +		      is not supported for PM8941.
> +
> +- label
> +	Usage:        required
> +	Value type:   <string>
> +	Definition:   The name of the backlight device
> +
> +- default-brightness
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   brightness value on boot, value from: 0-4095
> +		      Default: 2048
> +
> +- qcom,current-limit
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   uA; per-string current limit

You can't change unit on an existing property, that breaks any existing
dts using the qcom,pm8941-wled compatible.

> +		      value:
> +		      For pm8941: from 0 to 25000 with 5000 ua step
> +				  Default 20000 uA
> +		      For pmi8998: from 0 to 30000 with 5000 ua step
> +				   Default 25000 uA.

These values could be described just as well in mA, so keep the original
unit - in particular since the boot-limit is in mA...

> +
> +- qcom,current-boost-limit
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   mA; boost current limit.
> +		      For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
> +				 1680. Default: 805 mA
> +		      For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
> +				  1500. Default: 970 mA
> +
> +- qcom,switching-freq
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   kHz; switching frequency; one of: 600, 640, 685, 738,
> +		      800, 872, 960, 1066, 1200, 1371, 1600, 1920, 2400, 3200,
> +		      4800, 9600.
> +		      Default: for pm8941: 1600 kHz
> +			      for pmi8998: 800 kHz
> +
> +- qcom,ovp
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   mV; Over-voltage protection limit;

The existing users of qcom,pm8941-wled depends on this being in V, so
you can't change the unit. I suggest that you add a new "qcom,ovp-mv"
property and make the driver fall back to looking for qcom,ovp if that
isn't specified.

PS. This is a very good example of why it is a good idea to not
restructure and make changes at the same time - I almost missed this.

> +		      For pm8941:  one of 27000, 29000, 32000, 35000
> +				  Default: 29000 mV
> +		      For pmi8998: one of 18100, 19600, 29600, 31100
> +				  Default: 29600 mV
> +
> +- qcom,num-strings
> +	Usage:        optional
> +	Value type:   <u32>
> +	Definition:   #; number of led strings attached;
> +		      for pm8941: value 3.
> +		      for pmi8998: value 4.

This was the number of strings actually used, so you can't turn this
into max number of strings supported. As we have different compatibles
for the different pmics this can be hard coded in the driver, based on
compatible, and qcom,num-strings can be kept as a special case of
qcom,enabled-strings (enabling string 0 through N).

> +
> +- qcom,enabled-strings
> +	Usage:        optional
> +	Value tyoe:   <u32>
> +	Definition:   Bit mask of the WLED strings. Bit 0 to 3 indicates strings
> +		      0 to 3 respectively. Each string of leds are operated
> +		      individually. Specify the strings using the bit mask. Any
> +		      combination of led strings can be used.
> +		      for pm8941: Default value is 7 (b111).
> +		      for pmi8998: Default value is 15 (b1111).

I think it's better if you describe the enabled strings in an array, as
it makes the dts easier to read and write for humans.

Also I'm uncertain about the validity of these defaults, as it's not
that the defaults are 7 and 15 it's that if this property is not
specified all strings will be enabled and the auto calibration will kick
in to figure out the proper configuration.

It would be better to describe this as "absence of this property will
trigger auto calibration".

> +
> +- qcom,cs-out
> +	Usage:        optional
> +	Value type:   <bool>
> +	Definition:   enable current sink output.
> +		      This property is supported only for PM8941.
> +
> +- qcom,cabc
> +	Usage:        optional
> +	Value type:   <bool>
> +	Definition:   enable content adaptive backlight control.
> +
> +- qcom,ext-gen
> +	Usage:        optional
> +	Value type:   <bool>
> +	Definition:   use externally generated modulator signal to dim.
> +		      This property is supported only for PM8941.
> +
> +- qcom,external-pfet
> +	Usage:        optional
> +	Value type:   <bool>
> +	Definition:   Specify if external PFET control for short circuit
> +		      protection is used. This property is supported only
> +		      for PMI8998.
> +
> +- qcom,auto-string-detection
> +	Usage:        optional
> +	Value type:   <bool>
> +	Definition:   Enables auto-detection of the WLED string configuration.
> +		      This feature is not supported for PM8941.

I don't like the idea of having the developer specifying
qcom,enabled-strings and then qcom,auto-string-detection just in case. I
think it's better to trust the developer when qcom,enabled-strings is
specified and if not use auto detection.

>  
>  Example:
>  
> @@ -34,9 +133,26 @@ pm8941-wled@d800 {
>  	label = "backlight";
>  
>  	qcom,cs-out;
> -	qcom,current-limit = <20>;
> +	qcom,current-limit = <20000>;
> +	qcom,current-boost-limit = <805>;
> +	qcom,switching-freq = <1600>;
> +	qcom,ovp = <29000>;
> +	qcom,num-strings = <3>;
> +	qcom,enabled-strings = <7>;

Having to change the existing example shows that you made non-backwards
compatible changes.

> +};
> +
> +pmi8998-wled@d800 {
> +	compatible = "qcom,pmi8998-wled";
> +	reg = <0xd800 0xd900>;
> +	label = "backlight";
> +
> +	interrupts = <3 0xd8 2 IRQ_TYPE_EDGE_RISING>,
> +		     <3 0xd8 1 IRQ_TYPE_EDGE_RISING>;
> +	interrupt-names = "short", "ovp";
> +	qcom,current-limit = <25000>;
>  	qcom,current-boost-limit = <805>;
>  	qcom,switching-freq = <1600>;
> -	qcom,ovp = <29>;
> -	qcom,num-strings = <2>;
> +	qcom,ovp = <29600>;
> +	qcom,num-strings = <4>;
> +	qcom,enabled-strings = <15>;
>  };
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 0b6d219..be8b8d3 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -15,253 +15,529 @@
>  #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 PM8941_WLED_DEFAULT_BRIGHTNESS		2048
> +#define WLED_DEFAULT_BRIGHTNESS				2048

These renames are good, but needs to go in a separate commit (either
patch 1 or a new one), the current approach makes it impossible to
review the rest of this patch.


So, as the DT change is substantial that should be split in one patch
that change the structure, then one that adds the new properties, one
patch that renames pm8941 -> wled3 and finally one that adds wled4
support.

Regards,
Bjorn

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

[kgunda]

On 2018-05-07 21:50, Bjorn Andersson wrote:
> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> 
>> WLED4 peripheral is present on some PMICs like pmi8998
>> and pm660l. It has a different register map and also
>> configurations are different. Add support for it.
>> 
> 
> Several things are going on in this patch, it needs to be split to
> not hide the functional changes from the structural/renames.
> 
Ok. I will split it in the next series.
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>>  drivers/video/backlight/qcom-wled.c                | 749 
>> +++++++++++++++------
>>  2 files changed, 696 insertions(+), 225 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> index fb39e32..0ceffa1 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> @@ -1,30 +1,129 @@
>>  Binding for Qualcomm Technologies, Inc. WLED driver
>> 
>> -Required properties:
>> -- compatible: should be "qcom,pm8941-wled"
>> -- reg: slave address
>> -
>> -Optional properties:
>> -- default-brightness: brightness value on boot, value from: 0-4095
>> -	default: 2048
>> -- label: The name of the backlight device
>> -- qcom,cs-out: bool; enable current sink output
>> -- qcom,cabc: bool; enable content adaptive backlight control
>> -- qcom,ext-gen: bool; use externally generated modulator signal to 
>> dim
>> -- qcom,current-limit: mA; per-string current limit; value from 0 to 
>> 25
>> -	default: 20mA
>> -- qcom,current-boost-limit: mA; boost current limit; one of:
>> -	105, 385, 525, 805, 980, 1260, 1400, 1680
>> -	default: 805mA
>> -- qcom,switching-freq: kHz; switching frequency; one of:
>> -	600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
>> -	1600, 1920, 2400, 3200, 4800, 9600,
>> -	default: 1600kHz
>> -- qcom,ovp: V; Over-voltage protection limit; one of:
>> -	27, 29, 32, 35
>> -	default: 29V
>> -- qcom,num-strings: #; number of led strings attached; value from 1 
>> to 3
>> -	default: 2
>> +WLED (White Light Emitting Diode) driver is used for controlling 
>> display
>> +backlight that is part of PMIC on Qualcomm Technologies, Inc. 
>> reference
>> +platforms. The PMIC is connected to the host processor via SPMI bus.
>> +
>> +- compatible
>> +	Usage:        required
>> +	Value type:   <string>
>> +	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
>> +		      or "qcom,pm660l-wled".
> 
> Better written as
> 
> 		should be one of:
> 		      	X
> 		      	Y
> 		      	Z
> 
Will do it in the next series.
>> +
>> +- reg
>> +	Usage:        required
>> +	Value type:   <prop encoded array>
>> +	Definition:   Base address of the WLED modules.
>> +
>> +- interrupts
>> +	Usage:        optional
>> +	Value type:   <prop encoded array>
>> +	Definition:   Interrupts associated with WLED. Interrupts can be
>> +		      specified as per the encoding listed under
>> +		      Documentation/devicetree/bindings/spmi/
>> +		      qcom,spmi-pmic-arb.txt.
> 
> Better to describe that this should be the "short" and "ovp" interrupts
> in this property than in the interrupt-names.
> 
Ok. I will do it in the next series.
>> +
>> +- interrupt-names
>> +	Usage:        optional
>> +	Value type:   <string>
>> +	Definition:   Interrupt names associated with the interrupts.
>> +		      Must be "short" and "ovp". The short circuit detection
>> +		      is not supported for PM8941.
>> +
>> +- label
>> +	Usage:        required
>> +	Value type:   <string>
>> +	Definition:   The name of the backlight device
>> +
>> +- default-brightness
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   brightness value on boot, value from: 0-4095
>> +		      Default: 2048
>> +
>> +- qcom,current-limit
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   uA; per-string current limit
> 
> You can't change unit on an existing property, that breaks any existing
> dts using the qcom,pm8941-wled compatible.
> 

>> +		      value:
>> +		      For pm8941: from 0 to 25000 with 5000 ua step
>> +				  Default 20000 uA
>> +		      For pmi8998: from 0 to 30000 with 5000 ua step
>> +				   Default 25000 uA.
> 
> These values could be described just as well in mA, so keep the 
> original
> unit - in particular since the boot-limit is in mA...
> 
Ok. Will keep the original as is in the next series.
>> +
>> +- qcom,current-boost-limit
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   mA; boost current limit.
>> +		      For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
>> +				 1680. Default: 805 mA
>> +		      For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
>> +				  1500. Default: 970 mA
>> +
>> +- qcom,switching-freq
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   kHz; switching frequency; one of: 600, 640, 685, 738,
>> +		      800, 872, 960, 1066, 1200, 1371, 1600, 1920, 2400, 3200,
>> +		      4800, 9600.
>> +		      Default: for pm8941: 1600 kHz
>> +			      for pmi8998: 800 kHz
>> +
>> +- qcom,ovp
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   mV; Over-voltage protection limit;
> 
> The existing users of qcom,pm8941-wled depends on this being in V, so
> you can't change the unit. I suggest that you add a new "qcom,ovp-mv"
> property and make the driver fall back to looking for qcom,ovp if that
> isn't specified.
> 
> PS. This is a very good example of why it is a good idea to not
> restructure and make changes at the same time - I almost missed this.
> 
Actually I have checked the current kernel and none of the properties 
are
being configured from the device tree node. Hence, i thought it is the 
right
time modify the units to mV to support the PMI8998.

You still want to have the qcom,ovp-mv, even though it is not being 
configured from
device tree ?
>> +		      For pm8941:  one of 27000, 29000, 32000, 35000
>> +				  Default: 29000 mV
>> +		      For pmi8998: one of 18100, 19600, 29600, 31100
>> +				  Default: 29600 mV
>> +
>> +- qcom,num-strings
>> +	Usage:        optional
>> +	Value type:   <u32>
>> +	Definition:   #; number of led strings attached;
>> +		      for pm8941: value 3.
>> +		      for pmi8998: value 4.
> 
> This was the number of strings actually used, so you can't turn this
> into max number of strings supported. As we have different compatibles
> for the different pmics this can be hard coded in the driver, based on
> compatible, and qcom,num-strings can be kept as a special case of
> qcom,enabled-strings (enabling string 0 through N).
> 
Ok. Actually I don't see the use of this property as the 
qcom,enabled-strings
it self is enough to know the strings to be enabled. But for backward 
compatibility
i keep it as original property.
>> +
>> +- qcom,enabled-strings
>> +	Usage:        optional
>> +	Value tyoe:   <u32>
>> +	Definition:   Bit mask of the WLED strings. Bit 0 to 3 indicates 
>> strings
>> +		      0 to 3 respectively. Each string of leds are operated
>> +		      individually. Specify the strings using the bit mask. Any
>> +		      combination of led strings can be used.
>> +		      for pm8941: Default value is 7 (b111).
>> +		      for pmi8998: Default value is 15 (b1111).
> 
> I think it's better if you describe the enabled strings in an array, as
> it makes the dts easier to read and write for humans.
> 
ok. I will do that in the next series.
> Also I'm uncertain about the validity of these defaults, as it's not
> that the defaults are 7 and 15 it's that if this property is not
> specified all strings will be enabled and the auto calibration will 
> kick
> in to figure out the proper configuration.
> 
> It would be better to describe this as "absence of this property will
> trigger auto calibration".
> 
Ok. I will describe it in the next series.
>> +
>> +- qcom,cs-out
>> +	Usage:        optional
>> +	Value type:   <bool>
>> +	Definition:   enable current sink output.
>> +		      This property is supported only for PM8941.
>> +
>> +- qcom,cabc
>> +	Usage:        optional
>> +	Value type:   <bool>
>> +	Definition:   enable content adaptive backlight control.
>> +
>> +- qcom,ext-gen
>> +	Usage:        optional
>> +	Value type:   <bool>
>> +	Definition:   use externally generated modulator signal to dim.
>> +		      This property is supported only for PM8941.
>> +
>> +- qcom,external-pfet
>> +	Usage:        optional
>> +	Value type:   <bool>
>> +	Definition:   Specify if external PFET control for short circuit
>> +		      protection is used. This property is supported only
>> +		      for PMI8998.
>> +
>> +- qcom,auto-string-detection
>> +	Usage:        optional
>> +	Value type:   <bool>
>> +	Definition:   Enables auto-detection of the WLED string 
>> configuration.
>> +		      This feature is not supported for PM8941.
> 
> I don't like the idea of having the developer specifying
> qcom,enabled-strings and then qcom,auto-string-detection just in case. 
> I
> think it's better to trust the developer when qcom,enabled-strings is
> specified and if not use auto detection.
> 
Ok. I will modify the description in that way.
>> 
>>  Example:
>> 
>> @@ -34,9 +133,26 @@ pm8941-wled@d800 {
>>  	label = "backlight";
>> 
>>  	qcom,cs-out;
>> -	qcom,current-limit = <20>;
>> +	qcom,current-limit = <20000>;
>> +	qcom,current-boost-limit = <805>;
>> +	qcom,switching-freq = <1600>;
>> +	qcom,ovp = <29000>;
>> +	qcom,num-strings = <3>;
>> +	qcom,enabled-strings = <7>;
> 
> Having to change the existing example shows that you made non-backwards
> compatible changes.
> 
Ok. I will keep them as is in the next series.
>> +};
>> +
>> +pmi8998-wled@d800 {
>> +	compatible = "qcom,pmi8998-wled";
>> +	reg = <0xd800 0xd900>;
>> +	label = "backlight";
>> +
>> +	interrupts = <3 0xd8 2 IRQ_TYPE_EDGE_RISING>,
>> +		     <3 0xd8 1 IRQ_TYPE_EDGE_RISING>;
>> +	interrupt-names = "short", "ovp";
>> +	qcom,current-limit = <25000>;
>>  	qcom,current-boost-limit = <805>;
>>  	qcom,switching-freq = <1600>;
>> -	qcom,ovp = <29>;
>> -	qcom,num-strings = <2>;
>> +	qcom,ovp = <29600>;
>> +	qcom,num-strings = <4>;
>> +	qcom,enabled-strings = <15>;
>>  };
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index 0b6d219..be8b8d3 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -15,253 +15,529 @@
>>  #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 PM8941_WLED_DEFAULT_BRIGHTNESS		2048
>> +#define WLED_DEFAULT_BRIGHTNESS				2048
> 
> These renames are good, but needs to go in a separate commit (either
> patch 1 or a new one), the current approach makes it impossible to
> review the rest of this patch.
> 
> 
> So, as the DT change is substantial that should be split in one patch
> that change the structure, then one that adds the new properties, one
> patch that renames pm8941 -> wled3 and finally one that adds wled4
> support.
> 
Ok. I will split the patches as you suggested.
> Regards,
> Bjorn

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

[Bjorn Andersson]

On Tue 08 May 03:25 PDT 2018, kgunda@codeaurora.org wrote:

> On 2018-05-07 21:50, Bjorn Andersson wrote:
> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
[..]
> > > +- qcom,ovp
> > > +	Usage:        optional
> > > +	Value type:   <u32>
> > > +	Definition:   mV; Over-voltage protection limit;
> > 
> > The existing users of qcom,pm8941-wled depends on this being in V, so
> > you can't change the unit. I suggest that you add a new "qcom,ovp-mv"
> > property and make the driver fall back to looking for qcom,ovp if that
> > isn't specified.
> > 
> > PS. This is a very good example of why it is a good idea to not
> > restructure and make changes at the same time - I almost missed this.
> > 
> Actually I have checked the current kernel and none of the properties are
> being configured from the device tree node. Hence, i thought it is the right
> time modify the units to mV to support the PMI8998.
> 

arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts does.

> You still want to have the qcom,ovp-mv, even though it is not being
> configured from device tree ?

Yes, please.

> > > +		      For pm8941:  one of 27000, 29000, 32000, 35000
> > > +				  Default: 29000 mV
> > > +		      For pmi8998: one of 18100, 19600, 29600, 31100
> > > +				  Default: 29600 mV
> > > +

Regards,
Bjorn

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

[kgunda]

On 2018-05-08 22:47, Bjorn Andersson wrote:
> On Tue 08 May 03:25 PDT 2018, kgunda@codeaurora.org wrote:
> 
>> On 2018-05-07 21:50, Bjorn Andersson wrote:
>> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> [..]
>> > > +- qcom,ovp
>> > > +	Usage:        optional
>> > > +	Value type:   <u32>
>> > > +	Definition:   mV; Over-voltage protection limit;
>> >
>> > The existing users of qcom,pm8941-wled depends on this being in V, so
>> > you can't change the unit. I suggest that you add a new "qcom,ovp-mv"
>> > property and make the driver fall back to looking for qcom,ovp if that
>> > isn't specified.
>> >
>> > PS. This is a very good example of why it is a good idea to not
>> > restructure and make changes at the same time - I almost missed this.
>> >
>> Actually I have checked the current kernel and none of the properties 
>> are
>> being configured from the device tree node. Hence, i thought it is the 
>> right
>> time modify the units to mV to support the PMI8998.
>> 
> 
> arch/arm/boot/dts/qcom-msm8974-sony-xperia-honami.dts does.
> 
>> You still want to have the qcom,ovp-mv, even though it is not being
>> configured from device tree ?
> 
> Yes, please.
> 
Sure.
>> > > +		      For pm8941:  one of 27000, 29000, 32000, 35000
>> > > +				  Default: 29000 mV
>> > > +		      For pmi8998: one of 18100, 19600, 29600, 31100
>> > > +				  Default: 29600 mV
>> > > +
> 
> Regards,
> Bjorn

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

[kgunda]

On 2018-05-08 15:55, kgunda@codeaurora.org wrote:
> On 2018-05-07 21:50, Bjorn Andersson wrote:
>> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>> 
>>> WLED4 peripheral is present on some PMICs like pmi8998
>>> and pm660l. It has a different register map and also
>>> configurations are different. Add support for it.
>>> 
>> 
>> Several things are going on in this patch, it needs to be split to
>> not hide the functional changes from the structural/renames.
>> 
> Ok. I will split it in the next series.
>>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>>> ---
>>>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>>>  drivers/video/backlight/qcom-wled.c                | 749 
>>> +++++++++++++++------
>>>  2 files changed, 696 insertions(+), 225 deletions(-)
>>> 
>>> diff --git 
>>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
>>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>> index fb39e32..0ceffa1 100644
>>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>> @@ -1,30 +1,129 @@
>>>  Binding for Qualcomm Technologies, Inc. WLED driver
>>> 
>>> -Required properties:
>>> -- compatible: should be "qcom,pm8941-wled"
>>> -- reg: slave address
>>> -
>>> -Optional properties:
>>> -- default-brightness: brightness value on boot, value from: 0-4095
>>> -	default: 2048
>>> -- label: The name of the backlight device
>>> -- qcom,cs-out: bool; enable current sink output
>>> -- qcom,cabc: bool; enable content adaptive backlight control
>>> -- qcom,ext-gen: bool; use externally generated modulator signal to 
>>> dim
>>> -- qcom,current-limit: mA; per-string current limit; value from 0 to 
>>> 25
>>> -	default: 20mA
>>> -- qcom,current-boost-limit: mA; boost current limit; one of:
>>> -	105, 385, 525, 805, 980, 1260, 1400, 1680
>>> -	default: 805mA
>>> -- qcom,switching-freq: kHz; switching frequency; one of:
>>> -	600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
>>> -	1600, 1920, 2400, 3200, 4800, 9600,
>>> -	default: 1600kHz
>>> -- qcom,ovp: V; Over-voltage protection limit; one of:
>>> -	27, 29, 32, 35
>>> -	default: 29V
>>> -- qcom,num-strings: #; number of led strings attached; value from 1 
>>> to 3
>>> -	default: 2
>>> +WLED (White Light Emitting Diode) driver is used for controlling 
>>> display
>>> +backlight that is part of PMIC on Qualcomm Technologies, Inc. 
>>> reference
>>> +platforms. The PMIC is connected to the host processor via SPMI bus.
>>> +
>>> +- compatible
>>> +	Usage:        required
>>> +	Value type:   <string>
>>> +	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
>>> +		      or "qcom,pm660l-wled".
>> 
>> Better written as
>> 
>> 		should be one of:
>> 		      	X
>> 		      	Y
>> 		      	Z
>> 
> Will do it in the next series.
>>> +
>>> +- reg
>>> +	Usage:        required
>>> +	Value type:   <prop encoded array>
>>> +	Definition:   Base address of the WLED modules.
>>> +
>>> +- interrupts
>>> +	Usage:        optional
>>> +	Value type:   <prop encoded array>
>>> +	Definition:   Interrupts associated with WLED. Interrupts can be
>>> +		      specified as per the encoding listed under
>>> +		      Documentation/devicetree/bindings/spmi/
>>> +		      qcom,spmi-pmic-arb.txt.
>> 
>> Better to describe that this should be the "short" and "ovp" 
>> interrupts
>> in this property than in the interrupt-names.
>> 
> Ok. I will do it in the next series.
>>> +
>>> +- interrupt-names
>>> +	Usage:        optional
>>> +	Value type:   <string>
>>> +	Definition:   Interrupt names associated with the interrupts.
>>> +		      Must be "short" and "ovp". The short circuit detection
>>> +		      is not supported for PM8941.
>>> +
>>> +- label
>>> +	Usage:        required
>>> +	Value type:   <string>
>>> +	Definition:   The name of the backlight device
>>> +
>>> +- default-brightness
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   brightness value on boot, value from: 0-4095
>>> +		      Default: 2048
>>> +
>>> +- qcom,current-limit
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   uA; per-string current limit
>> 
>> You can't change unit on an existing property, that breaks any 
>> existing
>> dts using the qcom,pm8941-wled compatible.
>> 
> 
>>> +		      value:
>>> +		      For pm8941: from 0 to 25000 with 5000 ua step
>>> +				  Default 20000 uA
>>> +		      For pmi8998: from 0 to 30000 with 5000 ua step
>>> +				   Default 25000 uA.
>> 
>> These values could be described just as well in mA, so keep the 
>> original
>> unit - in particular since the boot-limit is in mA...
>> 
> Ok. Will keep the original as is in the next series.
Here, I may have to go with the approach as in "qcom,ovp". Because for 
pm8941
the current step is 1 mA (I have wrongly mentioned as 5000uA here) and 
for PMI8998
the current step is 2.5 mA. Hence, I will add another variable 
"qcom,current-limit-ua"
just like "qcom,ovp-mv".
>>> +
>>> +- qcom,current-boost-limit
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   mA; boost current limit.
>>> +		      For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
>>> +				 1680. Default: 805 mA
>>> +		      For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
>>> +				  1500. Default: 970 mA
>>> +
>>> +- qcom,switching-freq
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   kHz; switching frequency; one of: 600, 640, 685, 738,
>>> +		      800, 872, 960, 1066, 1200, 1371, 1600, 1920, 2400, 3200,
>>> +		      4800, 9600.
>>> +		      Default: for pm8941: 1600 kHz
>>> +			      for pmi8998: 800 kHz
>>> +
>>> +- qcom,ovp
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   mV; Over-voltage protection limit;
>> 
>> The existing users of qcom,pm8941-wled depends on this being in V, so
>> you can't change the unit. I suggest that you add a new "qcom,ovp-mv"
>> property and make the driver fall back to looking for qcom,ovp if that
>> isn't specified.
>> 
>> PS. This is a very good example of why it is a good idea to not
>> restructure and make changes at the same time - I almost missed this.
>> 
> Actually I have checked the current kernel and none of the properties 
> are
> being configured from the device tree node. Hence, i thought it is the 
> right
> time modify the units to mV to support the PMI8998.
> 
> You still want to have the qcom,ovp-mv, even though it is not being
> configured from
> device tree ?
>>> +		      For pm8941:  one of 27000, 29000, 32000, 35000
>>> +				  Default: 29000 mV
>>> +		      For pmi8998: one of 18100, 19600, 29600, 31100
>>> +				  Default: 29600 mV
>>> +
>>> +- qcom,num-strings
>>> +	Usage:        optional
>>> +	Value type:   <u32>
>>> +	Definition:   #; number of led strings attached;
>>> +		      for pm8941: value 3.
>>> +		      for pmi8998: value 4.
>> 
>> This was the number of strings actually used, so you can't turn this
>> into max number of strings supported. As we have different compatibles
>> for the different pmics this can be hard coded in the driver, based on
>> compatible, and qcom,num-strings can be kept as a special case of
>> qcom,enabled-strings (enabling string 0 through N).
>> 
> Ok. Actually I don't see the use of this property as the 
> qcom,enabled-strings
> it self is enough to know the strings to be enabled. But for backward
> compatibility
> i keep it as original property.
>>> +
>>> +- qcom,enabled-strings
>>> +	Usage:        optional
>>> +	Value tyoe:   <u32>
>>> +	Definition:   Bit mask of the WLED strings. Bit 0 to 3 indicates 
>>> strings
>>> +		      0 to 3 respectively. Each string of leds are operated
>>> +		      individually. Specify the strings using the bit mask. Any
>>> +		      combination of led strings can be used.
>>> +		      for pm8941: Default value is 7 (b111).
>>> +		      for pmi8998: Default value is 15 (b1111).
>> 
>> I think it's better if you describe the enabled strings in an array, 
>> as
>> it makes the dts easier to read and write for humans.
>> 
> ok. I will do that in the next series.
>> Also I'm uncertain about the validity of these defaults, as it's not
>> that the defaults are 7 and 15 it's that if this property is not
>> specified all strings will be enabled and the auto calibration will 
>> kick
>> in to figure out the proper configuration.
>> 
>> It would be better to describe this as "absence of this property will
>> trigger auto calibration".
>> 
> Ok. I will describe it in the next series.
>>> +
>>> +- qcom,cs-out
>>> +	Usage:        optional
>>> +	Value type:   <bool>
>>> +	Definition:   enable current sink output.
>>> +		      This property is supported only for PM8941.
>>> +
>>> +- qcom,cabc
>>> +	Usage:        optional
>>> +	Value type:   <bool>
>>> +	Definition:   enable content adaptive backlight control.
>>> +
>>> +- qcom,ext-gen
>>> +	Usage:        optional
>>> +	Value type:   <bool>
>>> +	Definition:   use externally generated modulator signal to dim.
>>> +		      This property is supported only for PM8941.
>>> +
>>> +- qcom,external-pfet
>>> +	Usage:        optional
>>> +	Value type:   <bool>
>>> +	Definition:   Specify if external PFET control for short circuit
>>> +		      protection is used. This property is supported only
>>> +		      for PMI8998.
>>> +
>>> +- qcom,auto-string-detection
>>> +	Usage:        optional
>>> +	Value type:   <bool>
>>> +	Definition:   Enables auto-detection of the WLED string 
>>> configuration.
>>> +		      This feature is not supported for PM8941.
>> 
>> I don't like the idea of having the developer specifying
>> qcom,enabled-strings and then qcom,auto-string-detection just in case. 
>> I
>> think it's better to trust the developer when qcom,enabled-strings is
>> specified and if not use auto detection.
>> 
> Ok. I will modify the description in that way.
>>> 
>>>  Example:
>>> 
>>> @@ -34,9 +133,26 @@ pm8941-wled@d800 {
>>>  	label = "backlight";
>>> 
>>>  	qcom,cs-out;
>>> -	qcom,current-limit = <20>;
>>> +	qcom,current-limit = <20000>;
>>> +	qcom,current-boost-limit = <805>;
>>> +	qcom,switching-freq = <1600>;
>>> +	qcom,ovp = <29000>;
>>> +	qcom,num-strings = <3>;
>>> +	qcom,enabled-strings = <7>;
>> 
>> Having to change the existing example shows that you made 
>> non-backwards
>> compatible changes.
>> 
> Ok. I will keep them as is in the next series.
>>> +};
>>> +
>>> +pmi8998-wled@d800 {
>>> +	compatible = "qcom,pmi8998-wled";
>>> +	reg = <0xd800 0xd900>;
>>> +	label = "backlight";
>>> +
>>> +	interrupts = <3 0xd8 2 IRQ_TYPE_EDGE_RISING>,
>>> +		     <3 0xd8 1 IRQ_TYPE_EDGE_RISING>;
>>> +	interrupt-names = "short", "ovp";
>>> +	qcom,current-limit = <25000>;
>>>  	qcom,current-boost-limit = <805>;
>>>  	qcom,switching-freq = <1600>;
>>> -	qcom,ovp = <29>;
>>> -	qcom,num-strings = <2>;
>>> +	qcom,ovp = <29600>;
>>> +	qcom,num-strings = <4>;
>>> +	qcom,enabled-strings = <15>;
>>>  };
>>> diff --git a/drivers/video/backlight/qcom-wled.c 
>>> b/drivers/video/backlight/qcom-wled.c
>>> index 0b6d219..be8b8d3 100644
>>> --- a/drivers/video/backlight/qcom-wled.c
>>> +++ b/drivers/video/backlight/qcom-wled.c
>>> @@ -15,253 +15,529 @@
>>>  #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 PM8941_WLED_DEFAULT_BRIGHTNESS		2048
>>> +#define WLED_DEFAULT_BRIGHTNESS				2048
>> 
>> These renames are good, but needs to go in a separate commit (either
>> patch 1 or a new one), the current approach makes it impossible to
>> review the rest of this patch.
>> 
>> 
>> So, as the DT change is substantial that should be split in one patch
>> that change the structure, then one that adds the new properties, one
>> patch that renames pm8941 -> wled3 and finally one that adds wled4
>> support.
>> 
> Ok. I will split the patches as you suggested.
>> Regards,
>> Bjorn

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

[Rob Herring]

On Thu, May 17, 2018 at 4:47 AM,  <kgunda@codeaurora.org> wrote:
> On 2018-05-08 15:55, kgunda@codeaurora.org wrote:
>>
>> On 2018-05-07 21:50, Bjorn Andersson wrote:
>>>
>>> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>>>
>>>> WLED4 peripheral is present on some PMICs like pmi8998
>>>> and pm660l. It has a different register map and also
>>>> configurations are different. Add support for it.
>>>>
>>>
>>> Several things are going on in this patch, it needs to be split to
>>> not hide the functional changes from the structural/renames.
>>>
>> Ok. I will split it in the next series.
>>>>
>>>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>>>> ---
>>>>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>>>>  drivers/video/backlight/qcom-wled.c                | 749
>>>> +++++++++++++++------
>>>>  2 files changed, 696 insertions(+), 225 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>> index fb39e32..0ceffa1 100644
>>>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>> @@ -1,30 +1,129 @@
>>>>  Binding for Qualcomm Technologies, Inc. WLED driver
>>>>
>>>> -Required properties:
>>>> -- compatible: should be "qcom,pm8941-wled"
>>>> -- reg: slave address
>>>> -
>>>> -Optional properties:
>>>> -- default-brightness: brightness value on boot, value from: 0-4095
>>>> -       default: 2048
>>>> -- label: The name of the backlight device
>>>> -- qcom,cs-out: bool; enable current sink output
>>>> -- qcom,cabc: bool; enable content adaptive backlight control
>>>> -- qcom,ext-gen: bool; use externally generated modulator signal to dim
>>>> -- qcom,current-limit: mA; per-string current limit; value from 0 to 25
>>>> -       default: 20mA
>>>> -- qcom,current-boost-limit: mA; boost current limit; one of:
>>>> -       105, 385, 525, 805, 980, 1260, 1400, 1680
>>>> -       default: 805mA
>>>> -- qcom,switching-freq: kHz; switching frequency; one of:
>>>> -       600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
>>>> -       1600, 1920, 2400, 3200, 4800, 9600,
>>>> -       default: 1600kHz
>>>> -- qcom,ovp: V; Over-voltage protection limit; one of:
>>>> -       27, 29, 32, 35
>>>> -       default: 29V
>>>> -- qcom,num-strings: #; number of led strings attached; value from 1 to
>>>> 3
>>>> -       default: 2
>>>> +WLED (White Light Emitting Diode) driver is used for controlling
>>>> display
>>>> +backlight that is part of PMIC on Qualcomm Technologies, Inc. reference
>>>> +platforms. The PMIC is connected to the host processor via SPMI bus.
>>>> +
>>>> +- compatible
>>>> +       Usage:        required
>>>> +       Value type:   <string>
>>>> +       Definition:   should be "qcom,pm8941-wled" or
>>>> "qcom,pmi8998-wled".
>>>> +                     or "qcom,pm660l-wled".
>>>
>>>
>>> Better written as
>>>
>>>                 should be one of:
>>>                         X
>>>                         Y
>>>                         Z
>>>
>> Will do it in the next series.
>>>>
>>>> +
>>>> +- reg
>>>> +       Usage:        required
>>>> +       Value type:   <prop encoded array>
>>>> +       Definition:   Base address of the WLED modules.
>>>> +
>>>> +- interrupts
>>>> +       Usage:        optional
>>>> +       Value type:   <prop encoded array>
>>>> +       Definition:   Interrupts associated with WLED. Interrupts can be
>>>> +                     specified as per the encoding listed under
>>>> +                     Documentation/devicetree/bindings/spmi/
>>>> +                     qcom,spmi-pmic-arb.txt.
>>>
>>>
>>> Better to describe that this should be the "short" and "ovp" interrupts
>>> in this property than in the interrupt-names.
>>>
>> Ok. I will do it in the next series.
>>>>
>>>> +
>>>> +- interrupt-names
>>>> +       Usage:        optional
>>>> +       Value type:   <string>
>>>> +       Definition:   Interrupt names associated with the interrupts.
>>>> +                     Must be "short" and "ovp". The short circuit
>>>> detection
>>>> +                     is not supported for PM8941.
>>>> +
>>>> +- label
>>>> +       Usage:        required
>>>> +       Value type:   <string>
>>>> +       Definition:   The name of the backlight device
>>>> +
>>>> +- default-brightness
>>>> +       Usage:        optional
>>>> +       Value type:   <u32>
>>>> +       Definition:   brightness value on boot, value from: 0-4095
>>>> +                     Default: 2048
>>>> +
>>>> +- qcom,current-limit
>>>> +       Usage:        optional
>>>> +       Value type:   <u32>
>>>> +       Definition:   uA; per-string current limit
>>>
>>>
>>> You can't change unit on an existing property, that breaks any existing
>>> dts using the qcom,pm8941-wled compatible.
>>>
>>
>>>> +                     value:
>>>> +                     For pm8941: from 0 to 25000 with 5000 ua step
>>>> +                                 Default 20000 uA
>>>> +                     For pmi8998: from 0 to 30000 with 5000 ua step
>>>> +                                  Default 25000 uA.
>>>
>>>
>>> These values could be described just as well in mA, so keep the original
>>> unit - in particular since the boot-limit is in mA...
>>>
>> Ok. Will keep the original as is in the next series.
>
> Here, I may have to go with the approach as in "qcom,ovp". Because for
> pm8941
> the current step is 1 mA (I have wrongly mentioned as 5000uA here) and for
> PMI8998
> the current step is 2.5 mA. Hence, I will add another variable
> "qcom,current-limit-ua"
> just like "qcom,ovp-mv".

Use unit suffixes defined in bindings/property-units.txt.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[kgunda]

On 2018-05-17 18:01, Rob Herring wrote:
> On Thu, May 17, 2018 at 4:47 AM,  <kgunda@codeaurora.org> wrote:
>> On 2018-05-08 15:55, kgunda@codeaurora.org wrote:
>>> 
>>> On 2018-05-07 21:50, Bjorn Andersson wrote:
>>>> 
>>>> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>>>> 
>>>>> WLED4 peripheral is present on some PMICs like pmi8998
>>>>> and pm660l. It has a different register map and also
>>>>> configurations are different. Add support for it.
>>>>> 
>>>> 
>>>> Several things are going on in this patch, it needs to be split to
>>>> not hide the functional changes from the structural/renames.
>>>> 
>>> Ok. I will split it in the next series.
>>>>> 
>>>>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>>>>> ---
>>>>>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>>>>>  drivers/video/backlight/qcom-wled.c                | 749
>>>>> +++++++++++++++------
>>>>>  2 files changed, 696 insertions(+), 225 deletions(-)
>>>>> 
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>>> index fb39e32..0ceffa1 100644
>>>>> --- 
>>>>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>>> +++ 
>>>>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>>>>> @@ -1,30 +1,129 @@
>>>>>  Binding for Qualcomm Technologies, Inc. WLED driver
>>>>> 
>>>>> -Required properties:
>>>>> -- compatible: should be "qcom,pm8941-wled"
>>>>> -- reg: slave address
>>>>> -
>>>>> -Optional properties:
>>>>> -- default-brightness: brightness value on boot, value from: 0-4095
>>>>> -       default: 2048
>>>>> -- label: The name of the backlight device
>>>>> -- qcom,cs-out: bool; enable current sink output
>>>>> -- qcom,cabc: bool; enable content adaptive backlight control
>>>>> -- qcom,ext-gen: bool; use externally generated modulator signal to 
>>>>> dim
>>>>> -- qcom,current-limit: mA; per-string current limit; value from 0 
>>>>> to 25
>>>>> -       default: 20mA
>>>>> -- qcom,current-boost-limit: mA; boost current limit; one of:
>>>>> -       105, 385, 525, 805, 980, 1260, 1400, 1680
>>>>> -       default: 805mA
>>>>> -- qcom,switching-freq: kHz; switching frequency; one of:
>>>>> -       600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
>>>>> -       1600, 1920, 2400, 3200, 4800, 9600,
>>>>> -       default: 1600kHz
>>>>> -- qcom,ovp: V; Over-voltage protection limit; one of:
>>>>> -       27, 29, 32, 35
>>>>> -       default: 29V
>>>>> -- qcom,num-strings: #; number of led strings attached; value from 
>>>>> 1 to
>>>>> 3
>>>>> -       default: 2
>>>>> +WLED (White Light Emitting Diode) driver is used for controlling
>>>>> display
>>>>> +backlight that is part of PMIC on Qualcomm Technologies, Inc. 
>>>>> reference
>>>>> +platforms. The PMIC is connected to the host processor via SPMI 
>>>>> bus.
>>>>> +
>>>>> +- compatible
>>>>> +       Usage:        required
>>>>> +       Value type:   <string>
>>>>> +       Definition:   should be "qcom,pm8941-wled" or
>>>>> "qcom,pmi8998-wled".
>>>>> +                     or "qcom,pm660l-wled".
>>>> 
>>>> 
>>>> Better written as
>>>> 
>>>>                 should be one of:
>>>>                         X
>>>>                         Y
>>>>                         Z
>>>> 
>>> Will do it in the next series.
>>>>> 
>>>>> +
>>>>> +- reg
>>>>> +       Usage:        required
>>>>> +       Value type:   <prop encoded array>
>>>>> +       Definition:   Base address of the WLED modules.
>>>>> +
>>>>> +- interrupts
>>>>> +       Usage:        optional
>>>>> +       Value type:   <prop encoded array>
>>>>> +       Definition:   Interrupts associated with WLED. Interrupts 
>>>>> can be
>>>>> +                     specified as per the encoding listed under
>>>>> +                     Documentation/devicetree/bindings/spmi/
>>>>> +                     qcom,spmi-pmic-arb.txt.
>>>> 
>>>> 
>>>> Better to describe that this should be the "short" and "ovp" 
>>>> interrupts
>>>> in this property than in the interrupt-names.
>>>> 
>>> Ok. I will do it in the next series.
>>>>> 
>>>>> +
>>>>> +- interrupt-names
>>>>> +       Usage:        optional
>>>>> +       Value type:   <string>
>>>>> +       Definition:   Interrupt names associated with the 
>>>>> interrupts.
>>>>> +                     Must be "short" and "ovp". The short circuit
>>>>> detection
>>>>> +                     is not supported for PM8941.
>>>>> +
>>>>> +- label
>>>>> +       Usage:        required
>>>>> +       Value type:   <string>
>>>>> +       Definition:   The name of the backlight device
>>>>> +
>>>>> +- default-brightness
>>>>> +       Usage:        optional
>>>>> +       Value type:   <u32>
>>>>> +       Definition:   brightness value on boot, value from: 0-4095
>>>>> +                     Default: 2048
>>>>> +
>>>>> +- qcom,current-limit
>>>>> +       Usage:        optional
>>>>> +       Value type:   <u32>
>>>>> +       Definition:   uA; per-string current limit
>>>> 
>>>> 
>>>> You can't change unit on an existing property, that breaks any 
>>>> existing
>>>> dts using the qcom,pm8941-wled compatible.
>>>> 
>>> 
>>>>> +                     value:
>>>>> +                     For pm8941: from 0 to 25000 with 5000 ua step
>>>>> +                                 Default 20000 uA
>>>>> +                     For pmi8998: from 0 to 30000 with 5000 ua 
>>>>> step
>>>>> +                                  Default 25000 uA.
>>>> 
>>>> 
>>>> These values could be described just as well in mA, so keep the 
>>>> original
>>>> unit - in particular since the boot-limit is in mA...
>>>> 
>>> Ok. Will keep the original as is in the next series.
>> 
>> Here, I may have to go with the approach as in "qcom,ovp". Because for
>> pm8941
>> the current step is 1 mA (I have wrongly mentioned as 5000uA here) and 
>> for
>> PMI8998
>> the current step is 2.5 mA. Hence, I will add another variable
>> "qcom,current-limit-ua"
>> just like "qcom,ovp-mv".
> 
> Use unit suffixes defined in bindings/property-units.txt.
> 
> Rob
> --
> 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
Thanks for pointing it ! hope I can use "qcom,current-limit-microamp" 
and
"qcom,ovp-millivolt". I am asking this because i found only 
"-microvolt".
"-millivolt" is not present in the bindings you pointed.

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

[Rob Herring]

On Thu, May 17, 2018 at 10:10 AM,  <kgunda@codeaurora.org> wrote:
> On 2018-05-17 18:01, Rob Herring wrote:
>>
>> On Thu, May 17, 2018 at 4:47 AM,  <kgunda@codeaurora.org> wrote:
>>>
>>> On 2018-05-08 15:55, kgunda@codeaurora.org wrote:
>>>>
>>>>
>>>> On 2018-05-07 21:50, Bjorn Andersson wrote:
>>>>>
>>>>>
>>>>> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>>>>>
>>>>>> WLED4 peripheral is present on some PMICs like pmi8998
>>>>>> and pm660l. It has a different register map and also
>>>>>> configurations are different. Add support for it.

[...]

>>>>>> +                     value:
>>>>>> +                     For pm8941: from 0 to 25000 with 5000 ua step
>>>>>> +                                 Default 20000 uA
>>>>>> +                     For pmi8998: from 0 to 30000 with 5000 ua step
>>>>>> +                                  Default 25000 uA.
>>>>>
>>>>>
>>>>>
>>>>> These values could be described just as well in mA, so keep the
>>>>> original
>>>>> unit - in particular since the boot-limit is in mA...
>>>>>
>>>> Ok. Will keep the original as is in the next series.
>>>
>>>
>>> Here, I may have to go with the approach as in "qcom,ovp". Because for
>>> pm8941
>>> the current step is 1 mA (I have wrongly mentioned as 5000uA here) and
>>> for
>>> PMI8998
>>> the current step is 2.5 mA. Hence, I will add another variable
>>> "qcom,current-limit-ua"
>>> just like "qcom,ovp-mv".
>>
>>
>> Use unit suffixes defined in bindings/property-units.txt.
>
> Thanks for pointing it ! hope I can use "qcom,current-limit-microamp" and
> "qcom,ovp-millivolt". I am asking this because i found only "-microvolt".
> "-millivolt" is not present in the bindings you pointed.

That's by design so everyone doesn't just pick whatever random units
they like. Does microvolts not give you enough range?

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pavel Machek]

Hi!

> WLED4 peripheral is present on some PMICs like pmi8998
> and pm660l. It has a different register map and also
> configurations are different. Add support for it.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>  drivers/video/backlight/qcom-wled.c                | 749 +++++++++++++++------
>  2 files changed, 696 insertions(+), 225 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> index fb39e32..0ceffa1 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> @@ -1,30 +1,129 @@
>  Binding for Qualcomm Technologies, Inc. WLED driver
>  
> -Required properties:
> -- compatible: should be "qcom,pm8941-wled"
> -- reg: slave address
> -
> -Optional properties:
> -- default-brightness: brightness value on boot, value from: 0-4095
> -	default: 2048
> +- compatible
> +	Usage:        required
> +	Value type:   <string>
> +	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
> +		      or "qcom,pm660l-wled".
> +
> +- reg
> +	Usage:        required
> +	Value type:   <prop encoded array>
> +	Definition:   Base address of the WLED modules.

I'm not sure if this change of format is good idea here...

									Pavel

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

[kgunda]

On 2018-05-14 22:27, Pavel Machek wrote:
> Hi!
> 
>> WLED4 peripheral is present on some PMICs like pmi8998
>> and pm660l. It has a different register map and also
>> configurations are different. Add support for it.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  .../bindings/leds/backlight/qcom-wled.txt          | 172 ++++-
>>  drivers/video/backlight/qcom-wled.c                | 749 
>> +++++++++++++++------
>>  2 files changed, 696 insertions(+), 225 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
>> b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> index fb39e32..0ceffa1 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>> @@ -1,30 +1,129 @@
>>  Binding for Qualcomm Technologies, Inc. WLED driver
>> 
>> -Required properties:
>> -- compatible: should be "qcom,pm8941-wled"
>> -- reg: slave address
>> -
>> -Optional properties:
>> -- default-brightness: brightness value on boot, value from: 0-4095
>> -	default: 2048
>> +- compatible
>> +	Usage:        required
>> +	Value type:   <string>
>> +	Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
>> +		      or "qcom,pm660l-wled".
>> +
>> +- reg
>> +	Usage:        required
>> +	Value type:   <prop encoded array>
>> +	Definition:   Base address of the WLED modules.
> 
> I'm not sure if this change of format is good idea here...
> 
> 									Pavel
> --
This format is clean and easily readable. That's why I have moved to 
this format.
To avoid confusion, I will move out the existing properties 
(pm8941-wled.c) to other
patch. So that it will be easy to review.

> 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 V1 3/5] 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 | 134 ++++++++++++++++++++++++++++++++++--
 1 file changed, 130 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index be8b8d3..2cfba77 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -10,6 +10,9 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/ktime.h>
 #include <linux/kernel.h>
 #include <linux/backlight.h>
 #include <linux/module.h>
@@ -23,7 +26,9 @@
 
 #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
 
-/* Control registers */
+/* WLED3 Control registers */
+#define WLED3_CTRL_REG_FAULT_STATUS			0x08
+
 #define WLED3_CTRL_REG_MOD_EN				0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
 
@@ -36,7 +41,17 @@
 #define WLED3_CTRL_REG_ILIMIT				0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASK			GENMASK(2, 0)
 
-/* sink registers */
+/* WLED4 control registers */
+#define WLED4_CTRL_REG_SHORT_PROTECT			0x5e
+#define  WLED4_CTRL_REG_SHORT_EN_MASK			BIT(7)
+
+#define WLED4_CTRL_REG_SEC_ACCESS			0xd0
+#define  WLED4_CTRL_REG_SEC_UNLOCK			0xa5
+
+#define WLED4_CTRL_REG_TEST1				0xe2
+#define  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2		0x09
+
+/* WLED3 sink registers */
 #define WLED3_SINK_REG_SYNC				0x47
 #define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
 #define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
@@ -130,17 +145,23 @@ struct wled_config {
 	bool cs_out_en;
 	bool ext_gen;
 	bool cabc;
+	bool external_pfet;
 };
 
 struct wled {
 	const char *name;
 	struct device *dev;
 	struct regmap *regmap;
+	struct mutex lock;	/* Lock to avoid race from ISR */
+	ktime_t last_short_event;
 	u16 ctrl_addr;
 	u16 sink_addr;
 	u32 brightness;
 	u32 max_brightness;
+	u32 short_count;
 	const int *version;
+	int short_irq;
+	bool force_mod_disable;
 
 	struct wled_config cfg;
 	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
@@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, int val)
 {
 	int rc;
 
+	if (wled->force_mod_disable)
+		return 0;
+
 	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
 				WLED3_CTRL_REG_MOD_EN,
 				WLED3_CTRL_REG_MOD_EN_MASK,
@@ -248,12 +272,13 @@ static int wled_update_status(struct backlight_device *bl)
 	    bl->props.state & BL_CORE_FBBLANK)
 		brightness = 0;
 
+	mutex_lock(&wled->lock);
 	if (brightness) {
 		rc = wled->wled_set_brightness(wled, brightness);
 		if (rc < 0) {
 			dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
 				rc);
-			return rc;
+			goto unlock_mutex;
 		}
 
 		rc = wled->wled_sync_toggle(wled);
@@ -267,15 +292,60 @@ static int wled_update_status(struct backlight_device *bl)
 		rc = wled_module_enable(wled, !!brightness);
 		if (rc < 0) {
 			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
-			return rc;
+			goto unlock_mutex;
 		}
 	}
 
 	wled->brightness = brightness;
 
+unlock_mutex:
+	mutex_unlock(&wled->lock);
+
 	return rc;
 }
 
+#define WLED_SHORT_DLY_MS			20
+#define WLED_SHORT_CNT_MAX			5
+#define WLED_SHORT_RESET_CNT_DLY_US	HZ
+static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
+{
+	struct wled *wled = _wled;
+	int rc;
+	s64 elapsed_time;
+
+	wled->short_count++;
+	mutex_lock(&wled->lock);
+	rc = wled_module_enable(wled, false);
+	if (rc < 0) {
+		dev_err(wled->dev, "wled disable failed rc:%d\n", rc);
+		goto unlock_mutex;
+	}
+
+	elapsed_time = ktime_us_delta(ktime_get(),
+				      wled->last_short_event);
+	if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US)
+		wled->short_count = 0;
+
+	if (wled->short_count > WLED_SHORT_CNT_MAX) {
+		dev_err(wled->dev, "Short trigged %d times, disabling WLED forever!\n",
+			wled->short_count);
+		wled->force_mod_disable = true;
+		goto unlock_mutex;
+	}
+
+	wled->last_short_event = ktime_get();
+
+	msleep(WLED_SHORT_DLY_MS);
+	rc = wled_module_enable(wled, true);
+	if (rc < 0)
+		dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
+
+unlock_mutex:
+	mutex_unlock(&wled->lock);
+
+	return IRQ_HANDLED;
+}
+
 static int wled3_setup(struct wled *wled)
 {
 	u16 addr;
@@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled)
 		return rc;
 	}
 
+	if (wled->cfg.external_pfet) {
+		/* Unlock the secure register access */
+		rc = regmap_write(wled->regmap, wled->ctrl_addr +
+				  WLED4_CTRL_REG_SEC_ACCESS,
+				  WLED4_CTRL_REG_SEC_UNLOCK);
+		if (rc < 0)
+			return rc;
+
+		rc = regmap_write(wled->regmap,
+				  wled->ctrl_addr + WLED4_CTRL_REG_TEST1,
+				  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2);
+		if (rc < 0)
+			return rc;
+	}
+
 	return 0;
 }
 
@@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled)
 	.switch_freq = 11,
 	.enabled_strings = 0xf,
 	.cabc = false,
+	.external_pfet = true,
 };
 
 static const u32 wled3_boost_i_limit_values[] = {
@@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled)
 		{ "qcom,cs-out", &cfg->cs_out_en, },
 		{ "qcom,ext-gen", &cfg->ext_gen, },
 		{ "qcom,cabc", &cfg->cabc, },
+		{ "qcom,external-pfet", &cfg->external_pfet, },
 	};
 
 	prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
@@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled)
 	return 0;
 }
 
+static int wled_configure_short_irq(struct wled *wled,
+				    struct platform_device *pdev)
+{
+	int rc = 0;
+
+	/* PM8941 doesn't have the short detection support */
+	if (*wled->version == WLED_PM8941)
+		return 0;
+
+	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+				WLED4_CTRL_REG_SHORT_PROTECT,
+				WLED4_CTRL_REG_SHORT_EN_MASK,
+				WLED4_CTRL_REG_SHORT_EN_MASK);
+	if (rc < 0)
+		return rc;
+
+	wled->short_irq = platform_get_irq_byname(pdev, "short");
+	if (wled->short_irq < 0) {
+		dev_dbg(&pdev->dev, "short irq is not used\n");
+		return 0;
+	}
+
+	rc = devm_request_threaded_irq(wled->dev, wled->short_irq,
+				       NULL, wled_short_irq_handler,
+				       IRQF_ONESHOT,
+				       "wled_short_irq", wled);
+	if (rc < 0)
+		dev_err(wled->dev, "Unable to request short_irq (err:%d)\n",
+			rc);
+
+	return rc;
+}
+
 static const struct backlight_ops wled_ops = {
 	.update_status = wled_update_status,
 };
@@ -733,6 +853,8 @@ static int wled_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	mutex_init(&wled->lock);
+
 	rc = wled_configure(wled);
 	if (rc)
 		return rc;
@@ -752,6 +874,10 @@ static int wled_probe(struct platform_device *pdev)
 		}
 	}
 
+	rc = wled_configure_short_irq(wled, pdev);
+	if (rc < 0)
+		return rc;
+
 	val = WLED_DEFAULT_BRIGHTNESS;
 	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 3/5] backlight: qcom-wled: Add support for short circuit handling

[Dan Carpenter]

Hi Kiran,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on backlight/for-backlight-next]
[also build test WARNING on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kiran-Gunda/backlight-qcom-wled-Support-for-QCOM-wled-driver/20180504-011042
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next

smatch warnings:
drivers/video/backlight/qcom-wled.c:304 wled_update_status() warn: inconsistent returns 'mutex:&wled->lock'.
  Locked on:   line 287
  Unlocked on: line 304

# https://github.com/0day-ci/linux/commit/3856199804123df89ef9a712f0740978ec71ddf6
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 3856199804123df89ef9a712f0740978ec71ddf6
vim +304 drivers/video/backlight/qcom-wled.c

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  263  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  264  static int wled_update_status(struct backlight_device *bl)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  265  {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  266  	struct wled *wled = bl_get_data(bl);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  267  	u16 brightness = bl->props.brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  268  	int rc = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  269  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  270  	if (bl->props.power != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  271  	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  272  	    bl->props.state & BL_CORE_FBBLANK)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  273  		brightness = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  274  
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  275  	mutex_lock(&wled->lock);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  276  	if (brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  277  		rc = wled->wled_set_brightness(wled, brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  278  		if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  279  			dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  280  				rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  281  			goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  282  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  283  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  284  		rc = wled->wled_sync_toggle(wled);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  285  		if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  286  			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
93c64f1e drivers/leds/leds-pm8941-wled.c     Courtney Cavin 2015-03-12  287  			return rc;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  288  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  289  	}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  290  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  291  	if (!!brightness != !!wled->brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  292  		rc = wled_module_enable(wled, !!brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  293  		if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  294  			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  295  			goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  296  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  297  	}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  298  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  299  	wled->brightness = brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  300  
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  301  unlock_mutex:
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  302  	mutex_unlock(&wled->lock);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  303  
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03 @304  	return rc;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  305  }
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  306  

Re: [PATCH V1 3/5] backlight: qcom-wled: Add support for short circuit handling

[kgunda]

On 2018-05-07 13:36, Dan Carpenter wrote:
> Hi Kiran,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on backlight/for-backlight-next]
> [also build test WARNING on v4.17-rc3 next-20180504]
> [if your patch is applied to the wrong git tree, please drop us a note
> to help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Kiran-Gunda/backlight-qcom-wled-Support-for-QCOM-wled-driver/20180504-011042
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
> for-backlight-next
> 
> smatch warnings:
> drivers/video/backlight/qcom-wled.c:304 wled_update_status() warn:
> inconsistent returns 'mutex:&wled->lock'.
>   Locked on:   line 287
>   Unlocked on: line 304
> 
Will fix it in the next series.
> #
> https://github.com/0day-ci/linux/commit/3856199804123df89ef9a712f0740978ec71ddf6
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 3856199804123df89ef9a712f0740978ec71ddf6
> vim +304 drivers/video/backlight/qcom-wled.c
> 
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 263
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  264  static int wled_update_status(struct backlight_device *bl)
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 265  {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  266  	struct wled *wled = bl_get_data(bl);
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  267  	u16 brightness = bl->props.brightness;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  268  	int rc = 0;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 269
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  270  	if (bl->props.power != FB_BLANK_UNBLANK ||
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  271  	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  272  	    bl->props.state & BL_CORE_FBBLANK)
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  273  		brightness = 0;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 274
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  275  	mutex_lock(&wled->lock);
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  276  	if (brightness) {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  277  		rc = wled->wled_set_brightness(wled, brightness);
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  278  		if (rc < 0) {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  279  			dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  280  				rc);
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  281  			goto unlock_mutex;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 282  		}
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 283
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  284  		rc = wled->wled_sync_toggle(wled);
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  285  		if (rc < 0) {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  286  			dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> 93c64f1e drivers/leds/leds-pm8941-wled.c     Courtney Cavin 2015-03-12
>  287  			return rc;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 288  		}
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 289  	}
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 290
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  291  	if (!!brightness != !!wled->brightness) {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  292  		rc = wled_module_enable(wled, !!brightness);
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  293  		if (rc < 0) {
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  294  			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  295  			goto unlock_mutex;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 296  		}
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 297  	}
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 298
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  299  	wled->brightness = brightness;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 300
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  301  unlock_mutex:
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
>  302  	mutex_unlock(&wled->lock);
> 38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 303
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03
> @304  	return rc;
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 305  }
> 18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda    2018-05-03  
> 306
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology 
> Center
> https://lists.01.org/pipermail/kbuild-all                   Intel 
> Corporation
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 3/5] backlight: qcom-wled: Add support for short circuit handling

[Bjorn Andersson]

On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:

> Handle the short circuit interrupt and check if the short circuit
> interrupt is valid. Re-enable the module to check if it goes
> away. Disable the module altogether if the short circuit event
> persists.
> 
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 134 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 130 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index be8b8d3..2cfba77 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -10,6 +10,9 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/ktime.h>
>  #include <linux/kernel.h>
>  #include <linux/backlight.h>
>  #include <linux/module.h>
> @@ -23,7 +26,9 @@
>  
>  #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>  
> -/* Control registers */
> +/* WLED3 Control registers */

Minor nit, please move the change of this comment to the previous patch.

> +#define WLED3_CTRL_REG_FAULT_STATUS			0x08

Unused.

> +
>  #define WLED3_CTRL_REG_MOD_EN				0x46
>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>  
> @@ -36,7 +41,17 @@
>  #define WLED3_CTRL_REG_ILIMIT				0x4e
>  #define  WLED3_CTRL_REG_ILIMIT_MASK			GENMASK(2, 0)
>  
> -/* sink registers */

Please move comment change to previous commit.

> +/* WLED4 control registers */
> +#define WLED4_CTRL_REG_SHORT_PROTECT			0x5e
> +#define  WLED4_CTRL_REG_SHORT_EN_MASK			BIT(7)
> +
> +#define WLED4_CTRL_REG_SEC_ACCESS			0xd0
> +#define  WLED4_CTRL_REG_SEC_UNLOCK			0xa5
> +
> +#define WLED4_CTRL_REG_TEST1				0xe2
> +#define  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2		0x09
> +
> +/* WLED3 sink registers */
>  #define WLED3_SINK_REG_SYNC				0x47
>  #define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
>  #define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
> @@ -130,17 +145,23 @@ struct wled_config {
>  	bool cs_out_en;
>  	bool ext_gen;
>  	bool cabc;
> +	bool external_pfet;
>  };
>  
>  struct wled {
>  	const char *name;
>  	struct device *dev;
>  	struct regmap *regmap;
> +	struct mutex lock;	/* Lock to avoid race from ISR */
> +	ktime_t last_short_event;
>  	u16 ctrl_addr;
>  	u16 sink_addr;
>  	u32 brightness;
>  	u32 max_brightness;
> +	u32 short_count;
>  	const int *version;
> +	int short_irq;
> +	bool force_mod_disable;

"bool disabled_by_short" would describe what's going on better.

>  
>  	struct wled_config cfg;
>  	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
> @@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, int val)
>  {
>  	int rc;
>  
> +	if (wled->force_mod_disable)
> +		return 0;
> +

I don't fancy the idea of not telling user space that it's request to
turn on the backlight is ignored. Can we return some error here so that
it's possible for something to do something about this problem?

>  	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>  				WLED3_CTRL_REG_MOD_EN,
>  				WLED3_CTRL_REG_MOD_EN_MASK,
> @@ -248,12 +272,13 @@ static int wled_update_status(struct backlight_device *bl)
>  	    bl->props.state & BL_CORE_FBBLANK)
>  		brightness = 0;
>  
> +	mutex_lock(&wled->lock);
>  	if (brightness) {
>  		rc = wled->wled_set_brightness(wled, brightness);
>  		if (rc < 0) {
>  			dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
>  				rc);
> -			return rc;
> +			goto unlock_mutex;
>  		}
>  
>  		rc = wled->wled_sync_toggle(wled);
> @@ -267,15 +292,60 @@ static int wled_update_status(struct backlight_device *bl)
>  		rc = wled_module_enable(wled, !!brightness);
>  		if (rc < 0) {
>  			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
> -			return rc;
> +			goto unlock_mutex;
>  		}
>  	}
>  
>  	wled->brightness = brightness;
>  
> +unlock_mutex:
> +	mutex_unlock(&wled->lock);
> +
>  	return rc;
>  }
>  
> +#define WLED_SHORT_DLY_MS			20
> +#define WLED_SHORT_CNT_MAX			5
> +#define WLED_SHORT_RESET_CNT_DLY_US	HZ

HZ is in the unit of jiffies, not micro seconds.

> +static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
> +{
> +	struct wled *wled = _wled;
> +	int rc;
> +	s64 elapsed_time;
> +
> +	wled->short_count++;
> +	mutex_lock(&wled->lock);
> +	rc = wled_module_enable(wled, false);
> +	if (rc < 0) {
> +		dev_err(wled->dev, "wled disable failed rc:%d\n", rc);
> +		goto unlock_mutex;
> +	}
> +
> +	elapsed_time = ktime_us_delta(ktime_get(),
> +				      wled->last_short_event);
> +	if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US)
> +		wled->short_count = 0;
> +
> +	if (wled->short_count > WLED_SHORT_CNT_MAX) {
> +		dev_err(wled->dev, "Short trigged %d times, disabling WLED forever!\n",
> +			wled->short_count);
> +		wled->force_mod_disable = true;
> +		goto unlock_mutex;
> +	}
> +
> +	wled->last_short_event = ktime_get();
> +
> +	msleep(WLED_SHORT_DLY_MS);
> +	rc = wled_module_enable(wled, true);
> +	if (rc < 0)
> +		dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
> +
> +unlock_mutex:
> +	mutex_unlock(&wled->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int wled3_setup(struct wled *wled)
>  {
>  	u16 addr;
> @@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled)
>  		return rc;
>  	}
>  
> +	if (wled->cfg.external_pfet) {
> +		/* Unlock the secure register access */
> +		rc = regmap_write(wled->regmap, wled->ctrl_addr +
> +				  WLED4_CTRL_REG_SEC_ACCESS,
> +				  WLED4_CTRL_REG_SEC_UNLOCK);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = regmap_write(wled->regmap,
> +				  wled->ctrl_addr + WLED4_CTRL_REG_TEST1,
> +				  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled)
>  	.switch_freq = 11,
>  	.enabled_strings = 0xf,
>  	.cabc = false,
> +	.external_pfet = true,

You added the "qcom,external-pfet" boolean to dt, but this forces it to
always be set - i.e. this is either wrong or you can omit the new dt
property.

>  };
>  
>  static const u32 wled3_boost_i_limit_values[] = {
> @@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled)
>  		{ "qcom,cs-out", &cfg->cs_out_en, },
>  		{ "qcom,ext-gen", &cfg->ext_gen, },
>  		{ "qcom,cabc", &cfg->cabc, },
> +		{ "qcom,external-pfet", &cfg->external_pfet, },
>  	};
>  
>  	prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
> @@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled)
>  	return 0;
>  }
>  
> +static int wled_configure_short_irq(struct wled *wled,
> +				    struct platform_device *pdev)
> +{
> +	int rc = 0;
> +
> +	/* PM8941 doesn't have the short detection support */
> +	if (*wled->version == WLED_PM8941)
> +		return 0;

If you attempt to acquire the "short" irq first in this function you
don't need this check - as "short" won't exist on pm8941.

Otherwise, it would be better if you add a "bool has_short_detect" to
the wled struct and check that, rather than sprinkling version checks
throughout.


Or...is cfg->external_pfet already denoting this?

> +
> +	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> +				WLED4_CTRL_REG_SHORT_PROTECT,
> +				WLED4_CTRL_REG_SHORT_EN_MASK,
> +				WLED4_CTRL_REG_SHORT_EN_MASK);

Do you really want to enable the short protect thing before figuring out
if you have a "short" irq.

> +	if (rc < 0)
> +		return rc;
> +
> +	wled->short_irq = platform_get_irq_byname(pdev, "short");

short_irq isn't used outside this function, so preferably you turn it
into a local variable.

> +	if (wled->short_irq < 0) {
> +		dev_dbg(&pdev->dev, "short irq is not used\n");
> +		return 0;
> +	}
> +
> +	rc = devm_request_threaded_irq(wled->dev, wled->short_irq,
> +				       NULL, wled_short_irq_handler,
> +				       IRQF_ONESHOT,
> +				       "wled_short_irq", wled);
> +	if (rc < 0)
> +		dev_err(wled->dev, "Unable to request short_irq (err:%d)\n",
> +			rc);
> +
> +	return rc;
> +}
> +
>  static const struct backlight_ops wled_ops = {
>  	.update_status = wled_update_status,
>  };
> @@ -733,6 +853,8 @@ static int wled_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	mutex_init(&wled->lock);
> +
>  	rc = wled_configure(wled);
>  	if (rc)
>  		return rc;
> @@ -752,6 +874,10 @@ static int wled_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	rc = wled_configure_short_irq(wled, pdev);
> +	if (rc < 0)
> +		return rc;
> +
>  	val = WLED_DEFAULT_BRIGHTNESS;
>  	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);

Regards,
Bjorn

Re: [PATCH V1 3/5] backlight: qcom-wled: Add support for short circuit handling

[kgunda]

On 2018-05-07 22:36, Bjorn Andersson wrote:
> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> 
>> Handle the short circuit interrupt and check if the short circuit
>> interrupt is valid. Re-enable the module to check if it goes
>> away. Disable the module altogether if the short circuit event
>> persists.
>> 
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/video/backlight/qcom-wled.c | 134 
>> ++++++++++++++++++++++++++++++++++--
>>  1 file changed, 130 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/video/backlight/qcom-wled.c 
>> b/drivers/video/backlight/qcom-wled.c
>> index be8b8d3..2cfba77 100644
>> --- a/drivers/video/backlight/qcom-wled.c
>> +++ b/drivers/video/backlight/qcom-wled.c
>> @@ -10,6 +10,9 @@
>>   * GNU General Public License for more details.
>>   */
>> 
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/ktime.h>
>>  #include <linux/kernel.h>
>>  #include <linux/backlight.h>
>>  #include <linux/module.h>
>> @@ -23,7 +26,9 @@
>> 
>>  #define WLED3_SINK_REG_BRIGHT_MAX			0xFFF
>> 
>> -/* Control registers */
>> +/* WLED3 Control registers */
> 
> Minor nit, please move the change of this comment to the previous 
> patch.
> 
Ok. Will do it the next series.
>> +#define WLED3_CTRL_REG_FAULT_STATUS			0x08
> 
> Unused.
> 
Will remove it in the next series.
>> +
>>  #define WLED3_CTRL_REG_MOD_EN				0x46
>>  #define  WLED3_CTRL_REG_MOD_EN_MASK			BIT(7)
>> 
>> @@ -36,7 +41,17 @@
>>  #define WLED3_CTRL_REG_ILIMIT				0x4e
>>  #define  WLED3_CTRL_REG_ILIMIT_MASK			GENMASK(2, 0)
>> 
>> -/* sink registers */
> 
> Please move comment change to previous commit.
> 
Will remove it in the next series.
>> +/* WLED4 control registers */
>> +#define WLED4_CTRL_REG_SHORT_PROTECT			0x5e
>> +#define  WLED4_CTRL_REG_SHORT_EN_MASK			BIT(7)
>> +
>> +#define WLED4_CTRL_REG_SEC_ACCESS			0xd0
>> +#define  WLED4_CTRL_REG_SEC_UNLOCK			0xa5
>> +
>> +#define WLED4_CTRL_REG_TEST1				0xe2
>> +#define  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2		0x09
>> +
>> +/* WLED3 sink registers */
>>  #define WLED3_SINK_REG_SYNC				0x47
>>  #define  WLED3_SINK_REG_SYNC_MASK			GENMASK(2, 0)
>>  #define  WLED4_SINK_REG_SYNC_MASK			GENMASK(3, 0)
>> @@ -130,17 +145,23 @@ struct wled_config {
>>  	bool cs_out_en;
>>  	bool ext_gen;
>>  	bool cabc;
>> +	bool external_pfet;
>>  };
>> 
>>  struct wled {
>>  	const char *name;
>>  	struct device *dev;
>>  	struct regmap *regmap;
>> +	struct mutex lock;	/* Lock to avoid race from ISR */
>> +	ktime_t last_short_event;
>>  	u16 ctrl_addr;
>>  	u16 sink_addr;
>>  	u32 brightness;
>>  	u32 max_brightness;
>> +	u32 short_count;
>>  	const int *version;
>> +	int short_irq;
>> +	bool force_mod_disable;
> 
> "bool disabled_by_short" would describe what's going on better.
> 
Will rename it in the next series.
>> 
>>  	struct wled_config cfg;
>>  	int (*wled_set_brightness)(struct wled *wled, u16 brightness);
>> @@ -192,6 +213,9 @@ static int wled_module_enable(struct wled *wled, 
>> int val)
>>  {
>>  	int rc;
>> 
>> +	if (wled->force_mod_disable)
>> +		return 0;
>> +
> 
> I don't fancy the idea of not telling user space that it's request to
> turn on the backlight is ignored. Can we return some error here so that
> it's possible for something to do something about this problem?
> 
Sure. Will do it in the next series.
>>  	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>>  				WLED3_CTRL_REG_MOD_EN,
>>  				WLED3_CTRL_REG_MOD_EN_MASK,
>> @@ -248,12 +272,13 @@ static int wled_update_status(struct 
>> backlight_device *bl)
>>  	    bl->props.state & BL_CORE_FBBLANK)
>>  		brightness = 0;
>> 
>> +	mutex_lock(&wled->lock);
>>  	if (brightness) {
>>  		rc = wled->wled_set_brightness(wled, brightness);
>>  		if (rc < 0) {
>>  			dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
>>  				rc);
>> -			return rc;
>> +			goto unlock_mutex;
>>  		}
>> 
>>  		rc = wled->wled_sync_toggle(wled);
>> @@ -267,15 +292,60 @@ static int wled_update_status(struct 
>> backlight_device *bl)
>>  		rc = wled_module_enable(wled, !!brightness);
>>  		if (rc < 0) {
>>  			dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
>> -			return rc;
>> +			goto unlock_mutex;
>>  		}
>>  	}
>> 
>>  	wled->brightness = brightness;
>> 
>> +unlock_mutex:
>> +	mutex_unlock(&wled->lock);
>> +
>>  	return rc;
>>  }
>> 
>> +#define WLED_SHORT_DLY_MS			20
>> +#define WLED_SHORT_CNT_MAX			5
>> +#define WLED_SHORT_RESET_CNT_DLY_US	HZ
> 
> HZ is in the unit of jiffies, not micro seconds.
> 
Will address in next series.
>> +static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
>> +{
>> +	struct wled *wled = _wled;
>> +	int rc;
>> +	s64 elapsed_time;
>> +
>> +	wled->short_count++;
>> +	mutex_lock(&wled->lock);
>> +	rc = wled_module_enable(wled, false);
>> +	if (rc < 0) {
>> +		dev_err(wled->dev, "wled disable failed rc:%d\n", rc);
>> +		goto unlock_mutex;
>> +	}
>> +
>> +	elapsed_time = ktime_us_delta(ktime_get(),
>> +				      wled->last_short_event);
>> +	if (elapsed_time > WLED_SHORT_RESET_CNT_DLY_US)
>> +		wled->short_count = 0;
>> +
>> +	if (wled->short_count > WLED_SHORT_CNT_MAX) {
>> +		dev_err(wled->dev, "Short trigged %d times, disabling WLED 
>> forever!\n",
>> +			wled->short_count);
>> +		wled->force_mod_disable = true;
>> +		goto unlock_mutex;
>> +	}
>> +
>> +	wled->last_short_event = ktime_get();
>> +
>> +	msleep(WLED_SHORT_DLY_MS);
>> +	rc = wled_module_enable(wled, true);
>> +	if (rc < 0)
>> +		dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
>> +
>> +unlock_mutex:
>> +	mutex_unlock(&wled->lock);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int wled3_setup(struct wled *wled)
>>  {
>>  	u16 addr;
>> @@ -435,6 +505,21 @@ static int wled4_setup(struct wled *wled)
>>  		return rc;
>>  	}
>> 
>> +	if (wled->cfg.external_pfet) {
>> +		/* Unlock the secure register access */
>> +		rc = regmap_write(wled->regmap, wled->ctrl_addr +
>> +				  WLED4_CTRL_REG_SEC_ACCESS,
>> +				  WLED4_CTRL_REG_SEC_UNLOCK);
>> +		if (rc < 0)
>> +			return rc;
>> +
>> +		rc = regmap_write(wled->regmap,
>> +				  wled->ctrl_addr + WLED4_CTRL_REG_TEST1,
>> +				  WLED4_CTRL_REG_TEST1_EXT_FET_DTEST2);
>> +		if (rc < 0)
>> +			return rc;
>> +	}
>> +
>>  	return 0;
>>  }
>> 
>> @@ -446,6 +531,7 @@ static int wled4_setup(struct wled *wled)
>>  	.switch_freq = 11,
>>  	.enabled_strings = 0xf,
>>  	.cabc = false,
>> +	.external_pfet = true,
> 
> You added the "qcom,external-pfet" boolean to dt, but this forces it to
> always be set - i.e. this is either wrong or you can omit the new dt
> property.
> 
I will make it to false in the next series.
>>  };
>> 
>>  static const u32 wled3_boost_i_limit_values[] = {
>> @@ -628,6 +714,7 @@ static int wled_configure(struct wled *wled)
>>  		{ "qcom,cs-out", &cfg->cs_out_en, },
>>  		{ "qcom,ext-gen", &cfg->ext_gen, },
>>  		{ "qcom,cabc", &cfg->cabc, },
>> +		{ "qcom,external-pfet", &cfg->external_pfet, },
>>  	};
>> 
>>  	prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
>> @@ -701,6 +788,39 @@ static int wled_configure(struct wled *wled)
>>  	return 0;
>>  }
>> 
>> +static int wled_configure_short_irq(struct wled *wled,
>> +				    struct platform_device *pdev)
>> +{
>> +	int rc = 0;
>> +
>> +	/* PM8941 doesn't have the short detection support */
>> +	if (*wled->version == WLED_PM8941)
>> +		return 0;
> 
> If you attempt to acquire the "short" irq first in this function you
> don't need this check - as "short" won't exist on pm8941.
> 
> Otherwise, it would be better if you add a "bool has_short_detect" to
> the wled struct and check that, rather than sprinkling version checks
> throughout.
> 
> 
> Or...is cfg->external_pfet already denoting this?
> 
I think it is better to add the has_short_detect variable, as the 
external_pfet is
only specific to PMI8998.
>> +
>> +	rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
>> +				WLED4_CTRL_REG_SHORT_PROTECT,
>> +				WLED4_CTRL_REG_SHORT_EN_MASK,
>> +				WLED4_CTRL_REG_SHORT_EN_MASK);
> 
> Do you really want to enable the short protect thing before figuring 
> out
> if you have a "short" irq.
> 
Yes. Irrespective of the interrupt, the short circuit detection should 
be enabled
to protect the HW from the short circuit.
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	wled->short_irq = platform_get_irq_byname(pdev, "short");
> 
> short_irq isn't used outside this function, so preferably you turn it
> into a local variable.
> 
Ok. Will do that in the next series.
>> +	if (wled->short_irq < 0) {
>> +		dev_dbg(&pdev->dev, "short irq is not used\n");
>> +		return 0;
>> +	}
>> +
>> +	rc = devm_request_threaded_irq(wled->dev, wled->short_irq,
>> +				       NULL, wled_short_irq_handler,
>> +				       IRQF_ONESHOT,
>> +				       "wled_short_irq", wled);
>> +	if (rc < 0)
>> +		dev_err(wled->dev, "Unable to request short_irq (err:%d)\n",
>> +			rc);
>> +
>> +	return rc;
>> +}
>> +
>>  static const struct backlight_ops wled_ops = {
>>  	.update_status = wled_update_status,
>>  };
>> @@ -733,6 +853,8 @@ static int wled_probe(struct platform_device 
>> *pdev)
>>  		return -ENODEV;
>>  	}
>> 
>> +	mutex_init(&wled->lock);
>> +
>>  	rc = wled_configure(wled);
>>  	if (rc)
>>  		return rc;
>> @@ -752,6 +874,10 @@ static int wled_probe(struct platform_device 
>> *pdev)
>>  		}
>>  	}
>> 
>> +	rc = wled_configure_short_irq(wled, pdev);
>> +	if (rc < 0)
>> +		return rc;
>> +
>>  	val = WLED_DEFAULT_BRIGHTNESS;
>>  	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
> 
> Regards,
> Bjorn

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

[Kiran Gunda]

WLED peripheral has over voltage protection(OVP) circuitry and the OVP
fault is notified through an interrupt. Though this fault condition rising
is due to an incorrect hardware configuration is mitigated in the hardware,
it still needs to be detected and handled. Add support for it.

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

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

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

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

[Bjorn Andersson]

On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:

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

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

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

Do you really want to delay the work on disable?

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

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

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

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

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

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

> +		return 0;
> +	}
> +

Regards,
Bjorn

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

[kgunda]

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

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

[Bjorn Andersson]

On Tue 08 May 05:26 PDT 2018, kgunda@codeaurora.org wrote:

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

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

Regards,
Bjorn

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

[kgunda]

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

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

[kgunda]

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

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

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

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

>> Regards,
>> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V1 5/5] 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.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 80ae084..bacdf3f 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -33,11 +33,14 @@
 #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_MOD_EN_BIT			BIT(7)
 
+#define WLED3_CTRL_REG_FEEDBACK_CONTROL			0x48
+
 #define WLED3_CTRL_REG_FREQ				0x4c
 #define  WLED3_CTRL_REG_FREQ_MASK			GENMASK(3, 0)
 
@@ -152,6 +155,7 @@ struct wled_config {
 	bool ext_gen;
 	bool cabc;
 	bool external_pfet;
+	bool auto_detection_enabled;
 };
 
 struct wled {
@@ -160,8 +164,10 @@ 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;
@@ -170,6 +176,7 @@ struct wled {
 	int ovp_irq;
 	bool force_mod_disable;
 	bool ovp_irq_disabled;
+	bool auto_detection_done;
 
 	struct wled_config cfg;
 	struct delayed_work ovp_work;
@@ -386,6 +393,292 @@ 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;
+
+	if (wled->auto_detection_done)
+		return 0;
+
+	/* 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->force_mod_disable = 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;
+	}
+
+	wled->auto_detection_done = true;
+
+failed_detect:
+	return rc;
+}
+
+#define WLED_AUTO_DETECT_OVP_COUNT		5
+#define WLED_AUTO_DETECT_CNT_DLY_US		HZ /* 1 second */
+static bool wled_auto_detection_required(struct wled *wled)
+{
+	s64 elapsed_time_us;
+
+	if (*wled->version == WLED_PM8941)
+		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->version == WLED_PM8941)
+		return 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);
+		rc = wled_auto_string_detection(wled);
+		if (!rc)
+			wled->auto_detection_done = true;
+		mutex_unlock(&wled->lock);
+	}
+
+	return rc;
+}
+
+static void handle_ovp_fault(struct wled *wled)
+{
+	if (!wled->cfg.auto_detection_enabled)
+		return;
+
+	mutex_lock(&wled->lock);
+	if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
+		disable_irq_nosync(wled->ovp_irq);
+		wled->ovp_irq_disabled = true;
+	}
+
+	if (wled_auto_detection_required(wled))
+		wled_auto_string_detection(wled);
+
+	if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
+		enable_irq(wled->ovp_irq);
+		wled->ovp_irq_disabled = false;
+	}
+	mutex_unlock(&wled->lock);
+}
+
 static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
 {
 	struct wled *wled = _wled;
@@ -413,6 +706,9 @@ static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
 		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)
+		handle_ovp_fault(wled);
+
 	return IRQ_HANDLED;
 }
 
@@ -575,6 +871,10 @@ static int wled4_setup(struct wled *wled)
 		return rc;
 	}
 
+	rc = wled_auto_detection_at_init(wled);
+	if (rc < 0)
+		return rc;
+
 	if (wled->cfg.external_pfet) {
 		/* Unlock the secure register access */
 		rc = regmap_write(wled->regmap, wled->ctrl_addr +
@@ -602,6 +902,7 @@ static int wled4_setup(struct wled *wled)
 	.enabled_strings = 0xf,
 	.cabc = false,
 	.external_pfet = true,
+	.auto_detection_enabled = false,
 };
 
 static const u32 wled3_boost_i_limit_values[] = {
@@ -785,6 +1086,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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 5/5] backlight: qcom-wled: Add auto string detection logic

[Bjorn Andersson]

On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:

[..]
> +
> +#define WLED_AUTO_DETECT_OVP_COUNT		5
> +#define WLED_AUTO_DETECT_CNT_DLY_US		HZ /* 1 second */
> +static bool wled_auto_detection_required(struct wled *wled)

So cfg.auto_detection_enabled is set, but we didn't have a fault during
wled_auto_detection_at_init(), which I presume indicates that the boot
loader configured the strings appropriately (or didn't enable the BL).
Then first time we try to enable the backlight we will hit the ovp irq,
which will  enter here a few times to figure out that the strings are
incorrectly configured and then we will do the same thing that would
have been done if we probed with a fault.

This is convoluted!

If auto-detection is a feature allowing the developer to omit the string
configuration then just do the auto detection explicitly in probe when
the developer did so and then never do it again.

> +{
> +	s64 elapsed_time_us;
> +
> +	if (*wled->version == WLED_PM8941)
> +		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->version == WLED_PM8941)
> +		return 0;

cfg.auto_detection_enabled will be false in this case, so there's no
need for the extra check.

> +
> +	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)) {

So this would only happen if the boot loader set an invalid string
configuration, as we have yet to enable the module here?

> +		mutex_lock(&wled->lock);
> +		rc = wled_auto_string_detection(wled);
> +		if (!rc)
> +			wled->auto_detection_done = true;
> +		mutex_unlock(&wled->lock);
> +	}
> +
> +	return rc;
> +}
> +
> +static void handle_ovp_fault(struct wled *wled)
> +{
> +	if (!wled->cfg.auto_detection_enabled)

As this is the only reason for requesting the ovp_irq, how about just
not requesting it in this case?

> +		return;
> +
> +	mutex_lock(&wled->lock);
> +	if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {

The logic here is unnecessary, the only way handle_ovp_fault() is ever
executed is if wled_ovp_irq_handler() was called, which is a really good
indication that ovp_irq is valid and !ovp_irq_disabled. So this is
always going to be entered.

> +		disable_irq_nosync(wled->ovp_irq);
> +		wled->ovp_irq_disabled = true;
> +	}
> +
> +	if (wled_auto_detection_required(wled))
> +		wled_auto_string_detection(wled);
> +
> +	if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {

Again, ovp_irq is valid and we entered the function with either
ovp_irq_disabled = true due to some bug or it was set to true above. So
this check is useless - which renders ovp_irq_disabled unnecessary as
well.

> +		enable_irq(wled->ovp_irq);
> +		wled->ovp_irq_disabled = false;
> +	}
> +	mutex_unlock(&wled->lock);
> +}
> +
>  static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
>  {
>  	struct wled *wled = _wled;
> @@ -413,6 +706,9 @@ static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
>  		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)
> +		handle_ovp_fault(wled);

Just inline handle_ovp_fault() here and make things less "generic".

> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -575,6 +871,10 @@ static int wled4_setup(struct wled *wled)
>  		return rc;
>  	}
>  
> +	rc = wled_auto_detection_at_init(wled);
> +	if (rc < 0)
> +		return rc;
> +
>  	if (wled->cfg.external_pfet) {
>  		/* Unlock the secure register access */
>  		rc = regmap_write(wled->regmap, wled->ctrl_addr +
> @@ -602,6 +902,7 @@ static int wled4_setup(struct wled *wled)
>  	.enabled_strings = 0xf,
>  	.cabc = false,
>  	.external_pfet = true,
> +	.auto_detection_enabled = false,
>  };
>  
>  static const u32 wled3_boost_i_limit_values[] = {
> @@ -785,6 +1086,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, },
>  	};

So afaict the auto detect logic is triggered by two things:

* Boot loader enabled backlight with an invalid string configuration,
  which will make wled_auto_detection_at_init() do the detection.

* Once we the driver tries to enable the module, ovp faults will start
  arriving and we will trigger the auto detection.

But I think you can integrate this in a much more direct way. If the
module is enabled and there are no faults you should be able to just
read the config from the hardware (if auto detect is enabled!) and if
the module is not enabled you can just call auto detect from probe().

This will give you flicker free "auto detection" in the event that the
boot loader did its job and very clean logic in the other cases.

Regards,
Bjorn

Re: [PATCH V1 5/5] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2018-05-07 23:40, Bjorn Andersson wrote:
> On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> 
> [..]
>> +
>> +#define WLED_AUTO_DETECT_OVP_COUNT		5
>> +#define WLED_AUTO_DETECT_CNT_DLY_US		HZ /* 1 second */
>> +static bool wled_auto_detection_required(struct wled *wled)
> 
> So cfg.auto_detection_enabled is set, but we didn't have a fault during
> wled_auto_detection_at_init(), which I presume indicates that the boot
> loader configured the strings appropriately (or didn't enable the BL).
> Then first time we try to enable the backlight we will hit the ovp irq,
> which will  enter here a few times to figure out that the strings are
> incorrectly configured and then we will do the same thing that would
> have been done if we probed with a fault.
> 
> This is convoluted!
> 
> If auto-detection is a feature allowing the developer to omit the 
> string
> configuration then just do the auto detection explicitly in probe when
> the developer did so and then never do it again.
> 
As explained in the previous patch, the auto-detection is needed later,
because are also cases where one/more of the connected LED string of the
display-backlight is malfunctioning (because of damage) and requires the
damaged string to be turned off to prevent the complete panel and/or 
board
from being damaged.
>> +{
>> +	s64 elapsed_time_us;
>> +
>> +	if (*wled->version == WLED_PM8941)
>> +		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->version == WLED_PM8941)
>> +		return 0;
> 
> cfg.auto_detection_enabled will be false in this case, so there's no
> need for the extra check.
> 
Ok. I will remove it in the next series.
>> +
>> +	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)) {
> 
> So this would only happen if the boot loader set an invalid string
> configuration, as we have yet to enable the module here?
> 
Yes.
>> +		mutex_lock(&wled->lock);
>> +		rc = wled_auto_string_detection(wled);
>> +		if (!rc)
>> +			wled->auto_detection_done = true;
>> +		mutex_unlock(&wled->lock);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +static void handle_ovp_fault(struct wled *wled)
>> +{
>> +	if (!wled->cfg.auto_detection_enabled)
> 
> As this is the only reason for requesting the ovp_irq, how about just
> not requesting it in this case?
> 
This is also needed for information purpose if there is any other reason
and also discussing with HW systems what needs to do if the OVP keep on 
triggering.
>> +		return;
>> +
>> +	mutex_lock(&wled->lock);
>> +	if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
> 
> The logic here is unnecessary, the only way handle_ovp_fault() is ever
> executed is if wled_ovp_irq_handler() was called, which is a really 
> good
> indication that ovp_irq is valid and !ovp_irq_disabled. So this is
> always going to be entered.
> 
Ok. I will remove this logic in the next series.
>> +		disable_irq_nosync(wled->ovp_irq);
>> +		wled->ovp_irq_disabled = true;
>> +	}
>> +
>> +	if (wled_auto_detection_required(wled))
>> +		wled_auto_string_detection(wled);
>> +
>> +	if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
> 
> Again, ovp_irq is valid and we entered the function with either
> ovp_irq_disabled = true due to some bug or it was set to true above. So
> this check is useless - which renders ovp_irq_disabled unnecessary as
> well.
> 
Ok. I will remove it in the next series.
>> +		enable_irq(wled->ovp_irq);
>> +		wled->ovp_irq_disabled = false;
>> +	}
>> +	mutex_unlock(&wled->lock);
>> +}
>> +
>>  static irqreturn_t wled_ovp_irq_handler(int irq, void *_wled)
>>  {
>>  	struct wled *wled = _wled;
>> @@ -413,6 +706,9 @@ static irqreturn_t wled_ovp_irq_handler(int irq, 
>> void *_wled)
>>  		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)
>> +		handle_ovp_fault(wled);
> 
> Just inline handle_ovp_fault() here and make things less "generic".
> 
Sure. Will do it in the next series.
>> +
>>  	return IRQ_HANDLED;
>>  }
>> 
>> @@ -575,6 +871,10 @@ static int wled4_setup(struct wled *wled)
>>  		return rc;
>>  	}
>> 
>> +	rc = wled_auto_detection_at_init(wled);
>> +	if (rc < 0)
>> +		return rc;
>> +
>>  	if (wled->cfg.external_pfet) {
>>  		/* Unlock the secure register access */
>>  		rc = regmap_write(wled->regmap, wled->ctrl_addr +
>> @@ -602,6 +902,7 @@ static int wled4_setup(struct wled *wled)
>>  	.enabled_strings = 0xf,
>>  	.cabc = false,
>>  	.external_pfet = true,
>> +	.auto_detection_enabled = false,
>>  };
>> 
>>  static const u32 wled3_boost_i_limit_values[] = {
>> @@ -785,6 +1086,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, },
>>  	};
> 
> So afaict the auto detect logic is triggered by two things:
> 
> * Boot loader enabled backlight with an invalid string configuration,
>   which will make wled_auto_detection_at_init() do the detection.
> 
> * Once we the driver tries to enable the module, ovp faults will start
>   arriving and we will trigger the auto detection.
> 
> But I think you can integrate this in a much more direct way. If the
> module is enabled and there are no faults you should be able to just
> read the config from the hardware (if auto detect is enabled!) and if
> the module is not enabled you can just call auto detect from probe().
> 
> This will give you flicker free "auto detection" in the event that the
> boot loader did its job and very clean logic in the other cases.
> 
Sure. I will improve this logic 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 V1 5/5] backlight: qcom-wled: Add auto string detection logic

[Bjorn Andersson]

On Wed 09 May 00:14 PDT 2018, kgunda@codeaurora.org wrote:

> On 2018-05-07 23:40, Bjorn Andersson wrote:
> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
> > 
> > [..]
> > > +
> > > +#define WLED_AUTO_DETECT_OVP_COUNT		5
> > > +#define WLED_AUTO_DETECT_CNT_DLY_US		HZ /* 1 second */
> > > +static bool wled_auto_detection_required(struct wled *wled)
> > 
> > So cfg.auto_detection_enabled is set, but we didn't have a fault during
> > wled_auto_detection_at_init(), which I presume indicates that the boot
> > loader configured the strings appropriately (or didn't enable the BL).
> > Then first time we try to enable the backlight we will hit the ovp irq,
> > which will  enter here a few times to figure out that the strings are
> > incorrectly configured and then we will do the same thing that would
> > have been done if we probed with a fault.
> > 
> > This is convoluted!
> > 
> > If auto-detection is a feature allowing the developer to omit the string
> > configuration then just do the auto detection explicitly in probe when
> > the developer did so and then never do it again.
> > 
> As explained in the previous patch, the auto-detection is needed later,
> because are also cases where one/more of the connected LED string of the
> display-backlight is malfunctioning (because of damage) and requires the
> damaged string to be turned off to prevent the complete panel and/or board
> from being damaged.

Okay, that sounds very reasonable. Please ensure that it's clearly
described in the commit message, so that we have this documented if
someone wonders in the future.

Regards,
Bjorn

Re: [PATCH V1 5/5] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2018-05-14 22:32, Bjorn Andersson wrote:
> On Wed 09 May 00:14 PDT 2018, kgunda@codeaurora.org wrote:
> 
>> On 2018-05-07 23:40, Bjorn Andersson wrote:
>> > On Thu 03 May 02:57 PDT 2018, Kiran Gunda wrote:
>> >
>> > [..]
>> > > +
>> > > +#define WLED_AUTO_DETECT_OVP_COUNT		5
>> > > +#define WLED_AUTO_DETECT_CNT_DLY_US		HZ /* 1 second */
>> > > +static bool wled_auto_detection_required(struct wled *wled)
>> >
>> > So cfg.auto_detection_enabled is set, but we didn't have a fault during
>> > wled_auto_detection_at_init(), which I presume indicates that the boot
>> > loader configured the strings appropriately (or didn't enable the BL).
>> > Then first time we try to enable the backlight we will hit the ovp irq,
>> > which will  enter here a few times to figure out that the strings are
>> > incorrectly configured and then we will do the same thing that would
>> > have been done if we probed with a fault.
>> >
>> > This is convoluted!
>> >
>> > If auto-detection is a feature allowing the developer to omit the string
>> > configuration then just do the auto detection explicitly in probe when
>> > the developer did so and then never do it again.
>> >
>> As explained in the previous patch, the auto-detection is needed 
>> later,
>> because are also cases where one/more of the connected LED string of 
>> the
>> display-backlight is malfunctioning (because of damage) and requires 
>> the
>> damaged string to be turned off to prevent the complete panel and/or 
>> board
>> from being damaged.
> 
> Okay, that sounds very reasonable. Please ensure that it's clearly
> described in the commit message, so that we have this documented if
> someone wonders in the future.
> 
> Regards,
> Bjorn
> --
Thanks for that ! Sure I will describe it in the commit message.
> 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