| Message ID | 1322772564-27343-3-git-send-email-swarren@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Dec 01, 2011 at 01:49:21PM -0700, Stephen Warren wrote: > - if (pdata && pdata->gpio_base) > - wm8903->gpio_chip.base = pdata->gpio_base; > - else > - wm8903->gpio_chip.base = -1; > + wm8903->gpio_chip.base = pdata->gpio_base; This will break existing users in counjuntion with the previous patch. Previously if the user provided platform data but left gpio_base as zero we'd use -1 and let gpiolib pick for us. Now instead the driver will take that zero and pass it on to gpiolib, probably failing as the SoC will have taken the low numbered GPIOs. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote at Thursday, December 01, 2011 5:28 PM: > On Thu, Dec 01, 2011 at 01:49:21PM -0700, Stephen Warren wrote: > > > - if (pdata && pdata->gpio_base) > > - wm8903->gpio_chip.base = pdata->gpio_base; > > - else > > - wm8903->gpio_chip.base = -1; > > + wm8903->gpio_chip.base = pdata->gpio_base; > > This will break existing users in counjuntion with the previous patch. > Previously if the user provided platform data but left gpio_base as zero > we'd use -1 and let gpiolib pick for us. Now instead the driver will > take that zero and pass it on to gpiolib, probably failing as the SoC > will have taken the low numbered GPIOs. Yes, I suppose that's true. However, I don't see it as a problem. Surely if the user provided pdata, it's their responsibility to fill it in correctly and completely. It seems a little random to take the pdata, and try to guess whether 0 means 0 or "I didn't set the value, so use the default". I think the same comment applies w.r.t to your comment on patch 2 (gpio_cfg); 0 is a perfectly legitimate value for the register; why should the driver double-guess that value and assume 0 means "don't touch the pin"?
On Thu, Dec 01, 2011 at 04:48:11PM -0800, Stephen Warren wrote: > Mark Brown wrote at Thursday, December 01, 2011 5:28 PM: > > This will break existing users in counjuntion with the previous patch. > > Previously if the user provided platform data but left gpio_base as zero > > we'd use -1 and let gpiolib pick for us. Now instead the driver will > > take that zero and pass it on to gpiolib, probably failing as the SoC > > will have taken the low numbered GPIOs. > Yes, I suppose that's true. However, I don't see it as a problem. > Surely if the user provided pdata, it's their responsibility to fill > it in correctly and completely. It seems a little random to take the > pdata, and try to guess whether 0 means 0 or "I didn't set the value, > so use the default". I think the same comment applies w.r.t to your No, that's not been the general approach as it avoids breaking existing users when you add new elements to the platform data and it means that users don't have to worry about every single field which is much more friendly as the number of fields gets larger. In the case of GPIOs it was frankly a bad idea to have 0 be a valid GPIO value in the first place; it makes the API that little bit more fiddly to work with. Device tree is much nicer in this regard as you can just omit properties which is unambiguous. > comment on patch 2 (gpio_cfg); 0 is a perfectly legitimate value for > the register; why should the driver double-guess that value and assume > 0 means "don't touch the pin"? Yeah, that's annoying. There's a reason why most of the chips do the write of zero by setting an out of bounds bit in the pdata. At least for the devices I deal with directly 0 is fortunately usually a nonsensical value to want to set anyway so it's not so important. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 8249571..04d2393 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1863,20 +1863,16 @@ static struct gpio_chip wm8903_template_chip = { .can_sleep = 1, }; -static void wm8903_init_gpio(struct snd_soc_codec *codec) +static void wm8903_init_gpio(struct snd_soc_codec *codec, + struct wm8903_platform_data *pdata) { struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec); - struct wm8903_platform_data *pdata = dev_get_platdata(codec->dev); int ret; wm8903->gpio_chip = wm8903_template_chip; wm8903->gpio_chip.ngpio = WM8903_NUM_GPIO; wm8903->gpio_chip.dev = codec->dev; - - if (pdata && pdata->gpio_base) - wm8903->gpio_chip.base = pdata->gpio_base; - else - wm8903->gpio_chip.base = -1; + wm8903->gpio_chip.base = pdata->gpio_base; ret = gpiochip_add(&wm8903->gpio_chip); if (ret != 0) @@ -1893,7 +1889,8 @@ static void wm8903_free_gpio(struct snd_soc_codec *codec) dev_err(codec->dev, "Failed to remove GPIOs: %d\n", ret); } #else -static void wm8903_init_gpio(struct snd_soc_codec *codec) +static void wm8903_init_gpio(struct snd_soc_codec *codec, + struct wm8903_platform_data *pdata) { } @@ -2050,7 +2047,7 @@ static int wm8903_probe(struct snd_soc_codec *codec) snd_soc_add_controls(codec, wm8903_snd_controls, ARRAY_SIZE(wm8903_snd_controls)); - wm8903_init_gpio(codec); + wm8903_init_gpio(codec, pdata); return ret; }
Modify wm8903_init_gpio() so that it's passed the pdata structure rather than extracting it from the platform device. This allows the caller to pass in a default pdata structure when the platform device didn't contain one. wm8903_init_gpio() now uses the centralized default gpio_base definition added in the previous patch. Based on work by John Bonesio, but significantly reworked since then. Signed-off-by: Stephen Warren <swarren@nvidia.com> --- sound/soc/codecs/wm8903.c | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-)