linux 2.6.34 bug

DISCUSSION Marek Vasut 4 participants 4 messages Jul 8, 2010 – Jul 13, 2010 1 message not in the archive

Subsystems (3)

Arm (32-bit) Arm sub-architectures PXA2xx/PXA3xx support

lore

Share & tools

lore

Marek Vasut

author

2010-07-08 16:42 UTC #1 lore raw

In reply to a message not in the archive

Dne ?t 8. ?ervence 2010 17:36:21 dylan cristiani napsal(a):

Quoted 11 lines

Hi Marek,
we were in touch few moths ago about a ucb1400 touchscreen, issue if you
remember: you sent me a patch to pass the platform specific codec irq, through ucb1400_pdata; after that i went from 3.6.32 to 2.6.34 and i noticed that your patch was into mainline; in this kernel, related to the ucb1400 codec again, i noticed one bug about the ac97 codec reset that is bulverde platform specific: i don't know hot to submit a patch and to whom to submit it, so i bother, you sorry in advance....; in
particular: in new kernel the function
"void pxa27x_assert_ac97reset(int reset_gpio, int on)"
has been moved to arch/arm/mach-pxa/pxa27x.c file:

CCing linux-arm-kernel and Eric.

btw. to submit patch :

git add <list of changed files>

git commit -s
<editor will pop up, type the commit message>

git log # here check the hash of the last commit before yours

git send-email --to="linux-arm-kernel at lists.infradead.org" -- cc="eric.y.miao at gmail.com" --cc="marek.vasut at gmail.com" <the hash from previous step>

That's probably the easiest way :)

Quoted 22 lines

......

static unsigned long ac97_reset_config[] = {
	GPIO95_AC97_nRESET,
	GPIO95_GPIO,
	GPIO113_AC97_nRESET,
	GPIO113_GPIO,
};

void pxa27x_assert_ac97reset(int reset_gpio, int on)

{
	if (reset_gpio == 113)
		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
				       &ac97_reset_config[1], 1);

	if (reset_gpio == 95)
		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
				       &ac97_reset_config[3], 1);
}

EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
.......

Quoted 7 lines

but, as it seems to me, there are two bugs: the first is the cross exchange of the position of the
	'if (reset_gpio == 113)'
with position of
	'if (reset_gpio == 95)'
with regard to the position of the members of the array "static unsigned long ac97_reset_config[]"

Seems like a good catch, Eric ?

Quoted 34 lines

The 2nd one regards the "pxa2xx_mfp_config(..)" function's call at sound/arm/pxa2xx-ac97-lib.c line 340:
	if (cpu_is_pxa27x()) {
		/* Use GPIO 113 as AC97 Reset on Bulverde */
		pxa27x_assert_ac97reset(reset_gpio, 0);
		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
		.....
so, if the second param is 0, (as the comment /**/ states) you want to use gpio 113 as AC97 reset (that is the Alternative Function 2 of the pin), so the function call into arch/arm/mach-pxa/pxa27x.c has to be:
	if (reset_gpio == 113)
		pxa2xx_mfp_config(on ? &ac97_reset_config[3] :
				       &ac97_reset_config[2], 1);
instead the current:
	if (reset_gpio == 113)
		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
				       &ac97_reset_config[3], 1);
i noticed that you can solve the whole bugs just moving the array members of "ac97_reset_config[]" into this:
static unsigned long ac97_reset_config[] = {
	GPIO113_GPIO,
	GPIO113_AC97_nRESET,
	GPIO95_GPIO,
	GPIO95_AC97_nRESET,
};

Quoted 6 lines

i tryed this and for me it works; i'm sorry again to bother you but i don't really know how to submit a bug report so maybe you can do this if it sounds good to your expert ears
tks and sorry for the noise and don't hate me if i wrote some garbage stuff...

Eric, care to explain? I see a valid point here.
Thanks

Uwe Kleine-König

2010-07-08 19:22 UTC #2 lore raw

Hello,

Quoted 22 lines · Marek Vasut

Dne ?t 8. ?ervence 2010 17:36:21 dylan cristiani napsal(a):
Quoted 11 lines
Hi Marek,
we were in touch few moths ago about a ucb1400 touchscreen, issue if you
remember: you sent me a patch to pass the platform specific codec irq, through ucb1400_pdata; after that i went from 3.6.32 to 2.6.34 and i noticed that your patch was into mainline; in this kernel, related to the ucb1400 codec again, i noticed one bug about the ac97 codec reset that is bulverde platform specific: i don't know hot to submit a patch and to whom to submit it, so i bother, you sorry in advance....; in
particular: in new kernel the function
"void pxa27x_assert_ac97reset(int reset_gpio, int on)"
has been moved to arch/arm/mach-pxa/pxa27x.c file:
CCing linux-arm-kernel and Eric.
btw. to submit patch :
git add <list of changed files>
git commit -s
<editor will pop up, type the commit message>

Quoted 7 lines

git log # here check the hash of the last commit before yours
git send-email --to="linux-arm-kernel at lists.infradead.org" -- cc="eric.y.miao at gmail.com" --cc="marek.vasut at gmail.com" <the hash from previous step>
That's probably the easiest way :)

No. Instead of checking the hash of the last commit via git log, you can simply pass "HEAD~1" instead of the actual hash.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Eric Miao

2010-07-13 01:50 UTC #3 lore raw

In reply to Marek Vasut

Quoted 94 lines · Marek Vasut

Dne ?t 8. ?ervence 2010 17:36:21 dylan cristiani napsal(a):
Quoted 11 lines
Hi Marek,
we were in touch few moths ago about a ucb1400 touchscreen, issue if you
remember: you sent me a patch to pass the platform specific codec irq, through ucb1400_pdata; after that i went from 3.6.32 to 2.6.34 and i noticed that your patch was into mainline; in this kernel, related to the ucb1400 codec again, i noticed one bug about the ac97 codec reset that is bulverde platform specific: i don't know hot to submit a patch and to whom to submit it, so i bother, you sorry in advance....; in
particular: in new kernel the function
"void pxa27x_assert_ac97reset(int reset_gpio, int on)"
has been moved to arch/arm/mach-pxa/pxa27x.c file:
CCing linux-arm-kernel and Eric.
btw. to submit patch :
git add <list of changed files>
git commit -s
<editor will pop up, type the commit message>
git log # here check the hash of the last commit before yours
git send-email --to="linux-arm-kernel at lists.infradead.org" -- cc="eric.y.miao at gmail.com" --cc="marek.vasut at gmail.com" <the hash from previous step>
That's probably the easiest way :)
Quoted 22 lines
......
static unsigned long ac97_reset_config[] = {
? ? ? GPIO95_AC97_nRESET,
? ? ? GPIO95_GPIO,
? ? ? GPIO113_AC97_nRESET,
? ? ? GPIO113_GPIO,
};
void pxa27x_assert_ac97reset(int reset_gpio, int on) {
? ? ? if (reset_gpio == 113)
? ? ? ? ? ? ? pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&ac97_reset_config[1], 1);
? ? ? if (reset_gpio == 95)
? ? ? ? ? ? ? pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&ac97_reset_config[3], 1); }
EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
.......
Quoted 7 lines
but, as it seems to me, there are two bugs: the first is the cross exchange of the position of the
? ? ? 'if (reset_gpio == 113)'
with position of
? ? ? 'if (reset_gpio == 95)'
with regard to the position of the members of the array "static unsigned long ac97_reset_config[]"
Seems like a good catch, Eric ?
Quoted 34 lines
The 2nd one regards the "pxa2xx_mfp_config(..)" function's call at sound/arm/pxa2xx-ac97-lib.c line 340:
? ? ? if (cpu_is_pxa27x()) {
? ? ? ? ? ? ? /* Use GPIO 113 as AC97 Reset on Bulverde */ ? ? ? ? ? ? ? pxa27x_assert_ac97reset(reset_gpio, 0); ? ? ? ? ? ? ? ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK"); ? ? ? ? ? ? ? .....
so, if the second param is 0, (as the comment /**/ states) you want to use gpio 113 as AC97 reset (that is the Alternative Function 2 of the pin), so the function call into arch/arm/mach-pxa/pxa27x.c has to be:
? ? ? if (reset_gpio == 113)
? ? ? ? ? ? ? pxa2xx_mfp_config(on ? &ac97_reset_config[3] :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&ac97_reset_config[2], 1);
instead the current:
? ? ? if (reset_gpio == 113)
? ? ? ? ? ? ? pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&ac97_reset_config[3], 1);
i noticed that you can solve the whole bugs just moving the array members of "ac97_reset_config[]" into this:
static unsigned long ac97_reset_config[] = {
? ? ? GPIO113_GPIO,
? ? ? GPIO113_AC97_nRESET,
? ? ? GPIO95_GPIO,
? ? ? GPIO95_AC97_nRESET,
};

Quoted 8 lines

Quoted 6 lines
i tryed this and for me it works; i'm sorry again to bother you but i don't really know how to submit a bug report so maybe you can do this if it sounds good to your expert ears
tks and sorry for the noise and don't hate me if i wrote some garbage stuff...
Eric, care to explain? I see a valid point here.

It was my bad. Fixed updated as below. Dylan, please help review:

[ARM] pxa: fix incorrect order of AC97 reset pin configs

Reported-by Dylan Cristiani <[email protected]>

Signed-off-by Eric Miao <[email protected]>

arch/arm/mach-pxa/pxa27x.c +3 -3

index 0af3617..c059dac 100644
@@ -41,10 +41,10 @@ void pxa27x_clear_otgph(void)
 EXPORT_SYMBOL(pxa27x_clear_otgph);
 
 static unsigned long ac97_reset_config[] = {
-	GPIO95_AC97_nRESET,
-	GPIO95_GPIO,
-	GPIO113_AC97_nRESET,
 	GPIO113_GPIO,
+	GPIO113_AC97_nRESET,
+	GPIO95_GPIO,
+	GPIO95_AC97_nRESET,
 };
 
 void pxa27x_assert_ac97reset(int reset_gpio, int on)

Dylan Cristiani

2010-07-13 10:44 UTC #4 lore raw

Quoted 16 lines · On Tue, 13 Jul 2010 09:50:56 +0800 Eric Miao <[email protected]> wrote:

It was my bad. Fixed updated as below. Dylan, please help review:
[ARM] pxa: fix incorrect order of AC97 reset pin configs
Reported-by Dylan Cristiani <[email protected]>
Signed-off-by Eric Miao <[email protected]>
arch/arm/mach-pxa/pxa27x.c +0 -0
index 0af3617..c059dac 100644
@@ -41,10 +41,10 @@ void pxa27x_clear_otgph(void)
 EXPORT_SYMBOL(pxa27x_clear_otgph);
 
 static unsigned long ac97_reset_config[] = {

arch/arm/mach-pxa/pxa27x.c void pxa27x_clear_otgph(void) · line 50

Quoted 10 lines

arch/arm/mach-pxa/pxa27x.c +3 -3

-	GPIO95_AC97_nRESET,
-	GPIO95_GPIO,
-	GPIO113_AC97_nRESET,
 	GPIO113_GPIO,
+	GPIO113_AC97_nRESET,
+	GPIO95_GPIO,
+	GPIO95_AC97_nRESET,
 };
 
 void pxa27x_assert_ac97reset(int reset_gpio, int on)

for me is ok: ACK!