Put the driver into the shape with all new bells and whistles
from the kernel.
Tested on Intel Merrifield with MAX3111e connected.
In v3:
- dropped applied patches
- rebased on top of tty-testing
- v2 ([email protected])
In v2:
- fixed a few typos in the commit messages (Hugo)
- added an additional fix to patch 2 (Hugo)
- appended tag to patch 13 (Hugo)
- v1 ([email protected])
Andy Shevchenko (8):
serial: max3100: Enable TIOCM_LOOP
serial: max3100: Get crystal frequency via device property
serial: max3100: Remove duplicating irq field
serial: max3100: Switch to use dev_err_probe()
serial: max3100: Replace MODULE_ALIAS() with respective ID tables
serial: max3100: Switch to DEFINE_SIMPLE_DEV_PM_OPS()
serial: max3100: Extract to_max3100_port() helper macro
serial: max3100: Sort headers
drivers/tty/serial/max3100.c | 208 +++++++++++++++--------------------
1 file changed, 86 insertions(+), 122 deletions(-)
--
2.43.0.rc1.1.gbec44491f096
Instead of using container_of() explicitly, introduce a helper macro.
This saves a lot of lines of code.
Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max3100.c | 67 ++++++++++--------------------------
1 file changed, 19 insertions(+), 48 deletions(-)
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index db543eaa39c8..4e7cd037cfc2 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -16,6 +16,7 @@
/* 4 MAX3100s should be enough for everyone */
#define MAX_MAX3100 4
+#include <linux/container_of.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/device.h>
@@ -110,6 +111,8 @@ struct max3100_port {
struct timer_list timer;
};
+#define to_max3100_port(port) container_of(port, struct max3100_port, port)
+
static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
static DEFINE_MUTEX(max3100s_lock); /* race on probe */
@@ -320,9 +323,7 @@ static irqreturn_t max3100_irq(int irqno, void *dev_id)
static void max3100_enable_ms(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
mod_timer(&s->timer, jiffies);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -330,9 +331,7 @@ static void max3100_enable_ms(struct uart_port *port)
static void max3100_start_tx(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -341,9 +340,7 @@ static void max3100_start_tx(struct uart_port *port)
static void max3100_stop_rx(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -357,9 +354,7 @@ static void max3100_stop_rx(struct uart_port *port)
static unsigned int max3100_tx_empty(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -370,9 +365,7 @@ static unsigned int max3100_tx_empty(struct uart_port *port)
static unsigned int max3100_get_mctrl(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -384,9 +377,7 @@ static unsigned int max3100_get_mctrl(struct uart_port *port)
static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
int loopback, rts;
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -412,9 +403,7 @@ static void
max3100_set_termios(struct uart_port *port, struct ktermios *termios,
const struct ktermios *old)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
unsigned int baud = port->uartclk / 16;
unsigned int baud230400 = (baud == 230400) ? 1 : 0;
unsigned cflag;
@@ -530,9 +519,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
static void max3100_shutdown(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
u16 rx;
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -557,9 +544,7 @@ static void max3100_shutdown(struct uart_port *port)
static int max3100_startup(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
char b[12];
int ret;
@@ -605,9 +590,7 @@ static int max3100_startup(struct uart_port *port)
static const char *max3100_type(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -616,18 +599,14 @@ static const char *max3100_type(struct uart_port *port)
static void max3100_release_port(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
}
static void max3100_config_port(struct uart_port *port, int flags)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -638,9 +617,7 @@ static void max3100_config_port(struct uart_port *port, int flags)
static int max3100_verify_port(struct uart_port *port,
struct serial_struct *ser)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
int ret = -EINVAL;
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -652,18 +629,14 @@ static int max3100_verify_port(struct uart_port *port,
static void max3100_stop_tx(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
}
static int max3100_request_port(struct uart_port *port)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
return 0;
@@ -671,9 +644,7 @@ static int max3100_request_port(struct uart_port *port)
static void max3100_break_ctl(struct uart_port *port, int break_state)
{
- struct max3100_port *s = container_of(port,
- struct max3100_port,
- port);
+ struct max3100_port *s = to_max3100_port(port);
dev_dbg(&s->spi->dev, "%s\n", __func__);
}
--
2.43.0.rc1.1.gbec44491f096
Enable or disable loopback at run-time.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/tty/serial/max3100.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 75e96ff74571..4a4343e7b1fa 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -32,14 +32,12 @@
/**
* struct plat_max3100 - MAX3100 SPI UART platform data
- * @loopback: force MAX3100 in loopback
* @crystal: 1 for 3.6864 Mhz, 0 for 1.8432
*
* You should use this structure in your machine description to specify
* how the MAX3100 is connected.
*/
struct plat_max3100 {
- int loopback;
int crystal;
};
@@ -109,6 +107,7 @@ struct max3100_port {
int minor; /* minor number */
int crystal; /* 1 if 3.6864Mhz crystal 0 for 1.8432 */
+ int loopback_commit; /* need to change loopback */
int loopback; /* 1 if we are in loopback mode */
/* for handling irqs: need workqueue since we do spi_sync */
@@ -241,9 +240,9 @@ static void max3100_work(struct work_struct *w)
struct max3100_port *s = container_of(w, struct max3100_port, work);
struct tty_port *tport = &s->port.state->port;
unsigned char ch;
+ int conf, cconf, cloopback, crts;
int rxchars;
u16 tx, rx;
- int conf, cconf, crts;
dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -253,11 +252,15 @@ static void max3100_work(struct work_struct *w)
conf = s->conf;
cconf = s->conf_commit;
s->conf_commit = 0;
+ cloopback = s->loopback_commit;
+ s->loopback_commit = 0;
crts = s->rts_commit;
s->rts_commit = 0;
spin_unlock(&s->conf_lock);
if (cconf)
max3100_sr(s, MAX3100_WC | conf, &rx);
+ if (cloopback)
+ max3100_sr(s, 0x4001, &rx);
if (crts) {
max3100_sr(s, MAX3100_WD | MAX3100_TE |
(s->rts ? MAX3100_RTS : 0), &rx);
@@ -395,18 +398,24 @@ static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
struct max3100_port *s = container_of(port,
struct max3100_port,
port);
- int rts;
+ int loopback, rts;
dev_dbg(&s->spi->dev, "%s\n", __func__);
+ loopback = (mctrl & TIOCM_LOOP) > 0;
rts = (mctrl & TIOCM_RTS) > 0;
spin_lock(&s->conf_lock);
+ if (s->loopback != loopback) {
+ s->loopback = loopback;
+ s->loopback_commit = 1;
+ }
if (s->rts != rts) {
s->rts = rts;
s->rts_commit = 1;
- max3100_dowork(s);
}
+ if (s->loopback_commit || s->rts_commit)
+ max3100_dowork(s);
spin_unlock(&s->conf_lock);
}
@@ -593,12 +602,6 @@ static int max3100_startup(struct uart_port *port)
return -EBUSY;
}
- if (s->loopback) {
- u16 tx, rx;
- tx = 0x4001;
- max3100_sr(s, tx, &rx);
- }
-
s->conf_commit = 1;
max3100_dowork(s);
/* wait for clock to settle */
@@ -754,7 +757,6 @@ static int max3100_probe(struct spi_device *spi)
spi_set_drvdata(spi, max3100s[i]);
pdata = dev_get_platdata(&spi->dev);
max3100s[i]->crystal = pdata->crystal;
- max3100s[i]->loopback = pdata->loopback;
max3100s[i]->minor = i;
timer_setup(&max3100s[i]->timer, max3100_timeout, 0);
--
2.43.0.rc1.1.gbec44491f096
MODULE_ALIAS() in most cases is a pure hack to avoid placing ID tables.
Replace it with the respective ID tables.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/tty/serial/max3100.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 09ec2ca06989..3004a95e573f 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -19,6 +19,7 @@
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/device.h>
+#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/property.h>
#include <linux/serial_core.h>
@@ -840,13 +841,27 @@ static SIMPLE_DEV_PM_OPS(max3100_pm_ops, max3100_suspend, max3100_resume);
#define MAX3100_PM_OPS NULL
#endif
+static const struct spi_device_id max3100_spi_id[] = {
+ { "max3100" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, max3100_spi_id);
+
+static const struct of_device_id max3100_of_match[] = {
+ { .compatible = "maxim,max3100" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max3100_of_match);
+
static struct spi_driver max3100_driver = {
.driver = {
.name = "max3100",
+ .of_match_table = max3100_of_match,
.pm = MAX3100_PM_OPS,
},
.probe = max3100_probe,
.remove = max3100_remove,
+ .id_table = max3100_spi_id,
};
module_spi_driver(max3100_driver);
@@ -854,4 +869,3 @@ module_spi_driver(max3100_driver);
MODULE_DESCRIPTION("MAX3100 driver");
MODULE_AUTHOR("Christian Pellegrin <[email protected]>");
MODULE_LICENSE("GPL");
-MODULE_ALIAS("spi:max3100");
--
2.43.0.rc1.1.gbec44491f096
On 09. 04. 24, 16:45, Andy Shevchenko wrote:
> Instead of using container_of() explicitly, introduce a helper macro.
> This saves a lot of lines of code.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Hugo Villeneuve <[email protected]>
..
> @@ -110,6 +111,8 @@ struct max3100_port {
> struct timer_list timer;
> };
>
> +#define to_max3100_port(port) container_of(port, struct max3100_port, port)
This is wrong. If you pass something other than "port" to
to_max3100_port(), the third arg of container_of() will explode.
Use an inline to avoid mistakes like this.
regards,
--
js
suse labs
On Wed, Apr 10, 2024 at 09:17:28AM +0200, Jiri Slaby wrote:
> On 09. 04. 24, 16:45, Andy Shevchenko wrote:
> > Instead of using container_of() explicitly, introduce a helper macro.
> > This saves a lot of lines of code.
..
> > +#define to_max3100_port(port) container_of(port, struct max3100_port, port)
>
> This is wrong. If you pass something other than "port" to to_max3100_port(),
> the third arg of container_of() will explode.
Then don't do that :-)
> Use an inline to avoid mistakes like this.
Sure, thanks for catching this. Should I send an update to prevent this from
happening?
--
With Best Regards,
Andy Shevchenko