Re: [PATCH 2/3] MIPS: DTS: img: marduk: Add Cascoda CA8210 6LoWPAN

[Sergei Shtylyov <sergei.shtylyov@gmail.com>]

On 8/16/20 1:59 PM, Hauke Mehrtens wrote:
 
> Thank you for the review Sergei.

   You're welcome. :-)

[...]
>>> Add Cascoda CA8210 6LoWPAN controller to device tree.
>>>
>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>>> ---
>>>   arch/mips/boot/dts/img/pistachio_marduk.dts | 22 +++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts
>>> b/arch/mips/boot/dts/img/pistachio_marduk.dts
>>> index ea11a21b133b..633a41954cc0 100644
>>> --- a/arch/mips/boot/dts/img/pistachio_marduk.dts
>>> +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts
>>> @@ -75,6 +75,28 @@
>>>       VDD-supply = <&internal_dac_supply>;
>>>   };
>>>   +&spfi0 {
>>> +    status = "okay";
>>> +    pinctrl-0 = <&spim0_pins>, <&spim0_cs0_alt_pin>,
>>> <&spim0_cs2_alt_pin>, <&spim0_cs3_alt_pin>, <&spim0_cs4_alt_pin>;
>>> +    pinctrl-names = "default";
>>> +
>>> +    cs-gpios = <&gpio1 14 GPIO_ACTIVE_HIGH>, <&gpio0 2
>>> GPIO_ACTIVE_HIGH>,
>>> +            <&gpio1 12 GPIO_ACTIVE_HIGH>, <&gpio1 13 GPIO_ACTIVE_HIGH>;
>>> +
>>> +    ca8210: ca8210@0 {
>>
>>    The device nodes are supposed to have the generic names...
> 
> I am not referencing this, so I can also remove it:
> 	ca8210@0 {

   I said name, not label. :-)

> Otherwise, would this be ok:
> 	ieee802154: ca8210@0 {
> 
>>> +        status = "okay";
> 
> This status = "okay"; can be removed.

   Yes, and probably even shpould be removed...

>>> +        compatible = "cascoda,ca8210";
>>> +        reg = <0>;
>>> +        spi-max-frequency = <4000000>;
>>
>>    Only 4MHz?
> 
> The vendor device tree also use 4MHz:
> https://github.com/CreatorDev/openwrt/blob/ci40/target/linux/pistachio/dts/pistachio/pistachio_marduk.dts#L34
> The device tree binding says "Maximum clock speed, should be *less than*
> 4000000", it could even make sense to switch this to 3MHz.
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt

   It just sounds too low SPI frequency, those a usually one order higher...

> I do not have the datasheet for the cascoda,ca8210.
> 
>>> +        spi-cpol;
>>> +        reset-gpio = <&gpio0 12 GPIO_ACTIVE_HIGH>;
>>> +        irq-gpio = <&gpio2 12 GPIO_ACTIVE_HIGH>;
>>> +        extclock-enable;
>>> +        extclock-freq = <16000000>;
>>> +        extclock-gpio = <2>;
>>
>>    Hm, strange spec for a GPIO prop, shouldn't they all be alike?
> 
> I am also getting this compile warning:
> arch/mips/boot/dts/img/pistachio_marduk.dts:135.3-23: Warning
> (gpios_property): /spi@18100f00/ca8210@0:extclock-gpio: cell 0 is not a
> phandle reference
> arch/mips/boot/dts/img/pistachio_marduk.dts:126.19-136.4: Warning
> (gpios_property): /spi@18100f00/ca8210@0: Missing property '#gpio-cells'
> in node /clk@18144000 or bad phandle (referred from extclock-gpio[0])

     Expected. :-)

> I do not know how to fix this.

> The binding is defined here:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ieee802154/ca8210.txt

   Have to fix the bindings somehow, "extclock-gpio" was a bad name...

> Hauke

MBR, Sergei