[PATCH v2 0/2] Fix ODEV_ATTRIB_DRIVER overlapping with ODEV_ATTRIB_FD

[Hans de Goede]

Hi,

On 07/15/2014 11:33 AM, Keith Packard wrote:
> Hans de Goede <hdegoede at redhat.com> writes:
>
>> 1) I was not around when the OdevAttributes stuff got added, but I
>> think the idea behind it was to be able to add new atrributes without
>> breaking ABI (as was done this cycle when adding the driver string,
>> although that needed a bit of a fixup).
>
> The server ABI changes at every single X server release, so ABI
> compatibility of this interface isn't going to help driver authors at
> all.

True.

>> 2) It will require some ugly #ifdef-ery in almost all the drivers to work
>> with both the old and the new way.
>
> There are 18 references to ODEV_ in all of the open source drivers
> (master as of this morning):
>
> ./xf86-video-freedreno/src/msm-driver.c:		fd = xf86_get_platform_device_int_attrib(dev, ODEV_ATTRIB_FD, -1);
> ./xf86-video-opentegra/src/driver.c:                                               ODEV_ATTRIB_PATH);
> ./xf86-video-opentegra/src/driver.c:    char *path = xf86_get_platform_device_attrib(dev, ODEV_ATTRIB_PATH);
> ./xf86-video-modesetting/src/driver.c:        fd = xf86_get_platform_device_int_attrib(platform_dev, ODEV_ATTRIB_FD, -1);
> ./xf86-video-modesetting/src/driver.c:    const char *path = xf86_get_platform_device_attrib(dev, ODEV_ATTRIB_PATH);
> ./xf86-video-modesetting/src/driver.c:            ms->fd = xf86_get_platform_device_int_attrib(pEnt->location.id.plat, ODEV_ATTRIB_FD, -1);
> ./xf86-video-modesetting/src/driver.c:            char *path = xf86_get_platform_device_attrib(pEnt->location.id.plat, ODEV_ATTRIB_PATH);
> ./xf86-video-intel/src/intel_device.c:#if defined(ODEV_ATTRIB_PATH)
> ./xf86-video-intel/src/intel_device.c:	path = xf86_get_platform_device_attrib(dev, ODEV_ATTRIB_PATH);
> ./xf86-video-intel/src/intel_device.c:#if defined(ODEV_ATTRIB_FD)
> ./xf86-video-intel/src/intel_device.c:	return xf86_get_platform_device_int_attrib(dev, ODEV_ATTRIB_FD, -1);
> ./xf86-video-omap/src/omap_driver.c:				ODEV_ATTRIB_BUSID);
> ./xf86-video-omap/src/omap_driver.c:	char *busid = xf86_get_platform_device_attrib(dev, ODEV_ATTRIB_BUSID);
> ./xf86-video-ati/src/radeon_kms.c:                                                 ODEV_ATTRIB_FD, -1);
> ./xf86-video-vmware/vmwgfx/vmwgfx_driver.c:#ifdef ODEV_ATTRIB_FD
> ./xf86-video-vmware/vmwgfx/vmwgfx_driver.c:	                                                 ODEV_ATTRIB_FD, -1);
> ./xf86-video-qxl/src/qxl_kms.c:#if defined(ODEV_ATTRIB_FD)
> ./xf86-video-qxl/src/qxl_kms.c:                                                          ODEV_ATTRIB_FD, -1);
>
> We could either just have #ifdefs in these few places, or if we were
> really lazy, we could provide inline functions for the two getter
> functions used here and still net an overall savings in code.

I think defining 2 inlines for the 2 getter functions is a good idea.

Can you resend the patch as a proper top-level mail with the inlines
added? Then I'll review it.

I guess we should probably wait a bit with applying this to give others a
chance to respond.

Regards,

Hans