> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Thursday, January 19, 2017 4:22 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > Cc: alsa-devel@xxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; pierre- > louis.bossart@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
