[PATCH 0/6] ASoC: davinci-mcbsp: add binding for McBSP

[PATCH 0/6] ASoC: davinci-mcbsp: add binding for McBSP

[Petr Kulhavy]

This series of patches adds devicetree support for the davinci McBSP audio
interface.

[PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Petr Kulhavy]

Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx
MultiChannel Buffered Serial Port (McBSP)

The optional register range "dat" is not implemented at the moment.
The current driver supports only DMA into RX/TX registers but no FIFO.
Once the FIFO is implemented in the driver the "dat" range will be used.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 .../bindings/sound/davinci-mcbsp-audio.txt         | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt

diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
new file mode 100644
index 000000000000..f60fceb927dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
@@ -0,0 +1,57 @@
+Texas Instruments DaVinci McBSP module
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
+audio interface found in some TI DaVinci processors like e.g. the DA850,
+DM6446, DA355.
+
+
+Required properties:
+~~~~~~~~~~~~~~~~~~~~
+- compatible : "ti,da850-mcbsp-audio"
+
+- reg : physical base address and length of the controller memory mapped
+        region(s).
+- reg-names : Should contain:
+         * "mpu" for the main registers (required). For compatibility with
+           existing software, it is recommended this is the first entry.
+         * "dat" for the data FIFO (optional).
+
+- dmas: two element list of DMA controller phandles and DMA request line
+        ordered pairs.
+- dma-names: identifier string for each DMA request line in the dmas property.
+	     These strings correspond 1:1 with the ordered pairs in dmas. The dma
+	     identifiers must be "rx" and "tx".
+
+Optional properties:
+~~~~~~~~~~~~~~~~~~~~
+- interrupts : Interrupt numbers for McBSP
+- interrupt-names : Known interrupt names are "rx" and "tx"
+
+- pinctrl-0: Should specify pin control group used for this controller.
+- pinctrl-names: Should contain only one value - "default", for more details
+  		 please refer to pinctrl-bindings.txt
+
+- channel-combine : boolean. If present L and R channels are combined into one
+		DMA transfer, however the labelling of the channels is swapped.
+		Therefore this option should be used only if the channels can
+		be swapped back at the codec side again.
+
+Example (AM1808):
+~~~~~~~~~~~~~~~~~
+
+mcbsp0: mcbsp@1d10000 {
+	compatible = "ti,davinci-mcbsp-audio";
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcbsp0_pins>;
+
+	reg = 	<0x00110000 0x1000>,
+		<0x00310000 0x1000>;
+	reg-names = "mpu", "dat";
+	interrupts = <97 98>;
+	interrupts-names = "rx", "tx";
+	dmas = <&edma0 3
+		&edma0 2>;
+	dma-names = "tx", "rx";
+	status = "okay";
+};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Peter Ujfalusi]

On 04/06/16 16:21, Petr Kulhavy wrote:
> Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx
> MultiChannel Buffered Serial Port (McBSP)
> 
> The optional register range "dat" is not implemented at the moment.
> The current driver supports only DMA into RX/TX registers but no FIFO.
> Once the FIFO is implemented in the driver the "dat" range will be used.
> 
> Signed-off-by: Petr Kulhavy <petr@barix.com>
> ---
>  .../bindings/sound/davinci-mcbsp-audio.txt         | 57 ++++++++++++++++++++++

I would drop the -audio postfix.
I know the McASP introduced this and it is annoying. Let's not repeat it again...

>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
> new file mode 100644
> index 000000000000..f60fceb927dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
> @@ -0,0 +1,57 @@
> +Texas Instruments DaVinci McBSP module
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
> +audio interface found in some TI DaVinci processors like e.g. the DA850,
> +DM6446, DA355.

Given that the driver is actually a driver for daVinci ASP and it completely
ignores registers introduced when the IP is renamed from ASP to McBSP, should
we say something about this? That the ASP is compatible or subset of McBSP
(w/o the multichannel support) and these bindings could be used for ASP, with
adding new compatible?
Or just leave that out and bother with it when we have such a device booting
with DT?

