Siano's patches

Siano's patches

[Uri Shkolnik]

Hi Mauro,

Hope you had a great weekend :-)


I would like to know if you have already reached the conclusion whether to use the Mercurial tree option or the email option we discussed last week.   

Regarding the patches that have been already submitted to the linux-media@vger.kernel.org ML, any schedule for the merge? 


Thanks & Regard,

Uri


      

Re: Siano's patches

[Uri Shkolnik]

--- On Sun, 1/18/09, Michael Krufky <mkrufky@linuxtv.org> wrote:

> From: Michael Krufky <mkrufky@linuxtv.org>
> Subject: Re: Siano's patches
> To: urishk@yahoo.com
> Cc: "Mauro Carvalho" <mchehab@infradead.org>, linux-media@vger.kernel.org, linuxtv-commits@linuxtv.org, "linux-dvb" <linux-dvb@linuxtv.org>
> Date: Sunday, January 18, 2009, 8:07 PM
> Uri Shkolnik wrote:
> > Hi Mauro,
> > 
> > Hope you had a great weekend :-)
> > 
> > 
> > I would like to know if you have already reached the
> conclusion whether to use the Mercurial tree option or the
> email option we discussed last week.   
> > Regarding the patches that have been already submitted
> to the linux-media@vger.kernel.org ML, any schedule for the
> merge?   
> Uri,
> 
> I've already responded to your last email -- You should
> continue to submit patches to the mailing list.

Mauro suggested two options, I asked for the first (the Mercurial tree) which is more convenient for me. As it used by multiple parties here and if there is no objection based on some unknown (to me) reason, I would like to use that option.


> 
> As for your pending patches, I explained in my email that
> they are in my queue.  I am in the midst of reviewing the
> changes.  As you're already aware, your changes break
> the Hauppauge device's functionality.  After merging
> your changes, I will have to go back and re-implement the
> Hauppauge-specific changes in the driver, using the new
> methods in your pending patches.
> 

Some of the patches in the list are pending from early September, 2008.
( I think it's time they will be popped out of the queue.... :-)

Regarding restoring, modifying, enhancing, etc. - Please do it successively, submitting my patches and your after them, so (1) the congruity between the Mercurial DVB tree change-sets and Siano's change-set will be kept, and (2) I, and other reviewers, may review your patches/changes in their own change-sets.


> Once I have reviewed & merged your changes and after I
> can restore the proper functionality to the hauppauge
> devices, then I will post a new mercurial tree for testing
> against all siano-based devices.
> 

Please see above

> Please be patient -- this takes time.
> 
> Regards,
> 
> Mike
> --
> To unsubscribe from this list: send the line
> "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at 
> http://vger.kernel.org/majordomo-info.html


      

Re: Siano's patches

[Mauro Carvalho Chehab]

Hi Uri,

On Sun, 18 Jan 2009 10:37:32 -0800 (PST)
Uri Shkolnik <urishk@yahoo.com> wrote:

> > From: Michael Krufky <mkrufky@linuxtv.org>
> > Subject: Re: Siano's patches
> > To: urishk@yahoo.com
> > Cc: "Mauro Carvalho" <mchehab@infradead.org>, linux-media@vger.kernel.org, linuxtv-commits@linuxtv.org, "linux-dvb" <linux-dvb@linuxtv.org>
> > Date: Sunday, January 18, 2009, 8:07 PM
> > Uri Shkolnik wrote:
> > > Hi Mauro,
> > > 
> > > Hope you had a great weekend :-)

Yes, I had ;)

