Hi Andy, 

Thank you for review.

On 02/29/2016 04:29 PM, Andy Shevchenko wrote:

On Mon, Feb 29, 2016 at 2:02 PM, Aleksey Makarov
[off-list ref] wrote:

quoted

'ARM Server Base Boot Requiremets' [1] mentions SPCR (Serial Port
Console Redirection Table) [2] as a mandatory ACPI table that
specifies the configuration of serial console.

Parse this table and check if any registered console match the
description.  If it does, enable that console.

Introduce a new function acpi_console_check().  At the uart port
registration, this function checks if the ACPI SPCR table specifies
its argument of type struct uart_port to be a console
and if so calls add_preferred_console().

quoted

+       if (ACPI_FAILURE(status)) {
+               pr_err("could not get the table\n");

Is it worse to have on error level? Is it possible to have firmware
without this table? I think it would be a normal case for non-arm
world.
I'm also not sure if this message useful even on warn level.

I will delete the message in the next version, thank you.

quoted

+               return -ENOENT;
+       }
+
+       if (table->header.revision < 2) {
+               err = -EINVAL;
+               pr_err("wrong table version\n");

And this one quite good to have, indeed.

quoted

+ * acpi_console_check - Check if uart matches the console specified by SPCR.
+ *
+ * @uport:     uart port to check
+ *

Since you use sections, you may add:
+ * Description:

According to kernel-doc-nano-HOWTO.txt "Description: " is optional.

quoted

+ * This function checks if the ACPI SPCR table specifies @uport to be a console
+ * and if so calls add_preferred_console()
+ *
+ * Return: a non-error value if the console matches.

quoted

@@ -2654,8 +2655,17 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
                spin_lock_init(&uport->lock);
                lockdep_set_class(&uport->lock, &port_lock_key);
        }
-       if (uport->cons && uport->dev)
-               of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
+
+       /*
+        * Support both open FW and ACPI access to console definitions.
+        * Both of_console_check() and acpi_console_check() will call
+        * add_preferred_console() if a console definition is found.
+        */
+       if (uport->cons && uport->dev) {
+               if (!acpi_console_check(uport))

if (cond1) {
 if (cond2) {
...
 }
}

is equivalent to
if (cond1 && cond2) {
...
}

It is, but it's a style decision.  I would prefer to leave it as is because it emphasizes that
after meeting some condition we first call acpi_console_check() and then of_console_check().

Thank you
Aleksey Makarov

quoted

+                       of_console_check(uport->dev->of_node, uport->cons->name,
+                                        uport->line);
+       }