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

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: James Ogletree <jogletre@opensource.cirrus.com>
Cc: <dmitry.torokhov@gmail.com>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<lee@kernel.org>, <broonie@kernel.org>, <jeff@labundy.com>,
	<patches@opensource.cirrus.com>, <linux-sound@vger.kernel.org>,
	<linux-input@vger.kernel.org>, <devicetree@vger.kernel.org>
Subject: Re: [PATCH v7 1/5] firmware: cs_dsp: Add write sequencer interface
Date: Wed, 14 Feb 2024 10:24:43 +0000	[thread overview]
Message-ID: <ZcyU6xgpOvrhX57D@ediswmail9.ad.cirrus.com> (raw)
In-Reply-To: <20240212173111.771107-2-jogletre@opensource.cirrus.com>

On Mon, Feb 12, 2024 at 05:31:07PM +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@opensource.cirrus.com>
> ---
> +	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_tail(&op_new->list, &wseq->ops);

This means the new write will be after the terminator in the
list. Whilst I think the code will work this way it would be
much cleaner if we kept the list ordered to match the writes,
by inserting the new element just before op_end.

Thanks,
Charles