Hi Pierre,
A couple of comments below
On Wed, Aug 21, 2019 at 03:17:19PM -0500, Pierre-Louis Bossart wrote:
These callbacks are invoked when a matching hw_params/hw_free() DAI operation takes place, and will result in IPC operations with the SOF firmware.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/sof/intel/hda.c | 66 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index e754058e3679..1e84ea9e6fce 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -53,6 +53,70 @@ static void hda_sdw_int_enable(struct snd_sof_dev *sdev, bool enable) 0); }
+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.
+{
- struct snd_sof_dev *sdev = arg;
- struct snd_soc_dai *d = dai;
- struct sof_ipc_dai_config config;
- struct sof_ipc_reply reply;
- int ret;
- u32 size = sizeof(config);
- memset(&config, 0, size);
- config.hdr.size = size;
- config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
- config.type = SOF_DAI_INTEL_ALH;
- config.dai_index = (link_id << 8) | (d->id);
- config.alh.stream_id = alh_stream_id;
Entirely up to you, in such cases I usually do something like
+ struct sof_ipc_dai_config config = { + .type = SOF_DAI_INTEL_ALH, + .hre = { + .size = sizeof(config), + .cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG, + ...
which then also avoids a memset(). But that's mostly a matter of personal preference, since this is on stack, the compiler would probably internally anyway translate the above initialisation to a memset() with all the following assignments.
- /* send message to DSP */
- ret = sof_ipc_tx_message(sdev->ipc,
config.hdr.cmd, &config, size, &reply,sizeof(reply));- if (ret < 0) {
dev_err(sdev->dev,"error: failed to set DAI hw_params for link %d dai->id %d ALH %d\n",
Are readers really expected to understand what "dai->id" means? Wouldn't "DAI ID" be friendlier, although I understand you - who might not know what "x->y" stands for?.. ;-)
link_id, d->id, alh_stream_id);- }
- return ret;
+}
+static int sdw_free_stream(void *arg, void *s, void *dai, int link_id) +{
- struct snd_sof_dev *sdev = arg;
- struct snd_soc_dai *d = dai;
- struct sof_ipc_dai_config config;
- struct sof_ipc_reply reply;
- int ret;
- u32 size = sizeof(config);
- memset(&config, 0, size);
- config.hdr.size = size;
- config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
- config.type = SOF_DAI_INTEL_ALH;
- config.dai_index = (link_id << 8) | d->id;
- config.alh.stream_id = 0xFFFF; /* invalid value on purpose */
ditto
- /* send message to DSP */
- ret = sof_ipc_tx_message(sdev->ipc,
config.hdr.cmd, &config, size, &reply,sizeof(reply));- if (ret < 0) {
dev_err(sdev->dev,"error: failed to free stream for link %d dai->id %d\n",link_id, d->id);
ditto
- }
- return ret;
+}
+static const struct sdw_intel_ops sdw_callback = {
- .config_stream = sdw_config_stream,
- .free_stream = sdw_free_stream,
+};
static int hda_sdw_init(struct snd_sof_dev *sdev) { acpi_handle handle; @@ -67,6 +131,8 @@ static int hda_sdw_init(struct snd_sof_dev *sdev) res.mmio_base = sdev->bar[HDA_DSP_BAR]; res.irq = sdev->ipc_irq; res.parent = sdev->dev;
res.ops = &sdw_callback;
res.arg = sdev;
sdw = sdw_intel_init(handle, &res); if (!sdw) {
Hm, looks like this function is using spaces for indentation... Let me check if this is coming from an earlier patch
Thanks Guennadi
-- 2.20.1