Messages in this thread | | | Subject | Re: [alsa-devel] [RFC PATCH 4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks | From | Pierre-Louis Bossart <> | Date | Wed, 4 Sep 2019 12:49:24 -0500 |
| |
On 9/4/19 11:55 AM, Vinod Koul wrote: > On 04-09-19, 08:31, Pierre-Louis Bossart wrote: >> On 9/4/19 2:35 AM, Vinod Koul wrote: >>> On 22-08-19, 08:53, Pierre-Louis Bossart wrote: >>>> Thanks for the review Guennadi >>>> >>>>>> +static int sdw_config_stream(void *arg, void *s, void *dai, >>>>>> + void *params, int link_id, int alh_stream_id) >>>>> >>>>> I realise, that these function prototypes aren't being introduced by these >>>>> patches, but just wondering whether such overly generic prototype is really >>>>> a good idea here, whether some of those "void *" pointers could be given >>>>> real types. The first one could be "struct device *" etc. >>>> >>>> In this case the 'arg' parameter is actually a private 'struct snd_sof_dev', >>>> as shown below [1]. We probably want to keep this relatively opaque, this is >>>> a context that doesn't need to be exposed to the SoundWire code. >>> >>> This does look bit ugly. >>> >>>> The dai and params are indeed cases where we could use stronger types, they >>>> are snd_soc_dai and hw_params respectively. I don't recall why the existing >>>> code is this way, Vinod and Sanyog may have the history of this. >>> >>> Yes we wanted to decouple the sdw and audio bits that is the reason why >>> none of the audio types are used here, but I think it should be revisited >>> and perhaps made as: >>> >>> sdw_config_stream(struct device *sdw, struct sdw_callback_ctx *ctx) >>> >>> where the callback context contains all the other args. That would make >>> it look lot neater too and of course use real structs if possible >> >> the suggested sdw_callbback_ctx is really intel-specific at the moment, e.g. >> the notion of link_id and alh_stream_id are due to the hardware, it's not >> generic at all. And in the latest code we also pass the dai->id. > > s/sdw_callback_ctx/intel_sdw_callback_ctx > > Yes this code is intel specific and this would be intel specific too
ok, I'll fold all the fields in a structure then. That's a nice cleanup, thanks Guennadi and Vinod for the reviews.
| |