On Thu, Jul 18, 2013 at 3:01 AM, Sourav Poddar <[email protected]> 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)
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
