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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Andy Shevchenko <andy@infradead.org>,
	Lee Jones <lee.jones@linaro.org>,
	Alexander Duyck <alexander.h.duyck@linux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v2 2/3] mfd: Intel Platform Monitoring Technology support
Date: Fri, 8 May 2020 12:15:22 +0300	[thread overview]
Message-ID: <CAHp75VfSUFh5rtieJZnfjTJCTpmONHGu3R_T0xU3CnuFv80x7g@mail.gmail.com> (raw)
In-Reply-To: <20200508021844.6911-3-david.e.box@linux.intel.com>

On Fri, May 8, 2020 at 5:18 AM David E. Box <david.e.box@linux.intel.com> 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