Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

board_helper: support amp link #4666

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

brentlu
Copy link

@brentlu brentlu commented Oct 27, 2023

Two helper functions are implemented, sof_intel_board_set_amp_link() to initialize speaker amplifier dai link structure and sof_intel_board_set_codec_conf() to update codec_conf field of snd_soc_card structure. With these two functions implemented in a common module, we don't have to enable amplifier in machine drivers one by one.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully on board with the idea @brentlu, this PR changes the way we support boards and I am not very hot on the idea of bundling everything together.

break;
case CODEC_RT5650:
/* special case for all-in-one codec, caller to handle */
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change probably exposes randconfig problems. If you add these codec-specific parts in SND_SOC_INTEL_SOF_BOARD_HELPERS then you need to do a select ... of all the required codecs. Currently this is only added on a board-by-board basis.

IOW, you need this:

config SND_SOC_INTEL_SOF_BOARD_HELPERS
	tristate
        select SND_SOC_INTEL_SOF_MAXIM_COMMON
	select SND_SOC_INTEL_SOF_NUVOTON_COMMON
	select SND_SOC_INTEL_SOF_REALTEK_COMMON
        select SND_SOC_INTEL_SOF_CIRRUS_COMMON

And arguably this is the main issue with this PR, we would now have to select all codecs even when they are not used by a distro.

Can we instead pass the sof_codec_dai_link() function as an argument to avoid this switch case and avoid bundling everything together? Same for set_codec_conf(), centralizing everything is not very good in terms of modularity. We should let users or distros the ability to select which board they want, and not bundle everything together IMHO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I am trying to do is moving all amp support code to the intal-board module so we could add amp support once and for all machine drivers. Thanks for pointing out the module loading problem and I'll do experiment to observe the behavior of run-time kernel module loading and see if we could avoid it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new version was uploaded to left codec-specific part in machine drivers.

@brentlu
Copy link
Author

brentlu commented Nov 8, 2023

Update a new version which implements sof_intel_board_set_amp_link() function to initialize speaker amplifier dai link structure. Codec-specific fields are left for caller to complete.

@@ -248,6 +248,48 @@ int sof_intel_board_set_intel_hdmi_link(struct device *dev,
}
EXPORT_SYMBOL_NS(sof_intel_board_set_intel_hdmi_link, SND_SOC_INTEL_SOF_BOARD_HELPERS);

int sof_intel_board_set_amp_link(struct device *dev,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for SSP amp only, better to rename to sof_intel_board_set_ssp_amp_link. We may have a similar helper for sdw amp someday.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -44,6 +45,7 @@ struct sof_card_private {
int hdmi_num;

int ssp_codec;
int ssp_amp;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, where will ctx->ssp_amp be used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I planned to submit another PR later which implements a helper function to generate entire DAI link array according to the content of sof_card_private structure. You could check the code here:

brentlu@e259c8a

Add a helper function for machine drivers to initialize speaker
amplifier DAI link. The function will initialize common fields and let
caller to initialize codec-specific fields like codec, init, exit, and
ops fields.

Signed-off-by: Brent Lu <[email protected]>
Use intel_board module for speaker amplifier DAI link initialization.

Signed-off-by: Brent Lu <[email protected]>
Use intel_board module for speaker amplifier DAI link initialization.

Signed-off-by: Brent Lu <[email protected]>
Use intel_board module for speaker amplifier DAI link initialization.

Signed-off-by: Brent Lu <[email protected]>
Use intel_board module for speaker amplifier DAI link initialization.

Signed-off-by: Brent Lu <[email protected]>
Rename the parameter 'ssp_codec' of sof_card_dai_links_create() since
it's the port number of speaker amplifier. No functional change in
this commit.

Signed-off-by: Brent Lu <[email protected]>
@plbossart plbossart merged commit e5b4fd0 into thesofproject:topic/sof-dev Nov 13, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants