Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gregory CLEMENT <gregory.clement@bootlin.com>
To:     INAGAKI Hiroshi <musashino.open@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        sebastian.hesselbarth@gmail.com, arnd@arndb.de, olof@lixom.net,
        soc@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org
Subject: Re: [PATCH 2/2] ARM: dts: mvebu: add device tree for IIJ SA-W2
 appliance
In-Reply-To: <7d4a218d-8b8a-5a1d-eff8-e154bfde69be@gmail.com>
References: <20230223132502.2045-1-musashino.open@gmail.com>
 <20230223132502.2045-2-musashino.open@gmail.com>
 <Y/d7gjqQCKKXMHqj@lunn.ch>
 <7d4a218d-8b8a-5a1d-eff8-e154bfde69be@gmail.com>
Date:   Fri, 07 Apr 2023 17:34:34 +0200
Message-ID: <87jzyn3dv9.fsf@BL-laptop>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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