Re: [PATCH 1/1] MFD: Add U300 AB3100 core support v2

[Linus Walleij <linus.ml.walleij-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hm I shouldn't been so trigger-happy as to send v3 out so soon... :-/

Thanks for you quick review Mike!

2009/5/19 Mike Rapoport <mike-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>:

>> +     /* Set a pointer back to the container in device data */
>> +     for (i = 0; i < ARRAY_SIZE(ab3100_platform_devs); i++)
>> +             platform_set_drvdata(ab3100_platform_devs[i], ab3100_local);
>> +
>> +     /* Register the platform devices */
>> +     platform_add_devices(ab3100_platform_devs,
>> +                          ARRAY_SIZE(ab3100_platform_devs));
>
> If you register sub-devices this way, they won't appear as ab3100 children.

I'll set the parent in the first loop then (will test that this works) because I
really like that device table. (Similar to how the board setup does things.)

> Besides, setting the sub-devices drvdata to point to the master device makes it
> impossible for sub-device drivers to use this field for their needs.

Not really... It's just a pointer back to the AB3100 parent that can
be discarded
after copying it to a local driver struct. So the driver can actually:

foo_probe(platform_device *pdev) {
  foo = kmalloc(...);
  foo->ab_pointer = platform_get_drvdata(pdev);
  ...
  platform_set_drvdata(pdev, foo);
}

I could use platform_device_add_data() instead but that will start
kmalloc():ing and such...

>> +/*
>> + * This struct is PRIVATE and devices using it should NOT
>> + * access ANY fields. It is used as a token for calling the
>> + * AB3100 functions.
>> + */
>
> If the struct is private, probably it should be placed rather in the ".c" file?

From one point of view, yes, but from another point of view it feels bad to
pass around a (void *) as token for ab3100 operations. This is loosely
modeled on how wm8350 does it, so shouldn't be too controversial I think.

Yours,
Linus Walleij