[PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Andy Shevchenko]

In order to make GPIO ACPI library stricter prepare users of
gpiod_get_index() to correctly behave when there no mapping is
provided by firmware.

Here we add explicit mapping between _CRS GpioIo() resources and
their names used in the driver.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Nicolas Porcel <nicolasporcel06@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 - update device node to which the mapping table is attached
 - remove idx assignment to 0 (it's by default)
 - add Nicolas' tag (sent privately)
 sound/soc/intel/boards/byt-max98090.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/byt-max98090.c b/sound/soc/intel/boards/byt-max98090.c
index d9f81b8d915d..047be7fa0ce9 100644
--- a/sound/soc/intel/boards/byt-max98090.c
+++ b/sound/soc/intel/boards/byt-max98090.c
@@ -67,20 +67,27 @@ static struct snd_soc_jack_pin hs_jack_pins[] = {
 
 static struct snd_soc_jack_gpio hs_jack_gpios[] = {
 	{
-		.name		= "hp-gpio",
-		.idx		= 0,
+		.name		= "hp",
 		.report		= SND_JACK_HEADPHONE | SND_JACK_LINEOUT,
 		.debounce_time	= 200,
 	},
 	{
-		.name		= "mic-gpio",
-		.idx		= 1,
+		.name		= "mic",
 		.invert		= 1,
 		.report		= SND_JACK_MICROPHONE,
 		.debounce_time	= 200,
 	},
 };
 
+static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
+static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
+	{ "hp-gpios", &hp_gpios, 1 },
+	{ "mic-gpios", &mic_gpios, 1 },
+	{},
+};
+
 static int byt_max98090_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
@@ -140,8 +147,9 @@ static struct snd_soc_card byt_max98090_card = {
 
 static int byt_max98090_probe(struct platform_device *pdev)
 {
-	int ret_val = 0;
+	struct device *dev = &pdev->dev;
 	struct byt_max98090_private *priv;
+	int ret_val;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
 	if (!priv) {
@@ -149,6 +157,10 @@ static int byt_max98090_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ret_val = devm_acpi_dev_add_driver_gpios(dev->parent, acpi_byt_max98090_gpios);
+	if (ret_val)
+		dev_dbg(dev, "Unable to add GPIO mapping table\n");
+
 	byt_max98090_card.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&byt_max98090_card, priv);
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_max98090_card);
@@ -158,7 +170,7 @@ static int byt_max98090_probe(struct platform_device *pdev)
 		return ret_val;
 	}
 
-	return ret_val;
+	return 0;
 }
 
 static int byt_max98090_remove(struct platform_device *pdev)
-- 
2.11.0

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Takashi Iwai]

On Sat, 10 Jun 2017 18:37:41 +0200,
Andy Shevchenko wrote:
> 
> +static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
> +	{ "hp-gpios", &hp_gpios, 1 },
> +	{ "mic-gpios", &mic_gpios, 1 },

Better to use C99 style init?  I'm not always a fan of it, but this
seems deserving.  From the above, it's difficult to know which zero is
for what.


thanks,

Takashi

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Sat, 2017-06-10 at 21:41 +0200, Takashi Iwai wrote:
> On Sat, 10 Jun 2017 18:37:41 +0200,
> Andy Shevchenko wrote:
> > 
> > +static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
> > +static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
> > +
> > +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
> > +	{ "hp-gpios", &hp_gpios, 1 },
> > +	{ "mic-gpios", &mic_gpios, 1 },
> 
> Better to use C99 style init?

Here it makes a little advantage over more uglified code.

>   I'm not always a fan of it, but this
> seems deserving.  From the above, it's difficult to know which zero is
> for what.

I'm not sure it makes sense. Those fields are filed according to ACPI
specification and applies to the platform in question. Usually you don't
need to look into them on regular basis, perhaps one time per each
platform which uses such combination of SoC + ASoC + Codec which is
quite unlikely to be more than once twice per year(?).

So, for over few dozens of drivers which are using GPIO ACPI mapping
tables this is the first request like this I see.

Thus, if you insist, I can do it, but my vote is "it doesn't worth of
doing it at all".

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Takashi Iwai]

