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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 4 Apr 2011 09:03:19 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Prchal Jiří <jiri.prchal@...ignal.cz>
Cc:	alsa-devel@...r.kernel.org, alsa-devel@...a-project.org,
	vbarinov@...eddedalley.com, linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [PATCH 1/2] ALSA: ASoc: TLV320AIC3X: ad SPI and
	clock on GPIO2 or BCLK

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/