max3100 driver
PATCH 1/1 max3100 driver
This patch adds support for the MAX3100 SPI UART.
Generated on 20080920 against v2.6.27-rc6
--- drivers/serial/Kconfig | 6 +- drivers/serial/Makefile | 1 + drivers/serial/max3100.c | 976 ++++++++++++++++++++++++++++++++++++++++ include/linux/serial_core.h | 3 + include/linux/serial_max3100.h | 31 ++ 5 files changed, 1016 insertions(+), 1 deletions(-)
drivers/serial/Kconfig +5 -1
@@ -509,7 +509,11 @@ config SERIAL_S3C2440 help Serial port support for the Samsung S3C2440 and S3C2442 SoC - +config SERIAL_MAX3100 + tristate "MAX3100 support" + depends on SPI + help + MAX3100 chip support config SERIAL_DZ bool "DECstation DZ serial driver"
drivers/serial/Makefile +1 -0
@@ -31,6 +31,7 @@ obj-$(CONFIG_SERIAL_S3C2400) += s3c2400.o obj-$(CONFIG_SERIAL_S3C2410) += s3c2410.o obj-$(CONFIG_SERIAL_S3C2412) += s3c2412.o obj-$(CONFIG_SERIAL_S3C2440) += s3c2440.o +obj-$(CONFIG_SERIAL_MAX3100) += max3100.o obj-$(CONFIG_SERIAL_SUNCORE) += suncore.o obj-$(CONFIG_SERIAL_SUNHV) += sunhv.o obj-$(CONFIG_SERIAL_SUNZILOG) += sunzilog.o
drivers/serial/max3100.c new file +976 -0
976 lines not on this page · raw message
include/linux/serial_core.h +3 -0
@@ -155,6 +155,9 @@#define PORT_SC26XX 82 +/* MAX3100 */ +#define PORT_MAX3100 83 +#ifdef __KERNEL__#include <linux/compiler.h>
include/linux/serial_max3100.h new file +31 -0
@@ -0,0 +1,31 @@ + +/* +* +* Copyright (C) 2007 Christian Pellegrin +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +*/ + + +#ifndef _LINUX_SERIAL_MAX3100_H +#define _LINUX_SERIAL_MAX3100_H 1 + +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; +}; + +#endif
--
1.4.4.4
Review
Quoted 10 lines · On Sat, 20 Sep 2008 09:20:08 +0200 Christian Pellegrin <[email protected]>…
This patch adds support for the MAX3100 SPI UART.
...
+ * The initial minor number is 128 in the low-density serial port:
+ * mknod/dev/ttyMAX0c 204 128
+ */(quoted diff) +2
-0+ +#define MAX3100_MAJOR 204
Allocating a new major is a Big Deal. It involves getting the major registered by contacting [email protected].
It's better to dynamically allocate it - let udev handle it.
Quoted 24 lines
drivers/serial/max3100.c new file +24
-0+#define MAX3100_MINOR 128 +/* 4 MAX3100s should be enough for everyone */ +#define MAX_MAX3100 4 + +#include <linux/bitops.h> +#include <linux/console.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/fcntl.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/keyboard.h> +#include <linux/major.h> +#include <linux/mm.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/reboot.h> +#include <linux/sched.h> +#include <linux/serial_core.h>
drivers/serial/max3100.c line 73
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+#include <linux/serial.h> +#include <linux/signal.h> +#include <linux/spi/spi.h> +#include <linux/string.h> +#include <linux/timer.h> +#include <linux/tty.h> +#include <linux/tty_flip.h> +#include <linux/uaccess.h> +#include <linux/workqueue.h> +#include <linux/freezer.h>
Gad. Are all those includes needed?
drivers/serial/max3100.c line 120
Quoted 13 lines
drivers/serial/max3100.c new file +10
-0+#include <asm/system.h> + +#include <linux/serial_max3100.h> + +/* the following simulate a status reg for ignore_status_mask */ +#define MAX3100_STATUS_PE 1 +#define MAX3100_STATUS_FE 2 +#define MAX3100_STATUS_OE 4 + +struct max3100_port_s {
`struct max3100_port' is sufficient, and would be more typical.
drivers/serial/max3100.c line 125
Quoted 5 lines
drivers/serial/max3100.c new file +5
-0+struct uart_port port; +struct spi_device *spi; + + int cts:1; /* last CTS received for flow ctrl */ + int tx_empty:1; /* last TX empty bit */
These two bits will share a word and hence locking is needed to prevent modifications to one from trashing modifications to the other on SMP.
That's OK, but it would be best to document that locking right here, and to check that it is adhered to.
Quoted 9 lines
drivers/serial/max3100.c new file +9
-0+spinlock_t conf_lock;/* shared data */ + int conf_commit:1; /* need to make changes */ +int conf;/* configuration for the MAX31000 +* (bits 0-7, bits 8-11 are irqs) */ + int rts_commit:1; /* need to change rts */ + int rts:1; /* rts status */ + + int parity:3; /* keeps track if we should send parity */ +#define MAX3100_PARITY_ON 1
drivers/serial/max3100.c line 145
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+#define MAX3100_PARITY_ODD 2 +#define MAX3100_7BIT 4 + int rx_enabled:1; /* if we should rx chars */ + +int irq;/* irq assigned to the max3100 */ + +int minor;/* minor number */ + int crystal:1; /* 1 if 3.6864Mhz crystal 0 for 1.8432 */ + int loopback:1; /* 1 if we are in loopback mode */ + int only_edge_irq:1; /* 1 if we have only edge irqs (like PXA) */
Lots of dittoes.
These bitfields perhaps could be reordered to save a bit of space, but that depends on the implicit locking rules for them.
Quoted 9 lines
drivers/serial/max3100.c new file +9
-0+/* for handling irqs: need workqueue since we do spi_sync */ +struct workqueue_struct *workqueue; +struct work_struct work; +/* set to 1 to make the workhandler exit as soon as possible */ + int force_end_work:1; +/* need to know we are suspending to avoid deadlock on workqueue */ + int suspending:1; + +/* these are for IRQ on/off counting */
drivers/serial/max3100.c line 165
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+int irq_status; +/* avoid race condition on irq_status */ +spinlock_t irq_lock; + +/* hook for suspending MAX3100 via dedicated pin */ +void (*max3100_hw_suspend) (int suspend); + +/* poll time (in ms) for ctrl lines */ +int poll_time; +/* and it's timer */
s/it's/its/
Quoted 27 lines
drivers/serial/max3100.c new file +27
-0+struct timer_list timer; +}; + +static struct max3100_port_s *max3100s[MAX_MAX3100 ];/* the chips */ + +static void irq_state (struct max3100_port_s *s,int on) +{ +unsigned long flags; + +spin_lock_irqsave (&s->irq_lock, flags); +if (s->irq_status != on) { +if (on) +enable_irq (s->irq); +else +disable_irq (s->irq); + s->irq_status = on; + } +spin_unlock_irqrestore (&s->irq_lock, flags); +} + +static int max3100_do_parity (struct max3100_port_s *s,u16 c) +{ +int parity; +int i, n; + +if (s->parity &MAX3100_PARITY_ODD ) + parity =0 ;
drivers/serial/max3100.c line 202
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+else + parity =1 ; + +if (s->parity &MAX3100_7BIT ) + n =7 ; +else + n =8 ; + +for (i =0 ; i < n; i++) + parity = parity ^ ((c>>i) &1 );
hrmpf. Don't we have a library function for that?
Perhaps you could use hweight8(c)&1.
drivers/serial/max3100.c line 209
Quoted 7 lines
drivers/serial/max3100.c new file +7
-0+return parity; +} + +static int max3100_check_parity (struct max3100_port_s *s,u16 c) +{ +return max3100_do_parity (s, c) == + ((c >> (s->parity &MAX3100_7BIT ?7 :8 )) &1 );
<checks his C precedence table>
OK...
drivers/serial/max3100.c line 214
Quoted 5 lines
drivers/serial/max3100.c new file +5
-0+} + +static void max3100_calc_parity (struct max3100_port_s *s,u16 *c) +{ + *c &= ~MAX3100_PT ;
s/ / /
drivers/serial/max3100.c line 216
Quoted 2 lines
drivers/serial/max3100.c new file +2
-0+if (s->parity &MAX3100_PARITY_ON ) + *c |=max3100_do_parity (s, *c)<<8 ;
Most people put spaces around <<, but checkpatch pets this through.
drivers/serial/max3100.c line 224
Quoted 8 lines
drivers/serial/max3100.c new file +8
-0+} + +static void max3100_work (struct work_struct *w); + +static void max3100_dowork (struct max3100_port_s *s) +{ +if (!s->force_end_work && !work_pending (&s->work)) { +PREPARE_WORK (&s->work, max3100_work);
This is a strange place to run PREPARE_WORK(). Normally it would be done during driver/device initialsiation, and I think that can be done here.
Quoted 17 lines
drivers/serial/max3100.c new file +17
-0+queue_work (s->workqueue, &s->work); + } +} + +static void max3100_timeout (unsigned long data) +{ +struct max3100_port_s *s = (struct max3100_port_s *)data; + +if (s->port.info) { +max3100_dowork (s); +mod_timer (&s->timer, jiffies + s->poll_time); + } +} + +static int max3100_sr (struct max3100_port_s *s,u16 tx,u16 *rx) +{ +struct spi_message message;
drivers/serial/max3100.c line 251
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+struct spi_transfer tran; +u16 etx, erx; +int status; + + etx =htons (tx); +spi_message_init (&message); +memset (&tran,0 ,sizeof (tran)); + tran.tx_buf = &etx; + tran.rx_buf = &erx; + tran.len =2 ;
could do
struct spi_transfer tran = {
.tx_buf = &etx,
.rx_buf = &erx
.len = 2,
};and the compiler will zero out the rest. Minor point.
Quoted 14 lines
drivers/serial/max3100.c new file +12
-0+spi_message_add_tail (&tran, &message); + status =spi_sync (s->spi, &message); +if (status) { +dev_warn (&s->spi->dev,"error while calling spi_sync\n" ); +return -EIO ; + } + *rx =ntohs (erx); + s->tx_empty = (*rx &MAX3100_T ) >0 ; +dev_dbg (&s->spi->dev,"%04x - %04x\n" , tx, *rx); +return 0 ; +} +
drivers/serial/max3100.c
Quoted 10 lines
drivers/serial/max3100.c new file +9
-0+static unsigned int max3100_get_mctrl (struct uart_port *port) +{ +struct max3100_port_s *s =container_of (port, + struct max3100_port_s, + port); + +dev_dbg (&s->spi->dev,"%s\n" , __func__); + +/* may not be truely up-to-date */
s/truely/truly/
Quoted 19 lines
drivers/serial/max3100.c new file +16
-0+max3100_dowork (s); +/* always assert DCD and DSR since these lines are not wired */ +return (s->cts ?TIOCM_CTS :0 ) |TIOCM_DSR |TIOCM_CAR ; +} + +static int max3100_startup (struct uart_port *port) +{ +struct max3100_port_s *s =container_of (port, + struct max3100_port_s, + port); +char b[10 ]; +int irq_type; + +dev_dbg (&s->spi->dev,"%s\n" , __func__); + + s->conf =MAX3100_RM ;
drivers/serial/max3100.c line 663
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+ s->rx_enabled =1 ; + +if (s->suspending) +return 0 ; + + s->force_end_work =0 ; + s->parity =0 ; + s->rts =0 ; + +sprintf (b,"max3100-%d" , s->minor);
hm. This will go nasty if s->minor > 9. I'd suggest that b[] be made larger.
Quoted 5 lines
drivers/serial/max3100.c new file +5
-0+ s->workqueue =create_freezeable_workqueue (b); +if (!s->workqueue) { +dev_warn (&s->spi->dev,"cannot create workqueue\n" ); +return -EBUSY ; + }
drivers/serial/max3100.c line 678
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+INIT_WORK (&s->work, max3100_work); + + s->irq_status =1 ;/* IRQS are on after request */ +if (s->only_edge_irq) + irq_type =IRQF_TRIGGER_FALLING ; +else + irq_type =IRQF_TRIGGER_LOW ; +if (request_irq (s->irq, max3100_irq, irq_type,"max3100" , s) <0 ) { +dev_warn (&s->spi->dev,"cannot allocate irq %d\n" , s->irq); + s->irq =0 ;
Shouldn't we tear down that workqueue before returning?
Quoted 25 lines
drivers/serial/max3100.c new file +25
-0+return -EBUSY ; + } + +if (s->loopback) { +u16 tx, rx; + tx =0x4001 ; +max3100_sr (s, tx, &rx); + } + +if (s->max3100_hw_suspend) + s->max3100_hw_suspend (0 ); + s->conf_commit =1 ; +max3100_dowork (s); +/* wait for clock to settle */ +msleep (50 ); + +max3100_enable_ms (&s->port); + +return 0 ; +} + +static const char *max3100_type (struct uart_port *port) +{ + struct max3100_port_s *s = container_of(port, + struct max3100_port_s,
drivers/serial/max3100.c line 713
Quoted 10 lines
drivers/serial/max3100.c new file +10
-0+ port); + +dev_dbg (&s->spi->dev,"%s\n" , __func__); + +return s->port.type ==PORT_MAX3100 ?"MAX3100" :NULL ; +} + +static void max3100_release_port (struct uart_port *port) +{ +struct max3100_port_s *s = (struct max3100_port_s *)port;
container_of()
drivers/serial/max3100.c
Quoted 6 lines
drivers/serial/max3100.c new file +3
-0+dev_dbg (&s->spi->dev,"%s\n" , __func__); +} +
Please cc Alan Cox for the tty things and perhaps [email protected] for the SPI bits.
Sorry, sent HTML mail by mistake, so resending :-/
Quoted 7 lines · Andrew Morton
Quoted 1 line
+#define
MAX3100_MAJOR204Allocating a new major is a Big Deal. It involves getting the major registered by contacting [email protected].
It's better to dynamically allocate it - let udev handle it.
I looked at other serial driver as an example and checked devices.txt:
if I don't get it wrong major 204 should be already reserved for serial port. Anyway I choose a minor number already allocated by mistake (did not see the "...") and will correct that. Is this ok or I *have to* move to dynamic major (it's a bit a nuisance since max3100 is used in embedded system where udev is not always used)?
Quoted 8 lines
`struct
max3100_port' is sufficient, and would be more typical.drivers/serial/max3100.c line 125
Quoted 5 lines
drivers/serial/max3100.c new file +5
-0+struct uart_port port; +struct spi_device *spi; + + int cts:1; /* last CTS received for flow ctrl */ + int tx_empty:1; /* last TX empty bit */
Quoted 6 lines
These two bits will share a word and hence locking is needed to prevent modifications to one from trashing modifications to the other on SMP.
That's OK, but it would be best to document that locking right here, and to check that it is adhered to.
I did not realize this until you explained me. I'm not sure if actual packing of bit-fields is implementation dependent but I think so. If this is right I guess it's better to avoid bit-fields in structs that can be accessed concurrently (or otherwise I have to lock the entire struct). So, should I avoid bit-fields altogether?
I will correct the patch and resend.
Thanks,
--
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
\> > + int rx_enabled:1; /* if we should rx chars */
Quoted 13 lines
Quoted 10 lines
drivers/serial/max3100.c new file +5
-0+ +int irq;/* irq assigned to the max3100 */ + +int minor;/* minor number */ + int crystal:1; /* 1 if 3.6864Mhz crystal 0for 1.8432 */
(quoted diff) +1
-0+ int loopback:1; /* 1 if we are in loopbackmode */
(quoted diff) +1
-0+ int only_edge_irq:1; /* 1 if we have only edge irqs(like PXA) */
Lots of dittoes.
Quoted 3 lines
These bitfields perhaps could be reordered to save a bit of space, but that depends on the implicit locking rules for them.
I do have a question though: what does a signed bitfield of 1 mean? I mean.. the variables are "int", so signed.... where will the compiler store the sign bit???
Signature · 3 lines
Arjan van de Ven Intel Open Source Technology Centre For development, discussion and tips for power savings, visit http://www.lesswatts.org
drivers/serial/max3100.c line 42
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+#define MAX3100_MAJOR 204 +#define MAX3100_MINOR 128 +/* 4 MAX3100s should be enough for everyone */ +#define MAX_MAX3100 4
These need to be officially allocated if you need constant numbers
drivers/serial/max3100.c line 246
Quoted 8 lines
drivers/serial/max3100.c new file +8
-0+static int max3100_sr (struct max3100_port_s *s,u16 tx,u16 *rx) +{ +struct spi_message message; +struct spi_transfer tran; +u16 etx, erx; +int status; + + etx =htons (tx);
Use cpu_to_le/be or le/be_to_cpu functions, these make the intended endianness clear.
drivers/serial/max3100.c line 258
Quoted 1 line
drivers/serial/max3100.c new file +1
-0+ *rx =ntohs (erx);
Ditto
drivers/serial/max3100.c line 359
Quoted 3 lines
drivers/serial/max3100.c new file +3
-0+if (rxchars >0 ) +tty_flip_buffer_push (s->port.info->port.tty); +if (uart_circ_chars_pending (xmit) <WAKEUP_CHARS )
If there has been a hangup the port.tty will be NULL...
drivers/serial/max3100.c line 481
Quoted 8 lines
drivers/serial/max3100.c new file +7
-0+static void +max3100_set_termios (struct uart_port *port,struct ktermios *termios, +struct ktermios *old) +{ + struct max3100_port_s *s = container_of(port, + struct max3100_port_s, + if (!old || (termios->c_cflag != old->c_cflag)) {
This optimisation is wrong and not worth doing anyway
drivers/serial/max3100.c line 493
Quoted 2 lines
drivers/serial/max3100.c new file +2
-0+ i = cflag &CBAUD ; +switch (i) {
Use tty_get_baud_rate() to get the actual baud rate requested which is an arbitary value.
drivers/serial/max3100.c line 552
Quoted 3 lines
drivers/serial/max3100.c new file +3
-0+default : + param_new =1 ; +dev_warn (&s->spi->dev,"invalid baudrate\n" );
Shouldn't warn on these, just be sure to use tty_encode_baud_rate to pass back the actual rate the user ends up with.
drivers/serial/max3100.c line 590
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+if (termios->c_iflag &IGNPAR ) + s->port.ignore_status_mask |= +MAX3100_STATUS_PE |MAX3100_STATUS_FE | +MAX3100_STATUS_OE ;
Bits you don't support should also be cleared in the tty->termios struct (eg markspace you don't seem to do)
drivers/serial/max3100.c line 828
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+ max3100s[i] =kzalloc (sizeof (struct max3100_port_s ),GFP_KERNEL ); +if (!max3100s[i]) { +dev_warn (&spi->dev, +"kmalloc for max3100 structure %d failed!\n" , i);
Does this not then need to unregister the driver ?
Looks basically sound to me - just some minor cleanups needed.
Alan
Quoted 6 lines · Arjan van de Ven
I do have a question though: what does a signed bitfield of 1 mean? I mean.. the variables are "int", so signed.... where will the compiler store the sign bit???
In practice they stored just values 0 and 1 well. 2-complement representation with 1 bit doesn't have much more space (1 == -1). :-)
Anyway after Andrew comment I stopped using them since I'm too scared:
I'm not sure that it's safe without locking the entire struct.
Signature · 5 lines
Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
Quoted 3 lines
I do have a question though: what does a signed bitfield of 1 mean? I mean.. the variables are "int", so signed.... where will the compiler store the sign bit???
It sign extends so you get 0 or -1. Yes it would be good to use unsigned to avoid suprises.
Quoted 7 lines · Alan Cox
drivers/serial/max3100.c line 42
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+#define MAX3100_MAJOR 204 +#define MAX3100_MINOR 128 +/* 4 MAX3100s should be enough for everyone */ +#define MAX_MAX3100 4These need to be officially allocated if you need constant numbers
OK, I'll try to mail [email protected]
Quoted 7 lines
drivers/serial/max3100.c line 359
Quoted 4 lines
drivers/serial/max3100.c new file +3
-0+if (rxchars >0 ) +tty_flip_buffer_push (s->port.info->port.tty); +if (uart_circ_chars_pending (xmit) <WAKEUP_CHARS )If there has been a hangup the port.tty will be NULL...
I didn't notice an explicit test for port.tty being non-NULL in some drivers. I took as an example sa1100 driver (since it needs polling of control signals too). I'm missing the way these drivers take care to not cause an access of a NULL pointer?
Quoted 3 lines
Looks basically sound to me - just some minor cleanups needed.
Thanks, I will fix them and resubmitt
Signature · 5 lines
Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
Quoted 3 lines · Arjan van de Ven
I do have a question though: what does a signed bitfield of 1 mean? I mean.. the variables are "int", so signed.... where will the compiler store the sign bit???
Whether a bit-field declared as type "int" is signed or unsigned is compiler implementation-defined. As C99 6.7.2 says (there is similar text in C89):
...for bit-fields, it is implementation-defined whether the specifier int designates the same type as signed int or the same type as unsigned int.
Thus, it is never a good idea to declare a bit-field as plain "int". Declare it as "signed int" or "unsigned int" instead.
Signature · 2 lines
"Mon peu de succès près des femmes est toujours venu de les trop aimer." --Jean-Jacques Rousseau
Quoted 7 lines · Andrew Morton
Quoted 1 line
+#define
MAX3100_MAJOR204Allocating a new major is a Big Deal. It involves getting the major registered by contacting [email protected].
It's better to dynamically allocate it - let udev handle it.
The MAX3100 is used in just small embedded systems where there is usually no udev. I looked at other serial drivers but I haven't seen none that uses dynamic allocation (they are share just 2 major numbers). I followed the guide in devices.txt and asked for an allocation of 4 minor numbers in the "Low-density serial ports".
Quoted 4 lines
Quoted 1 line
+#include <
linux/freezer.h>Gad. Are all those includes needed?
cleaned
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ +struct max3100_port_s {`struct
max3100_port' is sufficient, and would be more typical.
done
Quoted 6 lines
drivers/serial/max3100.c line 125
Quoted 5 lines
drivers/serial/max3100.c new file +5
-0+struct uart_port port; +struct spi_device *spi; + + int cts:1; /* last CTS received for flow ctrl */ + int tx_empty:1; /* last TX empty bit */
Quoted 6 lines
These two bits will share a word and hence locking is needed to prevent modifications to one from trashing modifications to the other on SMP.
That's OK, but it would be best to document that locking right here, and to check that it is adhered to.
I reverted to using word aligned data.
Quoted 4 lines
Quoted 1 line
+ /* and it's timer */
s/it's/its/
done. I just found the useful flyspell-prog-mode so I will drastically reduce English errors in the patches!
Quoted 6 lines
Quoted 1 line
+ parity = parity ^ ((c>>i) & 1);
hrmpf. Don't we have a library function for that?
Perhaps you could use hweight8(c)&1.
done
Quoted 11 lines
drivers/serial/max3100.c line 209
Quoted 7 lines
drivers/serial/max3100.c new file +7
-0+return parity; +} + +static int max3100_check_parity (struct max3100_port_s *s,u16 c) +{ +return max3100_do_parity (s, c) == + ((c >> (s->parity &MAX3100_7BIT ?7 :8 )) &1 );<checks his C precedence table>
OK...
done. Anyway I think that if precedence is unclear to me, it may be unclear to others too; so I prefer to use a parenthesis too much than one too few.
Quoted 4 lines
Quoted 1 line
+ *c &= ~MAX3100_PT;
s/ / /
done
Quoted 14 lines
drivers/serial/max3100.c line 224
Quoted 9 lines
drivers/serial/max3100.c new file +8
-0+} + +static void max3100_work (struct work_struct *w); + +static void max3100_dowork (struct max3100_port_s *s) +{ +if (!s->force_end_work && !work_pending (&s->work)) { +PREPARE_WORK (&s->work, max3100_work);This is a strange place to run
PREPARE_WORK(). Normally it would be done during driver/device initialsiation, and I think that can be done here.
done, I checked the meaning of PREPARE_WORK in the header file.
Quoted 10 lines
drivers/serial/max3100.c line 251
Quoted 7 lines
drivers/serial/max3100.c new file +7
-0+ + etx =htons (tx); +spi_message_init (&message); +memset (&tran,0 ,sizeof (tran)); + tran.tx_buf = &etx; + tran.rx_buf = &erx; + tran.len =2 ;could do
Quoted 8 lines
struct spi_transfer tran = { .tx_buf = &etx, .rx_buf = &erx .len = 2, };and the compiler will zero out the rest. Minor point.
done
Quoted 7 lines
drivers/serial/max3100.c line 431
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+ +dev_dbg (&s->spi->dev,"%s\n" , __func__); + +/* may not be truely up-to-date */ s/truely/truly/
done, next time flypsell-prog-mode will help me :-)
Quoted 6 lines
drivers/serial/max3100.c line 663
Quoted 2 lines
drivers/serial/max3100.c new file +2
-0+ +sprintf (b,"max3100-%d" , s->minor);hm. This will go nasty if
s->minor> 9. I'd suggest that b[] be made larger.
I limit the number of MAX3100s to 4, anyway I enlarged the buffer by one so to be sure.
Quoted 6 lines
drivers/serial/max3100.c line 678
Quoted 3 lines
drivers/serial/max3100.c new file +3
-0+if (request_irq (s->irq, max3100_irq, irq_type,"max3100" , s) <0 ) { +dev_warn (&s->spi->dev,"cannot allocate irq %d\n" , s->irq); + s->irq =0 ;Shouldn't we tear down that workqueue before returning?
done
Quoted 6 lines
drivers/serial/max3100.c line 713
Quoted 3 lines
drivers/serial/max3100.c new file +3
-0+static void max3100_release_port (struct uart_port *port) +{ +struct max3100_port_s *s = (struct max3100_port_s *)port;
container_of()
done
Sorry again for not replying in firs time.
Signature · 5 lines
Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
Quoted 7 lines · Alan Cox
drivers/serial/max3100.c line 42
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+#define MAX3100_MAJOR 204 +#define MAX3100_MINOR 128 +/* 4 MAX3100s should be enough for everyone */ +#define MAX_MAX3100 4These need to be officially allocated if you need constant numbers
done, I followed the guide in devices.txt
Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + etx = htons(tx);Use cpu_to_le/be or le/be_to_cpu functions, these make the intended endianness clear.
Quoted 1 line
+ *rx = ntohs(erx);
Ditto
done
Quoted 6 lines
drivers/serial/max3100.c line 359
Quoted 3 lines
drivers/serial/max3100.c new file +3
-0+if (rxchars >0 ) +tty_flip_buffer_push (s->port.info->port.tty); +if (uart_circ_chars_pending (xmit) <WAKEUP_CHARS )If there has been a hangup the port.tty will be NULL...
I check against NULL. But let me ask again: the other drivers (for example SA1100) don't do it. How they avoid the nasty NULL pointer dereference?
Quoted 11 lines
drivers/serial/max3100.c line 481
Quoted 8 lines
drivers/serial/max3100.c new file +7
-0+static void +max3100_set_termios (struct uart_port *port,struct ktermios *termios, +struct ktermios *old) +{ + struct max3100_port_s *s = container_of(port, + struct max3100_port_s, + if (!old || (termios->c_cflag != old->c_cflag)) {This optimisation is wrong and not worth doing anyway
done
Quoted 5 lines
Bits you don't support should also be cleared in the
tty->termiosstruct (eg markspace you don't seem to do)
done, I completely rewrote termios handling following Alan instructions.
Quoted 8 lines
drivers/serial/max3100.c line 828
Quoted 4 lines
drivers/serial/max3100.c new file +4
-0+ max3100s[i] =kzalloc (sizeof (struct max3100_port_s ),GFP_KERNEL ); +if (!max3100s[i]) { +dev_warn (&spi->dev, +"kmalloc for max3100 structure %d failed!\n" , i);Does this not then need to unregister the driver ?
I think no: perhaps it's just the probe of the second MAX3100 port that failed so we cannot unegister the driver. And the user can try to reprobe the MAX3100 via the bind entry. As I see it: driver registration and port registration are 2 different things.
Quoted 5 lines
Looks basically sound to me - just some minor cleanups needed.
Alan
Sorry again for not having replied before resending the patch.
Signature · 5 lines
Christian Pellegrin, see http://www.evolware.org/chri/ "Real Programmers don't play tennis, or any other sport which requires you to change clothes. Mountain climbing is OK, and Real Programmers wear their climbing boots to work in case a mountain should suddenly spring up in the middle of the computer room."
Quoted 6 lines
Quoted 2 lines
If there has been a hangup the port.tty will be NULL...
I check against NULL. But let me ask again: the other drivers (for example SA1100) don't do it. How they avoid the nasty NULL pointer dereference?
Some drives keep the tty pointer and deliver data to closed ttys wrongly, others do careful locking, others are just broken.
Quoted 1 line
Sorry again for not having replied before resending the patch.
No problem
PATCH 1/1 max3100 driver
In reply to Andrew MortonHi,
This patch fixes a problem about PM in my code that I found when testing patches that went in -mm. The full patch against the Linus tree can be found at http://www.evolware.org/chri/paciugo/
---
drivers/serial/max3100.c +4 -1
@@ -566,6 +566,7 @@ static void max3100_shutdown(struct uart_port *port)if (s->workqueue) {flush_workqueue (s->workqueue);destroy_workqueue (s->workqueue); + s->workqueue =NULL ; }if (s->irq)free_irq (s->irq, s);
@@ -614,6 +615,7 @@ static int max3100_startup(struct uart_port *port)dev_warn (&s->spi->dev,"cannot allocate irq %d\n" , s->irq); s->irq =0 ;destroy_workqueue (s->workqueue); + s->workqueue =NULL ;return -EBUSY ; }
@@ -884,7 +886,8 @@ static int max3100_resume(struct spi_device *spi)enable_irq (s->irq); s->conf_commit =1 ; -max3100_dowork (s); +if (s->workqueue) +max3100_dowork (s);return 0 ; }