SoundWire: transition to generic machine driver

[plbossart]

this is compile-tested only, just to gather feedback.

Now that we have dynamic capabilities, we can support RT700 and RT5682 with the same generic machine driver.

@RanderWang can you take a look at the logic for the rtd_init, I am not sure how I can add the controls and widgets only once?

opens: do we need to change the card name to make UCM happy? Or can UCM detect what headset we use?

[bardliao]

Can we separate codec specific part such as init function, pin, route... to separate files? So we only need minimum changes on sof_sdw.c and avoid sof_sdw.c to become a huge file in the future. And I guess people can add new codecs easier.

[kv2019i]

@plbossart wrote:

opens: do we need to change the card name to make UCM happy? Or can UCM detect what headset we use?

Probably best to draft the UCM file as well. I see no showstoppers in just using one card name like we do for HDA. Potential downside is how complex the UCM rules will become -- adding new codecs should be easy in terms of UCM modifications as well.

Anyways for more complex case (e.g. multi-codec case with non-typical speaker topology), longname based more finetuned UCM file is always a possibility.

[RanderWang]

merge error ? duplicate snd_soc_dapm_add_routes. For above case, error is:

[    3.986730] sof_sdw sof_sdw: ASoC: no source widget found for rt1308-2 SPOL
[    3.986732] sof_sdw sof_sdw: ASoC: Failed to add route rt1308-2 SPOL -> direct -> Speaker
[    3.986736] sof_sdw sof_sdw: ASoC: no source widget found for rt1308-2 SPOR
[    3.986737] sof_sdw sof_sdw: ASoC: Failed to add route rt1308-2 SPOR -> direct -> Speaker
[    3.986739] sof_sdw sof_sdw: rt1308 map addition failed: -19
[    3.986741] sof_sdw sof_sdw: ASoC: failed to init SDW1-Playback: -19

[plbossart]

Fixed now, bad merge with copy/paste. Can you give it a try?

[lyakh]

yes, even just getting rid of the ever growing codec list in the file name is great! I also think moving per-codec code in separate files would be great! As a next step - wouldn't it be possible to make this a proper bus with probing / matching? Then we wouldn't even need a central array of all supported codecs?

[plbossart]

Can we separate codec specific part such as init function, pin, route... to separate files? So we only need minimum changes on sof_sdw.c and avoid sof_sdw.c to become a huge file in the future. And I guess people can add new codecs easier.

Good suggestion, let me try that.

[plbossart]

Can we separate codec specific part such as init function, pin, route... to separate files? So we only need minimum changes on sof_sdw.c and avoid sof_sdw.c to become a huge file in the future. And I guess people can add new codecs easier.

@bardliao done, I pushed what I could to separate files, let me know if you like it.

[plbossart]

yes, even just getting rid of the ever growing codec list in the file name is great! I also think moving per-codec code in separate files would be great! As a next step - wouldn't it be possible to make this a proper bus with probing / matching? Then we wouldn't even need a central array of all supported codecs?

The codecs are not discoverable as e.g. for USB or PCI devices, all they do is provide a 64-bit identifier, and you need tables to figure out what they are. In other words it's slightly better than I2C with an HID but not more elaborate.
The rationale for this is that tables in software are free, descriptors in a device are expensive - especially if flash is needed to store them in case you want to address different markets with the same device.

[RanderWang]

With the modification in all_spk_init(#1889 (comment)), I tested this PR on CML- U & H, TGL RVP. Everything works.
And we also need to refine our UCM now.

[bardliao]

Can we separate codec specific part such as init function, pin, route... to separate files? So we only need minimum changes on sof_sdw.c and avoid sof_sdw.c to become a huge file in the future. And I guess people can add new codecs easier.

@bardliao done, I pushed what I could to separate files, let me know if you like it.

Yes, I like it. Thanks @plbossart BTW, sof_sdw_dmic.c seems to include a lot of xxx.h which is included in sof_sdw_common.h already

[lyakh]

I very much suspect, that most of these headers aren't needed here. This header only uses bit-operations, struct list_head and pointers to other ALSA structs, which can be just forward declared. But many headers here really aren't used at all.

[kv2019i]

Yup, and I personally don't like gathering includes to intermediate .h files like this. I prefer that #include is in the file that needs the interface. I.e. this file should only include headers for definitions that are used directly in sof_sdw_common.h and not all headers that users of sof_sdw_common.h need.

I understand this can save some typing/lines in the c files, but maintenance is really difficult if you lose the link between include and why it is there. If it's all in the same file, then it's easy to maintain.

[plbossart]

I don't know how to identify what includes are needed for a specific config. so I took the lazy approach of keeping the list intact and include it everywhere. If you have a better solution that doesn't require trial and errors and works with cross-compilation I am all ears.

[lyakh]

I think it's actually a requirement (or at least a strong preference) to have those and only those headers included in each specific (.h or .c) file which are needed in that file. I.e. if a function is used - a header with its prototype should be included. If a struct / type is used as an object - the respective header obviously must be included. If only a pointer is used, it's preferred to forward-declare that type. Obviously for macros, enums... Also recursive includes aren't accepted. E.g. even if the code compiles because a header was included in another included header, it doesn't mean it can be omitted in this file. So we actually have to explicitly include all those kernel.h, types.h etc. headers explicitly where their macros or types are used.

As for how to identify - that's the complex part. In principle you begin with no headers, you write your code, and then every time you use something, which isn't "standard C" you include the respective header.

In this respective case it's pretty simple - the file is small. A good starting point should be

#include <linux/bits.h>
#include <linux/types.h>
#include <linux/list.h>
#include <sound/soc.h>

That should be it, the rest are structs that you can just forward declare.

[kv2019i]

A few comments to namespace and header inclusion hygiene, but otherwise looking ready to go.

[kv2019i]

Yup, and I personally don't like gathering includes to intermediate .h files like this. I prefer that #include is in the file that needs the interface. I.e. this file should only include headers for definitions that are used directly in sof_sdw_common.h and not all headers that users of sof_sdw_common.h need.

I understand this can save some typing/lines in the c files, but maintenance is really difficult if you lose the link between include and why it is there. If it's all in the same file, then it's easy to maintain.

[lyakh]

bus

@plbossart what about making a bus? Do you think it's too much trouble / not worth it? Wouldn't we then be able to re-use that bus also for non-sdw machines? Respectively: we now have sound/soc/codecs/rt700.c, sound/soc/codecs/rt700-sdw.c and this would also add sound/soc/intel/boards/sof_sdw_rt700.c. It's probably my lack of understanding: is the latter Intel-specific? Wouldn't it belong to the same directory as the former two? That would also solve the problem with probing / hard-coding, wouldn't it?

[plbossart]

bus

@plbossart what about making a bus? Do you think it's too much trouble / not worth it? Wouldn't we then be able to re-use that bus also for non-sdw machines? Respectively: we now have sound/soc/codecs/rt700.c, sound/soc/codecs/rt700-sdw.c and this would also add sound/soc/intel/boards/sof_sdw_rt700.c. It's probably my lack of understanding: is the latter Intel-specific? Wouldn't it belong to the same directory as the former two? That would also solve the problem with probing / hard-coding, wouldn't it?

there is already a bus, with the 'master_device' and a set of "slave devices' hanging off of it.
All these code register dais for cpu and codec dais respectively.

What the machine driver does is create the dailinks that connects those dais. I don't see how this might be handled by a bus. Again all the bus/device handling is done somewhere else, we are only doing dailink management here, and make sure it's aligned with the topology for that platform.

[plbossart]

Thanks to all reviewers, pushed update.
Two remaining things to do:
a) it'd like to break the codec_conf in pieces so that we have everything in the right file. Not sure if this can be done after the card is registered.

b) we need to add qualifiers to help UCM, as discussed with Jaroslav.

No merge for now until we get both of these issues right

[plbossart]

@RanderWang @bardliao I could use some help with the function

/*
 * Note this MUST be called before snd_soc_register_card(), so that the props
 * are in place before the codec component driver's probe function parses them.
 */
