Received-SPF: pass (google.com: domain of linux-kernel+bounces-77594-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240220161002.2983334-1-andriy.shevchenko@linux.intel.com>
 <CAGETcx8hzYzjKWPz4ACgS=XrVXqg46QHB4hTpP5bDTY9=WFK+Q@mail.gmail.com>
 <ZdXxNFB8bMMFPoki@smile.fi.intel.com> <ZdXxYjYsBQybbilQ@smile.fi.intel.com>
In-Reply-To: <ZdXxYjYsBQybbilQ@smile.fi.intel.com>
From: Saravana Kannan <saravanak@google.com>
Date: Thu, 22 Feb 2024 16:54:39 -0800
Message-ID: <CAGETcx9akN315asPP=e8xieHqs7gKXvHP-BwRxD=vCBuhh8_Jw@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] driver core: Move fw_devlink stuff to where it belongs
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, 
	linux-acpi@vger.kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>, 
	Daniel Scally <djrscally@gmail.com>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
	Sakari Ailus <sakari.ailus@linux.intel.com>, Len Brown <lenb@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 21, 2024 at 4:49=E2=80=AFAM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Feb 21, 2024 at 02:48:52PM +0200, Andy Shevchenko wrote:
> > On Tue, Feb 20, 2024 at 06:08:56PM -0800, Saravana Kannan wrote:
> > > On Tue, Feb 20, 2024 at 8:10=E2=80=AFAM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > A few APIs that belong specifically to the fw_devlink APIs
> > > > - are exposed to others without need
> > > > - prevents device property code to be cleaned up in the future
> > > >
> > > > Resolve this mess by moving fw_devlink code to where it belongs
> > > > and hide from others.
>
> ...
>
> > > The rest of the functions here are related to parents and children of
> > > a fwnode. So, why is this function considered to be in the wrong
> > > place?
> >
> > When devlink was added it made a few fields in struct fwnode_handle.
> > These fields have no common grounds with device properties. In particul=
ar
> > struct device pointer is solely for devlinks and shouldn't be used with
> > them. Hence this patch. TL;DR: they semantically do _not_ belong to
> > the device property APIs.

But fwnode_is_ancestor_of() uses none of those new fields and seems
like a very reasonable API to provide. I understand if you want to
make the "device link only" argument for fwnode_get_next_parent_dev().

> On top of that for the 4+ years no new users appeared, so exporting them =
was
> a clear mistake. Hence Fixes tags.

We have plenty of APIs in .h files (not the same as export to modules)
-- that have only 1 user. And even some with no users. You don't move
a string function out of lib/string.c just because there's only one
user of that function. We put functions in C files that make sense.

I think Fixes is a bit of an overkill. It's not a bug. Fixes get
propagated to LTS. This is certainly not LTS worthy. I'm not going to
NACK or push back on this patch for these reasons, but just letting
you know that you come off as unreasonably grumpy when you do these
things even for fwnode_is_ancestor_of() :)

-Saravana