lkml.org 
[lkml]   [2010]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 01/12] misc: add driver for sequencer serial port
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.


\
 
 \ /
  Last update: 2010-10-22 01:15    [W:0.130 / U:1.092 seconds]
©2003-2014 Jasper Spaans. Advertise on this site