Hi Ahmad, Sascha, have you reached a conclusion which way you want to take here? Either way I'd be happy to help, but as the implementation is rather simple, I guess you would be faster doing it yourself than explaining it to me and doing a review cycle afterwards.
Christian On Tue, Sep 16, 2025 at 11:32 AM Ahmad Fatoum <[email protected]> wrote: > > Hi, > > On 9/9/25 11:34 AM, Sascha Hauer wrote: > > On Wed, Sep 03, 2025 at 04:46:05PM +0200, Christian Thießen via B4 Relay > > wrote: > >> From: Christian Thießen <[email protected]> > >> > >> The fec_imx driver stores an enum fec_type value in the match data > >> pointer. When retrieving this value in fec_probe(), it handles > >> FEC_TYPE_IMX27 (which is 0) like an error result from > >> device_get_match_data and returns -ENODEV. > >> > >> To be able to differentiate NULL from errors, introduce a new function > >> device_get_match_data_default which returns its defaultval argument > >> instead of NULL in case of errors. > >> Add an FEC_TYPE_COUNT value to the fec_type enum. > >> In fec_probe, pass FEC_TYPE_COUNT as defaultval and return -ENODEV for > >> all type values greater or equal to that. > >> > >> Fixes: 20d87123a6 ("treewide: replace dev_get_drvdata with > >> device_get_match_data") > >> Signed-off-by: Christian Thießen <[email protected]> > >> --- > >> Hi again, > >> > >> this patch addresses the issue discussed in > >> https://github.com/barebox/barebox/issues/40 ("fec_imx (and possibly > >> others): Probe fails with ENODEV"). > >> > >> Cheers, > >> Christian > >> --- > >> drivers/base/driver.c | 13 +++++++++++++ > >> drivers/net/fec_imx.c | 12 +++++------- > >> drivers/net/fec_imx.h | 1 + > >> include/driver.h | 10 ++++++++++ > >> 4 files changed, 29 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c > >> index > >> 61118385f1df0f8d36208d236be66b077fa5e54d..614c3c26b04c6f252e3970e014a43bae6a1d9c2b > >> 100644 > >> --- a/drivers/base/driver.c > >> +++ b/drivers/base/driver.c > >> @@ -690,6 +690,19 @@ const void *device_get_match_data(struct device *dev) > >> } > >> EXPORT_SYMBOL(device_get_match_data); > >> > >> +const void *device_get_match_data_default(struct device *dev, > >> + const void *defaultval) > >> +{ > >> + if (dev->of_id_entry) > >> + return dev->of_id_entry->data; > >> + > >> + if (dev->id_entry) > >> + return (const void *)dev->id_entry->driver_data; > >> + > >> + return defaultval; > >> +} > >> +EXPORT_SYMBOL(device_get_match_data_default); > > > > This more goes to Ahmad, because he suggested this solution: > > > > I don't like this very much. With this solution the driver writer must > > still know that he has to call device_get_match_data_default() instead > > of plain device_get_match_data(). > > For drivers that are already written, it's easier to designate an error > code that shouldn't happen than shift around all existing values and > hope you don't miss anything. > > > In other words, the driver writer must be aware of this problem, so he > > could equally well avoid 0 as valid value and declare > > > > enum fec_type { > > FEC_TYPE_IMX27 = 1, > > ... > > }; > > Linux already has device_get_match_data() with the same semantics we > have in barebox now, so if we will not run into additional issues when > code is ported from Linux. > > > in this case. I have looked up all cases that pass an integer into data > > in the tree and these are not many, we could change them easily. > > > > Another possibility would be to return an error pointer instead of NULL > > from device_get_match_data(), but that would require some refactoring > > throughout the tree. > > That would change the semantics of the function different to what's done > in Linux and we don't want that. > > > Unless Ahmad objects I would prefer converting the in tree users not to > > use zero. > > I still think the approach here is easier to reason about for converting > drivers, but if you have invested the time to shift around enumerations > in all the drivers, we can go with that too. > > > Ahmad is on holiday this week, so it'll likely take some time, but > > anyway, we'll solve this issue before the next release. > > Yes, priority is getting this fixed in the next release. > > Thanks, > Ahmad > > > > > Sascha > > > > > >> + > >> static void device_set_deferred_probe_reason(struct device *dev, > >> const struct va_format *vaf) > >> { > >> diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c > >> index > >> 8474e6d5be9c6225af5708d21b0249b2618d36f6..fac190e6ea51950ed6085264e12617969847acbf > >> 100644 > >> --- a/drivers/net/fec_imx.c > >> +++ b/drivers/net/fec_imx.c > >> @@ -763,20 +763,18 @@ static int fec_probe(struct device *dev) > >> struct fec_priv *fec; > >> void *base; > >> int ret; > >> - enum fec_type type; > >> - void const *type_v; > >> + uintptr_t type_v; > >> int phy_reset; > >> u32 msec = 1, phy_post_delay = 0; > >> u32 reg; > >> > >> - type_v = device_get_match_data(dev); > >> - if (!type_v) > >> + type_v = (uintptr_t)device_get_match_data_default(dev, > >> + (const void *)FEC_TYPE_COUNT); > >> + if (type_v >= FEC_TYPE_COUNT) > >> return -ENODEV; > >> > >> - type = (uintptr_t)(type_v); > >> - > >> fec = xzalloc(sizeof(*fec)); > >> - fec->type = type; > >> + fec->type = type_v; > >> fec->dev = dev; > >> edev = &fec->edev; > >> dev->priv = fec; > >> diff --git a/drivers/net/fec_imx.h b/drivers/net/fec_imx.h > >> index > >> 1aaff87fddfb44cf558436ab4a846757c6cd51aa..9549dab0f8e88288bd9fe3a02ff8bd5573147842 > >> 100644 > >> --- a/drivers/net/fec_imx.h > >> +++ b/drivers/net/fec_imx.h > >> @@ -117,6 +117,7 @@ enum fec_type { > >> FEC_TYPE_IMX27, > >> FEC_TYPE_IMX28, > >> FEC_TYPE_IMX6, > >> + FEC_TYPE_COUNT, > >> }; > >> > >> enum fec_clock { > >> diff --git a/include/driver.h b/include/driver.h > >> index > >> c130a3cd63fd5e6dc365a33c765a22dc4e2ca90a..223799294fc0e32ceb13fc5bda2e3b8a78d71647 > >> 100644 > >> --- a/include/driver.h > >> +++ b/include/driver.h > >> @@ -650,6 +650,16 @@ int cdevfs_del_partition(struct cdev *cdev); > >> */ > >> const void *device_get_match_data(struct device *dev); > >> > >> +/** > >> + * device_get_match_data_default - get driver match data associated with > >> device > >> + * @dev: device instance > >> + * @defaultval: Value to return if no match data is associated with dev > >> + * > >> + * Returns match data on success and defaultval otherwise > >> + */ > >> +const void *device_get_match_data_default(struct device *dev, > >> + const void *defaultval); > >> + > >> int device_match_of_modalias(struct device *dev, const struct driver > >> *drv); > >> > >> struct device *device_find_child(struct device *parent, void *data, > >> > >> --- > >> base-commit: 7689314d0a78605e7d4a0c3ce4953d9d768aa343 > >> change-id: 20250903-fec_imx-fix-enodev-f7a9087054c8 > >> > >> Best regards, > >> -- > >> Christian Thießen <[email protected]> > >> > >> > >> > >> > > > > -- > 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 | > The information in this e-mail is confidential. The contents may not be disclosed or used by anyone other than the addressee. Access to this e-mail by anyone else is unauthorised. If you are not the intended recipient, please notify Airbus immediately and delete this e-mail. Airbus cannot accept any responsibility for the accuracy or completeness of this e-mail as it has been sent over public networks. If you have any concerns over the content of this message or its Accuracy or Integrity, please contact Airbus immediately. All outgoing e-mails from Airbus are checked using regularly updated virus scanning software but you should take whatever measures you deem to be appropriate to ensure that this message and any attachments are virus free.
