lkml.org 
[lkml]   [2009]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] spi: Add support for the OpenCores SPI controller.
    Date
    Hmm ... Open Hardware, Free Software.  Sounds like a good match!

    This driver has some all-too-common failings though:

    (a) doesn't correctly reject invalid device configurations
    in the setup() method

    (b) doesn't correctly reject unsupported per-message options
    in the transfer() method

    (c) the setup() method will trash concurrent transfers, since
    it wrongly assumes there's only one device

    There are other minor nits too.


    On Thursday 26 March 2009, Thierry Reding wrote:

    > +struct spioc {
    > + struct spi_master *master;
    > + struct spioc_info *info;

    Nothing in "info" is really needed after probe(); no
    point in recording it here.


    > static void process_transfers(unsigned long data)

    ... buggy (by inference), see below


    > +static int spioc_setup(struct spi_device *spi)
    > +{

    Hmm, you seem to have disregarded the Documentation/spi/spi-summary
    text describing what this does:

    Unless each SPI slave has its own configuration registers, don't
    change them right away ... otherwise drivers could corrupt I/O
    that's in progress for other SPI devices.

    Because:


    > + struct spioc *spioc = spi_master_get_devdata(spi->master);
    > + unsigned long clkdiv = 0x0000ffff;
    > + u32 ctrl = spioc_read(spioc, SPIOC_CTRL);

    CTRL is a controller-global register, not a per-chipselect one ...

    > +
    > + /* make sure we're not busy */
    > + ctrl &= ~CTRL_BUSY;
    > +
    > + if (!spi->bits_per_word)
    > + spi->bits_per_word = 8;
    > +
    > + if (spi->mode & SPI_LSB_FIRST)
    > + ctrl |= CTRL_LSB;
    > + else
    > + ctrl &= ~CTRL_LSB;
    > +
    > + /* adapt to clock polarity and phase */
    > + if (spi->mode & SPI_CPOL) {
    > + if (spi->mode & SPI_CPHA) {
    > + ctrl |= CTRL_TXNEG;
    > + ctrl &= ~CTRL_RXNEG;
    > + } else {
    > + ctrl &= ~CTRL_TXNEG;
    > + ctrl |= CTRL_RXNEG;
    > + }
    > + } else {
    > + if (spi->mode & SPI_CPHA) {
    > + ctrl &= ~CTRL_TXNEG;
    > + ctrl |= CTRL_RXNEG;
    > + } else {
    > + ctrl |= CTRL_TXNEG;
    > + ctrl &= ~CTRL_RXNEG;
    > + }
    > + }

    Notice how you're accepting *all* spi->mode bits, instead of
    insisting that the only bits set there be ones you support.

    Best that you do something along the lines of

    /* mask of all SPI options this driver currently handles */
    #define MODEBITS (SPI_CPOL|SPI_CPHA|SPI_LSB_FIRST)

    if (spi->mode & ~MODEBITS)
    return -EINVAL;

    near the top of this routine.

    > +
    > + /* set the clock divider */
    > + if (spi->max_speed_hz)
    > + clkdiv = DIV_ROUND_UP(clk_get_rate(spioc->clk),
    > + 2 * spi->max_speed_hz) - 1;
    > +
    > + if (clkdiv > 0x0000ffff)
    > + clkdiv = 0x0000ffff;

    Shouldn't you just be failing setup() if the slowest clock rate
    you support is still too fast for this peripheral?


    > +
    > + spioc_write(spioc, SPIOC_DIV, clkdiv);
    > + spioc_write(spioc, SPIOC_CTRL, ctrl);

    ... here you reprogram parameters in use by the current
    transfer.

    Probably what you should do instead is compute the values to
    be used for those two registers, and save them in a struct
    referenced through spi_device.controller_state before you
    start any individual transfer.

    However, your process_transfers() currently seems to presume
    a global setup model; that will need to change. Minimally
    you should set the transfer parameters for the current device.

    And it will need to cope with per-transfer overrides for the
    word size and bitrate, unless you reject them when messages
    get submitted.


    > +
    > + /* deassert chip-select */
    > + spioc_chipselect(spioc, NULL);
    > +
    > + return 0;
    > +}
    > +
    > +static int spioc_transfer(struct spi_device *spi, struct spi_message *message)
    > +{
    > + struct spi_master *master = spi->master;
    > + struct spioc *spioc = spi_master_get_devdata(master);
    > + unsigned long flags;

    Here you're accepting all messages, even ones that rely on
    mechanisms you don't support. You should either implement
    the per-transfer wordsize, bitrate, and chipselect overrides;
    or else scan the transfer list of this message to make sure
    that no transfer requires them.

    > +
    > + spin_lock_irqsave(&spioc->lock, flags);
    > +
    > + list_add_tail(&message->queue, &spioc->queue);
    > + queue_work(spioc->workqueue, &spioc->process_messages);
    > +
    > + spin_unlock_irqrestore(&spioc->lock, flags);
    > + return 0;
    > +}
    > +
    > +static void spioc_cleanup(struct spi_device *spi)
    > +{

    This would be where you should kfree() the struct holding
    per-device register settings that your setup() should allocate.

    Having such NOP methods should be a sign that something
    is wrong...

    > +}


    > +static int __devinit spioc_probe(struct platform_device *pdev)
    > +{
    > + ...
    > +
    > + res = request_mem_region(res->start, res->end - res->start + 1,
    > + res->name);

    I think resource_size(resource) should be used here and in the ioremap
    call below.

    > + if (!res) {
    > + dev_err(&pdev->dev, "unable to request memory region\n");
    > + return -ENXIO;
    > + }
    > +
    > + mmio = ioremap_nocache(res->start, res->end - res->start + 1);
    > + if (!mmio) {
    > + dev_err(&pdev->dev, "can't remap I/O region\n");
    > + retval = -ENXIO;
    > + goto release;
    > + }
    > + ...


    > +#ifdef CONFIG_PM
    > +static int spioc_suspend(struct platform_device *pdev, pm_message_t state)
    > +{
    > + return -ENOSYS;
    > +}
    > +
    > +static int spioc_resume(struct platform_device *pdev)
    > +{
    > + return -ENOSYS;
    > +}

    Why are you aborting system suspend transitions instead of
    just not supporting PM? Better IMO to depend on !PM if
    this is trying to prevent nasty stuff from happening.


    > +
    > +struct spioc_info {
    > + s16 bus_num;

    Better IMO to just use platform_device.id for this.

    > + u16 num_chipselect;
    > +};
    > +
    > +#endif /* !LINUX_SPI_SPIOC_H */
    > +
    >


    \
     
     \ /
      Last update: 2009-04-04 21:31    [W:0.034 / U:0.872 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site