Re: [PATCH 1/2] spi: Add SPI driver for Mikrotik RB4xx series boards

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]