ARM: Add GXP SROM Support

[PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver

Posted by clayc@hpe.com 3 years ago

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.
+
+
+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);
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 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2023 Hewlett Packard Enterprise Development Company, L.P. */
+
+#ifndef __GXP_SOCLIB_H__
+#define __GXP_SOCLIB_H__
+
+extern struct class *soc_class;
+
+#endif
diff --git a/drivers/soc/hpe/gxp-srom.c b/drivers/soc/hpe/gxp-srom.c
new file mode 100644
index 000000000000..5a8f005055cf
--- /dev/null
+++ b/drivers/soc/hpe/gxp-srom.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett Packard Enterprise Development Company, L.P. */
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#include "gxp-soclib.h"
+
+#define SROM_VROMOFF		0xf4 // 80fc_00f4
+
+struct gxp_srom_drvdata {
+	struct platform_device *pdev;
+	struct device *dev;
+	void __iomem *base;
+	struct mutex mutex;
+};
+
+static ssize_t vromoff_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct gxp_srom_drvdata *drvdata = dev_get_drvdata(dev);
+	unsigned int value;
+	ssize_t ret;
+
+	mutex_lock(&drvdata->mutex);
+	value = readl(drvdata->base + SROM_VROMOFF);
+	ret = sprintf(buf, "0x%08x", value);
+	mutex_unlock(&drvdata->mutex);
+
+	return ret;
+}
+
+static ssize_t vromoff_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct gxp_srom_drvdata *drvdata = dev_get_drvdata(dev);
+	unsigned int value;
+	int rc;
+
+	rc = kstrtouint(buf, 0, &value);
+	if (rc < 0)
+		return -EINVAL;
+
+	mutex_lock(&drvdata->mutex);
+	writel(value, drvdata->base + SROM_VROMOFF);
+	mutex_unlock(&drvdata->mutex);
+
+	return count;
+}
+static DEVICE_ATTR_RW(vromoff);
+
+static struct attribute *srom_attrs[] = {
+	&dev_attr_vromoff.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(srom);
+
+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),
+				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);
+	if (IS_ERR(drvdata->base)) {
+		dev_err(&pdev->dev, "ioremap_resource error\n");
+		ret = PTR_ERR(drvdata->base);
+		goto ioremap_err;
+	}
+
+	mutex_init(&drvdata->mutex);
+
+	ret = sysfs_register(&pdev->dev, drvdata);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "sysfs error\n");
+		goto sysfs_err;
+	}
+
+	dev_info(&pdev->dev, "initialized\n");
+	return 0;
+
+sysfs_err:
+ioremap_err:
+	platform_set_drvdata(pdev, NULL);
+out:
+	if (drvdata)
+		devm_kfree(&pdev->dev, (void *)drvdata);
+	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),
+	},
+};
+module_platform_driver(gxp_srom_driver);
+
+MODULE_AUTHOR("Clay Chang <clayc@hpe.com>");
+MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver

Posted by Krzysztof Kozlowski 3 years ago

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.

> +
> +
> +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.

> 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(*)

> +				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.

> +	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.

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

return dev_err_probe

> +		goto sysfs_err;
> +	}
> +
> +	dev_info(&pdev->dev, "initialized\n");

Drop silly probe successes.

> +	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?

> +	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.

> +	},
> +};
> +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

Re: [PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver

Posted by Clay Chang 3 years ago

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

[PATCH 1/5] soc: hpe: Add GXP SROM Control Register Driver
[PATCH 2/5] dt-bindings: soc: hpe: hpe,gxp-srom.yaml
[PATCH 3/5] ARM: dts: hpe: Add SROM Driver
[PATCH 4/5] ARM: multi_v7_defconfig: Add GXP SROM Driver
[PATCH 5/5] MAINTAINERS: Add maintainer of GXP SROM support