On Mon, Dec 25, 2023 at 06:26:56PM +0900, Shin'ichiro Kawasaki wrote:
> ...
> +static int p2sb_valid_resource(struct resource *res)
> +{
> + return res->flags ? 0 : -ENOENT;
> +}
This got worse because it's *named* like a boolean, but the return
value can't be used like a boolean, which makes callers really hard to
read, e.g., this:
if (p2sb_valid_resource(res))
/* do something */
does exactly the opposite of what the reader expects.
I see that you want to use this -ENOENT return value in the callers:
> +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> +{
> + ...
> + return p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res);
> +}
> + * 0 on success or appropriate errno value on error.
> + */
> +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
> +{
> + ...
> + ret = p2sb_valid_resource(&cache->res);
> + if (ret)
> + return ret;
But I think these would be much clearer as something like this:
static bool p2sb_valid_resource(struct resource *res)
{
if (res->flags)
return true;
return false;
}
static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
{
...
if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
return -ENOENT;
return 0;
}
Bjorn
