Wolfgang Grandegger wrote:
> 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.
Would the following more descriptive properties be OK?
clock-out-frequency = <0>, // CLKOUT pin clock off
= <4000000>; // frequency on CLKOUT pin
bypass-input-comparator; // allows to bypass the CAN input comparator.
tx1-output-on-rx-irq; // allows the TX1 output to be used as a
// dedicated RX interrupt output.
output-control-mode = <0x0> // bi-phase output mode
<0x1> // test output mode
<0x2> // normal output mode (default)
<0x3> // clock output mode
output-pin-config = <0x01> // TX0 invert
<0x02> // TX0 pull-down
<0x04> // TX0 pull-up
<0x06> // TX0 push-pull
<0x08> // TX1 invert
<0x10> // TX1 pull-down
<0x20> // TX1 pull-up
<0x30> // TX1 push-pull
Wolfgang.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
