Message-ID: <dec4b60d5e01066a6d3d665af35b6d4b6bb4206d.camel@mediatek.com>
Date: Wed, 20 Aug 2025 08:24:01 +0000
From: Irui Wang (王瑞) <Irui.Wang@...iatek.com>
To: Kyrie Wu (吴晗) <Kyrie.Wu@...iatek.com>,
	"krzk@...nel.org" <krzk@...nel.org>
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>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"nhebert@...omium.org" <nhebert@...omium.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

Dear Krzysztof,

Thanks for your reviewing.

On Wed, 2025-08-20 at 07:55 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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?

We need put the lore links in the change log, I mean put the previous
version(v2) patchset lore link? We should to indicate the changes in
each version in the change log and we did the same. If each version
lore links or URL is needed, we will start add them in next patch. 

>  Why aren't you using b4 to submit
> patches?

We have been using 'git send-email' to submit patches to community, b4
is not deployed ready on my server yet, we can study it first.

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

Maybe I would like to say:

phandle to the VCP node, the video encoder kernel driver uses this
phandle to coordinate with the encoder software driver which running in
VCP firmware for command submission and interrupt handling during
encoding process.

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

Currently, just mt8196 use VCP devices, so 'mediatek,vcp' property is
required on mt8196 platform, sorry I didn't get your opinion why 'each variant/device has now VCP.' 

> 
> You have entire commit msg to explain all this.
> 
> 
> Best regards,
> Krzysztof

Thanks again for your kindly reviewing.

Best regards

Powered by blists - more mailing lists