Re: [PATCH] backlight: qcom-wled: Use sink_addr for sync toggle

[kgunda]

On 2021-03-15 17:51, Daniel Thompson wrote:

On Sun, Mar 14, 2021 at 11:11:10AM +0100, Marijn Suijten wrote:

From: Obeida Shamoun 

WLED3_SINK_REG_SYNC is, as the name implies, a sink register offset.
Therefore, use the sink address as base instead of the ctrl address.

This fixes the sync toggle on wled4, which can be observed by the fact
that adjusting brightness now works.

It has no effect on wled3 because sink and ctrl base addresses are the
same.  This allows adjusting the brightness without having to disable
then reenable the module.

Signed-off-by: Obeida Shamoun 
Signed-off-by: Konrad Dybcio 
Signed-off-by: Marijn Suijten 


LGTM, although an acked-by from Kiran would be nice to have:
Reviewed-by: Daniel Thompson 


Daniel.



Acked-by: Kiran Gunda 


---
 drivers/video/backlight/qcom-wled.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index 091f07e7c145..fc8b443d10fd 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -336,13 +336,13 @@ static int wled3_sync_toggle(struct wled *wled)
unsigned int mask = GENMASK(wled->max_string_count - 1, 0);

rc = regmap_update_bits(wled->regmap,
-   wled->ctrl_addr + WLED3_SINK_REG_SYNC,
+   wled->sink_addr + WLED3_SINK_REG_SYNC,
mask, mask);
if (rc < 0)
return rc;

rc = regmap_update_bits(wled->regmap,
-   wled->ctrl_addr + WLED3_SINK_REG_SYNC,
+   wled->sink_addr + WLED3_SINK_REG_SYNC,
mask, WLED3_SINK_REG_SYNC_CLEAR);

return rc;
--
2.30.2


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

Re: [PATCH V3 2/2] backlight: qcom-wled: Correct the sync_toggle sequence

[kgunda]

On 2021-03-01 15:32, Daniel Thompson wrote:

On Mon, Mar 01, 2021 at 02:58:36PM +0530, Kiran Gunda wrote:

As per the current implementation, after FSC (Full Scale Current)
and brightness update the sync bits are transitioned from 
set-then-clear.


This does not makes sense since there are too many verbs. Set and clear
are both verbs so in this context: "the code will set the bit and then
the code will clear the bit".

Either:

s/transitioned from set-then-clear/set-then-cleared/.

Or:

s/transitioned from set-then-clear/using a set-then-clear approach/.


But, the FSC and brightness sync takes place during a clear-then-set
transition of the sync bits.


Likewise this no longer makes sense and had also become misleading.
Two changes of state, clear and then set, do not usually result in a
single transition.

Either:

s/clear-then-set/0 to 1/

Alternatively, if you want to stick exclusively to the set/clear
terminology then replace the whole quoted section with:

  But, the FSC and brightness sync takes place when the sync bits are
  set (e.g. on a rising edge).



So the hardware team recommends a
clear-then-set approach in order to guarantee such a transition
regardless of the previous register state.

Signed-off-by: Kiran Gunda 


With one of each of the changes proposed above:
Reviewed-by: Daniel Thompson 


Daniel.


Apologies for the mistake. I have corrected and submitted the
V4 series with the "reviewed-by" tag.



---
 drivers/video/backlight/qcom-wled.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

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

index aef52b9..19f83ac 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled)

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, mask);
+   mask, WLED3_SINK_REG_SYNC_CLEAR);
if (rc < 0)
return rc;

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, WLED3_SINK_REG_SYNC_CLEAR);
+   mask, mask);

return rc;
 }
@@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled 
*wled)

int rc;
u8 val;

-   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
-WLED5_SINK_REG_SYNC_MOD_B_BIT;
rc = regmap_update_bits(wled->regmap,
wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
-   WLED5_SINK_REG_SYNC_MASK, val);
+   WLED5_SINK_REG_SYNC_MASK, 0);
if (rc < 0)
return rc;

+   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
+WLED5_SINK_REG_SYNC_MOD_B_BIT;
return regmap_update_bits(wled->regmap,
  wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
- WLED5_SINK_REG_SYNC_MASK, 0);
+ WLED5_SINK_REG_SYNC_MASK, val);
 }

 static int wled_ovp_fault_status(struct wled *wled, bool *fault_set)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V2 2/2] backlight: qcom-wled: Correct the sync_toggle sequence

[kgunda]

On 2021-02-26 22:56, Daniel Thompson wrote:

On Fri, Feb 26, 2021 at 05:42:24PM +0530, Kiran Gunda wrote:

As per the current implementation, after FSC (Full Scale Current)
and brightness update the sync bits are transitioned from 1 to 0.


This still seems to incorrectly describe the current behaviour.

Surely in most cases (i.e. every time except the first) the value of 
the

sync bit is 0 when the function is called and we get both a 0 to 1
and then a 1 to 0 transition.

That is why I recommended set-then-clear terminology to describe the
current behaviour. It is concise and correct.


Daniel.



Okay. Actually I have mentioned the "clear-and-set" in explaining the 
fix.

Let me modify the same terminology in explaining the problem case also.


But, the FSC and brightness sync takes place during a 0 to 1
transition of the sync bits. So the hardware team recommends a
clear-then-set approach in order to guarantee such a transition
regardless of the previous register state.

Signed-off-by: Kiran Gunda 
---
 drivers/video/backlight/qcom-wled.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

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

index aef52b9..19f83ac 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled)

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, mask);
+   mask, WLED3_SINK_REG_SYNC_CLEAR);
if (rc < 0)
return rc;

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, WLED3_SINK_REG_SYNC_CLEAR);
+   mask, mask);

return rc;
 }
@@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled 
*wled)

int rc;
u8 val;

-   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
-WLED5_SINK_REG_SYNC_MOD_B_BIT;
rc = regmap_update_bits(wled->regmap,
wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
-   WLED5_SINK_REG_SYNC_MASK, val);
+   WLED5_SINK_REG_SYNC_MASK, 0);
if (rc < 0)
return rc;

+   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
+WLED5_SINK_REG_SYNC_MOD_B_BIT;
return regmap_update_bits(wled->regmap,
  wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
- WLED5_SINK_REG_SYNC_MASK, 0);
+ WLED5_SINK_REG_SYNC_MASK, val);
 }

 static int wled_ovp_fault_status(struct wled *wled, bool *fault_set)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V1 2/2] backlight: qcom-wled: Correct the sync_toggle sequence

[kgunda]

On 2021-02-24 16:56, Daniel Thompson wrote:

On Wed, Feb 24, 2021 at 09:20:48AM +0530, Kiran Gunda wrote:

Currently the FSC SYNC_BIT and MOD_SYNC_BIT are toggled
from 1 to 0 to update the FSC and brightenss settings.
Change this sequence form 0 to 1 as per the hardware team
recommendation to update the FSC and brightness correctly.


Again... this patch description feels somewhat rushed. A patch
description is there to support code reviewer and to go on the version
history to assist with future maintainance. They matter!

Anyhow I don't recognise the "from 1 to 0" in the code since both 
before

an after the change it goes "from 0 to 1" and "from 1 to 0" but in a
different order. Doesn't the code actually currently implement "set 
then

clear"? If so then, likewise the new code is adopting "clear then set".


I would have used "set" and "clear" instead of "0" and "1".
Yes. The current code implementation is "set" all SYN bits and then 
"clear"
all SYNC bits. The new code is modified to change the sequence from 
"clear"

first and then "set" to ensure both FSC and brightness are updated.


As with patch 1, the sync bits modified by wled3_sync_toggle singular
or plural?

It is plural. We have to "clear" and "set" all sync bits.


Finally a description that is more sympathetic to the reviewer would be
welcome.  For example the following (if my guess is right and it is
true) makes things much easier for the reviewer:
Sure. I will update the documentation and patch description clearly.
  "The sync takes place during a 0 to 1 transition of the sync
  bits so the hardware team recommends a clear-then-set approach in
  order to guarantee such a transition regardless of the previous
  register state".


Daniel.




Signed-off-by: Kiran Gunda 
---
 drivers/video/backlight/qcom-wled.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

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

index aef52b9..19f83ac 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled)

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, mask);
+   mask, WLED3_SINK_REG_SYNC_CLEAR);
if (rc < 0)
return rc;

rc = regmap_update_bits(wled->regmap,
wled->ctrl_addr + WLED3_SINK_REG_SYNC,
-   mask, WLED3_SINK_REG_SYNC_CLEAR);
+   mask, mask);

return rc;
 }
@@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled 
*wled)

int rc;
u8 val;

-   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
-WLED5_SINK_REG_SYNC_MOD_B_BIT;
rc = regmap_update_bits(wled->regmap,
wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
-   WLED5_SINK_REG_SYNC_MASK, val);
+   WLED5_SINK_REG_SYNC_MASK, 0);
if (rc < 0)
return rc;

+   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
+WLED5_SINK_REG_SYNC_MOD_B_BIT;
return regmap_update_bits(wled->regmap,
  wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
- WLED5_SINK_REG_SYNC_MASK, 0);
+ WLED5_SINK_REG_SYNC_MASK, val);
 }

 static int wled_ovp_fault_status(struct wled *wled, bool *fault_set)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V1 1/2] backlight: qcom-wled: Fix FSC update issue for WLED5

[kgunda]

On 2021-02-24 16:45, Daniel Thompson wrote:

On Wed, Feb 24, 2021 at 09:20:47AM +0530, Kiran Gunda wrote:

Currently, for WLED5, after FSC register update MOD_SYNC_BIT
is toggled instead of SYNC_BIT. MOD_SYNC_BIT has to be toggled
after the brightness update and SYNC_BIT has to be toggled after
FSC update for WLED5. Fix it.


Code looks fine but the description is a difficult to read (which makes
mining the history difficult).

Basically the descriptions here are very hard to read without the
context in PATCH 0/2. Since PATCH 0/2 won't enter the version history
that means these descriptions need to integrate some of the text from
what is currently PATCH 0/2.

I would expect this to be more like. It is basically joining together
text from PATCH 0 and PATCH 1 (I also switched to plural form for SYNC
bits... the code in the driver has mask generation based on the number
of strings, is that right?):

Sorry for the trouble. Yes, you are correct. The mask generation is
based on the number of strings defined in the device tree and only those
strings are enabled. However, there is no issue if the SYNC bits of all
the strings are cleared/set. The SYNC takes place only for enabled 
strings.



~~~
Currently, for WLED5, the FSC (Full scale current) setting is not
updated properly due to driver toggling the wrong register after an FSC
update.

On WLED5 we should only toggle the MOD_SYNC bit after a brightness
update. For an FSC update we need to toggle the SYNC bits instead.

Fix it by adopting the common wled3_sync_toggle() for WLED5 and
introducing new code to the brightness update path to
compensate.
~~~
I will update the Documentation/patch description clearly

as suggested.


Daniel.





Signed-off-by: Kiran Gunda 
---
 drivers/video/backlight/qcom-wled.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

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

index 3bc7800..aef52b9 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -348,7 +348,7 @@ static int wled3_sync_toggle(struct wled *wled)
return rc;
 }

-static int wled5_sync_toggle(struct wled *wled)
+static int wled5_mod_sync_toggle(struct wled *wled)
 {
int rc;
u8 val;
@@ -445,10 +445,23 @@ static int wled_update_status(struct 
backlight_device *bl)

goto unlock_mutex;
}

-   rc = wled->wled_sync_toggle(wled);
-   if (rc < 0) {
-   dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
-   goto unlock_mutex;
+   if (wled->version < 5) {
+   rc = wled->wled_sync_toggle(wled);
+   if (rc < 0) {
+   dev_err(wled->dev, "wled sync failed rc:%d\n", 
rc);
+   goto unlock_mutex;
+   }
+   } else {
+   /*
+* For WLED5 toggling the MOD_SYNC_BIT updates the
+* brightness
+*/
+   rc = wled5_mod_sync_toggle(wled);
+   if (rc < 0) {
+   dev_err(wled->dev, "wled mod sync failed 
rc:%d\n",
+   rc);
+   goto unlock_mutex;
+   }
}
}

@@ -1459,7 +1472,7 @@ static int wled_configure(struct wled *wled)
size = ARRAY_SIZE(wled5_opts);
*cfg = wled5_config_defaults;
wled->wled_set_brightness = wled5_set_brightness;
-   wled->wled_sync_toggle = wled5_sync_toggle;
+   wled->wled_sync_toggle = wled3_sync_toggle;
wled->wled_cabc_config = wled5_cabc_config;
wled->wled_ovp_delay = wled5_ovp_delay;
wled->wled_auto_detection_required =
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V1 0/2] Fix WLED FSC Sync and brightness Sync settings

[kgunda]

On 2021-02-19 23:38, Pavel Machek wrote:

Hi!

The FSC (Full scale current) setting is not updated properly due to 
the
wrong register toggling for WLED5. Also the ILED_SYNC toggle and 
MOD_SYNC
toggle sequence is updated as per the hardware team recommendation to 
fix

the FSC update and brightness update issue.


If this is phone hardware, it might make sense to cc:
phone-devel@vger...


Yes. it is for phone hardware. Will cc the email.

Best regards,
Pavel

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

Re: [PATCH V5 3/4] backlight: qcom-wled: Add WLED5 bindings

[kgunda]

On 2020-04-15 20:42, Rob Herring wrote:

On Tue, Apr 07, 2020 at 09:17:09PM +0530, Kiran Gunda wrote:

Add WLED5 specific bindings.



checkpatch.pl complains about some trailing whitespace. The previous
patch too.


Signed-off-by: Kiran Gunda 
Signed-off-by: Subbaraman Narayanamurthy 
---
 .../bindings/leds/backlight/qcom-wled.yaml | 60 
--

 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

index 770e780..5714631 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -21,6 +21,7 @@ properties:
   - qcom,pm8941-wled
   - qcom,pmi8998-wled
   - qcom,pm660l-wled
+  - qcom,pm8150l-wled

   reg:
 maxItems: 1
@@ -28,12 +29,13 @@ properties:
   default-brightness:
 description:
   brightness value on boot.
-minimum: 0
-maximum: 4095
-default: 2048

   label: true

+  max-brightness:
+description:
+  Maximum brightness level.
+
   qcom,cs-out:
 description:
   enable current sink output.
@@ -130,6 +132,31 @@ properties:
   This feature is not supported for WLED3.
 type: boolean

+  qcom,modulator-sel:
+description:


Need a '|' at the end to preserve formatting.


+  Selects the modulator used for brightness modulation.
+  Allowed values are,
+   0 - Modulator A
+   1 - Modulator B
+  This property is applicable only to WLED5 peripheral.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+  - enum: [ 0, 1 ]
+  - default: 0
+
+  qcom,cabc-sel:
+description:


Need a '|'.


+  Selects the CABC pin signal used for brightness modulation.
+  Allowed values are,
+   0 - CABC disabled
+   1 - CABC 1
+   2 - CABC 2
+   3 - External signal (e.g. LPG) is used for dimming
+  This property is applicable only to WLED5 peripheral.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+  - enum: [ 0, 1, 2, 3 ]
+
 allOf:
   - if:
   properties:
@@ -179,6 +206,33 @@ allOf:
 - const: ovp
 - const: short

+  - if:
+  properties:
+compatible:
+  contains:
+enum:
+  - qcom,pm8150l-wled
+
+then:
+  properties:
+default-brightness:
+  minimum: 0
+  maximum: 32767
+
+max-brightness:
+  minimum: 0
+  maximum: 32767
+
+else:
+  properties:
+default-brightness:
+minimum: 0
+maximum: 4095


Wrong indentation.


