Messages in this thread | | | Subject | Re: [PATCH 6/8] serial: Add Tegra Combined UART driver | From | Mikko Perttunen <> | Date | Mon, 14 May 2018 10:36:35 +0300 |
| |
On 14.05.2018 01:20, Andy Shevchenko wrote: > On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> On 05/13/2018 05:16 PM, Andy Shevchenko wrote: >>> >>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com> >>> wrote: >>>> >>>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows >>>> multiplexing multiple "virtual UARTs" into a single hardware serial >>>> port. The TCU is the primary serial port on Tegra194 devices. >>>> >>>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes >>>> are part of Tegra HSP blocks that are already controlled by the Tegra >>>> HSP mailbox driver. > >>>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned >>>> int mctrl) >>>> +{ >>> >>> >>>> + (void)port; >>>> + (void)mctrl; >>> >>> >>> Huh? >> >> >> The serial core calls these callbacks without checking if they are set. They >> don't make sense for this driver so they are stubbed out. > > My question why do you need these ugly lines? I'm pretty sure no other > driver with stubs using such style.
It's my personal style, being explicit about unused variables in this way - I don't consider them ugly. But I can certainly remove them for the next version.
> >>>> +} > >>>> + if (written == 3) { >>>> + value |= 3 << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>> >>> >>>> + } >>> >>> >>> (1) >>> >>>> + } >>>> + >>>> + if (written) { >>>> + value |= written << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>>> + } >>> >>> >>> (2) >>> >>> These are code duplications. >> >> >> Indeed - the length of the duplicated code is so short, and the instances >> are so close to each other, that I don't find it necessary (or clearer) to >> have an extra function. > > It makes sense. Consider to refactor other way w/o duplication then.
I'll see if I can refactor it out.
> >>>> +static void tegra_tcu_uart_set_termios(struct uart_port *port, >>>> + struct ktermios *new, >>>> + struct ktermios *old) >>>> +{ >>>> + (void)port; >>>> + (void)new; >>>> + (void)old; >>>> +} >>> >>> >>> Remove those unused stub contents. >> >> >> Sure. I had these here so that we don't get unused parameter warnings, but I >> can just as well remove the parameter names. > > What warnings? How did you get them? We have them disabled as far as I > know even with W=1.
May be - it's just a habit, maybe from other projects where the warning is enabled.
> >> >>> >>>> + return uart_set_options(&tegra_tcu_uart_port, cons, >>>> + 115200, 'n', 8, 'n'); >>> >>> >>> Can't it be one line? >> >> >> It would be a total of 81 characters in length on one line, so no. > > So, yes. 1 character doesn't prevent us make the readability better. > Please, put to one line. >
Ok, I'll change this.
Thanks, Mikko
| |