Re: [PATCH V8 2/2] serial: exar: Add RS-485 support for Sealevel XR17V35X based cards

["andriy.shevchenko@intel.com" <andriy.shevchenko@intel.com>]

On Fri, Sep 22, 2023 at 03:33:05PM +0000, Matthew Howell wrote:
> On Fri, 2023-09-22 at 17:48 +0300, andriy.shevchenko@intel.com wrote:
> > On Fri, Sep 22, 2023 at 02:22:11PM +0000, Matthew Howell wrote:

...

> > > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > > +                struct uart_8250_port *port, int idx)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     port->port.rs485_config = sealevel_rs485_config;
> > > +
> > > +     return 0;
> > > +}
> > 
> > This actually can be embedded into original pci_xr17v35x_setup() as
> > 
> >         if (pdev->subsystem_vendor == PCI_VENDOR_ID_SEALEVEL)
> >                 port->port.rs485_config = sealevel_rs485_config;
> 
> That was my original thought prior to the first submission, but I wasn't
> sure about adding vendor-specific changes into pci_xr17v35x_setup()
> since it appears that the convention in 8250_exar.c has been for vendors
> to have their own setup function when they needed to change the
> init/setup behaviour.
> 
> If that is not the case though and having this in pci_xr17v35x_setup()

Just use the common sense. The case you are now adding is using PCI IDs
(vendor and device) that are Exar's. So, I do not see any violation of
the above assumption. Checking for _sub_ IDs is fine, it's just a quirk
for the Exar based chips.

> is more appropriate I'll be happy to put it there instead of having yet
> another setup function.

...

> > > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> > 
> > This is kinda worries me. Original Exar card has 12 ports, why 16 is in use
> > for this one?
> 
> Ah, good catch. I had actually forgotten about the 12 port version. Will
> either fix or make redundant with move to pci_xr17v35x_setup() depending
> on feedback from my statement above about what is most appropriate.
> 
> > > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> > With the above suggestion this will be fixed automatically.

-- 
With Best Regards,
Andy Shevchenko