Re: [PATCH] asihpi update

Re: [PATCH] asihpi update

[Takashi Iwai]

At Fri, 18 Nov 2005 13:26:22 +1300,
Delio Brignoli wrote:
> 
> Hello Takashi,
> 
> this patch brings asihpi directory in alsa up to date with our internal 
> CVS repo. To be able to maintain ALSA reasonably up to date I've written 
> a preprocessor to strip most of the OS related code and generate asihpi 
> files automatically from a CVS workdir.

Well, I don't this it's good to go to this way.  We have imported the
stuff to CVS.  These codes have to be updated appropriately, too, not
generated at compile time.

> The code isn't as clean as the version you processed manually but I'm 
> happy to clean it further as long as I can automatically generate the 
> set of files. It would be great if you could apply the patch and send me 
> a list of must-change things.
> 
> my todo list for now is:
> 
> - automatically remove "HUGE".
> - rearrange HPI_OS_ #ifs to help the preprocessor strip
>     more linux unrelated code.

Please consider the following, too:

- Replacement of HW8/HW16/HW32 with u8/u16/u32 types.
  Replacement of PLONG, etc, too.

- Don't set EXTRA_CFLAGS.  They can be either in a local header for
  static definitions, or derived from Kconfig (CONFIG_XXX).

- No static hotplug directory in the kernel code.

- Proper indentation and spaces.  The indent is 8-chars in K&R style.
  Fold too long lines in 80 chars.
  See $KERNEL/Documentation/CodingStyle for details.
  If processing using a script, you can use "indent" program, too.


> diff -aurw asihpi-bak/asihpi.c asihpi/asihpi.c
> --- asihpi-bak/asihpi.c	2005-10-10 23:47:13.000000000 +1300
> +++ asihpi/asihpi.c	2005-11-18 13:18:32.000000000 +1300
> @@ -2186,7 +2186,7 @@
>  
>  	wVersion = asihpi->wVersion;
>  	snd_iprintf(buffer,
> -		    "Serial#=%d\nHw Version %c%d\nDSP code version %03d\n",
> +		    "Serial#=%ld\nHw Version %c%d\nDSP code version %03d\n",
>  		    asihpi->dwSerialNumber, ((wVersion >> 3) & 0xf) + 'A',
>  		    wVersion & 0x7,
>  		    ((wVersion >> 13) * 100) + ((wVersion >> 7) & 0x3f));

This change looks wrong.  dwSerialNumber is HW32, so it's fine with
%d.  %ld is for long type.


> @@ -2200,7 +2200,7 @@
>  			err += HPI_SampleClock_GetSource(phSubSys, hControl, &wSource);
>  
>  			if (!err)
> -					snd_iprintf(buffer, "SampleClock=%dHz, source %s\n", dwRate,
> +				snd_iprintf(buffer, "SampleClock=%ldHz, source %s\n", dwRate,
>  								sampleclock_sources[wSource]);
>  	} 
>  

Ditto.

> @@ -2288,7 +2288,7 @@
>  				 &asihpi->dwSerialNumber, &asihpi->wType);
>  	HPI_HandleError(err);
>  	wVersion = asihpi->wVersion;
> -	snd_printk(KERN_INFO "Adapter ID=%4X Index=%d NumOutStreams=%d NumInStreams=%d S/N=%d\n" "Hw Version %c%d DSP code version %03d\n", asihpi->wType, asihpi->wAdapterIndex, asihpi->wNumOutStreams, asihpi->wNumInStreams, asihpi->dwSerialNumber, ((wVersion >> 3) & 0xf) + 'A',	// Hw version major
> +	snd_printk(KERN_INFO "Adapter ID=%4X Index=%d NumOutStreams=%d NumInStreams=%d S/N=%ld\n" "Hw Version %c%d DSP code version %03d\n", asihpi->wType, asihpi->wAdapterIndex, asihpi->wNumOutStreams, asihpi->wNumInStreams, asihpi->dwSerialNumber, ((wVersion >> 3) & 0xf) + 'A',	// Hw version major
>  		   wVersion & 0x7,	// Hw version minor
>  		   ((wVersion >> 13) * 100) + ((wVersion >> 7) & 0x3f)	// DSP code version
>  	    );

Ditto.


> diff -aurw asihpi-bak/boot4ka.h asihpi/boot4ka.h
> --- asihpi-bak/boot4ka.h	2005-08-26 05:51:34.000000000 +1200
> +++ asihpi/boot4ka.h	2005-11-18 13:18:32.000000000 +1300
> @@ -20,33 +20,33 @@
>  ******************************************************************************/
>  // The following code came from boot4000.s.CLD 
>  
> -#include "hpios.h"        // for defn. 
> +#include <hpios.h>        // for HFAR defn. 

We shouldn't use <> for local headers.


> -static u32 adwDspCode_Boot4000a[] =
> +static unsigned long HFAR adwDspCode_Boot4000a[] =

u32 is not unsigned long.
long can (but not always) be 64 bit.


> -// static HW32 * adwDspCode_Boot4000Arrays[]={&adwDspCode_Boot4000a[0]};
> +// static HW32 HFAR * adwDspCode_Boot4000Arrays[]={&adwDspCode_Boot4000a[0]};

Yeah, HFAR and HUGE are evil...


Thanks,

Takashi


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Delio Brignoli]

Takashi Iwai wrote:
> At Fri, 18 Nov 2005 13:26:22 +1300,
> Delio Brignoli wrote:
> 
>>Hello Takashi,
>>
>>this patch brings asihpi directory in alsa up to date with our internal 
>>CVS repo. To be able to maintain ALSA reasonably up to date I've written 
>>a preprocessor to strip most of the OS related code and generate asihpi 
>>files automatically from a CVS workdir.
> 
> 
> Well, I don't this it's good to go to this way.  We have imported the
> stuff to CVS.  These codes have to be updated appropriately, too, not
> generated at compile time.

I think there is a misunderstanding, we do not plan to force people to 
generate the ALSA source at compile time BUT we need to be able to 
generate the source code for ALSA from our CVS automatically.
Once this patch (or a cleaner version of it) is accepted, we are going 
to send only differences. Is that ok?

> Please consider the following, too:
> 
> - Replacement of HW8/HW16/HW32 with u8/u16/u32 types.
>   Replacement of PLONG, etc, too.
> 
> - Don't set EXTRA_CFLAGS.  They can be either in a local header for
>   static definitions, or derived from Kconfig (CONFIG_XXX).
> 
> - No static hotplug directory in the kernel code.
> 
> - Proper indentation and spaces.  The indent is 8-chars in K&R style.
>   Fold too long lines in 80 chars.
>   See $KERNEL/Documentation/CodingStyle for details.
>   If processing using a script, you can use "indent" program, too.

I'll make these changes.

> This change looks wrong.  dwSerialNumber is HW32, so it's fine with
> %d.  %ld is for long type.

which version of gcc is the 'default' one for compiling ALSA?
I get a warning for using %d with HW32, I'll double check.

>>-#include "hpios.h"        // for defn. 
>>+#include <hpios.h>        // for HFAR defn. 
> 
> 
> We shouldn't use <> for local headers.

fixed this already.

thanks
--
Delio


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Takashi Iwai]

Hi Delio,

At Sat, 19 Nov 2005 00:58:37 +1300,
Delio Brignoli wrote:
> 
> Takashi Iwai wrote:
> > At Fri, 18 Nov 2005 13:26:22 +1300,
> > Delio Brignoli wrote:
> > 
> >>Hello Takashi,
> >>
> >>this patch brings asihpi directory in alsa up to date with our internal 
> >>CVS repo. To be able to maintain ALSA reasonably up to date I've written 
> >>a preprocessor to strip most of the OS related code and generate asihpi 
> >>files automatically from a CVS workdir.
> > 
> > 
> > Well, I don't this it's good to go to this way.  We have imported the
> > stuff to CVS.  These codes have to be updated appropriately, too, not
> > generated at compile time.
> 
> I think there is a misunderstanding, we do not plan to force people to 
> generate the ALSA source at compile time BUT we need to be able to 
> generate the source code for ALSA from our CVS automatically.
> Once this patch (or a cleaner version of it) is accepted, we are going 
> to send only differences. Is that ok?

OK, my concern was the change of alsa-driver/asihpi/Makefile in your
last patch.  You added the stuff to generate at compile time there.
I'm not against at all to process it _manually_ on your side.