> +
> +
> +Required properties:
> +~~~~~~~~~~~~~~~~~~~~
> +- compatible : "ti,da850-mcbsp-audio"
> +
> +- reg : physical base address and length of the controller memory mapped
> +        region(s).
> +- reg-names : Should contain:
> +         * "mpu" for the main registers (required). For compatibility with
> +           existing software, it is recommended this is the first entry.
> +         * "dat" for the data FIFO (optional).
> +
> +- dmas: two element list of DMA controller phandles and DMA request line
> +        ordered pairs.
> +- dma-names: identifier string for each DMA request line in the dmas property.
> +	     These strings correspond 1:1 with the ordered pairs in dmas. The dma
> +	     identifiers must be "rx" and "tx".
> +
> +Optional properties:
> +~~~~~~~~~~~~~~~~~~~~
> +- interrupts : Interrupt numbers for McBSP
> +- interrupt-names : Known interrupt names are "rx" and "tx"
> +
> +- pinctrl-0: Should specify pin control group used for this controller.
> +- pinctrl-names: Should contain only one value - "default", for more details
> +  		 please refer to pinctrl-bindings.txt
> +
> +- channel-combine : boolean. If present L and R channels are combined into one
> +		DMA transfer, however the labelling of the channels is swapped.
> +		Therefore this option should be used only if the channels can
> +		be swapped back at the codec side again.
> +
> +Example (AM1808):
> +~~~~~~~~~~~~~~~~~
> +
> +mcbsp0: mcbsp@1d10000 {
> +	compatible = "ti,davinci-mcbsp-audio";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mcbsp0_pins>;
> +
> +	reg = 	<0x00110000 0x1000>,
> +		<0x00310000 0x1000>;
> +	reg-names = "mpu", "dat";
> +	interrupts = <97 98>;
> +	interrupts-names = "rx", "tx";
> +	dmas = <&edma0 3
> +		&edma0 2>;
> +	dma-names = "tx", "rx";
> +	status = "okay";
> +};
> 


-- 
Péter

Re: [alsa-devel] [PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Petr Kulhavy]

On 07.04.2016 15:33, Peter Ujfalusi wrote:
> On 04/06/16 16:21, Petr Kulhavy wrote:
>> Add devicetree binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx
>> MultiChannel Buffered Serial Port (McBSP)
>>
>> The optional register range "dat" is not implemented at the moment.
>> The current driver supports only DMA into RX/TX registers but no FIFO.
>> Once the FIFO is implemented in the driver the "dat" range will be used.
>>
>> Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
>> ---
>>   .../bindings/sound/davinci-mcbsp-audio.txt         | 57 ++++++++++++++++++++++
> I would drop the -audio postfix.
> I know the McASP introduced this and it is annoying. Let's not repeat it again...
>
I'm going to correct it, thanks.

>>   1 file changed, 57 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
>> new file mode 100644
>> index 000000000000..f60fceb927dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/davinci-mcbsp-audio.txt
>> @@ -0,0 +1,57 @@
>> +Texas Instruments DaVinci McBSP module
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
>> +audio interface found in some TI DaVinci processors like e.g. the DA850,
>> +DM6446, DA355.
> Given that the driver is actually a driver for daVinci ASP and it completely
> ignores registers introduced when the IP is renamed from ASP to McBSP, should
> we say something about this? That the ASP is compatible or subset of McBSP
> (w/o the multichannel support) and these bindings could be used for ASP, with
> adding new compatible?
> Or just leave that out and bother with it when we have such a device booting
> with DT?
It would be nice to mention both but if there is only one compatible 
string then it doesn't make much sense. And saying "you can add your own 
compatible string" is the binding not the right place to say this.
But could we add another compatible string for the older ASP?

Petr
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Petr Kulhavy]

On 07.04.2016 15:33, Peter Ujfalusi wrote:

> Given that the driver is actually a driver for daVinci ASP and it 
> completely ignores registers introduced when the IP is renamed from 
> ASP to McBSP, should we say something about this? That the ASP is 
> compatible or subset of McBSP (w/o the multichannel support) and these 
> bindings could be used for ASP, with adding new compatible? Or just 
> leave that out and bother with it when we have such a device booting 
> with DT? 
Hi Peter,

what do you think about the following formulation?

--
This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
audio interface found in some TI DaVinci processors like the OMAP-L138 
or AM180x.

