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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Sep 06, 2023 at 11:05:20AM -0400, Matthew Howell wrote:
> On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > On Tue, Sep 05, 2023 at 12:06:20PM -0400, Matthew Howell wrote:

...

> > > +     generic_rs485_config(port, termios, rs485);
> > 
> > > +     if (rs485->flags & SER_RS485_ENABLED) {
> > 
> > What I meant is to have
> > 
> >         if (!)rs485->flags & SER_RS485_ENABLED))
> >                 return 0;
> > 
> > here, which allows you to reduce indentation level in the below block.
> > 
> > > +             /* Set EFR[4]=1 to enable enhanced feature registers */
> > > +             writeb(readb(p + UART_XR_EFR) | UART_EFR_ECB, p + UART_XR_EFR);
> > > +
> > > +             /* Set MCR to use DTR as Auto-RS485 Enable signal */
> > > +             writeb(UART_MCR_OUT1, 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 | UART_LCR_DLAB, p + UART_LCR);
> > > +
> > > +             /* Set DLD[7]=1 for inverted RS485 Enable logic */
> > > +             writeb(readb(p + UART_EXAR_DLD) | UART_EXAR_DLD_485_POLARITY, p + UART_EXAR_DLD);
> > > +
> > > +             writeb(old_lcr, p + UART_LCR);
> > > +    }
> > > +
> > > +     return 0;
> 
> I see where you are coming from now, but I find that slightly less clear 
> than having the 'main action' within the conditional statement. And since 
> the code is not heavily indented I don't see much benefit of removing the 
> indent.

In that case it might make sense to split to two functions:

func1()
{
	...
}

func2()
{
	if (...)
		return func1()

	return 0;
}

-- 
With Best Regards,
Andy Shevchenko