Re: [PULL] http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio

[Michael Krufky <mkrufky@xxxxxxxxxxx>]

Uri Shkolnik wrote:
> --- On Wed, 3/11/09, Michael Krufky <mkrufky@xxxxxxxxxxx> wrote:
>
>   
> > From: Michael Krufky <mkrufky@xxxxxxxxxxx>
> > Subject: Re: [PULL] http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio
> > To: "Uri Shkolnik" <urishk@xxxxxxxxx>
> > Cc: "Mauro Carvalho Chehab" <mchehab@xxxxxxxxxxxxx>, linux-media@xxxxxxxxxxxxxxx
> > Date: Wednesday, March 11, 2009, 3:18 PM
> > Uri Shkolnik wrote:
> >     
> > > --- On Mon, 3/9/09, Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
> > >       
> > wrote:
> >     
> > >    
> > >       
> > > > From: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
> > > > Subject: Re: [PULL] http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio
> > > > To: "Michael Krufky" <mkrufky@xxxxxxxxxxx>,
> > > >         
> > "Uri Shkolnik" <urishk@xxxxxxxxx>
> >     
> > > > Cc: linux-media@xxxxxxxxxxxxxxx
> > > > Date: Monday, March 9, 2009, 6:17 AM
> > > > On Sun, 08 Mar 2009 10:58:55 -0400
> > > > Michael Krufky <mkrufky@xxxxxxxxxxx>
> > > > wrote:
> > > >
> > > >      
> > > >         
> > > > > Uri Shkolnik wrote:
> > > > >        
> > > > >           
> > > > > > --- On Fri, 3/6/09, Michael Krufky <mkrufky@xxxxxxxxxxx>
> > > > > >          
> > > > > >             
> > > > wrote:
> > > >
> > > > <snip/>
> > > >
> > > >      
> > > >         
> > > > > > I reviewed your tree , which is indicated
> > > > > >             
> > by the
> >     
> > > > > >          
> > > > > >             
> > > > email above, and compared it to a tree I built
> > > >         
> > from the v4l
> >     
> > > > 'trunk' repository + Siano's patches. 
> > > >         
> >    
> >     
> > > > > > I found out two things -
> > > > > > 1) The trees are still *very* differ from
> > > > > >             
> > each
> >     
> > > > > >          
> > > > > >             
> > > > other
> > > >      
> > > >         
> > > > > > 2) You applied *new* things that are
> > > > > >             
> > _not_
> >     
> > > > > >          
> > > > > >             
> > > > included in the "old" Siano's patches (and some of
> > > >         
> > them even
> >     
> > > > break it).
> > > >
> > > > <snip/>
> > > >
> > > >      
> > > >         
> > > > > #1 The patches that you submitted break the
> > > > >           
> > currently
> >     
> > > > >        
> > > > >           
> > > > supported devices
> > > >      
> > > >         
> > > > > #2 The patches that you submitted are not
> > > > >           
> > atomic
> >     
> > > > > #3 The patches that you submitted introduce
> > > > >        
> > > > >           
> > > > regressions
> > > >      
> > > >         
> > > > > #4 The patches that you submitted have
> > > > >           
> > multiple
> >     
> > > > >        
> > > > >           
> > > > changes in single patches.
> > > >      
> > > >         
> > > > > #5 The patches that you submitted are not safe
> > > > >           
> > for a
> >     
> > > > >        
> > > > >           
> > > > git-bisection nor     
> > > >         
> > > > > hg-bisection
> > > > >
> > > > > #6 The "codingstyle cleanups" that are
> > > > >           
> > intertwined in
> >     
> > > > >        
> > > > >           
> > > > your patches are     
> > > >         
> > > > > actually codingstyle regressions rather than
> > > > >        
> > > > >           
> > > > cleanups.
> > > >
> > > > <snip/>
> > > >
> > > > Uri and Michael,
> > > >
> > > > We should try to do our best to avoid breaking the
> > > >         
> > devices
> >     
> > > > currently supported
> > > > in kernel. That means that we shouldn't add a
> > > >         
> > changeset
> >     
> > > > that we know that it
> > > > will break a device, except if we are committing,
> > > >         
> > in the
> >     
> > > > same patch series,
> > > > another patch fixing it. This is probably the
> > > >         
> > most
> >     
> > > > important reason why a patch
> > > > is not merged upstream.
> > > >
> > > > Also, we should warrant that a patch won't break
> > > >         
> > the
> >     
> > > > compilation of the kernel,
> > > > otherwise, the standard procedure to identify a
> > > >         
> > broken
> >     
> > > > patch (bisect) will
> > > > fail. This affects not only our subsystem, but the
> > > >         
> > entire
> >     
> > > > kernel, so this is
> > > > another important rule to be followed on patch
> > > >         
> > submission.
> >     
> > > > That's said, I agree that we shouldn't commit the
> > > >         
> > patch
> >     
> > > > series as-is. One of
> > > > you should rewrite the patch series to avoid
> > > >         
> > breaking the
> >     
> > > > existing cards. On
> > > > the other hand, a patch shouldn't suffer large
> > > >         
> > changes
> >     
> > > > without the
> > > > patch's author ack [1].
> > > >
> > > > So, I ask you both to agree on some procedure, and
> > > >         
> > to be
> >     
> > > > sure to not forward me
> > > > another patchset without both of you agree with
> > > >         
> > it.
> >     
> > > > [1] yet, small changes like codingstyle
> > > >         
> > adjustments and
> >     
> > > > merge conflict fixes
> > > > are ok, if properly documented with the proper
> > > >         
> > meta-tag,
> >     
> > > > just before the SOB:
> > > >
> > > > [my@xxxxxxxxx:
> > > > Codingstyle fixes and merge conflicts]
> > > > Signed-off-by: My name <my@xxxxxxxxx>
> > > >
> > > > Cheers,
> > > > Mauro
> > > >
> > > >      
> > > >         
> > > Hi all,
> > >
> > > Sorry for the delay in my replay, we had a holiday
> > >       
> > (always a good thing!), hope everything is OK with all of
> > you.
> >     
> > > Let me summaries the current status as I see it, and
> > >       
> > my suggestion for the near future.
> >     
> > > There is big amount of changes and additions to the
> > >       
> > SMS based devices drivers pending since August 2008 (and
> > even earlier), which should be merged into the hg (and the
> > kernel git).
> >     
> > > We have decided that Mike will take Siano's patches
> > >       
> > from the vger server, modify them if needed, and will merge
> > them to the hg. 
> >     
> > > IMHO that we should alter our decision from the
> > >       
> > following reasons:
> >     
> > > 1) Mike is very busy (you may just give a short
> > >       
> > glimpse in the ML and see how many various patches he has
> > introduced in the last week), that causes long delay
> > (months) with the SMS drivers merge process.
> >     
> > > 2) At Siano QA department, the drivers are tested with
> > >       
> > big variety of different SMS-based devices using advanced
> > automatic systems, with several DTV standards and sources.
> > The drivers are also checked against various kernel versions
> > (remember that the SMS drivers set is largely used with
> > embedded-Linux devices, which are not always use the latest
> > kernel, but they do upgrade their devices drivers), and all
> > interfaces (SDIO, SPP, USB) are tested.
> >     
> > > The drivers are also tested at various companies which
> > >       
> > either manufactures SMS-based devices, or use them (There is
> > company that implements multimedia player that tests the
> > recent drivers set with many devices, including Mike’s),
> > and so on…
> >     
> > > Mike can spend very limited time on testing, and he
> > >       
> > can test the drivers only on a single host-interface setup
> > (x86/USB) and with very few devices (his company’s and
> > Siano’s development modules). 
> >     
> > > My suggestion is that we’ll “reset” the process.
> > >       
> > > We know for sure (tested and verified) that the final
> > >       
> > result, after merging all Siano’s patches from the vger is
> > robust driver set (which has also been tested with Mike’s
> > devices). We also know it passes “checkpatch.pl” script
> > (from kernel version 2.6.28) so it’s quite good, maybe
> > some coding style issues still exist, but not too many.
> >     
> > > So, the code itself is in good condition.
> > >   So, my suggestion is as follow –
> > > Let’s take, one after the other, the patches from
> > >       
> > the vger server, and “put them on the surgeon’s
> > table”. Per *single* patch, one at a time, we’ll ask for
> > anyone's review.
> >     
> > > If there will be no comment, it will be committed as
> > >       
> > is. If there will be, I’ll fix it, pass it through the
> > needed QA tests, and than it will be submit for a second
> > review.
> >     
> > > Hope to get your opinions about my suggestion soon,
> > >
> > > Regards,
> > >
> > > Uri
> > >    
> > >       
> > Uri,
> >
> > I was already in the process of merging your changesets
> > into the master repository when you had interrupted the
> > process.
> >
> > Your patches are *not* ok -- They break the Hauppauge
> > devices -- I explained this to you in my previous email, and
> > I'd prefer not to repeat myself.  But, in summary:
> >
> > #1 - The way the gpios are being handled breaks the 
> > functionality of the Hauppauge devices.  For the
> > Hauppauge devices, when we toggle the GPIO's, we should only
> > use the "sendrequest" function instead of the "sendrequest
> > and wait" function -- The gpio functions need a "wait"
> > option to specify this behavior.  This was the only
> > change that I had made to your gpio changeset.  This is
> > a critical issue for the Hauppauge devices.  (merging
> > your patch without this change would cause serious
> > regressions in the Hauppauge devices)
> >
> >     
>
> Mike, as I explained some months ago, your GPIO implementation is wrong from multiple reasons, I can't explain all of them in this open forum, but some of them are:
> 1) You implement 10xx GPIO configuration on 11xx, which may lead to significant regression in RF performances.
> 2) You did not handle GPIO grouping, meaning that with other board layout/configuration, and/or with other host interface other than USB the device will stop to function, simple as that. 
> 3) The *generic* sms-cards implementation has been tested on Hauppauge devices and found to work flawlessly. If you experience any problem with it, please let me know what is the problem, and your bug report will be handled promptly. 
>
> BTW - Since I must support multiple dozens of devices other than Hauppauge's, I must insist on generic implementation
>   

