Re: [PATCH libdrm 2/2] xf85drm: de-duplicate drmParse{Platform.Host1x}{Bus, Device}Info

[Eric Engestrom <eric.engestrom@intel.com>]

On Wednesday, 2019-01-23 10:45:18 +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> The functions are virtually identical, fold them up.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

Assuming patch 1 is OK, and `foo` gets renamed to something better:
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

I don't know enough to review patch 1 though.

> ---
>  xf86drm.c | 98 +++++++++----------------------------------------------
>  1 file changed, 15 insertions(+), 83 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 374734eb..18c9693a 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3508,7 +3508,7 @@ free_device:
>      return ret;
>  }
>  
> -static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
> +static int drmParseOFBusInfo(int maj, int min, char *fullname)
>  {
>  #ifdef __linux__
>      char path[PATH_MAX + 1], *name, *foo;
> @@ -3532,19 +3532,18 @@ static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
>          foo++;
>      }
>  
> -    strncpy(info->fullname, foo, DRM_PLATFORM_DEVICE_NAME_LEN);
> -    info->fullname[DRM_PLATFORM_DEVICE_NAME_LEN - 1] = '\0';
> +    strncpy(fullname, foo, DRM_PLATFORM_DEVICE_NAME_LEN);
> +    fullname[DRM_PLATFORM_DEVICE_NAME_LEN - 1] = '\0';
>      free(name);
>  
>      return 0;
>  #else
> -#warning "Missing implementation of drmParsePlatformBusInfo"
> +#warning "Missing implementation of drmParseOFBusInfo"
>      return -EINVAL;
>  #endif
>  }
>  
> -static int drmParsePlatformDeviceInfo(int maj, int min,
> -                                      drmPlatformDeviceInfoPtr info)
> +static int drmParseOFDeviceInfo(int maj, int min, char ***compatible)
>  {
>  #ifdef __linux__
>      char path[PATH_MAX + 1], *value, *foo;
> @@ -3562,8 +3561,8 @@ static int drmParsePlatformDeviceInfo(int maj, int min,
>          count = 1;
>      }
>  
> -    info->compatible = calloc(count + 1, sizeof(*info->compatible));
> -    if (!info->compatible)
> +    *compatible = calloc(count + 1, sizeof(char *));
> +    if (!*compatible)
>          return -ENOMEM;
>  
>      for (i = 0; i < count; i++) {
> @@ -3587,19 +3586,19 @@ static int drmParsePlatformDeviceInfo(int maj, int min,
>              free(value);
>          }
>  
> -        info->compatible[i] = foo;
> +        *compatible[i] = foo;
>      }
>  
>      return 0;
>  
>  free:
>      while (i--)
> -        free(info->compatible[i]);
> +        free(*compatible[i]);
>  
> -    free(info->compatible);
> +    free(*compatible);
>      return err;
>  #else
> -#warning "Missing implementation of drmParsePlatformDeviceInfo"
> +#warning "Missing implementation of drmParseOFDeviceInfo"
>      return -EINVAL;
>  #endif
>  }
> @@ -3622,7 +3621,7 @@ static int drmProcessPlatformDevice(drmDevicePtr *device,
>  
>      dev->businfo.platform = (drmPlatformBusInfoPtr)ptr;
>  
> -    ret = drmParsePlatformBusInfo(maj, min, dev->businfo.platform);
> +    ret = drmParseOFBusInfo(maj, min, dev->businfo.platform->fullname);
>      if (ret < 0)
>          goto free_device;
>  
> @@ -3630,7 +3629,7 @@ static int drmProcessPlatformDevice(drmDevicePtr *device,
>          ptr += sizeof(drmPlatformBusInfo);
>          dev->deviceinfo.platform = (drmPlatformDeviceInfoPtr)ptr;
>  
> -        ret = drmParsePlatformDeviceInfo(maj, min, dev->deviceinfo.platform);
> +        ret = drmParseOFDeviceInfo(maj, min, &dev->deviceinfo.platform->compatible);
>          if (ret < 0)
>              goto free_device;
>      }
> @@ -3644,73 +3643,6 @@ free_device:
>      return ret;
>  }
>  
> -static int drmParseHost1xBusInfo(int maj, int min, drmHost1xBusInfoPtr info)
> -{
> -#ifdef __linux__
> -    char path[PATH_MAX + 1], *name;
> -
> -    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> -
> -    name = sysfs_uevent_get(path, "OF_FULLNAME");
> -    if (!name)
> -        return -ENOENT;
> -
> -    strncpy(info->fullname, name, DRM_HOST1X_DEVICE_NAME_LEN);
> -    info->fullname[DRM_HOST1X_DEVICE_NAME_LEN - 1] = '\0';
> -    free(name);
> -
> -    return 0;
> -#else
> -#warning "Missing implementation of drmParseHost1xBusInfo"
> -    return -EINVAL;
> -#endif
> -}
> -
> -static int drmParseHost1xDeviceInfo(int maj, int min,
> -                                    drmHost1xDeviceInfoPtr info)
> -{
> -#ifdef __linux__
> -    char path[PATH_MAX + 1], *value;
> -    unsigned int count, i;
> -    int err;
> -
> -    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> -
> -    value = sysfs_uevent_get(path, "OF_COMPATIBLE_N");
> -    if (!value)
> -        return -ENOENT;
> -
> -    sscanf(value, "%u", &count);
> -    free(value);
> -
> -    info->compatible = calloc(count + 1, sizeof(*info->compatible));
> -    if (!info->compatible)
> -        return -ENOMEM;
> -
> -    for (i = 0; i < count; i++) {
> -        value = sysfs_uevent_get(path, "OF_COMPATIBLE_%u", i);
> -        if (!value) {
> -            err = -ENOENT;
> -            goto free;
> -        }
> -
> -        info->compatible[i] = value;
> -    }
> -
> -    return 0;
> -
> -free:
> -    while (i--)
> -        free(info->compatible[i]);
> -
> -    free(info->compatible);
> -    return err;
> -#else
> -#warning "Missing implementation of drmParseHost1xDeviceInfo"
> -    return -EINVAL;
> -#endif
> -}
> -
>  static int drmProcessHost1xDevice(drmDevicePtr *device,
>                                    const char *node, int node_type,
>                                    int maj, int min, bool fetch_deviceinfo,
> @@ -3729,7 +3661,7 @@ static int drmProcessHost1xDevice(drmDevicePtr *device,
>  
>      dev->businfo.host1x = (drmHost1xBusInfoPtr)ptr;
>  
> -    ret = drmParseHost1xBusInfo(maj, min, dev->businfo.host1x);
> +    ret = drmParseOFBusInfo(maj, min, dev->businfo.host1x->fullname);
>      if (ret < 0)
>          goto free_device;
>  
> @@ -3737,7 +3669,7 @@ static int drmProcessHost1xDevice(drmDevicePtr *device,
>          ptr += sizeof(drmHost1xBusInfo);
>          dev->deviceinfo.host1x = (drmHost1xDeviceInfoPtr)ptr;
>  
> -        ret = drmParseHost1xDeviceInfo(maj, min, dev->deviceinfo.host1x);
> +        ret = drmParseOFDeviceInfo(maj, min, &dev->deviceinfo.host1x->compatible);
>          if (ret < 0)
>              goto free_device;
>      }
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel