Message ID | 1375693993.3852.143.camel@thor.local (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
On Mon, 2013-08-05 at 11:13 +0200, Michel Dänzer wrote: > > static struct platform_driver i2c_powermac_driver = { > > .probe = i2c_powermac_probe, > > .remove = i2c_powermac_remove, > > + .id_table = *i2c_powermac_id, > > This fails to build: > > CC [M] drivers/i2c/busses/i2c-powermac.o > drivers/i2c/busses/i2c-powermac.c:469:14: error: invalid type argument > of unary ‘*’ (have ‘const struct platform_device_id’) > make[1]: *** [drivers/i2c/busses/i2c-powermac.o] Error 1 Yeah, obvious typo, I said it was completely untested :-) > The version below builds, but the module still doesn't get loaded > automagically (unless I'm missing some command I need to run between > copying the new module to /lib/modules/$(uname -r)/ and rebooting?). depmod -a ? > Looking at other drivers in drivers/i2c/busses/, maybe > i2c_powermac_driver.driver needs an of_match_table entry? No, that would be messy, the driver is just an interface layer on top of the low-i2c.c stuff in arch, which abstracts 3 different i2c controllers and inconsistent device-tree representations. It's done outside of the normal i2c framework because it's used in some special contexts such as when running the platform functions, at early boot or sleep/wakeup time. Also it's historical stuff I'd rather not touch since I don't have that many different combos to test with anymore. However, the kernel creates platform device so the normal platform matching mechanism should work... we might be missing something. > diff --git a/drivers/i2c/busses/i2c-powermac.c > b/drivers/i2c/busses/i2c-powermac.c > index 8dc90da..74066fc 100644 > --- a/drivers/i2c/busses/i2c-powermac.c > +++ b/drivers/i2c/busses/i2c-powermac.c > @@ -458,9 +458,19 @@ static int i2c_powermac_probe(struct > platform_device *dev) > return rc; > } > > +static const struct platform_device_id i2c_powermac_id[] = { > + { > + .name = "i2c-powermac" > + }, { > + /* sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(platform, i2c_powermac_id); > + > static struct platform_driver i2c_powermac_driver = { > .probe = i2c_powermac_probe, > .remove = i2c_powermac_remove, > + .id_table = i2c_powermac_id, > .driver = { > .name = "i2c-powermac", > .bus = &platform_bus_type, > @@ -468,5 +478,3 @@ static struct platform_driver i2c_powermac_driver > = > { > }; > > module_platform_driver(i2c_powermac_driver); > - > -MODULE_ALIAS("platform:i2c-powermac"); Maybe add the module alias back ? It shouldn't be necessary... Cheers, Ben.
On Mon, 2013-08-05 at 19:22 +1000, Benjamin Herrenschmidt wrote: > On Mon, 2013-08-05 at 11:13 +0200, Michel Dänzer wrote: > > > The version below builds, but the module still doesn't get loaded > > automagically (unless I'm missing some command I need to run between > > copying the new module to /lib/modules/$(uname -r)/ and rebooting?). > > depmod -a ? I did that, sorry should have mentioned that. > > @@ -468,5 +478,3 @@ static struct platform_driver i2c_powermac_driver > > = > > { > > }; > > > > module_platform_driver(i2c_powermac_driver); > > - > > -MODULE_ALIAS("platform:i2c-powermac"); > > Maybe add the module alias back ? It shouldn't be necessary... Doesn't help.
On Mon, 2013-08-05 at 12:32 +0200, Michel Dänzer wrote: > > I did that, sorry should have mentioned that. > > > > > @@ -468,5 +478,3 @@ static struct platform_driver > i2c_powermac_driver > > > = > > > { > > > }; > > > > > > module_platform_driver(i2c_powermac_driver); > > > - > > > -MODULE_ALIAS("platform:i2c-powermac"); > > > > Maybe add the module alias back ? It shouldn't be necessary... > > Doesn't help. Hrm, that might require some more involved debugging, figuring out what's up with udev etc... that or maybe bisecting. From what I can tell, we do attach an OF node to the platform device, but since the driver has no of match table, we should still fallback to the old platform matching style. I think I see... adding Grant. Grant, something broke the auto-loading the of i2c-powermac module. It's a platform device, but while it does have a populated "of_node, its driver doesn't have an OF match table and relies on the old style platform device matching. That's broken with newer kernels, platform_uevent() calls of_device_uevent_modalias() which sees the of_node and thus doesn't return -ENOMEM, and we don't create a platform modalias anymore. Is it legit to add several MODALIAS ? If yes we could add both ... if not, we have a problem. Doing real OF matching with that critter is tricky at best for various reasons but it needs the of_node because it uses it to scan for children. Any suggestion ? Cheers, Ben.
On Mon, 2013-08-05 at 20:53 +1000, Benjamin Herrenschmidt wrote: > On Mon, 2013-08-05 at 12:32 +0200, Michel Dänzer wrote: > > > > I did that, sorry should have mentioned that. > > > > > > > > @@ -468,5 +478,3 @@ static struct platform_driver > > i2c_powermac_driver > > > > = > > > > { > > > > }; > > > > > > > > module_platform_driver(i2c_powermac_driver); > > > > - > > > > -MODULE_ALIAS("platform:i2c-powermac"); > > > > > > Maybe add the module alias back ? It shouldn't be necessary... > > > > Doesn't help. > > Hrm, that might require some more involved debugging, figuring out > what's up with udev etc... that or maybe bisecting. > > From what I can tell, we do attach an OF node to the platform device, > but since the driver has no of match table, we should still fallback to > the old platform matching style. > > I think I see... adding Grant. Did that work? He's not in Cc on the post I received from the list, but that might be due to his mailman settings. > Grant, something broke the auto-loading the of i2c-powermac module. It's > a platform device, but while it does have a populated "of_node, its > driver doesn't have an OF match table and relies on the old style > platform device matching. > > That's broken with newer kernels, platform_uevent() calls > of_device_uevent_modalias() which sees the of_node and thus doesn't > return -ENOMEM, and we don't create a platform modalias anymore. > > Is it legit to add several MODALIAS ? If yes we could add both ... if > not, we have a problem. Doing real OF matching with that critter is > tricky at best for various reasons but it needs the of_node because it > uses it to scan for children. > > Any suggestion ? Any news on this? I recently learned the hard way that loading i2c-powermac manually via /etc/modules isn't a good workaround, as that only happens after fsck...
diff --git a/drivers/i2c/busses/i2c-powermac.c b/drivers/i2c/busses/i2c-powermac.c index 8dc90da..74066fc 100644 --- a/drivers/i2c/busses/i2c-powermac.c +++ b/drivers/i2c/busses/i2c-powermac.c @@ -458,9 +458,19 @@ static int i2c_powermac_probe(struct platform_device *dev) return rc; } +static const struct platform_device_id i2c_powermac_id[] = { + { + .name = "i2c-powermac" + }, { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(platform, i2c_powermac_id); + static struct platform_driver i2c_powermac_driver = { .probe = i2c_powermac_probe, .remove = i2c_powermac_remove, + .id_table = i2c_powermac_id, .driver = { .name = "i2c-powermac",