I agree with you -- the generic implementation is the way to go.  That's 
why I took your exact GPIO patch, added the wait option & cleaned up the 
codingstyle issues, then pushed it here:

http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio/rev/4613747519c8

^^^ This is *your* generic GPIO implementation, cleaned up for 
codingstyle and added the wait option to prevent breakage on the 
Hauppauge devices.

Now I am getting the feeling that you're not actually looking at the 
code.  This is EXACTLY your function, just with the added wait option 
and cleaned up into a single atomic changeset.  10xx is separated from 
11xx, grouping is handled, and the Hauppauge-specific code paths are 
used only on the Hauppauge devices.



>   
> > #2 - Your patches are not atomic.  I had broken your
> > patches down into smaller, atomic changesets.  I had
> > completed this process, and was in the process of merging
> > these broken-down patches a few at a time.  I like to
> > make small merge requests, so that the amount of changes to
> > review are smaller, and the merge process is easier for
> > everybody.  You've interrupted this merge process, and
> > you don't seem to realize that these are your changes up for
> > merge, just cleaned up.
> >
> >     
>
> You comment is not accurate, please note that the patch list is quite long and we can divide it to three types:
> 1) Completely new components (such as SDIO bus driver and IR support), which there are no "change-set", since it is the first introduction.
> Example (add IR support) - http://patchwork.kernel.org/patch/6102/   
> 2) Various small change-set that stands in the "keep it small and often" patch (such as http://patchwork.kernel.org/patch/2999/ http://patchwork.kernel.org/patch/8591/ )
> 3) Some few are big, compound patches that should be break down to several peaces.
>
> Most of the patches are from the two non-problematic types, and for the few problematic, compound patches, I suggested that I'll break them to several patches.
>   

