Subject: Re: [PATCH v4] ASoC: dt-bindings: davinci-mcasp: convert McASP
 bindings to yaml schema
To:     =?UTF-8?Q?P=c3=a9ter_Ujfalusi?= <peter.ujfalusi@gmail.com>,
        <robh+dt@kernel.org>
CC:     <lgirdwood@gmail.com>, <broonie@kernel.org>,
        <alsa-devel@alsa-project.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20211203120243.24173-1-j-choudhary@ti.com>
 <ed6c9be9-32d3-719a-ee0d-608b228f36b3@gmail.com>
From:   Jayesh Choudhary <j-choudhary@ti.com>
Message-ID: <20449d7b-0524-a8df-7852-a4c495157682@ti.com>
Date:   Tue, 7 Dec 2021 10:33:01 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <ed6c9be9-32d3-719a-ee0d-608b228f36b3@gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 04/12/21 1:50 pm, Péter Ujfalusi wrote:
> Hi,
> 
> On 03/12/2021 14:02, Jayesh Choudhary wrote:
>> Convert the bindings for McASP controllers for TI SOCs from txt
>> to YAML schema.
>>
>> Adds additional properties 'clocks', 'clock-names', 'power-domains',
>> '#sound-dai-cells' and 'port' which were missing from txt file.
>> Removes properties 'sram-size-playback' and 'sram-size-capture'
>> since they are not used.
>> Adds 'dmas' and 'dma-names' in the example which were missing from
>> the txt file.
>> Changes 'interrupts' and 'interrupt-names' from optional to
>> required properties.
>> Changes 'op-mode', 'serial-dir' and 'tdm-slots' to optional properties
>> as they are not needed if the McASP is used only as GPIO.
>>
>> Adds the yaml file in the 'MAINTAINERS' under the heading 'TEXAS
>> INSTRUMENTS ASoC DRIVERS'
>>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>> Changelog:
>> v4:
>> - changes the commit message
>> - adds type and description to relevant properties
>> - changes maxItems for 'serial-dir'
>> - removes properties 'sram-size-playback' and 'sram-size-capture'
>>    as they are not used
>> - removes 'function-gpios'
>> - removes 'num-serializer'
>> - marks 'tdm-slots', 'serial-dir' and 'op-mode' as optional properties
>> - adds the yaml file in MAINTAINERS
>>
> 
> ...
> 
>> +  reg:
>> +    minItems: 1
>> +    items:
>> +      - description: main registers
> 
> The TRM refers to this area as CFG registers
> 
>> +      - description: data port register
> 
> Data registers
> 
>> +
>> +  op-mode:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: I2S - 0 or DIT - 1 operation mode
> 
> 0 - I2S, or 1 - DIT operation mode
> 

I will change the description for 'reg' and 'op-mode'.

>> +    enum:
>> +      - 0
>> +      - 1
>> +
>> +  tdm-slots:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: number of channels over one serializer
>> +    maxItems: 1
> 
> and it has to be between 2 and 32, ignored in DIT mode (384 slots)
> 

Will add minimum and maximum. Should this be added as a conditional
property when op-mode is 0 (I2S mode) and mark it as required?

>> +
>> +  serial-dir:
>> +    description:
>> +      A list of serializer configuration
>> +      Entry is indication for serializer pin direction
>> +      0 - Inactive, 1 - TX, 2 - RX
>> +      All AXR pins should be present in the array even if inactive
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 1
>> +    maxItems: 25
>> +    items:
>> +      minimum: 0
>> +      maximum: 2
>> +      default: 0
> 
> There is no default as all pins must be accounted for.
> 

Okay. Will remove it.

>> +
>> +  tx-num-evt:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: configures WFIFO threshold
> 
> 0 disables the FIFO use
> If the property is missing, it also disables the FIFO use.
> 

Will mention this in the description of both tx/rx-num-evt.

>> +    maxItems: 1
>> +
>> +  rx-num-evt:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: configures RFIFO threshold
>> +    maxItems: 1

>> +
>> +  interrupts:
>> +    anyOf:
>> +      - minItems: 1
>> +        items:
>> +          - description: TX FIFO interrupt
>> +          - description: RX FIFO interrupt
>> +      - items:
>> +          - description: common FIFO interrupt
> 
> These has nothing to do with FIFO, they are just transmit, receive and
> common or combined
> 

Okay.

>> +
>> +  port:
>> +    description: connection for when McASP is used via graph card
>> +    type: object
> 
> I understand that it can be present under the mcasp node as it is part
> of the graph card binding (or a card binding using graph).
> I mean if a new card binding comes around then we need to document it
> here as well?
> 

Specific properties are not marked for the port. So it should not be an
issue. Other alternative is to mark the additional properties as true
but that is not preferred.

Peter,
Any other changes I should make?

>> +
>