Re: [PATCH 1/2] dt-bindings: dmaengine: add DT binding for UniPhier MIO DMAC

[Jassi Brar]

On 23 August 2018 at 10:48, Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
> Hi Jassi,
>
>
> 2018-08-21 19:44 GMT+09:00 Jassi Brar <jaswinder.singh@xxxxxxxxxx>:
>> On 21 August 2018 at 15:17, Masahiro Yamada
>> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>>> (+CC Rob, DT, LKML)
>>>
>>> I forgot to CC this to DT community...
>>>
>>>
>>> 2018-08-21 18:30 GMT+09:00 Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>:
>>>> The MIO DMAC (Media IO DMA Controller) is used in UniPhier LD4,
>>>> Pro4, and sLD8 SoCs.
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>>>> ---
>>>>
>>>>  .../devicetree/bindings/dma/uniphier-mio-dmac.txt  | 28 ++++++++++++++++++++++
>>>>  1 file changed, 28 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/dma/uniphier-mio-dmac.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/dma/uniphier-mio-dmac.txt b/Documentation/devicetree/bindings/dma/uniphier-mio-dmac.txt
>>>> new file mode 100644
>>>> index 0000000..a9e969e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/dma/uniphier-mio-dmac.txt
>>>> @@ -0,0 +1,28 @@
>>>> +UniPhier Media IO DMA controller
>>>> +
>>>> +This works as an external DMA engine for SD/eMMC controllers etc.
>>>> +found in UniPhier LD4, Pro4, sLD8 SoCs.
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "socionext,uniphier-mio-dmac".
>>>> +- reg: offset and length of the register set for the device.
>>>> +- interrupts: a list of interrupt specifiers associated with the DMA channels.
>>>> +- clocks: a single clock specifier
>>>> +- #dma-cells: should be <1>. The single cell represents the channel number.
>>>> +- dma-channels: specify the number of the DMA channels. This should match to
>>>> +  the number of tuples in the interrupts property.
>>>> +
>> Can we not infer the number of channels from interrupt tuples? After
>> all the driver assumes they are same.
>
>
> It would be possible to count the number of tuples
> in "interrupts".
>
>
>
> I know of_irq_count(), but I do not see any driver
> in drivers/dma/ that calls it.
>
>
> I guess the reason is that of_irq_count() is not exported,
> so tristate drivers like this cannot use it.
>
>
> I checked Documentation/devicetree/bindings/dma/,
> and some controllers specify _redundant_ dma-channels property.
>
> fsl-mxs-dma.txt
> renesas,rcar-dmac.txt
> renesas,usb-dmac.txt
>
:) I am not sure "because others are doing it" is a good reason to
introduce redundancy.


> I also see counter-implementation.
>
>
> bcm2835-dma.c hard-codes the number of channels in the driver.
> tegra210-adma.c associates nr_channels with compatible string.
>
>
>
> I will wait for comments from the maintainers.
>
> If desired, I will export of_irq_count()
> and use it from my driver.
>
If you don't want to leave too much footprint, you could do

  count = 0;
  while (of_irq_parse_one(dev, count, &irq) == 0) count++

of_irq_parse_one() is already exported.

A good side-effect is you wouldn't have to hardcode the count in the
driver (like bcm and tegra examples you quote).

Having said that, I wouldn't lose sleep over it. So ....

Cheers!