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@xxxxxxxxxxxxxx>:
>> + /* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
