LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mikko Perttunen <cyndis@kapsi.fi>
To: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Mikko Perttunen <mperttunen@nvidia.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jon Hunter <jonathanh@nvidia.com>,
	araza@nvidia.com, devicetree <devicetree@vger.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	linux-tegra@vger.kernel.org,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 6/8] serial: Add Tegra Combined UART driver
Date: Sun, 13 May 2018 21:04:51 +0300	[thread overview]
Message-ID: <662ce972-56a9-9daa-d332-faa08b89c05a@kapsi.fi> (raw)
In-Reply-To: <CAHp75VfYF_u2bZ+temGbReEB9K-t3A75aUNYwffyv9-=zEi0Lw@mail.gmail.com>

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.
> 
> First question, can it be done utilizing SERDEV framework?

Based on some brief research, the SERDEV framework is for devices that 
are behind some UART interface. In this case, this driver implements the 
UART interface itself, so by my understanding SERDEV is not appropriate. 
Please correct me if I'm wrong.

> 
>> +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.

> 
>> +}
> 
>> +static void tegra_tcu_uart_stop_tx(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
> 
> Ditto.
> 
>> +               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.

> 
>> +static void tegra_tcu_uart_stop_rx(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
>> +
>> +static void tegra_tcu_uart_break_ctl(struct uart_port *port, int ctl)
>> +{
>> +       (void)port;
>> +       (void)ctl;
>> +}
>> +
>> +static int tegra_tcu_uart_startup(struct uart_port *port)
>> +{
>> +       (void)port;
>> +
>> +       return 0;
>> +}
>> +
>> +static void tegra_tcu_uart_shutdown(struct uart_port *port)
>> +{
>> +       (void)port;
>> +}
>> +
>> +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.

> 
>> +       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.

> 
>> +static void tegra_tcu_receive(struct mbox_client *client, void *msg_p)
>> +{
>> +       struct tty_port *port = &tegra_tcu_uart_port.state->port;
> 
>> +       uint32_t msg = *(uint32_t *)msg_p;
> 
> Redundant casting.

Will remove.

> 
>> +       unsigned int num_bytes;
>> +       int i;
>> +
> 
>> +       num_bytes = (msg >> 24) & 0x3;
> 
> Two magic numbers.

Sure, will add defines.

> 
>> +       for (i = 0; i < num_bytes; i++)
>> +               tty_insert_flip_char(port, (msg >> (i*8)) & 0xff, TTY_NORMAL);
>> +
>> +       tty_flip_buffer_push(port);
>> +}
> 
>> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("NVIDIA Tegra Combined UART driver");
>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index dce5f9dae121..eaf3c303cba6 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -281,4 +281,7 @@
>>   /* MediaTek BTIF */
>>   #define PORT_MTK_BTIF  117
>>
>> +/* NVIDIA Tegra Combined UART */
>> +#define PORT_TEGRA_TCU 118
> 
> Check if there is an unused gap. IIRC we still have one near to 40ish.
> 

Correct, looks like 41-43 are unused. I'll change this 41.

Thanks for reviewing!

Mikko

  reply	other threads:[~2018-05-13 18:05 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08 11:43 [PATCH 0/8] " Mikko Perttunen
2018-05-08 11:43 ` [PATCH 1/8] dt-bindings: tegra186-hsp: Add shared interrupts Mikko Perttunen
2018-05-22 15:15   ` Jon Hunter
2018-06-19 12:41     ` Mikko Perttunen
2018-05-08 11:43 ` [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu Mikko Perttunen
2018-05-22 15:15   ` Jon Hunter
2018-05-22 20:18     ` Rob Herring
2018-05-22 20:20   ` Rob Herring
2018-05-08 11:43 ` [PATCH 3/8] mailbox: Add transmit done by blocking option Mikko Perttunen
2018-05-08 11:43 ` [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes Mikko Perttunen
2018-05-22 15:36   ` Jon Hunter
2018-06-19 12:52     ` Mikko Perttunen
2018-05-08 11:44 ` [PATCH 5/8] mailbox: tegra-hsp: Add support for shared mailboxes Mikko Perttunen
2018-05-22 16:20   ` Jon Hunter
2018-05-08 11:44 ` [PATCH 6/8] serial: Add Tegra Combined UART driver Mikko Perttunen
2018-05-13 14:16   ` Andy Shevchenko
2018-05-13 18:04     ` Mikko Perttunen [this message]
2018-05-13 22:20       ` Andy Shevchenko
2018-05-14  7:36         ` Mikko Perttunen
2018-05-13 15:36   ` Jassi Brar
2018-05-13 18:06     ` Mikko Perttunen
2018-05-08 11:44 ` [PATCH 7/8] arm64: tegra: Add nodes for tcu on Tegra194 Mikko Perttunen
2018-05-22 16:28   ` Jon Hunter
2018-05-08 11:44 ` [PATCH 8/8] arm64: tegra: Mark tcu as primary serial port on Tegra194 P2888 Mikko Perttunen
2018-05-22 16:29   ` Jon Hunter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=662ce972-56a9-9daa-d332-faa08b89c05a@kapsi.fi \
    --to=cyndis@kapsi.fi \
    --cc=andy.shevchenko@gmail.com \
    --cc=araza@nvidia.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jassisinghbrar@gmail.com \
    --cc=jonathanh@nvidia.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mperttunen@nvidia.com \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --subject='Re: [PATCH 6/8] serial: Add Tegra Combined UART driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).