Message-ID: <CAL_Jsq+wGopPRcdp6t0h2cu03vrxxP3=msNmpju4nqq9XENmng@mail.gmail.com>
Date: Tue, 15 Apr 2025 07:46:19 -0500
From: Rob Herring <robh@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Danilo Krummrich <dakr@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

On Tue, Apr 15, 2025 at 6:11 AM Remo Senekowitsch <remo@...nzli.dev> wrote:
>
> On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> > On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> >> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> >> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> >> >> The device property API is a firmware agnostic API for reading
> >> >> properties from firmware (DT/ACPI) devices nodes and swnodes.
> >> >>
> >> >> While the C API takes a pointer to a caller allocated variable/buffer,
> >> >> the rust API is designed to return a value and can be used in struct
> >> >> initialization. Rust generics are also utilized to support different
> >> >> types of properties where appropriate.
> >> >>
> >> >> The PropertyGuard is a way to force users to specify whether a property
> >> >> is supposed to be required or not. This allows us to move error
> >> >> logging of missing required properties into core, preventing a lot of
> >> >> boilerplate in drivers.
> >> >
> >> > The patch adds a lot of thing, i.e.
> >> >   * implement PropertyInt
> >> >   * implement PropertyGuard
> >> >   * extend FwNode by a lot of functions
> >> >   * extend Device by some property functions
> >> >
> >> > I see that from v1 a lot of things have been squashed, likely because there are
> >> > a few circular dependencies. Is there really no reasonable way to break this
> >> > down a bit?
> >>
> >> I was explicitly asked to do this in the previous thread[1].
> >
> > I'm well aware that you were asked to do so and that one reason was that
> > subsequent patches started deleting code that was added in previous ones
> > (hence my suspicion of circular dependencies and that splitting up things might
> > not be super trivial).
> >
> >> I'm happy
> >> to invest time into organizing files and commits exactly the way people
> >> want, but squashing and splitting the same commits back and forth
> >> between subsequent patch series is a waste of my time.
> >
> > I don't think you were asked to go back and forth, but whether you see a
> > reasonable way to break things down a bit, where "reasonable" means without
> > deleting code that was just added.
>
> I was asked to squash two specific commits. The first was making the
> read method generic. That was the one that deleted much code. Totally
> reasonable, and the generic stuff might be discarded anyway, so I won't
> be moving stuff back and forth.
>
> However, the second commit was the one introducing PropertyGuard. That
> was a beautifully atomic commit, no circular dependencies in sight. If
> this commit is to be split up, one of the smaller ones would without
> doubt look exactly the same as the one before. I provided a link[1]
> to the exact email telling me to squash that exact patch to avoid any
> confusion.

Adding PropertyGuard changes the API. I don't think it makes sense to
review the API without it and then again with it.

It might be reasonable to split adding the fwnode bindings and then
the Device version. But those are in separate files anyway, so it's
not really making review easier.

> >> Do reviewers not typically read the review comments of others as well?
> >
> > I think mostly they do, but maintainers and reviewers are rather busy people.
> > So, I don't think you can expect everyone to follow every thread, especially
> > when they get lengthy.

Please understand maintainers are reviewing 10s to 100s of patches a
week. I don't necessarily remember my own comments from last week.

> >> What can I do to avoid this situation and make progress instead of
> >> running in circles?
> >
> > I suggest to investigate whether it can be split it up in a reasonable way and
> > subsequently answer the question.
>
> The point is that I agree with you that the PropertyGuard patch can be
> split out. And possibly more: I think a reasonable person could make a
> separate commit for every `property_read_<type>` method. And I'm happy
> to do that if it's the way to go.  But if I do that, send a v3 and then
> someone else asks me to squash it again (because they didn't read this
> exchange between you and me...) then I would've wasted my time.

A commit per method really seems excessive to me. I prefer to look at
the API as a whole, and to do that with separate patches only makes
that harder.

A reasonable split here is possibly splitting the fwnode and the
Device versions of the API. In any case, I think we've discussed this
enough and I don't care to discuss it more, so whatever reasonable
split you come up with is fine with me.

> > With your contribution you attempt to add a rather large portion of pretty core
> > code. This isn't an easy task and quite some discussion is totally expected;
> > please don't get frustrated, the series goes pretty well. :)
>
> I'm trying, but I can't say I have a good first impression of doing
> code review over a mailing list. Doing open-heart surgery with a
> pickfork and writing documents in Microsoft Word both seem like more
> productive and enjoyable activities to me.

Mailing lists are a good scapegoat, but you can have 2 reviewers with
2 different opinions anywhere. That's just the nature of a large,
distributed project with lots of reviewers.

Rob

Powered by blists - more mailing lists