+
+max-brightness:
+  minimum: 0
+  maximum: 4095
+
 required:
   - compatible
   - reg
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project

Thanks for reviewing. I will submit the next revision with all the 
fixes.

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

Re: [PATCH V5 1/4] backlight: qcom-wled: convert the wled bindings to .yaml format

[kgunda]

On 2020-04-15 20:39, Rob Herring wrote:

On Tue, Apr 07, 2020 at 09:17:07PM +0530, Kiran Gunda wrote:

Convert the qcom-wled bindings from .txt to .yaml format.
Also replace PM8941 to WLED3 and PMI8998 to WLED4.

Signed-off-by: Kiran Gunda 
Signed-off-by: Subbaraman Narayanamurthy 
Acked-by: Daniel Thompson 
---
 .../bindings/leds/backlight/qcom-wled.txt  | 154 

 .../bindings/leds/backlight/qcom-wled.yaml | 201 
+

 2 files changed, 201 insertions(+), 154 deletions(-)
 delete mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml



diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

new file mode 100644
index 000..770e780
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -0,0 +1,201 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/qcom-wled.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for Qualcomm Technologies, Inc. WLED driver
+
+maintainers:
+  - Bjorn Andersson 
+  - Kiran Gunda 
+
+description: |
+  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.

+
+properties:
+  compatible:
+enum:
+  - qcom,pm8941-wled
+  - qcom,pmi8998-wled
+  - qcom,pm660l-wled
+
+  reg:
+maxItems: 1
+
+  default-brightness:
+description:
+  brightness value on boot.
+minimum: 0
+maximum: 4095
+default: 2048
+
+  label: true
+
+  qcom,cs-out:
+description:
+  enable current sink output.
+  This property is supported only for WLED3.
+type: boolean
+
+  qcom,cabc:
+description:
+  enable content adaptive backlight control.
+type: boolean
+
+  qcom,ext-gen:
+description:
+  use externally generated modulator signal to dim.
+  This property is supported only for WLED3.
+type: boolean
+
+  qcom,current-limit:
+description:
+  mA; per-string current limit.
+  This property is supported only for WLED3.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+default: 20
+minimum: 0
+maximum: 25
+multipleOf: 1


No point in defining a multiple of 1.


+
+  qcom,current-limit-microamp:
+description:
+  uA; per-string current limit.
+default: 25
+minimum: 0
+maximum: 3
+multipleOf: 25
+
+  qcom,current-boost-limit:
+description:
+  mA; boost current limit.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+
+  qcom,switching-freq:
+description:
+  kHz; switching frequency.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+  - enum: [ 600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371, 
1600, 1920, 2400, 3200, 4800, 9600 ]

+
+  qcom,ovp:
+description:
+  V; Over-voltage protection limit.
+  This property is supported only for WLED3.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+  - enum: [ 27, 29, 32, 35 ]
+  - default: 29
+
+  qcom,ovp-millivolt:
+description:
+  Over-voltage protection limit. This property is for WLED4 only.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+  - enum: [ 18100, 19600, 29600, 31100 ]
+  - default: 29600
+
+  qcom,num-strings:
+description:
+  number of led strings attached.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+
+  qcom,enabled-strings:
+description:
+  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.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32-array
+minItems: 1
+maxItems: 4
+
+  qcom,external-pfet:
+description:
+  Specify if external PFET control for short circuit
+  protection is used. This property is supported only
+  for WLED4.
+type: boolean
+
+  qcom,auto-string-detection:
+description:
+  Enables auto-detection of the WLED string configuration.
+  This feature is not supported for WLED3.
+type: boolean
+
+allOf:
+  - if:
+  properties:
+compatible:
+  contains:
+const: qcom,pm8941-wled
+
+then:
+  properties:
+qcom,current-boost-limit:
+   enum: [ 105, 385, 525, 805, 980, 1260, 1400, 1680 ]
+   default: 805
+
+qcom,switching-freq:
+   default: 1600
+
+qcom,num-strings:
+   enum: [ 1, 2, 3 ]
+
+interrupts:
+  items:
+- description: over voltage protection interrupt.
+
+   

Re: [PATCH V4 1/4] backlight: qcom-wled: convert the wled bindings to .yaml format

[kgunda]

On 2020-04-06 14:20, Lee Jones wrote:

On Fri, 03 Apr 2020, Daniel Thompson wrote:


On Fri, Apr 03, 2020 at 04:45:49PM +0530, kgu...@codeaurora.org wrote:
> On 2020-03-31 23:24, Rob Herring wrote:
> > On Mon, Mar 23, 2020 at 11:16:55PM +0530, Kiran Gunda wrote:
> > > diff --git
> > > a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> > > b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> > > new file mode 100644
> > > index 000..8a388bf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
> > > @@ -0,0 +1,184 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/backlight/qcom-wled.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Binding for Qualcomm Technologies, Inc. WLED driver
> > > +
> > > +maintainers:
> > > +  - Lee Jones 
> >
> > Should be the h/w owner (you), not who applies patches.
> >
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.
> 
> will address in next post.

If you agree on all points raised I doubt there is any need for a 
point

by point reply since everyone who reads it will have to scroll down
simply to find out that you agree on all points.

Better just to acknowledge the feedback and reply to the first one
saying you'll agree on all points and will address all feedback in the
next revision (and then trim the reply to keep it short).


Or better still, just submit the next revision with all the fixes. :)

Noted.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V4 3/4] backlight: qcom-wled: Add WLED5 bindings

[kgunda]

On 2020-03-31 23:26, Rob Herring wrote:

On Mon, Mar 23, 2020 at 11:16:57PM +0530, Kiran Gunda wrote:

Add WLED5 specific bindings.



More of the same comments here...


Signed-off-by: Kiran Gunda 
Signed-off-by: Subbaraman Narayanamurthy 
---
 .../bindings/leds/backlight/qcom-wled.yaml | 39 
++

 1 file changed, 39 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

index 8a388bf..159115f 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -20,6 +20,7 @@ properties:
- qcom,pm8941-wled
- qcom,pmi8998-wled
- qcom,pm660l-wled
+   - qcom,pm8150l-wled

   reg:
 maxItems: 1
@@ -28,10 +29,23 @@ properties:
 maxItems: 1
 description:
   brightness value on boot, value from 0-4095.
+  For pm8150l this value vary from 0-4095 or 0-32767
+  depending on the brightness control mode. If CABC is
+  enabled 0-4095 range is used.


Constraints.


Will address it in next post.

 allOf:
   - $ref: /schemas/types.yaml#/definitions/uint32
 default: 2048

+  max-brightness:
+maxItems: 1
+description:
+  Maximum brightness level. Allowed values are,
+  for pmi8998 it is  0-4095.
+  For pm8150l, this can be either 4095 or 32767.


Constraints!


Will address it in next post.

+  If CABC is enabled, this is capped to 4095.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32


Standard property. Assume it has a type definition.'


Will address it in next post.

+
   label:
 maxItems: 1
 description:
@@ -124,6 +138,31 @@ properties:
   value for PM8941 from 1 to 3. Default 2
   For PMI8998 from 1 to 4.

+  qcom,modulator-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the modulator used for brightness modulation.
+  Allowed values are,
+   0 - Modulator A
+   1 - Modulator B
+  If not specified, then modulator A will be used by default.
+  This property is applicable only to WLED5 peripheral.
+
+  qcom,cabc-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the CABC pin signal used for brightness modulation.
+  Allowed values are,
+  0 - CABC disabled
+  1 - CABC 1
+  2 - CABC 2
+  3 - External signal (e.g. LPG) is used for dimming
+  This property is applicable only to WLED5 peripheral.
+
   interrupts:
 maxItems: 2
 description:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V4 1/4] backlight: qcom-wled: convert the wled bindings to .yaml format

[kgunda]

On 2020-03-31 23:24, Rob Herring wrote:

On Mon, Mar 23, 2020 at 11:16:55PM +0530, Kiran Gunda wrote:

Convert the qcom-wled bindings from .txt to .yaml format.

Signed-off-by: Kiran Gunda 
Signed-off-by: Subbaraman Narayanamurthy 
Acked-by: Daniel Thompson 
---
 .../bindings/leds/backlight/qcom-wled.txt  | 154 
-
 .../bindings/leds/backlight/qcom-wled.yaml | 184 
+

 2 files changed, 184 insertions(+), 154 deletions(-)
 delete mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml


diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

new file mode 100644
index 000..8a388bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -0,0 +1,184 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/qcom-wled.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for Qualcomm Technologies, Inc. WLED driver
+
+maintainers:
+  - Lee Jones 


Should be the h/w owner (you), not who applies patches.


will address in next post.

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

+
+properties:
+  compatible :


Drop the space ^


will address in next post.

+enum:
+   - qcom,pm8941-wled
+   - qcom,pmi8998-wled
+   - qcom,pm660l-wled


Wrong indent (1 space too many).


will address in next post.

+
+  reg:
+maxItems: 1
+
+  default-brightness:
+maxItems: 1


maxItems is for arrays and this is a single scalar.


+description:
+  brightness value on boot, value from 0-4095.


0-4095 sounds like a constraint.


will address in next post.

+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32


There should be a common definition for this.


will address in next post.

+default: 2048
+
+  label:
+maxItems: 1
+description:
+  The name of the backlight device.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/string


Already has a type. Just 'label: true' is enough.


will address in next post.

+
+  qcom,cs-out:
+description:
+  enable current sink output.
+  This property is supported only for PM8941.
+type: boolean
+
+  qcom,cabc:
+description:
+  enable content adaptive backlight control.
+type: boolean
+
+  qcom,ext-gen:
+description:
+  use externally generated modulator signal to dim.
+  This property is supported only for PM8941.
+type: boolean
+
+  qcom,current-limit:
+maxItems: 1


Not an array.


will address in next post.

+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  mA; per-string current limit; value from 0 to 25 with


25 sounds like a constraint.


will address in next post.

+  1 mA step. This property is supported only for pm8941.
+default: 20
+
+  qcom,current-limit-microamp:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32


properties with unit suffix already have a type.


will address in next post.

+description:
+  uA; per-string current limit; value from 0 to 3 with
+  2500 uA step.


steps can be expressed using 'multipleOf'


will address in next post.

+default: 25


25 can never be a multiple of 2500


will correct in next post.

+
+  qcom,current-boost-limit:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  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.


Constraints.


will address in next post.

+
+  qcom,switching-freq:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  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


Constraints!


will address in next post.

+
+  qcom,ovp:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  V; Over-voltage protection limit; one of 27, 29, 32, 35. 
Default 29V

+  This property is supported only for PM8941.
+
+  qcom,ovp-millivolt:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  mV; Over-voltage protection limit;
+  For pmi8998 one of 18100, 19600, 29600, 31100.
+  Default 29600 mV.
+  If this property is not 

Re: [PATCH V4 3/4] backlight: qcom-wled: Add WLED5 bindings

[kgunda]

On 2020-03-25 21:07, Daniel Thompson wrote:

On Mon, Mar 23, 2020 at 11:16:57PM +0530, Kiran Gunda wrote:

Add WLED5 specific bindings.

Signed-off-by: Kiran Gunda 
Signed-off-by: Subbaraman Narayanamurthy 
---
 .../bindings/leds/backlight/qcom-wled.yaml | 39 
++

 1 file changed, 39 insertions(+)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

index 8a388bf..159115f 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -20,6 +20,7 @@ properties:
- qcom,pm8941-wled
- qcom,pmi8998-wled
- qcom,pm660l-wled
+   - qcom,pm8150l-wled

   reg:
 maxItems: 1
@@ -28,10 +29,23 @@ properties:
 maxItems: 1
 description:
   brightness value on boot, value from 0-4095.
+  For pm8150l this value vary from 0-4095 or 0-32767
+  depending on the brightness control mode. If CABC is
+  enabled 0-4095 range is used.


I rather dislike some of the property descriptions using PMIC version
numbers to distinguish between peripheral versions and others using
WLEDx version numbers.

Could the property description be rephrased to use WLED3/4/5 
terminology

instead?


Sure. I will modify in the next post.



 allOf:
   - $ref: /schemas/types.yaml#/definitions/uint32
 default: 2048

+  max-brightness:
+maxItems: 1
+description:
+  Maximum brightness level. Allowed values are,
+  for pmi8998 it is  0-4095.
+  For pm8150l, this can be either 4095 or 32767.
+  If CABC is enabled, this is capped to 4095.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+


Similar thing here, is PMI8998 simply a synonym for WLED4 or there
something special about the PMIC versioning that requires it to be 
used?



Daniel.


Sure. It is synonym for WLED4. I will modify in the next post.



   label:
 maxItems: 1
 description:
@@ -124,6 +138,31 @@ properties:
   value for PM8941 from 1 to 3. Default 2
   For PMI8998 from 1 to 4.

+  qcom,modulator-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the modulator used for brightness modulation.
+  Allowed values are,
+   0 - Modulator A
+   1 - Modulator B
+  If not specified, then modulator A will be used by default.
+  This property is applicable only to WLED5 peripheral.
+
+  qcom,cabc-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the CABC pin signal used for brightness modulation.
+  Allowed values are,
+  0 - CABC disabled
+  1 - CABC 1
+  2 - CABC 2
+  3 - External signal (e.g. LPG) is used for dimming
+  This property is applicable only to WLED5 peripheral.
+
   interrupts:
 maxItems: 2
 description:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project

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

Re: [PATCH V4 2/4] backlight: qcom-wled: Add callback functions

[kgunda]

On 2020-03-25 21:00, Daniel Thompson wrote:

On Mon, Mar 23, 2020 at 11:16:56PM +0530, Kiran Gunda wrote:

Add wled_cabc_config, wled_sync_toggle, wled_ovp_fault_status
and wled_ovp_delay callback functions to prepare the driver for
adding WLED5 support.

wled_cabc_config() ===> Used to configure the cabc register.
 It is applicable for wled4 and wled5.

wled_sync_toggle() ===> used to toggle the Sync register bit for the
brightness update to take place.
It is applicable for WLED3, WLED4 and WLED5.

wled_ovp_fault_status() ===> Used to determine if the OVP fault is 
triggered.

 It is applicable for WLED4 and WLED5.

wled_ovp_delay() ===> Provides the time to wait before checking the 
OVP status

after wled module enable.
It is applicable for WLED4 and WLED5.


These look like comments to me. Move them out of the patch header and
make them into real comments!


Sure. Will do it in next post.



Signed-off-by: Kiran Gunda 


This patch does not compile. Please fix this.



Sorry for that. Some how it didn't show up in my compilation.
Will fix it in next post.

---
 drivers/video/backlight/qcom-wled.c | 188 
++--

 1 file changed, 118 insertions(+), 70 deletions(-)

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

index 3d276b3..a3daf9e 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -147,6 +147,7 @@ struct wled {
u32 max_brightness;
u32 short_count;
u32 auto_detect_count;
+   u32 version;


Why does some of the changes here use function pointers and other parts
use if/else networks (wled->version == X) ?


There is one place (in auto_string_detection) where we have to add the
if (version == 4) because the per string modulator_enable is present 
only

for WLED4 and there is no equivalent functionality available for WLED5.

There is another place, that i can convert it to the function pointer 
(auto_detection_required).



Overall I almost wonder if the reduced clarify that comes from function
pointers is actually adding much value?

I believe adding the function pointer makes it cleaner and the same 
approach is used for

WLED4/WLED3 also.




bool disabled_by_short;
bool has_short_detect;
int short_irq;
@@ -155,6 +156,10 @@ struct wled {
struct wled_config cfg;
struct delayed_work ovp_work;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+   int (*wled_cabc_config)(struct wled *wled, bool enable);
+   int (*wled_sync_toggle)(struct wled *wled);
+   int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set);
+   int (*wled_ovp_delay)(struct wled *wled);
 };

 static int wled3_set_brightness(struct wled *wled, u16 brightness)
