On Tue, Feb 04, 2025 at 06:04:57PM +0000, Paul Barker wrote: > Hi Thierry, > > > > +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? > Yes, the 'regs' field is used in sh-sci.c, most essentially through the sci_getregs macro. Notice that the field is however not used in the later patch that adds rzsci support. > > + 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 Barker