Stephen Street <[email protected]> wrote:
>
> Attached is an updated patch to add SPI master controller for PXA2xx
> boards. This update includes fixes for the PXA27x CPU to correctly
> handle the differences peripheral clock speeds with in the PXA2xx
> family.
>
Driver looks pretty clean. It's refreshingly deviod of comments ;)
Random minor observations:
> +static inline void flush(struct driver_data *drv_data)
> +{
> + u32 sssr = drv_data->sssr;
> + u32 ssdr = drv_data->ssdr;
> +
> + do {
> + while (SSP_REG(sssr) & SSSR_RNE) {
> + (void)SSP_REG(ssdr);
> + }
> + } while (SSP_REG(sssr) & SSSR_BSY);
> + SSP_REG(sssr) = SSSR_ROR ;
> +}
Suggest this be uninlined.
> +static inline void save_state(struct driver_data *drv_data)
> +static inline void restore_state(struct driver_data *drv_data)
> +static inline void dump_dma_state(struct driver_data *drv_data)
> +static inline void dump_ssp_state(struct driver_data *drv_data)
> +static inline void dump_message(char *header, struct spi_message *msg)
> +static inline void dump_transfer_state(char* header, struct driver_data *drv_data)
> +static inline void dump_chip_state(struct device *dev,
> + char * header,
> + struct chip_data *chip)
These appear to not have any callers?
Suggest they be uninlined, but that'd generate defined-but-not-used warnings.
> +static void null_writer(struct driver_data *drv_data)
> +{
> + u32 sssr = drv_data->sssr;
> + u32 ssdr = drv_data->ssdr;
> + u8 n_bytes = drv_data->cur_chip->n_bytes;
> +
> + while ((SSP_REG(sssr) & SSSR_TNF)
> + && (drv_data->tx < drv_data->tx_end)) {
> + SSP_REG(ssdr) = 0;
> + drv_data->tx += n_bytes;
> + }
> +}
hm.
#define SSP_REG(x) (*((volatile unsigned long *)x))
what's this doing? Accessing a device register? Cannot we use readl/writel?
> +static inline void* next_transfer(struct driver_data *drv_data)
^^ swap these chars!
> +{
> + struct spi_message *msg = drv_data->cur_msg;
> + struct spi_transfer *trans = drv_data->cur_transfer;
> +
> + /* Move to next transfer */
> + if (trans->transfer_list.next != &msg->transfers) {
> + drv_data->cur_transfer =
> + list_entry(trans->transfer_list.next,
> + struct spi_transfer,
> + transfer_list);
> + return RUNNING_STATE;
> + } else
> + return DONE_STATE;
> +}
Suggest this be uninlined.
> + *(u32 *)(drv_data->null_dma_buf) = 0;
null_dma_buf gets typecast a lot. Maybe make it a u32*?
> +static void giveback(struct spi_message *message, struct driver_data *drv_data)
> +{
> + struct spi_transfer* last_transfer;
> +
> + last_transfer = list_entry(message->transfers.prev,
> + struct spi_transfer,
> + transfer_list);
> +
> + if (!last_transfer->cs_change)
> + drv_data->cs_control(PXA2XX_CS_DEASSERT);
> +
> + message->state = NULL;
> + if (message->complete) {
> + message->complete(message->context);
> + }
Unneeded braces.
> +static void dma_handler(int channel, void *data, struct pt_regs *regs)
> +{
> + struct driver_data *drv_data = (struct driver_data *)data;
Unneeded typecast.
> + /* PXA255x_SSP has no timeout interrupt, wait for tailing bytes */
> + if ((drv_data->ssp_type == PXA25x_SSP)
> + && (channel == drv_data->tx_channel)
> + && (irq_status & DCSR_ENDINTR)) {
> +
> + /* Wait for rx to stall */
> + while (SSP_REG(sssr) & SSSR_BSY)
> + cpu_relax();
A timeout here, perhaps?
> + while (!(DCSR(drv_data->rx_channel) & DCSR_STOPSTATE))
> + cpu_relax();
And here.
> +static irqreturn_t dma_transfer(struct driver_data *drv_data)
> +{
> + u32 sssr = drv_data->sssr;
> + u32 sscr1 = drv_data->sscr1;
> + u32 ssto = drv_data->ssto;
> + u32 irq_status = SSP_REG(sssr) & drv_data->mask_sr;
> + u32 trailing_sssr = 0;
> + struct spi_message *msg = drv_data->cur_msg;
> +
> + if (irq_status & SSSR_ROR) {
> + /* Clear and disable interrupts on SSP and DMA channels*/
> + SSP_REG(ssto) = 0;
> + SSP_REG(sssr) = drv_data->clear_sr;
> + SSP_REG(sscr1) &= ~(drv_data->dma_cr1);
> + DCSR(drv_data->tx_channel) = RESET_DMA_CHANNEL;
> + DCSR(drv_data->rx_channel) = RESET_DMA_CHANNEL;
> + unmap_dma_buffers(drv_data);
> + flush(drv_data);
> +
> + dev_warn(&drv_data->pdev->dev, "dma_transfer: fifo overun\n");
> +
> + drv_data->cur_msg->state = ERROR_STATE;
> + tasklet_schedule(&drv_data->pump_transfers);
> +
> + return IRQ_HANDLED;
> + }
> +
> + /* Check for false positive timeout */
> + if ((irq_status & SSSR_TINT) && DCSR(drv_data->tx_channel) & DCSR_RUN) {
> + SSP_REG(sssr) = SSSR_TINT;
> + return IRQ_HANDLED;
> + }
> +
> + if (irq_status & SSSR_TINT || drv_data->rx == drv_data->rx_end) {
> +
> + /* Clear and disable interrupts on SSP and DMA channels*/
> + SSP_REG(ssto) = 0;
> + SSP_REG(sssr) = drv_data->clear_sr;
> + SSP_REG(sscr1) &= ~(drv_data->dma_cr1);
> + DCSR(drv_data->tx_channel) = RESET_DMA_CHANNEL;
> + DCSR(drv_data->rx_channel) = RESET_DMA_CHANNEL;
> + while (!(DCSR(drv_data->rx_channel) & DCSR_STOPSTATE)
> + || (SSP_REG(sssr) & SSSR_BSY))
> + cpu_relax();
And here.
> + unmap_dma_buffers(drv_data);
> +
> + /* Calculate number of trailing bytes, read them */
> + trailing_sssr = SSP_REG(sssr);
> + if ((trailing_sssr & 0xf008) != 0xf000) {
> + drv_data->rx = drv_data->rx_end -
> + (((trailing_sssr >> 12) & 0x0f) + 1);
> + drv_data->read(drv_data);
> + }
> + msg->actual_length += drv_data->len;
> +
> + /* Release chip select if requested, transfer delays are
> + * handled in pump_transfers */
> + if (drv_data->cs_change)
> + drv_data->cs_control(PXA2XX_CS_DEASSERT);
> +
> + /* Move to next transfer */
> + msg->state = next_transfer(drv_data);
> +
> + /* Schedule transfer tasklet */
> + tasklet_schedule(&drv_data->pump_transfers);
> +
> + return IRQ_HANDLED;
> + }
> +
> + /* Never Fail */
WARN_ON(1)?
Why not return IRQ_NONE here? That way, the IRQ system will save the
machine if the IRQ gets stuck.
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
> +{
> ...
> + return IRQ_HANDLED;
> +}
Ditto.
> +static void pump_transfers(unsigned long data)
> +{
> ...
> + /* Enable dma end irqs on SSP to detect end of transfer */
> + if (drv_data->ssp_type == PXA25x_SSP) {
> + DCMD(drv_data->tx_channel) |= DCMD_ENDIRQEN;
> + }
Braces.
> +static int setup(struct spi_device *spi)
> +{
> + struct pxa2xx_spi_chip *chip_info = NULL;
> + struct chip_data *chip;
> + struct driver_data *drv_data = spi_master_get_devdata(spi->master);
> + unsigned int clk_div;
> +
> + if (!spi->bits_per_word)
> + spi->bits_per_word = 8;
> +
> + if (drv_data->ssp_type != PXA25x_SSP
> + && (spi->bits_per_word < 4 || spi->bits_per_word > 32))
> + return -EINVAL;
> + else if (spi->bits_per_word < 4 || spi->bits_per_word > 16)
> + return -EINVAL;
> +
> + /* Only alloc (or use chip_info) on first setup */
> + chip = spi_get_ctldata(spi);
> + if (chip == NULL) {
> + chip = kzalloc(sizeof(struct chip_data), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->cs_control = null_cs_control;
> + chip->enable_dma = 0;
> + chip->timeout = 5;
> + chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
> + chip->dma_burst_size = drv_data->master_info->enable_dma ?
> + DCMD_BURST8 : 0;
> +
> + chip_info = (struct pxa2xx_spi_chip *)spi->controller_data;
Unneeded cast.
> + switch (drv_data->sscr0) {
> + case SSP1_VIRT:
> + clk_div = SSP1_SerClkDiv(spi->max_speed_hz);
> + break;
> + case SSP2_VIRT:
> + clk_div = SSP2_SerClkDiv(spi->max_speed_hz);
> + break;
> + case SSP3_VIRT:
> + clk_div = SSP3_SerClkDiv(spi->max_speed_hz);
> + break;
> + default:
> + return -ENODEV;
> + }
We normally lay out switch statements one tabstop less than this.
> +static void cleanup(const struct spi_device *spi)
> +{
> + struct chip_data *chip = spi_get_ctldata((struct spi_device *)spi);
Remove the typecast, change spi_get_ctldata() to take a const struct
spi_device *? I guess that might cause warnings too - the compiler might
want spi_get_ctldata() to return a const thing.
Might be simpler to not have a const arg here.
> + if (chip)
> + kfree(chip);
> +}
kfree(NULL) is legal.
> +
> + queue_work(drv_data->workqueue, &drv_data->pump_messages);
I see a queue_work(), but I see no flush_workqueue(). Basically a flush is
always needed to push through any pending work in the shutdown/close/rmmod
paths.
> +static int destroy_queue(struct driver_data *drv_data)
> +{
> + int status;
> +
> + status = stop_queue(drv_data);
> + if (status != 0)
> + return status;
> +
> + destroy_workqueue(drv_data->workqueue);
hm, OK, destroy_workqueue() does flush_workqueue.
> +static int pxa2xx_spi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pxa2xx_spi_master *platform_info;
> + struct spi_master *master;
> + struct driver_data *drv_data = 0;
> + struct resource *memory_resource;
> + int irq;
> + int status = 0;
> +
> + platform_info = (struct pxa2xx_spi_master *)dev->platform_data;
Unneeded cast.
> + drv_data->null_dma_buf = drv_data + sizeof(struct driver_data);
> + drv_data->null_dma_buf = (void *)(((u32)(drv_data->null_dma_buf)
> + & 0xfffffff8) | 8);
Consider using the ALIGN() macro here.
This all looks very non-64-bit-capable.
> +out_error_master_alloc:
> + (void)spi_master_put(master);
Remove the (void). Unless it does something??
> +static void pxa2xx_spi_shutdown(struct platform_device *pdev)
> +{
> + int status = 0;
> +
> + if ((status = pxa2xx_spi_remove(pdev)) != 0) {
> + dev_err(&pdev->dev, "shutdown failed with %d\n", status);
> + }
Braces.
> +#ifdef CONFIG_PM
> +static int stall_queue(struct driver_data *drv_data)
> +{
> + unsigned long flags;
> + unsigned limit = 500;
> +
> + spin_lock_irqsave(&drv_data->lock, flags);
> +
> + drv_data->run = QUEUE_STALLED;
> +
> + while (drv_data->busy && limit--) {
> + spin_unlock_irqrestore(&drv_data->lock, flags);
> + msleep(10);
> + spin_lock_irqsave(&drv_data->lock, flags);
> + }
That looks a bit lame. What's happening here?
> + spin_unlock_irqrestore(&drv_data->lock, flags);
> +
> + if (!list_empty(&drv_data->queue) || drv_data->busy)
> + return -EBUSY;
Does the list_empty() make sense outside the lock?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
