[PATCH 3/5] ASoC: xilinx: xlnx_i2s.c: Handle sysclk setting

[Mark Brown]

On Wed, Jan 05, 2022 at 04:51:44PM -0600, Robert Hancock wrote:

> +	unsigned int last_sysclk;

Same naming issue.

> +	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.

> +		unsigned int sclk = params_rate(params) * bits_per_sample *
> +				    params_channels(params);

snd_soc_params_to_bclk().

> +		unsigned int sclk_div = DIV_ROUND_CLOSEST(drv_data->last_sysclk, sclk) / 2;

Same issue with _ROUND_CLOSEST()

> +
> +		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.

> +		writel(sclk_div, drv_data->base + I2S_I2STIM_OFFSET);
> +	}

Does the device actually support operation without knowing the sysclk?

> @@ -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);
>  		break;

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20220106/43084aa9/attachment.sig>