[PATCH/RFC] V4L core cleanups

* [PATCH/RFC] V4L core cleanups
@ 2009-11-18  0:38 Laurent Pinchart
  2009-11-18  0:38 ` v4l: Add video_device_node_name function Laurent Pinchart
                   ` (10 more replies)
  0 siblings, 11 replies; 27+ messages in thread
From: Laurent Pinchart @ 2009-11-18  0:38 UTC (permalink / raw)
  To: linux-media; +Cc: hverkuil, mchehab, sakari.ailus

Hi everybody,

this patch sets attemp to clean up the V4L core to remove the
video_device::minor and video_device::num references in most drivers.

There are two reasons for this. The first one is that drivers really
shouldn't care about those fields, especially the minor number. This
doesn't mean a driver can't request a specific device number, that
remains a perfectly valid use case, but most use cases of those fields
after device registration shouldn't be needed.

The second reason is that most drivers use those fields in bogus ways,
making it obvious they shouldn't have cared about them in the first
place :-) We've had a video_drvdata function for a long time, but many
drivers still have their own private minor -> data mapping lists for
historical reasons. That code is error prone and completely unneeded.

So this patch sets tries to clean up the V4L core by porting drivers to
the most "recent" APIs (which are actually quite old) and introducing a
new helper function.

The first two patches add and use the video_device_node_name function.
The function returns a const pointer to the video device name. On
systems using udev, the name is passed as a hint to udev and will likely
become the /dev device node name, unless overwritten by udev rules (I've
heard that some distributions put the V4L device nodes in /dev/v4l).
Some drivers erroneously created the name from the video_device::minor
field instead of video_device::num, which is fixed by the second patch.

This is an example video_device_node_name usage typical from what can be
found in the second patch.

-       printk(KERN_INFO "bttv%d: registered device radio%d\n",
-              btv->c.nr, btv->radio_dev->num);
+       printk(KERN_INFO "bttv%d: registered device %s\n",
+              btv->c.nr, video_device_node_name(btv->radio_dev));

The third patch removes left video_device::num usage from the drivers.
The field was used to create information strings that shouldn't include
the device node name (such as video_device::name) or that should be
created using a stable identifier (such as i2c_adapter::name).

The fourth, fifth and sixth patches replace video_is_unregistered with
video_is_registered and use the new function in device drivers. As
explained in the fourth patch commit message, the rationale behind that
is to have video_is_registered return false when called on an
initialized but not yet registered video_device instance. The function
can be used instead of checking video_device::minor manually, making it
less error-prone as drivers don't need to make sure they
video_device::minor to -1 correctly for all error paths.

A typical use case is

-       if (-1 != dev->radio_dev->minor)
+       if (video_is_registered(dev->radio_dev))
                video_unregister_device(dev->radio_dev);
        else
                video_device_release(dev->radio_dev);

The seventh patch replace local minor to data lists by video_drvdata().
The function has been there for a long time but wasn't used by many
drivers, probably because they were written before it was available, or,
for some of them, because they were written based on drivers that were
not using it. This patch removes lots of identical unneeded code blocks,
making the result less bug-prone.

The eight patch removes now unneeded video_device::minor assignments to
-1, as the previous patches made them unneeded.

The last patch removes a few more video_device::minor users. As
explained in the patch description, the field was used either to

- test for error conditions that can't happen anymore with the current
  v4l-dvb core,
- store the value in a driver private field that isn't used anymore,
- check the video device type where video_device::vfl_type should be
  used, or
- create the name of a kernel thread that should get a stable name.

There are still two video_device::num users and those can easily be
removed. Hans Verkuil is working on a patch, as one of the drivers is
the ivtv driver and the other one is based on the same code.

There are also still a few video_device::minor users. One of them is
the pvrusb2 driver that creates sysfs attributes storing the minor
numbers of the device nodes created by the driver. I'm not sure what to
do about that one. All the others are V4L1 drivers that need the minor
number for the VIDIOCGUNIT ioctl. Hopefully that will die when the
drivers will be ported to V4L2 :-)

I've split the patches into core and device patches to make them easier
to apply on my work trees. I'll merge the core and device code together
when submitting a pull request to avoid bisection errors.

I'll send a pull request after receiving (and incorporating) your
comments, or in a few days if there's no comments.

Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 27+ messages in thread