> -----Original Message-----
> From: Takashi Iwai [mailto:ti...@suse.de]
> Sent: Wednesday, December 14, 2016 5:13 PM
> To: Anand, Jerome <jerome.an...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org;
> ville.syrj...@linux.intel.com; broo...@kernel.org; pierre-
> louis.boss...@linux.intel.com; Ughreja, Rakesh A
> <rakesh.a.ughr...@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 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
