[PATCH] ASoC: s3c64xx/smartq: use dynamic registration

[PATCH] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

As a prerequisite for moving s3c64xx into multiplatform configurations,
we need to change the smartq audio driver to stop using hardcoded
gpio numbers from the header file, and instead pass the gpio data
through platform_data.

In order to do that, we also move the code to use module_platform_driver
and register the platform device using platform_device_register_data.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Maurus Cuelenaere <mcuelenaere@gmail.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/mach-s3c64xx/mach-smartq.c | 10 +++++
 sound/soc/samsung/smartq_wm8987.c   | 73 ++++++++++++++-----------------------
 2 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index b3d1353..4c111f6 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -20,6 +20,7 @@
 #include <linux/spi/spi_gpio.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/platform_data/s3c-hsotg.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -379,6 +380,11 @@ void __init smartq_map_io(void)
 	smartq_lcd_mode_set();
 }
 
+static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
+	.gpio_jack = S3C64XX_GPL(12),
+	.gpio_amp = S3C64XX_GPK(12),
+};
+
 void __init smartq_machine_init(void)
 {
 	s3c_i2c0_set_platdata(NULL);
@@ -397,4 +403,8 @@ void __init smartq_machine_init(void)
 	WARN_ON(smartq_wifi_init());
 
 	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
+
+	platform_device_register_data(NULL, "smartq-audio", -1,
+				      &smartq_audio_pdata,
+				      sizeof (smartq_audio_pdata));
 }
diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
index 9b0ffac..8c4a0a2 100644
--- a/sound/soc/samsung/smartq_wm8987.c
+++ b/sound/soc/samsung/smartq_wm8987.c
@@ -19,8 +19,7 @@
 #include <sound/soc.h>
 #include <sound/jack.h>
 
-#include <mach/gpio-samsung.h>
-#include <asm/mach-types.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include "i2s.h"
 #include "../codecs/wm8750.h"
@@ -110,7 +109,7 @@ static struct snd_soc_jack_pin smartq_jack_pins[] = {
 
 static struct snd_soc_jack_gpio smartq_jack_gpios[] = {
 	{
-		.gpio		= S3C64XX_GPL(12),
+		.gpio		= -1,
 		.name		= "headphone detect",
 		.report		= SND_JACK_HEADPHONE,
 		.debounce_time	= 200,
@@ -127,7 +126,10 @@ static int smartq_speaker_event(struct snd_soc_dapm_widget *w,
 				struct snd_kcontrol *k,
 				int event)
 {
-	gpio_set_value(S3C64XX_GPK(12), SND_SOC_DAPM_EVENT_OFF(event));
+	struct smartq_audio_pdata *pdata;
+	pdata = snd_soc_smartq.dev->platform_data;
+
+	gpio_set_value(pdata->gpio_amp, SND_SOC_DAPM_EVENT_OFF(event));
 
 	return 0;
 }
@@ -218,62 +220,41 @@ static struct snd_soc_card snd_soc_smartq = {
 	.num_controls = ARRAY_SIZE(wm8987_smartq_controls),
 };
 
-static struct platform_device *smartq_snd_device;
-
-static int __init smartq_init(void)
+static int smartq_probe(struct platform_device *pdev)
 {
+	struct smartq_audio_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
-	if (!machine_is_smartq7() && !machine_is_smartq5()) {
-		pr_info("Only SmartQ is supported by this ASoC driver\n");
-		return -ENODEV;
-	}
-
-	smartq_snd_device = platform_device_alloc("soc-audio", -1);
-	if (!smartq_snd_device)
-		return -ENOMEM;
-
-	platform_set_drvdata(smartq_snd_device, &snd_soc_smartq);
-
-	ret = platform_device_add(smartq_snd_device);
-	if (ret) {
-		platform_device_put(smartq_snd_device);
-		return ret;
-	}
+	platform_set_drvdata(pdev, &snd_soc_smartq);
 
 	/* Initialise GPIOs used by amplifiers */
-	ret = gpio_request(S3C64XX_GPK(12), "amplifiers shutdown");
+	ret = devm_gpio_request_one(&pdev->dev, pdata->gpio_amp,
+				    GPIOF_DIR_OUT | GPIOF_INIT_HIGH,
+				    "amplifiers shutdown");
 	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to register GPK12\n");
-		goto err_unregister_device;
+		dev_err(&pdev->dev, "Failed to register GPK12\n");
+		goto out;
 	}
 
-	/* Disable amplifiers */
-	ret = gpio_direction_output(S3C64XX_GPK(12), 1);
-	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to configure GPK12\n");
-		goto err_free_gpio_amp_shut;
-	}
+	smartq_jack_gpios[0].gpio = pdata->gpio_jack;
 
-	return 0;
-
-err_free_gpio_amp_shut:
-	gpio_free(S3C64XX_GPK(12));
-err_unregister_device:
-	platform_device_unregister(smartq_snd_device);
+	ret = devm_snd_soc_register_card(&pdev->dev, &snd_soc_smartq);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to register card\n");
 
+out:
 	return ret;
 }
 
-static void __exit smartq_exit(void)
-{
-	gpio_free(S3C64XX_GPK(12));
-
-	platform_device_unregister(smartq_snd_device);
-}
+static struct platform_driver smartq_driver = {
+	.driver = {
+		.name = "smartq-audio",
+		.owner = THIS_MODULE,
+	},
+	.probe = smartq_probe,
+};
 
-module_init(smartq_init);
-module_exit(smartq_exit);
+module_platform_driver(smartq_driver);
 
 /* Module information */
 MODULE_AUTHOR("Maurus Cuelenaere <mcuelenaere@gmail.com>");
-- 
1.8.3.2

[PATCH 0/4] ASoC: samsung updates from arm testing

[Arnd Bergmann]

Hi Mark,

Here are a few patches from my randconfig and multiplatform series.
The smartq patch is a re-send of the one I screwed up the other day.
the dependencies and dma data removal should be trivial, the
dmaengine one probably needs some closer inspection.

	Arnd

Arnd Bergmann (4):
  ASoC: samsung: add explicit i2c/spi dependencies
  ASoC: samsung/smartq: use dynamic registration
  ASoC: samsung: s3c24xx dmaengine follow-up
  ASoC: samsung: remove unused DMA data

 arch/arm/mach-s3c64xx/mach-smartq.c           | 10 ++++
 include/linux/platform_data/asoc-s3c-smartq.h | 10 ++++
 sound/soc/samsung/Kconfig                     | 18 +++----
 sound/soc/samsung/Makefile                    |  2 +-
 sound/soc/samsung/ac97.c                      | 32 ------------
 sound/soc/samsung/dma.h                       |  7 ---
 sound/soc/samsung/i2s.c                       |  6 ---
 sound/soc/samsung/pcm.c                       | 12 -----
 sound/soc/samsung/s3c-i2s-v2.c                |  2 -
 sound/soc/samsung/s3c2412-i2s.c               |  4 --
 sound/soc/samsung/s3c24xx-i2s.c               |  4 --
 sound/soc/samsung/smartq_wm8987.c             | 73 ++++++++++-----------------
 sound/soc/samsung/spdif.c                     |  5 --
 13 files changed, 54 insertions(+), 131 deletions(-)
 create mode 100644 include/linux/platform_data/asoc-s3c-smartq.h

-- 
1.8.3.2

[PATCH 1/4] ASoC: samsung: add explicit i2c/spi dependencies

[Arnd Bergmann]

I got another build error from SND_SOC_SMARTQ selecting
SND_SOC_WM8750 without ensuring that I2C is enabled,
in this example, i2c is a loadable module:

sound/built-in.o: In function `wm8750_i2c_probe':
:(.text+0x3e6c0): undefined reference to `devm_regmap_init_i2c'
sound/built-in.o: In function `wm8750_exit':
:(.exit.text+0x3f4): undefined reference to `i2c_del_driver'
sound/built-in.o: In function `wm8750_modinit':
:(.init.text+0x13d0): undefined reference to `i2c_register_driver'

This changes the samsund ASoC Kconfig to have explicit
I2C dependencies for each driver that uses an i2c bus and
SPI dependencies for the ones using those.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/samsung/Kconfig | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 2c4786c6fe91..7622af82ac4f 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -49,7 +49,7 @@ config SND_SOC_SAMSUNG_NEO1973_WM8753
 
 config SND_SOC_SAMSUNG_JIVE_WM8750
 	tristate "SoC I2S Audio support for Jive"
-	depends on SND_SOC_SAMSUNG && MACH_JIVE
+	depends on SND_SOC_SAMSUNG && MACH_JIVE && I2C
 	select SND_SOC_WM8750
 	select SND_S3C2412_SOC_I2S
 	help
@@ -148,7 +148,7 @@ config SND_SOC_SAMSUNG_SMDK_WM9713
 
 config SND_SOC_SMARTQ
 	tristate "SoC I2S Audio support for SmartQ board"
-	depends on SND_SOC_SAMSUNG && MACH_SMARTQ
+	depends on SND_SOC_SAMSUNG && MACH_SMARTQ && I2C
 	select SND_SAMSUNG_I2S
 	select SND_SOC_WM8750
 
@@ -200,7 +200,7 @@ config SND_SOC_SPEYSIDE
 
 config SND_SOC_TOBERMORY
 	tristate "Audio support for Wolfson Tobermory"
-	depends on SND_SOC_SAMSUNG && MACH_WLF_CRAGG_6410 && INPUT
+	depends on SND_SOC_SAMSUNG && MACH_WLF_CRAGG_6410 && INPUT && I2C
 	select SND_SAMSUNG_I2S
 	select SND_SOC_WM8962
 
@@ -216,7 +216,7 @@ config SND_SOC_BELLS
 
 config SND_SOC_LOWLAND
 	tristate "Audio support for Wolfson Lowland"
-	depends on SND_SOC_SAMSUNG && MACH_WLF_CRAGG_6410
+	depends on SND_SOC_SAMSUNG && MACH_WLF_CRAGG_6410 && I2C
 	select SND_SAMSUNG_I2S
 	select SND_SOC_WM5100
 	select SND_SOC_WM9081
-- 
1.8.3.2

[PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

As a prerequisite for moving s3c64xx into multiplatform configurations,
we need to change the smartq audio driver to stop using hardcoded
gpio numbers from the header file, and instead pass the gpio data
through platform_data.

In order to do that, we also move the code to use module_platform_driver
and register the platform device using platform_device_register_data.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Maurus Cuelenaere <mcuelenaere@gmail.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/mach-s3c64xx/mach-smartq.c           | 10 ++++
 include/linux/platform_data/asoc-s3c-smartq.h | 10 ++++
 sound/soc/samsung/smartq_wm8987.c             | 73 ++++++++++-----------------
 3 files changed, 47 insertions(+), 46 deletions(-)
 create mode 100644 include/linux/platform_data/asoc-s3c-smartq.h

diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index b3d13537a7f0..4c111f60dbf2 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -20,6 +20,7 @@
 #include <linux/spi/spi_gpio.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/platform_data/s3c-hsotg.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -379,6 +380,11 @@ void __init smartq_map_io(void)
 	smartq_lcd_mode_set();
 }
 
+static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
+	.gpio_jack = S3C64XX_GPL(12),
+	.gpio_amp = S3C64XX_GPK(12),
+};
+
 void __init smartq_machine_init(void)
 {
 	s3c_i2c0_set_platdata(NULL);
@@ -397,4 +403,8 @@ void __init smartq_machine_init(void)
 	WARN_ON(smartq_wifi_init());
 
 	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
+
+	platform_device_register_data(NULL, "smartq-audio", -1,
+				      &smartq_audio_pdata,
+				      sizeof (smartq_audio_pdata));
 }
diff --git a/include/linux/platform_data/asoc-s3c-smartq.h b/include/linux/platform_data/asoc-s3c-smartq.h
new file mode 100644
index 000000000000..5bddc3586802
--- /dev/null
+++ b/include/linux/platform_data/asoc-s3c-smartq.h
@@ -0,0 +1,10 @@
+#ifndef __PLATFORM_DATA_ASOC_S3C_SMARTQ
+#define __PLATFORM_DATA_ASOC_S3C_SMARTQ
+
+struct smartq_audio_pdata {
+	int gpio_jack;
+	int gpio_amp;
+};
+
+#endif
+
diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
index 9b0ffacab790..8c4a0a285ce7 100644
--- a/sound/soc/samsung/smartq_wm8987.c
+++ b/sound/soc/samsung/smartq_wm8987.c
@@ -19,8 +19,7 @@
 #include <sound/soc.h>
 #include <sound/jack.h>
 
-#include <mach/gpio-samsung.h>
-#include <asm/mach-types.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include "i2s.h"
 #include "../codecs/wm8750.h"
@@ -110,7 +109,7 @@ static struct snd_soc_jack_pin smartq_jack_pins[] = {
 
 static struct snd_soc_jack_gpio smartq_jack_gpios[] = {
 	{
-		.gpio		= S3C64XX_GPL(12),
+		.gpio		= -1,
 		.name		= "headphone detect",
 		.report		= SND_JACK_HEADPHONE,
 		.debounce_time	= 200,
@@ -127,7 +126,10 @@ static int smartq_speaker_event(struct snd_soc_dapm_widget *w,
 				struct snd_kcontrol *k,
 				int event)
 {
-	gpio_set_value(S3C64XX_GPK(12), SND_SOC_DAPM_EVENT_OFF(event));
+	struct smartq_audio_pdata *pdata;
+	pdata = snd_soc_smartq.dev->platform_data;
+
+	gpio_set_value(pdata->gpio_amp, SND_SOC_DAPM_EVENT_OFF(event));
 
 	return 0;
 }
@@ -218,62 +220,41 @@ static struct snd_soc_card snd_soc_smartq = {
 	.num_controls = ARRAY_SIZE(wm8987_smartq_controls),
 };
 
-static struct platform_device *smartq_snd_device;
-
-static int __init smartq_init(void)
+static int smartq_probe(struct platform_device *pdev)
 {
+	struct smartq_audio_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
-	if (!machine_is_smartq7() && !machine_is_smartq5()) {
-		pr_info("Only SmartQ is supported by this ASoC driver\n");
-		return -ENODEV;
-	}
-
-	smartq_snd_device = platform_device_alloc("soc-audio", -1);
-	if (!smartq_snd_device)
-		return -ENOMEM;
-
-	platform_set_drvdata(smartq_snd_device, &snd_soc_smartq);
-
-	ret = platform_device_add(smartq_snd_device);
-	if (ret) {
-		platform_device_put(smartq_snd_device);
-		return ret;
-	}
+	platform_set_drvdata(pdev, &snd_soc_smartq);
 
 	/* Initialise GPIOs used by amplifiers */
-	ret = gpio_request(S3C64XX_GPK(12), "amplifiers shutdown");
+	ret = devm_gpio_request_one(&pdev->dev, pdata->gpio_amp,
+				    GPIOF_DIR_OUT | GPIOF_INIT_HIGH,
+				    "amplifiers shutdown");
 	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to register GPK12\n");
-		goto err_unregister_device;
+		dev_err(&pdev->dev, "Failed to register GPK12\n");
+		goto out;
 	}
 
-	/* Disable amplifiers */
-	ret = gpio_direction_output(S3C64XX_GPK(12), 1);
-	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to configure GPK12\n");
-		goto err_free_gpio_amp_shut;
-	}
+	smartq_jack_gpios[0].gpio = pdata->gpio_jack;
 
-	return 0;
-
-err_free_gpio_amp_shut:
-	gpio_free(S3C64XX_GPK(12));
-err_unregister_device:
-	platform_device_unregister(smartq_snd_device);
+	ret = devm_snd_soc_register_card(&pdev->dev, &snd_soc_smartq);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to register card\n");
 
+out:
 	return ret;
 }
 
-static void __exit smartq_exit(void)
-{
-	gpio_free(S3C64XX_GPK(12));
-
-	platform_device_unregister(smartq_snd_device);
-}
+static struct platform_driver smartq_driver = {
+	.driver = {
+		.name = "smartq-audio",
+		.owner = THIS_MODULE,
+	},
+	.probe = smartq_probe,
+};
 
-module_init(smartq_init);
-module_exit(smartq_exit);
+module_platform_driver(smartq_driver);
 
 /* Module information */
 MODULE_AUTHOR("Maurus Cuelenaere <mcuelenaere@gmail.com>");
-- 
1.8.3.2

[PATCH 2/4] ASoC: samsung/smartq: use dynamic registration

[Arnd Bergmann]

As a prerequisite for moving s3c64xx into multiplatform configurations,
we need to change the smartq audio driver to stop using hardcoded
gpio numbers from the header file, and instead pass the gpio data
through platform_data.

In order to do that, we also move the code to use module_platform_driver
and register the platform device using platform_device_register_data.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Maurus Cuelenaere <mcuelenaere@gmail.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/mach-s3c64xx/mach-smartq.c           | 10 ++++
 include/linux/platform_data/asoc-s3c-smartq.h | 10 ++++
 sound/soc/samsung/smartq_wm8987.c             | 73 ++++++++++-----------------
 3 files changed, 47 insertions(+), 46 deletions(-)
 create mode 100644 include/linux/platform_data/asoc-s3c-smartq.h

diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index b3d13537a7f0..4c111f60dbf2 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -20,6 +20,7 @@
 #include <linux/spi/spi_gpio.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/platform_data/s3c-hsotg.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -379,6 +380,11 @@ void __init smartq_map_io(void)
 	smartq_lcd_mode_set();
 }
 
+static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
+	.gpio_jack = S3C64XX_GPL(12),
+	.gpio_amp = S3C64XX_GPK(12),
+};
+
 void __init smartq_machine_init(void)
 {
 	s3c_i2c0_set_platdata(NULL);
@@ -397,4 +403,8 @@ void __init smartq_machine_init(void)
 	WARN_ON(smartq_wifi_init());
 
 	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
+
+	platform_device_register_data(NULL, "smartq-audio", -1,
+				      &smartq_audio_pdata,
+				      sizeof (smartq_audio_pdata));
 }
diff --git a/include/linux/platform_data/asoc-s3c-smartq.h b/include/linux/platform_data/asoc-s3c-smartq.h
new file mode 100644
index 000000000000..5bddc3586802
--- /dev/null
+++ b/include/linux/platform_data/asoc-s3c-smartq.h
@@ -0,0 +1,10 @@
+#ifndef __PLATFORM_DATA_ASOC_S3C_SMARTQ
+#define __PLATFORM_DATA_ASOC_S3C_SMARTQ
+
+struct smartq_audio_pdata {
+	int gpio_jack;
+	int gpio_amp;
+};
+
+#endif
+
diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
index 9b0ffacab790..8c4a0a285ce7 100644
--- a/sound/soc/samsung/smartq_wm8987.c
+++ b/sound/soc/samsung/smartq_wm8987.c
@@ -19,8 +19,7 @@
 #include <sound/soc.h>
 #include <sound/jack.h>
 
-#include <mach/gpio-samsung.h>
-#include <asm/mach-types.h>
+#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include "i2s.h"
 #include "../codecs/wm8750.h"
@@ -110,7 +109,7 @@ static struct snd_soc_jack_pin smartq_jack_pins[] = {
 
 static struct snd_soc_jack_gpio smartq_jack_gpios[] = {
 	{
-		.gpio		= S3C64XX_GPL(12),
+		.gpio		= -1,
 		.name		= "headphone detect",
 		.report		= SND_JACK_HEADPHONE,
 		.debounce_time	= 200,
@@ -127,7 +126,10 @@ static int smartq_speaker_event(struct snd_soc_dapm_widget *w,
 				struct snd_kcontrol *k,
 				int event)
 {
-	gpio_set_value(S3C64XX_GPK(12), SND_SOC_DAPM_EVENT_OFF(event));
+	struct smartq_audio_pdata *pdata;
+	pdata = snd_soc_smartq.dev->platform_data;
+
+	gpio_set_value(pdata->gpio_amp, SND_SOC_DAPM_EVENT_OFF(event));
 
 	return 0;
 }
@@ -218,62 +220,41 @@ static struct snd_soc_card snd_soc_smartq = {
 	.num_controls = ARRAY_SIZE(wm8987_smartq_controls),
 };
 
-static struct platform_device *smartq_snd_device;
-
-static int __init smartq_init(void)
+static int smartq_probe(struct platform_device *pdev)
 {
+	struct smartq_audio_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
-	if (!machine_is_smartq7() && !machine_is_smartq5()) {
-		pr_info("Only SmartQ is supported by this ASoC driver\n");
-		return -ENODEV;
-	}
-
-	smartq_snd_device = platform_device_alloc("soc-audio", -1);
-	if (!smartq_snd_device)
-		return -ENOMEM;
-
-	platform_set_drvdata(smartq_snd_device, &snd_soc_smartq);
-
-	ret = platform_device_add(smartq_snd_device);
-	if (ret) {
-		platform_device_put(smartq_snd_device);
-		return ret;
-	}
+	platform_set_drvdata(pdev, &snd_soc_smartq);
 
 	/* Initialise GPIOs used by amplifiers */
-	ret = gpio_request(S3C64XX_GPK(12), "amplifiers shutdown");
+	ret = devm_gpio_request_one(&pdev->dev, pdata->gpio_amp,
+				    GPIOF_DIR_OUT | GPIOF_INIT_HIGH,
+				    "amplifiers shutdown");
 	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to register GPK12\n");
-		goto err_unregister_device;
+		dev_err(&pdev->dev, "Failed to register GPK12\n");
+		goto out;
 	}
 
-	/* Disable amplifiers */
-	ret = gpio_direction_output(S3C64XX_GPK(12), 1);
-	if (ret) {
-		dev_err(&smartq_snd_device->dev, "Failed to configure GPK12\n");
-		goto err_free_gpio_amp_shut;
-	}
+	smartq_jack_gpios[0].gpio = pdata->gpio_jack;
 
-	return 0;
-
-err_free_gpio_amp_shut:
-	gpio_free(S3C64XX_GPK(12));
-err_unregister_device:
-	platform_device_unregister(smartq_snd_device);
+	ret = devm_snd_soc_register_card(&pdev->dev, &snd_soc_smartq);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to register card\n");
 
+out:
 	return ret;
 }
 
-static void __exit smartq_exit(void)
-{
-	gpio_free(S3C64XX_GPK(12));
-
-	platform_device_unregister(smartq_snd_device);
-}
+static struct platform_driver smartq_driver = {
+	.driver = {
+		.name = "smartq-audio",
+		.owner = THIS_MODULE,
+	},
+	.probe = smartq_probe,
+};
 
-module_init(smartq_init);
-module_exit(smartq_exit);
+module_platform_driver(smartq_driver);
 
 /* Module information */
 MODULE_AUTHOR("Maurus Cuelenaere <mcuelenaere@gmail.com>");
-- 
1.8.3.2

[PATCH 3/4] ASoC: samsung: s3c24xx dmaengine follow-up

[Arnd Bergmann]

Commit ae602456e83c92 ("ASoC: samsung: drop support for legacy
S3C24XX DMA API") removed the old code for the samsung specific
DMA interfaces, now that everybody can use dmaengine.

This picks up the few remaining pieces left over by that patch:

The most important one is the removal of the dma_data->ops->started()
calls in ac97. My understanding is that these are only required
for drivers that do not support cyclic transfers, which the new dma
engine driver now does, so we can simply remove them. This would also
fix at least one bug in the ac97 driver on newer machines, which
currently gives us a NULL pointer dereference from trying to call
dma_data->ops->started().

Further, we must no longer 'select' S3C2410_DMA, which conflicts
with the dmaengine driver. The SND_S3C_DMA symbol is now
useless, because it is always selected, so we can remove it
and build the dmaengine support unconditionally.

Finally, we should not 'select' S3C24XX_DMAC or S3C64XX_PL080,
which may have additional dependencies. This replaces it with
'depends on', to be more conservative.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Sangbeom Kim <sbkim73@samsung.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: linux-samsung-soc at vger.kernel.org
---
 sound/soc/samsung/Kconfig  | 10 ++--------
 sound/soc/samsung/Makefile |  2 +-
 sound/soc/samsung/ac97.c   | 17 -----------------
 3 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 7622af82ac4f..afcf95c6e212 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -1,18 +1,14 @@
 config SND_SOC_SAMSUNG
 	tristate "ASoC support for Samsung"
 	depends on PLAT_SAMSUNG
-	select S3C24XX_DMAC if ARCH_S3C24XX
-	select S3C64XX_PL080 if ARCH_S3C64XX
-	select SND_S3C_DMA
+	depends on S3C64XX_PL080 || !ARCH_S3C64XX
+	depends on S3C24XX_DMAC || !ARCH_S3C24XX
 	select SND_SOC_GENERIC_DMAENGINE_PCM
 	help
 	  Say Y or M if you want to add support for codecs attached to
 	  the Samsung SoCs' Audio interfaces. You will also need to
 	  select the audio interfaces to support below.
 
-config SND_S3C_DMA
-	tristate
-
 config SND_S3C24XX_I2S
 	tristate
 
@@ -77,7 +73,6 @@ config SND_SOC_SAMSUNG_SMDK_WM8994
 config SND_SOC_SAMSUNG_SMDK2443_WM9710
 	tristate "SoC AC97 Audio support for SMDK2443 - WM9710"
 	depends on SND_SOC_SAMSUNG && MACH_SMDK2443
-	select S3C2410_DMA
 	select AC97_BUS
 	select SND_SOC_AC97_CODEC
 	select SND_SAMSUNG_AC97
@@ -88,7 +83,6 @@ config SND_SOC_SAMSUNG_SMDK2443_WM9710
 config SND_SOC_SAMSUNG_LN2440SBC_ALC650
 	tristate "SoC AC97 Audio support for LN2440SBC - ALC650"
 	depends on SND_SOC_SAMSUNG && ARCH_S3C24XX
-	select S3C2410_DMA
 	select AC97_BUS
 	select SND_SOC_AC97_CODEC
 	select SND_SAMSUNG_AC97
diff --git a/sound/soc/samsung/Makefile b/sound/soc/samsung/Makefile
index e8d9ccdc41fd..91505ddaaf95 100644
--- a/sound/soc/samsung/Makefile
+++ b/sound/soc/samsung/Makefile
@@ -9,7 +9,7 @@ snd-soc-samsung-spdif-objs := spdif.o
 snd-soc-pcm-objs := pcm.o
 snd-soc-i2s-objs := i2s.o
 
-obj-$(CONFIG_SND_S3C_DMA) += snd-soc-s3c-dma.o
+obj-$(CONFIG_SND_SOC_SAMSUNG) += snd-soc-s3c-dma.o
 obj-$(CONFIG_SND_S3C24XX_I2S) += snd-soc-s3c24xx-i2s.o
 obj-$(CONFIG_SND_SAMSUNG_AC97) += snd-soc-ac97.o
 obj-$(CONFIG_SND_S3C2412_SOC_I2S) += snd-soc-s3c2412-i2s.o
diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
index 68d9303047e8..2aa24d052a4a 100644
--- a/sound/soc/samsung/ac97.c
+++ b/sound/soc/samsung/ac97.c
@@ -19,7 +19,6 @@
 
 #include <sound/soc.h>
 
-#include <mach/dma.h>
 #include "regs-ac97.h"
 #include <linux/platform_data/asoc-s3c.h>
 
@@ -225,9 +224,6 @@ static int s3c_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
 				struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct s3c_dma_params *dma_data =
-		snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
 
 	ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL);
 	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
@@ -253,11 +249,6 @@ static int s3c_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
 
-	if (!dma_data->ops)
-		dma_data->ops = samsung_dma_get_ops();
-
-	dma_data->ops->started(dma_data->channel);
-
 	return 0;
 }
 
@@ -265,9 +256,6 @@ static int s3c_ac97_mic_trigger(struct snd_pcm_substream *substream,
 				    int cmd, struct snd_soc_dai *dai)
 {
 	u32 ac_glbctrl;
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct s3c_dma_params *dma_data =
-		snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
 
 	ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL);
 	ac_glbctrl &= ~S3C_AC97_GLBCTRL_MICINTM_MASK;
@@ -287,11 +275,6 @@ static int s3c_ac97_mic_trigger(struct snd_pcm_substream *substream,
 
 	writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
 
-	if (!dma_data->ops)
-		dma_data->ops = samsung_dma_get_ops();
-
-	dma_data->ops->started(dma_data->channel);
-
 	return 0;
 }
 
-- 
1.8.3.2

[PATCH 4/4] ASoC: samsung: remove unused DMA data

[Arnd Bergmann]

The s3c_dma_client structures and the 'ch' and 'ops' members in
s3c_dma_params were only used by the legacy DMA driver and serve
no function any more. This removes any reference to them.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Sangbeom Kim <sbkim73@samsung.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: linux-samsung-soc at vger.kernel.org
---
 sound/soc/samsung/ac97.c        | 15 ---------------
 sound/soc/samsung/dma.h         |  7 -------
 sound/soc/samsung/i2s.c         |  6 ------
 sound/soc/samsung/pcm.c         | 12 ------------
 sound/soc/samsung/s3c-i2s-v2.c  |  2 --
 sound/soc/samsung/s3c2412-i2s.c |  4 ----
 sound/soc/samsung/s3c24xx-i2s.c |  4 ----
 sound/soc/samsung/spdif.c       |  5 -----
 8 files changed, 55 deletions(-)

diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
index 2aa24d052a4a..e1615113fd84 100644
--- a/sound/soc/samsung/ac97.c
+++ b/sound/soc/samsung/ac97.c
@@ -38,30 +38,15 @@ struct s3c_ac97_info {
 };
 static struct s3c_ac97_info s3c_ac97;
 
-static struct s3c_dma_client s3c_dma_client_out = {
-	.name = "AC97 PCMOut"
-};
-
-static struct s3c_dma_client s3c_dma_client_in = {
-	.name = "AC97 PCMIn"
-};
-
-static struct s3c_dma_client s3c_dma_client_micin = {
-	.name = "AC97 MicIn"
-};
-
 static struct s3c_dma_params s3c_ac97_pcm_out = {
-	.client		= &s3c_dma_client_out,
 	.dma_size	= 4,
 };
 
 static struct s3c_dma_params s3c_ac97_pcm_in = {
-	.client		= &s3c_dma_client_in,
 	.dma_size	= 4,
 };
 
 static struct s3c_dma_params s3c_ac97_mic_in = {
-	.client		= &s3c_dma_client_micin,
 	.dma_size	= 4,
 };
 
diff --git a/sound/soc/samsung/dma.h b/sound/soc/samsung/dma.h
index 070ab0f09609..0e85dcfec023 100644
--- a/sound/soc/samsung/dma.h
+++ b/sound/soc/samsung/dma.h
@@ -14,17 +14,10 @@
 
 #include <sound/dmaengine_pcm.h>
 
-struct s3c_dma_client {
-	char *name;
-};
-
 struct s3c_dma_params {
-	struct s3c_dma_client *client;	/* stream identifier */
 	int channel;				/* Channel ID */
 	dma_addr_t dma_addr;
 	int dma_size;			/* Size of the DMA transfer */
-	unsigned ch;
-	struct samsung_dma_ops *ops;
 	char *ch_name;
 	struct snd_dmaengine_dai_dma_data dma_data;
 };
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 5f9b255a8b38..db8b29a0d106 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -1212,11 +1212,7 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 
 	pri_dai->dma_playback.dma_addr = regs_base + I2STXD;
 	pri_dai->dma_capture.dma_addr = regs_base + I2SRXD;
-	pri_dai->dma_playback.client =
-		(struct s3c_dma_client *)&pri_dai->dma_playback;
 	pri_dai->dma_playback.ch_name = "tx";
-	pri_dai->dma_capture.client =
-		(struct s3c_dma_client *)&pri_dai->dma_capture;
 	pri_dai->dma_capture.ch_name = "rx";
 	pri_dai->dma_playback.dma_size = 4;
 	pri_dai->dma_capture.dma_size = 4;
@@ -1234,8 +1230,6 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 			goto err;
 		}
 		sec_dai->dma_playback.dma_addr = regs_base + I2STXDS;
-		sec_dai->dma_playback.client =
-			(struct s3c_dma_client *)&sec_dai->dma_playback;
 		sec_dai->dma_playback.ch_name = "tx-sec";
 
 		if (!np) {
diff --git a/sound/soc/samsung/pcm.c b/sound/soc/samsung/pcm.c
index 4c5f97fe45c8..bac034b15a27 100644
--- a/sound/soc/samsung/pcm.c
+++ b/sound/soc/samsung/pcm.c
@@ -131,32 +131,20 @@ struct s3c_pcm_info {
 	struct s3c_dma_params	*dma_capture;
 };
 
-static struct s3c_dma_client s3c_pcm_dma_client_out = {
-	.name		= "PCM Stereo out"
-};
-
-static struct s3c_dma_client s3c_pcm_dma_client_in = {
-	.name		= "PCM Stereo in"
-};
-
 static struct s3c_dma_params s3c_pcm_stereo_out[] = {
 	[0] = {
-		.client		= &s3c_pcm_dma_client_out,
 		.dma_size	= 4,
 	},
 	[1] = {
-		.client		= &s3c_pcm_dma_client_out,
 		.dma_size	= 4,
 	},
 };
 
 static struct s3c_dma_params s3c_pcm_stereo_in[] = {
 	[0] = {
-		.client		= &s3c_pcm_dma_client_in,
 		.dma_size	= 4,
 	},
 	[1] = {
-		.client		= &s3c_pcm_dma_client_in,
 		.dma_size	= 4,
 	},
 };
diff --git a/sound/soc/samsung/s3c-i2s-v2.c b/sound/soc/samsung/s3c-i2s-v2.c
index de6c321b8b68..df65c5b494b1 100644
--- a/sound/soc/samsung/s3c-i2s-v2.c
+++ b/sound/soc/samsung/s3c-i2s-v2.c
@@ -22,8 +22,6 @@
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
 
-#include <mach/dma.h>
-
 #include "regs-i2s-v2.h"
 #include "s3c-i2s-v2.h"
 #include "dma.h"
diff --git a/sound/soc/samsung/s3c2412-i2s.c b/sound/soc/samsung/s3c2412-i2s.c
index d6bc6dc0aafb..9180310e862a 100644
--- a/sound/soc/samsung/s3c2412-i2s.c
+++ b/sound/soc/samsung/s3c2412-i2s.c
@@ -34,16 +34,12 @@
 #include "s3c2412-i2s.h"
 
 static struct s3c_dma_params s3c2412_i2s_pcm_stereo_out = {
-	.client		=
-		(struct s3c_dma_client *)&s3c2412_i2s_pcm_stereo_out,
 	.channel	= DMACH_I2S_OUT,
 	.ch_name	= "tx",
 	.dma_size	= 4,
 };
 
 static struct s3c_dma_params s3c2412_i2s_pcm_stereo_in = {
-	.client		=
-		(struct s3c_dma_client *)&s3c2412_i2s_pcm_stereo_in,
 	.channel	= DMACH_I2S_IN,
 	.ch_name	= "rx",
 	.dma_size	= 4,
diff --git a/sound/soc/samsung/s3c24xx-i2s.c b/sound/soc/samsung/s3c24xx-i2s.c
index e8b98528e356..e87d9a2053b8 100644
--- a/sound/soc/samsung/s3c24xx-i2s.c
+++ b/sound/soc/samsung/s3c24xx-i2s.c
@@ -32,16 +32,12 @@
 #include "s3c24xx-i2s.h"
 
 static struct s3c_dma_params s3c24xx_i2s_pcm_stereo_out = {
-	.client		=
-		(struct s3c_dma_client *)&s3c24xx_i2s_pcm_stereo_out,
 	.channel	= DMACH_I2S_OUT,
 	.ch_name	= "tx",
 	.dma_size	= 2,
 };
 
 static struct s3c_dma_params s3c24xx_i2s_pcm_stereo_in = {
-	.client		=
-		(struct s3c_dma_client *)&s3c24xx_i2s_pcm_stereo_in,
 	.channel	= DMACH_I2S_IN,
 	.ch_name	= "rx",
 	.dma_size	= 2,
diff --git a/sound/soc/samsung/spdif.c b/sound/soc/samsung/spdif.c
index d9ffc48fce5e..d7d2e208f486 100644
--- a/sound/soc/samsung/spdif.c
+++ b/sound/soc/samsung/spdif.c
@@ -93,10 +93,6 @@ struct samsung_spdif_info {
 	struct s3c_dma_params	*dma_playback;
 };
 
-static struct s3c_dma_client spdif_dma_client_out = {
-	.name		= "S/PDIF Stereo out",
-};
-
 static struct s3c_dma_params spdif_stereo_out;
 static struct samsung_spdif_info spdif_info;
 
@@ -435,7 +431,6 @@ static int spdif_probe(struct platform_device *pdev)
 	}
 
 	spdif_stereo_out.dma_size = 2;
-	spdif_stereo_out.client = &spdif_dma_client_out;
 	spdif_stereo_out.dma_addr = mem_res->start + DATA_OUTBUF;
 	spdif_stereo_out.channel = dma_res->start;
 
-- 
1.8.3.2

[PATCH 0/4] ASoC: samsung updates from arm testing

[Mark Brown]

On Fri, Jul 11, 2014 at 03:45:03PM +0200, Arnd Bergmann wrote:

> Here are a few patches from my randconfig and multiplatform series.
> The smartq patch is a re-send of the one I screwed up the other day.
> the dependencies and dma data removal should be trivial, the
> dmaengine one probably needs some closer inspection.

Hrm, can you check what's going on with the series - I've got two copies
of patch 2 and an extra patch [PATCH] ASoC: s3c64xx/smartq: use dynamic
registration which is threaded before this cover letter and looks like a
resend of the prior copy with the platform_data header missing that got
included by mistake?  The patches look OK but I just want to make sure
I'm applying the right things.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140711/beecccc7/attachment-0001.sig>

[PATCH 0/4] ASoC: samsung updates from arm testing

[Arnd Bergmann]

On Friday 11 July 2014, Mark Brown wrote:
>   On Fri, Jul 11, 2014 at 03:45:03PM +0200, Arnd Bergmann wrote:
> 
> > Here are a few patches from my randconfig and multiplatform series.
> > The smartq patch is a re-send of the one I screwed up the other day.
> > the dependencies and dma data removal should be trivial, the
> > dmaengine one probably needs some closer inspection.
> 
> Hrm, can you check what's going on with the series - I've got two copies
> of patch 2 and an extra patch [PATCH] ASoC: s3c64xx/smartq: use dynamic
> registration which is threaded before this cover letter and looks like a
> resend of the prior copy with the platform_data header missing that got
> included by mistake?  The patches look OK but I just want to make sure
> I'm applying the right things.

It seems I accidentally left the old version of the patch in the directory
I used for sending the patches, so git-send-email was sending both the
old and the new version. Please just disregard the patch named "[PATCH]
ASoC: s3c64xx/smartq: use dynamic registration", the other ones are
those I wanted to send.

	Arnd

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/11/2014 03:45 PM, Arnd Bergmann wrote:
> As a prerequisite for moving s3c64xx into multiplatform configurations,
> we need to change the smartq audio driver to stop using hardcoded
> gpio numbers from the header file, and instead pass the gpio data
> through platform_data.

This should be using the gpiod API. The gpiod API allows you to pass the GPIOs 
without having to add a platform_data struct.

- Lars

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Sat, Jul 12, 2014 at 05:27:59PM +0200, Lars-Peter Clausen wrote:
> On 07/11/2014 03:45 PM, Arnd Bergmann wrote:

> >As a prerequisite for moving s3c64xx into multiplatform configurations,
> >we need to change the smartq audio driver to stop using hardcoded
> >gpio numbers from the header file, and instead pass the gpio data
> >through platform_data.

> This should be using the gpiod API. The gpiod API allows you to pass the
> GPIOs without having to add a platform_data struct.

OTOH that's a more invasive change that's harder to do mechanically -
I'm not sure it's sensible to insist on someone doing it for generic
cleanups (rather than actively working with the particular platform).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140712/1a08ede5/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/12/2014 09:49 PM, Mark Brown wrote:
> On Sat, Jul 12, 2014 at 05:27:59PM +0200, Lars-Peter Clausen wrote:
>> On 07/11/2014 03:45 PM, Arnd Bergmann wrote:
>
>>> As a prerequisite for moving s3c64xx into multiplatform configurations,
>>> we need to change the smartq audio driver to stop using hardcoded
>>> gpio numbers from the header file, and instead pass the gpio data
>>> through platform_data.
>
>> This should be using the gpiod API. The gpiod API allows you to pass the
>> GPIOs without having to add a platform_data struct.
>
> OTOH that's a more invasive change that's harder to do mechanically -
> I'm not sure it's sensible to insist on someone doing it for generic
> cleanups (rather than actively working with the particular platform).

I don't think it is more invasive than using platform data. I did the same 
recently for jz4740 qi-lb60[1] and the changes in the patch are fairly trivial. 
The non-descriptor API is deprecated, so this even if this patch is applied as 
is sooner or later somebody will mechanically convert it to the descriptor API.

- Lars

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=218e18a3728507ee82ed2eb10c789671a00e34bd

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

On Sunday 13 July 2014 15:36:52 Lars-Peter Clausen wrote:
> On 07/12/2014 09:49 PM, Mark Brown wrote:
> > On Sat, Jul 12, 2014 at 05:27:59PM +0200, Lars-Peter Clausen wrote:
> >> On 07/11/2014 03:45 PM, Arnd Bergmann wrote:
> >
> >>> As a prerequisite for moving s3c64xx into multiplatform configurations,
> >>> we need to change the smartq audio driver to stop using hardcoded
> >>> gpio numbers from the header file, and instead pass the gpio data
> >>> through platform_data.
> >
> >> This should be using the gpiod API. The gpiod API allows you to pass the
> >> GPIOs without having to add a platform_data struct.
> >
> > OTOH that's a more invasive change that's harder to do mechanically -
> > I'm not sure it's sensible to insist on someone doing it for generic
> > cleanups (rather than actively working with the particular platform).
> 
> I don't think it is more invasive than using platform data. I did the same 
> recently for jz4740 qi-lb60[1] and the changes in the patch are fairly trivial. 
> The non-descriptor API is deprecated, so this even if this patch is applied as 
> is sooner or later somebody will mechanically convert it to the descriptor API.

I've given it a try now. Can you confirm that this is a valid transformation?

If it's ok, I'll fold it into the original patch and re-send.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

 ./arch/arm/mach-s3c64xx/mach-smartq.c           |   15 ++++++++-------
 ./sound/soc/samsung/smartq_wm8987.c             |   19 +++++++------------
 ./include/linux/platform_data/asoc-s3c-smartq.h |   10 ----------
 3 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
index 4c111f60dbf2..dce449c0e855 100644
--- a/arch/arm/mach-s3c64xx/mach-smartq.c
+++ b/arch/arm/mach-s3c64xx/mach-smartq.c
@@ -20,7 +20,6 @@
 #include <linux/spi/spi_gpio.h>
 #include <linux/usb/gpio_vbus.h>
 #include <linux/platform_data/s3c-hsotg.h>
-#include <linux/platform_data/asoc-s3c-smartq.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -380,9 +379,12 @@ void __init smartq_map_io(void)
 	smartq_lcd_mode_set();
 }
 
-static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
-	.gpio_jack = S3C64XX_GPL(12),
-	.gpio_amp = S3C64XX_GPK(12),
+static struct gpiod_lookup_table smartq_audio_gpios = {
+	.dev_id = "smartq-audio",
+	.table = {
+		GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
+		GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
+	},
 };
 
 void __init smartq_machine_init(void)
@@ -404,7 +406,6 @@ void __init smartq_machine_init(void)
 
 	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
 
-	platform_device_register_data(NULL, "smartq-audio", -1,
-				      &smartq_audio_pdata,
-				      sizeof (smartq_audio_pdata));
+	platform_device_register_simple("smartq-audio", -1, NULL, 0);
+	gpiod_add_lookup_table(&smartq_audio_gpios);
 }
diff --git a/include/linux/platform_data/asoc-s3c-smartq.h b/include/linux/platform_data/asoc-s3c-smartq.h
deleted file mode 100644
index 5bddc3586802..000000000000
--- a/include/linux/platform_data/asoc-s3c-smartq.h
+++ /dev/null
@@ -1,10 +0,0 @@
-#ifndef __PLATFORM_DATA_ASOC_S3C_SMARTQ
-#define __PLATFORM_DATA_ASOC_S3C_SMARTQ
-
-struct smartq_audio_pdata {
-	int gpio_jack;
-	int gpio_amp;
-};
-
-#endif
-
diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
index 8c4a0a285ce7..8da7c67903c6 100644
--- a/sound/soc/samsung/smartq_wm8987.c
+++ b/sound/soc/samsung/smartq_wm8987.c
@@ -19,8 +19,6 @@
 #include <sound/soc.h>
 #include <sound/jack.h>
 
-#include <linux/platform_data/asoc-s3c-smartq.h>
-
 #include "i2s.h"
 #include "../codecs/wm8750.h"
 
@@ -126,10 +124,9 @@ static int smartq_speaker_event(struct snd_soc_dapm_widget *w,
 				struct snd_kcontrol *k,
 				int event)
 {
-	struct smartq_audio_pdata *pdata;
-	pdata = snd_soc_smartq.dev->platform_data;
+	struct gpio_desc *gpio = dev_get_drvdata(snd_soc_smartq.dev);
 
-	gpio_set_value(pdata->gpio_amp, SND_SOC_DAPM_EVENT_OFF(event));
+	gpiod_set_value(gpio, SND_SOC_DAPM_EVENT_OFF(event));
 
 	return 0;
 }
@@ -222,21 +219,19 @@ static struct snd_soc_card snd_soc_smartq = {
 
 static int smartq_probe(struct platform_device *pdev)
 {
-	struct smartq_audio_pdata *pdata = pdev->dev.platform_data;
+	struct gpio_desc *gpio;
 	int ret;
 
 	platform_set_drvdata(pdev, &snd_soc_smartq);
 
 	/* Initialise GPIOs used by amplifiers */
-	ret = devm_gpio_request_one(&pdev->dev, pdata->gpio_amp,
-				    GPIOF_DIR_OUT | GPIOF_INIT_HIGH,
-				    "amplifiers shutdown");
-	if (ret) {
+	gpio = devm_gpiod_get(&pdev->dev, "amplifiers shutdown");
+	if (IS_ERR(gpio)) {
 		dev_err(&pdev->dev, "Failed to register GPK12\n");
+		ret = PTR_ERR(gpio);
 		goto out;
 	}
-
-	smartq_jack_gpios[0].gpio = pdata->gpio_jack;
+	platform_set_drvdata(pdev, gpio);
 
 	ret = devm_snd_soc_register_card(&pdev->dev, &snd_soc_smartq);
 	if (ret)

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

[...]
> I've given it a try now. Can you confirm that this is a valid transformation?
>
> If it's ok, I'll fold it into the original patch and re-send.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>   ./arch/arm/mach-s3c64xx/mach-smartq.c           |   15 ++++++++-------
>   ./sound/soc/samsung/smartq_wm8987.c             |   19 +++++++------------
>   ./include/linux/platform_data/asoc-s3c-smartq.h |   10 ----------
>   3 files changed, 15 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
> index 4c111f60dbf2..dce449c0e855 100644
> --- a/arch/arm/mach-s3c64xx/mach-smartq.c
> +++ b/arch/arm/mach-s3c64xx/mach-smartq.c
> @@ -20,7 +20,6 @@
>   #include <linux/spi/spi_gpio.h>
>   #include <linux/usb/gpio_vbus.h>
>   #include <linux/platform_data/s3c-hsotg.h>
> -#include <linux/platform_data/asoc-s3c-smartq.h>

#include <linux/gpio/driver.h>

>
>   #include <asm/mach-types.h>
>   #include <asm/mach/map.h>
> @@ -380,9 +379,12 @@ void __init smartq_map_io(void)
>   	smartq_lcd_mode_set();
>   }
>
> -static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
> -	.gpio_jack = S3C64XX_GPL(12),
> -	.gpio_amp = S3C64XX_GPK(12),
> +static struct gpiod_lookup_table smartq_audio_gpios = {
> +	.dev_id = "smartq-audio",
> +	.table = {
> +		GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
> +		GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),

There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.


This needs a { } terminating entry.

> +	},
>   };
>
>   void __init smartq_machine_init(void)
> @@ -404,7 +406,6 @@ void __init smartq_machine_init(void)
>
>   	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
>
> -	platform_device_register_data(NULL, "smartq-audio", -1,
> -				      &smartq_audio_pdata,
> -				      sizeof (smartq_audio_pdata));
> +	platform_device_register_simple("smartq-audio", -1, NULL, 0);
> +	gpiod_add_lookup_table(&smartq_audio_gpios);

To avoid a extra round of -EPROBE_DEFER the lookup table should be 
registered before the device.

>   }
[...]
> diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
> index 8c4a0a285ce7..8da7c67903c6 100644
> --- a/sound/soc/samsung/smartq_wm8987.c
> +++ b/sound/soc/samsung/smartq_wm8987.c
> @@ -19,8 +19,6 @@
>   #include <sound/soc.h>
>   #include <sound/jack.h>

#include <linux/gpio/consumer.h>

>
[..]
> -
> -	smartq_jack_gpios[0].gpio = pdata->gpio_jack;

smartq_jack_gpios[0].gpiod_dev = &pdev->dev;

> +	platform_set_drvdata(pdev, gpio);

This unfortunately wont work. snd_soc_register_card() will overwrite the 
devices drvdata. Use snd_soc_card_set_drvdata(card, gpio) and in the speaker 
event handler snd_soc_card_get_drvdata(w->dapm->card);

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

On Monday 14 July 2014 14:40:42 Lars-Peter Clausen wrote:
> [...]
> > I've given it a try now. Can you confirm that this is a valid transformation?
> >
> > If it's ok, I'll fold it into the original patch and re-send.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> >   ./arch/arm/mach-s3c64xx/mach-smartq.c           |   15 ++++++++-------
> >   ./sound/soc/samsung/smartq_wm8987.c             |   19 +++++++------------
> >   ./include/linux/platform_data/asoc-s3c-smartq.h |   10 ----------
> >   3 files changed, 15 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/arm/mach-s3c64xx/mach-smartq.c b/arch/arm/mach-s3c64xx/mach-smartq.c
> > index 4c111f60dbf2..dce449c0e855 100644
> > --- a/arch/arm/mach-s3c64xx/mach-smartq.c
> > +++ b/arch/arm/mach-s3c64xx/mach-smartq.c
> > @@ -20,7 +20,6 @@
> >   #include <linux/spi/spi_gpio.h>
> >   #include <linux/usb/gpio_vbus.h>
> >   #include <linux/platform_data/s3c-hsotg.h>
> > -#include <linux/platform_data/asoc-s3c-smartq.h>
> 
> #include <linux/gpio/driver.h>

ok.

> >
> >   #include <asm/mach-types.h>
> >   #include <asm/mach/map.h>
> > @@ -380,9 +379,12 @@ void __init smartq_map_io(void)
> >   	smartq_lcd_mode_set();
> >   }
> >
> > -static const __initconst struct smartq_audio_pdata smartq_audio_pdata = {
> > -	.gpio_jack = S3C64XX_GPL(12),
> > -	.gpio_amp = S3C64XX_GPK(12),
> > +static struct gpiod_lookup_table smartq_audio_gpios = {
> > +	.dev_id = "smartq-audio",
> > +	.table = {
> > +		GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
> > +		GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
> 
> There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.

The original driver does gpio_direction_output(..., 1); 

For some reason I earlier concluded that this was what the '1' would need
to get converted to. Are you sure '0' is correct then?

> This needs a { } terminating entry.

ok

> > +	},
> >   };
> >
> >   void __init smartq_machine_init(void)
> > @@ -404,7 +406,6 @@ void __init smartq_machine_init(void)
> >
> >   	platform_add_devices(smartq_devices, ARRAY_SIZE(smartq_devices));
> >
> > -	platform_device_register_data(NULL, "smartq-audio", -1,
> > -				      &smartq_audio_pdata,
> > -				      sizeof (smartq_audio_pdata));
> > +	platform_device_register_simple("smartq-audio", -1, NULL, 0);
> > +	gpiod_add_lookup_table(&smartq_audio_gpios);
> 
> To avoid a extra round of -EPROBE_DEFER the lookup table should be 
> registered before the device.

ok

> >   }
> [...]
> > diff --git a/sound/soc/samsung/smartq_wm8987.c b/sound/soc/samsung/smartq_wm8987.c
> > index 8c4a0a285ce7..8da7c67903c6 100644
> > --- a/sound/soc/samsung/smartq_wm8987.c
> > +++ b/sound/soc/samsung/smartq_wm8987.c
> > @@ -19,8 +19,6 @@
> >   #include <sound/soc.h>
> >   #include <sound/jack.h>
> 
> #include <linux/gpio/consumer.h>
> 
> >
> [..]
> > -
> > -	smartq_jack_gpios[0].gpio = pdata->gpio_jack;
> 
> smartq_jack_gpios[0].gpiod_dev = &pdev->dev;
> 
> > +	platform_set_drvdata(pdev, gpio);
> 
> This unfortunately wont work. snd_soc_register_card() will overwrite the 
> devices drvdata. Use snd_soc_card_set_drvdata(card, gpio) and in the speaker 
> event handler snd_soc_card_get_drvdata(w->dapm->card);

ok.

Thanks for the review!

	Arnd

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Sun, Jul 13, 2014 at 03:36:52PM +0200, Lars-Peter Clausen wrote:
> On 07/12/2014 09:49 PM, Mark Brown wrote:

> >OTOH that's a more invasive change that's harder to do mechanically -
> >I'm not sure it's sensible to insist on someone doing it for generic
> >cleanups (rather than actively working with the particular platform).

> I don't think it is more invasive than using platform data. I did the same
> recently for jz4740 qi-lb60[1] and the changes in the patch are fairly
> trivial. The non-descriptor API is deprecated, so this even if this patch is
> applied as is sooner or later somebody will mechanically convert it to the
> descriptor API.

> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=218e18a3728507ee82ed2eb10c789671a00e34bd

The translation into the GPIO_LOOKUP() macros isn't sufficiently direct,
with platform data it's just a case of putting the same data in a
different file but with this you need to figure out what the GPIO bank
is called which won't appear in the diff typically.  Someone will
doubtless want to finish up the gpiod_ transition at some point but it's
not yet at the point where it's worth blocking unrelated cleanups for
it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140714/188c61c2/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/14/2014 05:46 PM, Arnd Bergmann wrote:
[...]
>>> +static struct gpiod_lookup_table smartq_audio_gpios = {
>>> +	.dev_id = "smartq-audio",
>>> +	.table = {
>>> +		GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
>>> +		GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
>>
>> There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.
>
> The original driver does gpio_direction_output(..., 1);
>
> For some reason I earlier concluded that this was what the '1' would need
> to get converted to. Are you sure '0' is correct then?
>

Yes. But now that you say it the gpiod_direction_output() call is missing 
from this patch.

- Lars

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> On 07/14/2014 05:46 PM, Arnd Bergmann wrote:
> [...]
> >>> +static struct gpiod_lookup_table smartq_audio_gpios = {
> >>> +   .dev_id = "smartq-audio",
> >>> +   .table = {
> >>> +           GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
> >>> +           GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
> >>
> >> There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.
> >
> > The original driver does gpio_direction_output(..., 1);
> >
> > For some reason I earlier concluded that this was what the '1' would need
> > to get converted to. Are you sure '0' is correct then?
> >
> 
> Yes. But now that you say it the gpiod_direction_output() call is missing 
> from this patch.

I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
and as Linus Walleij explained to me the other day, the lookup is supposed
to replace devm_gpio_request_one(), which in turn replaced both the
gpio_request and the gpio_direction_output(). Do I need to put the
gpiod_direction_output() back or is there another interface for that when
registering the board gpios?

	Arnd

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/14/2014 08:23 PM, Arnd Bergmann wrote:
> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>> On 07/14/2014 05:46 PM, Arnd Bergmann wrote:
>> [...]
>>>>> +static struct gpiod_lookup_table smartq_audio_gpios = {
>>>>> +   .dev_id = "smartq-audio",
>>>>> +   .table = {
>>>>> +           GPIO_LOOKUP("GPL", 12, "headphone detect", 0),
>>>>> +           GPIO_LOOKUP("GPK", 12, "amplifiers shutdown", GPIO_ACTIVE_HIGH),
>>>>
>>>> There is no such thing as GPIO_ACTIVE_HIGH, just 0 for flags.
>>>
>>> The original driver does gpio_direction_output(..., 1);
>>>
>>> For some reason I earlier concluded that this was what the '1' would need
>>> to get converted to. Are you sure '0' is correct then?
>>>
>>
>> Yes. But now that you say it the gpiod_direction_output() call is missing
>> from this patch.
>
> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
> and as Linus Walleij explained to me the other day, the lookup is supposed
> to replace devm_gpio_request_one(), which in turn replaced both the
> gpio_request and the gpio_direction_output(). Do I need to put the
> gpiod_direction_output() back or is there another interface for that when
> registering the board gpios?

Hm, ok looks like there is a GPIO_ACTIVE_HIGH now, but its value 0. But it does 
not change the direction or set the initial output value. But maybe it is planed.

- Lars

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:

> > Yes. But now that you say it the gpiod_direction_output() call is missing 
> > from this patch.

> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
> and as Linus Walleij explained to me the other day, the lookup is supposed
> to replace devm_gpio_request_one(), which in turn replaced both the
> gpio_request and the gpio_direction_output(). Do I need to put the
> gpiod_direction_output() back or is there another interface for that when
> registering the board gpios?

Indeed.  If you *do* need an explicit _output() then that sounds to me
like we either need a gpiod_get_one() or an extension to the table,
looking at the code it seems like this is indeed the case.  We can set
if the GPIO is active high/low, or open source/drain but there's no flag
for the initial state.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140714/e30fb34a/attachment.sig>

[PATCH 1/4] ASoC: samsung: add explicit i2c/spi dependencies

[Mark Brown]

On Fri, Jul 11, 2014 at 03:45:04PM +0200, Arnd Bergmann wrote:
> I got another build error from SND_SOC_SMARTQ selecting
> SND_SOC_WM8750 without ensuring that I2C is enabled,
> in this example, i2c is a loadable module:

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140714/f16b302c/attachment-0001.sig>

[PATCH 3/4] ASoC: samsung: s3c24xx dmaengine follow-up

[Mark Brown]

On Fri, Jul 11, 2014 at 03:45:07PM +0200, Arnd Bergmann wrote:
> Commit ae602456e83c92 ("ASoC: samsung: drop support for legacy
> S3C24XX DMA API") removed the old code for the samsung specific
> DMA interfaces, now that everybody can use dmaengine.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140714/7fffcac1/attachment.sig>

[PATCH 4/4] ASoC: samsung: remove unused DMA data

[Mark Brown]

On Fri, Jul 11, 2014 at 03:45:08PM +0200, Arnd Bergmann wrote:
> The s3c_dma_client structures and the 'ch' and 'ops' members in
> s3c_dma_params were only used by the legacy DMA driver and serve
> no function any more. This removes any reference to them.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140714/1d2de040/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Arnd Bergmann]

On Monday 14 July 2014 19:36:24 Mark Brown wrote:
> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> > On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> 
> > > Yes. But now that you say it the gpiod_direction_output() call is missing 
> > > from this patch.
> 
> > I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
> > and as Linus Walleij explained to me the other day, the lookup is supposed
> > to replace devm_gpio_request_one(), which in turn replaced both the
> > gpio_request and the gpio_direction_output(). Do I need to put the
> > gpiod_direction_output() back or is there another interface for that when
> > registering the board gpios?
> 
> Indeed.  If you *do* need an explicit _output() then that sounds to me
> like we either need a gpiod_get_one() or an extension to the table,
> looking at the code it seems like this is indeed the case.  We can set
> if the GPIO is active high/low, or open source/drain but there's no flag
> for the initial state.

(adding Alexandre and the gpio list)

GPIO people: any guidance on how a board file should set a gpio to
output/default-high in a GPIO_LOOKUP() table to replace a
devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
Do we need to add an interface extension to do this, e.g. passing
GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?

	Arnd

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>> > On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>
>> > > Yes. But now that you say it the gpiod_direction_output() call is missing
>> > > from this patch.
>>
>> > I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
>> > and as Linus Walleij explained to me the other day, the lookup is supposed
>> > to replace devm_gpio_request_one(), which in turn replaced both the
>> > gpio_request and the gpio_direction_output(). Do I need to put the
>> > gpiod_direction_output() back or is there another interface for that when
>> > registering the board gpios?
>>
>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>> like we either need a gpiod_get_one() or an extension to the table,
>> looking at the code it seems like this is indeed the case.  We can set
>> if the GPIO is active high/low, or open source/drain but there's no flag
>> for the initial state.
>
> (adding Alexandre and the gpio list)
>
> GPIO people: any guidance on how a board file should set a gpio to
> output/default-high in a GPIO_LOOKUP() table to replace a
> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
> Do we need to add an interface extension to do this, e.g. passing
> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?

The way I see it, GPIO mappings (whether they are done using the
lookup tables, DT, or ACPI) should only care about details that are
relevant to the device layout and that should be abstracted to the
driver (e.g. whether the GPIO is active low or open drain) so drivers
do not need to check X conditions every time they want to drive the
GPIO.

Direction and initial value, on the other hand, are clearly properties
that ought to be set by the driver itself. Thus my expectation here
would be that the driver sets the GPIO direction and initial value as
soon as it gets it using gpiod_direction_output(). In other words,
there is no replacement for gpio_request_one() with the gpiod
interface. Is there any use-case that cannot be covered by calling
gpiod_direction_output() right after gpiod_get()? AFAICT this is what
gpio_request_one() was doing anyway.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>>
>>>>> Yes. But now that you say it the gpiod_direction_output() call is missing
>>>>> from this patch.
>>>
>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from Documentation/gpio/board.txt
>>>> and as Linus Walleij explained to me the other day, the lookup is supposed
>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>>>> gpiod_direction_output() back or is there another interface for that when
>>>> registering the board gpios?
>>>
>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>>> like we either need a gpiod_get_one() or an extension to the table,
>>> looking at the code it seems like this is indeed the case.  We can set
>>> if the GPIO is active high/low, or open source/drain but there's no flag
>>> for the initial state.
>>
>> (adding Alexandre and the gpio list)
>>
>> GPIO people: any guidance on how a board file should set a gpio to
>> output/default-high in a GPIO_LOOKUP() table to replace a
>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>> Do we need to add an interface extension to do this, e.g. passing
>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>
> The way I see it, GPIO mappings (whether they are done using the
> lookup tables, DT, or ACPI) should only care about details that are
> relevant to the device layout and that should be abstracted to the
> driver (e.g. whether the GPIO is active low or open drain) so drivers
> do not need to check X conditions every time they want to drive the
> GPIO.
>
> Direction and initial value, on the other hand, are clearly properties
> that ought to be set by the driver itself. Thus my expectation here
> would be that the driver sets the GPIO direction and initial value as
> soon as it gets it using gpiod_direction_output(). In other words,
> there is no replacement for gpio_request_one() with the gpiod
> interface. Is there any use-case that cannot be covered by calling
> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
> gpio_request_one() was doing anyway.

I agree with you that this is something that should be done in the driver 
and not in the lookup table. I think that it is still a good idea to have a 
replacement for gpio_request_one with the new GPIO descriptor API. A large 
share of the drivers want to call either gpio_direction_input() or 
gpio_direction_output() right after requesting the GPIO. Combining both the 
requesting and the configuration of the GPIO into one function call makes 
the code a bit shorter and also simplifies the error handling. Even more so 
if e.g. the GPIO is optional. This was one of the main reasons why 
gpio_request_one was introduced, see the commit[1] that added it.

- Lars

[1] 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3e45f1d1155894e6f4291f5536b224874d52d8e2

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>>
>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>>>
>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>>>>
>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>>>
>>>>
>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
>>>>>> missing
>>>>>> from this patch.
>>>>
>>>>
>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>>>>> Documentation/gpio/board.txt
>>>>> and as Linus Walleij explained to me the other day, the lookup is
>>>>> supposed
>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>>>>> gpiod_direction_output() back or is there another interface for that
>>>>> when
>>>>> registering the board gpios?
>>>>
>>>>
>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>>>> like we either need a gpiod_get_one() or an extension to the table,
>>>> looking at the code it seems like this is indeed the case.  We can set
>>>> if the GPIO is active high/low, or open source/drain but there's no flag
>>>> for the initial state.
>>>
>>>
>>> (adding Alexandre and the gpio list)
>>>
>>> GPIO people: any guidance on how a board file should set a gpio to
>>> output/default-high in a GPIO_LOOKUP() table to replace a
>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>>> Do we need to add an interface extension to do this, e.g. passing
>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>>
>>
>> The way I see it, GPIO mappings (whether they are done using the
>> lookup tables, DT, or ACPI) should only care about details that are
>> relevant to the device layout and that should be abstracted to the
>> driver (e.g. whether the GPIO is active low or open drain) so drivers
>> do not need to check X conditions every time they want to drive the
>> GPIO.
>>
>> Direction and initial value, on the other hand, are clearly properties
>> that ought to be set by the driver itself. Thus my expectation here
>> would be that the driver sets the GPIO direction and initial value as
>> soon as it gets it using gpiod_direction_output(). In other words,
>> there is no replacement for gpio_request_one() with the gpiod
>> interface. Is there any use-case that cannot be covered by calling
>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
>> gpio_request_one() was doing anyway.
>
>
> I agree with you that this is something that should be done in the driver
> and not in the lookup table. I think that it is still a good idea to have a
> replacement for gpio_request_one with the new GPIO descriptor API. A large
> share of the drivers want to call either gpio_direction_input() or
> gpio_direction_output() right after requesting the GPIO. Combining both the
> requesting and the configuration of the GPIO into one function call makes
> the code a bit shorter and also simplifies the error handling. Even more so
> if e.g. the GPIO is optional. This was one of the main reasons why
> gpio_request_one was introduced, see the commit[1] that added it.

I am not opposed to it as a convenience function. Note that since the
open-source and open-drain flags are already handled by the lookup
table, the only flags it should handle are those related to direction,
value, and (maybe) sysfs export.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Tue, Jul 15, 2014 at 04:36:20PM +0900, Alexandre Courbot wrote:

> Direction and initial value, on the other hand, are clearly properties
> that ought to be set by the driver itself. Thus my expectation here
> would be that the driver sets the GPIO direction and initial value as
> soon as it gets it using gpiod_direction_output(). In other words,

Well, it's a bit of a mix really - from a hardware point of view they
normally are actually often fixed.

> there is no replacement for gpio_request_one() with the gpiod
> interface. Is there any use-case that cannot be covered by calling
> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
> gpio_request_one() was doing anyway.

Right, the original goal was to provide a better interface for
requesting a GPIO - it saves a lot of error handling code throughout the
kernel and avoids the possibility of bugs due to users forgetting to
mark the directio for the GPIO.  Having a direct replacement also makes
conversions to gpiod easier.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140715/ceaf2154/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>>>
>>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>
>>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>>>>
>>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>>>>>
>>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>>>>
>>>>>
>>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
>>>>>>> missing
>>>>>>> from this patch.
>>>>>
>>>>>
>>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>>>>>> Documentation/gpio/board.txt
>>>>>> and as Linus Walleij explained to me the other day, the lookup is
>>>>>> supposed
>>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>>>>>> gpiod_direction_output() back or is there another interface for that
>>>>>> when
>>>>>> registering the board gpios?
>>>>>
>>>>>
>>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>>>>> like we either need a gpiod_get_one() or an extension to the table,
>>>>> looking at the code it seems like this is indeed the case.  We can set
>>>>> if the GPIO is active high/low, or open source/drain but there's no flag
>>>>> for the initial state.
>>>>
>>>>
>>>> (adding Alexandre and the gpio list)
>>>>
>>>> GPIO people: any guidance on how a board file should set a gpio to
>>>> output/default-high in a GPIO_LOOKUP() table to replace a
>>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>>>> Do we need to add an interface extension to do this, e.g. passing
>>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>>>
>>>
>>> The way I see it, GPIO mappings (whether they are done using the
>>> lookup tables, DT, or ACPI) should only care about details that are
>>> relevant to the device layout and that should be abstracted to the
>>> driver (e.g. whether the GPIO is active low or open drain) so drivers
>>> do not need to check X conditions every time they want to drive the
>>> GPIO.
>>>
>>> Direction and initial value, on the other hand, are clearly properties
>>> that ought to be set by the driver itself. Thus my expectation here
>>> would be that the driver sets the GPIO direction and initial value as
>>> soon as it gets it using gpiod_direction_output(). In other words,
>>> there is no replacement for gpio_request_one() with the gpiod
>>> interface. Is there any use-case that cannot be covered by calling
>>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
>>> gpio_request_one() was doing anyway.
>>
>>
>> I agree with you that this is something that should be done in the driver
>> and not in the lookup table. I think that it is still a good idea to have a
>> replacement for gpio_request_one with the new GPIO descriptor API. A large
>> share of the drivers want to call either gpio_direction_input() or
>> gpio_direction_output() right after requesting the GPIO. Combining both the
>> requesting and the configuration of the GPIO into one function call makes
>> the code a bit shorter and also simplifies the error handling. Even more so
>> if e.g. the GPIO is optional. This was one of the main reasons why
>> gpio_request_one was introduced, see the commit[1] that added it.
>
> I am not opposed to it as a convenience function. Note that since the
> open-source and open-drain flags are already handled by the lookup
> table, the only flags it should handle are those related to direction,
> value, and (maybe) sysfs export.

Problem is, too much convenience functions seems to ultimately kill convenience.

The canonical way to request a GPIO is by providing a (device,
function, index) triplet to gpiod_get_index(). Since most functions
only need one GPIO, we have gpiod_get(device, function) which is
basically an alias to gpiod_get_index(device, function, 0) (note to
self: we should probably inline it).

On top of these comes another set of convenience functions,
gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
instead of -ENOENT if the requested GPIO mapping does not exist. This
is useful for the common case where a driver can work without a GPIO.

Of course these functions all have devm counterparts, so we currently
have 8 (devm_)gpiod_get(_index)(_optional) functions.

If we are to add functions with an init flags parameter, we will end
with 16 functions. That starts to be a bit too much to my taste, and
maybe that's where GPIO consumers should sacrifice some convenience to
preserve a comprehensible GPIO API.

There might be other ways to work around this though. For instance, we
could replace the _optional functions by a GPIOF_OPTIONAL flag to be
passed to a more generic function that would also accept direction and
init value flags. Actually I am not seeing any user of the _optional
variant in -next, so maybe we should just do this. Thierry, since you
introduced the _optional functions, can we get your thoughts about
this?

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Thierry Reding]

On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> >>>
> >>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>>
> >>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
> >>>>>
> >>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> >>>>>>
> >>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> >>>>>
> >>>>>
> >>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
> >>>>>>> missing
> >>>>>>> from this patch.
> >>>>>
> >>>>>
> >>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
> >>>>>> Documentation/gpio/board.txt
> >>>>>> and as Linus Walleij explained to me the other day, the lookup is
> >>>>>> supposed
> >>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
> >>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
> >>>>>> gpiod_direction_output() back or is there another interface for that
> >>>>>> when
> >>>>>> registering the board gpios?
> >>>>>
> >>>>>
> >>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
> >>>>> like we either need a gpiod_get_one() or an extension to the table,
> >>>>> looking at the code it seems like this is indeed the case.  We can set
> >>>>> if the GPIO is active high/low, or open source/drain but there's no flag
> >>>>> for the initial state.
> >>>>
> >>>>
> >>>> (adding Alexandre and the gpio list)
> >>>>
> >>>> GPIO people: any guidance on how a board file should set a gpio to
> >>>> output/default-high in a GPIO_LOOKUP() table to replace a
> >>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
> >>>> Do we need to add an interface extension to do this, e.g. passing
> >>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
> >>>
> >>>
> >>> The way I see it, GPIO mappings (whether they are done using the
> >>> lookup tables, DT, or ACPI) should only care about details that are
> >>> relevant to the device layout and that should be abstracted to the
> >>> driver (e.g. whether the GPIO is active low or open drain) so drivers
> >>> do not need to check X conditions every time they want to drive the
> >>> GPIO.
> >>>
> >>> Direction and initial value, on the other hand, are clearly properties
> >>> that ought to be set by the driver itself. Thus my expectation here
> >>> would be that the driver sets the GPIO direction and initial value as
> >>> soon as it gets it using gpiod_direction_output(). In other words,
> >>> there is no replacement for gpio_request_one() with the gpiod
> >>> interface. Is there any use-case that cannot be covered by calling
> >>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
> >>> gpio_request_one() was doing anyway.
> >>
> >>
> >> I agree with you that this is something that should be done in the driver
> >> and not in the lookup table. I think that it is still a good idea to have a
> >> replacement for gpio_request_one with the new GPIO descriptor API. A large
> >> share of the drivers want to call either gpio_direction_input() or
> >> gpio_direction_output() right after requesting the GPIO. Combining both the
> >> requesting and the configuration of the GPIO into one function call makes
> >> the code a bit shorter and also simplifies the error handling. Even more so
> >> if e.g. the GPIO is optional. This was one of the main reasons why
> >> gpio_request_one was introduced, see the commit[1] that added it.
> >
> > I am not opposed to it as a convenience function. Note that since the
> > open-source and open-drain flags are already handled by the lookup
> > table, the only flags it should handle are those related to direction,
> > value, and (maybe) sysfs export.
> 
> Problem is, too much convenience functions seems to ultimately kill convenience.
> 
> The canonical way to request a GPIO is by providing a (device,
> function, index) triplet to gpiod_get_index(). Since most functions
> only need one GPIO, we have gpiod_get(device, function) which is
> basically an alias to gpiod_get_index(device, function, 0) (note to
> self: we should probably inline it).
> 
> On top of these comes another set of convenience functions,
> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
> instead of -ENOENT if the requested GPIO mapping does not exist. This
> is useful for the common case where a driver can work without a GPIO.
> 
> Of course these functions all have devm counterparts, so we currently
> have 8 (devm_)gpiod_get(_index)(_optional) functions.
> 
> If we are to add functions with an init flags parameter, we will end
> with 16 functions. That starts to be a bit too much to my taste, and
> maybe that's where GPIO consumers should sacrifice some convenience to
> preserve a comprehensible GPIO API.
> 
> There might be other ways to work around this though. For instance, we
> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
> passed to a more generic function that would also accept direction and
> init value flags. Actually I am not seeing any user of the _optional
> variant in -next, so maybe we should just do this. Thierry, since you
> introduced the _optional functions, can we get your thoughts about
> this?

I personally prefer explicit naming of the functions rather than putting
a bunch of flags into some parameter. If you're overly concerned about
the amount of convenience functions, perhaps the _index variants can be
left out for gpiod_get_one(). I'd argue that if drivers want to deal
with that level of detail anyway, they may just as well add the index
explicitly when calling the function.

While we're at it, gpiod_get_one() doesn't sound like a very good name.
All other variants only request "one" as well. Perhaps something like
gpiod_get_with_flags() would be a better name.

Then again, maybe rather than add a new set of functions we should bite
the bullet and change gpiod_get() (and variants) to take an additional
flags parameter. There aren't all that many users yet (I count 26
outside of drivers/gpio), so maybe now would still be a good time to do
that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140716/7d4241c6/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> > On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> >> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>> >>>
>> >>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >>>>
>> >>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>> >>>>>
>> >>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>> >>>>>>
>> >>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>> >>>>>
>> >>>>>
>> >>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
>> >>>>>>> missing
>> >>>>>>> from this patch.
>> >>>>>
>> >>>>>
>> >>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>> >>>>>> Documentation/gpio/board.txt
>> >>>>>> and as Linus Walleij explained to me the other day, the lookup is
>> >>>>>> supposed
>> >>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>> >>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>> >>>>>> gpiod_direction_output() back or is there another interface for that
>> >>>>>> when
>> >>>>>> registering the board gpios?
>> >>>>>
>> >>>>>
>> >>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>> >>>>> like we either need a gpiod_get_one() or an extension to the table,
>> >>>>> looking at the code it seems like this is indeed the case.  We can set
>> >>>>> if the GPIO is active high/low, or open source/drain but there's no flag
>> >>>>> for the initial state.
>> >>>>
>> >>>>
>> >>>> (adding Alexandre and the gpio list)
>> >>>>
>> >>>> GPIO people: any guidance on how a board file should set a gpio to
>> >>>> output/default-high in a GPIO_LOOKUP() table to replace a
>> >>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>> >>>> Do we need to add an interface extension to do this, e.g. passing
>> >>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>> >>>
>> >>>
>> >>> The way I see it, GPIO mappings (whether they are done using the
>> >>> lookup tables, DT, or ACPI) should only care about details that are
>> >>> relevant to the device layout and that should be abstracted to the
>> >>> driver (e.g. whether the GPIO is active low or open drain) so drivers
>> >>> do not need to check X conditions every time they want to drive the
>> >>> GPIO.
>> >>>
>> >>> Direction and initial value, on the other hand, are clearly properties
>> >>> that ought to be set by the driver itself. Thus my expectation here
>> >>> would be that the driver sets the GPIO direction and initial value as
>> >>> soon as it gets it using gpiod_direction_output(). In other words,
>> >>> there is no replacement for gpio_request_one() with the gpiod
>> >>> interface. Is there any use-case that cannot be covered by calling
>> >>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
>> >>> gpio_request_one() was doing anyway.
>> >>
>> >>
>> >> I agree with you that this is something that should be done in the driver
>> >> and not in the lookup table. I think that it is still a good idea to have a
>> >> replacement for gpio_request_one with the new GPIO descriptor API. A large
>> >> share of the drivers want to call either gpio_direction_input() or
>> >> gpio_direction_output() right after requesting the GPIO. Combining both the
>> >> requesting and the configuration of the GPIO into one function call makes
>> >> the code a bit shorter and also simplifies the error handling. Even more so
>> >> if e.g. the GPIO is optional. This was one of the main reasons why
>> >> gpio_request_one was introduced, see the commit[1] that added it.
>> >
>> > I am not opposed to it as a convenience function. Note that since the
>> > open-source and open-drain flags are already handled by the lookup
>> > table, the only flags it should handle are those related to direction,
>> > value, and (maybe) sysfs export.
>>
>> Problem is, too much convenience functions seems to ultimately kill convenience.
>>
>> The canonical way to request a GPIO is by providing a (device,
>> function, index) triplet to gpiod_get_index(). Since most functions
>> only need one GPIO, we have gpiod_get(device, function) which is
>> basically an alias to gpiod_get_index(device, function, 0) (note to
>> self: we should probably inline it).
>>
>> On top of these comes another set of convenience functions,
>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
>> instead of -ENOENT if the requested GPIO mapping does not exist. This
>> is useful for the common case where a driver can work without a GPIO.
>>
>> Of course these functions all have devm counterparts, so we currently
>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
>>
>> If we are to add functions with an init flags parameter, we will end
>> with 16 functions. That starts to be a bit too much to my taste, and
>> maybe that's where GPIO consumers should sacrifice some convenience to
>> preserve a comprehensible GPIO API.
>>
>> There might be other ways to work around this though. For instance, we
>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
>> passed to a more generic function that would also accept direction and
>> init value flags. Actually I am not seeing any user of the _optional
>> variant in -next, so maybe we should just do this. Thierry, since you
>> introduced the _optional functions, can we get your thoughts about
>> this?
>
> I personally prefer explicit naming of the functions rather than putting
> a bunch of flags into some parameter. If you're overly concerned about
> the amount of convenience functions, perhaps the _index variants can be
> left out for gpiod_get_one(). I'd argue that if drivers want to deal
> with that level of detail anyway, they may just as well add the index
> explicitly when calling the function.
>
> While we're at it, gpiod_get_one() doesn't sound like a very good name.
> All other variants only request "one" as well. Perhaps something like
> gpiod_get_with_flags() would be a better name.
>
> Then again, maybe rather than add a new set of functions we should bite
> the bullet and change gpiod_get() (and variants) to take an additional
> flags parameter. There aren't all that many users yet (I count 26
> outside of drivers/gpio), so maybe now would still be a good time to do
> that.

That sounds reasonable indeed. And preferable to getting an aneurysm
after trying to spell devm_gpiod_get_index_optional_with_flags().

This also makes the most sense since most GPIO users will want to set
a direction and value right after obtaining one. So if there is no
objection I will probably start refactoring gpiod_get() this week.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Thierry Reding]

On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
> >> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >> > On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> >> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> >> >>>
> >> >>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >>>>
> >> >>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
> >> >>>>>
> >> >>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> >> >>>>>>
> >> >>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> >> >>>>>
> >> >>>>>
> >> >>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
> >> >>>>>>> missing
> >> >>>>>>> from this patch.
> >> >>>>>
> >> >>>>>
> >> >>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
> >> >>>>>> Documentation/gpio/board.txt
> >> >>>>>> and as Linus Walleij explained to me the other day, the lookup is
> >> >>>>>> supposed
> >> >>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
> >> >>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
> >> >>>>>> gpiod_direction_output() back or is there another interface for that
> >> >>>>>> when
> >> >>>>>> registering the board gpios?
> >> >>>>>
> >> >>>>>
> >> >>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
> >> >>>>> like we either need a gpiod_get_one() or an extension to the table,
> >> >>>>> looking at the code it seems like this is indeed the case.  We can set
> >> >>>>> if the GPIO is active high/low, or open source/drain but there's no flag
> >> >>>>> for the initial state.
> >> >>>>
> >> >>>>
> >> >>>> (adding Alexandre and the gpio list)
> >> >>>>
> >> >>>> GPIO people: any guidance on how a board file should set a gpio to
> >> >>>> output/default-high in a GPIO_LOOKUP() table to replace a
> >> >>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
> >> >>>> Do we need to add an interface extension to do this, e.g. passing
> >> >>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
> >> >>>
> >> >>>
> >> >>> The way I see it, GPIO mappings (whether they are done using the
> >> >>> lookup tables, DT, or ACPI) should only care about details that are
> >> >>> relevant to the device layout and that should be abstracted to the
> >> >>> driver (e.g. whether the GPIO is active low or open drain) so drivers
> >> >>> do not need to check X conditions every time they want to drive the
> >> >>> GPIO.
> >> >>>
> >> >>> Direction and initial value, on the other hand, are clearly properties
> >> >>> that ought to be set by the driver itself. Thus my expectation here
> >> >>> would be that the driver sets the GPIO direction and initial value as
> >> >>> soon as it gets it using gpiod_direction_output(). In other words,
> >> >>> there is no replacement for gpio_request_one() with the gpiod
> >> >>> interface. Is there any use-case that cannot be covered by calling
> >> >>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
> >> >>> gpio_request_one() was doing anyway.
> >> >>
> >> >>
> >> >> I agree with you that this is something that should be done in the driver
> >> >> and not in the lookup table. I think that it is still a good idea to have a
> >> >> replacement for gpio_request_one with the new GPIO descriptor API. A large
> >> >> share of the drivers want to call either gpio_direction_input() or
> >> >> gpio_direction_output() right after requesting the GPIO. Combining both the
> >> >> requesting and the configuration of the GPIO into one function call makes
> >> >> the code a bit shorter and also simplifies the error handling. Even more so
> >> >> if e.g. the GPIO is optional. This was one of the main reasons why
> >> >> gpio_request_one was introduced, see the commit[1] that added it.
> >> >
> >> > I am not opposed to it as a convenience function. Note that since the
> >> > open-source and open-drain flags are already handled by the lookup
> >> > table, the only flags it should handle are those related to direction,
> >> > value, and (maybe) sysfs export.
> >>
> >> Problem is, too much convenience functions seems to ultimately kill convenience.
> >>
> >> The canonical way to request a GPIO is by providing a (device,
> >> function, index) triplet to gpiod_get_index(). Since most functions
> >> only need one GPIO, we have gpiod_get(device, function) which is
> >> basically an alias to gpiod_get_index(device, function, 0) (note to
> >> self: we should probably inline it).
> >>
> >> On top of these comes another set of convenience functions,
> >> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
> >> instead of -ENOENT if the requested GPIO mapping does not exist. This
> >> is useful for the common case where a driver can work without a GPIO.
> >>
> >> Of course these functions all have devm counterparts, so we currently
> >> have 8 (devm_)gpiod_get(_index)(_optional) functions.
> >>
> >> If we are to add functions with an init flags parameter, we will end
> >> with 16 functions. That starts to be a bit too much to my taste, and
> >> maybe that's where GPIO consumers should sacrifice some convenience to
> >> preserve a comprehensible GPIO API.
> >>
> >> There might be other ways to work around this though. For instance, we
> >> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
> >> passed to a more generic function that would also accept direction and
> >> init value flags. Actually I am not seeing any user of the _optional
> >> variant in -next, so maybe we should just do this. Thierry, since you
> >> introduced the _optional functions, can we get your thoughts about
> >> this?
> >
> > I personally prefer explicit naming of the functions rather than putting
> > a bunch of flags into some parameter. If you're overly concerned about
> > the amount of convenience functions, perhaps the _index variants can be
> > left out for gpiod_get_one(). I'd argue that if drivers want to deal
> > with that level of detail anyway, they may just as well add the index
> > explicitly when calling the function.
> >
> > While we're at it, gpiod_get_one() doesn't sound like a very good name.
> > All other variants only request "one" as well. Perhaps something like
> > gpiod_get_with_flags() would be a better name.
> >
> > Then again, maybe rather than add a new set of functions we should bite
> > the bullet and change gpiod_get() (and variants) to take an additional
> > flags parameter. There aren't all that many users yet (I count 26
> > outside of drivers/gpio), so maybe now would still be a good time to do
> > that.
> 
> That sounds reasonable indeed. And preferable to getting an aneurysm
> after trying to spell devm_gpiod_get_index_optional_with_flags().
> 
> This also makes the most sense since most GPIO users will want to set
> a direction and value right after obtaining one. So if there is no
> objection I will probably start refactoring gpiod_get() this week.

Sounds good to me.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140716/2bcaeb35/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Rob Jones]

On 16/07/14 08:51, Thierry Reding wrote:
> On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
>> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
>>>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>>>> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>>>>>>>
>>>>>>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>>>>>
>>>>>>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>>>>>>>>>
>>>>>>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
>>>>>>>>>>> missing
>>>>>>>>>>> from this patch.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>>>>>>>>>> Documentation/gpio/board.txt
>>>>>>>>>> and as Linus Walleij explained to me the other day, the lookup is
>>>>>>>>>> supposed
>>>>>>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>>>>>>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>>>>>>>>>> gpiod_direction_output() back or is there another interface for that
>>>>>>>>>> when
>>>>>>>>>> registering the board gpios?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>>>>>>>>> like we either need a gpiod_get_one() or an extension to the table,
>>>>>>>>> looking at the code it seems like this is indeed the case.  We can set
>>>>>>>>> if the GPIO is active high/low, or open source/drain but there's no flag
>>>>>>>>> for the initial state.
>>>>>>>>
>>>>>>>>
>>>>>>>> (adding Alexandre and the gpio list)
>>>>>>>>
>>>>>>>> GPIO people: any guidance on how a board file should set a gpio to
>>>>>>>> output/default-high in a GPIO_LOOKUP() table to replace a
>>>>>>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>>>>>>>> Do we need to add an interface extension to do this, e.g. passing
>>>>>>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>>>>>>>
>>>>>>>
>>>>>>> The way I see it, GPIO mappings (whether they are done using the
>>>>>>> lookup tables, DT, or ACPI) should only care about details that are
>>>>>>> relevant to the device layout and that should be abstracted to the
>>>>>>> driver (e.g. whether the GPIO is active low or open drain) so drivers
>>>>>>> do not need to check X conditions every time they want to drive the
>>>>>>> GPIO.
>>>>>>>
>>>>>>> Direction and initial value, on the other hand, are clearly properties
>>>>>>> that ought to be set by the driver itself. Thus my expectation here
>>>>>>> would be that the driver sets the GPIO direction and initial value as
>>>>>>> soon as it gets it using gpiod_direction_output(). In other words,
>>>>>>> there is no replacement for gpio_request_one() with the gpiod
>>>>>>> interface. Is there any use-case that cannot be covered by calling
>>>>>>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
>>>>>>> gpio_request_one() was doing anyway.
>>>>>>
>>>>>>
>>>>>> I agree with you that this is something that should be done in the driver
>>>>>> and not in the lookup table. I think that it is still a good idea to have a
>>>>>> replacement for gpio_request_one with the new GPIO descriptor API. A large
>>>>>> share of the drivers want to call either gpio_direction_input() or
>>>>>> gpio_direction_output() right after requesting the GPIO. Combining both the
>>>>>> requesting and the configuration of the GPIO into one function call makes
>>>>>> the code a bit shorter and also simplifies the error handling. Even more so
>>>>>> if e.g. the GPIO is optional. This was one of the main reasons why
>>>>>> gpio_request_one was introduced, see the commit[1] that added it.
>>>>>
>>>>> I am not opposed to it as a convenience function. Note that since the
>>>>> open-source and open-drain flags are already handled by the lookup
>>>>> table, the only flags it should handle are those related to direction,
>>>>> value, and (maybe) sysfs export.
>>>>
>>>> Problem is, too much convenience functions seems to ultimately kill convenience.
>>>>
>>>> The canonical way to request a GPIO is by providing a (device,
>>>> function, index) triplet to gpiod_get_index(). Since most functions
>>>> only need one GPIO, we have gpiod_get(device, function) which is
>>>> basically an alias to gpiod_get_index(device, function, 0) (note to
>>>> self: we should probably inline it).
>>>>
>>>> On top of these comes another set of convenience functions,
>>>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
>>>> instead of -ENOENT if the requested GPIO mapping does not exist. This
>>>> is useful for the common case where a driver can work without a GPIO.
>>>>
>>>> Of course these functions all have devm counterparts, so we currently
>>>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
>>>>
>>>> If we are to add functions with an init flags parameter, we will end
>>>> with 16 functions. That starts to be a bit too much to my taste, and
>>>> maybe that's where GPIO consumers should sacrifice some convenience to
>>>> preserve a comprehensible GPIO API.
>>>>
>>>> There might be other ways to work around this though. For instance, we
>>>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
>>>> passed to a more generic function that would also accept direction and
>>>> init value flags. Actually I am not seeing any user of the _optional
>>>> variant in -next, so maybe we should just do this. Thierry, since you
>>>> introduced the _optional functions, can we get your thoughts about
>>>> this?
>>>
>>> I personally prefer explicit naming of the functions rather than putting
>>> a bunch of flags into some parameter. If you're overly concerned about
>>> the amount of convenience functions, perhaps the _index variants can be
>>> left out for gpiod_get_one(). I'd argue that if drivers want to deal
>>> with that level of detail anyway, they may just as well add the index
>>> explicitly when calling the function.
>>>
>>> While we're at it, gpiod_get_one() doesn't sound like a very good name.
>>> All other variants only request "one" as well. Perhaps something like
>>> gpiod_get_with_flags() would be a better name.
>>>
>>> Then again, maybe rather than add a new set of functions we should bite
>>> the bullet and change gpiod_get() (and variants) to take an additional
>>> flags parameter. There aren't all that many users yet (I count 26
>>> outside of drivers/gpio), so maybe now would still be a good time to do
>>> that.
>>
>> That sounds reasonable indeed. And preferable to getting an aneurysm
>> after trying to spell devm_gpiod_get_index_optional_with_flags().
>>
>> This also makes the most sense since most GPIO users will want to set
>> a direction and value right after obtaining one. So if there is no
>> objection I will probably start refactoring gpiod_get() this week.
>
> Sounds good to me.
>

In light of this, should I hold off starting on devm_gpiod_get_array()
as discussed on here last week?

> Thierry
>

-- 
Rob Jones
Codethink Ltd
mailto:rob.jones at codethink.co.uk
tel:+44 161 236 5575

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding

> > Then again, maybe rather than add a new set of functions we should bite
> > the bullet and change gpiod_get() (and variants) to take an additional
> > flags parameter. There aren't all that many users yet (I count 26
> > outside of drivers/gpio), so maybe now would still be a good time to do
> > that.

> That sounds reasonable indeed. And preferable to getting an aneurysm
> after trying to spell devm_gpiod_get_index_optional_with_flags().

> This also makes the most sense since most GPIO users will want to set
> a direction and value right after obtaining one. So if there is no
> objection I will probably start refactoring gpiod_get() this week.

Yes, please!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140716/9ef242e2/attachment-0001.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Thierry Reding]

On Wed, Jul 16, 2014 at 09:50:02AM +0100, Rob Jones wrote:
> 
> 
> On 16/07/14 08:51, Thierry Reding wrote:
> >On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
> >>On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
> >><thierry.reding@gmail.com> wrote:
> >>>On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
> >>>>On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >>>>>On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>>>>On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> >>>>>>>
> >>>>>>>On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>>>>>>
> >>>>>>>>On Monday 14 July 2014 19:36:24 Mark Brown wrote:
> >>>>>>>>>
> >>>>>>>>>On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> >>>>>>>>>>
> >>>>>>>>>>On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>>>Yes. But now that you say it the gpiod_direction_output() call is
> >>>>>>>>>>>missing
> >>>>>>>>>>>from this patch.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>>I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
> >>>>>>>>>>Documentation/gpio/board.txt
> >>>>>>>>>>and as Linus Walleij explained to me the other day, the lookup is
> >>>>>>>>>>supposed
> >>>>>>>>>>to replace devm_gpio_request_one(), which in turn replaced both the
> >>>>>>>>>>gpio_request and the gpio_direction_output(). Do I need to put the
> >>>>>>>>>>gpiod_direction_output() back or is there another interface for that
> >>>>>>>>>>when
> >>>>>>>>>>registering the board gpios?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>Indeed.  If you *do* need an explicit _output() then that sounds to me
> >>>>>>>>>like we either need a gpiod_get_one() or an extension to the table,
> >>>>>>>>>looking at the code it seems like this is indeed the case.  We can set
> >>>>>>>>>if the GPIO is active high/low, or open source/drain but there's no flag
> >>>>>>>>>for the initial state.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>(adding Alexandre and the gpio list)
> >>>>>>>>
> >>>>>>>>GPIO people: any guidance on how a board file should set a gpio to
> >>>>>>>>output/default-high in a GPIO_LOOKUP() table to replace a
> >>>>>>>>devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
> >>>>>>>>Do we need to add an interface extension to do this, e.g. passing
> >>>>>>>>GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
> >>>>>>>
> >>>>>>>
> >>>>>>>The way I see it, GPIO mappings (whether they are done using the
> >>>>>>>lookup tables, DT, or ACPI) should only care about details that are
> >>>>>>>relevant to the device layout and that should be abstracted to the
> >>>>>>>driver (e.g. whether the GPIO is active low or open drain) so drivers
> >>>>>>>do not need to check X conditions every time they want to drive the
> >>>>>>>GPIO.
> >>>>>>>
> >>>>>>>Direction and initial value, on the other hand, are clearly properties
> >>>>>>>that ought to be set by the driver itself. Thus my expectation here
> >>>>>>>would be that the driver sets the GPIO direction and initial value as
> >>>>>>>soon as it gets it using gpiod_direction_output(). In other words,
> >>>>>>>there is no replacement for gpio_request_one() with the gpiod
> >>>>>>>interface. Is there any use-case that cannot be covered by calling
> >>>>>>>gpiod_direction_output() right after gpiod_get()? AFAICT this is what
> >>>>>>>gpio_request_one() was doing anyway.
> >>>>>>
> >>>>>>
> >>>>>>I agree with you that this is something that should be done in the driver
> >>>>>>and not in the lookup table. I think that it is still a good idea to have a
> >>>>>>replacement for gpio_request_one with the new GPIO descriptor API. A large
> >>>>>>share of the drivers want to call either gpio_direction_input() or
> >>>>>>gpio_direction_output() right after requesting the GPIO. Combining both the
> >>>>>>requesting and the configuration of the GPIO into one function call makes
> >>>>>>the code a bit shorter and also simplifies the error handling. Even more so
> >>>>>>if e.g. the GPIO is optional. This was one of the main reasons why
> >>>>>>gpio_request_one was introduced, see the commit[1] that added it.
> >>>>>
> >>>>>I am not opposed to it as a convenience function. Note that since the
> >>>>>open-source and open-drain flags are already handled by the lookup
> >>>>>table, the only flags it should handle are those related to direction,
> >>>>>value, and (maybe) sysfs export.
> >>>>
> >>>>Problem is, too much convenience functions seems to ultimately kill convenience.
> >>>>
> >>>>The canonical way to request a GPIO is by providing a (device,
> >>>>function, index) triplet to gpiod_get_index(). Since most functions
> >>>>only need one GPIO, we have gpiod_get(device, function) which is
> >>>>basically an alias to gpiod_get_index(device, function, 0) (note to
> >>>>self: we should probably inline it).
> >>>>
> >>>>On top of these comes another set of convenience functions,
> >>>>gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
> >>>>instead of -ENOENT if the requested GPIO mapping does not exist. This
> >>>>is useful for the common case where a driver can work without a GPIO.
> >>>>
> >>>>Of course these functions all have devm counterparts, so we currently
> >>>>have 8 (devm_)gpiod_get(_index)(_optional) functions.
> >>>>
> >>>>If we are to add functions with an init flags parameter, we will end
> >>>>with 16 functions. That starts to be a bit too much to my taste, and
> >>>>maybe that's where GPIO consumers should sacrifice some convenience to
> >>>>preserve a comprehensible GPIO API.
> >>>>
> >>>>There might be other ways to work around this though. For instance, we
> >>>>could replace the _optional functions by a GPIOF_OPTIONAL flag to be
> >>>>passed to a more generic function that would also accept direction and
> >>>>init value flags. Actually I am not seeing any user of the _optional
> >>>>variant in -next, so maybe we should just do this. Thierry, since you
> >>>>introduced the _optional functions, can we get your thoughts about
> >>>>this?
> >>>
> >>>I personally prefer explicit naming of the functions rather than putting
> >>>a bunch of flags into some parameter. If you're overly concerned about
> >>>the amount of convenience functions, perhaps the _index variants can be
> >>>left out for gpiod_get_one(). I'd argue that if drivers want to deal
> >>>with that level of detail anyway, they may just as well add the index
> >>>explicitly when calling the function.
> >>>
> >>>While we're at it, gpiod_get_one() doesn't sound like a very good name.
> >>>All other variants only request "one" as well. Perhaps something like
> >>>gpiod_get_with_flags() would be a better name.
> >>>
> >>>Then again, maybe rather than add a new set of functions we should bite
> >>>the bullet and change gpiod_get() (and variants) to take an additional
> >>>flags parameter. There aren't all that many users yet (I count 26
> >>>outside of drivers/gpio), so maybe now would still be a good time to do
> >>>that.
> >>
> >>That sounds reasonable indeed. And preferable to getting an aneurysm
> >>after trying to spell devm_gpiod_get_index_optional_with_flags().
> >>
> >>This also makes the most sense since most GPIO users will want to set
> >>a direction and value right after obtaining one. So if there is no
> >>objection I will probably start refactoring gpiod_get() this week.
> >
> >Sounds good to me.
> >
> 
> In light of this, should I hold off starting on devm_gpiod_get_array()
> as discussed on here last week?

I'll let Alex or Linus answer this, since I wasn't involved in the
devm_gpiod_get_array() discussions. It's probably going to be tricky to
pass around an array of everything, but I suspect you've already got a
solution to that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140716/8f862613/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Wed, Jul 16, 2014 at 5:50 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>
>
> On 16/07/14 08:51, Thierry Reding wrote:
>>
>> On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
>>>
>>> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
>>> <thierry.reding@gmail.com> wrote:
>>>>
>>>> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
>>>>>
>>>>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> Yes. But now that you say it the gpiod_direction_output() call
>>>>>>>>>>>> is
>>>>>>>>>>>> missing
>>>>>>>>>>>> from this patch.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>>>>>>>>>>> Documentation/gpio/board.txt
>>>>>>>>>>> and as Linus Walleij explained to me the other day, the lookup is
>>>>>>>>>>> supposed
>>>>>>>>>>> to replace devm_gpio_request_one(), which in turn replaced both
>>>>>>>>>>> the
>>>>>>>>>>> gpio_request and the gpio_direction_output(). Do I need to put
>>>>>>>>>>> the
>>>>>>>>>>> gpiod_direction_output() back or is there another interface for
>>>>>>>>>>> that
>>>>>>>>>>> when
>>>>>>>>>>> registering the board gpios?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Indeed.  If you *do* need an explicit _output() then that sounds
>>>>>>>>>> to me
>>>>>>>>>> like we either need a gpiod_get_one() or an extension to the
>>>>>>>>>> table,
>>>>>>>>>> looking at the code it seems like this is indeed the case.  We can
>>>>>>>>>> set
>>>>>>>>>> if the GPIO is active high/low, or open source/drain but there's
>>>>>>>>>> no flag
>>>>>>>>>> for the initial state.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> (adding Alexandre and the gpio list)
>>>>>>>>>
>>>>>>>>> GPIO people: any guidance on how a board file should set a gpio to
>>>>>>>>> output/default-high in a GPIO_LOOKUP() table to replace a
>>>>>>>>> devm_gpio_request_one() call in a device driver with
>>>>>>>>> devm_gpiod_get()?
>>>>>>>>> Do we need to add an interface extension to do this, e.g. passing
>>>>>>>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The way I see it, GPIO mappings (whether they are done using the
>>>>>>>> lookup tables, DT, or ACPI) should only care about details that are
>>>>>>>> relevant to the device layout and that should be abstracted to the
>>>>>>>> driver (e.g. whether the GPIO is active low or open drain) so
>>>>>>>> drivers
>>>>>>>> do not need to check X conditions every time they want to drive the
>>>>>>>> GPIO.
>>>>>>>>
>>>>>>>> Direction and initial value, on the other hand, are clearly
>>>>>>>> properties
>>>>>>>> that ought to be set by the driver itself. Thus my expectation here
>>>>>>>> would be that the driver sets the GPIO direction and initial value
>>>>>>>> as
>>>>>>>> soon as it gets it using gpiod_direction_output(). In other words,
>>>>>>>> there is no replacement for gpio_request_one() with the gpiod
>>>>>>>> interface. Is there any use-case that cannot be covered by calling
>>>>>>>> gpiod_direction_output() right after gpiod_get()? AFAICT this is
>>>>>>>> what
>>>>>>>> gpio_request_one() was doing anyway.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I agree with you that this is something that should be done in the
>>>>>>> driver
>>>>>>> and not in the lookup table. I think that it is still a good idea to
>>>>>>> have a
>>>>>>> replacement for gpio_request_one with the new GPIO descriptor API. A
>>>>>>> large
>>>>>>> share of the drivers want to call either gpio_direction_input() or
>>>>>>> gpio_direction_output() right after requesting the GPIO. Combining
>>>>>>> both the
>>>>>>> requesting and the configuration of the GPIO into one function call
>>>>>>> makes
>>>>>>> the code a bit shorter and also simplifies the error handling. Even
>>>>>>> more so
>>>>>>> if e.g. the GPIO is optional. This was one of the main reasons why
>>>>>>> gpio_request_one was introduced, see the commit[1] that added it.
>>>>>>
>>>>>>
>>>>>> I am not opposed to it as a convenience function. Note that since the
>>>>>> open-source and open-drain flags are already handled by the lookup
>>>>>> table, the only flags it should handle are those related to direction,
>>>>>> value, and (maybe) sysfs export.
>>>>>
>>>>>
>>>>> Problem is, too much convenience functions seems to ultimately kill
>>>>> convenience.
>>>>>
>>>>> The canonical way to request a GPIO is by providing a (device,
>>>>> function, index) triplet to gpiod_get_index(). Since most functions
>>>>> only need one GPIO, we have gpiod_get(device, function) which is
>>>>> basically an alias to gpiod_get_index(device, function, 0) (note to
>>>>> self: we should probably inline it).
>>>>>
>>>>> On top of these comes another set of convenience functions,
>>>>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
>>>>> instead of -ENOENT if the requested GPIO mapping does not exist. This
>>>>> is useful for the common case where a driver can work without a GPIO.
>>>>>
>>>>> Of course these functions all have devm counterparts, so we currently
>>>>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
>>>>>
>>>>> If we are to add functions with an init flags parameter, we will end
>>>>> with 16 functions. That starts to be a bit too much to my taste, and
>>>>> maybe that's where GPIO consumers should sacrifice some convenience to
>>>>> preserve a comprehensible GPIO API.
>>>>>
>>>>> There might be other ways to work around this though. For instance, we
>>>>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
>>>>> passed to a more generic function that would also accept direction and
>>>>> init value flags. Actually I am not seeing any user of the _optional
>>>>> variant in -next, so maybe we should just do this. Thierry, since you
>>>>> introduced the _optional functions, can we get your thoughts about
>>>>> this?
>>>>
>>>>
>>>> I personally prefer explicit naming of the functions rather than putting
>>>> a bunch of flags into some parameter. If you're overly concerned about
>>>> the amount of convenience functions, perhaps the _index variants can be
>>>> left out for gpiod_get_one(). I'd argue that if drivers want to deal
>>>> with that level of detail anyway, they may just as well add the index
>>>> explicitly when calling the function.
>>>>
>>>> While we're at it, gpiod_get_one() doesn't sound like a very good name.
>>>> All other variants only request "one" as well. Perhaps something like
>>>> gpiod_get_with_flags() would be a better name.
>>>>
>>>> Then again, maybe rather than add a new set of functions we should bite
>>>> the bullet and change gpiod_get() (and variants) to take an additional
>>>> flags parameter. There aren't all that many users yet (I count 26
>>>> outside of drivers/gpio), so maybe now would still be a good time to do
>>>> that.
>>>
>>>
>>> That sounds reasonable indeed. And preferable to getting an aneurysm
>>> after trying to spell devm_gpiod_get_index_optional_with_flags().
>>>
>>> This also makes the most sense since most GPIO users will want to set
>>> a direction and value right after obtaining one. So if there is no
>>> objection I will probably start refactoring gpiod_get() this week.
>>
>>
>> Sounds good to me.
>>
>
> In light of this, should I hold off starting on devm_gpiod_get_array()
> as discussed on here last week?

These are two independant issues, and adapting the get_array() patch
to a refactored gpiod_get() should be trivial.

But while we are at it (and sorry for going further off-topic), I also
think that gpiod_get_array() should not follow the same calling
convention as gpio_request_array() by taking an array of whatever
gpiod_get() would require. Instead, I think it should be redefined to
mean "get all the GPIOs for a given function". For instance, say that
in the DT you have

foo-gpios = <&gpio 0 GPIO_ACTIVE_HIGH &gpio 1 GPIO_ACTIVE_HIGH &gpio 2
GPIO_ACTIVE_HIGH>;

Then gpiod_get_array(dev, "foo", &num) should return descriptors to
these 3 GPIOs and assign "3" to num. The same thing can be done with
the platform lookup tables.

Actually it would be even better if this API could be designed to
interact nicely with the multiple GPIO setting patch by Rojhalat:
http://www.spinics.net/lists/linux-gpio/msg00827.html

gpiod_get_array() would thus allocate and return an array of GPIO
descriptors suitable to be used immediatly with gpiod_set_array(). And
bam, a nice full-circle API for handling multiple GPIOs. My
expectations have risen all of a sudden. ;)

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Thierry Reding]

