On Wed, Feb 23, 2011 at 09:50:58AM -0700, Grant Likely wrote:

On Wed, Feb 23, 2011 at 11:38:17AM +0100, Richard Cochran wrote:

quoted

+Clock Properties:
+
+  - tclk-period  Timer reference clock period in nanoseconds.
+  - tmr-prsc     Prescaler, divides the output clock.
+  - tmr-add      Frequency compensation value.
+  - cksel        0= external clock, 1= eTSEC system clock, 3= RTC clock input.
+                 Currently the driver only supports choice "1".

I'd be hesitant about defining something that isn't actually
implemented yet.  You may find the binding to be insufficient at a
later date.

Okay, I'll remove it.
We never got the external VCO working anyhow.

quoted

+  - tmr-fiper1   Fixed interval period pulse generator.
+  - tmr-fiper2   Fixed interval period pulse generator.
+  - max-adj      Maximum frequency adjustment in parts per billion.

These are all custom properties (not part of any shared binding) so
they should probably be prefixed with 'fsl,'.

Okay, fine.

quoted

+  The calculation for tmr_fiper2 is the same as for tmr_fiper1. The
+  driver expects that tmr_fiper1 will be correctly set to produce a 1
+  Pulse Per Second (PPS) signal, since this will be offered to the PPS
+  subsystem to synchronize the Linux clock.

Good documentation, thanks.  Question though, how many of these values
will the end user (or board builder) be likely to want to change.  It
is risky encoding the calculation results into the device tree when
they aren't the actually parameters that will be manipulated, or at
least very user-unfriendly.

The whole thing is pretty opaque, and my explanation is (IMHO) way
better that Freescale's documentation of how the fipers work.

The board designer / system designer will want to set these carefully,
but never change them. Basically, for a given input clock, there is
only one optimal setting.

I think the device tree is the right place for that kind of setting.

The fiper1 signal should always be a 1 PPS.  We could make fiper2 run
time programmable via PHC ioctls, but I think this can wait.

quoted

+	etsects->irq = irq_of_parse_and_map(node, 0);

Use platform_get_irq().

Okay.

quoted

+	etsects->regs = of_iomap(node, 0);

Use platform_get_resource(), and don't forget to request the
resources.

Okay, but didn't you tell me before to do this way?

   http://marc.info/?l=linux-netdev&m=127662247203659&w=4

quoted

+static struct of_platform_driver gianfar_ptp_driver = {

Use a platform_driver instead.  of_platform_driver is deprecated and
being removed.

Ja, should have noticed that myself, sorry.

quoted

+++ b/drivers/net/gianfar_ptp_reg.h

This data is only used by gianfar_ptp.c, so there is no need for a
separate include file.  Move the contents of gianfar_ptp_reg.h into
gianfar_ptp.c

You are right, of course, since private #defines and declarations
should simply stay in their .c files. Some people think that all
#defines and declarations must go into a header file.

I am not one of those people, but in this case, I generated the file
from a little tool I wrote and so kept it separate.

Still, it is no trouble to combine the header into the driver .c file.

Thanks for your review,

Richard