On Thu, 2022-01-06 at 12:42 +0000, Mark Brown wrote:
On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:
- unsigned int last_sysclk;
Same naming issue.
Will switch to sysclk here as well.
- if (drv_data->last_sysclk) {
unsigned int bits_per_sample = drv_data->is_32bit_lrclk ?32 : drv_data->data_width;Please write normal conditional statements, it improves legibility.
Will do.
unsigned int sclk = params_rate(params) * bits_per_sample *params_channels(params);snd_soc_params_to_bclk().
I don't think that works here since that depends on the result of snd_soc_params_to_frame_size, which doesn't account for the bits per sample being forced to 32 when the 32bit_lrclk mode is active?
unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data-last_sysclk, sclk) / 2;
Same issue with _ROUND_CLOSEST()
Will update to require exact divisibility.
if (!sclk_div || (sclk_div & ~I2S_I2STIM_VALID_MASK)) {dev_warn(i2s_dai->dev, "invalid SCLK divisor for sysclk%u and sclk %u\n",
drv_data->last_sysclk, sclk);return -EINVAL;}This indicates that we should be setting some constraints on sample rate based on sysclk.
Is there a way to do this at this level given that it can only be enforced after set_sysclk is called? Most of the other drivers that enforce rate constraints seem to be more of a fixed list..
writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);- }
Does the device actually support operation without knowing the sysclk?
It could work if set_clkdiv is called to directly set the divider, rather than set_sysclk. simple-card doesn't do that, but possibly some other setup which uses this does?
@@ -56,18 +90,18 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream *substream, static int xlnx_i2s_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *i2s_dai) {
- void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
writel(1, base + I2S_CORE_CTRL_OFFSET);
writel(I2S_CORE_CTRL_ENABLE, drv_data->base +I2S_CORE_CTRL_OFFSET); break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
writel(0, base + I2S_CORE_CTRL_OFFSET);
writel(0, drv_data->base + I2S_CORE_CTRL_OFFSET);Please split the change to have a struct for driver data out into a separate change, it's a large reformatting and is big enough to justify it - more of the diff is that than the change described in the changelog which doesn't mention this at all.
Will do.