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

[Dan Carpenter]

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