Re: [PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes

[Johan Hovold <johan@kernel.org>]

On Fri, Jan 19, 2024 at 07:37:14AM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 18/01/2024 16:58, Johan Hovold wrote:
> > The UCM configuration for the Lenovo ThinkPad X13s has up until now
> > been setting the speaker PA volume to -3 dB when enabling the speakers,
> > but this does not prevent the user from increasing the volume further.
> > 
> > Limit the PA volume to -3 dB in the machine driver to reduce the risk of
> > speaker damage until we have active speaker protection in place.
> > 
> > Note that this will probably need to be generalised using
> > machine-specific limits, but a common limit should do for now.
> > 
> > Cc: stable@vger.kernel.org	# 6.5
> > Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   sound/soc/qcom/sc8280xp.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> > index ed4bb551bfbb..a19bfa354af8 100644
> > --- a/sound/soc/qcom/sc8280xp.c
> > +++ b/sound/soc/qcom/sc8280xp.c
> > @@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
> >   	case WSA_CODEC_DMA_RX_0:
> >   	case WSA_CODEC_DMA_RX_1:
> >   		/*
> > -		 * set limit of 0dB on Digital Volume for Speakers,
> > -		 * this can prevent damage of speakers to some extent without
> > -		 * active speaker protection
> > +		 * Set limit of 0 dB on Digital Volume and -3 dB on PA Volume
> > +		 * to reduce the risk of speaker damage until we have active
> > +		 * speaker protection in place.
> 
> I would prefer a 0dB here instead of -3dB, this could become issue if we 
> are testing speakers without any pluseaudio or any software 
> amplification. ex: console

I know you want that, but I'm not willing to be the one raising the
default volume that people have been using so far and that you have
(unknowingly) used in your tests to verify that you did not break your
speakers.

Once you've run some more tests we can easily raise this limit.

I just want to make sure we have something safe in place ASAP now that
people will soon be able to change the hardware volume control more
easily (i.e. with the fixed UCM files).

> >   		 */
> >   		snd_soc_limit_volume(card, "WSA_RX0 Digital Volume", 84);
> >   		snd_soc_limit_volume(card, "WSA_RX1 Digital Volume", 84);
> > +		snd_soc_limit_volume(card, "SpkrLeft PA Volume", 1);
> > +		snd_soc_limit_volume(card, "SpkrRight PA Volume", 1)
> 
> It would be nice to consider using component->name_prefix here.

That can possibly be done later.

Johan