Re: [PATCH v2 3/6] irqchip: Add the ingenic-tcu-intc driver

[Paul Cercueil <paul@crapouillou.net>]

Hi,


Le mer. 3 janv. 2018 à 21:58, Rob Herring <robh@kernel.org> a écrit :
> On Mon, Jan 01, 2018 at 03:33:41PM +0100, Paul Cercueil wrote:
>>  This simple driver handles the IRQ chip of the TCU
>>  (Timer Counter Unit) of the JZ47xx Ingenic SoCs.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../bindings/interrupt-controller/ingenic,tcu.txt  |  32 +++++
>>   drivers/irqchip/Kconfig                            |   6 +
>>   drivers/irqchip/Makefile                           |   1 +
>>   drivers/irqchip/irq-ingenic-tcu.c                  | 151 
>> +++++++++++++++++++++
>>   4 files changed, 190 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>   create mode 100644 drivers/irqchip/irq-ingenic-tcu.c
>> 
>>   v2: Use SPDX identifier for the license
>>       Make KConfig option select CONFIG_IRQ_DOMAIN since we depend 
>> on it
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>  new file mode 100644
>>  index 000000000000..a3e6318f8461
>>  --- /dev/null
>>  +++ 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>  @@ -0,0 +1,32 @@
>>  +Ingenic SoCs Timer/Counter Unit Interrupt Controller
>>  +
>>  +Required properties:
>>  +
>>  +- compatible : should be "ingenic,<socname>-tcu-intc". Valid 
>> strings are:
>>  +  * ingenic,jz4740-tcu-intc
>>  +  * ingenic,jz4770-tcu-intc
>>  +  * ingenic,jz4780-tcu-intc
>>  +- interrupt-controller : Identifies the node as an interrupt 
>> controller
>>  +- #interrupt-cells : Specifies the number of cells needed to 
>> encode an
>>  +  interrupt source. The value shall be 1.
>>  +- interrupt-parent : phandle of the interrupt controller.
>>  +- interrupts : Specifies the interrupt the controller is connected 
>> to.
>>  +
>>  +Example:
>>  +
>>  +/ {
>>  +	regmap {
> 
> regmap is a Linuxism.

Should I just call it "mfd" then? (or better, "mfd@10002000")


>>  +		compatible = "simple-mfd", "syscon";
> 
> Need a specific compatible string here. Neither of these are valid by
> themselves.

So a compatible string not used by any driver? Something like 
"ingenic,tcu"?
Would I need to document it too? (it's just a simple-mfd after all)


>>  +		reg = <0x10002000 0x1000>;
>>  +
>>  +		tcu: interrupt-controller {
> 
> The TCU is only an interrupt controller?

The TCU is a multi-function silicon. It has 8 channels, each one with 
its own
interrupt line, demultiplexed from 2-3 parent IRQs (depending on the 
SoC).
The TCU IRQ driver handles this.

Each channel has a clock, that can be reparented, reclocked, and gated.
That's the job for the TCU clocks driver.

Being hardware timers, they can be used for accounting and timekeeping 
within
Linux, that's the job for the clocksource driver.

The TCU block also feeds the clocks to the watchdog and the OST (a 
separate
timer block, not handled here).

Each channel can be configured as PWM. A future patchset will convert 
the PWM
driver for Ingenic SoCs to use the TCU clocks and regmap provided by 
these
new drivers.

If your remark was only reffering to the name of the node handle, I can 
rename
it to "tcu_intc", there's no problem.

>>  +			compatible = "ingenic,jz4740-tcu-intc";
> 
> Is there a register range associated with this block? If so, add a reg
> property even if regmap doesn't need it.

Sure.

Thanks for the review!

-Paul