[net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver

[Grant Likely]

On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg at grandegger.com> wrote:
> Wolfgang Grandegger wrote:
>> Grant Likely wrote:
>>>> +- 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.

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

Ugh, I see what you mean.

> 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,
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?

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

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.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.