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
