On Fri, Mar 20, 2015 at 01:16:32PM +0100, Bert Vermeulen wrote:
> +#define DRV_NAME "rb4xx-spi"
> +#define DRV_DESC "Mikrotik RB4xx SPI controller driver"
Both of these are used exactly once, the defines aren't adding anything
except indirection.
> +#define DRV_VERSION "0.1.0"
The kernel is already versioned, don't include versions for individual
drivers - nobody is going to update it anyway.
> +static unsigned spi_clk_low = AR71XX_SPI_IOC_CS1;
No global variables, use driver data.
> +#ifdef RB4XX_SPI_DEBUG
> +static inline void do_spi_delay(void)
> +{
> + ndelay(20000);
> +}
> +#else
> +static inline void do_spi_delay(void) { }
> +#endif
Remove this, if it's useful implement it generically.
> +static inline void do_spi_init(struct spi_device *spi)
> +{
> + unsigned cs = AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1;
> +
> + if (!(spi->mode & SPI_CS_HIGH))
> + cs ^= (spi->chip_select == 2) ? AR71XX_SPI_IOC_CS1 :
> + AR71XX_SPI_IOC_CS0;
Please write this expression in a more legible fashion, I can't really
tell what it's supposed to do.
> +static void do_spi_byte(void __iomem *base, unsigned char byte)
> +{
> + do_spi_clk(base, byte >> 7);
> + do_spi_clk(base, byte >> 6);
> + do_spi_clk(base, byte >> 5);
> + do_spi_clk(base, byte >> 4);
> + do_spi_clk(base, byte >> 3);
> + do_spi_clk(base, byte >> 2);
> + do_spi_clk(base, byte >> 1);
> + do_spi_clk(base, byte);
This looks awfully like it's bitbanging the value out, can we not use
spi-bitbang here?
> + pr_debug("spi_byte sent 0x%02x got 0x%02x\n",
> + (unsigned)byte,
> + (unsigned char)__raw_readl(base + AR71XX_SPI_REG_RDS));
dev_dbg().
> +static inline void do_spi_clk_fast(void __iomem *base, unsigned bit1,
> + unsigned bit2)
Why would we ever want the slow version?
> +static int rb4xx_spi_msg(struct rb4xx_spi *rbspi, struct spi_message *m)
> +{
> + struct spi_transfer *t = NULL;
> + void __iomem *base = rbspi->base;
> +
> + m->status = 0;
> + if (list_empty(&m->transfers))
> + return -1;
> +
> + __raw_writel(AR71XX_SPI_FS_GPIO, base + AR71XX_SPI_REG_FS);
> + __raw_writel(SPI_CTRL_FASTEST, base + AR71XX_SPI_REG_CTRL);
> + do_spi_init(m->spi);
> +
> + list_for_each_entry(t, &m->transfers, transfer_list) {
> + int len;
This is reimplementing the core message queue code, provide a
transfer_one() operation if there's some reason not to use bitbang.
> +static void rb4xx_spi_process_queue_locked(struct rb4xx_spi *rbspi,
> + unsigned long *flags)
Similarly all the queue code is reimplementing core functionality.
> +static int __init rb4xx_spi_init(void)
> +{
> + return platform_driver_register(&rb4xx_spi_drv);
> +}
> +subsys_initcall(rb4xx_spi_init);
> +
> +static void __exit rb4xx_spi_exit(void)
> +{
> + platform_driver_unregister(&rb4xx_spi_drv);
> +}
> +
> +module_exit(rb4xx_spi_exit);
module_platform_driver()
Attachment:
signature.asc
Description: Digital signature
