Re: [PATCH v3 3/5] media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/04/2025 12:03, Dave Stevenson wrote:
> Hi Krzysztof
> 
> On Fri, 25 Apr 2025 at 08:53, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Wed, Apr 23, 2025 at 06:20:20PM GMT, Dave Stevenson wrote:
>>> Adds a binding for the HEVC decoder found on th +maintainers:
>>> +  - John Cox <john.cox@raspberrypi.com>
>>> +  - Dom Cobley <dom@raspberrypi.com>
>>> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>>> +  - Raspberry Pi internal review list <kernel-list@raspberrypi.com>
>>
>> Drop, no mailing lists in bindings maintainers. These must be people.
> 
> Ack
> 
>>> +
>>> +description:
>>> +  The Raspberry Pi HEVC decoder is a hardware video decode accelerator block
>>> +  found in the BCM2711 and BCM2712 processors used on Raspberry Pi 4 and 5
>>> +  boards respectively.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - brcm,bcm2711-hevc-dec
>>> +          - brcm,bcm2712-hevc-dec
>>> +      - const: raspberrypi,hevc-dec
>>
>> Not what Rob asked. You should use specific SoC compatible as fallback.
> 
> In which case I don't understand what Rob was asking for.
> I asked for clarification in [1], but got no reply. Sending a new
> version has at least got an answer, but I'm none the wiser.
> 
> Staring at this trying to work out your meaning, you want the generic
> first, and SoC specific second? ie
> +  compatible:
> +    items:
> +      - const: raspberrypi,hevc-dec

Drop

> +      - enum:

That's enum, not fallback.

> +          - brcm,bcm2711-hevc-dec
> +          - brcm,bcm2712-hevc-dec
> 
>> You referred to file "raspberrypi,pisbe.yaml" before, but there is no
>> such file in the next.
> 
> Typo.
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/media/raspberrypi%2Cpispbe.yaml
> Reviewed by Rob only just over a year ago [2]

There were some discussions and reasons with explanations.

Feel free to use same arguments and accurately describe the hardware, so
we won't be needing to ask usual questions.

Anyway, we asked for fallback, so you need items, just like every SoC
compatible (see also example schema).


> 
>> Before you reply that there is a binding using different rules: well,
>> there is always poor code. Above two comments are repeated, especially
>> this about specific compatible - all the time, so these are not new
>> rules. These are given in reviews since some years.
> 
> My Google-foo is totally failing with the only directly relevant
> mention of "fallback compatible" I find is [3], which just says to use
> them.
> 
> You're effectively saying I can't take anything in the kernel tree as
> being a valid example as it could be poor code, and a layman such as
> myself has no way of telling.

No, I am saying that argument "I saw someone doing this, so I am allowed
to do the same" is not correct. There are good and bad examples. For
example in my talks I mentioned good examples. The list of good examples
was not accepted to the kernel so well... I just use as an example any
recent Qcom binding using specific compatibles.

> Could you please point me at documentation and examples I can rely on,
> or educate me with what is wanted in this situation to avoid me having
> to guess?
> 
> A further mailing list search has brought up [4] which is a thread
> with yourself from 2 years ago which looks to be a very similar
> situation. Other than missing the const on the SoC strings (although
> that isn't in the merged version of cnm,wave521c.yaml), and two SoC
> specific strings, I'm not seeing an obvious difference between there
> and here either.

How is the [4] relevant? That's IP block from other vendor.

https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html#properties

https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31

Best regards,
Krzysztof