[11/15] sound: soc: poodle: make use of new locomo GPIO interface
diff mbox

Message ID 1414454528-24240-12-git-send-email-dbaryshkov@gmail.com
State Not Applicable
Headers show

Commit Message

Dmitry Eremin-Solenikov Oct. 28, 2014, 12:02 a.m. UTC
Since LoCoMo driver has been converted to provide proper gpiolib
interface, make poodle ASoC platform driver use gpiolib API.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 sound/soc/pxa/poodle.c | 51 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

Comments

Mark Brown Oct. 28, 2014, 2:58 p.m. UTC | #1
On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
> Since LoCoMo driver has been converted to provide proper gpiolib
> interface, make poodle ASoC platform driver use gpiolib API.

Please use subject lines matching the style for the subsystem.

> +	ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
> +	if (ret) {
> +		dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
> +				ret);
> +		return ret;
> +	}

I sense a need for devm_gpio_request_array() here.  Otherwise this looks
fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
so no need to block on this.

Acked-by: Mark Brown <broonie@kernel.org>

with at least the subject line fixed.
Dmitry Eremin-Solenikov Oct. 28, 2014, 4:45 p.m. UTC | #2
On 10/28/2014 05:58 PM, Mark Brown wrote:
> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>> Since LoCoMo driver has been converted to provide proper gpiolib
>> interface, make poodle ASoC platform driver use gpiolib API.
>
> Please use subject lines matching the style for the subsystem.
>
>> +	ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>> +				ret);
>> +		return ret;
>> +	}
>
> I sense a need for devm_gpio_request_array() here.  Otherwise this looks
> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
> so no need to block on this.

I like the idea of devm_gpio_request_array. But I would like not to add 
additional (core) patches to this patchset.

>
> Acked-by: Mark Brown <broonie@kernel.org>
>
> with at least the subject line fixed.

Subject line fixed.
Alexandre Courbot Oct. 29, 2014, 3:03 a.m. UTC | #3
On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>> Since LoCoMo driver has been converted to provide proper gpiolib
>> interface, make poodle ASoC platform driver use gpiolib API.
>
> Please use subject lines matching the style for the subsystem.
>
>> +     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>> +                             ret);
>> +             return ret;
>> +     }
>
> I sense a need for devm_gpio_request_array() here.  Otherwise this looks
> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
> so no need to block on this.

I wish Dmitry took the opportunity to move this driver to the gpiod
API, especially since doing so would be trivial for this driver. Not a
critical requirement though, the present patch is already an
improvement. But if you want to do that last step, please have a look
at Documentation/gpio/consumer.txt and the "Platform Data" section of
Documentation/gpio/board.txt.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 31, 2014, 9:52 a.m. UTC | #4
On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>> interface, make poodle ASoC platform driver use gpiolib API.
>>
>> Please use subject lines matching the style for the subsystem.
>>
>>> +     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>> +     if (ret) {
>>> +             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>> +                             ret);
>>> +             return ret;
>>> +     }
>>
>> I sense a need for devm_gpio_request_array() here.  Otherwise this looks
>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>> so no need to block on this.
>
> I wish Dmitry took the opportunity to move this driver to the gpiod
> API, especially since doing so would be trivial for this driver.

+1 on this.

However this platform is not device tree, so this implies setting up
a descriptor table for the affected driver(s) to work properly.
See Documentation/gpio/board.txt

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Eremin-Solenikov Oct. 31, 2014, 9:58 a.m. UTC | #5
2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>>> interface, make poodle ASoC platform driver use gpiolib API.
>>>
>>> Please use subject lines matching the style for the subsystem.
>>>
>>>> +     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>>> +     if (ret) {
>>>> +             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>>> +                             ret);
>>>> +             return ret;
>>>> +     }
>>>
>>> I sense a need for devm_gpio_request_array() here.  Otherwise this looks
>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>>> so no need to block on this.
>>
>> I wish Dmitry took the opportunity to move this driver to the gpiod
>> API, especially since doing so would be trivial for this driver.
>
> +1 on this.
>
> However this platform is not device tree, so this implies setting up
> a descriptor table for the affected driver(s) to work properly.
> See Documentation/gpio/board.txt

I checked the gpiod interfaces after original suggestion by Alexandre.

Introducing those mapping tables (much like pinctrl tables) look like
a duplicate effort if Russell will permit adding a DT support. So
I thought that I will reconsider gpiod/pinctrl/etc after fixing
LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK
and (finally) thinking about device tree support.
Alexandre Courbot Nov. 1, 2014, 5:42 a.m. UTC | #6
On Fri, Oct 31, 2014 at 6:58 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> 2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote:
>>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote:
>>>>> Since LoCoMo driver has been converted to provide proper gpiolib
>>>>> interface, make poodle ASoC platform driver use gpiolib API.
>>>>
>>>> Please use subject lines matching the style for the subsystem.
>>>>
>>>>> +     ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
>>>>> +     if (ret) {
>>>>> +             dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
>>>>> +                             ret);
>>>>> +             return ret;
>>>>> +     }
>>>>
>>>> I sense a need for devm_gpio_request_array() here.  Otherwise this looks
>>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win
>>>> so no need to block on this.
>>>
>>> I wish Dmitry took the opportunity to move this driver to the gpiod
>>> API, especially since doing so would be trivial for this driver.
>>
>> +1 on this.
>>
>> However this platform is not device tree, so this implies setting up
>> a descriptor table for the affected driver(s) to work properly.
>> See Documentation/gpio/board.txt
>
> I checked the gpiod interfaces after original suggestion by Alexandre.
>
> Introducing those mapping tables (much like pinctrl tables) look like
> a duplicate effort if Russell will permit adding a DT support. So
> I thought that I will reconsider gpiod/pinctrl/etc after fixing
> LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK
> and (finally) thinking about device tree support.

Note that the mapping tables are likely not going to end up being
huge, and taking that step will allow you to convert to the gpiod
interface, something you would probably want to do later when adding
DT support anyway. So at the end of the day there would be very little
wasted effort: once converting to DT, just add the GPIO properties
into the appropriate node, remove the mapping tables, and you're done.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/sound/soc/pxa/poodle.c b/sound/soc/pxa/poodle.c
index 21f3400..a593bff 100644
--- a/sound/soc/pxa/poodle.c
+++ b/sound/soc/pxa/poodle.c
@@ -20,12 +20,12 @@ 
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/soc.h>
 
 #include <asm/mach-types.h>
-#include <asm/hardware/locomo.h>
 #include <mach/poodle.h>
 #include <mach/audio.h>
 
@@ -48,16 +48,12 @@  static void poodle_ext_control(struct snd_soc_dapm_context *dapm)
 	/* set up jack connection */
 	if (poodle_jack_func == POODLE_HP) {
 		/* set = unmute headphone */
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_MUTE_L, 1);
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_MUTE_R, 1);
+		gpio_set_value(POODLE_GPIO_MUTE_L, 1);
+		gpio_set_value(POODLE_GPIO_MUTE_R, 1);
 		snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
 	} else {
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_MUTE_L, 0);
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_MUTE_R, 0);
+		gpio_set_value(POODLE_GPIO_MUTE_L, 0);
+		gpio_set_value(POODLE_GPIO_MUTE_R, 0);
 		snd_soc_dapm_disable_pin(dapm, "Headphone Jack");
 	}
 
@@ -85,10 +81,8 @@  static int poodle_startup(struct snd_pcm_substream *substream)
 static void poodle_shutdown(struct snd_pcm_substream *substream)
 {
 	/* set = unmute headphone */
-	locomo_gpio_write(&poodle_locomo_device.dev,
-		POODLE_LOCOMO_GPIO_MUTE_L, 1);
-	locomo_gpio_write(&poodle_locomo_device.dev,
-		POODLE_LOCOMO_GPIO_MUTE_R, 1);
+	gpio_set_value(POODLE_GPIO_MUTE_L, 1);
+	gpio_set_value(POODLE_GPIO_MUTE_R, 1);
 }
 
 static int poodle_hw_params(struct snd_pcm_substream *substream,
@@ -178,12 +172,7 @@  static int poodle_set_spk(struct snd_kcontrol *kcontrol,
 static int poodle_amp_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *k, int event)
 {
-	if (SND_SOC_DAPM_EVENT_ON(event))
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_AMP_ON, 0);
-	else
-		locomo_gpio_write(&poodle_locomo_device.dev,
-			POODLE_LOCOMO_GPIO_AMP_ON, 1);
+	gpio_set_value(POODLE_GPIO_AMP_ON, !(SND_SOC_DAPM_EVENT_ON(event)));
 
 	return 0;
 }
@@ -263,25 +252,32 @@  static struct snd_soc_card poodle = {
 	.num_dapm_routes = ARRAY_SIZE(poodle_audio_map),
 };
 
+struct gpio poodle_gpios[] = {
+	{ POODLE_GPIO_AMP_ON, GPIOF_OUT_INIT_HIGH, "Amplifier" },
+	{ POODLE_GPIO_MUTE_L, GPIOF_OUT_INIT_LOW, "Mute left" },
+	{ POODLE_GPIO_MUTE_R, GPIOF_OUT_INIT_LOW, "Mute right" },
+};
+
 static int poodle_probe(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = &poodle;
 	int ret;
 
-	locomo_gpio_set_dir(&poodle_locomo_device.dev,
-		POODLE_LOCOMO_GPIO_AMP_ON, 0);
-	/* should we mute HP at startup - burning power ?*/
-	locomo_gpio_set_dir(&poodle_locomo_device.dev,
-		POODLE_LOCOMO_GPIO_MUTE_L, 0);
-	locomo_gpio_set_dir(&poodle_locomo_device.dev,
-		POODLE_LOCOMO_GPIO_MUTE_R, 0);
+	ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+	if (ret) {
+		dev_err(&pdev->dev, "gpio_request_array() failed: %d\n",
+				ret);
+		return ret;
+	}
 
 	card->dev = &pdev->dev;
 
 	ret = snd_soc_register_card(card);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n",
 			ret);
+		gpio_free_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
+	}
 	return ret;
 }
 
@@ -290,6 +286,7 @@  static int poodle_remove(struct platform_device *pdev)
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
 
 	snd_soc_unregister_card(card);
+	gpio_free_array(poodle_gpios, ARRAY_SIZE(poodle_gpios));
 	return 0;
 }