On Wed, Feb 07, 2024 at 12:36:08AM +0000, James Ogletree wrote:
> A write sequencer is a sequence of register addresses
> and values executed by some Cirrus DSPs following
> power state transitions.
>
> Add support for Cirrus drivers to update or add to a
> write sequencer present in firmware.
>
> Signed-off-by: James Ogletree <jogletre@xxxxxxxxxxxxxxxxxxxxx>
> ---
Mostly down to some very minor nit picks from me on this one,
only one comment on the list order.
> + ret = cs_dsp_coeff_read_ctrl(wseq->ctl, 0, words, wseq->ctl->len);
> + if (ret) {
> + cs_dsp_err(dsp, "Failed to read %s: %d\n", wseq->name, ret);
Use wseq->ctl->subname here should be the same string.
> + switch (op->operation) {
> + case CS_DSP_WSEQ_END:
> + op->data = WSEQ_END_OF_SCRIPT;
> + break;
> + case CS_DSP_WSEQ_UNLOCK:
> + op->data = cs_dsp_chunk_read(&ch, 16);
> + break;
> + case CS_DSP_WSEQ_ADDR8:
> + op->address = cs_dsp_chunk_read(&ch, 8);
> + op->data = cs_dsp_chunk_read(&ch, 32);
> + break;
> + case CS_DSP_WSEQ_H16:
> + case CS_DSP_WSEQ_L16:
> + op->address = cs_dsp_chunk_read(&ch, 24);
> + op->data = cs_dsp_chunk_read(&ch, 16);
> + break;
> + case CS_DSP_WSEQ_FULL:
> + op->address = cs_dsp_chunk_read(&ch, 32);
> + op->data = cs_dsp_chunk_read(&ch, 32);
> + break;
> + default:
> + ret = -EINVAL;
> + cs_dsp_err(dsp, "Unsupported op: %X\n", op->operation);
> + goto err_free;
> + }
> +
> + list_add(&op->list, &wseq->ops);
Unless I am mistaken, would be better to use list_add_tail
here. Which would keep the order of writes in the list matching
the order in the write sequence. Currently I think the list
holds the writes in reverse order, so your search for the
first write will actually find the last write.
> +EXPORT_SYMBOL_GPL(cs_dsp_wseq_init);
These should all be EXPORT_SYMBOL_NS_GPL(..., FW_CS_DSP).
> + * cs_dsp_wseq_write() - Add or update an entry in a write sequence
> + * @dsp: Pointer to a DSP structure
> + * @wseq: Write sequence to write to
> + * @addr: Address of the register to be written to
> + * @data: Data to be written
> + * @op_code: The type of operation of the new entry
> + * @update: If true, searches for the first entry in the write sequence with
> + * the same address and op_code, and replaces it. If false, creates a new entry
> + * at the tail.
This should now be expanded to note that it will also add a new
entry if no matching entry is found even when update is set to
true. The comment below says it but good to have it in the docs
too.
> + if (!update) {
> + if (wseq->ctl->len - op_end->offset < new_op_size) {
> + cs_dsp_err(dsp, "Not enough memory in write sequence for entry\n");
> + ret = -ENOMEM;
> + goto op_new_free;
> + }
> +
> + op_end->offset += new_op_size;
> +
> + ret = cs_dsp_coeff_write_ctrl(wseq->ctl, op_end->offset / sizeof(u32),
> + &op_end->data, sizeof(u32));
> + if (ret)
> + goto op_new_free;
> +
> + list_add(&op_new->list, &wseq->ops);
I think if you use list_add_tail(&op_new->list, &op_end->list),
that will also keep the list order matching the hardware order
here.
> +/**
> + * struct cs_dsp_wseq - Describes a write sequence
> + * @name: Name of cs_dsp control
> + * @ctl: Write sequence cs_dsp control
> + * @ops: Operations contained within this write sequence
> + */
> +struct cs_dsp_wseq {
> + const char *name;
Drop name from the struct here it is redundant the ctl pointer
precisely identifies a control.
Thanks,
Charles
