Re: [PATCH v3 2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure

From: Robert Hancock <robert.hancock@calian.com>
To: "amadeuszx.slawinski@linux.intel.com" 
	<amadeuszx.slawinski@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Cc: "maruthi.srinivas.bayyavarapu@xilinx.com" 
	<maruthi.srinivas.bayyavarapu@xilinx.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"michal.simek@xilinx.com" <michal.simek@xilinx.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"kuninori.morimoto.gx@renesas.com"
	<kuninori.morimoto.gx@renesas.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>
Subject: Re: [PATCH v3 2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure
Date: Fri, 21 Jan 2022 17:26:59 +0000	[thread overview]
Message-ID: <c4c59c75e30238a7582584e187b8852808a1bfc3.camel@calian.com> (raw)
In-Reply-To: <aa839d9e-76b8-1ee2-048b-1982ff9bdb31@linux.intel.com>

On Fri, 2022-01-21 at 10:06 +0100, Amadeusz Sławiński wrote:
> On 1/20/2022 8:58 PM, Robert Hancock wrote:
> > An upcoming change will require storing additional driver data other
> > than the memory base address. Create a drvdata structure and use that
> > rather than storing the raw base address pointer.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >   sound/soc/xilinx/xlnx_i2s.c | 66 ++++++++++++++++++++-----------------
> >   1 file changed, 35 insertions(+), 31 deletions(-)
> > 
> > diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
> > index cc641e582c82..3bafa34b789a 100644
> > --- a/sound/soc/xilinx/xlnx_i2s.c
> > +++ b/sound/soc/xilinx/xlnx_i2s.c
> > @@ -22,15 +22,20 @@
> >   #define I2S_CH0_OFFSET			0x30
> >   #define I2S_I2STIM_VALID_MASK		GENMASK(7, 0)
> >   
> > +struct xlnx_i2s_drv_data {
> > +	struct snd_soc_dai_driver dai_drv;
> > +	void __iomem *base;
> > +};
> > +
> >   static int xlnx_i2s_set_sclkout_div(struct snd_soc_dai *cpu_dai,
> >   				    int div_id, int div)
> >   {
> > -	void __iomem *base = snd_soc_dai_get_drvdata(cpu_dai);
> > +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(cpu_dai);
> >   
> >   	if (!div || (div & ~I2S_I2STIM_VALID_MASK))
> >   		return -EINVAL;
> >   
> > -	writel(div, base + I2S_I2STIM_OFFSET);
> > +	writel(div, drv_data->base + I2S_I2STIM_OFFSET);
> >   
> >   	return 0;
> >   }
> > @@ -40,13 +45,13 @@ static int xlnx_i2s_hw_params(struct snd_pcm_substream
> > *substream,
> >   			      struct snd_soc_dai *i2s_dai)
> >   {
> >   	u32 reg_off, chan_id;
> > -	void __iomem *base = snd_soc_dai_get_drvdata(i2s_dai);
> > +	struct xlnx_i2s_drv_data *drv_data = snd_soc_dai_get_drvdata(i2s_dai);
> >   
> >   	chan_id = params_channels(params) / 2;
> >   
> >   	while (chan_id > 0) {
> >   		reg_off = I2S_CH0_OFFSET + ((chan_id - 1) * 4);
> > -		writel(chan_id, base + reg_off);
> > +		writel(chan_id, drv_data->base + reg_off);
> >   		chan_id--;
> >   	}
> >   
> > @@ -56,18 +61,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(1, 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;
> >   	default:
> >   		return -EINVAL;
> > @@ -95,20 +100,19 @@ MODULE_DEVICE_TABLE(of, xlnx_i2s_of_match);
> >   
> >   static int xlnx_i2s_probe(struct platform_device *pdev)
> >   {
> > -	void __iomem *base;
> > -	struct snd_soc_dai_driver *dai_drv;
> > +	struct xlnx_i2s_drv_data *drv_data;
> >   	int ret;
> >   	u32 ch, format, data_width;
> >   	struct device *dev = &pdev->dev;
> >   	struct device_node *node = dev->of_node;
> >   
> > -	dai_drv = devm_kzalloc(&pdev->dev, sizeof(*dai_drv), GFP_KERNEL);
> > -	if (!dai_drv)
> > +	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
> > +	if (!drv_data)
> >   		return -ENOMEM;
> >   
> > -	base = devm_platform_ioremap_resource(pdev, 0);
> > -	if (IS_ERR(base))
> > -		return PTR_ERR(base);
> > +	drv_data->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(drv_data->base))
> > +		return PTR_ERR(drv_data->base);
> >   
> >   	ret = of_property_read_u32(node, "xlnx,num-channels", &ch);
> >   	if (ret < 0) {
> > @@ -134,35 +138,35 @@ static int xlnx_i2s_probe(struct platform_device
> > *pdev)
> >   	}
> >   
> >   	if (of_device_is_compatible(node, "xlnx,i2s-transmitter-1.0")) {
> > -		dai_drv->name = "xlnx_i2s_playback";
> > -		dai_drv->playback.stream_name = "Playback";
> > -		dai_drv->playback.formats = format;
> > -		dai_drv->playback.channels_min = ch;
> > -		dai_drv->playback.channels_max = ch;
> > -		dai_drv->playback.rates	= SNDRV_PCM_RATE_8000_192000;
> > -		dai_drv->ops = &xlnx_i2s_dai_ops;
> > +		drv_data->dai_drv.name = "xlnx_i2s_playback";
> > +		drv_data->dai_drv.playback.stream_name = "Playback";
> > +		drv_data->dai_drv.playback.formats = format;
> > +		drv_data->dai_drv.playback.channels_min = ch;
> > +		drv_data->dai_drv.playback.channels_max = ch;
> > +		drv_data->dai_drv.playback.rates	=
> > SNDRV_PCM_RATE_8000_192000;
> > +		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
> >   	} else if (of_device_is_compatible(node, "xlnx,i2s-receiver-1.0")) {
> > -		dai_drv->name = "xlnx_i2s_capture";
> > -		dai_drv->capture.stream_name = "Capture";
> > -		dai_drv->capture.formats = format;
> > -		dai_drv->capture.channels_min = ch;
> > -		dai_drv->capture.channels_max = ch;
> > -		dai_drv->capture.rates = SNDRV_PCM_RATE_8000_192000;
> > -		dai_drv->ops = &xlnx_i2s_dai_ops;
> > +		drv_data->dai_drv.name = "xlnx_i2s_capture";
> > +		drv_data->dai_drv.capture.stream_name = "Capture";
> > +		drv_data->dai_drv.capture.formats = format;
> > +		drv_data->dai_drv.capture.channels_min = ch;
> > +		drv_data->dai_drv.capture.channels_max = ch;
> > +		drv_data->dai_drv.capture.rates = SNDRV_PCM_RATE_8000_192000;
> > +		drv_data->dai_drv.ops = &xlnx_i2s_dai_ops;
> >   	} else {
> >   		return -ENODEV;
> >   	}
> >   
> > -	dev_set_drvdata(&pdev->dev, base);
> > +	dev_set_drvdata(&pdev->dev, drv_data);
> >   
> >   	ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
> > -					      dai_drv, 1);
> > +					      &drv_data->dai_drv, 1);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "i2s component registration failed\n");
> >   		return ret;
> >   	}
> >   
> > -	dev_info(&pdev->dev, "%s DAI registered\n", dai_drv->name);
> > +	dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
> >   
> >   	return ret;
> >   }
> 
> I don't think this patch is needed, snd_soc_dai, already has pointer to 
> its snd_soc_dai_driver, so there is no need to keep it additionally in 
> drvdata?
> 
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/sound/soc-dai.h?h=v5.16*n431__;Iw!!IOGos0k!wK-VYGvndh29eBo3CZIPn7xG_X7ib4R8-hEVEyJc8aXkGoYORJ8cLH25u-K31eZAYe4$ 
> 
> 

It's not a pointer to the struct snd_soc_dai_driver that's in the drvdata
structure, snd_soc_dai_driver is actually part of the drvdata structure.
Previously it was allocating snd_soc_dai_driver by itself, and stuffing the
base address into the drvdata pointer. Now it's allocating one
xlnx_i2s_drv_data structure which contains both (and more attributes to come).

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