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

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

\
 
 \ /
  Last update: 2018-05-09 09:15    [W:0.587 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site