| Message ID | 20200713060842.471356-3-acourbot@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | mtk-vcodec: venc: support for MT8183 and v4l2-compliance fixes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Mon, Jul 13, 2020 at 2:09 PM 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. ^ typo / extra o ChenYu
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?). 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? 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. Regards, Ezequiel > > Example: > -- > 2.27.0.383.g050319c2ae-goog >
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.
On Mon, 27 Jul 2020 at 06:06, Alexandre Courbot <acourbot@chromium.org> wrote: > > 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. Yeah, I guess that makes sense. Not only from a language precision point of view (after all DT are not designed to be human readable). But as you mention, given the processors will have different compatible strings it would make sense to not allow overloading the property. In any case, I don't have a strong opinion either. Thanks, Ezequiel
On Mon, Jul 13, 2020 at 3:20 PM Chen-Yu Tsai <wens@kernel.org> wrote: > > On Mon, Jul 13, 2020 at 2:09 PM 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. > > ^ typo / extra o Fixed, thanks!
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. Example: