From: Wolfgang Grandegger <wg@grandegger•com>
To: Grant Likely <grant.likely@secretlab•ca>
Cc: Linux Netdev List <netdev@vger•kernel.org>,
devicetree-discuss <devicetree-discuss@ozlabs•org>,
linuxppc-dev <Linuxppc-dev@ozlabs•org>
Subject: Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver
Date: Sat, 23 May 2009 08:29:25 +0200 [thread overview]
Message-ID: <4A1797C5.1040907@grandegger.com> (raw)
In-Reply-To: <fa686aa40905220808kecd06cdqb570b78ca97f0ca6@mail.gmail.com>
Hi Grant,
Grant Likely wrote:
> 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.
> 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.
>> +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.
>> +- 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.
>> +- 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.
Wolfgang.
>
next prev parent reply other threads:[~2009-05-23 6:29 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-05-22 14:46 [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver Wolfgang Grandegger
2009-05-22 15:08 ` Grant Likely
2009-05-23 6:29 ` Wolfgang Grandegger [this message]
2009-05-23 16:44 ` Wolfgang Grandegger
2009-05-25 6:53 ` Grant Likely
2009-05-25 8:15 ` Wolfgang Grandegger
2009-05-23 11:15 ` Arnd Bergmann
2009-05-23 16:51 ` Wolfgang Grandegger
2009-05-24 22:27 ` Arnd Bergmann
2009-05-25 6:58 ` Wolfgang Grandegger
2009-05-26 9:10 ` Arnd Bergmann
2009-05-26 9:25 ` David Miller
2009-05-26 9:42 ` Arnd Bergmann
2009-05-26 11:20 ` Sascha Hauer
2009-05-26 14:23 ` Wolfgang Grandegger
2009-05-30 17:59 ` Wolfgang Grandegger
2009-05-26 9:40 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A1797C5.1040907@grandegger.com \
--to=wg@grandegger$(echo .)com \
--cc=Linuxppc-dev@ozlabs$(echo .)org \
--cc=devicetree-discuss@ozlabs$(echo .)org \
--cc=grant.likely@secretlab$(echo .)ca \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox