Hello Mark, I am looking to address review comments and push v2 of this series (we need it for pixel3 and poco phones' mainline efforts): I have a query on your review comment below: On Thu, 13 Jun 2019 at 22:57, Mark Brown <broonie@xxxxxxxxxx> wrote: > > On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote: > > > +static void labibb_sc_err_recovery_work(void *_labibb) > > +{ > > + int ret; > > + struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb; > > + > > + labibb->ibb_vreg.vreg_enabled = 0; > > + labibb->lab_vreg.vreg_enabled = 0; > > + > > + ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev); > > The driver should *never* enable the regulator itself, it should only do > this if the core told it to. > > > + /* > > + * The SC(short circuit) fault would trigger PBS(Portable Batch > > + * System) to disable regulators for protection. This would > > + * cause the SC_DETECT status being cleared so that it's not > > + * able to get the SC fault status. > > + * Check if LAB/IBB regulators are enabled in the driver but > > + * disabled in hardware, this means a SC fault had happened > > + * and SCP handling is completed by PBS. > > + */ > > Let the core worry about this, the driver should just report the problem > to the core like all other devices do (and this driver doesn't...). I (and Bjorn too) looked to find the api that allows us to do this short circuit reporting and recovery in the core, but couldn't find anything except REGULATOR_ERROR_OVER_CURRENT which also looks like it's used only once in the code. I am sure I'm missing something, maybe you could please help me find it? Best, Sumit.