On Wed, 2015-05-27 at 20:00 +0100, Mark Brown wrote:
On Mon, May 25, 2015 at 06:22:49PM +0100, Liam Girdwood wrote:
This looks really nice at a high level, obviously it's a lot of code so there might be detail problems but I expect we can fix those up later.
@@ -564,6 +565,7 @@ struct snd_soc_dapm_widget { int num_kcontrols; const struct snd_kcontrol_new *kcontrol_news; struct snd_kcontrol **kcontrols;
struct snd_soc_dobj dobj;
/* widget input and outputs */ struct list_head sources;
This adds the dynamic object (which isn't enormous but isn't tiny) to every widget which could add up a bit. I don't know if it's worth making it a pointer to a dobj and allocating that separately? OTOH it's really routes rather than widgets that are the big cost, and it depends on allocators being friendly and not putting lots of padding round dobjs. Definitely fixable incrementally if it is an issue though.
Fwiw, we are planning to shrink DAPM a bit as part of the reduced memory work that Keyon and Vivian are doing. This will be on the list.
- /* external kcontrol init - used to set ext funcs + private data */
- int (*control_load)(struct snd_soc_component *,
struct snd_kcontrol_new *, struct snd_soc_tplg_ctl_hdr *);- int (*control_unload)(struct snd_soc_component *,
struct snd_soc_dobj *);Do we have examples of all these external users yet? Just thinking about what happens if we need a load/unload function but the kernel doesn't know about this particular firmware type - the file didn't say code would be needed and perhaps the code is optional anyway (some optimisation or something). Is that something that was thought about yet, what sort of uses are we expecting?
Vinod has some example code that has the callback on SKL (dont know if it's on public git atm). The intention here was to allow the driver to setup any configuration outside the standard kcontrol registration, so it would be used for things like sending DSP IPC messages to create the control in FW, or loading the private data into the DSP for that control etc. The comment is probably misleading since the get/put/info function binding is done in the core. I'll fix the comment for V3.
+/* pass vendor data to component driver for processing */ +static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
- struct snd_soc_tplg_hdr *hdr)
+{
- int ret = 0;
- if (tplg->comp && tplg->ops && tplg->ops->vendor_load)
ret = tplg->ops->vendor_load(tplg->comp, hdr);- if (ret < 0)
dev_err(tplg->dev,"ASoC: vendor load failed at hdr offset %ld/0x%lx for type %d:%d\n",soc_tplg_get_hdr_offset(tplg),soc_tplg_get_hdr_offset(tplg),hdr->type, hdr->vendor_type);- return ret;
+}
For example I wonder if we should warn if we don't have a handler for vendor data that we see?
Yes, will fix in V3.