Re: [alsa-devel] [PATCH V3 3/5] ALSA: add Intel HDMI LPE audio driver for BYT/CHT-T

On Tue, 10 Jan 2017 06:08:39 +0100, Jerome Anand wrote:

On Baytrail and Cherrytrail, HDaudio may be fused out or disabled by the BIOS. This driver enables an alternate path to the i915 display registers and DMA.

Although there is no hardware path between i915 display and LPE/SST audio clusters, this HDMI capability is referred to in the documentation as "HDMI LPE Audio" so we keep the name for consistency. There is no hardware path or control dependencies with the LPE/SST DSP functionality.

The hdmi-lpe-audio driver will be probed when the i915 driver creates a child platform device.

Since this driver is neither SoC nor PCI, a new x86 folder is added Additional indirections in the code will be cleaned up in the next series to aid smoother DP integration

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Jerome Anand jerome.anand@intel.com

Just a few knitpicks below:

--- a/sound/Makefile +++ b/sound/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_SOUND) += soundcore.o obj-$(CONFIG_SOUND_PRIME) += oss/ obj-$(CONFIG_DMASOUND) += oss/ obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ sh/ synth/ usb/ \

• firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/

• firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/ x86/

obj-$(CONFIG_SND_AOA) += aoa/

# This one must be compilable even if sound is configured out diff --git a/sound/x86/Kconfig b/sound/x86/Kconfig new file mode 100644 index 0000000..e9297d0 --- /dev/null +++ b/sound/x86/Kconfig @@ -0,0 +1,16 @@ +menuconfig SND_X86

• tristate "X86 sound devices"

This should depends on X86.

• ---help---

•  X86 sound devices that don't fall under SoC or PCI categories
  

+if SND_X86

+config HDMI_LPE_AUDIO

• tristate "HDMI audio without HDaudio on Intel Atom platforms"
• depends on DRM_I915
• default n

default=n is superfluous.

• help

• Choose this option to support HDMI LPE Audio mode
  

+endif # SND_X86 diff --git a/sound/x86/Makefile b/sound/x86/Makefile new file mode 100644 index 0000000..54d002d --- /dev/null +++ b/sound/x86/Makefile @@ -0,0 +1,6 @@ +ccflags-y += -Iinclude/drm

Do we really need this?

+snd-hdmi-lpe-audio-objs += \

• intel_hdmi_lpe_audio.o

+obj-$(CONFIG_HDMI_LPE_AUDIO) += snd-hdmi-lpe-audio.o diff --git a/sound/x86/intel_hdmi_lpe_audio.c b/sound/x86/intel_hdmi_lpe_audio.c new file mode 100644 index 0000000..c2ebce1 --- /dev/null +++ b/sound/x86/intel_hdmi_lpe_audio.c @@ -0,0 +1,614 @@ +/*

• • intel_hdmi_lpe_audio.c - Intel HDMI LPE audio driver for Atom platforms
• • Copyright (C) 2016 Intel Corp
• • Authors:

• • Jerome Anand <jerome.anand@intel.com>
    

• • Aravind Siddappaji <aravindx.siddappaji@intel.com>
    

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; version 2 of the License.
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License for more details.
• */

+#include <linux/platform_device.h> +#include <linux/irqreturn.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <sound/pcm.h> +#include <sound/core.h>

Usually sound/core.h comes before other sound/* inclusions.

+#include <sound/pcm_params.h> +#include <sound/initval.h> +#include <sound/control.h> +#include <sound/initval.h> +#include <drm/intel_lpe_audio.h> +#include "intel_hdmi_lpe_audio.h"

+/* globals*/ +struct platform_device *hlpe_pdev; +int hlpe_state; +union otm_hdmi_eld_t hlpe_eld;

Are these really globals? I couldn't find the reference from others in the patchset.

+struct hdmi_lpe_audio_ctx {

• int irq;
• void __iomem *mmio_start;
• had_event_call_back had_event_callbacks;
• struct snd_intel_had_interface *had_interface;
• void *had_pvt_data;
• int tmds_clock_speed;
• unsigned int had_config_offset;
• int hdmi_audio_interrupt_mask;
• struct work_struct hdmi_audio_wq;

+};

+static inline void hdmi_set_eld(void *eld)

You need no inline unless it's really mandatory. Let compiler optimize. It's often smarter than us.

+{

• int size;
• BUILD_BUG_ON(sizeof(hlpe_eld) > HDMI_MAX_ELD_BYTES);
• size = sizeof(hlpe_eld);
• memcpy((void *)&hlpe_eld, eld, size);

+}

+static inline int hdmi_get_eld(void *eld) +{

• uint8_t *eld_data = (uint8_t *)&hlpe_eld;
• memcpy(eld, (void *)&hlpe_eld, sizeof(hlpe_eld));
• print_hex_dump_bytes("eld: ", DUMP_PREFIX_NONE, eld_data,

• 	sizeof(hlpe_eld));
  

• return 0;

+}

