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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4A1A53B1.7040708@grandegger.com>
Date:	Mon, 25 May 2009 10:15:45 +0200
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	Grant Likely <grant.likely@...retlab.ca>
CC:	Segher Boessenkool <segher@...nel.crashing.org>,
	Linux Netdev List <netdev@...r.kernel.org>,
	devicetree-discuss <devicetree-discuss@...abs.org>,
	linuxppc-dev <Linuxppc-dev@...abs.org>
Subject: Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus
 	driver

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

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