Received-SPF: pass (google.com: domain of linux-kernel+bounces-148401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 17 Apr 2024 13:24:20 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Parker Newman <parker@finest.io>
Cc: Jiri Slaby <jirislaby@kernel.org>, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org,
	Parker Newman <pnewman@connecttech.com>
Subject: Re: [PATCH v3 7/8] serial: exar: add CTI specific setup code
Message-ID: <2024041726-fall-debunk-6cc5@gregkh>
References: <cover.1713270624.git.pnewman@connecttech.com>
 <354ff4aa9d8bdea1a21c503e685e81ac3df48eb8.1713270624.git.pnewman@connecttech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <354ff4aa9d8bdea1a21c503e685e81ac3df48eb8.1713270624.git.pnewman@connecttech.com>

On Tue, Apr 16, 2024 at 08:55:34AM -0400, Parker Newman wrote:
>  struct exar8250 {
>  	unsigned int		nr;
> +	unsigned int		osc_freq;
> +	struct pci_dev		*pcidev;
> +	struct device		*dev;

Why do you need both a pci_dev and a device?  Aren't they the same thing
here?

> +/**
> + * _cti_set_tristate() - Enable/Disable RS485 transciever tristate
> + * @priv: Device's private structure
> + * @port_num: Port number to set tristate on/off
> + * @enable: Enable tristate if true, disable if false
> + *
> + * Most RS485 capable cards have a power on tristate jumper/switch that ensures
> + * the RS422/RS485 transciever does not drive a multi-drop RS485 bus when it is
> + * not the master. When this jumper is installed the user must set the RS485
> + * mode to disable tristate prior to using the port.
> + *
> + * Some Exar UARTs have an auto-tristate feature while others require setting
> + * an MPIO to disable the tristate.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int _cti_set_tristate(struct exar8250 *priv,
> +			unsigned int port_num, bool enable)
> +{
> +	int ret = 0;
> +
> +	if (port_num >= priv->nr)
> +		return -EINVAL;
> +
> +	// Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;

How can this ever happen?  Only the exar devices will call this
function, or am I missing a path here?


> +
> +	dev_dbg(priv->dev, "%s tristate for port %u\n",
> +		str_enable_disable(enable), port_num);
> +
> +	if (enable)
> +		ret = exar_mpio_set_low(priv, port_num);
> +	else
> +		ret = exar_mpio_set_high(priv, port_num);
> +	if (ret)
> +		return ret;
> +
> +	// Ensure MPIO is an output
> +	ret = exar_mpio_config_output(priv, port_num);
> +
> +	return ret;
> +}
> +
> +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> +{
> +	return _cti_set_tristate(priv, port_num, false);
> +}

Do you ever call _cti_set_tristate() with "true"?

> +
> +/**
> + * _cti_set_plx_int_enable() - Enable/Disable PCI interrupts
> + * @priv: Device's private structure
> + * @enable: Enable interrupts if true, disable if false

But false is never used here, so why have this at all?

> + *
> + * Some older CTI cards require MPIO_0 to be set low to enable the PCI
> + * interupts from the UART to the PLX PCI->PCIe bridge.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int _cti_set_plx_int_enable(struct exar8250 *priv, bool enable)
> +{
> +	int ret = 0;
> +
> +	// Only Exar based cards use MPIO, return 0 otherwise
> +	if (priv->pcidev->vendor != PCI_VENDOR_ID_EXAR)
> +		return 0;

Same question here.

> +
> +	if (enable)
> +		ret = exar_mpio_set_low(priv, 0);
> +	else
> +		ret = exar_mpio_set_high(priv, 0);
> +	if (ret)
> +		return ret;
> +
> +	// Ensure MPIO is an output
> +	ret = exar_mpio_config_output(priv, 0);
> +
> +	return ret;
> +}
> +
> +static int cti_plx_int_enable(struct exar8250 *priv)
> +{
> +	return _cti_set_plx_int_enable(priv, true);

Again, no wrapper needed if you never actually call that function with
"false", right?  Or am I missing a path here?

thanks,

greg k-h