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

From: Mark Brown
Date: Mon Apr 04 2011 - 04:03:26 EST


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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/