From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.irobotique.be ([92.243.18.41]:37517 "EHLO perceval.irobotique.be" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755417Ab0GGMxY (ORCPT ); Wed, 7 Jul 2010 08:53:24 -0400 From: Laurent Pinchart To: "Hans Verkuil" Subject: Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support Date: Wed, 7 Jul 2010 14:53:21 +0200 Cc: linux-media@vger.kernel.org, sakari.ailus@maxwell.research.nokia.com References: <1278503608-9126-1-git-send-email-laurent.pinchart@ideasonboard.com> <1278503608-9126-3-git-send-email-laurent.pinchart@ideasonboard.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201007071453.22261.laurent.pinchart@ideasonboard.com> 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. [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. 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