On Wed, 2 Jun 2021 14:32:59 +0300
Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx> wrote:
> Add support for Intel Quadrature Encoder Peripheral found on Intel
> Elkhart Lake platform.
>
> Initial implementation was done by Felipe Balbi while he was working at
> Intel with later changes from Raymond Tan and me.
>
> Co-developed-by: Felipe Balbi (Intel) <balbi@xxxxxxxxxx>
> Signed-off-by: Felipe Balbi (Intel) <balbi@xxxxxxxxxx>
> Co-developed-by: Raymond Tan <raymond.tan@xxxxxxxxx>
> Signed-off-by: Raymond Tan <raymond.tan@xxxxxxxxx>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
Hi,
Sorry I only got to this on v4 :(
Given all my comments are either minor or not specifically about code here,
feel free to send a follow up if you want to tidy them up.
Applied to the togreg branch of iio.git, initially pushed out as testing for
0-day etc to poke at it.
Thanks,
Jonathan
> ---
...
> +
> +static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> + struct intel_qep *qep;
> + struct device *dev = &pci->dev;
> + void __iomem *regs;
> + int ret;
> +
> + qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> + if (!qep)
> + return -ENOMEM;
> +
> + ret = pcim_enable_device(pci);
> + if (ret)
> + return ret;
> +
> + pci_set_master(pci);
> +
> + ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> + if (ret)
> + return ret;
> +
> + regs = pcim_iomap_table(pci)[0];
> + if (!regs)
> + return -ENOMEM;
> +
> + qep->dev = dev;
> + qep->regs = regs;
> + mutex_init(&qep->lock);
> +
> + intel_qep_init(qep);
> + pci_set_drvdata(pci, qep);
> +
> + qep->counter.name = pci_name(pci);
> + qep->counter.parent = dev;
> + qep->counter.ops = &intel_qep_counter_ops;
> + qep->counter.counts = intel_qep_counter_count;
> + qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> + qep->counter.signals = intel_qep_signals;
> + qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> + qep->counter.priv = qep;
> + qep->enabled = false;
> +
> + pm_runtime_put(dev);
> + pm_runtime_allow(dev);
> +
> + return devm_counter_register(&pci->dev, &qep->counter);
> +}
> +
> +static void intel_qep_remove(struct pci_dev *pci)
> +{
> + struct intel_qep *qep = pci_get_drvdata(pci);
to_pci_dev()
> + struct device *dev = &pci->dev;
> +
> + pm_runtime_forbid(dev);
> + if (!qep->enabled)
> + pm_runtime_get(dev);
Ouch, I'd not encountered this pci related weirdness before
(All about overriding the fact PCI opts out of runtime)
> +
> + intel_qep_writel(qep, INTEL_QEPCON, 0);
> +}
> +
> +#ifdef CONFIG_PM
Up to William and yourself, but I would prefer not to see these ifdefs
but instead mark the functions __maybe_unused and let the linker
drop them. It tends to be less error prone if the pm handling gets
more complex in future.
> +static int intel_qep_suspend(struct device *dev)
> +{
> + struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
to_pci_dev()
Though if all you are doing is using it to then get the drvdata
avoid the round trip. There have been a few patch sets tidying this up
in recent years and good not to add the noise of having that happen here.
struct intel_qep *qep = dev_get_drvdata(dev);
> + struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> + qep->qepcon = intel_qep_readl(qep, INTEL_QEPCON);
> + qep->qepflt = intel_qep_readl(qep, INTEL_QEPFLT);
> + qep->qepmax = intel_qep_readl(qep, INTEL_QEPMAX);
> +
> + return 0;
> +}
> +
> +static int intel_qep_resume(struct device *dev)
> +{
> + struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
to_pci_dev()
> + struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> + /*
> + * Make sure peripheral is disabled when restoring registers and
> + * control register bits that are writable only when the peripheral
> + * is disabled
> + */
> + intel_qep_writel(qep, INTEL_QEPCON, 0);
> + intel_qep_readl(qep, INTEL_QEPCON);
> +
> + intel_qep_writel(qep, INTEL_QEPFLT, qep->qepflt);
> + intel_qep_writel(qep, INTEL_QEPMAX, qep->qepmax);
> + intel_qep_writel(qep, INTEL_QEPINT_MASK, INTEL_QEPINT_MASK_ALL);
> +
> + /* Restore all other control register bits except enable status */
> + intel_qep_writel(qep, INTEL_QEPCON, qep->qepcon & ~INTEL_QEPCON_EN);
> + intel_qep_readl(qep, INTEL_QEPCON);
> +
> + /* Restore enable status */
> + intel_qep_writel(qep, INTEL_QEPCON, qep->qepcon);
> +
> + return 0;
> +}
> +#endif
> +
> +static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
> + intel_qep_suspend, intel_qep_resume, NULL);
> +
> +static const struct pci_device_id intel_qep_id_table[] = {
> + /* EHL */
> + { PCI_VDEVICE(INTEL, 0x4bc3), },
> + { PCI_VDEVICE(INTEL, 0x4b81), },
> + { PCI_VDEVICE(INTEL, 0x4b82), },
> + { PCI_VDEVICE(INTEL, 0x4b83), },
> + { } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
> +
> +static struct pci_driver intel_qep_driver = {
> + .name = "intel-qep",
> + .id_table = intel_qep_id_table,
> + .probe = intel_qep_probe,
> + .remove = intel_qep_remove,
> + .driver = {
> + .pm = &intel_qep_pm_ops,
> + }
> +};
> +
> +module_pci_driver(intel_qep_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi (Intel)");
> +MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Raymond Tan <raymond.tan@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Intel Quadrature Encoder Peripheral driver");
