Hi Grant, Grant Likely wrote: > Hi Wolfgang, thanks for the quick response. Comments below... > > On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <w...@grandegger.com> > wrote: >> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt >> @@ -0,0 +1,37 @@ >> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips) >> + >> +Required properties: >> + >> +- compatible : should be "nxp,sja1000". >> +- reg : should specify the chip select, address offset and size used >> + for the chip depending on the bus it is connected to. >> +- interrupts: property with a value describing the interrupt source >> + (number and sensitivity) for that device. The encoding depends >> + on the type of interrupt controller used. > > Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood > properties. I don't think we need to keep boilerplate defining the > meaning every time a new binding is added. (general musing; not an > ack or nack of this patch)
OK. > However, what should be defined is *what* the register range is (ie. > one tuple; location of device registers), and what the interrupts are > (ie. single tuple for device's irq line). Granted this is a trivial > case, but in the case of devices with more than one address range or > irq line, the meaning of each tuple is critical information. I think > it would be a good pattern to establish. Sounds reasonable. >> +Optional properties: >> + >> +- interrupt-parent : the phandle for the interrupt controller that >> + services interrupts for that device. > > Thinking further; I wouldn't even mention "interrupt-parent" here. > Anyone working with this stuff must already understand irq routing. OK, I will remove it. >> +- clock-frequency : CAN system clock frequency in Hz, which is normally >> + half of the oscillator clock frequency. If not specified, a >> + default value of 8000000 (8 MHz) is used. > > A clock-frequency property typically refers to the bus clock > frequency. Something like can-frequency would be better. Ah, right, but I'm also not happy with "can-frequency". The manual speaks about the "internal clock", which is half of the external oscillator frequency. Maybe "internal-clock-frequency" might be better. >> +- cdr-reg : value of the SJA1000 clock divider register according to >> + the SJA1000 data sheet. If not specified, a default value of >> + 0x48 is used. > > Ewh. The driver should be clueful enough to derive the clock divider > value given both the bus and can frequencies. I don't like this > property. The clock divider register controls the CLKOUT frequency for the microcontroller another CAN controller and allows to deactivate the CLKOUT pin. It's not used to configure the CAN bus frequency. > >> +- ocr-reg : value of the SJA1000 output control register according to >> + the SJA1000 data sheet. If not specified, a default value of >> + 0x0a is used. > > Ditto here; the binding should describe the usage mode; not the > register settings to get the usage mode. What sort of settings will > the .dts author be writing here? Unfortunately, there are many: clkout-frequency bypass-comperator tx1-output-on-rx-irq #define OCR_MODE_BIPHASE 0x00 #define OCR_MODE_TEST 0x01 #define OCR_MODE_NORMAL 0x02 #define OCR_MODE_CLOCK 0x03 #define OCR_TX0_INVERT 0x04 #define OCR_TX0_PULLDOWN 0x08 #define OCR_TX0_PULLUP 0x10 #define OCR_TX0_PUSHPULL 0x18 #define OCR_TX1_INVERT 0x20 #define OCR_TX1_PULLDOWN 0x40 #define OCR_TX1_PULLUP 0x80 #define OCR_TX1_PUSHPULL 0xc0 I think implementing properties for each option is overkill. Wolfgang. > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