> > > 
> > > 
> > > I would like to know if you have already reached the
> > conclusion whether to use the Mercurial tree option or the
> > email option we discussed last week.   
> > > Regarding the patches that have been already submitted
> > to the linux-media@vger.kernel.org ML, any schedule for the
> > merge?   
> > Uri,
> > 
> > I've already responded to your last email -- You should
> > continue to submit patches to the mailing list.
> 
> Mauro suggested two options, I asked for the first (the Mercurial tree) which is more convenient for me. As it used by multiple parties here and if there is no objection based on some unknown (to me) reason, I would like to use that option.
> 
> 
> > 
> > As for your pending patches, I explained in my email that
> > they are in my queue.  I am in the midst of reviewing the
> > changes.  As you're already aware, your changes break
> > the Hauppauge device's functionality.  After merging
> > your changes, I will have to go back and re-implement the
> > Hauppauge-specific changes in the driver, using the new
> > methods in your pending patches.
> > 
> 
> Some of the patches in the list are pending from early September, 2008.
> ( I think it's time they will be popped out of the queue.... :-)
> 
> Regarding restoring, modifying, enhancing, etc. - Please do it successively, submitting my patches and your after them, so (1) the congruity between the Mercurial DVB tree change-sets and Siano's change-set will be kept, and (2) I, and other reviewers, may review your patches/changes in their own change-sets.
> 
> 
> > Once I have reviewed & merged your changes and after I
> > can restore the proper functionality to the hauppauge
> > devices, then I will post a new mercurial tree for testing
> > against all siano-based devices.
> > 
> 
> Please see above
> 
> > Please be patient -- this takes time.

Since the current Siano implementation is needed by some existing cards and
that, from what I understood from your request and Michael comments, those
patches could cause some regressions with the supported boards, we should wait
for Michael's reviews and tests, if this doesn't take that long.

Uri, I'm assuming that you're familiar with kernel development. If not, I
recommend you to read README.patches and the related docs at kernel's
Documentation tree. If you have any doubts about that, we're here to answer.

Those patches are late for kernel 2.6.29, since the merge window for that
kernel were already closed. So, we will have some time until the next open
window opens (we should have at least 5 weeks). This allows us to do a better
review the changesets to be sure that:
	they don't cause any regressions;
	they don't violate the DVB API;
	they don't create undocumented userspace API's;
	they are using the best development practices on kernel development;
	individual patches don't break compilation (to avoid breaking git bissect).

For a large changeset like Siano ones, such reviews take time.

Ah, next time, please number your changesets with something like [PATCH 01/xx].
This helps me to properly identify and honour the correct patch order.

Cheers,
Mauro

Re: Siano's patches

[Uri Shkolnik]

--- On Mon, 1/19/09, Mauro Carvalho Chehab <mchehab@infradead.org> wrote:

> From: Mauro Carvalho Chehab <mchehab@infradead.org>
> Subject: Re: Siano's patches
> To: urishk@yahoo.com
> Cc: "Michael Krufky" <mkrufky@linuxtv.org>, linux-media@vger.kernel.org, linuxtv-commits@linuxtv.org, "linux-dvb" <linux-dvb@linuxtv.org>
> Date: Monday, January 19, 2009, 1:29 PM
> Hi Uri,
> 
> On Sun, 18 Jan 2009 10:37:32 -0800 (PST)
> Uri Shkolnik <urishk@yahoo.com> wrote:
> 
> > > From: Michael Krufky <mkrufky@linuxtv.org>
> > > Subject: Re: Siano's patches
> > > To: urishk@yahoo.com
> > > Cc: "Mauro Carvalho"
> <mchehab@infradead.org>, linux-media@vger.kernel.org,
> linuxtv-commits@linuxtv.org, "linux-dvb"
> <linux-dvb@linuxtv.org>
> > > Date: Sunday, January 18, 2009, 8:07 PM
> > > Uri Shkolnik wrote:
> > > > Hi Mauro,
> > > > 
> > > > Hope you had a great weekend :-)
> 
> Yes, I had ;)
> 
> > > > 
> > > > 
> > > > I would like to know if you have already
> reached the
> > > conclusion whether to use the Mercurial tree
> option or the
> > > email option we discussed last week.   
> > > > Regarding the patches that have been already
> submitted
> > > to the linux-media@vger.kernel.org ML, any
> schedule for the
> > > merge?   
> > > Uri,
> > > 
> > > I've already responded to your last email --
> You should
> > > continue to submit patches to the mailing list.
> > 
> > Mauro suggested two options, I asked for the first
> (the Mercurial tree) which is more convenient for me. As it
> used by multiple parties here and if there is no objection
> based on some unknown (to me) reason, I would like to use
> that option.
> > 
> > 
> > > 
> > > As for your pending patches, I explained in my
> email that
> > > they are in my queue.  I am in the midst of
> reviewing the
> > > changes.  As you're already aware, your
> changes break
> > > the Hauppauge device's functionality.  After
> merging
> > > your changes, I will have to go back and
> re-implement the
> > > Hauppauge-specific changes in the driver, using
> the new
> > > methods in your pending patches.
> > > 
> > 
> > Some of the patches in the list are pending from early
> September, 2008.
> > ( I think it's time they will be popped out of the
> queue.... :-)
> > 
> > Regarding restoring, modifying, enhancing, etc. -
> Please do it successively, submitting my patches and your
> after them, so (1) the congruity between the Mercurial DVB
> tree change-sets and Siano's change-set will be kept,
> and (2) I, and other reviewers, may review your
> patches/changes in their own change-sets.
> > 
> > 
> > > Once I have reviewed & merged your changes
> and after I
> > > can restore the proper functionality to the
> hauppauge
> > > devices, then I will post a new mercurial tree
> for testing
> > > against all siano-based devices.
> > > 
> > 
> > Please see above
> > 
> > > Please be patient -- this takes time.
> 
> Since the current Siano implementation is needed by some
> existing cards and
> that, from what I understood from your request and Michael
> comments, those
> patches could cause some regressions with the supported
> boards, we should wait
> for Michael's reviews and tests, if this doesn't
> take that long.
> 
> Uri, I'm assuming that you're familiar with kernel
> development. If not, I
> recommend you to read README.patches and the related docs
> at kernel's
> Documentation tree. If you have any doubts about that,
> we're here to answer.
> 
> Those patches are late for kernel 2.6.29, since the merge
> window for that
> kernel were already closed. So, we will have some time
> until the next open
> window opens (we should have at least 5 weeks). This allows
> us to do a better
> review the changesets to be sure that:
> 	they don't cause any regressions;
> 	they don't violate the DVB API;
> 	they don't create undocumented userspace API's;
> 	they are using the best development practices on kernel
> development;
> 	individual patches don't break compilation (to avoid
> breaking git bissect).
> 
> For a large changeset like Siano ones, such reviews take
> time.
> 
> Ah, next time, please number your changesets with something
> like [PATCH 01/xx].
> This helps me to properly identify and honour the correct
> patch order.
> 
> Cheers,
> Mauro

Hi Mauro,

Thanks for your detailed response.
Some comments and remarks -

Please note that the vast majority of the added (new files) code is SDIO and SPI bus interfaces, (and also big endian support). I don't know anyone reading this ML (Mike is included), who has the tools to test this code, which has been tested thoroughly. However, comments about coding style and kernel-coding related remarks are most welcome (I already submitted a patch for review to fix some endianity issue Trent Piepho commented about).
Please note that the SDIO patches has been written by none other than Pierre Ossman, who is the Linux kernel MMC maintainer (who wrote this code back in August 2008).

Siano has some dozens of commercial Linux-based customers using the discussed sources. Those customers have their own QA engineers additionally to Siano internal QA team (which includes dedicated engineer for this task). Some of those companies products are already in the market (production level). 

Second note, regarding regressions - the current code in the Mercurial tree includes code that Mike added without any review, which manipulate the chip-set's general purpose I/O. This code includes logical errors that causes boards with different layouts than Mike's to either deteriorate in performances (reception quality) or to stop functioning altogether. This issue got "blocker" status on my issues' list (the most severe type on my scale) and it prevents the various companies and individuals to use the Mercurial (and the kernel git) as a reliable source for Linux TV.

I am fully aware that Mike need to add/modify some code in order to support current/additional HPG cards (boards). All those modification are (1) Related to single source file (sms-cards.c) and (2) Can and should be done *after* submitting Siano's patches. (3) Pass a review, like any other patch, *before* submitting them to the main Mercurial tree.

Regarding "violate the DVB API" & "don't create undocumented userspace API's" - I guess you refers to the 'Siano Sub-system'. Well, .... this subject has been discussed a lot, and I don't mind at all to give it another round of discussions... 

