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

From: Takashi Iwai <tiwai@suse.de>
To: Jerome Anand <jerome.anand@intel.com>
Cc: alsa-devel@alsa-project.org, intel-gfx@lists.freedesktop.org,
	broonie@kernel.org, rakesh.a.ughreja@intel.com
Subject: Re: [PATCH 1/7] drm/i915: setup bridge for HDMI LPE audio driver
Date: Wed, 14 Dec 2016 12:43:18 +0100	[thread overview]
Message-ID: <s5hy3zienxl.wl-tiwai@suse.de> (raw)
In-Reply-To: <20161212181043.12512-2-jerome.anand@intel.com>

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.

> +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?

> +
> +	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.

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.

> +}
> +
> +/**
> + * 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.

> +/**
> + * 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.

> +	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?

thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx