[PATCH libdrm v3 3/5] xf86drm: Add platform and host1x bus support

[walter harms]

Am 19.01.2017 11:25, schrieb Thierry Reding:
> Adding back dri-devel at lists.freedesktop.org
> 
> On Wed, Jan 18, 2017 at 04:00:20PM +0100, walter harms wrote:
>> Am 18.01.2017 10:02, schrieb Thierry Reding:
> [...]
>>> diff --git a/xf86drm.c b/xf86drm.c
> [...]
>>> @@ -3187,11 +3199,55 @@ static int drmParsePciDeviceInfo(int maj, int min,
>>>  #endif
>>>  }
>>>  
>>> +static void drmFreePlatformDevice(drmDevicePtr device)
>>> +{
>>> +    if (device->deviceinfo.platform) {
>>
>> to save 2 indent level and improve readability :
>>
>> if (! device->deviceinfo.platform ||
>>     ! device->deviceinfo.platform->compatible)
>> 	return;
>>
>>> +        if (device->deviceinfo.platform->compatible) {
>>> +            char **compatible = device->deviceinfo.platform->compatible;
>>> +
>>> +            while (*compatible) {
>>> +                free(*compatible);
>>> +                compatible++;
>>> +            }
>>> +
>>> +            free(device->deviceinfo.platform->compatible);
>>> +        }
>>> +    }
> 
> I chose the above style because I thought readability wasn't impacted
> and it has the advantage of making the code more easily extensible
> should we ever need to add code to it.

i prefer otherwise, NTL its your patch if you are happy its ok for me

> 
>>> +static int drmProcessHost1xDevice(drmDevicePtr *device,
>>> +                                  const char *node, int node_type,
>>> +                                  int maj, int min, bool fetch_deviceinfo,
>>> +                                  uint32_t flags)
>>> +{
>>> +    drmDevicePtr dev;
>>> +    char *ptr;
>>> +    int ret;
>>> +
>>> +    dev = drmDeviceAlloc(node_type, node, sizeof(drmHost1xBusInfo),
>>> +                         sizeof(drmHost1xDeviceInfo), &ptr);
>>> +    if (!dev)
>>> +        return -ENOMEM;
>>> +
>>> +    dev->bustype = DRM_BUS_HOST1X;
>>> +
>>> +    dev->businfo.host1x = (drmHost1xBusInfoPtr)ptr;
>>> +
>>> +    ret = drmParseHost1xBusInfo(maj, min, dev->businfo.host1x);
>>> +    if (ret < 0)
>>> +        goto free_device;
>>> +
>>> +    if (fetch_deviceinfo) {
>>> +        ptr += sizeof(drmHost1xBusInfo);
>>> +        dev->deviceinfo.host1x = (drmHost1xDeviceInfoPtr)ptr;
>>> +
>>> +        ret = drmParseHost1xDeviceInfo(maj, min, dev->deviceinfo.host1x);
>>> +        if (ret < 0)
>>> +            goto free_device;
>>> +    }
>>> +
>>> +    *device = dev;
>>> +
>>
>> do you assume that fetch_deviceinfo may change dev ?
> 
> No, why?

You do *device = dev; after the if block that leaves the impression (for me)
that dev may change somewhere. therefor i would suggest moving this upwards.

re,
 wh

> 
> Thierry
> 
> 
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel