On Thu, Jan 16, 2014 at 06:11:24PM +0100, Maxime Ripard wrote: Looks pretty clean, a few fairly small things below. > +- clocks: phandle to the clocks feeding the SPI controller. Two are > + needed: > + - "ahb": the gated AHB parent clock > + - "mod": the parent module clock I guess you should specify that this needs to be done with clock-names too then? > --- a/drivers/spi/Makefile > +++ b/drivers/spi/Makefile > @@ -69,6 +69,7 @@ obj-$(CONFIG_SPI_SH_HSPI) += spi-sh-hspi.o > obj-$(CONFIG_SPI_SH_MSIOF) += spi-sh-msiof.o > obj-$(CONFIG_SPI_SH_SCI) += spi-sh-sci.o > obj-$(CONFIG_SPI_SIRF) += spi-sirf.o > +obj-$(CONFIG_ARCH_SUNXI) += spi-sun6i.o I would expect a new Kconfig symbol for this. > +static inline void sun6i_spi_drain_fifo(struct sun6i_spi *sspi, int len) > +{ > + u32 reg, cnt; > + u8 byte; > + > + /* See how much data are available */ data is available. > + while (len--) { > + byte = readb(sspi->base_addr + SUN6I_RXDATA_REG); > + if (sspi->rx_buf) > + *sspi->rx_buf++ = byte; > + } It seems like this hardware is only able to handle bidirectional operation - this is actually quite common and isn't always as simple as it is here. Can I persuade you to put something in the core which provides dummy data buffers for this case? I was thinking flags like must_tx and must_rx or something but didn't get around to this yet. > +static int sun6i_spi_finish_transfer(struct spi_device *spi, > + struct spi_transfer *tfr, > + bool cs_change) > +{ > + struct sun6i_spi *sspi = spi_master_get_devdata(spi->master); > + > + sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH); > + > + if (tfr->delay_usecs) > + udelay(tfr->delay_usecs); If you implement this using transfer_one() (as you should) the core will do this for you. > + if (status & SUN6I_INT_CTL_RF_OVF) { > + sun6i_spi_drain_fifo(sspi, SUN6I_FIFO_DEPTH); > + sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_RF_OVF); > + return IRQ_HANDLED; > + } This looks like an overflow - a log message would be helpful for users and you should possibly be flagging an error on the current transfer. > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sspi->base_addr = devm_request_and_ioremap(&pdev->dev, res); > + if (!sspi->base_addr) { > + dev_err(&pdev->dev, "Unable to remap IO\n"); > + ret = -ENXIO; > + goto err; > + } devm_ioremap_resource() is nicer in that it returns an error and then you don't need to log either since it's noisy itself. > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "No spi IRQ specified\n"); > + ret = -ENXIO; Don't overwrite the error code. > + ret = clk_set_rate(sspi->mclk, 100000000); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't change module clock rate\n"); > + goto err2; > + } Does this really need to be fatal (or done at all)? There seems to be another reasonably flexible divider in the IP and it's more common to either set this per transfer to something that rounds nicely or just use the default and rely on the dividers. > + ret = clk_prepare_enable(sspi->mclk); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't enable clock 'ahb spi'\n"); > + goto err2; > + } I would recommend moving these to runtime PM so the clocks are only active when the device is actually in use, the core will do the runtime PM management if you set auto_runtime_pm so it's really easy to implement. > + ret = reset_control_deassert(sspi->rstc); > + if (ret) { > + dev_err(&pdev->dev, "Couldn't deassert the device from reset\n"); > + goto err3; > + } > + > + sun6i_spi_write(sspi, SUN6I_GBL_CTL_REG, > + SUN6I_GBL_CTL_BUS_ENABLE | SUN6I_GBL_CTL_MASTER | SUN6I_GBL_CTL_TP); Similarly here the IP could be kept in reset when not in use.
Attachment:
signature.asc
Description: Digital signature