[PATCH 1/2] staging: bcm2835-audio: Add support for simultanous HDMI and Headphone audio

Dan Carpenter dan.carpenter at oracle.com
Tue Mar 14 10:54:16 UTC 2017


On Mon, Mar 13, 2017 at 11:45:08PM -0700, Michael Zoran wrote:
> +int snd_bcm2835_new_simple_pcm(struct bcm2835_chip *chip,
> +			       const char *name,
> +			       enum snd_bcm2835_route route,
> +			       u32 numchannels)
> +{
> +	struct snd_pcm *pcm;
> +	int err;
> +
> +	mutex_init(&chip->audio_mutex);
> +	if (mutex_lock_interruptible(&chip->audio_mutex)) {

There is no way no how that a mutex_init() followed by a mutex_lock()
makes sense.

> +		audio_error("Interrupted whilst waiting for lock\n");
> +		return -EINTR;
> +	}
> +	err = snd_pcm_new(chip->card, name, 0, numchannels,
> +			  0, &pcm);
> +	if (err < 0)
> +		goto out;

I hate "out" labels.  Call it goto unlock.  Als it should be "return
ret;" not "return 0;".

> +
> +	pcm->private_data = chip;
> +	strcpy(pcm->name, name);
> +	chip->pcm = pcm;
> +	chip->dest = route;
> +	chip->volume = alsa2chip(0);
> +	chip->mute = CTRL_VOL_UNMUTE;
> +
> +	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
> +			&snd_bcm2835_playback_ops);
> +
> +	snd_pcm_lib_preallocate_pages_for_all(
> +		pcm,
> +		SNDRV_DMA_TYPE_CONTINUOUS,
> +		snd_dma_continuous_data(GFP_KERNEL),
> +		snd_bcm2835_playback_hw.buffer_bytes_max,
> +		snd_bcm2835_playback_hw.buffer_bytes_max);
> +
> +out:
> +	mutex_unlock(&chip->audio_mutex);
> +	return 0;
> +}
> +
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 3a5e528e0ec6..9076de6ae82f 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -21,15 +21,60 @@
>  
>  #include "bcm2835.h"
>  
> -/* HACKY global pointers needed for successive probes to work : ssp
> - * But compared against the changes we will have to do in VC audio_ipc code
> - * to export 8 audio_ipc devices as a single IPC device and then monitor all
> - * four devices in a thread, this gets things done quickly and should be easier
> - * to debug if we run into issues
> - */
> +static void snd_devm_unregister_child(struct device *dev, void *res)
> +{
> +	struct device *childdev = *(struct device **)res;
> +
> +	device_unregister(childdev);
> +}
> +
> +static int snd_devm_add_child(struct device *dev, struct device *child)
> +{
> +	struct device **dr;
> +	int ret;
> +
> +	dr = devres_alloc(snd_devm_unregister_child, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	ret = device_add(child);
> +
> +	if (ret < 0) {

Don't put a blank between the call and the check for failure.  It's
part of the same thing.

Btw, I really wish you had standardize on "if (ret) " instead of
"if (ret < 0) "...  It just makes me itch to think "does this return
positive values?".  I told myself I wouldn't let it bother me but it
does.  No need to change though, that's just an aside.

> +		devres_free(dr);
> +		return ret;
> +	}
> +
> +	*dr = child;
> +	devres_add(dev, dr);
> +
> +	return 0;
> +}
> +
> +static struct device *
> +snd_create_device(struct device *parent,
> +		  struct device_driver *driver,
> +		  const char *name)
> +{
> +	struct device *device;
> +	int ret;
> +
> +	device = devm_kzalloc(parent, sizeof(*device), GFP_KERNEL);
> +
> +	if (IS_ERR(device))
> +		return device;

This should be a NULL check not IS_ERR().

> +
> +	device_initialize(device);
> +	device->parent = parent;
> +	device->driver = driver;
>  

[ snip ]

> +static int snd_add_child_device(struct device *device,
> +				struct bcm2835_audio_driver *audio_driver,
> +				u32 numchans)
> +{
> +	struct snd_card *card;
> +	struct device *child;
> +	struct bcm2835_chip *chip;
> +	int err, i;
> +
> +	child = snd_create_device(device, &audio_driver->driver,
> +				  audio_driver->driver.name);
> +
> +	if (IS_ERR(child)) {
> +		dev_err(device,
> +			"Unable to create child device %p, error %ld",
> +			audio_driver->driver.name,
> +			PTR_ERR(child));
> +		return PTR_ERR(child);
> +	}
> +
> +	card = snd_devm_card_new(child);
> +	if (IS_ERR(card)) {
> +		dev_err(child, "Failed to create card");

The lower levels will print an error if this fails.  I'm not a huge
fan of extra printks.  No one is ever going to see it, and it makes the
code slightly more complicated.

regards,
dan carpenter



More information about the devel mailing list