Grant Likely wrote:

On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger [off-list ref] wrote:

quoted

Wolfgang Grandegger wrote:

quoted

Grant Likely wrote:

quoted

quoted

+- 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.

Would it be better to specify the external clock frequency, and the
driver know that internal freq is half that?  I ask because external
clock freq is a value the HW designer actually has control over.

I'm sharing your arguments: "external-clock-frequency". There is always
some confusion about external and internal clock frequency but the name
should make it clear.

quoted

quoted

quoted

quoted

+- 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.

quoted

quoted

+- 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.

Ugh, I see what you mean.

quoted

Would the following more descriptive properties be OK?

 clock-out-frequency = <0>, // CLKOUT pin clock off
                     = <4000000>; // frequency on CLKOUT pin

Or how about CLKOUT pin off if the property isn't present?  Otherwise,

Yep, that will be the default anyhow.

this looks okay.  BTW, I'd consider prefixing this with 'nxp,' or
'sja1000,' to protect the namespace.  clock-out-frequency sounds like
one of those names which could be commonly used in the future.  I'd so
the same for the other chip-specific properties too.

Segher, what's your opinion on this?

I personally don't have a real preference.

quoted

 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

hmmm; It is very chip specific and it is a lot of mucking around.  If
you prefix the property with the manufacturer name, then perhaps the
explicit register setting is okay.

OK.

Are TX0 & TX1 protocol pins or GPIOs?  If gpio, then maybe it is worth
the mucking about to then use the gpios binding to specify the pin
mode.

These are the output from the CAN output driver 0 or 1 to the physical
bus line. E.g., in normal output mode the CAN bit sequence is sent via
TX0 or TX1. From a non-hardware expert point of view, they must be
programmed properly to get the hardware to work.

Wolfgang.