Re: [PATCH 15/15] serial: 8250_mxpcie: implement rx_trig_bytes callbacks via MUEx50 RTL

[Andy Shevchenko]

On Mon, May 4, 2026 at 11:51 AM Crescent Hsieh
<crescentcy.hsieh@xxxxxxxx> wrote:
>
> The MUEx50 UART exposes a programmable RX trigger level via the RTL
> register.
>
> Implement uart_port RX trigger set/get callbacks for the mxpcie driver
> and wire them up to the generic rx_trig_bytes sysfs interface. Store the
> configured trigger level in the per-port private data.

...

>  struct mxpcie8250_port {
>         int line;
> +       u8 rx_trig_level;
>         unsigned long event_flags;
>         struct uart_port *port;
>         struct work_struct work;

Whenever you add a member to or modify existing data structure, always
run `pahole` to check if the layout is good enough (or even the best
fit). Same applies to the whole series.

...

> +static int mxpcie8250_set_rxtrig(struct uart_port *port, unsigned char bytes)
> +{
> +       struct mxpcie8250 *priv = dev_get_drvdata(port->dev);
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       if (bytes > 128)

Magic! Should it be defined or already has been?

> +               return -EINVAL;
> +
> +       serial_out(up, MOXA_PUART_RTL, bytes);
> +       priv->port[port->port_id].rx_trig_level = bytes;
> +
> +       return 0;
> +}

...

> +static int mxpcie8250_get_rxtrig(struct uart_port *port)
> +{
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
> +       return serial_in(up, MOXA_PUART_RTL);

No need to have an intermediate pointer as you can simply use
serial_port_in(). Same applies to other places when the uart_8250_port
is not used otherwise.

> +}

...

> +               priv->port[i].rx_trig_level = 96;

As per above, this magic together with the above should be defined
somehow. And together they make more context to the reader.

-- 
With Best Regards,
Andy Shevchenko