Re: [PATCH] max3100 driver

Previous thread: [PATCH 2/2]: softirq: Add support for triggering softirq work on softirqs. by David Miller on Friday, September 19, 2008 - 11:48 pm. (9 messages)

Next thread: [PATCH] net: add net poll support for atl2 driver by Kevin Hao on Saturday, September 20, 2008 - 12:56 am. (2 messages)
From: Christian Pellegrin
Date: Saturday, September 20, 2008 - 12:20 am

This patch adds support for the MAX3100 SPI UART.

Generated on  20080920  against v2.6.27-rc6

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 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(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 77cb342..de3193d 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -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"
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index 7e7383e..21c3daf 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -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
diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
new file mode 100644
index 0000000..f8a8af1
--- /dev/null
+++ b/drivers/serial/max3100.c
@@ -0,0 +1,976 @@
+/*
+ *
+ *  Copyright (C) 2008 Christian Pellegrin <chripell@evolware.org>
+ *
+ * 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.
+ *
+ *
+ * Notes: the MAX3100 doesn't provide an ...
From: Andrew Morton
Date: Saturday, September 20, 2008 - 1:24 am

Allocating a new major is a Big Deal.  It involves getting the major
registered by contacting device@lanana.org.




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

Lots of dittoes.

These bitfields perhaps could be reordered to save a bit of space, but


hrmpf.  Don't we have a library function for that?


<checks his C precedence table>




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

could do

	struct spi_transfer tran = {
		.tx_buf = &etx,
		.rx_buf = &erx
		.len = 2,
	};



hm.  This will go nasty if s->minor > 9.  I'd suggest that b[] be made



Please cc Alan Cox for the tty things and perhaps
spi-devel-general@lists.sourceforge.net for the SPI bits.

--

From: chri
Date: Saturday, September 20, 2008 - 3:35 am

Sorry, sent HTML mail by mistake, so resending :-/


On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton


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


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."
--

From: Arjan van de Ven
Date: Saturday, September 20, 2008 - 6:56 am

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???


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
--

From: chri
Date: Saturday, September 20, 2008 - 7:30 am

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.

-- 
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."
--

From: Alan Cox
Date: Saturday, September 20, 2008 - 7:34 am

It sign extends so you get 0 or -1. Yes it would be good to use unsigned
to avoid suprises.
--

From: chri
Date: Wednesday, October 8, 2008 - 11:23 pm

On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton

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




done. I just found the useful flyspell-prog-mode so I will drastically


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





I limit the number of MAX3100s to 4, anyway I enlarged the buffer by


done


Sorry again for not replying in firs time.


-- 
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."
--

From: Christian Pellegrin
Date: Friday, October 10, 2008 - 5:08 am

Hi,

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/

---
diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 7a269a6..462d6a4 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -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;
 }
--

From: Alan Cox
Date: Saturday, September 20, 2008 - 7:11 am

Use cpu_to_le/be or le/be_to_cpu functions, these make the intended




Use tty_get_baud_rate() to get the actual baud rate requested which is an

Shouldn't warn on these, just be sure to use tty_encode_baud_rate to pass

Bits you don't support should also be cleared in the tty->termios struct

Does this not then need to unregister the driver ?



Looks basically sound to me - just some minor cleanups needed.

Alan
--

From: chri
Date: Saturday, September 20, 2008 - 7:37 am

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

Thanks, I will fix them and resubmitt


-- 
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."
--

From: chri
Date: Wednesday, October 8, 2008 - 11:30 pm

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



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

Sorry again for not having replied before resending the patch.


-- 
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."
--

From: Alan Cox
Date: Thursday, October 9, 2008 - 2:18 am

Some drives keep the tty pointer and deliver data to closed ttys wrongly,

No problem
--

Previous thread: [PATCH 2/2]: softirq: Add support for triggering softirq work on softirqs. by David Miller on Friday, September 19, 2008 - 11:48 pm. (9 messages)

Next thread: [PATCH] net: add net poll support for atl2 driver by Kevin Hao on Saturday, September 20, 2008 - 12:56 am. (2 messages)