Re: therm_pm72 units, interface

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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.