Re: [PATCH v3 01/12] misc: add driver for sequencer serial port

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 21 Oct 2010 17:01:02 -0400
Cyril Chemparathy <cyril@ti.com> wrote:

> TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port
> device.  It has a built-in programmable execution engine that can be programmed
> to operate as almost any serial bus (I2C, SPI, EasyScale, and others).
> 
> This patch adds a driver for this controller device.  The driver does not
> expose a user-land interface.  Protocol drivers built on top of this layer are
> expected to remain in-kernel.
> 
>
> ...
>
> +struct ti_ssp_dev_data {
> +	const char	*dev_name;
> +	unsigned long	iosel; /* see note below */
> +	unsigned long	config;
> +	const void	*pdata;
> +	int		pdata_sz;


I suppose this really should have type size_t.  Also a better name is
"pdata_size" - we prefer to avoid this random omission of vowels from
kernel identifiers.  Just spell it out; it makes it easier to remember.

> +};
> +
> +struct ti_ssp_data {
> +	unsigned long		out_clock;
> +	struct ti_ssp_dev_data	dev_data[2];
> +};
> +
>
> ...
>
> +config TI_SSP
> +	depends on ARCH_DAVINCI_TNETV107X
> +	tristate "Sequencer Serial Port support"
> +	default y

Was `y' a good choice?

> +	---help---
> +	  Say Y here if you want support for the Sequencer Serial Port
> +	  in a Texas Instruments TNETV107X SoC.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ti_ssp.
>
> ...
>
> +#define dev2ssp(dev)	dev_get_drvdata(dev->parent)
> +#define dev2port(dev)	(to_platform_device(dev)->id)

These could be implemented as C funtions.  That's superior because of
the typechecking.  At present dev2ssp() will happily compile and fail
at runtime if passed anystructure which has a 'const struct device
*parent'.

> +/* Register Access Helpers */
> +static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
> +{
> +	return __raw_readl(ssp->regs + reg);
> +}
> +
> +static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
> +{
> +	__raw_writel(val, ssp->regs + reg);
> +}

Why are the __raw functions used here?

> +static inline void ssp_rmw(struct ti_ssp *ssp, int reg, u32 mask, u32 bits)
> +{
> +	u32 val = ssp_read(ssp, reg);
> +	val &= ~mask;
> +	val |= bits;
> +	ssp_write(ssp, reg, val);
> +}

Locking?  Perhaps this function must be called under ssp->lock?  If so,
that should be documented here and it appears that not all callsites
actually do that correctly.

>
> ...
>
> +static int __set_iosel(struct ti_ssp *ssp, int port, u32 iosel)
> +{
> +	unsigned val;
> +
> +	/* IOSEL1 gets the least significant 16 bits */
> +	val = ssp_read(ssp, REG_IOSEL_1);
> +	val &= 0xffff << (port ? 0 : 16);
> +	val |= (iosel & 0xffff) << (port ? 16 : 0);
> +	ssp_write(ssp, REG_IOSEL_1, val);
> +
> +	/* IOSEL2 gets the most significant 16 bits */
> +	val = ssp_read(ssp, REG_IOSEL_2);
> +	val &= 0x0007 << (port ? 0 : 16);
> +	val |= (iosel & 0x00070000) >> (port ? 0 : 16);
> +	ssp_write(ssp, REG_IOSEL_2, val);

More rmw's which need locking.  It should be documented please.  Both
callers get it right this time.

> +	return 0;
> +}
> +
>
> ...
>
> +int ti_ssp_run(struct device *dev, u32 pc, u32 input, u32 *output)
> +{
> +	struct ti_ssp *ssp = dev2ssp(dev);
> +	int port = dev2port(dev);
> +	int count;
> +
> +	if (pc & ~(0x3f))
> +		return -EINVAL;
> +
> +	ssp_port_write(ssp, port, PORT_ADDR, input >> 16);
> +	ssp_port_write(ssp, port, PORT_DATA, input & 0xffff);
> +	ssp_port_rmw(ssp, port, PORT_CFG_1, 0x3f, pc);
> +
> +	ssp_port_set_bits(ssp, port, PORT_CFG_1, SSP_START);
> +
> +	for (count = 10000; count; count--) {
> +		if ((ssp_port_read(ssp, port, PORT_CFG_1) & SSP_BUSY) == 0)
> +			break;
> +		udelay(1);
> +	}
> +
> +	if (output) {
> +		*(output) = (ssp_port_read(ssp, port, PORT_ADDR) << 16) |
> +			    (ssp_port_read(ssp, port, PORT_DATA) &  0xffff);
> +	}
> +
> +	if (!count) {
> +		dev_err(ssp->dev, "timed out waiting for SSP operation\n");
> +		return -EIO;
> +	}

There doesn't seem much point in writing to *output if the port_read()
timed out?

>
> ...
>

That's all fairly minor stuff.  It looks Good Enough For Linux to me.