Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Hans Verkuil]

On Thursday 12 March 2009 02:40:09 Patch from Sri Deevi wrote:
> The patch number 10954 was added via Mauro Carvalho Chehab
> <mchehab@redhat.com> to http://linuxtv.org/hg/v4l-dvb master development
> tree.
>
> Kernel patches in this development tree may be modified to be backward
> compatible with older kernels. Compatibility modifications will be
> removed before inclusion into the mainstream Kernel
>
> If anyone has any objections, please let us know by sending a message to:
> 	Linux Media Mailing List <linux-media@vger.kernel.org>

Mauro,

What the hell??!

Since when does a big addition like this get merged without undergoing a 
public review?

I've been working my ass off converting drivers to the new i2c API and 
v4l2_subdev structures and here you merge a big driver that uses old-style 
(which will lead to 'deprecated' warnings when compiling with 2.6.29, BTW), 
where the driver writes directly to i2c modules instead of adding a proper 
i2c module for them. And what are 'colibri', 'flatrion' and 'hammerhead' 
anyway? Are they integrated devices of the cx231xx? Can they be used 
separately in other products as well?

So yes, I have objections. At the minimum it should be converted first to 
use v4l2_device/v4l2_subdev and I need more information on the new i2c 
devices so I can tell whether the code for those should be split off into 
separate i2c modules. Not to mention that I want to have the time to review 
this code more closely.

Sorry Sri, this isn't your fault.

Regards,

	Hans

>
> ------
>
> From: Sri Deevi  <Srinivasa.Deevi@conexant.com>
> Add cx231xx USB driver
>
>
> Signed-off-by: Srinivasa Deevi <srinivasa.deevi@conexant.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
>
> ---
>
>  linux/drivers/media/video/Kconfig                    |    2
>  linux/drivers/media/video/Makefile                   |    1
>  linux/drivers/media/video/cx231xx/Kconfig            |   35
>  linux/drivers/media/video/cx231xx/Makefile           |   15
>  linux/drivers/media/video/cx231xx/cx231xx-audio.c    |  664 ++
>  linux/drivers/media/video/cx231xx/cx231xx-avcore.c   | 2289 ++++++++++
>  linux/drivers/media/video/cx231xx/cx231xx-cards.c    |  947 ++++
>  linux/drivers/media/video/cx231xx/cx231xx-conf-reg.h |  491 ++
>  linux/drivers/media/video/cx231xx/cx231xx-core.c     | 1197 +++++
>  linux/drivers/media/video/cx231xx/cx231xx-dvb.c      |  566 ++
>  linux/drivers/media/video/cx231xx/cx231xx-i2c.c      |  580 ++
>  linux/drivers/media/video/cx231xx/cx231xx-input.c    |  267 +
>  linux/drivers/media/video/cx231xx/cx231xx-reg.h      | 1574 +++++++
>  linux/drivers/media/video/cx231xx/cx231xx-vbi.c      |  697 +++
>  linux/drivers/media/video/cx231xx/cx231xx-vbi.h      |   61
>  linux/drivers/media/video/cx231xx/cx231xx-video.c    | 2440 +++++++++++
>  linux/drivers/media/video/cx231xx/cx231xx.h          |  771 +++
>  linux/include/linux/i2c-id.h                         |    1
>  18 files changed, 12598 insertions(+)
>
> <diff discarded since it is too big>
>
> ---
>
> Patch is available at:
> http://linuxtv.org/hg/v4l-dvb/rev/1d836224ecbf5ce6cf60696b4590630a92a4587
>5
>
> _______________________________________________
> linuxtv-commits mailing list
> linuxtv-commits@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Mauro Carvalho Chehab]

On Thu, 12 Mar 2009 08:38:59 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Mauro,
> 
> What the hell??!
> 
> Since when does a big addition like this get merged without undergoing a 
> public review?
> 
> I've been working my ass off converting drivers to the new i2c API and 
> v4l2_subdev structures and here you merge a big driver that uses old-style 
> (which will lead to 'deprecated' warnings when compiling with 2.6.29, BTW), 
> where the driver writes directly to i2c modules instead of adding a proper 
> i2c module for them. And what are 'colibri', 'flatrion' and 'hammerhead' 
> anyway? Are they integrated devices of the cx231xx? Can they be used 
> separately in other products as well?
> 
> So yes, I have objections. At the minimum it should be converted first to 
> use v4l2_device/v4l2_subdev and I need more information on the new i2c 
> devices so I can tell whether the code for those should be split off into 
> separate i2c modules. Not to mention that I want to have the time to review 
> this code more closely.
> 
> Sorry Sri, this isn't your fault.

Hans,

I know that you're rushing to convert all drivers to the new model, but you
should give time to time. Even with Kernel's fast development cycle, we should
take care to not depreciate things faster than developers can track (btw,
lwn.net already complained that V4L subsystem changes their APIs faster than
usual).

First of all, except for ivtv drivers, the first conversion to the new model
occurred just few weeks ago. The new model will bring some gains, but this
shouldn't stop the merge of the drivers whose development started before we
port the drivers used as example by the developer.

This is a new model, and we should give people some time to adapt to it. This
is the way we worked in the past and it is the way we should keep working. For
example, video_ioctl2 were added several Kernel releases before merging uvc
driver. Yet, we've accepted uvc driver without using the new model, because its
development that occurred before video_ioctl2.

The second point is that there's nothing at
Documentation/feature-removal-schedule.txt informing that those stuff is
deprecated.

So, we should still accept new drivers without the conversion at least until
the end of 2.6.30 window, and add some notice at feature-removal-schedule.txt
on 2.6.30 clearly stating what's deprecated and when, before generating penalty
over the developers that are using the current drivers as their model of
development.

In the specific case of cx231xx driver, I've explained to Sri, there are still
some issues to be fixed at the driver. Although the driver works, It is not
ready for 2.6.30 yet. 

However, keeping this on a separate tree will just create more mess (according
with Sri, he already had to rebase this driver 4 times during 2.6.29-rc cycle,
due to the high speed of internal API changes).

Since his driver seems to be based on em28xx, he had no sample on how to convert it to
v4l2_device/v4l2_subdev/new_i2c model.

After committing Devin's Austek patches (also seemed to be based on em28xx), it will
probably be easier for Uri to convert his driver to the new approach.

-- 

Cheers,
Mauro

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Mauro Carvalho Chehab]

Hans,

I've summarized your comments into a few answers, to avoid repeating myself.

On Fri, 13 Mar 2009 01:53:42 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Hi Mauro,
> 
> Just one last reply, and than we can close this discussion. Luckily the 
> conversion of this driver to v4l2_device/subdev is a lot simpler than I 
> feared initially. So no harm done in that respect.

Good!

> Please announce this in the future. I checked the linux-media list and there 
> is no mention of this whatsoever. Just putting it up in a tree is not 
> sufficient, you have to tell people about it as well.

I always request the developers to submit their work at the public ML.
Unfortunately, a few don't do it due to some random reasons.

It is much better for the Linux users if I merge such drivers or patches than
to just nack a driver due to that reason.

Anyway, after the merge, the community can still review the driver and/or the
patches, as announced by the hg mailbomb script. 

> It's not v4l2_device/v4l2_subdev that's at stake here. It's the removal of 
> the old i2c autoprobing behavior. v4l2_device/v4l2_subdev is just the 
> fastest way to do this for v4l drivers. As you can see here:
> 
> http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted
> 
> the conversion is almost done. This weekend I hope to finish cx88, cx23885 
> and bttv. Hopefully Douglas Landgraf can convert em28xx and I know Mike 
> Isely only needs to do some final tweaks for pvrusb2.

Good to know. We still need testing from the users for those drivers, since
there are risks of regressions when converting from the automatic probe method,
to a manual binding one. If a i2c bind for a needed driver is forgot (or bad coded),
some board variants will break.

