Message-ID: <d994ec8a-ff6a-4228-bfc9-2fcf9b35a460@iscas.ac.cn>
Date: Sun, 1 Feb 2026 15:54:45 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Konstantin Ryabitsev <konstantin@...uxfoundation.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
 Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
 Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>
Cc: devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
 spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Han Gao <rabenda.cn@...il.com>, Han Gao <gaohan@...as.ac.cn>
Subject: Re: [PATCH 0/7] riscv: dts: spacemit: Add PMIC regulators usb pcie

On 2/1/26 13:18, Konstantin Ryabitsev wrote:
> On Sun, 01 Feb 2026 05:38:07 +0800, Han Gao <gaohan@...as.ac.cn> wrote:
>> This patch series enables the PMIC, voltage regulators, usb and pcie
>> for the OrangePi rv2 board.
> Hi:
>
> Thank you for sending this in. I am working on "b4 review" functionality 
> and I am using arbitrary series to perform agent-assisted reviews to 
> find bugs in the process.
>
> This review was done by Claude Opus 4.5. I do not know enough about the 
> subsystem to make any judgment about the quality of these reviews, but 
> they look valid enough.

I'm not sure if you see this, but your bot gave two contradictory
recommendations.

On patch 3 it says:

> This vin-supply is replaced by per-rail supplies (vin1-supply through
> vin6-supply, aldoin-supply, dldoin1-supply, dldoin2-supply) in patch
> 5/7.  Consider using the final supply properties here directly to avoid
> an intermediate state that is immediately superseded.
Whereas on patch 5 it says

> These per-rail supply properties (vin1-supply through dldoin2-supply)
> are not declared in the spacemit,p1.yaml binding, which only has
> vin-supply.  With unevaluatedProperties: false on the PMIC node, these
> will cause dtbs_check failures.  The binding needs to be updated to
> match, or these should remain as the single vin-supply.
"Use vin1-supply through dldoin2-supply, not vin-supply" and "use
vin-supply only, not vin1-supply etc" can't both be correct, so
obviously something weird is going on.

So what on earth is going on? Granted, Han could have described it
better in his cover letter, but there's a proposed update to the
spacemit,p1 bindings [1] that would have deprecated vin-supply and
allowed vin1-supply etc. So there's a dependency in this series of
something like:

    patch 1,2 -> patch 3 -> bindings update [1] -> patch 5

If/when the bindings update is accepted, patch 5 can be accepted into
the DTS tree. Until then, patch 5 should be withheld.

This isn't obvious from the patch itself, but still, the two
recommendations are contradictory on their own, and make dtbs_check
(mentioned in the patch 5 comment!) would have caught the problem with
the patch 3 comment. 

> Hopefully, these were useful. If not, I am sorry for wasting your time.

I acknowledge that this is a sample size of 1, but this is not making me
feel inclined to trust the future of whatever "agent-assisted review" is
going to be happening. What good is an "agent" if it knows about make
dtbs_check but doesn't even run it to see what would happen to its
suggested fixes?

I don't know what you have in mind for "b4 review", but I think at the
very least, "b4 review" should not encourage "I do not know much about
the subsystem but the LLM comments look valid" replies.

Regards,
Vivian "dramforever" Wang

[1]: https://lore.kernel.org/spacemit/20260124-spacemit-p1-v2-0-2c86b06694ba@riscstar.com/

Powered by blists - more mailing lists