On Thu, Jan 19, 2023 at 08:16:52AM +0000, Ki-Seok Jo wrote:
> > > > + if (!(sma1303->amp_power_status)) {
> > > > + dev_info(component->dev, "%s : %s\n",
> > > > + __func__, "Already AMP Shutdown");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + cancel_delayed_work_sync(&sma1303->check_fault_work);
> > > > +
> > > > + msleep(55);
> > > That sleep looks odd - what are we delaying after?
> > It need for IC(Amp) issue.
> Right, but what is the issue? It's not clear what event we're delaying for so it's not clear it'll work properly.
This device requires a delay from mute to power off. If it is turned off while the signal is coming out, it takes some time to become muted. (need to about 50ms). So, this delay is required to turn off the power after completely muting. If the time is too long, we can reduce the delay a bit.
> > > > +static void sma1303_check_fault_worker(struct work_struct *work) {
> > > > + struct sma1303_priv *sma1303 =
> > > > + container_of(work, struct sma1303_priv, check_fault_work.work);
> > > > + int ret = 0;
> > > > + unsigned int over_temp, ocp_val, uvlo_val;
> > > > +
> > > > + mutex_lock(&sma1303->lock);
> > > It looks like this mutex is only taken in this function, is it needed?
> > This function is in workqueue. So, I think it can be done at the same time.
> A given work_struct should only be schedulable once.
Oh, I was wrong. I should remove this. After receiving feedback on the previously sent V3 or if there is no problem, I will include this in V4 and send it. Thanks!
