Hi,
On Mon, Jul 08, 2013 at 03:33:30PM -0500, Nishanth Menon wrote:
> > +static inline unsigned long dra7xxx_readl_data(struct dra7xxx_qspi *qspi,
> > + unsigned long reg, int wlen)
> > +{
> > + switch (wlen) {
> > + case 8:
> > + return readw(qspi->base + reg);
> > + break;
> > + case 16:
> > + return readb(qspi->base + reg);
> > + break;
> > + case 32:
> > + return readl(qspi->base + reg);
> > + break;
> > + default:
> > + return -1;
> > + }
> > +}
> > +
> > +static inline void dra7xxx_writel_data(struct dra7xxx_qspi *qspi,
> > + unsigned long val, unsigned long reg, int wlen)
> > +{
> > + switch (wlen) {
> > + case 8:
> > + writew(val, qspi->base + reg);
> > + break;
> > + case 16:
> > + writeb(val, qspi->base + reg);
> > + break;
> > + case 32:
> > + writeb(val, qspi->base + reg);
> > + break;
> > + default:
> > + dev_dbg(qspi->dev, "word lenght out of range");
> > + break;
> > + }
> > +}
> Looks like a case to use regmap?
> Dumb q: why cant we use regmap_spi? worst case, you should be able to
read regmap-spi and you'll see why it can't be used in this case.
regmap-spi is for SPI clients who want to read their register map
through SPI commands. This is a driver for the SPI master which has its
registers memory mapped.
> > +static int dra7xxx_qspi_start_transfer_one(struct spi_master *master,
> > + struct spi_message *m)
> > +{
> > + struct dra7xxx_qspi *qspi = spi_master_get_devdata(master);
> > + struct spi_device *spi = m->spi;
> > + struct spi_transfer *t;
> > + int status = 0;
> > + int frame_length;
> > +
> > + /* setup device control reg */
> > + qspi->dc = 0;
> > +
> > + if (spi->mode & SPI_CPHA)
> > + qspi->dc |= QSPI_CKPHA(spi->chip_select);
> > + if (spi->mode & SPI_CPOL)
> > + qspi->dc |= QSPI_CKPOL(spi->chip_select);
> > + if (spi->mode & SPI_CS_HIGH)
> > + qspi->dc |= QSPI_CSPOL(spi->chip_select);
> > +
> > + frame_length = DIV_ROUND_UP(m->frame_length * spi->bits_per_word,
> > + spi->bits_per_word);
> > +
> > + frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
> > +
> > + /* setup command reg */
> > + qspi->cmd = 0;
> > + qspi->cmd |= QSPI_EN_CS(spi->chip_select);
> > + qspi->cmd |= QSPI_FLEN(frame_length);
> > +
> how does one ensure pm runtime has not disabled clocks in
> background? e.g. long latency between transfers.
because pm_runtime_put*() has not been called ?
There's no way clocks will be gated until we kick the pm autosuspend
timer, which is only done when the transfer is finished.
> > +static irqreturn_t dra7xxx_qspi_isr(int irq, void *dev_id)
> > +{
> > + struct dra7xxx_qspi *qspi = dev_id;
> > + u16 mask, stat;
> > +
> > + irqreturn_t ret = IRQ_HANDLED;
> > +
> > + spin_lock(&qspi->lock);
> > +
> what if autosuspend has triggered here? before ISR was scheduled?
how ?
the pm timer hasn't been kicked yet
> > + stat = dra7xxx_readl(qspi, QSPI_SPI_STATUS_REG);
> > + mask = dra7xxx_readl(qspi, QSPI_SPI_CMD_REG);
> > +
> > + if ((stat & QSPI_WC) && (mask & QSPI_WC_CMD_INT_EN))
> > + ret = IRQ_WAKE_THREAD;
> > +
> > + spin_unlock(&qspi->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static irqreturn_t dra7xxx_qspi_threaded_isr(int this_irq, void *dev_id)
> > +{
> > + struct dra7xxx_qspi *qspi = dev_id;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&qspi->lock, flags);
> > +
> what if autosuspend has triggered here? before ISR was scheduled?
how ?
pm timer hasn't been kicked yet
> > + qspi->base = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(qspi->base)) {
> > + ret = -ENOMEM;
> > + goto free_master;
> > + }
> why not use devm_request_and_ioremap? Lock that region down so that no
> two drivers can manage the same region?
read devm_ioremap_resource() and look at the git log for all the
numerous drivers which were converted to devm_ioremap_resource() to find
the reason.
> > +static struct platform_driver dra7xxx_qspi_driver = {
> > + .probe = dra7xxx_qspi_probe,
> > + .remove = dra7xxx_qspi_remove,
> > + .driver = {
> > + .name = "ti,dra7xxx-qspi",
> > + .owner = THIS_MODULE,
> > + .of_match_table = dra7xxx_qspi_match,
> > + }
> no need for pm_ops?
+1
> > +};
> > +
> > +module_platform_driver(dra7xxx_qspi_driver);
> > +
> > +MODULE_LICENSE("GPL");
> GPL V2?
+1
> > +MODULE_DESCRIPTION("TI QSPI controller driver");
> MODULE_AUTHOR?
+1
--
balbi
signature.asc
Description: Digital signature