Since nobody ever cared to document all those I2C driver alternatives, I
suspect that we'll have some regressions, that will likely be identified only
when the kernel reaches the distros.

> I do not expect newly submitted drivers to use v4l2_device/subdev. I know it 
> is a very recent development. But it is very easy to implement and all I 
> need is a chance to review and help them with that to ensure that dropping 
> the old i2c API isn't blocked by a new driver. Not in the least because it 
> is likely that the i2c core maintainer will block such a new driver if it 
> is the only driver preventing the removal of the old i2c API.
> 
> In addition, once a driver is in the v4l-dvb tree I cannot just drop the old 
> i2c API support in v4l-dvb since that will just break any driver that uses 
> it. So whether it goes to the git tree or not, that doesn't matter for me.

This will just mean that those drivers won't compile against 2.6.30 during
2.6.30-rc cycle. You can always disable it via make menuconfig if needed, until
it would be converted to the new i2c methods.

Since just the core developers compile kernels against the latest -rc cycle,
This is for sure a much minor problem that can be easily solved by a .config
file, than letting a driver to grow out of the tree.

-- 

Cheers,
Mauro

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Hans Verkuil]

Hi Mauro,

Just one last reply, and than we can close this discussion. Luckily the 
conversion of this driver to v4l2_device/subdev is a lot simpler than I 
feared initially. So no harm done in that respect.

On Thursday 12 March 2009 14:20:11 Mauro Carvalho Chehab wrote:
> On Thu, 12 Mar 2009 12:14:22 +0100 (CET)
>
> "Hans Verkuil" <hverkuil@xs4all.nl> wrote:
> > Mauro, you did not answer the question why this driver was just merged
> > without going through a public review? If I'd seen it beforehand I'd
> > have worked together with Sri to get it fixed first. I don't expect him
> > to know about this, but I didn't even get a chance to discuss it and
> > help with it. Everyone else has to go through the normal review
> > channels, but apparently this was just fast-tracked and merged. That's
> > not the way to do it.
>
> It were added one week ago on a temporary public tree, at linuxtv.

Please announce this in the future. I checked the linux-media list and there 
is no mention of this whatsoever. Just putting it up in a tree is not 
sufficient, you have to tell people about it as well.

> > Please back out this driver, put it in a separate tree and let me 1)
> > review this driver first, and 2) help Sri implementing the
> > v4l2_device/v4l2_subdev stuff.
>
> It is better to review it at the tree. I won't merge it upstream until
> the remaining bugs would be fixed. Until then, it will wait on my staging
> -git tree (the pending tree).
>
> > > First of all, except for ivtv drivers, the first conversion to the
> > > new model
> > > occurred just few weeks ago. The new model will bring some gains, but
> > > this shouldn't stop the merge of the drivers whose development
> > > started before we
> > > port the drivers used as example by the developer.
> > >
> > > This is a new model, and we should give people some time to adapt to
> > > it. This
> > > is the way we worked in the past and it is the way we should keep
> > > working.
> >
> > It's not a new model.
>
> It is. The first changeset were committed on Nov, 30, and the last
> internal API changes, according with the docs, happened on Feb, 14. So,
> if we don't touch on it, the first stable version of the framework will
> be available upstream on 2.6.30.
>
> If we keep it stable during 2.6.30, convert the drivers merged on 2.6.30
> to the new framework, and mark the legacy approach as a feature to be
> removed on a patch applied at 2.6.30, then we can remove the previous
> support for 2.6.31.

It's not v4l2_device/v4l2_subdev that's at stake here. It's the removal of 
the old i2c autoprobing behavior. v4l2_device/v4l2_subdev is just the 
fastest way to do this for v4l drivers. As you can see here:

http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted

the conversion is almost done. This weekend I hope to finish cx88, cx23885 
and bttv. Hopefully Douglas Landgraf can convert em28xx and I know Mike 
Isely only needs to do some final tweaks for pvrusb2.

I do not expect newly submitted drivers to use v4l2_device/subdev. I know it 
is a very recent development. But it is very easy to implement and all I 
need is a chance to review and help them with that to ensure that dropping 
the old i2c API isn't blocked by a new driver. Not in the least because it 
is likely that the i2c core maintainer will block such a new driver if it 
is the only driver preventing the removal of the old i2c API.

In addition, once a driver is in the v4l-dvb tree I cannot just drop the old 
i2c API support in v4l-dvb since that will just break any driver that uses 
it. So whether it goes to the git tree or not, that doesn't matter for me.

> $ hg log linux/Documentation/video4linux/v4l2-framework.txt
>
> changeset:   10648:e471b963bef6
> parent:      10640:4f6c3f9efa58
> parent:      10647:63256532f5a7
> user:        Mauro Carvalho Chehab <mchehab@redhat.com>
> date:        Tue Feb 17 23:44:45 2009 -0300
> summary:     merge: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb
>
> changeset:   10644:44b5df81ab02
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Sat Feb 14 16:00:53 2009 +0100
> summary:     v4l2-subdev: rename dev field to v4l2_dev
>
> changeset:   10643:9eb2f6220a18
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Sat Feb 14 15:54:23 2009 +0100
> summary:     v4l2-device: allow a NULL parent device when registering.
>
> changeset:   10573:b73e7bdad8c4
> user:        Mauro Carvalho Chehab <mchehab@redhat.com>
> date:        Mon Feb 16 15:54:29 2009 -0300
> summary:     v4l2-framework.txt: Whitespace clenups
>
> changeset:   10571:12a10f808bfd
> user:        Mauro Carvalho Chehab <mchehab@redhat.com>
> date:        Sat Feb 14 08:51:28 2009 -0200
> summary:     v4l2-framework.txt: Fixes the videobuf init functions
>
> changeset:   10570:6f4cff0e7f16
> user:        Mauro Carvalho Chehab <mchehab@redhat.com>
> date:        Sat Feb 14 08:29:07 2009 -0200
> summary:     v4l2-framework: documments videobuf usage on drivers
>
> changeset:   10489:c84416787a43
> user:        Mauro Carvalho Chehab <mchehab@redhat.com>
> date:        Sat Feb 07 11:07:04 2009 +0100
> summary:     doc: use consistent naming conventions for vdev and
> v4l2_dev.
>
> changeset:   10252:09cabe4f0c63
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Thu Jan 15 10:09:05 2009 +0100
> summary:     v4l2 doc: explain why v4l2_device_unregister_subdev() has to
> be called.
>
> changeset:   10141:4cc8ed11e2e0
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Tue Dec 30 11:14:19 2008 +0100
> summary:     v4l2: debugging API changed to match against driver name
> instead of ID.
>
> changeset:   10136:ffe112f306a3
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Tue Dec 23 17:42:25 2008 +0100
> summary:     v4l2 doc: update v4l2-framework.txt
>
> changeset:   10134:a11cf6774c04
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Tue Dec 23 16:17:23 2008 +0100
> summary:     v4l2 doc: set v4l2_dev instead of parent.
>
> changeset:   10133:f03ab4ab3f87
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Mon Dec 22 13:13:11 2008 +0100
> summary:     v4l2-framework: use correct comment style.
>
> changeset:   9943:2e680d8a3b2f
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Fri Dec 19 14:20:22 2008 +0100
> summary:     v4l2: document video_device.
>
> changeset:   9820:5611723c9512
> parent:      9767:7100e78482d7
> user:        Hans Verkuil <hverkuil@xs4all.nl>
> date:        Sun Nov 30 01:36:58 2008 +0100
> summary:     v4l2: add v4l2_device and v4l2_subdev structs to the v4l2
> framework.
>
> > The I2C core changes went in in 2.6.22.
>
> I'm not referring to i2c core changes, but to v4l2 dev/subdev stuff.

That's all there to implement the i2c core changes. Yes, it will do a lot 
more in the future, but currently its primary purpose is to switch to the 
new i2c API.

> > And please note that the use of the old API isn't the only question I
> > have, there are more oddities with the i2c handling that I'd like to
> > have more information about. Writing i2c registers directly from the
> > adapter driver doesn't look good to me at first sight.
>
> Yes, I'm aware of the I2C GPIO and similar stuff. This is one of the
> reasons why this shouldn't be merged upstream yet.

Well, these are integral i2c devices, so I'm OK with it. Although I didn't 
have time to do a full review, so I may have missed some issues.

> > > The second point is that there's nothing at
> > > Documentation/feature-removal-schedule.txt informing that those stuff
> > > is deprecated.
> >
> > Yes it is, see this from the 2.6.29 kernel:
> >
> > What:   i2c_attach_client(), i2c_detach_client(),
> > i2c_driver->detach_client() When:   2.6.29 (ideally) or 2.6.30 (more
> > likely)
> > Why:    Deprecated by the new (standard) device driver binding model.
> > Use i2c_driver->probe() and ->remove() instead.
> > Who:    Jean Delvare <khali@linux-fr.org>
>
> I'm not referring to the i2c changes, but to the v4l2 framework ones.
>
> Anyway, checkpatch should be generating warning about this, at least on
> my environment. In this case, no warnings are generated.
>
> Hmm... there's no "check" field on its entry.
>
> Jean should have added a line like:
> Check: i2c_attach_client i2c_detach_client i2c_driver->detach_client
>
> to let checkpatch.pl to do his job.
>
> > > Since his driver seems to be based on em28xx, he had no sample on how
> > > to convert it to
> > > v4l2_device/v4l2_subdev/new_i2c model.
> >
> > Again, if I'd known about it I'd be happy to help with it. Why didn't
> > you put me in contact with him? You know I'm spending a lot of time on
> > this.
>
> Why don't you do it, instead of complaining? It is not upstream, and I
> won't move it upstream yet, as I said before.

Of course I'll do it. The point remains that nobody but you got the 
opportunity to review the code before it went into v4l-dvb. The normal 
procedure is to put it up in a tree and ask for a review first.

> Btw, there's another driver that will likely be sent to me on the next
> few days. I'm not sure if it is using the new model or not. I haven't
> seen the code yet, so I'm not sure what functions it is being used. I
> suspect that it is still using the old model, since its development
> started before months ago, before the v4l2 dev/subdev conversion of the
> drivers.

Just put it up for review and I'll look at it and help the developer fix 
anything in this area.

> > > After committing Devin's Austek patches (also seemed to be based on
> > > em28xx), it will
> > > probably be easier for Uri to convert his driver to the new approach.
> >
> > That depends. As I said, there are other i2c issues that need to be
> > clarified, but I *never* got the chance to ask him since you just
> > merged this driver without the customary public review.
>
> You have the chance right now.
>
> That's why we have the staging v4l/dvb -hg tree: to give everybody a view
> of what's is planned. Also, a public announcement is done automatically
> by hg mailbomb scripts, that sends all committed patches to the patch
> announcement ML.

No, since if you merge a driver using e.g. a deprecated API then that will 
prevent it from being removed in v4l-dvb until you have fixed the driver, 
unless you accept that that driver is marked broken in v4l-dvb. Large 
changes or new drivers might have complications for other works in progress 
that you are not aware of, and so it is important that other developers can 
review it first. I don't know everything that is happening with v4l-dvb, 
and neither do you. But if the community can take a look at new code, then 
the chances are that all together we do know (almost) everything and can 
catch potential problems in time.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Mauro Carvalho Chehab]

On Thu, 12 Mar 2009 12:14:22 +0100 (CET)
"Hans Verkuil" <hverkuil@xs4all.nl> wrote:

