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@xxxxxxxxxxxx>
>>> ---
> [...]
>>
>>> 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...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
