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

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

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

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

This is still

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

Regards, Bjorn

---

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

diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt similarity index 95% rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt index e5b294d..fb39e32 100644 --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt @@ -1,4 +1,4 @@ -Binding for Qualcomm PM8941 WLED driver +Binding for Qualcomm Technologies, Inc. WLED driver

Required properties:

• compatible: should be "qcom,pm8941-wled"

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 4e1d2ad..8c095e3 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA If you have an Sharp SL-6000 Zaurus say Y to enable a driver for its backlight

-config BACKLIGHT_PM8941_WLED

• tristate "Qualcomm PM8941 WLED Driver"

+config BACKLIGHT_QCOM_WLED

• tristate "Qualcomm PMIC WLED Driver" select REGMAP help

•  If you have the Qualcomm PM8941, say Y to enable a driver for the
  

•  WLED block.
  

•  If you have the Qualcomm PMIC, say Y to enable a driver for the
  

•  WLED block. Currently it supports PM8941 and PMI8998.
  
  

config BACKLIGHT_SAHARA tristate "Tabletkiosk Sahara Touch-iT Backlight Driver" diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index 5e28f01..6fd76ef 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o -obj-$(CONFIG_BACKLIGHT_PM8941_WLED) += pm8941-wled.o obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o +obj-$(CONFIG_BACKLIGHT_QCOM_WLED) += qcom-wled.o obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c similarity index 100% rename from drivers/video/backlight/pm8941-wled.c rename to drivers/video/backlight/qcom-wled.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Thu, Jun 21, 2018 at 02:05:59PM +0100, Daniel Thompson wrote:

On Tue, Jun 19, 2018 at 04:43:36PM +0530, Kiran Gunda wrote:

...

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

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

With changes proposed for v4:

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

It's just a plain (no caveat):

Acked-by: Daniel Thompson daniel.thompson@linaro.org

Daniel.

...

---

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

diff --git a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt similarity index 95% rename from Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt rename to Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt index e5b294d..fb39e32 100644 --- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt @@ -1,4 +1,4 @@ -Binding for Qualcomm PM8941 WLED driver +Binding for Qualcomm Technologies, Inc. WLED driver

Required properties:

• compatible: should be "qcom,pm8941-wled"

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 4e1d2ad..8c095e3 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -299,12 +299,12 @@ config BACKLIGHT_TOSA If you have an Sharp SL-6000 Zaurus say Y to enable a driver for its backlight

-config BACKLIGHT_PM8941_WLED

• tristate "Qualcomm PM8941 WLED Driver"

+config BACKLIGHT_QCOM_WLED

• tristate "Qualcomm PMIC WLED Driver" select REGMAP help

•  If you have the Qualcomm PM8941, say Y to enable a driver for the
  

•  WLED block.
  

•  If you have the Qualcomm PMIC, say Y to enable a driver for the
  

•  WLED block. Currently it supports PM8941 and PMI8998.
  
  

config BACKLIGHT_SAHARA tristate "Tabletkiosk Sahara Touch-iT Backlight Driver" diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index 5e28f01..6fd76ef 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile @@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o obj-$(CONFIG_BACKLIGHT_PANDORA) += pandora_bl.o obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o -obj-$(CONFIG_BACKLIGHT_PM8941_WLED) += pm8941-wled.o obj-$(CONFIG_BACKLIGHT_PWM) += pwm_bl.o +obj-$(CONFIG_BACKLIGHT_QCOM_WLED) += qcom-wled.o obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o obj-$(CONFIG_BACKLIGHT_SKY81452) += sky81452-backlight.o obj-$(CONFIG_BACKLIGHT_TOSA) += tosa_bl.o diff --git a/drivers/video/backlight/pm8941-wled.c b/drivers/video/backlight/qcom-wled.c similarity index 100% rename from drivers/video/backlight/pm8941-wled.c rename to drivers/video/backlight/qcom-wled.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

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

Restructure the qcom-wled bindings for the better readability.

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

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

Regards, Bjorn

---

.../bindings/leds/backlight/qcom-wled.txt | 110 ++++++++++++++++----- 1 file changed, 85 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt index fb39e32..14f28f2 100644 --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt @@ -1,30 +1,90 @@ 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 one of:

• 	"qcom,pm8941-wled"
  

• 	"qcom,pmi8998-wled"
  

• 	"qcom,pm660l-wled"
  

+- reg

• Usage: required
• Value type: <prop encoded array>
• Definition: Base address of the WLED modules.

+- default-brightness

• Usage: optional
• Value type: <u32>
• Definition: brightness value on boot, value from: 0-4095

