RE: [MeeGo-Dev][PATCH] Topcliff: Update PCH_IEEE1588 driver to2.6.35

[Wang, Qi]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxx]
> Sent: Wednesday, August 11, 2010 1:14 AM
> To: Masayuki Ohtak
> Cc: meego-dev@xxxxxxxxx; LKML; Wang, Qi; Wang, Yong Y; Khor, Andrew
> Chih Howe; arjan@xxxxxxxxxxxxxxx
> Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_IEEE1588 driver to
> 2.6.35
> 
> On Tue, Aug 10, 2010 at 07:32:04PM +0900, Masayuki Ohtak wrote:
> > IEEE1588 driver of Topcliff PCH
> >
> > Topcliff PCH is the platform controller hub that is going to be used in
> > Intel's upcoming general embedded platform. All IO peripherals in
> > Topcliff PCH are actually devices sitting on AMBA bus.
> > Topcliff PCH has IEEE1588 I/F. This driver enables IEEE1588 function on CAN
> or
> > Ethernet communications.
> 
> So it's a CAN or Ethernet driver?  If so, why isn't this a network
> driver like the CAN and Ethernet drivers?

This socket CAN drivers which can use Berkeley socket API, and the Linux network stack and implements the CAN device drivers as network interfaces.
> 
> > +#ifdef __GNUC__
> > +#define UNUSED __attribute__ ((unused))
> > +#define UNUSED_ARG(x)
> > +#else
> > +#define UNUSED
> > +#define UNUSED_ARG(x) (void) x
> > +#endif
> > +
> > +
> > +#define TRUE 1
> > +#define FALSE 0
> 
> These lines should not be needed at all.
OKI-san, please remove them, Those definition is useless.
> 
> > +#define IOC_1588_BASE 0xf8
> 
> Do you have documentation for these new ioctls you are creating?  It's a
> lot of them:
> 
> > +#define IOCTL_1588_PORT_CONFIG_SET \
> > +			_IOW(IOC_1588_BASE, 0, struct pch_portcfg_ioctl)
> > +
> > +#define IOCTL_1588_PORT_CONFIG_GET \
> > +			_IOWR(IOC_1588_BASE, 1, struct pch_portcfg_ioctl)
> > +
> > +#define IOCTL_1588_RX_POLL _IOWR(IOC_1588_BASE, 2, struct
> pch_rxtxpoll_ioctl)
> > +
> > +#define IOCTL_1588_TX_POLL _IOWR(IOC_1588_BASE, 3, struct
> pch_rxtxpoll_ioctl)
> > +
> > +#define IOCTL_1588_CAN_POLL _IOWR(IOC_1588_BASE, 4, struct
> pch_canpoll_ioctl)
> > +
> > +#define IOCTL_1588_SYS_TIME_GET _IOR(IOC_1588_BASE, 5, struct
> pch_tim_val)
> > +
> > +#define IOCTL_1588_SYS_TIME_SET _IOW(IOC_1588_BASE, 6, struct
> pch_tim_val)
> > +
> > +#define IOCTL_1588_TICK_RATE_SET _IOW(IOC_1588_BASE, 7, __u32)
> > +
> > +#define IOCTL_1588_TICK_RATE_GET _IOR(IOC_1588_BASE, 8, __u32)
> > +
> > +#define IOCTL_1588_TARG_TIME_INTRPT_ENABLE _IO(IOC_1588_BASE, 9)
> > +
> > +#define IOCTL_1588_TARG_TIME_INTRPT_DISABLE _IO(IOC_1588_BASE,
> 10)
> > +
> > +#define IOCTL_1588_TARG_TIME_POLL \
> > +			_IOR(IOC_1588_BASE, 11, struct pch_timepoll_ioctl)
> > +
> > +#define IOCTL_1588_TARG_TIME_SET \
> > +				_IOW(IOC_1588_BASE, 12, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_TARG_TIME_GET \
> > +				_IOR(IOC_1588_BASE, 13, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_AUX_TIME_INTRPT_ENABLE _IOW(IOC_1588_BASE,
> 14,\
> > +							enum pch_auxmode)
> > +
> > +#define IOCTL_1588_AUX_TIME_INTRPT_DISABLE _IOW(IOC_1588_BASE,
> 15,\
> > +							enum pch_auxmode)
> > +
> > +#define IOCTL_1588_AUX_TIME_POLL \
> > +			_IOWR(IOC_1588_BASE, 16, struct pch_timepoll_ioctl)
> > +
> > +#define IOCTL_1588_RESET _IO(IOC_1588_BASE, 17)
> > +
> > +#define IOCTL_1588_CHNL_RESET _IOW(IOC_1588_BASE, 18, enum
> pch_ptpport)
> > +
> > +#define IOCTL_1588_STATS_GET _IOR(IOC_1588_BASE, 19, struct
> pch_stats)
> > +
> > +#define IOCTL_1588_STATS_RESET _IO(IOC_1588_BASE, 20)
> > +
> > +#define IOCTL_1588_SHOW_ALL _IO(IOC_1588_BASE, 21)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_INTRPT_ENABLE
> _IO(IOC_1588_BASE, 22)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_INTRPT_DISABLE
> _IO(IOC_1588_BASE, 23)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_POLL \
> > +			_IOR(IOC_1588_BASE, 24, struct pch_timepoll_ioctl)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_SET \
> > +				_IOW(IOC_1588_BASE, 25, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_GET \
> > +				_IOR(IOC_1588_BASE, 26, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_INTRPT_ENABLE
> _IO(IOC_1588_BASE, 27)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_INTRPT_DISABLE
> _IO(IOC_1588_BASE, 28)
> > +
> > +#define IOCTL_1588_TARG_TIME_NOTIFY \
> > +				_IOR(IOC_1588_BASE, 29, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_AUX_TIME_NOTIFY \
> > +			_IOR(IOC_1588_BASE, 30, struct pch_auxtimeioctl)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_NOTIFY \
> > +				_IOR(IOC_1588_BASE, 31, struct pch_tim_val)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_NOTIFY _IOR(IOC_1588_BASE, 32,
> __u32)
> > +
> > +#define IOCTL_1588_TARG_TIME_CLR_NOTIFY _IO(IOC_1588_BASE, 33)
> > +
> > +#define IOCTL_1588_AUX_TIME_CLR_NOTIFY _IO(IOC_1588_BASE, 34)
> > +
> > +#define IOCTL_1588_AUX_TARG_TIME_CLR_NOTIFY _IO(IOC_1588_BASE,
> 35)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_CLR_NOTIFY _IO(IOC_1588_BASE,
> 36)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_TIME_GET _IOR(IOC_1588_BASE, 37,
> __u32)
> > +
> > +#define IOCTL_1588_PULSE_PER_SEC_TIME_SET _IOW(IOC_1588_BASE, 38,
> __u32)
> > +
> > +#define IOCTL_1588_PORT_VERSION_SET \
> > +			_IOW(IOC_1588_BASE, 39, struct pch_versionioctl)
> > +
> > +#define IOCTL_1588_PORT_VERSION_GET \
> > +			_IOWR(IOC_1588_BASE, 40, struct pch_versionioctl)
> > +
> > +#define IOCTL_1588_PORT_OPERATION_MODE_SET \
> > +		_IOW(IOC_1588_BASE, 41, struct pch_opemode_ioctl)
> > +
> > +#define IOCTL_1588_PORT_OPERATION_MODE_GET \
> > +		_IOWR(IOC_1588_BASE, 42, struct pch_opemode_ioctl)
> 
> Do they all have to be ioctls?  What exactly are they doing?
> 
> And are they 32/64bit safe?
> 
> thanks,
> 
> greg k-h

Best Regards,
Qi
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_