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: > > +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. > > + - 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. > > + 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. > > + etsects->irq = irq_of_parse_and_map(node, 0); > > Use platform_get_irq(). Okay. > > + 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 > > +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. > > +++ 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 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
