linux-kernel - Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+7tXij7W_F0ZTQZ=YvK+qOLO+O-6WpTKxLerv8NXyrdydt7FA@mail.gmail.com>
Date:	Thu, 18 Jul 2013 12:08:30 -0700
From:	Trent Piepho <tpiepho@...il.com>
To:	Sourav Poddar <sourav.poddar@...com>
Cc:	broonie@...nel.org, spi-devel-general@...ts.sourceforge.net,
	grant.likely@...aro.org, linux-omap@...r.kernel.org, rnayak@...com,
	linux-kernel@...r.kernel.org, balbi@...com
Subject: Re: [PATCHv4 2/3] drivers: spi: Add qspi flash controller

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