diff mbox

therm_pm72 units, interface

Message ID 1375693993.3852.143.camel@thor.local (mailing list archive)
State Not Applicable
Headers show

Commit Message

Michel Dänzer Aug. 5, 2013, 9:13 a.m. UTC 
On Sam, 2013-08-03 at 07:02 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2013-08-02 at 18:52 +0200, Michel Dänzer wrote:
> 
> > Thanks for the suggestion. The same windfarm modules were loaded in both
> > cases, but i2c_powermac wasn't loaded with the newer kernels. Loading it
> > manually fixes the problem.
> > 
> > How is i2c_powermac supposed to get loaded with current kernels?
> 
> It's a platform driver, but it's missing a module device-table
> 
> Can you try this completely untested patch ?
> 
> diff --git a/drivers/i2c/busses/i2c-powermac.c b/drivers/i2c/busses/i2c-powermac.c
> index 8dc90da..5af5153 100644
> --- a/drivers/i2c/busses/i2c-powermac.c
> +++ b/drivers/i2c/busses/i2c-powermac.c
> @@ -458,9 +458,15 @@ static int i2c_powermac_probe(struct platform_device *dev)
>  	return rc;
>  }
>  
> +static const struct platform_device_id i2c_powermac_id = {
> +	.name = "i2c-powermac"
> +};
> +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,

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

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?).

Looking at other drivers in drivers/i2c/busses/, maybe
i2c_powermac_driver.driver needs an of_match_table entry?


                .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");

Comments

Benjamin Herrenschmidt Aug. 5, 2013, 9:22 a.m. UTC | #1 
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.
Michel Dänzer Aug. 5, 2013, 10:32 a.m. UTC | #2 
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.
Benjamin Herrenschmidt Aug. 5, 2013, 10:53 a.m. UTC | #3 
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.
Michel Dänzer Sept. 27, 2013, 3:44 p.m. UTC | #4 
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 mbox

Patch

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",