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.035 / U:29.928 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site