[PATCH <i-g-t> v4 1/4] lib/igt_device_scan: Enable finding all IGT devices for a specific driver

[Krzysztof Karas]

Hi Soham,

[...]

> +/**
> + * Iterate over all igt_devices array and find all discrete/integrated cards.
> + * @card: double pointer to igt_device_card structure, containing
> + * an array of igt_device_card structures upon successful return.
You could rename this parameter to "cards" and write something
like "an array of cards to be filled". The fact that this is
a double pointer of igt_device struct is apparent upon first
glance at the function signature.

> + * @card_type: flag to indicate whether to find discrete, integrated, or
> + * both types of cards. Use 0 for integrated, 1 for discrete, and 2 for both.
Instead of explaining what certain values mean here, you could
use "dev_type" enum defined earlier in this file. That would
prevent the code from using magic numbers.

> + * @drv_name: name of the driver to match.
> + *
> + * Returns the number of cards found, or -1 on error.
> + */
> +int find_all_intel_card_by_driver_name(struct igt_device_card **card,
> +				       uint8_t card_type, const char *drv_name)
In this file there are already a few functions that begin with
"igt_device_find" - you could keep that convention here and
rename this function to something like
"igt_device_find_all_by_driver".

> +{
> +	int count = 0;
> +	struct igt_device *dev;
> +	int is_integrated;
> +	struct igt_device_card *tmp;
> +	struct igt_device_card *crd = NULL;
> +
> +	igt_assert(drv_name);
> +	*card = NULL;
> +
> +	igt_list_for_each_entry(dev, &igt_devs.all, link) {
> +		if (!is_pci_subsystem(dev) || strcmp(dev->driver, drv_name))
> +			continue;
> +
> +		is_integrated = !strncmp(dev->pci_slot_name, INTEGRATED_I915_GPU_PCI_ID,
> +						PCI_SLOT_NAME_SIZE);
> +
> +		if ((card_type == 1 && !is_integrated) ||
> +		    (card_type == 0 && is_integrated) ||
> +		    card_type == 2) {
It would be more readable to see named values from earlier
mentioned enum: DEVTYPE_ALL, DEVTYPE_INTEGRATED,
DEVTYPE_DISCRETE.

> +			tmp = realloc(crd, sizeof(struct igt_device_card) * (count + 1));
> +			if (!tmp) {
> +				free(crd);
> +				return -1;
> +			}
> +
> +			crd = tmp;
> +			__copy_dev_to_card(dev, &crd[count]);
> +			count++;
> +		}
> +	}
> +
> +	if (!count) {
> +		if (!crd)
> +			free(crd);
If the "count" is equal to 0 at this point then "crd" will be
NULL, so this call to free() here seems unnecessary (unless I
missed something).

Best Regards,
Krzysztof