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

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

On 08/09/16 15:08, Mars Cheng wrote:
> Hi Marc
> 
> Thanks for your review. the response inlined.
> 
> On Thu, 2016-09-08 at 13:37 +0100, Marc Zyngier wrote:
>> 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>
>>> ---
> [...]
>>
>>> 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.
> 
> Will fix this in the next patch set.
> 
>>
>> - 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.
>>
> 
> May I know the meaning of "simply expose it as a separate controller"?

At the moment, you have something like this:

	sysirq: intpol-controller@10200620 {
		compatible = "mediatek,mt6755-sysirq",
			     "mediatek,mt6577-sysirq";
		interrupt-controller;
		#interrupt-cells = <3>;
		interrupt-parent = <&gic>;
		reg = <0 0x10200620 0 0x20>;
	};

I suggest that, when you have a second base (which is effectively
another controller), you add:

	sysirq2: intpol-controller@10201620 {
		compatible = "mediatek,mt6755-sysirq",
			     "mediatek,mt6577-sysirq";
		interrupt-controller;
		#interrupt-cells = <3>;
		interrupt-parent = <&gic>;
		irq-base = <32>;
		reg = <0 0x10201620 0 0x20>;
	};

Where irq-base is the first SPI this is connected to (the lack of
property indicates implies that irq-base is 0). This becomes a very
simple change in the driver.

> Or you might like to suggest me any similar driver as a reference? I
> will examine it. Current design is based on the fact: We expect
> irq-mtk-sysirq needs the optional second base but the third one will not
> happen.
> 
> If we really need more than 2 bases, we can figure out a more generic
> driver at the time, right?

I'd rather fix the driver and the binding to do the right thing once and
for all. In my experience, you will need to add a third base in six
months, and a fourth soon after. I'd rather either support an arbitrary
number of bases, or a single one per controller (and have multiple
controllers).

Thanks,

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