Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 12 Apr 2022 16:58:22 +0200
Message-ID: <s5hr1628i29.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Stefan Wahren <stefan.wahren@i2se.com>
Cc:     Adrien Thierry <athierry@redhat.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        bcm-kernel-feedback-list@broadcom.com,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Takashi Iwai <tiwai@suse.com>,
        Maxime Ripard <mripard@kernel.org>,
        linux-staging@lists.linux.dev,
        linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: bcm2835-audio: remove compat ALSA card
In-Reply-To: <69de07fb-efdb-5296-f48b-e3af2ac5ef49@i2se.com>
References: <20220408150359.26661-1-athierry@redhat.com>
        <69de07fb-efdb-5296-f48b-e3af2ac5ef49@i2se.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 08 Apr 2022 23:00:52 +0200,
Stefan Wahren wrote:
> 
> Hi Adrien,
> 
> [ add Maxime and Takashi ]
> 
> Am 08.04.22 um 17:03 schrieb Adrien Thierry:
> > Remove compat ALSA card, which has overlapping functionality with the
> > two other cards described by the driver (HDMI and headphones)
> >
> > This handles TODO item "Revisit multi-cards options and PCM route mixer
> > control".
> >
> > Move the S/PDIF device that was part of the compat ALSA card to the HDMI
> > card.
> >
> > Only enable headphones card by default, because HDMI breaks when using
> > both vc4 and bcm2835-audio with HDMI card enabled.
> >
> > Signed-off-by: Adrien Thierry <athierry@redhat.com>
> > ---
> >
> > I was able to test PCM output on 3.5mm and HDMI on a Raspberry Pi 4.
> > However, I couldn't test the HDMI S/PDIF device since I don't own a S/PDIF
> > capable speaker.
> 
> thanks for taking care of this issue. Unfortunately i cannot give a
> technical review to this patch. So hoping Maxime or Takashi can do ...

I have no longer RPi around me, so cannot actually test, but the code
changes (mostly dropping the superfluous part) looks OK.

The concern is, though, about the compatibility with the already
running setup.  But vc04 is still in staging, and that can be an
excuse for the possible breakage, I suppose.


thanks,

Takashi

> 
> Best regards
> 
> >
> >   .../vc04_services/bcm2835-audio/bcm2835-ctl.c | 86 ++-----------------
> >   .../vc04_services/bcm2835-audio/bcm2835.c     | 33 ++-----
> >   2 files changed, 15 insertions(+), 104 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
> > index 3703409715da..1c1f040122d7 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
> > @@ -117,15 +117,6 @@ static const struct snd_kcontrol_new snd_bcm2835_ctl[] = {
> >   		.get = snd_bcm2835_ctl_get,
> >   		.put = snd_bcm2835_ctl_put,
> >   	},
> > -	{
> > -		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > -		.name = "PCM Playback Route",
> > -		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
> > -		.private_value = PCM_PLAYBACK_DEVICE,
> > -		.info = snd_bcm2835_ctl_info,
> > -		.get = snd_bcm2835_ctl_get,
> > -		.put = snd_bcm2835_ctl_put,
> > -	},
> >   };
> >     static int snd_bcm2835_spdif_default_info(struct snd_kcontrol
> > *kcontrol,
> > @@ -220,7 +211,14 @@ static int create_ctls(struct bcm2835_chip *chip, size_t size,
> >   	return 0;
> >   }
> >   -int snd_bcm2835_new_ctl(struct bcm2835_chip *chip)
> > +int snd_bcm2835_new_headphones_ctl(struct bcm2835_chip *chip)
> > +{
> > +	strscpy(chip->card->mixername, "Broadcom Mixer", sizeof(chip->card->mixername));
> > +	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_ctl),
> > +			   snd_bcm2835_ctl);
> > +}
> > +
> > +int snd_bcm2835_new_hdmi_ctl(struct bcm2835_chip *chip)
> >   {
> >   	int err;
> >   @@ -232,71 +230,3 @@ int snd_bcm2835_new_ctl(struct bcm2835_chip
> > *chip)
> >   			   snd_bcm2835_spdif);
> >   }
> >   -static const struct snd_kcontrol_new snd_bcm2835_headphones_ctl[]
> > = {
> > -	{
> > -		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > -		.name = "Headphone Playback Volume",
> > -		.index = 0,
> > -		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
> > -			  SNDRV_CTL_ELEM_ACCESS_TLV_READ,
> > -		.private_value = PCM_PLAYBACK_VOLUME,
> > -		.info = snd_bcm2835_ctl_info,
> > -		.get = snd_bcm2835_ctl_get,
> > -		.put = snd_bcm2835_ctl_put,
> > -		.count = 1,
> > -		.tlv = {.p = snd_bcm2835_db_scale}
> > -	},
> > -	{
> > -		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > -		.name = "Headphone Playback Switch",
> > -		.index = 0,
> > -		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
> > -		.private_value = PCM_PLAYBACK_MUTE,
> > -		.info = snd_bcm2835_ctl_info,
> > -		.get = snd_bcm2835_ctl_get,
> > -		.put = snd_bcm2835_ctl_put,
> > -		.count = 1,
> > -	}
> > -};
> > -
> > -int snd_bcm2835_new_headphones_ctl(struct bcm2835_chip *chip)
> > -{
> > -	strscpy(chip->card->mixername, "Broadcom Mixer", sizeof(chip->card->mixername));
> > -	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_headphones_ctl),
> > -			   snd_bcm2835_headphones_ctl);
> > -}
> > -
> > -static const struct snd_kcontrol_new snd_bcm2835_hdmi[] = {
> > -	{
> > -		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > -		.name = "HDMI Playback Volume",
> > -		.index = 0,
> > -		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE |
> > -			  SNDRV_CTL_ELEM_ACCESS_TLV_READ,
> > -		.private_value = PCM_PLAYBACK_VOLUME,
> > -		.info = snd_bcm2835_ctl_info,
> > -		.get = snd_bcm2835_ctl_get,
> > -		.put = snd_bcm2835_ctl_put,
> > -		.count = 1,
> > -		.tlv = {.p = snd_bcm2835_db_scale}
> > -	},
> > -	{
> > -		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > -		.name = "HDMI Playback Switch",
> > -		.index = 0,
> > -		.access = SNDRV_CTL_ELEM_ACCESS_READWRITE,
> > -		.private_value = PCM_PLAYBACK_MUTE,
> > -		.info = snd_bcm2835_ctl_info,
> > -		.get = snd_bcm2835_ctl_get,
> > -		.put = snd_bcm2835_ctl_put,
> > -		.count = 1,
> > -	}
> > -};
> > -
> > -int snd_bcm2835_new_hdmi_ctl(struct bcm2835_chip *chip)
> > -{
> > -	strscpy(chip->card->mixername, "Broadcom Mixer", sizeof(chip->card->mixername));
> > -	return create_ctls(chip, ARRAY_SIZE(snd_bcm2835_hdmi),
> > -			   snd_bcm2835_hdmi);
> > -}
> > -
> > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> > index 628732d7bf6a..00bc898b0189 100644
> > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> > @@ -10,17 +10,13 @@
> >   #include "bcm2835.h"
> >     static bool enable_hdmi;
> > -static bool enable_headphones;
> > -static bool enable_compat_alsa = true;
> > +static bool enable_headphones = true;
> >   static int num_channels = MAX_SUBSTREAMS;
> >     module_param(enable_hdmi, bool, 0444);
> >   MODULE_PARM_DESC(enable_hdmi, "Enables HDMI virtual audio device");
> >   module_param(enable_headphones, bool, 0444);
> >   MODULE_PARM_DESC(enable_headphones, "Enables Headphones virtual audio device");
> > -module_param(enable_compat_alsa, bool, 0444);
> > -MODULE_PARM_DESC(enable_compat_alsa,
> > -		 "Enables ALSA compatibility virtual audio device");
> >   module_param(num_channels, int, 0644);
> >   MODULE_PARM_DESC(num_channels, "Number of audio channels (default: 8)");
> >   @@ -63,19 +59,20 @@ struct bcm2835_audio_driver {
> >   	enum snd_bcm2835_route route;
> >   };
> >   -static int bcm2835_audio_alsa_newpcm(struct bcm2835_chip *chip,
> > +static int bcm2835_audio_dual_newpcm(struct bcm2835_chip *chip,
> >   				     const char *name,
> >   				     enum snd_bcm2835_route route,
> >   				     u32 numchannels)
> >   {
> >   	int err;
> >   -	err = snd_bcm2835_new_pcm(chip, "bcm2835 ALSA", 0,
> > AUDIO_DEST_AUTO,
> > -				  numchannels - 1, false);
> > +	err = snd_bcm2835_new_pcm(chip, name, 0, route,
> > +				  numchannels, false);
> > +
> >   	if (err)
> >   		return err;
> >   -	err = snd_bcm2835_new_pcm(chip, "bcm2835 IEC958/HDMI", 1, 0,
> > 1, true);
> > +	err = snd_bcm2835_new_pcm(chip, "IEC958", 1, route, 1, true);
> >   	if (err)
> >   		return err;
> >   @@ -90,18 +87,6 @@ static int bcm2835_audio_simple_newpcm(struct
> > bcm2835_chip *chip,
> >   	return snd_bcm2835_new_pcm(chip, name, 0, route, numchannels, false);
> >   }
> >   -static struct bcm2835_audio_driver bcm2835_audio_alsa = {
> > -	.driver = {
> > -		.name = "bcm2835_alsa",
> > -		.owner = THIS_MODULE,
> > -	},
> > -	.shortname = "bcm2835 ALSA",
> > -	.longname  = "bcm2835 ALSA",
> > -	.minchannels = 2,
> > -	.newpcm = bcm2835_audio_alsa_newpcm,
> > -	.newctl = snd_bcm2835_new_ctl,
> > -};
> > -
> >   static struct bcm2835_audio_driver bcm2835_audio_hdmi = {
> >   	.driver = {
> >   		.name = "bcm2835_hdmi",
> > @@ -110,7 +95,7 @@ static struct bcm2835_audio_driver bcm2835_audio_hdmi = {
> >   	.shortname = "bcm2835 HDMI",
> >   	.longname  = "bcm2835 HDMI",
> >   	.minchannels = 1,
> > -	.newpcm = bcm2835_audio_simple_newpcm,
> > +	.newpcm = bcm2835_audio_dual_newpcm,
> >   	.newctl = snd_bcm2835_new_hdmi_ctl,
> >   	.route = AUDIO_DEST_HDMI
> >   };
> > @@ -134,10 +119,6 @@ struct bcm2835_audio_drivers {
> >   };
> >     static struct bcm2835_audio_drivers children_devices[] = {
> > -	{
> > -		.audio_driver = &bcm2835_audio_alsa,
> > -		.is_enabled = &enable_compat_alsa,
> > -	},
> >   	{
> >   		.audio_driver = &bcm2835_audio_hdmi,
> >   		.is_enabled = &enable_hdmi,
> >
> > base-commit: 1831fed559732b132aef0ea8261ac77e73f7eadf
>