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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aab06f96-83c9-4e1c-9e1b-6148a3055e67@kernel.org>
Date: Wed, 20 Aug 2025 07:55:38 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Irui Wang (王瑞) <Irui.Wang@...iatek.com>,
 Kyrie Wu (吴晗) <Kyrie.Wu@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 George Sun (孙林) <George.Sun@...iatek.com>,
 Tiffany Lin (林慧珊) <tiffany.lin@...iatek.com>,
 "andrzejtp2010@...il.com" <andrzejtp2010@...il.com>,
 "nhebert@...omium.org" <nhebert@...omium.org>,
 "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "mchehab@...nel.org" <mchehab@...nel.org>,
 "hverkuil@...all.nl" <hverkuil@...all.nl>,
 "nicolas.dufresne@...labora.com" <nicolas.dufresne@...labora.com>,
 Yunfei Dong (董云飞) <Yunfei.Dong@...iatek.com>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "robh@...nel.org" <robh@...nel.org>,
 "sebastian.fricke@...labora.com" <sebastian.fricke@...labora.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "christophe.jaillet@...adoo.fr" <christophe.jaillet@...adoo.fr>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>, "arnd@...db.de" <arnd@...db.de>,
 Andrew-CT Chen (陳智迪)
 <Andrew-CT.Chen@...iatek.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 "neil.armstrong@...aro.org" <neil.armstrong@...aro.org>
Subject: Re: [PATCH v3 5/6] dt-bindings: media: Add MT8196
 mediatek,vcodec-encoder

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@...iatek.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@...iatek.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