Messages in this thread | | | Date | Fri, 20 Mar 2015 12:51:39 +0000 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] spi: Add SPI driver for Mikrotik RB4xx series boards |
| |
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() [unhandled content-type:application/pgp-signature] | |