Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 12 Jan 2023 20:46:43 +0800
From:   Clay Chang <clayc@hpe.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     <linux-kernel@vger.kernel.org>, <soc@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <verdun@hpe.com>,
        <nick.hawkins@hpe.com>, <arnd@arndb.de>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <linux@armlinux.org.uk>,
        <olof@lixom.net>
Subject: Re: [PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver
Message-ID: <Y8ABM1XNcyeSh7iZ@enigma.ccjz.io>
References: <20230110042533.12894-1-clayc@hpe.com>
 <20230110042533.12894-2-clayc@hpe.com>
 <34191e6f-9a00-114e-4fd8-15ccaec3535b@linaro.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <34191e6f-9a00-114e-4fd8-15ccaec3535b@linaro.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?VbwxreAm3HmdyB/US3dpLJmJDg/jdbN2hCPAHXdw8S79HM2XliW77W/nnwGD?=
 =?us-ascii?Q?sPrEjlMcpzTYixO8pQ3lHC1YRgm2n2tf+ALdb/0fzaM+gt02tmZLyK5hsF3i?=
 =?us-ascii?Q?bkf7ToTddvyD904wqY11IcbstXQL/z7TldTbm0Vj/Wytr/JVHY+GWrOD+qQ7?=
 =?us-ascii?Q?xb3zDz8ntmMm97hYseDs3kHg725buHUlcs6Hg/R25WqA5de/VCxoY396T9Sz?=
 =?us-ascii?Q?SN+WGr94i2G7FLzESP8Z5P5If1h8xZoW3TJD0+f+KhyUemb6cunRFAVMAI/O?=
 =?us-ascii?Q?0E2+EBw4Q/aw27QfQ9wo/trzREC1dKzLUvvmmKJeAyWKYntis2tTKkzTPooH?=
 =?us-ascii?Q?swXegdA5+bNqC1Zgf0KYmM9xWLUzEOgbKHGo89AitA5qcc09yzF6C07VeHph?=
 =?us-ascii?Q?/WoLdGmPLOSkUWBYgBc4SuacOx4jeQlpmBLaHbg5ibVpUSOh2TMn1jVyCprU?=
 =?us-ascii?Q?miP/CovaHnVBVUPtqQUSabJAfC6iy5+20NLDLuz4+sDAwqJMz0QY1V9KB2fS?=
 =?us-ascii?Q?RmrvHI6WltrhKbEHB/FNno4kyKF5TmmJkRLEduI15JlujbPJ80Jqzuusywaf?=
 =?us-ascii?Q?Et8lMpAT05G2v4++YGiyHXTc3hzOjbe2cjtB4GB+NvtArYXwdshpeza41Mvc?=
 =?us-ascii?Q?1R25/4SpQqJb7wtRV5DrtFss58HGz6LRrAChZf8xCRst2ZzWrlivUhapA6nK?=
 =?us-ascii?Q?cgBOtQ+6REXVQUGlczw7LcysijK3u1OlYQfbZw3mqXgtCf9miVrO/Msw8u+W?=
 =?us-ascii?Q?J7YN+cthj3PgAxCws+fCF/Fe0ElieMVnfjw+2sAbmsDWJkaSKY+LH1SuvFKh?=
 =?us-ascii?Q?EOi+8TPhw3T7FpJVP4irRVxB1IZJ6++lGR5f4/VRtsVoZXjw06cfo0S8z3Jn?=
 =?us-ascii?Q?AakNtuwNeN+v3PNPjFvf32fbj2Lg0kLD2s7XJWgYJLuW2923B6GZZ7aoswfR?=
 =?us-ascii?Q?H4hGYWbkpDUgblX87m/ZCM7q3UuHARX9WtZPa58mLc343HFvGURZYnQXMzZI?=
 =?us-ascii?Q?p4MiAzeU3bjY6zRk5IrKx/35aA0RGtH6fHkDGXmw8QSrOQzN4GnlfTiiDqIq?=
 =?us-ascii?Q?Y1YBlJkwvbgeUO4Jq56+feXPij0wr4tHjK+zEsdGKwvGjHOQa9fv4XSnFIsH?=
 =?us-ascii?Q?AvVpsp8+PunsdhbkxuOVAFiKIKfi9N7h/2u6xmgh1q/eRGkY/stcnbATiWXj?=
 =?us-ascii?Q?rI4+SzU0Vg8vccGEGdCAJbdS/+54ZCYkDWClYRg8XbT35R3ypu2Oh6SlpomQ?=
 =?us-ascii?Q?pryC7XgyHIZrc1ow0Obq1pzkFBKjiyPq38OUIuvRUekOLtrgj4s3j2c6nJnc?=
 =?us-ascii?Q?WmIepCb96HqQCkROufrRFTvg0TnW3Ol3rbytcZoQdX4I0VqGu9NbMdgilQJw?=
 =?us-ascii?Q?LWjqM7KBSqO72ZpLDvO6IT+YBFdWF0JUB2cSwHBcUGCqlmcHoKHM3K0T2xQU?=
 =?us-ascii?Q?gIcP2XzG4AklQ095zHBHXJD7dX/ZTDssPls0+XMo1gYz0EmNnnt3oL0hQO8c?=
 =?us-ascii?Q?rLBr8myHqIVHyMUxnEaYJLr8JFfGF36X2D6rHpnOg/8zNyFPJ8lOJrfg34K9?=
 =?us-ascii?Q?QYixdh1jhdTcsjtbKN+R6JkaGcUkdWz7T5P2HS68ZedM8BPi0Hdu0OkO9unR?=
 =?us-ascii?Q?Mw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b1d49ea-dc14-42fa-9736-08daf49b13cc
X-MS-Exchange-CrossTenant-AuthSource: PH0PR84MB1480.NAMPRD84.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2023 12:46:51.8521
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +noYIk4YiVVIJHeqv8HyXBiRSw2nW46YBF8txDKK6ycPWIvI6/88q6aLXmL5t+iJ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR84MB1673
X-OriginatorOrg: hpe.com
X-Proofpoint-GUID: 1-5fxM5HL8t_bgtPDXp_1CO-yqrr3xDm
X-Proofpoint-ORIG-GUID: 1-5fxM5HL8t_bgtPDXp_1CO-yqrr3xDm
X-HPE-SCL: -1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-12_07,2023-01-12_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0
 priorityscore=1501 phishscore=0 lowpriorityscore=0 mlxscore=0
 malwarescore=0 clxscore=1011 adultscore=0 impostorscore=0 spamscore=0
 mlxlogscore=999 suspectscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2212070000 definitions=main-2301120091
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Krzysztof,

Thank you for the time to review my code.

On Tue, Jan 10, 2023 at 10:46:57AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2023 05:25, clayc@hpe.com wrote:
> > From: Clay Chang <clayc@hpe.com>
> > 
> > The GXP SROM control register can be used to configure LPC related
> > legacy I/O registers. Currently only the SROM RAM Offset Register
> > (vromoff) is exported.
> > 
> > The GXP SOCLIB is a common library used for creating the common
> > "soc" class in the kernel.
> > 
> > Signed-off-by: Clay Chang <clayc@hpe.com>
> > ---
> >  drivers/soc/Kconfig          |   1 +
> >  drivers/soc/Makefile         |   1 +
> >  drivers/soc/hpe/Kconfig      |  29 +++++++
> >  drivers/soc/hpe/Makefile     |   2 +
> >  drivers/soc/hpe/gxp-soclib.c |  17 +++++
> >  drivers/soc/hpe/gxp-soclib.h |   9 +++
> >  drivers/soc/hpe/gxp-srom.c   | 141 +++++++++++++++++++++++++++++++++++
> >  7 files changed, 200 insertions(+)
> >  create mode 100644 drivers/soc/hpe/Kconfig
> >  create mode 100644 drivers/soc/hpe/Makefile
> >  create mode 100644 drivers/soc/hpe/gxp-soclib.c
> >  create mode 100644 drivers/soc/hpe/gxp-soclib.h
> >  create mode 100644 drivers/soc/hpe/gxp-srom.c
> > 
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 5dbb09f843f7..faff0f036b61 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/canaan/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> >  source "drivers/soc/fujitsu/Kconfig"
> > +source "drivers/soc/hpe/Kconfig"
> >  source "drivers/soc/imx/Kconfig"
> >  source "drivers/soc/ixp4xx/Kconfig"
> >  source "drivers/soc/litex/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index fff513bd522d..d257b9d654b3 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE)		+= dove/
> >  obj-y				+= fsl/
> >  obj-y				+= fujitsu/
> >  obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
> > +obj-$(CONFIG_ARCH_HPE)		+= hpe/
> >  obj-y				+= imx/
> >  obj-y				+= ixp4xx/
> >  obj-$(CONFIG_SOC_XWAY)		+= lantiq/
> > diff --git a/drivers/soc/hpe/Kconfig b/drivers/soc/hpe/Kconfig
> > new file mode 100644
> > index 000000000000..88f5d46b06b6
> > --- /dev/null
> > +++ b/drivers/soc/hpe/Kconfig
> > @@ -0,0 +1,29 @@
> > +#
> > +# HPE GXP SoC drivers
> > +#
> > +menu "HPE GXP SoC drivers"
> > +	depends on ARCH_HPE || COMPILE_TEST
> > +
> > +config HPE_GXP_SOCLIB
> > +	bool "GXP Common SoC Library"
> > +	default y
> > +	depends on ARCH_HPE_GXP || COMPILE_TEST
> > +	help
> > +	  This is for the common library for all HPE SoC drivers. It
> > +	  creates the root soc class (/sys/class/soc) for all GXP SoC
> > +	  drivers. It must be yes if any one of the GXP SoC drivers were
> > +	  added, so the config of all GXP SoC drivers must select this.
> 
> Don't open-code Kconfig dependencies in free-form text. IOW, drop last
> sentence.

Understood, I will correct this part in the next revision.

> 
> > +
> > +
> > +config HPE_GXP_SROM
> > +	bool "GXP SROM Configuration Driver"
> > +	default y
> > +	depends on ARCH_HPE_GXP || COMPILE_TEST
> > +	select HPE_GXP_SOCLIB
> > +	help
> > +	  Say yes here to add support for SROM Configuration. The GXP SROM
> > +	  control register can be used to configure LPC related legacy I/O
> > +	  registers. Currently only the SROM RAM Offset Register (vromoff)
> > +	  is exported.
> > +
> > +endmenu
> > diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
> > new file mode 100644
> > index 000000000000..78de24ecb606
> > --- /dev/null
> > +++ b/drivers/soc/hpe/Makefile
> > @@ -0,0 +1,2 @@
> > +obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
> > +obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
> > diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
> > new file mode 100644
> > index 000000000000..11b0afe09070
> > --- /dev/null
> > +++ b/drivers/soc/hpe/gxp-soclib.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (C) 2023 Hewlett Packard Enteprise Development Company, L.P. */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +
> > +struct class *soc_class;
> > +
> > +static int __init gxp_soclib_init(void)
> > +{
> > +	soc_class = class_create(THIS_MODULE, "soc");
> > +	if (IS_ERR(soc_class))
> > +		return PTR_ERR(soc_class);
> > +	return 0;
> > +}
> > +
> > +module_init(gxp_soclib_init);
> 
> I don't see a point of making it a shared object and a "kernel library".
> Module inits are not libraries. Drop entire file.

Sure, I will re-write this part in the next revision.

> 
> > diff --git a/drivers/soc/hpe/gxp-soclib.h b/drivers/soc/hpe/gxp-soclib.h
> > new file mode 100644
> > index 000000000000..eb0e72b67aee
> > --- /dev/null
> > +++ b/drivers/soc/hpe/gxp-soclib.h
> > @@ -0,0 +1,9 @@
> 
> 
> > +
> > +static int sysfs_register(struct device *parent,
> > +			  struct gxp_srom_drvdata *drvdata)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = device_create_with_groups(soc_class, parent, 0,
> > +					drvdata, srom_groups, "srom");
> > +	if (IS_ERR(dev))
> > +		return PTR_ERR(dev);
> > +	drvdata->dev = dev;
> > +	return 0;
> > +}
> > +
> > +static int gxp_srom_probe(struct platform_device *pdev)
> > +{
> > +	struct gxp_srom_drvdata *drvdata;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_srom_drvdata),
> 
> sizeof(*)

