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

[Robert Hancock]

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);
> >  		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.

Will do.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com