@@ -237,7 +242,7 @@ static int wled_module_enable(struct wled *wled, 
int val)

return 0;
 }

-static int wled_sync_toggle(struct wled *wled)
+static int wled3_sync_toggle(struct wled *wled)
 {
int rc;
unsigned int mask = GENMASK(wled->max_string_count - 1, 0);
@@ -255,6 +260,46 @@ static int wled_sync_toggle(struct wled *wled)
return rc;
 }

+static int wled4_ovp_fault_status(struct wled *wled, bool *fault_set)
+{
+   int rc;
+   u32 int_rt_sts, fault_sts;
+
+   *fault_set = false;
+   rc = regmap_read(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_INT_RT_STS,
+   _rt_sts);
+   if (rc < 0) {
+   dev_err(wled->dev, "Failed to read INT_RT_STS rc=%d\n", rc);
+   return rc;
+   }
+
+   rc = regmap_read(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_FAULT_STATUS,
+   _sts);
+   if (rc < 0) {
+   dev_err(wled->dev, "Failed to read FAULT_STATUS rc=%d\n", rc);
+   return rc;
+   }
+
+   if (int_rt_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
+   *fault_set = true;
+
+   if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT)
+   *fault_set = true;
+
+   if (*fault_set)
+		dev_dbg(wled->dev, "WLED OVP fault detected, int_rt_sts=0x%x 
fault_sts=0x%x\n",

+   int_rt_sts, fault_sts);
+
+   return rc;
+}
+
+static int wled4_ovp_delay(struct wled *wled)
+{
+   return WLED_SOFT_START_DLY_US;
+}
+
 static int wled_update_status(struct backlight_device *bl)
 {
struct wled *wled = bl_get_data(bl);
@@ -275,7 +320,7 @@ static int wled_update_status(struct 
backlight_device *bl)

goto unlock_mutex;
}

-   rc = wled_sync_toggle(wled);
+   rc = wled->wled_sync_toggle(wled);
if (rc < 0) {
dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
goto 

Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions

[kgunda]

On 2020-03-11 16:00, Daniel Thompson wrote:

On Wed, Mar 11, 2020 at 12:11:00PM +0530, kgu...@codeaurora.org wrote:

On 2020-03-10 20:57, Daniel Thompson wrote:
> On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote:
> > Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay
> > callback functions to prepare the driver for adding WLED5 support.
> >
> > Signed-off-by: Kiran Gunda 
>
> Overall this code would a lot easier to review if
> > ---
> >  drivers/video/backlight/qcom-wled.c | 196
> > +++-
> >  1 file changed, 126 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> > index 3d276b3..b73f273 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -128,6 +128,7 @@ struct wled_config {
> >   bool cs_out_en;
> >   bool ext_gen;
> >   bool cabc;
> > + bool en_cabc;
>
> Does this ever get set to true?
>
Yes. If user wants use the cabc pin to control the brightness and
use the "qcom,cabc" DT property in the device tree.


That sounds like what you intended the code to do!

Is the code that does this present in the patch? I could not find
it.

okay... It's my bad. We already have the "cabc" for this. I will remove 
the en_cabc in

next series.


Daniel.

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

Re: [PATCH V3 2/4] backlight: qcom-wled: Add callback functions

[kgunda]

On 2020-03-10 20:57, Daniel Thompson wrote:

On Mon, Mar 09, 2020 at 06:56:00PM +0530, Kiran Gunda wrote:

Add cabc_config, sync_toggle, wled_ovp_fault_status and wled_ovp_delay
callback functions to prepare the driver for adding WLED5 support.

Signed-off-by: Kiran Gunda 


Overall this code would a lot easier to review if

---
 drivers/video/backlight/qcom-wled.c | 196 
+++-

 1 file changed, 126 insertions(+), 70 deletions(-)

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

index 3d276b3..b73f273 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -128,6 +128,7 @@ struct wled_config {
bool cs_out_en;
bool ext_gen;
bool cabc;
+   bool en_cabc;


Does this ever get set to true?


Yes. If user wants use the cabc pin to control the brightness and
use the "qcom,cabc" DT property in the device tree.


bool external_pfet;
bool auto_detection_enabled;
 };
@@ -147,14 +148,20 @@ struct wled {
u32 max_brightness;
u32 short_count;
u32 auto_detect_count;
+   u32 version;
bool disabled_by_short;
bool has_short_detect;
+   bool cabc_disabled;
int short_irq;
int ovp_irq;

struct wled_config cfg;
struct delayed_work ovp_work;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+   int (*cabc_config)(struct wled *wled, bool enable);
+   int (*wled_sync_toggle)(struct wled *wled);
+   int (*wled_ovp_fault_status)(struct wled *wled, bool *fault_set);
+   int (*wled_ovp_delay)(struct wled *wled);


Let's get some doc comments explaining what these callbacks do (and
which versions they apply to).


Sure. I will update it in the commit text in next post.


cabc_config() in particular appears to have a very odd interface for
wled4.  It looks like it relies on being initially called with enable
set a particular way and prevents itself from acting again. Therefore 
if

the comment you end up writing doesn't sound "right" then please also
fix the API!

Actually this variable is useful for WLED5, where the default HW state 
is
CABC1 enabled mode. So, if the user doesn't want to use the CABC we are 
configuring
the HW state to "0" based on the DT property and then setting a flag to 
not enable it again.

This is not needed for WLED4. I will remove it for WLED4 in next post.


Finally, why is everything except cabc_config() prefixed with wled?


It is typo. I will correct it in the next post.


Daniel.

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

Re: [PATCH V3 1/4] backlight: qcom-wled: convert the wled bindings to .yaml format

[kgunda]

On 2020-03-11 00:01, Rob Herring wrote:

On Mon,  9 Mar 2020 18:55:59 +0530, Kiran Gunda wrote:

Convert the qcom-wled bindings from .txt to .yaml format.

Signed-off-by: Kiran Gunda 
---
 .../bindings/leds/backlight/qcom-wled.txt  | 154 
-
 .../bindings/leds/backlight/qcom-wled.yaml | 184 
+

 2 files changed, 184 insertions(+), 154 deletions(-)
 delete mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml




My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml: $id:
relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/leds/backlight/qcom-wled.yaml#

See https://patchwork.ozlabs.org/patch/1251567
Please check and re-submit.

I will fix it in next post.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V3 3/4] backlight: qcom-wled: Add support for WLED5 peripheral in PM8150L

[kgunda]

On 2020-03-10 21:15, Daniel Thompson wrote:

On Mon, Mar 09, 2020 at 06:56:01PM +0530, Kiran Gunda wrote:

Add support for WLED5 peripheral that is present on PM8150L PMICs.

PM8150L WLED supports the following:
- Two modulators and each sink can use any of the modulator
- Multiple CABC selection options
- Multiple brightness width selection (12 bits to 15 bits)

Signed-off-by: Kiran Gunda 


Mostly just style comments below...



---
 .../bindings/leds/backlight/qcom-wled.yaml |  39 +++
 drivers/video/backlight/qcom-wled.c| 336 
-


Shouldn't the bindings and driver be separate?



Ok. I will split it out in to a separate patch in next post.

 2 files changed, 374 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml

index d334f81..e0dadc4 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml
@@ -20,6 +20,7 @@ properties:
- qcom,pm8941-wled
- qcom,pmi8998-wled
- qcom,pm660l-wled
+   - qcom,pm8150l-wled

   reg:
 maxItems: 1
@@ -28,10 +29,23 @@ properties:
 maxItems: 1
 description:
   brightness value on boot, value from 0-4095.
+  For pm8150l this value vary from 0-4095 or 0-32767
+  depending on the brightness control mode. If CABC is
+  enabled 0-4095 range is used.


Is this a pm8150l restriction or a WLED5 restriction (will other WLED5
have different ranges)?


It is a WLED5 restriction which is used in pm8150l PMIC.



 allOf:
   - $ref: /schemas/types.yaml#/definitions/uint32
 default: 2048

+  max-brightness:
+maxItems: 1
+description:
+  Maximum brightness level. Allowed values are,
+  for pmi8998 it is  0-4095.
+  For pm8150l, this can be either 4095 or 32767.


Ditto.


It is a WLED5 restriction which is used in pm8150l PMIC.



+  If CABC is enabled, this is capped to 4095.
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+
   label:
 maxItems: 1
 description:
@@ -124,6 +138,31 @@ properties:
   value for PM8941 from 1 to 3. Default 2
   For PMI8998 from 1 to 4.

+  qcom,modulator-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the modulator used for brightness modulation.
+  Allowed values are,
+   0 - Modulator A
+   1 - Modulator B
+  If not specified, then modulator A will be used by default.
+  This property is applicable only to WLED5 peripheral.
+
+  qcom,cabc-sel:
+maxItems: 1
+allOf:
+  - $ref: /schemas/types.yaml#/definitions/uint32
+description:
+  Selects the CABC pin signal used for brightness modulation.
+  Allowed values are,
+  0 - CABC disabled
+  1 - CABC 1
+  2 - CABC 2
+  3 - External signal (e.g. LPG) is used for dimming
+  This property is applicable only to WLED5 peripheral.
+
   interrupts:
 maxItems: 2
 description:
diff --git a/drivers/video/backlight/qcom-wled.c 
b/drivers/video/backlight/qcom-wled.c

index b73f273..edbbcb2 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -19,6 +19,8 @@
 #define WLED_DEFAULT_BRIGHTNESS2048
 #define WLED_SOFT_START_DLY_US 1
 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF
+#define WLED5_SINK_REG_BRIGHT_MAX_12B  0xFFF
+#define WLED5_SINK_REG_BRIGHT_MAX_15B  0x7FFF

 /* WLED3/WLED4 control registers */
 #define WLED3_CTRL_REG_FAULT_STATUS0x08
@@ -40,6 +42,7 @@

 #define WLED3_CTRL_REG_OVP 0x4d
 #define  WLED3_CTRL_REG_OVP_MASK   GENMASK(1, 0)
+#define  WLED5_CTRL_REG_OVP_MASK   GENMASK(3, 0)

 #define WLED3_CTRL_REG_ILIMIT  0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASKGENMASK(2, 0)
@@ -101,6 +104,40 @@

 #define WLED4_SINK_REG_BRIGHT(n)   (0x57 + (n * 0x10))

+/* WLED5 specific sink registers */
+#define WLED5_SINK_REG_MOD_A_EN0x50
+#define WLED5_SINK_REG_MOD_B_EN0x60
+#define  WLED5_SINK_REG_MOD_EN_MASKBIT(7)
+
+#define WLED5_SINK_REG_MOD_A_SRC_SEL   0x51
+#define WLED5_SINK_REG_MOD_B_SRC_SEL   0x61
+#define  WLED5_SINK_REG_MOD_SRC_SEL_HIGH   0
+#define  WLED5_SINK_REG_MOD_SRC_SEL_EXT0x03
+#define  WLED5_SINK_REG_MOD_SRC_SEL_MASK   GENMASK(1, 0)
+
+#define WLED5_SINK_REG_MOD_A_BRIGHTNESS_WIDTH_SEL  0x52
+#define WLED5_SINK_REG_MOD_B_BRIGHTNESS_WIDTH_SEL  0x62
+#define  

Re: [PATCH V3 1/4] backlight: qcom-wled: convert the wled bindings to .yaml format

[kgunda]

On 2020-03-10 20:31, Daniel Thompson wrote:

On Mon, Mar 09, 2020 at 06:55:59PM +0530, Kiran Gunda wrote:

Convert the qcom-wled bindings from .txt to .yaml format.

Signed-off-by: Kiran Gunda 


Acked-by: Daniel Thompson 


Thanks.



---
 .../bindings/leds/backlight/qcom-wled.txt  | 154 
-
 .../bindings/leds/backlight/qcom-wled.yaml | 184 
+

 2 files changed, 184 insertions(+), 154 deletions(-)
 delete mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
 create mode 100644 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml


diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

deleted file mode 100644
index c06863b..000
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
+++ /dev/null
@@ -1,154 +0,0 @@
-Binding for Qualcomm Technologies, Inc. WLED driver
-
-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:   
-   Definition:   should be one of:
-   "qcom,pm8941-wled"
-   "qcom,pmi8998-wled"
-   "qcom,pm660l-wled"
-
-- reg
-   Usage:required
-   Value type:   
-   Definition:   Base address of the WLED modules.
-
-- default-brightness
-   Usage:optional
-   Value type:   
-   Definition:   brightness value on boot, value from: 0-4095.
- Default: 2048
-
-- label
-   Usage:required
-   Value type:   
-   Definition:   The name of the backlight device
-
-- qcom,cs-out
-   Usage:optional
-   Value type:   
-   Definition:   enable current sink output.
- This property is supported only for PM8941.
-
-- qcom,cabc
-   Usage:optional
-   Value type:   
-   Definition:   enable content adaptive backlight control.
-
-- qcom,ext-gen
-   Usage:optional
-   Value type:   
-   Definition:   use externally generated modulator signal to dim.
- This property is supported only for PM8941.
-
-- qcom,current-limit
-   Usage:optional
-   Value type:   
-   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:   
-	Definition:   uA; per-string current limit; value from 0 to 3 
with

- 2500 uA step. Default 25 mA.
-
-- qcom,current-boost-limit
-   Usage:optional
-   Value type:   
-   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:   
-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:   
-   Definition:   V; Over-voltage protection limit; one of:
- 27, 29, 32, 35. Default: 29V
- This property is supported only for PM8941.
-
-- qcom,ovp-millivolt
-   Usage:optional
-   Value type:   
-   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:   
-   Definition:   #; number of led strings attached;
- value: For PM8941 from 1 to 3. Default: 2
-For PMI8998 from 1 to 4.
-
-- interrupts
-   Usage:optional
-   Value type:   
-   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:   
-   Definition:   Interrupt names 

Re: [PATCH V1 2/2] backlight: qcom-wled: Add support for WLED5 peripheral in PM8150L

[kgunda]

On 2020-03-09 03:17, Bjorn Andersson wrote:

On Mon 02 Mar 04:55 PST 2020, Kiran Gunda wrote:
diff --git a/drivers/video/backlight/qcom-wled.c 
b/drivers/video/backlight/qcom-wled.c

[..]

@@ -147,14 +187,39 @@ struct wled {
u32 max_brightness;
u32 short_count;
u32 auto_detect_count;
+   u32 version;
bool disabled_by_short;
bool has_short_detect;
+   bool cabc_disabled;
int short_irq;
int ovp_irq;

struct wled_config cfg;
struct delayed_work ovp_work;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
+   int (*cabc_config)(struct wled *wled, bool enable);
+   int (*wled_sync_toggle)(struct wled *wled);


Please split this patch in one that adds these and breaks out the wled3
support, and then a second patch that adds wled5.


Sure. Will make this change in the next post.

+};
+

[..]

+static int wled5_set_brightness(struct wled *wled, u16 brightness)
+{
+   int rc, offset;
+   u16 low_limit = wled->max_brightness * 1 / 1000;
+   u8 v[2], brightness_msb_mask;
+
+   /* WLED5's lower limit is 0.1% */
+   if (brightness > 0 && brightness < low_limit)
+   brightness = low_limit;
+
+   brightness_msb_mask = 0xf;
+   if (wled->max_brightness == WLED5_SINK_REG_BRIGHT_MAX_15B)
+   brightness_msb_mask = 0x7f;


Why not just brightness &= wled->max_brightness? But given that it 
seems

like the framework ensures that brightness <= max_brightness, why not
skip this altogether?

Okay. I will modify the code to remove the min/max, low_limit checks in 
next post.

+
+   v[0] = brightness & 0xff;
+   v[1] = (brightness >> 8) & brightness_msb_mask;
+
+   offset = wled5_brightness_reg[wled->cfg.mod_sel];
+   rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset,
+   v, 2);
+   return rc;
+}
+
 static int wled4_set_brightness(struct wled *wled, u16 brightness)
 {
int rc, i;
@@ -237,7 +325,28 @@ static int wled_module_enable(struct wled *wled, 
int val)

return 0;
 }

