[Intel-gfx] [alsa-devel] [PATCH V3 3/5] ALSA: add Intel HDMI LPE audio driver for BYT/CHT-T

[Anand, Jerome]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Thursday, January 19, 2017 4:22 PM
> To: Anand, Jerome <jerome.anand at intel.com>
> Cc: alsa-devel at alsa-project.org; intel-gfx at lists.freedesktop.org; pierre-
> louis.bossart at linux.intel.com; broonie at kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja at intel.com>; ville.syrjala at 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