Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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