headers
John L. Utz III | 20 Aug 02:14
Favicon

PATCH - vt1618 7.1 Audio

Signed-off-by: John L. Utz III john.utz <at> dmx.com

diff --git a/pci/ac97/ac97_codec.c b/pci/ac97/ac97_codec.c
index d0023e9..a34f1ea 100644
--- a/pci/ac97/ac97_codec.c
+++ b/pci/ac97/ac97_codec.c
@@ -168,7 +168,7 @@ static const struct ac97_codec_id snd_ac97_codec_ids[]  
= {
  { 0x54584e20, 0xffffffff, "TLC320AD9xC",	NULL,		NULL },
  { 0x56494161, 0xffffffff, "VIA1612A",		NULL,		NULL }, // modified ICE1232  
with S/PDIF
  { 0x56494170, 0xffffffff, "VIA1617A",		patch_vt1617a,	NULL }, // modified  
VT1616 with S/PDIF
-{ 0x56494182, 0xffffffff, "VIA1618",		NULL,		NULL },
+{ 0x56494182, 0xffffffff, "VIA1618",		patch_vt1618,   NULL }, // clean  
sheet of crinkled paper
  { 0x57454301, 0xffffffff, "W83971D",		NULL,		NULL },
  { 0x574d4c00, 0xffffffff, "WM9701,WM9701A",	NULL,		NULL },
  { 0x574d4C03, 0xffffffff, "WM9703,WM9707,WM9708,WM9717", patch_wolfson03,  
NULL},
@@ -609,7 +609,6 @@ AC97_SINGLE("PC Speaker Playback Volume",  
AC97_PC_BEEP, 1, 15, 1)
  static const struct snd_kcontrol_new snd_ac97_controls_mic_boost =
  	AC97_SINGLE("Mic Boost (+20dB)", AC97_MIC, 6, 1, 0);

-
  static const char* std_rec_sel[] = {"Mic", "CD", "Video", "Aux", "Line",  
"Mix", "Mix Mono", "Phone"};
  static const char* std_3d_path[] = {"pre 3D", "post 3D"};
  static const char* std_mix[] = {"Mix", "Mic"};
(Continue reading)

Permalink | Reply |
headers
Takashi Iwai | 20 Aug 10:28
Favicon

Re: PATCH - vt1618 7.1 Audio

At Tue, 19 Aug 2008 17:17:44 -0700,
John L. Utz III wrote:
> 
> Signed-off-by: John L. Utz III john.utz <at> dmx.com

Thanks for the patch.

First off, could you give a brief description what this patch
does/fixes exactly?

Below is a quick review.

> diff --git a/pci/ac97/ac97_codec.c b/pci/ac97/ac97_codec.c
> index d0023e9..a34f1ea 100644
> --- a/pci/ac97/ac97_codec.c
> +++ b/pci/ac97/ac97_codec.c
> @@ -168,7 +168,7 @@ static const struct ac97_codec_id snd_ac97_codec_ids[]  
> = {
>   { 0x54584e20, 0xffffffff, "TLC320AD9xC",	NULL,		NULL },
>   { 0x56494161, 0xffffffff, "VIA1612A",		NULL,		NULL }, // modified ICE1232  
> with S/PDIF
>   { 0x56494170, 0xffffffff, "VIA1617A",		patch_vt1617a,	NULL }, // modified  
> VT1616 with S/PDIF
> -{ 0x56494182, 0xffffffff, "VIA1618",		NULL,		NULL },
> +{ 0x56494182, 0xffffffff, "VIA1618",		patch_vt1618,   NULL }, // clean  

Please avoid C++ comments.  Yeah, we see it in other places here and
there, but you don't need to introduce more in the newly added code.

> +
(Continue reading)

Permalink | Reply |
headers
John L. Utz III | 20 Aug 19:50
Favicon

Re: PATCH - vt1618 7.1 Audio

Hi Takashi;

tnx for the review, a few questions inline.

On Wed, 20 Aug 2008 01:28:39 -0700, Takashi Iwai <tiwai <at> suse.de> wrote:

> At Tue, 19 Aug 2008 17:17:44 -0700,
> John L. Utz III wrote:
>>
>> Signed-off-by: John L. Utz III john.utz <at> dmx.com
>
> Thanks for the patch.
>
> First off, could you give a brief description what this patch
> does/fixes exactly?
>
> Below is a quick review.
>
>> diff --git a/pci/ac97/ac97_codec.c b/pci/ac97/ac97_codec.c
>> index d0023e9..a34f1ea 100644
>> --- a/pci/ac97/ac97_codec.c
>> +++ b/pci/ac97/ac97_codec.c
>> @@ -168,7 +168,7 @@ static const struct ac97_codec_id  
>> snd_ac97_codec_ids[]
>> = {
>>   { 0x54584e20, 0xffffffff, "TLC320AD9xC",	NULL,		NULL },
>>   { 0x56494161, 0xffffffff, "VIA1612A",		NULL,		NULL }, // modified  
>> ICE1232
>> with S/PDIF
>>   { 0x56494170, 0xffffffff, "VIA1617A",		patch_vt1617a,	NULL }, //
(Continue reading)

Permalink | Reply |
headers
Rene Herman | 20 Aug 21:05

Re: PATCH - vt1618 7.1 Audio

On 20-08-08 19:50, John L. Utz III wrote:

> On Wed, 20 Aug 2008 01:28:39 -0700, Takashi Iwai <tiwai <at> suse.de> wrote:

>> Also, a Windows style variable name should be avoided.  People tend to
>> hate it.
> 
> Why would you call this 'Windows Style?' Is that supposed to be a  
> perjorative comment?

You insult really quickly... :-)

The style is more generally known as Hungarian Notation after its 
Hungarian "inventor" who was a chief architect at Microsoft. This 
notational conventation saw widespread use in the Windows API and not 
much use anywhere beyond that.

Rene.
Permalink | Reply |
headers
John L. Utz III | 20 Aug 21:16
Favicon

Re: PATCH - vt1618 7.1 Audio

On Wed, 20 Aug 2008 12:05:33 -0700, Rene Herman <rene.herman <at> keyaccess.nl>  
wrote:

> On 20-08-08 19:50, John L. Utz III wrote:
>
>> On Wed, 20 Aug 2008 01:28:39 -0700, Takashi Iwai <tiwai <at> suse.de> wrote:
>
>>> Also, a Windows style variable name should be avoided.  People tend to
>>> hate it.
>>  Why would you call this 'Windows Style?' Is that supposed to be a   
>> perjorative comment?
>
> You insult really quickly... :-)

Quickly, but not deeply. :-)

