| Message ID | 20220120195832.1742271-1-robert.hancock@calian.com |
|---|---|
| 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).
On Thu, 20 Jan 2022 13:58:26 -0600, Robert Hancock wrote: > There are drivers in mainline for the Xilinx Audio Formatter and Xilinx > I2S IP cores. However, because of a few issues, these were only really > usable with Xilinx's xlnx_pl_snd_card top-level driver, which is not in > mainline (and not suitable for mainline). > > The fixes in this patchset, for the simple-card layer as well as the > Xilinx drivers, now allow these drivers to be properly used with > simple-card without any out-of-tree support code. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/6] ASoC: xilinx: xlnx_formatter_pcm: Handle sysclk setting commit: 1c5091fbe7e0d0804158200b7feac5123f7b4fbd [2/6] ASoC: xilinx: xlnx_i2s: create drvdata structure commit: 5e46c63ca22278fe363dfd9f5360c2e2ad082087 [3/6] ASoC: xilinx: xlnx_i2s: Handle sysclk setting commit: c47aef899c1bb0cbda48808356e7c040d95ca612 [4/6] ASoC: simple-card-utils: Set sysclk on all components commit: ce2f7b8d4290c22e462e465d1da38a1c113ae66a [5/6] ASoC: dt-bindings: simple-card: document new system-clock-fixed flag commit: e9fed03aebacb8873dee8e2edfbce96f27f6c730 [6/6] ASoC: simple-card-utils: Add new system-clock-fixed flag commit: 5ca2ab4598179a2690a38420f3fde9f2ad79d55c All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark