On Thu, Apr 18, 2024 at 04:57:34PM +0300, Konstantin Pugin wrote:
> From: Konstantin Pugin <ria.freelander@xxxxxxxxx>
>
> 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
