[5/5,v4] ASoC: dwc: Update DOCUMENTATION for I2S Driver
diff mbox

Message ID ce97e6bb7ddb71e532c4066334ceead4d4d3e692.1460047238.git.joabreu@synopsys.com
State New
Headers show

Commit Message

Jose Abreu April 7, 2016, 4:53 p.m. UTC
This patch updates documentation for the Designware I2S
driver.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---

This patch was only introduced in v4.

 Documentation/devicetree/bindings/sound/designware-i2s.txt | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Mark Brown April 7, 2016, 5:53 p.m. UTC | #1
On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:

> + Optional properties:
> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
> +   it is required to use the properties 'dmas' and 'dma-names'.

This is not a good interface, it's describing Linux internal APIs.  If
the device needs to operate in PIO mode it should just do that.
Jose Abreu April 8, 2016, 10:06 a.m. UTC | #2
Hi Mark,


On 07-04-2016 18:53, Mark Brown wrote:
> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>
>> + Optional properties:
>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>> +   it is required to use the properties 'dmas' and 'dma-names'.
> This is not a good interface, it's describing Linux internal APIs.  If
> the device needs to operate in PIO mode it should just do that.

I added this interface because there is no direct way to check if DMA is
available on the I2S controller so it is not possible to automatically change
between DMA and PIO mode. As the I2S controller can be built with or without DMA
support it is necessary to somehow check if DMA is enabled or not and according
to that use either ALSA DMA engine or the custom platform driver sent in these
patches. I did not want to remove drivers functionality so I added this property
to the DT. This way a user can select between DMA and PIO mode.

Is there a better option to do this without removing the possibility of using
ALSA DMA engine in the driver?

Best regards,
Jose Miguel Abreu
Lars-Peter Clausen April 8, 2016, 3:52 p.m. UTC | #3
On 04/08/2016 12:06 PM, Jose Abreu wrote:
> Hi Mark,
> 
> 
> On 07-04-2016 18:53, Mark Brown wrote:
>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>
>>> + Optional properties:
>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>> This is not a good interface, it's describing Linux internal APIs.  If
>> the device needs to operate in PIO mode it should just do that.
> 
> I added this interface because there is no direct way to check if DMA is
> available on the I2S controller so it is not possible to automatically change
> between DMA and PIO mode. As the I2S controller can be built with or without DMA
> support it is necessary to somehow check if DMA is enabled or not and according
> to that use either ALSA DMA engine or the custom platform driver sent in these
> patches. I did not want to remove drivers functionality so I added this property
> to the DT. This way a user can select between DMA and PIO mode.

That's OK, but you need to describe the hardware, not the indented behavior
of the software driver.
Jose Abreu April 8, 2016, 4:08 p.m. UTC | #4
Hi Lars,


On 08-04-2016 16:52, Lars-Peter Clausen wrote:
> On 04/08/2016 12:06 PM, Jose Abreu wrote:
>> Hi Mark,
>>
>>
>> On 07-04-2016 18:53, Mark Brown wrote:
>>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>>
>>>> + Optional properties:
>>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>>> This is not a good interface, it's describing Linux internal APIs.  If
>>> the device needs to operate in PIO mode it should just do that.
>> I added this interface because there is no direct way to check if DMA is
>> available on the I2S controller so it is not possible to automatically change
>> between DMA and PIO mode. As the I2S controller can be built with or without DMA
>> support it is necessary to somehow check if DMA is enabled or not and according
>> to that use either ALSA DMA engine or the custom platform driver sent in these
>> patches. I did not want to remove drivers functionality so I added this property
>> to the DT. This way a user can select between DMA and PIO mode.
> That's OK, but you need to describe the hardware, not the indented behavior
> of the software driver.
>

Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S controller
has DMA support. If set the properties 'dmas' and 'dma-names' must be also set" ?

