Date: Tue, 9 Jan 2018 12:14:32 +0100
From: Johan Hovold <johan@kernel.org>
To: "Ji-Ze Hong (Peter Hong)" <hpeter@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        peter_hong@fintek.com.tw,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V2 2/5] usb: serial: f81534: add auto RTS direction
 support
Message-ID: <20180109111432.GO11344@localhost>
References: <1515032961-29131-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1515032961-29131-2-git-send-email-hpeter+linux_kernel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1515032961-29131-2-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.7.2 (2016-11-26)
Sender: linux-kernel-owner@vger.kernel.org

On Thu, Jan 04, 2018 at 10:29:18AM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 had auto RTS direction support for RS485 mode.
> We'll read it from internal Flash with address 0x2f01~0x2f04 for 4 ports.
> There are 4 conditions below:
> 	0: F81534_PORT_CONF_RS232.
> 	1: F81534_PORT_CONF_RS485.
> 	2: value error, default to F81534_PORT_CONF_RS232.
> 	3: F81534_PORT_CONF_RS485_INVERT.
> 
> F81532/534 Clock register (offset +08h)
> 
> Bit0:	UART Enable (always on)
> Bit2-1:	Clock source selector
> 			00: 1.846MHz.
> 			01: 18.46MHz.
> 			10: 24MHz.
> 			11: 14.77MHz.
> Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
> Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
> V2:
> 	1: Read the configure data from flash and save it to shadow clock
> 	   register.
> 
>  drivers/usb/serial/f81534.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 758ef0424164..8a778bc1d492 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -98,11 +98,16 @@
>  
>  #define F81534_DEFAULT_BAUD_RATE	9600
>  
> +#define F81534_PORT_CONF_RS232		0
> +#define F81534_PORT_CONF_RS485		BIT(0)
> +#define F81534_PORT_CONF_RS485_INVERT	(BIT(0) | BIT(1))
>  #define F81534_PORT_CONF_DISABLE_PORT	BIT(3)
>  #define F81534_PORT_CONF_NOT_EXIST_PORT	BIT(7)
>  #define F81534_PORT_UNAVAILABLE		\
>  	(F81534_PORT_CONF_DISABLE_PORT | F81534_PORT_CONF_NOT_EXIST_PORT)
>  
> +#define F81534_UART_MODE_MASK		(BIT(0) | BIT(1))

Use GENMASK()?

> +
>  #define F81534_1X_RXTRIGGER		0xc3
>  #define F81534_8X_RXTRIGGER		0xcf
>  
> @@ -115,6 +120,8 @@
>   *			01: 18.46MHz.
>   *			10: 24MHz.
>   *			11: 14.77MHz.
> + * Bit4:	Auto direction(RTS) control (RTS pin Low when TX)
> + * Bit5:	Invert direction(RTS) when Bit4 enabled (RTS pin high when TX)
>   */
>  
>  #define F81534_UART_EN			BIT(0)
> @@ -123,6 +130,9 @@
>  #define F81534_CLK_24_MHZ		(F81534_UART_EN | BIT(2))
>  #define F81534_CLK_14_77_MHZ		(F81534_UART_EN | BIT(1) | BIT(2))
>  
> +#define F81534_CLK_RS485_MODE		BIT(4)
> +#define F81534_CLK_RS485_INVERT		BIT(5)
> +
>  static const struct usb_device_id f81534_id_table[] = {
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_1, FINTEK_DEVICE_ID) },
>  	{ USB_DEVICE(FINTEK_VENDOR_ID_2, FINTEK_DEVICE_ID) },
> @@ -517,7 +527,8 @@ static int f81534_set_port_config(struct usb_serial_port *port,
>  	}
>  
>  	port_priv->baud_base = baudrate_table[idx];
> -	port_priv->shadow_clk = clock_table[idx];
> +	port_priv->shadow_clk &= ~F81534_CLK_14_77_MHZ;

Add a dedicated mask define instead of using a clock value here. No need
to clear the enable bit for example (which shouldn't be part of the
clock value as I mentioned earlier).

> +	port_priv->shadow_clk |= clock_table[idx];
>  
>  	status = f81534_set_port_register(port, F81534_CLOCK_REG,
>  			port_priv->shadow_clk);
> @@ -1269,9 +1280,12 @@ static void f81534_lsr_worker(struct work_struct *work)
>  
>  static int f81534_port_probe(struct usb_serial_port *port)
>  {
> +	struct f81534_serial_private *serial_priv;
>  	struct f81534_port_private *port_priv;
>  	int ret;
> +	u8 value;
>  
> +	serial_priv = usb_get_serial_data(port->serial);
>  	port_priv = devm_kzalloc(&port->dev, sizeof(*port_priv), GFP_KERNEL);
>  	if (!port_priv)
>  		return -ENOMEM;
> @@ -1303,6 +1317,24 @@ static int f81534_port_probe(struct usb_serial_port *port)
>  	if (ret)
>  		return ret;
>  
> +	value = serial_priv->conf_data[port_priv->phy_num];
> +	switch (value & F81534_UART_MODE_MASK) {
> +	case F81534_PORT_CONF_RS485_INVERT:
> +		port_priv->shadow_clk = F81534_CLK_RS485_MODE |
> +					F81534_CLK_RS485_INVERT;
> +		dev_info(&port->dev, "RS485 invert mode.\n");
> +		break;
> +	case F81534_PORT_CONF_RS485:
> +		port_priv->shadow_clk = F81534_CLK_RS485_MODE;
> +		dev_info(&port->dev, "RS485 mode.\n");
> +		break;
> +
> +	default:
> +	case F81534_PORT_CONF_RS232:
> +		dev_info(&port->dev, "RS232 mode.\n");

Again, I think these dev_info should really be dev_dbg.

> +		break;
> +	}
> +
>  	return 0;
>  }

Johan