From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH v2 21/41] arm : acpi Initialize serial port
 from ACPI SPCR table
Date: Tue, 26 May 2015 17:04:40 +0200
Message-ID: <55648B88.7050006@citrix.com>
References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org>
	<1431893048-5214-22-git-send-email-parth.dixit@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1431893048-5214-22-git-send-email-parth.dixit@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Parth Dixit <parth.dixit@linaro.org>, xen-devel@lists.xen.org
Cc: keir@xen.org, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, jbeulich@suse.com, christoffer.dall@linaro.org
List-Id: xen-devel@lists.xenproject.org

Hi Parth,

On 17/05/2015 22:03, Parth Dixit wrote:
> @@ -307,6 +308,54 @@ DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL)
>           .init = dt_pl011_uart_init,
>   DT_DEVICE_END
>
> +#ifdef CONFIG_ACPI
> +static int __init acpi_pl011_uart_init(const void *data)
> +{
> +    struct pl011 *uart;
> +    acpi_status status;
> +    struct acpi_table_spcr *spcr=NULL;
> +    int res;
> +
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +            (struct acpi_table_header **)&spcr);

Indentation.

And I think this could have been done in the generic UART code. Every 
UART driver will likely need to get the SPCR table.

> +
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk("\nFailed to get SPCR table \n");

No need of the first newline and the last space.

> +        return 1;
> +    }
> +
> +    uart = &pl011_com;
> +
> +    if ( spcr->interrupt < 0 )

No need of the check, the field interrupt is an u32. Is there any other 
way to find check if the interrupt is valid?

> +    {
> +        printk("pl011: Unable to retrieve the IRQ\n");
> +        return -EINVAL;
> +    }
> +
> +    uart->irq = spcr->interrupt;
> +    /* trigger/polarity information is not available in spcr */

If so, how did you find/device that the interrupt is edge? Shouldn't we 
just avoid to configure it?

Regards,

-- 
Julien Grall