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.boss...@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
>
