Re: [spi-devel-general] [PATCH RESEND] max3100 driver

[David Brownell <david-b@pacbell.net>]

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.