Re: [RFC][PATCH 2/5] OMAP SSI driver interface

[Felipe Balbi <me@felipebalbi.com>]

On Fri, Oct 03, 2008 at 02:52:27PM +0300, Carlos Chinea wrote:

add a patch description body here.

> Signed-off-by: Carlos Chinea <carlos.chinea@nokia.com>
> ---
>  include/linux/ssi_driver_if.h |  137 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 137 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/ssi_driver_if.h
> 
> diff --git a/include/linux/ssi_driver_if.h b/include/linux/ssi_driver_if.h
> new file mode 100644
> index 0000000..3379dd0
> --- /dev/null
> +++ b/include/linux/ssi_driver_if.h

why wouldn't ssi.h be enough as a header name ?

looks much better and much easier to type: #include <linux/ssi.h>

> @@ -0,0 +1,137 @@
> +/*
> + * ssi_driver_if.h
> + *
> + * Header for the SSI driver low level interface.
> + *
> + * Copyright (C) 2007-2008 Nokia Corporation. All rights reserved.
> + *
> + * Contact: Carlos Chinea <carlos.chinea@nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +#ifndef __SSI_DRIVER_IF_H__
> +#define __SSI_DRIVER_IF_H__
> +
> +#include <linux/device.h>
> +
> +#define SSI_IOMEM_NAME		"SSI_IO_MEM"
> +#define SSI_P1_MPU_IRQ0_NAME	"SSI_P1_MPU_IRQ0"
> +#define SSI_P2_MPU_IRQ0_NAME	"SSI_P2_MPU_IRQ0"
> +#define SSI_P1_MPU_IRQ1_NAME	"SSI_P1_MPU_IRQ1"
> +#define SSI_P2_MPU_IRQ1_NAME	"SSI_P2_MPU_IRQ1"
> +#define SSI_GDD_MPU_IRQ_NAME	"GDD_MPU_IRQ"

hmm... I wonder you really need these ? Maybe I have to wait until I
review the other patches but at least for the irq names, they look
weird. Are them used for request_irq() only ? If so, remove them and
pass it in the driver. There's no need for such a global definition.

> +
> +/* IRQ values */
> +#define SSI_P1_MPU_IRQ0		67
> +#define SSI_P2_MPU_IRQ0		68
> +#define SSI_P1_MPU_IRQ1		69
> +#define SSI_P2_MPU_IRQ1		70
> +#define SSI_GDD_MPU_IRQ		71

Most likely this will be platform_specific right ? So pass it to the
driver using a struct resource.

> +
> +/* The number of ports handled by the driver. (MAX:2) */
> +#define SSI_MAX_PORTS		1
> +
> +/*
> + * Masks used to enable or disable the reception of certain hardware events
> + * for the ssi_device_drivers
> + */
> +#define SSI_EVENT_CLEAR			0x00
> +#define SSI_EVENT_MASK			0xFF
> +#define SSI_EVENT_BREAK_DETECTED_MASK	0x01
> +#define SSI_EVENT_ERROR_MASK		0x02
> +
> +#define ANY_SSI_CONTROLLER	-1
> +#define ANY_CHANNEL		-1
> +#define CHANNEL(channel)	(1<<channel)

CHANNEL is not generic enough name, use, at least, SSI_CHANNEL and add
spaces around that left shift.

> +
> +enum {
> +	SSI_EVENT_BREAK_DETECTED = 0,
> +	SSI_EVENT_ERROR,
> +};
> +
> +enum {
> +	SSI_IOCTL_WAKE_UP,
> +	SSI_IOCTL_WAKE_DOWN,
> +	SSI_IOCTL_SEND_BREAK,
> +	SSI_IOCTL_WAKE,
> +};

hmm... ioctls, let's if they're really needed later.

> +
> +/* Forward references */
> +struct ssi_device;
> +struct ssi_dev;
> +struct ssi_port;
> +struct ssi_channel;
> +
> +struct ssi_port_pd {
> +	u32 tx_mode;
> +	u32 tx_frame_size;
> +	u32 divisor;
> +	u32 tx_ch;
> +	u32 arb_mode;
> +	u32 rx_mode;
> +	u32 rx_frame_size;
> +	u32 rx_ch;
> +	u32 timeout;
> +	u8 n_irq;
> +};
> +
> +struct ssi_platform_data {
> +	unsigned char *clk_name;

please don't pass clock names via platform_data. It's ugly and we're
having quite a big amount of work trying to find a solution to clean
omap drivers. Looks like we're gonna introduce omap_clk_associate()
which will associate the user device with the clock structure and
introduce a clk alias name (called function name) to avoid the problem
we have now with different omap versions (different clock names).

> +	struct ssi_dev *ssi_ctrl;
> +	struct ssi_port_pd *ports;
> +	u8 num_ports;
> +};
> +
> +struct ssi_device {
> +	int n_ctrl;
> +	unsigned int n_p;
> +	unsigned int n_ch;
> +	char modalias[BUS_ID_SIZE];
> +	struct ssi_channel *ch;
> +	struct device device;
> +};
> +
> +#define to_ssi_device(dev)	container_of(dev, struct ssi_device, device)
> +
> +struct ssi_device_driver {
> +	unsigned long		ctrl_mask;
> +	unsigned long		ch_mask[SSI_MAX_PORTS];
> +	unsigned long		event_mask;
> +	void 			(*port_event) (int c_id, unsigned int port,
	    ^ trailing whitespace

> +						unsigned int event, void *arg);
> +	int			(*probe)(struct ssi_device *dev);

and then this will be the only bus not using the MODULE_DEVICE_TABLE,
please fix. Introduce a MODULE_DEVICE_TABLE. i2c did it recently and
it's quite simple. Most likely this will have a similar implementation.

> +	int			(*remove)(struct ssi_device *dev);
> +	int			(*suspend)(struct ssi_device *dev,
> +						pm_message_t mesg);
> +	int			(*resume)(struct ssi_device *dev);
> +	struct device_driver 	driver;
			    ^ trailing whitespace
> +};
> +
> +#define to_ssi_device_driver(drv) container_of(drv, \
> +						struct ssi_device_driver, \
> +						driver)
> +
> +int register_ssi_driver(struct ssi_device_driver *driver);

let's try to keep a consistency with several other register and
unregister functions in the kernel and rename this to
ssi_register_driver(), likewise to ssi_unregister_driver()

> +void unregister_ssi_driver(struct ssi_device_driver *driver);
> +int ssi_open(struct ssi_device *dev);
> +int ssi_write(struct ssi_device *dev, u32 *data, unsigned int count);
> +void ssi_write_cancel(struct ssi_device *dev);
> +int ssi_read(struct ssi_device *dev, u32 *data, unsigned int w_count);
> +void ssi_read_cancel(struct ssi_device *dev);
> +int ssi_ioctl(struct ssi_device *dev, unsigned int command, void *arg);
> +void ssi_close(struct ssi_device *dev);
> +void ssi_dev_set_cb(struct ssi_device *dev, void (*r_cb)(struct ssi_device *dev)
> +					, void (*w_cb)(struct ssi_device *dev));
					^ this comma should be in the
					previous line

> +#endif

#endif /* __SSI__ blablabla */

btw, a bit of kerneldoc wouldn't hurt. Please document all these
structures.

-- 
balbi