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

[Sourav Poddar <sourav.poddar@ti.com>]

On Friday 19 July 2013 12:38 AM, Trent Piepho wrote:
> 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?
Yes, I need to change this. should be readb.
> The break after the return isn't necessary.
>
>> +       case 16:
>> +               return readb(qspi->base + reg);
>> +               break;
> wlen == 16, but readb == 8 bit read?
>
same here.
>> +       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.
>
Yes, I will change this in the next version.
>> +
>> +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.
>
But I see in this documentation, that setup is usually used for setting up
device clock rate, modes etc.
So, what do you recommend here, should we move clk settings to prepare
hardware ?
>> +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.
>
hmm. Ok, will add.
>> +
>> +       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?
>
Actually, it is no different. This also means the total number of 
chipselects.
I used it from omap mcspi. I will make this property in accordance with the
generic binding.
Will also send a seperate patch for omap mcspi.
>> +       qspi->base = devm_ioremap_resource(&pdev->dev, r);
>> +       if (IS_ERR(qspi->base)) {
>> +               ret = -ENOMEM;
> Shouldn't that be ret = PTR_ERR(qspi->base)
hmm.will change.