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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
