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

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

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