int sof_sdw_rt711_add_codec_device_props(const char *sdw_dev_name)

This is problematic if I want to reuse this machine driver for the rt700 case (or rt5682).

Is there a way to detect if this applies or not and make it dynamic? Thanks!

[lyakh]

@plbossart please, let's not do this. Here's an excerpt from Documentation/process/submit-checklist.rst:

1. If you use a facility then #include the file that defines/declares
   that facility. Don't depend on other header files pulling in ones
   that you use.

sof_sdw_common.h really shouldn't be an "include it all" header.

[plbossart]

Thanks for volunteering to identify which include files are needed for each .c file :-)

[lyakh]

Sure, I can gladly help with that. But now we have a working version, I don't see a reason to make it worse. If someone finds a problem - superfluous or missing headers - that can always be fixed. It's difficult to have it perfect and I don't think it's expected. I'm pretty sure many or even most files in the kernel have less than perfect header sets. It's just good to make a reasonable effort to have some good approximation of that perfect header set. If someone finds an issue - it can always be fixed. But this kind of change really makes it worse and contradicts the kernel style requirements.

[bardliao]

@RanderWang @bardliao I could use some help with the function

/*
 * Note this MUST be called before snd_soc_register_card(), so that the props
 * are in place before the codec component driver's probe function parses them.
 */
int sof_sdw_rt711_add_codec_device_props(const char *sdw_dev_name)

This is problematic if I want to reuse this machine driver for the rt700 case (or rt5682).

Is there a way to detect if this applies or not and make it dynamic? Thanks!

We can call device_property_read_u32(sdw_dev, "realtek,jd-src", &jd_src) in sof_sdw_rt711_add_codec_device_props and test jd_src to know if realtek,jd-src is already applied.
And I think
int sof_sdw_add_codec_device_props(const char *sdw_dev_name, const char *prop, u32 val)
can be a generic function for all codecs.

[bardliao]

Most of them above are included in sof_sdw_common.h already

[lyakh]

@bardliao it doesn't matter. .c files shouldn't rely on headers being included by other headers. They should include any required headers themselves.

[RanderWang]

sof_sdw_add_codec_device_props

while we can init this device prop in sof_sdw_rt711_init or other sof_sdw_xxx_init function.

[plbossart]

@RanderWang @bardliao I could use some help with the function

/*
 * Note this MUST be called before snd_soc_register_card(), so that the props
 * are in place before the codec component driver's probe function parses them.
 */
int sof_sdw_rt711_add_codec_device_props(const char *sdw_dev_name)

This is problematic if I want to reuse this machine driver for the rt700 case (or rt5682).
Is there a way to detect if this applies or not and make it dynamic? Thanks!

We can call device_property_read_u32(sdw_dev, "realtek,jd-src", &jd_src) in sof_sdw_rt711_add_codec_device_props and test jd_src to know if realtek,jd-src is already applied.
And I think
int sof_sdw_add_codec_device_props(const char *sdw_dev_name, const char *prop, u32 val)
can be a generic function for all codecs.

@bardliao the problem is that sof_sdw_rt711_add_codec_device_props() needs to be called before the card is registered, while all the inits we do happen during a dai_link .init() - which happen too late.

So the problem is when to call sof_sdw_rt711_add_codec_device_props(), not what to do inside.

[kv2019i]

Seems good to. A few valid comments form @lyakh plus test on some device -- those seem to be the missing pieces at this point. Not approving yet.

[plbossart]

Tested w/ CML+ RT700 + DMIC and CML+RT711/RT1308x1+RT715
All previous feedback addressed.
@RanderWang can you re-check on TGL please?

[RanderWang]

Tested w/ CML+ RT700 + DMIC and CML+RT711/RT1308x1+RT715
All previous feedback addressed.
@RanderWang can you re-check on TGL please?

I tested this branch on TGL (sdw711+i2s 1308, sdw711+sdw1308). I didn't find any issue.

[plbossart]

ok, thanks reviewers, let's merge and retest. Note that the UCM file at thesofproject/alsa-ucm-conf#29 will be needed