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
