On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.s...@pengutronix.de> wrote:
> Hi Grant,
>
> some comments below:
Hi Wolfram. Thanks for the review.
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...
No, not needed at all. Will remove.
>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?
Yes, really asm. Needed for get_tlb()
>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.lik...@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1 0x00
>> +#define SPI_CTRL1_SPIE (1 << 7)
>> +#define SPI_CTRL1_SPE (1 << 6)
>> +#define SPI_CTRL1_MSTR (1 << 4)
>> +#define SPI_CTRL1_CPOL (1 << 3)
>> +#define SPI_CTRL1_CPHA (1 << 2)
>> +#define SPI_CTRL1_SSOE (1 << 1)
>> +#define SPI_CTRL1_LSBFE (1 << 0)
>> +
>> +#define SPI_CTRL2 0x01
>> +#define SPI_BRR 0x04
>> +
>> +#define SPI_STATUS 0x05
>> +#define SPI_STATUS_SPIF (1 << 7)
>> +#define SPI_STATUS_WCOL (1 << 6)
>> +#define SPI_STATUS_MODF (1 << 4)
>> +
>> +#define SPI_DATA 0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR 0x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP 0 /* Nothing more for the state machine to */
>> + /* do. If something interesting happens */
>> + /* then and IRQ will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.
The double spaces at the end of sentences are intentional. It is
perhaps a bit quaint and old fashioned, but it is my writing style.
>
>> +#define FSM_POLL 1 /* need to poll for completion, an IRQ is */
>> + /* not expected */
>> +#define FSM_CONTINUE 2 /* Keep iterating the state machine */
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> + struct spi_master *master;
>> + u32 sysclk;
>
> unused?
yes, fixed.
>> + void __iomem *regs;
>> + int irq0; /* MODF irq */
>> + int irq1; /* SPIF irq */
>> + int ipb_freq;
>
> unsigned int will suit it better IMHO.
right.
>> +
>> + /* Statistics */
>> + int msg_count;
>> + int wcol_count;
>> + int wcol_ticks;
>> + u32 wcol_tx_timestamp;
>> + int modf_count;
>> + int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> them will be ugly. Well, I can't make up if this is just overhead or useful
> debugging aid, so no real objection :)
There used to be a sysfs interface for dumping these, but it was an
ugly misuse. I'd like to leave these in. I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.
> But I wonder more about the usage of the SS pin and if this chipsel is needed
> at all (sadly I cannot test as I don't have any board with SPI connected to
> that device). You define the SS-pin as output, but do not set the SSOE-bit.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic
> purpose
> output.
That's right. The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.
>> + /* initialize the device */
>> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
>
> spaces around operator.
Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators. I think it is easier to
read that way.
>> diff --git a/include/linux/spi/mpc52xx_spi.h
>> b/include/linux/spi/mpc52xx_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> + void (*hook)(struct spi_message *m,
>> + void *context),
>> + void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.
Yeah, this is cruft. Removed.
Thanks!
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
