From: Hugo Villeneuve <[email protected]>
Hello,
this patch series brings a few clean-ups and improvements to the max310x
driver.
Some of these changes are based on suggestions for the sc16is7xx driver by
Andy Shevchenko following this dicussion:
Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
The changes have been tested on a custom board using a max14830 in SPI
mode, with an external oscillator (not crystal). Tests included a simple
communication test with a GPS connected to UART0.
They also have been tested by using i2c-stub to simulate the four ports on a
virtual I2C max14830 device, but with obvious limitations as this cannot
simulate all the hardware functions.
Thank you.
Hugo Villeneuve (18):
serial: max310x: fix NULL pointer dereference in I2C instantiation
serial: max310x: add I2C device table for instantiation from userspace
serial: max310x: use i2c_get_match_data()
serial: max310x: use spi_get_device_match_data()
serial: max310x: fix syntax error in IRQ error message
serial: max310x: remove holes in struct max310x_devtype
serial: max310x: add macro for max number of ports
serial: max310x: use separate regmap name for each port
serial: max310x: simplify probe() and remove() error handling
serial: max310x: add explicit return for some switch default cases
serial: max310x: use dev_err_probe() instead of dev_err()
serial: max310x: replace hardcoded masks with preferred GENMASK()
serial: max310x: use common detect function for all variants
serial: max310x: use common power function for all variants
serial: max310x: replace ENOTSUPP with preferred EOPNOTSUPP
(checkpatch)
serial: max310x: replace bare use of 'unsigned' with 'unsigned int'
(checkpatch)
serial: max310x: reformat and improve comments
serial: max310x: fix indentation
drivers/tty/serial/max310x.c | 329 ++++++++++++++++++-----------------
1 file changed, 165 insertions(+), 164 deletions(-)
base-commit: 0c84bea0cabc4e2b98a3de88eeb4ff798931f056
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Simplify error handling and only call uart_remove_one_port() if line bit
is set, instead of having to manually set s->p[i].port.dev to NULL.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index d6219077d23c..9ef146f09d5b 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1395,10 +1395,9 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
/* Register port */
ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
- if (ret) {
- s->p[i].port.dev = NULL;
+ if (ret)
goto out_uart;
- }
+
set_bit(line, max310x_lines);
/* Go to suspend mode */
@@ -1433,10 +1432,8 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
out_uart:
for (i = 0; i < devtype->nr; i++) {
- if (s->p[i].port.dev) {
+ if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
uart_remove_one_port(&max310x_uart, &s->p[i].port);
- clear_bit(s->p[i].port.line, max310x_lines);
- }
}
out_clk:
@@ -1454,8 +1451,10 @@ static void max310x_remove(struct device *dev)
cancel_work_sync(&s->p[i].tx_work);
cancel_work_sync(&s->p[i].md_work);
cancel_work_sync(&s->p[i].rs_work);
- uart_remove_one_port(&max310x_uart, &s->p[i].port);
- clear_bit(s->p[i].port.line, max310x_lines);
+
+ if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
+ uart_remove_one_port(&max310x_uart, &s->p[i].port);
+
s->devtype->power(&s->p[i].port, 0);
}
--
2.39.2
From: Hugo Villeneuve <[email protected]>
When trying to instantiate a max14830 device from userspace:
echo max14830 0x60 > /sys/bus/i2c/devices/i2c-2/new_device
we get the following error:
Unable to handle kernel NULL pointer dereference at virtual address...
...
Call trace:
max310x_i2c_probe+0x48/0x170 [max310x]
i2c_device_probe+0x150/0x2a0
...
Add check for validity of devtype to prevent the error, and abort probe
with a meaningful error message.
Fixes: 2e1f2d9a9bdb ("serial: max310x: implement I2C support")
Cc: <[email protected]>
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index f3a99daebdaa..4a33fd950ed2 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1602,13 +1602,16 @@ static unsigned short max310x_i2c_slave_addr(unsigned short addr,
static int max310x_i2c_probe(struct i2c_client *client)
{
- const struct max310x_devtype *devtype =
- device_get_match_data(&client->dev);
+ const struct max310x_devtype *devtype;
struct i2c_client *port_client;
struct regmap *regmaps[4];
unsigned int i;
u8 port_addr;
+ devtype = device_get_match_data(&client->dev);
+ if (!devtype)
+ return dev_err_probe(&client->dev, -ENODEV, "Failed to match device\n");
+
if (client->addr < devtype->slave_addr.min ||
client->addr > devtype->slave_addr.max)
return dev_err_probe(&client->dev, -EINVAL,
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Use preferred spi_get_device_match_data() instead of
device_get_match_data() and spi_get_device_id() to get the driver match
data.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index a051bc773c4b..2314ec2afd3f 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1514,9 +1514,9 @@ static int max310x_spi_probe(struct spi_device *spi)
if (ret)
return ret;
- devtype = device_get_match_data(&spi->dev);
+ devtype = spi_get_device_match_data(spi);
if (!devtype)
- devtype = (struct max310x_devtype *)spi_get_device_id(spi)->driver_data;
+ return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n");
for (i = 0; i < devtype->nr; i++) {
u8 port_mask = i * 0x20;
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Replace dev_err() with dev_err_probe().
This helps in simplifing code and standardizing the error output.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 048ae432ba48..701bf54e4084 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1275,10 +1275,9 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
/* Alloc port structure */
s = devm_kzalloc(dev, struct_size(s, p, devtype->nr), GFP_KERNEL);
- if (!s) {
- dev_err(dev, "Error allocating port structure\n");
- return -ENOMEM;
- }
+ if (!s)
+ return dev_err_probe(dev, -ENOMEM,
+ "Error allocating port structure\n");
/* Always ask for fixed clock rate from a property. */
device_property_read_u32(dev, "clock-frequency", &uartclk);
@@ -1299,8 +1298,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
if (freq == 0)
freq = uartclk;
if (freq == 0) {
- dev_err(dev, "Cannot get clock rate\n");
- ret = -EINVAL;
+ ret = dev_err_probe(dev, -EINVAL, "Cannot get clock rate\n");
goto out_clk;
}
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Running pahole shows that there are some holes within the
max310x_devtype structure.
Remove holes and optimize alignment by reorganizing structure members.
This can also lead to data structure size reduction for some CPUs.
On 64-bit CPU (arm64):
Before:
/* size: 40, cachelines: 1, members: 6 */
/* sum members: 34, holes: 2, sum holes: 6 */
/* last cacheline: 40 bytes */
After:
/* size: 40, cachelines: 1, members: 6 */
/* padding: 6 */
/* last cacheline: 40 bytes */
On 32-bit CPU (i386):
Before:
/* size: 32, cachelines: 1, members: 6 */
/* sum members: 26, holes: 2, sum holes: 6 */
/* last cacheline: 32 bytes */
After:
/* size: 24, cachelines: 1, members: 8 */
/* padding: 2 */
/* last cacheline: 24 bytes */
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 27c8ec956691..21f2fa3a91e5 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -258,11 +258,11 @@ struct max310x_devtype {
unsigned short min;
unsigned short max;
} slave_addr;
- char name[9];
int nr;
- u8 mode1;
int (*detect)(struct device *);
void (*power)(struct uart_port *, int);
+ char name[9];
+ u8 mode1;
};
struct max310x_one {
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Allows to simplify code by removing the break statement in the default
switch/case in some functions.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 9ef146f09d5b..048ae432ba48 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -483,10 +483,8 @@ static bool max310x_reg_writeable(struct device *dev, unsigned int reg)
case MAX310X_RXFIFOLVL_REG:
return false;
default:
- break;
+ return true;
}
-
- return true;
}
static bool max310x_reg_volatile(struct device *dev, unsigned int reg)
@@ -505,10 +503,8 @@ static bool max310x_reg_volatile(struct device *dev, unsigned int reg)
case MAX310X_REG_1F:
return true;
default:
- break;
+ return false;
}
-
- return false;
}
static bool max310x_reg_precious(struct device *dev, unsigned int reg)
@@ -520,10 +516,8 @@ static bool max310x_reg_precious(struct device *dev, unsigned int reg)
case MAX310X_STS_IRQSTS_REG:
return true;
default:
- break;
+ return false;
}
-
- return false;
}
static bool max310x_reg_noinc(struct device *dev, unsigned int reg)
--
2.39.2
From: Hugo Villeneuve <[email protected]>
GENMASK() is preferred when defining bitmasks.
Of all the masks changed, only MAX310x_REV_MASK is actually used.
No functional change.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 701bf54e4084..c93b326faf89 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -161,14 +161,14 @@
#define MAX310X_IRDA_SIR_BIT (1 << 1) /* SIR mode enable */
/* Flow control trigger level register masks */
-#define MAX310X_FLOWLVL_HALT_MASK (0x000f) /* Flow control halt level */
-#define MAX310X_FLOWLVL_RES_MASK (0x00f0) /* Flow control resume level */
+#define MAX310X_FLOWLVL_HALT_MASK GENMASK(3, 0) /* Flow control halt level */
+#define MAX310X_FLOWLVL_RES_MASK GENMASK(7, 4) /* Flow control resume level */
#define MAX310X_FLOWLVL_HALT(words) ((words / 8) & 0x0f)
#define MAX310X_FLOWLVL_RES(words) (((words / 8) & 0x0f) << 4)
/* FIFO interrupt trigger level register masks */
-#define MAX310X_FIFOTRIGLVL_TX_MASK (0x0f) /* TX FIFO trigger level */
-#define MAX310X_FIFOTRIGLVL_RX_MASK (0xf0) /* RX FIFO trigger level */
+#define MAX310X_FIFOTRIGLVL_TX_MASK GENMASK(3, 0) /* TX FIFO trigger level */
+#define MAX310X_FIFOTRIGLVL_RX_MASK GENMASK(7, 4) /* RX FIFO trigger level */
#define MAX310X_FIFOTRIGLVL_TX(words) ((words / 8) & 0x0f)
#define MAX310X_FIFOTRIGLVL_RX(words) (((words / 8) & 0x0f) << 4)
@@ -215,8 +215,8 @@
*/
/* PLL configuration register masks */
-#define MAX310X_PLLCFG_PREDIV_MASK (0x3f) /* PLL predivision value */
-#define MAX310X_PLLCFG_PLLFACTOR_MASK (0xc0) /* PLL multiplication factor */
+#define MAX310X_PLLCFG_PREDIV_MASK GENMASK(5, 0) /* PLL predivision value */
+#define MAX310X_PLLCFG_PLLFACTOR_MASK GENMASK(7, 6) /* PLL multiplication factor */
/* Baud rate generator configuration register bits */
#define MAX310X_BRGCFG_2XMODE_BIT (1 << 4) /* Double baud rate */
@@ -235,7 +235,7 @@
/* Misc definitions */
#define MAX310X_FIFO_SIZE (128)
-#define MAX310x_REV_MASK (0xf8)
+#define MAX310x_REV_MASK GENMASK(7, 3)
#define MAX310X_WRITE_BIT 0x80
/* MAX3107 specific */
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Simplify driver by defining a common function to handle the detection
of all variants.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 134 ++++++++++++++---------------------
1 file changed, 54 insertions(+), 80 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index c93b326faf89..83beaab3a0c5 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -67,6 +67,7 @@
#define MAX310X_BRGDIVMSB_REG (0x1d) /* Baud rate divisor MSB */
#define MAX310X_CLKSRC_REG (0x1e) /* Clock source */
#define MAX310X_REG_1F (0x1f)
+#define MAX310X_EXTREG_START (0x20) /* Only relevant in SPI mode. */
#define MAX310X_REVID_REG MAX310X_REG_1F /* Revision ID */
@@ -74,9 +75,9 @@
#define MAX310X_GLOBALCMD_REG MAX310X_REG_1F /* Global Command (WO) */
/* Extended registers */
-#define MAX310X_SPI_REVID_EXTREG MAX310X_REG_05 /* Revision ID */
-#define MAX310X_I2C_REVID_EXTREG (0x25) /* Revision ID */
-
+#define MAX310X_REVID_EXTREG (0x25) /* Revision ID
+ * (extended addressing space)
+ */
/* IRQ register bits */
#define MAX310X_IRQ_LSR_BIT (1 << 0) /* LSR interrupt */
#define MAX310X_IRQ_SPCHR_BIT (1 << 1) /* Special char interrupt */
@@ -250,8 +251,7 @@
struct max310x_if_cfg {
int (*extended_reg_enable)(struct device *dev, bool enable);
-
- unsigned int rev_id_reg;
+ u8 rev_id_offset;
};
struct max310x_devtype {
@@ -260,10 +260,11 @@ struct max310x_devtype {
unsigned short max;
} slave_addr;
int nr;
- int (*detect)(struct device *);
void (*power)(struct uart_port *, int);
char name[9];
u8 mode1;
+ u8 rev_id_val;
+ u8 rev_id_reg; /* Relevant only if rev_id_val is defined. */
};
struct max310x_one {
@@ -324,62 +325,52 @@ static void max310x_port_update(struct uart_port *port, u8 reg, u8 mask, u8 val)
regmap_update_bits(one->regmap, reg, mask, val);
}
-static int max3107_detect(struct device *dev)
+static int max310x_detect(struct device *dev)
{
struct max310x_port *s = dev_get_drvdata(dev);
unsigned int val = 0;
int ret;
- ret = regmap_read(s->regmap, MAX310X_REVID_REG, &val);
- if (ret)
- return ret;
+ /* Check if variant supports REV ID register: */
+ if (s->devtype->rev_id_val) {
+ u8 rev_id_reg = s->devtype->rev_id_reg;
- if (((val & MAX310x_REV_MASK) != MAX3107_REV_ID)) {
- dev_err(dev,
- "%s ID 0x%02x does not match\n", s->devtype->name, val);
- return -ENODEV;
- }
+ /* Check if REV ID is in extended addressing space: */
+ if (s->devtype->rev_id_reg >= MAX310X_EXTREG_START) {
+ ret = s->if_cfg->extended_reg_enable(dev, true);
+ if (ret)
+ return ret;
- return 0;
-}
+ /* Adjust REV ID extended addressing space address: */
+ if (s->if_cfg->rev_id_offset)
+ rev_id_reg -= s->if_cfg->rev_id_offset;
+ }
-static int max3108_detect(struct device *dev)
-{
- struct max310x_port *s = dev_get_drvdata(dev);
- unsigned int val = 0;
- int ret;
+ regmap_read(s->regmap, rev_id_reg, &val);
- /* MAX3108 have not REV ID register, we just check default value
- * from clocksource register to make sure everything works.
- */
- ret = regmap_read(s->regmap, MAX310X_CLKSRC_REG, &val);
- if (ret)
- return ret;
+ if (s->devtype->rev_id_reg >= MAX310X_EXTREG_START) {
+ ret = s->if_cfg->extended_reg_enable(dev, false);
+ if (ret)
+ return ret;
+ }
- if (val != (MAX310X_CLKSRC_EXTCLK_BIT | MAX310X_CLKSRC_PLLBYP_BIT)) {
- dev_err(dev, "%s not present\n", s->devtype->name);
- return -ENODEV;
- }
+ if (((val & MAX310x_REV_MASK) != s->devtype->rev_id_val))
+ return dev_err_probe(dev, -ENODEV,
+ "%s ID 0x%02x does not match\n",
+ s->devtype->name, val);
+ } else {
+ /*
+ * For variant without REV ID register, just check default value
+ * from clocksource register to make sure everything works.
+ */
+ ret = regmap_read(s->regmap, MAX310X_CLKSRC_REG, &val);
+ if (ret)
+ return ret;
- return 0;
-}
-
-static int max3109_detect(struct device *dev)
-{
- struct max310x_port *s = dev_get_drvdata(dev);
- unsigned int val = 0;
- int ret;
-
- ret = s->if_cfg->extended_reg_enable(dev, true);
- if (ret)
- return ret;
-
- 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);
- return -ENODEV;
+ if (val != (MAX310X_CLKSRC_EXTCLK_BIT | MAX310X_CLKSRC_PLLBYP_BIT))
+ return dev_err_probe(dev, -ENODEV,
+ "%s not present\n",
+ s->devtype->name);
}
return 0;
@@ -394,27 +385,6 @@ static void max310x_power(struct uart_port *port, int on)
msleep(50);
}
-static int max14830_detect(struct device *dev)
-{
- struct max310x_port *s = dev_get_drvdata(dev);
- unsigned int val = 0;
- int ret;
-
- ret = s->if_cfg->extended_reg_enable(dev, true);
- if (ret)
- return ret;
-
- 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);
- return -ENODEV;
- }
-
- return 0;
-}
-
static void max14830_power(struct uart_port *port, int on)
{
max310x_port_update(port, MAX310X_BRGCFG_REG,
@@ -428,7 +398,8 @@ static const struct max310x_devtype max3107_devtype = {
.name = "MAX3107",
.nr = 1,
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT | MAX310X_MODE1_IRQSEL_BIT,
- .detect = max3107_detect,
+ .rev_id_val = MAX3107_REV_ID,
+ .rev_id_reg = MAX310X_REVID_REG,
.power = max310x_power,
.slave_addr = {
.min = 0x2c,
@@ -440,7 +411,8 @@ static const struct max310x_devtype max3108_devtype = {
.name = "MAX3108",
.nr = 1,
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT,
- .detect = max3108_detect,
+ .rev_id_val = 0, /* Unsupported. */
+ .rev_id_reg = 0, /* Irrelevant when rev_id_val is not defined. */
.power = max310x_power,
.slave_addr = {
.min = 0x60,
@@ -452,7 +424,8 @@ static const struct max310x_devtype max3109_devtype = {
.name = "MAX3109",
.nr = 2,
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT,
- .detect = max3109_detect,
+ .rev_id_val = MAX3109_REV_ID,
+ .rev_id_reg = MAX310X_REVID_EXTREG,
.power = max310x_power,
.slave_addr = {
.min = 0x60,
@@ -464,7 +437,8 @@ static const struct max310x_devtype max14830_devtype = {
.name = "MAX14830",
.nr = 4,
.mode1 = MAX310X_MODE1_IRQSEL_BIT,
- .detect = max14830_detect,
+ .rev_id_val = MAX14830_REV_ID,
+ .rev_id_reg = MAX310X_REVID_EXTREG,
.power = max14830_power,
.slave_addr = {
.min = 0x60,
@@ -1322,7 +1296,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
dev_set_drvdata(dev, s);
/* Check device to ensure we are talking to what we expect */
- ret = devtype->detect(dev);
+ ret = max310x_detect(dev);
if (ret)
goto out_clk;
@@ -1501,7 +1475,7 @@ static int max310x_spi_extended_reg_enable(struct device *dev, bool enable)
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,
+ .rev_id_offset = MAX310X_EXTREG_START,
};
static int max310x_spi_probe(struct spi_device *spi)
@@ -1574,7 +1548,7 @@ static struct regmap_config regcfg_i2c = {
.writeable_reg = max310x_reg_writeable,
.volatile_reg = max310x_reg_volatile,
.precious_reg = max310x_reg_precious,
- .max_register = MAX310X_I2C_REVID_EXTREG,
+ .max_register = MAX310X_REVID_EXTREG,
.writeable_noinc_reg = max310x_reg_noinc,
.readable_noinc_reg = max310x_reg_noinc,
.max_raw_read = MAX310X_FIFO_SIZE,
@@ -1583,7 +1557,7 @@ static struct regmap_config regcfg_i2c = {
static const struct max310x_if_cfg max310x_i2c_if_cfg = {
.extended_reg_enable = max310x_i2c_extended_reg_enable,
- .rev_id_reg = MAX310X_I2C_REVID_EXTREG,
+ .rev_id_offset = 0, /* No offset in I2C mode. */
};
static unsigned short max310x_i2c_slave_addr(unsigned short addr,
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Fixes the following checkpatch warning:
WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
According to include/linux/errno.h, ENOTSUPP is
"Defined for the NFSv3 protocol", so replace it with preferred EOPNOTSUPP.
Similar to commit c7581a414d28 ("drm: Use EOPNOTSUPP, not ENOTSUPP").
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index e39d8ea51e4e..12219b22b880 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1217,7 +1217,7 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
1 << ((offset % 4) + 4), 0);
return 0;
default:
- return -ENOTSUPP;
+ return -EOPNOTSUPP;
}
}
#endif
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Use preferred i2c_get_match_data() instead of device_get_match_data()
to get the driver match data.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 053cf2458264..a051bc773c4b 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1608,7 +1608,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
unsigned int i;
u8 port_addr;
- devtype = device_get_match_data(&client->dev);
+ devtype = i2c_get_match_data(client);
if (!devtype)
return dev_err_probe(&client->dev, -ENODEV, "Failed to match device\n");
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Simplify driver by defining a common function to handle the power
control of all variants.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 44 ++++++++++++++++--------------------
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 83beaab3a0c5..e39d8ea51e4e 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -260,11 +260,12 @@ struct max310x_devtype {
unsigned short max;
} slave_addr;
int nr;
- void (*power)(struct uart_port *, int);
char name[9];
u8 mode1;
u8 rev_id_val;
u8 rev_id_reg; /* Relevant only if rev_id_val is defined. */
+ u8 power_reg; /* Register address for power/sleep control. */
+ u8 power_bit; /* Bit for sleep or power-off mode (active high). */
};
struct max310x_one {
@@ -378,18 +379,10 @@ static int max310x_detect(struct device *dev)
static void max310x_power(struct uart_port *port, int on)
{
- max310x_port_update(port, MAX310X_MODE1_REG,
- MAX310X_MODE1_FORCESLEEP_BIT,
- on ? 0 : MAX310X_MODE1_FORCESLEEP_BIT);
- if (on)
- msleep(50);
-}
+ struct max310x_port *s = dev_get_drvdata(port->dev);
-static void max14830_power(struct uart_port *port, int on)
-{
- max310x_port_update(port, MAX310X_BRGCFG_REG,
- MAX14830_BRGCFG_CLKDIS_BIT,
- on ? 0 : MAX14830_BRGCFG_CLKDIS_BIT);
+ max310x_port_update(port, s->devtype->power_reg, s->devtype->power_bit,
+ on ? 0 : s->devtype->power_bit);
if (on)
msleep(50);
}
@@ -400,7 +393,8 @@ static const struct max310x_devtype max3107_devtype = {
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT | MAX310X_MODE1_IRQSEL_BIT,
.rev_id_val = MAX3107_REV_ID,
.rev_id_reg = MAX310X_REVID_REG,
- .power = max310x_power,
+ .power_reg = MAX310X_MODE1_REG,
+ .power_bit = MAX310X_MODE1_FORCESLEEP_BIT,
.slave_addr = {
.min = 0x2c,
.max = 0x2f,
@@ -413,7 +407,8 @@ static const struct max310x_devtype max3108_devtype = {
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT,
.rev_id_val = 0, /* Unsupported. */
.rev_id_reg = 0, /* Irrelevant when rev_id_val is not defined. */
- .power = max310x_power,
+ .power_reg = MAX310X_MODE1_REG,
+ .power_bit = MAX310X_MODE1_FORCESLEEP_BIT,
.slave_addr = {
.min = 0x60,
.max = 0x6f,
@@ -426,7 +421,8 @@ static const struct max310x_devtype max3109_devtype = {
.mode1 = MAX310X_MODE1_AUTOSLEEP_BIT,
.rev_id_val = MAX3109_REV_ID,
.rev_id_reg = MAX310X_REVID_EXTREG,
- .power = max310x_power,
+ .power_reg = MAX310X_MODE1_REG,
+ .power_bit = MAX310X_MODE1_FORCESLEEP_BIT,
.slave_addr = {
.min = 0x60,
.max = 0x6f,
@@ -439,7 +435,8 @@ static const struct max310x_devtype max14830_devtype = {
.mode1 = MAX310X_MODE1_IRQSEL_BIT,
.rev_id_val = MAX14830_REV_ID,
.rev_id_reg = MAX310X_REVID_EXTREG,
- .power = max14830_power,
+ .power_reg = MAX310X_BRGCFG_REG,
+ .power_bit = MAX14830_BRGCFG_CLKDIS_BIT,
.slave_addr = {
.min = 0x60,
.max = 0x6f,
@@ -1025,10 +1022,9 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
static int max310x_startup(struct uart_port *port)
{
- struct max310x_port *s = dev_get_drvdata(port->dev);
unsigned int val;
- s->devtype->power(port, 1);
+ max310x_power(port, 1);
/* Configure MODE1 register */
max310x_port_update(port, MAX310X_MODE1_REG,
@@ -1073,12 +1069,10 @@ static int max310x_startup(struct uart_port *port)
static void max310x_shutdown(struct uart_port *port)
{
- struct max310x_port *s = dev_get_drvdata(port->dev);
-
/* Disable all interrupts */
max310x_port_write(port, MAX310X_IRQEN_REG, 0);
- s->devtype->power(port, 0);
+ max310x_power(port, 0);
}
static const char *max310x_type(struct uart_port *port)
@@ -1140,7 +1134,7 @@ static int __maybe_unused max310x_suspend(struct device *dev)
for (i = 0; i < s->devtype->nr; i++) {
uart_suspend_port(&max310x_uart, &s->p[i].port);
- s->devtype->power(&s->p[i].port, 0);
+ max310x_power(&s->p[i].port, 0);
}
return 0;
@@ -1152,7 +1146,7 @@ static int __maybe_unused max310x_resume(struct device *dev)
int i;
for (i = 0; i < s->devtype->nr; i++) {
- s->devtype->power(&s->p[i].port, 1);
+ max310x_power(&s->p[i].port, 1);
uart_resume_port(&max310x_uart, &s->p[i].port);
}
@@ -1367,7 +1361,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
set_bit(line, max310x_lines);
/* Go to suspend mode */
- devtype->power(&s->p[i].port, 0);
+ max310x_power(&s->p[i].port, 0);
}
#ifdef CONFIG_GPIOLIB
@@ -1421,7 +1415,7 @@ static void max310x_remove(struct device *dev)
if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
uart_remove_one_port(&max310x_uart, &s->p[i].port);
- s->devtype->power(&s->p[i].port, 0);
+ max310x_power(&s->p[i].port, 0);
}
clk_disable_unprepare(s->clk);
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Fixes the following checkpatch warnings:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
With this change, the affected functions now match the prototypes in
struct gpio_chip.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 12219b22b880..6ef0155074dd 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1156,7 +1156,7 @@ static int __maybe_unused max310x_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(max310x_pm_ops, max310x_suspend, max310x_resume);
#ifdef CONFIG_GPIOLIB
-static int max310x_gpio_get(struct gpio_chip *chip, unsigned offset)
+static int max310x_gpio_get(struct gpio_chip *chip, unsigned int offset)
{
unsigned int val;
struct max310x_port *s = gpiochip_get_data(chip);
@@ -1167,7 +1167,7 @@ static int max310x_gpio_get(struct gpio_chip *chip, unsigned offset)
return !!((val >> 4) & (1 << (offset % 4)));
}
-static void max310x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+static void max310x_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
{
struct max310x_port *s = gpiochip_get_data(chip);
struct uart_port *port = &s->p[offset / 4].port;
@@ -1176,7 +1176,7 @@ static void max310x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
value ? 1 << (offset % 4) : 0);
}
-static int max310x_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+static int max310x_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
{
struct max310x_port *s = gpiochip_get_data(chip);
struct uart_port *port = &s->p[offset / 4].port;
@@ -1187,7 +1187,7 @@ static int max310x_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
}
static int max310x_gpio_direction_output(struct gpio_chip *chip,
- unsigned offset, int value)
+ unsigned int offset, int value)
{
struct max310x_port *s = gpiochip_get_data(chip);
struct uart_port *port = &s->p[offset / 4].port;
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Fix indentation and add line after do/while() block.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index c7849b92abb0..afe882146639 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -802,6 +802,7 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
if (ists & MAX310X_IRQ_TXEMPTY_BIT)
max310x_start_tx(port);
} while (1);
+
return res;
}
@@ -1598,7 +1599,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
return dev_err_probe(&client->dev, -ENODEV, "Failed to match device\n");
if (client->addr < devtype->slave_addr.min ||
- client->addr > devtype->slave_addr.max)
+ client->addr > devtype->slave_addr.max)
return dev_err_probe(&client->dev, -EINVAL,
"Slave addr 0x%x outside of range [0x%x, 0x%x]\n",
client->addr, devtype->slave_addr.min,
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Add comments about I2C slave address structure, and reformat to
improve readability.
Also reformat some comments according to kernel coding style.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 40 ++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 6ef0155074dd..c7849b92abb0 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -179,7 +179,8 @@
#define MAX310X_FLOWCTRL_GPIADDR_BIT (1 << 2) /* Enables that GPIO inputs
* are used in conjunction with
* XOFF2 for definition of
- * special character */
+ * special character
+ */
#define MAX310X_FLOWCTRL_SWFLOWEN_BIT (1 << 3) /* Auto SW flow ctrl enable */
#define MAX310X_FLOWCTRL_SWFLOW0_BIT (1 << 4) /* SWFLOW bit 0 */
#define MAX310X_FLOWCTRL_SWFLOW1_BIT (1 << 5) /* SWFLOW bit 1
@@ -258,7 +259,7 @@ struct max310x_devtype {
struct {
unsigned short min;
unsigned short max;
- } slave_addr;
+ } slave_addr; /* Relevant only in I2C mode. */
int nr;
char name[9];
u8 mode1;
@@ -639,7 +640,8 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
u8 ch, flag;
if (port->read_status_mask == MAX310X_LSR_RXOVR_BIT) {
- /* We are just reading, happily ignoring any error conditions.
+ /*
+ * We are just reading, happily ignoring any error conditions.
* Break condition, parity checking, framing errors -- they
* are all ignored. That means that we can do a batch-read.
*
@@ -648,7 +650,7 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
* that the LSR register applies to the "current" character.
* That's also the reason why we cannot do batched reads when
* asked to check the individual statuses.
- * */
+ */
sts = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG);
max310x_batch_read(port, one->rx_buf, rxlen);
@@ -752,8 +754,10 @@ static void max310x_handle_tx(struct uart_port *port)
to_send = (to_send > txlen) ? txlen : to_send;
if (until_end < to_send) {
- /* It's a circ buffer -- wrap around.
- * We could do that in one SPI transaction, but meh. */
+ /*
+ * It's a circ buffer -- wrap around.
+ * We could do that in one SPI transaction, but meh.
+ */
max310x_batch_write(port, xmit->buf + xmit->tail, until_end);
max310x_batch_write(port, xmit->buf, to_send - until_end);
} else {
@@ -842,7 +846,8 @@ static unsigned int max310x_tx_empty(struct uart_port *port)
static unsigned int max310x_get_mctrl(struct uart_port *port)
{
- /* DCD and DSR are not wired and CTS/RTS is handled automatically
+ /*
+ * DCD and DSR are not wired and CTS/RTS is handled automatically
* so just indicate DSR and CAR asserted
*/
return TIOCM_DSR | TIOCM_CAR;
@@ -934,7 +939,8 @@ static void max310x_set_termios(struct uart_port *port,
max310x_port_write(port, MAX310X_XON1_REG, termios->c_cc[VSTART]);
max310x_port_write(port, MAX310X_XOFF1_REG, termios->c_cc[VSTOP]);
- /* Disable transmitter before enabling AutoCTS or auto transmitter
+ /*
+ * Disable transmitter before enabling AutoCTS or auto transmitter
* flow control
*/
if (termios->c_cflag & CRTSCTS || termios->c_iflag & IXOFF) {
@@ -961,7 +967,8 @@ static void max310x_set_termios(struct uart_port *port,
}
max310x_port_write(port, MAX310X_FLOWCTRL_REG, flow);
- /* Enable transmitter after disabling AutoCTS and auto transmitter
+ /*
+ * Enable transmitter after disabling AutoCTS and auto transmitter
* flow control
*/
if (!(termios->c_cflag & CRTSCTS) && !(termios->c_iflag & IXOFF)) {
@@ -1052,8 +1059,11 @@ static int max310x_startup(struct uart_port *port)
MAX310X_MODE2_ECHOSUPR_BIT);
}
- /* Configure flow control levels */
- /* Flow control halt level 96, resume level 48 */
+ /*
+ * Configure flow control levels:
+ * resume: 48
+ * halt: 96
+ */
max310x_port_write(port, MAX310X_FLOWLVL_REG,
MAX310X_FLOWLVL_RES(48) | MAX310X_FLOWLVL_HALT(96));
@@ -1561,10 +1571,10 @@ static unsigned short max310x_i2c_slave_addr(unsigned short addr,
* For MAX14830 and MAX3109, the slave address depends on what the
* A0 and A1 pins are tied to.
* See Table I2C Address Map of the datasheet.
- * Based on that table, the following formulas were determined.
- * UART1 - UART0 = 0x10
- * UART2 - UART1 = 0x20 + 0x10
- * UART3 - UART2 = 0x10
+ * Based on that table, the following formulas were determined:
+ * UART1 - UART0 = 0x10
+ * UART2 - UART1 = 0x20 + 0x10
+ * UART3 - UART2 = 0x10
*/
addr -= nr * 0x10;
--
2.39.2
From: Hugo Villeneuve <[email protected]>
This allows to instantiate a max14830 I2C device from userspace.
Helpful when testing driver with i2c-stub.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 4a33fd950ed2..053cf2458264 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1639,6 +1639,15 @@ static void max310x_i2c_remove(struct i2c_client *client)
max310x_remove(&client->dev);
}
+static const struct i2c_device_id max310x_i2c_id_table[] = {
+ { "max3107", (kernel_ulong_t)&max3107_devtype, },
+ { "max3108", (kernel_ulong_t)&max3108_devtype, },
+ { "max3109", (kernel_ulong_t)&max3109_devtype, },
+ { "max14830", (kernel_ulong_t)&max14830_devtype, },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max310x_i2c_id_table);
+
static struct i2c_driver max310x_i2c_driver = {
.driver = {
.name = MAX310X_NAME,
@@ -1647,6 +1656,7 @@ static struct i2c_driver max310x_i2c_driver = {
},
.probe = max310x_i2c_probe,
.remove = max310x_i2c_remove,
+ .id_table = max310x_i2c_id_table,
};
#endif
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Replace g with q.
Helpful when grepping thru source code or logs for
"request" keyword.
Fixes: f65444187a66 ("serial: New serial driver MAX310X")
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 2314ec2afd3f..27c8ec956691 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1428,7 +1428,7 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
if (!ret)
return 0;
- dev_err(dev, "Unable to reguest IRQ %i\n", irq);
+ dev_err(dev, "Unable to request IRQ %i\n", irq);
out_uart:
for (i = 0; i < devtype->nr; i++) {
--
2.39.2
From: Hugo Villeneuve <[email protected]>
Add macro to hold the maximum number of UART ports per IC/device.
Reviewed-by: Jan Kundrát <[email protected]>
Tested-by: Jan Kundrát <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/max310x.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 21f2fa3a91e5..6549eee4f6a6 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -30,6 +30,7 @@
#define MAX310X_MAJOR 204
#define MAX310X_MINOR 209
#define MAX310X_UART_NRMAX 16
+#define MAX310X_MAX_PORTS 4 /* Maximum number of UART ports per IC. */
/* MAX310X register definitions */
#define MAX310X_RHR_REG (0x00) /* RX FIFO */
@@ -1502,7 +1503,7 @@ static const struct max310x_if_cfg __maybe_unused max310x_spi_if_cfg = {
static int max310x_spi_probe(struct spi_device *spi)
{
const struct max310x_devtype *devtype;
- struct regmap *regmaps[4];
+ struct regmap *regmaps[MAX310X_MAX_PORTS];
unsigned int i;
int ret;
@@ -1604,7 +1605,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
{
const struct max310x_devtype *devtype;
struct i2c_client *port_client;
- struct regmap *regmaps[4];
+ struct regmap *regmaps[MAX310X_MAX_PORTS];
unsigned int i;
u8 port_addr;
--
2.39.2
On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
>
> From: Hugo Villeneuve <[email protected]>
>
> Hello,
> this patch series brings a few clean-ups and improvements to the max310x
> driver.
>
> Some of these changes are based on suggestions for the sc16is7xx driver by
> Andy Shevchenko following this dicussion:
discussion
> Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
Perhaps you may add Suggested-by to the related changes.
> The changes have been tested on a custom board using a max14830 in SPI
> mode, with an external oscillator (not crystal). Tests included a simple
> communication test with a GPS connected to UART0.
>
> They also have been tested by using i2c-stub to simulate the four ports on a
> virtual I2C max14830 device, but with obvious limitations as this cannot
> simulate all the hardware functions.
..
LGTM, except this one (I have commented individually)
> serial: max310x: replace ENOTSUPP with preferred EOPNOTSUPP
> (checkpatch)
So, for the rest
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
>
> From: Hugo Villeneuve <[email protected]>
>
> Fixes the following checkpatch warning:
>
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
NAK.
It's a false positive.
> According to include/linux/errno.h, ENOTSUPP is
> "Defined for the NFSv3 protocol", so replace it with preferred EOPNOTSUPP.
The GPIO subsystem uses this internal error code internally. User
space won't get it, so users may not see this one.
--
With Best Regards,
Andy Shevchenko
On Thu, 18 Jan 2024 01:24:11 +0200
Andy Shevchenko <[email protected]> wrote:
> On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
> >
> > From: Hugo Villeneuve <[email protected]>
> >
> > Fixes the following checkpatch warning:
> >
> > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
>
> NAK.
> It's a false positive.
>
> > According to include/linux/errno.h, ENOTSUPP is
> > "Defined for the NFSv3 protocol", so replace it with preferred EOPNOTSUPP.
>
> The GPIO subsystem uses this internal error code internally. User
> space won't get it, so users may not see this one.
Hi Andy,
I will drop the patch then.
What about adding a comment to prevent future fixes?
- return -ENOTSUPP;
+ return -ENOTSUPP; /*
+ * ENOTSUPP is used for backward compatibility
+ * with GPIO subsystem.
+ */
Hugo Villeneuve
Hi,
On Thu, 18 Jan 2024 01:26:59 +0200
Andy Shevchenko <[email protected]> wrote:
> On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
> >
> > From: Hugo Villeneuve <[email protected]>
> >
> > Hello,
> > this patch series brings a few clean-ups and improvements to the max310x
> > driver.
> >
> > Some of these changes are based on suggestions for the sc16is7xx driver by
> > Andy Shevchenko following this dicussion:
>
> discussion
Will fix in V2.
>
> > Link: https://lore.kernel.org/all/CAHp75VebCZckUrNraYQj9k=Mrn2kbYs1Lx26f5-8rKJ3RXeh-w@mail.gmail.com/
>
> Perhaps you may add Suggested-by to the related changes.
Ok, will do for some of the patches in V2.
> > The changes have been tested on a custom board using a max14830 in SPI
> > mode, with an external oscillator (not crystal). Tests included a simple
> > communication test with a GPS connected to UART0.
> >
> > They also have been tested by using i2c-stub to simulate the four ports on a
> > virtual I2C max14830 device, but with obvious limitations as this cannot
> > simulate all the hardware functions.
>
> ...
>
> LGTM, except this one (I have commented individually)
> > serial: max310x: replace ENOTSUPP with preferred EOPNOTSUPP
> > (checkpatch)
>
> So, for the rest
> Reviewed-by: Andy Shevchenko <[email protected]>
Thank you for the review,
Hugo Villeneuve
On Thu, Jan 18, 2024 at 1:59 AM Hugo Villeneuve <[email protected]> wrote:
> On Thu, 18 Jan 2024 01:24:11 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
..
> > > Fixes the following checkpatch warning:
> > >
> > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> >
> > NAK.
> > It's a false positive.
> >
> > > According to include/linux/errno.h, ENOTSUPP is
> > > "Defined for the NFSv3 protocol", so replace it with preferred EOPNOTSUPP.
> >
> > The GPIO subsystem uses this internal error code internally. User
> > space won't get it, so users may not see this one.
>
> Hi Andy,
> I will drop the patch then.
>
> What about adding a comment to prevent future fixes?
>
> - return -ENOTSUPP;
> + return -ENOTSUPP; /*
> + * ENOTSUPP is used for backward compatibility
> + * with GPIO subsystem.
> + */
It's kinda useless to add it to a single (GPIO) driver.
Rather it needs to be mentioned somewhere between
https://www.kernel.org/doc/html/latest/driver-api/gpio/index.html.
+Cc: Kent, Bart. It seems we have a handful of drivers violating this
(basically following what checkpatch says) and GPIO not documenting
this specific error code and its scope. Did I miss anything?
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 18, 2024 at 10:59:34AM +0200, Andy Shevchenko wrote:
> On Thu, Jan 18, 2024 at 1:59 AM Hugo Villeneuve <[email protected]> wrote:
> > On Thu, 18 Jan 2024 01:24:11 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Thu, Jan 18, 2024 at 12:39 AM Hugo Villeneuve <[email protected]> wrote:
>
> ...
>
> > > > Fixes the following checkpatch warning:
> > > >
> > > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> > >
> > > NAK.
> > > It's a false positive.
> > >
> > > > According to include/linux/errno.h, ENOTSUPP is
> > > > "Defined for the NFSv3 protocol", so replace it with preferred EOPNOTSUPP.
> > >
> > > The GPIO subsystem uses this internal error code internally. User
> > > space won't get it, so users may not see this one.
> >
> > Hi Andy,
> > I will drop the patch then.
> >
> > What about adding a comment to prevent future fixes?
> >
> > - return -ENOTSUPP;
> > + return -ENOTSUPP; /*
> > + * ENOTSUPP is used for backward compatibility
> > + * with GPIO subsystem.
> > + */
>
> It's kinda useless to add it to a single (GPIO) driver.
> Rather it needs to be mentioned somewhere between
> https://www.kernel.org/doc/html/latest/driver-api/gpio/index.html.
>
> +Cc: Kent, Bart. It seems we have a handful of drivers violating this
> (basically following what checkpatch says) and GPIO not documenting
> this specific error code and its scope. Did I miss anything?
>
You are correct - the GPIO subsystem is expecting ENOTSUPP if the config
is not supported. In some cases it absorbs the failure or emulates the
feature instead (open drain/source, debounce). Returning EOPNOTSUPP
would be unfortunate, so checkpatch is not being helpful here.
And don't get me started on the gpio_chip interface contract being too
vague.
There are a handful of ways this could be addressed (documentation,
checkpatch, handle either, switch to EOPNOTSUPP, ... or some combination),
but making that call is definitely in Bart's court.
Cheers,
Kent.