Re: [PATCH v2 3/3] serial: sc16is7xx: add support for EXAR XR20M1172 UART

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Apr 18, 2024 at 04:57:34PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@gmail.com>
> 
> Its register set is mostly compatible with SC16IS762, but

"Its"? Whose? Elaborate, please.

> it has a support for additional division rates of UART
> with special DLD register. So, add handling this register
> via UPF_MAGIC_MULTIPLIER port flag.

Oh, can we avoid using this? You can redefine ->set_termios() if required and
before factor out the common pieces to the helper functions.

...

All three commit messages seems follow different text width, please keep it around

~60 for Subject and ~72 for the commit message.

...

>  /* Special Register set: Only if ((LCR[7] == 1) && (LCR != 0xBF)) */
>  #define SC16IS7XX_DLL_REG		(0x00) /* Divisor Latch Low */
>  #define SC16IS7XX_DLH_REG		(0x01) /* Divisor Latch High */
> +#define SC16IS7XX_DLD_REG		(0x02) /* Divisor Latch Mode (only on EXAR chips) */

Is it called DLD in the datasheet? If so, can you match the comment to
the datasheet, otherwise make it DLM and I would even go for the EXAR
namespace.

...

> +/* Divisor Latch Mode bits (EXAR extension)
> + *
> + * EXAR hardware is mostly compatible with SC16IS7XX, but supports additional feature:
> + * 4x and 8x divisor, instead of default 16x. It has a special register to program it.
> + * Bits 0 to 3 is fractional divisor, it used to set value of last 16 bits of
> + * uartclk * (16 / divisor) / baud, in case of default it will be uartclk / baud.
> + * Bits 4 and 5 used as switches, and should not be set to 1 simultaneously.
> + */

/*
 * This is wrong multi-line comment
 * style for this subsystem. Use this
 * example.
 */

...

> +#define SC16IS7XX_DLD_16X		0
> +#define SC16IS7XX_DLD_DIV(m)	((m) & 0xf)

GENMASK() (since you already use BIT() below)

> +#define SC16IS7XX_DLD_8X		BIT(4)
> +#define SC16IS7XX_DLD_4X		BIT(5)

Perhaps also EXAR namespace.

...

>  	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
>  	u8 lcr;
>  	u8 prescaler = 0;
> -	unsigned long clk = port->uartclk, div = clk / 16 / baud;
> +	u8 divisor = 16;
> +	u8 dld_mode = SC16IS7XX_DLD_16X;
> +	bool has_dld = !!(port->flags & UPF_MAGIC_MULTIPLIER);
> +	unsigned long clk = port->uartclk, div, div16;

Please, try to keep it in reversed xmas tree order (longer lines first).

...

> +	if (has_dld)

Can we actually replace this with some ID checks?

> +		while (DIV_ROUND_CLOSEST(port->uartclk, baud) < divisor)
> +			divisor /= 2;

Bit shifts and ffs() / fls() from bitops.h (or respective round*() / ilog2() /
etc. from log2.h) will help you to avoid while loop.

...

> +	div16 = clk * (16 / divisor) / baud;
> +	div = div16 / 16; /* For divisor = 16, it is the same as clk / 16 / baud */


So, these may loose in precision, right?

Wouldn't be better to have

	div16 = (clk * 16) / divisor / baud;
	div = div16 / 16;

?

>  	if (div >= BIT(16)) {
>  		prescaler = SC16IS7XX_MCR_CLKSEL_BIT;
>  		div /= 4;
>  	}
>  

...

>  	{ .compatible = "nxp,sc16is752",	.data = &sc16is752_devtype, },
>  	{ .compatible = "nxp,sc16is760",	.data = &sc16is760_devtype, },
>  	{ .compatible = "nxp,sc16is762",	.data = &sc16is762_devtype, },
> +	{ .compatible = "exar,xr20m1172",	.data = &xr20m1172_devtype, },

Sorted?

...

> +	{ "xr20m1172",	(kernel_ulong_t)&xr20m1172_devtype, },

This gives a hint about the above mentioned EXAR namespace for the definitions,
i.e. use

#define XR20M1172_...

-- 
With Best Regards,
Andy Shevchenko