| Message ID | 1459948893-4206-3-git-send-email-petr@barix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/06/16 16:21, Petr Kulhavy wrote: > This adds DT support for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx McBSP driver. > > Signed-off-by: Petr Kulhavy <petr@barix.com> > --- > sound/soc/davinci/Kconfig | 6 ++- > sound/soc/davinci/davinci-i2s.c | 106 ++++++++++++++++++++++++++++++++++------ > 2 files changed, 96 insertions(+), 16 deletions(-) > > diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig > index 50ca291cc225..6b732d8e5896 100644 > --- a/sound/soc/davinci/Kconfig > +++ b/sound/soc/davinci/Kconfig > @@ -16,7 +16,11 @@ config SND_EDMA_SOC > - DRA7xx family > > config SND_DAVINCI_SOC_I2S > - tristate > + tristate "DaVinci Multichannel Buffered Serial Port (McBSP) support" > + depends on SND_EDMA_SOC > + help > + Say Y or M here if you want to have support for McBSP IP found in > + Texas Instruments DaVinci DA850 SoCs. > > config SND_DAVINCI_SOC_MCASP > tristate "Multichannel Audio Serial Port (McASP) support" > diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c > index ec98548a5fc9..2ebfe86dd584 100644 > --- a/sound/soc/davinci/davinci-i2s.c > +++ b/sound/soc/davinci/davinci-i2s.c > @@ -4,9 +4,15 @@ > * Author: Vladimir Barinov, <vbarinov@embeddedalley.com> > * Copyright: (C) 2007 MontaVista Software, Inc., <source@mvista.com> > * > + * DT support (c) 2016 Petr Kulhavy, Barix AG <petr@barix.com> > + * based on davinci-mcasp.c DT support > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > * published by the Free Software Foundation. > + * > + * TODO: > + * on DA850 implement HW FIFOs instead of DMA into DXR and DRR registers The BFIFO looks identical to AFIFO for McASP... > */ > > #include <linux/init.h> > @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { > .name = "davinci-i2s", > }; > > +static struct snd_platform_data* > +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) > +{ > + struct snd_platform_data *pdata = NULL; > + struct device_node *np; > + struct of_phandle_args dma_spec; > + int ret; > + > + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) > + return dev_get_platdata(&pdev->dev); > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return ERR_PTR(-ENOMEM); > + > + np = pdev->dev.of_node; > + > + ret = of_property_match_string(np, "dma-names", "tx"); > + if (ret >= 0) { > + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, > + &dma_spec); > + if (ret >= 0) > + pdata->tx_dma_channel = dma_spec.args[0]; > + } > + > + ret = of_property_match_string(np, "dma-names", "rx"); > + if (ret >= 0) { > + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, > + &dma_spec); > + if (ret >= 0) > + pdata->rx_dma_channel = dma_spec.args[0]; > + } Why would you do this? If we booted with DT the only thing needed by the edma-pcm (dmaengine) is the name of the DMA channel... > + > + /* optional parameters */ > + > + pdata->enable_channel_combine = > + of_property_read_bool(np, "channel-combine"); This can only be done when the McBSP is used in DSP_x mode since this will make the McBSP to transmit the stereo audio as mono on the bus. After a quick look at the relevant parts in the driver, I think most of the things are pretty much broken or at least they are not totally correct. McBSP do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on how well things are working. > + > + /* > + * pdata->clk_input_pin is deliberately not exported to DT > + * and the default value of the clk_input_pin is MCBSP_CLKR. > + * The value MCBSP_CLKS makes no sense as it turns the CPU > + * to a bit clock master in the SND_SOC_DAIFMT_CBM_CFS mode > + * where it should be bit clock slave! > + */ Interesting. As a whole the clock selection part is mostly broken in the driver... It would be better to add davinci_i2s_set_sysclk() to deal with the system clock configuration. > + > + return pdata; > +} > + > static int davinci_i2s_probe(struct platform_device *pdev) > { > + struct snd_platform_data *pdata; > struct davinci_mcbsp_dev *dev; > struct resource *mem, *res; > void __iomem *io_base; > int *dma; > int ret; > > - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + pdata = davinci_i2s_set_pdata_from_of(pdev); > + if (IS_ERR(pdata)) { > + dev_err(&pdev->dev, "Error populating platform data, err %ld\n", > + PTR_ERR(pdata)); > + return PTR_ERR(pdata); > + } > + > + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); > + if (!mem) { > + dev_warn(&pdev->dev, > + "\"mpu\" mem resource not found, using index 0\n"); > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) { > + dev_err(&pdev->dev, "no mem resource?\n"); > + return -ENODEV; > + } > + } > + > io_base = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(io_base)) > return PTR_ERR(io_base); > @@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) > (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG); > > /* first TX, then RX */ > - res = platform_get_resource(pdev, IORESOURCE_DMA, 0); > - if (!res) { > - dev_err(&pdev->dev, "no DMA resource\n"); > - ret = -ENXIO; > - goto err_release_clk; > - } > dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; > - *dma = res->start; > dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma; > + res = platform_get_resource(pdev, IORESOURCE_DMA, 0); > + if (res) > + *dma = res->start; > + else > + *dma = pdata->tx_dma_channel; Please follow the way davinci-mcasp does it right now: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) *dma = res->start; else *dma = pdata->tx_dma_channel; /* dmaengine filter data for DT and non-DT boot */ if (pdev->dev.of_node) dma_data->filter_data = "tx"; else dma_data->filter_data = dma; while we are here, I think the tx/rx_dma_channel is something we should just get rid of. It is not used as far as I can tell. Something like this would do I think: dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; ... res = platform_get_resource(pdev, IORESOURCE_DMA, 0); if (res) { dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; *dma = res->start; dma_data->filter_data = dma; } else if (pdev->dev.of_node) { dma_data->filter_data = "tx"; } else { dev_err(&pdev->dev, "Missing DMA tx resource\n"); return -ENODEV; } > > - res = platform_get_resource(pdev, IORESOURCE_DMA, 1); > - if (!res) { > - dev_err(&pdev->dev, "no DMA resource\n"); > - ret = -ENXIO; > - goto err_release_clk; > - } > dma = &dev->dma_request[SNDRV_PCM_STREAM_CAPTURE]; > - *dma = res->start; > dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].filter_data = dma; > + res = platform_get_resource(pdev, IORESOURCE_DMA, 1); > + if (res) > + *dma = res->start; > + else > + *dma = pdata->rx_dma_channel; same comment as for the TX dma part. > > dev->dev = &pdev->dev; > dev_set_drvdata(&pdev->dev, dev); > @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) > return 0; > } > > +static const struct of_device_id davinci_mcbsp_match[] = { > + { .compatible = "ti,da850-mcbsp-audio" }, I would drop the "-audio" for the binding... > + {}, > +}; > +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match); > + > static struct platform_driver davinci_mcbsp_driver = { > .probe = davinci_i2s_probe, > .remove = davinci_i2s_remove, > .driver = { > .name = "davinci-mcbsp", > + .of_match_table = of_match_ptr(davinci_mcbsp_match), The driver calls itself davinci-i2s.c and the prefixes internally used are davinci_i2s_*. The driver name is "davinci-mcbsp". And it is basically a driver for daVinci ASP (the McBSP additions are not configured at all). I still think we should keep the davinci_i2s_* when adding new code, or rename the whole driver and it's prefixes to something more sensible? > }, > }; > >
On 07.04.2016 14:42, Peter Ujfalusi wrote: > On 04/06/16 16:21, Petr Kulhavy wrote: > >> */ >> >> #include <linux/init.h> >> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { >> .name = "davinci-i2s", >> }; >> >> +static struct snd_platform_data* >> +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) >> +{ >> + struct snd_platform_data *pdata = NULL; >> + struct device_node *np; >> + struct of_phandle_args dma_spec; >> + int ret; >> + >> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) >> + return dev_get_platdata(&pdev->dev); >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return ERR_PTR(-ENOMEM); >> + >> + np = pdev->dev.of_node; >> + >> + ret = of_property_match_string(np, "dma-names", "tx"); >> + if (ret >= 0) { >> + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, >> + &dma_spec); >> + if (ret >= 0) >> + pdata->tx_dma_channel = dma_spec.args[0]; >> + } >> + >> + ret = of_property_match_string(np, "dma-names", "rx"); >> + if (ret >= 0) { >> + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, >> + &dma_spec); >> + if (ret >= 0) >> + pdata->rx_dma_channel = dma_spec.args[0]; >> + } > Why would you do this? If we booted with DT the only thing needed by the > edma-pcm (dmaengine) is the name of the DMA channel... I don't like this code either. But I have tried just to set the dma_filter to "rx" or "tx" and it didn't work. Perhaps because the edma pcm filter function filters by channel number and not by name? Or do you see an easier way of getting the channel number from the name? > >> + >> + /* optional parameters */ >> + >> + pdata->enable_channel_combine = >> + of_property_read_bool(np, "channel-combine"); > This can only be done when the McBSP is used in DSP_x mode since this will > make the McBSP to transmit the stereo audio as mono on the bus. I have actually never used this option and don't see a benefit of it. Is it just some workaround that should not be included in the binding? > After a quick look at the relevant parts in the driver, I think most of the > things are pretty much broken or at least they are not totally correct. McBSP > do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. > I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on > how well things are working. The I2S worked for me if the frame size equalled the PCM data size. E.g. 16-bit audio, 2 channels, 16 bit frame size -> 32 clock ticks per sample in total. However with bigger frame sizes it didn't work because it cannot insert "blanks" between the channels. E.g. with 32-bit wide slots and 16-bit audio it sends both left and right data in the first 32 clock ticks (i.e. still the first channel) and then the during the other channel the data line goes to high impedance which results in effectively playing only the left channel (and noise in the other channel). That's probably what the comment about broken I2S is trying to say. I couldn't find any help in the datasheet either. On some codecs the frame size is configurable and there it works, but codecs that require 32-bit frames don't work with 16-bit audio or need to use DSP_x format. > >> + >> + return pdata; >> +} >> + >> static int davinci_i2s_probe(struct platform_device *pdev) >> { >> + struct snd_platform_data *pdata; >> struct davinci_mcbsp_dev *dev; >> struct resource *mem, *res; >> void __iomem *io_base; >> int *dma; >> int ret; >> >> - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + pdata = davinci_i2s_set_pdata_from_of(pdev); >> + if (IS_ERR(pdata)) { >> + dev_err(&pdev->dev, "Error populating platform data, err %ld\n", >> + PTR_ERR(pdata)); >> + return PTR_ERR(pdata); >> + } >> + >> + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); >> + if (!mem) { >> + dev_warn(&pdev->dev, >> + "\"mpu\" mem resource not found, using index 0\n"); >> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!mem) { >> + dev_err(&pdev->dev, "no mem resource?\n"); >> + return -ENODEV; >> + } >> + } >> + >> io_base = devm_ioremap_resource(&pdev->dev, mem); >> if (IS_ERR(io_base)) >> return PTR_ERR(io_base); >> @@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) >> (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG); >> >> /* first TX, then RX */ >> - res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >> - if (!res) { >> - dev_err(&pdev->dev, "no DMA resource\n"); >> - ret = -ENXIO; >> - goto err_release_clk; >> - } >> dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; >> - *dma = res->start; >> dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma; >> + res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >> + if (res) >> + *dma = res->start; >> + else >> + *dma = pdata->tx_dma_channel; > Please follow the way davinci-mcasp does it right now: > dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; > ... > dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; > res = platform_get_resource(pdev, IORESOURCE_DMA, 0); > if (res) > *dma = res->start; > else > *dma = pdata->tx_dma_channel; > > /* dmaengine filter data for DT and non-DT boot */ > if (pdev->dev.of_node) > dma_data->filter_data = "tx"; > else > dma_data->filter_data = dma; > > while we are here, I think the tx/rx_dma_channel is something we should just > get rid of. It is not used as far as I can tell. > Something like this would do I think: > dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; > ... > res = platform_get_resource(pdev, IORESOURCE_DMA, 0); > if (res) { > dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; > *dma = res->start; > dma_data->filter_data = dma; > } else if (pdev->dev.of_node) { > dma_data->filter_data = "tx"; > } else { > dev_err(&pdev->dev, "Missing DMA tx resource\n"); > return -ENODEV; > } Is the edma pcm filter function able to filter in once case by the dma number and in the other case by the dma name? See my above comment as well... >> >> dev->dev = &pdev->dev; >> dev_set_drvdata(&pdev->dev, dev); >> @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static const struct of_device_id davinci_mcbsp_match[] = { >> + { .compatible = "ti,da850-mcbsp-audio" }, > I would drop the "-audio" for the binding... I was trying to stay consistent with the mcasp. But I don't mind keeping the name shorter :-) >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match); >> + >> static struct platform_driver davinci_mcbsp_driver = { >> .probe = davinci_i2s_probe, >> .remove = davinci_i2s_remove, >> .driver = { >> .name = "davinci-mcbsp", >> + .of_match_table = of_match_ptr(davinci_mcbsp_match), > The driver calls itself davinci-i2s.c and the prefixes internally used are > davinci_i2s_*. > The driver name is "davinci-mcbsp". > And it is basically a driver for daVinci ASP (the McBSP additions are not > configured at all). > I still think we should keep the davinci_i2s_* when adding new code, or rename > the whole driver and it's prefixes to something more sensible? Good point, thanks! I will rename the table to davinci_i2s_match to stay consistent. Petr
On 04/07/16 16:32, Petr Kulhavy wrote: > > > On 07.04.2016 14:42, Peter Ujfalusi wrote: >> On 04/06/16 16:21, Petr Kulhavy wrote: >> >>> */ >>> #include <linux/init.h> >>> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver >>> davinci_i2s_component = { >>> .name = "davinci-i2s", >>> }; >>> +static struct snd_platform_data* >>> +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) >>> +{ >>> + struct snd_platform_data *pdata = NULL; >>> + struct device_node *np; >>> + struct of_phandle_args dma_spec; >>> + int ret; >>> + >>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) >>> + return dev_get_platdata(&pdev->dev); >>> + >>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >>> + if (!pdata) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + np = pdev->dev.of_node; >>> + >>> + ret = of_property_match_string(np, "dma-names", "tx"); >>> + if (ret >= 0) { >>> + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, >>> + &dma_spec); >>> + if (ret >= 0) >>> + pdata->tx_dma_channel = dma_spec.args[0]; >>> + } >>> + >>> + ret = of_property_match_string(np, "dma-names", "rx"); >>> + if (ret >= 0) { >>> + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, >>> + &dma_spec); >>> + if (ret >= 0) >>> + pdata->rx_dma_channel = dma_spec.args[0]; >>> + } >> Why would you do this? If we booted with DT the only thing needed by the >> edma-pcm (dmaengine) is the name of the DMA channel... > I don't like this code either. But I have tried just to set the dma_filter to > "rx" or "tx" and it didn't work. > Perhaps because the edma pcm filter function filters by channel number and not > by name? the filter_fn is only used in legacy boot case not in DT boot. > Or do you see an easier way of getting the channel number from the name? It was failing because you were using wrong dt bindings for the eDMA, if you update that it will work. > >> >>> + >>> + /* optional parameters */ >>> + >>> + pdata->enable_channel_combine = >>> + of_property_read_bool(np, "channel-combine"); >> This can only be done when the McBSP is used in DSP_x mode since this will >> make the McBSP to transmit the stereo audio as mono on the bus. > I have actually never used this option and don't see a benefit of it. > Is it just some workaround that should not be included in the binding? It is to have more time for the DMA to write, read the audio data as McBSP does not have FIFO. This is a SW parameter, so it does not belong to the DT if we take things by the letter. Probably it is better to leave it out for now and add it later if there is a need? >> After a quick look at the relevant parts in the driver, I think most of the >> things are pretty much broken or at least they are not totally correct. McBSP >> do support real I2S (SRGR:FWID), and the phase setup is wrong in the code. IMO. >> I don't have HW to test the daVinci-MCBSP or the ASP so I can not comment on >> how well things are working. > The I2S worked for me if the frame size equalled the PCM data size. E.g. > 16-bit audio, 2 channels, 16 bit frame size -> 32 clock ticks per sample in > total. > However with bigger frame sizes it didn't work because it cannot insert > "blanks" between the channels. > E.g. with 32-bit wide slots and 16-bit audio it sends both left and right data > in the first 32 clock ticks (i.e. still the first channel) and then the during > the other channel the data line goes to high impedance which results in > effectively playing only the left channel (and noise in the other channel). Yeah, this is not working on OMAP-McBSP either. > That's probably what the comment about broken I2S is trying to say. > I couldn't find any help in the datasheet either. With exact bclk the I2S is working fine, with most codecs at least we are using that with OMAPs. > On some codecs the frame size is configurable and there it works, but codecs > that require 32-bit frames don't work with 16-bit audio or need to use DSP_x > format. >> >>> + >>> + return pdata; >>> +} >>> + >>> static int davinci_i2s_probe(struct platform_device *pdev) >>> { >>> + struct snd_platform_data *pdata; >>> struct davinci_mcbsp_dev *dev; >>> struct resource *mem, *res; >>> void __iomem *io_base; >>> int *dma; >>> int ret; >>> - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + pdata = davinci_i2s_set_pdata_from_of(pdev); >>> + if (IS_ERR(pdata)) { >>> + dev_err(&pdev->dev, "Error populating platform data, err %ld\n", >>> + PTR_ERR(pdata)); >>> + return PTR_ERR(pdata); >>> + } >>> + >>> + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); >>> + if (!mem) { >>> + dev_warn(&pdev->dev, >>> + "\"mpu\" mem resource not found, using index 0\n"); >>> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + if (!mem) { >>> + dev_err(&pdev->dev, "no mem resource?\n"); >>> + return -ENODEV; >>> + } >>> + } >>> + >>> io_base = devm_ioremap_resource(&pdev->dev, mem); >>> if (IS_ERR(io_base)) >>> return PTR_ERR(io_base); >>> @@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device >>> *pdev) >>> (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG); >>> /* first TX, then RX */ >>> - res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >>> - if (!res) { >>> - dev_err(&pdev->dev, "no DMA resource\n"); >>> - ret = -ENXIO; >>> - goto err_release_clk; >>> - } >>> dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; >>> - *dma = res->start; >>> dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma; >>> + res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >>> + if (res) >>> + *dma = res->start; >>> + else >>> + *dma = pdata->tx_dma_channel; >> Please follow the way davinci-mcasp does it right now: >> dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; >> ... >> dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; >> res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >> if (res) >> *dma = res->start; >> else >> *dma = pdata->tx_dma_channel; >> >> /* dmaengine filter data for DT and non-DT boot */ >> if (pdev->dev.of_node) >> dma_data->filter_data = "tx"; >> else >> dma_data->filter_data = dma; >> >> while we are here, I think the tx/rx_dma_channel is something we should just >> get rid of. It is not used as far as I can tell. >> Something like this would do I think: >> dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK]; >> ... >> res = platform_get_resource(pdev, IORESOURCE_DMA, 0); >> if (res) { >> dma = &mcasp->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; >> *dma = res->start; >> dma_data->filter_data = dma; >> } else if (pdev->dev.of_node) { >> dma_data->filter_data = "tx"; >> } else { >> dev_err(&pdev->dev, "Missing DMA tx resource\n"); >> return -ENODEV; >> } > Is the edma pcm filter function able to filter in once case by the dma number > and in the other case by the dma name? > See my above comment as well... The filter_fn is for the legacy boot, DT does not use filter, it looks up the channel. The reason for the failure in your case was that the dma bindings in mcbsps were not correct, if you update them they will work. > >>> dev->dev = &pdev->dev; >>> dev_set_drvdata(&pdev->dev, dev); >>> @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device >>> *pdev) >>> return 0; >>> } >>> +static const struct of_device_id davinci_mcbsp_match[] = { >>> + { .compatible = "ti,da850-mcbsp-audio" }, >> I would drop the "-audio" for the binding... > I was trying to stay consistent with the mcasp. But I don't mind keeping the > name shorter :-) Not just shorter, but the -audio postfix is just stupid. IMHO. >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match); >>> + >>> static struct platform_driver davinci_mcbsp_driver = { >>> .probe = davinci_i2s_probe, >>> .remove = davinci_i2s_remove, >>> .driver = { >>> .name = "davinci-mcbsp", >>> + .of_match_table = of_match_ptr(davinci_mcbsp_match), >> The driver calls itself davinci-i2s.c and the prefixes internally used are >> davinci_i2s_*. >> The driver name is "davinci-mcbsp". >> And it is basically a driver for daVinci ASP (the McBSP additions are not >> configured at all). >> I still think we should keep the davinci_i2s_* when adding new code, or rename >> the whole driver and it's prefixes to something more sensible? > Good point, thanks! I will rename the table to davinci_i2s_match to stay > consistent. > > Petr > >
diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig index 50ca291cc225..6b732d8e5896 100644 --- a/sound/soc/davinci/Kconfig +++ b/sound/soc/davinci/Kconfig @@ -16,7 +16,11 @@ config SND_EDMA_SOC - DRA7xx family config SND_DAVINCI_SOC_I2S - tristate + tristate "DaVinci Multichannel Buffered Serial Port (McBSP) support" + depends on SND_EDMA_SOC + help + Say Y or M here if you want to have support for McBSP IP found in + Texas Instruments DaVinci DA850 SoCs. config SND_DAVINCI_SOC_MCASP tristate "Multichannel Audio Serial Port (McASP) support" diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c index ec98548a5fc9..2ebfe86dd584 100644 --- a/sound/soc/davinci/davinci-i2s.c +++ b/sound/soc/davinci/davinci-i2s.c @@ -4,9 +4,15 @@ * Author: Vladimir Barinov, <vbarinov@embeddedalley.com> * Copyright: (C) 2007 MontaVista Software, Inc., <source@mvista.com> * + * DT support (c) 2016 Petr Kulhavy, Barix AG <petr@barix.com> + * based on davinci-mcasp.c DT support + * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. + * + * TODO: + * on DA850 implement HW FIFOs instead of DMA into DXR and DRR registers */ #include <linux/init.h> @@ -648,15 +654,82 @@ static const struct snd_soc_component_driver davinci_i2s_component = { .name = "davinci-i2s", }; +static struct snd_platform_data* +davinci_i2s_set_pdata_from_of(struct platform_device *pdev) +{ + struct snd_platform_data *pdata = NULL; + struct device_node *np; + struct of_phandle_args dma_spec; + int ret; + + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) + return dev_get_platdata(&pdev->dev); + + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + np = pdev->dev.of_node; + + ret = of_property_match_string(np, "dma-names", "tx"); + if (ret >= 0) { + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, + &dma_spec); + if (ret >= 0) + pdata->tx_dma_channel = dma_spec.args[0]; + } + + ret = of_property_match_string(np, "dma-names", "rx"); + if (ret >= 0) { + ret = of_parse_phandle_with_args(np, "dmas", "#dma-cells", ret, + &dma_spec); + if (ret >= 0) + pdata->rx_dma_channel = dma_spec.args[0]; + } + + /* optional parameters */ + + pdata->enable_channel_combine = + of_property_read_bool(np, "channel-combine"); + + /* + * pdata->clk_input_pin is deliberately not exported to DT + * and the default value of the clk_input_pin is MCBSP_CLKR. + * The value MCBSP_CLKS makes no sense as it turns the CPU + * to a bit clock master in the SND_SOC_DAIFMT_CBM_CFS mode + * where it should be bit clock slave! + */ + + return pdata; +} + static int davinci_i2s_probe(struct platform_device *pdev) { + struct snd_platform_data *pdata; struct davinci_mcbsp_dev *dev; struct resource *mem, *res; void __iomem *io_base; int *dma; int ret; - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pdata = davinci_i2s_set_pdata_from_of(pdev); + if (IS_ERR(pdata)) { + dev_err(&pdev->dev, "Error populating platform data, err %ld\n", + PTR_ERR(pdata)); + return PTR_ERR(pdata); + } + + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpu"); + if (!mem) { + dev_warn(&pdev->dev, + "\"mpu\" mem resource not found, using index 0\n"); + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(&pdev->dev, "no mem resource?\n"); + return -ENODEV; + } + } + io_base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(io_base)) return PTR_ERR(io_base); @@ -680,25 +753,21 @@ static int davinci_i2s_probe(struct platform_device *pdev) (dma_addr_t)(mem->start + DAVINCI_MCBSP_DRR_REG); /* first TX, then RX */ - res = platform_get_resource(pdev, IORESOURCE_DMA, 0); - if (!res) { - dev_err(&pdev->dev, "no DMA resource\n"); - ret = -ENXIO; - goto err_release_clk; - } dma = &dev->dma_request[SNDRV_PCM_STREAM_PLAYBACK]; - *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = dma; + res = platform_get_resource(pdev, IORESOURCE_DMA, 0); + if (res) + *dma = res->start; + else + *dma = pdata->tx_dma_channel; - res = platform_get_resource(pdev, IORESOURCE_DMA, 1); - if (!res) { - dev_err(&pdev->dev, "no DMA resource\n"); - ret = -ENXIO; - goto err_release_clk; - } dma = &dev->dma_request[SNDRV_PCM_STREAM_CAPTURE]; - *dma = res->start; dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].filter_data = dma; + res = platform_get_resource(pdev, IORESOURCE_DMA, 1); + if (res) + *dma = res->start; + else + *dma = pdata->rx_dma_channel; dev->dev = &pdev->dev; dev_set_drvdata(&pdev->dev, dev); @@ -737,11 +806,18 @@ static int davinci_i2s_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id davinci_mcbsp_match[] = { + { .compatible = "ti,da850-mcbsp-audio" }, + {}, +}; +MODULE_DEVICE_TABLE(of, davinci_mcbsp_match); + static struct platform_driver davinci_mcbsp_driver = { .probe = davinci_i2s_probe, .remove = davinci_i2s_remove, .driver = { .name = "davinci-mcbsp", + .of_match_table = of_match_ptr(davinci_mcbsp_match), }, };
This adds DT support for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx McBSP driver. Signed-off-by: Petr Kulhavy <petr@barix.com> --- sound/soc/davinci/Kconfig | 6 ++- sound/soc/davinci/davinci-i2s.c | 106 ++++++++++++++++++++++++++++++++++------ 2 files changed, 96 insertions(+), 16 deletions(-)