[RFC,4/5] ASoC: SOF: Intel: hda: add SoundWire stream config/free callbacks
diff mbox series

Message ID 20190821201720.17768-5-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • ASoC: SOF: Intel: SoundWire initial integration
Related show

Commit Message

Pierre-Louis Bossart Aug. 21, 2019, 8:17 p.m. UTC
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(+)

Comments

Guennadi Liakhovetski Aug. 22, 2019, 7:18 a.m. UTC | #1
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
>
Guennadi Liakhovetski Aug. 22, 2019, 7:23 a.m. UTC | #2
On Thu, Aug 22, 2019 at 09:18:35AM +0200, Guennadi Liakhovetski wrote:

[snip]

> >  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

Ouch, it's mutt or whatever editor it's using... Sorry for the noise.

Thanks
Guennadi
Pierre-Louis Bossart Aug. 22, 2019, 1:53 p.m. UTC | #3
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.

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.

> 
>> +{
>> +	struct snd_sof_dev *sdev = arg;
>> +	struct snd_soc_dai *d = dai;
[1]

>> +	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.

I have no preference, so in this case I will go with consistency with 
existing code, which uses the suggested style for all IPCs.

> 
>> +
>> +	/* 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?.. ;-)

I was trying to avoid a confusion here, we have config->dai_index which 
are shared concepts between topology and firmware, and dai->id which is 
shared between topology and machine driver (which refers to the dai in 
the dai_link which has its own .id). In topology files we have the three 
indices and of course after a couple of weeks I can't recall which one 
maps to what.
I am afraid DAI ID might be confused with dai_index. If there are 
suggestions on this I am all ears, all I care about is avoiding 
ambiguity and having to ask Ranjani what index this really is :-)
Guennadi Liakhovetski Aug. 22, 2019, 3:11 p.m. UTC | #4
On Thu, Aug 22, 2019 at 08:53:06AM -0500, 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.

Right, that's why I proposed struct device and not struct snd_sof_dev, to not 
make it SOF-specific, then sdev could be obtained from dev_get_drvdata(). But 
yes, that's unrelated to this series.

Thanks
Guennadi
Vinod Koul Sept. 4, 2019, 7:35 a.m. UTC | #5
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
Pierre-Louis Bossart Sept. 4, 2019, 1:31 p.m. UTC | #6
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.
Vinod Koul Sept. 4, 2019, 4:55 p.m. UTC | #7
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
Pierre-Louis Bossart Sept. 4, 2019, 5:49 p.m. UTC | #8
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.

Patch
diff mbox series

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)
+{
+	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;
+
+	/* 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",
+			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 */
+
+	/* 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);
+	}
+
+	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) {