[alsa-devel] [PATCH v3 01/14] ASoC: SOF: Add Sound Open Firmware driver core

[Pierre-Louis Bossart]

Hi Daniel,

On 12/12/18 2:42 PM, Daniel Baluta wrote:
> Thanks a lot Pierre, Liam and all the people at Intel for does such a great job
> with this project!
you're welcome, thanks for the comments
>
> Few comments inline:
>
> <snip>
>
>> +/* SOF probe type */
>> +enum sof_device_type {
>> +       SOF_DEVICE_PCI = 0,
>> +       SOF_DEVICE_APCI,
>> +       SOF_DEVICE_SPI
> Maybe comma after last element here? We have at least one more: SOF_DEVICE_DT.
Based on the comments from Andy, we've already removed this type since 
it's not really needed any longer. We would probably need to have a 
sof-dt-dev.c file to deal with the different enumeration style but we'd 
need a platform to work with (or patches welcome). Hint hint wink wink.
>
>> +static int sof_probe(struct platform_device *pdev)
>> +{
>> +       struct snd_sof_pdata *plat_data = dev_get_platdata(&pdev->dev);
>> +       struct snd_sof_dev *sdev;
>> +       const char *drv_name;
>> +       const void *mach;
>> +       int size;
>> +       int ret;
>> +
>> +       sdev = devm_kzalloc(&pdev->dev, sizeof(*sdev), GFP_KERNEL);
>> +       if (!sdev)
>> +               return -ENOMEM;
>> +
>> +       dev_dbg(&pdev->dev, "probing SOF DSP device....\n");
>> +
>> +       /* initialize sof device */
>> +       sdev->dev = &pdev->dev;
>> +       sdev->parent = plat_data->dev;
>> +       if (plat_data->type == SOF_DEVICE_PCI)
>> +               sdev->pci = container_of(plat_data->dev, struct pci_dev, dev);
>> +       sdev->ops = plat_data->machine->pdata;
>> +
>> +       sdev->pdata = plat_data;
>> +       sdev->first_boot = true;
>> +       INIT_LIST_HEAD(&sdev->pcm_list);
>> +       INIT_LIST_HEAD(&sdev->kcontrol_list);
>> +       INIT_LIST_HEAD(&sdev->widget_list);
>> +       INIT_LIST_HEAD(&sdev->dai_list);
>> +       INIT_LIST_HEAD(&sdev->route_list);
>> +       dev_set_drvdata(&pdev->dev, sdev);
>> +       spin_lock_init(&sdev->ipc_lock);
>> +       spin_lock_init(&sdev->hw_lock);
>> +
>> +       /* set up platform component driver */
>> +       snd_sof_new_platform_drv(sdev);
>> +
>> +       /* set default timeouts if none provided */
>> +       if (plat_data->desc->ipc_timeout == 0)
>> +               sdev->ipc_timeout = TIMEOUT_DEFAULT_IPC;
>> +       else
>> +               sdev->ipc_timeout = plat_data->desc->ipc_timeout;
>> +       if (plat_data->desc->boot_timeout == 0)
>> +               sdev->boot_timeout = TIMEOUT_DEFAULT_BOOT;
>> +       else
>> +               sdev->boot_timeout = plat_data->desc->boot_timeout;
>> +
>> +       /* probe the DSP hardware */
>> +       ret = snd_sof_probe(sdev);
> Something looks fishy here.
>
> I couldn't find this function's definition anywhere in the first
> patch which means that after first patch the code doesn't compile :D.
>
> For bisection reasons would be great if the code compiles after each patch.

the build is only added last (in the 37th patch). We didn't really plan 
to be able to compile the initial patches one after the other. we 
*could* add the core compilation earlier and add support for each device 
incrementally, but you need a set of files that are somewhat 
interdependent first.


>
> <snip>
>
>> +       /* now register audio DSP platform driver and dai */
>> +       ret = snd_soc_register_component(&pdev->dev,  &sdev->plat_drv,
>> +                                        sdev->ops->drv,
>> +                                        sdev->ops->num_drv);
>> +       if (ret < 0) {
>> +               dev_err(sdev->dev,
>> +                       "error: failed to register DSP DAI driver %d\n", ret);
>> +               goto comp_err;
> This goes to snd_soc_unregister_component which is not really needed because
> the component didn't register succcesfully.
Right. we went through a series of checks for error flows and missed 
this one. Thanks for reporting this.
>
> <snd>
>
>> +comp_err:
>> +       snd_soc_unregister_component(&pdev->dev);
>> +       snd_sof_free_topology(sdev);
> It is not very clear to me where does the snd_sof_load_topology it is called
> so that there is a need to free it here.
the free_topology part is something we are currently looking it. It 
seems that the framework frees almost everything. It's one of the 
difficulties we are facing, it's not always obvious what the topology 
framework does for you and what needs to be freed manually.  It's a 
known open we will fix shortly.
>
>> +static int sof_remove(struct platform_device *pdev)
>> +{
>> +       struct snd_sof_dev *sdev = dev_get_drvdata(&pdev->dev);
>> +       struct snd_sof_pdata *pdata = sdev->pdata;
>> +
>> +       if (pdata && !IS_ERR(pdata->pdev_mach))
>> +               platform_device_unregister(pdata->pdev_mach);
>> +
>> +       snd_soc_unregister_component(&pdev->dev);
>> +       snd_sof_fw_unload(sdev);
>> +       snd_sof_ipc_free(sdev);
>> +       snd_sof_free_debug(sdev);
>> +       snd_sof_free_trace(sdev);
>> +       snd_sof_remove(sdev);
> There is no snd_sof_free_topology here. Which means that either this
> call is not needed in the
> probe function or it is missing from here.

The topology is leaded in sof_pcm_probe and released in sof_pcm_remove.

But like I said above it's not clear if we need to really free anything...

>
>> +void snd_sof_shutdown(struct device *dev)
>> +{
>> +}
>> +EXPORT_SYMBOL(snd_sof_shutdown);
> I would avoid this empty function calls for the moment.
> Will add it later when there is a need for it.
yes, we removed it already.
>
> <snip>
>
>> +/*
>> + * SOF Device Level.
>> + */
>> +struct snd_sof_dev {
>> +       struct device *dev;
>> +       struct device *parent;
>> +       spinlock_t ipc_lock;    /* lock for IPC users */
>> +       spinlock_t hw_lock;     /* lock for HW IO access */
>> +       struct pci_dev *pci;
>> +
>> +       /* ASoC components */
>> +       struct snd_soc_component_driver plat_drv;
>> +
>> +       /* DSP firmware boot */
>> +       wait_queue_head_t boot_wait;
>> +       u32 boot_complete;
>> +       u32 first_boot;
>> +
>> +       /* DSP HW differentiation */
>> +       struct snd_sof_pdata *pdata;
>> +       const struct snd_sof_dsp_ops *ops;
>> +       struct sof_intel_hda_dev *hda;  /* for HDA based DSP HW */
> Just a design question. Is it ok to hold SOC specific data here? For example on
> arm there is no HDA but there is some other DAI. Would it be ok for me to just
> add it here?

Darn, this was added more than a year ago and should have been 
cleaned-up. There is absolutely no reason why we have an Intel-specific 
definition in there, as you mentioned it it should have been a pointer 
to SoC-specific data. we'll fix this. nice catch, much appreciated.

-Pierre