Patchwork [3/7] ALSA: add shell for Intel HDMI LPE audio driver

login
register
mail settings
Submitter Jerome Anand
Date Dec. 12, 2016, 6:10 p.m.
Message ID <20161212181043.12512-4-jerome.anand@intel.com>
Download mbox | patch
Permalink /patch/9470077/
State New
Headers show

Comments

Jerome Anand - Dec. 12, 2016, 6:10 p.m.
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

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Jerome Anand <jerome.anand@intel.com>
---
 sound/Kconfig                    |   2 +
 sound/Makefile                   |   2 +-
 sound/x86/Kconfig                |  16 +
 sound/x86/Makefile               |   8 +
 sound/x86/intel_hdmi_lpe_audio.c | 622 +++++++++++++++++++++++++++++++++++
 sound/x86/intel_hdmi_lpe_audio.h | 692 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 1341 insertions(+), 1 deletion(-)
 create mode 100644 sound/x86/Kconfig
 create mode 100644 sound/x86/Makefile
 create mode 100644 sound/x86/intel_hdmi_lpe_audio.c
 create mode 100644 sound/x86/intel_hdmi_lpe_audio.h
Takashi Iwai - Dec. 14, 2016, 12:45 p.m.
On Mon, 12 Dec 2016 19:10: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
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.anand@intel.com>

Why do we need a "shell" and indirect calls?
This is a small driver set, so it's not utterly unacceptable, but it
still makes things a bit more complicated than necessary, so I'd like
to understand the idea behind it.


> ---
>  sound/Kconfig                    |   2 +
>  sound/Makefile                   |   2 +-
>  sound/x86/Kconfig                |  16 +
>  sound/x86/Makefile               |   8 +
>  sound/x86/intel_hdmi_lpe_audio.c | 622 +++++++++++++++++++++++++++++++++++
>  sound/x86/intel_hdmi_lpe_audio.h | 692 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 1341 insertions(+), 1 deletion(-)
>  create mode 100644 sound/x86/Kconfig
>  create mode 100644 sound/x86/Makefile
>  create mode 100644 sound/x86/intel_hdmi_lpe_audio.c
>  create mode 100644 sound/x86/intel_hdmi_lpe_audio.h
> 
> diff --git a/sound/Kconfig b/sound/Kconfig
> index 5a240e0..ee2e69a 100644
> --- a/sound/Kconfig
> +++ b/sound/Kconfig
> @@ -108,6 +108,8 @@ source "sound/parisc/Kconfig"
>  
>  source "sound/soc/Kconfig"
>  
> +source "sound/x86/Kconfig"
> +
>  endif # SND
>  
>  menuconfig SOUND_PRIME
> diff --git a/sound/Makefile b/sound/Makefile
> index c41bdf5..6de45d2 100644
> --- 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..182adf3
> --- /dev/null
> +++ b/sound/x86/Kconfig
> @@ -0,0 +1,16 @@
> +menuconfig SND_X86
> +	tristate "X86 sound devices"
> +	---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
> +        help
> +          Choose this option to support HDMI LPE Audio mode

Please fix the indentation.  Also a bit more description would be more
user-friendly.


> +
> +endif	# SND_X86
> diff --git a/sound/x86/Makefile b/sound/x86/Makefile
> new file mode 100644
> index 0000000..78b2ae1
> --- /dev/null
> +++ b/sound/x86/Makefile
> @@ -0,0 +1,8 @@
> +DRIVER_NAME := hdmi_lpe_audio
> +
> +ccflags-y += -Idrivers/gpu/drm/i915

Is it just for intel_lpe_audio.h?  Then rather put intel_lpe_audio.h
to include/drm.


> +$(DRIVER_NAME)-objs += \
> +	intel_hdmi_lpe_audio.o
> +
> +obj-$(CONFIG_HDMI_LPE_AUDIO) += $(DRIVER_NAME).o

Don't use substitution, it's rather confusing.
Also, we give "snd-" prefix for the sound driver in general.
If no big reason against it, keep this rule please.


> --- /dev/null
> +++ b/sound/x86/intel_hdmi_lpe_audio.c
(snip)
> @@ -0,0 +1,622 @@
> +/*
> + *  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.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#define pr_fmt(fmt)	"hdmi_lpe_audio: " fmt

Better to use dev_*() variant.


> +#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>
> +#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 *gpdev;
> +int _hdmi_state;
> +union otm_hdmi_eld_t hdmi_eld;

For the global variables, give some prefix, as we have no proper
namespace in C.

And, what's the reason to keep copies of the state here?  Can't we
peek the pdata at each time?


> +
> +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)
> +{
> +	int size = (sizeof(hdmi_eld)) > HDMI_MAX_ELD_BYTES ?
> +				HDMI_MAX_ELD_BYTES :
> +				(sizeof(hdmi_eld));

How can the size differ...?  If any, it should be checked by
BUILD_BUG_ON() or such.

> +
> +	memcpy((void *)&hdmi_eld, eld, size);

Also, when copying from the graphics side, isn't it safer to pass the
given ELD size as well?  Then we can zero-clear the rest bytes if it's
short.

> +}
> +
> +static inline int hdmi_get_eld(void *eld)
> +{
> +	memcpy(eld, (void *)&hdmi_eld, sizeof(hdmi_eld));
> +
> +	{
> +		int i;
> +		uint8_t *eld_data = (uint8_t *)&hdmi_eld;
> +
> +		pr_debug("hdmi_get_eld:\n{{");
> +
> +		for (i = 0; i < sizeof(hdmi_eld); i++)
> +			pr_debug("0x%x, ", eld_data[i]);
> +
> +		pr_debug("}}\n");
> +	}

There is a hexdump debug print helper, too.


> +	return HAD_SUCCESS;

Let's use the normal return code, i.e. 0 or a negative error.


> +}
> +
> +
> +static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void)
> +{
> +	struct hdmi_lpe_audio_ctx *ctx;
> +
> +	ctx = platform_get_drvdata(gpdev);
> +	return ctx;
> +}

Hrm...  Why not storing the audio pdev in i915 pdata?
Then the notify callback can extract it easily.

(snip)
> +static int hdmi_lpe_audio_probe(struct platform_device *pdev)
> +{
> +	struct hdmi_lpe_audio_ctx *ctx;
> +	struct intel_hdmi_lpe_audio_pdata *pdata;
> +	int irq;
> +	struct resource *res_mmio;
> +	void __iomem *mmio_start;
> +	int ret = 0;
> +	unsigned long flag_irq;
> +	const struct pci_device_id cherryview_ids[] = {

Missing static.

> +		{PCI_DEVICE(0x8086, 0x22b0)},
> +		{PCI_DEVICE(0x8086, 0x22b1)},
> +		{PCI_DEVICE(0x8086, 0x22b2)},
> +		{PCI_DEVICE(0x8086, 0x22b3)},
> +		{}
> +	};
> +
> +	pr_debug("Enter %s\n", __func__);
> +
> +	/*TBD:remove globals*/
> +	gpdev = pdev;
> +	_hdmi_state = hdmi_connector_status_disconnected;
> +
> +	/* get resources */
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		pr_err("Could not get irq resource\n");
> +		return -ENODEV;
> +	}
> +
> +	res_mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res_mmio) {
> +		pr_err("Could not get IO_MEM resources\n");
> +		return -ENXIO;
> +	}
> +
> +	pr_debug("%s: mmio_start = 0x%x, mmio_end = 0x%x\n", __func__,
> +		(unsigned int)res_mmio->start, (unsigned int)res_mmio->end);
> +
> +	mmio_start = ioremap_nocache(res_mmio->start,
> +				(size_t)((res_mmio->end - res_mmio->start)
> +						+ 1));
> +	if (!mmio_start) {
> +		pr_err("Could not get ioremap\n");
> +		return -EACCES;
> +	}
> +
> +	/* setup interrupt handler */
> +	ret = request_irq(irq, display_pipe_interrupt_handler,
> +			0, /* FIXME: is IRQF_SHARED needed ? */

It's a irqchip, so no sharing is considered, right?
Then IRQF_SHARED is rather wrong.

> --- /dev/null
> +++ b/sound/x86/intel_hdmi_lpe_audio.h
(snip)
> +#define HMDI_LPE_AUDIO_DRIVER_NAME		"intel-hdmi-lpe-audio"
> +#define HAD_DRIVER_VERSION	"0.01.003"

The driver version string doesn't make sense in most cases for
in-kernel codes.

> +#define HAD_MAX_DEVICES		1
> +#define HAD_MIN_CHANNEL		2
> +#define HAD_MAX_CHANNEL		8
> +#define HAD_NUM_OF_RING_BUFS	4
> +
> +/* HDMI Audio LPE Error Codes */
> +#define HAD_SUCCESS		0
> +#define HAD_FAIL		1

Avoid the own definition.


> +/* Assume 192KHz, 8channel, 25msec period */
> +#define HAD_MAX_BUFFER		(600*1024)
> +#define HAD_MIN_BUFFER		(32*1024)
> +#define HAD_MAX_PERIODS		4
> +#define HAD_MIN_PERIODS		4
> +#define HAD_MAX_PERIOD_BYTES	(HAD_MAX_BUFFER/HAD_MIN_PERIODS)
> +#define HAD_MIN_PERIOD_BYTES	256
> +#define HAD_FIFO_SIZE		0 /* fifo not being used */
> +#define MAX_SPEAKERS		8
> +/* TODO: Add own tlv when channel map is ported for user space */
> +#define USE_ALSA_DEFAULT_TLV
> +
> +#define AUD_SAMPLE_RATE_32	32000
> +#define AUD_SAMPLE_RATE_44_1	44100
> +#define AUD_SAMPLE_RATE_48	48000
> +#define AUD_SAMPLE_RATE_88_2	88200
> +#define AUD_SAMPLE_RATE_96	96000
> +#define AUD_SAMPLE_RATE_176_4	176400
> +#define AUD_SAMPLE_RATE_192	192000
> +
> +#define HAD_MIN_RATE		AUD_SAMPLE_RATE_32
> +#define HAD_MAX_RATE		AUD_SAMPLE_RATE_192
> +
> +#define DIS_SAMPLE_RATE_25_2	25200
> +#define DIS_SAMPLE_RATE_27	27000
> +#define DIS_SAMPLE_RATE_54	54000
> +#define DIS_SAMPLE_RATE_74_25	74250
> +#define DIS_SAMPLE_RATE_148_5	148500
> +#define HAD_REG_WIDTH		0x08
> +#define HAD_MAX_HW_BUFS		0x04
> +#define HAD_MAX_DIP_WORDS		16
> +#define INTEL_HAD		"IntelHdmiLpeAudio"
> +
> +/* _AUD_CONFIG register MASK */
> +#define AUD_CONFIG_MASK_UNDERRUN	0xC0000000
> +#define AUD_CONFIG_MASK_SRDBG		0x00000002
> +#define AUD_CONFIG_MASK_FUNCRST		0x00000001
> +
> +#define MAX_CNT			0xFF
> +#define HAD_SUSPEND_DELAY	1000
> +
> +#define OTM_HDMI_ELD_SIZE 128
> +
> +union otm_hdmi_eld_t {
> +	uint8_t eld_data[OTM_HDMI_ELD_SIZE];
> +	#pragma pack(1)

Use __packed notion for the packed structs.


> +	struct {
> +		/* Byte[0] = ELD Version Number */
> +		union {
> +			uint8_t   byte0;
> +			struct {
> +				uint8_t reserved:3; /* Reserf */
> +				uint8_t eld_ver:5; /* ELD Version Number */
> +				/* 00000b - reserved
> +				 * 00001b - first rev, obsoleted
> +				 * 00010b - version 2, supporting CEA version
> +				 *			861D or below
> +				 * 00011b:11111b - reserved
> +				 * for future
> +				 */
> +			};
> +		};
> +
> +		/* Byte[1] = Vendor Version Field */
> +		union {
> +			uint8_t vendor_version;
> +			struct {
> +				uint8_t reserved1:3;
> +				uint8_t veld_ver:5; /* Version number of the ELD
> +						     * extension. This value is
> +						     * provisioned and unique to
> +						     * each vendor.
> +						     */
> +			};
> +		};
> +
> +		/* Byte[2] = Baseline Length field */
> +		uint8_t baseline_eld_length; /* Length of the Baseline structure
> +					      *	divided by Four.
> +					      */
> +
> +		/* Byte [3] = Reserved for future use */
> +		uint8_t byte3;
> +
> +		/* Starting of the BaseLine EELD structure
> +		 * Byte[4] = Monitor Name Length
> +		 */
> +		union {
> +			uint8_t byte4;
> +			struct {
> +				uint8_t mnl:5;
> +				uint8_t cea_edid_rev_id:3;
> +			};
> +		};
> +
> +		/* Byte[5] = Capabilities */
> +		union {
> +			uint8_t capabilities;
> +			struct {
> +				uint8_t hdcp:1; /* HDCP support */
> +				uint8_t ai_support:1;   /* AI support */
> +				uint8_t connection_type:2; /* Connection type
> +							    * 00 - HDMI
> +							    * 01 - DP
> +							    * 10 -11  Reserved
> +							    * for future
> +							    * connection types
> +							    */
> +				uint8_t sadc:4; /* Indicates number of 3 bytes
> +						 * Short Audio Descriptors.
> +						 */
> +			};
> +		};
> +
> +		/* Byte[6] = Audio Synch Delay */
> +		uint8_t audio_synch_delay; /* Amount of time reported by the
> +					    * sink that the video trails audio
> +					    * in milliseconds.
> +					    */
> +
> +		/* Byte[7] = Speaker Allocation Block */
> +		union {
> +			uint8_t speaker_allocation_block;
> +			struct {
> +				uint8_t flr:1; /*Front Left and Right channels*/
> +				uint8_t lfe:1; /*Low Frequency Effect channel*/
> +				uint8_t fc:1;  /*Center transmission channel*/
> +				uint8_t rlr:1; /*Rear Left and Right channels*/
> +				uint8_t rc:1; /*Rear Center channel*/
> +				uint8_t flrc:1; /*Front left and Right of Center
> +						 *transmission channels
> +						 */
> +				uint8_t rlrc:1; /*Rear left and Right of Center
> +						 *transmission channels
> +						 */
> +				uint8_t reserved3:1; /* Reserved */
> +			};
> +		};
> +
> +		/* Byte[8 - 15] - 8 Byte port identification value */
> +		uint8_t port_id_value[8];
> +
> +		/* Byte[16 - 17] - 2 Byte Manufacturer ID */
> +		uint8_t manufacturer_id[2];
> +
> +		/* Byte[18 - 19] - 2 Byte Product ID */
> +		uint8_t product_id[2];
> +
> +		/* Byte [20-83] - 64 Bytes of BaseLine Data */
> +		uint8_t mn_sand_sads[64]; /* This will include
> +					   * - ASCII string of Monitor name
> +					   * - List of 3 byte SADs
> +					   * - Zero padding
> +					   */
> +
> +		/* Vendor ELD Block should continue here!
> +		 * No Vendor ELD block defined as of now.
> +		 */
> +	};
> +	#pragma pack()
> +};

Well, we should have a single definition of ELD somewhere.
We can keep this locally until we have some unified header, though.

In anyway, better to avoid uint8_t.


thanks,

Takashi
Jerome Anand - Dec. 15, 2016, 10:55 a.m.
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, December 14, 2016 6: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 3/7] ALSA: add shell for Intel HDMI LPE audio driver
> 
> On Mon, 12 Dec 2016 19:10: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
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> 
> Why do we need a "shell" and indirect calls?
> This is a small driver set, so it's not utterly unacceptable, but it still makes
> things a bit more complicated than necessary, so I'd like to understand the
> idea behind it.
> 

This solution does not use component interface to talk between
Audio and i915 driver. The reason for that is the HDMI audio device is created 
by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio
driver does not get loaded if the i915 does not create child device.
Since there is only one callback needed we are not using the component 
interface to make things simpler.
This design was co-worked with i915 folks to makes interaction simpler.


> 
> > ---
> >  sound/Kconfig                    |   2 +
> >  sound/Makefile                   |   2 +-
> >  sound/x86/Kconfig                |  16 +
> >  sound/x86/Makefile               |   8 +
> >  sound/x86/intel_hdmi_lpe_audio.c | 622
> > +++++++++++++++++++++++++++++++++++
> >  sound/x86/intel_hdmi_lpe_audio.h | 692
> > +++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 1341 insertions(+), 1 deletion(-)  create mode
> > 100644 sound/x86/Kconfig  create mode 100644 sound/x86/Makefile
> > create mode 100644 sound/x86/intel_hdmi_lpe_audio.c  create mode
> > 100644 sound/x86/intel_hdmi_lpe_audio.h
> >
> > diff --git a/sound/Kconfig b/sound/Kconfig index 5a240e0..ee2e69a
> > 100644
> > --- a/sound/Kconfig
> > +++ b/sound/Kconfig
> > @@ -108,6 +108,8 @@ source "sound/parisc/Kconfig"
> >
> >  source "sound/soc/Kconfig"
> >
> > +source "sound/x86/Kconfig"
> > +
> >  endif # SND
> >
> >  menuconfig SOUND_PRIME
> > diff --git a/sound/Makefile b/sound/Makefile index c41bdf5..6de45d2
> > 100644
> > --- 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..182adf3
> > --- /dev/null
> > +++ b/sound/x86/Kconfig
> > @@ -0,0 +1,16 @@
> > +menuconfig SND_X86
> > +	tristate "X86 sound devices"
> > +	---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
> > +        help
> > +          Choose this option to support HDMI LPE Audio mode
> 
> Please fix the indentation.  Also a bit more description would be more user-
> friendly.
> 

OK

> 
> > +
> > +endif	# SND_X86
> > diff --git a/sound/x86/Makefile b/sound/x86/Makefile new file mode
> > 100644 index 0000000..78b2ae1
> > --- /dev/null
> > +++ b/sound/x86/Makefile
> > @@ -0,0 +1,8 @@
> > +DRIVER_NAME := hdmi_lpe_audio
> > +
> > +ccflags-y += -Idrivers/gpu/drm/i915
> 
> Is it just for intel_lpe_audio.h?  Then rather put intel_lpe_audio.h to
> include/drm.
> 

OK

> 
> > +$(DRIVER_NAME)-objs += \
> > +	intel_hdmi_lpe_audio.o
> > +
> > +obj-$(CONFIG_HDMI_LPE_AUDIO) += $(DRIVER_NAME).o
> 
> Don't use substitution, it's rather confusing.
> Also, we give "snd-" prefix for the sound driver in general.
> If no big reason against it, keep this rule please.
> 
> 

