Received-SPF: pass (google.com: domain of linux-kernel+bounces-148536-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 17 Apr 2024 09:17:35 -0400
From: Parker Newman <parker@finest.io>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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: <20240417091735.17686470@SWDEV2.connecttech.local>
In-Reply-To: <2024041726-fall-debunk-6cc5@gregkh>
References: <cover.1713270624.git.pnewman@connecttech.com>
	<354ff4aa9d8bdea1a21c503e685e81ac3df48eb8.1713270624.git.pnewman@connecttech.com>
	<2024041726-fall-debunk-6cc5@gregkh>
Organization: Connect Tech Inc.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:KFn6XB/g77w=;bS2dE+5r+ISIlTwNXr5YUaHmTX2
 vmKjNHqb/h0j45VCQQ48V+aCS+HpHTvC30y15zPvuFJuYNto8ggeutT8ir374S71RtxSYDxNY
 5Z4YHSt5FpzADA8m0uc2PAvaVUUckG6lXNIz3t91/MS1dngcSfLJHVtPzrsaSynFWJBD4jRyv
 weLDdkofogN2j3sazw3YXzZASi+qHlmY0yq5ABDgOOLTlLUylMWhshBQtS5G5zJso8phYO53l
 Ha8AtaVXDgbNKBAYqxOD/SL6i1V006gGGPfMLkxlAd8V9S0AUaJhN67sm81j4UPTN/cPglni0
 tko0vU8Q1Al9yNOrw8wXNNdLMsYbidr3yFG/6ttQYB58wofv1QdMZCwrDW5CF+zavGbtLdmvu
 9Ekh58/UveEkvqsvqogEr7nBb5HuagSWucVxuDMu/yfTsLNyoWzmgkq7oJZ/CYRN3K/DPSUxU
 MrgOF+qFa4sMyucNEU2mi9kOmKduc0QER/il4A7tTiYfy2yapWgNQes7+zq/mURMNYXUwuKC2
 /hKMFZEWZ9a7Epyt8knudbuYjHM3sqtgCA+r3yUKbgBZFCSWKAXHCki87TQOLh06Bxtowv2zK
 XKY4lfWY1/Xl93WTW9Q7pcQ97KjmtMO4SqN7QZ6PYv3HI2ZFXjph3/VcNxQJabu/hsytBDUkQ
 huQpHgIz8o31hm5AxJdFrhEVtRBfrobZ6x4HmPSltxOWvMwzUdDLfy8ywM+rkHlgBU0XXGEXz
 Y3FHNMXr+tA/jhuydpI4Mc+SMAvCL43tLdaNZtKVOHD01PjVGQ4Zis=

On Wed, 17 Apr 2024 13:24:20 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> 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?
>

I added dev to make the prints cleaner. I personally prefer:

	dev_err(priv->dev, ...);
to
	dev_err(&priv->pcidev->dev, ...);

or to adding a:
	struct device *dev =3D &priv->pcidev->dev;

to every function just for printing.

However, I do understand your point. I can drop dev if you prefer.

> > +/**
> > + * _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 th=
at 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 th=
e 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 =3D 0;
> > +
> > +	if (port_num >=3D priv->nr)
> > +		return -EINVAL;
> > +
> > +	// Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor !=3D 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?
>

Yes that can go now, it used to be needed but not now. I will remove.

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

Currently no. However, re-enabling tristate via a custom ioctl was a featu=
re in
our old out-of-tree driver (which was created prior to linux RS485 support=
).

I am not sure how it would be activated now, but I left enabling tristate =
as an
option in to make it easier down the line when we need it.

I can add a note to the patch or remove it if you would prefer.

> > +
> > +/**
> > + * _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 PC=
I
> > + * 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 =3D 0;
> > +
> > +	// Only Exar based cards use MPIO, return 0 otherwise
> > +	if (priv->pcidev->vendor !=3D PCI_VENDOR_ID_EXAR)
> > +		return 0;
>
> Same question here.
>

Same as above, not needed. I will remove.

> > +
> > +	if (enable)
> > +		ret =3D exar_mpio_set_low(priv, 0);
> > +	else
> > +		ret =3D exar_mpio_set_high(priv, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	// Ensure MPIO is an output
> > +	ret =3D 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?
>

This one is similar to _cti_set_tristate() but is less likely to be used.

Thanks again,
Parker

> thanks,
>
> greg k-h