| Message ID | 20220221162652.103834-4-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | add support for fwnode in i2c mux system and sfp | expand |
On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote: > In order to allow matching devices with software node with > device_get_match_data(), use fwnode_get_match_data() for > .device_get_match_data operation. ... > + .device_get_match_data = fwnode_get_match_data, Huh? It should be other way around, no? I mean that each of the resource providers may (or may not) provide a method for the specific fwnode abstraction.
Le Mon, 21 Feb 2022 19:48:00 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote: > > In order to allow matching devices with software node with > > device_get_match_data(), use fwnode_get_match_data() for > > .device_get_match_data operation. > > ... > > > + .device_get_match_data = fwnode_get_match_data, > > Huh? It should be other way around, no? > I mean that each of the resource providers may (or may not) provide a > method for the specific fwnode abstraction. > Indeed, it should be the other way. But since this function is generic and uses only fwnode API I guessed it would be more convenient to define it in the fwnode generic part and use it for specific implementation. I could have modified device_get_match_data to call it if there was no .device_get_match_data operation like this: const void *device_get_match_data(struct device *dev) { if (!fwnode_has_op(fwnode, device_get_match_data) return fwnode_get_match_data(dev); return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev); } But I thought it was more convenient to do it by setting the .device_get_match_data field of software_node operations.
Hi Clément, On Tue, Feb 22, 2022 at 09:39:21AM +0100, Clément Léger wrote: > Le Mon, 21 Feb 2022 19:48:00 +0200, > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > On Mon, Feb 21, 2022 at 05:26:45PM +0100, Clément Léger wrote: > > > In order to allow matching devices with software node with > > > device_get_match_data(), use fwnode_get_match_data() for > > > .device_get_match_data operation. > > > > ... > > > > > + .device_get_match_data = fwnode_get_match_data, > > > > Huh? It should be other way around, no? > > I mean that each of the resource providers may (or may not) provide a > > method for the specific fwnode abstraction. > > > > Indeed, it should be the other way. But since this function is generic > and uses only fwnode API I guessed it would be more convenient to > define it in the fwnode generic part and use it for specific > implementation. I could have modified device_get_match_data to call it > if there was no .device_get_match_data operation like this: > > const void *device_get_match_data(struct device *dev) > { > if (!fwnode_has_op(fwnode, device_get_match_data) > return fwnode_get_match_data(dev); > return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev); > } > > But I thought it was more convenient to do it by setting the > .device_get_match_data field of software_node operations. Should this function be called e.g. software_node_get_match_data() instead, as it seems to be specific to software nodes?
Le Wed, 23 Feb 2022 17:05:22 +0200, Sakari Ailus <sakari.ailus@linux.intel.com> a écrit : > > const void *device_get_match_data(struct device *dev) > > { > > if (!fwnode_has_op(fwnode, device_get_match_data) > > return fwnode_get_match_data(dev); > > return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev); > > } > > > > But I thought it was more convenient to do it by setting the > > .device_get_match_data field of software_node operations. > > Should this function be called e.g. software_node_get_match_data() instead, > as it seems to be specific to software nodes? Hi Sakari, You are right, since the only user of this function currently is the software_node operations, then I should rename it and move it to swnode.c maybe.
On Wed, Feb 23, 2022 at 04:15:35PM +0100, Clément Léger wrote: > Le Wed, 23 Feb 2022 17:05:22 +0200, > Sakari Ailus <sakari.ailus@linux.intel.com> a écrit : > > > > const void *device_get_match_data(struct device *dev) > > > { > > > if (!fwnode_has_op(fwnode, device_get_match_data) > > > return fwnode_get_match_data(dev); > > > return fwnode_call_ptr_op(dev_fwnode(dev),device_get_match_data, dev); > > > } > > > > > > But I thought it was more convenient to do it by setting the > > > .device_get_match_data field of software_node operations. > > > > Should this function be called e.g. software_node_get_match_data() instead, > > as it seems to be specific to software nodes? > > Hi Sakari, > > You are right, since the only user of this function currently is the > software_node operations, then I should rename it and move it to > swnode.c maybe. It might be also fit to be used in OF, based on how it looks like. But currently the original naming makes it seem an fwnode property API function and that is misleading. I'd move this to swnode.c now with a new software node specific name, and rethink the naming matter if there would seem to be possibilities for code re-use.
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 0a482212c7e8..783ad18f49af 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -662,6 +662,7 @@ software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode, static const struct fwnode_operations software_node_ops = { .get = software_node_get, .put = software_node_put, + .device_get_match_data = fwnode_get_match_data, .property_present = software_node_property_present, .property_read_int_array = software_node_read_int_array, .property_read_string_array = software_node_read_string_array,
In order to allow matching devices with software node with device_get_match_data(), use fwnode_get_match_data() for .device_get_match_data operation. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/base/swnode.c | 1 + 1 file changed, 1 insertion(+)