Reply to review question [PATCH V2 1/2] ASoc:codes:Add Awinic AW883XX audio amplifier driver

[Mark Brown]

On Mon, Oct 17, 2022 at 04:09:12PM +0800, wangweidong.a at awinic.com wrote:
> Hi: Mark Brown
> 
> Thank you for your suggestion. I will fix the problem you raised in the next
> patch, but there is still a question here and I want to discuss it with you
> 
> This is rather too big to go through in one go so the review here is very
> high level but that's probably a good level for initial review here as there

Please fix your mail client to clearly identify quoted text, as you can
see above it's very dificult for me to tell where you've replied to my
mail.

> > +	if (mute) {
> > +		aw883xx->pstream = AW883XX_STREAM_CLOSE;
> > +		cancel_delayed_work_sync(&aw883xx->start_work);
> > +		mutex_lock(&aw883xx->lock);
> > +		aw883xx_device_stop(aw883xx->aw_pa);
> > +		mutex_unlock(&aw883xx->lock);
> > +	} else {
> > +		aw883xx->pstream = AW883XX_STREAM_OPEN;
> > +		mutex_lock(&aw883xx->lock);
> > +		aw883xx_start(aw883xx, AW_ASYNC_START);
> > +		aw883xx_hold_dsp_spin_st(&aw883xx->aw_pa->spin_desc);
> > +		mutex_unlock(&aw883xx->lock);
> > +	}
> 
> This doesn't look like a mute operation, it looks like it's starting and
> stopping the DSP.
> 
> Answer: This is a mute operation ,aw883xx_device_stop is called in th
> aw883xx_mute function. This function not only executes the mute function
> aw883xx_dev_mute, but also disables dsp and power down. This is for the
> aw883xx chip low power optimization.

Then it's not a mute function, the goal of the mute function is to run
before all the power management code to minimise glitches during power
management.  Just implement the power management via the standard ASoC
power management APIs.

> > +	aw883xx_dev_set_fade_time(ucontrol->value.integer.value[0], true);
> > +
> > +	aw_pr_dbg("step time %ld", ucontrol->value.integer.value[0]);
> > +	return 0;
> > +}
> 
> If a control write changes a value it should return 1, you should run the
> mixer-test selftest which will identify this and a number of other issues.

tools/testing/selftests/alsa

> Answer: Could you tell me what is mixer-test selftest? I have checked other
> drivers, and there is no return 1.

Are you *sure* there's none?  Other drivers being buggy isn't a good
reason to introduce more bugs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20221017/9ba405c2/attachment.sig>