> Am 28.09.2021 um 11:18 schrieb Maxime Ripard <maxime@xxxxxxxxxx>:
>
> On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote:
>>>> +properties:
>>>> + compatible:
>>>> + items:
>>>> + - const: ingenic,jz4780-dw-hdmi
>>>
>>> This can just be a const, there's no need for the items
>>
>> Maybe starting with an enum is better if more compatible strings are to be added.
>
> it's still fairly easy to change if needed, there's no need to confuse
> anyone.
>
>>>
>>>> + reg-io-width:
>>>> + const: 4
>>>
>>> If it's fixed, why do you need it in the first place?
>>
>> There is a fixed default of 1 if not specified.
>
> My point was more about why do you need to have that property at all?
> Can't you just drop it and assume that the register width is 32 bits if
> it's all you will ever run on?
No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from:
reg-io-width:
description:
Width (in bytes) of the registers specified by the reg property.
allOf:
- $ref: /schemas/types.yaml#/definitions/uint32
- enum: [1, 4]
default: 1
Other bindings define it explicitly to be 4, e.g.
Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml: reg-io-width:
Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml: reg-io-width:
Therefore I'd assume that a regmap is not properly set up
if we don't require the DTS to include it with const: 4.
>
>>>> + clocks:
>>>> + maxItems: 2
>>>> + description: Clock specifiers for isrf and iahb clocks
>>>
>>> This can be defined as
>>>
>>> clocks:
>>> items:
>>> - description: isrf
>>> - description: iahb
>>>
>>> A better description about what these clocks are would be nice as well
>>
>> Generally I see that this all is nowadays not independent of
>>
>> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml
>>
>> where there is already a description.
>
> Ok, good then
>
>> On the other hand every SoC specialization runs its own copy. e.g.
>>
>> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml
>> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam
>>
>>>
>>>> + clock-names:
>>>> + items:
>>>> + - const: isfr
>>>
>>> Is it isfr or isrf?
>>
>> isfr. Seems to be a typo in the description. See
>> bridge/synopsys,dw-hdmi.yaml#
>>
>> One question to the yaml specialists:
>>
>> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we
>> have to repeat? Or can we reduce to just the changes?
>
> If you add the ref you mentionned above, you don't have to repeat
Nice. It defines:
clocks:
minItems: 2
maxItems: 5
items:
- description: The bus clock for either AHB and APB
- description: The internal register configuration clock
additionalItems: true
> yourself indeed. You can just put clock-names: true
Or should we do
clocks:
maxItems: 2
additionalItems: false
>
>> [I am still not familiar enough with the yaml stuff to understand if
>> it has sort of inheritance like device tree include files, so that you
>> just have to change relevant properties]
>
> Kind of, but not entirely. schemas are all applied separately, unlike DT
> includes that will just expand to one big DT. In practice, it means that
> your device must validate against all the schemas, not just the sum of
> them.
>
> For example, if you have a generic schema that has:
>
> properties:
> compatible:
> const: vendor,my-generic-compatible
>
>
> and your schema that extends the generic binding, with a ref to the
> generic one that has:
>
> properties:
> compatible:
> items:
> - const: other-vendor,my-device-compatible
> - const: vendor,my-generic-compatible
>
>
> It will still fail since the generic schema expects only a single
> compatible, whereas your device would have two.
Ok, I see. it is not a simple "overwrite" rule.
>
>>>
>>>> + - const: iahb
>>
>> would it make sense to add additionalItems: false here?
>>
>> In the jz4780 case there are just two clocks while other specializations
>> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true.
>
> If you want to refine the generic one, and it's all the clocks you ever
> expect then there's no need for additionalItems
Ok.
>
>>>
>>>> + description: An I2C interface if the internal DDC I2C driver is not to be used
>>>> + ports: true
>>>
>>> If there's a single port, you don't need ports
>>
>> There can be two ports - one for input from LCDC and one
>> for output (HDMI connector). But explicitly defining an output
>> port is optional to some extent (depending on driver structure).
>
> This needs to be defined then (and port@0 made mandatory)
Ok. I'll try to make the best out of it for v5 series. Maybe
it is still not perfect by then, but close...
>
> Maxime
BR and thanks,
Nikolaus
