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 5 Aug 2010 07:50 > > > > 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? Pro sectors are normally 512 bytes. > > Small question, can I use Pro-HG stick in my reader that is > designed for > Standard/PRO only? Does your subsystem support it? It should work. It works for me on TI, which has no 8b mode either. > > > > Alex, how should I proceed in merge of my driver? > Do you have any objections against it now? > I may have done a thing or two differently, but otherwise no objection. I suppose, you should ask Andrew Morton to put it in. -- 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 08:40 On Thu, 2010-08-05 at 04:48 -0700, Alex Dubov wrote: > > > > > > 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? > > Pro sectors are normally 512 bytes. > > > > > Small question, can I use Pro-HG stick in my reader that is > > designed for > > Standard/PRO only? Does your subsystem support it? > > It should work. It works for me on TI, which has no 8b mode either. In that case, is there an upper limit on sector size? > > > > > > > > Alex, how should I proceed in merge of my driver? > > Do you have any objections against it now? > > > > I may have done a thing or two differently, but otherwise no objection. > I suppose, you should ask Andrew Morton to put it in. Ok, will do. 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 08:50 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. Thinking again about that I agree with you. Then I can remove 2 more lines from my driver. Btw, I want to add a callback from driver to card driver to be able to reset card in case of error (windows driver does that in case of any error) I would use most of the mspro_block_resume for the implementation for mspro. Any objections, suggestions? > > > > > 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 > > 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 13:50 On Thu, 2010-08-05 at 15:30 +0300, Maxim Levitsky wrote: > On Thu, 2010-08-05 at 04:48 -0700, Alex Dubov wrote: > > > > > > > > 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? > > > > Pro sectors are normally 512 bytes. > > > > > > > > Small question, can I use Pro-HG stick in my reader that is > > > designed for > > > Standard/PRO only? Does your subsystem support it? > > > > It should work. It works for me on TI, which has no 8b mode either. > In that case, is there an upper limit on sector size? Also, you don't use the MS_TPC_READ_QUAD_DATA at all. So mspro_blk.c won't work with >512 bytes/sector disk? Or there is some compatibility? 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: Joe Perches on 5 Aug 2010 21:20
On Thu, 2010-08-05 at 17:25 +0300, Maxim Levitsky wrote: > Signed-off-by: Maxim Levitsky <maximlevitsky(a)gmail.com> Trivial comments: > --- > MAINTAINERS | 6 + > +RICOH R5C592 MEMORYSTICK DRIVER > +M: Maxim Levitsky <maximlevitsky(a)gmail.com> > +S: Maintained > +F: drivers/memstick/host/r592.c > +F: drivers/memstick/host/r592.h Patterns in maintainers can use wildcards F: drivers/memstick/host/r592* > --- /dev/null > +++ b/drivers/memstick/host/r592.c > @@ -0,0 +1,889 @@ [] > +static char *tpc_names[] = { const ? > + "MS_TPC_READ_MG_STATUS", > + "MS_TPC_READ_LONG_DATA", [] > +#define dbg(format, ...) \ > + if (debug) \ > + printk(KERN_DEBUG DRV_NAME ": " format "\n", ## __VA_ARGS__) > + > +#define dbg_verbose(format, ...) \ > + if (debug > 1) \ > + printk(KERN_DEBUG DRV_NAME ": " format "\n", ## __VA_ARGS__) > + > +#define dbg_reg(format, ...) \ > + if (debug > 2) \ > + printk(KERN_DEBUG DRV_NAME ": " format "\n", ## __VA_ARGS__) > + These style macros should use do { if (test) print(); } while (9) so they could be used without problems in if/else blocks. Maybe it'd be better to have and use 1 macro with a level passed: #define dbg(level, format, ...) \ do { \ if (debug > level) \ printk(KERN_DEBUG pr_fmt(format), ##__VA_ARGS__); \ } while (0) > +#define message(format, ...) \ > + printk(KERN_INFO DRV_NAME ": " format "\n", ## __VA_ARGS__) Why not just use pr_info? -- 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/ |