lkml.org 
[lkml]   [2019]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
Date
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
\
 
 \ /
  Last update: 2019-05-16 13:10    [W:0.046 / U:3.396 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site