lkml.org 
[lkml]   [2006]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller
Date

On Apr 7, 2006, at 12:22 AM, David Brownell wrote:

>> This driver supports the SPI controller on the MPC83xx SoC devices
>> from Freescale.
>> Note, this driver supports only the simple shift register SPI
>> controller and not
>> the descriptor based CPM or QUICCEngine SPI controller.
>
> Or the QSPI on Coldfire; there's a driver for that floating around,
> but for an
> older version of this framework (sans lists).

I'll leave that for others to figure out :)

>
>
>> --- /dev/null
>> +++ b/drivers/spi/spi_mpc83xx.c
>> @@ -0,0 +1,349 @@
>> +/*
>> + * MPC83xx SPI controller driver.
>> + *
>> + * Maintainer: Kumar Gala
>
> Needs a "Copyright (C) 2006 <NAME>" for the GPL to be valid; it's
> the copyright holder who licences the code.

Will fix.

>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify it
>> + * under the terms of the GNU General Public License as
>> published by the
>> + * Free Software Foundation; either version 2 of the License,
>> or (at your
>> + * option) any later version.
>> + */
>> ...
>> +
>> +/* Default for SPI Mode, slowest speed, MSB, inactive high, 8-bit
>> char */
>> +#define SPMODE_INIT_VAL (SPMODE_CI_INACTIVEHIGH |
>> SPMODE_CP_RISE_EDGECLK | \
>> + SPMODE_DIV16 | SPMODE_REV | SPMODE_MS | \
>> + SPMODE_LEN(7) | SPMODE_PM(0xf))
>
> Hmm, it will of course be overridden as soon as needed, but shouldn't
> that default be "inactive low" clock? SPI mode 0 that is. That
> stands
> out mostly because you were interpreting CPOL=0 as inactive high, and
> that's not my understanding of how that signal works...

I'll change it, not sure what I was thinking but SPI mode 0 being the
default makes
sense.

>> +struct mpc83xx_spi {
>> + /* bitbang has to be first */
>> + struct spi_bitbang bitbang;
>> + struct completion tx_done, rx_ready;
>> +
>> + u32 __iomem *base;
>
> Erm, OK, but fwiw my preference is to have pointer-to-struct and let
> the compiler calculate the offsets (and tell you when you pass the
> wrong
> kind of pointer). Otherwise such pointers should use "void __iomem *"
> (or maybe in your case "__be32 *"?) for explicit {base,offset}
> addressing.

I'll make that change.

>> +static inline void mpc83xx_spi_write_reg(__be32 * base, u32 reg,
>> u32 val)
>> +{
>> + out_be32(base + (reg >> 2), val);
>> +}
>> +
>> +static inline u32 mpc83xx_spi_read_reg(__be32 * base, u32 reg)
>> +{
>> + return in_be32(base + (reg >> 2));
>> +}
>
> ... here you use "__be32" not "u32", and no "__iomem" annotation. So
> this is inconsistent with the declaration above. Note that if you
> just made this "&bank->regname" you'd be having the compiler do any
> offset calculation magic, and the source code will be more obvious.

Yep, I know what you mean.

>> +static
>> +int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct
>> spi_transfer *t)
>> +{
>> + struct mpc83xx_spi *mpc83xx_spi;
>> + u32 regval;
>> + u32 len = t->bits_per_word - 1;
>> +
>> + if (len == 32)
>> + len = 0;
>
> So the hardware handles 1-33 bit words? It'd be good to filter
> the spi_setup() path directly then, returning EINVAL for illegal
> word lengths (and clock speeds).

Uhh, no. The HW supports 4-bit to 32-bit words. However the
encoding of 32-bit is 0 in the register field, and 8-bit is a value
of 7, etc.. (bit encodings 1 & 2 are invalid).

I'm not following you on spi_setup(), but I think you mean to error
change bits_per_word there and return EINVAL if its not one we support.

>> +static u32
>> +mpc83xx_spi_txrx(struct spi_device *spi, unsigned nsecs, u32
>> word, u8 bits)
>> +{
>> + struct mpc83xx_spi *mpc83xx_spi;
>> + mpc83xx_spi = spi_master_get_devdata(spi->master);
>> +
>> + INIT_COMPLETION(mpc83xx_spi->tx_done);
>> + INIT_COMPLETION(mpc83xx_spi->rx_ready);
>> +
>> + /* enable tx/rx ints */
>> + mpc83xx_spi_write_reg(mpc83xx_spi->base, SPIM_REG, SPIM_NF |
>> SPIM_NE);
>> +
>> + /* transmit word */
>> + mpc83xx_spi_write_reg(mpc83xx_spi->base, SPITD_REG, word);
>> +
>> + /* wait for both a tx & rx interrupt */
>> + wait_for_completion(&mpc83xx_spi->tx_done);
>> + wait_for_completion(&mpc83xx_spi->rx_ready);
>
> I guess I'm surprised you're not using txrx_buffers() and having
> that whole thing be IRQ driven, so the per-word cost eliminates
> the task scheduling. You already paid for IRQ handling ... why
> not have it store the rx byte into the buffer, and write the tx
> byte froom the other buffer? That'd be cheaper than what you're
> doing now ... in both time and code. Only wake up a task at
> the end of a given spi_transfer().

I dont follow you at all here. What are you suggesting I do?

>
> Plus, your IRQ handler should _not_ always return IRQ_HANDLED.
> Only return it if you actually do enter one of those branches...
>
>
>> + mpc83xx_spi->sysclk = pdata->sysclk;
>
> When MPC/PPC starts to support <linux/clk.h> would seem to be
> the right sort of time to
>
> mpc83xx-spi->clk = clk_get(&pdev->dev, "spi_clk");
>
> or whatever.

Will do that once we support <linux/clk.h>

>> +MODULE_DESCRIPTION("Simple Platform SPI Driver");
>
> How about "Simple MPC83xx SPI driver"?

Will change.

- k
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-04-07 16:09    [W:0.102 / U:0.600 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site