Messages in this thread | | | From | Anson Huang <> | Subject | RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support | Date | Thu, 16 May 2019 11:09:29 +0000 |
| |
Hi, Leonard
> -----Original Message----- > From: Leonard Crestez > Sent: Thursday, May 16, 2019 6:07 PM > To: Anson Huang <anson.huang@nxp.com>; shawnguo@kernel.org > Cc: catalin.marinas@arm.com; will.deacon@arm.com; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > agross@kernel.org; maxime.ripard@bootlin.com; olof@lixom.net; > horms+renesas@verge.net.au; jagan@amarulasolutions.com; > bjorn.andersson@linaro.org; marc.w.gonzalez@free.fr; > dinguyen@kernel.org; enric.balletbo@collabora.com; > l.stach@pengutronix.de; Aisheng Dong <aisheng.dong@nxp.com>; Abel Vesa > <abel.vesa@nxp.com>; robh@kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support > > On 16.05.2019 06:24, Anson Huang wrote: > > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce driver > > dependency into Kconfig as CONFIG_IMX_SCU must be selected to support > > i.MX SCU SoC driver, also need to use platform driver model to make > > sure IMX_SCU driver is probed before i.MX SCU SoC driver. > > > +#define imx_scu_revision(soc_rev) \ > > + soc_rev ? \ > > + kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & > 0xf) : \ > > + "unknown" > > > + id = of_match_node(imx_scu_soc_match, pdev->dev.of_node); > > + data = id->data; > > + if (data) { > > + soc_dev_attr->soc_id = data->name; > > + if (data->soc_revision) > > + soc_rev = data->soc_revision(); > > + } > > + > > + soc_dev_attr->revision = imx_scu_revision(soc_rev); > > + if (!soc_dev_attr->revision) > > + return -ENODEV; > > The imx_scu_revision macro returns either kasprintf or "unknown", never > NULL. So it's not clear what this return -ENODEV does exactly.
The kasprintf could return NULL though.
> > It makes more sense to return -ENODEV if get_soc_revision fails, so maybe > check "soc_rev != 0" instead? > > If you really want to check the kasprintf result then you should return - > ENOMEM for it. It would be clearer if you dropped the imx_scu_revision > revision macro and open-coded instead.
This makes more sense, I think maybe we can remove the imx_scu_revision macro, just use below code instead, and return -ENOMEM if kasprintf returns NULL.
113 soc_dev_attr->revision = soc_rev ? kasprintf(GFP_KERNEL, 114 "%d.%d", 115 (soc_rev >> 4) & 0xf, 116 soc_rev & 0xf) : "unknown"; 117 if (!soc_dev_attr->revision) 118 return -ENOMEM;
BTW, the soc-imx8.c looks like also having same issue, do you think we should fix it as well?
Anson
> > -- > Regards, > Leonard
| |