>
> The style is more generally known as Hungarian Notation after its  
> Hungarian "inventor" who was a chief architect at Microsoft.

Yuppers. I know. Charles Simonyi. I really, really, like it.

> This notational conventation saw widespread use in the Windows API and  
> not much use anywhere beyond that.

with an example of 'beyond that' being John Utz's code.

Be it C, Java, C#, Javascript, Ada or any languages that use strong types.

I dont do it in shell script tho:
(Continue reading)

Permalink | Reply |
headers
Rene Herman | 20 Aug 21:54

Re: PATCH - vt1618 7.1 Audio

On 20-08-08 21:16, John L. Utz III wrote:

> anyway, we can argue about it again after i get a checkpatch
> compliant patch to submit....

Let's get a headstart...

Note that Hungarian is not a private Takashi Iwai or ALSA dislike; it's 
rather specifically against the Linux kernel coding style (although the 
actual CodingStyle document only mentions Hungarian for function names).

Most contributors have had to adjust a few local habits and in the end, 
it's generally worth it. If all is good and well, open source code is 
read many more times than it's written after all and any (significant) 
style inconsistency costs every reader just a little mental hickup. 
Review resources are one of the more scarcely available ones in this 
development process, so a consistent style definitely pays.

Sure, sure, I also try ty get away with things, but Hungarians hickup 
rather loudly.

Rene.
Permalink | Reply |
headers
John L. Utz III | 20 Aug 22:50
Favicon

Re: PATCH - vt1618 7.1 Audio

On Wed, 20 Aug 2008 12:54:23 -0700, Rene Herman <rene.herman <at> keyaccess.nl>  
wrote:

> On 20-08-08 21:16, John L. Utz III wrote:
>
>> anyway, we can argue about it again after i get a checkpatch
>> compliant patch to submit....
>
> Let's get a headstart...

hokydokysmoky!

> Note that Hungarian is not a private Takashi Iwai or ALSA dislike; it's  
> rather specifically against the Linux kernel coding style (although the  
> actual CodingStyle document only mentions Hungarian for function names).

'rather specifically' is your interpretation, not a fact.

CodingStyle.C4 just asks that naming be 'Spartan'.

So, to map 'Spartan' to my code:

this is an unsigned short variable that contains a value that will be used  
to shift another variable:

    usigned short usShift;

    ucontrol->value.enumerated.item[0] << usShift;

IMHO, this is a legitimate name in the context of CodingStyle.C4
(Continue reading)

Permalink | Reply |
headers
Rene Herman | 20 Aug 23:00

Re: PATCH - vt1618 7.1 Audio

On 20-08-08 22:50, John L. Utz III wrote:

> On Wed, 20 Aug 2008 12:54:23 -0700, Rene Herman 
> <rene.herman <at> keyaccess.nl> wrote:

>> Note that Hungarian is not a private Takashi Iwai or ALSA dislike; 
>> it's rather specifically against the Linux kernel coding style 
>> (although the actual CodingStyle document only mentions Hungarian for 
>> function names).
> 
> 'rather specifically' is your interpretation, not a fact.

No it's not my interpretation. It's based on many years of reading the 
linux kernel mailing list and seing patches being rejected over things 
like it for example. Also ssee the rest of the tree.

As to being surprised, I wouldn't be, having used Hungarian for years in 
Windows programming.

Rene.
Permalink | Reply |
headers
Mark Brown | 20 Aug 21:21
Gravatar

Re: PATCH - vt1618 7.1 Audio

On Wed, Aug 20, 2008 at 10:50:49AM -0700, John L. Utz III wrote:
> On Wed, 20 Aug 2008 01:28:39 -0700, Takashi Iwai <tiwai <at> suse.de> wrote:

> > They must be consistent -- usually 0 = disable, 1 = enable.
> > In general, the controls like "XXX Disable" is bad.  A switch should
> > be to turn on, not to turn off.  If the register bit is to disable
> > something, implement the control element in a reverse way.

> OK.

> note that it makes the code more complex in some places because the chip  
> is inconsistent.

ASoC does this by having a flag in the control specific data saying if
the control is inverted which works pretty well for dealing with this
situation.

> > Avoid too long lines (also in other places).

> OK. 80 column?

Yes.  See Documentation/CodingStyle (and checkpatch).

> > Also, a Windows style variable name should be avoided.  People tend to
> > hate it.

> Why would you call this 'Windows Style?' Is that supposed to be a  
> perjorative comment?

This style is used throughout the Windows API documentation and is
(Continue reading)

Permalink | Reply |

Gmane