Re: [PATCH v2 2/3] mfd: Intel Platform Monitoring Technology support

[Andy Shevchenko]

On Fri, May 8, 2020 at 5:18 AM David E. Box <david.e.box@xxxxxxxxxxxxxxx> wrote:
>
> Intel Platform Monitoring Technology (PMT) is an architecture for
> enumerating and accessing hardware monitoring facilities. PMT supports
> multiple types of monitoring capabilities. This driver creates platform
> devices for each type so that they may be managed by capability specific
> drivers (to be introduced). Capabilities are discovered using PCIe DVSEC
> ids. Support is included for the 3 current capability types, Telemetry,
> Watcher, and Crashlog. The features are available on new Intel platforms
> starting from Tiger Lake for which support is added. Tiger Lake however
> will not support Watcher and Crashlog even though the capabilities appear
> on the device. So add a quirk facility and use it to disable them.

Thank you for an update.
Some nitpicks below.

...

> +       case DVSEC_INTEL_ID_TELEM:

Is this from the spec? Or can we also spell TELEMETRY ?

> +               name = TELEM_DEV_NAME;

Ditto for all occurrences.

> +               break;

...

> +       cell = devm_kcalloc(&pdev->dev, header->num_entries,
> +                           sizeof(*cell), GFP_KERNEL);

I think if you use temporary
  struct device *dev = &pdev->dev;
you may squeeze this to one line and make others smaller as well.

> +       if (!cell)
> +               return -ENOMEM;

...

> +               res->start = pdev->resource[header->tbir].start +
> +                            header->offset +
> +                            (i * (INTEL_DVSEC_ENTRY_SIZE << 2));

Outer parentheses are redundant. And perhaps last two lines can be one.

...

> +static int
> +pmt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +       u16 vid;
> +       u32 table;

> +       int ret, pos = 0, last_pos = 0;

Redundant assignment of pos.

> +       while ((pos = pci_find_next_ext_capability(pdev, pos, PCI_EXT_CAP_ID_DVSEC))) {
> +               pci_read_config_word(pdev, pos + PCI_DVSEC_HEADER1, &vid);
> +               if (vid != PCI_VENDOR_ID_INTEL)
> +                       continue;
> +

> +               last_pos = pos;

Can we simple use a boolean flag?

> +       }
> +
> +       if (!last_pos) {
> +               dev_err(&pdev->dev, "No supported PMT capabilities found.\n");
> +               return -ENODEV;
> +       }

> +}

...

> +};

> +

Extra blank line.

> +module_pci_driver(pmt_pci_driver);

...

+ bits.h since GENMASK() is in use.

> +#include <linux/types.h>

...

> +enum pmt_quirks {
> +       /* Watcher capability not supported */
> +       PMT_QUIRK_NO_WATCHER    = (1 << 0),

BIT() ?

> +
> +       /* Crashlog capability not supported */
> +       PMT_QUIRK_NO_CRASHLOG   = (1 << 1),

BIT() ?

> +};

-- 
With Best Regards,
Andy Shevchenko