OK

> > --- /dev/null
> > +++ b/sound/x86/intel_hdmi_lpe_audio.c
> (snip)
> > @@ -0,0 +1,622 @@
> > +/*
> > + *  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.
> > + *
> > + *
> >
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~
> > +~~~~~
> > + */
> > +
> > +#define pr_fmt(fmt)	"hdmi_lpe_audio: " fmt
> 
> Better to use dev_*() variant.
> 

OK

> 
> > +#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>
> > +#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 *gpdev;
> > +int _hdmi_state;
> > +union otm_hdmi_eld_t hdmi_eld;
> 
> For the global variables, give some prefix, as we have no proper namespace
> in C.
> 

OK

> And, what's the reason to keep copies of the state here?  Can't we peek the
> pdata at each time?
> 
> 

OK

> > +
> > +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) {
> > +	int size = (sizeof(hdmi_eld)) > HDMI_MAX_ELD_BYTES ?
> > +				HDMI_MAX_ELD_BYTES :
> > +				(sizeof(hdmi_eld));
> 
> How can the size differ...?  If any, it should be checked by
> BUILD_BUG_ON() or such.
> 

OK

> > +
> > +	memcpy((void *)&hdmi_eld, eld, size);
> 
> Also, when copying from the graphics side, isn't it safer to pass the given ELD
> size as well?  Then we can zero-clear the rest bytes if it's short.
> 

OK

> > +}
> > +
> > +static inline int hdmi_get_eld(void *eld) {
> > +	memcpy(eld, (void *)&hdmi_eld, sizeof(hdmi_eld));
> > +
> > +	{
> > +		int i;
> > +		uint8_t *eld_data = (uint8_t *)&hdmi_eld;
> > +
> > +		pr_debug("hdmi_get_eld:\n{{");
> > +
> > +		for (i = 0; i < sizeof(hdmi_eld); i++)
> > +			pr_debug("0x%x, ", eld_data[i]);
> > +
> > +		pr_debug("}}\n");
> > +	}
> 
> There is a hexdump debug print helper, too.
> 

OK

> 
> > +	return HAD_SUCCESS;
> 
> Let's use the normal return code, i.e. 0 or a negative error.
> 

OK

> 
> > +}
> > +
> > +
> > +static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void) {
> > +	struct hdmi_lpe_audio_ctx *ctx;
> > +
> > +	ctx = platform_get_drvdata(gpdev);
> > +	return ctx;
> > +}
> 
> Hrm...  Why not storing the audio pdev in i915 pdata?
> Then the notify callback can extract it easily.
> 

The current audio driver interface implementation treats the input pdata as
Read-only and I don't want to store any audio info in that.

> (snip)
> > +static int hdmi_lpe_audio_probe(struct platform_device *pdev) {
> > +	struct hdmi_lpe_audio_ctx *ctx;
> > +	struct intel_hdmi_lpe_audio_pdata *pdata;
> > +	int irq;
> > +	struct resource *res_mmio;
> > +	void __iomem *mmio_start;
> > +	int ret = 0;
> > +	unsigned long flag_irq;
> > +	const struct pci_device_id cherryview_ids[] = {
> 
> Missing static.
> 

OK

> > +		{PCI_DEVICE(0x8086, 0x22b0)},
> > +		{PCI_DEVICE(0x8086, 0x22b1)},
> > +		{PCI_DEVICE(0x8086, 0x22b2)},
> > +		{PCI_DEVICE(0x8086, 0x22b3)},
> > +		{}
> > +	};
> > +
> > +	pr_debug("Enter %s\n", __func__);
> > +
> > +	/*TBD:remove globals*/
> > +	gpdev = pdev;
> > +	_hdmi_state = hdmi_connector_status_disconnected;
> > +
> > +	/* get resources */
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		pr_err("Could not get irq resource\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	res_mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res_mmio) {
> > +		pr_err("Could not get IO_MEM resources\n");
> > +		return -ENXIO;
> > +	}
> > +
> > +	pr_debug("%s: mmio_start = 0x%x, mmio_end = 0x%x\n", __func__,
> > +		(unsigned int)res_mmio->start, (unsigned int)res_mmio-
> >end);
> > +
> > +	mmio_start = ioremap_nocache(res_mmio->start,
> > +				(size_t)((res_mmio->end - res_mmio->start)
> > +						+ 1));
> > +	if (!mmio_start) {
> > +		pr_err("Could not get ioremap\n");
> > +		return -EACCES;
> > +	}
> > +
> > +	/* setup interrupt handler */
> > +	ret = request_irq(irq, display_pipe_interrupt_handler,
> > +			0, /* FIXME: is IRQF_SHARED needed ? */
> 
> It's a irqchip, so no sharing is considered, right?
> Then IRQF_SHARED is rather wrong.
> 

OK

> > --- /dev/null
> > +++ b/sound/x86/intel_hdmi_lpe_audio.h
> (snip)
> > +#define HMDI_LPE_AUDIO_DRIVER_NAME		"intel-hdmi-lpe-
> audio"
> > +#define HAD_DRIVER_VERSION	"0.01.003"
> 
> The driver version string doesn't make sense in most cases for in-kernel
> codes.
> 

OK - will be removed

> > +#define HAD_MAX_DEVICES		1
> > +#define HAD_MIN_CHANNEL		2
> > +#define HAD_MAX_CHANNEL		8
> > +#define HAD_NUM_OF_RING_BUFS	4
> > +
> > +/* HDMI Audio LPE Error Codes */
> > +#define HAD_SUCCESS		0
> > +#define HAD_FAIL		1
> 
> Avoid the own definition.
> 
> 


OK

> > +/* Assume 192KHz, 8channel, 25msec period */
> > +#define HAD_MAX_BUFFER		(600*1024)
> > +#define HAD_MIN_BUFFER		(32*1024)
> > +#define HAD_MAX_PERIODS		4
> > +#define HAD_MIN_PERIODS		4
> > +#define HAD_MAX_PERIOD_BYTES
> 	(HAD_MAX_BUFFER/HAD_MIN_PERIODS)
> > +#define HAD_MIN_PERIOD_BYTES	256
> > +#define HAD_FIFO_SIZE		0 /* fifo not being used */
> > +#define MAX_SPEAKERS		8
> > +/* TODO: Add own tlv when channel map is ported for user space */
> > +#define USE_ALSA_DEFAULT_TLV
> > +
> > +#define AUD_SAMPLE_RATE_32	32000
> > +#define AUD_SAMPLE_RATE_44_1	44100
> > +#define AUD_SAMPLE_RATE_48	48000
> > +#define AUD_SAMPLE_RATE_88_2	88200
> > +#define AUD_SAMPLE_RATE_96	96000
> > +#define AUD_SAMPLE_RATE_176_4	176400
> > +#define AUD_SAMPLE_RATE_192	192000
> > +
> > +#define HAD_MIN_RATE		AUD_SAMPLE_RATE_32
> > +#define HAD_MAX_RATE		AUD_SAMPLE_RATE_192
> > +
> > +#define DIS_SAMPLE_RATE_25_2	25200
> > +#define DIS_SAMPLE_RATE_27	27000
> > +#define DIS_SAMPLE_RATE_54	54000
> > +#define DIS_SAMPLE_RATE_74_25	74250
> > +#define DIS_SAMPLE_RATE_148_5	148500
> > +#define HAD_REG_WIDTH		0x08
> > +#define HAD_MAX_HW_BUFS		0x04
> > +#define HAD_MAX_DIP_WORDS		16
> > +#define INTEL_HAD		"IntelHdmiLpeAudio"
> > +
> > +/* _AUD_CONFIG register MASK */
> > +#define AUD_CONFIG_MASK_UNDERRUN	0xC0000000
> > +#define AUD_CONFIG_MASK_SRDBG		0x00000002
> > +#define AUD_CONFIG_MASK_FUNCRST		0x00000001
> > +
> > +#define MAX_CNT			0xFF
> > +#define HAD_SUSPEND_DELAY	1000
> > +
> > +#define OTM_HDMI_ELD_SIZE 128
> > +
> > +union otm_hdmi_eld_t {
> > +	uint8_t eld_data[OTM_HDMI_ELD_SIZE];
> > +	#pragma pack(1)
> 
> Use __packed notion for the packed structs.
> 

OK

> 
> > +	struct {
> > +		/* Byte[0] = ELD Version Number */
> > +		union {
> > +			uint8_t   byte0;
> > +			struct {
> > +				uint8_t reserved:3; /* Reserf */
> > +				uint8_t eld_ver:5; /* ELD Version Number */
> > +				/* 00000b - reserved
> > +				 * 00001b - first rev, obsoleted
> > +				 * 00010b - version 2, supporting CEA version
> > +				 *			861D or below
> > +				 * 00011b:11111b - reserved
> > +				 * for future
> > +				 */
> > +			};
> > +		};
> > +
> > +		/* Byte[1] = Vendor Version Field */
> > +		union {
> > +			uint8_t vendor_version;
> > +			struct {
> > +				uint8_t reserved1:3;
> > +				uint8_t veld_ver:5; /* Version number of the
> ELD
> > +						     * extension. This value is
> > +						     * provisioned and unique
> to
> > +						     * each vendor.
> > +						     */
> > +			};
> > +		};
> > +
> > +		/* Byte[2] = Baseline Length field */
> > +		uint8_t baseline_eld_length; /* Length of the Baseline
> structure
> > +					      *	divided by Four.
> > +					      */
> > +
> > +		/* Byte [3] = Reserved for future use */
> > +		uint8_t byte3;
> > +
> > +		/* Starting of the BaseLine EELD structure
> > +		 * Byte[4] = Monitor Name Length
> > +		 */
> > +		union {
> > +			uint8_t byte4;
> > +			struct {
> > +				uint8_t mnl:5;
> > +				uint8_t cea_edid_rev_id:3;
> > +			};
> > +		};
> > +
> > +		/* Byte[5] = Capabilities */
> > +		union {
> > +			uint8_t capabilities;
> > +			struct {
> > +				uint8_t hdcp:1; /* HDCP support */
> > +				uint8_t ai_support:1;   /* AI support */
> > +				uint8_t connection_type:2; /* Connection
> type
> > +							    * 00 - HDMI
> > +							    * 01 - DP
> > +							    * 10 -11  Reserved
> > +							    * for future
> > +							    * connection types
> > +							    */
> > +				uint8_t sadc:4; /* Indicates number of 3
> bytes
> > +						 * Short Audio Descriptors.
> > +						 */
> > +			};
> > +		};
> > +
> > +		/* Byte[6] = Audio Synch Delay */
> > +		uint8_t audio_synch_delay; /* Amount of time reported by
> the
> > +					    * sink that the video trails audio
> > +					    * in milliseconds.
> > +					    */
> > +
> > +		/* Byte[7] = Speaker Allocation Block */
> > +		union {
> > +			uint8_t speaker_allocation_block;
> > +			struct {
> > +				uint8_t flr:1; /*Front Left and Right
> channels*/
> > +				uint8_t lfe:1; /*Low Frequency Effect
> channel*/
> > +				uint8_t fc:1;  /*Center transmission
> channel*/
> > +				uint8_t rlr:1; /*Rear Left and Right channels*/
> > +				uint8_t rc:1; /*Rear Center channel*/
> > +				uint8_t flrc:1; /*Front left and Right of Center
> > +						 *transmission channels
> > +						 */
> > +				uint8_t rlrc:1; /*Rear left and Right of Center
> > +						 *transmission channels
> > +						 */
> > +				uint8_t reserved3:1; /* Reserved */
> > +			};
> > +		};
> > +
> > +		/* Byte[8 - 15] - 8 Byte port identification value */
> > +		uint8_t port_id_value[8];
> > +
> > +		/* Byte[16 - 17] - 2 Byte Manufacturer ID */
> > +		uint8_t manufacturer_id[2];
> > +
> > +		/* Byte[18 - 19] - 2 Byte Product ID */
> > +		uint8_t product_id[2];
> > +
> > +		/* Byte [20-83] - 64 Bytes of BaseLine Data */
> > +		uint8_t mn_sand_sads[64]; /* This will include
> > +					   * - ASCII string of Monitor name
> > +					   * - List of 3 byte SADs
> > +					   * - Zero padding
> > +					   */
> > +
> > +		/* Vendor ELD Block should continue here!
> > +		 * No Vendor ELD block defined as of now.
> > +		 */
> > +	};
> > +	#pragma pack()
> > +};
> 
> Well, we should have a single definition of ELD somewhere.
> We can keep this locally until we have some unified header, though.
> 

OK 

> In anyway, better to avoid uint8_t.
> 

OK I'll use unsigned char

> 
> thanks,
> 
> Takashi
Takashi Iwai - Dec. 15, 2016, 11:14 a.m.
On Thu, 15 Dec 2016 11:55:23 +0100,
Anand, Jerome wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, December 14, 2016 6: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 3/7] ALSA: add shell for Intel HDMI LPE audio driver
> > 
> > On Mon, 12 Dec 2016 19:10: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
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > 
> > Why do we need a "shell" and indirect calls?
> > This is a small driver set, so it's not utterly unacceptable, but it still makes
> > things a bit more complicated than necessary, so I'd like to understand the
> > idea behind it.
> > 
> 
> This solution does not use component interface to talk between
> Audio and i915 driver. The reason for that is the HDMI audio device is created 
> by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio
> driver does not get loaded if the i915 does not create child device.
> Since there is only one callback needed we are not using the component 
> interface to make things simpler.
> This design was co-worked with i915 folks to makes interaction simpler.

The interaction between i915 and audio is simple, yes, it just exposes
a few things, mmio ptr, irq, etc.  But still I don't understand why
multiple layers of indirect accesses are needed *inside* lpe audio
driver itself.  For example, suspend/resume action is cascaded to yet
another ops.

I would understand if this "shell" provides a few thin accessors to
the raw i915 registers.  But another layering over a single driver
implementation makes little sense in regard of abstraction.  (If there
were multiple class inherits, it's a different story, of course.)


> > > +static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void) {
> > > +	struct hdmi_lpe_audio_ctx *ctx;
> > > +
> > > +	ctx = platform_get_drvdata(gpdev);
> > > +	return ctx;
> > > +}
> > 
> > Hrm...  Why not storing the audio pdev in i915 pdata?
> > Then the notify callback can extract it easily.
> > 
> 
> The current audio driver interface implementation treats the input pdata as
> Read-only and I don't want to store any audio info in that.

Well, it's not read-only, you already write it to register the
notifier callback :)


