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

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

[wangweidong.a]

Hi Mark Brown

Thank you for your valuable advice to let me understand the problem.
I will fix this issue in the next patch

Best regards
Weidong Wang

On Mon, Oct 24, 2022 at 10:41:03AM +0800, wangweidong.a@awinic.com wrote:

> > 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.

> The essence of calling mute is not switch dsp, but switch PA. We think 
> that PA has only two states. When no audio stream enters, turn off the 
> PA and turn off the PA's dsp. When the audio stream enters, the PA is 
> turned on, and the dsp is turned on at the same time

> That's not what the mute function is for, the interface is specifically
for muting the stream while power management changes are 
> going on.  What you're describing is power management so should be
controlled via DAPM.
> If your device doesn't have any support for a separate mute function then
it should just not implement anything for that in the 
> driver.

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

[wangweidong.a]

HI: Mark Brown

Thank you very much for your help. I will correct these problems.

Best regards,
Weidong Wang





On Mon, Oct 17, 2022 at 04:09:12PM +0800, wangweidong.a@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.