linux-kernel - Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 8 Feb 2022 12:31:49 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Adrien Thierry <athierry@...hat.com>
Cc:     "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        linux-rpi-kernel <linux-rpi-kernel@...ts.infradead.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jeremy Linton <jeremy.linton@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

On Tue, Feb 8, 2022 at 11:13 AM Adrien Thierry <athierry@...hat.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