Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220207232129.402882-1-athierry@redhat.com>
In-Reply-To: <20220207232129.402882-1-athierry@redhat.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 8 Feb 2022 12:31:49 +0200
Message-ID: <CAHp75VexSp1kijGHoOijSHB44fHTWqj6LZPzhSBhN2ms2huh-g@mail.gmail.com>
Subject: Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support
To:     Adrien Thierry <athierry@redhat.com>
Cc:     "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        linux-rpi-kernel <linux-rpi-kernel@lists.infradead.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jeremy Linton <jeremy.linton@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 8, 2022 at 11:13 AM Adrien Thierry <athierry@redhat.com> wrote:
>
> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to
> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI

TianoCore EDK II

> firmware.

...

>         /* get the clock - this also enables the HW */
> -       data->clk = devm_clk_get(&pdev->dev, NULL);

> -       if (IS_ERR(data->clk))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n");

You shouldn't remove the error handling. Even if optional there may be
other types of errors that need to be reported.

> +       data->clk = devm_clk_get_optional(&pdev->dev, NULL);

...


> +       bcm_data = device_get_match_data(&pdev->dev);

Move this closer to the condition where it's used the first time.

> +       /* Some UEFI implementations (e.g. tianocore/edk2 for the Raspberry Pi)
.
If there are some not doing that, can we end up in the situation when
for the same ID we have different offset?
Also, Why not go and fix that implementation?
Can you provide a DSDT excerpt to show how it looks there?

TianoCore EDK II

/*
 * Multi-line comments should look
 * like this.
 */

> +        * describe the miniuart with a base address that encompasses the auxiliary
> +        * registers shared between the miniuart and spi.

SPI

> +        *
> +        * This is due to historical reasons, see discussion here :
> +        * https://edk2.groups.io/g/devel/topic/87501357#84349
> +        *
> +        * We need to add the offset between the miniuart and auxiliary
> +        * registers to get the real miniuart base address.
> +        */
> +       if (bcm_data)
> +               offset = bcm_data->offset;

...

> +static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
> +       { "BCM2836", (kernel_ulong_t)&bcm2835_acpi_data },
> +       { }
> +};

I believe this ID is allocated by Broadcom. Can we have a confirmation, please?

-- 
With Best Regards,
Andy Shevchenko