Re: [PATCH v3 02/16] dt-bindings: media: mtk-vcodec: document SCP node

[Alexandre Courbot <acourbot@chromium.org>]

On Thu, Jul 23, 2020 at 6:37 AM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> On Mon, 13 Jul 2020 at 03:09, Alexandre Courbot <acourbot@chromium.org> wrote:
> >
> > The mediatek codecs can use either the VPU or the SCP as their interface
> > to firmware. Reflect this in the DT bindings.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/media/mediatek-vcodec.txt | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> > index b6b5dde6abd8..7aef0a4fe207 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> > +++ b/Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> > @@ -19,7 +19,9 @@ Required properties:
> >  - iommus : should point to the respective IOMMU block with master port as
> >    argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> >    for details.
> > -- mediatek,vpu : the node of video processor unit
> > +One of the two following nodes:
> > +- mediatek,vpu : the node of the video processor unit, if using VPU.
> > +- mediatek,scp : the noode of the SCP unit, if using SCP.
> >
>
> This interface doesn't enforce the fact only one of the two
> should be present, but not both (which is the case, right?).

That's correct.

>
> I hope I'm not bikeshedding here, but from an interface POV,
> would it be cleaner to just have a single mediatek,coprocessor
> property, and then use of_device_is_compatible
> to distinguish VPU from SCP type?

From an interface point of view maybe, however doing so would
introduce a backward-incompatible change with the existing MT8173
bindings. I also feel like it is less error-prone to have the property
explicitly state what it is expecting at the other end of the phandle
(vpu or scp) instead of the more generic "coprocessor".

>
> Moreover, I'd argue you don't need a dt-binding change
> and should just keep the current mediatek-vpu property,
> and then rely on of_device_is_compatible.

VPU and SCP are different kinds of processors, so I'm not sure whether
it is desirable to use VPU interchangeably like this. Note that I'm
not strongly against it either, but for things like bindings I tend to
prefer precise language to avoid confusions.