linux-kernel - Re: [PATCH v5] sound/soc/lapis: add platform driver for ML7213

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]

Message-ID: <20120321160913.GF3226@opensource.wolfsonmicro.com>
Date:	Wed, 21 Mar 2012 16:09:13 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>, lars@...afoo.de,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	qi.wang@...el.com, yong.y.wang@...el.com, joel.clark@...el.com,
	kok.howg.ewe@...el.com
Subject: Re: [PATCH v5] sound/soc/lapis: add platform driver for ML7213

On Mon, Mar 19, 2012 at 09:00:59PM +0900, Tomoya MORINAGA wrote:
> This driver is for LAPIS Semiconductor ML7213 IOH I2S.

So, I just said to the SPEAr guys that I really want to see support for
non-cyclic dmaengine added to the helper library rather than open
coding.  We're seeing lots of platforms moving to dmaengine (as well as
those that are in already yours is one of three out of tree platforms
I'm aware of that have actively submitted stuff) and with this number of
platforms we really need to get stuff factored out - there's enough
people working on these platforms that it should be possible to add the
support to the dmaengine library code.

A few relatively minor comments below and due to the above I haven't
really reviwed the DMA but overall this looks pretty good.

> +static struct ioh_i2s_data *i2s_data;
> +static struct ioh_i2s_dma *dmadata;

These shouldn't be global, they should be device specific and passed
around.

> +static int ml7213i2s_dai_set_dai_sysclk(struct snd_soc_dai *dai,
> +					int clk_id, unsigned int freq, int dir)
> +{
> +	u32 reg[MAX_I2S_CH];
> +	void *iobase = i2s_data->iobase;
> +	int i;
> +
> +	for (i = 0; i < MAX_I2S_CH; i++) {
> +		reg[i] = ioread32(iobase + I2SCLKCNT0_OFFSET + 0x10 * i);
> +		if (clk_id == IOH_MASTERCLKSEL_MCLK)
> +			reg[i] &= ~ML7213I2S_MASTER_CLK_SEL;
> +		else if (clk_id == IOH_MASTERCLKSEL_MLBCLK)
> +			reg[i] |= ML7213I2S_MASTER_CLK_SEL;
> +		iowrite32(reg[i], iobase + I2SCLKCNT0_OFFSET + 0x10 * i);

This looks like it should be a switch statement on clk_id, this will
also catch bad parameters that might get passed in.

> +		if ((slot < MAX_I2S_CH) &&
> +				(!(tx_mapped & (1 << slot)))) {
> +			dmadata[i].mapped_tx_ch = slot;
> +			tx_mapped |= 1 << slot;
> +		} else
> +			return -EINVAL;

Braces on both sides of the if please.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)