Ok, I will correct this in the next.

> 
> > +				GFP_KERNEL);
> > +	if (!drvdata)
> > +		return -ENOMEM;
> > +
> > +	drvdata->pdev = pdev;
> > +	platform_set_drvdata(pdev, drvdata);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "no srom resource defined\n");
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> > +
> > +	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> 
> Combine these two. There is a helper for this.
> 

Sure, I will combine these two with devm_platform_ioremap_resource.

> > +	if (IS_ERR(drvdata->base)) {
> > +		dev_err(&pdev->dev, "ioremap_resource error\n");
> > +		ret = PTR_ERR(drvdata->base);
> 
> return dev_err_probe().
> > +		goto ioremap_err;
> > +	}
> > +
> > +	mutex_init(&drvdata->mutex);
> > +
> > +	ret = sysfs_register(&pdev->dev, drvdata);
> 
> Missing ABI documentation.

Ok, I will provide with the proper ABI documentation in the next revision.

> 
> > +	if (ret != 0) {
> > +		dev_err(&pdev->dev, "sysfs error\n");
> 
> return dev_err_probe

Ok, will correct this in the next.

> 
> > +		goto sysfs_err;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "initialized\n");
> 
> Drop silly probe successes.

Sure, will drop this in the next revision.

> 
> > +	return 0;
> > +
> > +sysfs_err:
> > +ioremap_err:
> > +	platform_set_drvdata(pdev, NULL);
> > +out:
> > +	if (drvdata)
> > +		devm_kfree(&pdev->dev, (void *)drvdata);
> 
> 1. Why? if it is here, it must be in remove() callback. If it is invalid
> (because it is really not correct) for remove(), it cannot be here, right?
> 2. Why cast?

Ok, sorry for my mis-understanding about the error handling path in
device probing. I will re-write this part in the next revision.

> 
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id gxp_srom_of_match[] = {
> > +	{ .compatible = "hpe,gxp-srom" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, gxp_srom_of_match);
> > +
> > +static struct platform_driver gxp_srom_driver = {
> > +	.probe = gxp_srom_probe,
> > +	.driver = {
> > +		.name = "gxp-srom",
> > +		.of_match_table = of_match_ptr(gxp_srom_of_match),
> 
> That will cause a warning. Drop of_match_ptr.

Got it!

> 
> > +	},
> > +};
> > +module_platform_driver(gxp_srom_driver);
> > +
> > +MODULE_AUTHOR("Clay Chang <clayc@hpe.com>");
> > +MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
> > +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof
> 

Thanks,
Clay