Messages in this thread | | | Date | Fri, 20 Apr 2018 11:13:19 +0530 | From | kgunda@codeauro ... | Subject | Re: [PATCH V1 4/4] qcom: spmi-wled: Add auto-calibration logic support |
| |
On 2018-04-19 21:28, Bjorn Andersson wrote: > On Thu 19 Apr 03:45 PDT 2018, kgunda@codeaurora.org wrote: > >> >> On 2017-12-05 11:10, Bjorn Andersson wrote: >> > On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote: >> > >> > > The auto-calibration 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. >> > > >> > >> > So it's not auto "calibration" it's auto "detection" of strings? >> > >> Hi Bjorn, >> Sorry for late response. Please find my answers. >> > > No worries, happy to hear back from you! > Thanks! >> Correct. This is the auto detection, This is the name given by the >> HW/systems team. > > I think the name should be considered a "hardware bug", that we can > work > around in software (give it a useful name and document what the > original > name was). > I don't think this is the "hardware bug". Rather we can say HW doesn't support it. Hence, we are implementing it as a SW feature to detect the strings present on the display panel, if the user fails to give the correct strings. As you suggested I will rename this to "auto detection" instead of "auto calibration".
>> > When is this feature needed? >> > >> This feature is needed if the string configuration is given wrong in >> the DT node by the user. > > DT describes the hardware and for all other nodes it must do so > accurately. > But the user may not be aware of the strings present on the display panel or may be using the same software on different devices which have different strings present. > For cases where the hardware supports auto detection of functionality > we > remove information from DT and rely on that logic to figure out the > hardware. We do not use it to reconfigure the hardware once we detect > an > error. So when auto-detection is enabled it should always be used to > probe the hardware. > The auto string detection is not supported in any qcom hardware and i don't think there is a plan to introduce in new hardware also.
> Regards, > Bjorn > >> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> >> > > --- >> > > .../bindings/leds/backlight/qcom-spmi-wled.txt | 5 + >> > > drivers/video/backlight/qcom-spmi-wled.c | 304 >> > > ++++++++++++++++++++- >> > > 2 files changed, 306 insertions(+), 3 deletions(-) >> > > >> > > diff --git >> > > a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt >> > > b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt >> > > index d39ee93..f06c0cd 100644 >> > > --- >> > > a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt >> > > +++ >> > > b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt >> > > @@ -94,6 +94,11 @@ The PMIC is connected to the host processor via >> > > SPMI bus. >> > > Definition: Interrupt names associated with the interrupts. >> > > Currently supported interrupts are "sc-irq" and "ovp-irq". >> > > >> > > +- qcom,auto-calibration >> > >> > qcom,auto-string-detect? >> > >> ok. Will address in the next patch. >> > > + Usage: optional >> > > + Value type: <bool> >> > > + Definition: Enables auto-calibration of the WLED sink configuration. >> > > + >> > > Example: >> > > >> > > qcom-wled@d800 { >> > > diff --git a/drivers/video/backlight/qcom-spmi-wled.c >> > > b/drivers/video/backlight/qcom-spmi-wled.c >> > > index 8b2a77a..aee5c56 100644 >> > > --- a/drivers/video/backlight/qcom-spmi-wled.c >> > > +++ b/drivers/video/backlight/qcom-spmi-wled.c >> > > @@ -38,11 +38,14 @@ >> > > #define QCOM_WLED_CTRL_SC_FAULT_BIT BIT(2) >> > > >> > > #define QCOM_WLED_CTRL_INT_RT_STS 0x10 >> > > +#define QCOM_WLED_CTRL_OVP_FLT_RT_STS_BIT BIT(1) >> > >> > The use of BIT() makes this a mask and not a bit number, so if you just >> > drop that you can afford to spell out the "FAULT" like the data sheet >> > does. Perhaps even making it QCOM_WLED_CTRL_OVP_FAULT_STATUS ? >> > >> ok. Will change it in the next series. >> > > >> > > #define QCOM_WLED_CTRL_MOD_ENABLE 0x46 >> > > #define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7) >> > > #define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7 >> > > >> > > +#define QCOM_WLED_CTRL_FDBK_OP 0x48 >> > >> > This is called WLED_CTRL_FEEDBACK_CONTROL, why the need to make it >> > unreadable? >> > >> Ok. Will address it in next series. >> > > + >> > > #define QCOM_WLED_CTRL_SWITCH_FREQ 0x4c >> > > #define QCOM_WLED_CTRL_SWITCH_FREQ_MASK GENMASK(3, 0) >> > > >> > > @@ -99,6 +102,7 @@ struct qcom_wled_config { >> > > int ovp_irq; >> > > bool en_cabc; >> > > bool ext_pfet_sc_pro_en; >> > > + bool auto_calib_enabled; >> > > }; >> > > >> > > struct qcom_wled { >> > > @@ -108,18 +112,25 @@ struct qcom_wled { >> > > struct mutex lock; >> > > struct qcom_wled_config cfg; >> > > ktime_t last_sc_event_time; >> > > + ktime_t start_ovp_fault_time; >> > > u16 sink_addr; >> > > u16 ctrl_addr; >> > > + u16 auto_calibration_ovp_count; >> > > u32 brightness; >> > > u32 sc_count; >> > > bool prev_state; >> > > bool ovp_irq_disabled; >> > > + bool auto_calib_done; >> > > + bool force_mod_disable; >> > > }; >> > > >> > > static int qcom_wled_module_enable(struct qcom_wled *wled, int val) >> > > { >> > > int rc; >> > > >> > > + if (wled->force_mod_disable) >> > > + return 0; >> > > + >> > > rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + >> > > QCOM_WLED_CTRL_MOD_ENABLE, QCOM_WLED_CTRL_MOD_EN_MASK, >> > > val << QCOM_WLED_CTRL_MODULE_EN_SHIFT); >> > > @@ -187,12 +198,10 @@ static int qcom_wled_set_brightness(struct >> > > qcom_wled *wled, u16 brightness) >> > > v[1] = (brightness >> 8) & 0xf; >> > > >> > > for (i = 0; (string_cfg >> i) != 0; i++) { >> > > - if (string_cfg & BIT(i)) { >> > >> > Why was this check here in the first place, if it's now fine to >> > configure the brightness of all strings? >> > >> > Also, a single-string config of 0b0001 will only set brightness on the >> > first string, while 0b1000 will set brightness on all strings. >> > >> I will correct/remove it next series. >> > > rc = regmap_bulk_write(wled->regmap, wled->sink_addr + >> > > QCOM_WLED_SINK_BRIGHT_LSB_REG(i), v, 2); >> > > if (rc < 0) >> > > return rc; >> > > - } >> > > } >> > > >> > > return 0; >> > > @@ -294,6 +303,262 @@ static irqreturn_t >> > > qcom_wled_sc_irq_handler(int irq, void *_wled) >> > > return IRQ_HANDLED; >> > > } >> > > >> > > +#define AUTO_CALIB_BRIGHTNESS 200 >> > > +static int qcom_wled_auto_calibrate(struct qcom_wled *wled) >> > > +{ >> > > + int rc = 0, i; >> > > + u32 sink_config = 0, int_sts; >> > > + u8 reg = 0, sink_test = 0, sink_valid = 0; >> > > + u8 string_cfg = wled->cfg.string_cfg; >> > > + >> > > + /* read configured sink configuration */ >> > > + rc = regmap_read(wled->regmap, wled->sink_addr + >> > > + QCOM_WLED_SINK_CURR_SINK_EN, &sink_config); >> > > + if (rc < 0) { >> > > + pr_err("Failed to read SINK configuration rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* disable the module before starting calibration */ >> > > + rc = regmap_update_bits(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK, 0); >> > > + if (rc < 0) { >> > > + pr_err("Failed to disable WLED module rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > >> > Any error handling beyond this point seems to leave the backlight off >> > (indefinitely?), this does seem like potentially bad user experience... >> Ok. will address in next series. >> > >> > In particular I wonder about the case when this would happen at some >> > random time, minutes, hours, days, months after the device was booted. >> > >> This will happen for every reboot. >> > > + >> > > + /* set low brightness across all sinks */ >> > > + rc = qcom_wled_set_brightness(wled, AUTO_CALIB_BRIGHTNESS); >> > > + if (rc < 0) { >> > > + pr_err("Failed to set brightness for calibration rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + if (wled->cfg.en_cabc) { >> > > + for (i = 0; (string_cfg >> i) != 0; i++) { >> > > + reg = 0; >> > > + rc = regmap_update_bits(wled->regmap, wled->sink_addr + >> > > + QCOM_WLED_SINK_CABC_REG(i), >> > > + QCOM_WLED_SINK_CABC_MASK, reg); >> > >> > Just replace "reg" with 0. >> Ok. will address in next series. >> > >> > > + if (rc < 0) >> > > + goto failed_calib; >> > > + } >> > > + } >> > > + >> > > + /* disable all sinks */ >> > > + rc = regmap_write(wled->regmap, >> > > + wled->sink_addr + QCOM_WLED_SINK_CURR_SINK_EN, 0); >> > > + if (rc < 0) { >> > > + pr_err("Failed to disable all sinks rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* iterate through the strings one by one */ >> > > + for (i = 0; (string_cfg >> i) != 0; i++) { >> > > + sink_test = 1 << (QCOM_WLED_SINK_CURR_SINK_SHFT + i); >> > >> > BIT(QCOM_WLED_SINK_CURR_SINK_SHFT + i); >> > >> Will address in next series. >> > > + >> > > + /* Enable feedback control */ >> > > + rc = regmap_write(wled->regmap, wled->ctrl_addr + >> > > + QCOM_WLED_CTRL_FDBK_OP, i + 1); >> > > + if (rc < 0) { >> > > + pr_err("Failed to enable feedback for SINK %d rc = %d\n", >> > > + i + 1, rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* enable the sink */ >> > > + rc = regmap_write(wled->regmap, wled->sink_addr + >> > > + QCOM_WLED_SINK_CURR_SINK_EN, sink_test); >> > > + if (rc < 0) { >> > > + pr_err("Failed to configure SINK %d rc=%d\n", >> > > + i + 1, rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* Enable the module */ >> > > + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr + >> > > + QCOM_WLED_CTRL_MOD_ENABLE, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK); >> > >> > I like the use of regmap_update_bits(..., MASK, MASK) it's clean, but >> > makes me wonder why it's done differently in qcom_wled_module_enable(). >> > >> will address it in the next series. >> > > + if (rc < 0) { >> > > + pr_err("Failed to enable WLED module rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + usleep_range(QCOM_WLED_SOFT_START_DLY_US, >> > > + QCOM_WLED_SOFT_START_DLY_US + 1000); >> > > + >> > > + rc = regmap_read(wled->regmap, wled->ctrl_addr + >> > > + QCOM_WLED_CTRL_INT_RT_STS, &int_sts); >> > > + if (rc < 0) { >> > > + pr_err("Error in reading WLED_INT_RT_STS rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + if (int_sts & QCOM_WLED_CTRL_OVP_FAULT_BIT) >> > > + pr_debug("WLED OVP fault detected with SINK %d\n", >> > > + i + 1); >> > > + else >> > > + sink_valid |= sink_test; >> > > + >> > > + /* Disable the module */ >> > > + rc = regmap_update_bits(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK, 0); >> > > + if (rc < 0) { >> > > + pr_err("Failed to disable WLED module rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + } >> > > + >> > > + if (sink_valid == sink_config) { >> > > + pr_debug("WLED auto-calibration complete, default sink-config=%x >> > > OK!\n", >> > > + sink_config); >> > > + } else { >> > > + pr_warn("Invalid WLED default sink config=%x changing it to=%x\n", >> > > + sink_config, sink_valid); >> > > + sink_config = sink_valid; >> > > + } >> > > + >> > > + if (!sink_config) { >> > > + pr_warn("No valid WLED sinks found\n"); >> > > + wled->force_mod_disable = true; >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* write the new sink configuration */ >> > > + rc = regmap_write(wled->regmap, >> > > + wled->sink_addr + QCOM_WLED_SINK_CURR_SINK_EN, >> > > + sink_config); >> > > + if (rc < 0) { >> > > + pr_err("Failed to reconfigure the default sink rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* MODULATOR_EN setting for valid sinks */ >> > >> > "Enable valid sinks" >> > >> Will address it in the next series. >> > > + for (i = 0; (string_cfg >> i) != 0; i++) { >> > > + if (wled->cfg.en_cabc) { >> > > + reg = QCOM_WLED_SINK_CABC_EN; >> > >> > "reg" is a bad name of a variable holding the "value" to be written to a >> > register. >> > >> Will address it in the next series. >> > > + rc = regmap_update_bits(wled->regmap, wled->sink_addr + >> > > + QCOM_WLED_SINK_CABC_REG(i), >> > > + QCOM_WLED_SINK_CABC_MASK, reg); >> > >> > Again, just inline the value in the function call. >> > >> Will address it in the next series. >> > > + if (rc < 0) >> > > + goto failed_calib; >> > > + } >> > > + >> > > + if (sink_config & (1 << (QCOM_WLED_SINK_CURR_SINK_SHFT + i))) >> > >> > BIT(QCOM_WLED_SINK_CURR_SINK_SHFT + i) >> > >> Will address it in the next series. >> > > + reg = QCOM_WLED_SINK_REG_STR_MOD_EN; >> > > + else >> > > + reg = 0x0; /* disable modulator_en for unused sink */ >> > > + >> > > + rc = regmap_write(wled->regmap, wled->sink_addr + >> > > + QCOM_WLED_SINK_MOD_EN_REG(i), reg); >> > > + if (rc < 0) { >> > > + pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + } >> > > + >> > > + /* restore the feedback setting */ >> > > + rc = regmap_write(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_FDBK_OP, 0); >> > > + if (rc < 0) { >> > > + pr_err("Failed to restore feedback setting rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* restore brightness */ >> > > + rc = qcom_wled_set_brightness(wled, wled->brightness); >> > > + if (rc < 0) { >> > > + pr_err("Failed to set brightness after calibration rc=%d\n", >> > > + rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + rc = regmap_update_bits(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK, >> > > + QCOM_WLED_CTRL_MOD_EN_MASK); >> > > + if (rc < 0) { >> > > + pr_err("Failed to enable WLED module rc=%d\n", rc); >> > > + goto failed_calib; >> > > + } >> > > + >> > > + /* delay for WLED soft-start */ >> > >> > What comes after this that you want to delay? >> > >> > This delay is used to make the OVP IRQ not fire immediately, but as >> > we've now successfully executed the string auto detection run we're >> > never going to do anything in the OVP handler. >> > >> Will correct it in the next series. >> > > + usleep_range(QCOM_WLED_SOFT_START_DLY_US, >> > > + QCOM_WLED_SOFT_START_DLY_US + 1000); >> > > + >> > > +failed_calib: >> > > + return rc; >> > > +} >> > > + >> > > +#define WLED_AUTO_CAL_OVP_COUNT 5 >> > > +#define WLED_AUTO_CAL_CNT_DLY_US 1000000 /* 1 second */ >> > > +static bool qcom_wled_auto_cal_required(struct qcom_wled *wled) >> > > +{ >> > > + s64 elapsed_time_us; >> > > + >> > > + /* >> > > + * Check if the OVP fault was an occasional one >> > > + * or if its firing continuously, the latter qualifies >> > > + * for an auto-calibration check. >> > > + */ >> > > + if (!wled->auto_calibration_ovp_count) { >> > > + wled->start_ovp_fault_time = ktime_get(); >> > > + wled->auto_calibration_ovp_count++; >> > > + } else { >> > > + elapsed_time_us = ktime_us_delta(ktime_get(), >> > > + wled->start_ovp_fault_time); >> > > + if (elapsed_time_us > WLED_AUTO_CAL_CNT_DLY_US) >> > > + wled->auto_calibration_ovp_count = 0; >> > > + else >> > > + wled->auto_calibration_ovp_count++; >> > > + >> > > + if (wled->auto_calibration_ovp_count >= >> > > + WLED_AUTO_CAL_OVP_COUNT) { >> > > + wled->auto_calibration_ovp_count = 0; >> > > + return true; >> > > + } >> > > + } >> > > + >> > > + return false; >> > > +} >> > > + >> > > +static int qcom_wled_auto_calibrate_at_init(struct qcom_wled *wled) >> > >> > I presume this function is expected to detect if there is a invalid >> > configuration at boot and try to figure out which strings are actually >> > wired. >> > >> Correct. >> > > +{ >> > > + int rc; >> > > + u32 fault_status = 0, rt_status = 0; >> > > + >> > > + if (!wled->cfg.auto_calib_enabled) >> > > + return 0; >> > > + >> > > + rc = regmap_read(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_INT_RT_STS, >> > > + &rt_status); >> > > + if (rc < 0) >> > > + pr_err("Failed to read RT status rc=%d\n", rc); >> > > + >> > > + rc = regmap_read(wled->regmap, >> > > + wled->ctrl_addr + QCOM_WLED_CTRL_FAULT_STATUS, >> > > + &fault_status); >> > > + if (rc < 0) >> > > + pr_err("Failed to read fault status rc=%d\n", rc); >> > > + >> > > + if ((rt_status & QCOM_WLED_CTRL_OVP_FLT_RT_STS_BIT) || >> > > + (fault_status & QCOM_WLED_CTRL_OVP_FAULT_BIT)) { >> > >> > You should be able to drop the extra () around these. >> > >> Ok. Will remove it in the next series. >> > > + mutex_lock(&wled->lock); >> > > + rc = qcom_wled_auto_calibrate(wled); >> > > + if (rc < 0) >> > > + pr_err("Failed auto-calibration rc=%d\n", rc); >> > >> > qcom_wled_auto_calibrate() did already print, no need to repeat this. >> > >> Ok. Will remove this in the next series. >> > > + else >> > > + wled->auto_calib_done = true; >> > > + mutex_unlock(&wled->lock); >> > > + } >> > > + >> > > + return rc; >> > > +} >> > > + >> > > static irqreturn_t qcom_wled_ovp_irq_handler(int irq, void *_wled) >> > > { >> > > struct qcom_wled *wled = _wled; >> > > @@ -319,6 +584,33 @@ static irqreturn_t >> > > qcom_wled_ovp_irq_handler(int irq, void *_wled) >> > > pr_err("WLED OVP fault detected, int_sts=%x fault_sts= %x\n", >> > > int_sts, fault_sts); >> > > >> > > + if (fault_sts & QCOM_WLED_CTRL_OVP_FAULT_BIT) { >> > > + if (wled->cfg.auto_calib_enabled && !wled->auto_calib_done) { >> > > + if (qcom_wled_auto_cal_required(wled)) { >> > >> > So this will be invoked only once, iff we didn't boot with a faulty >> > configuration in which case the qcom_wled_auto_calibrate_at_init() has >> > already done this step and set auto_calib_done. >> > >> > >> > Which also would mean that all logic in this handler, beyond the >> > printouts, are only ever going to be executed zero or one times. >> > >> > Why don't you just do auto-detection during probe (iff the flag is set >> > in DT) and you can remove all this extra logic? >> > >> I think we have seen a issue, where the OVP interrupt is not getting >> set >> some times during the execution of this function at boot. In that case >> the >> auto calibration is >> done bit later. That's why this code is added. >> > > + mutex_lock(&wled->lock); >> > > + if (wled->cfg.ovp_irq > 0 && >> > > + !wled->ovp_irq_disabled) { >> > > + disable_irq_nosync(wled->cfg.ovp_irq); >> > > + wled->ovp_irq_disabled = true; >> > > + } >> > > + >> > > + rc = qcom_wled_auto_calibrate(wled); >> > > + if (rc < 0) >> > > + pr_err("Failed auto-calibration rc=%d\n", >> > > + rc); >> > >> > qcom_wled_auto_calibrate() did already print. >> > >> Ok. I will remove it in the next series. >> > > + else >> > > + wled->auto_calib_done = true; >> > > + >> > > + if (wled->cfg.ovp_irq > 0 && >> > > + wled->ovp_irq_disabled) { >> > > + enable_irq(wled->cfg.ovp_irq); >> > > + wled->ovp_irq_disabled = false; >> > > + } >> > > + mutex_unlock(&wled->lock); >> > > + } >> > > + } >> > > + } >> > > + >> > >> > 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 > -- > 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
| |