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

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

On Mon, Sep 11, 2023 at 09:22:45AM -0400, Matthew Howell wrote:
> On Wed, 6 Sep 2023, Andy Shevchenko wrote:
> > 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:

...

> > > > > +     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;
> > }
> 
> I will have to respectfully disagree on this. Splitting the function into 
> two still adds additional redirection, however small, to the function.

You mean in the C code? Because in assembly it will be the same (as long as
the compiler optimisation is on).

But besides that it's a common practice to split in case the function is long
enough to be on a single screen page (meaning the body of the conditional ~20
LoCs threshold).

> I would also like to point out that the level of indent I am using is not 
> uncommon in 8250_exar.c and as such I do not find the styling of the 
> function to be out of place. 

You are probably referring to actually two functions, i.e.
pci_fastcom335_setup() and xr17v35x_register_gpio(), right?

Each of these three cases (including yours) are different. I do not think
there is a common ground to support your way by pointing out on them.

> I will resubmit and try to address the other concerns raised by you and 
> Ilpo but unless Greg KH says otherwise I don't see any reason to change 
> the indent at this time.


-- 
With Best Regards,
Andy Shevchenko