> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
