[2/2] sound: soc: wm8903: Cut gpio_base data

Message ID 20180918191233.17178-1-linus.walleij@linaro.org
State New
Headers show
Series
  • [1/2] sound: soc: wm8903: Pull in platform data
Related show

Commit Message

Linus Walleij Sept. 18, 2018, 7:12 p.m.
This variable is only used inside the file, and we should
never hard-code the GPIO base, so delete this.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 sound/soc/codecs/wm8903.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

-- 
2.17.1

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

Comments

Mark Brown Sept. 18, 2018, 7:52 p.m. | #1
On Tue, Sep 18, 2018 at 12:12:33PM -0700, Linus Walleij wrote:
> This variable is only used inside the file, and we should

> never hard-code the GPIO base, so delete this.


Adding the Cirrus people again...  hard coding the GPIO base is needed
for board file based systems if they're using GPIO numbers isn't it?

> 

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  sound/soc/codecs/wm8903.c | 8 +-------

>  1 file changed, 1 insertion(+), 7 deletions(-)

> 

> diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c

> index 23e43ff40ded..545512dc4c45 100644

> --- a/sound/soc/codecs/wm8903.c

> +++ b/sound/soc/codecs/wm8903.c

> @@ -144,7 +144,6 @@ struct wm8903_platform_data {

>  

>  	int micdet_delay;      /* Delay after microphone detection (ms) */

>  

> -	int gpio_base;

>  	u32 gpio_cfg[WM8903_NUM_GPIO]; /* Default register values for GPIO pin mux */

>  };

>  

> @@ -1876,17 +1875,12 @@ static const struct gpio_chip wm8903_template_chip = {

>  

>  static void wm8903_init_gpio(struct wm8903_priv *wm8903)

>  {

> -	struct wm8903_platform_data *pdata = wm8903->pdata;

>  	int ret;

>  

>  	wm8903->gpio_chip = wm8903_template_chip;

>  	wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO;

>  	wm8903->gpio_chip.parent = wm8903->dev;

> -

> -	if (pdata->gpio_base)

> -		wm8903->gpio_chip.base = pdata->gpio_base;

> -	else

> -		wm8903->gpio_chip.base = -1;

> +	wm8903->gpio_chip.base = -1;

>  

>  	ret = gpiochip_add_data(&wm8903->gpio_chip, wm8903);

>  	if (ret != 0)

> -- 

> 2.17.1

>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Linus Walleij Sept. 18, 2018, 11:51 p.m. | #2
On Tue, Sep 18, 2018 at 12:52 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 18, 2018 at 12:12:33PM -0700, Linus Walleij wrote:

> > This variable is only used inside the file, and we should

> > never hard-code the GPIO base, so delete this.

>

> Adding the Cirrus people again...


Sorry about all beginners mistakes :/

> hard coding the GPIO base is needed

> for board file based systems if they're using GPIO numbers isn't it?


It is needed if there are board files hardcoding GPIOs referencing
the global number range on this specific gpiochip.

Since there are no boards defining platform data (hence patch 1)
I assume all users are using device tree and thus dynamic
GPIO number assignment, so this can go.

Even if new board files are added they should use machine
descriptor tables for referencing the individual GPIO chip and
offset. In this case by the label "wm8903" and local offset.

Yours,
Linus Walleij
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Mark Brown Sept. 19, 2018, 12:09 a.m. | #3
On Tue, Sep 18, 2018 at 04:51:23PM -0700, Linus Walleij wrote:

> Since there are no boards defining platform data (hence patch 1)

> I assume all users are using device tree and thus dynamic

> GPIO number assignment, so this can go.


> Even if new board files are added they should use machine

> descriptor tables for referencing the individual GPIO chip and

> offset. In this case by the label "wm8903" and local offset.


I tend to worry about breaking out of tree users unless there's an
active reason to do it, is there something coming along - this looked
more like just a cleanup?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Linus Walleij Sept. 19, 2018, 2:07 a.m. | #4
On Tue, Sep 18, 2018 at 5:09 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 18, 2018 at 04:51:23PM -0700, Linus Walleij wrote:

>

> > Since there are no boards defining platform data (hence patch 1)

> > I assume all users are using device tree and thus dynamic

> > GPIO number assignment, so this can go.

>

> > Even if new board files are added they should use machine

> > descriptor tables for referencing the individual GPIO chip and

> > offset. In this case by the label "wm8903" and local offset.

>

> I tend to worry about breaking out of tree users unless there's an

> active reason to do it, is there something coming along - this looked

> more like just a cleanup?


This is to get rid of the global GPIO numberspace, little by little.
As long as we hardcode the base of the GPIO chip anywhere,
that numberspace prevails.

Yours,
Linus Walleij
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch

diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index 23e43ff40ded..545512dc4c45 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -144,7 +144,6 @@  struct wm8903_platform_data {
 
 	int micdet_delay;      /* Delay after microphone detection (ms) */
 
-	int gpio_base;
 	u32 gpio_cfg[WM8903_NUM_GPIO]; /* Default register values for GPIO pin mux */
 };
 
@@ -1876,17 +1875,12 @@  static const struct gpio_chip wm8903_template_chip = {
 
 static void wm8903_init_gpio(struct wm8903_priv *wm8903)
 {
-	struct wm8903_platform_data *pdata = wm8903->pdata;
 	int ret;
 
 	wm8903->gpio_chip = wm8903_template_chip;
 	wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO;
 	wm8903->gpio_chip.parent = wm8903->dev;
-
-	if (pdata->gpio_base)
-		wm8903->gpio_chip.base = pdata->gpio_base;
-	else
-		wm8903->gpio_chip.base = -1;
+	wm8903->gpio_chip.base = -1;
 
 	ret = gpiochip_add_data(&wm8903->gpio_chip, wm8903);
 	if (ret != 0)