> Mauro, you did not answer the question why this driver was just merged
> without going through a public review? If I'd seen it beforehand I'd have
> worked together with Sri to get it fixed first. I don't expect him to know
> about this, but I didn't even get a chance to discuss it and help with it.
> Everyone else has to go through the normal review channels, but apparently
> this was just fast-tracked and merged. That's not the way to do it.

It were added one week ago on a temporary public tree, at linuxtv.

> Please back out this driver, put it in a separate tree and let me 1)
> review this driver first, and 2) help Sri implementing the
> v4l2_device/v4l2_subdev stuff.

It is better to review it at the tree. I won't merge it upstream until the
remaining bugs would be fixed. Until then, it will wait on my staging -git tree
(the pending tree).

> > First of all, except for ivtv drivers, the first conversion to the new
> > model
> > occurred just few weeks ago. The new model will bring some gains, but this
> > shouldn't stop the merge of the drivers whose development started before
> > we
> > port the drivers used as example by the developer.
> >
> > This is a new model, and we should give people some time to adapt to it.
> > This
> > is the way we worked in the past and it is the way we should keep working.
> 
> It's not a new model.

It is. The first changeset were committed on Nov, 30, and the last internal API
changes, according with the docs, happened on Feb, 14. So, if we don't touch on
it, the first stable version of the framework will be available upstream on
2.6.30. 

If we keep it stable during 2.6.30, convert the drivers merged on 2.6.30 to the
new framework, and mark the legacy approach as a feature to be removed on a
patch applied at 2.6.30, then we can remove the previous support for 2.6.31.

$ hg log linux/Documentation/video4linux/v4l2-framework.txt 

changeset:   10648:e471b963bef6
parent:      10640:4f6c3f9efa58
parent:      10647:63256532f5a7
user:        Mauro Carvalho Chehab <mchehab@redhat.com>
date:        Tue Feb 17 23:44:45 2009 -0300
summary:     merge: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb

changeset:   10644:44b5df81ab02
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Sat Feb 14 16:00:53 2009 +0100
summary:     v4l2-subdev: rename dev field to v4l2_dev

changeset:   10643:9eb2f6220a18
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Sat Feb 14 15:54:23 2009 +0100
summary:     v4l2-device: allow a NULL parent device when registering.

changeset:   10573:b73e7bdad8c4
user:        Mauro Carvalho Chehab <mchehab@redhat.com>
date:        Mon Feb 16 15:54:29 2009 -0300
summary:     v4l2-framework.txt: Whitespace clenups

changeset:   10571:12a10f808bfd
user:        Mauro Carvalho Chehab <mchehab@redhat.com>
date:        Sat Feb 14 08:51:28 2009 -0200
summary:     v4l2-framework.txt: Fixes the videobuf init functions

changeset:   10570:6f4cff0e7f16
user:        Mauro Carvalho Chehab <mchehab@redhat.com>
date:        Sat Feb 14 08:29:07 2009 -0200
summary:     v4l2-framework: documments videobuf usage on drivers

changeset:   10489:c84416787a43
user:        Mauro Carvalho Chehab <mchehab@redhat.com>
date:        Sat Feb 07 11:07:04 2009 +0100
summary:     doc: use consistent naming conventions for vdev and v4l2_dev.

changeset:   10252:09cabe4f0c63
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Thu Jan 15 10:09:05 2009 +0100
summary:     v4l2 doc: explain why v4l2_device_unregister_subdev() has to be called.

changeset:   10141:4cc8ed11e2e0
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Tue Dec 30 11:14:19 2008 +0100
summary:     v4l2: debugging API changed to match against driver name instead of ID.

changeset:   10136:ffe112f306a3
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Tue Dec 23 17:42:25 2008 +0100
summary:     v4l2 doc: update v4l2-framework.txt

changeset:   10134:a11cf6774c04
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Tue Dec 23 16:17:23 2008 +0100
summary:     v4l2 doc: set v4l2_dev instead of parent.

changeset:   10133:f03ab4ab3f87
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Mon Dec 22 13:13:11 2008 +0100
summary:     v4l2-framework: use correct comment style.

changeset:   9943:2e680d8a3b2f
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Fri Dec 19 14:20:22 2008 +0100
summary:     v4l2: document video_device.

changeset:   9820:5611723c9512
parent:      9767:7100e78482d7
user:        Hans Verkuil <hverkuil@xs4all.nl>
date:        Sun Nov 30 01:36:58 2008 +0100
summary:     v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.

> The I2C core changes went in in 2.6.22.

I'm not referring to i2c core changes, but to v4l2 dev/subdev stuff.

> And please note that the use of the old API isn't the only question I
> have, there are more oddities with the i2c handling that I'd like to have
> more information about. Writing i2c registers directly from the adapter
> driver doesn't look good to me at first sight.

Yes, I'm aware of the I2C GPIO and similar stuff. This is one of the reasons
why this shouldn't be merged upstream yet.

> > The second point is that there's nothing at
> > Documentation/feature-removal-schedule.txt informing that those stuff is
> > deprecated.
> 
> Yes it is, see this from the 2.6.29 kernel:
> 
> What:   i2c_attach_client(), i2c_detach_client(), i2c_driver->detach_client()
> When:   2.6.29 (ideally) or 2.6.30 (more likely)
> Why:    Deprecated by the new (standard) device driver binding model. Use
>         i2c_driver->probe() and ->remove() instead.
> Who:    Jean Delvare <khali@linux-fr.org>

I'm not referring to the i2c changes, but to the v4l2 framework ones.

Anyway, checkpatch should be generating warning about this, at least on my
environment. In this case, no warnings are generated.

Hmm... there's no "check" field on its entry. 

Jean should have added a line like:
Check: i2c_attach_client i2c_detach_client i2c_driver->detach_client

to let checkpatch.pl to do his job.

> > Since his driver seems to be based on em28xx, he had no sample on how to
> > convert it to
> > v4l2_device/v4l2_subdev/new_i2c model.
> 
> Again, if I'd known about it I'd be happy to help with it. Why didn't you
> put me in contact with him? You know I'm spending a lot of time on this.

Why don't you do it, instead of complaining? It is not upstream, and I won't
move it upstream yet, as I said before.

Btw, there's another driver that will likely be sent to me on the next few days.
I'm not sure if it is using the new model or not. I haven't seen the code yet,
so I'm not sure what functions it is being used. I suspect that it is still
using the old model, since its development started before months ago, before
the v4l2 dev/subdev conversion of the drivers.

> > After committing Devin's Austek patches (also seemed to be based on
> > em28xx), it will
> > probably be easier for Uri to convert his driver to the new approach.
> 
> That depends. As I said, there are other i2c issues that need to be
> clarified, but I *never* got the chance to ask him since you just merged
> this driver without the customary public review.

You have the chance right now. 

That's why we have the staging v4l/dvb -hg tree: to give everybody a view of
what's is planned. Also, a public announcement is done automatically by hg
mailbomb scripts, that sends all committed patches to the patch announcement ML.

Cheers,
Mauro

Re: [linuxtv-commits] [hg:v4l-dvb] Add cx231xx USB driver

[Hans Verkuil]

>
> On Thu, 12 Mar 2009 08:38:59 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> Mauro,
>>
>> What the hell??!
>>
>> Since when does a big addition like this get merged without undergoing a
>> public review?
>>
>> I've been working my ass off converting drivers to the new i2c API and
>> v4l2_subdev structures and here you merge a big driver that uses
>> old-style
>> (which will lead to 'deprecated' warnings when compiling with 2.6.29,
>> BTW),
>> where the driver writes directly to i2c modules instead of adding a
>> proper
>> i2c module for them. And what are 'colibri', 'flatrion' and 'hammerhead'
>> anyway? Are they integrated devices of the cx231xx? Can they be used
>> separately in other products as well?
>>
>> So yes, I have objections. At the minimum it should be converted first
>> to
>> use v4l2_device/v4l2_subdev and I need more information on the new i2c
>> devices so I can tell whether the code for those should be split off
>> into
>> separate i2c modules. Not to mention that I want to have the time to
>> review
>> this code more closely.
>>
>> Sorry Sri, this isn't your fault.
>
> Hans,
>
> I know that you're rushing to convert all drivers to the new model, but
> you
> should give time to time. Even with Kernel's fast development cycle, we
> should
> take care to not depreciate things faster than developers can track (btw,
> lwn.net already complained that V4L subsystem changes their APIs faster
> than
> usual).

