mbox series

[v3,00/11] serial: sc16is7xx: fix GPIO regression and rs485 improvements

Message ID 20230525040324.3773741-1-hugo@hugovil.com
Headers show
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements | expand

Message

Hugo Villeneuve May 25, 2023, 4:03 a.m. UTC
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Hello,
this patch series mainly fixes a GPIO regression and improve RS485 flags and properties
detection from DT.

It now also includes various small fixes and improvements that were previously
sent as separate patches, but that made testing everything difficult.

Patches 1 and 2 are simple comments fix/improvements.

Patch 3 fixes an issue when debugging IOcontrol register. After testing the GPIO
regression patches (patches 6 and 7, tests done by Lech Perczak), it appers that
this patch is also necessary for having the correct IOcontrol register values.

Patch 4 introduces a delay after a reset operation to respect datasheet
timing recommandations.

Patch 5 fixes an issue with init of first port during probing. This commit
brings some questions and I appreciate if people from the serial subsystem could
comment on my proposed solution.

Patch 6 fixes a bug with the output value when first setting the GPIO direction.

Patch 7, 8 and 9 fix a GPIO regression by (re)allowing to choose GPIO function for
GPIO pins shared with modem status lines.

Patch 10 allows to read common rs485 device-tree flags and properties.

Patch 11 add a custom dump function as relying on regmal debugfs is not really
practical for this driver.

I have tested the changes on a custom board with two SC16IS752 DUART using a
Variscite IMX8MN NANO SOM.

Thank you.

Link: [v1] https://lkml.org/lkml/2023/5/17/967
      [v1] https://lkml.org/lkml/2023/5/17/777
      [v1] https://lkml.org/lkml/2023/5/17/780
      [v1] https://lkml.org/lkml/2023/5/17/785
      [v1] https://lkml.org/lkml/2023/5/17/1311
      [v2] https://lkml.org/lkml/2023/5/18/516

Changes for V3:
- Integrated all patches into single serie to facilitate debugging and tests.
- Reduce number of exported GPIOs depending on new property nxp,modem-control-line-ports
- Added additional example in DT bindings

Hugo Villeneuve (11):
  serial: sc16is7xx: fix syntax error in comments
  serial: sc16is7xx: improve comments about variants
  serial: sc16is7xx: mark IOCONTROL register as volatile
  serial: sc16is7xx: add post reset delay
  serial: sc16is7xx: fix broken port 0 uart init
  serial: sc16is7xx: fix bug when first setting GPIO direction
  dt-bindings: sc16is7xx: Add property to change GPIO function
  serial: sc16is7xx: fix regression with GPIO configuration
  serial: sc16is7xx: add I/O register translation offset
  serial: sc16is7xx: add call to get rs485 DT flags and properties
  serial: sc16is7xx: add dump registers function

 .../bindings/serial/nxp,sc16is7xx.txt         |  46 +++++
 drivers/tty/serial/sc16is7xx.c                | 180 +++++++++++++++---
 2 files changed, 199 insertions(+), 27 deletions(-)


base-commit: 933174ae28ba72ab8de5b35cb7c98fc211235096

Comments

Andy Shevchenko May 26, 2023, 6:28 p.m. UTC | #1
Thu, May 25, 2023 at 10:34:43AM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 14:11:22 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:21AM -0400, Hugo Villeneuve kirjoitti:

...

> > I'm wondering if we can convert this to YAML first and then add a new
> > property.
> 
> I also thought about it, then decided to focus on simply adding the new
> property first since I am not an expert in YAML.
> 
> I think it would be best to do it after this patch series. Keep in mind that
> the original intent of this patch series, and this new property, was to fix a
> regression related to the GPIOs, and I think that converting to YAML would
> simply delay and add much noise to the discussion at this point.

The patch doesn't have Fixes tag, so it's definitely not a fix. Also new
property can fix a regression, it's impossible to fix the users' DTBs.

> If someone wants to do it as a separate patch after this, fine. If not, I an
> willing to give it a go.

Not a DT maintainer here, I'm fine with either way.
Andy Shevchenko May 26, 2023, 6:38 p.m. UTC | #2
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.
Greg KH May 28, 2023, 11:56 a.m. UTC | #3
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