Some older DaVinci processors like the DM6446 or DA355 use a simpler 
version of
the digital audio interface, called ASP. This driver can support them as 
well if
the appropriate compatible string is added.
--

Thanks
Petr

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Peter Ujfalusi]

On 04/08/16 12:24, Petr Kulhavy wrote:
> On 07.04.2016 15:33, Peter Ujfalusi wrote:
> 
>> Given that the driver is actually a driver for daVinci ASP and it completely
>> ignores registers introduced when the IP is renamed from ASP to McBSP,
>> should we say something about this? That the ASP is compatible or subset of
>> McBSP (w/o the multichannel support) and these bindings could be used for
>> ASP, with adding new compatible? Or just leave that out and bother with it
>> when we have such a device booting with DT? 
> Hi Peter,
> 
> what do you think about the following formulation?
> 
> -- 
> This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
> audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
> 
> Some older DaVinci processors like the DM6446 or DA355 use a simpler version of
> the digital audio interface, called ASP. This driver can support them as well if
> the appropriate compatible string is added.

Hrm, I don't think we should mention the ASP, it looks out of context.
The ASP will have compatible of ti,dm644x-asp or something similar. We should
worry about this when the time comes.

-- 
Péter

Re: [alsa-devel] [PATCH 1/6] ASoC: davinci-mcbsp: add binding for McBSP

[Petr Kulhavy]

On 11.04.2016 10:11, Peter Ujfalusi wrote:
> On 04/08/16 12:24, Petr Kulhavy wrote:
>>
>> Hi Peter,
>>
>> what do you think about the following formulation?
>>
>> -- 
>> This binding describes the "Multi-channel Buffered Serial Port" (McBSP)
>> audio interface found in some TI DaVinci processors like the OMAP-L138 or AM180x.
>>
>> Some older DaVinci processors like the DM6446 or DA355 use a simpler version of
>> the digital audio interface, called ASP. This driver can support them as well if
>> the appropriate compatible string is added.
> Hrm, I don't think we should mention the ASP, it looks out of context.
> The ASP will have compatible of ti,dm644x-asp or something similar. We should
> worry about this when the time comes.
OK, I will keep just the first paragraph.

Thanks!
Petr
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] ASoC: Davinci: McBSP: add device tree support for McBSP

[Petr Kulhavy]

This adds DT support for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx McBSP driver.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 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-L1vi/lXTdtvkgf6YlCu6wwC/G2K4zDHf@public.gmane.org>
  * Copyright:   (C) 2007 MontaVista Software, Inc., <source-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
  *
+ * DT support	(c) 2016 Petr Kulhavy, Barix AG <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
+ *		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),
 	},
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] ASoC: Davinci: McBSP: add device tree support for McBSP

[Peter Ujfalusi]

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?


>  	},
>  };
>  
> 


-- 
Péter

Re: [alsa-devel] [PATCH 2/6] ASoC: Davinci: McBSP: add device tree support for McBSP

[Petr Kulhavy]

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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] ASoC: Davinci: McBSP: add device tree support for McBSP

[Peter Ujfalusi]

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


-- 
Péter

[PATCH 3/6] ARM: davinci: da850: add clocks for mcbsp0 and 1

[Petr Kulhavy]

Add clock definitions "davinci-mcbsp.0" and "davinci-mcbsp.1" in order
to make McBSP driver work on the DA850 platform.

The McBSP 0 and 1 interfaces were not defined for the DA850 platform.
Neither were the related clocks. In order to make the use of McBSP via
devicetree the clocks need to be defined.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/mach-davinci/da850.c | 16 ++++++++++++++++
 arch/arm/mach-davinci/psc.h   |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 97d8779a9a65..2b9d972e30ac 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -306,6 +306,20 @@ static struct clk mcasp_clk = {
 	.flags		= DA850_CLK_ASYNC3,
 };
 
