| Message ID | 20220120195832.1742271-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5e46c63ca22278fe363dfd9f5360c2e2ad082087 |
| Headers | show |
| Series | ASoC: Xilinx fixes | expand |
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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/sound/soc-dai.h?h=v5.16#n431
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).
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; }
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(-)