| Message ID | 20230525040324.3773741-12-hugo@hugovil.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand |
Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > With this driver, it is very hard to debug the registers using > the /sys/kernel/debug/regmap interface. > > The main reason is that bits 0 and 1 of the register address > correspond to the channels bits, so the register address itself starts > at bit 2, so we must 'mentally' shift each register address by 2 bits > to get its offset. > > Also, only channels 0 and 1 are supported, so combinations of bits > 0 and 1 being 10b and 11b are invalid, and the display of these > registers is useless. > > For example: > > cat /sys/kernel/debug/regmap/spi0.0/registers > 04: 10 -> Port 0, register offset 1 > 05: 10 -> Port 1, register offset 1 > 06: 00 -> Port 2, register offset 1 -> invalid > 07: 00 -> port 3, register offset 1 -> invalid > ... > > Add a debug module parameter to call a custom dump function for each > port registers after the probe phase to help debug. Not sure about this. Can we rather create an abstract mapping on regmap? (Something like gpio-pca953x.c has)
On Thu, 25 May 2023 14:26:43 +0300 andy.shevchenko@gmail.com wrote: > Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > With this driver, it is very hard to debug the registers using > > the /sys/kernel/debug/regmap interface. > > > > The main reason is that bits 0 and 1 of the register address > > correspond to the channels bits, so the register address itself starts > > at bit 2, so we must 'mentally' shift each register address by 2 bits > > to get its offset. > > > > Also, only channels 0 and 1 are supported, so combinations of bits > > 0 and 1 being 10b and 11b are invalid, and the display of these > > registers is useless. > > > > For example: > > > > cat /sys/kernel/debug/regmap/spi0.0/registers > > 04: 10 -> Port 0, register offset 1 > > 05: 10 -> Port 1, register offset 1 > > 06: 00 -> Port 2, register offset 1 -> invalid > > 07: 00 -> port 3, register offset 1 -> invalid > > ... > > > > Add a debug module parameter to call a custom dump function for each > > port registers after the probe phase to help debug. > > Not sure about this. Can we rather create an abstract mapping on regmap? > (Something like gpio-pca953x.c has) Hi, maybe we can, but more like they do in the driver max310x.c (single, dual and quad UART versions). I will look into it, but it will probably be a patch that affects a lot of the code, and that I would like to submit separately after this serie, and so I will probably simply drop this current patch (11/11) since it will not be needed anymore. Hugo.
Thu, May 25, 2023 at 03:49:46PM -0400, Hugo Villeneuve kirjoitti: > On Thu, 25 May 2023 14:26:43 +0300 > andy.shevchenko@gmail.com wrote: > > Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve kirjoitti: ... > > Not sure about this. Can we rather create an abstract mapping on regmap? > > (Something like gpio-pca953x.c has) > > maybe we can, but more like they do in the driver max310x.c (single, dual and > quad UART versions). > > I will look into it, but it will probably be a patch that affects a lot of > the code, and that I would like to submit separately after this serie, and so > I will probably simply drop this current patch (11/11) since it will not be > needed anymore. Whatever, I commented on this solely because Greg KH is usually against new module parameters. If you sell your point to him, fine to me.
On Thu, May 25, 2023 at 12:03:25AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > With this driver, it is very hard to debug the registers using > the /sys/kernel/debug/regmap interface. > > The main reason is that bits 0 and 1 of the register address > correspond to the channels bits, so the register address itself starts > at bit 2, so we must 'mentally' shift each register address by 2 bits > to get its offset. > > Also, only channels 0 and 1 are supported, so combinations of bits > 0 and 1 being 10b and 11b are invalid, and the display of these > registers is useless. > > For example: > > cat /sys/kernel/debug/regmap/spi0.0/registers > 04: 10 -> Port 0, register offset 1 > 05: 10 -> Port 1, register offset 1 > 06: 00 -> Port 2, register offset 1 -> invalid > 07: 00 -> port 3, register offset 1 -> invalid > ... > > Add a debug module parameter to call a custom dump function for each > port registers after the probe phase to help debug. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > --- > drivers/tty/serial/sc16is7xx.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 03d00b144304..693b6cc371f8 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -347,6 +347,10 @@ struct sc16is7xx_port { > struct sc16is7xx_one p[]; > }; > > +static bool debug; > +module_param(debug, bool, 0644); > +MODULE_PARM_DESC(debug, "enable/disable debug messages"); Sorry, but no, use the normal dynamic debugging logic that the whole rest of the kernel uses. Do not add random per-driver module parameters like this, that would be a regression from the existing infrastructure that we have in place already. thanks, greg k-h
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 03d00b144304..693b6cc371f8 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -347,6 +347,10 @@ struct sc16is7xx_port { struct sc16is7xx_one p[]; }; +static bool debug; +module_param(debug, bool, 0644); +MODULE_PARM_DESC(debug, "enable/disable debug messages"); + static unsigned long sc16is7xx_lines; static struct uart_driver sc16is7xx_uart = { @@ -387,6 +391,28 @@ static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val) regmap_write(s->regmap, (reg << SC16IS7XX_REG_SHIFT) | line, val); } +static int sc16is7xx_port_dump(struct uart_port *port) +{ + int i; + unsigned char *buf; + char name[64]; + const int regs_count_per_port = 16; + + buf = devm_kzalloc(port->dev, regs_count_per_port, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + for (i = 0; i < regs_count_per_port; i++) + buf[i] = sc16is7xx_port_read(port, i); + + snprintf(name, sizeof(name), "sc16is7xx %s%i: dump ", + sc16is7xx_uart.dev_name, port->line); + print_hex_dump(KERN_ERR, name, DUMP_PREFIX_OFFSET, 16, 1, + &((u8 *)buf)[0], regs_count_per_port, 1); + + return 0; +} + static void sc16is7xx_fifo_read(struct uart_port *port, unsigned int rxlen) { struct sc16is7xx_port *s = dev_get_drvdata(port->dev); @@ -1614,6 +1640,10 @@ static int sc16is7xx_probe(struct device *dev, } #endif + if (debug) + for (i = 0; i < devtype->nr_uart; ++i) + sc16is7xx_port_dump(&s->p[i].port); + /* * Setup interrupt. We first try to acquire the IRQ line as level IRQ. * If that succeeds, we can allow sharing the interrupt as well.