Re: [PATCH v2 2/2] ns16550: add Exar PCIe UART cards support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2021 14:16, Marek Marczykowski-Górecki wrote:
> @@ -169,6 +172,29 @@ static void handle_dw_usr_busy_quirk(struct ns16550 
> *uart)
>      }
>  }
>  
> +static void enable_exar_enhanced_bits(struct ns16550 *uart)

Afaics the parameter can be pointer-to-const.

> +{
> +#ifdef NS16550_PCI
> +    if ( uart->bar &&
> +         pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[2],
> +                         uart->ps_bdf[2]), PCI_VENDOR_ID) == 
> PCI_VENDOR_ID_EXAR )
> +    {
> +        u8 devid = ns_read_reg(uart, UART_XR_DVID);
> +        u8 efr;
> +        /*
> +         * Exar XR17V35x cards ignore setting MCR[2] (hardware flow control)
> +         * unless "Enhanced control bits" is enabled.
> +         * The below checks for a 2, 4 or 8 port UART, following Linux 
> driver.
> +         */
> +        if ( devid == 0x82 || devid == 0x84 || devid == 0x88 ) {

Hmm, now I'm in trouble as to a question you did ask earlier (once
on irc and I think also once in email): You asked whether to use
the PCI device ID _or_ the DVID register. Now you're using both,
albeit in a way not really making the access here safe: You assume
that you can access the byte at offset 0x8d on all Exar devices. I
don't know whether there is anything written anywhere telling you
this is safe. With the earlier vendor/device ID match, it would feel
safer to me if you replaced vendor ID and DVID checks here by a
check of uart->param: While you must not deref that pointer, you can
still check that it points at one of the three new entries you add
to uart_param[]. Perhaps via "switch ( uart->param - uart_param )".

Also there are a number of style nits:
- opening braces go on their own lines (except after "do"),
- blank lines are wanted between declarations and statements,
- we try to move away from u<N> and want new code to use uint<N>_t,
- with "devid" declared in the narrowest possible scope, please do
  so also for "efr" (albeit this logic may get rearranged enough to
  make this point moot).

Jan