Hello Sascha, On 9/29/20 9:32 AM, Sascha Hauer wrote: > On Mon, Sep 28, 2020 at 05:42:41PM +0200, Ahmad Fatoum wrote: >> We use dev_get_drvdata to get the driver match data associated with a >> device. This has two shortcomings: >> >> - Linux has dev_get_drvdata too, which returns a private pointer for >> driver specific info to associate with a device. We use dev->priv >> (or more often container_of) for that in barebox instead >> >> - It nearly always involves a cast to a double pointer, which is >> error-prone as size and alignment match need to be ensured >> on the programmer's part and can easily be gotten wrong: >> enum dev_type type >> dev_get_drvdata(dev, (const void **)&type); >> >> Add a new function that instead of using a double pointer argument, >> returns the pointer directly: >> >> - For normal pointer driver data, no cast is necessary >> - For integer driver data casted to a pointer for storage, >> the cast is still necessary, but >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> >> --- >> drivers/base/driver.c | 11 +++++++++++ >> include/driver.h | 18 ++++++++++++++++++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c >> index 412db6c40699..3205bbc3c33b 100644 >> --- a/drivers/base/driver.c >> +++ b/drivers/base/driver.c >> @@ -500,3 +500,14 @@ int dev_get_drvdata(struct device_d *dev, const void **data) >> >> return -ENODEV; >> } >> + >> +const void *device_get_match_data(struct device_d *dev) >> +{ >> + if (dev->of_id_entry) >> + return dev->of_id_entry->data; >> + >> + if (dev->id_entry) >> + return (void *)dev->id_entry->driver_data; >> + >> + return NULL; >> +} > > Can we please give the caller a possibility to distinguish between > errors and a valid NULL pointer? The only error possible is that we matched the device by name and not of_compatible or platform id entry. If the match data is a valid pointer: -> It doesn't matter, why we have no match data either way. If the match data is a casted integer (e.g. enum): The driver author should either: -> place the default enum value as first one, so no match data => default -> should add an initial DEVICE_TYPE_UNKNOWN = 0 in the enum and handle it appropriately I like the function signature like that, I don't really see a need to adjust it. > As you realize in your series some drivers cast the returned value into > an integer type and use 0 as a valid possibility. These need an extra > explanation why we can accept that case. I can think of different > possibilies to get that straight: > > - Put a real pointer into matchdata. This is really preferred as it > motivates people to put flags into a (struct type) matchdata which > doesn't lead to excessive if (type == foo || type == bar || type == > baz) we sometimes see in drivers. We have a real pointer there already. The problem is migrating the existing drivers. > - Return an ERR_PTR from device_get_match_data(). this is less likely > interpreted as a valid int value Doesn't cover all cases. Also for the normal use, it means you need to have to check with IS_ERR_OR_NULL everywhere to be sure you don't dereference a NULL pointer. > - add a int * ret argument to device_get_match_data() which returns > the error code. Would work, but my preference is leaving it to the drivers. The drivers need to handle errors anyway. > > Sascha > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox