> -----Original Message-----
> From: Takashi Iwai [mailto:ti...@suse.de]
> Sent: Wednesday, January 18, 2017 5:16 PM
> To: Anand, Jerome <jerome.an...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org;
> ville.syrj...@linux.intel.com; broo...@kernel.org; pierre-
> louis.boss...@linux.intel.com; Ughreja, Rakesh A
> <rakesh.a.ughr...@intel.com>
> Subject: Re: [PATCH V3 3/5] ALSA: add Intel HDMI LPE audio driver for
> BYT/CHT-T
> 
> On Tue, 10 Jan 2017 06:08:39 +0100,
> Jerome Anand wrote:
> >
> > On Baytrail and Cherrytrail, HDaudio may be fused out or disabled by
> > the BIOS. This driver enables an alternate path to the i915 display
> > registers and DMA.
> >
> > Although there is no hardware path between i915 display and LPE/SST
> > audio clusters, this HDMI capability is referred to in the
> > documentation as "HDMI LPE Audio" so we keep the name for consistency.
> > There is no hardware path or control dependencies with the LPE/SST DSP
> functionality.
> >
> > The hdmi-lpe-audio driver will be probed when the i915 driver creates
> > a child platform device.
> >
> > Since this driver is neither SoC nor PCI, a new x86 folder is added
> > Additional indirections in the code will be cleaned up in the next
> > series to aid smoother DP integration
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.boss...@linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.an...@intel.com>
> 
> Just a few knitpicks below:
> 

Thank you for the detailed review.

> > --- a/sound/Makefile
> > +++ b/sound/Makefile
> > @@ -5,7 +5,7 @@ obj-$(CONFIG_SOUND) += soundcore.o
> >  obj-$(CONFIG_SOUND_PRIME) += oss/
> >  obj-$(CONFIG_DMASOUND) += oss/
> >  obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ sh/ synth/
> usb/ \
> > -   firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/
> > +   firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/ x86/
> >  obj-$(CONFIG_SND_AOA) += aoa/
> >
> >  # This one must be compilable even if sound is configured out diff
> > --git a/sound/x86/Kconfig b/sound/x86/Kconfig new file mode 100644
> > index 0000000..e9297d0
> > --- /dev/null
> > +++ b/sound/x86/Kconfig
> > @@ -0,0 +1,16 @@
> > +menuconfig SND_X86
> > +   tristate "X86 sound devices"
> 
> This should depends on X86.
> 

Ok will add it

> 
> > +   ---help---
> > +
> > +     X86 sound devices that don't fall under SoC or PCI categories
> > +
> > +if SND_X86
> > +
> > +config HDMI_LPE_AUDIO
> > +   tristate "HDMI audio without HDaudio on Intel Atom platforms"
> > +   depends on DRM_I915
> > +   default n
> 
> default=n is superfluous.
> 

Ok will remove it

> > +   help
> > +    Choose this option to support HDMI LPE Audio mode
> > +
> > +endif      # SND_X86
> > diff --git a/sound/x86/Makefile b/sound/x86/Makefile new file mode
> > 100644 index 0000000..54d002d
> > --- /dev/null
> > +++ b/sound/x86/Makefile
> > @@ -0,0 +1,6 @@
> > +ccflags-y += -Iinclude/drm
> 
> Do we really need this?
> 

Not needed now I believe , will remove it

> > +snd-hdmi-lpe-audio-objs += \
> > +   intel_hdmi_lpe_audio.o
> > +
> > +obj-$(CONFIG_HDMI_LPE_AUDIO) += snd-hdmi-lpe-audio.o
> > diff --git a/sound/x86/intel_hdmi_lpe_audio.c
> > b/sound/x86/intel_hdmi_lpe_audio.c
> > new file mode 100644
> > index 0000000..c2ebce1
> > --- /dev/null
> > +++ b/sound/x86/intel_hdmi_lpe_audio.c
> > @@ -0,0 +1,614 @@
> > +/*
> > + *  intel_hdmi_lpe_audio.c - Intel HDMI LPE audio driver for Atom
> > +platforms
> > + *
> > + *  Copyright (C) 2016 Intel Corp
> > + *  Authors:
> > + *         Jerome Anand <jerome.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.
> 

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to