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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Tue, 22 Aug 2023, Matthew Howell wrote:
> On Tue, 22 Aug 2023, Ilpo Järvinen wrote:
> > 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?

Implicit conversion to bool handles it just fine without those (and as 
always, sure there are many examples in the kernel with those but it 
doesn't mean we should keep adding more).

Although now that I look more into the surrounding code, it seems that 
is_rs485 only used in the if condition so having a local bool for that 
seems a bit overkill and the if condition could have that check.

> > > +     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.

Okay, thanks for checking.

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

-- 
 i.