Re: [PATCH 1/7] Adding empia base driver - Kernel

This is a discussion on Re: [PATCH 1/7] Adding empia base driver - Kernel ; Hi Markus, As promised I've done a review of your empia driver and looked at what needs to be done to get it into the kernel. First of all, I've no doubt that your empia driver is better and supports ...

+ Reply to Thread
Results 1 to 9 of 9

Thread: Re: [PATCH 1/7] Adding empia base driver

  1. Re: [PATCH 1/7] Adding empia base driver

    Hi Markus,

    As promised I've done a review of your empia driver and looked at what
    needs to be done to get it into the kernel.

    First of all, I've no doubt that your empia driver is better and
    supports more devices than the current em28xx driver. I also have no
    problem adding your driver separate from the current driver. It's been
    done before (certain networking drivers spring to mind) and while
    obviously not ideal I expect that the older em28xx driver can probably
    be removed after a year or something like that.

    In my opinion it's pretty much hopeless trying to convert the current
    em28xx driver into what you have. It's a huge amount of work that no
    one wants to do and (in this case) with very little benefit. Of course,
    Mauro has the final say in this.

    While there is some cleaning up to do in your code (coding style, some
    copyright/license clarifications), that is not a big deal. The coding
    style cleanups are a fair amount of work, but I think we can probably
    spread that out over several people and get that done quickly.

    What I definitely would recommend is to use video_ioctl2 rather than
    video_usercopy. The latter function will disappear in the future. I
    think the policy for new drivers is to use video_ioctl2, so this is
    probably a required task before it can be merged. Doing this will
    improve maintenance of the code as well, so it's useful to do this
    anyway. I think it's better if you do it, but I guess some volunteer
    can probably be found if needed. It's not a difficult task.

    Now the real problems are with three duplicate i2c drivers: cx25843,
    xc3028 and xc5000.

    To start with the easiest one: cx25843. There already is a driver for
    this (cx25840) and the empia driver should use that one. Since I
    maintain cx25840 the easiest approach for you is to see if you can get
    me hardware (em28xx + cx25843) so that I can test and update cx25843 to
    provide proper empia support. The alternative is that we work together
    on this, but that's probably more work for both of us. I most
    definitely do *not* want to duplicate this driver. Windows drivers
    duplicate this stuff all the time, but we're not Windows and having one
    driver ensures that fixes or new functionality become available to all
    bridge drivers that use it.

    The same reasoning is true for xc3028 and xc5000. In addition, the
    licensing of these sources is very vague. Is it even allowed to
    distribute them under a GPL license? There is no GPL license in the
    sources, yet in the module you specify GPL. This needs to be clarified
    first.

    I see two ways forward when it comes to these drivers:

    1) The empia driver switches to the current xceive drivers that are
    already in the kernel. No doubt this means that xceive driver bugs will
    surface with certain devices, but those are bugs that the xceive driver
    maintainer will have to fix. Obviously assistance would be appreciated,
    but the bottom line is that a) your driver is finally in the kernel,
    and b) there is a lot more pressure to fix bugs in the xceive drivers
    than is the case otherwise. Yes, some devices will not work initially
    with the empia driver, but I expect that to be a temporary situation.

    2) Your xceive drivers replace the current drivers. This will require
    that a) the license situation is clarified, b) the drivers are modified
    to fit the current v4l-dvb tuner architecture. This option will mean a
    lot of work for you since you are the maintainer of these drivers. In
    addition, I forsee a lot of flaming if we go this way.

    BTW, I noticed that the current xc5000 driver is very similar to yours
    but with proper copyrights/license notices and coding style. So for
    this driver option 1 is definitely the way to go.

    To be honest, I don't see option 2 as viable. I forsee too many
    inter-personal problems that will appear if we go that way. Option 1
    has the big advantage that all you need to do is to file a bug report
    if it doesn't work with a certain device. And in the meantime users can
    fallback to your stand-alone driver until it's fixed in the kernel.
    Obviously, if you can state in the bug report what the precise problem
    is, so much the better.

    So my recommendation would be to:

    1) Switch to using the current xceive drivers in your empia driver. This
    is something you have to do, I'm afraid. Unless someone would
    volunteer? I personally do not have enough experience with this to do
    it.

    2) Switch to using the cx25840 driver. If I can get hardware, then I can
    do this, otherwise we have to do this together. Initially we probably
    have to disable devices using the cx25840 until the cx25840 driver has
    been fixed for the empia driver.

    3) Switch to video_ioctl2 in the empia driver. You can do that, but we
    can probably find a volunteer as well.

    4) Conform the code to the coding style. If several people can help with
    this we can get it done pretty quickly.

    5) Merge the empia driver alongside the current em28xx driver.

    There are no doubt some things I missed, but I don't think I missed
    anything major. I've put up a hg tree here:

    http://linuxtv.org/hg/~hverkuil/v4l-dvb-em28xx/

    This allows you to compile the empia module alongside the em28xx module.
    Note that I've removed the empia cx25843 module in the last changeset
    in order to test which dependencies the driver had on that module. So
    if you want to test this tree with an empia+cx25843 device, then don't
    get the latest changeset, but the one before that.

    My tree does contain the empia xceive drivers, though. Perhaps someone
    more knowledgeable with DVB can take a look to see how much work it is
    to convert to the kernel xceive drivers? And to see if I overlooked any
    DVB-related major obstacles?

    I think this is a reasonable roadmap to finally get this in. If everyone
    can pitch in then it really shouldn't take that much work to get it
    into v4l-dvb and from there to 2.6.29.

    Regards,

    Hans Verkuil
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/7] Adding empia base driver

    On Sat, 1 Nov 2008 14:59:17 +0100
    Hans Verkuil wrote:

    > Hi Markus,
    >
    > As promised I've done a review of your empia driver and looked at what
    > needs to be done to get it into the kernel.
    >
    > First of all, I've no doubt that your empia driver is better and
    > supports more devices than the current em28xx driver. I also have no
    > problem adding your driver separate from the current driver. It's been
    > done before (certain networking drivers spring to mind) and while
    > obviously not ideal I expect that the older em28xx driver can probably
    > be removed after a year or something like that.
    >
    > In my opinion it's pretty much hopeless trying to convert the current
    > em28xx driver into what you have. It's a huge amount of work that no
    > one wants to do and (in this case) with very little benefit. Of course,
    > Mauro has the final say in this.
    >


    Both upstream and the 4 duplicated drivers have similar functionality. Also,
    the upstream driver is actively maintained. So, there's no sense on accepting
    those duplicated drivers.

    Also, just replacing one existing driver by a newer one will cause regressions
    on some already fixed bugs and remove some improvements that the upstream driver
    suffered.

    If there's a bug or a lack of functionality on em28xx, cx25843, xc5000 or
    tuner-xc2028, it is just a matter of submitting patches fixing those bugs or
    adding newer features.

    Cheers,
    Mauro
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 1/7] Adding empia base driver

    On Sat, 8 Nov 2008 03:50:20 -0200
    "Andre Kelmanson" wrote:

    > Dears,
    >
    > I'm using this version of em28xx for a long time and it's working fine. It
    > has three very important features for me. The first one is Kaiomy device,
    > the second one is the new em28xx-audoep module and the third one is PAL-M
    > support. Kaiomy and PAL-M support were developed based on my support on the
    > em28xx mailinglist.
    >
    > Now I can use my device (Kaiomy) outside Windows with sound (em28xx-audioep)
    > and colors (PAL-M)! I'm using this version everyday with no problems.
    >
    > Because of this, it will be nice if this work could be included in the
    > kernel code. What do you (other users) think about having that driver in
    > kernel?


    André,

    First of all, the big issue why we aren't merging em28xx improvements from
    Markus is that he is not following the rules.

    For example, you said that you've contributed with Markus tree.

    However, on Markus pull request, I see no patch from you on his series of patches.

    The correct procedure would be just to forward your patch as-is, adding with his SOB bellow yours.

    Not doing this, he would be considered as the author of your patch. IANAL, but
    this doesn't seem to be right, from GPL's perspective. Probably, there are
    other patches there not authored by Markus that are just merged inside his big patch.

    About PAL-M, this always worked at the upstream driver. I have myself lots of
    em28xx devices, all working in colors with PAL-M, and with audio. I live in
    Brazil, so I always check if PAL-M is ok

    If you want to have your device supported, just send us a patch against the
    upstream driver.

    Cheers,
    Mauro
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 1/7] Adding empia base driver

    On Sat, Nov 8, 2008 at 11:15 AM, Mauro Carvalho Chehab
    wrote:
    > On Sat, 8 Nov 2008 03:50:20 -0200
    > "Andre Kelmanson" wrote:
    >
    >> Dears,
    >>
    >> I'm using this version of em28xx for a long time and it's working fine. It
    >> has three very important features for me. The first one is Kaiomy device,
    >> the second one is the new em28xx-audoep module and the third one is PAL-M
    >> support. Kaiomy and PAL-M support were developed based on my support on the
    >> em28xx mailinglist.
    >>
    >> Now I can use my device (Kaiomy) outside Windows with sound (em28xx-audioep)
    >> and colors (PAL-M)! I'm using this version everyday with no problems.
    >>
    >> Because of this, it will be nice if this work could be included in the
    >> kernel code. What do you (other users) think about having that driver in
    >> kernel?

    >
    > André,
    >
    > First of all, the big issue why we aren't merging em28xx improvements from
    > Markus is that he is not following the rules.
    >
    > For example, you said that you've contributed with Markus tree.
    >
    > However, on Markus pull request, I see no patch from you on his series of patches.
    >
    > The correct procedure would be just to forward your patch as-is, adding with his SOB bellow yours.
    >
    > Not doing this, he would be considered as the author of your patch. IANAL, but
    > this doesn't seem to be right, from GPL's perspective. Probably, there are
    > other patches there not authored by Markus that are just merged inside his big patch.
    >
    > About PAL-M, this always worked at the upstream driver. I have myself lots of
    > em28xx devices, all working in colors with PAL-M, and with audio. I live in
    > Brazil, so I always check if PAL-M is ok
    >
    > If you want to have your device supported, just send us a patch against the
    > upstream driver.
    >


    As written earlier already I don't think that I didn't follow any
    rules here since I provided single
    patches at the very first beginning

    http://mcentral.de/v4l-dvb/
    (this is all kernel code and only kernel code).

    That work didn't get attention and due a different decision of
    framework changes (which that codebase relied
    on) it all had to be rebased, I doubt that anyone
    would have reworked all that patch for patch. Instead it went into one
    repository and finally got modified to work again
    with the available framework rather than relying onto any such modifications.

    The modification which I received from André suggested to use the NTSC
    VBI offset lines for PAL-M, and the
    rest for audio comes from my side.

    Markus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 1/7] Adding empia base driver

    On Sat, 8 Nov 2008, Markus Rechberger wrote:

    > As written earlier already I don't think that I didn't follow any
    > rules here since I provided single
    > patches at the very first beginning
    >
    > http://mcentral.de/v4l-dvb/
    > (this is all kernel code and only kernel code).
    >
    > That work didn't get attention and due a different decision of
    > framework changes (which that codebase relied
    > on) it all had to be rebased, I doubt that anyone
    > would have reworked all that patch for patch. Instead it went into one
    > repository and finally got modified to work again
    > with the available framework rather than relying onto any such modifications.


    One thing is to rebase a tree, another is to merge all patches into a big
    one, not preserving the original authorships.

    Development trees sometimes need rebase. This is done by popping all newer
    patches from the tree, applying the upstream patches, and then pushing
    again every individual patches, fixing the ones that don't apply well, but
    preserving their authorships.

    The modified patches should receive a special tag before the
    maintainer's SOB, like:

    [me@mymail: I did this to apply this patch]

    as stated at the kernel docs.

    This method will reduce a lot the risk of breaking improvements and other
    fixes that happened upstream, and will properly preserve authorship of
    individual patches.

    If you were doing a rebase, your patches would likely be accepted.

    --
    Cheers,
    Mauro
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 1/7] Adding empia base driver

    On Sat, Nov 8, 2008 at 11:42 AM, Markus Rechberger
    wrote:
    > On Sat, Nov 8, 2008 at 11:37 AM, Mauro Carvalho Chehab
    > wrote:
    >> On Sat, 8 Nov 2008, Markus Rechberger wrote:
    >>
    >>> As written earlier already I don't think that I didn't follow any
    >>> rules here since I provided single
    >>> patches at the very first beginning
    >>>
    >>> http://mcentral.de/v4l-dvb/
    >>> (this is all kernel code and only kernel code).
    >>>
    >>> That work didn't get attention and due a different decision of
    >>> framework changes (which that codebase relied
    >>> on) it all had to be rebased, I doubt that anyone
    >>> would have reworked all that patch for patch. Instead it went into one
    >>> repository and finally got modified to work again
    >>> with the available framework rather than relying onto any such
    >>> modifications.

    >>
    >> One thing is to rebase a tree, another is to merge all patches into a big
    >> one, not preserving the original authorships.
    >>
    >> Development trees sometimes need rebase. This is done by popping all newer
    >> patches from the tree, applying the upstream patches, and then pushing again
    >> every individual patches, fixing the ones that don't apply well, but
    >> preserving their authorships.
    >>
    >> The modified patches should receive a special tag before the maintainer's
    >> SOB, like:
    >>
    >> [me@mymail: I did this to apply this patch]
    >>
    >> as stated at the kernel docs.
    >>
    >> This method will reduce a lot the risk of breaking improvements and other
    >> fixes that happened upstream, and will properly preserve authorship of
    >> individual patches.
    >>
    >> If you were doing a rebase, your patches would likely be accepted.
    >>

    >


    let's link back to the only review done:
    http://linux.derkeiler.com/Mailing-L.../msg00060.html


    In my opinion it's pretty much hopeless trying to convert the current
    em28xx driver into what you have. It's a huge amount of work that no
    one wants to do and (in this case) with very little benefit.


    Markus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 1/7] Adding empia base driver

    On Sat, Nov 8, 2008 at 11:37 AM, Mauro Carvalho Chehab
    wrote:
    > On Sat, 8 Nov 2008, Markus Rechberger wrote:
    >
    >> As written earlier already I don't think that I didn't follow any
    >> rules here since I provided single
    >> patches at the very first beginning
    >>
    >> http://mcentral.de/v4l-dvb/
    >> (this is all kernel code and only kernel code).
    >>
    >> That work didn't get attention and due a different decision of
    >> framework changes (which that codebase relied
    >> on) it all had to be rebased, I doubt that anyone
    >> would have reworked all that patch for patch. Instead it went into one
    >> repository and finally got modified to work again
    >> with the available framework rather than relying onto any such
    >> modifications.

    >
    > One thing is to rebase a tree, another is to merge all patches into a big
    > one, not preserving the original authorships.
    >
    > Development trees sometimes need rebase. This is done by popping all newer
    > patches from the tree, applying the upstream patches, and then pushing again
    > every individual patches, fixing the ones that don't apply well, but
    > preserving their authorships.
    >
    > The modified patches should receive a special tag before the maintainer's
    > SOB, like:
    >
    > [me@mymail: I did this to apply this patch]
    >
    > as stated at the kernel docs.
    >
    > This method will reduce a lot the risk of breaking improvements and other
    > fixes that happened upstream, and will properly preserve authorship of
    > individual patches.
    >
    > If you were doing a rebase, your patches would likely be accepted.
    >


    Should I start picking patches from the linuxtv.org tree where patches
    from my tree are taken
    and where the Sign off is not provided?

    Just recently one patch went into not even stating that it comes from
    my codebase, although not
    worth the time making an elephant out of it.

    Markus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 1/7] Adding empia base driver

    On Sat, 8 Nov 2008, Markus Rechberger wrote:

    > Should I start picking patches from the linuxtv.org tree where patches
    > from my tree are taken
    > and where the Sign off is not provided?


    SOB's are just meant to testify that the code is GPL. It has nothing to do
    with authorship.

    On all cases I'm aware, when some code from your tree were merged
    upstream, your authorship were marked inside the patch's description.
    Also, your authorship are explicit at the em28xx files you've contributed.

    --
    Cheers,
    Mauro Carvalho Chehab
    http://linuxtv.org
    mchehab@infradead.org
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 1/7] Adding empia base driver

    On Sat, Nov 8, 2008 at 11:56 AM, Mauro Carvalho Chehab
    wrote:
    > On Sat, 8 Nov 2008, Markus Rechberger wrote:
    >
    >> Should I start picking patches from the linuxtv.org tree where patches
    >> from my tree are taken
    >> and where the Sign off is not provided?

    >
    > SOB's are just meant to testify that the code is GPL. It has nothing to do
    > with authorship.
    >
    > On all cases I'm aware, when some code from your tree were merged upstream,
    > your authorship were marked inside the patch's description. Also, your
    > authorship are explicit at the em28xx files you've contributed.
    >


    really?
    http://linuxtv.org/hg/v4l-dvb/rev/5f3c3af9c1f9

    http://article.gmane.org/gmane.linux.drivers.dvb/44726

    nevermind.

    Markus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread