Re: [PATCH 09/14] serial: sh-sci: Introduced function pointers

[Thierry Bultel <thierry.bultel.yh@bp.renesas.com>]

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