MAX310X I2C support

PATCH In review Cosmin Tanislav 2 participants 6 messages Jun 5, 2022 – Jun 6, 2022 TTY layer and serial drivers

Share & tools

Cosmin Tanislav

author

2022-06-05 14:47 UTC #0 lore raw

v2 cover MAX310X I2C support

V1 -> V2:
* set_ext_reg_en -> extended_reg_enable
* move extended_reg_enable functions under guards
* move if_cfg structs under guards
* remove __maybe_unused from max310x_i2c_driver
* fix order of error labels in module register
* guard i2c register error label

Cosmin Tanislav (4):
serial: max310x: use regmap methods for SPI batch operations
serial: max310x: use a separate regmap for each port
serial: max310x: make accessing revision id interface-agnostic
serial: max310x: implement I2C support

 drivers/tty/serial/Kconfig   |   1 +
 drivers/tty/serial/max310x.c | 261 ++++++++++++++++++++++++++---------
 2 files changed, 200 insertions(+), 62 deletions(-)

Signature · 1 line

2.36.1

Cosmin Tanislav

author

2022-06-05 14:47 UTC #1 lore raw

PATCH v2 1/4 serial: max310x: use regmap methods for SPI batch operations

From: Cosmin Tanislav <[email protected]>

The SPI batch read/write operations can be implemented as simple regmap raw read and write, which will also try to do a gather write just as it is done here.

Use the regmap raw read and write methods.

Signed-off-by Cosmin Tanislav <[email protected]>

---
 drivers/tty/serial/max310x.c | 36 ++++++++----------------------------
 1 file changed, 8 insertions(+), 28 deletions(-)

drivers/tty/serial/max310x.c +8 ~~-28~~

index a0b6ea52d133..46887a4ffea4 100644
@@ -259,8 +259,6 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 
-	u8 wr_header;
-	u8 rd_header;
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
 #define to_max310x_port(_port) \

@@ -623,32 +621,18 @@ static u32 max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
 
 static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
 {
-	struct max310x_one *one = to_max310x_port(port);
-	struct spi_transfer xfer[] = {
-		{
-			.tx_buf = &one->wr_header,
-			.len = sizeof(one->wr_header),
-		}, {
-			.tx_buf = txbuf,
-			.len = len,
-		}
-	};
-	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
+	struct max310x_port *s = dev_get_drvdata(port->dev);
+	u8 reg = port->iobase + MAX310X_THR_REG;
+
+	regmap_raw_write(s->regmap, reg, txbuf, len);
 }
 
 static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
 {
-	struct max310x_one *one = to_max310x_port(port);
-	struct spi_transfer xfer[] = {
-		{
-			.tx_buf = &one->rd_header,
-			.len = sizeof(one->rd_header),
-		}, {
-			.rx_buf = rxbuf,
-			.len = len,
-		}
-	};
-	spi_sync_transfer(to_spi_device(port->dev), xfer, ARRAY_SIZE(xfer));
+	struct max310x_port *s = dev_get_drvdata(port->dev);
+	u8 reg = port->iobase + MAX310X_RHR_REG;
+
+	regmap_raw_read(s->regmap, reg, rxbuf, len);
 }
 
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)

@@ -1368,10 +1352,6 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
 		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
-		/* Initialize SPI-transfer buffers */
-		s->p[i].wr_header = (s->p[i].port.iobase + MAX310X_THR_REG) |
-				    MAX310X_WRITE_BIT;
-		s->p[i].rd_header = (s->p[i].port.iobase + MAX310X_RHR_REG);
 
 		/* Register port */
 		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);

Signature · 1 line

2.36.1

Cosmin Tanislav

author

2022-06-05 14:47 UTC #2 lore raw

PATCH v2 2/4 serial: max310x: use a separate regmap for each port

In reply to Cosmin Tanislav

From: Cosmin Tanislav <[email protected]>

The driver currently does manual register manipulation in multiple places to talk to a specific UART port.

In order to talk to a specific UART port over SPI, the bits U1 and U0 of the register address can be set, as explained in the Command byte configuration section of the datasheet.

Make this more elegant by creating regmaps for each UART port and setting the read_flag_mask and write_flag_mask accordingly.

All communcations regarding global registers are done on UART port 0, so replace the global regmap entirely with the port 0 regmap.

Also, remove the 0x1f masks from reg_writeable(), reg_volatile() and reg_precious() methods, since setting the U1 and U0 bits of the register address happens inside the regmap core now.

Signed-off-by Cosmin Tanislav <[email protected]>

---
 drivers/tty/serial/max310x.c | 68 +++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 32 deletions(-)

drivers/tty/serial/max310x.c +36 ~~-32~~

index 46887a4ffea4..6fd133c177a3 100644
@@ -258,6 +258,7 @@ struct max310x_one {
 	struct work_struct	tx_work;
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
+	struct regmap		*regmap;
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };

@@ -287,26 +288,26 @@ static DECLARE_BITMAP(max310x_lines, MAX310X_UART_NRMAX);
 
 static u8 max310x_port_read(struct uart_port *port, u8 reg)
 {
-	struct max310x_port *s = dev_get_drvdata(port->dev);
+	struct max310x_one *one = to_max310x_port(port);
 	unsigned int val = 0;
 
-	regmap_read(s->regmap, port->iobase + reg, &val);
+	regmap_read(one->regmap, reg, &val);
 
 	return val;
 }
 
 static void max310x_port_write(struct uart_port *port, u8 reg, u8 val)
 {
-	struct max310x_port *s = dev_get_drvdata(port->dev);
+	struct max310x_one *one = to_max310x_port(port);
 
-	regmap_write(s->regmap, port->iobase + reg, val);
+	regmap_write(one->regmap, reg, val);
 }
 
 static void max310x_port_update(struct uart_port *port, u8 reg, u8 mask, u8 val)
 {
-	struct max310x_port *s = dev_get_drvdata(port->dev);
+	struct max310x_one *one = to_max310x_port(port);
 
-	regmap_update_bits(s->regmap, port->iobase + reg, mask, val);
+	regmap_update_bits(one->regmap, reg, mask, val);
 }
 
 static int max3107_detect(struct device *dev)

@@ -445,7 +446,7 @@ static const struct max310x_devtype max14830_devtype = {
 
 static bool max310x_reg_writeable(struct device *dev, unsigned int reg)
 {
-	switch (reg & 0x1f) {
+	switch (reg) {
 	case MAX310X_IRQSTS_REG:
 	case MAX310X_LSR_IRQSTS_REG:
 	case MAX310X_SPCHR_IRQSTS_REG:

@@ -462,7 +463,7 @@ static bool max310x_reg_writeable(struct device *dev, unsigned int reg)
 
 static bool max310x_reg_volatile(struct device *dev, unsigned int reg)
 {
-	switch (reg & 0x1f) {
+	switch (reg) {
 	case MAX310X_RHR_REG:
 	case MAX310X_IRQSTS_REG:
 	case MAX310X_LSR_IRQSTS_REG:

@@ -484,7 +485,7 @@ static bool max310x_reg_volatile(struct device *dev, unsigned int reg)
 
 static bool max310x_reg_precious(struct device *dev, unsigned int reg)
 {
-	switch (reg & 0x1f) {
+	switch (reg) {
 	case MAX310X_RHR_REG:
 	case MAX310X_IRQSTS_REG:
 	case MAX310X_SPCHR_IRQSTS_REG:

@@ -621,18 +622,16 @@ static u32 max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
 
 static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
 {
-	struct max310x_port *s = dev_get_drvdata(port->dev);
-	u8 reg = port->iobase + MAX310X_THR_REG;
+	struct max310x_one *one = to_max310x_port(port);
 
-	regmap_raw_write(s->regmap, reg, txbuf, len);
+	regmap_raw_write(one->regmap, MAX310X_THR_REG, txbuf, len);
 }
 
 static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
 {
-	struct max310x_port *s = dev_get_drvdata(port->dev);
-	u8 reg = port->iobase + MAX310X_RHR_REG;
+	struct max310x_one *one = to_max310x_port(port);
 
-	regmap_raw_read(s->regmap, reg, rxbuf, len);
+	regmap_raw_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
 }
 
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)

@@ -1234,15 +1233,16 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 #endif
 
 static int max310x_probe(struct device *dev, const struct max310x_devtype *devtype,
-			 struct regmap *regmap, int irq)
+			 struct regmap *regmaps[], int irq)
 {
 	int i, ret, fmin, fmax, freq;
 	struct max310x_port *s;
 	u32 uartclk = 0;
 	bool xtal;
 
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
+	for (i = 0; i < devtype->nr; i++)
+		if (IS_ERR(regmaps[i]))
+			return PTR_ERR(regmaps[i]);
 
 	/* Alloc port structure */
 	s = devm_kzalloc(dev, struct_size(s, p, devtype->nr), GFP_KERNEL);

@@ -1289,7 +1289,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		goto out_clk;
 	}
 
-	s->regmap = regmap;
+	s->regmap = regmaps[0];
 	s->devtype = devtype;
 	dev_set_drvdata(dev, s);

@@ -1299,22 +1299,18 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		goto out_clk;
 
 	for (i = 0; i < devtype->nr; i++) {
-		unsigned int offs = i << 5;
-
 		/* Reset port */
-		regmap_write(s->regmap, MAX310X_MODE2_REG + offs,
+		regmap_write(regmaps[i], MAX310X_MODE2_REG,
 			     MAX310X_MODE2_RST_BIT);
 		/* Clear port reset */
-		regmap_write(s->regmap, MAX310X_MODE2_REG + offs, 0);
+		regmap_write(regmaps[i], MAX310X_MODE2_REG, 0);
 
 		/* Wait for port startup */
 		do {
-			regmap_read(s->regmap,
-				    MAX310X_BRGDIVLSB_REG + offs, &ret);
+			regmap_read(regmaps[i], MAX310X_BRGDIVLSB_REG, &ret);
 		} while (ret != 0x01);
 
-		regmap_write(s->regmap, MAX310X_MODE1_REG + offs,
-			     devtype->mode1);
+		regmap_write(regmaps[i], MAX310X_MODE1_REG, devtype->mode1);
 	}
 
 	uartclk = max310x_set_ref_clk(dev, s, freq, xtal);

@@ -1337,11 +1333,13 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		s->p[i].port.fifosize	= MAX310X_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
 		s->p[i].port.iotype	= UPIO_PORT;
-		s->p[i].port.iobase	= i * 0x20;
+		s->p[i].port.iobase	= i;
 		s->p[i].port.membase	= (void __iomem *)~0;
 		s->p[i].port.uartclk	= uartclk;
 		s->p[i].port.rs485_config = max310x_rs485_config;
 		s->p[i].port.ops	= &max310x_ops;
+		s->p[i].regmap		= regmaps[i];
+
 		/* Disable all interrupts */
 		max310x_port_write(&s->p[i].port, MAX310X_IRQEN_REG, 0);
 		/* Clear IRQ status register */

@@ -1436,6 +1434,7 @@ static struct regmap_config regcfg = {
 	.val_bits = 8,
 	.write_flag_mask = MAX310X_WRITE_BIT,
 	.cache_type = REGCACHE_RBTREE,
+	.max_register = MAX310X_REG_1F,
 	.writeable_reg = max310x_reg_writeable,
 	.volatile_reg = max310x_reg_volatile,
 	.precious_reg = max310x_reg_precious,

@@ -1445,7 +1444,8 @@ static struct regmap_config regcfg = {
 static int max310x_spi_probe(struct spi_device *spi)
 {
 	const struct max310x_devtype *devtype;
-	struct regmap *regmap;
+	struct regmap *regmaps[4];
+	unsigned int i;
 	int ret;
 
 	/* Setup SPI bus */

@@ -1460,10 +1460,14 @@ static int max310x_spi_probe(struct spi_device *spi)
 	if (!devtype)
 		devtype = (struct max310x_devtype *)spi_get_device_id(spi)->driver_data;
 
-	regcfg.max_register = devtype->nr * 0x20 - 1;
-	regmap = devm_regmap_init_spi(spi, &regcfg);
+	for (i = 0; i < devtype->nr; i++) {
+		u8 port_mask = i * 0x20;
+		regcfg.read_flag_mask = port_mask;
+		regcfg.write_flag_mask = port_mask | MAX310X_WRITE_BIT;
+		regmaps[i] = devm_regmap_init_spi(spi, &regcfg);
+	}
 
-	return max310x_probe(&spi->dev, devtype, regmap, spi->irq);
+	return max310x_probe(&spi->dev, devtype, regmaps, spi->irq);
 }
 
 static void max310x_spi_remove(struct spi_device *spi)

Signature · 1 line

2.36.1

Cosmin Tanislav

author

2022-06-05 14:47 UTC #3 lore raw

PATCH v2 3/4 serial: max310x: make accessing revision id interface-agnostic

In reply to Cosmin Tanislav

From: Cosmin Tanislav <[email protected]>

SPI can only use 5 address bits, since one bit is reserved for specifying R/W and 2 bits are used to specify the UART port. To access registers that have addresses past 0x1F, an extended register space can be enabled by writing to the GlobalCommand register (address 0x1F).

I2C uses 8 address bits. The R/W bit is placed in the slave address, and so is the UART port. Because of this, registers that have addresses higher than 0x1F can be accessed normally.

To access the RevID register, on SPI, 0xCE must be written to the 0x1F address to enable the extended register space, after which the RevID register is accessible at address 0x5. 0xCD must be written to the 0x1F address to disable the extended register space.

On I2C, the RevID register is accessible at address 0x25.

Create an interface config struct, and add a method for toggling the extended register space and a member for the RevId register address. Implement these for SPI.

Signed-off-by Cosmin Tanislav <[email protected]>

---
 drivers/tty/serial/max310x.c | 40 +++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 10 deletions(-)

drivers/tty/serial/max310x.c +30 ~~-10~~

index 6fd133c177a3..ef6b91242524 100644
@@ -72,7 +72,7 @@
 #define MAX310X_GLOBALCMD_REG		MAX310X_REG_1F /* Global Command (WO) */
 
 /* Extended registers */
-#define MAX310X_REVID_EXTREG		MAX310X_REG_05 /* Revision ID */
+#define MAX310X_SPI_REVID_EXTREG	MAX310X_REG_05 /* Revision ID */
 
 /* IRQ register bits */
 #define MAX310X_IRQ_LSR_BIT		(1 << 0) /* LSR interrupt */

@@ -245,6 +245,12 @@
 #define MAX14830_BRGCFG_CLKDIS_BIT	(1 << 6) /* Clock Disable */
 #define MAX14830_REV_ID			(0xb0)
 
+struct max310x_if_cfg {
+	int (*extended_reg_enable)(struct device *dev, bool enable);
+
+	unsigned int rev_id_reg;
+};
+
 struct max310x_devtype {
 	char	name[9];
 	int	nr;

@@ -267,6 +273,7 @@ struct max310x_one {
 
 struct max310x_port {
 	const struct max310x_devtype *devtype;
+	const struct max310x_if_cfg *if_cfg;
 	struct regmap		*regmap;
 	struct clk		*clk;
 #ifdef CONFIG_GPIOLIB

@@ -356,13 +363,12 @@ static int max3109_detect(struct device *dev)
 	unsigned int val = 0;
 	int ret;
 
-	ret = regmap_write(s->regmap, MAX310X_GLOBALCMD_REG,
-			   MAX310X_EXTREG_ENBL);
+	ret = s->if_cfg->extended_reg_enable(dev, true);
 	if (ret)
 		return ret;
 
-	regmap_read(s->regmap, MAX310X_REVID_EXTREG, &val);
-	regmap_write(s->regmap, MAX310X_GLOBALCMD_REG, MAX310X_EXTREG_DSBL);
+	regmap_read(s->regmap, s->if_cfg->rev_id_reg, &val);
+	s->if_cfg->extended_reg_enable(dev, false);
 	if (((val & MAX310x_REV_MASK) != MAX3109_REV_ID)) {
 		dev_err(dev,
 			"%s ID 0x%02x does not match\n", s->devtype->name, val);

@@ -387,13 +393,12 @@ static int max14830_detect(struct device *dev)
 	unsigned int val = 0;
 	int ret;
 
-	ret = regmap_write(s->regmap, MAX310X_GLOBALCMD_REG,
-			   MAX310X_EXTREG_ENBL);
+	ret = s->if_cfg->extended_reg_enable(dev, true);
 	if (ret)
 		return ret;
 	
-	regmap_read(s->regmap, MAX310X_REVID_EXTREG, &val);
-	regmap_write(s->regmap, MAX310X_GLOBALCMD_REG, MAX310X_EXTREG_DSBL);
+	regmap_read(s->regmap, s->if_cfg->rev_id_reg, &val);
+	s->if_cfg->extended_reg_enable(dev, false);
 	if (((val & MAX310x_REV_MASK) != MAX14830_REV_ID)) {
 		dev_err(dev,
 			"%s ID 0x%02x does not match\n", s->devtype->name, val);

@@ -1233,6 +1238,7 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 #endif
 
 static int max310x_probe(struct device *dev, const struct max310x_devtype *devtype,
+			 const struct max310x_if_cfg *if_cfg,
 			 struct regmap *regmaps[], int irq)
 {
 	int i, ret, fmin, fmax, freq;

@@ -1291,6 +1297,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 
 	s->regmap = regmaps[0];
 	s->devtype = devtype;
+	s->if_cfg = if_cfg;
 	dev_set_drvdata(dev, s);
 
 	/* Check device to ensure we are talking to what we expect */

@@ -1441,6 +1448,19 @@ static struct regmap_config regcfg = {
 };
 
 #ifdef CONFIG_SPI_MASTER
+static int max310x_spi_extended_reg_enable(struct device *dev, bool enable)
+{
+	struct max310x_port *s = dev_get_drvdata(dev);
+
+	return regmap_write(s->regmap, MAX310X_GLOBALCMD_REG,
+			    enable ? MAX310X_EXTREG_ENBL : MAX310X_EXTREG_DSBL);
+}
+
+static const struct max310x_if_cfg __maybe_unused max310x_spi_if_cfg = {
+	.extended_reg_enable = max310x_spi_extended_reg_enable,
+	.rev_id_reg = MAX310X_SPI_REVID_EXTREG,
+};
+
 static int max310x_spi_probe(struct spi_device *spi)
 {
 	const struct max310x_devtype *devtype;

@@ -1467,7 +1487,7 @@ static int max310x_spi_probe(struct spi_device *spi)
 		regmaps[i] = devm_regmap_init_spi(spi, &regcfg);
 	}
 
-	return max310x_probe(&spi->dev, devtype, regmaps, spi->irq);
+	return max310x_probe(&spi->dev, devtype, &max310x_spi_if_cfg, regmaps, spi->irq);
 }
 
 static void max310x_spi_remove(struct spi_device *spi)

Signature · 1 line

2.36.1

Cosmin Tanislav

author

2022-06-05 14:47 UTC #4 lore raw

PATCH v2 4/4 serial: max310x: implement I2C support

In reply to Cosmin Tanislav

From: Cosmin Tanislav <[email protected]>

I2C implementation on this chip has a few key differences compared to SPI, as described in previous patches.
* extended register space access needs no extra logic
* slave address is used to select which UART to communicate with

To accommodate these differences, add an I2C interface config, set the RevID register address and implement an empty method for setting the GlobalCommand register, since no special handling is needed for the extended register space.

To handle the port-specific slave address, create an I2C dummy device for each port, except the base one (UART0), which is expected to be the one specified in firmware, and create a regmap for each I2C device.
Add minimum and maximum slave addresses to each devtype for sanity checking.

Also, use a separate regmap config with no write_flag_mask, since I2C has a R/W bit in its slave address, and set the max register to the address of the RevID register, since the extended register space needs no extra logic.

Finally, add the I2C driver.

Signed-off-by Cosmin Tanislav <[email protected]>

---
 drivers/tty/serial/Kconfig   |   1 +
 drivers/tty/serial/max310x.c | 135 ++++++++++++++++++++++++++++++++++-
 2 files changed, 135 insertions(+), 1 deletion(-)

drivers/tty/serial/Kconfig +1 -0

index a452748c69b2..8a3ee1525d80 100644
@@ -324,6 +324,7 @@ config SERIAL_MAX310X
 	depends on SPI_MASTER
 	select SERIAL_CORE
 	select REGMAP_SPI if SPI_MASTER
+	select REGMAP_I2C if I2C
 	help
 	  This selects support for an advanced UART from Maxim (Dallas).
 	  Supported ICs are MAX3107, MAX3108, MAX3109, MAX14830.

drivers/tty/serial/max310x.c +134 -1

197 lines not on this page · raw message

Signature · 1 line

2.36.1

Andy Shevchenko

Reviewed-by on the v2 series

2022-06-06 14:25 UTC #5 lore raw

In reply to Cosmin Tanislav

Quoted 8 lines · Cosmin Tanislav

V1 -> V2:
* set_ext_reg_en -> extended_reg_enable
* move extended_reg_enable functions under guards
* move if_cfg structs under guards
* remove __maybe_unused from max310x_i2c_driver
* fix order of error labels in module register
* guard i2c register error label

LGTM, thanks!

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 13 lines

Cosmin Tanislav (4):
serial: max310x: use regmap methods for SPI batch operations
serial: max310x: use a separate regmap for each port
serial: max310x: make accessing revision id interface-agnostic
serial: max310x: implement I2C support
 drivers/tty/serial/Kconfig   |   1 +
 drivers/tty/serial/max310x.c | 261 ++++++++++++++++++++++++++---------
 2 files changed, 200 insertions(+), 62 deletions(-)
--
2.36.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko