Re: [cip-dev] [PATCH v2 4.4.y-cip 03/11] soc: renesas: Identify SoC and register with the SoC bus

From: "Pavel Machek" <pavel@denx.de>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc: cip-dev@lists.cip-project.org,
	Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
	Pavel Machek <pavel@denx.de>,
	Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [cip-dev] [PATCH v2 4.4.y-cip 03/11] soc: renesas: Identify SoC and register with the SoC bus
Date: Tue, 1 Dec 2020 10:21:49 +0100	[thread overview]
Message-ID: <20201201092149.GA4219@duo.ucw.cz> (raw)
In-Reply-To: <20201201083938.32688-4-prabhakar.mahadev-lad.rj@bp.renesas.com>

[-- Attachment #1.1: Type: text/plain, Size: 1823 bytes --]

Hi!

> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> commit 8d6799a9ba23acd675f3243580ee6f1756fb4381 upstream.
> 
> Identify the SoC type and revision, and register this information with
> the SoC bus, so it is available under /sys/devices/soc0/, and can be
> checked where needed using soc_device_match().
> 
> Identification is done using the Product Register or Common Chip Code
> Register, as declared in DT (PRR only for now), or using a hardcoded
> fallback if missing.

Just a minor nitpicks; no need to fix them for merge but maybe
consideration for mainline?

> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr)
> +		return -ENOMEM;
> +
> +	np = of_find_node_by_path("/");
> +	of_property_read_string(np, "model", &soc_dev_attr->machine);
> +	of_node_put(np);
> +
> +	soc_dev_attr->family = kstrdup_const(family->name, GFP_KERNEL);
> +	soc_dev_attr->soc_id = kstrdup_const(strchr(match->compatible, ',') + 1,
> +					     GFP_KERNEL);
> +	if (chipid)
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "ES%u.%u",
> +						   ((product >> 4) & 0x0f) + 1,
> +						   product & 0xf);

If memory allocations fail here, would it be better to fail the probe?

Would it be better to return error if !strchr(match->compatible, ',')
instead of trying to kstrdup_const((void *) 1) and oopsing?

> +	pr_info("Detected Renesas %s %s %s\n", soc_dev_attr->family,
> +		soc_dev_attr->soc_id, soc_dev_attr->revision ?: "");

Revision is NULL is handled here, but family may well be NULL, too, if
allocation failed. It is not going to cause problems here, but...

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 420 bytes --]

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#5925): https://lists.cip-project.org/g/cip-dev/message/5925
Mute This Topic: https://lists.cip-project.org/mt/78628861/4520388
Group Owner: cip-dev+owner@lists.cip-project.org
Unsubscribe: https://lists.cip-project.org/g/cip-dev/leave/8129055/727948398/xyzzy [cip-dev@archiver.kernel.org]
-=-=-=-=-=-=-=-=-=-=-=-