snd_soc_component_driver substream ops

snd_soc_component_driver substream ops

[Daniel Baluta]

Hi Morimoto-san,

Looking at snd_soc_component_driver I see there
are some operations like: open, close, hw_params, hw_free. (1)

Now, snd_soc_component_driver has snd_compress_ops.

Do you think it is worth it to group operations from (1) in a similar structure
maybe snd_<xyz>_ops.

Daniel.

Re: snd_soc_component_driver substream ops

[Kuninori Morimoto]

Hi Daniel

Thank you for asking
And sorry for late response, Japan was holiday.

> Looking at snd_soc_component_driver I see there
> are some operations like: open, close, hw_params, hw_free. (1)
> 
> Now, snd_soc_component_driver has snd_compress_ops.
> 
> Do you think it is worth it to group operations from (1) in a similar structure
> maybe snd_<xyz>_ops.

It seems snd_soc_component_driver is using many functions and flags.
Keeping these in the some structure is better, IMO.

I think separating "component" and "compress" is better cleaning ?

	struct snd_compress_ops {
		...
	};

	struct snd_soc_component_driver {
		...
-		const struct snd_compress_ops *compress_ops;
		...
	};

	struct snd_soc_component {
		...
		struct snd_soc_component_driver *driver;
+		const struct snd_compress_ops *compress_ops;
		...
	};	


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: snd_soc_component_driver substream ops

[Daniel Baluta]

On Mon, Jul 27, 2020 at 1:13 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>
>
> Hi Daniel
>
> Thank you for asking
> And sorry for late response, Japan was holiday.
>
> > Looking at snd_soc_component_driver I see there
> > are some operations like: open, close, hw_params, hw_free. (1)
> >
> > Now, snd_soc_component_driver has snd_compress_ops.
> >
> > Do you think it is worth it to group operations from (1) in a similar structure
> > maybe snd_<xyz>_ops.
>
> It seems snd_soc_component_driver is using many functions and flags.
> Keeping these in the some structure is better, IMO.
>
> I think separating "component" and "compress" is better cleaning ?
>
>         struct snd_compress_ops {
>                 ...
>         };
>
>         struct snd_soc_component_driver {
>                 ...
> -               const struct snd_compress_ops *compress_ops;
>                 ...
>         };
>
>         struct snd_soc_component {
>                 ...
>                 struct snd_soc_component_driver *driver;
> +               const struct snd_compress_ops *compress_ops;
>                 ...
>         };
>


Hi Morimoto-san,

Thanks for your answer.

Although I still have many months ahead to understand ASoC framework, I don't
think moving compress_ops out of snd_soc_component_driver it is a good idea.

For me it looks like snd_soc_component_driver abstracts the functionality of a
component so operations should still stay under snd_soc_component_driver.

Indeed snd_soc_component_driver has a lot of operations and flags, I think we
can group the operations as follows:

* operations on PCM substreams (e.g: open(component,substream))
* operations on DT nodes (e.g: of_xlate_dai_id(component, device_node)
* component operations: (e.g: set_pll(component)
* constructor / destructor.

I think a first step would be to create an equivalent of
snd_compress_ops for PCM substreams.
The name ideally would be snd_pcm_ops and all the functions will have
a component and a PCM
substream.

Anyhow, snd_pcm_ops already exists and acts only on a component.

So, I think those operations should be called:
* for PCM: snd_substream_pcm_ops
* for Compress: rename snd_compr_ops with snd_stream_compr_ops.

Another thing I don't understand is why PCM uses 'substream' term
while compress uses 'stream' term.

I must admit that my head hurts every time I try to understand ASoC related
stuff. Thanks a lot for taking your time to clean it up and make the code
easier to understand.