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