Messages in this thread | | | From | David Brownell <> | Subject | Re: [spi-devel-general] [PATCH RESEND] max3100 driver | Date | Wed, 24 Sep 2008 21:56:29 -0700 |
| |
You got this working ... congrats! :)
Small suggestion: next time you resend this, make sure that $SUBJECT mentions it's a UART driver. Maybe even that it's a SPI UART driver. That should help get more comments.
On Saturday 20 September 2008, Christian Pellegrin wrote: > +struct plat_max3100 { > +/* force MAX3100 in loopback */ > + int loopback; > +/* 0 for 3.6864 Mhz, 1 for 1.8432 */ > + int crystal; > +/* for archs like PXA with only edge irqs */ > + int only_edge_irq; > +/* MAX3100 has a shutdown pin. This is a hook > + called on suspend and resume to activate it.*/ > + void (*max3100_hw_suspend) (int suspend); > +/* poll time for ctr signals in ms, 0 disables (so no hw flow > + * ctrl is possible) */ > + int poll_time; > +};
This is a bit picky, but it's the first thing I noticed when scanning the patch ... wierd comment layout! Either indent those all, or (better) convert to kerneldoc style.
Potentially less picky: probe() doesn't lock max3100s[], neither does remove(), and in fact there seems to be no lock for that table. Which suggests trouble in cases like concurrent I/O (including open) and driver remove(). You should probably just allocate a mutex to help control that table, like most other drivers.
And is that workqueue single threaded?
I just skimmed the rest of the driver. Seems to be fairly straightforward -- at least the non-TTY bits. -- 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/
| |