+static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void) +{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• return ctx;

+}

+/*

• • return whether HDMI audio device is busy.
• */

+bool mid_hdmi_audio_is_busy(void *ddev) +{

• struct hdmi_lpe_audio_ctx *ctx;
• int hdmi_audio_busy = 0;
• struct hdmi_audio_event hdmi_audio_event;
• dev_dbg(&hlpe_pdev->dev, "%s: Enter", __func__);
• ctx = platform_get_drvdata(hlpe_pdev);
• if (hlpe_state == hdmi_connector_status_disconnected) {

• /* HDMI is not connected, assuming audio device is idle. */
  

• return false;
  

• }
• if (ctx->had_interface) {

• hdmi_audio_event.type = HAD_EVENT_QUERY_IS_AUDIO_BUSY;
  

• hdmi_audio_busy = ctx->had_interface->query(
  

• 		ctx->had_pvt_data,
  

• 		hdmi_audio_event);
  

• return hdmi_audio_busy != 0;
  

• }
• return false;

+}

+/*

• • return whether HDMI audio device is suspended.

It's a bit confusing description. The function does suspend and returns true if it has suspended, or disconnected / IF doesn't exist.

IMO, a standard negative error code or zero return value would be more straightforward in such a case.

• */

+bool mid_hdmi_audio_suspend(void *ddev) +{

• struct hdmi_lpe_audio_ctx *ctx;
• struct hdmi_audio_event hdmi_audio_event;
• int ret = 0;
• ctx = platform_get_drvdata(hlpe_pdev);
• if (hlpe_state == hdmi_connector_status_disconnected) {

• /* HDMI is not connected, assuming audio device
  

•  * is suspended already.
  

•  */
  

• return true;
  

• }
• dev_dbg(&hlpe_pdev->dev, "%s: hlpe_state %d", __func__,

• 	hlpe_state);
  

• if (ctx->had_interface) {

• hdmi_audio_event.type = 0;
  

• ret = ctx->had_interface->suspend(ctx->had_pvt_data,
  

• 		hdmi_audio_event);
  

• return (ret == 0) ? true : false;
  

• }
• return true;

+}

+void mid_hdmi_audio_resume(void *ddev) +{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• if (hlpe_state == hdmi_connector_status_disconnected) {

• /* HDMI is not connected, there is no need
  

•  * to resume audio device.
  

•  */
  

• return;
  

• }
• dev_dbg(&hlpe_pdev->dev, "%s: hlpe_state %d", __func__, hlpe_state);
• if (ctx->had_interface)

• ctx->had_interface->resume(ctx->had_pvt_data);
  

+}

+void mid_hdmi_audio_signal_event(enum had_event_type event) +{

• struct hdmi_lpe_audio_ctx *ctx;
• dev_dbg(&hlpe_pdev->dev, "%s: Enter\n", __func__);
• ctx = platform_get_drvdata(hlpe_pdev);
• if (ctx->had_event_callbacks)

• (*ctx->had_event_callbacks)(event,
  

• 	ctx->had_pvt_data);
  

Isn't this racy? This dispatcher seems called from multiple places including the interrupt handler below.

+}

+/**

• • hdmi_audio_write:
• • used to write into display controller HDMI audio registers.

No need to use kernel-doc style comments here unless you really want to list in the kernel API documentation.

• */

+static int hdmi_audio_write(uint32_t reg, uint32_t val)

We don't use uint32_t in kernel codes, but a simpler form, u32, is preferred in most cases. uint32_t is still allowed, but it's usually only for user API definitions.

+{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• dev_dbg(&hlpe_pdev->dev, "%s: reg[0x%x] = 0x%x\n", __func__, reg, val);
• iowrite32(val, (ctx->mmio_start+reg));
• return 0;

+}

+/**

• • hdmi_audio_read:
• • used to get the register value read from
• • display controller HDMI audio registers.
• */

+static int hdmi_audio_read(uint32_t reg, uint32_t *val) +{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• *val = ioread32(ctx->mmio_start+reg);
• dev_dbg(&hlpe_pdev->dev, "%s: reg[0x%x] = 0x%x\n", __func__, reg, *val);
• return 0;

+}

+/**

• • hdmi_audio_rmw:
• • used to update the masked bits in display controller HDMI
• • audio registers.
• */

+static int hdmi_audio_rmw(uint32_t reg, uint32_t val, uint32_t mask) +{

• struct hdmi_lpe_audio_ctx *ctx;
• uint32_t val_tmp = 0;
• ctx = platform_get_drvdata(hlpe_pdev);
• val_tmp = (val & mask) |

• 	((ioread32(ctx->mmio_start + reg)) & ~mask);
  

• iowrite32(val_tmp, (ctx->mmio_start+reg));
• dev_dbg(&hlpe_pdev->dev, "%s: reg[0x%x] = 0x%x\n", __func__, reg, val_tmp);
• return 0;

+}

