gmane.linux.serial

Christian Pellegrin | 20 Sep 09:19

[PATCH] max3100 driver

This patch adds support for the MAX3100 SPI UART.

Generated on  20080920  against v2.6.27-rc6

Signed-off-by: Christian Pellegrin <chripell <at> 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"

(Continue reading)

Andrew Morton | 20 Sep 10:24

Re: [PATCH] max3100 driver

On Sat, 20 Sep 2008 09:20:08 +0200 Christian Pellegrin <chripell <at> gmail.com> wrote:

> This patch adds support for the MAX3100 SPI UART.
> 
>
> ...
>
> + * The initial minor number is 128 in the low-density serial port:
> + * mknod /dev/ttyMAX0 c 204 128
> + */
> +
> +#define MAX3100_MAJOR 204

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

It's better to dynamically allocate it - let udev handle it.

> +#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>

(Continue reading)

chri | 20 Sep 12:35

Re: [PATCH] max3100 driver

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

On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton
<akpm <at> linux-foundation.org> wrote:

> > +#define MAX3100_MAJOR 204
>
> Allocating a new major is a Big Deal.  It involves getting the major
> registered by contacting device <at> lanana.org.
>
> 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)?

> `struct max3100_port' is sufficient, and would be more typical.
>
> > +     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.

(Continue reading)

Arjan van de Ven | 20 Sep 15:56

Re: [PATCH] max3100 driver

\> > +	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.
> 

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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(Continue reading)

chri | 20 Sep 16:30

Re: [PATCH] max3100 driver

On Sat, Sep 20, 2008 at 3:56 PM, Arjan van de Ven <arjan <at> infradead.org> wrote:
>
> 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.

--

-- 
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Cox | 20 Sep 16:34

Re: [PATCH] max3100 driver

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ben Pfaff | 21 Sep 18:09

Re: [PATCH] max3100 driver

Arjan van de Ven <arjan <at> infradead.org> writes:

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

-- 
"Mon peu de succès près des femmes est toujours venu de les trop aimer."
--Jean-Jacques Rousseau

chri | 9 Oct 08:23

Re: [PATCH] max3100 driver

On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton
<akpm <at> linux-foundation.org> wrote:

>> +#define MAX3100_MAJOR 204
>
> Allocating a new major is a Big Deal.  It involves getting the major
> registered by contacting device <at> lanana.org.
>
> 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".

>> +#include <linux/freezer.h>
>
> Gad. Are all those includes needed?
>

cleaned

>> +
>> +struct max3100_port_s {
>
> `struct max3100_port' is sufficient, and would be more typical.
>

(Continue reading)

Christian Pellegrin | 10 Oct 14:08

[PATCH] max3100 driver


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);

(Continue reading)

Michael Trimarchi | 20 Sep 12:48

Re: [PATCH] max3100 driver

Hi,

----- Messaggio originale -----
> Da: chri <chripell <at> gmail.com>
> A: Andrew Morton <akpm <at> linux-foundation.org>
> Cc: linux-kernel <at> vger.kernel.org; linux-serial <at> vger.kernel.org
> Inviato: Sabato 20 settembre 2008, 12:35:26
> Oggetto: Re: [PATCH] max3100 driver
> 
> Sorry, sent HTML mail by mistake, so resending :-/
> 
> 
> On Sat, Sep 20, 2008 at 10:24 AM, Andrew Morton
> wrote:
> 
> > > +#define MAX3100_MAJOR 204
> >
> > Allocating a new major is a Big Deal.  It involves getting the major
> > registered by contacting device <at> lanana.org.
> >
> > 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)?

(Continue reading)

Alan Cox | 20 Sep 16:11

Re: [PATCH] max3100 driver

> +#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

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

> +	*rx = ntohs(erx);

Ditto

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

> +static void

(Continue reading)

chri | 20 Sep 16:37

Re: [PATCH] max3100 driver

On Sat, Sep 20, 2008 at 4:11 PM, Alan Cox <alan <at> lxorguk.ukuu.org.uk> wrote:
>> +#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
>

OK, I'll try to mail device <at> lanana.org

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

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

Thanks, I will fix them and resubmitt

(Continue reading)

chri | 9 Oct 08:30

Re: [PATCH] max3100 driver

On Sat, Sep 20, 2008 at 4:11 PM, Alan Cox <alan <at> lxorguk.ukuu.org.uk> wrote:
>> +#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
>

done, I followed the guide in devices.txt

>> +
>> +     etx = htons(tx);
>
> Use cpu_to_le/be or le/be_to_cpu functions, these make the intended
> endianness clear.
>
>> +     *rx = ntohs(erx);
>
> Ditto
>

done

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

(Continue reading)

Alan Cox | 9 Oct 11:18

Re: [PATCH] max3100 driver

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

> Sorry again for not having replied before resending the patch.

No problem
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Return

Return to gmane.linux.serial.

Advertisement

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane