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

[Marc Zyngier <marc.zyngier@arm.com>]

On 08/09/16 11:49, Mars Cheng wrote:
> This adds DT binding documentation for Mediatek MT6797.
> 
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> ---
>  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...