Re: [RFC PATCH 27/57] drivers: Unify the match prototype for bus_find_device with class_find_device

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: rafael@kernel.org
Cc: linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
	alexander.shishkin@linux.intel.com, wsa@the-dreams.de,
	jic23@kernel.org, knaack.h@gmx.de, grygorii.strashko@ti.com,
	davem@davemloft.net, bhelgaas@google.com, sebott@linux.ibm.com,
	oberpar@linux.ibm.com, freude@linux.ibm.com, jejb@linux.ibm.com,
	martin.petersen@oracle.com, andreas.noever@gmail.com,
	michael.jamet@intel.com, balbi@kernel.org,
	david.kershner@unisys.com
Subject: Re: [RFC PATCH 27/57] drivers: Unify the match prototype for bus_find_device with class_find_device
Date: Tue, 4 Jun 2019 12:39:50 +0100	[thread overview]
Message-ID: <d51d8361-8807-5795-7da0-68c299217213@arm.com> (raw)
In-Reply-To: <CAJZ5v0ghb969phwKDJd_hJV6mBm0_B8R9Lrk8RG3Tu9ieO69OQ@mail.gmail.com>

On 04/06/2019 12:26, Rafael J. Wysocki wrote:
> On Mon, Jun 3, 2019 at 5:51 PM Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> We have iterators for devices by bus and class, with a supplied
>> "match" function to do the comparison. However, both of the helper
>> function have slightly different prototype for the "match" argument.
>>
>>   int (*) (struct device *dev, void *data)  // bus_find_device
>>    vs
>>   int (*) (struct device *dev, const void *data) // class_find_device
>>
>> Unify the prototype by promoting the match function to use that of
>> the class_find_device(). This will allow us to share the generic
>> match helpers with class_find_device() users.
> 
> The patch looks good to me, but the changelog might be a bit better.
> 
> It seems to be all about the bus_find_device() and class_find_device()
> prototype consolidation, so that the same pair of data and match()
> arguments can be passed to both of them, which then will allow some
> optimizations to be made, so what about the following:
> 
> "There is an arbitrary difference between the prototypes of
> bus_find_device() and class_find_device() preventing their callers
> from passing the same pair of data and match() arguments to both of
> them, which is the const qualifier used in the prototype of
> class_find_device().  If that qualifier is also used in the
> bus_find_device() prototype, it will be possible to pass the same
> match() callback function to both bus_find_device() and
> class_find_device(), which will allow some optimizations to be made in
> order to avoid code duplication going forward.
> 
> For this reason, change the prototype of bus_find_device() to match
> the prototype of class_find_device() and adjust its callers to use the
> const qualifier in accordance with the new prototype of it.".

Agreed, I will reword the description.

> 
> Also, it looks like there is no need to make all of the following
> changes in the series along with this one in one go and making them
> separately would be *much* better from the patch review perspective.

Sure. I started with the helpers in the hope that, I would need fewer
changes to individual subsystems, once I convert them to use the
new helpers.

i.e, driver A -> use new helper and the change the new helper.

rather than

change all callers of *_find_device() and then all to switch to new helper.

Anyways, looks like the latter is better in terms of splitting the series.
I will rework the series.

Thanks a lot for your input

Cheers
Suzuki