> 
> > Please consider the following, too:
> > 
> > - Replacement of HW8/HW16/HW32 with u8/u16/u32 types.
> >   Replacement of PLONG, etc, too.
> > 
> > - Don't set EXTRA_CFLAGS.  They can be either in a local header for
> >   static definitions, or derived from Kconfig (CONFIG_XXX).
> > 
> > - No static hotplug directory in the kernel code.
> > 
> > - Proper indentation and spaces.  The indent is 8-chars in K&R style.
> >   Fold too long lines in 80 chars.
> >   See $KERNEL/Documentation/CodingStyle for details.
> >   If processing using a script, you can use "indent" program, too.
> 
> I'll make these changes.
> 
> > This change looks wrong.  dwSerialNumber is HW32, so it's fine with
> > %d.  %ld is for long type.
> 
> which version of gcc is the 'default' one for compiling ALSA?
> I get a warning for using %d with HW32, I'll double check.

Ah, then it might be my fix in alsa-driver tree.
HW32 must be u32 (equivalent with unsigned int) on linux kernel.
Using long for 32bit is wrong.


> >>-#include "hpios.h"        // for defn. 
> >>+#include <hpios.h>        // for HFAR defn. 
> > 
> > 
> > We shouldn't use <> for local headers.
> 
> fixed this already.


Thanks,

Takashi


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Delio Brignoli]

Hello Takashi,

Takashi Iwai wrote:
> OK, my concern was the change of alsa-driver/asihpi/Makefile in your
> last patch.  You added the stuff to generate at compile time there.
> I'm not against at all to process it _manually_ on your side.

The extra rules in the makefile are to be used by us internally to 
prepare dist files (and are only triggered when source files are 
missing) or by customers that want to compile ALSA with bleeding edge 
code from our CVS rep.

I'm not 100% sure about what you mean with "_manually_".
if you mean that: I generate code for an ALSA dist from our CVS, then 
diff, send the patch to alsa-devel. Then it's fine. :)

> Ah, then it might be my fix in alsa-driver tree.
> HW32 must be u32 (equivalent with unsigned int) on linux kernel.
> Using long for 32bit is wrong.

I'll look into this one too.

thanks
--
Delio


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Takashi Iwai]

At Sat, 19 Nov 2005 01:21:37 +1300,
Delio Brignoli wrote:
> 
> 
> Hello Takashi,
> 
> Takashi Iwai wrote:
> > OK, my concern was the change of alsa-driver/asihpi/Makefile in your
> > last patch.  You added the stuff to generate at compile time there.
> > I'm not against at all to process it _manually_ on your side.
> 
> The extra rules in the makefile are to be used by us internally to 
> prepare dist files (and are only triggered when source files are 
> missing) or by customers that want to compile ALSA with bleeding edge 
> code from our CVS rep.

Then please provide a separate script to do that, instead of merging
to Makefile.  (In addition, embedding the directory path is not good,
too.  You can mangle it via configure script if necessary.)

> I'm not 100% sure about what you mean with "_manually_".
> if you mean that: I generate code for an ALSA dist from our CVS, then 
> diff, send the patch to alsa-devel. Then it's fine. :)

Yes, I meant it exactly.  Basically, the alsa-driver tree should be
self-consistent except for kernel source and chain tools.

If you need to update the driver codes, just give us the patches.
Of course, you can put a script to generate such cdes for help to
someone else into the public tree, but it shouldn't be called
unconditionally.


thanks,

Takashi


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Delio Brignoli]

Hello takashi,

Takashi Iwai wrote:
>>>Please consider the following, too:
>>>
>>>- Replacement of HW8/HW16/HW32 with u8/u16/u32 types.

I've updated my script to substitute HW8/16/32 with u8/16/32 types, it 
will result in a very big patch against your current dist, are you still 
happy with that?

thanks
--
Delio




-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click

Re: Re: [PATCH] asihpi update

[Takashi Iwai]

At Tue, 22 Nov 2005 14:22:14 +1300,
Delio Brignoli wrote:
> 
> Hello takashi,
> 
> Takashi Iwai wrote:
> >>>Please consider the following, too:
> >>>
> >>>- Replacement of HW8/HW16/HW32 with u8/u16/u32 types.
> 
> I've updated my script to substitute HW8/16/32 with u8/16/32 types, it 
> will result in a very big patch against your current dist, are you still 
> happy with that?

The size of the patch doesn't matter.
Sooner or later we need to fix these things.


thanks,

Takashi


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click