Message ID | 20191206085432.19962-1-michael.kupfer@fau.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | staging/vc04_services/bcm2835-camera: distinct numeration and names for devices | expand |
Hi Michael, Kay, On 12/6/19 9:54 AM, Michael Kupfer wrote: > Create a static atomic counter for numerating cameras. > Use the Media Subsystem Kernel Internal API to create distinct > device-names, so that the camera-number (given by the counter) > matches the camera-name. > > Co-developed-by: Kay Friedrich <kay.friedrich@fau.de> > Signed-off-by: Kay Friedrich <kay.friedrich@fau.de> > Signed-off-by: Michael Kupfer <michael.kupfer@fau.de> > --- > .../vc04_services/bcm2835-camera/bcm2835-camera.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > index beb6a0063bb8..be5f90a8b49d 100644 > --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > @@ -60,6 +60,9 @@ MODULE_PARM_DESC(max_video_width, "Threshold for video mode"); > module_param(max_video_height, int, 0644); > MODULE_PARM_DESC(max_video_height, "Threshold for video mode"); > > +/* camera instance counter */ > +static atomic_t camera_instance = ATOMIC_INIT(0); > + > /* global device data array */ > static struct bm2835_mmal_dev *gdev[MAX_BCM2835_CAMERAS]; > > @@ -1870,7 +1873,6 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) > > /* v4l2 core mutex used to protect all fops and v4l2 ioctls. */ > mutex_init(&dev->mutex); > - dev->camera_num = camera; > dev->max_width = resolutions[camera][0]; > dev->max_height = resolutions[camera][1]; > > @@ -1886,8 +1888,9 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) > dev->capture.fmt = &formats[3]; /* JPEG */ > > /* v4l device registration */ > - snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), > - "%s", BM2835_MMAL_MODULE_NAME); > + dev->camera_num = v4l2_device_set_name(&dev->v4l2_dev, > + BM2835_MMAL_MODULE_NAME, > + &camera_instance); > ret = v4l2_device_register(NULL, &dev->v4l2_dev); > if (ret) { > dev_err(&pdev->dev, "%s: could not register V4L2 device: %d\n", > Actually, in this specific case I would not use v4l2_device_set_name(). Instead just use: snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s-%u", BM2835_MMAL_MODULE_NAME, camera); It would be even better if there would be just one top-level v4l2_device used for all the camera instances. After all, there really is just one platform device for all of the cameras, and I would expect to see just a single v4l2_device as well. It doesn't hurt to have multiple v4l2_device structs, but it introduces a slight memory overhead since one would have been sufficient. v4l2_device_set_name() is meant for pci-like devices. And it really is a bit overkill to have it as a helper function. Regards, Hans
Hi Hans On Fri, 10 Jan 2020 at 13:25, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > Hi Michael, Kay, > > On 12/6/19 9:54 AM, Michael Kupfer wrote: > > Create a static atomic counter for numerating cameras. > > Use the Media Subsystem Kernel Internal API to create distinct > > device-names, so that the camera-number (given by the counter) > > matches the camera-name. > > > > Co-developed-by: Kay Friedrich <kay.friedrich@fau.de> > > Signed-off-by: Kay Friedrich <kay.friedrich@fau.de> > > Signed-off-by: Michael Kupfer <michael.kupfer@fau.de> > > --- > > .../vc04_services/bcm2835-camera/bcm2835-camera.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > > index beb6a0063bb8..be5f90a8b49d 100644 > > --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > > +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c > > @@ -60,6 +60,9 @@ MODULE_PARM_DESC(max_video_width, "Threshold for video mode"); > > module_param(max_video_height, int, 0644); > > MODULE_PARM_DESC(max_video_height, "Threshold for video mode"); > > > > +/* camera instance counter */ > > +static atomic_t camera_instance = ATOMIC_INIT(0); > > + > > /* global device data array */ > > static struct bm2835_mmal_dev *gdev[MAX_BCM2835_CAMERAS]; > > > > @@ -1870,7 +1873,6 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) > > > > /* v4l2 core mutex used to protect all fops and v4l2 ioctls. */ > > mutex_init(&dev->mutex); > > - dev->camera_num = camera; > > dev->max_width = resolutions[camera][0]; > > dev->max_height = resolutions[camera][1]; > > > > @@ -1886,8 +1888,9 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) > > dev->capture.fmt = &formats[3]; /* JPEG */ > > > > /* v4l device registration */ > > - snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), > > - "%s", BM2835_MMAL_MODULE_NAME); > > + dev->camera_num = v4l2_device_set_name(&dev->v4l2_dev, > > + BM2835_MMAL_MODULE_NAME, > > + &camera_instance); > > ret = v4l2_device_register(NULL, &dev->v4l2_dev); > > if (ret) { > > dev_err(&pdev->dev, "%s: could not register V4L2 device: %d\n", > > > > Actually, in this specific case I would not use v4l2_device_set_name(). > > Instead just use: > > snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), > "%s-%u", BM2835_MMAL_MODULE_NAME, camera); > > It would be even better if there would be just one top-level v4l2_device used > for all the camera instances. After all, there really is just one platform > device for all of the cameras, and I would expect to see just a single > v4l2_device as well. > > It doesn't hurt to have multiple v4l2_device structs, but it introduces a > slight memory overhead since one would have been sufficient. Doesn't that make all controls for all cameras common? The struct v4l2_ctrl_handler is part of struct v4l2_device. Or do we: - ditch the use of ctrl_handler in struct v4l2_device - create and initialise a ctrl_handler per camera on an internal structure so we retain the control state - assign ctrl_handler in struct v4l2_fh to it every time a file handle on the device is opened? And if we only have one struct v4l2_device then is there the possibility of the unique names that Michael and Kay are trying to introduce? I'm a little confused as to whether there really is a gain in having a single v4l2_device. In this case the two cameras are independent devices, even if they are loaded by a single platform driver. Dave > v4l2_device_set_name() is meant for pci-like devices. And it really > is a bit overkill to have it as a helper function. > > Regards, > > Hans
On 1/10/20 3:35 PM, Dave Stevenson wrote: > Hi Hans > > On Fri, 10 Jan 2020 at 13:25, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> Hi Michael, Kay, >> >> On 12/6/19 9:54 AM, Michael Kupfer wrote: >>> Create a static atomic counter for numerating cameras. >>> Use the Media Subsystem Kernel Internal API to create distinct >>> device-names, so that the camera-number (given by the counter) >>> matches the camera-name. >>> >>> Co-developed-by: Kay Friedrich <kay.friedrich@fau.de> >>> Signed-off-by: Kay Friedrich <kay.friedrich@fau.de> >>> Signed-off-by: Michael Kupfer <michael.kupfer@fau.de> >>> --- >>> .../vc04_services/bcm2835-camera/bcm2835-camera.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c >>> index beb6a0063bb8..be5f90a8b49d 100644 >>> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c >>> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c >>> @@ -60,6 +60,9 @@ MODULE_PARM_DESC(max_video_width, "Threshold for video mode"); >>> module_param(max_video_height, int, 0644); >>> MODULE_PARM_DESC(max_video_height, "Threshold for video mode"); >>> >>> +/* camera instance counter */ >>> +static atomic_t camera_instance = ATOMIC_INIT(0); >>> + >>> /* global device data array */ >>> static struct bm2835_mmal_dev *gdev[MAX_BCM2835_CAMERAS]; >>> >>> @@ -1870,7 +1873,6 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) >>> >>> /* v4l2 core mutex used to protect all fops and v4l2 ioctls. */ >>> mutex_init(&dev->mutex); >>> - dev->camera_num = camera; >>> dev->max_width = resolutions[camera][0]; >>> dev->max_height = resolutions[camera][1]; >>> >>> @@ -1886,8 +1888,9 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) >>> dev->capture.fmt = &formats[3]; /* JPEG */ >>> >>> /* v4l device registration */ >>> - snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), >>> - "%s", BM2835_MMAL_MODULE_NAME); >>> + dev->camera_num = v4l2_device_set_name(&dev->v4l2_dev, >>> + BM2835_MMAL_MODULE_NAME, >>> + &camera_instance); >>> ret = v4l2_device_register(NULL, &dev->v4l2_dev); >>> if (ret) { >>> dev_err(&pdev->dev, "%s: could not register V4L2 device: %d\n", >>> >> >> Actually, in this specific case I would not use v4l2_device_set_name(). >> >> Instead just use: >> >> snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), >> "%s-%u", BM2835_MMAL_MODULE_NAME, camera); >> >> It would be even better if there would be just one top-level v4l2_device used >> for all the camera instances. After all, there really is just one platform >> device for all of the cameras, and I would expect to see just a single >> v4l2_device as well. >> >> It doesn't hurt to have multiple v4l2_device structs, but it introduces a >> slight memory overhead since one would have been sufficient. > > Doesn't that make all controls for all cameras common? The struct > v4l2_ctrl_handler is part of struct v4l2_device. > > Or do we: > - ditch the use of ctrl_handler in struct v4l2_device > - create and initialise a ctrl_handler per camera on an internal > structure so we retain the control state > - assign ctrl_handler in struct v4l2_fh to it every time a file handle > on the device is opened? You can set ctrl_handlers at three levels: v4l2_device (common to all video devices), video_device (specific to a single video device), or for v4l2_fh (specific to that filehandle only). In this case you would assign it to the video_device: dev->vdev.ctrl_handler = &dev->ctrl_handler; > > And if we only have one struct v4l2_device then is there the > possibility of the unique names that Michael and Kay are trying to > introduce? > > I'm a little confused as to whether there really is a gain in having a > single v4l2_device. In this case the two cameras are independent > devices, even if they are loaded by a single platform driver. There probably is little to no gain. Hence my proposal to just stick the camera index after the module name. Regards, Hans > > Dave > >> v4l2_device_set_name() is meant for pci-like devices. And it really >> is a bit overkill to have it as a helper function. >> >> Regards, >> >> Hans
diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c index beb6a0063bb8..be5f90a8b49d 100644 --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c @@ -60,6 +60,9 @@ MODULE_PARM_DESC(max_video_width, "Threshold for video mode"); module_param(max_video_height, int, 0644); MODULE_PARM_DESC(max_video_height, "Threshold for video mode"); +/* camera instance counter */ +static atomic_t camera_instance = ATOMIC_INIT(0); + /* global device data array */ static struct bm2835_mmal_dev *gdev[MAX_BCM2835_CAMERAS]; @@ -1870,7 +1873,6 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) /* v4l2 core mutex used to protect all fops and v4l2 ioctls. */ mutex_init(&dev->mutex); - dev->camera_num = camera; dev->max_width = resolutions[camera][0]; dev->max_height = resolutions[camera][1]; @@ -1886,8 +1888,9 @@ static int bcm2835_mmal_probe(struct platform_device *pdev) dev->capture.fmt = &formats[3]; /* JPEG */ /* v4l device registration */ - snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), - "%s", BM2835_MMAL_MODULE_NAME); + dev->camera_num = v4l2_device_set_name(&dev->v4l2_dev, + BM2835_MMAL_MODULE_NAME, + &camera_instance); ret = v4l2_device_register(NULL, &dev->v4l2_dev); if (ret) { dev_err(&pdev->dev, "%s: could not register V4L2 device: %d\n",