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@xxxxxxxxxx> 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:
signature.asc
Description: Digital signature