On Thu, Jul 17, 2014 at 01:28:00PM +0900, Alexandre Courbot wrote:
> On Wed, Jul 16, 2014 at 5:50 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
> >
> >
> > On 16/07/14 08:51, Thierry Reding wrote:
> >>
> >> On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
> >>>
> >>> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
> >>> <thierry.reding@gmail.com> wrote:
> >>>>
> >>>> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
> >>>>>
> >>>>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>> Yes. But now that you say it the gpiod_direction_output() call
> >>>>>>>>>>>> is
> >>>>>>>>>>>> missing
> >>>>>>>>>>>> from this patch.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
> >>>>>>>>>>> Documentation/gpio/board.txt
> >>>>>>>>>>> and as Linus Walleij explained to me the other day, the lookup is
> >>>>>>>>>>> supposed
> >>>>>>>>>>> to replace devm_gpio_request_one(), which in turn replaced both
> >>>>>>>>>>> the
> >>>>>>>>>>> gpio_request and the gpio_direction_output(). Do I need to put
> >>>>>>>>>>> the
> >>>>>>>>>>> gpiod_direction_output() back or is there another interface for
> >>>>>>>>>>> that
> >>>>>>>>>>> when
> >>>>>>>>>>> registering the board gpios?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Indeed.  If you *do* need an explicit _output() then that sounds
> >>>>>>>>>> to me
> >>>>>>>>>> like we either need a gpiod_get_one() or an extension to the
> >>>>>>>>>> table,
> >>>>>>>>>> looking at the code it seems like this is indeed the case.  We can
> >>>>>>>>>> set
> >>>>>>>>>> if the GPIO is active high/low, or open source/drain but there's
> >>>>>>>>>> no flag
> >>>>>>>>>> for the initial state.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> (adding Alexandre and the gpio list)
> >>>>>>>>>
> >>>>>>>>> GPIO people: any guidance on how a board file should set a gpio to
> >>>>>>>>> output/default-high in a GPIO_LOOKUP() table to replace a
> >>>>>>>>> devm_gpio_request_one() call in a device driver with
> >>>>>>>>> devm_gpiod_get()?
> >>>>>>>>> Do we need to add an interface extension to do this, e.g. passing
> >>>>>>>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> The way I see it, GPIO mappings (whether they are done using the
> >>>>>>>> lookup tables, DT, or ACPI) should only care about details that are
> >>>>>>>> relevant to the device layout and that should be abstracted to the
> >>>>>>>> driver (e.g. whether the GPIO is active low or open drain) so
> >>>>>>>> drivers
> >>>>>>>> do not need to check X conditions every time they want to drive the
> >>>>>>>> GPIO.
> >>>>>>>>
> >>>>>>>> Direction and initial value, on the other hand, are clearly
> >>>>>>>> properties
> >>>>>>>> that ought to be set by the driver itself. Thus my expectation here
> >>>>>>>> would be that the driver sets the GPIO direction and initial value
> >>>>>>>> as
> >>>>>>>> soon as it gets it using gpiod_direction_output(). In other words,
> >>>>>>>> there is no replacement for gpio_request_one() with the gpiod
> >>>>>>>> interface. Is there any use-case that cannot be covered by calling
> >>>>>>>> gpiod_direction_output() right after gpiod_get()? AFAICT this is
> >>>>>>>> what
> >>>>>>>> gpio_request_one() was doing anyway.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> I agree with you that this is something that should be done in the
> >>>>>>> driver
> >>>>>>> and not in the lookup table. I think that it is still a good idea to
> >>>>>>> have a
> >>>>>>> replacement for gpio_request_one with the new GPIO descriptor API. A
> >>>>>>> large
> >>>>>>> share of the drivers want to call either gpio_direction_input() or
> >>>>>>> gpio_direction_output() right after requesting the GPIO. Combining
> >>>>>>> both the
> >>>>>>> requesting and the configuration of the GPIO into one function call
> >>>>>>> makes
> >>>>>>> the code a bit shorter and also simplifies the error handling. Even
> >>>>>>> more so
> >>>>>>> if e.g. the GPIO is optional. This was one of the main reasons why
> >>>>>>> gpio_request_one was introduced, see the commit[1] that added it.
> >>>>>>
> >>>>>>
> >>>>>> I am not opposed to it as a convenience function. Note that since the
> >>>>>> open-source and open-drain flags are already handled by the lookup
> >>>>>> table, the only flags it should handle are those related to direction,
> >>>>>> value, and (maybe) sysfs export.
> >>>>>
> >>>>>
> >>>>> Problem is, too much convenience functions seems to ultimately kill
> >>>>> convenience.
> >>>>>
> >>>>> The canonical way to request a GPIO is by providing a (device,
> >>>>> function, index) triplet to gpiod_get_index(). Since most functions
> >>>>> only need one GPIO, we have gpiod_get(device, function) which is
> >>>>> basically an alias to gpiod_get_index(device, function, 0) (note to
> >>>>> self: we should probably inline it).
> >>>>>
> >>>>> On top of these comes another set of convenience functions,
> >>>>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
> >>>>> instead of -ENOENT if the requested GPIO mapping does not exist. This
> >>>>> is useful for the common case where a driver can work without a GPIO.
> >>>>>
> >>>>> Of course these functions all have devm counterparts, so we currently
> >>>>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
> >>>>>
> >>>>> If we are to add functions with an init flags parameter, we will end
> >>>>> with 16 functions. That starts to be a bit too much to my taste, and
> >>>>> maybe that's where GPIO consumers should sacrifice some convenience to
> >>>>> preserve a comprehensible GPIO API.
> >>>>>
> >>>>> There might be other ways to work around this though. For instance, we
> >>>>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
> >>>>> passed to a more generic function that would also accept direction and
> >>>>> init value flags. Actually I am not seeing any user of the _optional
> >>>>> variant in -next, so maybe we should just do this. Thierry, since you
> >>>>> introduced the _optional functions, can we get your thoughts about
> >>>>> this?
> >>>>
> >>>>
> >>>> I personally prefer explicit naming of the functions rather than putting
> >>>> a bunch of flags into some parameter. If you're overly concerned about
> >>>> the amount of convenience functions, perhaps the _index variants can be
> >>>> left out for gpiod_get_one(). I'd argue that if drivers want to deal
> >>>> with that level of detail anyway, they may just as well add the index
> >>>> explicitly when calling the function.
> >>>>
> >>>> While we're at it, gpiod_get_one() doesn't sound like a very good name.
> >>>> All other variants only request "one" as well. Perhaps something like
> >>>> gpiod_get_with_flags() would be a better name.
> >>>>
> >>>> Then again, maybe rather than add a new set of functions we should bite
> >>>> the bullet and change gpiod_get() (and variants) to take an additional
> >>>> flags parameter. There aren't all that many users yet (I count 26
> >>>> outside of drivers/gpio), so maybe now would still be a good time to do
> >>>> that.
> >>>
> >>>
> >>> That sounds reasonable indeed. And preferable to getting an aneurysm
> >>> after trying to spell devm_gpiod_get_index_optional_with_flags().
> >>>
> >>> This also makes the most sense since most GPIO users will want to set
> >>> a direction and value right after obtaining one. So if there is no
> >>> objection I will probably start refactoring gpiod_get() this week.
> >>
> >>
> >> Sounds good to me.
> >>
> >
> > In light of this, should I hold off starting on devm_gpiod_get_array()
> > as discussed on here last week?
> 
> These are two independant issues, and adapting the get_array() patch
> to a refactored gpiod_get() should be trivial.
> 
> But while we are at it (and sorry for going further off-topic), I also
> think that gpiod_get_array() should not follow the same calling
> convention as gpio_request_array() by taking an array of whatever
> gpiod_get() would require. Instead, I think it should be redefined to
> mean "get all the GPIOs for a given function". For instance, say that
> in the DT you have
> 
> foo-gpios = <&gpio 0 GPIO_ACTIVE_HIGH &gpio 1 GPIO_ACTIVE_HIGH &gpio 2
> GPIO_ACTIVE_HIGH>;
> 
> Then gpiod_get_array(dev, "foo", &num) should return descriptors to
> these 3 GPIOs and assign "3" to num. The same thing can be done with
> the platform lookup tables.
> 
> Actually it would be even better if this API could be designed to
> interact nicely with the multiple GPIO setting patch by Rojhalat:
> http://www.spinics.net/lists/linux-gpio/msg00827.html
> 
> gpiod_get_array() would thus allocate and return an array of GPIO
> descriptors suitable to be used immediatly with gpiod_set_array(). And
> bam, a nice full-circle API for handling multiple GPIOs. My
> expectations have risen all of a sudden. ;)

Should the new gpiod_get_array() function also have a way to specify the
flags similar to gpiod_get()? I agree that making it return all GPIOs of
a given function is a good idea. And given that GPIOs associated with
the same function probably behave very similarly it should be safe to
add flags handling to that as well. That is, I don't think we'd need to
worry about different GPIOs of the same function requiring different
directions or initial values (note that polarity is still handled by the
GPIO specifier).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/f2af4808/attachment-0001.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Thu, Jul 17, 2014 at 4:44 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, Jul 17, 2014 at 01:28:00PM +0900, Alexandre Courbot wrote:
>> On Wed, Jul 16, 2014 at 5:50 PM, Rob Jones <rob.jones@codethink.co.uk> wrote:
>> >
>> >
>> > On 16/07/14 08:51, Thierry Reding wrote:
>> >>
>> >> On Wed, Jul 16, 2014 at 04:28:33PM +0900, Alexandre Courbot wrote:
>> >>>
>> >>> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
>> >>> <thierry.reding@gmail.com> wrote:
>> >>>>
>> >>>> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
>> >>>>>
>> >>>>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com>
>> >>>>> wrote:
>> >>>>>>
>> >>>>>> On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de>
>> >>>>>> wrote:
>> >>>>>>>
>> >>>>>>> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de>
>> >>>>>>>> wrote:
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>>> Yes. But now that you say it the gpiod_direction_output() call
>> >>>>>>>>>>>> is
>> >>>>>>>>>>>> missing
>> >>>>>>>>>>>> from this patch.
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>> >>>>>>>>>>> Documentation/gpio/board.txt
>> >>>>>>>>>>> and as Linus Walleij explained to me the other day, the lookup is
>> >>>>>>>>>>> supposed
>> >>>>>>>>>>> to replace devm_gpio_request_one(), which in turn replaced both
>> >>>>>>>>>>> the
>> >>>>>>>>>>> gpio_request and the gpio_direction_output(). Do I need to put
>> >>>>>>>>>>> the
>> >>>>>>>>>>> gpiod_direction_output() back or is there another interface for
>> >>>>>>>>>>> that
>> >>>>>>>>>>> when
>> >>>>>>>>>>> registering the board gpios?
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>> Indeed.  If you *do* need an explicit _output() then that sounds
>> >>>>>>>>>> to me
>> >>>>>>>>>> like we either need a gpiod_get_one() or an extension to the
>> >>>>>>>>>> table,
>> >>>>>>>>>> looking at the code it seems like this is indeed the case.  We can
>> >>>>>>>>>> set
>> >>>>>>>>>> if the GPIO is active high/low, or open source/drain but there's
>> >>>>>>>>>> no flag
>> >>>>>>>>>> for the initial state.
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> (adding Alexandre and the gpio list)
>> >>>>>>>>>
>> >>>>>>>>> GPIO people: any guidance on how a board file should set a gpio to
>> >>>>>>>>> output/default-high in a GPIO_LOOKUP() table to replace a
>> >>>>>>>>> devm_gpio_request_one() call in a device driver with
>> >>>>>>>>> devm_gpiod_get()?
>> >>>>>>>>> Do we need to add an interface extension to do this, e.g. passing
>> >>>>>>>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>> The way I see it, GPIO mappings (whether they are done using the
>> >>>>>>>> lookup tables, DT, or ACPI) should only care about details that are
>> >>>>>>>> relevant to the device layout and that should be abstracted to the
>> >>>>>>>> driver (e.g. whether the GPIO is active low or open drain) so
>> >>>>>>>> drivers
>> >>>>>>>> do not need to check X conditions every time they want to drive the
>> >>>>>>>> GPIO.
>> >>>>>>>>
>> >>>>>>>> Direction and initial value, on the other hand, are clearly
>> >>>>>>>> properties
>> >>>>>>>> that ought to be set by the driver itself. Thus my expectation here
>> >>>>>>>> would be that the driver sets the GPIO direction and initial value
>> >>>>>>>> as
>> >>>>>>>> soon as it gets it using gpiod_direction_output(). In other words,
>> >>>>>>>> there is no replacement for gpio_request_one() with the gpiod
>> >>>>>>>> interface. Is there any use-case that cannot be covered by calling
>> >>>>>>>> gpiod_direction_output() right after gpiod_get()? AFAICT this is
>> >>>>>>>> what
>> >>>>>>>> gpio_request_one() was doing anyway.
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> I agree with you that this is something that should be done in the
>> >>>>>>> driver
>> >>>>>>> and not in the lookup table. I think that it is still a good idea to
>> >>>>>>> have a
>> >>>>>>> replacement for gpio_request_one with the new GPIO descriptor API. A
>> >>>>>>> large
>> >>>>>>> share of the drivers want to call either gpio_direction_input() or
>> >>>>>>> gpio_direction_output() right after requesting the GPIO. Combining
>> >>>>>>> both the
>> >>>>>>> requesting and the configuration of the GPIO into one function call
>> >>>>>>> makes
>> >>>>>>> the code a bit shorter and also simplifies the error handling. Even
>> >>>>>>> more so
>> >>>>>>> if e.g. the GPIO is optional. This was one of the main reasons why
>> >>>>>>> gpio_request_one was introduced, see the commit[1] that added it.
>> >>>>>>
>> >>>>>>
>> >>>>>> I am not opposed to it as a convenience function. Note that since the
>> >>>>>> open-source and open-drain flags are already handled by the lookup
>> >>>>>> table, the only flags it should handle are those related to direction,
>> >>>>>> value, and (maybe) sysfs export.
>> >>>>>
>> >>>>>
>> >>>>> Problem is, too much convenience functions seems to ultimately kill
>> >>>>> convenience.
>> >>>>>
>> >>>>> The canonical way to request a GPIO is by providing a (device,
>> >>>>> function, index) triplet to gpiod_get_index(). Since most functions
>> >>>>> only need one GPIO, we have gpiod_get(device, function) which is
>> >>>>> basically an alias to gpiod_get_index(device, function, 0) (note to
>> >>>>> self: we should probably inline it).
>> >>>>>
>> >>>>> On top of these comes another set of convenience functions,
>> >>>>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
>> >>>>> instead of -ENOENT if the requested GPIO mapping does not exist. This
>> >>>>> is useful for the common case where a driver can work without a GPIO.
>> >>>>>
>> >>>>> Of course these functions all have devm counterparts, so we currently
>> >>>>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
>> >>>>>
>> >>>>> If we are to add functions with an init flags parameter, we will end
>> >>>>> with 16 functions. That starts to be a bit too much to my taste, and
>> >>>>> maybe that's where GPIO consumers should sacrifice some convenience to
>> >>>>> preserve a comprehensible GPIO API.
>> >>>>>
>> >>>>> There might be other ways to work around this though. For instance, we
>> >>>>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
>> >>>>> passed to a more generic function that would also accept direction and
>> >>>>> init value flags. Actually I am not seeing any user of the _optional
>> >>>>> variant in -next, so maybe we should just do this. Thierry, since you
>> >>>>> introduced the _optional functions, can we get your thoughts about
>> >>>>> this?
>> >>>>
>> >>>>
>> >>>> I personally prefer explicit naming of the functions rather than putting
>> >>>> a bunch of flags into some parameter. If you're overly concerned about
>> >>>> the amount of convenience functions, perhaps the _index variants can be
>> >>>> left out for gpiod_get_one(). I'd argue that if drivers want to deal
>> >>>> with that level of detail anyway, they may just as well add the index
>> >>>> explicitly when calling the function.
>> >>>>
>> >>>> While we're at it, gpiod_get_one() doesn't sound like a very good name.
>> >>>> All other variants only request "one" as well. Perhaps something like
>> >>>> gpiod_get_with_flags() would be a better name.
>> >>>>
>> >>>> Then again, maybe rather than add a new set of functions we should bite
>> >>>> the bullet and change gpiod_get() (and variants) to take an additional
>> >>>> flags parameter. There aren't all that many users yet (I count 26
>> >>>> outside of drivers/gpio), so maybe now would still be a good time to do
>> >>>> that.
>> >>>
>> >>>
>> >>> That sounds reasonable indeed. And preferable to getting an aneurysm
>> >>> after trying to spell devm_gpiod_get_index_optional_with_flags().
>> >>>
>> >>> This also makes the most sense since most GPIO users will want to set
>> >>> a direction and value right after obtaining one. So if there is no
>> >>> objection I will probably start refactoring gpiod_get() this week.
>> >>
>> >>
>> >> Sounds good to me.
>> >>
>> >
>> > In light of this, should I hold off starting on devm_gpiod_get_array()
>> > as discussed on here last week?
>>
>> These are two independant issues, and adapting the get_array() patch
>> to a refactored gpiod_get() should be trivial.
>>
>> But while we are at it (and sorry for going further off-topic), I also
>> think that gpiod_get_array() should not follow the same calling
>> convention as gpio_request_array() by taking an array of whatever
>> gpiod_get() would require. Instead, I think it should be redefined to
>> mean "get all the GPIOs for a given function". For instance, say that
>> in the DT you have
>>
>> foo-gpios = <&gpio 0 GPIO_ACTIVE_HIGH &gpio 1 GPIO_ACTIVE_HIGH &gpio 2
>> GPIO_ACTIVE_HIGH>;
>>
>> Then gpiod_get_array(dev, "foo", &num) should return descriptors to
>> these 3 GPIOs and assign "3" to num. The same thing can be done with
>> the platform lookup tables.
>>
>> Actually it would be even better if this API could be designed to
>> interact nicely with the multiple GPIO setting patch by Rojhalat:
>> http://www.spinics.net/lists/linux-gpio/msg00827.html
>>
>> gpiod_get_array() would thus allocate and return an array of GPIO
>> descriptors suitable to be used immediatly with gpiod_set_array(). And
>> bam, a nice full-circle API for handling multiple GPIOs. My
>> expectations have risen all of a sudden. ;)
>
> Should the new gpiod_get_array() function also have a way to specify the
> flags similar to gpiod_get()?

Probably an optional array of flags could not hurt, to follow the
example of gpiod_get().

> I agree that making it return all GPIOs of
> a given function is a good idea. And given that GPIOs associated with
> the same function probably behave very similarly it should be safe to
> add flags handling to that as well. That is, I don't think we'd need to
> worry about different GPIOs of the same function requiring different
> directions or initial values (note that polarity is still handled by the
> GPIO specifier).

Right. It may very well be that a single flag specifier (as opposed to
an array) will be enough for this case. If you need to request some
GPIOs as input and some other as output then they are clearly
different functions and requesting them together would be an abuse of
the API.

Initial values of output GPIOs might be a reason why we want an array
though, and I leave it to Rob to decide what is best here since he has
a better idea of how this is going to be used.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Thu, Jul 17, 2014 at 05:55:36PM +0900, Alexandre Courbot wrote:

> Right. It may very well be that a single flag specifier (as opposed to
> an array) will be enough for this case. If you need to request some
> GPIOs as input and some other as output then they are clearly
> different functions and requesting them together would be an abuse of
> the API.

Not so sure about that - what about requesting GPIOs for a bidirectional
bus?  Thinking about SPI bitbanging here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/d8637095/attachment-0001.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Thierry Reding]

On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:
> On Thu, Jul 17, 2014 at 05:55:36PM +0900, Alexandre Courbot wrote:
> 
> > Right. It may very well be that a single flag specifier (as opposed to
> > an array) will be enough for this case. If you need to request some
> > GPIOs as input and some other as output then they are clearly
> > different functions and requesting them together would be an abuse of
> > the API.
> 
> Not so sure about that - what about requesting GPIOs for a bidirectional
> bus?  Thinking about SPI bitbanging here.

Wouldn't you want to use a different means that the gpiod_array_*() API
to handle those cases? gpiod_array_*() is probably most useful to handle
bulk operations on a set of GPIOs that do essentially the same thing. If
you get and then need to index into that array to handle them all
differently then you don't gain very much.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/7c167e5d/attachment-0001.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Lars-Peter Clausen]

On 07/17/2014 12:41 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:
>> On Thu, Jul 17, 2014 at 05:55:36PM +0900, Alexandre Courbot wrote:
>>
>>> Right. It may very well be that a single flag specifier (as opposed to
>>> an array) will be enough for this case. If you need to request some
>>> GPIOs as input and some other as output then they are clearly
>>> different functions and requesting them together would be an abuse of
>>> the API.
>>
>> Not so sure about that - what about requesting GPIOs for a bidirectional
>> bus?  Thinking about SPI bitbanging here.
>
> Wouldn't you want to use a different means that the gpiod_array_*() API
> to handle those cases? gpiod_array_*() is probably most useful to handle
> bulk operations on a set of GPIOs that do essentially the same thing. If
> you get and then need to index into that array to handle them all
> differently then you don't gain very much.

I think the goal of a gpiod_array_* API should be to make requesting 
multiple GPIOs that are used by a driver as convenient as possible and at 
the same time reduce the amount of boiler plate code necessary. E.g compare

gpios[0] = gpio_get(...);
if (IS_ERR(gpios[0])) {
	ret = PTR_ERR(gpios[0]);
	goto cleanup;
}

gpios[1] = gpio_get(...);
if (IS_ERR(gpios[1])) {
	ret = PTR_ERR(gpios[1]);
	goto cleanup;
}

gpios[2] = gpio_get(...);
if (IS_ERR(gpios[2])) {
	ret = PTR_ERR(gpioss[2]);
	goto cleanup;
}

with

ret = gpio_array_get(..., gpios);
if (ret)
	goto err_cleanup;

- Lars

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Thu, Jul 17, 2014 at 12:41:23PM +0200, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:

> > Not so sure about that - what about requesting GPIOs for a bidirectional
> > bus?  Thinking about SPI bitbanging here.

> Wouldn't you want to use a different means that the gpiod_array_*() API
> to handle those cases? gpiod_array_*() is probably most useful to handle
> bulk operations on a set of GPIOs that do essentially the same thing. If
> you get and then need to index into that array to handle them all
> differently then you don't gain very much.

For set and get, sure - but it's still useful to be able to do bulk
requests for GPIOs especially since that's the only bit of the interface
that requires error handling.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140717/c32114b8/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Thu, Jul 17, 2014 at 8:05 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 17, 2014 at 12:41:23PM +0200, Thierry Reding wrote:
>> On Thu, Jul 17, 2014 at 11:17:23AM +0100, Mark Brown wrote:
>
>> > Not so sure about that - what about requesting GPIOs for a bidirectional
>> > bus?  Thinking about SPI bitbanging here.
>
>> Wouldn't you want to use a different means that the gpiod_array_*() API
>> to handle those cases? gpiod_array_*() is probably most useful to handle
>> bulk operations on a set of GPIOs that do essentially the same thing. If
>> you get and then need to index into that array to handle them all
>> differently then you don't gain very much.
>
> For set and get, sure - but it's still useful to be able to do bulk
> requests for GPIOs especially since that's the only bit of the interface
> that requires error handling.

I foresee many problems if people start using gpiod_array_get() as a
way to spare a few lines of error-checking code. First all the GPIOs
would end into an array instead of members with meaningful names -
unless they are moved later on, but doing so would add extra code and
somewhat kill the purpose. It also becomes more difficult to maintain
as you are dealing with array indexes to update all over the code.
Finally, it will make it more difficult to use gpiod_array_*() the way
it is intended to be used, as you would have to discriminate between
GPIOs of the same function and the rest by yourself.

Also, if such a convenience function is legitimate for GPIO, shouldn't
it also apply to other sub-systems? E.g. regulator_array_get()?

Maybe I am missing your point, but I still think some error-handling
code really doesn't hurt here, and the few drivers that would actually
benefit from a more automated GPIO request error handling can easily
implement it themselves. Let's keep gpiod_array_*() single-purposed
and to the point.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Mark Brown]

On Mon, Jul 21, 2014 at 12:36:43PM +0900, Alexandre Courbot wrote:
> On Thu, Jul 17, 2014 at 8:05 PM, Mark Brown <broonie@kernel.org> wrote:

> > For set and get, sure - but it's still useful to be able to do bulk
> > requests for GPIOs especially since that's the only bit of the interface
> > that requires error handling.

> I foresee many problems if people start using gpiod_array_get() as a
> way to spare a few lines of error-checking code. First all the GPIOs
> would end into an array instead of members with meaningful names -
> unless they are moved later on, but doing so would add extra code and
> somewhat kill the purpose. It also becomes more difficult to maintain
> as you are dealing with array indexes to update all over the code.

You just need a few defines for the names, it's not a big deal.

> Finally, it will make it more difficult to use gpiod_array_*() the way
> it is intended to be used, as you would have to discriminate between
> GPIOs of the same function and the rest by yourself.

Yes, you probably shouldn't mix and match here but that's fine.

> Also, if such a convenience function is legitimate for GPIO, shouldn't
> it also apply to other sub-systems? E.g. regulator_array_get()?

It's certainly a totally reasonable and expected way of using
regulator_bulk_get().

> Maybe I am missing your point, but I still think some error-handling
> code really doesn't hurt here, and the few drivers that would actually
> benefit from a more automated GPIO request error handling can easily
> implement it themselves. Let's keep gpiod_array_*() single-purposed
> and to the point.

I'm not sure I see the massive complication TBH - it's not so much about
complexity as it is about reducing the amount of boilerplate that people
need to get right.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140721/ce403552/attachment.sig>

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Mon, Jul 21, 2014 at 7:04 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 21, 2014 at 12:36:43PM +0900, Alexandre Courbot wrote:
>> On Thu, Jul 17, 2014 at 8:05 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > For set and get, sure - but it's still useful to be able to do bulk
>> > requests for GPIOs especially since that's the only bit of the interface
>> > that requires error handling.
>
>> I foresee many problems if people start using gpiod_array_get() as a
>> way to spare a few lines of error-checking code. First all the GPIOs
>> would end into an array instead of members with meaningful names -
>> unless they are moved later on, but doing so would add extra code and
>> somewhat kill the purpose. It also becomes more difficult to maintain
>> as you are dealing with array indexes to update all over the code.
>
> You just need a few defines for the names, it's not a big deal.
>
>> Finally, it will make it more difficult to use gpiod_array_*() the way
>> it is intended to be used, as you would have to discriminate between
>> GPIOs of the same function and the rest by yourself.
>
> Yes, you probably shouldn't mix and match here but that's fine.
>
>> Also, if such a convenience function is legitimate for GPIO, shouldn't
>> it also apply to other sub-systems? E.g. regulator_array_get()?
>
> It's certainly a totally reasonable and expected way of using
> regulator_bulk_get().
>
>> Maybe I am missing your point, but I still think some error-handling
>> code really doesn't hurt here, and the few drivers that would actually
>> benefit from a more automated GPIO request error handling can easily
>> implement it themselves. Let's keep gpiod_array_*() single-purposed
>> and to the point.
>
> I'm not sure I see the massive complication TBH - it's not so much about
> complexity as it is about reducing the amount of boilerplate that people
> need to get right.

I guess our disagreement came from the fact that we want the same
function to perform two different things. GPIOs are different from
regulators in that the former are really requested using a (dev,
function, index) vs. a (dev, function) tuple for regulators. I want a
convenience function to easily request all the GPIOs that match (dev,
function) so that functionally identical GPIOs can be manipulated as
an "atomic" set using the rest of the gpiod_array API (which would
make no sense for regulators which have no "index". You want an
equivalent to regulator_bulk_get() so a driver can conveniently
request all its GPIOs no matter the function they fullfil.

These should really be two different functions for two different
use-cases - gpiod_array_get() that returns an array suitable for being
manipulated using the rest of the gpiod_array API ; gpiod_bulk_get()
that takes the equivalent of regulator_bulk_data for GPIOs and fills
it the same way.

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Linus Walleij]

On Wed, Jul 16, 2014 at 1:09 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jul 16, 2014 at 09:50:02AM +0100, Rob Jones wrote:

>> In light of this, should I hold off starting on devm_gpiod_get_array()
>> as discussed on here last week?
>
> I'll let Alex or Linus answer this, since I wasn't involved in the
> devm_gpiod_get_array() discussions.

I'm gonna shut up in this thread as Alex is doing such an excellent
job at hashing out the details of gpiod*, and he understands it way
better than me.

Yours,
Linus Walleij

[alsa-devel] [PATCH 2/4] ASoC: s3c64xx/smartq: use dynamic registration

[Alexandre Courbot]

On Wed, Jul 16, 2014 at 4:28 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Jul 16, 2014 at 4:12 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Wed, Jul 16, 2014 at 12:00:45PM +0900, Alexandre Courbot wrote:
>>> On Tue, Jul 15, 2014 at 6:14 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> > On Tue, Jul 15, 2014 at 4:58 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> >> On 07/15/2014 09:36 AM, Alexandre Courbot wrote:
>>> >>>
>>> >>> On Tue, Jul 15, 2014 at 4:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> >>>>
>>> >>>> On Monday 14 July 2014 19:36:24 Mark Brown wrote:
>>> >>>>>
>>> >>>>> On Mon, Jul 14, 2014 at 08:23:55PM +0200, Arnd Bergmann wrote:
>>> >>>>>>
>>> >>>>>> On Monday 14 July 2014 18:18:12 Lars-Peter Clausen wrote:
>>> >>>>>
>>> >>>>>
>>> >>>>>>> Yes. But now that you say it the gpiod_direction_output() call is
>>> >>>>>>> missing
>>> >>>>>>> from this patch.
>>> >>>>>
>>> >>>>>
>>> >>>>>> I'm lost now. The GPIO_ACTIVE_HIGH I added comes from
>>> >>>>>> Documentation/gpio/board.txt
>>> >>>>>> and as Linus Walleij explained to me the other day, the lookup is
>>> >>>>>> supposed
>>> >>>>>> to replace devm_gpio_request_one(), which in turn replaced both the
>>> >>>>>> gpio_request and the gpio_direction_output(). Do I need to put the
>>> >>>>>> gpiod_direction_output() back or is there another interface for that
>>> >>>>>> when
>>> >>>>>> registering the board gpios?
>>> >>>>>
>>> >>>>>
>>> >>>>> Indeed.  If you *do* need an explicit _output() then that sounds to me
>>> >>>>> like we either need a gpiod_get_one() or an extension to the table,
>>> >>>>> looking at the code it seems like this is indeed the case.  We can set
>>> >>>>> if the GPIO is active high/low, or open source/drain but there's no flag
>>> >>>>> for the initial state.
>>> >>>>
>>> >>>>
>>> >>>> (adding Alexandre and the gpio list)
>>> >>>>
>>> >>>> GPIO people: any guidance on how a board file should set a gpio to
>>> >>>> output/default-high in a GPIO_LOOKUP() table to replace a
>>> >>>> devm_gpio_request_one() call in a device driver with devm_gpiod_get()?
>>> >>>> Do we need to add an interface extension to do this, e.g. passing
>>> >>>> GPIOF_OUT_INIT_HIGH as the flags rather than GPIO_ACTIVE_HIGH?
>>> >>>
>>> >>>
>>> >>> The way I see it, GPIO mappings (whether they are done using the
>>> >>> lookup tables, DT, or ACPI) should only care about details that are
>>> >>> relevant to the device layout and that should be abstracted to the
>>> >>> driver (e.g. whether the GPIO is active low or open drain) so drivers
>>> >>> do not need to check X conditions every time they want to drive the
>>> >>> GPIO.
>>> >>>
>>> >>> Direction and initial value, on the other hand, are clearly properties
>>> >>> that ought to be set by the driver itself. Thus my expectation here
>>> >>> would be that the driver sets the GPIO direction and initial value as
>>> >>> soon as it gets it using gpiod_direction_output(). In other words,
>>> >>> there is no replacement for gpio_request_one() with the gpiod
>>> >>> interface. Is there any use-case that cannot be covered by calling
>>> >>> gpiod_direction_output() right after gpiod_get()? AFAICT this is what
>>> >>> gpio_request_one() was doing anyway.
>>> >>
>>> >>
>>> >> I agree with you that this is something that should be done in the driver
>>> >> and not in the lookup table. I think that it is still a good idea to have a
>>> >> replacement for gpio_request_one with the new GPIO descriptor API. A large
>>> >> share of the drivers want to call either gpio_direction_input() or
>>> >> gpio_direction_output() right after requesting the GPIO. Combining both the
>>> >> requesting and the configuration of the GPIO into one function call makes
>>> >> the code a bit shorter and also simplifies the error handling. Even more so
>>> >> if e.g. the GPIO is optional. This was one of the main reasons why
>>> >> gpio_request_one was introduced, see the commit[1] that added it.
>>> >
>>> > I am not opposed to it as a convenience function. Note that since the
>>> > open-source and open-drain flags are already handled by the lookup
>>> > table, the only flags it should handle are those related to direction,
>>> > value, and (maybe) sysfs export.
>>>
>>> Problem is, too much convenience functions seems to ultimately kill convenience.
>>>
>>> The canonical way to request a GPIO is by providing a (device,
>>> function, index) triplet to gpiod_get_index(). Since most functions
>>> only need one GPIO, we have gpiod_get(device, function) which is
>>> basically an alias to gpiod_get_index(device, function, 0) (note to
>>> self: we should probably inline it).
>>>
>>> On top of these comes another set of convenience functions,
>>> gpiod_get_optional() and gpiod_get_index_optional(), which return NULL
>>> instead of -ENOENT if the requested GPIO mapping does not exist. This
>>> is useful for the common case where a driver can work without a GPIO.
>>>
>>> Of course these functions all have devm counterparts, so we currently
>>> have 8 (devm_)gpiod_get(_index)(_optional) functions.
>>>
>>> If we are to add functions with an init flags parameter, we will end
>>> with 16 functions. That starts to be a bit too much to my taste, and
>>> maybe that's where GPIO consumers should sacrifice some convenience to
>>> preserve a comprehensible GPIO API.
>>>
>>> There might be other ways to work around this though. For instance, we
>>> could replace the _optional functions by a GPIOF_OPTIONAL flag to be
>>> passed to a more generic function that would also accept direction and
>>> init value flags. Actually I am not seeing any user of the _optional
>>> variant in -next, so maybe we should just do this. Thierry, since you
>>> introduced the _optional functions, can we get your thoughts about
>>> this?
>>
>> I personally prefer explicit naming of the functions rather than putting
>> a bunch of flags into some parameter. If you're overly concerned about
>> the amount of convenience functions, perhaps the _index variants can be
>> left out for gpiod_get_one(). I'd argue that if drivers want to deal
>> with that level of detail anyway, they may just as well add the index
>> explicitly when calling the function.
>>
>> While we're at it, gpiod_get_one() doesn't sound like a very good name.
>> All other variants only request "one" as well. Perhaps something like
>> gpiod_get_with_flags() would be a better name.
>>
>> Then again, maybe rather than add a new set of functions we should bite
>> the bullet and change gpiod_get() (and variants) to take an additional
>> flags parameter. There aren't all that many users yet (I count 26
>> outside of drivers/gpio), so maybe now would still be a good time to do
>> that.
>
> That sounds reasonable indeed. And preferable to getting an aneurysm
> after trying to spell devm_gpiod_get_index_optional_with_flags().
>
> This also makes the most sense since most GPIO users will want to set
> a direction and value right after obtaining one. So if there is no
> objection I will probably start refactoring gpiod_get() this week.

Spammed half of the kernel developers with a patch adding a flags
argument to the gpiod getters and updating all gpiod users (there were
more than I thought!). I'm not sure how such a cross-subsystem patch
is supposed to be applied ; suggestions are welcome!