[PATCH 01/14] xfree86: use udev to provide device enumeration for kms devices (v9)

[Dave Airlie]

On Wed, Jun 20, 2012 at 9:10 PM, Keith Packard <keithp at keithp.com> wrote:
> Dave Airlie <airlied at gmail.com> writes:
>
>> +        else if (strcmp(udev_device_get_subsystem(udev_device), "drm"))
>> +            goto no_probe;
>> +        else if (strncmp(sysname, "card", 4))
>> +            goto no_probe;
>
> bikeshed -- strcmp(a,b) != 0; otherwise, I'm left wondering if you
> expected strcmp to return a bool.

Yeah makes things easier to read will fix these up in the next rev.
>
> From my reading of the code, it seems like pciSlotClaimed,
> platformSlotClaimed and sbusSlotClaimed can all be merged into a single
> 'we found something better than the generic driver' variable.

Sounds like a follow-up patch would be needed since I don't want to
cause a regression in this
patch by doing something too magical.

> Looks like xf86PostProbe is also broken here -- if you don't have
> LIBPCIACCESS, and try to use fbdev, it will refuse.

/me wonders who builds without libpciaccess these days, I suppose arm people.
>
> Should the Platform Bus be above SBUS?

Yeah probably makes sense for platform to go first, addressed in next revision.

>
> Ok, this seems a bit kludgy -- from what I can tell, you're essentially
> pulling PCI ids out of the udev device so that you can match drivers
> based on those instead of some 'other' mechanism. I don't have a better
> plan though; PCI ids are the best identifiers we've ever found...

Yeah the platform and pci layers had to be bit friendlier than I'd
like so if you probe a platform device as a primary but only a pci
interface driver is available it'll do the right thing. Its mostly
there as a fallback.

>
>> +/*
>> + * xf86IsPrimaryPlatform() -- return TRUE if primary device
>> + * is PCI and bus, dev and func numbers match.
>> + */
>> +
>> +static Bool
>> +xf86IsPrimaryPlatform(struct xf86_platform_device *plat)
>> +{
>> +    return ((primaryBus.type == BUS_PLATFORM) && (plat == primaryBus.id.plat));
>> +}
>
> The comment seems to imply matching on the PCI numbers, but instead
> you're matching on the platform device itself. Are those equivalent?

Yeah cut-n-paste, fixed in new rev.

>
> Ick. passing around indices to global arrays. No way to actually use a
> pointer to the xf86_platform_device structure here?

Fixed in new rev.

>
> bikeshed -- I prefer strncmp() == 0; strncmp isn't a boolean function.

Fixed in new rev.

I'll post the fixed up version later.

Dave.