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

[Michael Krufky]

Uri Shkolnik wrote:

--- On Wed, 3/11/09, Michael Krufky <mkru...@linuxtv.org> wrote:

  
From: Michael Krufky <mkru...@linuxtv.org>
Subject: Re: [PULL] http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio
To: "Uri Shkolnik" <uri...@yahoo.com>
Cc: "Mauro Carvalho Chehab" <mche...@infradead.org>, linux-media@vger.kernel.org
Date: Wednesday, March 11, 2009, 3:18 PM
Uri Shkolnik wrote:
    
--- On Mon, 3/9/09, Mauro Carvalho Chehab <mche...@infradead.org>
      
wrote:
    
   
      
From: Mauro Carvalho Chehab <mche...@infradead.org>
Subject: Re: [PULL] http://linuxtv.org/hg/~mkrufky/sms1xxx-gpio
To: "Michael Krufky" <mkru...@linuxtv.org>,
        
"Uri Shkolnik" <uri...@yahoo.com>
    
Cc: linux-media@vger.kernel.org
Date: Monday, March 9, 2009, 6:17 AM
On Sun, 08 Mar 2009 10:58:55 -0400
Michael Krufky <mkru...@linuxtv.org>
wrote:

     
        
Uri Shkolnik wrote:
       
          
--- On Fri, 3/6/09, Michael Krufky <mkru...@linuxtv.org>
         
            
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:

[...@email.com:
Codingstyle fixes and merge conflicts]
Signed-off-by: My name <m...@email.com>

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html