Re: [PATCH 2/2] ARM: dts: mvebu: add device tree for IIJ SA-W2 appliance

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello INAGAKI Hiroshi,

do you plan to send a v2 soon based on the reviews you got ?

Or if you already sent it, I missed it, in this case could you resend it
with me in CC ?

Thanks,

Gregory

> Hi Andrew,
>
> thank you for your reviews and detailed descriptions.
>
> On 2023/02/23 23:43, Andrew Lunn wrote:
>>> +		pcie {
>>> +			status = "okay";
>>> +
>>> +			pcie@1,0 {
>>> +				status = "okay";
>>> +
>>> +				/* Atheros AR9287 */
>>> +				wifi@0,0 {
>>> +					compatible = "pci168c,002e";
>>> +					reg = <0000 0 0 0 0>;
>>> +				};
>>> +			};
>>> +
>>> +			pcie@3,0 {
>>> +				status = "okay";
>>> +
>>> +				/* Qualcomm Atheros QCA9880 */
>>> +				wifi@0,0 {
>>> +					compatible = "qcom,ath10k";
>>> +					reg = <0000 0 0 0 0>;
>>> +				};
>>> +			};
>>> +		};
>>> +	};
>> These are not wrong, but they are also not needed. PCI devices should
>> be discovered by enumeration, and you don't have any additional
>> properties here, or phandles pointing to these nodes.
>>
>> I assume these are COTS wifi modules? By listing them here you are
>> restricting some flexibility. The OEM could for example swap the
>> modules around, and Linux would not care, but the DT would then be
>> wrong. Or you could have a device with a different module because it
>> is cheaper, and again, Linux would not care, but the DT would be
>> wrong.
>
> Got it. SA-W2 is not designed to allow users to swap cards under 
> normal use, but certainly things like you said can happen...
> I'll remove "wifi" nodes.
>
>  > I assume these are COTS wifi modules?
>
> Yes, those are the modules manufactured by Silex Technology, Inc. [1][2].
>
> [1]: https://www.silex.jp/products/wireless-module/sxpcegn.html
> [2]: https://www.silex.jp/products/wireless-module/sxpceac.html
>
>>
>>> +&usb0 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&pmx_usb_pins>;
>>> +	status = "okay";
>>> +	#address-cells = <1>;
>>> +	#size-cells = <0>;
>>> +
>>> +	/* SMSC USB2514B */
>>> +	hub@1 {
>>> +		compatible = "usb424,2514";
>>> +		reg = <1>;
>>> +		#address-cells = <1>;
>>> +		#size-cells = <0>;
>>> +
>>> +		hub_port1: port@1 {
>>> +			reg = <1>;
>>> +			#trigger-source-cells = <0>;
>>> +		};
>>> +
>>> +		hub_port2: port@2 {
>>> +			reg = <2>;
>>> +			#trigger-source-cells = <0>;
>>> +		};
>>> +	};
>>> +};
>> Same comment as PCI. However, it is likely that the USB hub is
>> actually on the board, not a module, so it is a lot less likely to
>> change.
>
> Yes, that USB hub is on the PCB and wired to the SoC directly. But 
> I'll keep it in mind...
>
>>
>> As i said, they are not wrong, so you don't need to remove them.
>>
>> 	Andrew
>>
>
> Regards,
> Hiroshi

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com