[PATCH v1 00/16] serial: max3100: Put into shape

[PATCH v1 00/16] serial: max3100: Put into shape

[Andy Shevchenko]

Put the driver into the shape with all new bells and whistles
from the kernel.

The first three patches marked as fixes, but there is no hurry (as it
was for ages like this in the kernel) to pipe them to stable. That's
why I sent all in one series and it's good for tty-next.

Tested on Intel Merrifield with MAX3111e connected.

Andy Shevchenko (16):
  serial: max3100: Lock port->lock when calling uart_handle_cts_change()
  serial: max3100: Update uart_driver_registered on driver removal
  serial: max3100: Fix bitwise types
  serial: max3100: Make struct plat_max3100 local
  serial: max3100: Remove custom HW shutdown support
  serial: max3100: Replace custom polling timeout with standard one
  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: Remove unneeded forward declaration
  serial: max3100: Sort headers
  serial: max3100: Update Kconfig entry

 drivers/tty/serial/Kconfig     |   7 +-
 drivers/tty/serial/max3100.c   | 320 +++++++++++++--------------------
 include/linux/serial_max3100.h |  48 -----
 3 files changed, 131 insertions(+), 244 deletions(-)
 delete mode 100644 include/linux/serial_max3100.h

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 01/16] serial: max3100: Lock port->lock when calling uart_handle_cts_change()

[Andy Shevchenko]

uart_handle_cts_change() has to be called with port lock taken,
Since we run it in a separate work, the lcok maybe not taken at
the time of running. Make sure that it's taken by explicitly doing
that. Without it we got a splat:

  WARNING: CPU: 0 PID: 10 at drivers/tty/serial/serial_core.c:3491 uart_handle_cts_change+0xa6/0xb0
  ...
  Workqueue: max3100-0 max3100_work [max3100]
  RIP: 0010:uart_handle_cts_change+0xa6/0xb0
  ...
   max3100_handlerx+0xc5/0x110 [max3100]
   max3100_work+0x12a/0x340 [max3100]

Fixes: 7831d56b0a35 ("tty: MAX3100")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 5efb2b593be3..45022f2909f0 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -213,7 +213,7 @@ static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
 	return 0;
 }
 
-static int max3100_handlerx(struct max3100_port *s, u16 rx)
+static int max3100_handlerx_unlocked(struct max3100_port *s, u16 rx)
 {
 	unsigned int status = 0;
 	int ret = 0, cts;
@@ -254,6 +254,17 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
 	return ret;
 }
 
+static int max3100_handlerx(struct max3100_port *s, u16 rx)
+{
+	unsigned long flags;
+	int ret;
+
+	uart_port_lock_irqsave(&s->port, &flags);
+	ret = max3100_handlerx_unlocked(s, rx);
+	uart_port_unlock_irqrestore(&s->port, flags);
+	return ret;
+}
+
 static void max3100_work(struct work_struct *w)
 {
 	struct max3100_port *s = container_of(w, struct max3100_port, work);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 02/16] serial: max3100: Update uart_driver_registered on driver removal

[Andy Shevchenko]

The removal of the last MAX3100 device triggers the removal of
the driver. However, code doesn't update the respective global
variable and after insmod — rmmod — insmod cycle the kernel
oopses:

  max3100 spi-PRP0001:01: max3100_probe: adding port 0
  BUG: kernel NULL pointer dereference, address: 0000000000000408
  ...
  RIP: 0010:serial_core_register_port+0xa0/0x840
  ...
   max3100_probe+0x1b6/0x280 [max3100]
   spi_probe+0x8d/0xb0

Update the actual state so next time UART driver will be registered
again.

Fixes: 7831d56b0a35 ("tty: MAX3100")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 45022f2909f0..efe26f6d5ebf 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -841,6 +841,7 @@ static void max3100_remove(struct spi_device *spi)
 		}
 	pr_debug("removing max3100 driver\n");
 	uart_unregister_driver(&max3100_uart_driver);
+	uart_driver_registered = 0;
 
 	mutex_unlock(&max3100s_lock);
 }
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 03/16] serial: max3100: Fix bitwise types

[Andy Shevchenko]

Sparse is not happy about misuse of bitwise types:

  .../max3100.c:194:13: warning: incorrect type in assignment (different base types)
  .../max3100.c:194:13:    expected unsigned short [addressable] [usertype] etx
  .../max3100.c:194:13:    got restricted __be16 [usertype]
  .../max3100.c:202:15: warning: cast to restricted __be16

Fix this by choosing proper types for the respective variables.

Fixes: 7831d56b0a35 ("tty: MAX3100")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index efe26f6d5ebf..3c68b8e1a226 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -45,6 +45,9 @@
 #include <linux/freezer.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/types.h>
+
+#include <asm/unaligned.h>
 
 #include <linux/serial_max3100.h>
 
@@ -191,7 +194,7 @@ static void max3100_timeout(struct timer_list *t)
 static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
 {
 	struct spi_message message;
-	u16 etx, erx;
+	__be16 etx, erx;
 	int status;
 	struct spi_transfer tran = {
 		.tx_buf = &etx,
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 04/16] serial: max3100: Make struct plat_max3100 local

[Andy Shevchenko]

There is no user of the struct plat_max3100 outside the driver.
Inline its contents into the driver. While at it, drop outdated
example in the comment.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c   | 38 +++++++++++++--------------
 include/linux/serial_max3100.h | 48 ----------------------------------
 2 files changed, 18 insertions(+), 68 deletions(-)
 delete mode 100644 include/linux/serial_max3100.h

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 3c68b8e1a226..f30050248130 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- *
  *  Copyright (C) 2008 Christian Pellegrin <chripell@evolware.org>
  *
  * Notes: the MAX3100 doesn't provide an interrupt on CTS so we have
@@ -8,24 +7,6 @@
  * 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,
- .poll_time = 100,
- };
-
- 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 209 in the low-density serial port:
  * mknod /dev/ttyMAX0 c 204 209
  */
@@ -49,7 +30,24 @@
 
 #include <asm/unaligned.h>
 
-#include <linux/serial_max3100.h>
+/**
+ * struct plat_max3100 - MAX3100 SPI UART platform data
+ * @loopback:            force MAX3100 in loopback
+ * @crystal:             1 for 3.6864 Mhz, 0 for 1.8432
+ * @max3100_hw_suspend:  MAX3100 has a shutdown pin. This is a hook
+ *                       called on suspend and resume to activate it.
+ * @poll_time:           poll time for CTS signal in ms, 0 disables (so no hw
+ *                       flow ctrl is possible but you have less CPU usage)
+ *
+ * You should use this structure in your machine description to specify
+ * how the MAX3100 is connected.
+ */
+struct plat_max3100 {
+	int loopback;
+	int crystal;
+	void (*max3100_hw_suspend) (int suspend);
+	int poll_time;
+};
 
 #define MAX3100_C    (1<<14)
 #define MAX3100_D    (0<<14)
diff --git a/include/linux/serial_max3100.h b/include/linux/serial_max3100.h
deleted file mode 100644
index befd55c08a7c..000000000000
--- a/include/linux/serial_max3100.h
+++ /dev/null
@@ -1,48 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- *
- *  Copyright (C) 2007 Christian Pellegrin
- */
-
-
-#ifndef _LINUX_SERIAL_MAX3100_H
-#define _LINUX_SERIAL_MAX3100_H 1
-
-
-/**
- * struct plat_max3100 - MAX3100 SPI UART platform data
- * @loopback:            force MAX3100 in loopback
- * @crystal:             1 for 3.6864 Mhz, 0 for 1.8432
- * @max3100_hw_suspend:  MAX3100 has a shutdown pin. This is a hook
- *                       called on suspend and resume to activate it.
- * @poll_time:           poll time for CTS signal in ms, 0 disables (so no hw
- *                       flow ctrl is possible but you have less CPU usage)
- *
- * You should use this structure in your machine description to specify
- * how the MAX3100 is connected. Example:
- *
- * static struct plat_max3100 max3100_plat_data = {
- *  .loopback = 0,
- *  .crystal = 0,
- *  .poll_time = 100,
- * };
- *
- * 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,
- * },
- * };
- *
- **/
-struct plat_max3100 {
-	int loopback;
-	int crystal;
-	void (*max3100_hw_suspend) (int suspend);
-	int poll_time;
-};
-
-#endif
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 05/16] serial: max3100: Remove custom HW shutdown support

[Andy Shevchenko]

While there is no user of this callback in the kernel, it also breaks
the relationship in the driver model. The correct implementation should
be done via GPIO or regulator framework.

Remove custom HW shutdown support for good and, if needed, we will
implement it correctly later on.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 42 ++++++------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index f30050248130..0931d7be9c62 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -34,8 +34,6 @@
  * struct plat_max3100 - MAX3100 SPI UART platform data
  * @loopback:            force MAX3100 in loopback
  * @crystal:             1 for 3.6864 Mhz, 0 for 1.8432
- * @max3100_hw_suspend:  MAX3100 has a shutdown pin. This is a hook
- *                       called on suspend and resume to activate it.
  * @poll_time:           poll time for CTS signal in ms, 0 disables (so no hw
  *                       flow ctrl is possible but you have less CPU usage)
  *
@@ -45,7 +43,6 @@
 struct plat_max3100 {
 	int loopback;
 	int crystal;
-	void (*max3100_hw_suspend) (int suspend);
 	int poll_time;
 };
 
@@ -125,9 +122,6 @@ struct max3100_port {
 	/* need to know we are suspending to avoid deadlock on workqueue */
 	int suspending;
 
-	/* hook for suspending MAX3100 via dedicated pin */
-	void (*max3100_hw_suspend) (int suspend);
-
 	/* poll time (in ms) for ctrl lines */
 	int poll_time;
 	/* and its timer */
@@ -553,6 +547,7 @@ static void max3100_shutdown(struct uart_port *port)
 	struct max3100_port *s = container_of(port,
 					      struct max3100_port,
 					      port);
+	u16 rx;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -572,14 +567,7 @@ static void max3100_shutdown(struct uart_port *port)
 		free_irq(s->irq, s);
 
 	/* set shutdown mode to save power */
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(1);
-	else  {
-		u16 tx, rx;
-
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(s, tx, &rx);
-	}
+	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
 }
 
 static int max3100_startup(struct uart_port *port)