If you merge the patch series as-is, merge one patch then build the 
whole tree, you'll find that your patches, when applied one-by-one, 
actually break the build.  I have remedied this issue by breaking down 
the patches into smaller changesets to be merged in groups, gradually.



>   
> > #3 - Your patches introduce regressions.  In the
> > process of breaking down your large patches into smaller
> > patches, I've removed the regressions from the changesets.
> >
> >     
>
> I don't know about any kind of regressions which is caused by my patches, on the contrary, they fix multiple issues with the current hg version.
> Please elaborate.
>   

I've explained it already.  To explain again would be repeating myself.  
But in summary, your changesets as-is thoroughly break the Hauppauge 
devices, and you are UNDOing codingstyle cleanups and various other 
fixes that have been made to the driver by myself and other upstream 
kernel janitors.

We don't generate changesets by diff'ing a file from your Siano 
repository against the mercurial repository -- that is how regressions 
are introduced.

>   
> > #4 - Your patches have multiple changes in a single
> > patch.   I broke down those changes into
> > smaller patches, and you saw the first round of results -
> > this is what you prevented from getting merged.
> >
> >     
>
> Answered above
>
>   
> > #5 - Your patches break bisection tests.  I broke them
> > down to avoid this issue, but you prevented the merge.
> >
> >     
>
> I don't know what are "bisection tests", please elaborate.
>   

see "atomic changesets" and explanation above.

>   
> > #6 - Your codingstyle "cleanups" are regressions rather
> > than cleanups.  I've taken care of those while I was
> > cleaning your patches.
> >     
>
>
> This is very generic and non-accurate. All patches under http://patchwork.kernel.org/project/linux-media/list/ passes the checkpatch.pl script without *any* error or warning.
>
> If you find otherwise - please provide me with details and I'll fix it.
>
>   
> > Uri,  I have done all this work for you, and I was
> > finally merging it all into the master branch.  Your
> > patches cant be merged as-is.  We had already agreed
> > that I would clean up your patches and merge them, you have
> > let me spend all of my free time working on your code, and
> > now you've done this.
> >
> > As we agreed previously, I have been working to clean up
> > your patches, and I have begun the process of merging the
> > results into the master branch.  I have an interest in
> > seeing all of your pending changes get merged, and I'd like
> > to see the support for additional functionality appear in
> > the driver.
> >
> > However, we don't rebase against other repositories. 
> > You have submitted a large amount of changes that cant be
> > merged all at once.  I've already started breaking down
> > your patches and I have issued the first merge request.
> >
> > If you would like to see the bulk of your changes merged
> > quickly, then I suggest that you respond to this email
> > thread and explain to Mauro that you were mistaken when you
> > prevented my merge request.
> >
> > If Mauro merges my pending "sms1xxx-gpio" tree, then I can
> > move forward and push the remaining changesets up to
> > linuxtv.org for merge.
> >
> > Uri, nobody else cares about this driver as much as you and
> > I do -- By interrupting the work that I've been doing to
> > merge your changesets, I believe that you are simply
> > shooting yourself in the foot.
> >
> >     
>
>   
> > The only alternative is to break down your huge patches all
> > by yourself, but why bother doing that work when you know
> > that I have already spent weeks doing it myself?
> >
> > It's your decision.
> >     
>
>
> I'm truly sorry about how you think and feel about it, but the situation is that I can't lift my objection to those changes. The changes you introduce prevents multiple devices to work properly, break the SDIO and SPI implementations, cause performances regression and many more problems.  
>
> I know you spent lot of your free time with the SMS code, and I appreciate it very much, but you surely understand that I can't let code that work for only fraction of Siano's customers and partners to be qualified.

I repeat:  I merged a portion of your changes into the "sms1xxx-gpio" 
repository.  This was a step in the direction of merging the all of your 
changes, but I make these merges small.  Once the first round of 
changesets is merged, then I push in the next round, so that the review 
of all changes can be small and gradual.  This way, it's easier for 
others to analyze and test the changes without complication.

You'll find that when all is done, the final code will look very much 
like what you have in your repository.  The only difference would be the 
way in which we get there.  As I said before, I recommend that you allow 
me to move forward with the merge, and when all is done, we should work 
out the remaining differences.

Regards,

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