On Thu, Jul 18, 2013 at 3:01 AM, Sourav Poddar <sourav.poddar@xxxxxx> 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
