Re: [PATCH 1/4] ch341: Name more registers

[Johan Hovold <johan@kernel.org>]

On Fri, Mar 06, 2020 at 07:00:42PM +0000, Michael Hanselmann wrote:
> Add more #defines with register names.

Please be more specific (e.g. which registers are you naming). Update
the patch summary too.

> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c5ecdcd51ffc..518209072c50 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -59,6 +59,8 @@
>  #define CH341_REQ_MODEM_CTRL   0xA4
>  
>  #define CH341_REG_BREAK        0x05
> +#define CH341_REG_PRESCALER    0x12
> +#define CH341_REG_DIVISOR      0x13
>  #define CH341_REG_LCR          0x18
>  #define CH341_NBREAK_BITS      0x01
>  
> @@ -221,6 +223,7 @@ static int ch341_get_divisor(speed_t speed)
>  static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  				  struct ch341_private *priv, u8 lcr)
>  {
> +	uint16_t reg;

Use u16.

>  	int val;
>  	int r;
>  
> @@ -237,11 +240,15 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	 */
>  	val |= BIT(7);
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
> +	reg = ((uint16_t)CH341_REG_DIVISOR << 8) | CH341_REG_PRESCALER;

No need to cast.

> +
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, val);
>  	if (r)
>  		return r;
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
> +	reg = ((uint16_t)0x2500) | CH341_REG_LCR;

Ditto, and please add a bit shift for consistency.

Hmm, but this is unrelated to the defines you are adding, and there are
other magic constants for registers in this driver. Perhaps drop this
bit or break it out in its own patch (rule of thumb: one logical change
per patch).

> +
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, lcr);
>  	if (r)
>  		return r;

I'd also move this last in the series as it's more of a clean up or
documentation patch.

Johan