thanks,

Takashi
Mark Brown - Dec. 15, 2016, 11:44 a.m.
On Thu, Dec 15, 2016 at 12:14:30PM +0100, Takashi Iwai wrote:

> The interaction between i915 and audio is simple, yes, it just exposes
> a few things, mmio ptr, irq, etc.  But still I don't understand why
> multiple layers of indirect accesses are needed *inside* lpe audio
> driver itself.  For example, suspend/resume action is cascaded to yet
> another ops.

> I would understand if this "shell" provides a few thin accessors to
> the raw i915 registers.  But another layering over a single driver
> implementation makes little sense in regard of abstraction.  (If there
> were multiple class inherits, it's a different story, of course.)

We saw the same thing with the DSP code as well...
Pierre-Louis Bossart - Dec. 15, 2016, 8:37 p.m.
>>> Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver

>>> Why do we need a "shell" and indirect calls?
>>> This is a small driver set, so it's not utterly unacceptable, but it still makes
>>> things a bit more complicated than necessary, so I'd like to understand the
>>> idea behind it.

The 'shell' comes from an early prototype which didn't do much except 
create the device. The title of the commit should be changed if it threw 
you off.

>> This solution does not use component interface to talk between
>> Audio and i915 driver. The reason for that is the HDMI audio device is created
>> by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio
>> driver does not get loaded if the i915 does not create child device.
>> Since there is only one callback needed we are not using the component
>> interface to make things simpler.
>> This design was co-worked with i915 folks to makes interaction simpler.
>
> The interaction between i915 and audio is simple, yes, it just exposes
> a few things, mmio ptr, irq, etc.  But still I don't understand why
> multiple layers of indirect accesses are needed *inside* lpe audio
> driver itself.  For example, suspend/resume action is cascaded to yet
> another ops.
>
> I would understand if this "shell" provides a few thin accessors to
> the raw i915 registers.  But another layering over a single driver
> implementation makes little sense in regard of abstraction.  (If there
> were multiple class inherits, it's a different story, of course.)

No disagreement here Takashi.
I was planning to remove this abstraction in a second incremental pass 
that would only be on the audio side, for now keeping this layer would 
allow me to add the DisplayPort changes faster. If we simplify this now 
then I will have so many differences with the legacy code it'll be a 
nightmare. I have no emotional attachment to the legacy but it's just 
pragmatic to avoid changing everything at once.

As you also mentioned it, this second pass could also reuse all the ELD 
parsing that is still duplicated. These HDMI patches are not a 
single-shot contribution, you'll have updates coming your way.
Takashi Iwai - Dec. 15, 2016, 9:01 p.m.
On Thu, 15 Dec 2016 21:37:03 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> >>> Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver
> 
> >>> Why do we need a "shell" and indirect calls?
> >>> This is a small driver set, so it's not utterly unacceptable, but it still makes
> >>> things a bit more complicated than necessary, so I'd like to understand the
> >>> idea behind it.
> 
> The 'shell' comes from an early prototype which didn't do much except
> create the device. The title of the commit should be changed if it
> threw you off.
> 
> >> This solution does not use component interface to talk between
> >> Audio and i915 driver. The reason for that is the HDMI audio device is created
> >> by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio
> >> driver does not get loaded if the i915 does not create child device.
> >> Since there is only one callback needed we are not using the component
> >> interface to make things simpler.
> >> This design was co-worked with i915 folks to makes interaction simpler.
> >
> > The interaction between i915 and audio is simple, yes, it just exposes
> > a few things, mmio ptr, irq, etc.  But still I don't understand why
> > multiple layers of indirect accesses are needed *inside* lpe audio
> > driver itself.  For example, suspend/resume action is cascaded to yet
> > another ops.
> >
> > I would understand if this "shell" provides a few thin accessors to
> > the raw i915 registers.  But another layering over a single driver
> > implementation makes little sense in regard of abstraction.  (If there
> > were multiple class inherits, it's a different story, of course.)
> 
> No disagreement here Takashi.
> I was planning to remove this abstraction in a second incremental pass
> that would only be on the audio side, for now keeping this layer would
> allow me to add the DisplayPort changes faster. If we simplify this
> now then I will have so many differences with the legacy code it'll be
> a nightmare. I have no emotional attachment to the legacy but it's
> just pragmatic to avoid changing everything at once.
> 
> As you also mentioned it, this second pass could also reuse all the
> ELD parsing that is still duplicated. These HDMI patches are not a
> single-shot contribution, you'll have updates coming your way.

OK, if the current structure helps for DP support, it's no problem to
keep it.  We can refactor the code later easily, too -- it's a single
driver code, after all.


thanks,

Takashi

Patch

diff --git a/sound/Kconfig b/sound/Kconfig
index 5a240e0..ee2e69a 100644
--- a/sound/Kconfig
+++ b/sound/Kconfig
@@ -108,6 +108,8 @@  source "sound/parisc/Kconfig"
 
 source "sound/soc/Kconfig"
 
+source "sound/x86/Kconfig"
+
 endif # SND
 
 menuconfig SOUND_PRIME
diff --git a/sound/Makefile b/sound/Makefile
index c41bdf5..6de45d2 100644
--- 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..182adf3
--- /dev/null
+++ b/sound/x86/Kconfig
@@ -0,0 +1,16 @@ 
+menuconfig SND_X86
+	tristate "X86 sound devices"
+	---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
+        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..78b2ae1
--- /dev/null
+++ b/sound/x86/Makefile
@@ -0,0 +1,8 @@ 
+DRIVER_NAME := hdmi_lpe_audio
+
+ccflags-y += -Idrivers/gpu/drm/i915
+
+$(DRIVER_NAME)-objs += \
+	intel_hdmi_lpe_audio.o
+
+obj-$(CONFIG_HDMI_LPE_AUDIO) += $(DRIVER_NAME).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..f31ab72
--- /dev/null
+++ b/sound/x86/intel_hdmi_lpe_audio.c
@@ -0,0 +1,622 @@ 
+/*
+ *  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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#define pr_fmt(fmt)	"hdmi_lpe_audio: " fmt
+
+#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>
+#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 *gpdev;
+int _hdmi_state;
+union otm_hdmi_eld_t hdmi_eld;
+
+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)
+{
+	int size = (sizeof(hdmi_eld)) > HDMI_MAX_ELD_BYTES ?
+				HDMI_MAX_ELD_BYTES :
+				(sizeof(hdmi_eld));
+
+	memcpy((void *)&hdmi_eld, eld, size);
+}
+
+static inline int hdmi_get_eld(void *eld)
+{
+	memcpy(eld, (void *)&hdmi_eld, sizeof(hdmi_eld));
+
+	{
+		int i;
+		uint8_t *eld_data = (uint8_t *)&hdmi_eld;
+
+		pr_debug("hdmi_get_eld:\n{{");
+
+		for (i = 0; i < sizeof(hdmi_eld); i++)
+			pr_debug("0x%x, ", eld_data[i]);
+
+		pr_debug("}}\n");
+	}
+	return HAD_SUCCESS;
+}
+
+
+static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void)
+{
+	struct hdmi_lpe_audio_ctx *ctx;
+
+	ctx = platform_get_drvdata(gpdev);
+	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;
+
+	pr_debug("%s: Enter",  __func__);
+
+	ctx = platform_get_drvdata(gpdev);
+
+	if (_hdmi_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.
+ */
+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(gpdev);
+
+	if (_hdmi_state == hdmi_connector_status_disconnected) {
+		/* HDMI is not connected, assuming audio device
+		 * is suspended already.
+		 */
+		return true;
+	}
+
+	pr_debug("%s: _hdmi_state %d",  __func__,
+			_hdmi_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(gpdev);
+
+	if (_hdmi_state == hdmi_connector_status_disconnected) {
+		/* HDMI is not connected, there is no need
+		 * to resume audio device.
+		 */
+		return;
+	}
+
+	pr_debug("%s: _hdmi_state %d",  __func__, _hdmi_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;
+
+	pr_debug("%s: Enter\n", __func__);
+
+	ctx = platform_get_drvdata(gpdev);
+
+	if (ctx->had_event_callbacks)
+		(*ctx->had_event_callbacks)(event,
+			ctx->had_pvt_data);
+}
+
+/**
+ * hdmi_audio_write:
+ * used to write into display controller HDMI audio registers.
+ *
+ */
+static int hdmi_audio_write(uint32_t reg, uint32_t val)
+{
+	struct hdmi_lpe_audio_ctx *ctx;
+
+	ctx = platform_get_drvdata(gpdev);
+
+	pr_debug("%s: reg[0x%x] = 0x%x\n", __func__, reg, val);
+
+	iowrite32(val, (ctx->mmio_start+reg));
+
+	return HAD_SUCCESS;
+}
+
+/**
+ * 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(gpdev);
+	*val = ioread32(ctx->mmio_start+reg);
+	pr_debug("%s: reg[0x%x] = 0x%x\n", __func__, reg, *val);
+	return HAD_SUCCESS;
+}
+
+/**
+ * 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(gpdev);
+
+	val_tmp = (val & mask) |
+			((ioread32(ctx->mmio_start + reg)) & ~mask);
+
+	iowrite32(val_tmp, (ctx->mmio_start+reg));
+	pr_debug("%s: reg[0x%x] = 0x%x\n", __func__, reg, val_tmp);
+
+	return HAD_SUCCESS;
+}
+
+/**
+ * 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 = HAD_SUCCESS;
+
+	ctx = get_hdmi_context();
+
+	pr_debug("%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));
+		pr_debug("%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(gpdev);
+	*reg_base = (uint32_t *)(ctx->mmio_start);
+	*config_offset = ctx->had_config_offset;
+	pr_debug("%s: reg_base = 0x%p, cfg_off = 0x%x\n", __func__,
+			*reg_base, *config_offset);
+	return HAD_SUCCESS;
+}
+
+/**
+ * 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(gpdev);
+
+	pr_debug("%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 HAD_SUCCESS;
+}
+
+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(gpdev);
+
+	pr_debug("%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 HAD_SUCCESS;
+}
+
+void _had_wq(struct work_struct *work)
+{
+	mid_hdmi_audio_signal_event(HAD_EVENT_HOT_PLUG);
+}
+
+int mid_hdmi_audio_register(struct snd_intel_had_interface *driver,
+				void *had_data)
+{
+	struct hdmi_lpe_audio_ctx *ctx;
+
+	ctx = platform_get_drvdata(gpdev);
+
+	pr_debug("%s: called\n", __func__);
+
+	ctx->had_pvt_data = had_data;
+	ctx->had_interface = driver;
+
+	/* The Audio driver is loading now and we need to notify
+	 * it if there is an HDMI device attached
+	 */
+	INIT_WORK(&ctx->hdmi_audio_wq, _had_wq);
+	pr_debug("%s: Scheduling HDMI audio work queue\n", __func__);
+	schedule_work(&ctx->hdmi_audio_wq);
+
+	return HAD_SUCCESS;
+}
+
+static irqreturn_t display_pipe_interrupt_handler(int irq, void *dev_id)
+{
+	u32 audio_stat, audio_reg;
+
+	struct hdmi_lpe_audio_ctx *ctx;
+
+	pr_debug("%s: Enter\n", __func__);
+
+	ctx = platform_get_drvdata(gpdev);
+
+	audio_reg = ctx->had_config_offset + AUD_HDMI_STATUS_v2;
+	hdmi_audio_read(audio_reg, &audio_stat);
+
+	if (audio_stat & HDMI_AUDIO_UNDERRUN) {
+		hdmi_audio_write(audio_reg, HDMI_AUDIO_UNDERRUN);
+		mid_hdmi_audio_signal_event(
+				HAD_EVENT_AUDIO_BUFFER_UNDERRUN);
+	}
+
+	if (audio_stat & HDMI_AUDIO_BUFFER_DONE) {
+		hdmi_audio_write(audio_reg, HDMI_AUDIO_BUFFER_DONE);
+		mid_hdmi_audio_signal_event(
+				HAD_EVENT_AUDIO_BUFFER_DONE);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void notify_audio_lpe(void *audio_ptr)
+{
+	struct hdmi_lpe_audio_ctx *ctx = get_hdmi_context();
+	struct intel_hdmi_lpe_audio_pdata *pdata = gpdev->dev.platform_data;
+	struct intel_hdmi_lpe_audio_eld *eld = audio_ptr;
+
+	if (pdata->hdmi_connected != true) {
+
+		pr_debug("%s: Event: HAD_NOTIFY_HOT_UNPLUG\n",
+			__func__);
+
+		if (_hdmi_state == hdmi_connector_status_connected) {
+
+			_hdmi_state =
+				hdmi_connector_status_disconnected;
+
+			mid_hdmi_audio_signal_event(
+				HAD_EVENT_HOT_UNPLUG);
+		} else
+			pr_debug("%s: Already Unplugged!\n", __func__);
+
+	} else if (eld != NULL) {
+
+		hdmi_set_eld(eld->eld_data);
+
+		mid_hdmi_audio_signal_event(HAD_EVENT_HOT_PLUG);
+
+		_hdmi_state = hdmi_connector_status_connected;
+
+		pr_debug("%s: HAD_NOTIFY_ELD : port = %d, tmds = %d\n",
+			__func__, eld->port_id,
+			pdata->tmds_clock_speed);
+
+		if (pdata->tmds_clock_speed) {
+			ctx->tmds_clock_speed = pdata->tmds_clock_speed;
+			mid_hdmi_audio_signal_event(HAD_EVENT_MODE_CHANGING);
+		}
+	} else
+		pr_debug("%s: Event: NULL EDID!!\n", __func__);
+}
+
+/**
+ * hdmi_lpe_audio_probe - start bridge with i915
+ *
+ * This function is called when the i915 driver creates the hdmi-lpe-audio
+ * platform device. Card creation is deferred until a hot plug event is
+ * received
+ */
+static int hdmi_lpe_audio_probe(struct platform_device *pdev)
+{
+	struct hdmi_lpe_audio_ctx *ctx;
+	struct intel_hdmi_lpe_audio_pdata *pdata;
+	int irq;
+	struct resource *res_mmio;
+	void __iomem *mmio_start;
+	int ret = 0;
+	unsigned long flag_irq;
+	const struct pci_device_id cherryview_ids[] = {
+		{PCI_DEVICE(0x8086, 0x22b0)},
+		{PCI_DEVICE(0x8086, 0x22b1)},
+		{PCI_DEVICE(0x8086, 0x22b2)},
+		{PCI_DEVICE(0x8086, 0x22b3)},
+		{}
+	};
+
+	pr_debug("Enter %s\n", __func__);
+
+	/*TBD:remove globals*/
+	gpdev = pdev;
+	_hdmi_state = hdmi_connector_status_disconnected;
+
+	/* get resources */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		pr_err("Could not get irq resource\n");
+		return -ENODEV;
+	}
+
+	res_mmio = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res_mmio) {
+		pr_err("Could not get IO_MEM resources\n");
+		return -ENXIO;
+	}
+
+	pr_debug("%s: mmio_start = 0x%x, mmio_end = 0x%x\n", __func__,
+		(unsigned int)res_mmio->start, (unsigned int)res_mmio->end);
+
+	mmio_start = ioremap_nocache(res_mmio->start,
+				(size_t)((res_mmio->end - res_mmio->start)
+						+ 1));
+	if (!mmio_start) {
+		pr_err("Could not get ioremap\n");
+		return -EACCES;
+	}
+
+	/* setup interrupt handler */
+	ret = request_irq(irq, display_pipe_interrupt_handler,
+			0, /* FIXME: is IRQF_SHARED needed ? */
+			pdev->name,
+			NULL);
+	if (ret < 0) {
+		pr_err("request_irq failed\n");
+		iounmap(mmio_start);
+		return -ENODEV;
+	}
+
+	/* alloc and save context */
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (ctx == NULL) {
+		free_irq(irq, NULL);
+		iounmap(mmio_start);
+		return -ENOMEM;
+	}
+
+	ctx->irq = irq;
+	pr_debug("hdmi lpe audio: irq num = %d\n", irq);
+	ctx->mmio_start = mmio_start;
+	ctx->tmds_clock_speed = DIS_SAMPLE_RATE_148_5;
+
+	if (pci_dev_present(cherryview_ids)) {
+		pr_debug("%s: Cherrytrail LPE - Detected\n", __func__);
+		ctx->had_config_offset = AUDIO_HDMI_CONFIG_C;
+	} else {
+		pr_debug("%s: Baytrail LPE - Assume\n", __func__);
+		ctx->had_config_offset = AUDIO_HDMI_CONFIG_A;
+	}
+
+	pdata = pdev->dev.platform_data;
+
+	if (pdata == NULL) {
+		pr_err("%s: quit: pdata not allocated by i915!!\n", __func__);
+		kfree(ctx);
+		free_irq(irq, NULL);
+		iounmap(mmio_start);
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, ctx);
+
+	pr_debug("hdmi lpe audio: setting pin eld notify callback\n");
+
+	spin_lock_irqsave(&pdata->lpe_audio_slock, flag_irq);
+	pdata->notify_audio_lpe = notify_audio_lpe;
+
+	if (pdata->notify_pending) {
+
+		pr_debug("%s: handle pending notification\n", __func__);
+		notify_audio_lpe(&pdata->eld);
+		pdata->notify_pending = false;
+	}
+	spin_unlock_irqrestore(&pdata->lpe_audio_slock, flag_irq);
+
+	return ret;
+}
+
+/**
+ * hdmi_lpe_audio_remove - stop bridge with i915
+ *
+ * This function is called when the platform device is destroyed. The sound
+ * card should have been removed on hot plug event.
+ */
+static int hdmi_lpe_audio_remove(struct platform_device *pdev)
+{
+	struct hdmi_lpe_audio_ctx *ctx;
+
+	pr_debug("Enter %s\n", __func__);
+
+	/* get context, release resources */
+	ctx = platform_get_drvdata(pdev);
+	iounmap(ctx->mmio_start);
+	free_irq(ctx->irq, NULL);
+	kfree(ctx);
+	return HAD_SUCCESS;
+}
+
+static int hdmi_lpe_audio_suspend(struct platform_device *pt_dev,
+				pm_message_t state)
+{
+	pr_debug("Enter %s\n", __func__);
+	mid_hdmi_audio_suspend(NULL);
+	return HAD_SUCCESS;
+}
+
+static int hdmi_lpe_audio_resume(struct platform_device *pt_dev)
+{
+	pr_debug("Enter %s\n", __func__);
+	mid_hdmi_audio_resume(NULL);
+	return HAD_SUCCESS;
+}
+
+static struct platform_driver hdmi_lpe_audio_driver = {
+	.driver		= {
+		.name  = "hdmi-lpe-audio",
+	},
+	.probe          = hdmi_lpe_audio_probe,
+	.remove		= hdmi_lpe_audio_remove,
+	.suspend	= hdmi_lpe_audio_suspend,
+	.resume		= hdmi_lpe_audio_resume
+};
+
+module_platform_driver(hdmi_lpe_audio_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hdmi_lpe_audio");
diff --git a/sound/x86/intel_hdmi_lpe_audio.h b/sound/x86/intel_hdmi_lpe_audio.h
new file mode 100644
index 0000000..d4b94f0a
--- /dev/null
+++ b/sound/x86/intel_hdmi_lpe_audio.h
@@ -0,0 +1,692 @@ 
+/*
+ *   intel_hdmi_lpe_audio.h - Intel HDMI LPE audio driver
+ *
+ *  Copyright (C) 2016 Intel Corp
+ *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
+ *		Ramesh Babu K V <ramesh.babu@intel.com>
+ *		Vaibhav Agarwal <vaibhav.agarwal@intel.com>
+ *		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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#ifndef __INTEL_HDMI_LPE_AUDIO_H
+#define __INTEL_HDMI_LPE_AUDIO_H
+
+#include <linux/types.h>
+#include <sound/initval.h>
+#include <linux/version.h>
+#include <linux/pm_runtime.h>
+#include <sound/asoundef.h>
+#include <sound/control.h>
+#include <sound/pcm.h>
+
+#define HMDI_LPE_AUDIO_DRIVER_NAME		"intel-hdmi-lpe-audio"
+#define HAD_DRIVER_VERSION	"0.01.003"
+#define HAD_MAX_DEVICES		1
+#define HAD_MIN_CHANNEL		2
+#define HAD_MAX_CHANNEL		8
+#define HAD_NUM_OF_RING_BUFS	4
+
+/* HDMI Audio LPE Error Codes */
+#define HAD_SUCCESS		0
+#define HAD_FAIL		1
+
+/* Assume 192KHz, 8channel, 25msec period */
+#define HAD_MAX_BUFFER		(600*1024)
+#define HAD_MIN_BUFFER		(32*1024)
+#define HAD_MAX_PERIODS		4
+#define HAD_MIN_PERIODS		4
+#define HAD_MAX_PERIOD_BYTES	(HAD_MAX_BUFFER/HAD_MIN_PERIODS)
+#define HAD_MIN_PERIOD_BYTES	256
+#define HAD_FIFO_SIZE		0 /* fifo not being used */
+#define MAX_SPEAKERS		8
+/* TODO: Add own tlv when channel map is ported for user space */
+#define USE_ALSA_DEFAULT_TLV
+
+#define AUD_SAMPLE_RATE_32	32000
+#define AUD_SAMPLE_RATE_44_1	44100
+#define AUD_SAMPLE_RATE_48	48000
+#define AUD_SAMPLE_RATE_88_2	88200
+#define AUD_SAMPLE_RATE_96	96000
+#define AUD_SAMPLE_RATE_176_4	176400
+#define AUD_SAMPLE_RATE_192	192000
+
+#define HAD_MIN_RATE		AUD_SAMPLE_RATE_32
+#define HAD_MAX_RATE		AUD_SAMPLE_RATE_192
+
+#define DIS_SAMPLE_RATE_25_2	25200
+#define DIS_SAMPLE_RATE_27	27000
+#define DIS_SAMPLE_RATE_54	54000
+#define DIS_SAMPLE_RATE_74_25	74250
+#define DIS_SAMPLE_RATE_148_5	148500
+#define HAD_REG_WIDTH		0x08
+#define HAD_MAX_HW_BUFS		0x04
+#define HAD_MAX_DIP_WORDS		16
+#define INTEL_HAD		"IntelHdmiLpeAudio"
+
+/* _AUD_CONFIG register MASK */
+#define AUD_CONFIG_MASK_UNDERRUN	0xC0000000
+#define AUD_CONFIG_MASK_SRDBG		0x00000002
+#define AUD_CONFIG_MASK_FUNCRST		0x00000001
+
+#define MAX_CNT			0xFF
+#define HAD_SUSPEND_DELAY	1000
+
+#define OTM_HDMI_ELD_SIZE 128
+
+union otm_hdmi_eld_t {
+	uint8_t eld_data[OTM_HDMI_ELD_SIZE];
+	#pragma pack(1)
+	struct {
+		/* Byte[0] = ELD Version Number */
+		union {
+			uint8_t   byte0;
+			struct {
+				uint8_t reserved:3; /* Reserf */
+				uint8_t eld_ver:5; /* ELD Version Number */
+				/* 00000b - reserved
+				 * 00001b - first rev, obsoleted
+				 * 00010b - version 2, supporting CEA version
+				 *			861D or below
+				 * 00011b:11111b - reserved
+				 * for future
+				 */
+			};
+		};
+
+		/* Byte[1] = Vendor Version Field */
+		union {
+			uint8_t vendor_version;
+			struct {
+				uint8_t reserved1:3;
+				uint8_t veld_ver:5; /* Version number of the ELD
+						     * extension. This value is
+						     * provisioned and unique to
+						     * each vendor.
+						     */
+			};
+		};
+
+		/* Byte[2] = Baseline Length field */
+		uint8_t baseline_eld_length; /* Length of the Baseline structure
+					      *	divided by Four.
+					      */
+
+		/* Byte [3] = Reserved for future use */
+		uint8_t byte3;
+
+		/* Starting of the BaseLine EELD structure
+		 * Byte[4] = Monitor Name Length
+		 */
+		union {
+			uint8_t byte4;
+			struct {
+				uint8_t mnl:5;
+				uint8_t cea_edid_rev_id:3;
+			};
+		};
+
+		/* Byte[5] = Capabilities */
+		union {
+			uint8_t capabilities;
+			struct {
+				uint8_t hdcp:1; /* HDCP support */
+				uint8_t ai_support:1;   /* AI support */
+				uint8_t connection_type:2; /* Connection type
+							    * 00 - HDMI
+							    * 01 - DP
+							    * 10 -11  Reserved
+							    * for future
+							    * connection types
+							    */
+				uint8_t sadc:4; /* Indicates number of 3 bytes
+						 * Short Audio Descriptors.
+						 */
+			};
+		};
+
+		/* Byte[6] = Audio Synch Delay */
+		uint8_t audio_synch_delay; /* Amount of time reported by the
+					    * sink that the video trails audio
+					    * in milliseconds.
+					    */
+
+		/* Byte[7] = Speaker Allocation Block */
+		union {
+			uint8_t speaker_allocation_block;
+			struct {
+				uint8_t flr:1; /*Front Left and Right channels*/
+				uint8_t lfe:1; /*Low Frequency Effect channel*/
+				uint8_t fc:1;  /*Center transmission channel*/
+				uint8_t rlr:1; /*Rear Left and Right channels*/
+				uint8_t rc:1; /*Rear Center channel*/
+				uint8_t flrc:1; /*Front left and Right of Center
+						 *transmission channels
+						 */
+				uint8_t rlrc:1; /*Rear left and Right of Center
+						 *transmission channels
+						 */
+				uint8_t reserved3:1; /* Reserved */
+			};
+		};
+
+		/* Byte[8 - 15] - 8 Byte port identification value */
+		uint8_t port_id_value[8];
+
+		/* Byte[16 - 17] - 2 Byte Manufacturer ID */
+		uint8_t manufacturer_id[2];
+
+		/* Byte[18 - 19] - 2 Byte Product ID */
+		uint8_t product_id[2];
+
+		/* Byte [20-83] - 64 Bytes of BaseLine Data */
+		uint8_t mn_sand_sads[64]; /* This will include
+					   * - ASCII string of Monitor name
+					   * - List of 3 byte SADs
+					   * - Zero padding
+					   */
+
+		/* Vendor ELD Block should continue here!
+		 * No Vendor ELD block defined as of now.
+		 */
+	};
+	#pragma pack()
+};
+
+/**
+ * enum had_status - Audio stream states
+ *
+ * @STREAM_INIT: Stream initialized
+ * @STREAM_RUNNING: Stream running
+ * @STREAM_PAUSED: Stream paused
+ * @STREAM_DROPPED: Stream dropped
+ */
+enum had_stream_status {
+	STREAM_INIT = 0,
+	STREAM_RUNNING = 1,
+	STREAM_PAUSED = 2,
+	STREAM_DROPPED = 3
+};
+
+/**
+ * enum had_status_stream - HAD stream states
+ */
+enum had_status_stream {
+	HAD_INIT = 0,
+	HAD_RUNNING_STREAM,
+};
+
+enum had_drv_status {
+	HAD_DRV_CONNECTED,
+	HAD_DRV_RUNNING,
+	HAD_DRV_DISCONNECTED,
+	HAD_DRV_SUSPENDED,
+	HAD_DRV_ERR,
+};
+
+/* enum intel_had_aud_buf_type - HDMI controller ring buffer types */
+enum intel_had_aud_buf_type {
+	HAD_BUF_TYPE_A = 0,
+	HAD_BUF_TYPE_B = 1,
+	HAD_BUF_TYPE_C = 2,
+	HAD_BUF_TYPE_D = 3,
+};
+
+enum num_aud_ch {
+	CH_STEREO = 0,
+	CH_THREE_FOUR = 1,
+	CH_FIVE_SIX = 2,
+	CH_SEVEN_EIGHT = 3
+};
+
+/* HDMI Controller register offsets - audio domain common */
+/* Base address for below regs = 0x65000 */
+enum hdmi_ctrl_reg_offset_common {
+	AUDIO_HDMI_CONFIG_A	= 0x000,
+	AUDIO_HDMI_CONFIG_B = 0x800,
+	AUDIO_HDMI_CONFIG_C = 0x900,
+};
+/* HDMI controller register offsets */
+enum hdmi_ctrl_reg_offset_v1 {
+	AUD_CONFIG		= 0x0,
+	AUD_CH_STATUS_0		= 0x08,
+	AUD_CH_STATUS_1		= 0x0C,
+	AUD_HDMI_CTS		= 0x10,
+	AUD_N_ENABLE		= 0x14,
+	AUD_SAMPLE_RATE		= 0x18,
+	AUD_BUF_CONFIG		= 0x20,
+	AUD_BUF_CH_SWAP		= 0x24,
+	AUD_BUF_A_ADDR		= 0x40,
+	AUD_BUF_A_LENGTH	= 0x44,
+	AUD_BUF_B_ADDR		= 0x48,
+	AUD_BUF_B_LENGTH	= 0x4c,
+	AUD_BUF_C_ADDR		= 0x50,
+	AUD_BUF_C_LENGTH	= 0x54,
+	AUD_BUF_D_ADDR		= 0x58,
+	AUD_BUF_D_LENGTH	= 0x5c,
+	AUD_CNTL_ST		= 0x60,
+	AUD_HDMI_STATUS		= 0x68,
+	AUD_HDMIW_INFOFR	= 0x114,
+};
+
+/*
+ * Delta changes in HDMI controller register offsets
+ * compare to v1 version
+ */
+
+enum hdmi_ctrl_reg_offset_v2 {
+	AUD_HDMI_STATUS_v2	= 0x64,
+	AUD_HDMIW_INFOFR_v2	= 0x68,
+};
+
+/*
+ *	CEA speaker placement:
+ *
+ *	FL  FLC   FC   FRC   FR
+ *
+ *						LFE
+ *
+ *	RL  RLC   RC   RRC   RR
+ *
+ *	The Left/Right Surround channel _notions_ LS/RS in SMPTE 320M
+ *	corresponds to CEA RL/RR; The SMPTE channel _assignment_ C/LFE is
+ *	swapped to CEA LFE/FC.
+ */
+enum cea_speaker_placement {
+	FL  = (1 <<  0),        /* Front Left           */
+	FC  = (1 <<  1),        /* Front Center         */
+	FR  = (1 <<  2),        /* Front Right          */
+	FLC = (1 <<  3),        /* Front Left Center    */
+	FRC = (1 <<  4),        /* Front Right Center   */
+	RL  = (1 <<  5),        /* Rear Left            */
+	RC  = (1 <<  6),        /* Rear Center          */
+	RR  = (1 <<  7),        /* Rear Right           */
+	RLC = (1 <<  8),        /* Rear Left Center     */
+	RRC = (1 <<  9),        /* Rear Right Center    */
+	LFE = (1 << 10),        /* Low Frequency Effect */
+};
+
+struct cea_channel_speaker_allocation {
+	int ca_index;
+	int speakers[8];
+
+	/* derived values, just for convenience */
+	int channels;
+	int spk_mask;
+};
+
+struct channel_map_table {
+	unsigned char map;              /* ALSA API channel map position */
+	unsigned char cea_slot;         /* CEA slot value */
+	int spk_mask;                   /* speaker position bit mask */
+};
+
+/**
+ * union aud_cfg - Audio configuration
+ *
+ * @cfg_regx: individual register bits
+ * @cfg_regval: full register value
+ *
+ */
+union aud_cfg {
+	struct {
+		u32 aud_en:1;
+		u32 layout:1;
+		u32 fmt:2;
+		u32 num_ch:2;
+		u32 rsvd0:1;
+		u32 set:1;
+		u32 flat:1;
+		u32 val_bit:1;
+		u32 user_bit:1;
+		u32 underrun:1;
+		u32 rsvd1:20;
+	} cfg_regx;
+	struct {
+		u32 aud_en:1;
+		u32 layout:1;
+		u32 fmt:2;
+		u32 num_ch:3;
+		u32 set:1;
+		u32 flat:1;
+		u32 val_bit:1;
+		u32 user_bit:1;
+		u32 underrun:1;
+		u32 packet_mode:1;
+		u32 left_align:1;
+		u32 bogus_sample:1;
+		u32 dp_modei:1;
+		u32 rsvd:16;
+	} cfg_regx_v2;
+	u32 cfg_regval;
+};
+
+/**
+ * union aud_ch_status_0 - Audio Channel Status 0 Attributes
+ *
+ * @status_0_regx:individual register bits
+ * @status_0_regval:full register value
+ *
+ */
+union aud_ch_status_0 {
+	struct {
+		u32 ch_status:1;
+		u32 lpcm_id:1;
+		u32 cp_info:1;
+		u32 format:3;
+		u32 mode:2;
+		u32 ctg_code:8;
+		u32 src_num:4;
+		u32 ch_num:4;
+		u32 samp_freq:4;
+		u32 clk_acc:2;
+		u32 rsvd:2;
+	} status_0_regx;
+	u32 status_0_regval;
+};
+
+/**
+ * union aud_ch_status_1 - Audio Channel Status 1 Attributes
+ *
+ * @status_1_regx: individual register bits
+ * @status_1_regval: full register value
+ *
+ */
+union aud_ch_status_1 {
+	struct {
+		u32 max_wrd_len:1;
+		u32 wrd_len:3;
+		u32 rsvd:28;
+		} status_1_regx;
+	u32 status_1_regval;
+};
+
+/**
+ * union aud_hdmi_cts - CTS register
+ *
+ * @cts_regx: individual register bits
+ * @cts_regval: full register value
+ *
+ */
+union aud_hdmi_cts {
+	struct {
+		u32 cts_val:20;
+		u32 en_cts_prog:1;
+		u32 rsvd:11;
+	} cts_regx;
+	struct {
+		u32 cts_val:24;
+		u32 en_cts_prog:1;
+		u32 rsvd:7;
+	} cts_regx_v2;
+	u32 cts_regval;
+};
+
+/**
+ * union aud_hdmi_n_enable - N register
+ *
+ * @n_regx: individual register bits
+ * @n_regval: full register value
+ *
+ */
+union aud_hdmi_n_enable {
+	struct {
+		u32 n_val:20;
+		u32 en_n_prog:1;
+		u32 rsvd:11;
+	} n_regx;
+	struct {
+		u32 n_val:24;
+		u32 en_n_prog:1;
+		u32 rsvd:7;
+	} n_regx_v2;
+	u32 n_regval;
+};
+
+/**
+ * union aud_buf_config -  Audio Buffer configurations
+ *
+ * @buf_cfg_regx: individual register bits
+ * @buf_cfgval: full register value
+ *
+ */
+union aud_buf_config {
+	struct {
+		u32 fifo_width:8;
+		u32 rsvd0:8;
+		u32 aud_delay:8;
+		u32 rsvd1:8;
+	} buf_cfg_regx;
+	struct {
+		u32 audio_fifo_watermark:8;
+		u32 dma_fifo_watermark:3;
+		u32 rsvd0:5;
+		u32 aud_delay:8;
+		u32 rsvd1:8;
+	} buf_cfg_regx_v2;
+	u32 buf_cfgval;
+};
+
+/**
+ * union aud_buf_ch_swap - Audio Sample Swapping offset
+ *
+ * @buf_ch_swap_regx: individual register bits
+ * @buf_ch_swap_val: full register value
+ *
+ */
+union aud_buf_ch_swap {
+	struct {
+		u32 first_0:3;
+		u32 second_0:3;
+		u32 first_1:3;
+		u32 second_1:3;
+		u32 first_2:3;
+		u32 second_2:3;
+		u32 first_3:3;
+		u32 second_3:3;
+		u32 rsvd:8;
+	} buf_ch_swap_regx;
+	u32 buf_ch_swap_val;
+};
+
+/**
+ * union aud_buf_addr - Address for Audio Buffer
+ *
+ * @buf_addr_regx: individual register bits
+ * @buf_addr_val: full register value
+ *
+ */
+union aud_buf_addr {
+	struct {
+		u32 valid:1;
+		u32 intr_en:1;
+		u32 rsvd:4;
+		u32 addr:26;
+	} buf_addr_regx;
+	u32 buf_addr_val;
+};
+
+/**
+ * union aud_buf_len - Length of Audio Buffer
+ *
+ * @buf_len_regx: individual register bits
+ * @buf_len_val: full register value
+ *
+ */
+union aud_buf_len {
+	struct {
+		u32 buf_len:20;
+		u32 rsvd:12;
+	} buf_len_regx;
+	u32 buf_len_val;
+};
+
+/**
+ * union aud_ctrl_st - Audio Control State Register offset
+ *
+ * @ctrl_regx: individual register bits
+ * @ctrl_val: full register value
+ *
+ */
+union aud_ctrl_st {
+	struct {
+		u32 ram_addr:4;
+		u32 eld_ack:1;
+		u32 eld_addr:4;
+		u32 eld_buf_size:5;
+		u32 eld_valid:1;
+		u32 cp_ready:1;
+		u32 dip_freq:2;
+		u32 dip_idx:3;
+		u32 dip_en_sta:4;
+		u32 rsvd:7;
+	} ctrl_regx;
+	u32 ctrl_val;
+};
+
+/**
+ * union aud_info_frame1 - Audio HDMI Widget Data Island Packet offset
+ *
+ * @fr1_regx: individual register bits
+ * @fr1_val: full register value
+ *
+ */
+union aud_info_frame1 {
+	struct {
+		u32 pkt_type:8;
+		u32 ver_num:8;
+		u32 len:5;
+		u32 rsvd:11;
+	} fr1_regx;
+	u32 fr1_val;
+};
+
+/**
+ * union aud_info_frame2 - DIP frame 2
+ *
+ * @fr2_regx: individual register bits
+ * @fr2_val: full register value
+ *
+ */
+union aud_info_frame2 {
+	struct {
+		u32 chksum:8;
+		u32 chnl_cnt:3;
+		u32 rsvd0:1;
+		u32 coding_type:4;
+		u32 smpl_size:2;
+		u32 smpl_freq:3;
+		u32 rsvd1:3;
+		u32 format:8;
+	} fr2_regx;
+	u32 fr2_val;
+};
+
+/**
+ * union aud_info_frame3 - DIP frame 3
+ *
+ * @fr3_regx: individual register bits
+ * @fr3_val: full register value
+ *
+ */
+union aud_info_frame3 {
+	struct {
+		u32 chnl_alloc:8;
+		u32 rsvd0:3;
+		u32 lsv:4;
+		u32 dm_inh:1;
+		u32 rsvd1:16;
+	} fr3_regx;
+	u32 fr3_val;
+};
+
+enum hdmi_connector_status {
+	hdmi_connector_status_connected = 1,
+	hdmi_connector_status_disconnected = 2,
+	hdmi_connector_status_unknown = 3,
+};
+
+#define HDMI_AUDIO_UNDERRUN     (1UL<<31)
+#define HDMI_AUDIO_BUFFER_DONE  (1UL<<29)
+
+
+#define PORT_ENABLE			(1 << 31)
+#define SDVO_AUDIO_ENABLE	(1 << 6)
+
+enum had_caps_list {
+	HAD_GET_ELD = 1,
+	HAD_GET_DISPLAY_RATE,
+	HAD_SET_ENABLE_AUDIO,
+	HAD_SET_DISABLE_AUDIO,
+	HAD_SET_ENABLE_AUDIO_INT,
+	HAD_SET_DISABLE_AUDIO_INT,
+};
+
+enum had_event_type {
+	HAD_EVENT_HOT_PLUG = 1,
+	HAD_EVENT_HOT_UNPLUG,
+	HAD_EVENT_MODE_CHANGING,
+	HAD_EVENT_AUDIO_BUFFER_DONE,
+	HAD_EVENT_AUDIO_BUFFER_UNDERRUN,
+	HAD_EVENT_QUERY_IS_AUDIO_BUSY,
+	HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED,
+};
+
+/*
+ * HDMI Display Controller Audio Interface
+ *
+ */
+typedef int (*had_event_call_back) (enum had_event_type event_type,
+		void *ctxt_info);
+
+struct hdmi_audio_registers_ops {
+	int (*hdmi_audio_get_register_base)(uint32_t **reg_base,
+			uint32_t *config_offset);
+	int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data);
+	int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data);
+	int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data,
+			uint32_t mask);
+};
+
+struct hdmi_audio_query_set_ops {
+	int (*hdmi_audio_get_caps)(enum had_caps_list query_element,
+			void *capabilties);
+	int (*hdmi_audio_set_caps)(enum had_caps_list set_element,
+			void *capabilties);
+};
+
+struct hdmi_audio_event {
+	int type;
+};
+
+struct snd_intel_had_interface {
+	const char *name;
+	int (*query)(void *had_data, struct hdmi_audio_event event);
+	int (*suspend)(void *had_data, struct hdmi_audio_event event);
+	int (*resume)(void *had_data);
+};
+
+bool mid_hdmi_audio_is_busy(void *dev);
+bool mid_hdmi_audio_suspend(void *dev);
+void mid_hdmi_audio_resume(void *dev);
+void mid_hdmi_audio_signal_event(enum had_event_type event);
+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);
+int mid_hdmi_audio_register(
+	struct snd_intel_had_interface *driver,
+	void *had_data);
+
+#endif