Re: [PATCH] max3100 driver
On 12/17/2007 09:55 AM, chri wrote: > On Dec 17, 2007 9:45 AM, Jiri Slaby <[EMAIL PROTECTED]> wrote: >> You probably wanted to unregister it after _all_ cards are removed, not after >> each... >> > > Hi, > > thanks you are right. I haven't noticed this since spi devices are not > hot-pluggable so I could remove them only with rmmod (and so all of > them toghether). Anyway the code I sent is ugly, I will correct and > resend. Won't bind/unbind in sysfs help you? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] max3100 driver
On Dec 17, 2007 9:45 AM, Jiri Slaby <[EMAIL PROTECTED]> wrote: > > You probably wanted to unregister it after _all_ cards are removed, not after > each... > Hi, thanks you are right. I haven't noticed this since spi devices are not hot-pluggable so I could remove them only with rmmod (and so all of them toghether). Anyway the code I sent is ugly, I will correct and resend. > > Also you should synchronize_irq() after stopping them here. > I see. I guess I will have problems on suspending with serial activity otherwise. 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." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] max3100 driver
On 12/17/2007 09:17 AM, [EMAIL PROTECTED] wrote:
> This patch adds support for the MAX3100 SPI UART.
>
> Generated on 20071217 against v2.6.23
>
> Signed-off-by: Christian Pellegrin <[EMAIL PROTECTED]>
> ---
> drivers/serial/Kconfig |7 +
> drivers/serial/Makefile|1 +
> drivers/serial/max3100.c | 956
>
> include/linux/serial_max3100.h | 25 +
> 4 files changed, 989 insertions(+), 0 deletions(-)
>
[...]
> diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
> new file mode 100644
> index 000..d07936d
> --- /dev/null
> +++ b/drivers/serial/max3100.c
[...]
> +static int __devexit max3100_remove(struct spi_device *spi)
> +{
> + int i;
> +
> + i = 0;
> +
> + for (i = 0; i < MAX_MAX3100; i++)
> + if (max3100s[i] && max3100s[i]->ref_count > 0)
> + return -EBUSY;
> +
> + for (i = 0; i < MAX_MAX3100; i++)
> + if (max3100s[i]) {
> + tty_unregister_device(serial_driver, i);
> + kfree(max3100s[i]);
> + max3100s[i] = NULL;
> + }
> +
> + if (serial_driver)
> + tty_unregister_driver(serial_driver);
You probably wanted to unregister it after _all_ cards are removed, not after
each...
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int max3100_suspend(struct spi_device *spi, pm_message_t state)
> +{
> + struct max3100_port_s *s = dev_get_drvdata(&spi->dev);
> + u16 tx, rx;
> +
> + tx = MAX3100_WC | MAX3100_SHDN;
> + max3100_sr(s, tx, &rx, 0);
Also you should synchronize_irq() after stopping them here.
> + return 0;
> +}
regards,
--
Jiri Slaby ([EMAIL PROTECTED])
Faculty of Informatics, Masaryk University
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] max3100 driver
This patch adds support for the MAX3100 SPI UART.
Generated on 20071217 against v2.6.23
Signed-off-by: Christian Pellegrin <[EMAIL PROTECTED]>
---
drivers/serial/Kconfig |7 +
drivers/serial/Makefile|1 +
drivers/serial/max3100.c | 956
include/linux/serial_max3100.h | 25 +
4 files changed, 989 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 81b52b7..4e7111b 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -461,6 +461,13 @@ config SERIAL_S3C2410_CONSOLE
your boot loader about how to pass options to the kernel at
boot time.)
+config SERIAL_MAX3100
+tristate "MAX3100 support"
+depends on SPI
+help
+ MAX3100 chip support
+
+
config SERIAL_DZ
bool "DECstation DZ serial driver"
depends on MACH_DECSTATION && 32BIT
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index af6377d..9f67e52 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o
obj-$(CONFIG_SERIAL_SA1100) += sa1100.o
obj-$(CONFIG_SERIAL_BFIN) += bfin_5xx.o
obj-$(CONFIG_SERIAL_S3C2410) += s3c2410.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 000..d07936d
--- /dev/null
+++ b/drivers/serial/max3100.c
@@ -0,0 +1,956 @@
+
+/*
+ *
+ * Copyright (C) 2007 Christian Pellegrin <[EMAIL PROTECTED]>
+ *
+ * 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 interrupt on CTS so we have
+ * to use polling for flow control. TX empty IRQ is unusable, since
+ * writing conf clears FIFO buffer and we cannot have this interrupt
+ * always asking us for attention.
+ *
+ * Example platform data:
+
+static struct plat_max3100 max3100_plat_data = {
+ .loopback = 0,
+ .crystal = 0,
+ .only_edge_irq = 0,
+};
+
+static struct spi_board_info spi_board_info[] = {
+ {
+ .modalias = "max3100",
+ .platform_data = &max3100_plat_data,
+ .irq= IRQ_EINT12,
+ .max_speed_hz = 5*1000*1000,
+ .chip_select= 0,
+ },
+};
+
+ * The initial minor number is 128 to prevent clashes with ttyS:
+ * mknod /dev/ttyMAX0 c 4 128
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include
+
+#define MAX3100_C(1<<14)
+#define MAX3100_D(0<<14)
+#define MAX3100_W(1<<15)
+#define MAX3100_RX (0<<15)
+
+#define MAX3100_WC (MAX3100_W | MAX3100_C)
+#define MAX3100_RC (MAX3100_RX | MAX3100_C)
+#define MAX3100_WD (MAX3100_W | MAX3100_D)
+#define MAX3100_RD (MAX3100_RX | MAX3100_D)
+#define MAX3100_CMD (3 << 14)
+
+#define MAX3100_T(1<<14)
+#define MAX3100_R(1<<15)
+
+#define MAX3100_FEN (1<<13)
+#define MAX3100_SHDN (1<<12)
+#define MAX3100_TM (1<<11)
+#define MAX3100_RM (1<<10)
+#define MAX3100_PM (1<<9)
+#define MAX3100_RAM (1<<8)
+#define MAX3100_IR (1<<7)
+#define MAX3100_ST (1<<6)
+#define MAX3100_PE (1<<5)
+#define MAX3100_L(1<<4)
+#define MAX3100_BAUD (0xf)
+
+#define MAX3100_TE (1<<10)
+#define MAX3100_RAFE (1<<10)
+#define MAX3100_RTS (1<<9)
+#define MAX3100_CTS (1<<9)
+#define MAX3100_PT (1<<8)
+#define MAX3100_DATA (0xff)
+
+#define MAX3100_RT (MAX3100_R | MAX3100_T)
+#define MAX3100_RTC (MAX3100_RT | MAX3100_CTS | MAX3100_RAFE)
+
+struct max3100_port_s {
+ struct uart_port port;
+ struct spi_device *spi;
+ struct tty_struct *tty;
+
+ struct mutex spi_txrx;/* protects access to the hw */
+
+ int rts;/* rts status, can be MAX3100_RTS or 0 */
+ int conf; /* configuration for the MAX31000
+* (bits 0-7, bits 8-11 are irqs) */
+ int last_cts_rx;/* last CTS received for flow ctrl */
+
+ int tx_buf_cur; /* current char to tx */
+ int tx_buf_tot; /* current number of chars in tx buf */
+ unsigned char *tx_buf;
+ /* shared tx buffer spinlock (no sem
+* since write may sleep) */
+ spinlock_t tx_buf_lock;
+
+ int tx_stopped; /* when we should not send chars */
+ int parity; /* keeps
Re: [PATCH] max3100 driver
Hi,
thank you for your extensive review, I will fix and resubmitt. Anyway
I learned an important lesson: checkpatch.pl will be my best friend.
Sorry I haven't read about it earlier.
Just a question:
>
> > +/*
> > + *
> > + * 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 {
> > + int loopback;
> > +/* force MAX3100 in loopback */
> > + int crystal;
> > +/* 0 for 3.6864 Mhz, 1 for 1.8432 */
> > + int only_edge_irq;
> > +/* for archs like PXA with only edge irqs */
> > +};
>
> Does this header file need to exist? afaict plat_max3100 is only used by
> drivers/serial/max3100.c and hence could be defined there?
>
It's used in the machine definition file for platform devices that
cannot be autoprobed. Anyway where is the best place to put such
struct definition / include files? And what is the best way to
document them (if not inline)? For example I put in my machine
definition file (let's say arch/arm/mach-s3c2410/mach-smdk2410.c):
static struct plat_max3100 max3100_plat_data = {
.loopback = 0,
.crystal = 0,
.only_edge_irq = 0,
};
static struct spi_board_info spi_board_info[] = {
{
.modalias = "max3100",
.platform_data = &max3100_plat_data,
.irq= IRQ_EINT12,
.max_speed_hz = 5*1000*1000,
.chip_select= 0,
},
};
...
spi_register_board_info(spi_board_info, ARRAY_SIZE(spi_board_info));
--
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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] max3100 driver
On Wed, 5 Dec 2007 11:26:45 +0100 [EMAIL PROTECTED] wrote:
> This patch adds support for the MAX3100 SPI UART.
>
Doesn't compile on x86_64:
drivers/serial/max3100.c: In function 'startup':
drivers/serial/max3100.c:706: error: 'IRQT_FALLING' undeclared (first use in
this function)
drivers/serial/max3100.c:706: error: (Each undeclared identifier is reported
only once
drivers/serial/max3100.c:706: error: for each function it appears in.)
drivers/serial/max3100.c:708: error: 'IRQT_LOW' undeclared (first use in this
function)
> include/linux/serial_max3100.h | 25 +
> 4 files changed, 993 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 81b52b7..4e7111b 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -461,6 +461,13 @@ config SERIAL_S3C2410_CONSOLE
> your boot loader about how to pass options to the kernel at
> boot time.)
>
> +config SERIAL_MAX3100
> +tristate "MAX3100 support"
> +depends on SPI
> +help
> + MAX3100 chip support
> +
Putting a depends on ARM in here might be sufficient.
> +++ b/drivers/serial/max3100.c
> @@ -0,0 +1,960 @@
> +
> +/*
> + *
> + * Copyright (C) 2007 Christian Pellegrin <[EMAIL PROTECTED]>
> + *
> + * 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.
> + *
> + * version 0.1 31/5/2006 first public release
> + * version 0.2 11/11/2007 ported to kernel 2.6.23
Normally we skip info like this - it's all in the git tree, in better form.
> +static struct tty_driver *serial_driver = NULL;
This initialisation is unneeded and is undesirable because it increases the
kernel image's .data.
Please run the diff through scripts/checkpatch.pl. It would have mentioned
this, and a whole lot of other things, too.
> +/* global since we do register the driver only once */
I assume this comment refers to the preceding line?
Please put comments like this above the code which they're talking about.
Doing it this way is quite confusing to kernel developers.
> +#define MAX_MAX3100 4/* 4 MAX3100s should be enough for
> everyone */
> +static struct max3100_port_s * max3100s[MAX_MAX3100]; /* the chips */
No space after the *, please (checkpatch should detect this).
> +
> +#define MAX3100_TX_BUF_N 16
> +/* our buffer for sendind chars */
Typo. Misplaced comment.
> +#define MAX3100_FLIP_EVERY 8
> +/* how many chars we wait befor flipping tty buffer */
Misplaced comment.
> +static void irq_state(struct max3100_port_s *s, int on) {
That brace should go in column 1.
> + 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);
> + }
The `else' should be like this:
} else {
and as the brqaces are unneeded we'd prefer
if (on)
enable_irq(s->irq);
else
disable_irq(s->irq);
but checkpatch knows about this too.
> + 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;
> + else
> + parity = 1;
> +
> + if (s->parity & MAX3100_7BIT)
> + n = 7;
> + else
> + n = 8;
> +
> + for(i=0;i + parity = parity ^ ( (c>>i) & 1);
> + }
Those three lines will cause a checkpatch frenzy.
> + return parity;
> +}
> +
> +static inline int max3100_check_parity(struct max3100_port_s *s, u16 c)
> +{
> + return max3100_do_parity(s, c) == ( (c>>9) & 1);
> +}
> +
> +static inline void max3100_calc_parity(struct max3100_port_s *s, u16 *c)
> +{
> + *c &= ~MAX3100_PT;
> + *c |= max3100_do_parity(s, *c)<<8;
> +}
> +
> +
> +static void max3100_work(struct work_struct *w);
> +
> +static inline void max3100_dowork(struct max3100_port_s *s)
> +{
> + if (!s->force_end_work) {
> + PREPARE_WORK(&s->work, max3100_work);
> + queue_work(s->workqueue, &s->work);
> + }
> +}
You'd be surprised how small an inlined function can be and yet still be
too large to be inlined (if that sentence makes sense).
I'd suggest that you try removing inlines then recmpiling and doing `size
drivers/serial/max3100.o'. You'll probably find that most of them are
either unneeded or plain wrong.
> +static int max3100_sr(struct max3100_port_s *s, u16 tx, u16 *rx, int push)
> +{
> + /* note: we suppose that the T bit fron
[PATCH] max3100 driver
This patch adds support for the MAX3100 SPI UART.
Generated on 20071205 against v2.6.23
Signed-off-by: Christian Pellegrin <[EMAIL PROTECTED]>
---
drivers/serial/Kconfig |7 +
drivers/serial/Makefile|1 +
drivers/serial/max3100.c | 960
include/linux/serial_max3100.h | 25 +
4 files changed, 993 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 81b52b7..4e7111b 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -461,6 +461,13 @@ config SERIAL_S3C2410_CONSOLE
your boot loader about how to pass options to the kernel at
boot time.)
+config SERIAL_MAX3100
+tristate "MAX3100 support"
+depends on SPI
+help
+ MAX3100 chip support
+
+
config SERIAL_DZ
bool "DECstation DZ serial driver"
depends on MACH_DECSTATION && 32BIT
diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
index af6377d..9f67e52 100644
--- a/drivers/serial/Makefile
+++ b/drivers/serial/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o
obj-$(CONFIG_SERIAL_SA1100) += sa1100.o
obj-$(CONFIG_SERIAL_BFIN) += bfin_5xx.o
obj-$(CONFIG_SERIAL_S3C2410) += s3c2410.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 000..dd48258
--- /dev/null
+++ b/drivers/serial/max3100.c
@@ -0,0 +1,960 @@
+
+/*
+ *
+ * Copyright (C) 2007 Christian Pellegrin <[EMAIL PROTECTED]>
+ *
+ * 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.
+ *
+ * version 0.1 31/5/2006 first public release
+ * version 0.2 11/11/2007 ported to kernel 2.6.23
+ *
+ *
+ *
+ * Notes: the MAX3100 doesn't provide an interrupt on CTS
+ * so we have to use polling for flow control. TX empty
+ * IRQ is unusable, since writing conf clears FIFO buffer.
+ *
+ * Example platform data:
+
+static struct plat_max3100 max3100_plat_data = {
+ .loopback = 0,
+ .crystal = 0,
+ .only_edge_irq = 0,
+};
+
+static struct spi_board_info spi_board_info[] = {
+ {
+ .modalias = "max3100",
+ .platform_data = &max3100_plat_data,
+ .irq= IRQ_EINT12,
+ .max_speed_hz = 5*1000*1000,
+ .chip_select= 0,
+ },
+};
+
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define MAX3100_C(1<<14)
+#define MAX3100_D(0<<14)
+#define MAX3100_W(1<<15)
+#define MAX3100_RX (0<<15)
+
+#define MAX3100_WC (MAX3100_W | MAX3100_C)
+#define MAX3100_RC (MAX3100_RX| MAX3100_C)
+#define MAX3100_WD (MAX3100_W | MAX3100_D)
+#define MAX3100_RD (MAX3100_RX| MAX3100_D)
+#define MAX3100_CMD (3 << 14)
+
+#define MAX3100_T(1<<14)
+#define MAX3100_R(1<<15)
+
+#define MAX3100_FEN (1<<13)
+#define MAX3100_SHDN (1<<12)
+#define MAX3100_TM (1<<11)
+#define MAX3100_RM (1<<10)
+#define MAX3100_PM (1<<9)
+#define MAX3100_RAM (1<<8)
+#define MAX3100_IR (1<<7)
+#define MAX3100_ST (1<<6)
+#define MAX3100_PE (1<<5)
+#define MAX3100_L(1<<4)
+#define MAX3100_BAUD (0xf)
+
+#define MAX3100_TE (1<<10)
+#define MAX3100_RAFE (1<<10)
+#define MAX3100_RTS (1<<9)
+#define MAX3100_CTS (1<<9)
+#define MAX3100_PT (1<<8)
+#define MAX3100_DATA (0xff)
+
+#define MAX3100_RT (MAX3100_R | MAX3100_T)
+#define MAX3100_RTC (MAX3100_RT | MAX3100_CTS | MAX3100_RAFE)
+
+struct max3100_port_s {
+ struct uart_port port;
+ struct spi_device *spi;
+ struct tty_struct *tty;
+
+ struct semaphore spi_txrx; /* protects access to the hw */
+
+ int rts;/* rts status, can be MAX3100_RTS or 0 */
+ int conf; /* configuration for the MAX31000
+* (bits 0-7, bits 8-11 are irqs) */
+ int last_cts_rx;/* last CTS received for flow ctrl */
+
+ int tx_buf_cur; /* current char to tx */
+ int tx_buf_tot; /* current number of chars in tx buf */
+ unsigned char *tx_buf;
+ spinlock_t tx_buf_lock; /* shared tx buffer spinlock (no sem
+* since write may sleep) */
+
+ int tx_stopped; /* when we should not send chars */
+ int parity; /* keeps track if w
