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

[Takashi Iwai]

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.


> > > +/**
> > > + * 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;
?


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


thanks,

Takashi