At Tue, 02 Oct 2007 20:17:52 +0200, Clemens Ladisch wrote:

sh_okada@d4.dion.ne.jp wrote

...

I wrote a driver for the SE-200PCI and SE-90PCI what are salling in Japan. And I want to merge it to current source tree. How should I do?

Post the patch here. The developers will then suggest enhancements and/or apply it. (Well, they will when they come back from vacation.)

At least, one developer is available now :)

So, don't hesitate to post a patch (or mail it directly if you prefer, before publishing it).

thanks,

Takashi

At Tue, 09 Oct 2007 20:57:58 +0900, sh_okada@d4.dion.ne.jp wrote:

It tries by way of experiment by this method though how to put up Res is not understood.

You can simply paste the patch in a mail. Some MUA cannot pass the text as is, and it would break the embedded patch. In such a case, use an attachment.

I put the patch on my HP http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch.tgz Please take it here.

Thanks, I took a look at it. The code itself seems almost OK.

But, before merging, please fix the coding style to follow the standard style. Read $LINUX_KERNEL/Documentation/CodingStyle file first. This describes many details. In doubt, run scripts/checkpatch.pl script included in the recent linux kernel tree. It's a really picky guy, so you don't have to obey every detail :)

Second, the mixer control names should follow standard names (somehow). For example, in your code,

{"Front(green) Playback Volume" ,WM8776output ,VOLUME1,0,NULL} ,{"Surround(orange) Playback Volume" ,WM8766 ,VOLUME1,1,NULL} ,{"SurroundBack(white) Playback Volume" ,WM8766 ,VOLUME1,2,NULL} ,{"CenterAndSW(black) Playback Volume" ,WM8766 ,VOLUME1,0,NULL}

we have standard names: Front Playback Volume, Surround Playback Volume, Side Playback Volume, CLFE Playback Volume. If Center and LFE can be split, it'd be much better to have two mono controls, Center Playback Volume and LFE Playback Volume.

thanks,

Takashi

At Wed, 10 Oct 2007 21:49:37 +0900, sh_okada@d4.dion.ne.jp wrote:

...

Second, the mixer control names should follow standard names (somehow). For example, in your code, {"Front(green) Playback Volume" ,WM8776output ,VOLUME1,0,NULL} ,{"Surround(orange) Playback Volume" ,WM8766 ,VOLUME1,1,NULL} ,{"SurroundBack(white) Playback Volume" ,WM8766 ,VOLUME1,2,NULL } ,{"CenterAndSW(black) Playback Volume" ,WM8766 ,VOLUME1,0,NULL} we have standard names: Front Playback Volume, Surround Playback Volume, Side Playback Volume, CLFE Playback Volume. If Center and LFE can be split, it'd be much better to have two mono controls, Center Playback Volume and LFE Playback Volume.

And I want to rewrite as this.

But in Japan, How to call is different. the speaker put the side of the seat position say SurroundSpeaker and the speaker put the back say BackSpeaker. When comparing it Side <-> SurroundSpeaker Surround <-> BackSpeaker

The definition of "Surround" in ALSA mixer is the primary surround channels, and it usually corresponds to Surround Rear (Back). Thus your mapping above looks correct.

The twist phenomenon has occurred. What shall we do?

Though it is a proposal, How about saying that the comment area will be added to the control name? Like "Surround(orange) Playback Volume", and When it is necessary to match the pattern, the area of parentheses is deleted.

This simply breaks the existing rule, and present mixer apps won't be able to parse the name as is. The mixer name string is used not for an ID but as a component role. If we inevitablly need a new attribute such as jack colors, let's use TLV information, for example.

IMO, this is a pure option. The life goes well without such information. So, let's implement the code in the usual style, then add extra information to give improvements.

Thanks,

Takashi

At Wed, 10 Oct 2007 23:57:16 +0900, sh_okada@d4.dion.ne.jp wrote:

A new code was put on my HP. http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-2.tgz

Thanks!

The change is as follows. 1 The coding style

Well, the current code still doesn't follow properly. Below are some nitpickings.

static void se200pci_WM8766_write(struct snd_ice1712 *ice, unsigned int addr,unsigned int data)

You need a space after comma (,)

st = (((addr&0x7f)<<9)|(data&0x1ff));

Ditto for each operator.

for (i=0 ; i<16 ; i++) {

Spaces around '=' and '<', no space before ';'

if (st & 0x10000) bits |= DATA; else bits &= ~DATA;

Break lines for each statement, namely

if (st & 0x10000)
	bits |= DATA;
else
	bits &= ~DATA;

And,

if (ch == 0) { se200pci_WM8766_write(ice, 0x000, vol1); se200pci_WM8766_write(ice, 0x001, vol2|0x100); } else if (ch == 1) {

Put 'else if...' with the same line as the close brace,

} else if (ch == 1) {

But in this case, it's better to use switch, or a value integer array. Ditto for se200pci_WM8776_set_input_selector().

if (rate > 96000) se200pci_WM8766_write(ice, 0x0a, 0x000); /* MCLK=128fs */ else se200pci_WM8766_write(ice, 0x0a, 0x080); /* MCLK=256fs */

Break the line of 'else'.

struct { char *name; enum { WM8766, WM8776in, WM8776out, WM8776sel, WM8776agc, WM8776afl } target; enum { VOLUME1,VOLUME2,BOOLEAN,ENUM } type; int ch; char **member; char *comment; } se200pci_cont[]={

Missing static here. Also the above definition still doesn't follow the coding style well. The enum should be defined outside the struct as its scope is always global in C (unlike C++).

{"Front Playback Volume", WM8776out,VOLUME1,0,NULL,"Front(green)"},

Use C99 initialization style, such as

{ .name = "Front Playback Volume", .target = WM8776out, .type = VOLUME1, .ch = 0, .comment = "Front(green)" }

Note that 0 and NULL can be omitted in initializations if you want (as I did for member in the above example).

se200pci_cont_info() should use switch instead of if..else.

for (c=0 ; c<100 ; c++) {
	if (member[c] == NULL) break;
}

First, coding style error, and second, a magic number 100 appears here suddenly.

In se200pci_WM8766_write(),

unsigned int DATA = 0x010000; unsigned int CLOCK = 0x020000; unsigned int LOAD = 0x040000; unsigned int ALL_MASK = (DATA|CLOCK|LOAD);

Apparently they should be defined as const (or use define).

You don't need braces around a single-line if, for example,

if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) { /* nothing to do */ } else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) { se200pci_add_controls(ice); }

should be

if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) /* nothing to do */ else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) se200pci_add_controls(ice);

Well, let's stop here.

2 The name of the control

Now looks better. One remaining thing is you're using an enum control for "ACG Capture Switch". If it's a switch, you should use a boolean type.

thanks,

Takashi

At Sat, 13 Oct 2007 00:05:43 +0900, sh_okada@d4.dion.ne.jp wrote:

...

Well, the current code still doesn't follow properly. Below are some nitpickings.

Thank you for a detailed check. A new one put my HP http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-3.tgz

Thanks, I'll take a look later.

...

You don't need braces around a single-line if, for example,

...

if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) { /* nothing to do */ } else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) { se200pci_add_controls(ice); }

should be

...

if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE90PCI) /* nothing to do */ else if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) se200pci_add_controls(ice);

This becomes a compile error, Because when omitting it

if (A) else if (B) func();

It needs ';' or '{}' like if (A) ; else if (B) func(); or if (A) {} else if (B) func();

Therefore, "/* nothing to do */" cannot be written by this style. When forcibly writing It becomes tricky.

if (A) /* nothing to do */; else if (B) func();

or

if (A) { /* nothing to do */ } else if (B) func();

So I wrote as

/* nothing to do for VT1724_SUBDEVICE_SE90PCI */ if (ice->eeprom.subvendor == VT1724_SUBDEVICE_SE200PCI) err = se200pci_add_controls(ice);

Yeah, that's better.

...

...

2 The name of the control

Now looks better. One remaining thing is you're using an enum control for "ACG Capture Switch". If it's a switch, you should use a boolean type.

AGC has some modes, but now coded one mode. It makes it to such a style, Because it is likely to be going to increase it in the future.

now OFF, ON(type=1) future OFF, ON(type=1), ON(type=2),...

Then let's rename it to another one, such as, 'AGC Capture Mode'. Also, a value like "ON(xxx)" look ugly. I'd suggest items like 'Off', 'Type1', 'Type2'.

Takashi

Hi

A new one put my HP http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-4.tgz

Then let's rename it to another one, such as, 'AGC Capture Mode'. Also, a value like "ON(xxx)" look ugly. I'd suggest items like 'Off', 'Type1', 'Type2'.

It changed from "AGC Capture Switch" to "AGC Capture Mode" and changed the mode name "ON(type=1)" to "LimiterMode" and added a new mode "ALCMode".

As a result the mode is "Off", "LimiterMode", "ALCMode".

Thanks

At Wed, 17 Oct 2007 09:32:18 +0000 (UTC), Daniel Mayer wrote:

Just a question: You said the 8740 is hardwired on the board, so is the "Selectable digital low pass filter" controllable by any means?

If the codec chip supports it, yes, should be feasible...

Takashi

At Tue, 16 Oct 2007 21:32:53 +0900, sh_okada@d4.dion.ne.jp wrote:

...

Hi

A new one put my HP http://www.d4.dion.ne.jp/~sh_okada/misc/alsa-SE200PCI.patch-4.tgz

...

Then let's rename it to another one, such as, 'AGC Capture Mode'. Also, a value like "ON(xxx)" look ugly. I'd suggest items like 'Off', 'Type1', 'Type2'.

It changed from "AGC Capture Switch" to "AGC Capture Mode" and changed the mode name "ON(type=1)" to "LimiterMode" and added a new mode "ALCMode".

As a result the mode is "Off", "LimiterMode", "ALCMode".

FYI, the patch is now merged to ALSA HG tree (with slight modifications and fixes). Please check whether it works for you.

All analog outputs, all analog inputs with selector and the AFL path switch works well.

But though it forgot to say, because I do not have a digital device, I doesn't confirm it. However, it is said on HP that it works by modprobe snd-ice1724 model=prodigy71 So I selected prodigy71's mode, and I think that it moves perhaps.

Thanks