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