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

["Irui Wang (王瑞)" <Irui.Wang@mediatek.com>]

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?

> 
> > 
> > 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
vcp device, for platforms using vcp firmware."

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. 

> 
> 
> Best regards,
> Krzysztof
> 

Thanks again for your kindly reviewing.

Best Regards