Messages in this thread | | | From | David Brownell <> | Subject | Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver | Date | Fri, 6 Oct 2006 16:35:23 -0700 |
| |
On Tuesday 03 October 2006 9:08 am, Andrea Paterniani wrote: > Here some questions and answers to your comments, (please consider I'm nearly new to kernel programming). > > > > > ... > > ug. Why not simply open-code > > > > readl(addr + DATA); > > I found usefull to define macros to use inside code something like > rd_CONTROL(regs) > instead of > readl(regs + 0x08) > since to me the macro sounds more friendly. > Should I have to adhere to some standard ?
The standards are more or less to avoid creating namespace clutter, and to make explicit where register access happens. Defining new macros violates the former; not being able to tell where the chip registers are accessed (because they're wrapped in macros) violates the latter.
> > The use of loops_per_jiffy seems inappropriate. That's an IO-space read in > > there, which is slow. This timeout will be very long indeed. > > Please suggest me what it's more appropriate.
Pick a constant, use it.
> > I see tasklets being scheduled, but no tasklet_disable() or tasklet_kill(), > > etc. Is this driver racy against shutdown or rmmod? > > Do you mean I should use tasklet_kill() inside spi_imx_remove ?
That's how I read it. :)
> > > + drv_data->rd_data_phys = (dma_addr_t)res->start; > > > > I don't think it's correct to cast a kernel virtual address straight to a > > dma_addr_t. > > File include/asm-arm/types.h defines > typedef u32 dma_addr_t; > Also I think that for ARM architecture resource_size_t in practice > is u32 since CONFIG_RESOURCES_64BIT isn't defined. > Is this construction correct ? If not what should I do ?
I think it's correct; it's certainly standard for converting physical addresses to DMA addresses. (Andrew got that one wrong; resource addresses are physical, not virtual.)
- Dave
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |