Hi Mark,
On Fri, Apr 4, 2014 at 2:59 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Thu, Apr 03, 2014 at 10:33:07PM +0530, Punnaiah Choudary Kalluri wrote:
>
> Overall this looks fairly good, there are a few issues that need to be
> looked at but they're not too invasive. Please also check for coding
> style issues, quite a few spaces before commas for example.
>
Thanks. I'll check that.
<snip>
>> +/**
>> + * zynq_qspi_copy_read_data - Copy data to RX buffer
>> + * @xqspi: Pointer to the zynq_qspi structure
>> + * @data: The 32 bit variable where data is stored
>> + * @size: Number of bytes to be copied from data to RX buffer
>> + */
>> +static void zynq_qspi_copy_read_data(struct zynq_qspi *xqspi, u32 data, u8 size)
>> +{
>> + if (xqspi->rxbuf) {
>> + memcpy(xqspi->rxbuf, ((u8 *) &data) + 4 - size, size);
>> + xqspi->rxbuf += size;
>> + }
>> + xqspi->bytes_to_receive -= size;
>> +}
>
> Does this and the write function really need to be a separate function -
> it's trivial and used once? It's probably more beneficial to split out
> some of the more complex logic later on that's causing the indentation
> to get too deep.
>
I'm aware it's used in only one place but it does make receive data handling
easier for future. As you may have noticed there are 4 different ways to
write into transmit FIFO and the data read also differs accordingly.
I'll try to reduce the indentation in other places.
<snip>
>> +static int zynq_qspi_setup_transfer(struct spi_device *qspi,
>> + struct spi_transfer *transfer)
>> +{
>> + struct zynq_qspi *xqspi = spi_master_get_devdata(qspi->master);
>> + u32 config_reg, req_hz, baud_rate_val = 0;
>> +
>> + if (transfer)
>> + req_hz = transfer->speed_hz;
>> + else
>> + req_hz = qspi->max_speed_hz;
>
> Why would a transfer be being set up without a transfer being provided?
>
The setup function calls this function before a transfer is initiated.
In this case NULL is passed to setup_transfer (see below) and
SPI is initialized with default clock configuration.
This initialization is necessary because otherwise this clock config
would be done
only after SPI is enabled in prepare_hardware, which is wrong.
(I'm checking for master->busy in setup to address your previous
comment on SPI).
I explained the same in SPI v2 changes and this valid there too.
>> +/**
>> + * zynq_qspi_setup - Configure the QSPI controller
>> + * @qspi: Pointer to the spi_device structure
>> + *
>> + * Sets the operational mode of QSPI controller for the next QSPI transfer, baud
>> + * rate and divisor value to setup the requested qspi clock.
>> + *
>> + * Return: 0 on success and error value on failure
>> + */
>> +static int zynq_qspi_setup(struct spi_device *qspi)
>> +{
>> + if (qspi->master->busy)
>> + return -EBUSY;
>> +
>> + return zynq_qspi_setup_transfer(qspi, NULL);
>> +}
>
> No, this is broken - you have to support setup() while the hardware is
> active. Just remove this if there's nothing to do and set up on the
> transfer.
But where do you suggest this clock configuration be done?
I've looked at the option of doing it in prepare_hardware but
spi_device structure is not passed to it.
<snip>
>
>> + if (xqspi->rxbuf) {
>> + (*(u32 *)xqspi->rxbuf) =
>> + zynq_qspi_read(xqspi,
>> + ZYNQ_QSPI_RXD_OFFSET);
>> + xqspi->rxbuf += 4;
>
> This only works in 4 byte words? That seems a bit limited.
> Alternatively, if it works with smaller sizes (as it appears to) then
> isn't this at risk of overflowing buffers?
>
There is a
if (xqspi->bytes_to_receive < 4) {
above and this statement is in the else loop.
When less than 4 bytes are being read/received, the handling is different.
>> +static int __maybe_unused zynq_qspi_suspend(struct device *_dev)
>> +{
>> + struct platform_device *pdev = container_of(_dev,
>> + struct platform_device, dev);
>> + struct spi_master *master = platform_get_drvdata(pdev);
>> +
>> + spi_master_suspend(master);
>> +
>> + zynq_unprepare_transfer_hardware(master);
>
> Why are you unpreparing the hardware - the framework should be doing
> that for you if the device is active, if it's not you've got an extra
> clock disable here?
>
I called unprepare_hardware becuase it does the things necessary
after master suspend - disable clock and controller.
(I thought this was your suggestion for SPI?)
>> +static int __maybe_unused zynq_qspi_resume(struct device *dev)
>
> This doesn't appear to be calling init_hw() - is it guaranteed that all
> the register settings written there are OK after power on?
>
>> + ret = clk_prepare_enable(xqspi->aperclk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Unable to enable APER clock.\n");
>> + goto remove_master;
>> + }
>> +
>> + ret = clk_prepare_enable(xqspi->devclk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Unable to enable device clock.\n");
>> + goto clk_dis_aper;
>> + }
>
> The driver isn't using runtime_pm or otherwise disabling the clocks when
> idle so it looks like the clocks will always be enabled and the
> management in prepare and unprepare won't have any practical effect. I
> can see needing at least one of the clocks for setting up the device but
> probably either the probe should disable them as it finishes or you
> should move the clock enable/disable from prepare/unprepare to runtime
> PM.
OK I understand.
Thanks for the review.
Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
