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@xxxxxxxxxx> 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);
>> + }
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
