Re: [alsa-devel] [PATCH 1/2] ALSA: ASoc: TLV320AIC3X: ad SPI and clock on GPIO2 or BCLK

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Mon, Apr 04, 2011 at 09:49:49AM +0200, Prchal Jiří wrote:
> Dne 2.4.2011 10:26, Mark Brown napsal(a):

> >> TODO: Set the model in SPI probe the right way. I don't know how.

> > Register two SPI drivers with different names.

> ???

Register a SPI driver per model.

> > This looks mostly good but there's some cleanups needed, mostly from
> > extra stuff which snuck in there, and you need to rebase against current
> > code.

> What cleanup? I copy functions snd_soc_8_8_* and made change to 7_8.

The comments I've made in the rest of the review such as removing random
whitespace updates.

> > As covered in SubmittingPatches you should always CC maintainers on
> > patches.

> Who is maintainer? I didn't find anyone, so I CC to author.

Myself and Liam Girdwood.  At the most basic level the people you'd
expect to apply the patch.

> >> +	/* set external clock on GPIO2 or BCLK */
> >> +	data = snd_soc_read(codec, AIC3X_CLKGEN_CTRL_REG);
> >> +	data &= 0x0f;
> >> +	data |= ((clk_id << PLLCLK_IN_SHIFT) | (clk_id << CLKDIV_IN_SHIFT));
> >> +	snd_soc_write(codec, AIC3X_CLKGEN_CTRL_REG, data);

> > This looks like an unrelated change which is specific to your board.
> > You should add an interface for configuring this functionality .

> No, it's for all, who uses other clock source. Interface is standard function:
> 	/* Set the codec system clock for DAC and ADC */
> 	ret = snd_soc_dai_set_sysclk(codec_dai,
> 				     CLKIN_BCLK, cdu_audio[i].codecclk,
> 				     SND_SOC_CLOCK_IN);

In that case you should submit a separate patch adding this
functionality, taking care to ensure that you don't break existing
users.

> >> +static struct spi_driver aic3x_spi_driver = {
> >> +	.driver = {
> >> +		.name	= "tlv320aic3x-codec",
> >> +		.owner	= THIS_MODULE,

> > Remove the -codec from the driver name.

> Why? In I2C part it must be there, so should it be same?

No, it should be removed from the I2C driver too.