linux-mips
[Top] [All Lists]

Re: [PATCH 2/2] SPI: ralink: add Ralink SoC spi driver

To: John Crispin <blogic@openwrt.org>
Subject: Re: [PATCH 2/2] SPI: ralink: add Ralink SoC spi driver
From: Mark Brown <broonie@kernel.org>
Date: Sun, 11 Aug 2013 14:26:42 +0100
Cc: Gabor Juhos <juhosg@openwrt.org>, linux-spi@vger.kernel.org, linux-mips@linux-mips.org
In-reply-to: <1376074288-29302-2-git-send-email-blogic@openwrt.org>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1376074288-29302-1-git-send-email-blogic@openwrt.org> <1376074288-29302-2-git-send-email-blogic@openwrt.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Aug 09, 2013 at 08:51:27PM +0200, John Crispin wrote:

Looks fairly good, a few things below but most of them are just using
the core to do things instead of open coding them in the driver rather
than anything substantial.

> +#ifdef DEBUG
> +#define spi_debug(args...) printk(args)
> +#else
> +#define spi_debug(args...)
> +#endif

This shouldn't be driver specific if it's useful, though really it looks
like the driver should just be using dev_dbg() and friends.

> +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 
> mask)
> +{
> +     void __iomem *addr = rs->base + reg;
> +     u32 val;
> +
> +     val = ioread32(addr);
> +     val |= mask;
> +     iowrite32(val, addr);
> +}

Is this always called with a suitable lock held?

> +     if (bits_per_word != 8) {

You should be setting bits_per_word_mask in the master structure, then
you don't need to check for this.

> +static inline int rt2880_spi_wait_till_ready(struct rt2880_spi *rs)
> +{
> +     int i;
> +
> +     for (i = 0; i < RALINK_SPI_WAIT_RDY_MAX_LOOP; i++) {
> +             u32 status;
> +
> +             status = rt2880_spi_read(rs, RAMIPS_SPI_STAT);
> +             if ((status & SPISTAT_BUSY) == 0)
> +                     return 0;
> +
> +             udelay(1);
> +     }

A cpu_relax() here would be nice.

> +static unsigned int
> +rt2880_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
> +{
> +     struct rt2880_spi *rs = spidev_to_rt2880_spi(spi);
> +     unsigned count = 0;
> +     u8 *rx = xfer->rx_buf;
> +     const u8 *tx = xfer->tx_buf;
> +     int err;
> +
> +     spi_debug("%s(%d): %s %s\n", __func__, xfer->len,
> +               (tx != NULL) ? "tx" : "  ",
> +               (rx != NULL) ? "rx" : "  ");
> +
> +     if (tx) {
> +             for (count = 0; count < xfer->len; count++) {
> +                     rt2880_spi_write(rs, RAMIPS_SPI_DATA, tx[count]);
> +                     rt2880_spi_setbits(rs, RAMIPS_SPI_CTL, SPICTL_STARTWR);
> +                     err = rt2880_spi_wait_till_ready(rs);
> +                     if (err) {
> +                             dev_err(&spi->dev, "TX failed, err=%d\n", err);
> +                             goto out;
> +                     }
> +             }
> +     }
> +
> +     if (rx) {

There is presumably a maximum transfer size here from the FIFO that is
holding the data?

> +             if (bits_per_word != 8) {
> +                     dev_err(&spi->dev,
> +                             "message rejected: invalid transfer 
> bits_per_word (%d bits)\n",
> +                             bits_per_word);

Like I say set bits_per_word_mask...

> +             if (t->speed_hz && t->speed_hz < (rs->sys_freq / 128)) {
> +                     dev_err(&spi->dev,
> +                             "message rejected: device min speed (%d Hz) 
> exceeds required transfer speed (%d Hz)\n",
> +                             (rs->sys_freq / 128), t->speed_hz);
> +                     status = -EIO;
> +                     goto msg_done;
> +             }

Set min_speed_hz in the spi_master and the core will check this for you.

> +     if (spi->max_speed_hz < (rs->sys_freq / 128)) {
> +             dev_err(&spi->dev, "setup: requested speed is too low %d Hz\n",
> +                     spi->max_speed_hz);
> +             return -EINVAL;
> +     }
> +
> +     if (spi->bits_per_word != 0 && spi->bits_per_word != 8) {
> +             dev_err(&spi->dev,
> +                     "setup: requested bits per words - os wrong %d bpw\n",
> +                     spi->bits_per_word);
> +             return -EINVAL;
> +     }

Again the core can do this for you.

> +     r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     base = devm_request_and_ioremap(&pdev->dev, r);
> +     if (IS_ERR(base))
> +             return PTR_ERR(base);

devm_ioremap_resource().

> +     status = clk_enable(clk);
> +     if (status)
> +             return status;

clk_prepare_enable(), and it'd be nice to use runtime PM and enable the
clock only when doing transfers though that's not essential.

> +static int rt2880_spi_remove(struct platform_device *pdev)
> +{
> +     struct spi_master *master;
> +     struct rt2880_spi *rs;
> +
> +     master = dev_get_drvdata(&pdev->dev);
> +     rs = spi_master_get_devdata(master);
> +
> +     clk_disable(rs->clk);
> +     clk_put(rs->clk);

No clk_put if you've used devm_clk_get().

Attachment: signature.asc
Description: Digital signature

<Prev in Thread] Current Thread [Next in Thread>