Hi Grant,

Grant Likely wrote:
quoted text
> Hi Wolfgang, thanks for the quick response.  Comments below...
> 
> On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@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.

quoted text
> 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.

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

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

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

quoted text
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html