[alsa-devel] [PATCH 1/7] drm/i915: setup bridge for HDMI LPE audio driver

[Anand, Jerome]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Wednesday, December 14, 2016 5:13 PM
> To: Anand, Jerome <jerome.anand at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; alsa-devel at alsa-project.org;
> ville.syrjala at linux.intel.com; broonie at kernel.org; pierre-
> louis.bossart at linux.intel.com; Ughreja, Rakesh A
> <rakesh.a.ughreja at 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