Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220426201539.12829-1-sven@svenpeter.dev> <20220426201539.12829-7-sven@svenpeter.dev>
In-Reply-To: <20220426201539.12829-7-sven@svenpeter.dev>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Tue, 26 Apr 2022 23:07:17 +0200
Message-ID: <CAK8P3a263ojCtYRec2RwvvcY1H1k7chqXyRhovrWRXh=hFYgfQ@mail.gmail.com>
Subject: Re: [PATCH v3 6/6] nvme-apple: Add initial Apple SoC NVMe driver
To:     Sven Peter <sven@svenpeter.dev>
Cc:     Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        Hector Martin <marcan@marcan.st>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Marc Zyngier <maz@kernel.org>,
        Janne Grunau <j@jannau.net>, DTML <devicetree@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-nvme@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote:
>
> Apple SoCs such as the M1 come with an embedded NVMe controller that
> is not attached to any PCIe bus. Additionally, it doesn't conform
> to the NVMe specification and requires a bunch of changes to command
> submission and IOMMU configuration to work.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>

I did not do a detailed review of this again, but the previous version seemed
mostly fine already.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

One suggestion for a very minor improvement:

> +#ifdef CONFIG_PM_SLEEP
> +static int apple_nvme_resume(struct device *dev)
> +{
...
> +}
> +
> +static const struct dev_pm_ops apple_nvme_pm_ops = {
> +       .suspend        = apple_nvme_suspend,
> +       .resume         = apple_nvme_resume,
> +};
> +#endif
> +
> +static const struct of_device_id apple_nvme_of_match[] = {
> +       { .compatible = "apple,nvme-ans2" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, apple_nvme_of_match);
> +
> +static struct platform_driver apple_nvme_driver = {
> +       .driver = {
> +               .name = "nvme-apple",
> +               .of_match_table = apple_nvme_of_match,
> +#ifdef CONFIG_PM_SLEEP
> +               .pm = &apple_nvme_pm_ops,
> +#endif
> +       },

You can now use "static DEFINE_SIMPLE_DEV_PM_OPS()" to define
the pm operations without these #ifdefs.

       Arnd