Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
> Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Tested-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jonathan Richardson <jonathar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

No description at all?

You are introducing a new subsystem here, which means you get to put
a lot of thought into the API design, to ensure that it works with
other drivers of the same type, and that we don't already have
a subsystem that does what you need here.

Please write a few pages of text about the tradeoffs that went into
the internal and user-facing API design, and explain the differences
to the existing infrastructure we have for the clocksource, clockevent,
k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks,
in particular why your hardware cannot fit into the existing frameworks
and has to have a new one.

> +struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname)
> +{
> +	struct bcm_cygnus_dte *cygnus_dte = NULL;
> +	bool found = false;
> +
> +	if (!devname)
> +		return NULL;
> +
> +	list_for_each_entry(cygnus_dte, &dtedev_list, node) {
> +		if (!strcmp(dev_name(&cygnus_dte->pdev->dev), devname)) {
> +			/* Matched on device name */
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	return found ? cygnus_dte : NULL;
> +}
> +EXPORT_SYMBOL(dte_get_dev_from_devname);

No, don't match on a device name. If you must have a reference, use
a phandle in DT.

> +int dte_get_timestamp(
> +		struct bcm_cygnus_dte *cygnus_dte,
> +		enum dte_client client,
> +		struct timespec *ts)

Please use timespec64 or ktime_t for internal interfaces.

> +	case DTE_IOCTL_SET_DIVIDER:

For the IOCTLs, please write a documentation in man-page form. No need
for troff formatting, plain text is fine.

> +/**
> + * DTE Client
> + */
> +enum dte_client {
> +	DTE_CLIENT_MIN = 0,
> +	DTE_CLIENT_I2S0_BITCLOCK = 0,
> +	DTE_CLIENT_I2S1_BITCLOCK,
> +	DTE_CLIENT_I2S2_BITCLOCK,
> +	DTE_CLIENT_I2S0_WORDCLOCK,
> +	DTE_CLIENT_I2S1_WORDCLOCK,
> +	DTE_CLIENT_I2S2_WORDCLOCK,
> +	DTE_CLIENT_LCD_CLFP,
> +	DTE_CLIENT_LCD_CLLP,
> +	DTE_CLIENT_GPIO14,
> +	DTE_CLIENT_GPIO15,
> +	DTE_CLIENT_GPIO22,
> +	DTE_CLIENT_GPIO23,
> +	DTE_CLIENT_MAX,
> +};

Make this more abstract, so we can reuse the API for other vendors.

> +#define DTE_IOCTL_BASE          'd'
> +#define DTE_IO(nr)              _IO(DTE_IOCTL_BASE, nr)
> +#define DTE_IOR(nr, type)       _IOR(DTE_IOCTL_BASE, nr, type)
> +#define DTE_IOW(nr, type)       _IOW(DTE_IOCTL_BASE, nr, type)
> +#define DTE_IOWR(nr, type)      _IOWR(DTE_IOCTL_BASE, nr, type)
> +
> +#define DTE_IOCTL_SET_DIVIDER       DTE_IOW(0x00, struct dte_data)
> +#define DTE_IOCTL_ENABLE_TIMESTAMP  DTE_IOW(0x01, struct dte_data)
> +#define DTE_IOCTL_SET_IRQ_INTERVAL  DTE_IOW(0x02, struct dte_data)
> +#define DTE_IOCTL_GET_TIMESTAMP     DTE_IOWR(0x03, struct dte_timestamp)
> +#define DTE_IOCTL_SET_TIME          DTE_IOW(0x04, struct timespec)
> +#define DTE_IOCTL_GET_TIME          DTE_IOR(0x05, struct timespec)

Instead of timespec, use a pair of '__u64' values, or alternatively
just a '__u64' for nanoseconds if the API does not have to cover
times before 1970 or after 2262.

> +#define DTE_IOCTL_ADJ_TIME          DTE_IOW(0x06, int64_t)
> +#define DTE_IOCTL_ADJ_FREQ          DTE_IOW(0x07, int32_t)

Maybe 'struct timex' for the adjustment?

Also, how about adding new syscalls along the lines of
the timerfd stuff instead of using ioctl?

> +struct dte_data {
> +	enum dte_client client;
> +	unsigned int data;
> +};

No 'enum' in ioctl data, always use '__u32' etc.

> +
> +struct dte_timestamp {
> +	enum dte_client client;
> +	struct timespec ts;
> +};

Instead of timespec, use a pair of '__u64' values, or alternatively
just a '__u64' for nanoseconds if the API does not have to cover
times before 1970 or after 2262.

> +struct bcm_cygnus_dte;
> +
> +extern struct bcm_cygnus_dte *dte_get_dev_from_devname(
> +		const char *devname);
> +extern int dte_enable_timestamp(
> +		struct bcm_cygnus_dte *cygnus_dte,
> +		enum dte_client client,
> +		int enable);


Put the internal declarations into one header in include/linux, and the
user space facing ones in another one in include/uapi/linux.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html