From: Stanislav Brabec <hidden> Date: 2011-06-30 15:15:49
Marek Vasut wrote:
On Thursday, June 23, 2011 06:09:40 PM Stanislav Brabec wrote:
quoted
Hallo.
These Oops and kernel panic were observed on a Zaurus (spitz) machine
(ARMv5, PXA270).
Looks similar to "Re: [PATCH v2] Input: Make ADS7846 independent on regulator"
I don't see any trace in that thread. But yes, I was testing your patch
from this thread. Without it, ADS7846 does not work on spitz (ADS7846
does not have any dedicated regulator there). I want to
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
After sending the trace I was able to reproduced it several times by
attaching of external charger.
Then I tried to apply "[PATCH] MAX1111: Fix race condition causing NULL
pointer exception", connected charger that periodically disconnects and
not seen the crash again. No OOPS was seen after ~100 reconnects.
So I guess that MAX1111 AC voltage reading (via SPI) was involved in an
incorrect moment and race happened there and your MAX1111 race condition
fix fixes it.
--
Stanislav Brabec
http://www.penguin.cz/~utx
From: Marek Vasut <hidden> Date: 2011-06-30 15:09:58
On Thursday, June 30, 2011 04:45:18 PM Stanislav Brabec wrote:
Marek Vasut wrote:
quoted
On Thursday, June 23, 2011 06:09:40 PM Stanislav Brabec wrote:
quoted
Hallo.
These Oops and kernel panic were observed on a Zaurus (spitz) machine
(ARMv5, PXA270).
Looks similar to "Re: [PATCH v2] Input: Make ADS7846 independent on
regulator"
I don't see any trace in that thread. But yes, I was testing your patch
from this thread. Without it, ADS7846 does not work on spitz (ADS7846
does not have any dedicated regulator there). I want to
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
After sending the trace I was able to reproduced it several times by
attaching of external charger.
Then I tried to apply "[PATCH] MAX1111: Fix race condition causing NULL
pointer exception", connected charger that periodically disconnects and
not seen the crash again. No OOPS was seen after ~100 reconnects.
So I guess that MAX1111 AC voltage reading (via SPI) was involved in an
incorrect moment and race happened there and your MAX1111 race condition
fix fixes it.
It's not mine, it's from Pavel Herrmann actually ;-)
But yes, it's likely either this or the regulator stuff again (which I believed
was fixed).
From: Pavel Herrmann <hidden> Date: 2011-06-30 15:37:21
On Thursday, June 30, 2011 04:45:18 PM Stanislav Brabec wrote:
Then I tried to apply "[PATCH] MAX1111: Fix race condition causing NULL
pointer exception", connected charger that periodically disconnects and
not seen the crash again. No OOPS was seen after ~100 reconnects.
So I guess that MAX1111 AC voltage reading (via SPI) was involved in an
incorrect moment and race happened there and your MAX1111 race condition
fix fixes it.
Hi,
Are you using the first or second version of the patch? if the former, please
use v2 (sent a few days later), which has solved the same problem by using a
mutex instead of allocating message data on stack (which is not good for DMA)
as for the backstory, this crash ocurrs when a short (measured in time spent)
message was enqueued after a long message, so that the short one finished first
(the actual bug was present even if the long one finished first, but in that
case there was double complete() on the one completion instead of a NULL
dereference)
Pavel
From: Stanislav Brabec <hidden> Date: 2011-06-30 16:22:06
Pavel Herrmann wrote:
On Thursday, June 30, 2011 04:45:18 PM Stanislav Brabec wrote:
quoted
Then I tried to apply "[PATCH] MAX1111: Fix race condition causing NULL
quoted
So I guess that MAX1111 AC voltage reading (via SPI) was involved in an
incorrect moment and race happened there and your MAX1111 race condition
fix fixes it.
Are you using the first or second version of the patch? if the former, please
use v2 (sent a few days later), which has solved the same problem by using a
mutex instead of allocating message data on stack (which is not good for DMA)
as for the backstory, this crash ocurrs when a short (measured in time spent)
message was enqueued after a long message, so that the short one finished first
(the actual bug was present even if the long one finished first, but in that
case there was double complete() on the one completion instead of a NULL
dereference)
I guess that inserting of power supply initiates reading of voltages on
MAX1111. The long one may be touchscreen or LCD control.
--
Stanislav Brabec
http://www.penguin.cz/~utx
From: Igor Grinberg <hidden> Date: 2011-06-30 15:26:36
On 06/30/11 17:45, Stanislav Brabec wrote:
Marek Vasut wrote:
quoted
On Thursday, June 23, 2011 06:09:40 PM Stanislav Brabec wrote:
quoted
Hallo.
These Oops and kernel panic were observed on a Zaurus (spitz) machine
(ARMv5, PXA270).
Looks similar to "Re: [PATCH v2] Input: Make ADS7846 independent on regulator"
I don't see any trace in that thread. But yes, I was testing your patch
from this thread. Without it, ADS7846 does not work on spitz (ADS7846
does not have any dedicated regulator there). I want to
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
Please, don't...
I thought we've finished discussing the regulator issue...
Instead of patching the driver and the platform data and the board file,
you should do _either_ of the following:
1) add regulator definition for ads7846 into the board file
2) enable the CONFIG_REGULATOR_DUMMY in your kernel configuration
3) use regulator_use_dummy_regulator() call in the board file.
Using either of the above, you will not need the patch for ads7846, only for the board file.
--
Regards,
Igor.
From: Mark Brown <hidden> Date: 2011-06-30 16:00:07
On Thu, Jun 30, 2011 at 06:25:51PM +0300, Igor Grinberg wrote:
On 06/30/11 17:45, Stanislav Brabec wrote:
quoted
from this thread. Without it, ADS7846 does not work on spitz (ADS7846
does not have any dedicated regulator there). I want to
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
Please, don't...
I thought we've finished discussing the regulator issue...
Instead of patching the driver and the platform data and the board file,
you should do _either_ of the following:
1) add regulator definition for ads7846 into the board file
2) enable the CONFIG_REGULATOR_DUMMY in your kernel configuration
3) use regulator_use_dummy_regulator() call in the board file.
Using either of the above, you will not need the patch for ads7846, only for the board file.
Yes, we've been through this repeatedly. None of these options take any
appreciable time to implement, it'd be much more productive to just do
so.
From: Stanislav Brabec <hidden> Date: 2011-06-30 16:13:33
Igor Grinberg wrote:
On 06/30/11 17:45, Stanislav Brabec wrote:
I want to
quoted
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
Please, don't...
I thought we've finished discussing the regulator issue...
The discussion ended without a fix (at least for spitz). I seen two
proposals:
- As ADS7846 hardware does not require dedicated regulator, don't
require it in driver and fail only on platforms that have a dedicated
regulator.
- Use dummy regulators for all platforms without dedicated ADS7846
regulator.
1) add regulator definition for ads7846 into the board file
There is no dedicated regulator on spitz, ADS7846 uses common always-on
power supply.
2) enable the CONFIG_REGULATOR_DUMMY in your kernel configuration
3) use regulator_use_dummy_regulator() call in the board file.
OK, I will do 2 or 3. In the new kernel spitz has a regulator, but it is
not related to ADS7846. And it is actually broken:
I2C: i2c-0: PXA I2C adapter
Resetting I2C Controller Unit
(null): i2c_pxa_abort: called in slave mode
i2c i2c-1: adapter [pxa_i2c-i2c.1] registered
i2c 1-000c: uevent
isl6271a 1-000c: probe
i2c i2c-1: master_xfer[0] R, addr=0x0c, len=1
i2c i2c-1: setting to bus master
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000442, ICR=000007e0, IBMR=03
i2c i2c-1: state:i2c_pxa_irq_txempty:932: ISR=00000002, ICR=000007e0, IBMR=03
i2c i2c-1: Retrying transmission
i2c i2c-1: setting to bus master
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000045, ICR=000007e1, IBMR=00
i2c i2c-1: state:i2c_pxa_irq_txempty:932: ISR=00000005, ICR=000017ee, IBMR=00
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000087, ICR=000017e6, IBMR=00
print_constraints: vcc_core range: 850 <--> 1600 mV at 1350 mV
print_constraints: vcc_core range: 1100 mV
isl6271a 1-000c: Failed to set supply vcc_core
isl6271a 1-000c: failed to register isl6271a
isl6271a: probe of 1-000c failed with error -16
i2c i2c-1: client [isl6271a] registered with bus id 1-000c
I2C: i2c-1: PXA I2C adapter
...
cpufreq: Didn't find vcc_core regulator
--
Stanislav Brabec
http://www.penguin.cz/~utx
From: Igor Grinberg <hidden> Date: 2011-06-30 17:41:02
On 06/30/11 19:13, Stanislav Brabec wrote:
Igor Grinberg wrote:
quoted
On 06/30/11 17:45, Stanislav Brabec wrote:
I want to
quoted
add .needs_regulator bool to ads7846.c and send the patch again to the
list.
Please, don't...
I thought we've finished discussing the regulator issue...
The discussion ended without a fix (at least for spitz).
Well, the discussion was mainly about how the regulator API should be used.
The proposed patch, used the regulator API incorrectly.
If I recall correctly, in the end of the discussion,
Mark provided another option for configuring the regulator API,
which is number 3 below.
I seen two
proposals:
- As ADS7846 hardware does not require dedicated regulator, don't
require it in driver and fail only on platforms that have a dedicated
regulator.
The thing is that ads7846 chip itself just requires power supply
(as most of the peripheral chips), the one who decides if it will be switchable
is the board vendor and the vendor may use any source for the power supply.
It can be fixed, it can be some kind of discrete LDO or one of the power
supplies coming from the system PMIC.
The ads7846 driver provides the regulator functionality via the regulator API
and it is up to board code to provide the appropriate regulator definitions or
configure the regulator API so the ads7846 will function correctly.
- Use dummy regulators for all platforms without dedicated ADS7846
regulator.
IMO, this is the right way.
quoted
1) add regulator definition for ads7846 into the board file
There is no dedicated regulator on spitz, ADS7846 uses common always-on
power supply.
Mark,
Is there a kind of regulator for this case (except dummy)?
Some kind of fixed regulator which is not binded to any supply?
quoted
2) enable the CONFIG_REGULATOR_DUMMY in your kernel configuration
3) use regulator_use_dummy_regulator() call in the board file.
OK, I will do 2 or 3. In the new kernel spitz has a regulator, but it is
not related to ADS7846. And it is actually broken:
I2C: i2c-0: PXA I2C adapter
Resetting I2C Controller Unit
(null): i2c_pxa_abort: called in slave mode
i2c i2c-1: adapter [pxa_i2c-i2c.1] registered
i2c 1-000c: uevent
isl6271a 1-000c: probe
i2c i2c-1: master_xfer[0] R, addr=0x0c, len=1
i2c i2c-1: setting to bus master
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000442, ICR=000007e0, IBMR=03
i2c i2c-1: state:i2c_pxa_irq_txempty:932: ISR=00000002, ICR=000007e0, IBMR=03
i2c i2c-1: Retrying transmission
i2c i2c-1: setting to bus master
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000045, ICR=000007e1, IBMR=00
i2c i2c-1: state:i2c_pxa_irq_txempty:932: ISR=00000005, ICR=000017ee, IBMR=00
i2c i2c-1: state:i2c_pxa_handler:981: ISR=00000087, ICR=000017e6, IBMR=00
print_constraints: vcc_core range: 850 <--> 1600 mV at 1350 mV
print_constraints: vcc_core range: 1100 mV
isl6271a 1-000c: Failed to set supply vcc_core
isl6271a 1-000c: failed to register isl6271a
isl6271a: probe of 1-000c failed with error -16
i2c i2c-1: client [isl6271a] registered with bus id 1-000c
I2C: i2c-1: PXA I2C adapter
...
cpufreq: Didn't find vcc_core regulator
This looks like the regulator definitions for spitz are broken and need to be fixed.
--
Regards,
Igor.
From: Mark Brown <hidden> Date: 2011-06-30 18:01:44
On Thu, Jun 30, 2011 at 08:40:40PM +0300, Igor Grinberg wrote:
On 06/30/11 19:13, Stanislav Brabec wrote:
quoted
Igor Grinberg wrote:
quoted
quoted
I thought we've finished discussing the regulator issue...
quoted
The discussion ended without a fix (at least for spitz).
Well, the discussion was mainly about how the regulator API should be used.
The proposed patch, used the regulator API incorrectly.
If I recall correctly, in the end of the discussion,
Mark provided another option for configuring the regulator API,
which is number 3 below.
The discussion ended with you guys having the three different options
that would work, you just need to use one of them.
quoted
I seen two
proposals:
- As ADS7846 hardware does not require dedicated regulator, don't
require it in driver and fail only on platforms that have a dedicated
regulator.
The thing is that ads7846 chip itself just requires power supply
Right, and the regulator API hides the non-switchability of the supply
from the driver so there's no need for the driver to worry about how the
supplies are wired up. It just turns the regulator on when it needs it
and turns it off when it doesn't.
quoted
quoted
1) add regulator definition for ads7846 into the board file
There is no dedicated regulator on spitz, ADS7846 uses common always-on
power supply.
Is there a kind of regulator for this case (except dummy)?
Some kind of fixed regulator which is not binded to any supply?
This is just a fixed voltage regulator, support for that has been in the
kernel since the regulator API was merged. This is the best solution,
it ensures that you don't mistakenly activate dummy reglators for
supplies that really need software control.
From: Igor Grinberg <hidden> Date: 2011-06-30 20:50:44
On 06/30/11 21:01, Mark Brown wrote:
On Thu, Jun 30, 2011 at 08:40:40PM +0300, Igor Grinberg wrote:
quoted
On 06/30/11 19:13, Stanislav Brabec wrote:
quoted
quoted
I seen two
proposals:
- As ADS7846 hardware does not require dedicated regulator, don't
require it in driver and fail only on platforms that have a dedicated
regulator.
The thing is that ads7846 chip itself just requires power supply
Right, and the regulator API hides the non-switchability of the supply
from the driver so there's no need for the driver to worry about how the
supplies are wired up. It just turns the regulator on when it needs it
and turns it off when it doesn't.
quoted
quoted
quoted
1) add regulator definition for ads7846 into the board file
There is no dedicated regulator on spitz, ADS7846 uses common always-on
power supply.
Is there a kind of regulator for this case (except dummy)?
Some kind of fixed regulator which is not binded to any supply?
This is just a fixed voltage regulator, support for that has been in the
kernel since the regulator API was merged. This is the best solution,
it ensures that you don't mistakenly activate dummy reglators for
supplies that really need software control.
Right, just as I thought (I still haven't made myself familiar with all the
regulator API aspects).
Stanislav,
Can't you define a fixed voltage regulator for the ads7846 in spitz board file?
This shouldn't be a hard task and is the right solution from the regulator API POV.
--
Regards,
Igor.
From: Stanislav Brabec <hidden> Date: 2011-06-30 22:20:22
Igor Grinberg wrote:
Stanislav,
Can't you define a fixed voltage regulator for the ads7846 in spitz board file?
This shouldn't be a hard task and is the right solution from the regulator API POV.
Yes, I can.
ADS7846 needs more definitions in the platform file to work well on
spitz. The current platform definition is not sufficient and pointer
noise makes it unusable (e. g. 100 pixels jumps). I am comparing Lineo's
2.4 code with 2.6 algorithm and experimenting to get the best results.
--
________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx