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

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

Hi

On 05/16/2014 09:54 PM, Mark Brown wrote:
>> +
>> +	/*
>> +	 * 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.

Looks like Mika was already proposing it :-)
>> +	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.

Indeed yes.
>> +	snd_soc_jack_report(jack, SND_JACK_LINEOUT | SND_JACK_LINEIN,
>> +			    SND_JACK_HEADSET | SND_JACK_LINEOUT |
>> +			    SND_JACK_LINEIN);
> Why is this here?

Hmm.. I'd guess either a left over from a test code or attempt to send a 
wrong initial jack state. I'll check.

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

Right, thanks for pointing, I was blind to see this.

Your other comments are valid to byt-rt5640 too. I'll update it also 
accordingly.

-- 
Jarkko