Re: [RFC 3/5] ASoC: Add GPIO based jack device

[Mark Brown <broonie@kernel.org>]

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

On Fri, May 22, 2015 at 03:09:21PM -0700, Dylan Reid wrote:

> Add a jack device that allows for separate headphone and mic detect
> GPIOs.  This device will be used as an aux device and will registered
> the jack with the card at init time.

This looks basically fine, a couple of things below but they're
nitpicks.

> +- gpio-audio-jack,debounce-times	: Debounce time for each sw-det-gpio
> +					  entry.

specified in...?

> +config SND_SOC_GPIO_AUDIO_JACK
> +        tristate "GPIO based audio jack detection"
> +

This should surely depend on GPIOLIB || COMPILE_TEST.

> +	gpio_names = devm_kzalloc(dev, sizeof(*gpio_names) * priv->gpio_count,
> +				  GFP_KERNEL);

We have devm_kcalloc() (not that it makes a huge difference but may as
well be clear about the intent).

> +	if (!gpio_names)
> +		return -ENOMEM;
> +	gpio_report = devm_kzalloc(dev, sizeof(*gpio_report) * priv->gpio_count,
> +				   GFP_KERNEL);

Blank lines between blocks please.

> +	snd_soc_card_jack_new(component->card, jack_name, report_mask,
> +			      &priv->jack, NULL, 0);

Ignoring the return value here (not likely to fail but...).

> +static void gpio_audio_component_remove(struct snd_soc_component *component)
> +{
> +	struct gpio_audio_jack *priv = container_of(component->driver,
> +			struct gpio_audio_jack, component_drv);
> +	int i;
> +
> +	for (i = 0; i < priv->gpio_count; i++) {
> +		if (!IS_ERR(priv->gpios[i].desc))
> +			gpiod_put(priv->gpios[i].desc);
> +	}

I would have expected us to be acquiring and releasing the GPIOs in the
platform device probe, not in the ASoC level probe - that way we handle
deferred probe better.

> +static int gpio_audio_jack_probe(struct platform_device *pdev)
> +{
> +	struct gpio_audio_jack *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, priv);
> +
> +	priv->component_drv.probe = gpio_audio_component_probe;
> +	priv->component_drv.remove = gpio_audio_component_remove;
> +
> +	return devm_snd_soc_register_component(&pdev->dev, &priv->component_drv,
> +					       NULL, 0);

Why do we allocate a component driver structure per device?

> +static const struct of_device_id gpio_audio_of_match[] = {
> +	{ .compatible = "gpio-audio-jack", },

linux,gpio-audio-jack possibly.  Not entirely sure what the current best
practice is on that.

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

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