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

[Matthew Howell <matthew.howell@sealevel.com>]

[-- Attachment #1: Type: text/plain, Size: 6654 bytes --]


On Tue, 22 Aug 2023, Ilpo Järvinen wrote:

> ⚠Caution: External email. Exercise extreme caution with links or attachments.⚠
> 
> 
> On Mon, 21 Aug 2023, Matthew Howell wrote:
> 
> Thanks for the patch.
> 
> > From: Matthew Howell <matthew.howell@sealevel.com>
> >
> > Sealevel XR1735X based cards utilize DTR to control RS-485 Enable, but the
> > current implementation of 8250_exar uses RTS for the auto-RS485-Enable
> > mode of the XR17V35X UARTs.
> >
> > sealevel_rs485_config(): Configures XR17V35X registers for Auto-RS485
> > Enable using DTR.
> > pci_sealevel_startup(): Calls pci_xr17v35x_setup(), then sets
> > rs485_config to sealevel_rs485_config().
> 
> The changelog doesn't read well as is. The logical flow breaks when you
> jump to describe functions.
> 
> > signed-off-by: Matthew Howell <matthew.howell@sealevel.com>
> 
> Start with capital S.
> 
> > ---
> > diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> > index 018cbaaf238c..246cfb3cc3f8 100644
> > --- a/drivers/tty/serial/8250/8250_exar.c
> > +++ b/drivers/tty/serial/8250/8250_exar.c
> > @@ -390,6 +390,8 @@ static void __xr17v35x_unregister_gpio(struct platform_device *pdev)
> >       platform_device_unregister(pdev);
> >  }
> >
> > +
> > +
> 
> Please remove this extra change.
> 
> >  static const struct property_entry exar_gpio_properties[] = {
> >       PROPERTY_ENTRY_U32("exar,first-pin", 0),
> >       PROPERTY_ENTRY_U32("ngpios", 16),
> > @@ -439,6 +441,35 @@ static int generic_rs485_config(struct uart_port *port, struct ktermios *termios
> >       return 0;
> >  }
> >
> > +static int sealevel_rs485_config(struct uart_port *port, struct ktermios *termios,
> > +                             struct serial_rs485 *rs485)
> > +{
> > +     bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
> 
> No need for !!() if assigning to bool.
> 

I just did it that way because generic_rs485_config() does it the same and
I assumed there was a reason for doing it that way. Are you sure !!() 
isn't needed here for some reason?

> > +     u8 __iomem *p = port->membase;
> > +     u8 old_lcr;
> > +    generic_rs485_config(port, termios, rs485);
> 
> Use tab to indent. Leave one empty line after declarations.
> 
> > +
> > +     if (is_rs485) {
> > +             // Set EFR[4]=1 to enable enhanced feature registers
> > +             writeb(readb(p + UART_XR_EFR) | 0x10, p + 0x09);
> > +
> > +             // Set MCR to use DTR as Auto-RS485 Enable signal
> > +             writeb(0x04, p + UART_MCR);
> > +
> > +             // Store original LCR and set LCR[7]=1 to enable access to DLD register
> > +             old_lcr = readb(p + UART_LCR);
> > +             writeb(old_lcr | 0x80, p + UART_LCR);
> > +
> > +             // Set DLD[7]=1 for inverted RS485 Enable logic
> > +             writeb(readb(p + 0x02) | 0x80, p + 0x02);
> 
> Please don't use numeric literals. You should add defines (or use existing
> ones) with proper names in all these cases.
> 
> If the names for the defines are good enough, the comments become
> redundant and can be removed so please check that too after naming things
> properly.
> 
> > +
> > +             // Reset LCR to orginal value
> 
> Unnecessary (very obvious) comment, please drop it.
> 
> > +             writeb(old_lcr, p + UART_LCR);
> > +    }
> > +
> > +     return 0;
> > + }
> > +
> >  static const struct serial_rs485 generic_rs485_supported = {
> >       .flags = SER_RS485_ENABLED,
> >  };
> > @@ -744,6 +775,19 @@ static int __maybe_unused exar_resume(struct device *dev)
> >       return 0;
> >  }
> >
> > +static int pci_sealevel_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> > +                                                       struct uart_8250_port *port, int idx)
> > +{
> > +     int ret = pci_xr17v35x_setup(priv, pcidev, port, idx);
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     port->port.rs485_config = sealevel_rs485_config;
> 
> Do you need to setup .rs485_supported too? (I'm not sure how the init
> works with this driver whether the default one gets used by some logic
> or not).
> 
> --
>  i.

I don't believe so since .rs485_supported is assigned in 
pci_xr17v35x_setup(). If I assigned .rs485_supported here I would just use 
the same code as pci_xr17v35x_setup(), which seems like needless 
duplication to me.

Will fix the various small issues and resubmit within the next few days.

> 
> 
> > +     return ret;
> > +}
> > +
> >  static SIMPLE_DEV_PM_OPS(exar_pci_pm, exar_suspend, exar_resume);
> >
> >  static const struct exar8250_board pbn_fastcom335_2 = {
> > @@ -809,6 +853,17 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> >       .exit           = pci_xr17v35x_exit,
> >  };
> >
> > +static const struct exar8250_board pbn_sealevel = {
> > +     .setup          = pci_sealevel_setup,
> > +     .exit           = pci_xr17v35x_exit,
> > +};
> > +
> > +static const struct exar8250_board pbn_sealevel_16 = {
> > +     .num_ports  = 16,
> > +    .setup           = pci_sealevel_setup,
> > +     .exit           = pci_xr17v35x_exit,
> > +};
> > +
> >  #define CONNECT_DEVICE(devid, sdevid, bd) {                          \
> >       PCI_DEVICE_SUB(                                                 \
> >               PCI_VENDOR_ID_EXAR,                                     \
> > @@ -838,6 +893,15 @@ static const struct exar8250_board pbn_exar_XR17V8358 = {
> >               (kernel_ulong_t)&bd                     \
> >       }
> >
> > +#define SEALEVEL_DEVICE(devid, bd) {                 \
> > +     PCI_DEVICE_SUB(                                 \
> > +             PCI_VENDOR_ID_EXAR,                     \
> > +             PCI_DEVICE_ID_EXAR_##devid,             \
> > +             PCI_VENDOR_ID_SEALEVEL,                 \
> > +             PCI_ANY_ID), 0, 0,      \
> > +             (kernel_ulong_t)&bd                     \
> > +     }
> > +
> >  static const struct pci_device_id exar_pci_tbl[] = {
> >       EXAR_DEVICE(ACCESSIO, COM_2S, pbn_exar_XR17C15x),
> >       EXAR_DEVICE(ACCESSIO, COM_4S, pbn_exar_XR17C15x),
> > @@ -886,6 +950,12 @@ static const struct pci_device_id exar_pci_tbl[] = {
> >       EXAR_DEVICE(COMMTECH, 2324PCI335, pbn_fastcom335_4),
> >       EXAR_DEVICE(COMMTECH, 2328PCI335, pbn_fastcom335_8),
> >
> > +     SEALEVEL_DEVICE(XR17V352, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V354, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V358, pbn_sealevel),
> > +     SEALEVEL_DEVICE(XR17V4358, pbn_sealevel_16),
> > +     SEALEVEL_DEVICE(XR17V8358, pbn_sealevel_16),
> > +
> >       { 0, }
> >  };
> >  MODULE_DEVICE_TABLE(pci, exar_pci_tbl);
> >
>