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

[Keith Packard]

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.

> +
> +#if defined(XSERVER_PLATFORM_BUS)
> +extern _X_EXPORT int platformSlotClaimed;
> +#endif
> +

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.

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


> +#ifdef XSERVER_PLATFORM_BUS
> +    i = xf86PlatformMatchDriver(matches, nmatches);
> +#endif

Should the Platform Bus be above SBUS?

>  xf86IsPrimaryPci(struct pci_device *pPci)
>  {
> -    return ((primaryBus.type == BUS_PCI) && (pPci == primaryBus.id.pci));
> +    if (primaryBus.type == BUS_PCI)
> +        return pPci == primaryBus.id.pci;
> +#ifdef XSERVER_PLATFORM_BUS
> +    if (primaryBus.type == BUS_PLATFORM)
> +        if (primaryBus.id.plat->pdev)
> +            if (MATCH_PCI_DEVICES(primaryBus.id.plat->pdev, pPci))
> +                return TRUE;
> +#endif
> +    return FALSE;
>  }

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...

> +/*
> + * 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?

> +static void
> +platform_find_pci_info(int idx, char *busid)

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


> +        if (pci && !strncmp(busid, "pci:", 4)) {

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

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20120620/bce29e06/attachment.pgp>