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

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

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

quoted

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

quoted

On Wed, 16 Oct 2019, Kiran Gunda wrote:

quoted

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 <redacted>
---
 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

[...]

quoted

quoted

quoted

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

[...]

quoted

quoted

quoted

+	} 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.

[...]

quoted

quoted

quoted

+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(&pdev->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.

quoted

quoted

If, for instance, polling mode is enabled, maybe something like this
would be better?

      dev_warn(&pdev->dev, "No IRQ found, falling back to polling
mode\n");