Mauro, you did not answer the question why this driver was just merged
without going through a public review? If I'd seen it beforehand I'd have
worked together with Sri to get it fixed first. I don't expect him to know
about this, but I didn't even get a chance to discuss it and help with it.
Everyone else has to go through the normal review channels, but apparently
this was just fast-tracked and merged. That's not the way to do it.

Please back out this driver, put it in a separate tree and let me 1)
review this driver first, and 2) help Sri implementing the
v4l2_device/v4l2_subdev stuff.

> First of all, except for ivtv drivers, the first conversion to the new
> model
> occurred just few weeks ago. The new model will bring some gains, but this
> shouldn't stop the merge of the drivers whose development started before
> we
> port the drivers used as example by the developer.
>
> This is a new model, and we should give people some time to adapt to it.
> This
> is the way we worked in the past and it is the way we should keep working.

It's not a new model. The I2C core changes went in in 2.6.22. Jean is no
longer accepting new i2c drivers that use the old module, and neither
should we. And definitely NOT without discussing it first. The old i2c API
has been marked deprecated in 2.6.29, and that's for a reason. There are
good alternatives for this and I'm available to help with the conversion.
Not to mention that this will make Jean quite unhappy.

And please note that the use of the old API isn't the only question I
have, there are more oddities with the i2c handling that I'd like to have
more information about. Writing i2c registers directly from the adapter
driver doesn't look good to me at first sight.

> For
> example, video_ioctl2 were added several Kernel releases before merging
> uvc
> driver. Yet, we've accepted uvc driver without using the new model,
> because its
> development that occurred before video_ioctl2.
>
> The second point is that there's nothing at
> Documentation/feature-removal-schedule.txt informing that those stuff is
> deprecated.

Yes it is, see this from the 2.6.29 kernel:

What:   i2c_attach_client(), i2c_detach_client(), i2c_driver->detach_client()
When:   2.6.29 (ideally) or 2.6.30 (more likely)
Why:    Deprecated by the new (standard) device driver binding model. Use
        i2c_driver->probe() and ->remove() instead.
Who:    Jean Delvare <khali@linux-fr.org>

> So, we should still accept new drivers without the conversion at least
> until
> the end of 2.6.30 window, and add some notice at
> feature-removal-schedule.txt
> on 2.6.30 clearly stating what's deprecated and when, before generating
> penalty
> over the developers that are using the current drivers as their model of
> development.
>
> In the specific case of cx231xx driver, I've explained to Sri, there are
> still
> some issues to be fixed at the driver. Although the driver works, It is
> not
> ready for 2.6.30 yet.
>
> However, keeping this on a separate tree will just create more mess
> (according
> with Sri, he already had to rebase this driver 4 times during 2.6.29-rc
> cycle,
> due to the high speed of internal API changes).
>
> Since his driver seems to be based on em28xx, he had no sample on how to
> convert it to
> v4l2_device/v4l2_subdev/new_i2c model.

Again, if I'd known about it I'd be happy to help with it. Why didn't you
put me in contact with him? You know I'm spending a lot of time on this.

> After committing Devin's Austek patches (also seemed to be based on
> em28xx), it will
> probably be easier for Uri to convert his driver to the new approach.

That depends. As I said, there are other i2c issues that need to be
clarified, but I *never* got the chance to ask him since you just merged
this driver without the customary public review.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG