Message ID | 50062199.7090904@gmail.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
On 07/17/2012 09:38 PM, Rob Herring wrote: > On 07/17/2012 08:11 PM, Scott Wood wrote: >> Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 ("of: match by compatible >> property first") breaks the gianfar ethernet driver found on various >> Freescale PPC chips. > > You do know this is reverted, right? No, I didn't. I got it via Kumar's next branch, and saw that it was still in your fixes-for-grant branch, and didn't see any revert-related e-mail activity on the devicetree-discuss list about it. I now see that it was reverted directly in Linus's tree (I didn't see either the original or the revert in Linus's tree when I checked, but apparently I hadn't fetched that as recently as I thought). > Here's my fix (untested) which is a bit simpler. I'm assuming if we care > about which compatible string we are matching to, then we require name > and type are blank and we only care about compatible strings. Any particular reason for making that assumption? We should be avoiding the need for .name or .type matching in new bindings, but this seems like unnecessarily inconsistent behavior. -Scott
On 07/18/2012 11:04 AM, Scott Wood wrote: > On 07/17/2012 09:38 PM, Rob Herring wrote: >> On 07/17/2012 08:11 PM, Scott Wood wrote: >>> Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 ("of: match by compatible >>> property first") breaks the gianfar ethernet driver found on various >>> Freescale PPC chips. >> >> You do know this is reverted, right? > > No, I didn't. I got it via Kumar's next branch, and saw that it was > still in your fixes-for-grant branch, and didn't see any revert-related > e-mail activity on the devicetree-discuss list about it. I now see that > it was reverted directly in Linus's tree (I didn't see either the > original or the revert in Linus's tree when I checked, but apparently I > hadn't fetched that as recently as I thought). > >> Here's my fix (untested) which is a bit simpler. I'm assuming if we care >> about which compatible string we are matching to, then we require name >> and type are blank and we only care about compatible strings. > > Any particular reason for making that assumption? We should be avoiding > the need for .name or .type matching in new bindings, but this seems > like unnecessarily inconsistent behavior. Only to ensure we don't change existing behavior and I think trying to cover all possibilities will be nearly impossible. For example, what if we match on both a specific compatible prop alone and a less specific compatible prop plus name and/or type. Which one do we pick as the better match? Rob
On 07/22/2012 08:56 PM, Rob Herring wrote: > On 07/18/2012 11:04 AM, Scott Wood wrote: >> On 07/17/2012 09:38 PM, Rob Herring wrote: >>> On 07/17/2012 08:11 PM, Scott Wood wrote: >>>> Commit 107a84e61cdd3406c842a0e4be7efffd3a05dba6 ("of: match by compatible >>>> property first") breaks the gianfar ethernet driver found on various >>>> Freescale PPC chips. >>> >>> You do know this is reverted, right? >> >> No, I didn't. I got it via Kumar's next branch, and saw that it was >> still in your fixes-for-grant branch, and didn't see any revert-related >> e-mail activity on the devicetree-discuss list about it. I now see that >> it was reverted directly in Linus's tree (I didn't see either the >> original or the revert in Linus's tree when I checked, but apparently I >> hadn't fetched that as recently as I thought). >> >>> Here's my fix (untested) which is a bit simpler. I'm assuming if we care >>> about which compatible string we are matching to, then we require name >>> and type are blank and we only care about compatible strings. >> >> Any particular reason for making that assumption? We should be avoiding >> the need for .name or .type matching in new bindings, but this seems >> like unnecessarily inconsistent behavior. > > Only to ensure we don't change existing behavior and I think trying to > cover all possibilities will be nearly impossible. For example, what if > we match on both a specific compatible prop alone and a less specific > compatible prop plus name and/or type. Which one do we pick as the > better match? Whichever one has a compatible string that is listed earlier. Having an additional type/name field doesn't necessarily make it a better match -- it's just there to resolve ambiguity (or sometimes for no good reason at all). You're going to change existing behavior in some case no matter what, short of a match table flag saying "it's OK to not keep depending on link order", or just not making the change. Might as well change it to something sane. -Scott
diff --git a/drivers/of/base.c b/drivers/of/base.c index eada3f4..6e10004 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -518,6 +518,9 @@ static const struct of_device_id *of_match_compat(const struct of_device_id *mat const char *cp = matches->compatible; int len = strlen(cp); + if (matches->name[0] || matches->type[0]) + continue; + if (len > 0 && of_compat_cmp(compat, cp, len) == 0) return matches;