On Wed, 17 Apr 2024 13:24:20 +0200
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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 = &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 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?
>
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 = 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"?
>
Currently no. However, re-enabling tristate via a custom ioctl was a feature 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 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.
>
Same as above, not needed. I will remove.
> > +
> > + 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?
>
This one is similar to _cti_set_tristate() but is less likely to be used.
Thanks again,
Parker
> thanks,
>
> greg k-h
