Re: [PATCH 3/4] dt-bindings: serial: document esp32s3-acm bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/09/2023 22:47, Max Filippov wrote:
> On Wed, Sep 13, 2023 at 10:57 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 13/09/2023 23:14, Max Filippov wrote:
>>> Add documentation for the ESP32S3 ACM controller.
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
> 
> Ok.
> 
>>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>>> ---
>>>  .../bindings/serial/esp,esp32-acm.yaml        | 40 +++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml b/Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml
>>> new file mode 100644
>>> index 000000000000..dafbae38aa64
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml
>>> @@ -0,0 +1,40 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/serial/esp,esp32-acm.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: ESP32S3 ACM controller
>>> +
>>> +maintainers:
>>> +  - Max Filippov <jcmvbkbc@gmail.com>
>>> +
>>> +description: |
>>
>> Do not need '|' unless you need to preserve formatting.
> 
> Ok.
> 
>>> +  ESP32S3 ACM controller is a communication device found in the ESP32S3
>>
>> What is "ACM"?
> 
> It's an 'Abstract Control Model' as in USB CDC-ACM: 'Communication Device Class
> - Abstract Control Model'.
> 
>> Why is this in serial? Only serial controllers are in serial.
> 
> Because it's a serial communication device. The SoC TRM calls this peripheral
> 'USB Serial', but the USB part is fixed and is not controllable on the SoC side.
> When you plug it into a host USB socket you get a serial port called ttyACM on
> the host.
> 
>> The description is very vague, way too vague.
> 
> Is the following better?
> 
>   Fixed function USB CDC-ACM device controller of the Espressif ESP32S3 SoC.

Yes.

> 
>>> +  SoC that is connected to one of its USB controllers.
>>
>> Same comments as previous patch.
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: esp,esp32s3-acm
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    acm@60038000 {

So this must be named "serial" now. ACM describes how this is interfaces
to the SoC, right? Otherwise it would not be in "serial" directory and
you would not be able to put serial devices as children.


>>> +            compatible = "esp,esp32s3-acm";
>>
>> Use 4 spaces for example indentation.
> 
> Ok.
> 
>>> +            reg = <0x60038000 0x1000>;
>>> +            interrupts = <96 3 0>;
>>
>> Same comments as previous patch.
> 
> These are not IRQ flags. In any case the contents of the IRQ
> specification cells is not relevant here, right?

Yes, if 0 is not an IRQ flag :)
> 

Best regards,
Krzysztof