Revised patches for PCM Controller driver

Revised patches for PCM Controller driver

[jassisinghbrar]

From: Jassi Brar <jassi.brar@samsung.com>

Acting upon the inputs given by Mark and Ben, I have revised the code.
A few points to be noted:-

1) The prefix s3c24xx_pcm_ in the platform driver has been changed to
   more neutral s3c_audio_

2) ALSA platform driver s3c24xx-pcm.c/h have been renamed s3c-audio.c/h
   since the 'pcm' part will cause ambiguity once PCM Controller driver
   is added. Also, since it is not just for 24xx, the part is dropped
   from the prefix.
   Ofcourse, evey dependent code has been modified to include differently
   named, otherwise same, header s3c-audio.h

3) arch/arm/plat-s3c/include/plat/audio.h has been restored by with only
   necessary data structures.
   Having callbacks to configure controller pins appropriately is necessary
   if the driver is to handle more than one SoC type.
   Currently only callback to configure gpios has been defined, the data
   structure will grow as and when needed.

4) The PCM controller platform devices have been defined in the apparently
   common arch/arm/plat-s3c64xx/dev-audio.c rather than a new PCM specific one.

5) Here comes the tricky one.
   Breaking away from S3C convention, I have defined PCM controller register
   offsets and bit fields in sound/soc/s3c24xx/s3c-pcm.h instead of some
   platform/arch specific header.
   The reason for the move is that usually the device controllers depend upon
   platform type only as far as their base mapping address goes. Otherwise
   just one or two 'types' of same devices serve most SoCs.
   Having those definitions besides the driver helps avoid copyng the same
   definitions for each platform that essentially have the same device controller.

Regards.
   

Re: Revised patches for PCM Controller driver

[Ben Dooks]

On Wed, Nov 04, 2009 at 05:27:44PM +0900, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jassi.brar@samsung.com>
> 
> Acting upon the inputs given by Mark and Ben, I have revised the code.
> A few points to be noted:-
> 
> 1) The prefix s3c24xx_pcm_ in the platform driver has been changed to
>    more neutral s3c_audio_

Not a fan of renaming, but I suppose this has some merit. I'll make
some comments about this in the series, I think life could be made
enater if some of these are cleaned up.

It may be worth opening a discussion on the alsa list about renaming the
entire directory to samsung instead of s3c.
 
> 2) ALSA platform driver s3c24xx-pcm.c/h have been renamed s3c-audio.c/h
>    since the 'pcm' part will cause ambiguity once PCM Controller driver
>    is added. Also, since it is not just for 24xx, the part is dropped
>    from the prefix.
>    Ofcourse, evey dependent code has been modified to include differently
>    named, otherwise same, header s3c-audio.h

ok.
 
> 3) arch/arm/plat-s3c/include/plat/audio.h has been restored by with only
>    necessary data structures.
>    Having callbacks to configure controller pins appropriately is necessary
>    if the driver is to handle more than one SoC type.
>    Currently only callback to configure gpios has been defined, the data
>    structure will grow as and when needed.

This seems ok, the old one wasn't being used.
 
> 4) The PCM controller platform devices have been defined in the apparently
>    common arch/arm/plat-s3c64xx/dev-audio.c rather than a new PCM specific one.

This'll be shaken up by my dev changes... will try and take into account
of these patches before this change is done.
 
> 5) Here comes the tricky one.
>    Breaking away from S3C convention, I have defined PCM controller register
>    offsets and bit fields in sound/soc/s3c24xx/s3c-pcm.h instead of some
>    platform/arch specific header.
>    The reason for the move is that usually the device controllers depend upon
>    platform type only as far as their base mapping address goes. Otherwise
>    just one or two 'types' of same devices serve most SoCs.
>    Having those definitions besides the driver helps avoid copyng the same
>    definitions for each platform that essentially have the same device controller.

I'll have a look at that one.

Can you ensure that you send patches with rename detection enabled, it
would make it easier working out what has been changed.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: Revised patches for PCM Controller driver

[Mark Brown]

On Wed, Nov 04, 2009 at 05:27:44PM +0900, jassisinghbrar@gmail.com wrote:

> Acting upon the inputs given by Mark and Ben, I have revised the code.
> A few points to be noted:-

Please just post this stuff upstream, I'll review it fully there.

Unless the code is so awful it can clearly never be accepted upstream
without major work (which is not the case here) not submitting upstream
at the earliest opportunity is likely to slow things down, either
through the wait caused by an extra round of upstream submission or
through missing key subsystem specific feedback (as happened with the
IDE code, where several rounds of updates were done on code that would
never be accepted upstream due to using the old IDE subsystem).

> 1) The prefix s3c24xx_pcm_ in the platform driver has been changed to
>    more neutral s3c_audio_

Please use DMA or something else which indicates which part of the audio
subsystem it's driving.

Re: Revised patches for PCM Controller driver

[jassi brar]

On Wed, Nov 4, 2009 at 8:05 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Nov 04, 2009 at 05:27:44PM +0900, jassisinghbrar@gmail.com wrote:
>
>> Acting upon the inputs given by Mark and Ben, I have revised the code.
>> A few points to be noted:-
>
> Please just post this stuff upstream, I'll review it fully there.
>
> Unless the code is so awful it can clearly never be accepted upstream
> without major work (which is not the case here) not submitting upstream
> at the earliest opportunity is likely to slow things down, either
> through the wait caused by an extra round of upstream submission or
> through missing key subsystem specific feedback (as happened with the
> IDE code, where several rounds of updates were done on code that would
> never be accepted upstream due to using the old IDE subsystem).

One of the purposes of this list was to catch trivial mistakes in the
patches before they are sent upstream for maintainer review. That should help
save maintainers time and keep them 'receptive' to samsung's patches.
Second purpose being peer review withing samsung.


>> 1) The prefix s3c24xx_pcm_ in the platform driver has been changed to
>>    more neutral s3c_audio_
>
> Please use DMA or something else which indicates which part of the audio
> subsystem it's driving.
okay, will call it s3c_dma_

Re: Revised patches for PCM Controller driver

[jassi brar]

On Wed, Nov 4, 2009 at 6:33 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> Can you ensure that you send patches with rename detection enabled, it
> would make it easier working out what has been changed.
if you can, please have a cursory look at patches except those of renaming

tomorrow i will submit in alsa-devel accordingly. thanks

Re: Revised patches for PCM Controller driver

[Mark Brown]

On Wed, Nov 04, 2009 at 09:14:45PM +0900, jassi brar wrote:

> One of the purposes of this list was to catch trivial mistakes in the
> patches before they are sent upstream for maintainer review. That should help
> save maintainers time and keep them 'receptive' to samsung's patches.

Speaking as one of these upstreams it's not surprising to see issues in
submissions, especially where the drivers are from people who are not
regular contributors to the subsystem.  Like I say, there are some
things where a lot of cleanup is required where not posting does make
sense but much of the code that is being posted is of a perfectly
reasonable standard.  For example, even with posting initially to this
list the first response wasn't raising fundamental issues so there
should be no reason not to have moved upstream with the second round.

Some of the other SoCs have followed an approach similar to this one -
it tends to cause bad experiences with code never getting merged and
with developers becoming reluctant to work with the general kernel
community since they become used to the platform-specific environment
which gives limited subsystem-specific feedback.

> Second purpose being peer review withing samsung.

Sure, and it does make sense to also send the patches to this list when
submitting upstream - it's also useful for people outside Samsung to
keep up to speed on all the latest kernel support for their boards.  I
think it's a really good idea to have the list and keep it in the loop
like that.

When to post patches to this list or upstream

[Harald Welte]

Dear all,

On Wed, Nov 04, 2009 at 02:03:11PM +0000, Mark Brown wrote:
 
> > One of the purposes of this list was to catch trivial mistakes in the
> > patches before they are sent upstream for maintainer review. That should help
> > save maintainers time and keep them 'receptive' to samsung's patches.
> 
> Speaking as one of these upstreams it's not surprising to see issues in
> submissions, especially where the drivers are from people who are not
> regular contributors to the subsystem.  Like I say, there are some
> things where a lot of cleanup is required where not posting does make
> sense but much of the code that is being posted is of a perfectly
> reasonable standard.  For example, even with posting initially to this
> list the first response wasn't raising fundamental issues so there
> should be no reason not to have moved upstream with the second round.

I agree.  So we should define the following rule:

* trivial fixes such as a single-line bugfix should always go upstram
  directly

* entirely new patchsets / drivers should first be sent to this list,
  receive some comment/feedback.  Second and further submission should be
  upstream

After some more time / experience, the quality of the patches should be higher
and thus we can eliminate the need to first post to this list altogether.

Regards,
	Harald.
-- 
- Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)