[alsa-devel] [RFC v2 5/7] ASoC: stm32: add DFSDM DAI support

[Arnaud Pouliquen]

On 02/14/2017 06:45 PM, Mark Brown wrote:
> On Mon, Feb 13, 2017 at 05:38:27PM +0100, Arnaud Pouliquen wrote:
> 
> This looks basically fine as a system specific driver but as some 
> of the comments in here say there's bits of it could perhaps be 
> genericised but I'm not sure we need to do that right now.  I'm not
> sure the abstraction is exactly comfortable but having another bit
> of hardware doing a bridge to IIO might be the best way to figure
> out something better.
> 
Yes this is one point to clarify. I keep it as a specific API, as i
don't know if another hardware needs to support it...
I would say that objective of this version is to highlight interactions.
Then i can rework it in a way that could be  genericised in future (
i.e. using void* for params that would depend on platforms)...

Extend IIO customer API would be also another alternative, but i did
not find a generic way to do it. Some IIO attributes could be used for
Hw params but DMA and IRQs seems tricky to handle through this IIO
interface...




>> +	.period_bytes_min = 40, /* 8 khz 5 ms */ +	.period_bytes_max =
>>  4 * PAGE_SIZE, +	.buffer_bytes_max = 16 * PAGE_SIZE
> 
> What's the physical minimum period limit?  The comment makes this 
> sound like it's just made up.
I did not find physical minimum period limit, that why i considered
this value from a scheduling point of view. I will re-check these values.

> 
>> +	unsigned int shift = 24 -priv->max_scaling; +
> 
> Missing space after -.
> 
>> +	dev_dbg(dai->dev, "%s: enter\n", __func__); +	return 0; + 
>> return snd_pcm_hw_constraint_list(substream->runtime, 0, + 
>> SNDRV_PCM_HW_PARAM_RATE, +					  &priv->rates_const);
> 
> Looks like debug changes got left in?
yes exactly...
> 
>> +static int stm32_adfsdm_set_sysclk(struct snd_soc_dai *dai, int
>>  clk_id, +				   unsigned int freq, int dir) +{ +	struct 
>> stm32_adfsdm_priv *priv = snd_soc_dai_get_drvdata(dai); +	struct
>>  stm32_adfsdm_pdata *pdata = priv->pdata; + +	dev_dbg(dai->dev, 
>> "%s: enter for dai %d\n", __func__, dai->id); +	if (dir == 
>> SND_SOC_CLOCK_IN) { +		pdata->ops->set_sysclk(pdata->adc, freq); 
>> +		priv->dmic_clk = freq; +	} + +	/* Determine supported rate 
>> which depends on SPI/manchester clock */ +	return 
>> stm32_adfsdm_get_supported_rates(dai, &priv->rates_const.mask);
> 
> Since the DAI is unidirectional it doesn't matter but obviously if
>  it weren't then the fact that getting the supported rates involves
>  setting the hwparams means this could become disruptive.  If we're
>  going to genericise this to be a more general IIO/ASoC bridge that
>  could matter.
I think that the driver itself can not be generic. ST DFSDM is too
specific. Only API with IIO could be.

> 
>> +static int stm32_adfsdm_dai_remove(struct snd_soc_dai *dai) +{ +
>> dev_dbg(dai->dev, "%s: enter for dai %d\n", __func__, dai->id); +
>> +	return 0; +}
> 
> Remove empty functions, though in this case I think you want to
> add something to disconnect the XRUN callback just in order to be
> sure it can't be mistakenly called.

Yes Completely unnecessary here. Furthermore the driver should be
removed by the IIO driver.

Regards
Arnaud