DVB v3 never supported CMMB, T-DMB, ISDB-T and some other MDTV standards. DVB v5/S2 yet to support them. 
Those standards are used by ~2 Billion people. (yep, only little countries like Japan, China, S. Korea, Brazil, the entire Middle East and the Arab peninsula and "very few" others countries use these standards).... :-))))

Siano added some kernel support (tiny sub system) and user-space library.
The kernel code is of course GPLv2. 
*Everything* is available to *all*, include detailed documentation. You can see on the ML some recent posts about it (people who use it to get DAB streams with very little effort).

I strongly support any effort (and will help it myself) to have a single, unified, all-in-one API that will support all DTV standards. But we are not there, we are not even close :-(((

Until that time, when such generic, all-inclusive API will be introduced, we still have to support those "negligible" ~2 Billion people with some kind of additional API, *only* for those unsupported DTV standards.

Best regard,

Uri


      

Re: Siano's patches

[Barry Bouwsma]

(Oops, looks like I'm losing references, sorry)

Uri wrote...

>Siano added some kernel support (tiny sub system) and user-space library.
>The kernel code is of course GPLv2.
>*Everything* is available to *all*, include detailed documentation. You can see
> on the ML some recent posts about it (people who use it to get DAB streams wit
>h very little effort).

Of course, I'm assuming this user-space library is a substitute
for a full-fledged API, and exists to give end-users the
ability to at least make more use of their hardware than is
presently possible with the lack-of-API.

I'm happy to be able to tune DAB, though the library functionality
is limited and essentially it's like a computer-interface to an
ordinary desktop DAB radio, with the benefit of getting the actual
payload, or something resembling it (the original DAB error-
detection and -correction leaves much to be desired).  That
would be fine for the majority of users, I expect, but as you
noted, with this library it's not possible to get at the raw
ensemble data for whatever reason, study of the data within,
and the like.

So it seems this is somewhat like the `dabusb' source that
already exists for one specific receiver, neither of which
are a good basis for starting on an API, but which at least
bring something to the end-user who today, due to lack of a
common API to get access to DAB/DAB+ and the many other
widespread standards that Uri mentioned, but which, sadly,
so far few companies are producing hardware for, that can
be used in a generic Linux box, what a run-on sentence this
is, and I've forgotten my point already.


barry bouwsma
speaking of regressions, either someone broke the 44BSD
UFS filesystem in the latest kernel I'm working with, or
I've hosed more superblocks in my attempt to upgrade.
And I'd rather be spending my time trying to figure out
why I'm not getting the clean DAB reception I expect,
having eliminated some faulty hardware I haven't been
able to test until now...

Re: Siano's patches

[Trent Piepho]

On Mon, 19 Jan 2009, Uri Shkolnik wrote:
> Siano has some dozens of commercial Linux-based customers using the
> discussed sources.  Those customers have their own QA engineers
> additionally to Siano internal QA team (which includes dedicated engineer
> for this task).  Some of those companies products are already in the
> market (production level).

But how much testing do you give other manufacturers' hardware with your
code?  Your hardware might work, but you could have broken someone else's.

I've found that getting patches into the kernel is usually significantly
harder than writing them in the first place.

Re: Siano's patches

[Uri Shkolnik]

--- On Tue, 1/20/09, Trent Piepho <xyzzy@speakeasy.org> wrote:

> From: Trent Piepho <xyzzy@speakeasy.org>
> Subject: Re: Siano's patches
> To: "Uri Shkolnik" <urishk@yahoo.com>
> Cc: "Mauro Carvalho Chehab" <mchehab@infradead.org>, "Michael Krufky" <mkrufky@linuxtv.org>, linux-media@vger.kernel.org, "linux-dvb" <linux-dvb@linuxtv.org>
> Date: Tuesday, January 20, 2009, 3:49 AM
> On Mon, 19 Jan 2009, Uri Shkolnik wrote:
> > Siano has some dozens of commercial Linux-based
> customers using the
> > discussed sources.  Those customers have their own QA
> engineers
> > additionally to Siano internal QA team (which includes
> dedicated engineer
> > for this task).  Some of those companies products are
> already in the
> > market (production level).
> 
> But how much testing do you give other manufacturers'
> hardware with your
> code?  Your hardware might work, but you could have broken
> someone else's.
> 
> I've found that getting patches into the kernel is
> usually significantly
> harder than writing them in the first place.

mmm.... You have a good point here. I don't know, there is a kind of catch-22 here. True the code may break someone else', or violate something unknown to me. But, how can it be tested if you don't have suitable hardware?

Take the SPI as example. The code is for system with PXA CPU, which is connected with SPI/SPP bus to the SMS DTV chip-set. You need such (hw) device in order to check/test it. Who has this hardware but some manufacturers? Maybe someone bought & hacked such a commercial device, but I never got any indication that anyone did such a thing.

You can of course just compile the code and see if the build is completed successfully, without running it on a real target. This is some kind of code testing, and that is what I referred to as "kernel-coding related remarks" in my post. 

Uri 


      

Re: Siano's patches

[Mauro Carvalho Chehab]

Michael,

On Sun, 18 Jan 2009 13:07:34 -0500
Michael Krufky <mkrufky@linuxtv.org> wrote:

 
> Once I have reviewed & merged your changes and after I can restore the 
> proper functionality to the hauppauge devices, then I will post a new 
> mercurial tree for testing against all siano-based devices.
> 
> Please be patient -- this takes time.

Any news?


Cheers,
Mauro

Re: Siano's patches

[Michael Krufky]

Mauro Carvalho Chehab wrote:
> Michael,
>
> On Sun, 18 Jan 2009 13:07:34 -0500
> Michael Krufky <mkrufky@linuxtv.org> wrote:
>
>  
>   
>> Once I have reviewed & merged your changes and after I can restore the 
>> proper functionality to the hauppauge devices, then I will post a new 
>> mercurial tree for testing against all siano-based devices.
>>
>> Please be patient -- this takes time.
>>     
>
> Any news?

I haven't had time to fix the breakage that these changes caused on the 
Hauppauge devices.

This is what I will do -- I will gather all the patches up and push them 
all into one tree, regardless of any codingstyle issues or breakages, 
just so that we at least have track of everything.

Once it's all in one repository, I can just move the gpio function that 
the hauppauge devices depend on into the sms-cards.c file -- this way, 
they will not be used by other devices and the siano code can go in as 
desired by Uri.

The first tree that gets pushed up will likely need some cleanup, but 
we'll deal with that afterwards.

I saw some other interesting patches that Uri's been posting on the 
lists that I'd definitely like to merge along with these as soon as 
possible.

-Mike

Siano's patches

[Uri Shkolnik]

 Hi Mauro,
 
 
 Please note patches (1..19) series @ http://patchwork.kernel.org/project/linux-media/list/?submitter=uri
 
 I'll wait for reviews and commit for this series before submitting additional patches.
 
 
Pierre - the SDIO binding (registering) is still to be submitted, it isn't included within patches 1..19. 
 
 
 
Regards,
 
Uri
 


      

Re: Siano's patches

[Mauro Carvalho Chehab]

On Sun, 5 Apr 2009 07:19:59 -0700 (PDT)
Uri Shkolnik <urishk@yahoo.com> wrote:

> 
>  Hi Mauro,
>  
>  
>  Please note patches (1..19) series @ http://patchwork.kernel.org/project/linux-media/list/?submitter=uri
>  
>  I'll wait for reviews and commit for this series before submitting additional patches.

Uri,

I've finished reviewing and applying the patches. I had to skip some patches,
since they don't apply without some previous patches that I asked for changes.

Please re-submit those missing patches later.

I strongly suggest that you submit first all the patches that changes only
CodingStyle, and wait for my review before sending other patches (or otherwise
let them to happen only after merging all other patches).

In general, such patches won't generate discussions, provided
that checkpatch.pl doesn't complain, and that you manually review the results
of automatic tools like indent (that, in some cases, cause CodingStyle
regressions - such tool should be used with care).

The rationale is that patches that touches on CodingStyle replaces things on
almost everywhere. If a patch with CodingStyle changes got rejected by some
reason, the subsequent patches won't apply and will also be rejected.

Cheers,
Mauro