* [PATCH] crypto: fix FTBFS with ARM SHA1-asm and THUMB2_KERNEL
From: Nicolas Pitre @ 2013-01-21 22:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358787763-1226-1-git-send-email-matt@genesi-usa.com>
On Mon, 21 Jan 2013, Matt Sealey wrote:
> The optimized assembler SHA1 code for ARM does not conform to Thumb2
> register usage requirements, so it cannot be built when the kernel is
> configured with THUMB2_KERNEL.
>
> Fix the FTBFS for now by preventing misconfigurations of the kernel.
>
> Signed-off-by: Matt Sealey <matt@genesi-usa.com>
A .arm directive at the top of the assembly code would be a better
"fix", as that wouldn't reduce functionality.
Yet, I'd invite you to have a look at commit 638591cd7b in linux-next.
Nicolas
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: Russell King - ARM Linux @ 2013-01-21 22:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201301212230.07393.arnd@arndb.de>
On Mon, Jan 21, 2013 at 10:30:07PM +0000, Arnd Bergmann wrote:
> On Monday 21 January 2013, Matt Sealey wrote:
> > So is that a bug in that it is not available to ARM right now, a bug
> > in that it would be impossible for anyone on ARM to have ever tested
> > this code, or a bug in that it should NEVER be enabled for ARM for
> > some reason? John? Ingo? :)
> >
>
> I think it's a bug that it's not available. That does not look intentional.
What's a bug? kernel/Kconfig.hz not being available? No, it's
intentional. (See my replies).
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: Russell King - ARM Linux @ 2013-01-21 22:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50FDBEAC.2050606@linaro.org>
On Mon, Jan 21, 2013 at 02:18:20PM -0800, John Stultz wrote:
> So we used to have the ACTHZ code to handle error from the HZ rate
> requested and the HZ rate possible given the underlying hardware. That's
> been moved to the register_refined_jiffies(), but do you have a sense if
> there a reason it couldn't be used? I don't quite recall the bounds at
> this second, so ~7% error might very well be too large.
>
> So yes, I suspect these sorts of platforms, where there are no modern
> clocksource/clockevent driver, as well as further constraints (like
> specific HZ) are likely not good candidates for a multi-arch build.
In this particular case, EBSA110 is not a candidate for multi-arch
build anyway, because it's ARMv4 and we're only really bothering with
ARMv6 and better.
Not only that, but the IO stuff on it is sufficiently obscure and
non-standard...
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: Russell King - ARM Linux @ 2013-01-21 22:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKGA1bkZ4sN_eXAk1nUzvGE6cvvQ+b+jnTezTM863qFNL8NqWg@mail.gmail.com>
On Mon, Jan 21, 2013 at 04:20:14PM -0600, Matt Sealey wrote:
> I am sorry it sounded if I was being high and mighty about not being
> able to select my own HZ (or being forced by Exynos to be 200 or by
> not being able to test an Exynos board, forced to default to 100). My
> real "grievance" here is we got a configuration item for the scheduler
> which is being left out of ARM configurations which *can* use high
> resolution timers, but I don't know if this is a real problem or not,
> hence asking about it, and that HZ=100 is the ARM default whether we
> might be able to select that or not.. which seems low.
Well, I have a versatile platform here. It's the inteligence behind
the power control system for booting the boards on the nightly tests
(currently disabled because I'm waiting for my main server to lock up
again, and I need to use one of the serial ports for that.)
The point is, it talks via I2C to a load of power monitors to read
samples out. It does this at sub-100Hz intervals. Yet the kernel is
built with HZ=100. NO_HZ=y and highres timers are enabled... works
fine.
So, no, HZ=100 is not a limit in that scenario. With NO_HZ=y and
highres timers, it all works with epoll() - you get the interval that
you're after. I've verified this with calls to gettimeofday() and
the POSIX clocks.
> HZ=250 is the "current" kernel default if you don't touch anything, it
> seems, apologies for thinking it was HZ=100.
Actually, it always used to be 100Hz on everything, including x86.
It got upped when there were interactivity issues... which haven't
been reported on ARM - so why change something that we know works and
everyone is happy with?
> And that is too high for
> EBSA110 and a couple of other boards, especially where HZ must equal
> some exact divisor being pumped right into some timer unit.
EBSA110 can do 250Hz, but it'll mean manually recalculating the timer
arithmetic - because it's not a "reloading" counter - software has to
manually reload it, and you have to take account of how far it's
rolled over to get anything close to a regular interrupt rate which
NTP is happy with. And believe me, it used to be one of two main NTP
broadcasting servers on my network, so I know it works.
> Understood. Surely the correct divisor should be *derived* from HZ and
> not just dumped into the timer though, so HZ being set to an exact
> divisor (but a round-down-to-acceptable-value) is kind of a hacky
> concept..?
No. See above. It's not a simple bit of maths. You need to know how
fast the CPU runs, and how many instructions it takes to read the
current value, modify it, write it back and factor that into the
calculation. Get it wrong - by even as little as one count - and the
error is too large, and NTP fails to sync.
> For the global kernel guys, I'd ask what is the reasoning for using
> HZ=250 by default, I wonder? It seems like this number is from the
> dark ages (pre-git, pre-bitkeeper, maybe pre-recorded history ;) and
> the reason is lost. Why not HZ=100 or HZ=300 (if the help text is to
> be believed, and it is probably older than God, HZ=300 is great for
> playing back NTSC-format video.. :)? I can side with you on the
> premise that in actual fact, defining a default HZ value in the
> non-arch-specific kernel proper is a little quirky and it should be
> something the arches do themselves (i.e. move the default-setting
> stuff at the end into the arch/*/Kconfig - I would expect that now
> i386 CPU support is gone from arch/x86, there's potentially a better
> value than HZ=250 for the default?).
>From what I remember, the history is that HZ used to be 100. Then it
became 1000 as an experiment to do with desktop interactivity. That
was found to be too heavy, so it was then dropped by a factor of 4 as
a compromise.
That's why kernel/Kconfig.hz has 100, 250 and 1000 - those are the
values which were tried on x86 many years ago.
>
> Anyway, a patch for ARM could perhaps end up like this:
>
> ~~
> if ARCH_MULTIPLATFORM
> source kernel/Kconfig.hz
> else
> HZ
> default 100
> endif
>
> HZ
> default 200 if ARCH_EBSA110 || ARCH_ETC_ETC || ARCH_UND_SO_WEITER
> # any previous platform definitions where *really* required here.
> # but not default 100 since it would override kernel/Kconfig.hz every time
That doesn't work - if you define the same symbol twice, one definition
takes priority over the other (I don't remember which way it works).
They don't accumulate.
> Which preserves all previous behaviors on all possible ARM arch
> combinations, but where no reasonable override is set.. Kconfig.hz is
> king. I cannot imagine any situation except for AT91 or OMAP could not
> do this in their own {mach,plat}-*/Kconfigs and not in the core
> config, which cleans up the extra HZ block.
Because... it simply doesn't work like that. Try it and check to see
what Kconfig produces.
We know this, because our FRAME_POINTER config overrides the generic
one - not partially, but totally and utterly in every way.
> Could we also at least agree that if EBSA110 can handle HZ=200 with a
> 16-bit timer, or HZ=128 for OMAP and that AT91 will override it to 100
> on it's own, then that "default 100" is overly restrictive and we
> could remove it, allowing each {mach,plat}-*/Kconfig owner to
> investigate and find the correct HZ value and implement an override or
> selection, or just allow free configuration?
I just don't see how that's remotely possible.
^ permalink raw reply
* [PATCH 3/6] arm: kconfig: don't select TWD with local timer for Armada 370/XP
From: Gregory CLEMENT @ 2013-01-21 22:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201301211831.45947.arnd@arndb.de>
On 01/21/2013 07:31 PM, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> @@ -1624,7 +1624,7 @@ config LOCAL_TIMERS
>> bool "Use local timer interrupts"
>> depends on SMP
>> default y
>> - select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
>> + select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)
>>
>
> Your change is fine, but I just noticed that this line is asking for trouble
> when we enable multipleform support for MSM and/or EXYNOS.
>
> Also, I wonder if we should change this somehow in 3.8, because it seems
> that for a multiplatform kernel including armadaxp and e.g. versatile express,
> you have no ARM_TWD support in the kernel, which seems wrong. Is there any
> reason we can't enable the ARM_TWD code to be built-in on platforms that
> don't use it?
I don't see a strong reason to not enable it if we don't use it. My concern
was that I don't need it so I didn't want to include it and generating extra
code for nothing. Then just after having sent this patch set, I received your
patch set about build regression in 3.8 and especially the part about
CONFIG_MULTIPLATFORM made me realized that it could be a problem.
>
> Maybe it can be written as
>
> config LOCAL_TIMERS
> bool "Use local timer interrupts"
> depends on SMP
> default y
>
> config HAVE_ARM_TWD
> depends on LOCAL_TIMERS
> default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
So in this case why not written something like this:
default ARCH_MULTIPLATFORM || (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT && !ARMADA_370_XP_TIMER)
> default y
I am not a kconfig expert, but won't this line set HAVE_ARM_TWD to 'y' whatever
the result of the previous line?
>
> This will still be slightly wrong (generating extra code) on a multiplatform
> kernel that has no platform other than MSM or EXYNOS, but the other alternative
> would be that each platform with TWD support has to select HAVE_ARM_TWD itself.
Gregory
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: John Stultz @ 2013-01-21 22:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKGA1b=D5CNikijoOAk5xP6TC-nB_gEGtS05ncrPR68sQTMrxw@mail.gmail.com>
On 01/21/2013 01:14 PM, Matt Sealey wrote:
> On Mon, Jan 21, 2013 at 3:00 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On 01/21/2013 12:41 PM, Arnd Bergmann wrote:
>>> Right. It's pretty clear that the above logic does not work
>>> with multiplatform. Maybe we should just make ARCH_MULTIPLATFORM
>>> select NO_HZ to make the question much less interesting.
>> Although, even with NO_HZ, we still have some sense of HZ.
> I wonder if you can confirm my understanding of this by the way? The
> way I think this works is;
>
> CONFIG_HZ on it's own defines the rate at which the kernel wakes up
> from sleeping on the job, and checks for current or expired timer
> events such that it can do things like schedule_work (as in
> workqueues) or perform scheduler (as in processes/tasks) operations.
CONFIG_HZ defines the length of a jiffy.
In the absence of NOHZ and HRT, HZ defines how frequently the
timer/scheduler tick will fire.
> CONFIG_NO_HZ turns on logic which effectively only wakes up at a
> *maximum* of CONFIG_HZ times per second, but otherwise will go to
> sleep and stay that way if no events actually happened (so, we rely on
> a timer interrupt popping up).
NOHZ adds logic which basically allows us to skip ticks if the cpu is idle.
And HRT adds logic which allows us to fire timers more frequently then HZ.
> In this case, no matter whether CONFIG_HZ=1000 or CONFIG_HZ=250 (for
> example) combined with CONFIG_NO_HZ and less than e.g. 250 things
> happening per second will wake up "exactly" the same number of times?
Ideally, if both systems are completely idle, they may see similar
number of actual interrupts.
But when the cpus are running processes, the HZ=1000 system will see
more frequent interrupts, since the timer/scheduler interrupt will jump
in 4 times more frequently.
> CONFIG_HZ=1000 with CONFIG_NO_HZ would be an effective, all-round
> solution here, then, and CONFIG_HZ=100 should be a reasonable default
> (as it is anyway with an otherwise-unconfigured kernel on any other
> platform) for !CONFIG_NO_HZ.
Eeehhh... I'm not sure this is follows.
>>
>> Yea, as far as timekeeping is concerned, we shouldn't be HZ dependent (and
>> the register_refined_jiffies is really only necessary if you're not
>> expecting a proper clocksource to eventually be registered), assuming the
>> hardware can do something close to the HZ value requested.
>>
>> So I'd probably want to hear about what history caused the specific 200 HZ
>> selections, as I suspect there's actual hardware limitations there. So if
>> you can not get actual timer ticks any faster then 200 HZ on that hardware,
>> setting HZ higher could cause some jiffies related timer trouble (ie: if the
>> kernel thinks HZ is 1000 but the hardware can only do 200, that's a
>> different problem then if the hardware actually can only do 999.8 HZ). So
>> things like timer-wheel timeouts may not happen when they should.
>>
>> I suspect the best approach for multi-arch in those cases may be to select
>> HZ=100
> As above, or "not select anything at all" since HZ=100 if you don't
> touch anything, right?
Well, Russell brought up a case that doesn't handle this. If a system
*can't* do HZ=100, but can do HZ=200.
Though there are hacks, of course, that might get around this (skip
every other interrupt at 200HZ).
> If someone picks HZ=1000 and their platform can't support it, then
> that's their own damn problem (don't touch things you don't
> understand, right? ;)
Well, ideally with kconfig we try to add proper dependencies so
impossible options aren't left to the user.
HZ is a common enough knob to turn on most systems, I don't know if
leaving the user rope to hang himself is a great idea.
>
>> and use HRT to allow more modern systems to have finer-grained
>> timers.
> My question really has to be is CONFIG_SCHED_HRTICK useful, what
> exactly is it going to do on ARM here since nobody can ever have
> enabled it? Is it going to keel over and explode if nobody registers a
> non-jiffies sched_clock (since the jiffies clock is technically
> reporting itself as a ridiculously high resolution clocksource..)?
??? Not following this at all. jiffies is the *MOST* coarse resolution
clocksource there is (at least that I'm aware of.. I recall someone
wanting to do a 60Hz clocksource, but I don't think that ever happened).
> Or is this one of those things that if your platform doesn't have a
> real high resolution timer, you shouldn't enable HRTIMERS and
> therefore not enable SCHED_HRTICK as a result? That affects
> ARCH_MULTIPLATFORM here. Is the solution as simple as
> ARCH_MULTIPLATFORM compliant platforms kind of have to have a high
> resolution timer? Documentation to that effect?
SO HRITMERS was designed to be be build time enabled, while still giving
you a functioning system if it was booted on a system that didn't
support clockevents. We boot with standard HZ, and only switch over to
HRT mode if we have a proper clocksource and clockevent driver.
However, HRTIMERS or NOHZ doesn't fix the case of having a system boot
with HZ=1000 or HZ=100 if the system can *only* do HZ=200.
thanks
-john
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: Arnd Bergmann @ 2013-01-21 22:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKGA1bn0-83d+BFRMGZDZ9MZ2nnx8e8wh-VS3XoG5UQJDC0npg@mail.gmail.com>
On Monday 21 January 2013, Matt Sealey wrote:
> So is that a bug in that it is not available to ARM right now, a bug
> in that it would be impossible for anyone on ARM to have ever tested
> this code, or a bug in that it should NEVER be enabled for ARM for
> some reason? John? Ingo? :)
>
I think it's a bug that it's not available. That does not look intentional.
Arnd
^ permalink raw reply
* [PATCH 2/7] ARM: sunxi: Add pinctrl driver for Allwinner SoCs
From: Arnd Bergmann @ 2013-01-21 22:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358544639-14761-3-git-send-email-maxime.ripard@free-electrons.com>
On Friday 18 January 2013, Maxime Ripard wrote:
> The Allwinner SoCs have an IP module that handle both the muxing and the
> GPIOs.
>
> This IP has 8 banks of 32 bits, with a number of pins actually useful
> for each of these banks varying from one to another, and depending on
> the SoC used on the board.
>
> This driver only implements the pinctrl part, the gpio part will come
> eventually.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* [PATCH 2/2] ARM: OMAP2+: omap2plus_defconfig: enable CMA allocator
From: Javier Martinez Canillas @ 2013-01-21 22:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355920420-20852-2-git-send-email-javier.martinez@collabora.co.uk>
On Wed, Dec 19, 2012 at 1:33 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> The OMAP framebuffer driver now uses the standard dma allocator
> instead of the (now removed) omap specific vram allocator.
>
> Enable the Contiguous Memory Allocator by default to allow large
> dma memory buffers allocation.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> arch/arm/configs/omap2plus_defconfig | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig
> index da530a0..dc4789e 100644
> --- a/arch/arm/configs/omap2plus_defconfig
> +++ b/arch/arm/configs/omap2plus_defconfig
> @@ -64,6 +64,7 @@ CONFIG_MAC80211=m
> CONFIG_MAC80211_RC_PID=y
> CONFIG_MAC80211_RC_DEFAULT_PID=y
> CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
> +CONFIG_CMA=y
> CONFIG_CONNECTOR=y
> CONFIG_MTD=y
> CONFIG_MTD_CMDLINE_PARTS=y
> --
Hi Tony,
Are you considering adding this patch and
ARM: OMAP2+: omap2plus_defconfig: enable TFP410 chip support [1]
to the 3.8-rc series or 3.9?
These patches have been acked by Tomi and are necessary to have
framebuffer video working on many OMAP3 based boards.
Thanks a lot and best regards,
Javier
[1]: https://patchwork.kernel.org/patch/1895451/
^ permalink raw reply
* [PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory
From: Arnd Bergmann @ 2013-01-21 22:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50FDBBB9.8090504@free-electrons.com>
On Monday 21 January 2013, Gregory CLEMENT wrote:
> Hum, I am not sure they are different. From my point of view it is the same IP,
> but the one on Armada XP can use a 25MHz fix source clock. Apart from this,
> all the registers are the same. Moreover currently with this property we still have
> the possibility to not use the 25MHz clock source on Armada XP which is totally valid.
> The use of this source clock is not mandatory for the Armada XP SoCs.
>
> For now I prefer to keep this property.
Ok, fair enough.
Arnd
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: Matt Sealey @ 2013-01-21 22:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130121211218.GX23505@n2100.arm.linux.org.uk>
On Mon, Jan 21, 2013 at 3:12 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 21, 2013 at 01:00:15PM -0800, John Stultz wrote:
>> So if you can not get actual timer ticks any faster then 200 HZ on that
>> hardware, setting HZ higher could cause some jiffies related timer
>> trouble
>
> Err, no John. It's the other way around - especially on some platforms
> which are incapable of being converted to the clock source support.
>
> EBSA110 has _one_ counter. It counts down at a certain rate, and when
> it rolls over from 0 to FFFF, it produces an interrupt and continues
> counting down from FFFF.
>
> To produce anything close to a reasonable regular tick rate from that,
> the only way to do it is - with interrupts disabled - read the current
> value to find out how far the timer has rolled over, and set it so that
> the next event will expire as close as possible to the desired HZ rate.
>
> So, none of the clcokevent stuff can be used; and we rely _purely_ on
> counting interrupts in jiffy based increments to provide any reference
> of time.
>
> Moreover, because the counter is only 16-bit, and it's clocked from
> something around 7MHz, well, maths will tell you why 200Hz had to be
> chosen rather than 100Hz.
I am sorry it sounded if I was being high and mighty about not being
able to select my own HZ (or being forced by Exynos to be 200 or by
not being able to test an Exynos board, forced to default to 100). My
real "grievance" here is we got a configuration item for the scheduler
which is being left out of ARM configurations which *can* use high
resolution timers, but I don't know if this is a real problem or not,
hence asking about it, and that HZ=100 is the ARM default whether we
might be able to select that or not.. which seems low.
HZ=250 is the "current" kernel default if you don't touch anything, it
seems, apologies for thinking it was HZ=100. And that is too high for
EBSA110 and a couple of other boards, especially where HZ must equal
some exact divisor being pumped right into some timer unit.
Understood. Surely the correct divisor should be *derived* from HZ and
not just dumped into the timer though, so HZ being set to an exact
divisor (but a round-down-to-acceptable-value) is kind of a hacky
concept..?
For the global kernel guys, I'd ask what is the reasoning for using
HZ=250 by default, I wonder? It seems like this number is from the
dark ages (pre-git, pre-bitkeeper, maybe pre-recorded history ;) and
the reason is lost. Why not HZ=100 or HZ=300 (if the help text is to
be believed, and it is probably older than God, HZ=300 is great for
playing back NTSC-format video.. :)? I can side with you on the
premise that in actual fact, defining a default HZ value in the
non-arch-specific kernel proper is a little quirky and it should be
something the arches do themselves (i.e. move the default-setting
stuff at the end into the arch/*/Kconfig - I would expect that now
i386 CPU support is gone from arch/x86, there's potentially a better
value than HZ=250 for the default?).
Anyway, a patch for ARM could perhaps end up like this:
~~
if ARCH_MULTIPLATFORM
source kernel/Kconfig.hz
else
HZ
default 100
endif
HZ
default 200 if ARCH_EBSA110 || ARCH_ETC_ETC || ARCH_UND_SO_WEITER
# any previous platform definitions where *really* required here.
# but not default 100 since it would override kernel/Kconfig.hz every time
~~
Which preserves all previous behaviors on all possible ARM arch
combinations, but where no reasonable override is set.. Kconfig.hz is
king. I cannot imagine any situation except for AT91 or OMAP could not
do this in their own {mach,plat}-*/Kconfigs and not in the core
config, which cleans up the extra HZ block.
We can agree that the "default 200 if.." list is unwieldy and Arnd is
right in that there is some cargo-cult programming going on here,
right?
Even if we assume EBSA110 and a couple others are really affected by
having such timer setups, therefore "reasonable", I'd challenge anyone
to tell me Exynos4 or the S5P platforms do not have high resolution
timers capable of handling more than HZ=200 (or the default HZ=250)
which I would class as "unreasonable".. this is why I said it was
possibly both. I am not one to judge some of these platforms I've
never even heard of, that is why I am *asking* about it before I even
think of doing anything about it.
I tested this a few weeks ago with a *few* defconfigs (by sourcing
Kconfig.hz above the existing HZ definitions) and it does effectively
override the value I went in and stabbed into menuconfig, in the
resultant generated local .config file - if they themselves are
sourced AFTER the source kernel/Kconfig.hz (which they pretty much
are) in arch/arm/Kconfig.
Could we also at least agree that if EBSA110 can handle HZ=200 with a
16-bit timer, or HZ=128 for OMAP and that AT91 will override it to 100
on it's own, then that "default 100" is overly restrictive and we
could remove it, allowing each {mach,plat}-*/Kconfig owner to
investigate and find the correct HZ value and implement an override or
selection, or just allow free configuration?
As far as I can tell AT91 and SHMOBILE only supply defaults because HZ
*must* meet some exact timer divisor (OMAP says "Kernel internal timer
frequency should be a divisor of 32768") in which case their timer
drivers should not be so stupid and instead round down to the nearest
acceptable timer divisor or WARN_ON if the compile-time values are
unacceptable at runtime before anyone sees any freakish behavior. Is
it a hard requirement for the ARM architecture that a woefully
mis-configured kernel MUST boot completely to userspace?
--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply
* One of these things (CONFIG_HZ) is not like the others..
From: John Stultz @ 2013-01-21 22:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130121211218.GX23505@n2100.arm.linux.org.uk>
On 01/21/2013 01:12 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 21, 2013 at 01:00:15PM -0800, John Stultz wrote:
>> So if you can not get actual timer ticks any faster then 200 HZ on that
>> hardware, setting HZ higher could cause some jiffies related timer
>> trouble
> Err, no John. It's the other way around - especially on some platforms
> which are incapable of being converted to the clock source support.
>
> EBSA110 has _one_ counter. It counts down at a certain rate, and when
> it rolls over from 0 to FFFF, it produces an interrupt and continues
> counting down from FFFF.
>
> To produce anything close to a reasonable regular tick rate from that,
> the only way to do it is - with interrupts disabled - read the current
> value to find out how far the timer has rolled over, and set it so that
> the next event will expire as close as possible to the desired HZ rate.
>
> So, none of the clcokevent stuff can be used; and we rely _purely_ on
> counting interrupts in jiffy based increments to provide any reference
> of time.
> Moreover, because the counter is only 16-bit, and it's clocked from
> something around 7MHz, well, maths will tell you why 200Hz had to be
> chosen rather than 100Hz.
Ah, so the counter can't do anything *lower* then ~107HZ, right? (7MHZ/2^16)
So we used to have the ACTHZ code to handle error from the HZ rate
requested and the HZ rate possible given the underlying hardware. That's
been moved to the register_refined_jiffies(), but do you have a sense if
there a reason it couldn't be used? I don't quite recall the bounds at
this second, so ~7% error might very well be too large.
So yes, I suspect these sorts of platforms, where there are no modern
clocksource/clockevent driver, as well as further constraints (like
specific HZ) are likely not good candidates for a multi-arch build.
thanks
-john
^ permalink raw reply
* [PATCH 7/7] ARM: sunxi: olinuxino: Add muxing for the uart
From: Linus Walleij @ 2013-01-21 22:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358544639-14761-8-git-send-email-maxime.ripard@free-electrons.com>
On Fri, Jan 18, 2013 at 10:30 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
All pinctrl and device tree patches applied to my allwinner branch in the
pinctrl tree. Hope the ARM SoC can accept me poking around in your
device trees.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 6/7] tty: of_serial: Add pinctrl support
From: Linus Walleij @ 2013-01-21 22:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358544639-14761-7-git-send-email-maxime.ripard@free-electrons.com>
On Fri, Jan 18, 2013 at 10:30 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Use pinctrl to configure the SoCs pins directly from the driver.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
You can poke Greg about this which is fun because it does become
superfluous with the patch titled
"[PATCH v4] drivers/pinctrl: grab default handles from device core"
http://marc.info/?l=linux-kernel&m=135879594515932&w=2
Which will auto-grab the default state on any device.
Greg, can I have your ACK on that device core grab patch
so you don't have to merge this patch but can point out that
it solves the boilerplate problem?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 2/7] ARM: sunxi: Add pinctrl driver for Allwinner SoCs
From: Linus Walleij @ 2013-01-21 22:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358544639-14761-3-git-send-email-maxime.ripard@free-electrons.com>
On Fri, Jan 18, 2013 at 10:30 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The Allwinner SoCs have an IP module that handle both the muxing and the
> GPIOs.
>
> This IP has 8 banks of 32 bits, with a number of pins actually useful
> for each of these banks varying from one to another, and depending on
> the SoC used on the board.
>
> This driver only implements the pinctrl part, the gpio part will come
> eventually.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Patch title says "ARM: " but all changed except one line is to the pinctrl
tree so I have applied it to the pinctrl tree on a separate allwinner
branch for now (will merge it to devel when deemde stable).
ARM SoC boys: can I have an ACK on this oneliner?
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 1/6] arm: mvebu: Add support for local interrupt
From: Gregory CLEMENT @ 2013-01-21 22:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130121191744.0a04b941@skate>
On 01/21/2013 07:17 PM, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
>
> Just some minor nitpicks below.
OK I will take them into account for the V2.
Thanks
>
> On Mon, 21 Jan 2013 18:53:57 +0100, Gregory CLEMENT wrote:
>
>> + if (hw < ARMADA_370_XP_MAX_PER_CPU_IRQS) {
>> +
>
> Unneeded empty line.
>
>> + irq_set_percpu_devid(virq);
>> + irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
>> + handle_percpu_devid_irq);
>> +
>> + } else {
>> + irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
>> + handle_level_irq);
>> + }
>
> Braces useless since there is only one statement in the else.
>
>> + set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
>
> Incorrect indentation for this line.
>
> Thomas
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH 6/6] clocksource: update and move armada-370-xp-timer documentation to timer directory
From: Gregory CLEMENT @ 2013-01-21 22:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201301211822.54129.arnd@arndb.de>
On 01/21/2013 07:22 PM, Arnd Bergmann wrote:
> On Monday 21 January 2013, Gregory CLEMENT wrote:
>> +
>> +Required properties:
>> +- compatible: Should be "marvell,armada-370-xp-timer"
>> +- interrupts: Should contain the list of Global Timer interrupts and
>> + then local timer interrupts
>> +- reg: Should contain location and length for timers register. First
>> + pair for the Global Timer registers, second pair for the
>> + local/private timers.
>> +- clocks: clock driving the timer hardware
>> +
>> +Optional properties:
>> +- marvell,timer-25Mhz: Tells whether the Global timer supports the 25
>> + Mhz fixed mode (available on Armada XP and not on Armada 370)
>
> This works fine, but I would consider it slightly cleaner to kill
> the marvell,timer-25Mhz property and instead have separate
> marvell,armada-370-timer and marvell,armada-xp-timer compatible
> strings, since the two timers are actually different, rather than wired
> differently.
Hum, I am not sure they are different. From my point of view it is the same IP,
but the one on Armada XP can use a 25MHz fix source clock. Apart from this,
all the registers are the same. Moreover currently with this property we still have
the possibility to not use the 25MHz clock source on Armada XP which is totally valid.
The use of this source clock is not mandatory for the Armada XP SoCs.
For now I prefer to keep this property.
Gregory
^ permalink raw reply
* [PATCH 1/7] pinctrl: pinconf-generic: add drive strength parameter
From: Linus Walleij @ 2013-01-21 22:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1358544639-14761-2-git-send-email-maxime.ripard@free-electrons.com>
On Fri, Jan 18, 2013 at 10:30 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Some pin configurations IP allows to set the current output to the pin.
> This patch adds such a parameter to the pinconf-generic mechanism.
>
> This parameter takes as argument the drive strength in mA.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
This is already applied...
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
From: Marek Vasut @ 2013-01-21 21:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHXqBFJ5=P2OHez74gK-DBT+3O=KHoHjffxRAe2u-9ppXOmExA@mail.gmail.com>
Dear Micha? Miros?aw,
[...]
> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> >
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
>
> Hmm. Then is IMX23 LRADC going to just work after this series
> (assuming I don't use unsupported features) or this needs more patches
> to be usable?
It's gonna work, touchscreen will work as well. You won't have the touchbuttons,
but when these're gonna be implemented, we will need to be careful to enable
them only for mx28.
Best regards,
Marek Vasut
^ permalink raw reply
* [PATCH 11/15] mtd: davinci_nand: fix OF support
From: Arnd Bergmann @ 2013-01-21 21:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50FDB509.9030004@denx.de>
On Monday 21 January 2013, Heiko Schocher wrote:
> Already fixed here:
> http://lists.infradead.org/pipermail/linux-mtd/2013-January/045366.html
>
Ok, thanks for the confirmation. I had only checked -rc4, not any newer
snapshot.
Arnd
^ permalink raw reply
* [PATCH 13/15] USB: ehci: make orion and mxc bus glues coexist
From: Arnd Bergmann @ 2013-01-21 21:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1301211533580.24224-100000@netrider.rowland.org>
On Monday 21 January 2013, Alan Stern wrote:
> Is Manjunath aware of the first patch attached to this email message?
>
> http://marc.info/?l=linux-usb&m=135843902916416&w=2
>
I would not expect so. Manjunath is still learning about the open
source processes, but he has also done a similar patch in his tree,
and one for most of the other ARM based platforms.
Manjunath: following up on the discussions we've had, I would suggest
you take a look at that patch, and do a comparison with your version
(apply it on the same base, then use git-diff to show the differences
between the version) and thing about whether your they are significant
or not. Since Alan posted his version on the mailing list first, that's
the one that should get merged, but there might be something in it that
you can learn from it and that I did not see.
Alan, one comment about your version: You keep maintaining the
'#if IS_ENABLED' list in the main driver, which I think can actually
get removed now. Since the base driver can be built independent of
the presence of platform glue drivers, there is no need to forbid
it any more, and the #if block will cause merge conflicts for each
patch that converts or adds another platform. I think we can actually
get the same results by turning the Kconfig logic around and making
the platform glue drivers 'select USB_EHCI_HCD' than depending on
it.
Arnd
^ permalink raw reply
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
From: Marek Vasut @ 2013-01-21 21:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50FDB517.700@metafoo.de>
Dear Lars-Peter Clausen,
> On 01/21/2013 10:32 PM, Marek Vasut wrote:
> > Dear Micha? Miros?aw,
> >
> >> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>> Dear Micha? Miros?aw,
> >>>
> >>>> 2013/1/21 Marek Vasut <marex@denx.de>:
> >>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
> >>>>> block on MX23 is not much different from the one on MX28, thus this
> >>>>> is only a few changes fixing the parts that are specific to MX23.
> >>>>
> >>>> [...]
> >>>>
> >>>>> +struct mxs_lradc_of_config {
> >>>>> + const int irq_count;
> >>>>> + const char * const *irq_name;
> >>>>> +};
> >>>>> +
> >>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[]
> >>>>> = { + [IMX23_LRADC] = {
> >>>>> + .irq_count = ARRAY_SIZE(mx23_lradc_irq_names),
> >>>>> + .irq_name = mx23_lradc_irq_names,
> >>>>> + },
> >>>>> + [IMX28_LRADC] = {
> >>>>> + .irq_count = ARRAY_SIZE(mx28_lradc_irq_names),
> >>>>> + .irq_name = mx28_lradc_irq_names,
> >>>>> + },
> >>>>> +};
> >>>>> +
> >>>>>
> >>>>> enum mxs_lradc_ts {
> >>>>>
> >>>>> MXS_LRADC_TOUCHSCREEN_NONE = 0,
> >>>>> MXS_LRADC_TOUCHSCREEN_4WIRE,
> >>>>>
> >>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
> >>>>> *lradc)
> >>>>>
> >>>>> writel(0, lradc->base + LRADC_DELAY(i));
> >>>>>
> >>>>> }
> >>>>>
> >>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
> >>>>> + { .compatible = "fsl,imx23-lradc", .data = (void
> >>>>> *)IMX23_LRADC, }, + { .compatible = "fsl,imx28-lradc", .data =
> >>>>> (void
> >>>>> *)IMX28_LRADC, }, + { /* sentinel */ }
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
> >>>>> +
> >>>>
> >>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
> >>>
> >>> Check the register layout, it differs between MX23 and MX28, that's one
> >>> reason, since were we to access differently placed registers, we can do
> >>> it easily as in the SSP/I2C drivers.
> >>>
> >>> Moreover, there are some features on the MX28 that are not on the MX23
> >>> (like voltage treshold triggers and touchbuttons), with this setup, we
> >>> can easily check what we're running at at runtime and determine to
> >>> disallow these.
> >>>
> >>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
> >>> much more convenient in the long run.
> >>
> >> I'm asking, because you don't use this number anywhere other than in
> >> mxs_lradc_probe()
> >> and there only to dereference the irq-names table. After that the
> >> structure and number
> >> are forgotten.
> >
> > Certainly, so far it's used only this way. But please see my argument
> > about register layout, that's why I went down this road of abstraction.
>
> You'll probably be better of by putting these differences into the
> mxs_lradc_of_config struct as well, instead of adding switch statements
> here and there throughout the code.
Certainly. All that is needed is in place now.
Best regards,
Marek Vasut
^ permalink raw reply
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
From: Michał Mirosław @ 2013-01-21 21:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201301212232.52161.marex@denx.de>
2013/1/21 Marek Vasut <marex@denx.de>:
> Dear Micha? Miros?aw,
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>> > Dear Micha? Miros?aw,
>> >> 2013/1/21 Marek Vasut <marex@denx.de>:
>> >> > This patch adds support for i.MX23 into the LRADC driver. The LRADC
>> >> > block on MX23 is not much different from the one on MX28, thus this
>> >> > is only a few changes fixing the parts that are specific to MX23.
>> >>
>> >> [...]
>> >>
>> >> > +struct mxs_lradc_of_config {
>> >> > + const int irq_count;
>> >> > + const char * const *irq_name;
>> >> > +};
>> >> > +
>> >> > +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
>> >> > { + [IMX23_LRADC] = {
>> >> > + .irq_count = ARRAY_SIZE(mx23_lradc_irq_names),
>> >> > + .irq_name = mx23_lradc_irq_names,
>> >> > + },
>> >> > + [IMX28_LRADC] = {
>> >> > + .irq_count = ARRAY_SIZE(mx28_lradc_irq_names),
>> >> > + .irq_name = mx28_lradc_irq_names,
>> >> > + },
>> >> > +};
>> >> > +
>> >> >
>> >> > enum mxs_lradc_ts {
>> >> >
>> >> > MXS_LRADC_TOUCHSCREEN_NONE = 0,
>> >> > MXS_LRADC_TOUCHSCREEN_4WIRE,
>> >> >
>> >> > @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>> >> > *lradc)
>> >> >
>> >> > writel(0, lradc->base + LRADC_DELAY(i));
>> >> >
>> >> > }
>> >> >
>> >> > +static const struct of_device_id mxs_lradc_dt_ids[] = {
>> >> > + { .compatible = "fsl,imx23-lradc", .data = (void
>> >> > *)IMX23_LRADC, }, + { .compatible = "fsl,imx28-lradc", .data =
>> >> > (void
>> >> > *)IMX28_LRADC, }, + { /* sentinel */ }
>> >> > +};
>> >> > +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>> >> > +
>> >>
>> >> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>> >
>> > Check the register layout, it differs between MX23 and MX28, that's one
>> > reason, since were we to access differently placed registers, we can do
>> > it easily as in the SSP/I2C drivers.
>> >
>> > Moreover, there are some features on the MX28 that are not on the MX23
>> > (like voltage treshold triggers and touchbuttons), with this setup, we
>> > can easily check what we're running at at runtime and determine to
>> > disallow these.
>> >
>> > From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>> > much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
>
> Certainly, so far it's used only this way. But please see my argument about
> register layout, that's why I went down this road of abstraction.
Hmm. Then is IMX23 LRADC going to just work after this series
(assuming I don't use unsupported features) or this needs more patches
to be usable?
Best Regards,
Micha? Miros?aw
^ permalink raw reply
* [PATCH 13/15] USB: ehci: make orion and mxc bus glues coexist
From: Arnd Bergmann @ 2013-01-21 21:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1301211459550.24224-100000@netrider.rowland.org>
On Monday 21 January 2013, Alan Stern wrote:
> On Mon, 21 Jan 2013, Felipe Balbi wrote:
>
> > On Mon, Jan 21, 2013 at 05:16:06PM +0000, Arnd Bergmann wrote:
> > > In linux-3.8-rc1 it became possible to build the imx and
> > > mvebu platforms together in a single kernel, which clashes
> > > in the ehci driver.
> > >
> > > Manjunath Goudar is already working on a patch to convert
> > > both the imx and the mvebu glue drivers (along with all
> > > the other ARM specific ones) to the new probing method,
> > > but that will be too late to fix v3.8. This patch instead
> > > adds yet another hack like the existing ones to allow
> > > the ehci driver to load both back-ends.
>
> Pardon me for being confused. Is this about imx and mvebu (as
> mentioned here) or about orion and mxc (as described in the patch
> title, the error messages below, and the patch itself)?
mxc is the old name for imx: the platform got removed, but some
of the drivers still carry the old name. Similarly, orion was
used before as the name for the superset of various Marvell platforms,
and mvebu is the superset of that and some of the newer ones.
Sorry for the confusion.
> > > Without this patch, building allyesconfig results in:
> > >
> > > drivers/usb/host/ehci-hcd.c:1285:0: error: "PLATFORM_DRIVER" redefined
> > > drivers/usb/host/ehci-hcd.c:1255:0: note: this is the location of the previous definition
> > > In file included from drivers/usb/host/ehci-hcd.c:1254:0:
> > > drivers/usb/host/ehci-mxc.c:280:31: warning: 'ehci_mxc_driver' defined but not used
>
> Was the point here to fix the build error or to allow the two drivers
> to coexist? The first would be eaiser than the second.
Fixing the build error is probably the more important part here, but
we also really want this to work, since people are migrating towards
multiplatform kernels now, and mvebu/orion and imx/mxc are two of the
more important ones.
> > NAK, should be converted to the new usage of ehci library driver. Alan
> > Stern already implemented for a few drivers, help is very welcome.
>
> Arnd, please take a look at
>
> http://marc.info/?l=linux-usb&m=135843716515529&w=2
>
> I can't test it easily, not being set up for cross compilation. I'm
> waiting to hear from anybody whether it works before submitting it.
> (There's also a report of memory corruption involving a similar patch
> for ehci-omap; it hasn't been tracked down yet.)
Your patch looks good to me, but it also seems to do some other
changes that are not required to fix the problem but could wait
for 3.9 instead. You definitely have my Ack if you are willing
to take it for 3.8 though.
Shawn or Sascha should be able to test it.
Arnd
^ permalink raw reply
* [PATCH 1/2] iio: mxs: Add MX23 support into the IIO driver
From: Lars-Peter Clausen @ 2013-01-21 21:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201301212232.52161.marex@denx.de>
On 01/21/2013 10:32 PM, Marek Vasut wrote:
> Dear Micha? Miros?aw,
>
>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>> Dear Micha? Miros?aw,
>>>
>>>> 2013/1/21 Marek Vasut <marex@denx.de>:
>>>>> This patch adds support for i.MX23 into the LRADC driver. The LRADC
>>>>> block on MX23 is not much different from the one on MX28, thus this
>>>>> is only a few changes fixing the parts that are specific to MX23.
>>>>
>>>> [...]
>>>>
>>>>> +struct mxs_lradc_of_config {
>>>>> + const int irq_count;
>>>>> + const char * const *irq_name;
>>>>> +};
>>>>> +
>>>>> +static const struct mxs_lradc_of_config const mxs_lradc_of_config[] =
>>>>> { + [IMX23_LRADC] = {
>>>>> + .irq_count = ARRAY_SIZE(mx23_lradc_irq_names),
>>>>> + .irq_name = mx23_lradc_irq_names,
>>>>> + },
>>>>> + [IMX28_LRADC] = {
>>>>> + .irq_count = ARRAY_SIZE(mx28_lradc_irq_names),
>>>>> + .irq_name = mx28_lradc_irq_names,
>>>>> + },
>>>>> +};
>>>>> +
>>>>>
>>>>> enum mxs_lradc_ts {
>>>>>
>>>>> MXS_LRADC_TOUCHSCREEN_NONE = 0,
>>>>> MXS_LRADC_TOUCHSCREEN_4WIRE,
>>>>>
>>>>> @@ -857,8 +890,19 @@ static void mxs_lradc_hw_stop(struct mxs_lradc
>>>>> *lradc)
>>>>>
>>>>> writel(0, lradc->base + LRADC_DELAY(i));
>>>>>
>>>>> }
>>>>>
>>>>> +static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>>> + { .compatible = "fsl,imx23-lradc", .data = (void
>>>>> *)IMX23_LRADC, }, + { .compatible = "fsl,imx28-lradc", .data =
>>>>> (void
>>>>> *)IMX28_LRADC, }, + { /* sentinel */ }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>>>>> +
>>>>
>>>> Why not s/(void \*)\(IMX.._LRADC\)/\&mxs_lradc_of_config[\1]/ ?
>>>
>>> Check the register layout, it differs between MX23 and MX28, that's one
>>> reason, since were we to access differently placed registers, we can do
>>> it easily as in the SSP/I2C drivers.
>>>
>>> Moreover, there are some features on the MX28 that are not on the MX23
>>> (like voltage treshold triggers and touchbuttons), with this setup, we
>>> can easily check what we're running at at runtime and determine to
>>> disallow these.
>>>
>>> From my point of view, using the number (IMX23_LRADC / IMX28_LRADC) is
>>> much more convenient in the long run.
>>
>> I'm asking, because you don't use this number anywhere other than in
>> mxs_lradc_probe()
>> and there only to dereference the irq-names table. After that the
>> structure and number
>> are forgotten.
>
> Certainly, so far it's used only this way. But please see my argument about
> register layout, that's why I went down this road of abstraction.
You'll probably be better of by putting these differences into the
mxs_lradc_of_config struct as well, instead of adding switch statements here
and there throughout the code.
- Lars
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox