lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 7 Oct 2006 13:01:56 +0200 From: "Andrea Paterniani" <a.paterniani@...pp-eng.it> To: "David Brownell" <david-b@...bell.net> Cc: "Andrew Morton" <akpm@...l.org>, "Linux Kernel list" <linux-kernel@...r.kernel.org> Subject: RE: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver > > > ... > > > 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. What you're saying is clear. But I'm a little bit confused...what about the lot of definitions that use __REG or __REG2 macros to define registers address (inside imx-regs.h, pxa-regs.h and so on) ? > > > 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. How should I choose the value of that costant ? Please suggest me. - Andrea -----Messaggio originale----- Da: David Brownell [mailto:david-b@...bell.net] Inviato: sabato 7 ottobre 2006 1.35 A: Andrea Paterniani Cc: Andrew Morton; Linux Kernel list Oggetto: Re: [patch 2.6.18-git] SPI -- Freescale iMX SPI controller driver 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@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists