Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,


On 2024/2/2 19:58, Thomas Zimmermann wrote:
> +
> +/**
> + * screen_info_pci_dev() - Return PCI parent device that contains screen_info's framebuffer
> + * @si: the screen_info
> + *
> + * Returns:
> + * The screen_info's parent device on success, or NULL otherwise.
> + */
> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
> +{
> +	struct resource res[SCREEN_INFO_MAX_RESOURCES];
> +	ssize_t i, numres;
> +
> +	numres = screen_info_resources(si, res, ARRAY_SIZE(res));
> +	if (numres < 0)
> +		return ERR_PTR(numres);


Please return NULL at here, otherwise we have to use the IS_ERR or IS_ERR_OR_NULL()
in the caller function to check the returned value. Meanwhile, I noticed that you
didn't actually call IS_ERR() in the sysfb_parent_dev() function (introduced by the
3/8 patch), so I think we probably should return NULL at here.

Please also consider that the comments of this function says that it return NULL on
the otherwise cases.


> +	for (i = 0; i < numres; ++i) {
> +		struct pci_dev *pdev = __screen_info_pci_dev(&res[i]);
> +
> +		if (pdev)
> +			return pdev;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(screen_info_pci_dev);