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

[Wolfgang Grandegger <wg@grandegger.com>]

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

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.

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

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.

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