Best regards,
Jose Miguel Abreu
Lars-Peter Clausen April 9, 2016, 2:55 p.m. UTC | #5
On 04/08/2016 06:08 PM, Jose Abreu wrote:
> Hi Lars,
> 
> 
> On 08-04-2016 16:52, Lars-Peter Clausen wrote:
>> On 04/08/2016 12:06 PM, Jose Abreu wrote:
>>> Hi Mark,
>>>
>>>
>>> On 07-04-2016 18:53, Mark Brown wrote:
>>>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>>>
>>>>> + Optional properties:
>>>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>>>> This is not a good interface, it's describing Linux internal APIs.  If
>>>> the device needs to operate in PIO mode it should just do that.
>>> I added this interface because there is no direct way to check if DMA is
>>> available on the I2S controller so it is not possible to automatically change
>>> between DMA and PIO mode. As the I2S controller can be built with or without DMA
>>> support it is necessary to somehow check if DMA is enabled or not and according
>>> to that use either ALSA DMA engine or the custom platform driver sent in these
>>> patches. I did not want to remove drivers functionality so I added this property
>>> to the DT. This way a user can select between DMA and PIO mode.
>> That's OK, but you need to describe the hardware, not the indented behavior
>> of the software driver.
>>
> 
> Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S controller
> has DMA support. If set the properties 'dmas' and 'dma-names' must be also set" ?

The description is better. But the name of the property is still imperative
rather then descriptive. It tells the software what should be done rather
then describing what the hardware looks like.

Since there is already the dmas property which is present if a DMA is
connected and is absent when no DMA is present it should be enough to just
check that property rather than requiring an additional one.
Jose Abreu April 11, 2016, 9:24 a.m. UTC | #6
Hi Lars,


On 09-04-2016 15:55, Lars-Peter Clausen wrote:
> On 04/08/2016 06:08 PM, Jose Abreu wrote:
>> Hi Lars,
>>
>>
>> On 08-04-2016 16:52, Lars-Peter Clausen wrote:
>>> On 04/08/2016 12:06 PM, Jose Abreu wrote:
>>>> Hi Mark,
>>>>
>>>>
>>>> On 07-04-2016 18:53, Mark Brown wrote:
>>>>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>>>>
>>>>>> + Optional properties:
>>>>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>>>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>>>>> This is not a good interface, it's describing Linux internal APIs.  If
>>>>> the device needs to operate in PIO mode it should just do that.
>>>> I added this interface because there is no direct way to check if DMA is
>>>> available on the I2S controller so it is not possible to automatically change
>>>> between DMA and PIO mode. As the I2S controller can be built with or without DMA
>>>> support it is necessary to somehow check if DMA is enabled or not and according
>>>> to that use either ALSA DMA engine or the custom platform driver sent in these
>>>> patches. I did not want to remove drivers functionality so I added this property
>>>> to the DT. This way a user can select between DMA and PIO mode.
>>> That's OK, but you need to describe the hardware, not the indented behavior
>>> of the software driver.
>>>
>> Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S controller
>> has DMA support. If set the properties 'dmas' and 'dma-names' must be also set" ?
> The description is better. But the name of the property is still imperative
> rather then descriptive. It tells the software what should be done rather
> then describing what the hardware looks like.
>
> Since there is already the dmas property which is present if a DMA is
> connected and is absent when no DMA is present it should be enough to just
> check that property rather than requiring an additional one.

Ok, will then use the DMA property to decide which mode to use: PIO or DMA.

>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Best regards,
Jose Miguel Abreu

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/designware-i2s.txt b/Documentation/devicetree/bindings/sound/designware-i2s.txt
index 7bb5424..f3b5c17 100644
--- a/Documentation/devicetree/bindings/sound/designware-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/designware-i2s.txt
@@ -7,6 +7,10 @@  Required properties:
    clocks. The controller expects one clock: the clock used as the sampling
    rate reference clock sample.
  - clock-names : "i2sclk" for the sample rate reference clock.
+
+ Optional properties:
+ - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
+   it is required to use the properties 'dmas' and 'dma-names'.
  - dmas: Pairs of phandle and specifier for the DMA channels that are used by
    the core. The core expects one or two dma channels: one for transmit and
    one for receive.
@@ -26,6 +30,7 @@  Example:
 		clocks = <&scpi_i2sclk 0>;
 		clock-names = "i2sclk";
 		#sound-dai-cells = <0>;
+		snps,use-dmaengine;
 		dmas = <&dma0 5>;
 		dma-names = "tx";
 	};