@@ -625,8 +613,6 @@ static int max3100_startup(struct uart_port *port)
 		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 */
@@ -745,7 +731,7 @@ static int max3100_probe(struct spi_device *spi)
 {
 	int i, retval;
 	struct plat_max3100 *pdata;
-	u16 tx, rx;
+	u16 rx;
 
 	mutex_lock(&max3100s_lock);
 
@@ -785,7 +771,6 @@ static int max3100_probe(struct spi_device *spi)
 	max3100s[i]->poll_time = msecs_to_jiffies(pdata->poll_time);
 	if (pdata->poll_time > 0 && max3100s[i]->poll_time == 0)
 		max3100s[i]->poll_time = 1;
-	max3100s[i]->max3100_hw_suspend = pdata->max3100_hw_suspend;
 	max3100s[i]->minor = i;
 	timer_setup(&max3100s[i]->timer, max3100_timeout, 0);
 
@@ -805,12 +790,7 @@ static int max3100_probe(struct spi_device *spi)
 			 i, retval);
 
 	/* set shutdown mode to save power. Will be woken-up on open */
-	if (max3100s[i]->max3100_hw_suspend)
-		max3100s[i]->max3100_hw_suspend(1);
-	else {
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(max3100s[i], tx, &rx);
-	}
+	max3100_sr(max3100s[i], MAX3100_WC | MAX3100_SHDN, &rx);
 	mutex_unlock(&max3100s_lock);
 	return 0;
 }
@@ -852,6 +832,7 @@ static void max3100_remove(struct spi_device *spi)
 static int max3100_suspend(struct device *dev)
 {
 	struct max3100_port *s = dev_get_drvdata(dev);
+	u16 rx;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -860,15 +841,8 @@ static int max3100_suspend(struct device *dev)
 	s->suspending = 1;
 	uart_suspend_port(&max3100_uart_driver, &s->port);
 
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(1);
-	else {
-		/* no HW suspend, so do SW one */
-		u16 tx, rx;
-
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(s, tx, &rx);
-	}
+	/* no HW suspend, so do SW one */
+	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
 	return 0;
 }
 
@@ -878,8 +852,6 @@ static int max3100_resume(struct device *dev)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(0);
 	uart_resume_port(&max3100_uart_driver, &s->port);
 	s->suspending = 0;
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 06/16] serial: max3100: Replace custom polling timeout with standard one

[Andy Shevchenko]

Serial core provides a standard timeout for polling modem state.
Use it instead of a custom approach.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 0931d7be9c62..d4f0ea97a698 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -34,8 +34,6 @@
  * struct plat_max3100 - MAX3100 SPI UART platform data
  * @loopback:            force MAX3100 in loopback
  * @crystal:             1 for 3.6864 Mhz, 0 for 1.8432
- * @poll_time:           poll time for CTS signal in ms, 0 disables (so no hw
- *                       flow ctrl is possible but you have less CPU usage)
  *
  * You should use this structure in your machine description to specify
  * how the MAX3100 is connected.
@@ -43,7 +41,6 @@
 struct plat_max3100 {
 	int loopback;
 	int crystal;
-	int poll_time;
 };
 
 #define MAX3100_C    (1<<14)
@@ -122,9 +119,6 @@ struct max3100_port {
 	/* need to know we are suspending to avoid deadlock on workqueue */
 	int suspending;
 
-	/* poll time (in ms) for ctrl lines */
-	int poll_time;
-	/* and its timer */
 	struct timer_list	timer;
 };
 
@@ -177,10 +171,8 @@ static void max3100_timeout(struct timer_list *t)
 {
 	struct max3100_port *s = from_timer(s, t, timer);
 
-	if (s->port.state) {
-		max3100_dowork(s);
-		mod_timer(&s->timer, jiffies + s->poll_time);
-	}
+	max3100_dowork(s);
+	mod_timer(&s->timer, jiffies + uart_poll_timeout(&s->port));
 }
 
 static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
@@ -342,8 +334,7 @@ static void max3100_enable_ms(struct uart_port *port)
 					      struct max3100_port,
 					      port);
 
-	if (s->poll_time > 0)
-		mod_timer(&s->timer, jiffies);
+	mod_timer(&s->timer, jiffies);
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 }
 
@@ -526,9 +517,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 			MAX3100_STATUS_PE | MAX3100_STATUS_FE |
 			MAX3100_STATUS_OE;
 
-	if (s->poll_time > 0)
-		del_timer_sync(&s->timer);
-
+	del_timer_sync(&s->timer);
 	uart_update_timeout(port, termios->c_cflag, baud);
 
 	spin_lock(&s->conf_lock);
@@ -556,8 +545,7 @@ static void max3100_shutdown(struct uart_port *port)
 
 	s->force_end_work = 1;
 
-	if (s->poll_time > 0)
-		del_timer_sync(&s->timer);
+	del_timer_sync(&s->timer);
 
 	if (s->workqueue) {
 		destroy_workqueue(s->workqueue);
@@ -768,9 +756,6 @@ static int max3100_probe(struct spi_device *spi)
 	pdata = dev_get_platdata(&spi->dev);
 	max3100s[i]->crystal = pdata->crystal;
 	max3100s[i]->loopback = pdata->loopback;
-	max3100s[i]->poll_time = msecs_to_jiffies(pdata->poll_time);
-	if (pdata->poll_time > 0 && max3100s[i]->poll_time == 0)
-		max3100s[i]->poll_time = 1;
 	max3100s[i]->minor = i;
 	timer_setup(&max3100s[i]->timer, max3100_timeout, 0);
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 07/16] serial: max3100: Enable TIOCM_LOOP

[Andy Shevchenko]

Enable or disable loopback at run-time.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 d4f0ea97a698..dd98f8037b60 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 */
@@ -257,7 +256,7 @@ static void max3100_work(struct work_struct *w)
 	struct max3100_port *s = container_of(w, struct max3100_port, work);
 	int rxchars;
 	u16 tx, rx;
-	int conf, cconf, crts;
+	int conf, cconf, cloopback, crts;
 	struct circ_buf *xmit = &s->port.state->xmit;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
@@ -268,11 +267,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);
@@ -397,18 +400,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);
 }
 
@@ -595,12 +604,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 */
@@ -755,7 +758,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

[PATCH v1 08/16] serial: max3100: Get crystal frequency via device property

[Andy Shevchenko]

Get crystal frequency via device property instead of using
a platform data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 49 +++++++++++++++---------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index dd98f8037b60..e5a1a9171047 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/spi/spi.h>
@@ -30,17 +31,6 @@
 
 #include <asm/unaligned.h>
 
-/**
- * struct plat_max3100 - MAX3100 SPI UART platform data
- * @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 crystal;
-};
-
 #define MAX3100_C    (1<<14)
 #define MAX3100_D    (0<<14)
 #define MAX3100_W    (1<<15)
@@ -106,7 +96,6 @@ struct max3100_port {
 	int irq;		/* irq assigned to the max3100 */
 
 	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 */
 
@@ -428,7 +417,8 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 	struct max3100_port *s = container_of(port,
 					      struct max3100_port,
 					      port);
-	int baud = 0;
+	unsigned int baud = port->uartclk / 16;
+	unsigned int baud230400 = (baud == 230400) ? 1 : 0;
 	unsigned cflag;
 	u32 param_new, param_mask, parity = 0;
 
@@ -441,40 +431,40 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 	param_new = s->conf & MAX3100_BAUD;
 	switch (baud) {
 	case 300:
-		if (s->crystal)
+		if (baud230400)
 			baud = s->baud;
 		else
 			param_new = 15;
 		break;
 	case 600:
-		param_new = 14 + s->crystal;
+		param_new = 14 + baud230400;
 		break;
 	case 1200:
-		param_new = 13 + s->crystal;
+		param_new = 13 + baud230400;
 		break;
 	case 2400:
-		param_new = 12 + s->crystal;
+		param_new = 12 + baud230400;
 		break;
 	case 4800:
-		param_new = 11 + s->crystal;
+		param_new = 11 + baud230400;
 		break;
 	case 9600:
-		param_new = 10 + s->crystal;
+		param_new = 10 + baud230400;
 		break;
 	case 19200:
-		param_new = 9 + s->crystal;
+		param_new = 9 + baud230400;
 		break;
 	case 38400:
-		param_new = 8 + s->crystal;
+		param_new = 8 + baud230400;
 		break;
 	case 57600:
-		param_new = 1 + s->crystal;
+		param_new = 1 + baud230400;
 		break;
 	case 115200:
-		param_new = 0 + s->crystal;
+		param_new = 0 + baud230400;
 		break;
 	case 230400:
-		if (s->crystal)
+		if (baud230400)
 			param_new = 0;
 		else
 			baud = s->baud;
@@ -577,7 +567,7 @@ static int max3100_startup(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	s->conf = MAX3100_RM;
-	s->baud = s->crystal ? 230400 : 115200;
+	s->baud = port->uartclk / 16;
 	s->rx_enabled = 1;
 
 	if (s->suspending)
@@ -720,8 +710,8 @@ static int uart_driver_registered;
 
 static int max3100_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	int i, retval;
-	struct plat_max3100 *pdata;
 	u16 rx;
 
 	mutex_lock(&max3100s_lock);
@@ -756,20 +746,21 @@ static int max3100_probe(struct spi_device *spi)
 	max3100s[i]->irq = spi->irq;
 	spin_lock_init(&max3100s[i]->conf_lock);
 	spi_set_drvdata(spi, max3100s[i]);
-	pdata = dev_get_platdata(&spi->dev);
-	max3100s[i]->crystal = pdata->crystal;
 	max3100s[i]->minor = i;
 	timer_setup(&max3100s[i]->timer, max3100_timeout, 0);
 
 	dev_dbg(&spi->dev, "%s: adding port %d\n", __func__, i);
 	max3100s[i]->port.irq = max3100s[i]->irq;
-	max3100s[i]->port.uartclk = max3100s[i]->crystal ? 3686400 : 1843200;
 	max3100s[i]->port.fifosize = 16;
 	max3100s[i]->port.ops = &max3100_ops;
 	max3100s[i]->port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
 	max3100s[i]->port.line = i;
 	max3100s[i]->port.type = PORT_MAX3100;
 	max3100s[i]->port.dev = &spi->dev;
+
+	/* Read clock frequency from a property, uart_add_one_port() will fail if it's not set */
+	device_property_read_u32(dev, "clock-frequency", &max3100s[i]->port.uartclk);
+
 	retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	if (retval < 0)
 		dev_warn(&spi->dev,
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 09/16] serial: max3100: Remove duplicating irq field

[Andy Shevchenko]

The struct uart_port has a copy of the IRQ that is also stored
in the private data structure. Remove the duplication in the latter
one.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index e5a1a9171047..e7963382bbb6 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -93,8 +93,6 @@ struct max3100_port {
 #define MAX3100_7BIT 4
 	int rx_enabled;	        /* if we should rx chars */
 
-	int irq;		/* irq assigned to the max3100 */
-
 	int minor;		/* minor number */
 	int loopback_commit;	/* need to change loopback */
 	int loopback;		/* 1 if we are in loopback mode */
@@ -550,8 +548,8 @@ static void max3100_shutdown(struct uart_port *port)
 		destroy_workqueue(s->workqueue);
 		s->workqueue = NULL;
 	}
-	if (s->irq)
-		free_irq(s->irq, s);
+	if (port->irq)
+		free_irq(port->irq, s);
 
 	/* set shutdown mode to save power */
 	max3100_sr(s, MAX3100_WC | MAX3100_SHDN, &rx);
@@ -563,6 +561,7 @@ static int max3100_startup(struct uart_port *port)
 					      struct max3100_port,
 					      port);
 	char b[12];
+	int ret;
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -585,10 +584,10 @@ static int max3100_startup(struct uart_port *port)
 	}
 	INIT_WORK(&s->work, max3100_work);
 
