> Regards,
> Bjorn
>
> > > > Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
> > > > ---
> > > > .../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@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
