Re: [PATCH v8 2/3] dt-bindings: media: mediatek-jpeg-encoder: add MT8189 compatible string

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

On Tue, May 19, 2026 at 02:35:51AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The commit silently introduces a new `mediatek,larb` property and makes it mandatory for the MT8189 variant without any mention in the commit message.
> - [Low] The new compatible string `mediatek,mt8189-jpgenc` is defined using an `enum` containing a single item instead of `const`.
> --
> 
> commit de42f55ad078326b54b85cf561d120b901b074be
> Author: Jianhua Lin <jianhua.lin@mediatek.com>
> 
> dt-bindings: media: mediatek-jpeg-encoder: add MT8189 compatible string
> 
> This commit adds the compatible string for the JPEG encoder block found
> in the MediaTek MT8189 SoC. It is added as a standalone compatible string
> without falling back to the generic identifier to ensure proper 34-bit IOVA
> address space support.
> 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> > index 5b15f8977f67f..f75871e72633b 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> > @@ -14,13 +14,17 @@ description: |-
> >  
> >  properties:
> >    compatible:
> > -    items:
> > +    oneOf:
> >        - enum:
> > -          - mediatek,mt2701-jpgenc
> > -          - mediatek,mt8183-jpgenc
> > -          - mediatek,mt8186-jpgenc
> > -          - mediatek,mt8188-jpgenc
> > -      - const: mediatek,mtk-jpgenc
> > +          - mediatek,mt8189-jpgenc
> 
> This isn't a bug, but dt_binding_check usually warns about using an enum with
> a single item. Should this use const instead?

No it doesn't. Also "This isn't a bug, but it is a bug" is how this
sentence reads, makes no sense.

It should only be a const if there's not likely to be other devices
joining the enum.

> 
> > +      - items:
> > +          - enum:
> > +              - mediatek,mt2701-jpgenc
> > +              - mediatek,mt8183-jpgenc
> > +              - mediatek,mt8186-jpgenc
> > +              - mediatek,mt8188-jpgenc
> > +          - const: mediatek,mtk-jpgenc
> > +
> 
> [ ... ]
> 
> > @@ -45,6 +49,10 @@ properties:
> >        Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for details.
> >        Ports are according to the HW.
> >  
> > +  mediatek,larb:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: A phandle to the smi_larb node.
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -54,6 +62,16 @@ required:
> >    - power-domains
> >    - iommus
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: mediatek,mt8189-jpgenc
> > +    then:
> > +      required:
> > +        - mediatek,larb
> 
> The commit message mentions the standalone compatible string is needed for
> 34-bit IOVA address space support, but it doesn't mention adding the new
> mediatek,larb property or making it mandatory for the MT8189 variant.
> 
> Could the commit message be updated to explain the purpose and necessity of
> this new hardware constraint?

Yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]