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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 02.02.24 um 18:03 schrieb Sui Jingfeng:
> 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.

Right. The idea is to return NULL is there is no parent device. The 
errno pointer is for actual runtime problems. The doc comment is still 
incorrect and I think I need to review the callers of this function. 
Thanks for pointing this out.

Best regards
Thomas

> > +    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);

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature