> +#define for_each_indication(ind, inds) \
> + for (ind = inds; ind->bit; ind++)
I find this more confusing than just seeing the actual for loop in
the two places using it, but that my just be personal preference.
/*
* Driver has internal list of supported indications. Ideally, driver should not
s/drivers/the driver/ both times?
> + if (!npem_has_dsm(dev)) {
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_NPEM);
> + if (pos == 0)
> + return;
> +
> + if (pci_read_config_dword(dev, pos + PCI_NPEM_CAP, &cap) != 0 ||
> + (cap & PCI_NPEM_CAP_CAPABLE) == 0)
> + return;
> + } else {
> + /*
> + * OS should use the DSM for LED control if it is available
> + * PCI Firmware Spec r3.3 sec 4.7.
> + */
> + return;
If you just did a:
if (npem_has_dsm(dev))
return;
we'd save a level of identation for the !dsm case an make the
code a bit easier to read. Also I think "OS" above should be "The OS".
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
