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;
Date:   Thu, 15 Apr 2021 10:27:58 +0900
From:   Dominique MARTINET <dominique.martinet@atmark-techno.com>
To:     "Alice Guo (OSS)" <alice.guo@oss.nxp.com>
Cc:     Shawn Guo <shawnguo@kernel.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Peng Fan <peng.fan@nxp.com>, dl-linux-imx <linux-imx@nxp.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [EXT] regression due to soc_device_match not handling defer
 (Was: [PATCH v4 4/4] soc: imx8m: change to use platform driver)
Message-ID: <YHeWnuDQo76rYoz5@atmark-techno.com>
References: <20201120101112.31819-4-alice.guo@nxp.com>
 <YGGZJjAxA1IO+/VU@atmark-techno.com>
 <AM6PR04MB60536EF0DEEE6EB64CF29390E27D9@AM6PR04MB6053.eurprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <AM6PR04MB60536EF0DEEE6EB64CF29390E27D9@AM6PR04MB6053.eurprd04.prod.outlook.com>
Precedence: bulk

Alice Guo (OSS) wrote on Tue, Mar 30, 2021 at 02:41:23AM +0000:
> Thanks for reporting this issue, I'll check and add a fix to handle defer probe.

I haven't seen any follow up on this, have you had a chance to take a
look?
If this won't make it for 5.12 (in a couple of week probably?) would it
make sense to revert 7d981405d0fd ("soc: imx8m: change to use platform
driver") for now?


While looking at the code earlier I also have an unrelated, late-review
on the patch itself:

> +static u32 __init imx8mq_soc_revision(struct device *dev)
> [...]
>  @@ -191,8 +223,16 @@ static int __init imx8_soc_init(void)
>         data = id->data;
>         if (data) {
>                 soc_dev_attr->soc_id = data->name;
> -               if (data->soc_revision)
> -                       soc_rev = data->soc_revision();
> +               if (data->soc_revision) {
> +                       if (pdev) {
> +                               soc_rev = data->soc_revision(&pdev->dev);
> +                               ret = soc_rev;
> +                               if (ret < 0)

I appreciate current soc_revision are "small enough" (looking at
include/soc/imx/revision.h we're talking < 256) so this actually works,
but would it make sense to either make soc_rev signed, or to have
soc_revision() return a s64, or have the revision filled in another *u32
argument to make sure the error is an error and not just a large rev?

This is most definitely fine for now but that kind of code patterns can
lead to weird errors down the road.

Thanks,
-- 
Dominique