+static struct clk mcbsp0_clk = {
+	.name		= "mcbsp0",
+	.parent		= &pll1_sysclk2,
+	.lpsc		= DA850_LPSC1_McBSP0,
+	.gpsc		= 1,
+};
+
+static struct clk mcbsp1_clk = {
+	.name		= "mcbsp1",
+	.parent		= &pll1_sysclk2,
+	.lpsc		= DA850_LPSC1_McBSP1,
+	.gpsc		= 1,
+};
+
 static struct clk lcdc_clk = {
 	.name		= "lcdc",
 	.parent		= &pll0_sysclk2,
@@ -464,6 +478,8 @@ static struct clk_lookup da850_clks[] = {
 	CLK("davinci_emac.1",	NULL,		&emac_clk),
 	CLK("davinci_mdio.0",	"fck",		&emac_clk),
 	CLK("davinci-mcasp.0",	NULL,		&mcasp_clk),
+	CLK("davinci-mcbsp.0",	NULL,		&mcbsp0_clk),
+	CLK("davinci-mcbsp.1",	NULL,		&mcbsp1_clk),
 	CLK("da8xx_lcdc.0",	"fck",		&lcdc_clk),
 	CLK("da830-mmc.0",	NULL,		&mmcsd0_clk),
 	CLK("da830-mmc.1",	NULL,		&mmcsd1_clk),
diff --git a/arch/arm/mach-davinci/psc.h b/arch/arm/mach-davinci/psc.h
index 99d47cfa301f..8af9f09fc10c 100644
--- a/arch/arm/mach-davinci/psc.h
+++ b/arch/arm/mach-davinci/psc.h
@@ -171,6 +171,8 @@
 #define DA8XX_LPSC1_I2C			11
 #define DA8XX_LPSC1_UART1		12
 #define DA8XX_LPSC1_UART2		13
+#define DA850_LPSC1_McBSP0		14
+#define DA850_LPSC1_McBSP1		15
 #define DA8XX_LPSC1_LCDC		16
 #define DA8XX_LPSC1_PWM			17
 #define DA850_LPSC1_MMC_SD1		18
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] ARM: davinci: da8xx-dt: add OF_DEV_AUXDATA entries for mcbsp0 and mcbsp1

[Petr Kulhavy]

Add OF_DEV_AUXDATA for mcbsp to be able to use clocks.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/mach-davinci/da8xx-dt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index c4b5808ca7c1..fd9ac0bcf7a6 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -45,8 +45,13 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ns16550a", 0x01d0d000, "serial8250.2", NULL),
 	OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
 	OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
-		       NULL),
-	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0", NULL),
+			NULL),
+	OF_DEV_AUXDATA("ti,da830-mcasp-audio", 0x01d00000, "davinci-mcasp.0",
+			NULL),
+	OF_DEV_AUXDATA("ti,da850-mcbsp-audio", 0x01d10000, "davinci-mcbsp.0",
+			NULL),
+	OF_DEV_AUXDATA("ti,da850-mcbsp-audio", 0x01d11000, "davinci-mcbsp.1",
+			NULL),
 	{}
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] ARM: DTS: da850: Fix wrong number of interrupts

[Petr Kulhavy]

The DA850 supports interrupts 0 to 100. The number of interrupts
property "ti,intc-size" was wrongly set to 100. Corrected to 101.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/da850.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 226cda76e77c..5996e765e59c 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -19,7 +19,7 @@
 			compatible = "ti,cp-intc";
 			interrupt-controller;
 			#interrupt-cells = <1>;
-			ti,intc-size = <100>;
+			ti,intc-size = <101>;
 			reg = <0xfffee000 0x2000>;
 		};
 	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Petr Kulhavy]

Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
configurations.

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 5996e765e59c..9e2b1e97377c 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -148,7 +148,24 @@
 					0xc 0x88888888 0xffffffff
 				>;
 			};
-
+			mcbsp0_pins: pinmux_mcbsp0_pins {
+				pinctrl-single,bits = <
+					/* PINMUX2:
+					* CLKS0, DX0, DR0, FSX0
+					* FSR0, CLKX0, CLKR0
+					*/
+					0x8 0x02222220 0xfffffff0
+				>;
+			};
+			mcbsp1_pins: pinmux_mcbsp1_pins {
+				pinctrl-single,bits = <
+					/* PINMUX1:
+					* CLKS1, DX1, DR1, FSX1,
+					* FSR1, CLKX1, CLKR1
+					*/
+					0x4 0x22222220 0xfffffff0
+				>;
+			};
 		};
 		edma0: edma@01c00000 {
 			compatible = "ti,edma3-tpcc";
@@ -335,6 +352,32 @@
 				<&edma0 0 1>;
 			dma-names = "tx", "rx";
 		};
+
+		mcbsp0: mcbsp@1d10000 {
+			compatible = "ti,da850-mcbsp-audio";
+			reg =	<0x00110000 0x1000>,
+				<0x00310000 0x1000>;
+			reg-names = "mpu", "dat";
+			interrupts = <97 98>;
+			interrupts-names = "rx", "tx";
+			dmas = <&edma0 3
+				&edma0 2>;
+			dma-names = "tx", "rx";
+			status = "disabled";
+		};
+		mcbsp1: mcbsp@1d11000 {
+			compatible = "ti,da850-mcbsp-audio";
+			reg =	<0x00111000 0x1000>,
+				<0x00311000 0x1000>;
+			reg-names = "mpu", "dat";
+			interrupts = <99 100>;
+			interrupts-names = "rx", "tx";
+			dmas = <&edma0 5
+				&edma0 4>;
+			dma-names = "tx", "rx";
+			status = "disabled";
+		};
+
 	};
 	nand_cs3@62000000 {
 		compatible = "ti,davinci-nand";
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Peter Ujfalusi]

On 04/06/16 16:21, Petr Kulhavy wrote:
> Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
> configurations.
> 
> Signed-off-by: Petr Kulhavy <petr@barix.com>
> ---
>  arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 5996e765e59c..9e2b1e97377c 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -148,7 +148,24 @@
>  					0xc 0x88888888 0xffffffff
>  				>;
>  			};
> -
> +			mcbsp0_pins: pinmux_mcbsp0_pins {
> +				pinctrl-single,bits = <
> +					/* PINMUX2:
> +					* CLKS0, DX0, DR0, FSX0
> +					* FSR0, CLKX0, CLKR0
> +					*/
> +					0x8 0x02222220 0xfffffff0
> +				>;
> +			};
> +			mcbsp1_pins: pinmux_mcbsp1_pins {
> +				pinctrl-single,bits = <
> +					/* PINMUX1:
> +					* CLKS1, DX1, DR1, FSX1,
> +					* FSR1, CLKX1, CLKR1
> +					*/
> +					0x4 0x22222220 0xfffffff0

This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is
used by the board for audio. When I say conflict, I mean that audio will be
completely broken on the board.

> +				>;
> +			};
>  		};
>  		edma0: edma@01c00000 {
>  			compatible = "ti,edma3-tpcc";
> @@ -335,6 +352,32 @@
>  				<&edma0 0 1>;
>  			dma-names = "tx", "rx";
>  		};
> +
> +		mcbsp0: mcbsp@1d10000 {
> +			compatible = "ti,da850-mcbsp-audio";
> +			reg =	<0x00110000 0x1000>,
> +				<0x00310000 0x1000>;
> +			reg-names = "mpu", "dat";
> +			interrupts = <97 98>;
> +			interrupts-names = "rx", "tx";
> +			dmas = <&edma0 3
> +				&edma0 2>;

This will not work since the eDMA now has the new binding in use, you need to
have:
			dmas = <&edma0 3 1>,
				<&edma0 2 1>;

McBSP should also select the higher priority TPTC as the McASP does.

> +			dma-names = "tx", "rx";
> +			status = "disabled";
> +		};
> +		mcbsp1: mcbsp@1d11000 {
> +			compatible = "ti,da850-mcbsp-audio";
> +			reg =	<0x00111000 0x1000>,
> +				<0x00311000 0x1000>;
> +			reg-names = "mpu", "dat";
> +			interrupts = <99 100>;
> +			interrupts-names = "rx", "tx";
> +			dmas = <&edma0 5
> +				&edma0 4>;

Same here.

> +			dma-names = "tx", "rx";
> +			status = "disabled";
> +		};
> +
>  	};
>  	nand_cs3@62000000 {
>  		compatible = "ti,davinci-nand";
> 


-- 
Péter

Re: [alsa-devel] [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Petr Kulhavy]

On 07.04.2016 13:34, Peter Ujfalusi wrote:
> On 04/06/16 16:21, Petr Kulhavy wrote:
>> Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
>> configurations.
>>
>> Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/da850.dtsi | 45 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 5996e765e59c..9e2b1e97377c 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -148,7 +148,24 @@
>>   					0xc 0x88888888 0xffffffff
>>   				>;
>>   			};
>> -
>> +			mcbsp0_pins: pinmux_mcbsp0_pins {
>> +				pinctrl-single,bits = <
>> +					/* PINMUX2:
>> +					* CLKS0, DX0, DR0, FSX0
>> +					* FSR0, CLKX0, CLKR0
>> +					*/
>> +					0x8 0x02222220 0xfffffff0
>> +				>;
>> +			};
>> +			mcbsp1_pins: pinmux_mcbsp1_pins {
>> +				pinctrl-single,bits = <
>> +					/* PINMUX1:
>> +					* CLKS1, DX1, DR1, FSX1,
>> +					* FSR1, CLKX1, CLKR1
>> +					*/
>> +					0x4 0x22222220 0xfffffff0
> This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is
> used by the board for audio. When I say conflict, I mean that audio will be
> completely broken on the board.

I agree with you, the EVM uses the pins for other peripherals. However I 
understand that the da850.dtsi is a generic description of the DA850 
platform.
Other DA850 based designs that use the McBSP will not have conflicts. 
For instance my two AM1808 based boards don't use the McASP.
Of course the board's DTS must enable only non-conflicting 
peripherals/pinmux configurations.
But having the pinmuxes defined does not break anything and actually 
helps creating the DTS file.
Or did I miss something?

>
>> +				>;
>> +			};
>>   		};
>>   		edma0: edma@01c00000 {
>>   			compatible = "ti,edma3-tpcc";
>> @@ -335,6 +352,32 @@
>>   				<&edma0 0 1>;
>>   			dma-names = "tx", "rx";
>>   		};
>> +
>> +		mcbsp0: mcbsp@1d10000 {
>> +			compatible = "ti,da850-mcbsp-audio";
>> +			reg =	<0x00110000 0x1000>,
>> +				<0x00310000 0x1000>;
>> +			reg-names = "mpu", "dat";
>> +			interrupts = <97 98>;
>> +			interrupts-names = "rx", "tx";
>> +			dmas = <&edma0 3
>> +				&edma0 2>;
> This will not work since the eDMA now has the new binding in use, you need to
> have:
> 			dmas = <&edma0 3 1>,
> 				<&edma0 2 1>;
>
> McBSP should also select the higher priority TPTC as the McASP does.
Absolutely, you are right! I still use the old DMA phandles. I will 
correct that.

Petr
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Peter Ujfalusi]

