Re: [PATCH 2/2] SPI: Add support for Zynq Quad SPI controller

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]

On Fri, Apr 04, 2014 at 08:59:47AM +0530, Harini Katakam wrote:
> On Fri, Apr 4, 2014 at 2:59 AM, Mark Brown <broonie@kernel.org> wrote:

> > 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).

The requirement for setup() to work when other transfers are in progress
is clear and unambiguous, it really isn't acceptable to reconfigure
hardware in use by a runing transfer in setup().

> I explained the same in SPI v2 changes and this valid there too.

This is v2?

> >> +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.

You can readily access the device data from the master - look at how
other drivers do this.

> >> +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?)

Why are these things required after the core has already idled the
device (using exactly the same function)?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]