Messages in this thread |  | | | Date | Thu, 21 Oct 2010 16:12:24 -0700 | | From | Andrew Morton <> | | Subject | Re: [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.
|  |