[PATCH 1/6] ASoC: split pxa ssp for reusing code

[PATCH 1/6] ASoC: split pxa ssp for reusing code

[Haojian Zhuang]

[PATCH 1/6] ASoC: split pxa ssp for reusing code

[Mark Brown]

On Fri, Mar 19, 2010 at 11:51:55AM -0400, Haojian Zhuang wrote:
> Subject: [PATCH] ASoC: split pxa ssp for reusing code

Again, please remember to CC ALSA patches to the ALSA list.  You should
also remember to CC Liam, my ASoC comaintainer.

> hw_params() isn't put in common ssp files since cpu_is_pxa3xx() is used. The
> cpu_is_pxa3xx() macro is only valid in ARCH_PXA and it's used to distinguish
> pxa2xx and pxa3xx. This macro is meaning less in other architectures. So keep
> this function in device specific driver.

Looking at the code it seems (like I mentioned before) it'd be much
better to factor this out so that the common code is in a library
function which both PXA and MMP variants can use it, with the PXA3xx
specific stuff calling the library function and overriding its behaviour
(eg, by overwriting the registers) where required.

Like I keep saying the SSP port configuration is really rather fragile
(in part due to the documentation issues) so forking this code seems
like it's going to be painful in the long run.

Some other stuff below...

> - * TODO:
> - *  o Test network mode for > 16bit sample size

Have you actually done this testing?

> -static void ssp_enable(struct ssp_device *ssp)
> +void ssp_enable(struct ssp_device *ssp)
>  {
>  	uint32_t sscr0;
> 

If this is being marked non-static presumably it should be exported too?
Similarly for most of the other functions.  Might be worth considering
the namespacing too and shoving some pxa_ or similar prefixes on there
if the function is going global, right now there's a mix of ssp_ and
pxa_ssp_ being exported.

> +/*
> + * pxa2xx-ssp.c  --  ALSA Soc Audio Layer
> + *

I'm not a big fan of the naming here since this also covers PXA3xx
devices.  Is there some general term for the non-MMP processors which
could be used - I'm not aware of one but I might've missed something?

> +enum {
> +	PXA2XX_DAI_SSP1,
> +	PXA2XX_DAI_SSP2,
> +	PXA2XX_DAI_SSP3,
> +	PXA2XX_DAI_SSP4,
> +	PXA2XX_DAI_SSP_MAX,
> +};

TBH I'd be much happier with #defines here - the use of an enum is
rather random given that none of the other defines use one and since the
particular values on this one really matter (as a result of being array
indexes) something that sets the values explicitly seems better.

[PATCH 1/6] ASoC: split pxa ssp for reusing code

[Haojian Zhuang]

On Sat, Mar 20, 2010 at 1:00 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Mar 19, 2010 at 11:51:55AM -0400, Haojian Zhuang wrote:
>> Subject: [PATCH] ASoC: split pxa ssp for reusing code
>
> Again, please remember to CC ALSA patches to the ALSA list. ?You should
> also remember to CC Liam, my ASoC comaintainer.
>
I'm sorry to forget it.

>> hw_params() isn't put in common ssp files since cpu_is_pxa3xx() is used. The
>> cpu_is_pxa3xx() macro is only valid in ARCH_PXA and it's used to distinguish
>> pxa2xx and pxa3xx. This macro is meaning less in other architectures. So keep
>> this function in device specific driver.
>
> Looking at the code it seems (like I mentioned before) it'd be much
> better to factor this out so that the common code is in a library
> function which both PXA and MMP variants can use it, with the PXA3xx
> specific stuff calling the library function and overriding its behaviour
> (eg, by overwriting the registers) where required.
>
> Like I keep saying the SSP port configuration is really rather fragile
> (in part due to the documentation issues) so forking this code seems
> like it's going to be painful in the long run.
>
I don't want to merge hw_params() between pxa2xx and pxa168 together.
Because I won't change pxa2xx function code. In my zylonite platform,
sound can't work with 2.6.34-rc1 kernel. If I change pxa2xx
functionality code, I can not verify it. I need to check why sound
can't work on zylonite first. I'm planning to follow this after this
change set.

> Some other stuff below...
>
>> - * TODO:
>> - * ?o Test network mode for > 16bit sample size
>
> Have you actually done this testing?
>
>> -static void ssp_enable(struct ssp_device *ssp)
>> +void ssp_enable(struct ssp_device *ssp)
>> ?{
>> ? ? ? uint32_t sscr0;
>>
>
> If this is being marked non-static presumably it should be exported too?
> Similarly for most of the other functions. ?Might be worth considering
> the namespacing too and shoving some pxa_ or similar prefixes on there
> if the function is going global, right now there's a mix of ssp_ and
> pxa_ssp_ being exported.
>
ok. I'll do the change.

>> +/*
>> + * pxa2xx-ssp.c ?-- ?ALSA Soc Audio Layer
>> + *
>
> I'm not a big fan of the naming here since this also covers PXA3xx
> devices. ?Is there some general term for the non-MMP processors which
> could be used - I'm not aware of one but I might've missed something?
>
pxa2xx is enough. Actually there's no general term for the non-MMP
processors. The silicon behavior on SSP is defferent one by one. In
PXA910, we will use SQU as DMA. In PXA168, we will use another clock
system. In PXA968, we will use ABU in SSP. The difference between
pxa2xx and pxa3xx is so small. PXA168 and PXA910 is MMP processors.
PXA968 is non-MMP processors.

>> +enum {
>> + ? ? PXA2XX_DAI_SSP1,
>> + ? ? PXA2XX_DAI_SSP2,
>> + ? ? PXA2XX_DAI_SSP3,
>> + ? ? PXA2XX_DAI_SSP4,
>> + ? ? PXA2XX_DAI_SSP_MAX,
>> +};
>
> TBH I'd be much happier with #defines here - the use of an enum is
> rather random given that none of the other defines use one and since the
> particular values on this one really matter (as a result of being array
> indexes) something that sets the values explicitly seems better.
>
ok. I'll change.

[PATCH 1/6] ASoC: split pxa ssp for reusing code

[Mark Brown]

On Mon, Mar 22, 2010 at 07:58:49AM -0400, Haojian Zhuang wrote:
> On Sat, Mar 20, 2010 at 1:00 PM, Mark Brown

> > Like I keep saying the SSP port configuration is really rather fragile
> > (in part due to the documentation issues) so forking this code seems
> > like it's going to be painful in the long run.

> I don't want to merge hw_params() between pxa2xx and pxa168 together.
> Because I won't change pxa2xx function code. In my zylonite platform,

Why not?  Like I say, this seems like a really retrograde step for long
term support - it means having more drivers to maintain.  The existing
PXA2xx code is very much a work in progress as it is and having to deal
with two copies of it doesn't fill me with great joy.

If there were some substantial difference in the hardware it'd be worth
forking but I'm just not seeing any such difference showing up in the
code for the actual port (as opposed to the clocks behind it).

> sound can't work with 2.6.34-rc1 kernel. If I change pxa2xx
> functionality code, I can not verify it. I need to check why sound
> can't work on zylonite first. I'm planning to follow this after this
> change set.

I'd test but unfortunately my Zylonite doesn't boot with mainline and
the patch I used to apply to get it to do so has bitrotted :(  In any
case, the key audio there is the AC97 which should be independant of the
SSP port anyway.

> >> +/*
> >> + * pxa2xx-ssp.c ?-- ?ALSA Soc Audio Layer
> >> + *

> > I'm not a big fan of the naming here since this also covers PXA3xx
> > devices. ?Is there some general term for the non-MMP processors which
> > could be used - I'm not aware of one but I might've missed something?

> pxa2xx is enough. Actually there's no general term for the non-MMP
> processors. The silicon behavior on SSP is defferent one by one. In
> PXA910, we will use SQU as DMA. In PXA168, we will use another clock
> system. In PXA968, we will use ABU in SSP. The difference between
> pxa2xx and pxa3xx is so small. PXA168 and PXA910 is MMP processors.
> PXA968 is non-MMP processors.

Feh, I guess.  Get your marketing department to make up some shiny
names for the different device classes :)