Re: [PATCH v2 07/24] ASoC: SOF: compress: Rename compress ops with ipc3 prefix

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 9/9/26 11:09, Peter Ujfalusi wrote:
> From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> 
> In preparation for adding support for compressed offload support for
> IPC4, rename the current compress implementation with the IPC3 prefix.
> Introduce a new field in struct sof_ipc_pcm_ops to save the
> IPC-specific compressed ops pointer. This should be set when the
> component driver ops are assigned during SOF device probe. Expose a couple
> of common functions that will be used by both IPC-specific implementations
> and rename the compress.c file to ipc3-compress.c
> 
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
> ---
>  sound/soc/sof/Makefile                        |   2 +-
>  sound/soc/sof/core.c                          |  10 +-
>  sound/soc/sof/{compress.c => ipc3-compress.c} | 145 ++++--------------
>  sound/soc/sof/ipc3-pcm.c                      |   3 +
>  sound/soc/sof/ipc3-priv.h                     |   3 +
>  sound/soc/sof/pcm.c                           |   5 +-
>  sound/soc/sof/sof-audio.c                     |  81 ++++++++++
>  sound/soc/sof/sof-audio.h                     |   5 +
>  8 files changed, 137 insertions(+), 117 deletions(-)
>  rename sound/soc/sof/{compress.c => ipc3-compress.c} (66%)
> 

this looks like a trivial code move:

> -static void sof_set_transferred_bytes(struct sof_compr_stream *sstream,
> -				      u64 host_pos, u64 buffer_size)
> -{
> -	u64 prev_pos;
> -	unsigned int copied;
> -
> -	div64_u64_rem(sstream->copied_total, buffer_size, &prev_pos);
> -
> -	if (host_pos < prev_pos)
> -		copied = (buffer_size - prev_pos) + host_pos;
> -	else
> -		copied = host_pos - prev_pos;
> -
> -	sstream->copied_total += copied;
> -}
> -
> -static void snd_sof_compr_fragment_elapsed_work(struct work_struct *work)
> -{
> -	struct snd_sof_pcm_stream *sps =
> -		container_of(work, struct snd_sof_pcm_stream,
> -			     period_elapsed_work);
> -
> -	snd_compr_fragment_elapsed(sps->cstream);
> -}
> -
> -void snd_sof_compr_init_elapsed_work(struct work_struct *work)
> -{
> -	INIT_WORK(work, snd_sof_compr_fragment_elapsed_work);
> -}
> -
> -/*
> - * sof compr fragment elapse, this could be called in irq thread context
> - */
> -void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
> -{
> -	struct snd_soc_pcm_runtime *rtd;
> -	struct snd_compr_runtime *crtd;
> -	struct snd_soc_component *component;
> -	struct sof_compr_stream *sstream;
> -	struct snd_sof_pcm *spcm;
> -
> -	if (!cstream)
> -		return;
> -
> -	rtd = cstream->private_data;
> -	crtd = cstream->runtime;
> -	sstream = crtd->private_data;
> -	component = snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
> -
> -	spcm = snd_sof_find_spcm_dai(component, rtd);
> -	if (!spcm) {
> -		dev_err(component->dev,
> -			"fragment elapsed called for unknown stream!\n");
> -		return;
> -	}
> -
> -	sof_set_transferred_bytes(sstream, spcm->stream[cstream->direction].posn.host_posn,
> -				  crtd->buffer_size);
> -
> -	/* use the same workqueue-based solution as for PCM, cf. snd_sof_pcm_elapsed */
> -	schedule_work(&spcm->stream[cstream->direction].period_elapsed_work);
> -}
> -
> -static int create_page_table(struct snd_soc_component *component,
> -			     struct snd_compr_stream *cstream,
> -			     unsigned char *dma_area, size_t size)
> -{
> -	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> -	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> -	int dir = cstream->direction;
> -	struct snd_sof_pcm *spcm;
> -
> -	spcm = snd_sof_find_spcm_dai(component, rtd);
> -	if (!spcm)
> -		return -EINVAL;
> -
> -	return snd_sof_create_page_table(component->dev, dmab,
> -					 spcm->stream[dir].page_table.area, size);
> -}

...

> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
> +static void sof_set_transferred_bytes(struct sof_compr_stream *sstream,
> +				      u64 host_pos, u64 buffer_size)
> +{
> +	u64 prev_pos;
> +	unsigned int copied;
> +
> +	div64_u64_rem(sstream->copied_total, buffer_size, &prev_pos);
> +
> +	if (host_pos < prev_pos)
> +		copied = (buffer_size - prev_pos) + host_pos;
> +	else
> +		copied = host_pos - prev_pos;
> +
> +	sstream->copied_total += copied;
> +}
> +
> +static void snd_sof_compr_fragment_elapsed_work(struct work_struct *work)
> +{
> +	struct snd_sof_pcm_stream *sps = container_of(work, struct snd_sof_pcm_stream,
> +						      period_elapsed_work);
> +
> +	snd_compr_fragment_elapsed(sps->cstream);
> +}
> +
> +void snd_sof_compr_init_elapsed_work(struct work_struct *work)
> +{
> +	INIT_WORK(work, snd_sof_compr_fragment_elapsed_work);
> +}
> +
> +/*
> + * sof compr fragment elapse, this could be called in irq thread context
> + */
> +void snd_sof_compr_fragment_elapsed(struct snd_compr_stream *cstream)
> +{
> +	struct snd_soc_pcm_runtime *rtd;
> +	struct snd_compr_runtime *crtd;
> +	struct snd_soc_component *component;
> +	struct sof_compr_stream *sstream;
> +	struct snd_sof_pcm *spcm;
> +
> +	if (!cstream)
> +		return;
> +
> +	rtd = cstream->private_data;
> +	crtd = cstream->runtime;
> +	sstream = crtd->private_data;
> +	component = snd_soc_rtdcom_lookup(rtd, SOF_AUDIO_PCM_DRV_NAME);
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm) {
> +		dev_err(component->dev, "fragment elapsed called for unknown stream!\n");
> +		return;
> +	}
> +
> +	sof_set_transferred_bytes(sstream, spcm->stream[cstream->direction].posn.host_posn,
> +				  crtd->buffer_size);
> +
> +	/* use the same workqueue-based solution as for PCM, cf. snd_sof_pcm_elapsed */
> +	schedule_work(&spcm->stream[cstream->direction].period_elapsed_work);
> +}
> +
> +int snd_sof_compr_create_page_table(struct snd_soc_component *component,
> +				    struct snd_compr_stream *cstream,
> +				    unsigned char *dma_area, size_t size)
> +{
> +	struct snd_dma_buffer *dmab = cstream->runtime->dma_buffer_p;
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	int dir = cstream->direction;
> +	struct snd_sof_pcm *spcm;
> +
> +	spcm = snd_sof_find_spcm_dai(component, rtd);
> +	if (!spcm)
> +		return -EINVAL;
> +
> +	return snd_sof_create_page_table(component->dev, dmab,
> +					 spcm->stream[dir].page_table.area, size);
> +}
> +#endif

but then there is also the addition of new ops and renames.

This patch does a lot of things, most of them trivial but overall it's a
lot to review.

maybe split this patch in two with the code move in a first patch, then
the file rename and introduction of new ops?