Date:   Wed, 8 Dec 2021 20:40:54 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
Cc:     alsa-devel@alsa-project.org, Vijendar.Mukunda@amd.com,
        Alexander.Deucher@amd.com, Basavaraj.Hiregoudar@amd.com,
        Sunil-kumar.Dommati@amd.com, Liam Girdwood <lgirdwood@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: max98357a: Add mixer control to mute/unmute speaker
Message-ID: <YbEYVq+uvIcoxqko@sirena.org.uk>
References: <20211208185850.1555996-1-AjitKumar.Pandey@amd.com>
 <YbETxcwa83U8WXTO@sirena.org.uk>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="vxKfQpHFiM/DHo+x"
Content-Disposition: inline
In-Reply-To: <YbETxcwa83U8WXTO@sirena.org.uk>
Precedence: bulk


--vxKfQpHFiM/DHo+x
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 08, 2021 at 08:21:31PM +0000, Mark Brown wrote:
> On Thu, Dec 09, 2021 at 12:28:49AM +0530, Ajit Kumar Pandey wrote:
>=20
> > +static int speaker_mute_put(struct snd_kcontrol *kcontrol,
> > +			    struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component =3D snd_soc_kcontrol_component(kc=
ontrol);
> > +	struct max98357a_priv *max98357a =3D snd_soc_component_get_drvdata(co=
mponent);
> > +	int mode =3D ucontrol->value.enumerated.item[0];
> > +
> > +	max98357a->sdmode_switch =3D mode;
> > +	gpiod_set_value_cansleep(max98357a->sdmode, mode);
> > +	dev_dbg(component->dev, "set sdmode to %d", mode);
>=20
> This looks like it should just be a DAPM widget - it's just a generic
> GPIO control, there's no connection with the CODEC that I can see so it
> definitely shouldn't be in the CODEC driver.  Often trivial stuff like
> this is done in the machine driver, though the simple-amplifier driver
> is probably a good fit here.

Actually now I look again the only control interface this driver has is
GPIOs but it does expose a digital interface with constraints as input
so doesn't fit within simple-amplifier.  However this is just powering
off the amplifier to achieve mute rather than a separate mute control so
it'd probably be better to use a SND_SOC_DAPM_PIN_SWITCH() for the
Speaker widget to do this, this would also end up addressing Jaroslav's
thing with the naming as a side effect.  Sorry about the confusion there.

--vxKfQpHFiM/DHo+x
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmGxGFUACgkQJNaLcl1U
h9D/aQf/S3ZOBoeRlFrMlyiqHO6GldTqaps0Jb0ZtH6Cq6Zxle1FBwo+UMSq+lgK
zUa5D/Xfn0xptu0aRx+ylL9kuA6Nj//6bhOaLuLiAguPKB0KPfZFs3IwUKLc1oJ9
D+DkVfurjSOJE6bbz/YzPqVUiMkv6aT8ZpFqcyN8juTBqOoY2xoETmTfH0vM71lD
zYgZuXClqlocT5RijlKQ31rdAnyWxuOSCm7Q/MCgOJAUiR//z90wVurKzgUnie2i
W7E5Q3VJQIT5NWhNea56eWOLCXIxMq5QZArSpCIT0ycjiIsOXmLbE1REU+UK2SEV
+kr5IxEznvYKyl93LgUJiw/SCdMqxA==
=Gh2o
-----END PGP SIGNATURE-----

--vxKfQpHFiM/DHo+x--