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.boss...@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.an...@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.an...@intel.com>
> + *           Aravind Siddappaji <aravindx.siddapp...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to