On 10/01/2023 05:25, clayc@xxxxxxx wrote: > From: Clay Chang <clayc@xxxxxxx> > > 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@xxxxxxx> > --- > 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@xxxxxxx>"); > +MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver"); > +MODULE_LICENSE("GPL"); Best regards, Krzysztof