Re: [PATCH] SPI: MIPS: lantiq: adds spi-xway

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

On Thu, Aug 16, 2012 at 09:57:01AM +0200, John Crispin wrote:
> This patch adds support for the SPI core found on several Lantiq SoCs.
> The Driver has been runtime tested in combination with m25p80 Flash Devices
> on Amazon_SE and VR9.
> 
> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@googlemail.com>
> Signed-off-by: John Crispin <blogic@openwrt.org>

I'm not seeing any binding documentation in here but there's OF
bindings - any new device tree bindings should have documentation
attached.

> +static inline u32 ltq_spi_reg_read(struct ltq_spi *hw, u32 reg)
> +{
> +	return ioread32be(hw->base + reg);
> +}

Can you use regmap_mmio?  Not that it makes much difference, really -
it's totally unimportant, just something to consider.

> +static void ltq_spi_hw_enable(struct ltq_spi *hw)
> +{
> +	u32 clc;

Obviously it'd be nice if this were only done during SPI transfers,
currently the module is enabled whenever the driver is loaded.  Again
just something to consider.

> +static u32 ltq_spi_tx_word_u8(struct ltq_spi *hw)
> +{
> +	const u8 *tx = hw->tx;
> +	u32 data = *tx++;
> +
> +	hw->tx_cnt++;
> +	hw->tx++;
> +
> +	return data;
> +}

I can't help but think that there's some stuff here that ought to be
factored out for bitbanging controllers, but it's not that important.

> +static void ltq_spi_cleanup(struct spi_device *spi)
> +{
> +
> +}

Just remove empty functions - if the function is mandatory it at least
needs an explanation as to why your driver doesn't need to do anything.

> +       if (of_machine_is_compatible("lantiq,ase"))
> +               master->num_chipselect = 3;
> +       else
> +               master->num_chipselect = 6;

This is very suspicious - why is this being done based on the machine
rather than based on the IP?  Surely there can be machines with this SoC
on which aren't compatible with whatever (reference?) board this is
matching on.  I'd expect that the driver would have multiple compatible
strings which it uses to distinguish the capabilities of the IP.

Though actually the driver never reads this value so perhaps the code
can just be deleted and we rely on the fact that if the /CS isn't
physically present nobody's going to hook it up on a board so just
always set it to 6?

> +	pr_info("Lantiq SoC SPI controller rev %u (TXFS %u, RXFS %u, DMA %u)\n",
> +		id & LTQ_SPI_ID_REV_MASK, hw->txfs, hw->rxfs, hw->dma_support);

dev_info().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]