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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
