Quoting Taniya Das (2018-04-09 01:41:46) > @@ -64,18 +69,43 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en) > return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val); > } > > +static int gdsc_is_enabled_by_poll_cfg_reg(struct gdsc *sc, bool en) > +{ > + u32 val; > + int ret; > + > + ret = regmap_read(sc->regmap, sc->gdscr + CFG_GDSCR_OFFSET, &val); > + if (ret) > + return ret; > + > + if (en) > + return !!(val & GDSC_POWER_UP_COMPLETE); > + > + return !(val & GDSC_POWER_DOWN_COMPLETE); > +} > + > static int gdsc_poll_status(struct gdsc *sc, unsigned int reg, bool en) > { > ktime_t start; > > start = ktime_get(); > do { > - if (gdsc_is_enabled(sc, reg) == en) > - return 0; > + if (sc->flags & POLL_CFG_GDSCR) { > + if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en) > + return 0; > + } else { > + if (gdsc_is_enabled(sc, reg) == en) > + return 0; Ok, I thought you would bury the is_enabled_by_poll_cfg_reg() stuff into gdsc_is_enabled() directly. Because that function is used in one more place than here, in gdsc_init(), to figure out if a GDSC is on at boot time. The sc->gds_hw_ctrl check should be able to get buried into gdsc_is_enabled() as well so that the function doesn't have to take a register at all. > + } > } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US); > > - if (gdsc_is_enabled(sc, reg) == en) > - return 0; > + if (sc->flags & POLL_CFG_GDSCR) { > + if (gdsc_is_enabled_by_poll_cfg_reg(sc, en) == en) > + return 0; > + } else { > + if (gdsc_is_enabled(sc, reg) == en) > + return 0; > + } And then this duplicate diff wouldn't happen either. > > return -ETIMEDOUT; > } > @@ -254,6 +284,7 @@ static int gdsc_disable(struct generic_pm_domain *domain) > udelay(1); > > reg = sc->gds_hw_ctrl ? sc->gds_hw_ctrl : sc->gdscr; > + This change shouldn't be here? > ret = gdsc_poll_status(sc, reg, true); > if (ret) > return ret; -- To unsubscribe from this list: send the line "unsubscribe linux-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html