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

[Takashi Iwai]

On Fri, 20 Jan 2017 06:46:05 +0100,
Anand, Jerome wrote:
> 
> 
> 
> > -----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 *. 

Ah yes, then a cast is needed.


> > > > > +/**
> > > > > + * 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.

I don't mean about the separated layers (although I prefer removing
it).  My only question in the context above is why you have to pass
the I/O-mapped address back to the upper layer at all.

 From the abstraction POV, passing the raw address to the upper layer
makes no sense at all.  If the raw address is available in the upper
layer, it can access directly by itself, so it doesn't need the lower
layer any longer.  That is, passing such information breaks the
existence of the layer by itself. 

> 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.

OK.  Once when DP works, I can test it locally, so start working on
cleanup.


thanks,

Takashi