-	if (request_irq(s->irq, max3100_irq,
-			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
-		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
-		s->irq = 0;
+	ret = request_irq(port->irq, max3100_irq, IRQF_TRIGGER_FALLING, "max3100", s);
+	if (ret < 0) {
+		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", port->irq);
+		port->irq = 0;
 		destroy_workqueue(s->workqueue);
 		s->workqueue = NULL;
 		return -EBUSY;
@@ -743,14 +742,13 @@ static int max3100_probe(struct spi_device *spi)
 		return -ENOMEM;
 	}
 	max3100s[i]->spi = spi;
-	max3100s[i]->irq = spi->irq;
 	spin_lock_init(&max3100s[i]->conf_lock);
 	spi_set_drvdata(spi, max3100s[i]);
 	max3100s[i]->minor = i;
 	timer_setup(&max3100s[i]->timer, max3100_timeout, 0);
 
 	dev_dbg(&spi->dev, "%s: adding port %d\n", __func__, i);
-	max3100s[i]->port.irq = max3100s[i]->irq;
+	max3100s[i]->port.irq = spi->irq;
 	max3100s[i]->port.fifosize = 16;
 	max3100s[i]->port.ops = &max3100_ops;
 	max3100s[i]->port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF;
@@ -814,7 +812,7 @@ static int max3100_suspend(struct device *dev)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	disable_irq(s->irq);
+	disable_irq(s->port.irq);
 
 	s->suspending = 1;
 	uart_suspend_port(&max3100_uart_driver, &s->port);
@@ -833,7 +831,7 @@ static int max3100_resume(struct device *dev)
 	uart_resume_port(&max3100_uart_driver, &s->port);
 	s->suspending = 0;
 
-	enable_irq(s->irq);
+	enable_irq(s->port.irq);
 
 	s->conf_commit = 1;
 	if (s->workqueue)
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 10/16] serial: max3100: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index e7963382bbb6..c4e4dc5f0858 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -719,9 +719,8 @@ static int max3100_probe(struct spi_device *spi)
 		uart_driver_registered = 1;
 		retval = uart_register_driver(&max3100_uart_driver);
 		if (retval) {
-			printk(KERN_ERR "Couldn't register max3100 uart driver\n");
 			mutex_unlock(&max3100s_lock);
-			return retval;
+			return dev_err_probe(dev, retval, "Couldn't register max3100 uart driver\n");
 		}
 	}
 
@@ -729,15 +728,12 @@ static int max3100_probe(struct spi_device *spi)
 		if (!max3100s[i])
 			break;
 	if (i == MAX_MAX3100) {
-		dev_warn(&spi->dev, "too many MAX3100 chips\n");
 		mutex_unlock(&max3100s_lock);
-		return -ENOMEM;
+		return dev_err_probe(dev, -ENOMEM, "too many MAX3100 chips\n");
 	}
 
 	max3100s[i] = kzalloc(sizeof(struct max3100_port), GFP_KERNEL);
 	if (!max3100s[i]) {
-		dev_warn(&spi->dev,
-			 "kmalloc for max3100 structure %d failed!\n", i);
 		mutex_unlock(&max3100s_lock);
 		return -ENOMEM;
 	}
@@ -761,9 +757,7 @@ static int max3100_probe(struct spi_device *spi)
 
 	retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	if (retval < 0)
-		dev_warn(&spi->dev,
-			 "uart_add_one_port failed for line %d with error %d\n",
-			 i, retval);
+		dev_err_probe(dev, retval, "uart_add_one_port failed for line %d\n", i);
 
 	/* set shutdown mode to save power. Will be woken-up on open */
 	max3100_sr(max3100s[i], MAX3100_WC | MAX3100_SHDN, &rx);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 11/16] serial: max3100: Replace MODULE_ALIAS() with respective ID tables

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 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 c4e4dc5f0858..599665770b6e 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>
@@ -841,13 +842,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);
@@ -855,4 +870,3 @@ module_spi_driver(max3100_driver);
 MODULE_DESCRIPTION("MAX3100 driver");
 MODULE_AUTHOR("Christian Pellegrin <chripell@evolware.org>");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("spi:max3100");
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 12/16] serial: max3100: Switch to DEFINE_SIMPLE_DEV_PM_OPS()

[Andy Shevchenko]

The SIMPLE_DEV_PM_OPS() is deprecated, replace it with the
DEFINE_SIMPLE_DEV_PM_OPS() and use pm_sleep_ptr() for setting
the driver's PM routines.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 599665770b6e..585bf6c898b2 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -21,6 +21,7 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/pm.h>
 #include <linux/property.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
@@ -798,8 +799,6 @@ static void max3100_remove(struct spi_device *spi)
 	mutex_unlock(&max3100s_lock);
 }
 
-#ifdef CONFIG_PM_SLEEP
-
 static int max3100_suspend(struct device *dev)
 {
 	struct max3100_port *s = dev_get_drvdata(dev);
@@ -835,12 +834,7 @@ static int max3100_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(max3100_pm_ops, max3100_suspend, max3100_resume);
-#define MAX3100_PM_OPS (&max3100_pm_ops)
-
-#else
-#define MAX3100_PM_OPS NULL
-#endif
+static DEFINE_SIMPLE_DEV_PM_OPS(max3100_pm_ops, max3100_suspend, max3100_resume);
 
 static const struct spi_device_id max3100_spi_id[] = {
 	{ "max3100" },
@@ -858,7 +852,7 @@ static struct spi_driver max3100_driver = {
 	.driver = {
 		.name		= "max3100",
 		.of_match_table	= max3100_of_match,
-		.pm		= MAX3100_PM_OPS,
+		.pm		= pm_sleep_ptr(&max3100_pm_ops),
 	},
 	.probe		= max3100_probe,
 	.remove		= max3100_remove,
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 13/16] serial: max3100: Extract to_max3100_port() helper macro

[Andy Shevchenko]

Instead of using container_of() explicitly, introduce a heler macro.
This saves a lot of lines of code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 585bf6c898b2..19b05992a9ac 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 */
 
@@ -322,9 +325,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__);
@@ -332,9 +333,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__);
 
@@ -343,9 +342,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__);
 
@@ -359,9 +356,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__);
 
@@ -372,9 +367,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__);
 
@@ -386,9 +379,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__);
@@ -414,9 +405,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;
@@ -532,9 +521,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__);
@@ -559,9 +546,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;
 
@@ -607,9 +592,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__);
 
@@ -618,18 +601,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__);
 
@@ -640,9 +619,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__);
@@ -654,18 +631,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;
@@ -673,9 +646,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

[PATCH v1 14/16] serial: max3100: Remove unneeded forward declaration

[Andy Shevchenko]

There is no code using max3100_work() before the definition of it.
Remove unneeded forward declaration.

While at it, move max3100_dowork() and max3100_timeout() down in
the code to be after actual max3100_work() implementation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 19b05992a9ac..e63ac027b4f3 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -150,22 +150,6 @@ static void max3100_calc_parity(struct max3100_port *s, u16 *c)
 		*c |= max3100_do_parity(s, *c) << 8;
 }
 
-static void max3100_work(struct work_struct *w);
-
-static void max3100_dowork(struct max3100_port *s)
-{
-	if (!s->force_end_work && !freezing(current) && !s->suspending)
-		queue_work(s->workqueue, &s->work);
-}
-
-static void max3100_timeout(struct timer_list *t)
-{
-	struct max3100_port *s = from_timer(s, t, timer);
-
-	max3100_dowork(s);
-	mod_timer(&s->timer, jiffies + uart_poll_timeout(&s->port));
-}
-
 static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
 {
 	struct spi_message message;
@@ -313,6 +297,20 @@ static void max3100_work(struct work_struct *w)
 		tty_flip_buffer_push(&s->port.state->port);
 }
 
+static void max3100_dowork(struct max3100_port *s)
+{
+	if (!s->force_end_work && !freezing(current) && !s->suspending)
+		queue_work(s->workqueue, &s->work);
+}
+
+static void max3100_timeout(struct timer_list *t)
+{
+	struct max3100_port *s = from_timer(s, t, timer);
+
+	max3100_dowork(s);
+	mod_timer(&s->timer, jiffies + uart_poll_timeout(&s->port));
+}
+
 static irqreturn_t max3100_irq(int irqno, void *dev_id)
 {
 	struct max3100_port *s = dev_id;
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 15/16] serial: max3100: Sort headers

