Re: [PATCH v6 1/5] firmware: cs_dsp: Add write sequencer interface

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

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