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
