Hi Thierry,
I've just had time to review the header file changes in this patch
today.
On 29/01/2025 16:37, Thierry Bultel wrote:
> The aim here is to prepare support for new sci controllers like
> the T2H/RSCI whose registers are too much different for being
> handled in common code.
>
> This named serial controller also has 32 bits register,
> so some return types had to be changed.
>
> The needed generic functions are no longer static, with prototypes
> defined in sh-sci-common.h so that they can be used from specific
> implementation in a separate file, to keep this driver as little
> changed as possible.
>
> For doing so, a set of 'ops' is added to struct sci_port.
>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@xxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/sh-sci.c | 339 +++++++++++++++--------------
> drivers/tty/serial/sh-sci_common.h | 178 +++++++++++++++
> 2 files changed, 349 insertions(+), 168 deletions(-)
> create mode 100644 drivers/tty/serial/sh-sci_common.h
[snip]
> diff --git a/drivers/tty/serial/sh-sci_common.h b/drivers/tty/serial/sh-sci_common.h
> new file mode 100644
> index 000000000000..cbfacdc1a836
> --- /dev/null
> +++ b/drivers/tty/serial/sh-sci_common.h
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __SH_SCI_COMMON_H__
> +#define __SH_SCI_COMMON_H__
> +
> +#define SCI_MAJOR 204
> +#define SCI_MINOR_START 8
> +
> +#include <linux/serial_core.h>
> +
> +enum SCI_CLKS {
> + SCI_FCK, /* Functional Clock */
> + SCI_SCK, /* Optional External Clock */
> + SCI_BRG_INT, /* Optional BRG Internal Clock Source */
> + SCI_SCIF_CLK, /* Optional BRG External Clock Source */
> + SCI_NUM_CLKS
> +};
> +
> +/* Offsets into the sci_port->irqs array */
> +enum {
> + SCIx_ERI_IRQ,
> + SCIx_RXI_IRQ,
> + SCIx_TXI_IRQ,
> + SCIx_BRI_IRQ,
> + SCIx_DRI_IRQ,
> + SCIx_TEI_IRQ,
> + SCIx_NR_IRQS,
> +
> + SCIx_MUX_IRQ = SCIx_NR_IRQS, /* special case */
> +};
> +
> +/* Bit x set means sampling rate x + 1 is supported */
> +#define SCI_SR(x) BIT((x) - 1)
> +
> +extern void sci_release_port(struct uart_port *port);
> +extern int sci_request_port(struct uart_port *port);
> +extern void sci_config_port(struct uart_port *port, int flags);
> +extern int sci_verify_port(struct uart_port *port, struct serial_struct *ser);
> +extern void sci_pm(struct uart_port *port, unsigned int state,
> + unsigned int oldstate);
> +extern void sci_enable_ms(struct uart_port *port);
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +extern int sci_poll_get_char(struct uart_port *port);
> +extern void sci_poll_put_char(struct uart_port *port, unsigned char c);
The extern keyword isn't needed for function definitions in header
files.
> +#endif /* CONFIG_CONSOLE_POLL */
> +
> +struct plat_sci_reg {
> + u8 offset, size;
Please define struct members on separate lines.
> +};
> +
> +/* The actual number of needed registers depends on the sci controller;
> + * using this value as a max covers both sci and rsci cases
> + */
> +#define SCI_NR_REGS 20
> +
> +struct sci_port_params_bits {
> + unsigned int rxtx_enable;
> + unsigned int te_clear;
> + unsigned int poll_sent_bits;
> +};
> +
> +struct sci_common_regs {
> + unsigned int status;
> + unsigned int control;
> +};
> +
> +struct sci_port_params {
> + const struct plat_sci_reg regs[SCI_NR_REGS];
I don't see any usage of the regs field of this struct - is it needed?
If not, can we also get rid of SCI_NR_REGS?
> + const struct sci_common_regs *common_regs;
> + unsigned int fifosize;
> + unsigned int overrun_reg;
> + unsigned int overrun_mask;
> + unsigned int sampling_rate_mask;
> + unsigned int error_mask;
> + unsigned int error_clear;
> + struct sci_port_params_bits param_bits;
It looks like we always initialise param_bits via a `static const struct
sci_port_params_bits` instance. Is there any reason we copy the contents
of this into the sci_port_params instance instead of using a pointer?
> +};
> +
> +struct sci_port_ops {
> + u32 (*read_reg)(struct uart_port *port, int reg);
> + void (*write_reg)(struct uart_port *port, int reg, int value);
> + void (*clear_SCxSR)(struct uart_port *port, unsigned int mask);
> +
> + void (*transmit_chars)(struct uart_port *port);
> + void (*receive_chars)(struct uart_port *port);
> +
> + void (*poll_put_char)(struct uart_port *port, unsigned char c);
> +
> + int (*set_rtrg)(struct uart_port *port, int rx_trig);
> + int (*rtrg_enabled)(struct uart_port *port);
> +
> + void (*shutdown_complete)(struct uart_port *port);
> +
> + unsigned int (*get_ctrl_temp)(struct uart_port *port, unsigned int ctrl);
I think we need a better name for this one. ctrl_temp is just the name
of the value we want to write to the control register in the
serial_console_write function, the name doesn't give any clue as to its
intended function.
Perhaps it would be better to define a prepare_console_write operation
which modifies the control register state and returns the old control
register state (so that it can later be restored). That would result in
a little more code duplication but it'd be easier to understand.
> +};
[snipped the rest]
Thanks,
--
Paul BarkerAttachment:
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
