[PATCH 01/10] staging: bcm2835-camera: Abort probe if there is no camera

[Stefan Wahren]

Abort the probing of the camera driver in case there isn't a camera
actually connected to the Raspberry Pi. This solution also avoids a
NULL ptr dereference of mmal instance on driver unload.

Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera driver.")
Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index c04bdf0..d6fbef7 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -1841,6 +1841,12 @@ static int bcm2835_mmal_probe(struct platform_device *pdev)
num_cameras = get_num_cameras(instance,
resolutions,
MAX_BCM2835_CAMERAS);
+
+ if (num_cameras < 1) {
+ ret = -ENODEV;
+ goto cleanup_mmal;
+ }
+
if (num_cameras > MAX_BCM2835_CAMERAS)
num_cameras = MAX_BCM2835_CAMERAS;

@@ -1940,6 +1946,9 @@ static int bcm2835_mmal_probe(struct platform_device *pdev)
pr_info("%s: error %d while loading driver\n",
BM2835_MMAL_MODULE_NAME, ret);

+cleanup_mmal:
+ vchiq_mmal_finalise(instance);
+
return ret;
}

[Stefan Wahren]

There is not much value behind this boilerplate, so use
module_platform_driver() instead.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index e14b7c5..86b9210 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -354,25 +354,7 @@ static struct platform_driver bcm2835_alsa_driver = {
.of_match_table = snd_bcm2835_of_match_table,
},
};
-
-static int bcm2835_alsa_device_init(void)
-{
- int retval;
-
- retval = platform_driver_register(&bcm2835_alsa_driver);
- if (retval)
- pr_err("Error registering bcm2835_audio driver %d .\n", retval);
-
- return retval;
-}
-
-static void bcm2835_alsa_device_exit(void)
-{
- platform_driver_unregister(&bcm2835_alsa_driver);
-}
-
-late_initcall(bcm2835_alsa_device_init);
-module_exit(bcm2835_alsa_device_exit);
+module_platform_driver(bcm2835_alsa_driver);

MODULE_AUTHOR("Dom Cobley");
MODULE_DESCRIPTION("Alsa driver for BCM2835 chip");

[Stefan Wahren]

Enable the compilation test for bcm2835-audio to gain more build coverage.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
drivers/staging/vc04_services/bcm2835-audio/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/Kconfig b/drivers/staging/vc04_services/bcm2835-audio/Kconfig
index 9f53653..62c1c8b 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/Kconfig
+++ b/drivers/staging/vc04_services/bcm2835-audio/Kconfig
@@ -1,6 +1,6 @@
config SND_BCM2835
tristate "BCM2835 Audio"
- depends on ARCH_BCM2835 && SND
+ depends on (ARCH_BCM2835 || COMPILE_TEST) && SND
select SND_PCM
select BCM2835_VCHIQ
help

[Stefan Wahren]

Since the camera driver isn't probed via DT, we need to properly setup DMA.

Fixes: 37b7b3087a2f ("staging/vc04_services: Register a platform device for the camera driver.")
Signed-off-by: Stefan Wahren <***@i2se.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 27 +++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 0caee2d..f2c1dcd 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -49,6 +49,7 @@
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/compat.h>
+#include <linux/dma-mapping.h>
#include <soc/bcm2835/raspberrypi-firmware.h>

#include "vchiq_core.h"
@@ -3479,6 +3480,28 @@ static const struct of_device_id vchiq_of_match[] = {
};
MODULE_DEVICE_TABLE(of, vchiq_of_match);

+static struct platform_device *
+vchiq_register_child(struct platform_device *pdev, const char *name)
+{
+ struct platform_device_info pdevinfo;
+ struct platform_device *child;
+
+ memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+ pdevinfo.parent = &pdev->dev;
+ pdevinfo.name = name;
+ pdevinfo.id = PLATFORM_DEVID_NONE;
+ pdevinfo.dma_mask = DMA_BIT_MASK(32);
+
+ child = platform_device_register_full(&pdevinfo);
+ if (IS_ERR(child)) {
+ dev_warn(&pdev->dev, "%s not registered\n", name);
+ child = NULL;
+ }
+
+ return child;
+}
+
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
@@ -3529,9 +3552,7 @@ static int vchiq_probe(struct platform_device *pdev)
VCHIQ_VERSION, VCHIQ_VERSION_MIN,
MAJOR(vchiq_devid), MINOR(vchiq_devid));

- bcm2835_camera = platform_device_register_data(&pdev->dev,
- "bcm2835-camera", -1,
- NULL, 0);
+ bcm2835_camera = vchiq_register_child(pdev, "bcm2835-camera");

return 0;

[Stefan Wahren]

In order to make the module bcm2835-camera load automatically, we need to
add a module alias.

Fixes: 4bebb0312ea9 ("staging/bcm2835-camera: Set ourselves up as a platform driver.")
Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index d6fbef7..7d3222c 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
MODULE_AUTHOR("Vincent Sanders");
MODULE_LICENSE("GPL");
MODULE_VERSION(BM2835_MMAL_VERSION);
+MODULE_ALIAS("platform:bcm2835-camera");

int bcm2835_v4l2_debug;
module_param_named(debug, bcm2835_v4l2_debug, int, 0644);

[Stefan Wahren]

Following Eric's commit 37b7b3087a2f ("staging/vc04_services: Register a
platform device for the camera driver.") this register the audio driver as
a platform device, too.

Signed-off-by: Stefan Wahren <***@i2se.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f2c1dcd..b6cd0ed 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -167,6 +167,7 @@ static VCHIQ_STATE_T g_state;
static struct class *vchiq_class;
static DEFINE_SPINLOCK(msg_queue_spinlock);
static struct platform_device *bcm2835_camera;
+static struct platform_device *bcm2835_audio;

static struct vchiq_drvdata bcm2835_drvdata = {
.cache_line_size = 32,
@@ -3553,6 +3554,7 @@ static int vchiq_probe(struct platform_device *pdev)
MAJOR(vchiq_devid), MINOR(vchiq_devid));

bcm2835_camera = vchiq_register_child(pdev, "bcm2835-camera");
+ bcm2835_audio = vchiq_register_child(pdev, "bcm2835_audio");

return 0;

[Stefan Wahren]

Just like the bcm2835-video make this a platform driver which is probed
by vchiq. In order to change the number of channels use a module
parameter instead, but use the maximum as default.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
.../staging/vc04_services/bcm2835-audio/bcm2835.c | 31 +++++++---------------
1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 86b9210..cf5f80f 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -6,13 +6,13 @@
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/module.h>
-#include <linux/of.h>

#include "bcm2835.h"

static bool enable_hdmi;
static bool enable_headphones;
static bool enable_compat_alsa = true;
+static int num_channels = MAX_SUBSTREAMS;

module_param(enable_hdmi, bool, 0444);
MODULE_PARM_DESC(enable_hdmi, "Enables HDMI virtual audio device");
@@ -21,6 +21,8 @@ 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)");

static void bcm2835_devm_free_vchi_ctx(struct device *dev, void *res)
{
@@ -294,28 +296,19 @@ static int snd_add_child_devices(struct device *device, u32 numchans)
static int snd_bcm2835_alsa_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- u32 numchans;
int err;

- err = of_property_read_u32(dev->of_node, "brcm,pwm-channels",
- &numchans);
- if (err) {
- dev_err(dev, "Failed to get DT property 'brcm,pwm-channels'");
- return err;
- }
-
- if (numchans == 0 || numchans > MAX_SUBSTREAMS) {
- numchans = MAX_SUBSTREAMS;
- dev_warn(dev,
- "Illegal 'brcm,pwm-channels' value, will use %u\n",
- numchans);
+ if (num_channels <= 0 || num_channels > MAX_SUBSTREAMS) {
+ num_channels = MAX_SUBSTREAMS;
+ dev_warn(dev, "Illegal num_channels value, will use %u\n",
+ num_channels);
}

err = bcm2835_devm_add_vchi_ctx(dev);
if (err)
return err;

- err = snd_add_child_devices(dev, numchans);
+ err = snd_add_child_devices(dev, num_channels);
if (err)
return err;

@@ -337,12 +330,6 @@ static int snd_bcm2835_alsa_resume(struct platform_device *pdev)

#endif

-static const struct of_device_id snd_bcm2835_of_match_table[] = {
- { .compatible = "brcm,bcm2835-audio",},
- {},
-};
-MODULE_DEVICE_TABLE(of, snd_bcm2835_of_match_table);
-
static struct platform_driver bcm2835_alsa_driver = {
.probe = snd_bcm2835_alsa_probe,
#ifdef CONFIG_PM
@@ -351,7 +338,6 @@ static struct platform_driver bcm2835_alsa_driver = {
#endif
.driver = {
.name = "bcm2835_audio",
- .of_match_table = snd_bcm2835_of_match_table,
},
};
module_platform_driver(bcm2835_alsa_driver);
@@ -359,3 +345,4 @@ module_platform_driver(bcm2835_alsa_driver);
MODULE_AUTHOR("Dom Cobley");
MODULE_DESCRIPTION("Alsa driver for BCM2835 chip");
MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:bcm2835_audio");

[Stefan Wahren]

In order to have this more consistent between the vc04 services move
the module information to the end of the file.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
.../staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index 7d3222c..cd773eb 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -43,12 +43,6 @@

#define MAX_BCM2835_CAMERAS 2

-MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
-MODULE_AUTHOR("Vincent Sanders");
-MODULE_LICENSE("GPL");
-MODULE_VERSION(BM2835_MMAL_VERSION);
-MODULE_ALIAS("platform:bcm2835-camera");
-
int bcm2835_v4l2_debug;
module_param_named(debug, bcm2835_v4l2_debug, int, 0644);
MODULE_PARM_DESC(bcm2835_v4l2_debug, "Debug level 0-2");
@@ -1976,3 +1970,9 @@ static struct platform_driver bcm2835_camera_driver = {
};

module_platform_driver(bcm2835_camera_driver)
+
+MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
+MODULE_AUTHOR("Vincent Sanders");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(BM2835_MMAL_VERSION);
+MODULE_ALIAS("platform:bcm2835-camera");

[Stefan Wahren]

Currently there is only a catch-all info message which print the
relevant error code without any context. So add more specific error
messages in order to narrow down possible issues.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
.../vc04_services/bcm2835-camera/bcm2835-camera.c | 58 +++++++++++++++-------
1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index cd773eb..611a6ee 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -1539,8 +1539,11 @@ static int mmal_init(struct bm2835_mmal_dev *dev)
struct vchiq_mmal_component *camera;

ret = vchiq_mmal_init(&dev->instance);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: vchiq mmal init failed %d\n",
+ __func__, ret);
return ret;
+ }

/* get the camera component ready */
ret = vchiq_mmal_component_init(dev->instance, "ril.camera",
@@ -1549,7 +1552,9 @@ static int mmal_init(struct bm2835_mmal_dev *dev)
goto unreg_mmal;

camera = dev->component[MMAL_COMPONENT_CAMERA];
- if (camera->outputs < MMAL_CAMERA_PORT_COUNT) {
+ if (camera->outputs < MMAL_CAMERA_PORT_COUNT) {
+ v4l2_err(&dev->v4l2_dev, "%s: too few camera outputs %d needed %d\n",
+ __func__, camera->outputs, MMAL_CAMERA_PORT_COUNT);
ret = -EINVAL;
goto unreg_camera;
}
@@ -1557,8 +1562,11 @@ static int mmal_init(struct bm2835_mmal_dev *dev)
ret = set_camera_parameters(dev->instance,
camera,
dev);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: unable to set camera parameters: %d\n",
+ __func__, ret);
goto unreg_camera;
+ }

/* There was an error in the firmware that meant the camera component
* produced BGR instead of RGB.
@@ -1647,8 +1655,8 @@ static int mmal_init(struct bm2835_mmal_dev *dev)

if (dev->component[MMAL_COMPONENT_PREVIEW]->inputs < 1) {
ret = -EINVAL;
- pr_debug("too few input ports %d needed %d\n",
- dev->component[MMAL_COMPONENT_PREVIEW]->inputs, 1);
+ v4l2_err(&dev->v4l2_dev, "%s: too few input ports %d needed %d\n",
+ __func__, dev->component[MMAL_COMPONENT_PREVIEW]->inputs, 1);
goto unreg_preview;
}

@@ -1661,8 +1669,8 @@ static int mmal_init(struct bm2835_mmal_dev *dev)

if (dev->component[MMAL_COMPONENT_IMAGE_ENCODE]->inputs < 1) {
ret = -EINVAL;
- v4l2_err(&dev->v4l2_dev, "too few input ports %d needed %d\n",
- dev->component[MMAL_COMPONENT_IMAGE_ENCODE]->inputs,
+ v4l2_err(&dev->v4l2_dev, "%s: too few input ports %d needed %d\n",
+ __func__, dev->component[MMAL_COMPONENT_IMAGE_ENCODE]->inputs,
1);
goto unreg_image_encoder;
}
@@ -1676,8 +1684,8 @@ static int mmal_init(struct bm2835_mmal_dev *dev)

if (dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->inputs < 1) {
ret = -EINVAL;
- v4l2_err(&dev->v4l2_dev, "too few input ports %d needed %d\n",
- dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->inputs,
+ v4l2_err(&dev->v4l2_dev, "%s: too few input ports %d needed %d\n",
+ __func__, dev->component[MMAL_COMPONENT_VIDEO_ENCODE]->inputs,
1);
goto unreg_vid_encoder;
}
@@ -1706,8 +1714,11 @@ static int mmal_init(struct bm2835_mmal_dev *dev)
sizeof(enable));
}
ret = bm2835_mmal_set_all_camera_controls(dev);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: failed to set all camera controls: %d\n",
+ __func__, ret);
goto unreg_vid_encoder;
+ }

return 0;

@@ -1873,21 +1884,29 @@ static int bcm2835_mmal_probe(struct platform_device *pdev)
snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
"%s", BM2835_MMAL_MODULE_NAME);
ret = v4l2_device_register(NULL, &dev->v4l2_dev);
- if (ret)
+ if (ret) {
+ dev_err(&pdev->dev, "%s: could not register V4L2 device: %d\n",
+ __func__, ret);
goto free_dev;
+ }

/* setup v4l controls */
ret = bm2835_mmal_init_controls(dev, &dev->ctrl_handler);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: could not init controls: %d\n",
+ __func__, ret);
goto unreg_dev;
+ }
dev->v4l2_dev.ctrl_handler = &dev->ctrl_handler;

/* mmal init */
dev->instance = instance;
ret = mmal_init(dev);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: mmal init failed: %d\n",
+ __func__, ret);
goto unreg_dev;
-
+ }
/* initialize queue */
q = &dev->capture.vb_vidq;
memset(q, 0, sizeof(*q));
@@ -1905,16 +1924,19 @@ static int bcm2835_mmal_probe(struct platform_device *pdev)

/* initialise video devices */
ret = bm2835_mmal_init_device(dev, &dev->vdev);
- if (ret < 0)
+ if (ret < 0) {
+ v4l2_err(&dev->v4l2_dev, "%s: could not init device: %d\n",
+ __func__, ret);
goto unreg_dev;
+ }

/* Really want to call vidioc_s_fmt_vid_cap with the default
* format, but currently the APIs don't join up.
*/
ret = mmal_setup_components(dev, &default_v4l2_format);
if (ret < 0) {
- v4l2_err(&dev->v4l2_dev,
- "%s: could not setup components\n", __func__);
+ v4l2_err(&dev->v4l2_dev, "%s: could not setup components: %d\n",
+ __func__, ret);
goto unreg_dev;
}

@@ -1938,8 +1960,6 @@ static int bcm2835_mmal_probe(struct platform_device *pdev)
bcm2835_cleanup_instance(gdev[i]);
gdev[i] = NULL;
}
- pr_info("%s: error %d while loading driver\n",
- BM2835_MMAL_MODULE_NAME, ret);

cleanup_mmal:
vchiq_mmal_finalise(instance);

[Stefan Wahren]

As per default the GPU memory config of the Raspberry Pi isn't sufficient
for the camera usage. Even worse the bcm2835 camera driver doesn't provide a
helpful error message in this case. So let's add a hint to point the user
to the likely cause.

Signed-off-by: Stefan Wahren <***@i2se.com>
Reviewed-by: Nicolas Saenz Julienne <***@suse.de>
---
drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
index cc2d993..bffd75d 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
@@ -1623,8 +1623,11 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance,
component = &instance->component[instance->component_idx];

ret = create_component(instance, component, name);
- if (ret < 0)
+ if (ret < 0) {
+ pr_err("%s: failed to create component %d (Not enough GPU mem?)\n",
+ __func__, ret);
goto unlock;
+ }

/* ports info needs gathering */
component->control.type = MMAL_PORT_TYPE_CONTROL;

[Dan Carpenter]

This looks really nice.

Reviewed-by: Dan Carpenter <***@oracle.com>

regards,
dan carpenter