linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support
@ 2022-10-11 18:55 nick.hawkins
  2022-10-11 18:55 ` [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver nick.hawkins
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP SoC is interfaced with a programmable logic device that takes
inputs/outputs from the server board. All these inputs/outputs are
presented in register form to the SoC. The Programmable Logic
Register driver enables access to these registers and provides a
standard way to provide access across the HPE portfolio. Additionally
this patchset also enables the SPI driver that already exists in linux
in the spi driver as spi-gxp file

Nick Hawkins (5):
  soc: hpe: add support for HPE GXP Programmable Register Driver
  dt-bindings: soc: hpe: Add hpe,gxp-plreg
  ARM: dts: hpe: Add PLREG/SPI Support
  ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers
  MAINTAINERS: Add HPE SOC Drivers

 .../bindings/soc/hpe/hpe,gxp-plreg.yaml       |   43 +
 MAINTAINERS                                   |    3 +
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts      |  275 ++++
 arch/arm/boot/dts/hpe-gxp.dtsi                |   28 +-
 arch/arm/configs/multi_v7_defconfig           |    2 +
 drivers/soc/Kconfig                           |    1 +
 drivers/soc/Makefile                          |    1 +
 drivers/soc/hpe/Kconfig                       |   19 +
 drivers/soc/hpe/Makefile                      |    7 +
 drivers/soc/hpe/gxp-plreg.c                   | 1207 +++++++++++++++++
 drivers/soc/hpe/gxp-soclib.c                  |   19 +
 drivers/soc/hpe/gxp-soclib.h                  |   15 +
 include/linux/soc/hpe/gxp.h                   |   15 +
 13 files changed, 1634 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
 create mode 100644 drivers/soc/hpe/Kconfig
 create mode 100644 drivers/soc/hpe/Makefile
 create mode 100644 drivers/soc/hpe/gxp-plreg.c
 create mode 100644 drivers/soc/hpe/gxp-soclib.c
 create mode 100644 drivers/soc/hpe/gxp-soclib.h
 create mode 100644 include/linux/soc/hpe/gxp.h

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver
  2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
@ 2022-10-11 18:55 ` nick.hawkins
  2022-10-11 20:05   ` Krzysztof Kozlowski
  2022-10-11 18:55 ` [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg nick.hawkins
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP SoC is interfaced with a programmable logic device that takes
inputs/outputs from the server board. All these inputs/outputs are
presented in register form to the SoC. The Programmable Register Driver
enables access to these registers and provides a standard way to
provide access across the HPE portfolio.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/soc/Kconfig          |    1 +
 drivers/soc/Makefile         |    1 +
 drivers/soc/hpe/Kconfig      |   19 +
 drivers/soc/hpe/Makefile     |    7 +
 drivers/soc/hpe/gxp-plreg.c  | 1207 ++++++++++++++++++++++++++++++++++
 drivers/soc/hpe/gxp-soclib.c |   19 +
 drivers/soc/hpe/gxp-soclib.h |   15 +
 include/linux/soc/hpe/gxp.h  |   15 +
 8 files changed, 1284 insertions(+)
 create mode 100644 drivers/soc/hpe/Kconfig
 create mode 100644 drivers/soc/hpe/Makefile
 create mode 100644 drivers/soc/hpe/gxp-plreg.c
 create mode 100644 drivers/soc/hpe/gxp-soclib.c
 create mode 100644 drivers/soc/hpe/gxp-soclib.h
 create mode 100644 include/linux/soc/hpe/gxp.h

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index e461c071189b..e4fed449d619 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 69ba6508cf2c..ebdab9bcbe79 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..2bdc9785fe83
--- /dev/null
+++ b/drivers/soc/hpe/Kconfig
@@ -0,0 +1,19 @@
+config SOC_VENDOR_HPE
+	bool "HPE SoC drivers"
+	default y
+	depends on ARCH_HPE
+
+if SOC_VENDOR_HPE
+
+config HPE_GXP_SOCLIB
+	bool
+	default n
+
+config HPE_GXP_PLREG
+	bool "GXP Programmable logic register support"
+	depends on ARCH_HPE_GXP
+	select HPE_GXP_SOCLIB
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to add support for the PLREG.
+endif
diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
new file mode 100644
index 000000000000..f304fe0192c9
--- /dev/null
+++ b/drivers/soc/hpe/Makefile
@@ -0,0 +1,7 @@
+obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
+obj-$(CONFIG_HPE_GXP_PLREG) += gxp-plreg.o
+obj-$(CONFIG_HPE_GXP_FN2) += gxp-fn2.o
+obj-$(CONFIG_HPE_GXP_CSM) += gxp-csm.o
+obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
+obj-$(CONFIG_HPE_GXP_CHIF) += gxp-chif.o
+obj-$(CONFIG_HPE_GXP_DBG) += gxp-dbg.o
diff --git a/drivers/soc/hpe/gxp-plreg.c b/drivers/soc/hpe/gxp-plreg.c
new file mode 100644
index 000000000000..c2b038a3f4ba
--- /dev/null
+++ b/drivers/soc/hpe/gxp-plreg.c
@@ -0,0 +1,1207 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#include <linux/soc/hpe/gxp.h>
+#include "gxp-soclib.h"
+
+#define IOP_LED1 0
+#define IOP_LED2 1
+#define IOP_LED3 2
+#define IOP_LED4 3
+#define IOP_LED5 4
+#define IOP_LED6 5
+#define IOP_LED7 6
+#define IOP_LED8 7
+#define FAN1_INST 8
+#define FAN2_INST 9
+#define FAN3_INST 10
+#define FAN4_INST 11
+#define FAN5_INST 12
+#define FAN6_INST 13
+#define FAN7_INST 14
+#define FAN8_INST 15
+#define FAN9_INST 16
+#define FAN10_INST 17
+#define FAN11_INST 18
+#define FAN12_INST 19
+#define FAN13_INST 20
+#define FAN14_INST 21
+#define FAN15_INST 22
+#define FAN16_INST 23
+#define FAN1_FAIL 24
+#define FAN2_FAIL 25
+#define FAN3_FAIL 26
+#define FAN4_FAIL 27
+#define FAN5_FAIL 28
+#define FAN6_FAIL 29
+#define FAN7_FAIL 30
+#define FAN8_FAIL 31
+#define FAN9_FAIL 32
+#define FAN10_FAIL 33
+#define FAN11_FAIL 34
+#define FAN12_FAIL 35
+#define FAN13_FAIL 36
+#define FAN14_FAIL 37
+#define FAN15_FAIL 38
+#define FAN16_FAIL 39
+#define FAN1_ID 40
+#define FAN2_ID 41
+#define FAN3_ID 42
+#define FAN4_ID 43
+#define FAN5_ID 44
+#define FAN6_ID 45
+#define FAN7_ID 46
+#define FAN8_ID 47
+#define FAN9_ID 48
+#define FAN10_ID 49
+#define FAN11_ID 50
+#define FAN12_ID 51
+#define FAN13_ID 52
+#define FAN14_ID 53
+#define FAN15_ID 54
+#define FAN16_ID 55
+#define LED_IDENTIFY 56
+#define LED_HEALTH_RED 57
+#define LED_HEALTH_AMBER 58
+#define PWR_BTN_INT 59
+#define UID_PRESS_INT 60
+#define SLP_INT 61
+#define PME_INT 62
+#define RESV0 63
+#define RESV1 64
+#define RESV2 65
+#define ACM_FORCE_OFF 70
+#define ACM_REMOVED 71
+#define ACM_REQ_N 72
+
+#define PLREG_INT_GRP5_PIN_BASE	59
+
+#define MAX_FAN 16
+#define IOP_LED_QUANTITY 8
+#define BYTE 0
+#define MASK 1
+#define VALUE 2
+
+struct fan_access {
+	u32 inst;
+	u32 fail;
+	u32 id;
+	u32 bit;
+};
+
+struct iop_led_access {
+	u32 iop_led[2];
+};
+
+struct health_led_access {
+	u32 red[2];
+	u32 amber[2];
+	u32 green[2];
+};
+
+struct identify_led_access {
+	u32 off[2];
+	u32 on[2];
+};
+
+struct acm_access {
+	int exists;
+	u32 force_off[2];
+	u32 removed[2];
+	u32 unlatch_req[2];
+};
+
+struct server_id_access {
+	u32 upper[2];
+	u32 lower[2];
+};
+
+struct sideband_access {
+	u32 disabled[2];
+	u32 embedded[2];
+	u32 adaptive[2];
+};
+
+struct grp5_intr_flag_access {
+	u32 ack[2];
+	u32 pwrbtn[2];
+	u32 uidpress[2];
+	u32 slpintr[2];
+};
+
+struct grp5_intr_mask_access {
+	u32 pwrbtn[2];
+	u32 slpintr[2];
+};
+
+struct grp_intr_masks_access {
+	u32 grp5[2];
+};
+
+struct grp_intr_flags_access {
+	u32 grp5[2];
+};
+
+struct pwrbtn_access {
+	u32 latch[3];
+};
+
+struct gxp_plreg_drvdata {
+	void __iomem *base;
+	struct regmap *plreg_map;
+	struct gpio_chip gpio_chip;
+	int irq;
+	struct fan_access fan[16];
+	struct health_led_access health_led;
+	struct iop_led_access iop_led[8];
+	struct identify_led_access identify_led;
+	struct acm_access acm;
+	struct server_id_access server_id;
+	struct sideband_access sideband;
+	struct grp5_intr_flag_access grp5_intr_flag;
+	struct grp5_intr_mask_access grp5_intr_mask;
+	struct grp_intr_masks_access grp_intr_masks;
+	struct grp_intr_flags_access grp_intr_flags;
+	struct pwrbtn_access pwrbtn;
+};
+
+struct gxp_plreg_drvdata client_data;
+
+static void address_translation(u32 desired_offset, u32 *offset, u32 *bit_shift)
+{
+	*offset = (desired_offset & 0xffc);
+	*bit_shift = (desired_offset - *offset) * 8;
+}
+
+int gxp_plreg_get_fan_inst(int fan)
+{
+	struct gxp_plreg_drvdata *drvdata = &client_data;
+	u32 trans_offset;
+	u32 trans_shift;
+	u32 val;
+
+	address_translation(drvdata->fan[fan].inst,
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &val);
+	val = (val >> trans_shift) & drvdata->fan[fan].bit;
+	if (val == drvdata->fan[fan].bit)
+		return 1;
+	else
+		return 0;
+}
+EXPORT_SYMBOL(gxp_plreg_get_fan_inst);
+
+int gxp_plreg_get_fan_fail(int fan)
+{
+	struct gxp_plreg_drvdata *drvdata = &client_data;
+	u32 trans_offset;
+	u32 trans_shift;
+	u32 val;
+
+	address_translation(drvdata->fan[fan].fail,
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &val);
+	val = (val >> trans_shift) & drvdata->fan[fan].bit;
+	if (val == drvdata->fan[fan].bit)
+		return 1;
+	else
+		return 0;
+}
+EXPORT_SYMBOL(gxp_plreg_get_fan_fail);
+
+int gxp_plreg_get_fan_id(int fan)
+{
+	struct gxp_plreg_drvdata *drvdata = &client_data;
+	u32 trans_offset;
+	u32 trans_shift;
+	u32 val;
+
+	address_translation(drvdata->fan[fan].id,
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &val);
+	val = (val >> trans_shift) & drvdata->fan[fan].bit;
+	if (val == drvdata->fan[fan].bit)
+		return 1;
+	else
+		return 0;
+}
+EXPORT_SYMBOL(gxp_plreg_get_fan_id);
+
+void gxp_plreg_do_virt_pwr_btn_latch(void)
+{
+	struct gxp_plreg_drvdata *drvdata = &client_data;
+	u32 trans_offset;
+	u32 trans_shift;
+
+	address_translation(drvdata->pwrbtn.latch[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   drvdata->pwrbtn.latch[MASK] << trans_shift,
+			   drvdata->pwrbtn.latch[VALUE] << trans_shift);
+}
+EXPORT_SYMBOL(gxp_plreg_do_virt_pwr_btn_latch);
+
+static ssize_t server_id_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
+	int value_upper;
+	int value_lower;
+	ssize_t ret;
+	u32 trans_offset;
+	u32 trans_shift;
+
+	/* read upper first */
+	address_translation(drvdata->server_id.upper[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
+	value_upper = value_upper >> trans_shift;
+	value_upper = value_upper & drvdata->server_id.upper[MASK];
+
+	/* read lower last */
+	address_translation(drvdata->server_id.lower[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
+	value_lower = value_lower >> trans_shift;
+	value_lower = value_lower & drvdata->server_id.lower[MASK];
+
+	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
+
+	return ret;
+}
+
+static DEVICE_ATTR_RO(server_id);
+
+static ssize_t sideband_sel_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
+	int value;
+	ssize_t ret;
+	u32 trans_offset;
+	u32 trans_shift;
+	u32 status = PLREG_SIDEBAND_UNKNOWN;
+
+	/* check first to see if its disabled */
+	address_translation(drvdata->sideband.disabled[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &value);
+	value = value >> trans_shift;
+
+	if ((value & drvdata->sideband.disabled[MASK]) ==
+	    drvdata->sideband.disabled[MASK]) {
+		status = PLREG_SIDEBAND_DISABLED;
+		goto EXIT;
+	}
+
+	/* check next to see if its embedded */
+	address_translation(drvdata->sideband.embedded[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &value);
+	value = value >> trans_shift;
+
+	if ((value & drvdata->sideband.embedded[MASK]) ==
+	    drvdata->sideband.embedded[MASK]) {
+		status = PLREG_SIDEBAND_EMBEDDED;
+		goto EXIT;
+	}
+
+	/* check finally to see if its adaptive */
+	address_translation(drvdata->sideband.adaptive[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_read(drvdata->plreg_map, trans_offset, &value);
+	value = value >> trans_shift;
+
+	if ((value & drvdata->sideband.adaptive[MASK]) ==
+	    drvdata->sideband.adaptive[MASK])
+		status = PLREG_SIDEBAND_ADAPTIVE;
+EXIT:
+
+	ret = sprintf(buf, "0x%02x", status);
+
+	return ret;
+}
+
+static ssize_t sideband_sel_store(struct device *dev, struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
+	int input;
+	int rc;
+	u32 trans_offset;
+	u32 trans_shift;
+
+	rc = kstrtoint(buf, 0, &input);
+	if (rc < 0)
+		return -EINVAL;
+
+	if (input > PLREG_SIDEBAND_ADAPTIVE ||  input <  PLREG_SIDEBAND_DISABLED)
+		return -EINVAL;
+
+	if (input == PLREG_SIDEBAND_DISABLED) {
+		address_translation(drvdata->sideband.disabled[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->sideband.disabled[MASK] << trans_shift,
+				   drvdata->sideband.disabled[MASK] << trans_shift);
+	} else if (input == PLREG_SIDEBAND_EMBEDDED) {
+		address_translation(drvdata->sideband.embedded[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->sideband.embedded[MASK] << trans_shift,
+				   drvdata->sideband.embedded[MASK] << trans_shift);
+	} else {
+		address_translation(drvdata->sideband.adaptive[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->sideband.adaptive[MASK] << trans_shift,
+				   drvdata->sideband.adaptive[MASK] << trans_shift);
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(sideband_sel);
+
+static struct attribute *plreg_attrs[] = {
+	&dev_attr_server_id.attr,
+	&dev_attr_sideband_sel.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(plreg);
+
+static int sysfs_register(struct device *parent, struct gxp_plreg_drvdata *plreg)
+{
+	struct device *dev;
+
+	dev = device_create_with_groups(soc_class, parent, 0,
+					plreg, plreg_groups, "plreg");
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	return 0;
+}
+
+static int gxp_gpio_plreg_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+	int ret = 0;
+	u32 trans_offset;
+	u32 trans_shift;
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+		address_translation(drvdata->iop_led[offset].iop_led[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->iop_led[offset].iop_led[MASK];
+		if (val == drvdata->iop_led[offset].iop_led[MASK])
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case FAN1_INST ...FAN16_INST:
+		address_translation(drvdata->fan[offset - FAN1_INST].inst,
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->fan[offset - FAN1_INST].bit;
+		if (val == drvdata->fan[offset - FAN1_INST].bit)
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case FAN1_FAIL ... FAN16_FAIL:
+		address_translation(drvdata->fan[offset - FAN1_FAIL].fail,
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->fan[offset - FAN1_FAIL].bit;
+		if (val == drvdata->fan[offset - FAN1_FAIL].bit)
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case FAN1_ID ... FAN16_ID:
+		address_translation(drvdata->fan[offset - FAN1_ID].id,
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->fan[offset - FAN1_ID].bit;
+		if (val == drvdata->fan[offset - FAN1_ID].bit)
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case PWR_BTN_INT:
+		address_translation(drvdata->grp5_intr_flag.pwrbtn[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->grp5_intr_flag.pwrbtn[MASK];
+		if (val == drvdata->grp5_intr_flag.pwrbtn[MASK])
+			ret = 0;
+		else
+			ret = 1;
+		break;
+	case UID_PRESS_INT:
+		address_translation(drvdata->grp5_intr_flag.uidpress[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->grp5_intr_flag.uidpress[MASK];
+		if (val == drvdata->grp5_intr_flag.uidpress[MASK])
+			ret = 0;
+		else
+			ret = 1;
+		break;
+	case SLP_INT:
+		address_translation(drvdata->grp5_intr_flag.slpintr[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->grp5_intr_flag.slpintr[MASK];
+		if (val == drvdata->grp5_intr_flag.slpintr[MASK])
+			ret = 0;
+		else
+			ret = 1;
+		break;
+	case ACM_FORCE_OFF:
+		if (!drvdata->acm.exists)
+			return -1;
+
+		address_translation(drvdata->acm.force_off[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->acm.force_off[MASK];
+		if (val == drvdata->acm.force_off[MASK])
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case ACM_REMOVED:
+		if (!drvdata->acm.exists)
+			return -1;
+		address_translation(drvdata->acm.removed[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->acm.removed[MASK];
+		if (val == drvdata->acm.removed[MASK])
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	case ACM_REQ_N:
+		if (!drvdata->acm.exists)
+			return -1;
+		address_translation(drvdata->acm.unlatch_req[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_read(drvdata->plreg_map, trans_offset, &val);
+		val = (val >> trans_shift) & drvdata->acm.unlatch_req[MASK];
+		if (val == drvdata->acm.unlatch_req[MASK])
+			ret = 1;
+		else
+			ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_plreg_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	u32 trans_offset;
+	u32 trans_shift;
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+		if (value > 1 || 0 < value)
+			return;
+
+		address_translation(drvdata->iop_led[offset].iop_led[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->iop_led[offset].iop_led[MASK] << trans_shift,
+				   value << trans_shift);
+		break;
+	case LED_IDENTIFY:
+		//offset 0x05 bit 7:6
+		if (value == 1) {
+			address_translation(drvdata->identify_led.on[BYTE],
+					    &trans_offset,
+					    &trans_shift);
+			regmap_update_bits(drvdata->plreg_map, trans_offset,
+					   drvdata->identify_led.on[MASK] << trans_shift,
+					   drvdata->identify_led.on[MASK] << trans_shift);
+		} else if (value == 0) {
+			address_translation(drvdata->identify_led.off[BYTE],
+					    &trans_offset,
+					    &trans_shift);
+			regmap_update_bits(drvdata->plreg_map, trans_offset,
+					   drvdata->identify_led.off[MASK] << trans_shift,
+					   drvdata->identify_led.off[MASK] << trans_shift);
+		}
+		break;
+	case LED_HEALTH_RED:
+		if (value > 1 || 0 < value)
+			return;
+
+		address_translation(drvdata->health_led.red[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->health_led.red[MASK] << trans_shift,
+				   value << trans_shift);
+		break;
+	case LED_HEALTH_AMBER:
+		if (value > 1 || 0 < value)
+			return;
+
+		address_translation(drvdata->health_led.amber[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->health_led.amber[MASK] << trans_shift,
+				   value << trans_shift);
+		break;
+	case ACM_FORCE_OFF:
+		if (value > 1 || 0 < value || !drvdata->acm.exists)
+			return;
+
+		address_translation(drvdata->acm.force_off[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->acm.force_off[MASK] << trans_shift,
+				   value << trans_shift);
+		break;
+	case ACM_REQ_N:
+		if (value > 1 || 0 < value || !drvdata->acm.exists)
+			return;
+
+		address_translation(drvdata->acm.force_off[BYTE],
+				    &trans_offset,
+				    &trans_shift);
+
+		regmap_update_bits(drvdata->plreg_map, trans_offset,
+				   drvdata->acm.force_off[MASK] << trans_shift,
+				   value << trans_shift);
+		break;
+	default:
+		break;
+	}
+}
+
+static int gxp_gpio_plreg_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret = GPIO_DIR_IN;
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+	case LED_IDENTIFY ... LED_HEALTH_AMBER:
+	case ACM_FORCE_OFF:
+	case ACM_REQ_N:
+		ret = GPIO_DIR_OUT;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_plreg_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case FAN1_INST ... FAN16_ID:
+		ret = 0;
+		break;
+	case PWR_BTN_INT ... RESV2:
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_plreg_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+	case LED_IDENTIFY ... LED_HEALTH_AMBER:
+	case ACM_FORCE_OFF:
+	case ACM_REQ_N:
+		gxp_gpio_plreg_set(chip, offset, value);
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+	u32 trans_offset;
+	u32 trans_shift;
+
+	// Read latched interrupt
+	address_translation(drvdata->grp5_intr_flag.ack[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+	regmap_read(drvdata->plreg_map, trans_offset, &val);
+
+	//Clear latched interrupt
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift,
+			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift);
+}
+
+static void gxp_gpio_irq_set_mask(struct irq_data *d, bool set)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	u32 trans_offset;
+	u32 trans_shift;
+
+	address_translation(drvdata->grp5_intr_flag.ack[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   (drvdata->grp5_intr_flag.pwrbtn[MASK] |
+			   drvdata->grp5_intr_flag.slpintr[MASK]) << trans_shift,
+			   set == 1 ? 0 : (drvdata->grp5_intr_flag.pwrbtn[MASK] |
+			   drvdata->grp5_intr_flag.slpintr[MASK]) << trans_shift);
+
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift,
+			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift);
+}
+
+static void gxp_gpio_irq_mask(struct irq_data *d)
+{
+	gxp_gpio_irq_set_mask(d, false);
+}
+
+static void gxp_gpio_irq_unmask(struct irq_data *d)
+{
+	gxp_gpio_irq_set_mask(d, true);
+}
+
+static int gxp_gpio_set_type(struct irq_data *d, unsigned int type)
+{
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(d, handle_level_irq);
+	else
+		irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static irqreturn_t gxp_plreg_irq_handle(int irq, void *_drvdata)
+{
+	struct gxp_plreg_drvdata *drvdata = (struct gxp_plreg_drvdata *)_drvdata;
+	unsigned int val, girq, i;
+
+	/* handle plreg interrupt group5 */
+	val = readb(drvdata->base + drvdata->grp5_intr_flag.ack[BYTE]);
+
+	for_each_set_bit(i, (unsigned long *)&val, 3) {
+		girq = irq_find_mapping(drvdata->gpio_chip.irq.domain,
+					i + PLREG_INT_GRP5_PIN_BASE);
+		generic_handle_irq(girq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct gpio_chip plreg_chip = {
+	.label			= "gxp-plreg",
+	.owner			= THIS_MODULE,
+	.get			= gxp_gpio_plreg_get,
+	.set			= gxp_gpio_plreg_set,
+	.get_direction = gxp_gpio_plreg_get_direction,
+	.direction_input = gxp_gpio_plreg_direction_input,
+	.direction_output = gxp_gpio_plreg_direction_output,
+	.base = -1,
+	//.can_sleep		= true,
+};
+
+static struct irq_chip gxp_gpio_irqchip = {
+	.name		= "gxp-plreg",
+	.irq_ack	= gxp_gpio_irq_ack,
+	.irq_mask	= gxp_gpio_irq_mask,
+	.irq_unmask	= gxp_gpio_irq_unmask,
+	.irq_set_type	= gxp_gpio_set_type,
+};
+
+static const struct of_device_id gxp_plreg_of_match[] = {
+	{ .compatible = "hpe,gxp-plreg" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gxp_plreg_of_match);
+
+static int gxp_plreg_probe(struct platform_device *pdev)
+{
+	int i;
+	int j;
+	int ret;
+	struct gxp_plreg_drvdata *drvdata;
+	struct resource *res;
+	struct gpio_irq_chip *girq;
+	struct device_node *np;
+	char name_buf[10];
+	u32 trans_offset;
+	u32 trans_shift;
+
+	drvdata = devm_kzalloc(&pdev->dev,
+			       sizeof(struct gxp_plreg_drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, drvdata);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
+
+	drvdata->plreg_map = syscon_regmap_lookup_by_compatible("hpe,gxp-plreg");
+	if (IS_ERR(drvdata->plreg_map)) {
+		dev_err(&pdev->dev, "Unable to find plreg regmap\n");
+		return PTR_ERR(drvdata->plreg_map);
+	}
+
+	/* Supply driver with information to access specific offsets in plreg */
+	for (i = 0; i < MAX_FAN; i++) {
+		/* Find Fan Children */
+		snprintf(name_buf, sizeof(name_buf), "fan%d", i + 1);
+		np = of_get_child_by_name(pdev->dev.of_node, name_buf);
+
+		if (np) {
+		/* For each child there should be 3 fan properties */
+			if (of_property_read_u32(np, "inst",
+						 &drvdata->fan[i].inst)) {
+				dev_err(&pdev->dev, "%s is missing its 'inst' property\n",
+					name_buf);
+				return -ENODEV;
+			}
+
+			if (of_property_read_u32(np, "fail",
+						 &drvdata->fan[i].fail)) {
+				dev_err(&pdev->dev, "%s is missing its 'fail' property\n",
+					name_buf);
+				return -ENODEV;
+			}
+			if (of_property_read_u32(np, "id",
+						 &drvdata->fan[i].id)) {
+				dev_err(&pdev->dev, "%s is missing its 'id' property\n",
+					name_buf);
+				return -ENODEV;
+			}
+			if (of_property_read_u32(np, "bit",
+						 &drvdata->fan[i].bit)) {
+				dev_err(&pdev->dev, "%s is missing its 'bit' property\n",
+					name_buf);
+				return -ENODEV;
+			}
+		} else {
+			dev_warn(&pdev->dev, "%pOF is missing its '%s' node\n", np, name_buf);
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "healthled");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'healthled' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "red", i,
+					       &drvdata->health_led.red[i])) {
+			dev_err(&pdev->dev, "healthled is missing its 'red' property index %d\n",
+				i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "amber", i,
+					       &drvdata->health_led.amber[i])) {
+			dev_err(&pdev->dev, "healthled is missing its 'amber' property index %d\n",
+				i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "green", i,
+					       &drvdata->health_led.green[i])) {
+			dev_err(&pdev->dev, "healthled is missing its 'green' property index %d\n",
+				i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i < IOP_LED_QUANTITY; i++) {
+		/* Find iopLed Children */
+		snprintf(name_buf, sizeof(name_buf), "iopled%d", i + 1);
+		np = of_get_child_by_name(pdev->dev.of_node, name_buf);
+
+		if (np) {
+		/* For each child there should be 1 property */
+			for (j = 0; j <= MASK; j++) {
+				if (of_property_read_u32_index(np, "on", j,
+							       &drvdata->iop_led[i].iop_led[j]
+							       )) {
+					dev_err(&pdev->dev, "%s is missing its 'on' property index %d\n",
+						name_buf, j);
+					return -ENODEV;
+				}
+			}
+		} else {
+			dev_err(&pdev->dev, "%pOF is missing its '%s' node\n", np, name_buf);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "identifyled");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'identifyled' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "on", i, &drvdata->identify_led.on[i])) {
+			dev_err(&pdev->dev, "identifyled is missing its 'on' property index %d\n",
+				i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "off", i, &drvdata->identify_led.off[i])) {
+			dev_err(&pdev->dev, "identifyled is missing its 'off' property index %d\n",
+				i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "acm");
+	if (!np) {
+		dev_warn(&pdev->dev, "%pOF is missing its 'acm' node\n", np);
+	} else {
+		for (i = 0; i <= MASK; i++) {
+			if (of_property_read_u32_index(np, "forceoff", i,
+						       &drvdata->acm.force_off[i])) {
+				dev_err(&pdev->dev,
+					"acm is missing its 'forceoff' property index %d\n", i);
+				return -ENODEV;
+			}
+		}
+
+		for (i = 0; i <= MASK; i++) {
+			if (of_property_read_u32_index(np, "removed", i,
+						       &drvdata->acm.removed[i])) {
+				dev_err(&pdev->dev,
+					"acm is missing its 'removed' property index %d\n", i);
+				return -ENODEV;
+			}
+		}
+
+		for (i = 0; i <= MASK; i++) {
+			if (of_property_read_u32_index(np, "unlatchreq", i,
+						       &drvdata->acm.unlatch_req[i])) {
+				dev_err(&pdev->dev,
+					"acm is missing its 'unlatchreq' property index %d\n", i);
+				return -ENODEV;
+			}
+		}
+		drvdata->acm.exists = 1;
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "serverid");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'serverid' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "upper", i,
+					       &drvdata->server_id.upper[i])) {
+			dev_err(&pdev->dev,
+				"serverid is missing its 'upper' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "lower", i, &drvdata->server_id.lower[i])) {
+			dev_err(&pdev->dev,
+				"serverid is missing its 'lower' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "sideband");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'sideband' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "disabled", i,
+					       &drvdata->sideband.disabled[i])) {
+			dev_err(&pdev->dev,
+				"sideband is missing its 'disabled' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "embedded", i,
+					       &drvdata->sideband.embedded[i])) {
+			dev_err(&pdev->dev,
+				"sideband is missing its 'embedded' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "adaptive", i,
+					       &drvdata->sideband.adaptive[i])) {
+			dev_err(&pdev->dev,
+				"sideband is missing its 'adaptive' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "grp5intflag");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'grp5intflag' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "ack", i, &drvdata->grp5_intr_flag.ack[i])) {
+			dev_err(&pdev->dev,
+				"grp5intflag is missing its 'ack' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "pwrbtn", i,
+					       &drvdata->grp5_intr_flag.pwrbtn[i])) {
+			dev_err(&pdev->dev,
+				"grp5intflag is missing its 'pwrbtn' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "uidpress", i,
+					       &drvdata->grp5_intr_flag.uidpress[i])) {
+			dev_err(&pdev->dev,
+				"grp5intflag is missing its 'uid' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "slpint", i,
+					       &drvdata->grp5_intr_flag.slpintr[i])) {
+			dev_err(&pdev->dev,
+				"grp5intflag is missing its 'slpint' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "grp5intmask");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'grp5intmask' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "pwrbtn", i,
+					       &drvdata->grp5_intr_mask.pwrbtn[i])) {
+			dev_err(&pdev->dev,
+				"grp5intmask is missing its 'pwrbtn' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "slpint", i,
+					       &drvdata->grp5_intr_mask.slpintr[i])) {
+			dev_err(&pdev->dev,
+				"grp5intmask is missing its 'slpint' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "grpintsmasks");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'grpintsmasks' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "grp5", i,
+					       &drvdata->grp_intr_masks.grp5[i])) {
+			dev_err(&pdev->dev,
+				"grpintsmasks is missing its 'grp5' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "grpintsflags");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'grpintsflags' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= MASK; i++) {
+		if (of_property_read_u32_index(np, "grp5", i,
+					       &drvdata->grp_intr_flags.grp5[i])) {
+			dev_err(&pdev->dev,
+				"grp5intsflags is missing its 'grp5' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
+	if (!np) {
+		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
+		return -ENODEV;
+	}
+
+	for (i = 0; i <= VALUE; i++) {
+		if (of_property_read_u32_index(np, "latch", i, &drvdata->pwrbtn.latch[i])) {
+			dev_err(&pdev->dev, "pwrbtn is missing its 'latch' property index %d\n", i);
+			return -ENODEV;
+		}
+	}
+
+	drvdata->gpio_chip = plreg_chip;
+	drvdata->gpio_chip.ngpio = 100;
+	drvdata->gpio_chip.parent = &pdev->dev;
+
+	girq = &drvdata->gpio_chip.irq;
+	girq->chip = &gxp_gpio_irqchip;
+	/* This will let us handle the parent IRQ in the driver */
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_edge_irq;
+	/* Set up interrupt from PLREG Group 5 Mask */
+	address_translation(drvdata->grp_intr_flags.grp5[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   drvdata->grp_intr_flags.grp5[MASK] << trans_shift,
+			   drvdata->grp_intr_flags.grp5[MASK] << trans_shift);
+	address_translation(drvdata->grp_intr_masks.grp5[BYTE],
+			    &trans_offset,
+			    &trans_shift);
+	regmap_update_bits(drvdata->plreg_map, trans_offset,
+			   drvdata->grp_intr_masks.grp5[MASK] << trans_shift,
+			   0x00);
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);
+		return ret;
+	}
+	drvdata->irq = ret;
+
+	ret = devm_request_irq(&pdev->dev, drvdata->irq, gxp_plreg_irq_handle, IRQF_SHARED,
+			       "gxp-plreg", drvdata);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "IRQ handler failed - %d\n", ret);
+		return ret;
+	}
+
+	ret = sysfs_register(&pdev->dev, drvdata);
+	if (ret < 0) {
+		dev_warn(&pdev->dev, "Unable to register sysfs\n");
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->gpio_chip, NULL);
+	if (ret < 0)
+		dev_err(&pdev->dev, "Could not register gpiochip for plreg, %d\n", ret);
+
+	client_data = *drvdata;
+
+	dev_info(&pdev->dev, "HPE GXP PLREG driver loaded.\n");
+
+	return 0;
+}
+
+static struct platform_driver gxp_plreg_driver = {
+	.probe = gxp_plreg_probe,
+	.driver = {
+		.name = "gxp-plreg",
+		.of_match_table = of_match_ptr(gxp_plreg_of_match),
+	},
+};
+module_platform_driver(gxp_plreg_driver);
+
+MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
+MODULE_DESCRIPTION("HPE GXP Programmable Logic Registers Driver");
diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
new file mode 100644
index 000000000000..e32aa346e770
--- /dev/null
+++ b/drivers/soc/hpe/gxp-soclib.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/io.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..583902969ad5
--- /dev/null
+++ b/drivers/soc/hpe/gxp-soclib.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Copyright (C) 2019 Hewlett-Packard Development Company, L.P.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __GXP_SOCLIB_H__
+#define __GXP_SOCLIB_H__
+
+extern struct class *soc_class;
+
+#endif
diff --git a/include/linux/soc/hpe/gxp.h b/include/linux/soc/hpe/gxp.h
new file mode 100644
index 000000000000..3573c391ec5a
--- /dev/null
+++ b/include/linux/soc/hpe/gxp.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0=or-later */
+/* Copyright (C) 2022 Hewlett-Packard Development Company, L.P. */
+
+#ifndef _LINUX_SOC_HPE_GXP_
+#define _LINUX_SOC_HPE_GXP_H_
+
+#define PLREG_SIDEBAND_UNKNOWN 0x00
+#define PLREG_SIDEBAND_DISABLED 0x01
+#define PLREG_SIDEBAND_EMBEDDED 0x02
+#define PLREG_SIDEBAND_ADAPTIVE 0x03
+
+#define GPIO_DIR_OUT 0
+#define GPIO_DIR_IN  1
+
+#endif /* _LINUX_SOC_HPE_GXP_H_ */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
  2022-10-11 18:55 ` [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver nick.hawkins
@ 2022-10-11 18:55 ` nick.hawkins
  2022-10-11 19:51   ` Krzysztof Kozlowski
  2022-10-11 20:27   ` Rob Herring
  2022-10-11 18:55 ` [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support nick.hawkins
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

The hpe,gxp-plreg binding provides access to the board i/o through the
Programmable logic interface. The binding provides information to enable
use of the same driver across the HPE portfolio.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../bindings/soc/hpe/hpe,gxp-plreg.yaml       | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml

diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
new file mode 100644
index 000000000000..cdc54e66d9a9
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: HPE GXP Programmable Logic Registers Controller
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+
+properties:
+  compatible:
+    items:
+      - const: hpe,gxp-plreg
+      - const: simple-mfd
+      - const: syscon
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+
+additionalProperties: true
+
+examples:
+  - |
+    cpld@1e789000 {
+      compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
+      reg = <0x1e789000 0x1000>;
+      fan1 {
+        bit = <0x01>;
+        inst = <0x27>;
+        id = <0x2B>;
+      };
+      iopled1 {
+        on = <0x04 0x01>;
+      };
+      pwrbtn {
+        latch = <0xFF 0xFF 0x04>;
+      };
+    };
+
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support
  2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
  2022-10-11 18:55 ` [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver nick.hawkins
  2022-10-11 18:55 ` [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg nick.hawkins
@ 2022-10-11 18:55 ` nick.hawkins
  2022-10-11 19:53   ` Krzysztof Kozlowski
  2022-10-11 18:55 ` [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers nick.hawkins
  2022-10-11 18:55 ` [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers nick.hawkins
  4 siblings, 1 reply; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

Adding support for the Programmable Logic Register driver in the HPE GXP
SoC. Additionally adding support for the SPI driver that has already
been committed to linux (See: drivers/spi/spi-gxp.c).

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 275 +++++++++++++++++++++++
 arch/arm/boot/dts/hpe-gxp.dtsi           |  28 ++-
 2 files changed, 302 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
index 3a7382ce40ef..c97b052c4868 100644
--- a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -23,4 +23,279 @@
 		device_type = "memory";
 		reg = <0x40000000 0x20000000>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		led-power {
+			gpios = <&plreg 6 0>;
+			default_state = "off";
+		};
+
+		led-heartbeat {
+			gpios = <&plreg 7 0>;
+			default_state = "off";
+		};
+
+		led-identify {
+			gpios = <&plreg 56 0>;
+			default-state = "off";
+		};
+
+		led-health_red {
+			gpios = <&plreg 57 0>;
+			default_state = "off";
+		};
+
+		led-health_amber {
+			gpios = <&plreg 58 0>;
+			default-state = "off";
+		};
+	};
+};
+
+&spifi {
+	status = "okay";
+	flash@0 {
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			u-boot@0 {
+				label = "u-boot";
+				reg = <0x0 0x60000>;
+			};
+
+			u-boot-env@60000 {
+				label = "u-boot-env";
+				reg = <0x60000 0x20000>;
+			};
+
+			kernel@80000 {
+				label = "kernel";
+				reg = <0x80000 0x4c0000>;
+			};
+
+			rofs@540000 {
+				label = "rofs";
+				reg = <0x540000 0x1740000>;
+			};
+
+			rwfs@1c80000 {
+				label = "rwfs";
+				reg = <0x1c80000 0x250000>;
+			};
+
+			section@1ed0000{
+				label = "section";
+				reg = <0x1ed0000 0x130000>;
+			};
+		};
+	};
+	flash@1 {
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			host-prime@0 {
+				label = "host-prime";
+				reg = <0x0 0x02000000>;
+			};
+
+			host-second@2000000 {
+				label = "host-second";
+				reg = <0x02000000 0x02000000>;
+			};
+		};
+	};
+};
+
+&plreg {
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-line-names =
+	"", "", "", "", "",
+	"", "POWER", "HEARTBEAT", "FAN1_INST", "FAN2_INST",
+	"FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
+	"FAN8_INST", "FAN9_INST", "FAN10_INST", "FAN11_INST", "FAN12_INST",
+	"FAN13_INST", "FAN14_INST", "FAN15_INST", "FAN16_INST", "FAN1_FAIL",
+	"FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
+	"FAN7_FAIL", "FAN8_FAIL", "FAN9_FAIL", "FAN10_FAIL", "FAN11_FAIL",
+	"FAN12_FAIL", "FAN13_FAIL", "FAN14_FAIL", "FAN15_FAIL", "FAN16_FAIL",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON",
+	"", "SIO_POWER_GOOD", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5",
+	"SIO_ONCONTROL", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "",
+	"", "", "", "", "";
+	fan1 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x01>;
+	};
+	fan2 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x02>;
+	};
+	fan3 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x04>;
+	};
+	fan4 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x08>;
+	};
+	fan5 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x10>;
+	};
+	fan6 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x40>;
+	};
+	fan7 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x40>;
+	};
+	fan8 {
+		inst = <0x27>;
+		fail = <0x29>;
+		id = <0x2B>;
+		bit = <0x80>;
+	};
+	fan9 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x01>;
+	};
+	fan10 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x02>;
+	};
+	fan11 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x04>;
+	};
+	fan12 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x08>;
+	};
+	fan13 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x10>;
+	};
+	fan14 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x20>;
+	};
+	fan15 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x40>;
+	};
+	fan16 {
+		inst = <0x28>;
+		fail = <0x2A>;
+		id = <0x2C>;
+		bit = <0x80>;
+	};
+	healthled {
+		red = <0x0D 0x20>;
+		amber = <0x0D 0x30>;
+		green = <0x0D 0x10>;
+	};
+	iopled1 {
+		on = <0x04 0x01>;
+	};
+	iopled2 {
+		on = <0x04 0x02>;
+	};
+	iopled3 {
+		on = <0x04 0x04>;
+	};
+	iopled4 {
+		on = <0x04 0x08>;
+	};
+	iopled5 {
+		on = <0x04 0x10>;
+	};
+	iopled6 {
+		on = <0x04 0x20>;
+	};
+	iopled7 {
+		on = <0x04 0x40>;
+	};
+	iopled8 {
+		on = <0x04 0x80>;
+	};
+	identifyled {
+		on = <0x05 0xC0>;
+		off = <0x05 0x40>;
+	};
+	acm {
+		forceoff = <0x0A 0x01>;
+		removed = <0x0A 0x02>;
+		unlatchreq = <0x0A 0x04>;
+	};
+	serverid {
+		lower = <0x01 0xFF>;
+		upper = <0x02 0xFF>;
+	};
+	sideband {
+		disabled = <0x40 0x03>;
+		embedded = <0x40 0x02>;
+		adaptive = <0x40 0x01>;
+	};
+	grp5intflag {
+		ack = <0xB0 0xFF>;
+		pwrbtn = <0xB0 0x01>;
+		uidpress = <0xB0 0x02>;
+		slpint = <0xB0 0x04>;
+	};
+	grp5intmask {
+		pwrbtn = <0xB1 0x01>;
+		slpint = <0xB1 0x40>;
+	};
+	grpintsmasks {
+		grp5 = <0x88 0x10>;
+	};
+	grpintsflags {
+		grp5 = <0x8C 0x10>;
+	};
+	pwrbtn {
+		latch = <0x0F 0xFF 0x04>;
+	};
 };
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
index cf735b3c4f35..96003667bebe 100644
--- a/arch/arm/boot/dts/hpe-gxp.dtsi
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -56,9 +56,28 @@
 			compatible = "simple-bus";
 			#address-cells = <1>;
 			#size-cells = <1>;
-			ranges = <0x0 0xc0000000 0x30000000>;
+			ranges = <0x0 0xc0000000 0x40000000>;
 			dma-ranges;
 
+			spifi: spi@200 {
+				compatible = "hpe,gxp-spifi";
+				reg = <0x200 0x80>, <0xc000 0x100>, <0x38000000 0x8000000>;
+				interrupts = <20>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				flash@0 {
+					reg = <0>;
+					compatible = "jedec,spi-nor";
+				};
+
+				flash@1 {
+					reg = <1>;
+					compatible = "jedec,spi-nor";
+				};
+			};
+
 			vic0: interrupt-controller@eff0000 {
 				compatible = "arm,pl192-vic";
 				reg = <0xeff0000 0x1000>;
@@ -122,6 +141,13 @@
 				interrupts = <6>;
 				interrupt-parent = <&vic0>;
 			};
+
+			plreg: plreg@d1000000 {
+				compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
+				reg = <0xd1000000 0xff>;
+				interrupts = <26>;
+				interrupt-parent = <&vic0>;
+			};
 		};
 	};
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers
  2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
                   ` (2 preceding siblings ...)
  2022-10-11 18:55 ` [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support nick.hawkins
@ 2022-10-11 18:55 ` nick.hawkins
  2022-10-11 19:54   ` Krzysztof Kozlowski
  2022-10-11 18:55 ` [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers nick.hawkins
  4 siblings, 1 reply; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

Adding support for the GXP SPI and PLREG Drivers.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 arch/arm/configs/multi_v7_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 12b35008571f..b1932e258e2b 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -444,6 +444,7 @@ CONFIG_SPI_BCM2835AUX=y
 CONFIG_SPI_CADENCE=y
 CONFIG_SPI_DAVINCI=y
 CONFIG_SPI_FSL_QUADSPI=m
+CONFIG_SPI_GXP=m
 CONFIG_SPI_GPIO=m
 CONFIG_SPI_FSL_DSPI=m
 CONFIG_SPI_OMAP24XX=y
@@ -1136,6 +1137,7 @@ CONFIG_VF610_ADC=m
 CONFIG_XILINX_XADC=y
 CONFIG_IIO_CROS_EC_SENSORS_CORE=m
 CONFIG_IIO_CROS_EC_SENSORS=m
+CONFIG_HPE_GXP_PLREG=y
 CONFIG_STM32_DAC=m
 CONFIG_MPU3050_I2C=y
 CONFIG_CM36651=m
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers
  2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
                   ` (3 preceding siblings ...)
  2022-10-11 18:55 ` [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers nick.hawkins
@ 2022-10-11 18:55 ` nick.hawkins
  2022-10-11 20:06   ` Krzysztof Kozlowski
  4 siblings, 1 reply; 24+ messages in thread
From: nick.hawkins @ 2022-10-11 18:55 UTC (permalink / raw)
  To: verdun, nick.hawkins, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

From: Nick Hawkins <nick.hawkins@hpe.com>

Add the HPE GXP SOC Include files, yaml files, and driver files.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 MAINTAINERS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 56ff555ed5a4..17e8eeaa8178 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2178,14 +2178,17 @@ M:	Jean-Marie Verdun <verdun@hpe.com>
 M:	Nick Hawkins <nick.hawkins@hpe.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/arm/hpe,gxp.yaml
+F:	Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
 F:	Documentation/devicetree/bindings/spi/hpe,gxp-spifi.yaml
 F:	Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
 F:	arch/arm/boot/dts/hpe-bmc*
 F:	arch/arm/boot/dts/hpe-gxp*
 F:	arch/arm/mach-hpe/
 F:	drivers/clocksource/timer-gxp.c
+F:	drivers/soc/hpe/*
 F:	drivers/spi/spi-gxp.c
 F:	drivers/watchdog/gxp-wdt.c
+F:	include/linux/soc/hpe/*
 
 ARM/IGEP MACHINE SUPPORT
 M:	Enric Balletbo i Serra <eballetbo@gmail.com>
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-11 18:55 ` [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg nick.hawkins
@ 2022-10-11 19:51   ` Krzysztof Kozlowski
  2022-10-11 20:27   ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-11 19:51 UTC (permalink / raw)
  To: nick.hawkins, verdun, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The hpe,gxp-plreg binding provides access to the board i/o through the
> Programmable logic interface. The binding provides information to enable
> use of the same driver across the HPE portfolio.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/soc/hpe/hpe,gxp-plreg.yaml       | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
> new file mode 100644
> index 000000000000..cdc54e66d9a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both lines.

> +
> +title: HPE GXP Programmable Logic Registers Controller
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: hpe,gxp-plreg
> +      - const: simple-mfd
> +      - const: syscon

Blank line

> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +
> +additionalProperties: true

This must be false... and then you will see the errors to fix.

> +
> +examples:
> +  - |
> +    cpld@1e789000 {
> +      compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
> +      reg = <0x1e789000 0x1000>;
> +      fan1 {

fan (or fan-1)

> +        bit = <0x01>;
> +        inst = <0x27>;
> +        id = <0x2B>;
> +      };
> +      iopled1 {

led or led-1?

> +        on = <0x04 0x01>;
> +      };
> +      pwrbtn {

power-button?

> +        latch = <0xFF 0xFF 0x04>;

Keep lower case hex everywhere.

> +      };
> +    };
> +

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support
  2022-10-11 18:55 ` [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support nick.hawkins
@ 2022-10-11 19:53   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-11 19:53 UTC (permalink / raw)
  To: nick.hawkins, verdun, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Adding support for the Programmable Logic Register driver in the HPE GXP
> SoC. Additionally adding support for the SPI driver that has already
> been committed to linux (See: drivers/spi/spi-gxp.c).
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 275 +++++++++++++++++++++++
>  arch/arm/boot/dts/hpe-gxp.dtsi           |  28 ++-
>  2 files changed, 302 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> index 3a7382ce40ef..c97b052c4868 100644
> --- a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> @@ -23,4 +23,279 @@
>  		device_type = "memory";
>  		reg = <0x40000000 0x20000000>;
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-power {
> +			gpios = <&plreg 6 0>;
> +			default_state = "off";

Use generic properties for color and function. Same applies to other nodes.

> +		};
> +
> +		led-heartbeat {
> +			gpios = <&plreg 7 0>;
> +			default_state = "off";
> +		};
> +
> +		led-identify {
> +			gpios = <&plreg 56 0>;
> +			default-state = "off";
> +		};
> +
> +		led-health_red {
> +			gpios = <&plreg 57 0>;
> +			default_state = "off";
> +		};
> +
> +		led-health_amber {
> +			gpios = <&plreg 58 0>;
> +			default-state = "off";
> +		};
> +	};
> +};
> +
> +&spifi {
> +	status = "okay";

Blank line

> +	flash@0 {
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			u-boot@0 {
> +				label = "u-boot";
> +				reg = <0x0 0x60000>;
> +			};
> +
> +			u-boot-env@60000 {
> +				label = "u-boot-env";
> +				reg = <0x60000 0x20000>;
> +			};
> +
> +			kernel@80000 {
> +				label = "kernel";
> +				reg = <0x80000 0x4c0000>;
> +			};
> +
> +			rofs@540000 {
> +				label = "rofs";
> +				reg = <0x540000 0x1740000>;
> +			};
> +
> +			rwfs@1c80000 {
> +				label = "rwfs";
> +				reg = <0x1c80000 0x250000>;
> +			};
> +
> +			section@1ed0000{
> +				label = "section";
> +				reg = <0x1ed0000 0x130000>;
> +			};
> +		};
> +	};

Blank line


> +	flash@1 {
> +		partitions {
> +			compatible = "fixed-partitions";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +
> +			host-prime@0 {
> +				label = "host-prime";
> +				reg = <0x0 0x02000000>;
> +			};
> +
> +			host-second@2000000 {
> +				label = "host-second";
> +				reg = <0x02000000 0x02000000>;
> +			};
> +		};
> +	};
> +};
> +
> +&plreg {
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +	gpio-line-names =
> +	"", "", "", "", "",

Messed indentation.

> +	"", "POWER", "HEARTBEAT", "FAN1_INST", "FAN2_INST",
> +	"FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
> +	"FAN8_INST", "FAN9_INST", "FAN10_INST", "FAN11_INST", "FAN12_INST",
> +	"FAN13_INST", "FAN14_INST", "FAN15_INST", "FAN16_INST", "FAN1_FAIL",
> +	"FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
> +	"FAN7_FAIL", "FAN8_FAIL", "FAN9_FAIL", "FAN10_FAIL", "FAN11_FAIL",
> +	"FAN12_FAIL", "FAN13_FAIL", "FAN14_FAIL", "FAN15_FAIL", "FAN16_FAIL",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON",
> +	"", "SIO_POWER_GOOD", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5",
> +	"SIO_ONCONTROL", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "",
> +	"", "", "", "", "";
> +	fan1 {

fan-1? Difficult to suggest as you did not explain this in the binding.

> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x01>;
> +	};
> +	fan2 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x02>;
> +	};
> +	fan3 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x04>;
> +	};
> +	fan4 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x08>;
> +	};
> +	fan5 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x10>;
> +	};
> +	fan6 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x40>;
> +	};
> +	fan7 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x40>;
> +	};
> +	fan8 {
> +		inst = <0x27>;
> +		fail = <0x29>;
> +		id = <0x2B>;
> +		bit = <0x80>;
> +	};
> +	fan9 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x01>;
> +	};
> +	fan10 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x02>;
> +	};
> +	fan11 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x04>;
> +	};
> +	fan12 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x08>;
> +	};
> +	fan13 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x10>;
> +	};
> +	fan14 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x20>;
> +	};
> +	fan15 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x40>;
> +	};
> +	fan16 {
> +		inst = <0x28>;
> +		fail = <0x2A>;
> +		id = <0x2C>;
> +		bit = <0x80>;
> +	};
> +	healthled {
> +		red = <0x0D 0x20>;
> +		amber = <0x0D 0x30>;
> +		green = <0x0D 0x10>;
> +	};
> +	iopled1 {
> +		on = <0x04 0x01>;
> +	};
> +	iopled2 {
> +		on = <0x04 0x02>;
> +	};
> +	iopled3 {
> +		on = <0x04 0x04>;
> +	};
> +	iopled4 {
> +		on = <0x04 0x08>;
> +	};
> +	iopled5 {
> +		on = <0x04 0x10>;
> +	};
> +	iopled6 {
> +		on = <0x04 0x20>;
> +	};
> +	iopled7 {
> +		on = <0x04 0x40>;
> +	};
> +	iopled8 {
> +		on = <0x04 0x80>;
> +	};
> +	identifyled {
> +		on = <0x05 0xC0>;
> +		off = <0x05 0x40>;
> +	};
> +	acm {
> +		forceoff = <0x0A 0x01>;
> +		removed = <0x0A 0x02>;
> +		unlatchreq = <0x0A 0x04>;
> +	};
> +	serverid {
> +		lower = <0x01 0xFF>;
> +		upper = <0x02 0xFF>;
> +	};
> +	sideband {
> +		disabled = <0x40 0x03>;
> +		embedded = <0x40 0x02>;
> +		adaptive = <0x40 0x01>;
> +	};
> +	grp5intflag {
> +		ack = <0xB0 0xFF>;
> +		pwrbtn = <0xB0 0x01>;
> +		uidpress = <0xB0 0x02>;
> +		slpint = <0xB0 0x04>;
> +	};
> +	grp5intmask {
> +		pwrbtn = <0xB1 0x01>;
> +		slpint = <0xB1 0x40>;
> +	};
> +	grpintsmasks {
> +		grp5 = <0x88 0x10>;
> +	};
> +	grpintsflags {
> +		grp5 = <0x8C 0x10>;
> +	};
> +	pwrbtn {
> +		latch = <0x0F 0xFF 0x04>;
> +	};
>  };
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> index cf735b3c4f35..96003667bebe 100644
> --- a/arch/arm/boot/dts/hpe-gxp.dtsi
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -56,9 +56,28 @@
>  			compatible = "simple-bus";
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> -			ranges = <0x0 0xc0000000 0x30000000>;
> +			ranges = <0x0 0xc0000000 0x40000000>;
>  			dma-ranges;
>  
> +			spifi: spi@200 {
> +				compatible = "hpe,gxp-spifi";
> +				reg = <0x200 0x80>, <0xc000 0x100>, <0x38000000 0x8000000>;
> +				interrupts = <20>;
> +				interrupt-parent = <&vic0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				status = "disabled";

Blank line.

> +				flash@0 {
> +					reg = <0>;
> +					compatible = "jedec,spi-nor";
> +				};
> +
> +				flash@1 {
> +					reg = <1>;
> +					compatible = "jedec,spi-nor";
> +				};
> +			};
> +
>  			vic0: interrupt-controller@eff0000 {
>  				compatible = "arm,pl192-vic";
>  				reg = <0xeff0000 0x1000>;
> @@ -122,6 +141,13 @@
>  				interrupts = <6>;
>  				interrupt-parent = <&vic0>;
>  			};
> +
> +			plreg: plreg@d1000000 {

Use same node name as in bindings example...

> +				compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
> +				reg = <0xd1000000 0xff>;
> +				interrupts = <26>;
> +				interrupt-parent = <&vic0>;
> +			};
>  		};
>  	};
>  };

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers
  2022-10-11 18:55 ` [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers nick.hawkins
@ 2022-10-11 19:54   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-11 19:54 UTC (permalink / raw)
  To: nick.hawkins, verdun, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Adding support for the GXP SPI and PLREG Drivers.

This we see in the diff. You must explain why. Why are you doing it? Why
do we want it in the kernel?

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/configs/multi_v7_defconfig | 2 ++


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver
  2022-10-11 18:55 ` [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver nick.hawkins
@ 2022-10-11 20:05   ` Krzysztof Kozlowski
  2022-10-12 20:25     ` Hawkins, Nick
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-11 20:05 UTC (permalink / raw)
  To: nick.hawkins, verdun, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP SoC is interfaced with a programmable logic device that takes
> inputs/outputs from the server board. All these inputs/outputs are
> presented in register form to the SoC. The Programmable Register Driver
> enables access to these registers and provides a standard way to
> provide access across the HPE portfolio.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  drivers/soc/Kconfig          |    1 +
>  drivers/soc/Makefile         |    1 +
>  drivers/soc/hpe/Kconfig      |   19 +
>  drivers/soc/hpe/Makefile     |    7 +
>  drivers/soc/hpe/gxp-plreg.c  | 1207 ++++++++++++++++++++++++++++++++++
>  drivers/soc/hpe/gxp-soclib.c |   19 +
>  drivers/soc/hpe/gxp-soclib.h |   15 +
>  include/linux/soc/hpe/gxp.h  |   15 +
>  8 files changed, 1284 insertions(+)
>  create mode 100644 drivers/soc/hpe/Kconfig
>  create mode 100644 drivers/soc/hpe/Makefile
>  create mode 100644 drivers/soc/hpe/gxp-plreg.c
>  create mode 100644 drivers/soc/hpe/gxp-soclib.c
>  create mode 100644 drivers/soc/hpe/gxp-soclib.h
>  create mode 100644 include/linux/soc/hpe/gxp.h
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e461c071189b..e4fed449d619 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 69ba6508cf2c..ebdab9bcbe79 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..2bdc9785fe83
> --- /dev/null
> +++ b/drivers/soc/hpe/Kconfig
> @@ -0,0 +1,19 @@
> +config SOC_VENDOR_HPE
> +	bool "HPE SoC drivers"
> +	default y
> +	depends on ARCH_HPE 

|| COMPILE_TEST
(and test it)

> +
> +if SOC_VENDOR_HPE
> +
> +config HPE_GXP_SOCLIB
> +	bool
> +	default n
> +
> +config HPE_GXP_PLREG
> +	bool "GXP Programmable logic register support"
> +	depends on ARCH_HPE_GXP
> +	select HPE_GXP_SOCLIB

|| COMPILE_TEST
(and test it)

> +	select GPIOLIB_IRQCHIP
> +	help
> +	  Say yes here to add support for the PLREG.
> +endif
> diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
> new file mode 100644
> index 000000000000..f304fe0192c9
> --- /dev/null
> +++ b/drivers/soc/hpe/Makefile
> @@ -0,0 +1,7 @@
> +obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
> +obj-$(CONFIG_HPE_GXP_PLREG) += gxp-plreg.o
> +obj-$(CONFIG_HPE_GXP_FN2) += gxp-fn2.o
> +obj-$(CONFIG_HPE_GXP_CSM) += gxp-csm.o
> +obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
> +obj-$(CONFIG_HPE_GXP_CHIF) += gxp-chif.o
> +obj-$(CONFIG_HPE_GXP_DBG) += gxp-dbg.o

Where are all these CONFIGs defined?

> diff --git a/drivers/soc/hpe/gxp-plreg.c b/drivers/soc/hpe/gxp-plreg.c
> new file mode 100644
> index 000000000000..c2b038a3f4ba
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-plreg.c
> @@ -0,0 +1,1207 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
> +
> +#include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/soc/hpe/gxp.h>
> +#include "gxp-soclib.h"
> +
> +#define IOP_LED1 0
> +#define IOP_LED2 1
> +#define IOP_LED3 2
> +#define IOP_LED4 3
> +#define IOP_LED5 4
> +#define IOP_LED6 5
> +#define IOP_LED7 6
> +#define IOP_LED8 7
> +#define FAN1_INST 8
> +#define FAN2_INST 9
> +#define FAN3_INST 10
> +#define FAN4_INST 11
> +#define FAN5_INST 12
> +#define FAN6_INST 13
> +#define FAN7_INST 14
> +#define FAN8_INST 15
> +#define FAN9_INST 16
> +#define FAN10_INST 17
> +#define FAN11_INST 18
> +#define FAN12_INST 19
> +#define FAN13_INST 20
> +#define FAN14_INST 21
> +#define FAN15_INST 22
> +#define FAN16_INST 23
> +#define FAN1_FAIL 24
> +#define FAN2_FAIL 25
> +#define FAN3_FAIL 26
> +#define FAN4_FAIL 27
> +#define FAN5_FAIL 28
> +#define FAN6_FAIL 29
> +#define FAN7_FAIL 30
> +#define FAN8_FAIL 31
> +#define FAN9_FAIL 32
> +#define FAN10_FAIL 33
> +#define FAN11_FAIL 34
> +#define FAN12_FAIL 35
> +#define FAN13_FAIL 36
> +#define FAN14_FAIL 37
> +#define FAN15_FAIL 38
> +#define FAN16_FAIL 39
> +#define FAN1_ID 40
> +#define FAN2_ID 41
> +#define FAN3_ID 42
> +#define FAN4_ID 43
> +#define FAN5_ID 44
> +#define FAN6_ID 45
> +#define FAN7_ID 46
> +#define FAN8_ID 47
> +#define FAN9_ID 48
> +#define FAN10_ID 49
> +#define FAN11_ID 50
> +#define FAN12_ID 51
> +#define FAN13_ID 52
> +#define FAN14_ID 53
> +#define FAN15_ID 54
> +#define FAN16_ID 55
> +#define LED_IDENTIFY 56
> +#define LED_HEALTH_RED 57
> +#define LED_HEALTH_AMBER 58
> +#define PWR_BTN_INT 59
> +#define UID_PRESS_INT 60
> +#define SLP_INT 61
> +#define PME_INT 62
> +#define RESV0 63
> +#define RESV1 64
> +#define RESV2 65
> +#define ACM_FORCE_OFF 70
> +#define ACM_REMOVED 71
> +#define ACM_REQ_N 72
> +
> +#define PLREG_INT_GRP5_PIN_BASE	59
> +
> +#define MAX_FAN 16
> +#define IOP_LED_QUANTITY 8
> +#define BYTE 0
> +#define MASK 1
> +#define VALUE 2
> +
> +struct fan_access {
> +	u32 inst;
> +	u32 fail;
> +	u32 id;
> +	u32 bit;
> +};
> +
> +struct iop_led_access {
> +	u32 iop_led[2];
> +};
> +
> +struct health_led_access {
> +	u32 red[2];
> +	u32 amber[2];
> +	u32 green[2];
> +};
> +
> +struct identify_led_access {
> +	u32 off[2];
> +	u32 on[2];
> +};
> +
> +struct acm_access {
> +	int exists;
> +	u32 force_off[2];
> +	u32 removed[2];
> +	u32 unlatch_req[2];
> +};
> +
> +struct server_id_access {
> +	u32 upper[2];
> +	u32 lower[2];
> +};
> +
> +struct sideband_access {
> +	u32 disabled[2];
> +	u32 embedded[2];
> +	u32 adaptive[2];
> +};
> +
> +struct grp5_intr_flag_access {
> +	u32 ack[2];
> +	u32 pwrbtn[2];
> +	u32 uidpress[2];
> +	u32 slpintr[2];
> +};
> +
> +struct grp5_intr_mask_access {
> +	u32 pwrbtn[2];
> +	u32 slpintr[2];
> +};
> +
> +struct grp_intr_masks_access {
> +	u32 grp5[2];
> +};
> +
> +struct grp_intr_flags_access {
> +	u32 grp5[2];
> +};
> +
> +struct pwrbtn_access {
> +	u32 latch[3];
> +};
> +
> +struct gxp_plreg_drvdata {
> +	void __iomem *base;
> +	struct regmap *plreg_map;
> +	struct gpio_chip gpio_chip;
> +	int irq;
> +	struct fan_access fan[16];
> +	struct health_led_access health_led;
> +	struct iop_led_access iop_led[8];
> +	struct identify_led_access identify_led;
> +	struct acm_access acm;
> +	struct server_id_access server_id;
> +	struct sideband_access sideband;
> +	struct grp5_intr_flag_access grp5_intr_flag;
> +	struct grp5_intr_mask_access grp5_intr_mask;
> +	struct grp_intr_masks_access grp_intr_masks;
> +	struct grp_intr_flags_access grp_intr_flags;
> +	struct pwrbtn_access pwrbtn;
> +};
> +
> +struct gxp_plreg_drvdata client_data;

Global? Nope. Even if it were static - drop it.

> +
> +static void address_translation(u32 desired_offset, u32 *offset, u32 *bit_shift)
> +{
> +	*offset = (desired_offset & 0xffc);
> +	*bit_shift = (desired_offset - *offset) * 8;
> +}
> +
> +int gxp_plreg_get_fan_inst(int fan)
> +{
> +	struct gxp_plreg_drvdata *drvdata = &client_data;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +	u32 val;
> +
> +	address_translation(drvdata->fan[fan].inst,
> +			    &trans_offset,
> +			    &trans_shift);
> +
> +	regmap_read(drvdata->plreg_map, trans_offset, &val);
> +	val = (val >> trans_shift) & drvdata->fan[fan].bit;
> +	if (val == drvdata->fan[fan].bit)
> +		return 1;
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL(gxp_plreg_get_fan_inst);

No export - there is no user. And then you will see warning to make it
static... and then you will notice it is not used. Drop entire function.
Dead code (unused) is not accepted.

What is more - there is no possibility anything could call it, so it is
pure dead code.

> +
> +int gxp_plreg_get_fan_fail(int fan)
> +{
> +	struct gxp_plreg_drvdata *drvdata = &client_data;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +	u32 val;
> +
> +	address_translation(drvdata->fan[fan].fail,
> +			    &trans_offset,
> +			    &trans_shift);
> +
> +	regmap_read(drvdata->plreg_map, trans_offset, &val);
> +	val = (val >> trans_shift) & drvdata->fan[fan].bit;
> +	if (val == drvdata->fan[fan].bit)
> +		return 1;
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL(gxp_plreg_get_fan_fail);

Ditto

> +
> +int gxp_plreg_get_fan_id(int fan)
> +{
> +	struct gxp_plreg_drvdata *drvdata = &client_data;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +	u32 val;
> +
> +	address_translation(drvdata->fan[fan].id,
> +			    &trans_offset,
> +			    &trans_shift);
> +
> +	regmap_read(drvdata->plreg_map, trans_offset, &val);
> +	val = (val >> trans_shift) & drvdata->fan[fan].bit;
> +	if (val == drvdata->fan[fan].bit)
> +		return 1;
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL(gxp_plreg_get_fan_id);

More of these...

> +
> +void gxp_plreg_do_virt_pwr_btn_latch(void)
> +{
> +	struct gxp_plreg_drvdata *drvdata = &client_data;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +
> +	address_translation(drvdata->pwrbtn.latch[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +
> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   drvdata->pwrbtn.latch[MASK] << trans_shift,
> +			   drvdata->pwrbtn.latch[VALUE] << trans_shift);
> +}
> +EXPORT_SYMBOL(gxp_plreg_do_virt_pwr_btn_latch);

Ditto...

> +
> +static ssize_t server_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
> +	int value_upper;
> +	int value_lower;
> +	ssize_t ret;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +
> +	/* read upper first */
> +	address_translation(drvdata->server_id.upper[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
> +	value_upper = value_upper >> trans_shift;
> +	value_upper = value_upper & drvdata->server_id.upper[MASK];
> +
> +	/* read lower last */
> +	address_translation(drvdata->server_id.lower[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
> +	value_lower = value_lower >> trans_shift;
> +	value_lower = value_lower & drvdata->server_id.lower[MASK];
> +
> +	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RO(server_id);

Missing sysfs documentation.

(...)

> +
> +static void gxp_gpio_irq_ack(struct irq_data *d)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
> +	unsigned int val;
> +	u32 trans_offset;
> +	u32 trans_shift;
> +
> +	// Read latched interrupt
> +	address_translation(drvdata->grp5_intr_flag.ack[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +	regmap_read(drvdata->plreg_map, trans_offset, &val);
> +
> +	//Clear latched interrupt

Keep consistent style. Earlier comments were //_. This one lacks
space... Don't do things differently.

Comments go with /*

> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift,
> +			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift);
> +}
> +
> +static void gxp_gpio_irq_set_mask(struct irq_data *d, bool set)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(chip->parent);
> +	u32 trans_offset;
> +	u32 trans_shift;
> +
> +	address_translation(drvdata->grp5_intr_flag.ack[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +
> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   (drvdata->grp5_intr_flag.pwrbtn[MASK] |
> +			   drvdata->grp5_intr_flag.slpintr[MASK]) << trans_shift,
> +			   set == 1 ? 0 : (drvdata->grp5_intr_flag.pwrbtn[MASK] |
> +			   drvdata->grp5_intr_flag.slpintr[MASK]) << trans_shift);
> +
> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift,
> +			   drvdata->grp5_intr_flag.ack[MASK] << trans_shift);
> +}
> +
> +static void gxp_gpio_irq_mask(struct irq_data *d)
> +{
> +	gxp_gpio_irq_set_mask(d, false);
> +}
> +
> +static void gxp_gpio_irq_unmask(struct irq_data *d)
> +{
> +	gxp_gpio_irq_set_mask(d, true);
> +}
> +
> +static int gxp_gpio_set_type(struct irq_data *d, unsigned int type)
> +{
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		irq_set_handler_locked(d, handle_level_irq);
> +	else
> +		irq_set_handler_locked(d, handle_edge_irq);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t gxp_plreg_irq_handle(int irq, void *_drvdata)
> +{
> +	struct gxp_plreg_drvdata *drvdata = (struct gxp_plreg_drvdata *)_drvdata;
> +	unsigned int val, girq, i;
> +
> +	/* handle plreg interrupt group5 */
> +	val = readb(drvdata->base + drvdata->grp5_intr_flag.ack[BYTE]);
> +
> +	for_each_set_bit(i, (unsigned long *)&val, 3) {
> +		girq = irq_find_mapping(drvdata->gpio_chip.irq.domain,
> +					i + PLREG_INT_GRP5_PIN_BASE);
> +		generic_handle_irq(girq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct gpio_chip plreg_chip = {
> +	.label			= "gxp-plreg",
> +	.owner			= THIS_MODULE,
> +	.get			= gxp_gpio_plreg_get,
> +	.set			= gxp_gpio_plreg_set,
> +	.get_direction = gxp_gpio_plreg_get_direction,

Keep code consistent.

> +	.direction_input = gxp_gpio_plreg_direction_input,
> +	.direction_output = gxp_gpio_plreg_direction_output,
> +	.base = -1,
> +	//.can_sleep		= true,

No dead code in the kernel.

> +};
> +
> +static struct irq_chip gxp_gpio_irqchip = {
> +	.name		= "gxp-plreg",
> +	.irq_ack	= gxp_gpio_irq_ack,
> +	.irq_mask	= gxp_gpio_irq_mask,
> +	.irq_unmask	= gxp_gpio_irq_unmask,
> +	.irq_set_type	= gxp_gpio_set_type,
> +};
> +
> +static const struct of_device_id gxp_plreg_of_match[] = {
> +	{ .compatible = "hpe,gxp-plreg" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_plreg_of_match);
> +
> +static int gxp_plreg_probe(struct platform_device *pdev)
> +{
> +	int i;
> +	int j;
> +	int ret;
> +	struct gxp_plreg_drvdata *drvdata;
> +	struct resource *res;
> +	struct gpio_irq_chip *girq;
> +	struct device_node *np;
> +	char name_buf[10];
> +	u32 trans_offset;
> +	u32 trans_shift;
> +
> +	drvdata = devm_kzalloc(&pdev->dev,
> +			       sizeof(struct gxp_plreg_drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
> +
> +	drvdata->plreg_map = syscon_regmap_lookup_by_compatible("hpe,gxp-plreg");
> +	if (IS_ERR(drvdata->plreg_map)) {
> +		dev_err(&pdev->dev, "Unable to find plreg regmap\n");
> +		return PTR_ERR(drvdata->plreg_map);
> +	}
> +
> +	/* Supply driver with information to access specific offsets in plreg */
> +	for (i = 0; i < MAX_FAN; i++) {
> +		/* Find Fan Children */
> +		snprintf(name_buf, sizeof(name_buf), "fan%d", i + 1);
> +		np = of_get_child_by_name(pdev->dev.of_node, name_buf);
> +
> +		if (np) {
> +		/* For each child there should be 3 fan properties */
> +			if (of_property_read_u32(np, "inst",
> +						 &drvdata->fan[i].inst)) {
> +				dev_err(&pdev->dev, "%s is missing its 'inst' property\n",
> +					name_buf);
> +				return -ENODEV;
> +			}
> +
> +			if (of_property_read_u32(np, "fail",
> +						 &drvdata->fan[i].fail)) {
> +				dev_err(&pdev->dev, "%s is missing its 'fail' property\n",
> +					name_buf);
> +				return -ENODEV;
> +			}
> +			if (of_property_read_u32(np, "id",
> +						 &drvdata->fan[i].id)) {
> +				dev_err(&pdev->dev, "%s is missing its 'id' property\n",
> +					name_buf);
> +				return -ENODEV;
> +			}
> +			if (of_property_read_u32(np, "bit",
> +						 &drvdata->fan[i].bit)) {
> +				dev_err(&pdev->dev, "%s is missing its 'bit' property\n",
> +					name_buf);
> +				return -ENODEV;
> +			}
> +		} else {
> +			dev_warn(&pdev->dev, "%pOF is missing its '%s' node\n", np, name_buf);
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "healthled");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'healthled' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "red", i,
> +					       &drvdata->health_led.red[i])) {
> +			dev_err(&pdev->dev, "healthled is missing its 'red' property index %d\n",
> +				i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "amber", i,
> +					       &drvdata->health_led.amber[i])) {
> +			dev_err(&pdev->dev, "healthled is missing its 'amber' property index %d\n",
> +				i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "green", i,
> +					       &drvdata->health_led.green[i])) {
> +			dev_err(&pdev->dev, "healthled is missing its 'green' property index %d\n",
> +				i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i < IOP_LED_QUANTITY; i++) {
> +		/* Find iopLed Children */
> +		snprintf(name_buf, sizeof(name_buf), "iopled%d", i + 1);
> +		np = of_get_child_by_name(pdev->dev.of_node, name_buf);
> +
> +		if (np) {
> +		/* For each child there should be 1 property */
> +			for (j = 0; j <= MASK; j++) {
> +				if (of_property_read_u32_index(np, "on", j,
> +							       &drvdata->iop_led[i].iop_led[j]
> +							       )) {
> +					dev_err(&pdev->dev, "%s is missing its 'on' property index %d\n",
> +						name_buf, j);
> +					return -ENODEV;
> +				}
> +			}
> +		} else {
> +			dev_err(&pdev->dev, "%pOF is missing its '%s' node\n", np, name_buf);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "identifyled");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'identifyled' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "on", i, &drvdata->identify_led.on[i])) {
> +			dev_err(&pdev->dev, "identifyled is missing its 'on' property index %d\n",
> +				i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "off", i, &drvdata->identify_led.off[i])) {
> +			dev_err(&pdev->dev, "identifyled is missing its 'off' property index %d\n",
> +				i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "acm");
> +	if (!np) {
> +		dev_warn(&pdev->dev, "%pOF is missing its 'acm' node\n", np);
> +	} else {
> +		for (i = 0; i <= MASK; i++) {
> +			if (of_property_read_u32_index(np, "forceoff", i,
> +						       &drvdata->acm.force_off[i])) {
> +				dev_err(&pdev->dev,
> +					"acm is missing its 'forceoff' property index %d\n", i);
> +				return -ENODEV;
> +			}
> +		}
> +
> +		for (i = 0; i <= MASK; i++) {
> +			if (of_property_read_u32_index(np, "removed", i,
> +						       &drvdata->acm.removed[i])) {
> +				dev_err(&pdev->dev,
> +					"acm is missing its 'removed' property index %d\n", i);
> +				return -ENODEV;
> +			}
> +		}
> +
> +		for (i = 0; i <= MASK; i++) {
> +			if (of_property_read_u32_index(np, "unlatchreq", i,
> +						       &drvdata->acm.unlatch_req[i])) {
> +				dev_err(&pdev->dev,
> +					"acm is missing its 'unlatchreq' property index %d\n", i);
> +				return -ENODEV;
> +			}
> +		}
> +		drvdata->acm.exists = 1;
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "serverid");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'serverid' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "upper", i,
> +					       &drvdata->server_id.upper[i])) {
> +			dev_err(&pdev->dev,
> +				"serverid is missing its 'upper' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "lower", i, &drvdata->server_id.lower[i])) {
> +			dev_err(&pdev->dev,
> +				"serverid is missing its 'lower' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "sideband");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'sideband' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "disabled", i,
> +					       &drvdata->sideband.disabled[i])) {
> +			dev_err(&pdev->dev,
> +				"sideband is missing its 'disabled' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "embedded", i,
> +					       &drvdata->sideband.embedded[i])) {
> +			dev_err(&pdev->dev,
> +				"sideband is missing its 'embedded' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "adaptive", i,
> +					       &drvdata->sideband.adaptive[i])) {
> +			dev_err(&pdev->dev,
> +				"sideband is missing its 'adaptive' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "grp5intflag");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'grp5intflag' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "ack", i, &drvdata->grp5_intr_flag.ack[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intflag is missing its 'ack' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "pwrbtn", i,
> +					       &drvdata->grp5_intr_flag.pwrbtn[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intflag is missing its 'pwrbtn' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "uidpress", i,
> +					       &drvdata->grp5_intr_flag.uidpress[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intflag is missing its 'uid' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "slpint", i,
> +					       &drvdata->grp5_intr_flag.slpintr[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intflag is missing its 'slpint' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "grp5intmask");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'grp5intmask' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "pwrbtn", i,
> +					       &drvdata->grp5_intr_mask.pwrbtn[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intmask is missing its 'pwrbtn' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "slpint", i,
> +					       &drvdata->grp5_intr_mask.slpintr[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intmask is missing its 'slpint' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "grpintsmasks");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'grpintsmasks' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "grp5", i,
> +					       &drvdata->grp_intr_masks.grp5[i])) {
> +			dev_err(&pdev->dev,
> +				"grpintsmasks is missing its 'grp5' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "grpintsflags");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'grpintsflags' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= MASK; i++) {
> +		if (of_property_read_u32_index(np, "grp5", i,
> +					       &drvdata->grp_intr_flags.grp5[i])) {
> +			dev_err(&pdev->dev,
> +				"grp5intsflags is missing its 'grp5' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
> +	if (!np) {
> +		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i <= VALUE; i++) {
> +		if (of_property_read_u32_index(np, "latch", i, &drvdata->pwrbtn.latch[i])) {

Undocumented properties. NAK.

> +			dev_err(&pdev->dev, "pwrbtn is missing its 'latch' property index %d\n", i);
> +			return -ENODEV;
> +		}
> +	}
> +
> +	drvdata->gpio_chip = plreg_chip;
> +	drvdata->gpio_chip.ngpio = 100;
> +	drvdata->gpio_chip.parent = &pdev->dev;
> +
> +	girq = &drvdata->gpio_chip.irq;
> +	girq->chip = &gxp_gpio_irqchip;
> +	/* This will let us handle the parent IRQ in the driver */
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_edge_irq;
> +	/* Set up interrupt from PLREG Group 5 Mask */

Please read Linux coding style. There is a chapter about size of
functions. Read it. Actually, read the rest as well... It's ok if
probe() grows above our limit, but this is huge. Really too big.

> +	address_translation(drvdata->grp_intr_flags.grp5[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   drvdata->grp_intr_flags.grp5[MASK] << trans_shift,
> +			   drvdata->grp_intr_flags.grp5[MASK] << trans_shift);
> +	address_translation(drvdata->grp_intr_masks.grp5[BYTE],
> +			    &trans_offset,
> +			    &trans_shift);
> +	regmap_update_bits(drvdata->plreg_map, trans_offset,
> +			   drvdata->grp_intr_masks.grp5[MASK] << trans_shift,
> +			   0x00);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);

dev_err_probe

> +		return ret;
> +	}
> +	drvdata->irq = ret;
> +
> +	ret = devm_request_irq(&pdev->dev, drvdata->irq, gxp_plreg_irq_handle, IRQF_SHARED,
> +			       "gxp-plreg", drvdata);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "IRQ handler failed - %d\n", ret);

dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = sysfs_register(&pdev->dev, drvdata);
> +	if (ret < 0) {
> +		dev_warn(&pdev->dev, "Unable to register sysfs\n");

Missing sysfs documentation

> +		return ret;
> +	}
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->gpio_chip, NULL);
> +	if (ret < 0)
> +		dev_err(&pdev->dev, "Could not register gpiochip for plreg, %d\n", ret);

dev_err_probe

> +
> +	client_data = *drvdata;
> +
> +	dev_info(&pdev->dev, "HPE GXP PLREG driver loaded.\n");

Drop probe confirmations.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver gxp_plreg_driver = {
> +	.probe = gxp_plreg_probe,
> +	.driver = {
> +		.name = "gxp-plreg",
> +		.of_match_table = of_match_ptr(gxp_plreg_of_match),

Compile test and you will see here unused/mismatched pairing. Drop
of_match_ptr or add maybe_unused.

> +	},
> +};
> +module_platform_driver(gxp_plreg_driver);
> +
> +MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
> +MODULE_DESCRIPTION("HPE GXP Programmable Logic Registers Driver");

Missing module license.

> diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
> new file mode 100644
> index 000000000000..e32aa346e770
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/io.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..583902969ad5
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + * Copyright (C) 2019 Hewlett-Packard Development Company, L.P.
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Drop it.

> + */
> +
> +#ifndef __GXP_SOCLIB_H__
> +#define __GXP_SOCLIB_H__
> +
> +extern struct class *soc_class;

Drop entire file... what is the logic behind?

> +
> +#endif
> diff --git a/include/linux/soc/hpe/gxp.h b/include/linux/soc/hpe/gxp.h
> new file mode 100644
> index 000000000000..3573c391ec5a
> --- /dev/null
> +++ b/include/linux/soc/hpe/gxp.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0=or-later */
> +/* Copyright (C) 2022 Hewlett-Packard Development Company, L.P. */
> +
> +#ifndef _LINUX_SOC_HPE_GXP_
> +#define _LINUX_SOC_HPE_GXP_H_
> +
> +#define PLREG_SIDEBAND_UNKNOWN 0x00
> +#define PLREG_SIDEBAND_DISABLED 0x01
> +#define PLREG_SIDEBAND_EMBEDDED 0x02
> +#define PLREG_SIDEBAND_ADAPTIVE 0x03
> +
> +#define GPIO_DIR_OUT 0
> +#define GPIO_DIR_IN  1

Keep all defines prefixed with your device or move it better to
drivers/soc/hpe

> +
> +#endif /* _LINUX_SOC_HPE_GXP_H_ */

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers
  2022-10-11 18:55 ` [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers nick.hawkins
@ 2022-10-11 20:06   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-11 20:06 UTC (permalink / raw)
  To: nick.hawkins, verdun, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 11/10/2022 14:55, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Add the HPE GXP SOC Include files, yaml files, and driver files.

YAML is a format, you added before bindings, so just "bindings". And no
need to repeat files three times.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-11 18:55 ` [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg nick.hawkins
  2022-10-11 19:51   ` Krzysztof Kozlowski
@ 2022-10-11 20:27   ` Rob Herring
  2022-10-12 19:56     ` Hawkins, Nick
  1 sibling, 1 reply; 24+ messages in thread
From: Rob Herring @ 2022-10-11 20:27 UTC (permalink / raw)
  To: nick.hawkins
  Cc: verdun, krzysztof.kozlowski+dt, linux, devicetree, linux-kernel,
	linux-arm-kernel

On Tue, Oct 11, 2022 at 1:56 PM <nick.hawkins@hpe.com> wrote:
>
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> The hpe,gxp-plreg binding provides access to the board i/o through the
> Programmable logic interface. The binding provides information to enable
> use of the same driver across the HPE portfolio.
>
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/soc/hpe/hpe,gxp-plreg.yaml       | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
> new file mode 100644
> index 000000000000..cdc54e66d9a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-plreg.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/soc/hpe/hpe,gxp-plreg.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: HPE GXP Programmable Logic Registers Controller
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: hpe,gxp-plreg
> +      - const: simple-mfd
> +      - const: syscon
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    cpld@1e789000 {
> +      compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
> +      reg = <0x1e789000 0x1000>;
> +      fan1 {
> +        bit = <0x01>;
> +        inst = <0x27>;
> +        id = <0x2B>;

These property names are way too generic for device specific
properties. There is zero description of what the h/w does and any of
these child nodes to give any advice on direction. However, a node per
register or register field is generally the wrong direction.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-11 20:27   ` Rob Herring
@ 2022-10-12 19:56     ` Hawkins, Nick
  2022-10-22 15:52       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Hawkins, Nick @ 2022-10-12 19:56 UTC (permalink / raw)
  To: Rob Herring
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel

> > +examples:
> > +  - |
> > +    cpld@1e789000 {
> > +      compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
> > +      reg = <0x1e789000 0x1000>;
> > +      fan1 {
> > +        bit = <0x01>;
> > +        inst = <0x27>;
> > +        id = <0x2B>;

> These property names are way too generic for device specific properties. There is zero description of what the h/w does and any of these child nodes to give any advice on direction. However, a node per register or register field is generally the wrong direction.

Thank you for your valued feedback. The goal I was trying to achieve here was making our programmable logic register driver interface in a generic way across multiple platforms based on inputs we provide with the .dts file for each platform. For instance if we want to read the fan 1 install status on platform X it would be reading bit 0x01 of byte 0x27 where as on platform Y it could be bit 0x02 of byte 0x16. Is there a format you would recommend that I can adhere too?

Thanks!

-Nick Hawkins
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver
  2022-10-11 20:05   ` Krzysztof Kozlowski
@ 2022-10-12 20:25     ` Hawkins, Nick
  2022-10-13 12:27       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Hawkins, Nick @ 2022-10-12 20:25 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Verdun, Jean-Marie, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

Greetings Krysztof,

Thanks for the feedback! I have several questions below:

> > +
> > +static ssize_t server_id_show(struct device *dev, struct 
> > +device_attribute *attr, char *buf) {
> > +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
> > +	int value_upper;
> > +	int value_lower;
> > +	ssize_t ret;
> > +	u32 trans_offset;
> > +	u32 trans_shift;
> > +
> > +	/* read upper first */
> > +	address_translation(drvdata->server_id.upper[BYTE],
> > +			    &trans_offset,
> > +			    &trans_shift);
> > +	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
> > +	value_upper = value_upper >> trans_shift;
> > +	value_upper = value_upper & drvdata->server_id.upper[MASK];
> > +
> > +	/* read lower last */
> > +	address_translation(drvdata->server_id.lower[BYTE],
> > +			    &trans_offset,
> > +			    &trans_shift);
> > +	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
> > +	value_lower = value_lower >> trans_shift;
> > +	value_lower = value_lower & drvdata->server_id.lower[MASK];
> > +
> > +	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
> > +
> > +	return ret;
> > +}
> > +
> > +static DEVICE_ATTR_RO(server_id);

> Missing sysfs documentation.

Can you point me at the proper location / documentation for documenting sysfs? Thanks!

> > +	for (i = 0; i <= MASK; i++) {
> > +		if (of_property_read_u32_index(np, "grp5", i,
> > +					       &drvdata->grp_intr_flags.grp5[i])) {
> > +			dev_err(&pdev->dev,
> > +				"grp5intsflags is missing its 'grp5' property index %d\n", i);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
> > +	if (!np) {
> > +		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
> > +		return -ENODEV;
> > +	}
> > +
> > +	for (i = 0; i <= VALUE; i++) {
> > +		if (of_property_read_u32_index(np, "latch", i, 
> > +&drvdata->pwrbtn.latch[i])) {

> Undocumented properties. NAK.

If each child node of hpe,gxp-plreg were documented with their respective properties would this be acceptable?

Such as:
Fan-1 {
	Compatible="hpe,gxp-plreg-fan";
	id = <0x27>;
	bit = <0x01>;
	fail = <0x24>;
	inst = <0x22>;
};
	

> > +			dev_err(&pdev->dev, "pwrbtn is missing its 'latch' property index %d\n", i);
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	drvdata->gpio_chip = plreg_chip;
> > +	drvdata->gpio_chip.ngpio = 100;
> > +	drvdata->gpio_chip.parent = &pdev->dev;
> > +
> > +	girq = &drvdata->gpio_chip.irq;
> > +	girq->chip = &gxp_gpio_irqchip;
> > +	/* This will let us handle the parent IRQ in the driver */
> > +	girq->parent_handler = NULL;
> > +	girq->num_parents = 0;
> > +	girq->parents = NULL;
> > +	girq->default_type = IRQ_TYPE_NONE;
> > +	girq->handler = handle_edge_irq;
> > +	/* Set up interrupt from PLREG Group 5 Mask */

> Please read Linux coding style. There is a chapter about size of functions. Read it. Actually, read the rest as well... It's ok if
probe() grows above our limit, but this is huge. Really too big.

I will do this and work on breaking up this code into smaller functions.

Thanks,

-Nick Hawkins
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver
  2022-10-12 20:25     ` Hawkins, Nick
@ 2022-10-13 12:27       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-13 12:27 UTC (permalink / raw)
  To: Hawkins, Nick
  Cc: Verdun, Jean-Marie, robh+dt, krzysztof.kozlowski+dt, linux,
	devicetree, linux-kernel, linux-arm-kernel

On 12/10/2022 16:25, Hawkins, Nick wrote:
> Greetings Krysztof,
> 
> Thanks for the feedback! I have several questions below:
> 
>>> +
>>> +static ssize_t server_id_show(struct device *dev, struct 
>>> +device_attribute *attr, char *buf) {
>>> +	struct gxp_plreg_drvdata *drvdata = dev_get_drvdata(dev);
>>> +	int value_upper;
>>> +	int value_lower;
>>> +	ssize_t ret;
>>> +	u32 trans_offset;
>>> +	u32 trans_shift;
>>> +
>>> +	/* read upper first */
>>> +	address_translation(drvdata->server_id.upper[BYTE],
>>> +			    &trans_offset,
>>> +			    &trans_shift);
>>> +	regmap_read(drvdata->plreg_map, trans_offset, &value_upper);
>>> +	value_upper = value_upper >> trans_shift;
>>> +	value_upper = value_upper & drvdata->server_id.upper[MASK];
>>> +
>>> +	/* read lower last */
>>> +	address_translation(drvdata->server_id.lower[BYTE],
>>> +			    &trans_offset,
>>> +			    &trans_shift);
>>> +	regmap_read(drvdata->plreg_map, trans_offset, &value_lower);
>>> +	value_lower = value_lower >> trans_shift;
>>> +	value_lower = value_lower & drvdata->server_id.lower[MASK];
>>> +
>>> +	ret = sprintf(buf, "0x%04x", value_upper | value_lower);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(server_id);
> 
>> Missing sysfs documentation.
> 
> Can you point me at the proper location / documentation for documenting sysfs? Thanks!

Documentation/ABI/README

> 
>>> +	for (i = 0; i <= MASK; i++) {
>>> +		if (of_property_read_u32_index(np, "grp5", i,
>>> +					       &drvdata->grp_intr_flags.grp5[i])) {
>>> +			dev_err(&pdev->dev,
>>> +				"grp5intsflags is missing its 'grp5' property index %d\n", i);
>>> +			return -ENODEV;
>>> +		}
>>> +	}
>>> +
>>> +	np = of_get_child_by_name(pdev->dev.of_node, "pwrbtn");
>>> +	if (!np) {
>>> +		dev_err(&pdev->dev, "%pOF is missing its 'pwrbtn' node\n", np);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	for (i = 0; i <= VALUE; i++) {
>>> +		if (of_property_read_u32_index(np, "latch", i, 
>>> +&drvdata->pwrbtn.latch[i])) {
> 
>> Undocumented properties. NAK.
> 
> If each child node of hpe,gxp-plreg were documented with their respective properties would this be acceptable?

I would need to see the bindings.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-12 19:56     ` Hawkins, Nick
@ 2022-10-22 15:52       ` Krzysztof Kozlowski
  2022-10-25  0:03         ` Hawkins, Nick
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-22 15:52 UTC (permalink / raw)
  To: Hawkins, Nick, Rob Herring
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel

On 12/10/2022 15:56, Hawkins, Nick wrote:
>>> +examples:
>>> +  - |
>>> +    cpld@1e789000 {
>>> +      compatible = "hpe,gxp-plreg", "simple-mfd", "syscon";
>>> +      reg = <0x1e789000 0x1000>;
>>> +      fan1 {
>>> +        bit = <0x01>;
>>> +        inst = <0x27>;
>>> +        id = <0x2B>;
> 
>> These property names are way too generic for device specific properties. There is zero description of what the h/w does and any of these child nodes to give any advice on direction. However, a node per register or register field is generally the wrong direction.
> 
> Thank you for your valued feedback. The goal I was trying to achieve here was making our programmable logic register driver interface in a generic way across multiple platforms based on inputs we provide with the .dts file for each platform. For instance if we want to read the fan 1 install status on platform X it would be reading bit 0x01 of byte 0x27 where as on platform Y it could be bit 0x02 of byte 0x16. Is there a format you would recommend that I can adhere too?

I don't think DT place is to describe register or memory layout, with
some exceptions like MTD partitions or nvmem cells. Basically you are
representing here a device register map inside DT, just because it is a
CPLD.

Every regular multi-functional device handles its register map in the
driver itself and uses Linux framework to expose the internals. CPLD
should not be different, except that is programmable.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-22 15:52       ` Krzysztof Kozlowski
@ 2022-10-25  0:03         ` Hawkins, Nick
  2022-10-25  0:15           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Hawkins, Nick @ 2022-10-25  0:03 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring

> I don't think DT place is to describe register or memory layout, with some exceptions like MTD partitions or nvmem cells. Basically you are representing here a device register map inside DT, just because it is a CPLD.

> Every regular multi-functional device handles its register map in the driver itself and uses Linux framework to expose the internals. CPLD should not be different, except that is programmable.
Hi Krzysztof,

Thank you for your time and feedback. We are looking for a place to describe differences within our CPLD implementation due to our memory mapping not being consistent. The idea we are pursuing is to use the device tree to serve as an input to Linux to prevent driver code fragmentation from multiple platforms needing their own specific offsets. If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead? 

Thanks,

-Nick Hawkins
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-25  0:03         ` Hawkins, Nick
@ 2022-10-25  0:15           ` Krzysztof Kozlowski
  2022-10-25 18:44             ` Hawkins, Nick
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-25  0:15 UTC (permalink / raw)
  To: Hawkins, Nick
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring

On 24/10/2022 20:03, Hawkins, Nick wrote:
>> I don't think DT place is to describe register or memory layout, with some exceptions like MTD partitions or nvmem cells. Basically you are representing here a device register map inside DT, just because it is a CPLD.
> 
>> Every regular multi-functional device handles its register map in the driver itself and uses Linux framework to expose the internals. CPLD should not be different, except that is programmable.
> Hi Krzysztof,
> 
> Thank you for your time and feedback. We are looking for a place to describe differences within our CPLD implementation due to our memory mapping not being consistent. The idea we are pursuing is to use the device tree to serve as an input to Linux to prevent driver code fragmentation from multiple platforms needing their own specific offsets. 

I understand what you want to achieve, but Devicetree does not seem a
tool for that. DT describes the hardware characteristics, but not is
exact memory map.

Although your goal differs than for example goal of some developer of
I2C or MMIO device drivers, but essentially devices are similar. We do
not describe memory map of MMIO or register map of I2C devices in DT.


> If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead?

I would say use rather standard Linux subsystems and problem is gone.
You have fan? Sure, we have subsystem for fans. You have power supply or
battery - we have stuff for this as well.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-25  0:15           ` Krzysztof Kozlowski
@ 2022-10-25 18:44             ` Hawkins, Nick
  2022-10-25 18:55               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Hawkins, Nick @ 2022-10-25 18:44 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring

> I understand what you want to achieve, but Devicetree does not seem a tool for that. DT describes the hardware characteristics, but not is exact memory map.

> Although your goal differs than for example goal of some developer of I2C or MMIO device drivers, but essentially devices are similar. We do not describe memory map of MMIO or register map of I2C devices in DT.


> > If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead?

> I would say use rather standard Linux subsystems and problem is gone.
> You have fan? Sure, we have subsystem for fans. You have power supply or battery - we have stuff for this as well.

Greetings Krzysztof,

Thanks for your feedback. In this case for something like fans as you suggest above would it be acceptable for the fans to call into plreg(the proposed driver) just to read the fan related registers with the fan driver knowing what offset to use? Multiple drivers will need to access registers in this memory range so I am trying to determine if they all need to map this area or all goto one source to read/write to it.

Thanks,

-Nick Hawkins
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-25 18:44             ` Hawkins, Nick
@ 2022-10-25 18:55               ` Krzysztof Kozlowski
  2022-10-25 19:26                 ` Hawkins, Nick
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-25 18:55 UTC (permalink / raw)
  To: Hawkins, Nick
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring

On 25/10/2022 14:44, Hawkins, Nick wrote:
>> I understand what you want to achieve, but Devicetree does not seem a tool for that. DT describes the hardware characteristics, but not is exact memory map.
> 
>> Although your goal differs than for example goal of some developer of I2C or MMIO device drivers, but essentially devices are similar. We do not describe memory map of MMIO or register map of I2C devices in DT.
> 
> 
>>> If this is not acceptable to do through the device tree, should we rely on having an include file for each platform instead?
> 
>> I would say use rather standard Linux subsystems and problem is gone.
>> You have fan? Sure, we have subsystem for fans. You have power supply or battery - we have stuff for this as well.
> 
> Greetings Krzysztof,
> 
> Thanks for your feedback. In this case for something like fans as you suggest above would it be acceptable for the fans to call into plreg(the proposed driver) just to read the fan related registers with the fan driver knowing what offset to use? Multiple drivers will need to access registers in this memory range so I am trying to determine if they all need to map this area or all goto one source to read/write to it.

I don't know exactly what type of devices you represent in that plreg,
but in general the fan device would be the respective plreg. The same
with other pieces like hwmon, power supply.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-25 18:55               ` Krzysztof Kozlowski
@ 2022-10-25 19:26                 ` Hawkins, Nick
  2022-10-25 19:33                   ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Hawkins, Nick @ 2022-10-25 19:26 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring


> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply.
We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs. When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this:

plreg: plreg@d1000000 {
      compatible = "hpe,gxp-plreg";
      reg = <0xd1000000 0xFF>;
      interrupts = <26>;
      interrupt-parent = <&vic0>;
};

fanctrl: fanctrl@c1000c00 {
      compatible = "hpe,gxp-fan-ctrl";
      reg = <0xc1000c00 0x200>;
      plreg_handle = <&plreg>;
};

Thanks for the assistance,

-Nick Hawkins
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
  2022-10-25 19:26                 ` Hawkins, Nick
@ 2022-10-25 19:33                   ` Krzysztof Kozlowski
       [not found]                     ` <DM4PR84MB197662C12018090C312AF72DD6319@DM4PR84MB1976.NAMPRD84.PROD.OUTLOOK.COM>
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-25 19:33 UTC (permalink / raw)
  To: Hawkins, Nick
  Cc: Verdun, Jean-Marie, krzysztof.kozlowski+dt, linux, devicetree,
	linux-kernel, linux-arm-kernel, Rob Herring

On 25/10/2022 15:26, Hawkins, Nick wrote:
> 
>> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply.
> We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs.

So basically each register (or set of registers) is a device? How is it
different than any other multi-functional device? Why do you want to
model it differently?

> When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this:
> 
> plreg: plreg@d1000000 {
>       compatible = "hpe,gxp-plreg";
>       reg = <0xd1000000 0xFF>;
>       interrupts = <26>;
>       interrupt-parent = <&vic0>;
> };
> 
> fanctrl: fanctrl@c1000c00 {
>       compatible = "hpe,gxp-fan-ctrl";
>       reg = <0xc1000c00 0x200>;
>       plreg_handle = <&plreg>;
> };
> 

No, rather these are one node.

You insist to represent this all as programmable logic, but why? CPLD,
FPGA, ASIC, dedicated IC - all are just implementations and for us
what's matter are the interfaces, inputs and outputs.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
       [not found]                     ` <DM4PR84MB197662C12018090C312AF72DD6319@DM4PR84MB1976.NAMPRD84.PROD.OUTLOOK.COM>
@ 2022-10-25 19:48                       ` Krzysztof Kozlowski
  2022-10-25 19:49                       ` Krzysztof Kozlowski
  1 sibling, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-25 19:48 UTC (permalink / raw)
  To: Verdun, Jean-Marie, Hawkins, Nick
  Cc: krzysztof.kozlowski+dt, linux, devicetree, linux-kernel,
	linux-arm-kernel, Rob Herring

On 25/10/2022 15:39, Verdun, Jean-Marie wrote:
> Hi Krzysztof,
> 

Use mailing list style of replying. I can bear the lack of wrapping (one
huge sentence), but top-post is a no.

> I think what we try to do is to introduce an abstraction layer between the interfaces and the drivers, as our CPLD interfaces are platform dependents. I mean the Power On control could be at address 0x09 on one platform or 0x119 on another one. We would like to find a way to avoid to have to change the driver code, but just feeding the driver with relevant datas, which could be into a platform dependent include file or through the proposed solution that Nick is promoting.

I think this was already said. Repeating it, with very similar words
will not help us...

Let me be then clear: I care little about your goal of abstracting some
driver code. I care about proper Devicetree bindings and proper
representation of hardware in Devicetree sources.

Looks like you want to hack around Devicetree to match your needs. This
does not work. I gave you the proposed solution based on my feeling and
limited understanding of what you have there. If this does not work for
you, then life - you need to find other solution.

> 
> If the CPLD memory address space was consistent between platform and generation that would be great but unfortunately that is not the case that is why we try to break down the dependency into the driver code and retrieve the data from another place.

I don't see how this argument is relevant to my questions. I did not
propose anything which would require some "memory address space
consistency".

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
       [not found]                     ` <DM4PR84MB197662C12018090C312AF72DD6319@DM4PR84MB1976.NAMPRD84.PROD.OUTLOOK.COM>
  2022-10-25 19:48                       ` Krzysztof Kozlowski
@ 2022-10-25 19:49                       ` Krzysztof Kozlowski
  1 sibling, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-25 19:49 UTC (permalink / raw)
  To: Verdun, Jean-Marie, Hawkins, Nick
  Cc: krzysztof.kozlowski+dt, linux, devicetree, linux-kernel,
	linux-arm-kernel, Rob Herring

On 25/10/2022 15:39, Verdun, Jean-Marie wrote:
> Hi Krzysztof,
> 
> I think what we try to do is to introduce an abstraction layer between the interfaces and the drivers, as our CPLD interfaces are platform dependents. I mean the Power On control could be at address 0x09 on one platform or 0x119 on another one. We would like to find a way to avoid to have to change the driver code, but just feeding the driver with relevant datas, which could be into a platform dependent include file or through the proposed solution that Nick is promoting.
> 
> If the CPLD memory address space was consistent between platform and generation that would be great but unfortunately that is not the case that is why we try to break down the dependency into the driver code and retrieve the data from another place.
> 
> JM
> ________________________________
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, October 25, 2022 12:33 PM
> To: Hawkins, Nick <nick.hawkins@hpe.com>
> Cc: Verdun, Jean-Marie <verdun@hpe.com>; krzysztof.kozlowski+dt@linaro.org <krzysztof.kozlowski+dt@linaro.org>; linux@armlinux.org.uk <linux@armlinux.org.uk>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; Rob Herring <robh+dt@kernel.org>
> Subject: Re: [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg
> 
> On 25/10/2022 15:26, Hawkins, Nick wrote:
>>
>>> I don't know exactly what type of devices you represent in that plreg, but in general the fan device would be the respective plreg. The same with other pieces like hwmon, power supply.
>> We were primarily representing the registers that translate to the CPLD input/outputs from our platforms as well as handling the interrupts associated with those inputs/outputs.
> 
> So basically each register (or set of registers) is a device? How is it
> different than any other multi-functional device? Why do you want to
> model it differently?

How is it different, I am asking?

> 
>> When you say "would be the respective plreg" do you mean that each device/controller would need to perform the actions plreg does individually? In that case how should we get information for that register/memory region and interrupts from the dts? Could it be something like this:
>>
>> plreg: plreg@d1000000 {
>>       compatible = "hpe,gxp-plreg";
>>       reg = <0xd1000000 0xFF>;
>>       interrupts = <26>;
>>       interrupt-parent = <&vic0>;
>> };
>>
>> fanctrl: fanctrl@c1000c00 {
>>       compatible = "hpe,gxp-fan-ctrl";
>>       reg = <0xc1000c00 0x200>;
>>       plreg_handle = <&plreg>;
>> };
>>
> 
> No, rather these are one node.
> 
> You insist to represent this all as programmable logic, but why? CPLD,
> FPGA, ASIC, dedicated IC - all are just implementations and for us
> what's matter are the interfaces, inputs and outputs.


And seriously... this is not a chat. Take a bit of time to answer these
questions instead of replying immediately with a same response as yesterday.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2022-10-25 19:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 18:55 [PATCH v1 0/5] Add PLREG and SPI Driver GXP Support nick.hawkins
2022-10-11 18:55 ` [PATCH v1 1/5] soc: hpe: add support for HPE GXP Programmable Register Driver nick.hawkins
2022-10-11 20:05   ` Krzysztof Kozlowski
2022-10-12 20:25     ` Hawkins, Nick
2022-10-13 12:27       ` Krzysztof Kozlowski
2022-10-11 18:55 ` [PATCH v1 2/5] dt-bindings: soc: hpe: Add hpe,gxp-plreg nick.hawkins
2022-10-11 19:51   ` Krzysztof Kozlowski
2022-10-11 20:27   ` Rob Herring
2022-10-12 19:56     ` Hawkins, Nick
2022-10-22 15:52       ` Krzysztof Kozlowski
2022-10-25  0:03         ` Hawkins, Nick
2022-10-25  0:15           ` Krzysztof Kozlowski
2022-10-25 18:44             ` Hawkins, Nick
2022-10-25 18:55               ` Krzysztof Kozlowski
2022-10-25 19:26                 ` Hawkins, Nick
2022-10-25 19:33                   ` Krzysztof Kozlowski
     [not found]                     ` <DM4PR84MB197662C12018090C312AF72DD6319@DM4PR84MB1976.NAMPRD84.PROD.OUTLOOK.COM>
2022-10-25 19:48                       ` Krzysztof Kozlowski
2022-10-25 19:49                       ` Krzysztof Kozlowski
2022-10-11 18:55 ` [PATCH v1 3/5] ARM: dts: hpe: Add PLREG/SPI Support nick.hawkins
2022-10-11 19:53   ` Krzysztof Kozlowski
2022-10-11 18:55 ` [PATCH v1 4/5] ARM: multi_v7_defconfig: Enable GXP SPI and PLREG Drivers nick.hawkins
2022-10-11 19:54   ` Krzysztof Kozlowski
2022-10-11 18:55 ` [PATCH v1 5/5] MAINTAINERS: Add HPE SOC Drivers nick.hawkins
2022-10-11 20:06   ` Krzysztof Kozlowski

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