Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [RFC PATCH arm: initial TI-Nspire support] | Date | Sun, 7 Apr 2013 16:32:55 +0200 |
| |
On Sunday 07 April 2013, Daniel Tang wrote: > On 07/04/2013, at 12:24 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > > >> @@ -313,7 +314,7 @@ define archhelp > >> echo ' Image - Uncompressed kernel image (arch/$(ARCH)/boot/Image)' > >> echo '* xipImage - XIP kernel image, if configured (arch/$(ARCH)/boot/xipImage)' > >> echo ' uImage - U-Boot wrapped zImage' > >> - echo ' bootpImage - Combined zImage and initial RAM disk' > >> + echo ' bootpImage - Combined zImage and initial RAM disk' > >> echo ' (supply initrd image via make variable INITRD=<path>)' > >> echo '* dtbs - Build device tree blobs for enabled boards' > >> echo ' install - Install uncompressed kernel' > > > > This looks like it wasn't meant to be in the patch. > > It probably isn't. I think there was trailing whitespace on that and my editor happened to remove it automatically. > > Should this be a separate patch to fix up formatting or should I leave it in as a drive-by fix?
Any cleanups like this should be separate patches, and ideally even part of a different patch series.
> > > >> + uart: uart@90020000 { > >> + reg = <0x90020000 0x1000>; > >> + interrupts = <1>; > >> + > >> + clocks = <&uart_clk>; > >> + clock-names = "uart_clk"; > >> + }; > > > > The name for a uart should be "serial". Since this is a pl01x, please add > > the required properties for the device, e.g. > > > > compatible = "arm,pl011", "arm,primecell"; > > > > You will need the "arm,primecell" bit to make the device appear on the > > amba bus rather than the platform bus. > > That was actually deliberate because different models of the TI-NSPIRE have different > serial hardware. On the newer CX models, it is a PL01x and on the older models, it has > a 8250-like interface. They all reside at the same address with the same IRQ though. > > I thought it might be cleaner to specify the interrupts and registers in the common file > and leave it to the board specific ones to implement the "compatible" property.
I see. It seems a little confusing to the reader though. I don't have a good answer, but there are two other options how this can be handled:
* Put both devices in the devicetree and mark them as status="disabled" in the main file, but enable one of them in the version specific files.
* leave them out of the .dtsi file and only define them in the specific .dts files. > >> + err = of_property_read_string(of_aliases, "timer0", &path); > >> + if (WARN_ON(err)) > >> + return; > >> + > >> + timer = of_find_node_by_path(path); > >> + base = of_iomap(timer, 0); > >> + if (WARN_ON(!base)) > >> + return; > >> + > >> + clk = of_clk_get_by_name(timer, NULL); > >> + clk_register_clkdev(clk, timer->name, "sp804"); > >> + > >> + sp804_clocksource_init(base, timer->name); > >> + > >> + err = of_property_read_string(of_aliases, "timer1", &path); > >> + if (WARN_ON(err)) > >> + return; > > > > In particular, I think the method of using aliases to pick the right sp804 > > instance is being deprecated now. If both timers are identical, the kernel > > will now just pick one of them. > > Sorry, I don't quite understand. > > Out of the timers, I want to add one as a clocksource and one as a clockevent. > If they're identical (i.e. without using aliases), how should I tell the kernel, > "Take the first timer you see and make it a clocksource, take the next one you > see and make it a clockevent"?
The modified sp804 driver will have logic to do that. I think in the end we decided that the driver should first look for any device that can be used as a clocksource and use it that way. If it finds a second device, that can be used as clockevent.
Arnd
| |