-static int wled_sync_toggle(struct wled *wled)
+static int wled5_sync_toggle(struct wled *wled)
+{
+   int rc;
+   u8 val;
+
+   val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT :
+WLED5_SINK_REG_SYNC_MOD_B_BIT;
+   rc = regmap_update_bits(wled->regmap,
+   wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
+   WLED5_SINK_REG_SYNC_MASK, val);
+   if (rc < 0)
+   return rc;
+
+   val = 0;


Just plug 0 in the function call.


Sure. Will do it in next post.

+   rc = regmap_update_bits(wled->regmap,
+   wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT,
+   WLED5_SINK_REG_SYNC_MASK, val);
+
+   return rc;


And return regmap_update_bits(...);


Sure. Will do it in next post.

+}
+


Regards,
Bjorn

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

Re: [PATCH -next v2] backlight: qcom-wled: fix unsigned comparison to zero

[kgunda]

On 2020-01-22 16:25, Daniel Thompson wrote:

On Wed, Jan 22, 2020 at 09:32:40AM +0800, Chen Zhou wrote:

Fixes coccicheck warning:
./drivers/video/backlight/qcom-wled.c:1104:5-15:
WARNING: Unsigned expression compared with zero: string_len > 0

The unsigned variable string_len is assigned a return value from the 
call
to of_property_count_elems_of_size(), which may return negative error 
code.


Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for 
WLED3")

Signed-off-by: Chen Zhou 
Reviewed-by: Bjorn Andersson 


Reviewed-by: Daniel Thompson 


Reviewed-by: Kiran Gunda 

---

changes in v2:
- fix commit message description.

---
 drivers/video/backlight/qcom-wled.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index d46052d..3d276b3 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -956,8 +956,8 @@ static int wled_configure(struct wled *wled, int 
version)

struct wled_config *cfg = >cfg;
struct device *dev = wled->dev;
const __be32 *prop_addr;
-   u32 size, val, c, string_len;
-   int rc, i, j;
+   u32 size, val, c;
+   int rc, i, j, string_len;

const struct wled_u32_opts *u32_opts = NULL;
const struct wled_u32_opts wled3_opts[] = {
--
2.7.4


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

Re: [PATCH V9 1/6] backlight: qcom-wled: Add new properties for PMI8998.

[kgunda]

On 2019-10-31 14:28, Lee Jones wrote:

On Wed, 23 Oct 2019, Kiran Gunda wrote:


Update the bindings with the new properties used for
PMI8998.

Signed-off-by: Kiran Gunda 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Rob Herring 
Acked-by: Daniel Thompson 
---
 .../bindings/leds/backlight/qcom-wled.txt  | 74 
++

 1 file changed, 63 insertions(+), 11 deletions(-)


This patch no longer applies.

It looks like you dropped the rename patch.

Please rebase all of the patches in this set on top of a released
commit and resend.

Sure. I will resend the complete series with the dropped patches.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V8 4/6] backlight: qcom-wled: Add support for WLED4 peripheral.

[kgunda]

On 2019-10-21 15:49, Daniel Thompson wrote:

On Fri, Oct 18, 2019 at 06:03:27PM +0530, 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 
Reviewed-by: Bjorn Andersson 


Reviewed-by: Daniel Thompson 


Thanks !

---
 drivers/video/backlight/qcom-wled.c | 255 
+++-

 1 file changed, 253 insertions(+), 2 deletions(-)

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

index 45eeda4..5386ca9 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -17,7 +17,7 @@

 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

-/* WLED3 control registers */
+/* WLED3/WLED4 control registers */
 #define WLED3_CTRL_REG_MOD_EN  0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASKBIT(7)
 #define  WLED3_CTRL_REG_MOD_EN_SHIFT   7
@@ -31,7 +31,7 @@
 #define WLED3_CTRL_REG_ILIMIT  0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASKGENMASK(2, 0)

-/* WLED3 sink registers */
+/* WLED3/WLED4 sink registers */
 #define WLED3_SINK_REG_SYNC0x47
 #define  WLED3_SINK_REG_SYNC_CLEAR 0x00

@@ -56,6 +56,28 @@
 #define WLED3_SINK_REG_STR_CABC(n) (0x66 + (n * 0x10))
 #define  WLED3_SINK_REG_STR_CABC_MASK  BIT(7)

+/* WLED4 specific sink registers */
+#define WLED4_SINK_REG_CURR_SINK   0x46
+#define  WLED4_SINK_REG_CURR_SINK_MASK GENMASK(7, 4)
+#define  WLED4_SINK_REG_CURR_SINK_SHFT 4
+
+/* WLED4 specific 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_INT0x00
+#define  WLED4_SINK_REG_STR_MOD_SRC_EXT0x01
+
+#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))
+
 struct wled_var_cfg {
const u32 *values;
u32 (*fn)(u32);
@@ -90,6 +112,7 @@ struct wled {
struct device *dev;
struct regmap *regmap;
u16 ctrl_addr;
+   u16 sink_addr;
u16 max_string_count;
u32 brightness;
u32 max_brightness;
@@ -116,6 +139,29 @@ static int wled3_set_brightness(struct wled 
*wled, u16 brightness)

return 0;
 }

+static int wled4_set_brightness(struct wled *wled, u16 brightness)
+{
+   int rc, i;
+   u16 low_limit = wled->max_brightness * 4 / 1000;
+   u8 v[2];
+
+   /* 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;
@@ -267,6 +313,120 @@ static int wled3_setup(struct wled *wled)
.enabled_strings = {0, 1, 2, 3},
 };

+static int wled4_setup(struct wled *wled)
+{
+   int rc, temp, i, j;
+   u16 addr;
+   u8 sink_en = 0;
+   u32 sink_cfg = 0;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_OVP,
+   WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
+   if (rc < 0)
+   return rc;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
+   WLED3_CTRL_REG_ILIMIT_MASK,
+   wled->cfg.boost_i_limit);
+   if (rc < 0)
+   return rc;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_FREQ,
+   WLED3_CTRL_REG_FREQ_MASK,
+   wled->cfg.switch_freq);
+   if (rc < 0)
+   return rc;
+
+   rc = regmap_read(wled->regmap, wled->sink_addr +
+WLED4_SINK_REG_CURR_SINK, _cfg);
+   if (rc < 0)
+   return rc;
+
+ 

Re: [PATCH V8 6/6] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2019-10-21 16:01, Daniel Thompson wrote:

On Fri, Oct 18, 2019 at 06:03:29PM +0530, 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 
---
 drivers/video/backlight/qcom-wled.c | 398 
+++-

 1 file changed, 392 insertions(+), 6 deletions(-)

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

index 658b1e0..b2e6754 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -193,7 +216,23 @@ static int wled_module_enable(struct wled *wled, 
int val)

WLED3_CTRL_REG_MOD_EN,
WLED3_CTRL_REG_MOD_EN_MASK,
val << WLED3_CTRL_REG_MOD_EN_SHIFT);
-   return rc;
+   if (rc < 0)
+   return rc;
+
+   if (wled->ovp_irq > 0) {
+   if (val) {
+   /*
+* Wait for at least 10ms before enabling OVP interrupt
+* after module enable so that soft start is completed.
+*/


Comments should not say what is does (we can read that). It should be
saying what is weird about the hardware the results in us enabling the
interrupt in an unusual way.

More like:

"The hardware generates a storm of spurious OVP interrupts during soft
start operations so defer enabling the IRQ for 10ms to ensure that
the soft start is complete."

Note that I am only guessing that is an spurious interrupt storm that
caused you to defer the interrupt enable... I don't want to have to
guess which is why I am asking for a good quality comment!


Daniel.

Yes. Your guess is correct. The hardware document explains the same.
The WLED boost voltage can hit OVP limit in normal operating conditions 
such as

during the soft start timing.
Hence, waiting for the soft start to complete before enabling the OVP 
interrupt

to avoid the spurious interrupts.

I will update the comment as you suggested in the next series.

Re: [PATCH V7 6/6] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2019-10-17 19:09, Daniel Thompson wrote:

On Thu, Oct 17, 2019 at 05:47:47PM +0530, kgu...@codeaurora.org wrote:

On 2019-10-17 16:59, Daniel Thompson wrote:
> On Wed, Oct 16, 2019 at 03:43:46PM +0530, 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 
>
> It's a complex bit of code but I'm OK with it in principle. Everything
> below is about small details and/or nitpicking.
>
>
> > +static void wled_ovp_work(struct work_struct *work)
> > +{
> > + struct wled *wled = container_of(work,
> > +  struct wled, ovp_work.work);
> > + enable_irq(wled->ovp_irq);
> > +}
> > +
>
> A bit of commenting about why we have to wait 10ms before enabling the
> OVP interrupt would be appreciated.
>
>
Sure. Will add the comment in the next series.
> > +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, 
_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, _sts);
> > + if (rc < 0) {
> > + dev_err(wled->dev, "Error in reading WLED_FAULT_STATUS 
rc=%d\n",
> > + rc);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + if (fault_sts &
> > + (WLED3_CTRL_REG_OVP_FAULT_BIT | 
WLED3_CTRL_REG_ILIM_FAULT_BIT))
> > + dev_dbg(wled->dev, "WLED OVP fault detected, int_sts=%x
> > fault_sts= %x\n",
> > + int_sts, fault_sts);
> > +
> > + if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT) {
> > + mutex_lock(>lock);
> > + disable_irq_nosync(wled->ovp_irq);
>
> We're currently running the threaded ISR for this irq. Do we really need
> to disable it?
>
We need to disable this IRQ, during the auto string detection logic. 
Because
in the auto string detection we configure the current sinks one by one 
and

check the
status register for the OVPs and set the right string configuration. 
We

enable it later after
the auto string detection is completed.


This is a threaded oneshot interrupt handler. Why isn't the framework
masking sufficient for you here?


Daniel.

Right .. I overlooked that it is a oneshot interrupt earlier.
I will address it in the next series.

Re: [PATCH V7 6/6] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2019-10-17 19:00, Lee Jones wrote:

On Thu, 17 Oct 2019, kgu...@codeaurora.org wrote:


On 2019-10-17 17:56, Lee Jones wrote:
> On Wed, 16 Oct 2019, 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 
> > ---
> >  drivers/video/backlight/qcom-wled.c | 410
> > +++-
> >  1 file changed, 404 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> > index b5b125c..ff7c409 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c


[...]


> > + if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
> > + dev_dbg(wled->dev, "WLED OVP fault detected with SINK 
%d\n",
> > + i + 1);
>
> I haven't reviewed the whole patch, but this caught my eye.
>
> I think this should be upgraded to dev_warn().
>
Thought of keeping these messages silent, Because the string 
configuration

will be corrected in this
and informing it at end of the auto string detection.


[...]


> > + } else {
> > + dev_warn(wled->dev, "New WLED string configuration found 
%x\n",
> > +  sink_valid);
>
> Why would the user care about this?  Is it not normal?
>
Actually, it comes here if the user provided string configuration in 
the

device tree is in-correct.
That's why just informing the user about the right string 
configuration,

after the auto string detection.


Then I think we need to be more forthcoming.  Tell the user the
configuration is incorrect and what you've done to rectify it.

"XXX is not a valid configuration - using YYY instead"


Sure. Will update it in the next series.

[...]


> > +static int wled_configure_ovp_irq(struct wled *wled,
> > +   struct platform_device *pdev)
> > +{
> > + int rc;
> > + u32 val;
> > +
> > + wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
> > + if (wled->ovp_irq < 0) {
> > + dev_dbg(>dev, "ovp irq is not used\n");
>
> I assume this is optional.  What happens if no IRQ is provided?
>
Here OVP IRQ is used to detect the wrong string detection. If it is 
not

provided the auto string detection logic won't work.


"OVP IRQ not found - disabling automatic string detection"


Sure. Will update it in the next series.

> If, for instance, polling mode is enabled, maybe something like this
> would be better?
>
>   dev_warn(>dev, "No IRQ found, falling back to polling
> mode\n");

Re: [PATCH V7 6/6] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2019-10-17 17:56, Lee Jones wrote:

On Wed, 16 Oct 2019, 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 
---
 drivers/video/backlight/qcom-wled.c | 410 
+++-

 1 file changed, 404 insertions(+), 6 deletions(-)

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

index b5b125c..ff7c409 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c



[...]


+   /* iterate through the strings one by one */


Please use proper English in comments (less a full stop).

In this case, just s/iterate/Iterate/.


Sorry for that ! will fix it in next series.

+   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) {
+			dev_err(wled->dev, "Failed to enable feedback for SINK %d rc = 
%d\n",

+   i + 1, rc);
+   goto failed_detect;
+   }
+
+   /* enable the sink */


Here too.  And everywhere else.


Will fix it in next series.

+   rc = regmap_write(wled->regmap, wled->sink_addr +
+ WLED4_SINK_REG_CURR_SINK, sink_test);
+   if (rc < 0) {
+   dev_err(wled->dev, "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) {
+   dev_err(wled->dev, "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, _sts);
+   if (rc < 0) {
+			dev_err(wled->dev, "Error in reading WLED3_CTRL_INT_RT_STS 
rc=%d\n",

+   rc);
+   goto failed_detect;
+   }
+
+   if (int_sts & WLED3_CTRL_REG_OVP_FAULT_STATUS)
+   dev_dbg(wled->dev, "WLED OVP fault detected with SINK 
%d\n",
+   i + 1);


I haven't reviewed the whole patch, but this caught my eye.

I think this should be upgraded to dev_warn().

Thought of keeping these messages silent, Because the string 
configuration will be corrected in this

and informing it at end of the auto string detection.

+   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) {
+   dev_err(wled->dev, "Failed to disable WLED module 
rc=%d\n",
+   rc);
+   goto failed_detect;
+   }
+   }
+
+   if (!sink_valid) {
+   dev_err(wled->dev, "No valid WLED sinks found\n");
+   wled->disabled_by_short = true;
+   goto failed_detect;
+   }
+
+   if (sink_valid == sink_config) {
+		dev_dbg(wled->dev, "WLED auto-detection complete, sink-config=%x 
OK!\n",

+   sink_config);


Does this really need to be placed in the kernel log?


Ok. This can be removed. I will remove it in next series.

+   } else {
+   dev_warn(wled->dev, "New WLED string configuration found %x\n",
+sink_valid);


Why would the user care about this?  Is it not normal?

Actually, it comes here if the user provided string configuration in the 
device tree is 

Re: [PATCH V7 5/6] backlight: qcom-wled: add support for short circuit handling.

[kgunda]

On 2019-10-17 16:39, Daniel Thompson wrote:

On Wed, Oct 16, 2019 at 03:43:45PM +0530, 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 
Reviewed-by: Bjorn Andersson 


Reviewed-by: Daniel Thompson 


Thanks for that !

---
 drivers/video/backlight/qcom-wled.c | 132 
++--

 1 file changed, 128 insertions(+), 4 deletions(-)

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

index 2807b4b..b5b125c 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -2,6 +2,9 @@
 /* Copyright (c) 2015, Sony Mobile Communications, AB.
  */

+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -56,6 +59,16 @@
 #define WLED3_SINK_REG_STR_CABC(n) (0x66 + (n * 0x10))
 #define  WLED3_SINK_REG_STR_CABC_MASK  BIT(7)

+/* WLED4 specific 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
+
 /* WLED4 specific sink registers */
 #define WLED4_SINK_REG_CURR_SINK   0x46
 #define  WLED4_SINK_REG_CURR_SINK_MASK GENMASK(7, 4)
@@ -105,17 +118,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 thread irq handler */
+   ktime_t last_short_event;
u16 ctrl_addr;
u16 sink_addr;
u16 max_string_count;
u32 brightness;
u32 max_brightness;
+   u32 short_count;
+   bool disabled_by_short;
+   bool has_short_detect;

struct wled_config cfg;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
@@ -166,6 +185,9 @@ static int wled_module_enable(struct wled *wled, 
int val)

 {
int rc;

+   if (wled->disabled_by_short)
+   return -ENXIO;
+
rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
WLED3_CTRL_REG_MOD_EN,
WLED3_CTRL_REG_MOD_EN_MASK,
@@ -202,18 +224,19 @@ static int wled_update_status(struct 
backlight_device *bl)

bl->props.state & BL_CORE_FBBLANK)
brightness = 0;

+   mutex_lock(>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_sync_toggle(wled);
if (rc < 0) {
dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
-   return rc;
+   goto unlock_mutex;
}
}

@@ -221,15 +244,61 @@ 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(>lock);
+
return rc;
 }

+#define WLED_SHORT_DLY_MS  20
+#define WLED_SHORT_CNT_MAX 5
+#define WLED_SHORT_RESET_CNT_DLY_USUSEC_PER_SEC
+
+static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
+{
+   struct wled *wled = _wled;
+   int rc;
+   s64 elapsed_time;
+
+   wled->short_count++;
+   mutex_lock(>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 = 1;
+
+   if (wled->short_count > WLED_SHORT_CNT_MAX) {
+		dev_err(wled->dev, "Short trigged %d times, disabling WLED 
forever!\n",

+   wled->short_count);
+   wled->disabled_by_short = true;
+   goto unlock_mutex;
+   }
+
+   wled->last_short_event = 

Re: [PATCH V7 4/6] backlight: qcom-wled: Add support for WLED4 peripheral.

[kgunda]

On 2019-10-17 16:36, Daniel Thompson wrote:

On Wed, Oct 16, 2019 at 03:43:44PM +0530, 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.


There is code buried in this patch that looks like it changes the name
that will be handed to the backlight sub-system.

It's purpose needs to be explained in the patch description (or the 
code

moved to a new patch).


I will address it in next series.


Daniel.



Signed-off-by: Kiran Gunda 
Reviewed-by: Bjorn Andersson 
---
 drivers/video/backlight/qcom-wled.c | 263 
+++-

 1 file changed, 257 insertions(+), 6 deletions(-)

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

index 45eeda4..2807b4b 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -17,7 +17,7 @@

 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

-/* WLED3 control registers */
+/* WLED3/WLED4 control registers */
 #define WLED3_CTRL_REG_MOD_EN  0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASKBIT(7)
 #define  WLED3_CTRL_REG_MOD_EN_SHIFT   7
@@ -31,7 +31,7 @@
 #define WLED3_CTRL_REG_ILIMIT  0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASKGENMASK(2, 0)

-/* WLED3 sink registers */
+/* WLED3/WLED4 sink registers */
 #define WLED3_SINK_REG_SYNC0x47
 #define  WLED3_SINK_REG_SYNC_CLEAR 0x00

@@ -56,6 +56,28 @@
 #define WLED3_SINK_REG_STR_CABC(n) (0x66 + (n * 0x10))
 #define  WLED3_SINK_REG_STR_CABC_MASK  BIT(7)

+/* WLED4 specific sink registers */
+#define WLED4_SINK_REG_CURR_SINK   0x46
+#define  WLED4_SINK_REG_CURR_SINK_MASK GENMASK(7, 4)
+#define  WLED4_SINK_REG_CURR_SINK_SHFT 4
+
+/* WLED4 specific 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_INT0x00
+#define  WLED4_SINK_REG_STR_MOD_SRC_EXT0x01
+
+#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))
+
 struct wled_var_cfg {
const u32 *values;
u32 (*fn)(u32);
@@ -90,6 +112,7 @@ struct wled {
struct device *dev;
struct regmap *regmap;
u16 ctrl_addr;
+   u16 sink_addr;
u16 max_string_count;
u32 brightness;
u32 max_brightness;
@@ -116,6 +139,29 @@ static int wled3_set_brightness(struct wled 
*wled, u16 brightness)

return 0;
 }

+static int wled4_set_brightness(struct wled *wled, u16 brightness)
+{
+   int rc, i;
+   u16 low_limit = wled->max_brightness * 4 / 1000;
+   u8 v[2];
+
+   /* 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;
@@ -267,6 +313,120 @@ static int wled3_setup(struct wled *wled)
.enabled_strings = {0, 1, 2, 3},
 };

+static int wled4_setup(struct wled *wled)
+{
+   int rc, temp, i, j;
+   u16 addr;
+   u8 sink_en = 0;
+   u32 sink_cfg = 0;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_OVP,
+   WLED3_CTRL_REG_OVP_MASK, wled->cfg.ovp);
+   if (rc < 0)
+   return rc;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_ILIMIT,
+   WLED3_CTRL_REG_ILIMIT_MASK,
+   wled->cfg.boost_i_limit);
+   if (rc < 0)
+   return rc;
+
+   rc = regmap_update_bits(wled->regmap,
+   wled->ctrl_addr + WLED3_CTRL_REG_FREQ,
+   WLED3_CTRL_REG_FREQ_MASK,
+   

Re: [PATCH V7 6/6] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2019-10-17 16:59, Daniel Thompson wrote:

On Wed, Oct 16, 2019 at 03:43:46PM +0530, 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 


It's a complex bit of code but I'm OK with it in principle. Everything
below is about small details and/or nitpicking.



+static void wled_ovp_work(struct work_struct *work)
+{
+   struct wled *wled = container_of(work,
+struct wled, ovp_work.work);
+   enable_irq(wled->ovp_irq);
+}
+


A bit of commenting about why we have to wait 10ms before enabling the
OVP interrupt would be appreciated.



Sure. Will add the comment in the next series.

+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, _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, _sts);
+   if (rc < 0) {
+   dev_err(wled->dev, "Error in reading WLED_FAULT_STATUS rc=%d\n",
+   rc);
+   return IRQ_HANDLED;
+   }
+
+   if (fault_sts &
+   (WLED3_CTRL_REG_OVP_FAULT_BIT | WLED3_CTRL_REG_ILIM_FAULT_BIT))
+		dev_dbg(wled->dev, "WLED OVP fault detected, int_sts=%x fault_sts= 
%x\n",

+   int_sts, fault_sts);
+
+   if (fault_sts & WLED3_CTRL_REG_OVP_FAULT_BIT) {
+   mutex_lock(>lock);
+   disable_irq_nosync(wled->ovp_irq);


We're currently running the threaded ISR for this irq. Do we really 
need

to disable it?

We need to disable this IRQ, during the auto string detection logic. 
Because
in the auto string detection we configure the current sinks one by one 
and check the
status register for the OVPs and set the right string configuration. We 
enable it later after

the auto string detection is completed.

+
+   if (wled_auto_detection_required(wled))
+   wled_auto_string_detection(wled);
+
+   enable_irq(wled->ovp_irq);
+
+   mutex_unlock(>lock);
+   }
+
+   return IRQ_HANDLED;
+}
+


Snip.



+static int wled_remove(struct platform_device *pdev)
+{
+   struct wled *wled = dev_get_drvdata(>dev);
+
+   cancel_delayed_work_sync(>ovp_work);
+   mutex_destroy(>lock);


Have the irq handlers been disabled at this point?


Ok.. may not be. I will disable the irq's here in next series.


Also, if you want to destroy the mutex shouldn't that code be
introduced in the same patch that introduces the mutex?
Ok.. I will move it to the same patch where the mutex introduced in next 
series.

+
+   return 0;
+}



Daniel.

Re: [PATCH V6 3/8] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

On 2019-10-01 20:42, Dan Murphy wrote:

Kiran

On 9/30/19 1:39 AM, Kiran Gunda wrote:

Update the bindings with the new properties used for
PMI8998.

Signed-off-by: Kiran Gunda 
Reviewed-by: Bjorn Andersson 
Reviewed-by: Rob Herring 
Acked-by: Daniel Thompson 
---
  .../bindings/leds/backlight/qcom-wled.txt  | 76 
++

  1 file changed, 62 insertions(+), 14 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

index 14f28f2..9d840d5 100644
--- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
@@ -20,8 +20,7 @@ platforms. The PMIC is connected to the host 
processor via SPMI bus.

  - default-brightness
Usage:optional
Value type:   
-   Definition:   brightness value on boot, value from: 0-4095
- Default: 2048
+   Definition:   brightness value on boot, value from: 0-4095.
- label
Usage:required
@@ -48,20 +47,24 @@ platforms. The PMIC is connected to the host 
processor via SPMI bus.

  - qcom,current-limit
Usage:optional
Value type:   
-   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.
+ This property is supported only for pm8941.
+
+- qcom,current-limit-microamp
+   Usage:optional
+   Value type:   
+	Definition:   uA; per-string current limit; value from 0 to 3 
with

+ 2500 uA step.
- qcom,current-boost-limit
Usage:optional
Value type:   
Definition:   mA; boost current limit.
  For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
- 1680. Default: 805 mA
+ 1680.
  For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
- 1500. Default: 970 mA
+ 1500.
- qcom,switching-freq
Usage:optional
@@ -69,22 +72,66 @@ platforms. The PMIC is connected to the host 
processor via SPMI bus.
  	 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:   
Definition:   V; Over-voltage protection limit; one of:
- 27, 29, 32, 35. default: 29V
+ 27, 29, 32, 35.
  This property is supported only for PM8941.
  +- qcom,ovp-millivolt
+   Usage:optional
+   Value type:   
+   Definition:   mV; Over-voltage protection limit;
+ For pmi8998: one of 18100, 19600, 29600, 31100
+ If this property is not specified for PM8941, it
+ falls back to "qcom,ovp" property.
+
  - qcom,num-strings
Usage:optional
Value type:   
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.
+For PMI8998 from 1 to 4.


We probably don't need this since we define 1 led node per output. 
And if you need to define

multiple strings per node then you use led-sources.

Then you will use fwnode_property_count_u32(child, "led-sources"); to
get the number of outputs


I have kept this property as is to have the backward compatibility with 
pm8941-wled.

The backward compatibility is broken if this property is removed.


+
+- interrupts
+   Usage:optional
+   Value type:   
+   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:   
+   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:   
+   Definition:   Array of the WLED strings numbered from 0 to 3. Each
+   

Re: [PATCH V6 5/8] backlight: qcom-wled: Restructure the driver for WLED3

[kgunda]

On 2019-10-01 20:47, Dan Murphy wrote:

Kiran

On 9/30/19 1:39 AM, Kiran Gunda wrote:

Restructure the driver to add the support for new WLED
peripherals.

Signed-off-by: Kiran Gunda 
Acked-by: Daniel Thompson 
---
  drivers/video/backlight/qcom-wled.c | 395 
++--

  1 file changed, 245 insertions(+), 150 deletions(-)

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

index f191242..740f1b6 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -7,59 +7,71 @@
  #include 
  #include 
  #include 
+#include 
  #include 
/* From DT binding */
+#define WLED_MAX_STRINGS   4
+
  #define WLED_DEFAULT_BRIGHTNESS   2048
  -#define WLED3_SINK_REG_BRIGHT_MAX0xFFF
-#define WLED3_CTRL_REG_VAL_BASE0x40
+#define WLED_SINK_REG_BRIGHT_MAX   0xFFF


Why did you change some of these again?

Can you just change it to the final #define in patch 4/8?

Dan


Ok.. Looks like some thing went wrong with this series. I will re-upload 
it again.

Re: [PATCH V4 3/8] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

On 2018-09-04 00:25, Bjorn Andersson wrote:

On Wed 15 Aug 22:23 PDT 2018, kgu...@codeaurora.org wrote:


On 2018-08-07 10:53, Bjorn Andersson wrote:
> On Mon 09 Jul 03:22 PDT 2018, Kiran Gunda wrote:
> > diff --git
> > a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> > b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
> [..]
> >  - qcom,num-strings
> >   Usage:optional
> >   Value type:   
> >   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.
> > +  For PMI8998 from 1 to 4.
> [..]
> > +- qcom,enabled-strings
> > + Usage:optional
> > + Value tyoe:   
> > + 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.
> [..]
> >
> >  Example:
> >
> > @@ -99,4 +146,5 @@ pm8941-wled@d800 {
> >   qcom,switching-freq = <1600>;
> >   qcom,ovp = <29>;
> >   qcom,num-strings = <2>;
> > + qcom,enabled-strings = <0x00 0x01>;
>
> Nit. I would assume that specifying qcom,num-strings = <2> implies that
> the first 2 strings are used, so one would not also specify
> qcom,enabled-strings.
>
Thanks Bjorn for reviewing the series !

"qcom,enabled-strings" need be specified along with the 
"qcom,num-strings".
Because the enabled-strings can be <0, 2> or <0, 3 > also. The driver 
picks

the string
configuration from the enabled-strings array and enable only those
current-sinks.



The original binding described qcom,num-strings to mean "the first N
strings", requiring qcom,enabled-strings now would break backwards
compatibility with this binding.

In the case that qcom,enabled-strings is specified we can easily derive
num-strings from the listed entires. So I would suggest that you look
for enabled-strings and if not found fall back to checking for
num-strings.

Regards,
Bjorn


Sorry for the late reply. Actually the "qcom,enabled-strings" is 
initialized with
the strings 0, 1, 2, 3 in the driver. Even though this property is 
missing the first
N strings will be still configured with out any issue, based on the 
"qcom,num-strings".


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

Re: [PATCH V5 5/8] backlight: qcom-wled: Restructure the driver for WLED3

[kgunda]

On 2018-08-27 15:39, Pavel Machek wrote:

On Fri 2018-08-24 15:57:44, Kiran Gunda wrote:

Restructure the driver to add the support for new WLED
peripherals.

Signed-off-by: Kiran Gunda 
Acked-by: Daniel Thompson 
---
Changes from V3:
- This is the new patch after splitting the
  "backlight: qcom-wled: Add support for WLED4 peripheral" patch
  to seperate the WLED3 specific restructure.

Changes from V4:
- Initialize wled->cfg.enabled_strings to 0,1,2,3.
- Replaced the WLED3 macro with 3.

 drivers/video/backlight/qcom-wled.c | 395 
++--

 1 file changed, 245 insertions(+), 150 deletions(-)

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

index 3cd6e75..a746bec 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -15,59 +15,71 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 /* From DT binding */
+#define WLED_MAX_STRINGS   4
+
 #define WLED_DEFAULT_BRIGHTNESS2048

-#define WLED3_SINK_REG_BRIGHT_MAX  0xFFF
-#define WLED3_CTRL_REG_VAL_BASE0x40
+#define WLED_SINK_REG_BRIGHT_MAX   0xFFF


Stop this, no. In previous patch you renamed from ABC123_ to WLED3_,
now you are renaming back to WLED?

Stop messing with the names. I'd actually prefer you to stick with
original driver name, and just add note that this now supports more
hardware.

But yes, _one_ rename is okay. I guess. But renaming it twice in one
series is not acceptable.

ok. I will stop renaming from WLED3 to WLED in this patch. I did it 
because these registers
are common for both WLED3 and WLED4 and Bjorn also suggested the same. 
Anyways I will stop

this renaming in this patch from WLED3 to WLED.

@@ -365,6 +433,15 @@ static int wled_configure(struct wled *wled, 
struct device *dev)


cfg->num_strings = cfg->num_strings + 1;

+   string_len = of_property_count_elems_of_size(dev->of_node,
+"qcom,enabled-strings",
+sizeof(u32));
+   if (string_len > 0)
+   rc = of_property_read_u32_array(dev->of_node,
+   "qcom,enabled-strings",
+   wled->cfg.enabled_strings,
+   sizeof(u32));
+
return 0;
 }


rc is assigned but never used.

Will address it next series.

Pavel

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

Re: [PATCH V5 8/8] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2018-08-27 20:24, Bjorn Andersson wrote:

On Fri 24 Aug 03:27 PDT 2018, Kiran Gunda wrote:
diff --git a/drivers/video/backlight/qcom-wled.c 
b/drivers/video/backlight/qcom-wled.c

[..]

 struct wled {
@@ -135,16 +146,22 @@ struct wled {
struct regmap *regmap;
struct mutex lock;  /* Lock to avoid race from thread irq handler */
ktime_t last_short_event;
+   ktime_t start_ovp_fault_time;
u16 ctrl_addr;
u16 sink_addr;
u16 max_string_count;
+   u16 auto_detection_ovp_count;
u32 brightness;
u32 max_brightness;
u32 short_count;
+   u32 auto_detect_count;
bool disabled_by_short;
bool has_short_detect;
+   int ovp_irq;
+   bool ovp_irq_disabled;


ovp_irq_disabled is now only assigned, never read. So you should be 
able

to drop it.


Ok. will address in next series.

[..]
@@ -200,7 +226,18 @@ static int wled_module_enable(struct wled *wled, 
int val)

WLED_CTRL_REG_MOD_EN,
WLED_CTRL_REG_MOD_EN_MASK,
val << WLED_CTRL_REG_MOD_EN_SHIFT);
-   return rc;
+   if (rc < 0)
+   return rc;
+
+   if (val) {
+   schedule_delayed_work(>ovp_work, HZ / 100);


wled_ovp_work() is a nop when ovp_irq == 0, so wrap the entire if/else
in a check for ovp_irq > 0 (rather than include it in the else) and 
drop

it from the worker.


Will address in next series.

+   } else {
+   if (!cancel_delayed_work_sync(>ovp_work) &&
+   wled->ovp_irq > 0)
+   disable_irq(wled->ovp_irq);
+   }
+
+   return 0;


Also, if a user calls rmmod within 10ms of enabling module we will
schedule work of code that's no longer available in the kernel. So you
need to also cancel_delayed_work_sync() in the remove function of the
driver.



Will address in next series.

Apart from this I think this patch looks good now!


Thank you

Regards,
Bjorn

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

Re: [PATCH V5 3/8] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

On 2018-08-27 15:31, Pavel Machek wrote:

Hi!

On Fri 2018-08-24 15:57:42, Kiran Gunda wrote:

Update the bindings with the new properties used for
PMI8998.



Changes from V3:
- Removed the default values.


Why?


As per Bjorn's comment I have removed the default values.
Do you want to add them back ?


+- qcom,current-limit-microamp
+   Usage:optional
+   Value type:   
+	Definition:   uA; per-string current limit; value from 0 to 3 
with

+ 2500 uA step.

 - qcom,current-boost-limit
Usage:optional
Value type:   
Definition:   mA; boost current limit.
  For pm8941: one of: 105, 385, 525, 805, 980, 1260, 1400,
- 1680. Default: 805 mA
+ 1680.
  For pmi8998: one of: 105, 280, 450, 620, 970, 1150, 1300,
- 1500. Default: 970 mA
+ 1500.



I'd say that optional properties should list default values...?


Same as above.


 - qcom,ovp
Usage:optional
Value type:   
Definition:   V; Over-voltage protection limit; one of:
- 27, 29, 32, 35. default: 29V
+ 27, 29, 32, 35.
  This property is supported only for PM8941.



Same here.


Same as above.

+- qcom,ovp-millivolt
+   Usage:optional
+   Value type:   
+   Definition:   mV; Over-voltage protection limit;
+ For pmi8998: one of 18100, 19600, 29600, 31100
+ If this property is not specified for PM8941, it
+ falls back to "qcom,ovp" property.
+


"voltage-limit-millivolt"? "ovp" is not really well known acronym.
Pavel
"ovp" is already being used in pm8941-wled.c driver. That's why I am 
using ovp-millivolt.
If I rename this, i think I have to rename the existing "qcom,ovp" also, 
which will break

the backward compatibility.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V4 8/8] backlight: qcom-wled: Add auto string detection logic

[kgunda]

On 2018-08-07 12:02, Bjorn Andersson wrote:

On Mon 09 Jul 03:22 PDT 2018, Kiran Gunda wrote:
diff --git a/drivers/video/backlight/qcom-wled.c 
b/drivers/video/backlight/qcom-wled.c

[..]
@@ -189,6 +206,15 @@ static int wled4_set_brightness(struct wled 
*wled, u16 brightness)

return 0;
 }

+static void wled_ovp_work(struct work_struct *work)
+{
+   struct wled *wled = container_of(work,
+struct wled, ovp_work.work);
+
+   if (wled->ovp_irq > 0)
+   enable_irq(wled->ovp_irq);
+}
+
 static int wled_module_enable(struct wled *wled, int val)
 {
int rc;
@@ -200,7 +226,18 @@ static int wled_module_enable(struct wled *wled, 
int val)

WLED_CTRL_REG_MOD_EN,
WLED_CTRL_REG_MOD_EN_MASK,
val << WLED_CTRL_REG_MOD_EN_SHIFT);
-   return rc;
+   if (rc < 0)
+   return rc;
+
+   if (val) {
+   schedule_delayed_work(>ovp_work, WLED_SOFT_START_DLY_US);


schedule_delayed_work() takes a delay in jiffies, not micro seconds.


I will address it in the next series.

+   } else {
+   cancel_delayed_work_sync(>ovp_work);


If we get here within WLED_SOFT_START_DLY_US of a module enable we have
yet to execute the enable_irq() in the delayed worker, which should 
mean

that the irq is unbalanced from then on.

I believe you can check the return value of cancel_delayed_work_sync()
being true to know if wled_ovp_work() was yet to execute.


Ok. I will address it in next series.

+   if (wled->ovp_irq > 0)
+   disable_irq(wled->ovp_irq);
+   }
+
+   return 0;
 }



Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH V4 5/8] backlight: qcom-wled: Restructure the driver for WLED3

[kgunda]

On 2018-08-07 11:07, Bjorn Andersson wrote:

On Mon 09 Jul 03:22 PDT 2018, Kiran Gunda wrote:
diff --git a/drivers/video/backlight/qcom-wled.c 
b/drivers/video/backlight/qcom-wled.c

[..]
@@ -365,6 +434,15 @@ static int wled_configure(struct wled *wled, 
struct device *dev)


cfg->num_strings = cfg->num_strings + 1;

+   string_len = of_property_count_elems_of_size(dev->of_node,
+"qcom,enabled-strings",
+sizeof(u32));
+   if (string_len > 0)
+   rc = of_property_read_u32_array(dev->of_node,
+   "qcom,enabled-strings",
+   wled->cfg.enabled_strings,


qcom,enabled-strings is listed as optional, but without it we will end
up with qcom,num-strings zeros in an array.  Initialize
wled->cfg.enabled_strings to 0,1,2,3 and the driver will be backwards
compatible.


Correct. I will initialize the array in the next series.
I also think that if you do qcom,enabled-strings = <0, 1, 2>; there's 
no

need to also specify qcom,num-strings = <3>; and we can just use
string_len for num_strings.
Correct. But I have kept this property to have the backward 
compatibility with the

old driver (WLED3).



+   sizeof(u32));
+
return 0;
 }


Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[kgunda]

On 2018-08-07 11:11, Bjorn Andersson wrote:

On Mon 09 Jul 03:22 PDT 2018, Kiran Gunda wrote:


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

Signed-off-by: Kiran Gunda 


I agree with Daniel, regarding the mentioning of the variable rename.


Yes. As Daniel asked I will update the commit text.


Apart from that:

Reviewed-by: Bjorn Andersson 

Regards,
Bjorn


---
Changes from V3:
- Changed the MODULE_DESCRIPTION

 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..3cd6e75 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -18,77 +18,79 @@
 #include 

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

-#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_BASE0x40

-#define PM8941_WLED_REG_MOD_EN 0x46
-#define  PM8941_WLED_REG_MOD_EN_BITBIT(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_MASKBIT(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_CLEAR0x00
+#define WLED3_CTRL_REG_FREQ0x4c
+#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_OVP0x4d
-#define  PM8941_WLED_REG_OVP_MASK  0x03
+#define WLED3_CTRL_REG_ILIMIT  0x4e
+#define  WLED3_CTRL_REG_ILIMIT_MASK0x07

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

-#define PM8941_WLED_REG_SINK   0x4f
-#define  PM8941_WLED_REG_SINK_MASK 0xe0
-#define  PM8941_WLED_REG_SINK_SHFT 0x05
+#define WLED3_SINK_REG_CURR_SINK   0x4f
+#define  WLED3_SINK_REG_CURR_SINK_MASK 0xe0
+#define  WLED3_SINK_REG_CURR_SINK_SHFT 0x05

-/* Per-'string' registers below */
-#define PM8941_WLED_REG_STR_OFFSET 0x10
+/* WLED3 per-'string' registers below */
+#define WLED3_SINK_REG_STR_OFFSET  0x10

-#define PM8941_WLED_REG_STR_MOD_EN_BASE0x60
-#define  PM8941_WLED_REG_STR_MOD_MASK  BIT(7)
-#define  PM8941_WLED_REG_STR_MOD_ENBIT(7)
+#define WLED3_SINK_REG_STR_MOD_EN_BASE 0x60
+#define  WLED3_SINK_REG_STR_MOD_MASK   BIT(7)
+#define  WLED3_SINK_REG_STR_MOD_EN BIT(7)

-#define PM8941_WLED_REG_STR_SCALE_BASE 0x62
-#define  PM8941_WLED_REG_STR_SCALE_MASK0x1f
+#define WLED3_SINK_REG_STR_FULL_SCALE_CURR 0x62
+#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK   0x1f

-#define PM8941_WLED_REG_STR_MOD_SRC_BASE   0x63
-#define  PM8941_WLED_REG_STR_MOD_SRC_MASK  0x01
-#define  PM8941_WLED_REG_STR_MOD_SRC_INT   0x00
-#define  PM8941_WLED_REG_STR_MOD_SRC_EXT   0x01
+#define WLED3_SINK_REG_STR_MOD_SRC_BASE0x63
+#define  WLED3_SINK_REG_STR_MOD_SRC_MASK   0x01
+#define  WLED3_SINK_REG_STR_MOD_SRC_INT0x00
+#define  WLED3_SINK_REG_STR_MOD_SRC_EXT0x01

-#define PM8941_WLED_REG_STR_CABC_BASE  0x66
-#define  PM8941_WLED_REG_STR_CABC_MASK BIT(7)
-#define  PM8941_WLED_REG_STR_CABC_EN   BIT(7)
+#define WLED3_SINK_REG_STR_CABC_BASE   0x66
+#define  

Re: [PATCH V4 3/8] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

On 2018-08-07 10:53, Bjorn Andersson wrote:

On Mon 09 Jul 03:22 PDT 2018, Kiran Gunda wrote:
diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

[..]

 - qcom,num-strings
Usage:optional
Value type:   
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.
+For PMI8998 from 1 to 4.

[..]

+- qcom,enabled-strings
+   Usage:optional
+   Value tyoe:   
+   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.

[..]


 Example:

@@ -99,4 +146,5 @@ pm8941-wled@d800 {
qcom,switching-freq = <1600>;
qcom,ovp = <29>;
qcom,num-strings = <2>;
+   qcom,enabled-strings = <0x00 0x01>;


Nit. I would assume that specifying qcom,num-strings = <2> implies that
the first 2 strings are used, so one would not also specify
qcom,enabled-strings.


Thanks Bjorn for reviewing the series !

"qcom,enabled-strings" need be specified along with the 
"qcom,num-strings".
Because the enabled-strings can be <0, 2> or <0, 3 > also. The driver 
picks the string
configuration from the enabled-strings array and enable only those 
current-sinks.



(And you should use decimal for natural numbers)


I will modify it in next series.


Reviewed-by: Bjorn Andersson 

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH V4 0/8] backlight: qcom-wled: Support for QCOM wled driver

[kgunda]

On 2018-08-03 13:15, Daniel Thompson wrote:

On Fri, Aug 03, 2018 at 12:49:34PM +0530, kgu...@codeaurora.org wrote:

Hi Bjorn,
Can you please help review this patch series ?

Pavel, Rob, Daniel reviewed the series except the "auto string 
detection"

patch.


I did take a glance at the last patch... but in the end decided to wait
for v5 to look in depth.


Daniel.





Thanks Daniel! I will push the V5 series then.


Thanks,
Kiran

On 2018-07-09 15:52, Kiran Gunda wrote:
> This patch series renames the pm8941-wled.c driver to qcom-wled.c to add
> the support for multiple PMICs supported by qualcomm. This patch series
> supports both PM8941 and PMI8998 WLED. The PMI8998 WLED has the support
> to handle the OVP (over voltage protection) and the SC (short circuit
> protection)
> interrupts. It also has the auto string detection algorithm support to
> configure the right strings if the user specified string configuration
> is in-correct. These three features are added in this series for
> PMI8998.
>
> changes from v1:
>- Fixed the commit message for
>- backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c
>
> Changes from v2:
>- Fixed bjorn and other reviewer's comments
>- Seperated the device tree bindings
>- Splitted out the WLED4 changes in seperate patch
>- Merged OVP and auto string detection patch
>
> Changes from v3:
>   - Added Reviewed-by/Acked-by tags
>   - Fixed comments from Bjorn/Vinod/Rob
>   - Splitting the "backlight: qcom-wled: Add support for WLED4
> peripheral" patch
> to seperate the WLED3 specific restructure.
>
> Kiran Gunda (8):
>   backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c
>   backlight: qcom-wled: restructure the qcom-wled bindings
>   backlight: qcom-wled: Add new properties for PMI8998
>   backlight: qcom-wled: Rename PM8941* to WLED3
>   backlight: qcom-wled: Restructure the driver for WLED3
>   backlight: qcom-wled: Add support for WLED4 peripheral
>   backlight: qcom-wled: add support for short circuit handling
>   backlight: qcom-wled: Add auto string detection logic
>
>  .../bindings/leds/backlight/pm8941-wled.txt|   42 -
>  .../bindings/leds/backlight/qcom-wled.txt  |  150 +++
>  drivers/video/backlight/Kconfig|8 +-
>  drivers/video/backlight/Makefile   |2 +-
>  drivers/video/backlight/pm8941-wled.c  |  432 ---
>  drivers/video/backlight/qcom-wled.c| 1298
> 
>  6 files changed, 1453 insertions(+), 479 deletions(-)
>  delete mode 100644
> Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
>  create mode 100644
> Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
>  delete mode 100644 drivers/video/backlight/pm8941-wled.c
>  create mode 100644 drivers/video/backlight/qcom-wled.c

--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH V4 0/8] backlight: qcom-wled: Support for QCOM wled driver

[kgunda]

Hi Bjorn,
Can you please help review this patch series ?

Pavel, Rob, Daniel reviewed the series except the "auto string 
detection" patch.


Thanks,
Kiran

On 2018-07-09 15:52, Kiran Gunda wrote:
This patch series renames the pm8941-wled.c driver to qcom-wled.c to 
add

the support for multiple PMICs supported by qualcomm. This patch series
supports both PM8941 and PMI8998 WLED. The PMI8998 WLED has the support
to handle the OVP (over voltage protection) and the SC (short circuit
protection)
interrupts. It also has the auto string detection algorithm support to
configure the right strings if the user specified string configuration
is in-correct. These three features are added in this series for 
PMI8998.


changes from v1:
   - Fixed the commit message for
   - backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

Changes from v2:
   - Fixed bjorn and other reviewer's comments
   - Seperated the device tree bindings
   - Splitted out the WLED4 changes in seperate patch
   - Merged OVP and auto string detection patch

Changes from v3:
  - Added Reviewed-by/Acked-by tags
  - Fixed comments from Bjorn/Vinod/Rob
  - Splitting the "backlight: qcom-wled: Add support for WLED4 
peripheral" patch

to seperate the WLED3 specific restructure.

Kiran Gunda (8):
  backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c
  backlight: qcom-wled: restructure the qcom-wled bindings
  backlight: qcom-wled: Add new properties for PMI8998
  backlight: qcom-wled: Rename PM8941* to WLED3
  backlight: qcom-wled: Restructure the driver for WLED3
  backlight: qcom-wled: Add support for WLED4 peripheral
  backlight: qcom-wled: add support for short circuit handling
  backlight: qcom-wled: Add auto string detection logic

 .../bindings/leds/backlight/pm8941-wled.txt|   42 -
 .../bindings/leds/backlight/qcom-wled.txt  |  150 +++
 drivers/video/backlight/Kconfig|8 +-
 drivers/video/backlight/Makefile   |2 +-
 drivers/video/backlight/pm8941-wled.c  |  432 ---
 drivers/video/backlight/qcom-wled.c| 1298 


 6 files changed, 1453 insertions(+), 479 deletions(-)
 delete mode 100644
Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
 create mode 100644
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
 delete mode 100644 drivers/video/backlight/pm8941-wled.c
 create mode 100644 drivers/video/backlight/qcom-wled.c

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

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

[kgunda]

On 2018-07-20 18:53, Daniel Thompson wrote:

On 09/07/18 11:22, Kiran Gunda wrote:

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

Signed-off-by: Kiran Gunda 
---
Changes from V3:
- Changed the MODULE_DESCRIPTION

  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..3cd6e75 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -18,77 +18,79 @@
  #include 
/* From DT binding */
-#define PM8941_WLED_DEFAULT_BRIGHTNESS 2048
+#define WLED_DEFAULT_BRIGHTNESS2048
  -#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_BASE0x40
  -#define PM8941_WLED_REG_MOD_EN   0x46
-#define  PM8941_WLED_REG_MOD_EN_BITBIT(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_MASKBIT(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_CLEAR0x00
+#define WLED3_CTRL_REG_FREQ0x4c
+#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_MASK0x07
  -#define PM8941_WLED_REG_BOOST0x4e
-#define  PM8941_WLED_REG_BOOST_MASK0x07
+/* WLED3 sink registers */
+#define WLED3_SINK_REG_SYNC0x47
+#define  WLED3_SINK_REG_SYNC_MASK  0x07
+#define  WLED3_SINK_REG_SYNC_LED1  BIT(0)
+#define  WLED3_SINK_REG_SYNC_LED2  BIT(1)
+#define  WLED3_SINK_REG_SYNC_LED3  BIT(2)
+#define  WLED3_SINK_REG_SYNC_ALL   0x07
+#define  WLED3_SINK_REG_SYNC_CLEAR 0x00
  -#define PM8941_WLED_REG_SINK 0x4f
-#define  PM8941_WLED_REG_SINK_MASK 0xe0
-#define  PM8941_WLED_REG_SINK_SHFT 0x05
+#define WLED3_SINK_REG_CURR_SINK   0x4f
+#define  WLED3_SINK_REG_CURR_SINK_MASK 0xe0
+#define  WLED3_SINK_REG_CURR_SINK_SHFT 0x05
  -/* Per-'string' registers below */
-#define PM8941_WLED_REG_STR_OFFSET 0x10
+/* WLED3 per-'string' registers below */
+#define WLED3_SINK_REG_STR_OFFSET  0x10
  -#define PM8941_WLED_REG_STR_MOD_EN_BASE  0x60
-#define  PM8941_WLED_REG_STR_MOD_MASK  BIT(7)
-#define  PM8941_WLED_REG_STR_MOD_ENBIT(7)
+#define WLED3_SINK_REG_STR_MOD_EN_BASE 0x60
+#define  WLED3_SINK_REG_STR_MOD_MASK   BIT(7)
+#define  WLED3_SINK_REG_STR_MOD_EN BIT(7)
  -#define PM8941_WLED_REG_STR_SCALE_BASE   0x62
-#define  PM8941_WLED_REG_STR_SCALE_MASK0x1f
+#define WLED3_SINK_REG_STR_FULL_SCALE_CURR 0x62
+#define  WLED3_SINK_REG_STR_FULL_SCALE_CURR_MASK   0x1f
  -#define PM8941_WLED_REG_STR_MOD_SRC_BASE 0x63
-#define  PM8941_WLED_REG_STR_MOD_SRC_MASK  0x01
-#define  PM8941_WLED_REG_STR_MOD_SRC_INT   0x00
-#define  PM8941_WLED_REG_STR_MOD_SRC_EXT   0x01
+#define WLED3_SINK_REG_STR_MOD_SRC_BASE0x63
+#define  WLED3_SINK_REG_STR_MOD_SRC_MASK   0x01
+#define  WLED3_SINK_REG_STR_MOD_SRC_INT0x00
+#define  WLED3_SINK_REG_STR_MOD_SRC_EXT0x01
  -#define PM8941_WLED_REG_STR_CABC_BASE0x66
-#define  PM8941_WLED_REG_STR_CABC_MASK BIT(7)
-#define  PM8941_WLED_REG_STR_CABC_EN   BIT(7)
+#define WLED3_SINK_REG_STR_CABC_BASE   0x66
+#define  WLED3_SINK_REG_STR_CABC_MASK  BIT(7)
+#define  WLED3_SINK_REG_STR_CABC_ENBIT(7)
  -struct pm8941_wled_config {
-   u32 i_boost_limit;

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

[kgunda]

On 2018-07-20 19:37, Daniel Thompson wrote:

On 09/07/18 11:22, 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 
Reviewed-by: Bjorn Andersson 
---
Changes from V3:
- Added Reviewed by tag.
- Addressed minor comments from Vinod

  drivers/video/backlight/qcom-wled.c | 132 
++--

  1 file changed, 128 insertions(+), 4 deletions(-)

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

index 362d254..a14f1a6 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 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -64,6 +67,16 @@
  #define WLED3_SINK_REG_STR_CABC(n)(0x66 + (n * 0x10))
  #define  WLED3_SINK_REG_STR_CABC_MASK BIT(7)
  +/* WLED4 specific 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
+
  /* WLED4 specific sink registers */
  #define WLED4_SINK_REG_CURR_SINK  0x46
  #define  WLED4_SINK_REG_CURR_SINK_MASKGENMASK(7, 4)
@@ -113,17 +126,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 thread irq handler */
+   ktime_t last_short_event;
u16 ctrl_addr;
u16 sink_addr;
u16 max_string_count;
u32 brightness;
u32 max_brightness;
+   u32 short_count;
+   bool disabled_by_short;
+   bool has_short_detect;
struct wled_config cfg;
int (*wled_set_brightness)(struct wled *wled, u16 brightness);
@@ -174,6 +193,9 @@ static int wled_module_enable(struct wled *wled, 
int val)

  {
int rc;
  + if (wled->disabled_by_short)
+   return -EFAULT;


EFAULT means bad address (memory fault). Perhaps EIO or maybe even
something more exotic like ENXIO might be more appropriate?



+
rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
WLED_CTRL_REG_MOD_EN,
WLED_CTRL_REG_MOD_EN_MASK,
@@ -210,18 +232,19 @@ static int wled_update_status(struct 
backlight_device *bl)

bl->props.state & BL_CORE_FBBLANK)
brightness = 0;
  + mutex_lock(>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_sync_toggle(wled);
if (rc < 0) {
dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
-   return rc;
+   goto unlock_mutex;
}
}
  @@ -229,15 +252,61 @@ 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(>lock);
+
return rc;
  }
  +#define WLED_SHORT_DLY_MS20
+#define WLED_SHORT_CNT_MAX 5
+#define WLED_SHORT_RESET_CNT_DLY_USUSEC_PER_SEC
+
+static irqreturn_t wled_short_irq_handler(int irq, void *_wled)
+{
+   struct wled *wled = _wled;
+   int rc;
+   s64 elapsed_time;
+
+   wled->short_count++;
+   mutex_lock(>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;


Shouldn't this reset to 1 (e.g. we have just seen a short).


Daniel.

Ok. I will change it in the next series.

Re: [PATCH V4 6/8] backlight: qcom-wled: Add support for WLED4 peripheral

[kgunda]

On 2018-07-21 02:51, Pavel Machek wrote:

Hi!


>@@ -332,6 +529,7 @@ static u32 wled_values(const struct wled_var_cfg *cfg, u32 
idx)
>  }
>  #define   WLED3   3
>+#defineWLED4   4

Are these macros always going to define 3 to be 3 and 4 to be 4. If so 
we
probably don't need them (and they should be removed from patch 5/8 
too).


There is a degree of nitpicking here however. The rest looks OK to me.


Really nitpicking, and I don't care much either way, but as natural
numbering would be 0-based, I'd keep the defines.
Pavel
Thanks Pavale/Daniel! I will remove the macros and use the number 3 and 
4 instead.

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

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

[kgunda]

On 2018-06-23 04:39, Bjorn Andersson wrote:

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


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


Thanks!


> >
> > diff --git a/drivers/video/backlight/qcom-wled.c
> > b/drivers/video/backlight/qcom-wled.c
> [..]
> >
> >  /* WLED3 sink registers */
> >  #define WLED3_SINK_REG_SYNC  0x47
>
> Drop the 3 from this if it's common between the two.
>
> > -#define  WLED3_SINK_REG_SYNC_MASK0x07
> > +#define  WLED3_SINK_REG_SYNC_MASKGENMASK(2, 0)
> > +#define  WLED4_SINK_REG_SYNC_MASKGENMASK(3, 0)
> >  #define  WLED3_SINK_REG_SYNC_LED1BIT(0)
> >  #define  WLED3_SINK_REG_SYNC_LED2BIT(1)
> >  #define  WLED3_SINK_REG_SYNC_LED3BIT(2)
> > +#define  WLED4_SINK_REG_SYNC_LED4BIT(3)
> >  #define  WLED3_SINK_REG_SYNC_ALL 0x07
> > +#define  WLED4_SINK_REG_SYNC_ALL 0x0f
> >  #define  WLED3_SINK_REG_SYNC_CLEAR   0x00
> >
> [..]
> > +static int wled4_set_brightness(struct wled *wled, u16 brightness)
>
> Afaict this is identical to wled3_set_brightness() with the exception
> that there's a minimum brightness and the base address for the
> brightness registers are different.
>
> I would suggest that you add a min_brightness to wled and that you
> figure out a way to carry the brightness base register address; and by
> that you squash these two functions.
>
There are four different parameters. 1) minimum brightness 2) WLED 
base

addresses
3) Brightness base addresses 4) the brightness sink registers address
offsets also
different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as 
in

wled4 0x57, 0x67, 0x77, 0x87).



Sorry, I must have gotten lost in the defines, I see the difference
between the two register layouts now. If you retain the old mechanism 
of

doing the math openly in the function this would have been obvious.


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


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

Regards,
Bjorn
--

Thanks for that !
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH V3 3/7] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

On 2018-06-21 00:35, Rob Herring wrote: 

> 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 
>> ---
>> .../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:   
>> -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:   
>> +Definition:   uA; per-string current limit; value from 0 to 3 with
>> +  2500 uA step. Default 25000 uA.
> 
> This doesn't really seem worth adding just to add '-microamp'.
> Thanks for reviewing it!. I added this because the step value for 
> PM8941(WLED3) and PMI8998(WLED4)

> are different. for WLED3 the step is 5mA and for WLED4 the step is 2.5mA. To 
> mantain

> the backward compatibility i have added the new property with out modifying 
> the existing

> one (qcom,current-limit).
> 
>> - 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.

> Yes. It is same. WLED3 has the OVP values 27V, 29V, 32V, 35V, where as

> WELD4 has 18.1V, 19.6V, 29.6V, 31.1V.
> 
>> +Usage:optional
>> +Value type:   
>> +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:   
>> 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:   
>> +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:   
>> +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:   
>> +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 [].

> It is u32. I will correct dts syntax in next series as <0x00 0x01 0x02 0x03>,

> which is mentioned in the example.

+
+- qcom,external-pfet
+Usage:optional
+Value type:   
+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:   
+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 

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

[kgunda]

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

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


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

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

Signed-off-by: Kiran Gunda 
---
 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_BRIGHTNESS2048
-
+#define WLED_SOFT_START_DLY_US 1
 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

 /* WLED3 control registers */
+#define WLED3_CTRL_REG_FAULT_STATUS0x08
+#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_MASKBIT(7)

+#define WLED3_CTRL_REG_FEEDBACK_CONTROL0x48
+
 #define WLED3_CTRL_REG_FREQ0x4c
 #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,

+);
+   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(>ovp_work, WLED_SOFT_START_DLY_US);
+   } else {
+   cancel_delayed_work(>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) {
+  

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

[kgunda]

On 2018-06-20 11:17, Vinod wrote:

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


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


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

may do so with a thread handler


Will fix the comment in the next series.

+#define WLED_SHORT_DLY_MS  20
+#define WLED_SHORT_CNT_MAX 5
+#define WLED_SHORT_RESET_CNT_DLY_USUSEC_PER_SEC


an empty line after defines would be better


Will add it in the next series.

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


superfluous initialization of rc

Will remove the initialization in next series.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[kgunda]

On 2018-06-20 10:44, Bjorn Andersson wrote:

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


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

Signed-off-by: Kiran Gunda 
---
 drivers/video/backlight/qcom-wled.c | 635 


 1 file changed, 503 insertions(+), 132 deletions(-)


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



Sure. I will split it in the next series.


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

[..]


 /* WLED3 sink registers */
 #define WLED3_SINK_REG_SYNC0x47


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


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


[..]

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


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

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

There are four different parameters. 1) minimum brightness 2) WLED base 
addresses
3) Brightness base addresses 4) the brightness sink registers address 
offsets also
different for wled 3 and wled4. (in wled3 0x40, 0x42, 0x44, where as in 
wled4 0x57, 0x67, 0x77, 0x87).


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

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

-   rc = regmap_bulk_write(wled->regmap,
-   wled->addr + WLED3_CTRL_REG_VAL_BASE + 2 * i,
-   v, 2);
-   if (rc)
+   /* WLED4's lower limit of operation is 0.4% */
+   if (brightness > 0 && brightness < low_limit)
+   brightness = low_limit;
+
+   v[0] = brightness & 0xff;
+   v[1] = (brightness >> 8) & 0xf;
+
+   for (i = 0;  i < wled->cfg.num_strings; ++i) {
+   rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
+  WLED4_SINK_REG_BRIGHT(i), v, 2);
+   if (rc < 0)
return rc;
}

+   return 0;
+}
+
+static int wled_module_enable(struct wled *wled, int val)
+{
+   int rc;
+
+   rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
+   WLED3_CTRL_REG_MOD_EN,
+   WLED3_CTRL_REG_MOD_EN_MASK,
+   WLED3_CTRL_REG_MOD_EN_MASK);


This should depend on val.


Will fix it in the next series.

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

rc = regmap_update_bits(wled->regmap,
-   wled->addr + WLED3_SINK_REG_SYNC,
-   WLED3_SINK_REG_SYNC_MASK, WLED3_SINK_REG_SYNC_CLEAR);
+   wled->ctrl_addr + WLED3_SINK_REG_SYNC,
+   WLED3_SINK_REG_SYNC_MASK,
+   WLED3_SINK_REG_SYNC_CLEAR);
+
return rc;
 }

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


This is identical to wled3_sync_toggle() if you express the SYNC_MASK 
as

GENMASK(max-string-count, 0);


Will change it in the next series.

 {
int rc;
-   int i;

rc = regmap_update_bits(wled->regmap,
-   

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

[kgunda]

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

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


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



Looks good, just three minor comments below.


Signed-off-by: Kiran Gunda 
---
 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 

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

-#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_BASE0x40

-#define PM8941_WLED_REG_MOD_EN 0x46
-#define  PM8941_WLED_REG_MOD_EN_BITBIT(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_MASKBIT(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_CLEAR0x00
+#define WLED3_CTRL_REG_FREQ0x4c
+#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_OVP0x4d
-#define  PM8941_WLED_REG_OVP_MASK  0x03
+#define WLED3_CTRL_REG_ILIMIT  0x4e
+#define  WLED3_CTRL_REG_ILIMIT_MASK0x07

-#define PM8941_WLED_REG_BOOST  0x4e
-#define  PM8941_WLED_REG_BOOST_MASK0x07
+/* WLED3 sink registers */
+#define WLED3_SINK_REG_SYNC0x47
+#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

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

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

[kgunda]

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

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


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

Signed-off-by: Kiran Gunda 


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 

Regards,
Bjorn


Thanks Bjorn for reviewing. Sure I will keep the tags when re-posting.
I have mentioned the consolidated changes in the 0th patch. Anyways,
I will keep it for individual patches from next time.

---
 .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}| 2 
+-
 drivers/video/backlight/Kconfig   | 8 

 drivers/video/backlight/Makefile  | 2 
+-

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

 rename drivers/video/backlight/{pm8941-wled.c => qcom-wled.c} (100%)

diff --git 
a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

similarity index 95%
rename from 
Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
rename to 
Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

index e5b294d..fb39e32 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pm8941-wled.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt
@@ -1,4 +1,4 @@
-Binding for Qualcomm PM8941 WLED driver
+Binding for Qualcomm Technologies, Inc. WLED driver

 Required properties:
 - compatible: should be "qcom,pm8941-wled"
diff --git a/drivers/video/backlight/Kconfig 
b/drivers/video/backlight/Kconfig

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

-config BACKLIGHT_PM8941_WLED
-   tristate "Qualcomm PM8941 WLED Driver"
+config BACKLIGHT_QCOM_WLED
+   tristate "Qualcomm PMIC WLED Driver"
select REGMAP
help
- If you have the Qualcomm PM8941, say Y to enable a driver for the
- WLED block.
+ If you have the Qualcomm PMIC, say Y to enable a driver for the
+ WLED block. Currently it supports PM8941 and PMI8998.

 config BACKLIGHT_SAHARA
tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
diff --git a/drivers/video/backlight/Makefile 
b/drivers/video/backlight/Makefile

index 5e28f01..6fd76ef 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -49,8 +49,8 @@ obj-$(CONFIG_BACKLIGHT_OMAP1) += omap1_bl.o
 obj-$(CONFIG_BACKLIGHT_OT200)  += ot200_bl.o
 obj-$(CONFIG_BACKLIGHT_PANDORA)+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PCF50633)   += pcf50633-backlight.o
-obj-$(CONFIG_BACKLIGHT_PM8941_WLED)+= pm8941-wled.o
 obj-$(CONFIG_BACKLIGHT_PWM)+= pwm_bl.o
+obj-$(CONFIG_BACKLIGHT_QCOM_WLED)  += qcom-wled.o
 obj-$(CONFIG_BACKLIGHT_SAHARA) += kb3886_bl.o
 obj-$(CONFIG_BACKLIGHT_SKY81452)   += sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)   += tosa_bl.o
diff --git a/drivers/video/backlight/pm8941-wled.c 
b/drivers/video/backlight/qcom-wled.c

similarity index 100%
rename from drivers/video/backlight/pm8941-wled.c
rename to drivers/video/backlight/qcom-wled.c
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

 a Linux Foundation Collaborative Project


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

Re: [PATCH V3 3/7] backlight: qcom-wled: Add new properties for PMI8998

[kgunda]

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

On Tue 19 Jun 04:13 PDT 2018, Kiran Gunda wrote:
diff --git 
a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt 
b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.txt

[..]

 - qcom,num-strings
Usage:optional
Value type:   
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

[..]

+- qcom,enabled-strings
+   Usage:optional
+   Value tyoe:   
+   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].


I would suggest omitting the defaults, as we can see in several places
in this document we end up having to update the document with new
defaults for each platform.

Also, per the defaults of the optional qcom,num-strings these are
already the defaults...


Sure. I will remove the defaults 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-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>;


Please omit the pmi8998 example as well, there's no real benefit of
adding similar examples for each platform.


Sure. I will remove it.

Regards,
Bjorn

___
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,   wrote:

On 2018-05-08 15:55, kgu...@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 
---
 .../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:   
+   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:   
+   Definition:   Base address of the WLED modules.
+
+- interrupts
+   Usage:optional
+   Value type:   
+   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:   
+   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:   
+   Definition:   The name of the backlight device
+
+- default-brightness
+   Usage:optional
+   Value type:   
+   Definition:   brightness value on boot, value from: 0-4095
+ Default: 2048
+
+- qcom,current-limit
+   Usage:optional
+   Value type:   
+   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 2 uA
+ For pmi8998: from 0 to 3 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 

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

[kgunda]

On 2018-05-08 15:55, kgu...@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 
---
 .../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:   
+   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:   
+   Definition:   Base address of the WLED modules.
+
+- interrupts
+   Usage:optional
+   Value type:   
+   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:   
+   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:   
+   Definition:   The name of the backlight device
+
+- default-brightness
+   Usage:optional
+   Value type:   
+   Definition:   brightness value on boot, value from: 0-4095
+ Default: 2048
+
+- qcom,current-limit
+   Usage:optional
+   Value type:   
+   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 2 uA
+ For pmi8998: from 0 to 3 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:   
+   

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 
---
 .../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:   
+   Definition:   should be "qcom,pm8941-wled" or "qcom,pmi8998-wled".
+ or "qcom,pm660l-wled".
+
+- reg
+   Usage:required
+   Value type:   
+   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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, kgu...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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_USHZ /* 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,
+_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,
+_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(>lock);
+   rc = wled_auto_string_detection(wled);
+   if (!rc)
+   wled->auto_detection_done = true;
+   mutex_unlock(>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(>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. 

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, kgu...@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(>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

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

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

[kgunda]

On 2018-05-09 10:36, kgu...@codeaurora.org wrote:

On 2018-05-08 22:49, Bjorn Andersson wrote:

On Tue 08 May 05:26 PDT 2018, kgu...@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(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[kgunda]

On 2018-05-07 22:10, Rob Herring wrote:

On Thu, May 03, 2018 at 03:42:28PM +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 
---
 .../bindings/leds/backlight/{pm8941-wled.txt => qcom-wled.txt}| 2 
+-


Please split DT bindings from driver patches.


Sure. Will do that in the next series.
 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%)

--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

___
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-08 22:47, Bjorn Andersson wrote:

On Tue 08 May 03:25 PDT 2018, kgu...@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:   
> > + 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

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

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

[kgunda]

On 2018-05-08 16:05, Daniel Thompson wrote:

On Thu, May 03, 2018 at 03:42:30PM +0530, 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 
---
 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 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -23,7 +26,9 @@

 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

-/* Control registers */
+/* WLED3 Control registers */
+#define WLED3_CTRL_REG_FAULT_STATUS0x08
+
 #define WLED3_CTRL_REG_MOD_EN  0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASKBIT(7)

@@ -36,7 +41,17 @@
 #define WLED3_CTRL_REG_ILIMIT  0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASKGENMASK(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_SYNC0x47
 #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(>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(>lock);
+
return rc;
 }

+#define WLED_SHORT_DLY_MS  20
+#define WLED_SHORT_CNT_MAX 5
+#define WLED_SHORT_RESET_CNT_DLY_USHZ


So we think a short report every quarter second is harmless?


Yes. We wait for the short to report 5 times with in this time. If the 
short
still persists, we go and disable the WLED module to avoid the damage. 
This is

suggested by the HW team.


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

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

[kgunda]

On 2018-05-08 16:09, Daniel Thompson wrote:

On Thu, May 03, 2018 at 03:42:31PM +0530, 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.


Why detect and handle it? The interrupt handler doesn't appear to do
anything other than clear the interrupt... and it appears that the
interrupt can be masked.


Planing to squash this patch with the auto-string-detection patch in the 
next series,

where the OVP is used for the right string detection.
When WLED module is enabled, keep OVP fault interrupt disabled for 10 
ms to

account for soft start delay.

Signed-off-by: Kiran Gunda 
---
 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_BRIGHTNESS2048
-
+#define WLED_SOFT_START_DLY_US 1
 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

 /* WLED3 Control registers */
 #define WLED3_CTRL_REG_FAULT_STATUS0x08
+#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_MASKBIT(7)
+#define  WLED3_CTRL_REG_MOD_EN_BIT BIT(7)

 #define WLED3_CTRL_REG_FREQ0x4c
 #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,

+);
+   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(>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, _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, _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, 

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

[kgunda]

On 2018-05-08 14:06, Daniel Thompson wrote:

On Mon, May 07, 2018 at 11:26:59AM +0530, kgu...@codeaurora.org wrote:

Hi Jingoo Han,
Thanks for the response.

Thanks,
Kiran
On 2018-05-04 21:25, Jingoo Han wrote:
> On Thursday, May 3, 2018 6:12 AM, Kiran Gunda wrote:
>
> If you really want someone to review your patch, please add more
> detailed
> explanations.
>
> 1. Please add 0th patch.
I have already added the 0th patch (cover letter). Following is the 
subject

for the same.
forwarding the same to you.
[PATCH V2 0/5] backlight: qcom-wled: Support for QCOM wled driver


Perhaps you should interpret Jingoo's comment as a request to improve
the To: and Cc: list on the 0th patch the next time you circulate it.

Many maintainers configure their mailers to give patches where they are
in the To: or Cc: list special treatment so it confusing when a series
arrives with a spartan set of recipients for the 0th patch (and doubly
confusing when every patch in the set targets the same driver).


Daniel.
Ok. Got it. Not sure how the To: Cc: list on the 0th patch got shrink, 
while I was
using the To: Cc: list same for all the patches. Anyways, will double 
check of it while

circulating the next series.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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 
---
 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 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -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_STATUS0x08


Unused.


Will remove it in the next series.

+
 #define WLED3_CTRL_REG_MOD_EN  0x46
 #define  WLED3_CTRL_REG_MOD_EN_MASKBIT(7)

@@ -36,7 +41,17 @@
 #define WLED3_CTRL_REG_ILIMIT  0x4e
 #define  WLED3_CTRL_REG_ILIMIT_MASKGENMASK(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_SYNC0x47
 #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(>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(>lock);
+
return rc;
 }

+#define WLED_SHORT_DLY_MS  20
+#define WLED_SHORT_CNT_MAX 5
+#define WLED_SHORT_RESET_CNT_DLY_USHZ


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

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 
---
 .../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:   
+   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:   
+   Definition:   Base address of the WLED modules.
+
+- interrupts
+   Usage:optional
+   Value type:   
+   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:   
+   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:   
+   Definition:   The name of the backlight device
+
+- default-brightness
+   Usage:optional
+   Value type:   
+   Definition:   brightness value on boot, value from: 0-4095
+ Default: 2048
+
+- qcom,current-limit
+   Usage:optional
+   Value type:   
+   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 2 uA
+ For pmi8998: from 0 to 3 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:   
+   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
+   

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

[kgunda]

On 2018-05-07 22:15, Rob Herring wrote:

On Thu, May 03, 2018 at 03:42:29PM +0530, 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.

Signed-off-by: Kiran Gunda 
---
 .../bindings/leds/backlight/qcom-wled.txt  | 172 -


Please split bindings to a separate patch.

This is also a whole lot of churn re-formatting. That is fine, but 
don't

mix actual changes to the binding with re-formatting. You can do 2
patches in this case:

- move and reformat (as long as the move shows up as a move
and not a remove and add)
- additions for new chips


Sure. I will do it in the next series.
 drivers/video/backlight/qcom-wled.c| 749 
+++--

 2 files changed, 696 insertions(+), 225 deletions(-)

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

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 
---
 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_BRIGHTNESS2048
-
+#define WLED_SOFT_START_DLY_US 1
 #define WLED3_SINK_REG_BRIGHT_MAX  0xFFF

 /* WLED3 Control registers */
 #define WLED3_CTRL_REG_FAULT_STATUS0x08
+#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_MASKBIT(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_FREQ0x4c
 #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,

+);
+   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(>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, _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, _sts);
+

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:>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 Gunda2018-05-03  
263

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 264  static int wled_update_status(struct backlight_device *bl)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
265  {

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 266struct wled *wled = bl_get_data(bl);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 267u16 brightness = bl->props.brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 268int rc = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
269

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 270if (bl->props.power != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 271bl->props.fb_blank != FB_BLANK_UNBLANK ||
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 272bl->props.state & BL_CORE_FBBLANK)
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 273brightness = 0;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
274

38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 275mutex_lock(>lock);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 276if (brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 277rc = wled->wled_set_brightness(wled, brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 278if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 279dev_err(wled->dev, "wled failed to set brightness 
rc:%d\n",
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 280rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 281goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
282  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
283

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 284rc = wled->wled_sync_toggle(wled);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 285if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 286dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
93c64f1e drivers/leds/leds-pm8941-wled.c Courtney Cavin 2015-03-12
 287return rc;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
288  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
289  	}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
290

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 291if (!!brightness != !!wled->brightness) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 292rc = wled_module_enable(wled, !!brightness);
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 293if (rc < 0) {
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 294dev_err(wled->dev, "wled enable failed rc:%d\n", rc);
38561998 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 295goto unlock_mutex;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
296  		}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
297  	}
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
298

18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03
 299wled->brightness = brightness;
18e7abb8 drivers/video/backlight/qcom-wled.c Kiran Gunda2018-05-03  
300

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

[kgunda]

Hi Jingoo Han,
Thanks for the response.

Thanks,
Kiran
On 2018-05-04 21:25, Jingoo Han wrote:

On Thursday, May 3, 2018 6:12 AM, Kiran Gunda wrote:

If you really want someone to review your patch, please add more 
detailed

explanations.

1. Please add 0th patch.
I have already added the 0th patch (cover letter). Following is the 
subject for the same.

forwarding the same to you.
[PATCH V2 0/5] backlight: qcom-wled: Support for QCOM wled driver


2. Please add what the difference is between V1 and V2 patches.

Already added the difference between V1 and V2 in the cover letter (0th 
patch).
Please let me know if you are looking for any other info. Following is 
the difference

added between v1 and v2.



changes from v1:
Fixed the commit message for
backlight: qcom-wled: Rename pm8941-wled.c to qcom-wled.c

If you cannot understand what I am saying, just ask another person who 
have

an experience to send patches to Linux mailing-list.

Best regards,
Jingoo Han



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 
---
 .../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

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