| Message ID | 1357499640-13871-4-git-send-email-mikedunn@newsguy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/06/13 21:13, Mike Dunn wrote: > During initialization, the ac97 driver must obtain the gpio for the mfp that is > used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue > a warning and stack dump. Gpiolib may change this to a hard error soon. The > line is used during an ac97 warm reset for working around a hardware bug in the > controller. > > Tested on a palm treo 680 machine. > > Signed-off-by: Mike Dunn <mikedunn@newsguy.com> > --- > sound/arm/pxa2xx-ac97-lib.c | 10 +++++++++- > 1 files changed, 9 insertions(+), 1 deletions(-) > > diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c > index 1ecd0a66..416d2e3 100644 > --- a/sound/arm/pxa2xx-ac97-lib.c > +++ b/sound/arm/pxa2xx-ac97-lib.c > @@ -18,6 +18,7 @@ > #include <linux/delay.h> > #include <linux/module.h> > #include <linux/io.h> > +#include <linux/gpio.h> > > #include <sound/ac97_codec.h> > #include <sound/pxa2xx-lib.h> > @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev) > } > > if (cpu_is_pxa27x()) { > - /* Use GPIO 113 as AC97 Reset on Bulverde */ > + ret = gpio_request(reset_gpio, "pxa27x ac97 reset"); > + if (ret < 0) { > + pr_err("%s: gpio_request() failed: %d\n", > + __func__, ret); > + goto err_conf; > + } I think if we are fine with the request in the driver, then we should move the direction and value configuration also to the driver, but let the pxa27x_assert_ac97reset() only switch the AF. > pxa27x_assert_ac97reset(reset_gpio, 0); > ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK"); > if (IS_ERR(ac97conf_clk)) { > @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe); > > void pxa2xx_ac97_hw_remove(struct platform_device *dev) > { > + if (cpu_is_pxa27x()) > + gpio_free(reset_gpio); > GCR |= GCR_ACLINK_OFF; > free_irq(IRQ_AC97, NULL); > if (ac97conf_clk) {
On 01/07/13 11:36, Igor Grinberg wrote: > On 01/06/13 21:13, Mike Dunn wrote: >> During initialization, the ac97 driver must obtain the gpio for the mfp that is >> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue >> a warning and stack dump. Gpiolib may change this to a hard error soon. The >> line is used during an ac97 warm reset for working around a hardware bug in the >> controller. >> >> Tested on a palm treo 680 machine. >> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com> >> --- >> sound/arm/pxa2xx-ac97-lib.c | 10 +++++++++- >> 1 files changed, 9 insertions(+), 1 deletions(-) >> >> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c >> index 1ecd0a66..416d2e3 100644 >> --- a/sound/arm/pxa2xx-ac97-lib.c >> +++ b/sound/arm/pxa2xx-ac97-lib.c >> @@ -18,6 +18,7 @@ >> #include <linux/delay.h> >> #include <linux/module.h> >> #include <linux/io.h> >> +#include <linux/gpio.h> >> >> #include <sound/ac97_codec.h> >> #include <sound/pxa2xx-lib.h> >> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev) >> } >> >> if (cpu_is_pxa27x()) { >> - /* Use GPIO 113 as AC97 Reset on Bulverde */ >> + ret = gpio_request(reset_gpio, "pxa27x ac97 reset"); >> + if (ret < 0) { >> + pr_err("%s: gpio_request() failed: %d\n", >> + __func__, ret); >> + goto err_conf; >> + } > > I think if we are fine with the request in the driver, > then we should move the direction and value configuration also > to the driver, but let the pxa27x_assert_ac97reset() only switch > the AF. Also this patch should be combined with 2/4. > >> pxa27x_assert_ac97reset(reset_gpio, 0); >> ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK"); >> if (IS_ERR(ac97conf_clk)) { >> @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe); >> >> void pxa2xx_ac97_hw_remove(struct platform_device *dev) >> { >> + if (cpu_is_pxa27x()) >> + gpio_free(reset_gpio); >> GCR |= GCR_ACLINK_OFF; >> free_irq(IRQ_AC97, NULL); >> if (ac97conf_clk) { >
On 01/07/2013 01:36 AM, Igor Grinberg wrote: > On 01/06/13 21:13, Mike Dunn wrote: >> During initialization, the ac97 driver must obtain the gpio for the mfp that is >> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue >> a warning and stack dump. Gpiolib may change this to a hard error soon. The >> line is used during an ac97 warm reset for working around a hardware bug in the >> controller. >> >> Tested on a palm treo 680 machine. >> >> Signed-off-by: Mike Dunn <mikedunn@newsguy.com> >> --- >> sound/arm/pxa2xx-ac97-lib.c | 10 +++++++++- >> 1 files changed, 9 insertions(+), 1 deletions(-) >> >> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c >> index 1ecd0a66..416d2e3 100644 >> --- a/sound/arm/pxa2xx-ac97-lib.c >> +++ b/sound/arm/pxa2xx-ac97-lib.c >> @@ -18,6 +18,7 @@ >> #include <linux/delay.h> >> #include <linux/module.h> >> #include <linux/io.h> >> +#include <linux/gpio.h> >> >> #include <sound/ac97_codec.h> >> #include <sound/pxa2xx-lib.h> >> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev) >> } >> >> if (cpu_is_pxa27x()) { >> - /* Use GPIO 113 as AC97 Reset on Bulverde */ >> + ret = gpio_request(reset_gpio, "pxa27x ac97 reset"); >> + if (ret < 0) { >> + pr_err("%s: gpio_request() failed: %d\n", >> + __func__, ret); >> + goto err_conf; >> + } > > I think if we are fine with the request in the driver, > then we should move the direction and value configuration also > to the driver, but let the pxa27x_assert_ac97reset() only switch > the AF. Well, I won't make a fuss on this point, but with code that runs very infrequently, I thought that the cost of a call to gpio_direction_output() was worth the clarity and insurance. Thanks, Mike
On 01/07/13 16:10, Mike Dunn wrote: > On 01/07/2013 01:36 AM, Igor Grinberg wrote: >> On 01/06/13 21:13, Mike Dunn wrote: >>> During initialization, the ac97 driver must obtain the gpio for the mfp that is >>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue >>> a warning and stack dump. Gpiolib may change this to a hard error soon. The >>> line is used during an ac97 warm reset for working around a hardware bug in the >>> controller. >>> >>> Tested on a palm treo 680 machine. >>> >>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com> >>> --- >>> sound/arm/pxa2xx-ac97-lib.c | 10 +++++++++- >>> 1 files changed, 9 insertions(+), 1 deletions(-) >>> >>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c >>> index 1ecd0a66..416d2e3 100644 >>> --- a/sound/arm/pxa2xx-ac97-lib.c >>> +++ b/sound/arm/pxa2xx-ac97-lib.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/delay.h> >>> #include <linux/module.h> >>> #include <linux/io.h> >>> +#include <linux/gpio.h> >>> >>> #include <sound/ac97_codec.h> >>> #include <sound/pxa2xx-lib.h> >>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev) >>> } >>> >>> if (cpu_is_pxa27x()) { >>> - /* Use GPIO 113 as AC97 Reset on Bulverde */ >>> + ret = gpio_request(reset_gpio, "pxa27x ac97 reset"); >>> + if (ret < 0) { >>> + pr_err("%s: gpio_request() failed: %d\n", >>> + __func__, ret); >>> + goto err_conf; >>> + } >> >> I think if we are fine with the request in the driver, >> then we should move the direction and value configuration also >> to the driver, but let the pxa27x_assert_ac97reset() only switch >> the AF. > > > Well, I won't make a fuss on this point, but with code that runs very > infrequently, I thought that the cost of a call to gpio_direction_output() was > worth the clarity and insurance. This is not about how frequent the code runs, but the fact that the arch code relies on sound code (outside of arch) to request the GPIO for it. I would recommend to place the GPIO request call along with the direction/value calls - in the same subsystem. Also, the patch ordering is not good: patch 2 drives the GPIO, and patch 3 requests it, whereas it should be the other way around, so I would recommend to combine both into one.
On 01/07/2013 07:38 AM, Igor Grinberg wrote: > On 01/07/13 16:10, Mike Dunn wrote: [..] >> >> Well, I won't make a fuss on this point, but with code that runs very >> infrequently, I thought that the cost of a call to gpio_direction_output() was >> worth the clarity and insurance. > > This is not about how frequent the code runs, but the fact that the arch code > relies on sound code (outside of arch) to request the GPIO for it. > I would recommend to place the GPIO request call along with the > direction/value calls - in the same subsystem. OK. This is in some shared pxa code called by the ac97 driver during initialization, where I earlier placed the gpio_request() calls only. > > Also, the patch ordering is not good: > patch 2 drives the GPIO, and patch 3 requests it, > whereas it should be the other way around, so I would recommend to > combine both into one. Agreed. Thanks, Mike
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c index 1ecd0a66..416d2e3 100644 --- a/sound/arm/pxa2xx-ac97-lib.c +++ b/sound/arm/pxa2xx-ac97-lib.c @@ -18,6 +18,7 @@ #include <linux/delay.h> #include <linux/module.h> #include <linux/io.h> +#include <linux/gpio.h> #include <sound/ac97_codec.h> #include <sound/pxa2xx-lib.h> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev) } if (cpu_is_pxa27x()) { - /* Use GPIO 113 as AC97 Reset on Bulverde */ + ret = gpio_request(reset_gpio, "pxa27x ac97 reset"); + if (ret < 0) { + pr_err("%s: gpio_request() failed: %d\n", + __func__, ret); + goto err_conf; + } pxa27x_assert_ac97reset(reset_gpio, 0); ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK"); if (IS_ERR(ac97conf_clk)) { @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe); void pxa2xx_ac97_hw_remove(struct platform_device *dev) { + if (cpu_is_pxa27x()) + gpio_free(reset_gpio); GCR |= GCR_ACLINK_OFF; free_irq(IRQ_AC97, NULL); if (ac97conf_clk) {
During initialization, the ac97 driver must obtain the gpio for the mfp that is used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue a warning and stack dump. Gpiolib may change this to a hard error soon. The line is used during an ac97 warm reset for working around a hardware bug in the controller. Tested on a palm treo 680 machine. Signed-off-by: Mike Dunn <mikedunn@newsguy.com> --- sound/arm/pxa2xx-ac97-lib.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)