•       Default: 2048
  

+- label

• Usage: required
• Value type: <string>
• Definition: The name of the backlight device

+- 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,current-limit

• Usage: optional
• Value type: <u32>
• Definition: mA; per-string current limit

•       value: For pm8941: from 0 to 25 with 5 mA step
  

• 	     Default 20 mA.
  

• 	     For pmi8998: from 0 to 30 with 5 mA step
  

• 	     Default 25 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: V; Over-voltage protection limit; one of:

•       27, 29, 32, 35. default: 29V
  

•       This property is supported only for PM8941.
  

+- qcom,num-strings

• Usage: optional
• Value type: <u32>
• Definition: #; number of led strings attached;

•       value from 1 to 3. default: 2
  

•       This property is supported only for PM8941.
  
  

Example:

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Tue, Jun 19, 2018 at 04:43:37PM +0530, Kiran Gunda wrote:

Restructure the qcom-wled bindings for the better readability.

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

Acked-by: Daniel Thompson daniel.thompson@linaro.org

---

.../bindings/leds/backlight/qcom-wled.txt | 110 ++++++++++++++++----- 1 file changed, 85 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt index fb39e32..14f28f2 100644 --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt @@ -1,30 +1,90 @@ 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 one of:

• 	"qcom,pm8941-wled"
  

• 	"qcom,pmi8998-wled"
  

• 	"qcom,pm660l-wled"
  

+- reg

• Usage: required
• Value type: <prop encoded array>
• Definition: Base address of the WLED modules.

+- default-brightness

• Usage: optional
• Value type: <u32>
• Definition: brightness value on boot, value from: 0-4095

•       Default: 2048
  

+- label

• Usage: required
• Value type: <string>
• Definition: The name of the backlight device

+- 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,current-limit

• Usage: optional
• Value type: <u32>
• Definition: mA; per-string current limit

•       value: For pm8941: from 0 to 25 with 5 mA step
  

• 	     Default 20 mA.
  

• 	     For pmi8998: from 0 to 30 with 5 mA step
  

• 	     Default 25 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: V; Over-voltage protection limit; one of:

•       27, 29, 32, 35. default: 29V
  

•       This property is supported only for PM8941.
  

+- qcom,num-strings

• Usage: optional
• Value type: <u32>
• Definition: #; number of led strings attached;

•       value from 1 to 3. default: 2
  

•       This property is supported only for PM8941.
  
  

Example:

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Tue, Jun 19, 2018 at 04:43:38PM +0530, Kiran Gunda wrote:

Update the bindings with the new properties used for PMI8998.

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

.../bindings/leds/backlight/qcom-wled.txt | 84 ++++++++++++++++++++-- 1 file changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt index 14f28f2..503ce87 100644 --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt @@ -48,11 +48,15 @@ platforms. The PMIC is connected to the host processor via SPMI bus.

• qcom,current-limit Usage: optional Value type: <u32>
• Definition: mA; per-string current limit

•       value: For pm8941: from 0 to 25 with 5 mA step
  

• 	     Default 20 mA.
  

• 	     For pmi8998: from 0 to 30 with 5 mA step
  

• 	     Default 25 mA.
  

• Definition: mA; per-string current limit; value from 0 to 25 with

•       1 mA step. Default 20 mA.
  

•       This property is supported only for pm8941.
  

+- qcom,current-limit-microamp

• Usage: optional
• Value type: <u32>
• Definition: uA; per-string current limit; value from 0 to 30000 with

•       2500 uA step. Default 25000 uA.
  

This doesn't really seem worth adding just to add '-microamp'.

• qcom,current-boost-limit Usage: optional

@@ -79,12 +83,61 @@ platforms. The PMIC is connected to the host processor via SPMI bus. 27, 29, 32, 35. default: 29V This property is supported only for PM8941.

+- qcom,ovp-millivolt

Is this the same as qcom,ovp? If so, same comment.

• Usage: optional
• Value type: <u32>
• Definition: mV; Over-voltage protection limit;

•       For pmi8998: one of 18100, 19600, 29600, 31100
  

•       Default: 29600 mV
  

•       If this property is not specified for PM8941, it
  

•       falls back to "qcom,ovp" property.
  

• qcom,num-strings Usage: optional Value type: <u32> Definition: #; number of led strings attached;

•       value from 1 to 3. default: 2
  

•       This property is supported only for PM8941.
  

•       value: For PM8941 from 1 to 3. default: 2
  

• 	     For PMI8998 from 1 to 4. default: 4
  

+- interrupts

• Usage: optional
• Value type: <prop encoded array>
• Definition: Interrupts associated with WLED. This should be

