Re: [PATCH 1/2] serial: wk2xxx: Add WK2xxx SPI UART driver

[zjzhao]

From: zjzhao <zjzhao@xxxxxxxxx>

Hi Jiri,

thanks for the review. Answers and a [PATCH v2] follow; v2 also carries
the Sashiko round-2 findings.

> Have you checked that there is no similar driver which could be only
> extended?

Yes. The only SPI/I2C UART bridge drivers in the tree are max3100.c,
max310x.c and sc16is7xx.c. The latter two implement the 16550-style
register set (THR/RHR, IER, IIR/FCR, LCR, MCR/MSR, LSR) over regmap,
which is exactly what the WK2xxx does *not* provide: its register map
and SPI protocol are proprietary (the sub-UART number and a page-select
bit live in the SPI command byte, plus dedicated FIFO burst commands),
it has no modem-control registers and only supports 8 data bits. Folding
it into sc16is7xx/max310x would force a more capable shared layer to
degrade, and it would mix two vendors into one binding. I take the
kernel's own precedent (sc16is7xx and max310x are separate drivers even
though sc16is7xx is derived from max310x) as confirmation that a
dedicated driver is the right model.

> > +	scoped_guard(mutex, &s->reg_lock) { ... }
> why is this one scoped?

The early return inside made it scoped; that was overkill since the lock
covers the whole function anyway. v2 uses a plain guard(mutex) for
wk2xxx_port_reg_update() and wk2xxx_conf_port().

> throttle/unthrottle, set_termios: this can be a guard too, right?

Yes - v2 uses guard(uart_port_lock_irqsave).

> What's the reason not to use any of the uart_port_tx* helpers?

TX needs sleeping SPI transfers, which cannot run while the port
spinlock is held. The helpers acquire port->lock around the drain, so
they are not usable here; like sc16is7xx/max310x, TX is run from the
kthread worker under a per-port mutex.

> Why not guard?  (handle_tx)  /  And kill the goto then.

Done in v2: guard(mutex) + scoped_guard(uart_port_lock_irqsave), the
goto and the tail unlock are gone.

> Should you perhaps cap the loop count as well?

Done - the drain loop is now bounded by WK2XXX_IRQ_MAX_PASSES.

> return IRQ_HANDLED ... bad for shared irqs

Fixed in v2 (IRQ_NONE when nothing was pending).

> Did you mean to use explicit u64?

Fixed - the divisor computation now uses div_u64()/u64 and builds on
32-bit.

> req + rel are optional. Drop them.

Done - request_port/release_port and their no-op helpers are dropped.

> membase=(void __iomem *)~0 ... compiler/sparse not complain?

Rather than fixing the cast I switched the ports to UPIO_BUS (as
sc16is7xx/max310x do) and removed the membase workaround altogether.

The v2 cover letter lists the full change set. Please review; happy to
iterate.

thanks,

--
zjzhao
EDATEC Technology Co., Ltd.