MEMSTICK: Add driver for Ricoh R5C592 Card reader.
in [Kernel]
|
Prev: MEMSTICK: fix hangs on unexpected device removal in mspro_blk
Next: media: Add a cached version of the contiguous video buffers
From: Alex Dubov on 4 Aug 2010 04:00 I see two immediate problems with this patch: 1. On cosmetic level, custom debug macros should not be employed. Device core already have this functionality (dynamic debug levels and such). Please, use dev_dbg and friends for print-outs. 2. On a structural level, I'd rather prefer host drivers to not start their own threads. If you look at both current host implementations, they operate in callback fashion. Apart from saving some resources, this reduces the amount of problems encountered during suspend/resume and shutdown. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Maxim Levitsky on 4 Aug 2010 12:50 On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: > I see two immediate problems with this patch: > > 1. On cosmetic level, custom debug macros should not be employed. Device > core already have this functionality (dynamic debug levels and such). Please, > use dev_dbg and friends for print-outs. This allows much easier control for debug. Single module parameter is enough to adjust it. This helps me help users. (Eg, kernel compilation is out of question) > > 2. On a structural level, I'd rather prefer host drivers to not start their > own threads. If you look at both current host implementations, they operate > in callback fashion. Apart from saving some resources, this reduces the > amount of problems encountered during suspend/resume and shutdown. This isn't possible. Hardware doesn't support interrupts on memstick bus changes, it only supports DMA done from/to internal FIFO, and DMA it only possible for 512 byte TPCs. Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Maxim Levitsky on 4 Aug 2010 15:40 On Wed, 2010-08-04 at 19:48 +0300, Maxim Levitsky wrote: > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: > > I see two immediate problems with this patch: > > > > 1. On cosmetic level, custom debug macros should not be employed. Device > > core already have this functionality (dynamic debug levels and such). Please, > > use dev_dbg and friends for print-outs. > This allows much easier control for debug. > Single module parameter is enough to adjust it. > This helps me help users. > (Eg, kernel compilation is out of question) > > > > > > 2. On a structural level, I'd rather prefer host drivers to not start their > > own threads. If you look at both current host implementations, they operate > > in callback fashion. Apart from saving some resources, this reduces the > > amount of problems encountered during suspend/resume and shutdown. > This isn't possible. > Hardware doesn't support interrupts on memstick bus changes, it only > supports DMA done from/to internal FIFO, and DMA it only possible for > 512 byte TPCs. > Another question. I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT Instread mspro_blk.c enables this capability for parallel mode, assuming that hw supports it. Its true in my case, but might not be true in other cases. I think I should fix that, right? Also I see that you bath TPC_READ_LONG_DATA/TPC_READ_LONG_DATA Does that mean that every HW sector is larger that 512? If so, you are doing copy on write, right? I have small caching in my sm_ftl of last sector. It helps performance a lot. Also I want to clarify that the only kind of interrupts supported by hw (besides usual card detection interrupt), is DMA done interrupt. Thats why I have to use thread. Doing polling in r592_submit_req (which runs in atomic context is just cruel). Besides, under moderate IO load, the IO thread doesn't sleep, thus there is no overhead of wake/sleep. Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alex Dubov on 5 Aug 2010 04:40 > Maxim Levitsky wrote: > > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: > > > I see two immediate problems with this patch: > > > > > > 1. On cosmetic level, custom debug macros should > not be employed. Device > > > core already have this functionality (dynamic > debug levels and such). Please, > > > use dev_dbg and friends for print-outs. > > This allows much easier control for debug. > > Single module parameter is enough to adjust it. > > This helps me help users. > > (Eg, kernel compilation is out of question) I doubt it will be that useful, but it's not my call to make anyway. > > > > > > > > > > 2. On a structural level, I'd rather prefer host > drivers to not start their > > > own threads. If you look at both current host > implementations, they operate > > > in callback fashion. Apart from saving some > resources, this reduces the > > > amount of problems encountered during > suspend/resume and shutdown. > > This isn't possible. > > Hardware doesn't support interrupts on memstick bus > changes, it only > > supports DMA done from/to internal FIFO, and DMA it > only possible for > > 512 byte TPCs. > > > How depressing. > > Another question. > > I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT > Instread mspro_blk.c enables this capability for parallel > mode, assuming > that hw supports it. Its true in my case, but might not be > true in other > cases. > I think I should fix that, right? This is mandated by the spec. INT should be available automatically in parallel mode, and some hardware does it in serial as well. > > Also I see that you bath > TPC_READ_LONG_DATA/TPC_READ_LONG_DATA > Does that mean that every HW sector is larger that 512? > If so, you are doing copy on write, right? > I have small caching in my sm_ftl of last sector. It helps > performance a > lot. That's how its called in the spec. Sectors can be larger than 512b on Pro-HG sticks, and there's additional TPC_READ/WRITE_QUAD_DATA which operates on larger quantities. > > > Also I want to clarify that the only kind of interrupts > supported by hw > (besides usual card detection interrupt), is DMA done > interrupt. > Thats why I have to use thread. > Doing polling in r592_submit_req (which runs in atomic > context is just > cruel). Yes, I see you have a timed wait there. > Besides, under moderate IO load, the IO thread doesn't > sleep, thus there > is no overhead of wake/sleep. > > > Best regards, > Maxim Levitsky > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Maxim Levitsky on 5 Aug 2010 07:30
On Thu, 2010-08-05 at 01:30 -0700, Alex Dubov wrote: > > Maxim Levitsky wrote: > > > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: > > > > I see two immediate problems with this patch: > > > > > > > > 1. On cosmetic level, custom debug macros should > > not be employed. Device > > > > core already have this functionality (dynamic > > debug levels and such). Please, > > > > use dev_dbg and friends for print-outs. > > > This allows much easier control for debug. > > > Single module parameter is enough to adjust it. > > > This helps me help users. > > > (Eg, kernel compilation is out of question) > > I doubt it will be that useful, but it's not my call to make anyway. > > > > > > > > > > > > > > > > 2. On a structural level, I'd rather prefer host > > drivers to not start their > > > > own threads. If you look at both current host > > implementations, they operate > > > > in callback fashion. Apart from saving some > > resources, this reduces the > > > > amount of problems encountered during > > suspend/resume and shutdown. > > > This isn't possible. > > > Hardware doesn't support interrupts on memstick bus > > changes, it only > > > supports DMA done from/to internal FIFO, and DMA it > > only possible for > > > 512 byte TPCs. > > > > > > > How depressing. > > > > > Another question. > > > > I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT > > Instread mspro_blk.c enables this capability for parallel > > mode, assuming > > that hw supports it. Its true in my case, but might not be > > true in other > > cases. > > I think I should fix that, right? > > This is mandated by the spec. INT should be available automatically in > parallel mode, and some hardware does it in serial as well. > > > > > Also I see that you bath > > TPC_READ_LONG_DATA/TPC_READ_LONG_DATA > > Does that mean that every HW sector is larger that 512? > > If so, you are doing copy on write, right? > > I have small caching in my sm_ftl of last sector. It helps > > performance a > > lot. > > > That's how its called in the spec. > Sectors can be larger than 512b on Pro-HG sticks, and there's additional > TPC_READ/WRITE_QUAD_DATA which operates on larger quantities. But not on ordinary PRO, right? Small question, can I use Pro-HG stick in my reader that is designed for Standard/PRO only? Does your subsystem support it? 8-bit mode really isn't supported here, but it is optional I am sure. > > > > > > > Also I want to clarify that the only kind of interrupts > > supported by hw > > (besides usual card detection interrupt), is DMA done > > interrupt. > > Thats why I have to use thread. > > Doing polling in r592_submit_req (which runs in atomic > > context is just > > cruel). > > Yes, I see you have a timed wait there. > Alex, how should I proceed in merge of my driver? Do you have any objections against it now? Best regards, Maxim Levitsky -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |