On Mon, May 25, 2015 at 5:11 AM, Mark Brown broonie@kernel.org wrote:
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.
Thanks for taking a look Mark, and thanks for catching those things. I'll send an updated version of just this patch on top of your topic/gpio-jack branch tomorrow morning pacific time.
+- gpio-audio-jack,debounce-times : Debounce time for each sw-det-gpio
entry.specified in...?
That is important info, added.
+config SND_SOC_GPIO_AUDIO_JACK
tristate "GPIO based audio jack detection"This should surely depend on GPIOLIB || COMPILE_TEST.
indeed, added.
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).
Good call, done.
if (!gpio_names)return -ENOMEM;gpio_report = devm_kzalloc(dev, sizeof(*gpio_report) * priv->gpio_count,GFP_KERNEL);Blank lines between blocks please.
Added.
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...).
handled.
+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.
That does work better, I moved all the gpio get/put to the device probe. Thanks.
+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?
I'm not sure how that happened. I'll fix it =)
+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.
Changed.