lkml.org 
[lkml]   [2018]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 6/8] serial: Add Tegra Combined UART driver
    From
    Date
    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

    \
     
     \ /
      Last update: 2018-05-14 09:37    [W:3.420 / U:0.380 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site