-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 18, 2017 5:16 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; ville.syrjala@linux.intel.com; broonie@kernel.org; pierre- louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [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:
Thank you for the detailed review.
--- 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.
Ok will add it
- ---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.
Ok will remove it
- 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?
Not needed now I believe , will remove it
+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.
OK, will move it up
+#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.
These are internal to the file. I'll make them static.
+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.
Ok, will remove inline from the code
+{
- 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.
Ok, I'll change the description to accommodate disconnected state too
- */
+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.
No, It's taken care of in the respective callbacks based on the event
+}
+/**
- 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.
Ok, will remove the comments
- */
+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.
Ok, will change to u32, u16 and u8 respectively
+{
- 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?
Do you think *(int *)capabilities = ctx->tmds_clock_speed is better than memcpy?
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?
I wanted to have a cleaner separation, hence added this function in this file rather Than deriving it. So would prefer to keep it.
+}
+/**
- 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.
Ok, will add
thanks,
Takashi