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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