On 04/07/16 15:16, Petr Kulhavy wrote:
> 
> On 07.04.2016 13:34, Peter Ujfalusi wrote:
>> On 04/06/16 16:21, Petr Kulhavy wrote:
>>> Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
>>> configurations.
>>>
>>> Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
>>> ---
>>>   arch/arm/boot/dts/da850.dtsi | 45
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 44 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index 5996e765e59c..9e2b1e97377c 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -148,7 +148,24 @@
>>>                       0xc 0x88888888 0xffffffff
>>>                   >;
>>>               };
>>> -
>>> +            mcbsp0_pins: pinmux_mcbsp0_pins {
>>> +                pinctrl-single,bits = <
>>> +                    /* PINMUX2:
>>> +                    * CLKS0, DX0, DR0, FSX0
>>> +                    * FSR0, CLKX0, CLKR0
>>> +                    */
>>> +                    0x8 0x02222220 0xfffffff0
>>> +                >;
>>> +            };
>>> +            mcbsp1_pins: pinmux_mcbsp1_pins {
>>> +                pinctrl-single,bits = <
>>> +                    /* PINMUX1:
>>> +                    * CLKS1, DX1, DR1, FSX1,
>>> +                    * FSR1, CLKX1, CLKR1
>>> +                    */
>>> +                    0x4 0x22222220 0xfffffff0
>> This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is
>> used by the board for audio. When I say conflict, I mean that audio will be
>> completely broken on the board.
> 
> I agree with you, the EVM uses the pins for other peripherals. However I
> understand that the da850.dtsi is a generic description of the DA850 platform.
> Other DA850 based designs that use the McBSP will not have conflicts. For
> instance my two AM1808 based boards don't use the McASP.
> Of course the board's DTS must enable only non-conflicting peripherals/pinmux
> configurations.
> But having the pinmuxes defined does not break anything and actually helps
> creating the DTS file.

I think what the da850.dtsi does is wrong. The dtsi file should not set any
pinmux, those need to be set by the board .dts files If one board uses McASP0,
it will set up the pins for that and leave McBSP pins as they were, but other
board might use McBSP1 and not use McASP0, there you will have pincontrol for
McBSP1.

> Or did I miss something?
> 
>>
>>> +                >;
>>> +            };
>>>           };
>>>           edma0: edma@01c00000 {
>>>               compatible = "ti,edma3-tpcc";
>>> @@ -335,6 +352,32 @@
>>>                   <&edma0 0 1>;
>>>               dma-names = "tx", "rx";
>>>           };
>>> +
>>> +        mcbsp0: mcbsp@1d10000 {
>>> +            compatible = "ti,da850-mcbsp-audio";
>>> +            reg =    <0x00110000 0x1000>,
>>> +                <0x00310000 0x1000>;
>>> +            reg-names = "mpu", "dat";
>>> +            interrupts = <97 98>;
>>> +            interrupts-names = "rx", "tx";
>>> +            dmas = <&edma0 3
>>> +                &edma0 2>;
>> This will not work since the eDMA now has the new binding in use, you need to
>> have:
>>             dmas = <&edma0 3 1>,
>>                 <&edma0 2 1>;
>>
>> McBSP should also select the higher priority TPTC as the McASP does.
> Absolutely, you are right! I still use the old DMA phandles. I will correct that.
> 
> Petr


-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Petr Kulhavy]