+/**

• • hdmi_audio_get_caps:
• • used to return the HDMI audio capabilities.
• • e.g. resolution, frame rate.
• */

+static int hdmi_audio_get_caps(enum had_caps_list get_element,

• 	void *capabilities)
  

+{

• struct hdmi_lpe_audio_ctx *ctx;
• int ret = 0;
• ctx = get_hdmi_context();
• dev_dbg(&hlpe_pdev->dev, "%s: Enter\n", __func__);
• switch (get_element) {
• case HAD_GET_ELD:

• ret = hdmi_get_eld(capabilities);
  

• break;
  

• case HAD_GET_DISPLAY_RATE:

• /* ToDo: Verify if sampling freq logic is correct */
  

• memcpy(capabilities, &(ctx->tmds_clock_speed),
  

• 	sizeof(uint32_t));
  

Why memcpy? Both source and destination are 32bit int, no?

• dev_dbg(&hlpe_pdev->dev, "%s: tmds_clock_speed = 0x%x\n", __func__,
  

• 		ctx->tmds_clock_speed);
  

• break;
  

• default:

• break;
  

• }
• return ret;

+}

+/**

• • hdmi_audio_get_register_base
• • used to get the current hdmi base address
• */

+int hdmi_audio_get_register_base(uint32_t **reg_base,

• uint32_t *config_offset)
  

+{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• *reg_base = (uint32_t *)(ctx->mmio_start);
• *config_offset = ctx->had_config_offset;
• dev_dbg(&hlpe_pdev->dev, "%s: reg_base = 0x%p, cfg_off = 0x%x\n", __func__,

• 	*reg_base, *config_offset);
  

• return 0;

Well, I see no reason why this function / callback is needed. The base address is never referred in other codes, and the config_offset is always passed to read/write accessors, so it can be calculated there directly.

Any other missing cases?

+}

+/**

• • hdmi_audio_set_caps:
• • used to set the HDMI audio capabilities.
• • e.g. Audio INT.
• */

+int hdmi_audio_set_caps(enum had_caps_list set_element,

• 	void *capabilties)
  

+{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• dev_dbg(&hlpe_pdev->dev, "%s: cap_id = 0x%x\n", __func__, set_element);
• switch (set_element) {
• case HAD_SET_ENABLE_AUDIO_INT:

• {
  

• 	uint32_t status_reg;
  

• 	hdmi_audio_read(AUD_HDMI_STATUS_v2 +
  

• 		ctx->had_config_offset, &status_reg);
  

• 	status_reg |=
  

• 		HDMI_AUDIO_BUFFER_DONE | HDMI_AUDIO_UNDERRUN;
  

• 	hdmi_audio_write(AUD_HDMI_STATUS_v2 +
  

• 		ctx->had_config_offset, status_reg);
  

• 	hdmi_audio_read(AUD_HDMI_STATUS_v2 +
  

• 		ctx->had_config_offset, &status_reg);
  

• }
  

• break;
  

• default:

• break;
  

• }
• return 0;

+}

+static struct hdmi_audio_registers_ops hdmi_audio_reg_ops = {

• .hdmi_audio_get_register_base = hdmi_audio_get_register_base,
• .hdmi_audio_read_register = hdmi_audio_read,
• .hdmi_audio_write_register = hdmi_audio_write,
• .hdmi_audio_read_modify = hdmi_audio_rmw,

+};

+static struct hdmi_audio_query_set_ops hdmi_audio_get_set_ops = {

• .hdmi_audio_get_caps = hdmi_audio_get_caps,
• .hdmi_audio_set_caps = hdmi_audio_set_caps,

+};

+int mid_hdmi_audio_setup(

• had_event_call_back audio_callbacks,
  

• struct hdmi_audio_registers_ops *reg_ops,
  

• struct hdmi_audio_query_set_ops *query_ops)
  

+{

• struct hdmi_lpe_audio_ctx *ctx;
• ctx = platform_get_drvdata(hlpe_pdev);
• dev_dbg(&hlpe_pdev->dev, "%s: called\n", __func__);
• reg_ops->hdmi_audio_get_register_base =

• (hdmi_audio_reg_ops.hdmi_audio_get_register_base);
  

• reg_ops->hdmi_audio_read_register =

• (hdmi_audio_reg_ops.hdmi_audio_read_register);
  

• reg_ops->hdmi_audio_write_register =

• (hdmi_audio_reg_ops.hdmi_audio_write_register);
  

• reg_ops->hdmi_audio_read_modify =

• (hdmi_audio_reg_ops.hdmi_audio_read_modify);
  

• query_ops->hdmi_audio_get_caps =

• hdmi_audio_get_set_ops.hdmi_audio_get_caps;
  

• query_ops->hdmi_audio_set_caps =

• hdmi_audio_get_set_ops.hdmi_audio_set_caps;
  

• ctx->had_event_callbacks = audio_callbacks;
• return 0;

+}

+void _had_wq(struct work_struct *work)

Missing static.

thanks,

Takashi