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.
+ .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.
+ 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?
+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.
+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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170214/934596be/attachment.sig>
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...
quoted
+ .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.
quoted
+ unsigned int shift = 24 -priv->max_scaling; +
Missing space after -.
quoted
+ 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...
quoted
+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.
quoted
+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