•       "short" and "ovp" interrupts. 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.
  

+- qcom,enabled-strings

• Usage: optional
• Value tyoe: <u32 array>
• Definition: Array of the WLED strings numbered from 0 to 3. Each

•       string of leds are operated individually. Specify the
  

•       list of strings used by the device. Any combination of
  

•       led strings can be used.
  

•       for pm8941: Default values are [00 01].
  

•       for pmi8998: Default values are [00 01 02 03].
  

u32 or u8 because dts syntax for 8-bit array is [].

+- 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:

@@ -99,4 +152,21 @@ pm8941-wled@d800 { qcom,switching-freq = <1600>; qcom,ovp = <29>; qcom,num-strings = <2>;

• qcom,enabled-strings = <0x00 0x01>;

+};

+pmi8998-wled@d800 {

led-controller {

And needs a unit-address.

• 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-microamp = <25000>;
• qcom,current-boost-limit = <805>;
• qcom,switching-freq = <1600>;
• qcom,ovp-millivolt = <29600>;
• qcom,num-strings = <4>;
• qcom,enabled-strings = <0x00 0x01 0x02 0x03>;

};

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On 2018-06-20 04:41, Bjorn Andersson wrote:

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

...

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

Looks good, just three minor comments below.

...

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

drivers/video/backlight/qcom-wled.c | 248 ++++++++++++++++++------------------ 1 file changed, 125 insertions(+), 123 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 0b6d219..812f3cb 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -18,77 +18,79 @@ #include <linux/regmap.h>

/* From DT binding */ -#define PM8941_WLED_DEFAULT_BRIGHTNESS 2048 +#define WLED_DEFAULT_BRIGHTNESS 2048

-#define PM8941_WLED_REG_VAL_BASE 0x40 -#define PM8941_WLED_REG_VAL_MAX 0xFFF +#define WLED3_SINK_REG_BRIGHT_MAX 0xFFF +#define WLED3_CTRL_REG_VAL_BASE 0x40

-#define PM8941_WLED_REG_MOD_EN 0x46 -#define PM8941_WLED_REG_MOD_EN_BIT BIT(7) -#define PM8941_WLED_REG_MOD_EN_MASK BIT(7) +/* WLED3 control registers */ +#define WLED3_CTRL_REG_MOD_EN 0x46 +#define WLED3_CTRL_REG_MOD_EN_BIT BIT(7) +#define WLED3_CTRL_REG_MOD_EN_MASK BIT(7)

-#define PM8941_WLED_REG_SYNC 0x47

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

Actually, this is a sink specific register. That's why moved it to under the /* WLED3 sink specific registers */

...

-#define PM8941_WLED_REG_SYNC_MASK 0x07 -#define PM8941_WLED_REG_SYNC_LED1 BIT(0) -#define PM8941_WLED_REG_SYNC_LED2 BIT(1) -#define PM8941_WLED_REG_SYNC_LED3 BIT(2) -#define PM8941_WLED_REG_SYNC_ALL 0x07 -#define PM8941_WLED_REG_SYNC_CLEAR 0x00 +#define WLED3_CTRL_REG_FREQ 0x4c +#define WLED3_CTRL_REG_FREQ_MASK 0x0f

-#define PM8941_WLED_REG_FREQ 0x4c -#define PM8941_WLED_REG_FREQ_MASK 0x0f +#define WLED3_CTRL_REG_OVP 0x4d +#define WLED3_CTRL_REG_OVP_MASK 0x03

-#define PM8941_WLED_REG_OVP 0x4d -#define PM8941_WLED_REG_OVP_MASK 0x03 +#define WLED3_CTRL_REG_ILIMIT 0x4e +#define WLED3_CTRL_REG_ILIMIT_MASK 0x07

-#define PM8941_WLED_REG_BOOST 0x4e -#define PM8941_WLED_REG_BOOST_MASK 0x07 +/* WLED3 sink registers */ +#define WLED3_SINK_REG_SYNC 0x47 +#define WLED3_SINK_REG_SYNC_MASK 0x07 +#define WLED3_SINK_REG_SYNC_LED1 BIT(0) +#define WLED3_SINK_REG_SYNC_LED2 BIT(1) +#define WLED3_SINK_REG_SYNC_LED3 BIT(2) +#define WLED3_SINK_REG_SYNC_ALL 0x07 +#define WLED3_SINK_REG_SYNC_CLEAR 0x00

[..]

...

-struct pm8941_wled_config {

• u32 i_boost_limit;

+struct wled_config {

• u32 boost_i_limit; u32 ovp; u32 switch_freq; u32 num_strings;

• u32 i_limit;

• u32 string_i_limit;

Changing the members in this struct seems unrelated.

Changed the members, to have resemblance with the IPCAT and better readability .

...

bool cs_out_en; bool ext_gen; bool cabc_en; };

[..]

...

-MODULE_DESCRIPTION("pm8941 wled driver"); +MODULE_DESCRIPTION("qcom wled driver");

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

Sure. I will change it in the next series.

...

MODULE_LICENSE("GPL v2");

Regards, Bjorn

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

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

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

drivers/video/backlight/qcom-wled.c | 635 ++++++++++++++++++++++++++++-------- 1 file changed, 503 insertions(+), 132 deletions(-)

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c

[..]

/* WLED3 sink registers */ #define WLED3_SINK_REG_SYNC 0x47

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

-#define WLED3_SINK_REG_SYNC_MASK 0x07 +#define WLED3_SINK_REG_SYNC_MASK GENMASK(2, 0) +#define WLED4_SINK_REG_SYNC_MASK GENMASK(3, 0) #define WLED3_SINK_REG_SYNC_LED1 BIT(0) #define WLED3_SINK_REG_SYNC_LED2 BIT(1) #define WLED3_SINK_REG_SYNC_LED3 BIT(2) +#define WLED4_SINK_REG_SYNC_LED4 BIT(3) #define WLED3_SINK_REG_SYNC_ALL 0x07 +#define WLED4_SINK_REG_SYNC_ALL 0x0f #define WLED3_SINK_REG_SYNC_CLEAR 0x00

[..]

+static int wled4_set_brightness(struct wled *wled, u16 brightness)

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

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

+{

• int rc, i;
• u16 low_limit = wled->max_brightness * 4 / 1000;
• u8 v[2];

• rc = regmap_bulk_write(wled->regmap,
  

• 		wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
  

• 		v, 2);
  

• if (rc)
  

• /* WLED4's lower limit of operation is 0.4% */

• if (brightness > 0 && brightness < low_limit)

• brightness = low_limit;
  

• v[0] = brightness & 0xff;

• v[1] = (brightness >> 8) & 0xf;

• for (i = 0; i < wled->cfg.num_strings; ++i) {

• rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
  

• 		       WLED4_SINK_REG_BRIGHT(i), v, 2);
  

• if (rc < 0)
  return rc;
  

  }

• return 0;

+}

+static int wled_module_enable(struct wled *wled, int val) +{

• int rc;
• rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +

• 		WLED3_CTRL_REG_MOD_EN,
  

• 		WLED3_CTRL_REG_MOD_EN_MASK,
  

• 		WLED3_CTRL_REG_MOD_EN_MASK);
  

This should depend on val.

• return rc;

+}

