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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa686aa40905220808kecd06cdqb570b78ca97f0ca6@mail.gmail.com>
Date:	Fri, 22 May 2009 09:08:27 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	Linux Netdev List <netdev@...r.kernel.org>,
	linuxppc-dev <Linuxppc-dev@...abs.org>,
	devicetree-discuss <devicetree-discuss@...abs.org>
Subject: Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus 
	driver

Hi Wolfgang, thanks for the quick response.  Comments below...

On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@...ndegger.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)

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.

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

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

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

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

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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