Re: [PATCH 06/20] arm64: dts: qcom: kaanapali: Add USB support for Kaanapali SoC

[Krzysztof Kozlowski <krzk@kernel.org>]

On Thu, 25 Sept 2025 at 22:57, Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Sep 25, 2025 at 10:50:10AM +0900, Krzysztof Kozłowski wrote:
> > On Thu, 25 Sept 2025 at 09:17, Jingyi Wang <jingyi.wang@oss.qualcomm.com> wrote:
> > >
> > > From: Ronak Raheja <ronak.raheja@oss.qualcomm.com>
> > >
> > > Add the base USB devicetree definitions for Kaanapali platform. The overall
> > > chipset contains a single DWC3 USB3 controller (rev. 200a), SS QMP PHY
> > > (rev. v8) and M31 eUSB2 PHY.
> > >
> > > Signed-off-by: Ronak Raheja <ronak.raheja@oss.qualcomm.com>
> > > Signed-off-by: Jingyi Wang <jingyi.wang@oss.qualcomm.com>
> > > ---
> > >  arch/arm64/boot/dts/qcom/kaanapali.dtsi | 155 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 155 insertions(+)
> > >
> >
> >
> > Second try, without HTML:
> >
> > I really don't understand why you created such huge patchset.
>
> Because I looked at the logical changes that went into the big squash
> that was initially planned, and requested that some of those was kept
> intact - because they where independent logical changes.

Ack

>
> > Year
> > ago, two years ago, we were discussing it already and explained that's
> > just inflating the patchset without reason.
> >
>
> We used to add things node by node and that was indeed not
> comprehensible. Overall this adds features in large logical chunks, but
> there are a few of the patches that could have been squashed.

This patch adds three device nodes.

Other patches add one node. Some other remove the lines which were
added before!

The last argument is the most important. I don't say anything new...
If you add code, which you immediately remove, you do it wrong.

>
> > New Soc is one logical change. Maybe two. Not 18!
>
> I can see your argument for one patch to introduce the soc. But two
> doesn't make sense, because that incremental patch is going to be the
> kitchen sink.

Ack. Plus argument of actual patch line count
So maybe two SoC patches? Maybe three?

>
> >
> > Not one patch per node or feature.
> >
>
> Definitely agree that we don't want one patch for every tiny block!
>
> > This hides big picture, makes difficult to review everything,
> > difficult to test.
>
> The big picture is already obscured due to the size of the content
> added.

Not happy about this either, but understand the principle and business
choices, thus I don't comment on that.

>
> Comparing to previous targets, I see the baseline content in 2-3
> patches, and the remainder of the series being things that usually has
> been scattered in many more small changes in the following weeks or
> months.
>
> There's plenty of features in this series that are yet to be concluded
> for SM8750.
>
> > Your patch count for LWN stats doesn't matter to
> > us.
>
> I agree with this. That's why the QRD is 1 patch, and MTP is 4 (this I
> think should be squashed to 2) - compared to 13 patches for across the
> pair for SM8750 with less scope.

Ack, plus reorganize patches which are removing code added in previous patch.

>
> >
> > NAK and I'm really disappointed I have to repeat the same review .
>
> I'm not sure what you're disappointed in, this initial series is larger
> than any we've seen before. I really like the work Jingyi has done here,
> aggregating the otherwise scattered patches into one series.

Disappointing is only that I had this exactly talk with Qualcomm two
years ago and year ago and 2 months ago (QCS), and maybe more...

I'm sure that we had a internal meeting about Glymur and I said the
same. I emphasized this issue. Look at Glymur DTS - did my internal
emphasizing succeed?

No. I can speak on mailing lists every year, I can have internal
meeting(s) and it doesn't work.

Plus the timing of merge window with these two huge patch bombs is
just cherry on top.