lkml.org 
[lkml]   [2016]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 1/4] ACPI: parse SPCR and enable matching console
    From
    Date
    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
    > <aleksey.makarov@linaro.org> wrote:
    >> '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().
    >
    >> + 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.

    >> + return -ENOENT;
    >> + }
    >> +
    >> + if (table->header.revision < 2) {
    >> + err = -EINVAL;
    >> + pr_err("wrong table version\n");
    >
    > And this one quite good to have, indeed.
    >
    >> + * 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.

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

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

    \
     
     \ /
      Last update: 2016-02-29 15:41    [W:5.236 / U:0.512 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site