"Hald, Ulrik Bech" <ubh@xxxxxx> writes:
[...]
>>
>> Drop this and just use the inode match.
>>
> Was considering that, but ended up defaulting value instead of error checking later. I'll change the above.
>
>> > + /* Find match between device node and wdt device */
>> > + int i;
>> > + for (i = 0; i < NUM_WDTS; i++) {
>> > + if (omap_wdt_dev[i]) {
>> > + wdev = platform_get_drvdata(omap_wdt_dev[i]);
>> > + if (iminor(inode) == wdev->omap_wdt_miscdev.minor)
>> > + break;
>> > + }
>> > + }
>>
>> You should check for a valid match here.
>>
> The sanity check I would choose here is
>
> struct omap_wdt_dev *wdev = NULL;
> ...
> <inode matching>
> ...
> if(!wdev)
> return -ENODEV;
>
Looks fine.
[...]
>>
>> The more think about this, the more I don't like this pdev->id
>> switching in the driver. The only thing it is needed for
>> is to set the name of the node.
>>
>> Instead, why not set the name in devices.c and pass it in
>> using platform_data.
>>
>> Then you can drop the enum and the pdev->id switching.
>>
>
> That does simplify things a bit. Had overlooked that way of sharing info between device and driver.
> So, devices.c would have something like:
>
> static struct platform_device omap_secure_wdt_device = {
> .name = "omap_wdt",
> .id = 1,
> .num_resources = ARRAY_SIZE(secure_wdt_resources),
> .resource = secure_wdt_resources,
> .dev = {
> .platform_data = "watchdog_secure",
> },
> };
>
> And omap_wdt.c would have in probe():
>
> wdev->omap_wdt_miscdev.name = (char *) pdev->dev.platform_data;
>
> Is that more like what you had in mind?
>
Exactly.
This is typically done with a platform_data struct, and a pointer to
the struct is passed as the platform_data, but in this case since the
struct would only have one field for the name pointer, this should be
fine.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