On 07.04.2016 14:45, Peter Ujfalusi wrote:
> On 04/07/16 15:16, Petr Kulhavy wrote:
>> On 07.04.2016 13:34, Peter Ujfalusi wrote:
>>> On 04/06/16 16:21, Petr Kulhavy wrote:
>>>> Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
>>>> configurations.
>>>>
>>>> Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>    arch/arm/boot/dts/da850.dtsi | 45
>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 44 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index 5996e765e59c..9e2b1e97377c 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>> @@ -148,7 +148,24 @@
>>>>                        0xc 0x88888888 0xffffffff
>>>>                    >;
>>>>                };
>>>> -
>>>> +            mcbsp0_pins: pinmux_mcbsp0_pins {
>>>> +                pinctrl-single,bits = <
>>>> +                    /* PINMUX2:
>>>> +                    * CLKS0, DX0, DR0, FSX0
>>>> +                    * FSR0, CLKX0, CLKR0
>>>> +                    */
>>>> +                    0x8 0x02222220 0xfffffff0
>>>> +                >;
>>>> +            };
>>>> +            mcbsp1_pins: pinmux_mcbsp1_pins {
>>>> +                pinctrl-single,bits = <
>>>> +                    /* PINMUX1:
>>>> +                    * CLKS1, DX1, DR1, FSX1,
>>>> +                    * FSR1, CLKX1, CLKR1
>>>> +                    */
>>>> +                    0x4 0x22222220 0xfffffff0
>>> This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is
>>> used by the board for audio. When I say conflict, I mean that audio will be
>>> completely broken on the board.
>> I agree with you, the EVM uses the pins for other peripherals. However I
>> understand that the da850.dtsi is a generic description of the DA850 platform.
>> Other DA850 based designs that use the McBSP will not have conflicts. For
>> instance my two AM1808 based boards don't use the McASP.
>> Of course the board's DTS must enable only non-conflicting peripherals/pinmux
>> configurations.
>> But having the pinmuxes defined does not break anything and actually helps
>> creating the DTS file.
> I think what the da850.dtsi does is wrong. The dtsi file should not set any
> pinmux, those need to be set by the board .dts files If one board uses McASP0,
> it will set up the pins for that and leave McBSP pins as they were, but other
> board might use McBSP1 and not use McASP0, there you will have pincontrol for
> McBSP1.
It does not set the pinmux, it just defines the configurations.
As far as I understand the pin configuration is applied when a node 
includes these two lines:

                         pinctrl-names = "default";
                         pinctrl-0 = <&mcbspc0_pins>;

Petr

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Peter Ujfalusi]

