Received-SPF: pass (google.com: domain of linux-kernel+bounces-150309-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 18 Apr 2024 17:28:44 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Konstantin Pugin <rilian.la.te@ya.ru>
Cc: Konstantin Pugin <ria.freelander@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Hugo Villeneuve <hvilleneuve@dimonoff.com>,
	Lech Perczak <lech.perczak@camlingroup.com>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org
Subject: Re: [PATCH v2 3/3] serial: sc16is7xx: add support for EXAR XR20M1172
 UART
Message-ID: <ZiEuHE7aGnrPk5RK@smile.fi.intel.com>
References: <20240418135737.3659498-1-rilian.la.te@ya.ru>
 <20240418135737.3659498-4-rilian.la.te@ya.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240418135737.3659498-4-rilian.la.te@ya.ru>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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