Re: [PATCH] dt-bindings: media: st,stmipid02: Convert the text bindings to YAML

[Benjamin MUGNIER <benjamin.mugnier@foss.st.com>]

Hi Marek,

On 9/27/22 20:45, Marek Vasut wrote:
> On 9/27/22 13:06, Benjamin MUGNIER wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Thank you for your patch.
> 
> Thank you for the review.
> 
> [...]
> 
>>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml
>>> @@ -0,0 +1,175 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/st,st-mipid02.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: STMicroelectronics MIPID02 CSI-2 to PARALLEL bridge
>>> +
>>> +maintainers:
>>> +  - Mickael Guene <mickael.guene@foss.st.com>
>>
>> As Mickael is not the maintainer of the driver anymore, there should be both current maintainers instead, as listed in the MAINTAINER file:
>>   - Benjamin Mugnier <benjamin.mugnier@foss.st.com>
>>   - Sylvain Petinot <sylvain.petinot@foss.st.com>
>> Can you change it while at it ? Thank you.
> 
> Done and done, I also dropped myself from the list, which I hope is OK.
> 

No problem for dropping yourself, out of curiosity why so ?

>> By the way you will also need to update the MAINTAINERS file to change the st,st-mipid02.txt file to st,st-mipid02.yaml here:
>>    F:      Documentation/devicetree/bindings/media/i2c/st,st-mipid02.txt
> 
> [...]
> 
>>> +      port@2:
>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>> +        unevaluatedProperties: false
>>> +        description: Output port
>>> +        properties:
>>> +          endpoint:
>>> +            $ref: /schemas/media/video-interfaces.yaml#
>>> +            unevaluatedProperties: false
>>> +
>>> +            properties:
>>> +              bus-width:
>>> +                enum: [8, 10, 12, 14]
>>
>> Shouldn't this be [6, 8, 10, 12] as in the original file ?
> 
> They are both wrong, per:
> 
> https://www.st.com/resource/en/datasheet/stmipid02.pdf
> page 5
> Table 1. Image format vs. number of bits on output interface
> row 2
> RAW7 RAW7 7 bits
> 
> So this should be
> [6, 7, 8, 10, 12]
> 

Good catch. Thank you.

>>> +                default: 8
>>
>> As far as I understand there is no 'default' as this property is required anyway.
> 
> Dropped.
> 
>>> +              hsync-active: true
>>> +              vsync-active: true
>>> +
>>> +            required:
>>> +              - bus-width
>>> +
>>> +    required:
>>> +      - port@0
>>
>> Actually it could either be port0 or port1, but not both at the same time, see the 'description' part.
>> I'm not sure how to describe it in device tree binding though.
> 
> I think the requirement here is to have at least one of port@0 or port@1 described in DT. Both port@0 and port@1 can also be described, i.e. you can have FRONT and BACK sensor like on a contemporary smartphone, you just cannot start streaming from both sensors at the same time, which is a software policy and should be handled by the driver.

Definitely, the device tree should describe connected ports. This is the software's responsibility to not stream from both.
Rob's suggestion is perfect.


Regards,

Benjamin

> 
> See:
> https://www.st.com/resource/en/datasheet/stmipid02.pdf
> page 3
> 2 Application diagrams
> Figure 2. MIPI CSI-2 application diagram
> 
> Per suggestion from Rob, I think this should be
> 
> allOf:
>   - require:
>       - port@0
>   - require:
>       - port@1
> require:
>   - port@2
> 
>>> +      - port@2
>>> +
>>> +additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>
>> I noticed this was missing in the original txt file. Thanks for taking care of it.
> 
> Sure. I'm glad to see dtbs_check passing on the stm32mp15xx boards, largely thanks to ST doing good job keeping the upstream support in good shape.