From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp-vbr4.xs4all.nl ([194.109.24.24]:1914 "EHLO smtp-vbr4.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753592Ab0GGNEV (ORCPT ); Wed, 7 Jul 2010 09:04:21 -0400 Message-ID: <72c0021199a577840a434d9195cb9e61.squirrel@webmail.xs4all.nl> In-Reply-To: <201007071453.22261.laurent.pinchart@ideasonboard.com> References: <1278503608-9126-1-git-send-email-laurent.pinchart@ideasonboard.com> <1278503608-9126-3-git-send-email-laurent.pinchart@ideasonboard.com> <201007071453.22261.laurent.pinchart@ideasonboard.com> Date: Wed, 7 Jul 2010 15:04:17 +0200 Subject: Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support From: "Hans Verkuil" To: "Laurent Pinchart" Cc: linux-media@vger.kernel.org, sakari.ailus@maxwell.research.nokia.com MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT Sender: linux-media-owner@vger.kernel.org List-ID: > Hi Hans, > > Thanks for the quick review. > > On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote: >> > Create a device node named subdevX for every registered subdev. >> > >> > As the device node is registered before the subdev core::s_config >> > function is called, return -EGAIN on open until initialization >> > completes. >> >> The only reason we have s_config is for old i2c drivers that need to be >> supported in pre-2.6.26 kernels in the mercurial repository. >> >> I'm thinking that we should get rid of that legacy support in the git >> tree. It hurts my eyes every time I see that code. >> >> Not a blocker for this patch series, but if others agree that we should >> get rid of the legacy support then I can work on that. > > Some (most ?) I2C sensors need to be accessed during probe. This involves > powering the sensor up, which is handled by a board code function called > through a platform data function pointer. > > On the OMAP3 ISP the sensor clock can be generated by the ISP. This means > that > board code needs to call an ISP (exported) function to turn the clock on. > To > do so we currently retrieve a pointer to the ISP device through the > subdev's > parent v4l2_device, embedded in the ISP device structure. This requires > the > subdev to be registered, otherwise its parent will be NULL. For that > reason > we're still using the core::s_config operation. > > This is obviously not an ideal situation, and I'm open to suggestions on > how > to solve it without core::s_config. One possibility would be to use a > global > ISP device pointer in the board code, as there's only one ISP device in > the > system. It feels a bit hackish though. Or make the v4l2_device pointer part of the platform data? That way the subdev driver has access to the v4l2_device pointer in a fairly clean manner. > > [snip] > >> > diff --git a/drivers/media/video/v4l2-device.c >> > b/drivers/media/video/v4l2-device.c >> > index 5a7dc4a..685fa82 100644 >> > --- a/drivers/media/video/v4l2-device.c >> > +++ b/drivers/media/video/v4l2-device.c > > [snip] > >> > @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister); >> > >> > int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, >> > >> > struct v4l2_subdev *sd) >> > >> > { >> > >> > + struct video_device *vdev; >> > + int ret; >> > + >> > /* Check for valid input */ >> > if (v4l2_dev == NULL || sd == NULL || !sd->name[0]) >> > return -EINVAL; >> > + >> > /* Warn if we apparently re-register a subdev */ >> > WARN_ON(sd->v4l2_dev != NULL); >> > + >> > if (!try_module_get(sd->owner)) >> > return -ENODEV; >> > + >> > sd->v4l2_dev = v4l2_dev; >> > spin_lock(&v4l2_dev->lock); >> > list_add_tail(&sd->list, &v4l2_dev->subdevs); >> > spin_unlock(&v4l2_dev->lock); >> > >> > - return 0; >> > + >> > + /* Register the device node. */ >> > + vdev = &sd->devnode; >> > + snprintf(vdev->name, sizeof(vdev->name), "subdev"); >> >> Hmm, perhaps we should be more creative here. For example: >> >> snprintf(vdev->name, sizeof(vdev->name), "subdev %s", sd->name); > > I'm definitely open to alternative name suggestions. For instance, I'm not > sure to be happy with /dev/subdevX. /dev/v4l2-subdevX might be better. I think I would go with v4l-subdevX. I agree, that's better than the overly generic 'subdevX'. > > As for vdev->name, your suggestion sounds good. > >> > + vdev->parent = v4l2_dev->dev; >> > + vdev->fops = &v4l2_subdev_fops; >> > + vdev->release = video_device_release_empty; >> > + ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1); >> > + if (ret < 0) { >> > + spin_lock(&v4l2_dev->lock); >> > + list_del(&sd->list); >> > + spin_unlock(&v4l2_dev->lock); >> > + sd->v4l2_dev = NULL; >> > + module_put(sd->owner); >> >> You can just call v4l2_device_unregister_subdev(sd) here. The call >> to video_unregister_device will know that the registration failed and >> will >> just return. > > Good point, thanks. > >> > + } >> > + >> > + return ret; >> > >> > } >> > EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); >> > > > [snip] > >> > diff --git a/drivers/media/video/v4l2-subdev.c >> > b/drivers/media/video/v4l2-subdev.c >> > new file mode 100644 >> > index 0000000..a048161 >> > --- /dev/null >> > +++ b/drivers/media/video/v4l2-subdev.c >> > @@ -0,0 +1,65 @@ >> > +/* >> > + * V4L2 subdevice support. >> > + * >> > + * Copyright (C) 2009 Laurent Pinchart >> >> 2009 -> 2010 >> >> Might be wrong elsewhere as well. > > I'll fix that. > > -- > Regards, > > Laurent Pinchart > Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco