Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1566508

Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

Show all headers | View raw

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> There are now 2 possible separate/different Pinctrl states which can
> be provided from platform data.  One which encompasses the lines
> required for HW flow-control (CTS/RTS) and another which does not
> specify these lines, such that they can be used via GPIO mechanisms
> for manually toggling (i.e. from a request by `stty`).
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 397df50..03801ed 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -37,10 +37,16 @@
>  #define ASC_FIFO_SIZE 16
>  #define ASC_MAX_PORTS 8
>  
> +/* Pinctrl states */
> +#define DEFAULT		0
> +#define MANUAL_RTS	1

Nit: Would be better to have them aligned.

> +
>  struct asc_port {
>  	struct uart_port port;
>  	struct gpio_desc *rts;
>  	struct clk *clk;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *states[2];
>  	unsigned int hw_flow_control:1;
>  	unsigned int force_m1:1;
>  };
> @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
>  {
>  	struct uart_port *port = &ascport->port;
>  	struct resource *res;
> +	int ret;
>  
>  	port->iotype	= UPIO_MEM;
>  	port->flags	= UPF_BOOT_AUTOCONF;
> @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
>  	WARN_ON(ascport->port.uartclk == 0);
>  	clk_disable_unprepare(ascport->clk);
>  
> +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(ascport->pinctrl)) {
> +		ret = PTR_ERR(ascport->pinctrl);
> +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> +	}
> +
> +	ascport->states[DEFAULT] =
> +		pinctrl_lookup_state(ascport->pinctrl, "default");
> +	if (IS_ERR(ascport->states[DEFAULT])) {
> +		ret = PTR_ERR(ascport->states[DEFAULT]);
> +		dev_err(&pdev->dev,
> +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* "manual-rts" state is optional */
> +	ascport->states[MANUAL_RTS] =
> +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> +		ascport->states[MANUAL_RTS] = NULL;
> +

The different pinctrl states looks like a neat solution to the problem.

My only concern here is that 'default' state is implying a hw-flow-control
pinmux config, and manual-rts is implying what is the current upstream
'default' pinmux config.

Which maybe ok if you update all uarts, but currently only serial0
is updated. So the other uarts current 'default' is actually the same as serial0
'manual-rts' grouping, which conceptually is odd.

Would it not be better to make 'manual-rts' the default state? As that aligns
to what is currently already the default for the other UARTS? And then make
hw-flow-control the optional state for serial0?

That also has the advantage that 'default' has the same meaning with older DT's.

regards,

Peter.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/8] serial: st-asc: Allow handling of RTS line Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
  [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
    Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:10 +0100
      Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:50 +0100
        Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-27 12:40 +0100
      Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-27 12:40 +0100
  [PATCH 3/8] serial: st-asc: Read in all Pinctrl states Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
    Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:30 +0100
      Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Lee Jones <lee.jones@linaro.org> - 2017-01-27 13:00 +0100
        Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-30 15:40 +0100
          Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Lee Jones <lee.jones@linaro.org> - 2017-01-30 16:40 +0100
            Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-30 17:20 +0100
  Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line Greg KH <gregkh@linuxfoundation.org> - 2017-01-25 11:10 +0100
    Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line Lee Jones <lee.jones@linaro.org> - 2017-01-25 16:40 +0100

csiph-web