+static int wled3_sync_toggle(struct wled *wled) +{

• int rc;
• rc = regmap_update_bits(wled->regmap,

• 	wled->addr + WLED3_SINK_REG_SYNC,
  

• 	WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_ALL);
  

• if (rc)

• 		wled->ctrl_addr + WLED3_SINK_REG_SYNC,
  

• 		WLED3_SINK_REG_SYNC_MASK,
  

• 		WLED3_SINK_REG_SYNC_MASK);
  

• if (rc < 0) return rc;

  rc = regmap_update_bits(wled->regmap,

• 	wled->addr + WLED3_SINK_REG_SYNC,
  

• 	WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
  

• 		wled->ctrl_addr + WLED3_SINK_REG_SYNC,
  

• 		WLED3_SINK_REG_SYNC_MASK,
  

• 		WLED3_SINK_REG_SYNC_CLEAR);
  

• return rc;

}

-static int wled_setup(struct wled *wled) +static int wled4_sync_toggle(struct wled *wled)

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

{ int rc;

• int i;

  rc = regmap_update_bits(wled->regmap,

• 	wled->addr + WLED3_CTRL_REG_OVP,
  

• 	WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
  

• if (rc)

• 		wled->sink_addr + WLED3_SINK_REG_SYNC,
  

• 		WLED4_SINK_REG_SYNC_MASK,
  

• 		WLED4_SINK_REG_SYNC_MASK);
  

• if (rc < 0) return rc;

  rc = regmap_update_bits(wled->regmap,

• 	wled->addr + WLED3_CTRL_REG_ILIMIT,
  

• 	WLED3_CTRL_REG_ILIMIT_MASK, wled->cfg.boost_i_limit);
  

• 		wled->sink_addr + WLED3_SINK_REG_SYNC,
  

• 		WLED4_SINK_REG_SYNC_MASK,
  

• 		WLED3_SINK_REG_SYNC_CLEAR);
  

• return rc;

+}

+static int wled_update_status(struct backlight_device *bl)

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

+{

• struct wled *wled = bl_get_data(bl);
• u16 brightness = bl->props.brightness;
• int rc = 0;
• if (bl->props.power != FB_BLANK_UNBLANK ||

•    bl->props.fb_blank != FB_BLANK_UNBLANK ||
  

•    bl->props.state & BL_CORE_FBBLANK)
  

• brightness = 0;
  

• if (brightness) {

• rc = wled->wled_set_brightness(wled, brightness);
  

• if (rc < 0) {
  

• 	dev_err(wled->dev, "wled failed to set brightness rc:%d\n",
  

• 		rc);
  

• 	return rc;
  

• }
  

• rc = wled->wled_sync_toggle(wled);
  

• if (rc < 0) {
  

• 	dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
  

• 	return rc;
  

• }
  

• }
• if (!!brightness != !!wled->brightness) {

• rc = wled_module_enable(wled, !!brightness);
  

• if (rc < 0) {
  

• 	dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
  

• 	return rc;
  

• }
  

• }
• wled->brightness = brightness;
• return rc;

+}

+static int wled3_setup(struct wled *wled)

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

+{

• u16 addr;

• u8 sink_en = 0;

• int rc, i, j;

• rc = regmap_update_bits(wled->regmap,

• 		wled->ctrl_addr + WLED3_CTRL_REG_OVP,
  

• 		WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
  

  if (rc) return rc;

  rc = regmap_update_bits(wled->regmap,

• 	wled->addr + WLED3_CTRL_REG_FREQ,
  

• 	WLED3_CTRL_REG_FREQ_MASK, wled->cfg.switch_freq);
  

• 		wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
  

• 		WLED3_CTRL_REG_ILIMIT_MASK,
  

• 		wled->cfg.boost_i_limit);
  

[..]

@@ -392,15 +743,33 @@ static int wled_probe(struct platform_device *pdev) return -ENOMEM;

wled->regmap = regmap;

• wled->dev = &pdev->dev;

• rc = wled_configure(wled, &pdev->dev);
• if (rc)

• return rc;
  

• wled->version = of_device_get_match_data(&pdev->dev);
• if (!wled->version) {

• dev_err(&pdev->dev, "Unknown device version\n");
  

• return -ENODEV;
  

• }

• rc = wled_setup(wled);

• rc = wled_configure(wled);

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

if (rc) return rc;

• if (*wled->version == WLED_PM8941) {

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

• rc = wled3_setup(wled);
  

• if (rc) {
  

• 	dev_err(&pdev->dev, "wled3_setup failed\n");
  

• 	return rc;
  

• }
  

• } else if (*wled->version == WLED_PMI8998 ||

• 	*wled->version == WLED_PM660L) {
  

• rc = wled4_setup(wled);
  

• if (rc) {
  

• 	dev_err(&pdev->dev, "wled4_setup failed\n");
  

• 	return rc;
  

• }
  

• }
• val = WLED_DEFAULT_BRIGHTNESS; of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);

@@ -415,7 +784,9 @@ static int wled_probe(struct platform_device *pdev) };

static const struct of_device_id wled_match_table[] = {

• { .compatible = "qcom,pm8941-wled" },

• { .compatible = "qcom,pm8941-wled", .data = &version_table[0] },
• { .compatible = "qcom,pmi8998-wled", .data = &version_table[1] },
• { .compatible = "qcom,pm660l-wled", .data = &version_table[2] },

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

{} };

Regards, Bjorn

On 2018-06-20 11:52, Bjorn Andersson wrote:

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

...

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

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

Signed-off-by: Kiran Gunda kgunda@codeaurora.org

drivers/video/backlight/qcom-wled.c | 408 +++++++++++++++++++++++++++++++++++- 1 file changed, 403 insertions(+), 5 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index e87ba70..6bc627c 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -25,13 +25,23 @@ #define WLED_MAX_STRINGS 4

#define WLED_DEFAULT_BRIGHTNESS 2048

+#define WLED_SOFT_START_DLY_US 10000 #define WLED3_SINK_REG_BRIGHT_MAX 0xFFF

/* WLED3 control registers */ +#define WLED3_CTRL_REG_FAULT_STATUS 0x08 +#define WLED3_CTRL_REG_ILIM_FAULT_BIT BIT(0) +#define WLED3_CTRL_REG_OVP_FAULT_BIT BIT(1) +#define WLED4_CTRL_REG_SC_FAULT_BIT BIT(2)

