Messages in this thread | | | Subject | Re: [RFC PATCH 27/57] drivers: Unify the match prototype for bus_find_device with class_find_device | From | Suzuki K Poulose <> | Date | Tue, 4 Jun 2019 12:39:50 +0100 |
| |
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
| |