Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi Mauro,
On Thursday 08 July 2010 15:51:33 Mauro Carvalho Chehab wrote:
> Em 08-07-2010 09:08, Laurent Pinchart escreveu:
> > On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
> >> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> >>> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
[snip]
> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>
> This is a legacy call. Please, don't use it.
> >>>
> >>> What should I use instead then ? I need the functionality of
> >>> video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
> >>> video_ioctl2 shares lots of code with video_usercopy I think
> >>> video_usercopy should stay, and video_ioctl2 should use it.
> >>
> >> The bad thing of video_usercopy() is that it has a "compat" code, to fix
> >> broken definitions of some iotls (see video_fix_command()), and that a
> >> few drivers still use it, instead of video_ioctl2.
> >
> > video_ioctl2 has the same compat code.
>
> Yes, in order to avoid breaking binary compatibility with files compiled
> against the old videodev2.h header that used to declare some ioctls as:
[snip]
> This doesn't make sense for subdev, as old binary-only applications will
> never use the legacy ioctl definitions.
>
> Probably, we can mark this legacy support for removal at
> Documentation/feature-removal-schedule.txt, and remove
> it on a latter version of the kernel. It seems that the old ioctl
> definitions are before 2005 (before 2.6.12):
Good idea.
[snip]
> > What about renaming video_usercopy to __video_usercopy, adding an
> > argument to enable/disable the compat code, create a new video_usercopy
> > that calls __video_usercopy with compat code enabled, have video_ioctl2
> > call __video_usercopy with compat code enabled, and having subdev_ioctl
> > call __video_usercopy with compat code disabled ?
>
> Seems good, but maybe the better is to put the call to video_fix_command()
> outside the common function.
Agreed. It belongs to video_usercopy and video_ioctl2.
> We may add also a printk for the video_usercopy wrapper printing that the
> driver is using a legacy API call, and that this will be removed on a next
> kernel version. Maybe this way, people will finally submit patches porting
> the last remaining drivers to video_ioctl2.
>
> I suspect, however, that we'll end by needing a subdev-specific version of
> __video_usercopy, as we add new ioctls to subdev.
If we need one then I'll create one in v4l2-subdev.c, but as long as we don't
there's little point in duplicating the code.
I've had a look at video_usercopy and video_ioctl2, and the functions are
mostly identical. The differences are
- video_ioctl2 passes the original arg argument to __video_do_ioctl for _IO
ioctls. video_usercopy passes NULL. This seems to be used by the ivtv driver
to handle DVB _IO ioctls (VIDEO_SELECT_SOURCE, AUDIO_SET_MUTE,
AUDIO_CHANNEL_SELECT and AUDIO_BILINGUAL_CHANNEL_SELECT) that pass an integer
through the ioctl argument. Any objection against passing the original
argument to the ioctl handler in video_usercopy as well ? I've quickly checked
the video_usercopy users and none of them seem to check that arg is NULL for
_IO ioctls. This should thus be harmless.
- For argument of known ioctls that have a few input fields at the beginning
of the structure followed by output fields, video_ioctl2 only copies the input
fields from userspace and zeroes out the rest. video_usercopy doesn't. Do we
have video_usercopy drivers that abuse the output fields to pass information
to the driver, or could the video_ioctl2 behaviour be generalized to
video_usercopy ?
If the answer to the two questions is yes, video_usercopy and video_ioctl2
could use the same code. Otherwise they can't, and in that case I should
probably use video_usercopy in v4l2-subdev.c, replacing it with a private copy
when it will disappear.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Em 08-07-2010 09:08, Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
>> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
>>> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> 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.
>>>
>>> [snip]
>>>
> +static int subdev_open(struct file *file)
> +{
> + struct video_device *vdev = video_devdata(file);
> + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +
> + if (!sd->initialized)
> + return -EAGAIN;
Those internal interfaces should not be used on normal
devices/applications, as none of the existing drivers are tested or
supposed to properly work if an external program is touching on its
internal interfaces. So, please add:
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
>>>
>>> As Hans pointed out, subdev device nodes should only be created if the
>>> subdev request it explicitly. I'll fix the patch accordingly. Existing
>>> subdevs will not have a device node by default anymore, so the
>>> CAP_SYS_ADMIN capability won't be required (new subdevs that explicitly
>>> ask for a device node are supposed to handle the calls properly,
>>> otherwise it's a bit pointless :-)).
>>
>> It should be not requested by the subdev, but by the bridge driver (or
>> maybe by both).
>>
>> On several drivers, the bridge is connected to more than one subdev, and
>> some changes need to go to both subdevs, in order to work. As the glue
>> logic is at the bridge driver, creating subdev interfaces will not make
>> sense on those devices.
>
> Agreed. I've added a flag that subdev drivers can set to request creation of
> a
> device node explicitly, and an argument to to v4l2_i2c_new_subdev_board and
> v4l2_spi_new_subdev to overwrite the flag. A device node will only be created
> if the flag is set by the subdev (meaning "I can support device nodes") and
> the flag is not forced to 0 by the bridge driver (meaning "I allow userspace
> to access the subdev directly).
OK.
> [snip]
>
> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
This is a legacy call. Please, don't use it.
>>>
>>> What should I use instead then ? I need the functionality of
>>> video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
>>> video_ioctl2 shares lots of code with video_usercopy I think
>>> video_usercopy should stay, and video_ioctl2 should use it.
>>
>> The bad thing of video_usercopy() is that it has a "compat" code, to fix
>> broken definitions of some iotls (see video_fix_command()), and that a few
>> drivers still use it, instead of video_ioctl2.
>
> video_ioctl2 has the same compat code.
Yes, in order to avoid breaking binary compatibility with files compiled against
the old videodev2.h header that used to declare some ioctls as:
#define VIDIOC_OVERLAY _IOWR('V', 14, int)
#define VIDIOC_S_PARM_IOW('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL_IOW('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO _IOWR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT _IOWR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP _IOR('V', 58, struct v4l2_cropcap)
instead of:
#define VIDIOC_OVERLAY _IOW('V', 14, int)
#define VIDIOC_S_PARM _IOWR('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL _IOWR('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO _IOR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT _IOR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP _IOWR('V', 58, struct v4l2_cropcap)
(basically, the old ioctl's were using the wrong _IO macros, preventing a
generic
copy_from_user/copy_to_user code to work)
This doesn't make sense for subdev, as old binary-only applications will never
use the legacy ioctl definitions.
Probably, we can mark this legacy support for removal at
Documentation/feature-removal-schedule.txt, and remove
it on a latter version of the kernel. It seems that the old ioctl definitions
are
before 2005 (before 2.6.12):
^1da177e (Linus Torvalds2005-04-16 15:20:36 -0700 1461) #define
VIDIOC_OVERLAY_OLD _IOWR ('V', 14, int)
^1da177e (Linus Torvalds2005-04-16 15:20:36 -0700 1462) #define
VIDIOC_S_PARM_OLD _IOW ('V', 22, struct v4l2_streamparm)
^1da177e (Linus Torvalds2005-04-16 15:20:36 -0700 1463) #define
VIDIOC_S_CTRL_OLD _IOW ('V', 28, struct v4l2_control)
^1da177e (Linus Torvalds2005-04-16 15:20:36 -0700 1464) #define
VIDIOC_G_AUDIO_OLD _IOWR ('V', 3
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
> Hi Hans, > > On Wednesday 07 July 2010 15:04:17 Hans Verkuil wrote: >> > On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote: > > [snip] > >> > 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. > > As we've discussed on IRC, the platform data should really store hardware > properties, not software/driver information. Beside, storing the > v4l2_device > pointer in the platform data would be quite difficult, as > v4l2_device_register_subdev only sees a void * pointer for platform_data. > It > has no knowledge of the device-specific structure. I think my main problem is the use of s_config. This op was designed to pass irq and platform data to i2c drivers in pre-2.6.26 kernels. Looking at the current tree I see that the only driver using it is mt9v011.c. I think s_config should be removed and mt9v011.c/em28xx-cards.c be changed to use the platform_data directly. Currently s_config is used there to set the sensor xtal, which seems very much hardware specific to me. Instead of s_config I would probably add a 'registered' op that is called automatically by v4l2_device_register_subdev once the subdev has been registered successfully. Subdev drivers can then execute code that can only be run when registered. Later we might add an 'unregistered' op as well should we need it. This seems much more reasonable to me. Comments? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi Mauro,
On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> > On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> >> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> >>> 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.
> >
> > [snip]
> >
> >>> +static int subdev_open(struct file *file)
> >>> +{
> >>> + struct video_device *vdev = video_devdata(file);
> >>> + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> >>> +
> >>> + if (!sd->initialized)
> >>> + return -EAGAIN;
> >>
> >> Those internal interfaces should not be used on normal
> >> devices/applications, as none of the existing drivers are tested or
> >> supposed to properly work if an external program is touching on its
> >>
> >> internal interfaces. So, please add:
> >>if (!capable(CAP_SYS_ADMIN))
> >>
> >>return -EPERM;
> >
> > As Hans pointed out, subdev device nodes should only be created if the
> > subdev request it explicitly. I'll fix the patch accordingly. Existing
> > subdevs will not have a device node by default anymore, so the
> > CAP_SYS_ADMIN capability won't be required (new subdevs that explicitly
> > ask for a device node are supposed to handle the calls properly,
> > otherwise it's a bit pointless :-)).
>
> It should be not requested by the subdev, but by the bridge driver (or
> maybe by both).
>
> On several drivers, the bridge is connected to more than one subdev, and
> some changes need to go to both subdevs, in order to work. As the glue
> logic is at the bridge driver, creating subdev interfaces will not make
> sense on those devices.
Agreed. I've added a flag that subdev drivers can set to request creation of a
device node explicitly, and an argument to to v4l2_i2c_new_subdev_board and
v4l2_spi_new_subdev to overwrite the flag. A device node will only be created
if the flag is set by the subdev (meaning "I can support device nodes") and
the flag is not forced to 0 by the bridge driver (meaning "I allow userspace
to access the subdev directly).
[snip]
> >>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> >>> + unsigned long arg)
> >>> +{
> >>> + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
> >>
> >> This is a legacy call. Please, don't use it.
> >
> > What should I use instead then ? I need the functionality of
> > video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
> > video_ioctl2 shares lots of code with video_usercopy I think
> > video_usercopy should stay, and video_ioctl2 should use it.
>
> The bad thing of video_usercopy() is that it has a "compat" code, to fix
> broken definitions of some iotls (see video_fix_command()), and that a few
> drivers still use it, instead of video_ioctl2.
video_ioctl2 has the same compat code.
> For sure, we don't need the "compat" code for subdev interface. Also, as
> time goes by, we'll eventually have different needs at the subdev interface,
> as some types of ioctl's may be specific to subdevs and may require
> different copy logic.
We can change the logic then :-)
> IMO, the better is to use the same logic inside the subdev stuff, of course
> removing that "old ioctl" fix logic:
>
> #ifdef __OLD_VIDIOC_
> cmd = video_fix_command(cmd);
> #endif
>
> And replacing the call to:
> err = func(file, cmd, parg);
>
> By the proper subdev handling.
What about renaming video_usercopy to __video_usercopy, adding an argument to
enable/disable the compat code, create a new video_usercopy that calls
__video_usercopy with compat code enabled, have video_ioctl2 call
__video_usercopy with compat code enabled, and having subdev_ioctl call
__video_usercopy with compat code disabled ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi Hans, On Wednesday 07 July 2010 15:04:17 Hans Verkuil wrote: > > On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote: [snip] > > 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. As we've discussed on IRC, the platform data should really store hardware properties, not software/driver information. Beside, storing the v4l2_device pointer in the platform data would be quite difficult, as v4l2_device_register_subdev only sees a void * pointer for platform_data. It has no knowledge of the device-specific structure. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi,
>>v4l2_device *v4l2_dev,
>> if (err && err != -ENOIOCTLCMD) {
>> v4l2_device_unregister_subdev(sd);
>> sd = NULL;
>>+ } else {
>>+ sd->initialized = 1;
>> }
>
>Wouldn't checkpatch.pl script complain about { } on the else part since
>there is only one statement?
>> }
>>
CodingStyle is 100% clear on this:
Do not unnecessarily use braces where a single statement will do.
if (condition)
action();
This does not apply if one branch of a conditional statement is a single
statement. Use braces in both branches.
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thanks for the review.
>
> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
>> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
>>> 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.
>
> [snip]
>
>>> diff --git a/drivers/media/video/v4l2-subdev.c
>>> b/drivers/media/video/v4l2-subdev.c new file mode 100644
>>> index 000..a048161
>>> --- /dev/null
>>> +++ b/drivers/media/video/v4l2-subdev.c
>>> @@ -0,0 +1,65 @@
>
> [snip]
>
>>> +static int subdev_open(struct file *file)
>>> +{
>>> + struct video_device *vdev = video_devdata(file);
>>> + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>>> +
>>> + if (!sd->initialized)
>>> + return -EAGAIN;
>>
>> Those internal interfaces should not be used on normal
>> devices/applications, as none of the existing drivers are tested or
>> supposed to properly work if an external program is touching on its
>> internal interfaces. So, please add:
>>
>> if (!capable(CAP_SYS_ADMIN))
>> return -EPERM;
>
> As Hans pointed out, subdev device nodes should only be created if the subdev
> request it explicitly. I'll fix the patch accordingly. Existing subdevs will
> not have a device node by default anymore, so the CAP_SYS_ADMIN capability
> won't be required (new subdevs that explicitly ask for a device node are
> supposed to handle the calls properly, otherwise it's a bit pointless :-)).
It should be not requested by the subdev, but by the bridge driver (or maybe
by both).
On several drivers, the bridge is connected to more than one subdev, and some
changes need to go to both subdevs, in order to work. As the glue logic is at
the bridge driver, creating subdev interfaces will not make sense on those
devices.
>>> +
>>> + return 0;
>>> +}
>
> [snip]
>
>>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
>>> + unsigned long arg)
>>> +{
>>> + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>>
>> This is a legacy call. Please, don't use it.
>
> What should I use instead then ? I need the functionality of video_usercopy.
> I
> could copy it to v4l2-subdev.c verbatim. As video_ioctl2 shares lots of code
> with video_usercopy I think video_usercopy should stay, and video_ioctl2
> should use it.
The bad thing of video_usercopy() is that it has a "compat" code, to fix broken
definitions of some iotls (see video_fix_command()), and that a few drivers
still
use it, instead of video_ioctl2. For sure, we don't need the "compat" code for
subdev interface. Also, as time goes by, we'll eventually have different needs
at
the subdev interface, as some types of ioctl's may be specific to subdevs and
may
require different copy logic.
IMO, the better is to use the same logic inside the subdev stuff, of course
removing that "old ioctl" fix logic:
#ifdef __OLD_VIDIOC_
cmd = video_fix_command(cmd);
#endif
And replacing the call to:
err = func(file, cmd, parg);
By the proper subdev handling.
>> Also, while the API doc says that only certain ioctls are supported on
>> subdev, there's no code here preventing the usage of invalid ioctls. So,
>> it is possible to do bad things, like changing the video standard format
>> individually on each subdev, creating all sorts of problems.
>
> Invalid (or rather unsupported) ioctls will be routed to the subdev
> core::ioctl operation. Formats will not be changed automagically just because
> a userspace application issues a VIDIOC_S_FMT ioctl.
>
> As the device node creation will need to be requested explicitly this won't
> be
> an issue anyway.
>
Ok. It helps if you add the proper ioctl logic, instead of a stub.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi Mauro,
Thanks for the review.
On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> > 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.
[snip]
> > diff --git a/drivers/media/video/v4l2-subdev.c
> > b/drivers/media/video/v4l2-subdev.c new file mode 100644
> > index 000..a048161
> > --- /dev/null
> > +++ b/drivers/media/video/v4l2-subdev.c
> > @@ -0,0 +1,65 @@
[snip]
> > +static int subdev_open(struct file *file)
> > +{
> > + struct video_device *vdev = video_devdata(file);
> > + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > +
> > + if (!sd->initialized)
> > + return -EAGAIN;
>
> Those internal interfaces should not be used on normal
> devices/applications, as none of the existing drivers are tested or
> supposed to properly work if an external program is touching on its
> internal interfaces. So, please add:
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
As Hans pointed out, subdev device nodes should only be created if the subdev
request it explicitly. I'll fix the patch accordingly. Existing subdevs will
not have a device node by default anymore, so the CAP_SYS_ADMIN capability
won't be required (new subdevs that explicitly ask for a device node are
supposed to handle the calls properly, otherwise it's a bit pointless :-)).
> > +
> > + return 0;
> > +}
[snip]
> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>
> This is a legacy call. Please, don't use it.
What should I use instead then ? I need the functionality of video_usercopy. I
could copy it to v4l2-subdev.c verbatim. As video_ioctl2 shares lots of code
with video_usercopy I think video_usercopy should stay, and video_ioctl2
should use it.
> Also, while the API doc says that only certain ioctls are supported on
> subdev, there's no code here preventing the usage of invalid ioctls. So,
> it is possible to do bad things, like changing the video standard format
> individually on each subdev, creating all sorts of problems.
Invalid (or rather unsupported) ioctls will be routed to the subdev
core::ioctl operation. Formats will not be changed automagically just because
a userspace application issues a VIDIOC_S_FMT ioctl.
As the device node creation will need to be requested explicitly this won't be
an issue anyway.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Em 07-07-2010 11:14, Karicheri, Muralidharan escreveu:
>
>
>> v4l2_device *v4l2_dev,
>> if (err && err != -ENOIOCTLCMD) {
>> v4l2_device_unregister_subdev(sd);
>> sd = NULL;
>> +} else {
>> +sd->initialized = 1;
>> }
>
> Wouldn't checkpatch.pl script complain about { } on the else part since
> there is only one statement?
IMO, it is because it analyzes the entire if clause. As the first part of the
if has two
statements, CodingStyle accepts the usage of braces at the second part
(although this is not a common practice).
>> }
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> 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.
>
> Signed-off-by: Laurent Pinchart
> Signed-off-by: Vimarsh Zutshi
> ---
> drivers/media/video/Makefile |2 +-
> drivers/media/video/v4l2-common.c |3 ++
> drivers/media/video/v4l2-dev.c|5 +++
> drivers/media/video/v4l2-device.c | 27 +++-
> drivers/media/video/v4l2-subdev.c | 65
> +
> include/media/v4l2-dev.h |3 +-
> include/media/v4l2-subdev.h | 10 ++
> 7 files changed, 112 insertions(+), 3 deletions(-)
> create mode 100644 drivers/media/video/v4l2-subdev.c
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index cc93859..c9c07e5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -11,7 +11,7 @@ stkwebcam-objs := stk-webcam.o stk-sensor.o
> omap2cam-objs:= omap24xxcam.o omap24xxcam-dma.o
>
> videodev-objs:= v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
> \
> - v4l2-event.o
> + v4l2-event.o v4l2-subdev.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-common.c
> b/drivers/media/video/v4l2-common.c
> index 4e53b0b..3032aa3 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
>
> /* Register with the v4l2_device which increases the module's
> use count as well. */
> + sd->initialized = 0;
> if (v4l2_device_register_subdev(v4l2_dev, sd))
> sd = NULL;
> /* Decrease the module use count to match the first try_module_get. */
> @@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
> if (err && err != -ENOIOCTLCMD) {
> v4l2_device_unregister_subdev(sd);
> sd = NULL;
> + } else {
> + sd->initialized = 1;
> }
> }
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 0ca7ec9..5a9e9df 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
> * %VFL_TYPE_VBI - Vertical blank data (undecoded)
> *
> * %VFL_TYPE_RADIO - A radio card
> + *
> + * %VFL_TYPE_SUBDEV - A subdevice
> */
> static int __video_register_device(struct video_device *vdev, int type, int
> nr,
> int warn_if_nr_in_use)
> @@ -439,6 +441,9 @@ static int __video_register_device(struct video_device
> *vdev, int type, int nr,
> case VFL_TYPE_RADIO:
> name_base = "radio";
> break;
> + case VFL_TYPE_SUBDEV:
> + name_base = "subdev";
> + break;
> default:
> printk(KERN_ERR "%s called with unknown type: %d\n",
> __func__, type);
> 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
> @@ -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");
> + 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);
> + }
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>
> @@ -135,10 +157,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev
> *sd)
> /* return if it isn't registered */
>
RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Isn't it like there need to be {} for both "if" and "else" when
there is more than one line in either block?
Regards,
--
Sylwester Nawrocki
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Karicheri, Muralidharan
> Sent: Wednesday, July 07, 2010 4:15 PM
> To: Laurent Pinchart; linux-media@vger.kernel.org
> Cc: sakari.ai...@maxwell.research.nokia.com
> Subject: RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support
>
>
>
> >v4l2_device *v4l2_dev,
> > if (err && err != -ENOIOCTLCMD) {
> > v4l2_device_unregister_subdev(sd);
> > sd = NULL;
> >+} else {
> >+sd->initialized = 1;
> > }
>
> Wouldn't checkpatch.pl script complain about { } on the else part since
> there is only one statement?
> > }
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support
>v4l2_device *v4l2_dev,
> if (err && err != -ENOIOCTLCMD) {
> v4l2_device_unregister_subdev(sd);
> sd = NULL;
>+ } else {
>+ sd->initialized = 1;
> }
Wouldn't checkpatch.pl script complain about { } on the else part since
there is only one statement?
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
Hi Hans, On Wednesday 07 July 2010 14:43:08 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. [snip] > I'm missing one thing here: in this code the subdev device node is always > registered. But for most subdev drivers there is no need for a device > node. This should really be explicitly turned on by the subdev driver > itself. I'll fix that with a subdev flag. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
> 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 000..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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
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 000..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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
> 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.
>
> Signed-off-by: Laurent Pinchart
> Signed-off-by: Vimarsh Zutshi
> ---
> drivers/media/video/Makefile |2 +-
> drivers/media/video/v4l2-common.c |3 ++
> drivers/media/video/v4l2-dev.c|5 +++
> drivers/media/video/v4l2-device.c | 27 +++-
> drivers/media/video/v4l2-subdev.c | 65
> +
> include/media/v4l2-dev.h |3 +-
> include/media/v4l2-subdev.h | 10 ++
> 7 files changed, 112 insertions(+), 3 deletions(-)
...
> 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
> @@ -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");
> + 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);
> + }
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>
I'm missing one thing here: in this code the subdev device node is always
registered. But for most subdev drivers there is no need for a device
node. This should really be explicitly turned on by the subdev driver
itself.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support
> 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.
>
> Signed-off-by: Laurent Pinchart
> Signed-off-by: Vimarsh Zutshi
> ---
> drivers/media/video/Makefile |2 +-
> drivers/media/video/v4l2-common.c |3 ++
> drivers/media/video/v4l2-dev.c|5 +++
> drivers/media/video/v4l2-device.c | 27 +++-
> drivers/media/video/v4l2-subdev.c | 65
> +
> include/media/v4l2-dev.h |3 +-
> include/media/v4l2-subdev.h | 10 ++
> 7 files changed, 112 insertions(+), 3 deletions(-)
> create mode 100644 drivers/media/video/v4l2-subdev.c
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index cc93859..c9c07e5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -11,7 +11,7 @@ stkwebcam-objs := stk-webcam.o stk-sensor.o
> omap2cam-objs:= omap24xxcam.o omap24xxcam-dma.o
>
> videodev-objs:= v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
> \
> - v4l2-event.o
> + v4l2-event.o v4l2-subdev.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-common.c
> b/drivers/media/video/v4l2-common.c
> index 4e53b0b..3032aa3 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
>
> /* Register with the v4l2_device which increases the module's
> use count as well. */
> + sd->initialized = 0;
> if (v4l2_device_register_subdev(v4l2_dev, sd))
> sd = NULL;
> /* Decrease the module use count to match the first try_module_get. */
> @@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
> if (err && err != -ENOIOCTLCMD) {
> v4l2_device_unregister_subdev(sd);
> sd = NULL;
> + } else {
> + sd->initialized = 1;
> }
> }
>
> diff --git a/drivers/media/video/v4l2-dev.c
> b/drivers/media/video/v4l2-dev.c
> index 0ca7ec9..5a9e9df 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
> * %VFL_TYPE_VBI - Vertical blank data (undecoded)
> *
> * %VFL_TYPE_RADIO - A radio card
> + *
> + * %VFL_TYPE_SUBDEV - A subdevice
> */
> static int __video_register_device(struct video_device *vdev, int type,
> int nr,
> int warn_if_nr_in_use)
> @@ -439,6 +441,9 @@ static int __video_register_device(struct video_device
> *vdev, int type, int nr,
> case VFL_TYPE_RADIO:
> name_base = "radio";
> break;
> + case VFL_TYPE_SUBDEV:
> + name_base = "subdev";
> + break;
> default:
> printk(KERN_ERR "%s called with unknown type: %d\n",
> __func__, type);
> 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
> @@ -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);
> + 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);
> +
[RFC/PATCH 2/6] v4l: subdev: Add device node support
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.
Signed-off-by: Laurent Pinchart
Signed-off-by: Vimarsh Zutshi
---
drivers/media/video/Makefile |2 +-
drivers/media/video/v4l2-common.c |3 ++
drivers/media/video/v4l2-dev.c|5 +++
drivers/media/video/v4l2-device.c | 27 +++-
drivers/media/video/v4l2-subdev.c | 65 +
include/media/v4l2-dev.h |3 +-
include/media/v4l2-subdev.h | 10 ++
7 files changed, 112 insertions(+), 3 deletions(-)
create mode 100644 drivers/media/video/v4l2-subdev.c
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index cc93859..c9c07e5 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -11,7 +11,7 @@ stkwebcam-objs:= stk-webcam.o stk-sensor.o
omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o
videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
- v4l2-event.o
+ v4l2-event.o v4l2-subdev.o
# V4L2 core modules
diff --git a/drivers/media/video/v4l2-common.c
b/drivers/media/video/v4l2-common.c
index 4e53b0b..3032aa3 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
v4l2_device *v4l2_dev,
/* Register with the v4l2_device which increases the module's
use count as well. */
+ sd->initialized = 0;
if (v4l2_device_register_subdev(v4l2_dev, sd))
sd = NULL;
/* Decrease the module use count to match the first try_module_get. */
@@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
v4l2_device *v4l2_dev,
if (err && err != -ENOIOCTLCMD) {
v4l2_device_unregister_subdev(sd);
sd = NULL;
+ } else {
+ sd->initialized = 1;
}
}
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7ec9..5a9e9df 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
* %VFL_TYPE_VBI - Vertical blank data (undecoded)
*
* %VFL_TYPE_RADIO - A radio card
+ *
+ * %VFL_TYPE_SUBDEV - A subdevice
*/
static int __video_register_device(struct video_device *vdev, int type, int nr,
int warn_if_nr_in_use)
@@ -439,6 +441,9 @@ static int __video_register_device(struct video_device
*vdev, int type, int nr,
case VFL_TYPE_RADIO:
name_base = "radio";
break;
+ case VFL_TYPE_SUBDEV:
+ name_base = "subdev";
+ break;
default:
printk(KERN_ERR "%s called with unknown type: %d\n",
__func__, type);
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
@@ -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");
+ 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);
+ }
+
+ return ret;
}
EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
@@ -135,10 +157,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
/* return if it isn't registered */
if (sd == NULL || sd->v4l2_dev == NULL)
return;
+
spin_lock(&sd->v4l2_dev->lock);
list_del(&sd->list);
spin_unlock(&sd->v4l2_dev->lock);
sd->v4l2_dev = NULL;