[Andy Shevchenko]

Sort the headers in alphabetic order in order to ease
the maintenance for this part.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/max3100.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index e63ac027b4f3..d7c0e274e00c 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -18,18 +18,18 @@
 
 #include <linux/container_of.h>
 #include <linux/delay.h>
-#include <linux/slab.h>
 #include <linux/device.h>
+#include <linux/freezer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/property.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
+#include <linux/slab.h>
 #include <linux/spi/spi.h>
-#include <linux/freezer.h>
-#include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/tty.h>
 #include <linux/types.h>
 
 #include <asm/unaligned.h>
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 16/16] serial: max3100: Update Kconfig entry

[Andy Shevchenko]

The driver actually supports more than one chip.
Update Kconfig entry to list the supported chips.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/Kconfig | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index e636a51eb7b6..dcb67c40bf92 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -307,11 +307,14 @@ config SERIAL_TEGRA_TCU_CONSOLE
 	  If unsure, say Y.
 
 config SERIAL_MAX3100
-	tristate "MAX3100 support"
+	tristate "MAX3100/3110/3111/3222 support"
 	depends on SPI
 	select SERIAL_CORE
 	help
-	  MAX3100 chip support
+	  This selects support for an advanced UART from Maxim.
+	  Supported ICs are MAX3100, MAX3110, MAX3111, MAX3222.
+
+	  Say Y here if you want to support these ICs.
 
 config SERIAL_MAX310X
 	tristate "MAX310X support"
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 01/16] serial: max3100: Lock port->lock when calling uart_handle_cts_change()

[Hugo Villeneuve]

On Tue,  2 Apr 2024 18:38:07 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

Hi Andy,

> uart_handle_cts_change() has to be called with port lock taken,
> Since we run it in a separate work, the lcok maybe not taken at

lcok -> lock

and possibly: "may not be taken" ?


> the time of running. Make sure that it's taken by explicitly doing
> that. Without it we got a splat:
> 
>   WARNING: CPU: 0 PID: 10 at drivers/tty/serial/serial_core.c:3491 uart_handle_cts_change+0xa6/0xb0
>   ...
>   Workqueue: max3100-0 max3100_work [max3100]
>   RIP: 0010:uart_handle_cts_change+0xa6/0xb0
>   ...
>    max3100_handlerx+0xc5/0x110 [max3100]
>    max3100_work+0x12a/0x340 [max3100]
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/max3100.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 5efb2b593be3..45022f2909f0 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -213,7 +213,7 @@ static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
>  	return 0;
>  }
>  
> -static int max3100_handlerx(struct max3100_port *s, u16 rx)
> +static int max3100_handlerx_unlocked(struct max3100_port *s, u16 rx)
>  {
>  	unsigned int status = 0;
>  	int ret = 0, cts;
> @@ -254,6 +254,17 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
>  	return ret;
>  }
>  
> +static int max3100_handlerx(struct max3100_port *s, u16 rx)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	uart_port_lock_irqsave(&s->port, &flags);
> +	ret = max3100_handlerx_unlocked(s, rx);
> +	uart_port_unlock_irqrestore(&s->port, flags);
> +	return ret;
> +}
> +
>  static void max3100_work(struct work_struct *w)
>  {
>  	struct max3100_port *s = container_of(w, struct max3100_port, work);
> -- 
> 2.43.0.rc1.1.gbec44491f096
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v1 01/16] serial: max3100: Lock port->lock when calling uart_handle_cts_change()

[Andy Shevchenko]

On Tue, Apr 02, 2024 at 01:07:15PM -0400, Hugo Villeneuve wrote:
> On Tue,  2 Apr 2024 18:38:07 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> Hi Andy,
> 
> > uart_handle_cts_change() has to be called with port lock taken,
> > Since we run it in a separate work, the lcok maybe not taken at
> 
> lcok -> lock
> 
> and possibly: "may not be taken" ?

Thanks, I'll fix this in case a new version is required.

> > the time of running. Make sure that it's taken by explicitly doing
> > that. Without it we got a splat:
> > 
> >   WARNING: CPU: 0 PID: 10 at drivers/tty/serial/serial_core.c:3491 uart_handle_cts_change+0xa6/0xb0
> >   ...
> >   Workqueue: max3100-0 max3100_work [max3100]
> >   RIP: 0010:uart_handle_cts_change+0xa6/0xb0
> >   ...
> >    max3100_handlerx+0xc5/0x110 [max3100]
> >    max3100_work+0x12a/0x340 [max3100]

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 02/16] serial: max3100: Update uart_driver_registered on driver removal

[Hugo Villeneuve]

On Tue,  2 Apr 2024 18:38:08 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

Hi Andy,

> The removal of the last MAX3100 device triggers the removal of
> the driver. However, code doesn't update the respective global
> variable and after insmod — rmmod — insmod cycle the kernel
> oopses:
> 
>   max3100 spi-PRP0001:01: max3100_probe: adding port 0
>   BUG: kernel NULL pointer dereference, address: 0000000000000408
>   ...
>   RIP: 0010:serial_core_register_port+0xa0/0x840
>   ...
>    max3100_probe+0x1b6/0x280 [max3100]
>    spi_probe+0x8d/0xb0
> 
> Update the actual state so next time UART driver will be registered
> again.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/max3100.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 45022f2909f0..efe26f6d5ebf 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -841,6 +841,7 @@ static void max3100_remove(struct spi_device *spi)
>  		}
>  	pr_debug("removing max3100 driver\n");
>  	uart_unregister_driver(&max3100_uart_driver);
> +	uart_driver_registered = 0;

At the beginning of the probe function, we have:

-----------------------
if (!uart_driver_registered) {
		uart_driver_registered = 1;
		retval = uart_register_driver(&max3100_uart_driver);
		if (retval) {
			printk(KERN_ERR "Couldn't register max3100 uart
driver\n"); mutex_unlock(&max3100s_lock);
			return retval;
...
-----------------------

If uart_register_driver() fails, uart_driver_registered would still be
true and would it prevent any other subsequent devices from being
properly registered? If yes, then maybe "uart_driver_registered = 1"
should be set only after a sucessfull call to uart_register_driver()?

Hugo.


>  
>  	mutex_unlock(&max3100s_lock);
>  }
> -- 
> 2.43.0.rc1.1.gbec44491f096
> 
> 

Re: [PATCH v1 02/16] serial: max3100: Update uart_driver_registered on driver removal

[Andy Shevchenko]

On Tue, Apr 02, 2024 at 01:18:27PM -0400, Hugo Villeneuve wrote:
> On Tue,  2 Apr 2024 18:38:08 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >  	pr_debug("removing max3100 driver\n");
> >  	uart_unregister_driver(&max3100_uart_driver);
> > +	uart_driver_registered = 0;
> 
> At the beginning of the probe function, we have:
> 
> -----------------------
> if (!uart_driver_registered) {
> 		uart_driver_registered = 1;
> 		retval = uart_register_driver(&max3100_uart_driver);
> 		if (retval) {
> 			printk(KERN_ERR "Couldn't register max3100 uart
> driver\n"); mutex_unlock(&max3100s_lock);
> 			return retval;
> ...
> -----------------------
> 
> If uart_register_driver() fails, uart_driver_registered would still be
> true and would it prevent any other subsequent devices from being
> properly registered? If yes, then maybe "uart_driver_registered = 1"
> should be set only after a sucessfull call to uart_register_driver()?

Looks like yet another issue here (however I haven't hit it so far).
I guess I can combine both fixes.  What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 13/16] serial: max3100: Extract to_max3100_port() helper macro

[Hugo Villeneuve]

On Tue,  2 Apr 2024 18:38:19 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Instead of using container_of() explicitly, introduce a heler macro.

heler -> helper

Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>


> This saves a lot of lines of code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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 585bf6c898b2..19b05992a9ac 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 */
>  
> @@ -322,9 +325,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__);
> @@ -332,9 +333,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__);
>  
> @@ -343,9 +342,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__);
>  
> @@ -359,9 +356,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__);
>  
> @@ -372,9 +367,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__);
>  
> @@ -386,9 +379,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__);
> @@ -414,9 +405,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;
> @@ -532,9 +521,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__);
> @@ -559,9 +546,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;
>  
> @@ -607,9 +592,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__);
>  
> @@ -618,18 +601,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__);
>  
> @@ -640,9 +619,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__);
> @@ -654,18 +631,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;
> @@ -673,9 +646,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
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v1 02/16] serial: max3100: Update uart_driver_registered on driver removal

[Hugo Villeneuve]

On Tue, 2 Apr 2024 20:31:50 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 02, 2024 at 01:18:27PM -0400, Hugo Villeneuve wrote:
> > On Tue,  2 Apr 2024 18:38:08 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
> > >  	pr_debug("removing max3100 driver\n");
> > >  	uart_unregister_driver(&max3100_uart_driver);
> > > +	uart_driver_registered = 0;
> > 
> > At the beginning of the probe function, we have:
> > 
> > -----------------------
> > if (!uart_driver_registered) {
> > 		uart_driver_registered = 1;
> > 		retval = uart_register_driver(&max3100_uart_driver);
> > 		if (retval) {
> > 			printk(KERN_ERR "Couldn't register max3100 uart
> > driver\n"); mutex_unlock(&max3100s_lock);
> > 			return retval;
> > ...
> > -----------------------
> > 
> > If uart_register_driver() fails, uart_driver_registered would still be
> > true and would it prevent any other subsequent devices from being
> > properly registered? If yes, then maybe "uart_driver_registered = 1"
> > should be set only after a sucessfull call to uart_register_driver()?
> 
> Looks like yet another issue here (however I haven't hit it so far).
> I guess I can combine both fixes.  What do you think?

Hi Andy,
makes sense to me.

Hugo.


> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 13/16] serial: max3100: Extract to_max3100_port() helper macro

[Andy Shevchenko]

On Tue, Apr 02, 2024 at 01:32:09PM -0400, Hugo Villeneuve wrote:
> On Tue,  2 Apr 2024 18:38:19 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Instead of using container_of() explicitly, introduce a heler macro.
> 
> heler -> helper

Seems v2 will be needed, I'll fix this.

> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Thank you!

-- 
With Best Regards,
Andy Shevchenko