Re: [PATCH v3 5/6] dt-bindings: media: Add MT8196 mediatek,vcodec-encoder

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/08/2025 04:55, Irui Wang (王瑞) wrote:
> Dear Krzysztof,
> 
> Thanks for your reviewing.
> 
> On Fri, 2025-08-15 at 10:54 +0200, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On Thu, Aug 14, 2025 at 04:56:41PM +0800, Kyrie Wu wrote:
>>> From: Irui Wang <irui.wang@mediatek.com>
>>>
>>> Add MT8196 encoder compatible string, which will reference VCP
>>> device.
>>
>> You ignored comments from v2.
> 
> I think I misunderstood the v2 comments, I rewrote the title because it
> said dt-bindings and encoder twice, this is not enough, right? we need
> to describe more information in Body text?

where are lore links in the changelog? Why aren't you using b4 to submit
patches?

> 
>>
>>>
>>> Signed-off-by: Irui Wang <irui.wang@mediatek.com>
>>
>> Incorrect SoB chain.
>>
>>> ---
>>>  .../bindings/media/mediatek,vcodec-encoder.yaml | 17
>>> +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/media/mediatek,vcodec-
>>> encoder.yaml
>>> b/Documentation/devicetree/bindings/media/mediatek,vcodec-
>>> encoder.yaml
>>> index ebc615584f92..bb4dbf23ccc5 100644
>>> --- a/Documentation/devicetree/bindings/media/mediatek,vcodec-
>>> encoder.yaml
>>> +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-
>>> encoder.yaml
>>> @@ -24,6 +24,7 @@ properties:
>>>                - mediatek,mt8188-vcodec-enc
>>>                - mediatek,mt8192-vcodec-enc
>>>                - mediatek,mt8195-vcodec-enc
>>> +              - mediatek,mt8196-vcodec-enc
>>>        - items:
>>>            - const: mediatek,mt8186-vcodec-enc
>>>            - const: mediatek,mt8183-vcodec-enc
>>> @@ -58,6 +59,11 @@ properties:
>>>      description:
>>>        Describes point to scp.
>>>
>>> +  mediatek,vcp:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      Describes point to vcp.
>>
>> For what? You just repeated the property name. You must say here
>> something useful instead: why this is needed, what is its purpose.
> 
> I would like to say:
> "define this 'mediatek,vcp' property here, this is a phandle point to

Don't explain to us what DT is. Above is 100% redundant.

> vcp device, for platforms using vcp firmware."

Explain what is the purpose of this in hardware, how hardware uses it,
what problem for hardware this addresses.


> 
> Is this OK for you?
>>
>>
> 
>>> +
>>>    power-domains:
>>>      maxItems: 1
>>>
>>> @@ -76,6 +82,17 @@ required:
>>>    - iommus
>>>
>>>  allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - mediatek,mt8196-vcodec-enc
>>> +
>>> +    then:
>>> +      required:
>>> +        - mediatek,vcp
>>
>> else
>>
>> mediatek,vcp: false
> 
> I think the else statement is no need here. Different platforms are
> using different firmware phandle, vpu, scp, and vcp. so we use if-then
> to describe the required property for these platforms. 

Hm? I just told you it is needed. Otherwise, explain why each
variant/device has now VCP.

You have entire commit msg to explain all this.


Best regards,
Krzysztof