Re: [PATCH v3 2/2] ARM: dts: aspeed: add asrock x470d4u bmc

Hi Zev,

Many thanks for your review!

On 16.08.26 06:49, Zev Weiss wrote:
> On Wed, Aug 12, 2026 at 01:28:27PM PDT, Tan Siewert wrote:
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u- 
>> data64.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u- 
>> data64.dts
>> new file mode 100644
>> index 000000000000..6f11aeab087c
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u-data64.dts
>> @@ -0,0 +1,15 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include "aspeed-bmc-asrock-x470d4u.dts"
>> +
>> +/ {
>> +    compatible = "asrock,x470d4u-data64-bmc", "aspeed,ast2500";
>> +    model = "Asrock Rack X470D4U-series BMC (64MB store)";
> 
> Nit: for consistency with how they spell it, and (at least *most* of) 
> the other aspeed-bmc-asrock-*.dts files, I'd suggest "ASRock" instead of 
> "Asrock".  (And IMO s/store/flash/ would be a touch clearer.)
> 

Agreed on both, will fix it in v4.

> Also, looks like the other nearby DTS files pretty consistently have 
> compatible & model in the opposite order, so matching that might be nice 
> too.

This is something that was reported by `scripts/dtc/dt-style-check 
--mode strict`. Depending on how much time I have next week I'll look 
into fixing the dtc offsenses for arm/dts/aspeed (tho that'd come from 
my employer email).

> 
>> +};
>> +
>> +&fmc {
>> +    flash@0 {
>> +        /delete-node/partitions;
>> +#include "openbmc-flash-layout-64.dtsi"
>> +    };
>> +};
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u.dts b/ 
>> arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u.dts
>> new file mode 100644
>> index 000000000000..80fb2b7c1e0c
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-x470d4u.dts
> 
> <snip>
> 
>> +
>> +    fan@3 {
>> +        /* FAN4 (6-pin) */
>> +        reg = <0x03>;
>> +        aspeed,fan-tach-ch = /bits/ 8 <0x03 0x0b>;
>> +    };
>> +
>> +    fan@4 {
>> +        /* FAN5 (6-pin) */
>> +        reg = <0x04>;
>> +        aspeed,fan-tach-ch = /bits/ 8 <0x04 0x0d>;
>> +    };
>> +
>> +    fan@5 {
>> +        /* FAN6 (6-pin) */
>> +        reg = <0x05>;
>> +        aspeed,fan-tach-ch = /bits/ 8 <0x05 0x0e>;
>> +    };
>> +};
> 
> Just to confirm, skipping channel 0xc here is intentional I assume? 
> (Most other related platforms have them contiguously numbered, but not 
> all, so another slight oddity wouldn't be too shocking.)

The skip is intentional. That is what I found out during reverse 
engineering the SDR file in the vendor firmware.

Cheers,
Tan