> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Wednesday, January 18, 2017 5:16 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > ville.syrjala@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; pierre- > louis.bossart@xxxxxxxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx> > > 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@xxxxxxxxx> > > + * Aravind Siddappaji <aravindx.siddappaji@xxxxxxxxx> > > + * > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ~~~~~~~~~~~~ > > +~~~~~ > > + * > > + * 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
