Re: [PATCH 1/4] Document: DT: Add bindings for mediatek MT6797 SoC Platform

From: Marc Zyngier
Date: Thu Sep 08 2016 - 08:37:09 EST


On 08/09/16 11:49, Mars Cheng wrote:
> This adds DT binding documentation for Mediatek MT6797.
>
> Signed-off-by: Mars Cheng <mars.cheng@xxxxxxxxxxxx>
> ---
> Documentation/devicetree/bindings/arm/mediatek.txt | 4 ++++
> .../bindings/arm/mediatek/mediatek,apmixedsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,imgsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,infracfg.txt | 1 +
> .../bindings/arm/mediatek/mediatek,mmsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,topckgen.txt | 1 +
> .../bindings/arm/mediatek/mediatek,vdecsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,vencsys.txt | 3 ++-
> .../interrupt-controller/mediatek,sysirq.txt | 4 +++-
> .../devicetree/bindings/serial/mtk-uart.txt | 1 +
> 10 files changed, 16 insertions(+), 2 deletions(-)

[...]

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt b/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
> index 9d1d72c..3d97eb4 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,sysirq.txt
> @@ -8,6 +8,7 @@ Required properties:
> "mediatek,mt8173-sysirq"
> "mediatek,mt8135-sysirq"
> "mediatek,mt8127-sysirq"
> + "mediatek,mt6797-sysirq"
> "mediatek,mt6795-sysirq"
> "mediatek,mt6755-sysirq"
> "mediatek,mt6592-sysirq"
> @@ -21,7 +22,8 @@ Required properties:
> - interrupt-parent: phandle of irq parent for sysirq. The parent must
> use the same interrupt-cells format as GIC.
> - reg: Physical base address of the intpol registers and length of memory
> - mapped region.
> + mapped region. Could be up to 2 registers here at max. Ex: 6797 needs 2 reg,
> + others need 1.

Two things:

- Please make this a separate patch that can be reviewed independently
of the rest of the changes, which are just adding new compatible
identifiers.

- Why can't you simply expose it as a separate controller? Looking at
the way you're changing the corresponding driver, it looks like you're
simply adding an extra base/size. If you simply had a base for the
corresponding GIC interrupts, you could handle as many region as you
want, and have a more generic driver.

Thanks,

M.
--
Jazz is not dead. It just smells funny...