Re: [PATCH v8 11/14] media: dt-bindings: Add jpeg enc device tree node document

[Xia Jiang]

On Thu, 2020-05-21 at 16:00 +0000, Tomasz Figa wrote:
> Hi Xia,
> 
> On Fri, Apr 03, 2020 at 05:40:30PM +0800, Xia Jiang wrote:
> > Add jpeg enc device tree node document
> > 
> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > Signed-off-by: Xia Jiang <xia.jiang@xxxxxxxxxxxx>
> > ---
> > v8: no changes
> > 
> > v7: no changes
> > 
> > v6: no changes
> > 
> > v5: no changes
> > 
> > v4: no changes
> > 
> > v3: change compatible to SoC specific compatible
> > 
> > v2: no changes
> > ---
> >  .../bindings/media/mediatek-jpeg-encoder.txt  | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt
> > 
> 
> Thank you for the patch. Please see my comments inline.
Dear Tomasz,

Sorry for missing this message. Replied below.
> 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt
> > new file mode 100644
> > index 000000000000..fa8da699493b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt
> > @@ -0,0 +1,37 @@
> > +* MediaTek JPEG Encoder
> > +
> > +MediaTek JPEG Encoder is the JPEG encode hardware present in MediaTek SoCs
> > +
> > +Required properties:
> > +- compatible : should be one of:
> > +               "mediatek,mt2701-jpgenc"
> > +               ...
> 
> What does this "..." mean?
"..." means that compatible name is not just "mediatek,mt2701-jpgenc",
different project has different compatible name(for example the MT8173's
compatible name may be "mediatek,mt8173-jpgenc").
> 
> > +               followed by "mediatek,mtk-jpgenc"
> > +- reg : physical base address of the JPEG encoder registers and length of
> > +  memory mapped region.
> > +- interrupts : interrupt number to the interrupt controller.
> > +- clocks: device clocks, see
> > +  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> > +- clock-names: must contain "jpgenc". It is the clock of JPEG encoder.
> 
> nit: In principle the clocks should be named after the function the clock
> performs on the consumer side, i.e. the JPEG block in this case, I guess
> here it's just a generic clock that does everything, but I guess it comes
> from somewhere. Is it the AHB clock or something? In that case it would
> normally be called "ahb".
I have confirmed with hardware designer that the jpeg clock is not AHB
clock,it follows subsys clock(because 2701 is the old IC,I didn't get
the source name).It has the same source with venc clock.We can see that
the clocks = <imgsys CLK_IMG_VENC>, Should I name it "venc" or the
orignal "jpgenc"?
> 
> > +- power-domains: a phandle to the power domain, see
> > +  Documentation/devicetree/bindings/power/power_domain.txt for details.
> > +- mediatek,larb: must contain the local arbiters in the current SoCs, see
> > +  Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
> > +  for details.
> 
> I believe this isn't necessary anymore, because larbs are added
> automatically by the MTK IOMMU driver using device links. +CC Yong who
> worked on that.
Yes,I have confirmed with Yong that he will help me to modify this.Is it
ok that I keep the orignal larb code?

Best Regards,
Xia Jiang
> 
> > +- iommus: should point to the respective IOMMU block with master port as
> > +  argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> > +  for details.
> > +
> > +Example:
> > +	jpegenc: jpegenc@1500a000 {
> > +		compatible = "mediatek,mt2701-jpgenc",
> > +			     "mediatek,mtk-jpgenc";
> > +		reg = <0 0x1500a000 0 0x1000>;
> > +		interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks =  <&imgsys CLK_IMG_VENC>;
> > +		clock-names = "jpgenc";
> > +		power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> > +		mediatek,larb = <&larb2>;
> 
> Ditto.
> 
> Best regards,
> Tomasz