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

[Marek Vasut <marex@denx.de>]

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.

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

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

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.