On Sun, 11 Jun 2017 16:01:13 +0200,
Andy Shevchenko wrote:
> 
> On Sat, 2017-06-10 at 21:41 +0200, Takashi Iwai wrote:
> > On Sat, 10 Jun 2017 18:37:41 +0200,
> > Andy Shevchenko wrote:
> > > 
> > > +static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
> > > +static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
> > > +
> > > +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
> > > +	{ "hp-gpios", &hp_gpios, 1 },
> > > +	{ "mic-gpios", &mic_gpios, 1 },
> > 
> > Better to use C99 style init?
> 
> Here it makes a little advantage over more uglified code.
> 
> >   I'm not always a fan of it, but this
> > seems deserving.  From the above, it's difficult to know which zero is
> > for what.
> 
> I'm not sure it makes sense. Those fields are filed according to ACPI
> specification and applies to the platform in question. Usually you don't
> need to look into them on regular basis, perhaps one time per each
> platform which uses such combination of SoC + ASoC + Codec which is
> quite unlikely to be more than once twice per year(?).
> 
> So, for over few dozens of drivers which are using GPIO ACPI mapping
> tables this is the first request like this I see.

It's a proof showing that people are just too lazy and copy the first
implementation pattern :)

> Thus, if you insist, I can do it, but my vote is "it doesn't worth of
> doing it at all".

Well, I'm not insisting to change that, but I still think that your
argument is rather weak from the code quality POV.

The C99 style initialization is definitely an improvement for
understanding the code.  The most important question is not about how
often changing the code, but about how better a reader can understand
and how it give less errors.

It results in more lines?  Yes.  It's uglier?  Depends.
So what?  It makes clearer and less error-prone.
Hrm, must I do that?  No, it's still a kind of matter of taste.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Sun, 2017-06-11 at 18:48 +0200, Takashi Iwai wrote:
> On Sun, 11 Jun 2017 16:01:13 +0200,
> Andy Shevchenko wrote:
> > On Sat, 2017-06-10 at 21:41 +0200, Takashi Iwai wrote:
> > > On Sat, 10 Jun 2017 18:37:41 +0200,
> > > Andy Shevchenko wrote:
> > > > 
> > > > +static const struct acpi_gpio_params hp_gpios = { 0, 0, false
> > > > };
> > > > +static const struct acpi_gpio_params mic_gpios = { 1, 0, false
> > > > };
> > > > +
> > > > +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[]
> > > > = {
> > > > +	{ "hp-gpios", &hp_gpios, 1 },
> > > > +	{ "mic-gpios", &mic_gpios, 1 },
> > > 
> > > Better to use C99 style init?
> > 
> > Here it makes a little advantage over more uglified code.
> > 
> > >   I'm not always a fan of it, but this
> > > seems deserving.  From the above, it's difficult to know which
> > > zero is
> > > for what.
> > 
> > I'm not sure it makes sense. Those fields are filed according to
> > ACPI
> > specification and applies to the platform in question. Usually you
> > don't
> > need to look into them on regular basis, perhaps one time per each
> > platform which uses such combination of SoC + ASoC + Codec which is
> > quite unlikely to be more than once twice per year(?).
> > 
> > So, for over few dozens of drivers which are using GPIO ACPI mapping
> > tables this is the first request like this I see.
> 
> It's a proof showing that people are just too lazy and copy the first
> implementation pattern :)
> 
> > Thus, if you insist, I can do it, but my vote is "it doesn't worth
> > of
> > doing it at all".
> 
> Well, I'm not insisting to change that, but I still think that your
> argument is rather weak from the code quality POV.

> The C99 style initialization is definitely an improvement for
> understanding the code.  The most important question is not about how
> often changing the code, but about how better a reader can understand
> and how it give less errors.

In general I certainly agree with you. In this particular case
uglification vs. clearness not in the right balance (to me at least).

> It results in more lines?  Yes.  It's uglier?  Depends.
> So what?  It makes clearer and less error-prone.

Again, above case one has to file all members no matter what, since it's
a mapping and explicit better than implicit there. Taking above into
consideration the C99 style just adds a noise here for my POV.

> Hrm, must I do that?  No, it's still a kind of matter of taste.

So, let's agree not to touch this particular case(s)?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Takashi Iwai]

On Sun, 11 Jun 2017 19:02:07 +0200,
Andy Shevchenko wrote:
> 
> On Sun, 2017-06-11 at 18:48 +0200, Takashi Iwai wrote:
> > On Sun, 11 Jun 2017 16:01:13 +0200,
> > Andy Shevchenko wrote:
> > > On Sat, 2017-06-10 at 21:41 +0200, Takashi Iwai wrote:
> > > > On Sat, 10 Jun 2017 18:37:41 +0200,
> > > > Andy Shevchenko wrote:
> > > > > 
> > > > > +static const struct acpi_gpio_params hp_gpios = { 0, 0, false
> > > > > };
> > > > > +static const struct acpi_gpio_params mic_gpios = { 1, 0, false
> > > > > };
> > > > > +
> > > > > +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[]
> > > > > = {
> > > > > +	{ "hp-gpios", &hp_gpios, 1 },
> > > > > +	{ "mic-gpios", &mic_gpios, 1 },
> > > > 
> > > > Better to use C99 style init?
> > > 
> > > Here it makes a little advantage over more uglified code.
> > > 
> > > >   I'm not always a fan of it, but this
> > > > seems deserving.  From the above, it's difficult to know which
> > > > zero is
> > > > for what.
> > > 
> > > I'm not sure it makes sense. Those fields are filed according to
> > > ACPI
> > > specification and applies to the platform in question. Usually you
> > > don't
> > > need to look into them on regular basis, perhaps one time per each
> > > platform which uses such combination of SoC + ASoC + Codec which is
> > > quite unlikely to be more than once twice per year(?).
> > > 
> > > So, for over few dozens of drivers which are using GPIO ACPI mapping
> > > tables this is the first request like this I see.
> > 
> > It's a proof showing that people are just too lazy and copy the first
> > implementation pattern :)
> > 
> > > Thus, if you insist, I can do it, but my vote is "it doesn't worth
> > > of
> > > doing it at all".
> > 
> > Well, I'm not insisting to change that, but I still think that your
> > argument is rather weak from the code quality POV.
> 
> > The C99 style initialization is definitely an improvement for
> > understanding the code.  The most important question is not about how
> > often changing the code, but about how better a reader can understand
> > and how it give less errors.
> 
> In general I certainly agree with you. In this particular case
> uglification vs. clearness not in the right balance (to me at least).
> 
> > It results in more lines?  Yes.  It's uglier?  Depends.
> > So what?  It makes clearer and less error-prone.
> 
> Again, above case one has to file all members no matter what, since it's
> a mapping and explicit better than implicit there. Taking above into
> consideration the C99 style just adds a noise here for my POV.
> 
> > Hrm, must I do that?  No, it's still a kind of matter of taste.
> 
> So, let's agree not to touch this particular case(s)?

Yeah, that implies it, I don't mind too much details.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Sun, 2017-06-11 at 19:08 +0200, Takashi Iwai wrote:
> On Sun, 11 Jun 2017 19:02:07 +0200,
> Andy Shevchenko wrote:
> > 
> > On Sun, 2017-06-11 at 18:48 +0200, Takashi Iwai wrote:
> > > On Sun, 11 Jun 2017 16:01:13 +0200,
> > > Andy Shevchenko wrote:

> > > Hrm, must I do that?  No, it's still a kind of matter of taste.
> > 
> > So, let's agree not to touch this particular case(s)?
> 
> Yeah, that implies it, I don't mind too much details.
> 

Good.
Thanks for review!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

[Andy Shevchenko]

On Sat, 2017-06-10 at 19:37 +0300, Andy Shevchenko wrote:
> In order to make GPIO ACPI library stricter prepare users of
> gpiod_get_index() to correctly behave when there no mapping is
> provided by firmware.
> 
> Here we add explicit mapping between _CRS GpioIo() resources and
> their names used in the driver.
> 

Mark, we agreed with Takashi that the patch is okay in its initial form.
Should I resend it, or maybe you can take it from this thread?

> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Tested-by: Nicolas Porcel <nicolasporcel06@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  - update device node to which the mapping table is attached
>  - remove idx assignment to 0 (it's by default)
>  - add Nicolas' tag (sent privately)
>  sound/soc/intel/boards/byt-max98090.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/byt-max98090.c
> b/sound/soc/intel/boards/byt-max98090.c
> index d9f81b8d915d..047be7fa0ce9 100644
> --- a/sound/soc/intel/boards/byt-max98090.c
> +++ b/sound/soc/intel/boards/byt-max98090.c
> @@ -67,20 +67,27 @@ static struct snd_soc_jack_pin hs_jack_pins[] = {
>  
>  static struct snd_soc_jack_gpio hs_jack_gpios[] = {
>  	{
> -		.name		= "hp-gpio",
> -		.idx		= 0,
> +		.name		= "hp",
>  		.report		= SND_JACK_HEADPHONE |
> SND_JACK_LINEOUT,
>  		.debounce_time	= 200,
>  	},
>  	{
> -		.name		= "mic-gpio",
> -		.idx		= 1,
> +		.name		= "mic",
>  		.invert		= 1,
>  		.report		= SND_JACK_MICROPHONE,
>  		.debounce_time	= 200,
>  	},
>  };
>  
> +static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
> +static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
> +
> +static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
> +	{ "hp-gpios", &hp_gpios, 1 },
> +	{ "mic-gpios", &mic_gpios, 1 },
> +	{},
> +};
> +
>  static int byt_max98090_init(struct snd_soc_pcm_runtime *runtime)
>  {
>  	int ret;
> @@ -140,8 +147,9 @@ static struct snd_soc_card byt_max98090_card = {
>  
>  static int byt_max98090_probe(struct platform_device *pdev)
>  {
> -	int ret_val = 0;
> +	struct device *dev = &pdev->dev;
>  	struct byt_max98090_private *priv;
> +	int ret_val;
>  
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
>  	if (!priv) {
> @@ -149,6 +157,10 @@ static int byt_max98090_probe(struct
> platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	ret_val = devm_acpi_dev_add_driver_gpios(dev->parent,
> acpi_byt_max98090_gpios);
> +	if (ret_val)
> +		dev_dbg(dev, "Unable to add GPIO mapping table\n");
> +
>  	byt_max98090_card.dev = &pdev->dev;
>  	snd_soc_card_set_drvdata(&byt_max98090_card, priv);
>  	ret_val = devm_snd_soc_register_card(&pdev->dev,
> &byt_max98090_card);
> @@ -158,7 +170,7 @@ static int byt_max98090_probe(struct
> platform_device *pdev)
>  		return ret_val;
>  	}
>  
> -	return ret_val;
> +	return 0;
>  }
>  
>  static int byt_max98090_remove(struct platform_device *pdev)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Applied "ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7c197881e163f34679b941c75500a6c85560b7c9 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Sat, 10 Jun 2017 19:37:41 +0300
Subject: [PATCH] ASoC: Intel: byt-max98090: Add GPIO ACPI mapping table

In order to make GPIO ACPI library stricter prepare users of
gpiod_get_index() to correctly behave when there no mapping is
provided by firmware.

Here we add explicit mapping between _CRS GpioIo() resources and
their names used in the driver.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Nicolas Porcel <nicolasporcel06@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/byt-max98090.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/byt-max98090.c b/sound/soc/intel/boards/byt-max98090.c
index d9f81b8d915d..047be7fa0ce9 100644
--- a/sound/soc/intel/boards/byt-max98090.c
+++ b/sound/soc/intel/boards/byt-max98090.c
@@ -67,20 +67,27 @@ static struct snd_soc_jack_pin hs_jack_pins[] = {
 
 static struct snd_soc_jack_gpio hs_jack_gpios[] = {
 	{
-		.name		= "hp-gpio",
-		.idx		= 0,
+		.name		= "hp",
 		.report		= SND_JACK_HEADPHONE | SND_JACK_LINEOUT,
 		.debounce_time	= 200,
 	},
 	{
-		.name		= "mic-gpio",
-		.idx		= 1,
+		.name		= "mic",
 		.invert		= 1,
 		.report		= SND_JACK_MICROPHONE,
 		.debounce_time	= 200,
 	},
 };
 
+static const struct acpi_gpio_params hp_gpios = { 0, 0, false };
+static const struct acpi_gpio_params mic_gpios = { 1, 0, false };
+
+static const struct acpi_gpio_mapping acpi_byt_max98090_gpios[] = {
+	{ "hp-gpios", &hp_gpios, 1 },
+	{ "mic-gpios", &mic_gpios, 1 },
+	{},
+};
+
 static int byt_max98090_init(struct snd_soc_pcm_runtime *runtime)
 {
 	int ret;
@@ -140,8 +147,9 @@ static struct snd_soc_card byt_max98090_card = {
 
 static int byt_max98090_probe(struct platform_device *pdev)
 {
-	int ret_val = 0;
+	struct device *dev = &pdev->dev;
 	struct byt_max98090_private *priv;
+	int ret_val;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
 	if (!priv) {
@@ -149,6 +157,10 @@ static int byt_max98090_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ret_val = devm_acpi_dev_add_driver_gpios(dev->parent, acpi_byt_max98090_gpios);
+	if (ret_val)
+		dev_dbg(dev, "Unable to add GPIO mapping table\n");
+
 	byt_max98090_card.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&byt_max98090_card, priv);
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_max98090_card);
@@ -158,7 +170,7 @@ static int byt_max98090_probe(struct platform_device *pdev)
 		return ret_val;
 	}
 
-	return ret_val;
+	return 0;
 }
 
 static int byt_max98090_remove(struct platform_device *pdev)
-- 
2.11.0