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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
