On 2020-09-28 3:44 PM, Andy Shevchenko wrote:
> 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.
>
>> +}
>
In your example, last 'if' will cause exception if reply==NULL.
Got your point, although that's just few lines which already involve
'if' with { } spacing. After removing size-checks you suggested this
code is quite thin already.
>> + 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.
>
Well, well. Indeed we found where Willy is..
>> +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.
>
Sure, can indent for readability reasons.
>> +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.
>
Naming based on FW enum type equivalent.
>> +#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.
>
As basically everything in my code, style is based on existing example -
usually stuff from ASoC core - here, it's include/sound/soc.h. It's full
of such definitions so because catpt belongs to ASoC subsystem, I've
used the exact same style. No problem changing if that's your preference.
>> @@ -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?
>
Declaration of struct catpt_spec in patch 01/14 requires this while the
actual, logical (I'm not sure that's the right word here but I bet you
know what I mean) definition - one based on enum catpt_ssp_iface - is
available only here (02/14), with all other messages structs. I'd rather
modify the location of this constant than play with declaration of
struct catpt_spec between the patches.
Czarek
