Re: [PATCH v4 1/2] dt-bindings: arm: aspeed: add Meta Ventura board

Re: [PATCH v4 1/2] dt-bindings: arm: aspeed: add Meta Ventura board

[Krzysztof Kozlowski]

On 26/11/2024 12:18, Jason Hsu wrote:
> From: Jason-Hsu <jasonhell19@gmail.com>
> 
> Document the new compatibles used on Meta Ventura.
> Add subject prefix for the patch.
> 
> Signed-off-by: Jason-Hsu <jasonhell19@gmail.com>
<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v4 2/2] ARM: dts: aspeed: ventura: add Meta Ventura BMC

[Krzysztof Kozlowski]

On 26/11/2024 12:18, Jason Hsu wrote:
> Add Linux device tree related to Meta(Facebook) Ventura specific devices connected to BMC(AST2600) SoC.
> Add subject prefix for the patch.


Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597


Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.


> ---
>  arch/arm/boot/dts/aspeed/Makefile             |   1 +
>  .../aspeed/aspeed-bmc-facebook-ventura.dts    | 883 ++++++++++++++++++
>  2 files changed, 884 insertions(+)
>  create mode 100644 arch/arm/bo




> +		led-2 {
> +			label = "bmc_ready_noled";
> +			default-state = "on";
> +			gpios = <&gpio0 ASPEED_GPIO(B, 3) (GPIO_ACTIVE_HIGH|GPIO_TRANSITORY)>;
> +		};
> +
> +		led-3 {
> +			label = "power_blue";
> +			default-state = "off";
> +			gpios = <&gpio0 ASPEED_GPIO(P, 4) GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> +
> +	memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0x80000000>;
> +	};
> +
> +    p1v8_bmc_aux: regulator-p1v8-bmc-aux {


Fix indentation everywhere. It's broken in multiple places.

Best regards,
Krzysztof

Re: [PATCH v4 2/2] ARM: dts: aspeed: ventura: add Meta Ventura BMC

[Andrew Lunn]

> +    // 88E6393X CONFIG FRU
> +	eeprom@50 {
> +		compatible = "atmel,24c64";
> +		reg = <0x50>;
> +	};

I just noticed this and it seems odd. MV88E6393X is a Marvell Ethernet
switch. It can have an eeprom, and if so, the mv88e6xxx driver will
export it via ethtool. It is not normally visible to Linux as an I2C
device, because it hangs off the switches I2C Bus.

Do you have some multi master going on?

I also don't see the switch itself here. I would expect it to be
listed in the MDIO node.

	Andrew