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
Date: Thu, 12 Jan 2023 20:46:43 +0800 [thread overview]
Message-ID: <Y8ABM1XNcyeSh7iZ@enigma.ccjz.io> (raw)
In-Reply-To: <34191e6f-9a00-114e-4fd8-15ccaec3535b@linaro.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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-12 12:49 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-10 4:25 [PATCH 0/5] ARM: Add GXP SROM Support clayc
2023-01-10 4:25 ` [PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver clayc
2023-01-10 9:46 ` Krzysztof Kozlowski
2023-01-12 12:46 ` Clay Chang [this message]
2023-01-10 4:25 ` [PATCH 2/5] dt-bindings: soc: hpe: hpe,gxp-srom.yaml clayc
2023-01-10 9:49 ` Krzysztof Kozlowski
2023-01-12 13:16 ` Clay Chang
2023-01-12 13:37 ` Arnd Bergmann
2023-01-16 13:42 ` Clay Chang
2023-01-16 15:18 ` Arnd Bergmann
2023-01-19 7:39 ` Clay Chang
2023-01-19 7:56 ` Arnd Bergmann
2023-01-10 4:25 ` [PATCH 3/5] ARM: dts: hpe: Add SROM Driver clayc
2023-01-10 4:25 ` [PATCH 4/5] ARM: multi_v7_defconfig: Add GXP " clayc
2023-01-10 9:50 ` Krzysztof Kozlowski
2023-01-12 13:17 ` Clay Chang
2023-01-10 4:25 ` [PATCH 5/5] MAINTAINERS: Add maintainer of GXP SROM support clayc
2023-01-10 9:51 ` Krzysztof Kozlowski
2023-01-12 13:18 ` Clay Chang
2023-01-20 2:21 ` [PATCH 0/5] ARM: Add GXP SROM Support Andrew Jeffery
2023-01-31 13:46 ` Clay Chang
2023-02-01 13:28 ` Clay Chang
2023-02-02 1:12 ` Andrew Jeffery
2023-02-02 15:25 ` Clay Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8ABM1XNcyeSh7iZ@enigma.ccjz.io \
--to=clayc@hpe.com \
--cc=arnd@arndb.de \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=nick.hawkins@hpe.com \
--cc=olof@lixom.net \
--cc=robh+dt@kernel.org \
--cc=soc@kernel.org \
--cc=verdun@hpe.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).