Received-SPF: pass (google.com: domain of linux-kernel+bounces-149993-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Thu, 18 Apr 2024 14:43:32 +0300 (EEST)
To: Parker Newman <parker@finest.io>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
    Jiri Slaby <jirislaby@kernel.org>, linux-kernel@vger.kernel.org, 
    linux-serial@vger.kernel.org, Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v4 5/7] serial: exar: add CTI cards to
 exar_get_nr_ports
In-Reply-To: <0c64bdf852f39aec966b38696695d951e485d7e6.1713382717.git.pnewman@connecttech.com>
Message-ID: <60790bd3-d35a-026d-ef38-ae3b12160fbe@linux.intel.com>
References: <cover.1713382717.git.pnewman@connecttech.com> <0c64bdf852f39aec966b38696695d951e485d7e6.1713382717.git.pnewman@connecttech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Wed, 17 Apr 2024, Parker Newman wrote:

> From: Parker Newman <pnewman@connecttech.com>
> 
> Add code for getting number of ports of CTI cards to
> exar_get_nr_ports().
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
> Changes in v3:
> - moved to separate patch
> - added spaces to single line comments
> 
>  drivers/tty/serial/8250/8250_exar.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index e68029a59122..197f45e306ff 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -711,12 +711,28 @@ static unsigned int exar_get_nr_ports(struct exar8250_board *board,
>  {
>  	unsigned int nr_ports = 0;
> 
> -	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO)
> +	if (pcidev->vendor == PCI_VENDOR_ID_ACCESSIO) {

You can add the braces while you moved the code around so you don't need 
to play with them again here and this patch can be more to the point.

>  		nr_ports = BIT(((pcidev->device & 0x38) >> 3) - 1);
> -	else if (board->num_ports)
> +	} else if (board->num_ports > 0) {
> +		// Check if board struct overrides number of ports
>  		nr_ports = board->num_ports;

The comment just tells what the code does, IMO that comment doesn't add 
any value.

> -	else
> +	} else if (pcidev->vendor == PCI_VENDOR_ID_EXAR) {
> +		// Exar encodes # ports in last nibble of PCI Device ID ex. 0358

This comment you can also add while you moved the code around (or make 
another patch out of it after moving).

-- 
 i.

>  		nr_ports = pcidev->device & 0x0f;
> +	} else  if (pcidev->vendor == PCI_VENDOR_ID_CONNECT_TECH) {
> +		// Handle CTI FPGA cards
> +		switch (pcidev->device) {
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG00X:
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_12_XIG01X:
> +			nr_ports = 12;
> +			break;
> +		case PCI_DEVICE_ID_CONNECT_TECH_PCI_XR79X_16:
> +			nr_ports = 16;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> 
>  	return nr_ports;
>  }
> --
> 2.43.2
> 
>