-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, December 14, 2016 5:13 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; ville.syrjala@linux.intel.com; broonie@kernel.org; pierre- louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [PATCH 1/7] drm/i915: setup bridge for HDMI LPE audio driver
On Mon, 12 Dec 2016 19:10:37 +0100, Jerome Anand wrote:
--- /dev/null +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
(snip)
+static struct platform_device*
Missing space.
OK
+lpe_audio_platdev_create(struct drm_i915_private *dev_priv) {
- struct drm_device *dev = &dev_priv->drm;
- int ret;
- struct resource rsc[2] = {};
- struct platform_device *platdev;
- u64 *dma_mask;
- struct intel_hdmi_lpe_audio_pdata *pdata;
- if (dev_priv->lpe_audio.irq < 0)
return ERR_PTR(-EINVAL);This was already tested, no?
OK - can be removed
- platdev = platform_device_alloc("hdmi-lpe-audio", -1);
- if (!platdev) {
ret = -ENOMEM;DRM_ERROR("Failed to allocate LPE audio platformdevice\n");
goto err;- }
- /* to work-around check_addr in nommu_map_sg() */
- dma_mask = kmalloc(sizeof(*platdev->dev.dma_mask),
GFP_KERNEL);
- if (!dma_mask) {
ret = -ENOMEM;DRM_ERROR("Failed to allocate dma_mask\n");goto err_put_dev;- }
- *dma_mask = DMA_BIT_MASK(32);
- platdev->dev.dma_mask = dma_mask;
- platdev->dev.coherent_dma_mask = *dma_mask;
- rsc[0].start = rsc[0].end = dev_priv->lpe_audio.irq;
- rsc[0].flags = IORESOURCE_IRQ;
- rsc[0].name = "hdmi-lpe-audio-irq";
- rsc[1].start = pci_resource_start(dev->pdev, 0) +
I915_HDMI_LPE_AUDIO_BASE;- rsc[1].end = pci_resource_start(dev->pdev, 0) +
I915_HDMI_LPE_AUDIO_BASE +I915_HDMI_LPE_AUDIO_SIZE - 1;
- rsc[1].flags = IORESOURCE_MEM;
- rsc[1].name = "hdmi-lpe-audio-mmio";
- ret = platform_device_add_resources(platdev, rsc, 2);
- if (ret) {
DRM_ERROR("Failed to add resource for platform device:%d\n",
ret);goto err_put_dev;- }
- pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
- if (pdata == NULL) {
ret = -ENOMEM;goto err_put_dev;- }
- platdev->dev.platform_data = pdata;
- spin_lock_init(&pdata->lpe_audio_slock);
- /* for LPE audio driver's runtime-PM */
- platdev->dev.parent = dev->dev;
- ret = platform_device_add(platdev);
- if (ret) {
DRM_ERROR("Failed to add LPE audio platform device:%d\n",
ret);goto err_put_dev;- }
- return platdev;
I guess using platform_device_register_full() makes the code a bit simpler.
OK - agreed, but will keep it since its acked by Daniel.
static struct platform_device * lpe_audio_platdev_create(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; struct platform_device_info pinfo = {}; struct resource rsc[2]; struct platform_device *platdev; struct intel_hdmi_lpe_audio_pdata *pdata;
pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); if (!pdata) return ERR_PTR(-ENOMEM); spin_lock_init(&pdata->lpe_audio_slock);
rsc[0].start = rsc[0].end = dev_priv->lpe_audio.irq; rsc[0].flags = IORESOURCE_IRQ; rsc[0].name = "hdmi-lpe-audio-irq";
rsc[1].start = pci_resource_start(dev->pdev, 0) + I915_HDMI_LPE_AUDIO_BASE; rsc[1].end = pci_resource_start(dev->pdev, 0) + I915_HDMI_LPE_AUDIO_BASE + I915_HDMI_LPE_AUDIO_SIZE - 1; rsc[1].flags = IORESOURCE_MEM; rsc[1].name = "hdmi-lpe-audio-mmio";
pinfo.parent = dev->dev; pinfo.name = "hdmi-lpe-audio"; pinfo.id = -1; pinfo.res = res; pinfo.num_res = 2; pinfo.data = pdata; pinfo.size_data = sizeof(*pdata); pinfo.dma_mask = DMA_BIT_MASK(32);
platdev = platform_device_register_full(&pinfo); if (IS_ERR(platdev)) { ret = PTR_ERR(platdev); DRM_ERROR("Failed to allocate LPE audio platform device\n"); goto err; }
return platdev;
err: kfree(pdata); return ERR_PTR(ret); }
+/**
- intel_lpe_audio_irq_handler() - forwards the LPE audio irq
- @dev_priv: the i915 drm device private data
- the LPE Audio irq is forwarded to the irq handler registered by
+LPE audio
- driver.
- */
+void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv) {
- int ret;
- ret = generic_handle_irq(dev_priv->lpe_audio.irq);
- if (ret)
DRM_ERROR_RATELIMITED("error handling LPE audio irq:%d\n",
ret);This function may be called even without LPE registered, so safer to have HAS_LPE_AUDIO() check.
OK
+}
+/**
- intel_lpe_audio_detect() - check & setup lpe audio if present
- @dev_priv: the i915 drm device private data
- Detect if lpe audio is present
- Return: true if lpe audio present else Return = false */ bool
+intel_lpe_audio_detect(struct drm_i915_private *dev_priv) {
- int lpe_present = false;
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
static const struct pci_device_id atom_hdaudio_ids[] = {/* Baytrail */{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0f04)},/* Braswell */{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x2284)},{}};if (!pci_dev_present(atom_hdaudio_ids)) {DRM_INFO("%s%s\n", "HDaudio controller notdetected,",
"using LPE audio instead\n");Why %s%s? Just keep one line without line breakage. The 80 chars rule is just a suggestion, no strict rule at all.
OK
+/**
- intel_lpe_audio_teardown() - destroy the bridge between HDMI LPE
- audio driver and i915
- @dev_priv: the i915 drm device private data
- release all the resources for LPE audio <-> i915 bridge.
- */
+void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv) {
- unsigned long irqflags;
- struct irq_desc *desc;
- if (!HAS_LPE_AUDIO(dev_priv))
return;- desc = irq_to_desc(dev_priv->lpe_audio.irq);
- /**
* mask LPE audio irq before destroying*/No need for kernel-doc comment here.
OK
- lpe_audio_irq_mask(&desc->irq_data);
- spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- lpe_audio_platdev_destroy(dev_priv);
- irq_free_desc(dev_priv->lpe_audio.irq);
- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
What's the reason of this spinlock?
JIC - probably not needed now
thanks,
Takashi