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

[Andy Shevchenko]

On Tue, May 5, 2020 at 5:32 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. Capabilities are discovered
> using PCIe DVSEC with the Intel VID. Each capability is discovered as a
> separate DVSEC instance in a device's config space. This driver uses MFD to
> manage the creation of platform devices for each type so that they may be
> controlled by their own drivers (to be introduced).  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.

...

>  include/linux/intel-dvsec.h |  44 +++++++++

I guess it's no go for a such header, since we may end up with tons of
a such. Perhaps simple pcie-dvsec.h ?

...

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8783,6 +8783,11 @@ S:       Maintained
>  F:     arch/x86/include/asm/intel_telemetry.h
>  F:     drivers/platform/x86/intel_telemetry*
>
> +INTEL PMT DRIVER
> +M:     "David E. Box" <david.e.box@xxxxxxxxxxxxxxx>
> +S:     Maintained
> +F:     drivers/mfd/intel_pmt.c

I believe you forgot to run parse-maintainers.pl --order
--input=MAINTAINERS --output=MAINTAINERS

...

> +       info = devm_kmemdup(&pdev->dev, (void *)id->driver_data, sizeof(*info),
> +                           GFP_KERNEL);

> +

Extra blank line.

> +       if (!info)
> +               return -ENOMEM;
> +
> +       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;

Perhaps a candidate for for_each_vendor_cap() macro in pcie-dvsec.h.
Or how is it done for the rest of capabilities?

> +       }

...

> +static const struct pci_device_id pmt_pci_ids[] = {
> +       /* TGL */

> +       { PCI_VDEVICE(INTEL, 0x9a0d), (kernel_ulong_t)&tgl_info },

PCI_DEVICE_DATA()?

> +       { }
> +};

-- 
With Best Regards,
Andy Shevchenko