+#define WLED3_CTRL_REG_INT_RT_STS 0x10 +#define WLED3_CTRL_REG_OVP_FAULT_STATUS BIT(1)

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

Sure. I will change it in the next series.

...

#define WLED3_CTRL_REG_MOD_EN 0x46 #define WLED3_CTRL_REG_MOD_EN_MASK BIT(7)

+#define WLED3_CTRL_REG_FEEDBACK_CONTROL 0x48

#define WLED3_CTRL_REG_FREQ 0x4c #define WLED3_CTRL_REG_FREQ_MASK GENMASK(3, 0)

@@ -146,6 +156,7 @@ struct wled_config { bool ext_gen; bool cabc; bool external_pfet;

• bool auto_detection_enabled;

};

struct wled { @@ -154,16 +165,22 @@ struct wled { struct regmap *regmap; struct mutex lock; /* Lock to avoid race from ISR */ ktime_t last_short_event;

• ktime_t start_ovp_fault_time; u16 ctrl_addr; u16 sink_addr;

• u16 auto_detection_ovp_count; u32 brightness; u32 max_brightness; u32 short_count;

• u32 auto_detect_count; const int *version; bool disabled_by_short; bool has_short_detect;

• int ovp_irq;

• bool ovp_irq_disabled;

  struct wled_config cfg;

• struct delayed_work ovp_work; int (*wled_set_brightness)(struct wled *wled, u16 brightness); int (*wled_sync_toggle)(struct wled *wled);

}; @@ -209,6 +226,27 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) return 0; }

+static void wled_ovp_work(struct work_struct *work) +{

• u32 val;
• int rc;
• struct wled *wled = container_of(work,

• 			 struct wled, ovp_work.work);
  

• rc = regmap_read(wled->regmap, wled->ctrl_addr +

WLED3_CTRL_REG_MOD_EN,

• 	 &val);
  

• if (rc < 0)

• return;
  

• if (val & WLED3_CTRL_REG_MOD_EN_MASK) {

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

Ok. I will modify as per your suggestion in the next series.

...

• if (wled->ovp_irq > 0 && wled->ovp_irq_disabled) {
  

• 	enable_irq(wled->ovp_irq);
  

• 	wled->ovp_irq_disabled = false;
  

• }
  

• }

+}

static int wled_module_enable(struct wled *wled, int val) { int rc; @@ -220,7 +258,20 @@ static int wled_module_enable(struct wled *wled, int val) WLED3_CTRL_REG_MOD_EN, WLED3_CTRL_REG_MOD_EN_MASK, WLED3_CTRL_REG_MOD_EN_MASK);

• return rc;

• if (rc < 0)

• return rc;
  

• if (val) {

• schedule_delayed_work(&wled->ovp_work, WLED_SOFT_START_DLY_US);
  

• } else {

• cancel_delayed_work(&wled->ovp_work);
  

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

Sure. I will modify it in next series.

...

• if (wled->ovp_irq > 0 && !wled->ovp_irq_disabled) {
  

• 	disable_irq(wled->ovp_irq);
  

• 	wled->ovp_irq_disabled = true;
  

• }
  

• }
• return 0;

}

static int wled3_sync_toggle(struct wled *wled) @@ -346,6 +397,305 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled) return IRQ_HANDLED; }

+#define AUTO_DETECT_BRIGHTNESS 200 +static int wled_auto_string_detection(struct wled *wled)

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

I will make it void in the next series.

...

+{

• int rc = 0, i;
• u32 sink_config = 0, int_sts;
• u8 sink_test = 0, sink_valid = 0, val;
• /* read configured sink configuration */
• rc = regmap_read(wled->regmap, wled->sink_addr +

• 	 WLED4_SINK_REG_CURR_SINK, &sink_config);
  

• if (rc < 0) {

• pr_err("Failed to read SINK configuration rc=%d\n", rc);
  

• goto failed_detect;
  

• }
• /* disable the module before starting detection */
• rc = regmap_update_bits(wled->regmap,

• 		wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
  

• 		WLED3_CTRL_REG_MOD_EN_MASK, 0);
  

• if (rc < 0) {

• pr_err("Failed to disable WLED module rc=%d\n", rc);
  

• goto failed_detect;
  

• }
• /* set low brightness across all sinks */
• rc = wled4_set_brightness(wled, AUTO_DETECT_BRIGHTNESS);
• if (rc < 0) {

• pr_err("Failed to set brightness for auto detection rc=%d\n",
  

•        rc);
  

• goto failed_detect;
  

• }
• if (wled->cfg.cabc) {

• for (i = 0; i < wled->cfg.num_strings; i++) {
  

• 	rc = regmap_update_bits(wled->regmap, wled->sink_addr +
  

• 				WLED4_SINK_REG_STR_CABC(i),
  

• 				WLED4_SINK_REG_STR_CABC_MASK,
  

• 				0);
  

• 	if (rc < 0)
  

• 		goto failed_detect;
  

• }
  

• }
• /* disable all sinks */
• rc = regmap_write(wled->regmap,

• 	  wled->sink_addr + WLED4_SINK_REG_CURR_SINK, 0);
  

• if (rc < 0) {

• pr_err("Failed to disable all sinks rc=%d\n", rc);
  

• goto failed_detect;
  

• }
• /* iterate through the strings one by one */
• for (i = 0; i < wled->cfg.num_strings; i++) {

• sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i));
  

