-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, January 19, 2017 4:22 PM To: Anand, Jerome jerome.anand@intel.com Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; pierre- louis.bossart@linux.intel.com; broonie@kernel.org; Ughreja, Rakesh A rakesh.a.ughreja@intel.com; ville.syrjala@linux.intel.com Subject: Re: [alsa-devel] [PATCH V3 3/5] ALSA: add Intel HDMI LPE audio driver for BYT/CHT-T
On Thu, 19 Jan 2017 11:39:29 +0100, Anand, Jerome wrote:
+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
If the race protection must be handled inside the callback, please describe it.
Ok, will add a comment along with this code
+/**
- 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?
Why not simply substitution: capabilities = ctx->tmds_clock_speed; ?
Capabilities is a void *.
+/**
- 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.
Passing the base register address and the offset is never a "clean" separation from the abstraction POV. It's just a passthrough of the lowlevel interface. If you want an abstraction layer, such a lowlevel information should be protected inside.
IOW, why does th upper layer need to know these address and offset, if the lowlevel read/write accessor itself already knows of them?
Am maintaining the driver context along with the registers in one abstraction and read/write accesses in another; Hence this indirection is needed.
As already stated previously, once we deliver you the next patch series after DP integration, one level of indirection can be removed. As of now, I propose to keep it this way.
thanks,
Takashi