[alsa-devel] [RFC 3/3] ALSA: hda - add soc hda bus wrapper

[Takashi Iwai]

At Thu,  2 Apr 2015 15:27:31 +0530,
Vinod Koul wrote:
> 
> From: Ramesh Babu <ramesh.babu at intel.com>
> 
> The SKL controller will be ASoC device but needs access to HDA code, so
> create asoc wrppers on top of hdac.
> 
> Signed-off-by: Ramesh Babu <ramesh.babu at intel.com>
> Signed-off-by: Vinod Koul <vinod.koul at intel.com>

Just a few comment after a quicky glance.
Will do more reviews once after processing my huge backlogs after
vacation.

> +/**
> + * hda_device_unregister - unregister a hda device
> + * @pdev: hda device we're unregistering
> + *
> + * Unregistration is done in 2 steps. First we release all resources
> + * and remove it from the subsystem, then we drop reference count by
> + * calling hda_device_put().
> + */
> +void snd_soc_hdac_device_unregister(struct snd_soc_hdac_device *pdev)
> +{
> +	snd_hdac_device_exit(&pdev->hdac);
> +	snd_hdac_device_unregister(&pdev->hdac);
> +	snd_soc_hdac_device_put(pdev);
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_hdac_device_unregister);
> +

Doing this in a shot is basically wrong.  Since ASoC doesn't care much
about the hotplug/unplug, maybe it doesn't matter so much for now,
though...

In general, you should unregister from the subsystem at first.  And
calling snd_hdac_device_exit() should be done in the device object
destructor.  Unregister doesn't guarantee the immediate release of
resources that was being used, thus it's pretty much racy.

BTW, the function name in the comment must be same as the real name.

> +/**
> + * hda_match - bind hda device to hda driver.
> + * @dev: device.
> + * @drv: driver.
> + *
> + */
> +static int soc_hdac_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct snd_soc_hdac_device *pdev = to_soc_hdac_device(dev);
> +	struct snd_soc_hdac_driver *pdrv = to_soc_hdac_driver(drv);
> +
> +	/* Then try to match against the id table */
> +	if (pdrv->id_table)
> +		return soc_hda_match_id(pdrv->id_table, pdev) != NULL;
> +
> +	/* fall-back to driver name match */
> +	return (strcmp(pdev->name, drv->name) == 0);
> +}
> +EXPORT_SYMBOL_GPL(soc_hdac_match);

Any reason to drop snd_ prefix only for this function?

> +MODULE_DESCRIPTION("ASOC HDA bus core");

ASoC.


Takashi