[PATCH 2/4] Add dma sector to mpc8641hpcn board dts

[Segher Boessenkool]

>>> +		dma at 21000{
>>> +			compatible = "fsl,mpc8xxx-dma";
>>
>> Please use a real name, not this "xxx" stuff.
>
> Does the "xxx" seems to be an evil name? :-)

You are stating this particular device is compatible to
the "mpc8xxx-dma" device, which doesn't exist.  If it is
supposed to mean it is compatible to the DMA controller
on any 8000-series device, that is incorrect; in the future,
there might be many such devices with an incompatible DMA
controller.

Just put in some real device model name.

>>> +			reg = <21000 100>;
>>> +			ranges = <0 21000 1000>;
>>
>> These overlap, that can't be right; it is just begging for
>> trouble.
>
> There is no overlap. The 'reg' is used for the register of DMA
> controller. And the ranges is used for belows channel registers.

And 21000..210ff is a subset of 21000..21fff.  So there _is_
overlap.

> Such as ch0 at 100, reg=<100 80>, in fact, the ch0 register address is
> 0x21100.

Sure, none of the "reg"s in any of the children overlap the
parent "reg", but the parent "reg" overlaps the "ranges".

>>> +			ch0 at 100{
>>> +				reg = <100 80>;
>>> +				extended;
>>
>> I think you want a little more detailed property name
>> than "extended" here?
>
> I've explained it in [PATCH 1/4] Add DMA sector to
> Documentation/powerpc/booting-without-of.txt file.
> As below:
> +    - extended : Set the DMA channel to work at extended chain mode.
> +                 If not set, the DMA channel will work at basic
> +                 chain mode.

Sure it's documented, but it would be good if the property
name itself would say a bit more than just "extended".  Property
names can be 31 characters, there's no need to make short names
(terse is good, content-free isn't).


Segher