• /* Enable feedback control */
  

• rc = regmap_write(wled->regmap, wled->ctrl_addr +
  

• 		  WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1);
  

• if (rc < 0) {
  

• 	pr_err("Failed to enable feedback for SINK %d rc = %d\n",
  

• 	       i + 1, rc);
  

• 	goto failed_detect;
  

• }
  

• /* enable the sink */
  

• rc = regmap_write(wled->regmap, wled->sink_addr +
  

• 		  WLED4_SINK_REG_CURR_SINK, sink_test);
  

• if (rc < 0) {
  

• 	pr_err("Failed to configure SINK %d rc=%d\n", i + 1,
  

• 	       rc);
  

• 	goto failed_detect;
  

• }
  

• /* Enable the module */
  

• rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
  

• 			WLED3_CTRL_REG_MOD_EN,
  

• 			WLED3_CTRL_REG_MOD_EN_MASK,
  

• 			WLED3_CTRL_REG_MOD_EN_MASK);
  

• if (rc < 0) {
  

• 	pr_err("Failed to enable WLED module rc=%d\n", rc);
  

• 	goto failed_detect;
  

• }
  

• usleep_range(WLED_SOFT_START_DLY_US,
  

• 	     WLED_SOFT_START_DLY_US + 1000);
  

• rc = regmap_read(wled->regmap, wled->ctrl_addr +
  

• 		 WLED3_CTRL_REG_INT_RT_STS, &int_sts);
  

• if (rc < 0) {
  

• 	pr_err("Error in reading WLED3_CTRL_INT_RT_STS rc=%d\n",
  

• 	       rc);
  

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

This is a spmi read and it is very unlikely to fail. If it fails it leads the complete system to go in to bad state.

...

• 	goto failed_detect;
  

• }
  

• if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
  

• 	pr_debug("WLED OVP fault detected with SINK %d\n",
  

• 		 i + 1);
  

• else
  

• 	sink_valid |= sink_test;
  

• /* Disable the module */
  

• rc = regmap_update_bits(wled->regmap,
  

• 			wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
  

• 			WLED3_CTRL_REG_MOD_EN_MASK, 0);
  

• if (rc < 0) {
  

• 	pr_err("Failed to disable WLED module rc=%d\n", rc);
  

• 	goto failed_detect;
  

• }
  

• }
• if (sink_valid == sink_config) {

• pr_debug("WLED auto-detection complete, default sink-config=%x 
  

OK!\n",

• 	 sink_config);
  

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

I will correct it in the next series.

...

• } else {

• pr_warn("Invalid WLED default sink config=%x changing it to=%x\n",
  

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

I will modify it in the next series. I missed to modify here.

...

• 	sink_config, sink_valid);
  

• sink_config = sink_valid;
  

• }
• if (!sink_config) {

• pr_err("No valid WLED sinks found\n");
  

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

Sure. I will move it in the next series.

...

• wled->disabled_by_short = true;
  

• goto failed_detect;
  

• }
• /* write the new sink configuration */
• rc = regmap_write(wled->regmap,

• 	  wled->sink_addr + WLED4_SINK_REG_CURR_SINK,
  

• 	  sink_config);
  

• if (rc < 0) {

• pr_err("Failed to reconfigure the default sink rc=%d\n", rc);
  

• goto failed_detect;
  

• }
• /* Enable valid sinks */
• for (i = 0; i < wled->cfg.num_strings; i++) {

• if (wled->cfg.cabc) {
  

• 	rc = regmap_update_bits(wled->regmap, wled->sink_addr +
  

• 				WLED4_SINK_REG_STR_CABC(i),
  

• 				WLED4_SINK_REG_STR_CABC_MASK,
  

• 				WLED4_SINK_REG_STR_CABC_MASK);
  

• 	if (rc < 0)
  

• 		goto failed_detect;
  

• }
  

• if (sink_config & BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
  

• 	val = WLED4_SINK_REG_STR_MOD_MASK;
  

• else
  

• 	val = 0x0; /* disable modulator_en for unused sink */
  

• rc = regmap_write(wled->regmap, wled->sink_addr +
  

• 		  WLED4_SINK_REG_STR_MOD_EN(i), val);
  

• if (rc < 0) {
  

• 	pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc);
  

• 	goto failed_detect;
  

• }
  

• }
• /* restore the feedback setting */
• rc = regmap_write(wled->regmap,

• 	  wled->ctrl_addr + WLED3_CTRL_REG_FEEDBACK_CONTROL, 0);
  

• if (rc < 0) {

• pr_err("Failed to restore feedback setting rc=%d\n", rc);
  

• goto failed_detect;
  

• }
• /* restore brightness */

Extra space

Will remove it in the next series.

...

• rc = wled4_set_brightness(wled, wled->brightness);
• if (rc < 0) {

• pr_err("Failed to set brightness after auto detection rc=%d\n",
  

•        rc);
  

• goto failed_detect;
  

• }
• rc = regmap_update_bits(wled->regmap,

• 		wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN,
  

• 		WLED3_CTRL_REG_MOD_EN_MASK,
  

• 		WLED3_CTRL_REG_MOD_EN_MASK);
  

• if (rc < 0) {

• pr_err("Failed to enable WLED module rc=%d\n", rc);
  

• goto failed_detect;
  

• }

+failed_detect:

• return rc;

+}

+#define WLED_AUTO_DETECT_OVP_COUNT 5 +#define WLED_AUTO_DETECT_CNT_DLY_US USEC_PER_SEC +static bool wled_auto_detection_required(struct wled *wled) +{

• s64 elapsed_time_us;
• if (*wled->version == WLED_PM8941 ||

!wled->cfg.auto_detection_enabled)

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

I will change it in the next series.

...

• return false;
  

• /*

• * Check if the OVP fault was an occasional one
  

• * or if it's firing continuously, the latter qualifies
  

• * for an auto-detection check.
  

• */
  

• if (!wled->auto_detection_ovp_count) {

• wled->start_ovp_fault_time = ktime_get();
  

• wled->auto_detection_ovp_count++;
  

• } else {

• elapsed_time_us = ktime_us_delta(ktime_get(),
  

• 				 wled->start_ovp_fault_time);
  

• if (elapsed_time_us > WLED_AUTO_DETECT_CNT_DLY_US)
  

• 	wled->auto_detection_ovp_count = 0;
  

• else
  

• 	wled->auto_detection_ovp_count++;
  

• if (wled->auto_detection_ovp_count >=
  

• 		WLED_AUTO_DETECT_OVP_COUNT) {
  

• 	wled->auto_detection_ovp_count = 0;
  

• 	return true;
  

• }
  

• }
• return false;

+}

+static int wled_auto_detection_at_init(struct wled *wled) +{

• int rc;
• u32 fault_status = 0, rt_status = 0;

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

I will remove it in the next series.

...

• if (!wled->cfg.auto_detection_enabled)

• return 0;
  

[..]

...

@@ -836,6 +1192,42 @@ static int wled_configure_short_irq(struct wled *wled, return rc; }

+static int wled_configure_ovp_irq(struct wled *wled,

• 		  struct platform_device *pdev)
  

+{

• int rc = 0;

No need to initialize rc here.

I will remove it in the next series.

...

• u32 val;
• wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
• if (wled->ovp_irq < 0) {

• dev_dbg(&pdev->dev, "ovp irq is not used\n");
  

• return 0;
  

• }
• rc = devm_request_threaded_irq(wled->dev, wled->ovp_irq, NULL,

• 		       wled_ovp_irq_handler, IRQF_ONESHOT,
  

• 		       "wled_ovp_irq", wled);
  

• if (rc < 0) {

• dev_err(wled->dev, "Unable to request ovp_irq (err:%d)\n",
  

• 	rc);
  

• wled->ovp_irq = 0;
  

• return 0;
  

• }
• rc = regmap_read(wled->regmap, wled->ctrl_addr +

• 	 WLED3_CTRL_REG_MOD_EN, &val);
  

• if (rc < 0)

• return rc;
  

• /* Keep OVP irq disabled until module is enabled */
• if (!rc && !(val & WLED3_CTRL_REG_MOD_EN_MASK)) {

If rc isn't 0 we returned above.

I will remove it in the next series.

...

• disable_irq(wled->ovp_irq);
  

• wled->ovp_irq_disabled = true;
  

• }
• return rc;

As such, it is always 0 here.

I will modify it in the next series.

...

+}

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