On 04/07/16 15:55, Petr Kulhavy wrote:
> 
> 
> On 07.04.2016 14:45, Peter Ujfalusi wrote:
>> On 04/07/16 15:16, Petr Kulhavy wrote:
>>> On 07.04.2016 13:34, Peter Ujfalusi wrote:
>>>> On 04/06/16 16:21, Petr Kulhavy wrote:
>>>>> Add SoC nodes for McBSP0 and McBSP1 as well as the corresponding pinmux
>>>>> configurations.
>>>>>
>>>>> Signed-off-by: Petr Kulhavy <petr@barix.com>
>>>>> ---
>>>>>    arch/arm/boot/dts/da850.dtsi | 45
>>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 44 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>>> index 5996e765e59c..9e2b1e97377c 100644
>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>> @@ -148,7 +148,24 @@
>>>>>                        0xc 0x88888888 0xffffffff
>>>>>                    >;
>>>>>                };
>>>>> -
>>>>> +            mcbsp0_pins: pinmux_mcbsp0_pins {
>>>>> +                pinctrl-single,bits = <
>>>>> +                    /* PINMUX2:
>>>>> +                    * CLKS0, DX0, DR0, FSX0
>>>>> +                    * FSR0, CLKX0, CLKR0
>>>>> +                    */
>>>>> +                    0x8 0x02222220 0xfffffff0
>>>>> +                >;
>>>>> +            };
>>>>> +            mcbsp1_pins: pinmux_mcbsp1_pins {
>>>>> +                pinctrl-single,bits = <
>>>>> +                    /* PINMUX1:
>>>>> +                    * CLKS1, DX1, DR1, FSX1,
>>>>> +                    * FSR1, CLKX1, CLKR1
>>>>> +                    */
>>>>> +                    0x4 0x22222220 0xfffffff0
>>>> This will conflict with the mcasp0_pins in da850-evm as AXR11 and AXR12 is
>>>> used by the board for audio. When I say conflict, I mean that audio will be
>>>> completely broken on the board.
>>> I agree with you, the EVM uses the pins for other peripherals. However I
>>> understand that the da850.dtsi is a generic description of the DA850 platform.
>>> Other DA850 based designs that use the McBSP will not have conflicts. For
>>> instance my two AM1808 based boards don't use the McASP.
>>> Of course the board's DTS must enable only non-conflicting peripherals/pinmux
>>> configurations.
>>> But having the pinmuxes defined does not break anything and actually helps
>>> creating the DTS file.
>> I think what the da850.dtsi does is wrong. The dtsi file should not set any
>> pinmux, those need to be set by the board .dts files If one board uses McASP0,
>> it will set up the pins for that and leave McBSP pins as they were, but other
>> board might use McBSP1 and not use McASP0, there you will have pincontrol for
>> McBSP1.
> It does not set the pinmux, it just defines the configurations.
> As far as I understand the pin configuration is applied when a node includes
> these two lines:
> 
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&mcbspc0_pins>;

Yeah, true. just ignore my comment for the pinctrl part...

> 
> Petr
> 


-- 
Péter

Re: [alsa-devel] [PATCH 6/6] ARM: DTS: da850: Add McBSP0 and McBSP1

[Sekhar Nori]

On Thursday 07 April 2016 06:15 PM, Peter Ujfalusi wrote:

> I think what the da850.dtsi does is wrong. The dtsi file should not set any
> pinmux, those need to be set by the board .dts files If one board uses McASP0,
> it will set up the pins for that and leave McBSP pins as they were, but other
> board might use McBSP1 and not use McASP0, there you will have pincontrol for
> McBSP1.

Most functions on da850 are available only on one physical pin. So every
board that needs that functionality needs to setup the pinmux the same
way. This was the idea behind adding pinmux to da850.dtsi rather than
repeating it for every board.

Even though the pinctrl entries are there in da850.dtsi, the pinmux
registers wont be touched unless the status is set to "okay" for that
device in board .dts file.

The only care that needs to be taken is that the pins listed in
da850.dtsi need to be the minimum required for achieving that
functionality. If a board needs more than what is populated in
da850.dtsi then it can define its own pinctrl entries overriding that
coming from da850.dtsi

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html