| Message ID | 1559643115-15124-2-git-send-email-stu.hsieh@mediatek.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add mediatek mipicsi driver for Mediatek SOC MT2712 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi, Stu: "mediatek,mt2712-mipicsi" and "mediatek,mt2712-mipicsi-common" have many common part with "mediatek,mt8183-seninf", and I've a discussion in [1], so I would like these two to be merged together. [1] https://patchwork.kernel.org/patch/10979131/ Regards, CK On Tue, 2019-06-04 at 18:11 +0800, Stu Hsieh wrote: > Add MIPI-CSI2 dt-binding for Mediatek MT2712 SoC > > Signed-off-by: Stu Hsieh <stu.hsieh@mediatek.com> > --- > .../bindings/media/mediatek-mipicsi.txt | 58 +++++++++++++++++++ > 1 file changed, 58 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek-mipicsi.txt > > diff --git a/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt b/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt > new file mode 100644 > index 000000000000..e30b6a468129 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt > @@ -0,0 +1,58 @@ > +* Mediatek MIPI-CSI2 receiver > + > +Mediatek MIPI-CSI2 receiver is the MIPI Signal capture hardware present in Mediatek SoCs > + > +Required properties: > +- compatible: should be "mediatek,mt2712-mipicsi" > +- reg : physical base address of the mipicsi receiver registers and length of > + memory mapped region. > +- 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. > +- iommus: should point to the respective IOMMU block with master port as > + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > + for details. > +- mediatek,seninf_mux_camsv: seninf_mux_camsv the data go through of the mipicsi port > + any mipicsi port can contain max four seninf_mux_camsv > + The Total seninf_mux_camsv is six for mt2712 > +- mediatek,mipicsiid: the id of the mipicsi port, there are two port for mt2712 > +- mediatek,mipicsi: the common component of the two mipicsi port > +- mediatek,mipicsi_max_vc: the number of virtual channel which subdev used > +- mediatek,serdes_link_reg: the register of subdev to get the link status > + > +Example: > + mipicsi0: mipicsi@10217000 { > + compatible = "mediatek,mt2712-mipicsi"; > + mediatek,mipicsi = <&mipicsi>; > + iommus = <&iommu0 M4U_PORT_CAM_DMA0>, > + <&iommu0 M4U_PORT_CAM_DMA1>; > + mediatek,larb = <&larb2>; > + power-domains = <&scpsys MT2712_POWER_DOMAIN_ISP>; > + > + mediatek,seninf_mux_camsv = <&seninf1_mux_camsv0 > + &seninf2_mux_camsv1 > + &seninf3_mux_camsv2 > + &seninf4_mux_camsv3>; > + reg = <0 0x10217000 0 0x60>, > + <0 0x15002100 0 0x4>, > + <0 0x15002300 0 0x100>; > + mediatek,mipicsiid = <0>; > + mediatek,mipicsi_max_vc = <4>; > + mediatek,serdes_link_reg = <0x49>; > + }; > + > + mipicsi1: mipicsi@10218000 { > + compatible = "mediatek,mt2712-mipicsi"; > + mediatek,mipicsi = <&mipicsi>; > + iommus = <&iommu0 M4U_PORT_CAM_DMA2>; > + mediatek,larb = <&larb2>; > + power-domains = <&scpsys MT2712_POWER_DOMAIN_ISP>; > + mediatek,seninf_mux_camsv = <&seninf5_mux_camsv4 > + &seninf6_mux_camsv5>; > + reg = <0 0x10218000 0 0x60>, > + <0 0x15002500 0 0x4>, > + <0 0x15002700 0 0x100>; > + mediatek,mipicsiid = <1>; > + };
Hi CK, Stu, On Mon, Jun 10, 2019 at 11:34 AM CK Hu <ck.hu@mediatek.com> wrote: > > Hi, Stu: > > "mediatek,mt2712-mipicsi" and "mediatek,mt2712-mipicsi-common" have many > common part with "mediatek,mt8183-seninf", and I've a discussion in [1], > so I would like these two to be merged together. > > [1] https://patchwork.kernel.org/patch/10979131/ > Thanks CK for spotting this. I also noticed that the driver in fact handles two hardware blocks at the same time - SenInf and CamSV. Unless the architecture is very different from MT8183, I'd suggest splitting it. On a general note, the MT8183 SenInf driver has received several rounds of review comments already, but I couldn't find any comments posted for this one. Given the two aspects above and also based on my quick look at code added by this series, I'd recommend adding MT2712 support on top of the MT8183 series. Best regards, Tomasz
Hi, Tomasz: On Mon, 2019-06-10 at 12:32 +0900, Tomasz Figa wrote: > Hi CK, Stu, > > On Mon, Jun 10, 2019 at 11:34 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > Hi, Stu: > > > > "mediatek,mt2712-mipicsi" and "mediatek,mt2712-mipicsi-common" have many > > common part with "mediatek,mt8183-seninf", and I've a discussion in [1], > > so I would like these two to be merged together. > > > > [1] https://patchwork.kernel.org/patch/10979131/ > > > > Thanks CK for spotting this. > > I also noticed that the driver in fact handles two hardware blocks at > the same time - SenInf and CamSV. Unless the architecture is very > different from MT8183, I'd suggest splitting it. > > On a general note, the MT8183 SenInf driver has received several > rounds of review comments already, but I couldn't find any comments > posted for this one. > > Given the two aspects above and also based on my quick look at code > added by this series, I'd recommend adding MT2712 support on top of > the MT8183 series. In [1], "mediatek,mt8183-seninf" use one device to control multiple csi instance, so it duplicate many register definition. In [2], one "mediatek,mt2712-mipicsi" device control one csi instance, so there are multiple device and the register definition does not duplicate. You recommend adding MT2712 support on top of the MT8183 series, do you mean that "mediatek,mt2712-mipicsi" should use one device to control multiple csi instance and duplicate the register setting? [1] https://patchwork.kernel.org/patch/10979121/ [2] https://patchwork.kernel.org/patch/10974573/ Regards, CK > > Best regards, > Tomasz
On Mon, Jun 10, 2019 at 4:51 PM CK Hu <ck.hu@mediatek.com> wrote: > > Hi, Tomasz: > > On Mon, 2019-06-10 at 12:32 +0900, Tomasz Figa wrote: > > Hi CK, Stu, > > > > On Mon, Jun 10, 2019 at 11:34 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > > > Hi, Stu: > > > > > > "mediatek,mt2712-mipicsi" and "mediatek,mt2712-mipicsi-common" have many > > > common part with "mediatek,mt8183-seninf", and I've a discussion in [1], > > > so I would like these two to be merged together. > > > > > > [1] https://patchwork.kernel.org/patch/10979131/ > > > > > > > Thanks CK for spotting this. > > > > I also noticed that the driver in fact handles two hardware blocks at > > the same time - SenInf and CamSV. Unless the architecture is very > > different from MT8183, I'd suggest splitting it. > > > > On a general note, the MT8183 SenInf driver has received several > > rounds of review comments already, but I couldn't find any comments > > posted for this one. > > > > Given the two aspects above and also based on my quick look at code > > added by this series, I'd recommend adding MT2712 support on top of > > the MT8183 series. > > In [1], "mediatek,mt8183-seninf" use one device to control multiple csi > instance, so it duplicate many register definition. In [2], one > "mediatek,mt2712-mipicsi" device control one csi instance, so there are > multiple device and the register definition does not duplicate. I guess we didn't catch that in the review yet. It should be fixed. > You > recommend adding MT2712 support on top of the MT8183 series, do you mean > that "mediatek,mt2712-mipicsi" should use one device to control multiple > csi instance and duplicate the register setting? There are some aspects of MT8183 series that are done better than the MT2712 series, but apparently there are also some better aspects in MT2712. We should take the best aspects of both series. :) Best regards, Tomasz > > [1] https://patchwork.kernel.org/patch/10979121/ > [2] https://patchwork.kernel.org/patch/10974573/ > > Regards, > CK > > > > > Best regards, > > Tomasz > >
On Mon, Jun 10, 2019 at 04:58:02PM +0900, Tomasz Figa wrote: > On Mon, Jun 10, 2019 at 4:51 PM CK Hu <ck.hu@mediatek.com> wrote: > > > > Hi, Tomasz: > > > > On Mon, 2019-06-10 at 12:32 +0900, Tomasz Figa wrote: > > > Hi CK, Stu, > > > > > > On Mon, Jun 10, 2019 at 11:34 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > > > > > Hi, Stu: > > > > > > > > "mediatek,mt2712-mipicsi" and "mediatek,mt2712-mipicsi-common" have many > > > > common part with "mediatek,mt8183-seninf", and I've a discussion in [1], > > > > so I would like these two to be merged together. > > > > > > > > [1] https://patchwork.kernel.org/patch/10979131/ > > > > > > > > > > Thanks CK for spotting this. > > > > > > I also noticed that the driver in fact handles two hardware blocks at > > > the same time - SenInf and CamSV. Unless the architecture is very > > > different from MT8183, I'd suggest splitting it. > > > > > > On a general note, the MT8183 SenInf driver has received several > > > rounds of review comments already, but I couldn't find any comments > > > posted for this one. > > > > > > Given the two aspects above and also based on my quick look at code > > > added by this series, I'd recommend adding MT2712 support on top of > > > the MT8183 series. > > > > In [1], "mediatek,mt8183-seninf" use one device to control multiple csi > > instance, so it duplicate many register definition. In [2], one > > "mediatek,mt2712-mipicsi" device control one csi instance, so there are > > multiple device and the register definition does not duplicate. > > I guess we didn't catch that in the review yet. It should be fixed. > > > You > > recommend adding MT2712 support on top of the MT8183 series, do you mean > > that "mediatek,mt2712-mipicsi" should use one device to control multiple > > csi instance and duplicate the register setting? > > There are some aspects of MT8183 series that are done better than the > MT2712 series, but apparently there are also some better aspects in > MT2712. We should take the best aspects of both series. :) Stu: Are the two devices similar enough or not; does this look like a feasible approach to you?
diff --git a/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt b/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt new file mode 100644 index 000000000000..e30b6a468129 --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek-mipicsi.txt @@ -0,0 +1,58 @@ +* Mediatek MIPI-CSI2 receiver + +Mediatek MIPI-CSI2 receiver is the MIPI Signal capture hardware present in Mediatek SoCs + +Required properties: +- compatible: should be "mediatek,mt2712-mipicsi" +- reg : physical base address of the mipicsi receiver registers and length of + memory mapped region. +- 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. +- iommus: should point to the respective IOMMU block with master port as + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt + for details. +- mediatek,seninf_mux_camsv: seninf_mux_camsv the data go through of the mipicsi port + any mipicsi port can contain max four seninf_mux_camsv + The Total seninf_mux_camsv is six for mt2712 +- mediatek,mipicsiid: the id of the mipicsi port, there are two port for mt2712 +- mediatek,mipicsi: the common component of the two mipicsi port +- mediatek,mipicsi_max_vc: the number of virtual channel which subdev used +- mediatek,serdes_link_reg: the register of subdev to get the link status + +Example: + mipicsi0: mipicsi@10217000 { + compatible = "mediatek,mt2712-mipicsi"; + mediatek,mipicsi = <&mipicsi>; + iommus = <&iommu0 M4U_PORT_CAM_DMA0>, + <&iommu0 M4U_PORT_CAM_DMA1>; + mediatek,larb = <&larb2>; + power-domains = <&scpsys MT2712_POWER_DOMAIN_ISP>; + + mediatek,seninf_mux_camsv = <&seninf1_mux_camsv0 + &seninf2_mux_camsv1 + &seninf3_mux_camsv2 + &seninf4_mux_camsv3>; + reg = <0 0x10217000 0 0x60>, + <0 0x15002100 0 0x4>, + <0 0x15002300 0 0x100>; + mediatek,mipicsiid = <0>; + mediatek,mipicsi_max_vc = <4>; + mediatek,serdes_link_reg = <0x49>; + }; + + mipicsi1: mipicsi@10218000 { + compatible = "mediatek,mt2712-mipicsi"; + mediatek,mipicsi = <&mipicsi>; + iommus = <&iommu0 M4U_PORT_CAM_DMA2>; + mediatek,larb = <&larb2>; + power-domains = <&scpsys MT2712_POWER_DOMAIN_ISP>; + mediatek,seninf_mux_camsv = <&seninf5_mux_camsv4 + &seninf6_mux_camsv5>; + reg = <0 0x10218000 0 0x60>, + <0 0x15002500 0 0x4>, + <0 0x15002700 0 0x100>; + mediatek,mipicsiid = <1>; + };
Add MIPI-CSI2 dt-binding for Mediatek MT2712 SoC Signed-off-by: Stu Hsieh <stu.hsieh@mediatek.com> --- .../bindings/media/mediatek-mipicsi.txt | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/mediatek-mipicsi.txt