Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 8 May 2020 12:15:22 +0300 | Subject | Re: [PATCH v2 2/3] mfd: Intel Platform Monitoring Technology support |
| |
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
| |