Re: [PATCH] ASoC: Intel: Add Baytrail byt-max98090 machine driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2377 bytes --]

On Fri, May 16, 2014 at 04:58:04PM +0300, Jarkko Nikula wrote:

> +	snd_soc_dapm_enable_pin(dapm, "Headset Mic");
> +	snd_soc_dapm_enable_pin(dapm, "Headphone");
> +	snd_soc_dapm_enable_pin(dapm, "Ext Spk");
> +	snd_soc_dapm_enable_pin(dapm, "Int Mic");
> +
> +	snd_soc_dapm_sync(dapm);

None of the above code should have any effect - pins are enabled by
default and syncs suppressed until probe is complete.

> +
> +	/*
> +	 * ASoC still uses legacy GPIOs so we look both GPIOs using
> +	 * descriptors here, convert them to numbers and release the
> +	 * acquired descriptors. Once ASoC switches over to GPIO descriptor
> +	 * APIs we can pass them directly.
> +	 */

You could just add the required support to the framework...  it seems
quicker and simpler.

> +	hp_desc = gpiod_get_index(card->dev->parent, NULL, 0);
> +	if (IS_ERR(hp_desc))
> +		return 0;

This just eats errors, it's broken for deferred probe.

> +	snd_soc_jack_report(jack, SND_JACK_LINEOUT | SND_JACK_LINEIN,
> +			    SND_JACK_HEADSET | SND_JACK_LINEOUT |
> +			    SND_JACK_LINEIN);

Why is this here?

> +#ifdef CONFIG_PM_SLEEP
> +static const struct dev_pm_ops byt_max98090_pm_ops = {
> +	.suspend = snd_soc_suspend,
> +	.resume = snd_soc_resume,
> +};
> +
> +#define BYT_MAX98090_PM_OPS	(&byt_max98090_pm_ops)
> +#else
> +#define BYT_MAX98090_PM_OPS	NULL
> +#endif

Why not just use snd_soc_pm_ops?

> +	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_ATOMIC);
> +	if (!drv) {
> +		dev_err(&pdev->dev, "allocation failed\n");
> +		return -ENOMEM;
> +	}

The allocator is already quite noisy of it goes OOM, no need to print.

> +	ret_val = snd_soc_register_card(&byt_max98090_card);
> +	if (ret_val) {

devm_snd_soc_register_card().

> +static int byt_max98090_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *soc_card = platform_get_drvdata(pdev);
> +	struct byt_max98090_private *drv = snd_soc_card_get_drvdata(soc_card);
> +
> +	snd_soc_jack_free_gpios(&drv->jack, ARRAY_SIZE(hs_jack_gpios),
> +				hs_jack_gpios);

> +	snd_soc_card_set_drvdata(soc_card, NULL);
> +	snd_soc_unregister_card(soc_card);
> +	platform_set_drvdata(pdev, NULL);

Setting the data to NULL on removal is just a waste of time and is done
by the core anyway.

You're freeing these in the device level remove path but allocating them
in the ASoC level probe path.  They should be managed consistently.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]