Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-85-gd6d859e0cf-fm-20230116.001-gd6d859e0
Mime-Version: 1.0
Message-Id: <85b86d06-c63c-4481-a3dd-16b72572a5ee@app.fastmail.com>
In-Reply-To: <20230123073305.149940-7-lchen@ambarella.com>
References: <20230123073305.149940-1-lchen@ambarella.com>
 <20230123073305.149940-7-lchen@ambarella.com>
Date:   Mon, 23 Jan 2023 09:29:06 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Li Chen" <lchen@ambarella.com>, "Li Chen" <me@linux.beauty>
Cc:     =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        "Lubomir Rintel" <lkundrak@v3.sk>,
        "Conor.Dooley" <conor.dooley@microchip.com>,
        "Robert Jarzmik" <robert.jarzmik@free.fr>,
        "Sven Peter" <sven@svenpeter.dev>,
        "Yinbo Zhu" <zhuyinbo@loongson.cn>,
        "Brian Norris" <briannorris@chromium.org>,
        "Hitomi Hasegawa" <hasegawa-hitomi@fujitsu.com>,
        "open list" <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 06/15] soc: add Ambarella driver
Content-Type: text/plain
Precedence: bulk

On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> This driver add soc_id support for Ambarella,
> which is stored inside "cpuid" AXI address mapping.
>
> Also provide sys_config(POC, aka power on configuration)
> for other drivers.
>
> Signed-off-by: Li Chen <lchen@ambarella.com>

The soc_id support looks ok

> Change-Id: I4869a3497366ac7779e792835f8e0309239036a8

Please drop these lines in the submission, the IDs are
not reachable outside of your own git, so we don't want
these to show up in the public history.

> +static struct ambarella_soc_id {
> +	unsigned int id;
> +	const char *name;
> +	const char *family;
> +} soc_ids[] = {
> +	{ 0x00483245, "s6lm",  "10nm", },
> +};

I would suggest something more descriptive in the "family"
field to let users know they are on an Ambarella SoC.

Maybe just "Ambarella 10nm".

> +static int __init ambarella_socinfo_init(void)
> +{
> +	struct soc_device_attribute *soc_dev_attr;
> +	struct soc_device *soc_dev;
> +	struct device_node *np;
> +	struct regmap *cpuid_regmap;
> +	unsigned int soc_id;
> +
> +	cpuid_regmap = syscon_regmap_lookup_by_compatible("ambarella,cpuid");
> +	if (IS_ERR(cpuid_regmap))
> +		return PTR_ERR(cpuid_regmap);

Is there anything else in this syscon node? If the block
of registers only contains the identification bits, you
could just make this file a platform_driver that binds to
the node instead of using a syscon.

If there are other unrelated registers in there, the compatible
string should probably be changed to better describe the
entire area based on the name in the datasheet.

> +static unsigned int ambsys_config;
> +
> +unsigned int ambarella_sys_config(void)
> +{
> +	return ambsys_config;
> +}
> +EXPORT_SYMBOL(ambarella_sys_config);

Which drivers use this bit? Can they be changed to 
use soc_device_match() instead to avoid the export?

> +static int __init ambarella_soc_init(void)
> +{
> +	struct regmap *rct_regmap;
> +	int ret;
> +
> +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
> +	if (IS_ERR(rct_regmap)) {
> +		pr_err("failed to get ambarella rct regmap\n");
> +		return PTR_ERR(rct_regmap);
> +	}
...
> +arch_initcall(ambarella_soc_init);

It is not an error to use a chip from another manufacturer,
please drop the pr_err() and return success here.

> +#ifndef __SOC_AMBARELLA_MISC_H__
> +#define __SOC_AMBARELLA_MISC_H__
> +
> +extern unsigned int ambarella_sys_config(void);
> +extern struct proc_dir_entry *ambarella_proc_dir(void);
> +

The ambarella_proc_dir looks like a stale entry that should be
removed. Ideally you should not need a private header at all.

    Arnd