On Sat, Sep 26, 2020 at 10:58:58AM +0200, Cezary Rojewski wrote: > Implement IRQ handlers for immediate and delayed replies and > notifications. Communication is synchronous and allows for serialization > of maximum one message at a time. > > DSP may respond with ADSP_PENDING status for a request - known as > delayed reply - and when situation occurs, framework keeps the lock and > awaits upcoming response through IPCD channel which is handled in > bottom-half. Immediate replies spawn no BH at all as their processing is > very short. ... > +static int catpt_dsp_do_send_msg(struct catpt_dev *cdev, > + struct catpt_ipc_msg request, > + struct catpt_ipc_msg *reply, int timeout) > +{ > + struct catpt_ipc *ipc = &cdev->ipc; > + unsigned long flags; > + int ret; > + > + if (!ipc->ready) > + return -EPERM; > + if (request.size > ipc->config.outbox_size || > + (reply && reply->size > ipc->config.outbox_size)) > + return -EINVAL; > + > + spin_lock_irqsave(&ipc->lock, flags); > + catpt_ipc_msg_init(ipc, reply); > + catpt_dsp_send_tx(cdev, &request); > + spin_unlock_irqrestore(&ipc->lock, flags); > + > + ret = catpt_wait_msg_completion(cdev, timeout); > + if (ret) { > + dev_crit(cdev->dev, "communication severed: %d, rebooting dsp..\n", > + ret); > + ipc->ready = false; > + /* TODO: attempt recovery */ > + return ret; > + } > + ret = ipc->rx.rsp.status; > + if (reply) { > + reply->header = ipc->rx.header; > + if (!ret && reply->data) > + memcpy(reply->data, ipc->rx.data, reply->size); > + } > + > + return ret; One more looking into this... What about if (reply) reply->header = ipc->rx.header; ret = ipc->rx.rsp.status; // or even directly if (status) return status. if (ret) return ret; if (reply->data) memcpy(reply->data, ipc->rx.data, reply->size); return 0; It may be verbose but I think readability is better here. > +} ... > + CATPT_CHANNEL_CONFIG_5_POINT_0 = 7, /* L, C, R, Ls & Rs */ > + CATPT_CHANNEL_CONFIG_5_POINT_1 = 8, /* L, C, R, Ls, Rs & LFE */ > + CATPT_CHANNEL_CONFIG_DUAL_MONO = 9, /* One channel replicated in two */ > + CATPT_CHANNEL_CONFIG_INVALID = 10, Hmm... I think I got the point why DUAL_MONO was at the end of the switch-case. ... > +enum catpt_module_id { > + CATPT_MODID_BASE_FW = 0x0, > + CATPT_MODID_MP3 = 0x1, > + CATPT_MODID_AAC_5_1 = 0x2, > + CATPT_MODID_AAC_2_0 = 0x3, > + CATPT_MODID_SRC = 0x4, > + CATPT_MODID_WAVES = 0x5, > + CATPT_MODID_DOLBY = 0x6, > + CATPT_MODID_BOOST = 0x7, > + CATPT_MODID_LPAL = 0x8, > + CATPT_MODID_DTS = 0x9, > + CATPT_MODID_PCM_CAPTURE = 0xA, > + CATPT_MODID_PCM_SYSTEM = 0xB, > + CATPT_MODID_PCM_REFERENCE = 0xC, > + CATPT_MODID_PCM = 0xD, /* offload */ > + CATPT_MODID_BLUETOOTH_RENDER = 0xE, > + CATPT_MODID_BLUETOOTH_CAPTURE = 0xF, > + CATPT_MODID_LAST = CATPT_MODID_BLUETOOTH_CAPTURE, > +}; if you indent the values to be on the same column it will increase readability. ... > +enum catpt_mclk_frequency { > + CATPT_MCLK_OFF = 0, > + CATPT_MCLK_FREQ_6_MHZ = 1, > + CATPT_MCLK_FREQ_21_MHZ = 2, > + CATPT_MCLK_FREQ_24_MHZ = 3, > +}; Looks like a 3 MHz as base frequency with multiplicators 0, 2, 7, 8. ... > +#define CATPT_STREAM_MSG(msg_type) \ > +{ \ > + .stream_msg_type = CATPT_STRM_##msg_type, \ > + .global_msg_type = CATPT_GLB_STREAM_MESSAGE } > +#define CATPT_STAGE_MSG(msg_type) \ > +{ \ > + .stage_action = CATPT_STG_##msg_type, \ > + .stream_msg_type = CATPT_STRM_STAGE_MESSAGE, \ > + .global_msg_type = CATPT_GLB_STREAM_MESSAGE } Hmm... This split is interesting. I would rather move } to a new line. ... > @@ -15,7 +15,6 @@ > #define CATPT_SHIM_REGS_SIZE 4096 > #define CATPT_DMA_REGS_SIZE 1024 > #define CATPT_DMA_COUNT 2 > -#define CATPT_SSP_COUNT 2 Why is this? > #define CATPT_SSP_REGS_SIZE 512 > > /* DSP Shim registers */ -- With Best Regards, Andy Shevchenko