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

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature