[alsa-devel] [PATCH 1/4] ESI W192M: Update ESI W192M driver.

[Takashi Iwai]

At Sat,  5 Oct 2013 15:41:02 +0200,
klem.dev at gmail.com wrote:
> 
> From: Clement Guedez <klem.dev at gmail.com>
> 
> Update Waveterminal 192M driver to support alsa changes that happened since last driver commit.

The changes look good, but please give more exact description and a
proper subject (for each patch).

Most of the parts in this patch, for example, are just clean ups or
name fixes.  And the essential change is only the wtm_eeprom[] content
fix (SYSCONF), which is buried in the replacement of C99 struct
initialization changes.

In other words, document clearly what you changed in the patch.
Or, make it clear in the code change itself.  For example, if you
change SYSCONF value, do only it.  It'd be a one-liner patch, and
that's fine.  Then convert the whole struct to C99 in another patch
(where you can apply other coding-style fixes together, too), mention
that it's just coding style change and no functional change.

In that way, it becomes clear what you did, and more importantly, it
makes review easier, resulting in less bugs.


thanks,

Takashi

> 
> Signed-off-by: Clement Guedez <klem.dev at gmail.com>
> ---
>  sound/pci/ice1712/wtm.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/pci/ice1712/wtm.c b/sound/pci/ice1712/wtm.c
> index bcf30a3..b6b650c 100644
> --- a/sound/pci/ice1712/wtm.c
> +++ b/sound/pci/ice1712/wtm.c
> @@ -463,7 +463,7 @@ static int wtm_add_controls(struct snd_ice1712 *ice)
>  
>  static int wtm_init(struct snd_ice1712 *ice)
>  {
> -	static unsigned short stac_inits_prodigy[] = {
> +	static unsigned short stac_inits_wtm[] = {
>  		STAC946X_RESET, 0,
>  		(unsigned short)-1
>  	};
> @@ -473,9 +473,10 @@ static int wtm_init(struct snd_ice1712 *ice)
>  	ice->num_total_dacs = 8;
>  	ice->num_total_adcs = 4;
>  	ice->force_rdma1 = 1;
> +	ice->vt1720 = 0;  /* ice1724, e.g. 23 GPIOs */
>  
>  	/*initialize codec*/
> -	p = stac_inits_prodigy;
> +	p = stac_inits_wtm;
>  	for (; *p != (unsigned short)-1; p += 2) {
>  		stac9460_put(ice, p[0], p[1]);
>  		stac9460_2_put(ice, p[0], p[1]);
> @@ -485,19 +486,25 @@ static int wtm_init(struct snd_ice1712 *ice)
>  
>  
>  static unsigned char wtm_eeprom[] = {
> -	0x47,	/*SYSCONF: clock 192KHz, 4ADC, 8DAC */
> -	0x80,	/* ACLINK : I2S */
> -	0xf8,	/* I2S: vol; 96k, 24bit, 192k */
> -	0xc1	/*SPDIF: out-en, spidf ext out*/,
> -	0x9f,	/* GPIO_DIR */
> -	0xff,	/* GPIO_DIR1 */
> -	0x7f,	/* GPIO_DIR2 */
> -	0x9f,	/* GPIO_MASK */
> -	0xff,	/* GPIO_MASK1 */
> -	0x7f,	/* GPIO_MASK2 */
> -	0x16,	/* GPIO_STATE */
> -	0x80,	/* GPIO_STATE1 */
> -	0x00,	/* GPIO_STATE2 */
> +	[ICE_EEP2_SYSCONF]     = 0x67, /* 49MHz crystal, mpu401,
> +					* spdif-in+ 1 stereo ADC,
> +					* 4 stereo DACs
> +					*/
> +	[ICE_EEP2_ACLINK]      = 0x80,  /* I2S */
> +	[ICE_EEP2_I2S]         = 0xf8,  /* vol, 96k, 24bit, 192k */
> +	[ICE_EEP2_SPDIF]       = 0xc1,  /* out-en, out-int, spdif-in */
> +	[ICE_EEP2_GPIO_DIR]    = 0x9f,
> +	[ICE_EEP2_GPIO_DIR1]   = 0xff,
> +	[ICE_EEP2_GPIO_DIR2]   = 0x7f,
> +	[ICE_EEP2_GPIO_MASK]   = 0x9f,
> +	[ICE_EEP2_GPIO_MASK1]  = 0xff,
> +	[ICE_EEP2_GPIO_MASK2]  = 0x7f,
> +	[ICE_EEP2_GPIO_STATE]  = 0x16,
> +	[ICE_EEP2_GPIO_STATE1] = 0x80,
> +	[ICE_EEP2_GPIO_STATE2] = 0x00, /* GPIO20: 0 = CD drive dig. input
> +					* passthrough,
> +					* 1 = SPDIF-OUT from ice1724
> +					*/
>  };
>  
>  
> -- 
> 1.8.4.rc3
>