[alsa-devel] [PATCH V2 02/10] ASoC: img: Add driver for I2S input controller

[Damien Horsley]

On 23/10/15 23:57, Mark Brown wrote:
> On Thu, Oct 22, 2015 at 08:09:38PM +0100, Damien Horsley wrote:
>> On 19/10/15 18:47, Mark Brown wrote:
>>> On Mon, Oct 12, 2015 at 01:40:29PM +0100, Damien Horsley wrote:
> 
>>> The APIs here all seem a bit odd - for example the enable API taking a
>>> register value as an argument (normally reg is a register address BTW)
>>> and returning a value but the disable API doing a read/modify/write
>>> cycle.
> 
>> Sure. It reduces the number of register accesses this way, but the
>> difference in execution time is not significant. Would you prefer these
>> to both do read-modify-writes?
> 
> I would prefer that the functions look consistent with each other and
> ideally resemble common register acceess idioms in the kernel.
> 

Ok.

>>>> +static inline void img_i2s_in_flush(struct img_i2s_in *i2s)
>>>> +{
>>>> +	int i;
>>>> +	u32 reg;
>>>> +
>>>> +	for (i = 0; i < i2s->active_channels; i++) {
>>>> +		reg = img_i2s_in_ch_disable(i2s, i);
>>>> +		reg |= IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>>>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>>>> +		reg &= ~IMG_I2S_IN_CH_CTL_FIFO_FLUSH_MASK;
>>>> +		img_i2s_in_ch_writel(i2s, i, reg, IMG_I2S_IN_CH_CTL);
>>>> +		img_i2s_in_ch_enable(i2s, i, reg);
>>>> +	}
>>>> +}
> 
>>> This all seems to be connected to this, which is itself slightly funky
>>> especially in the context of the only user...
> 
>> They are also used during hw_params and set_format.
> 
> My point is that the flush function has only one user.
> 
>>>> +	case SNDRV_PCM_TRIGGER_STOP:
>>>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>>>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>>>> +		reg = img_i2s_in_readl(i2s, IMG_I2S_IN_CTL);
>>>> +		reg &= ~IMG_I2S_IN_CTL_ME_MASK;
>>>> +		img_i2s_in_writel(i2s, reg, IMG_I2S_IN_CTL);
>>>> +		img_i2s_in_flush(i2s);
>>>> +		break;
> 
>>> ...which looks like it'll enable everything, then disable and reenable.
>>> Plus needing to do a flush on trigger seems weird.
> 
>> If the FIFOs are not flushed, some samples from the previous stream will
>> be transferred to the user application when the block is started again
> 
> Shouldn't we be doing that flush on stream close instead?  If nothing
> else the flush is going to discard a bit of data if the stream is just
> paused.
> 

The FIFOs are only 8 frames in size, so I am not sure there is an
issue with these frames being lost.

I think it also makes sense to keep the blocks consistent with each
other. The spdif (out and in), and parallel out, all flush automatically
when stopped, and the fifo for the i2s out block is cleared when the
reset is asserted.

>>>> +	if ((channels < 2) ||
>>>> +			(channels > (i2s->max_i2s_chan * 2)) ||
>>>> +			(channels % 2))
>>>> +		return -EINVAL;
> 
>>> This indentation is very weird.
> 
>> Ok. What is the correct indentation for this?
> 
> Align the continuation lines of the if condition with the first line.
> 

Ok