> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Wednesday, December 14, 2016 5:13 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > ville.syrjala@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; pierre- > louis.bossart@xxxxxxxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx> > 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 platform > device\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 not > detected,", > > + "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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx