Messages in this thread | | | Date | Thu, 18 Jul 2013 12:08:30 -0700 | Subject | Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller | From | Trent Piepho <> |
| |
On Thu, Jul 18, 2013 at 3:01 AM, Sourav Poddar <sourav.poddar@ti.com> wrote: > +Required properties: > +- compatible : should be "ti,dra7xxx-qspi". > +- reg: Should contain QSPI registers location and length. > +- #address-cells, #size-cells : Must be present if the device has sub-nodes > +- ti,hwmods: Name of the hwmod associated to the QSPI
What is ti,hwmods? It's not clear from the description. It also doesn't appear to be used in the driver. At least, I did not find any occurrence of "hwmods" in the driver code.
> +static inline unsigned long ti_qspi_readl_data(struct ti_qspi *qspi, > + unsigned long reg, int wlen)
"readl" means read LONG. That's what the L is for. But this does different widths.
> +{ > + switch (wlen) { > + case 8: > + return readw(qspi->base + reg); > + break; wlen == 8, but readw == 16 bit read?
The break after the return isn't necessary.
> + case 16: > + return readb(qspi->base + reg); > + break; wlen == 16, but readb == 8 bit read?
> + case 32: > + return readl(qspi->base + reg);
wlen == 32, readl == 32, this one makes sense, but....
> +static inline void ti_qspi_writel_data(struct ti_qspi *qspi, > + unsigned long val, unsigned long reg, int wlen)
> + case 32: > + writeb(val, qspi->base + reg); > + break;
A 32 bit write uses an 8 bit write command, while read is 32 bits??
This doesn't make a lot of sense. If it's actually correct, there should be come kind of comment about it.
> + > +static int ti_qspi_setup(struct spi_device *spi) > +{
> + > + clk_ctrl_reg = ti_qspi_readl(qspi, QSPI_SPI_CLOCK_CNTRL_REG); > + > + clk_ctrl_reg &= ~QSPI_CLK_EN; > + > + /* disable SCLK */ > + ti_qspi_writel(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
Did you read this from Documentation/spi/spi-summary? ** BUG ALERT: for some reason the first version of ** many spi_master drivers seems to get this wrong. ** When you code setup(), ASSUME that the controller ** is actively processing transfers for another device.
> +static int ti_qspi_probe(struct platform_device *pdev) > +{ > + > + master->mode_bits = SPI_CPOL | SPI_CPHA;
Does your device support full duplex? It doesn't look like it does. You should set the SPI_MASER_HALF_DUPLEX flag in master->flags.
> + > + if (!of_property_read_u32(np, "ti,spi-num-cs", &num_cs)) > + master->num_chipselect = num_cs;
You didn't document this property. How is this different than the "num-cs" property already documented in spi-bus bindings?
> + qspi->base = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(qspi->base)) { > + ret = -ENOMEM;
Shouldn't that be ret = PTR_ERR(qspi->base)
| |