[PATCH v1 0/9] ARM: Add GPIO and PSU Support

[PATCH v1 0/9] ARM: Add GPIO and PSU Support

[nick.hawkins]

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

The GXP has multiple interfaces that provide I/O to it. There is GPIO
coming from the host and from the cpld. Both of these interfaces are
interruptable.

The GXP is able to monitor PSU's via I2C. There is support for up to 8
PSUs. The GXP gets presence information from I/O with the cpld.

The fan controller and the psu monitor consume I/O from the CPLD.
Thus for the GXP to be able to report this GPIO to the OpenBMC stack
calls have been enabled for the GPIO driver to use.

Nick Hawkins (9):
  gpio: gxp: Add HPE GXP GPIO
  hwmon: (gxp_fan_ctrl) Give GPIO access to fan data
  hwmon: (gxp-psu) Add driver to read HPE PSUs
  dt-bindings: hwmon: Modify hpe,gxp-fan-ctrl
  dt-bindings: gpio: Add HPE GXP GPIO
  dt-bindings: hwmon: Add HPE GXP PSU Support
  ARM: dts: gxp: add psu, i2c, gpio
  ARM: multi_v7_defconfig: Add PSU, GPIO, and I2C
  MAINTAINERS: hpe: Add GPIO, PSU

 .../bindings/gpio/hpe,gxp-gpio.yaml           |  137 +++
 .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml      |    6 +-
 .../bindings/hwmon/hpe,gxp-psu.yaml           |   45 +
 MAINTAINERS                                   |    4 +
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts      |  161 +++
 arch/arm/boot/dts/hpe-gxp.dtsi                |  197 ++-
 arch/arm/configs/multi_v7_defconfig           |    5 +-
 drivers/gpio/Kconfig                          |    9 +
 drivers/gpio/Makefile                         |    1 +
 drivers/gpio/gpio-gxp.c                       | 1056 +++++++++++++++++
 drivers/hwmon/Kconfig                         |   10 +
 drivers/hwmon/Makefile                        |    1 +
 drivers/hwmon/gxp-fan-ctrl.c                  |   58 +-
 drivers/hwmon/gxp-psu.c                       |  773 ++++++++++++
 14 files changed, 2404 insertions(+), 59 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
 create mode 100644 drivers/gpio/gpio-gxp.c
 create mode 100644 drivers/hwmon/gxp-psu.c

-- 
2.17.1

[PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[nick.hawkins]

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

The GXP SoC supports GPIO on multiple interfaces: Host, CPLD and Soc
pins. The interface from CPLD and Host are interruptable from vic0
and vic1. This driver allows support for both of these interfaces
through the use of different compatible bindings.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/gpio/Kconfig    |    9 +
 drivers/gpio/Makefile   |    1 +
 drivers/gpio/gpio-gxp.c | 1056 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 1066 insertions(+)
 create mode 100644 drivers/gpio/gpio-gxp.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..47435307698c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1235,6 +1235,15 @@ config HTC_EGPIO
 	  several HTC phones.  It provides basic support for input
 	  pins, output pins, and IRQs.
 
+config GPIO_GXP
+        tristate "GXP GPIO support"
+        depends on ARCH_HPE_GXP
+        select GPIOLIB_IRQCHIP
+        help
+	  Say Y here to support GXP GPIO controllers. It provides
+	  support for the multiple GPIO interfaces available to be
+	  available to the Host.
+
 config GPIO_JANZ_TTL
 	tristate "Janz VMOD-TTL Digital IO Module"
 	depends on MFD_JANZ_CMODIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c048ba003367..a7ce0ab097aa 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_GPIO_FTGPIO010)		+= gpio-ftgpio010.o
 obj-$(CONFIG_GPIO_GE_FPGA)		+= gpio-ge.o
 obj-$(CONFIG_GPIO_GPIO_MM)		+= gpio-gpio-mm.o
 obj-$(CONFIG_GPIO_GRGPIO)		+= gpio-grgpio.o
+obj-$(CONFIG_GPIO_GXP)                  += gpio-gxp.o
 obj-$(CONFIG_GPIO_GW_PLD)		+= gpio-gw-pld.o
 obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
 obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
diff --git a/drivers/gpio/gpio-gxp.c b/drivers/gpio/gpio-gxp.c
new file mode 100644
index 000000000000..86f69174434d
--- /dev/null
+++ b/drivers/gpio/gpio-gxp.c
@@ -0,0 +1,1056 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define GPIDATL		0x40
+#define GPIDATH		0x60
+#define GPODATL		0xb0
+#define GPODATH		0xb4
+#define GPODAT2L	0xf8
+#define GPODAT2H	0xfc
+#define GPOOWNL		0x110
+#define GPOOWNH		0x114
+#define GPOOWN2L	0x118
+#define GPOOWN2H	0x11c
+
+#define GPIO_DIR_OUT	0
+#define GPIO_DIR_IN	1
+
+#define PGOOD_MASK		1
+
+#define PLREG_INT_GRP_STAT_MASK	0x8
+#define PLREG_INT_HI_PRI_EN	0xC
+#define PLREG_INT_GRP5_BASE	0x31
+#define PLREG_INT_GRP6_BASE	0x35
+#define PLREG_INT_GRP5_FLAG	0x30
+#define PLREG_INT_GRP6_FLAG	0x34
+#define PLREG_INT_GRP5_PIN_BASE	59
+#define PLREG_INT_GRP6_PIN_BASE	90
+
+enum pl_gpio_pn {
+	IOP_LED1 = 0,
+	IOP_LED2,
+	IOP_LED3,
+	IOP_LED4,
+	IOP_LED5,
+	IOP_LED6,
+	IOP_LED7,
+	IOP_LED8,
+	FAN1_INST = 8,
+	FAN2_INST,
+	FAN3_INST,
+	FAN4_INST,
+	FAN5_INST,
+	FAN6_INST,
+	FAN7_INST,
+	FAN8_INST,
+	FAN1_FAIL,
+	FAN2_FAIL,
+	FAN3_FAIL,
+	FAN4_FAIL,
+	FAN5_FAIL,
+	FAN6_FAIL,
+	FAN7_FAIL,
+	FAN8_FAIL,
+	LED_IDENTIFY = 56,
+	LED_HEALTH_RED,
+	LED_HEALTH_AMBER,
+	PWR_BTN_INT = 59,
+	UID_PRESS_INT,
+	SLP_INT,
+	ACM_FORCE_OFF = 70,
+	ACM_REMOVED,
+	ACM_REQ_N,
+	PSU1_INST,
+	PSU2_INST,
+	PSU3_INST,
+	PSU4_INST,
+	PSU5_INST,
+	PSU6_INST,
+	PSU7_INST,
+	PSU8_INST,
+	PSU1_AC,
+	PSU2_AC,
+	PSU3_AC,
+	PSU4_AC,
+	PSU5_AC,
+	PSU6_AC,
+	PSU7_AC,
+	PSU8_AC,
+	PSU1_DC,
+	PSU2_DC,
+	PSU3_DC,
+	PSU4_DC,
+	PSU5_DC,
+	PSU6_DC,
+	PSU7_DC,
+	PSU8_DC
+};
+
+enum plreg_gpio_pn {
+	RESET = 192,
+	NMI_OUT = 193,
+	VPBTN = 210,
+	PGOOD,
+	PERST,
+	POST_COMPLETE,
+};
+
+struct gxp_gpio_drvdata {
+	struct regmap *csm_map;
+	void __iomem *fn2_vbtn;
+	struct regmap *fn2_stat;
+	struct regmap *vuhc0_map;
+	void __iomem *vbtn;
+	struct regmap *pl_led;
+	struct regmap *pl_health;
+	struct regmap *pl_int;
+	struct gpio_chip chip;
+	int irq;
+};
+
+extern u8 get_psu_inst(void);
+extern u8 get_psu_ac(void);
+extern u8 get_psu_dc(void);
+extern u8 get_fans_installed(void);
+extern u8 get_fans_failed(void);
+
+static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	int ret = 0;
+
+	switch (offset) {
+	case 0 ... 31:
+		regmap_read(drvdata->csm_map, GPIDATL,	&ret);
+		ret = (ret & BIT(offset)) ? 1 : 0;
+		break;
+	case 32 ... 63:
+		regmap_read(drvdata->csm_map, GPIDATH,	&ret);
+		ret = (ret & BIT(offset - 32)) ? 1 : 0;
+		break;
+	case 64 ... 95:
+		regmap_read(drvdata->csm_map, GPODATL,	&ret);
+		ret = (ret & BIT(offset - 64)) ? 1 : 0;
+		break;
+	case 96 ... 127:
+		regmap_read(drvdata->csm_map, GPODATH,	&ret);
+		ret = (ret & BIT(offset - 96)) ? 1 : 0;
+		break;
+	case 128 ...  159:
+		regmap_read(drvdata->csm_map, GPODAT2L,	&ret);
+		ret = (ret & BIT(offset - 128)) ? 1 : 0;
+		break;
+	case 160 ... 191:
+		regmap_read(drvdata->csm_map, GPODAT2H,	&ret);
+		ret = (ret & BIT(offset - 160)) ? 1 : 0;
+		break;
+	case 192:
+		/* SW_RESET */
+		regmap_read(drvdata->csm_map, 0x5C, &ret);
+		ret = (ret & BIT(15)) ? 1 : 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_csm_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	u32 tmp;
+
+	switch (offset) {
+	case 64 ... 95:
+		/* Keep ownership setting */
+		regmap_read(drvdata->csm_map, GPOOWNL, &tmp);
+		tmp = (tmp & BIT(offset - 64)) ? 1 : 0;
+
+		regmap_update_bits(drvdata->csm_map, GPOOWNL,
+				   BIT(offset - 64), BIT(offset - 64));
+		regmap_update_bits(drvdata->csm_map, GPODATL,
+				   BIT(offset - 64), value ? BIT(offset - 64) : 0);
+
+		/* Restore ownership setting */
+		regmap_update_bits(drvdata->csm_map, GPOOWNL,
+				   BIT(offset - 64), tmp ? BIT(offset - 64) : 0);
+		break;
+	case 96 ... 127:
+		/* Keep ownership setting */
+		regmap_read(drvdata->csm_map, GPOOWNH, &tmp);
+		tmp = (tmp & BIT(offset - 96)) ? 1 : 0;
+
+		regmap_update_bits(drvdata->csm_map, GPOOWNH,
+				   BIT(offset - 96), BIT(offset - 96));
+		regmap_update_bits(drvdata->csm_map, GPODATH,
+				   BIT(offset - 96), value ? BIT(offset - 96) : 0);
+
+		/* Restore ownership setting */
+		regmap_update_bits(drvdata->csm_map, GPOOWNH,
+				   BIT(offset - 96), tmp ? BIT(offset - 96) : 0);
+		break;
+	case 128 ... 159:
+		/* Keep ownership setting */
+		regmap_read(drvdata->csm_map, GPOOWN2L, &tmp);
+		tmp = (tmp & BIT(offset - 128)) ? 1 : 0;
+
+		regmap_update_bits(drvdata->csm_map, GPOOWN2L,
+				   BIT(offset - 128), BIT(offset - 128));
+		regmap_update_bits(drvdata->csm_map, GPODAT2L,
+				   BIT(offset - 128), value ? BIT(offset - 128) : 0);
+
+		/* Restore ownership setting */
+		regmap_update_bits(drvdata->csm_map, GPOOWN2L,
+				   BIT(offset - 128), tmp ? BIT(offset - 128) : 0);
+		break;
+	case 160 ... 191:
+		/* Keep ownership setting */
+		regmap_read(drvdata->csm_map, GPOOWN2H,	&tmp);
+		tmp = (tmp & BIT(offset - 160)) ? 1 : 0;
+
+		regmap_update_bits(drvdata->csm_map, GPOOWN2H,
+				   BIT(offset - 160), BIT(offset - 160));
+		regmap_update_bits(drvdata->csm_map, GPODAT2H,
+				   BIT(offset - 160), value ? BIT(offset - 160) : 0);
+
+		/* Restore ownership setting */
+		regmap_update_bits(drvdata->csm_map, GPOOWN2H,
+				   BIT(offset - 160), tmp ? BIT(offset - 160) : 0);
+		break;
+	case 192:
+		if (value) {
+			regmap_update_bits(drvdata->csm_map, 0x5C,
+					   BIT(0), BIT(0));
+			regmap_update_bits(drvdata->csm_map, 0x5C,
+					   BIT(15), BIT(15));
+		} else {
+			regmap_update_bits(drvdata->csm_map, 0x5C,
+					   BIT(15), 0);
+		}
+		break;
+	default:
+		break;
+	}
+}
+
+static int gxp_gpio_csm_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	int ret = 0;
+
+	switch (offset) {
+	case 0 ... 63:
+		ret = GPIO_DIR_IN;
+		break;
+	case 64 ... 191:
+		ret = GPIO_DIR_OUT;
+		break;
+	case 192 ... 193:
+		ret = GPIO_DIR_OUT;
+		break;
+	case 194:
+		ret = GPIO_DIR_IN;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_csm_direction_input(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case 0 ... 63:
+		ret = 0;
+		break;
+	case 194:
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_csm_direction_output(struct gpio_chip *chip,
+					 unsigned int offset, int value)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case 64 ... 191:
+	case 192 ... 193:
+		gxp_gpio_csm_set(chip, offset, value);
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+	int ret = 0;
+
+	if (offset < 8) {
+		regmap_read(drvdata->vuhc0_map, 0x64 + 4 * offset,   &val);
+		ret = (val & BIT(13)) ? 1 : 0;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset,
+			      int value)
+{
+	switch (offset) {
+	default:
+		break;
+	}
+}
+
+static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	int ret = 0;
+
+	switch (offset) {
+	case 0:
+	case 1:
+	case 2:
+		ret = GPIO_DIR_IN;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case 0:
+	case 1:
+	case 2:
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip,
+					  unsigned int offset, int value)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_fn2_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+	int ret = 0;
+
+	switch (offset) {
+	case PGOOD:
+		regmap_read(drvdata->fn2_stat, 0, &val);
+		ret = (val & BIT(24)) ? 1 : 0;
+
+		break;
+	case PERST:
+		regmap_read(drvdata->fn2_stat, 0, &val);
+		ret = (val & BIT(25)) ? 1 : 0;
+
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_fn2_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+	switch (offset) {
+	case VPBTN:
+		writeb(4, drvdata->vbtn);
+		writeb(1, drvdata->fn2_vbtn);
+		break;
+	default:
+		break;
+	}
+}
+
+static int gxp_gpio_fn2_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	int ret = GPIO_DIR_IN;
+
+	switch (offset) {
+	case VPBTN:
+		ret = GPIO_DIR_OUT;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_fn2_direction_input(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case PGOOD:
+	case PERST:
+	case POST_COMPLETE:
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret = 0;
+
+	if (offset < 200)
+		ret = gxp_gpio_csm_get(chip, offset);
+	else if (offset >= 250 && offset < 300)
+		ret = gxp_gpio_vuhc_get(chip, offset - 250);
+	else if (offset >= 300)
+		ret = gxp_gpio_fn2_get(chip, offset);
+
+	return ret;
+}
+
+static void gxp_gpio_set(struct gpio_chip *chip,
+			 unsigned int offset, int value)
+{
+	if (offset < 200)
+		gxp_gpio_csm_set(chip, offset, value);
+	else if (offset >= 250 && offset < 300)
+		gxp_gpio_vuhc_set(chip, offset - 250, value);
+	else if (offset >= 300)
+		gxp_gpio_fn2_set(chip, offset, value);
+}
+
+static int gxp_gpio_get_direction(struct gpio_chip *chip,
+				  unsigned int offset)
+{
+	int ret = 0;
+
+	if (offset < 200)
+		ret = gxp_gpio_csm_get_direction(chip, offset);
+	else if (offset >= 250 && offset < 300)
+		ret = gxp_gpio_vuhc_get_direction(chip, offset - 250);
+	else if (offset >= 300)
+		ret = gxp_gpio_fn2_get_direction(chip, offset);
+
+	return ret;
+}
+
+static int gxp_gpio_direction_input(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	int ret = 0;
+
+	if (offset < 200)
+		ret = gxp_gpio_csm_direction_input(chip, offset);
+	else if (offset >= 250 && offset < 300)
+		ret = gxp_gpio_vuhc_direction_input(chip, offset - 250);
+	else if (offset >= 300)
+		ret = gxp_gpio_fn2_direction_input(chip, offset);
+
+	return ret;
+}
+
+static int gxp_gpio_direction_output(struct gpio_chip *chip,
+				     unsigned int offset, int value)
+{
+	int ret = 0;
+
+	if (offset < 200)
+		ret = gxp_gpio_csm_direction_output(chip, offset, value);
+	else if (offset >= 250 && offset < 300)
+		ret = gxp_gpio_vuhc_direction_output(chip, offset - 250, value);
+	return ret;
+}
+
+static struct regmap *gxp_gpio_init_regmap(struct platform_device *pdev,
+					   char *reg_name, u8 bits)
+{
+	struct regmap_config regmap_config = {
+		.reg_bits = 32,
+		.reg_stride = 4,
+		.val_bits = 32,
+	};
+	void __iomem *base;
+
+	if (bits == 8) {
+		regmap_config.reg_bits = 8;
+		regmap_config.reg_stride = 1;
+		regmap_config.val_bits = 8;
+		regmap_config.max_register = 0xff;
+	}
+
+	base = devm_platform_ioremap_resource_byname(pdev, reg_name);
+	if (IS_ERR(base))
+		return ERR_CAST(base);
+
+	regmap_config.name = reg_name;
+
+	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
+}
+
+static void gxp_gpio_fn2_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+
+	/* Read latched interrupt */
+	regmap_read(drvdata->fn2_stat, 0, &val);
+	/* Clear latched interrupt */
+	regmap_update_bits(drvdata->fn2_stat, 0,
+			   0xFFFF, 0xFFFF);
+}
+
+#define FN2_SEVMASK 4
+static void gxp_gpio_fn2_irq_set_mask(struct irq_data *d, bool set)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+	regmap_update_bits(drvdata->fn2_stat, FN2_SEVMASK,
+			   BIT(0), set ? BIT(0) : 0);
+}
+
+static void gxp_gpio_fn2_irq_mask(struct irq_data *d)
+{
+	gxp_gpio_fn2_irq_set_mask(d, false);
+}
+
+static void gxp_gpio_fn2_irq_unmask(struct irq_data *d)
+{
+	gxp_gpio_fn2_irq_set_mask(d, true);
+}
+
+static int gxp_gpio_fn2_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_gpio_fn2_irq_handle(int irq, void *_drvdata)
+{
+	struct gxp_gpio_drvdata *drvdata = (struct gxp_gpio_drvdata *)_drvdata;
+	unsigned int val, girq;
+
+	regmap_read(drvdata->fn2_stat, 0, &val);
+
+	if (val & PGOOD_MASK) {
+		girq = irq_find_mapping(drvdata->chip.irq.domain, PGOOD);
+		generic_handle_irq(girq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int gxp_gpio_pl_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+	int ret = 0;
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+		regmap_read(drvdata->pl_led, 0x00, &val);
+		ret = (val & BIT(offset)) ? 1 : 0;
+		break;
+	case FAN1_INST ...FAN8_INST:
+		ret = (get_fans_installed() & BIT((offset - FAN1_INST))) ? 1 : 0;
+		break;
+	case FAN1_FAIL ... FAN8_FAIL:
+		ret = (get_fans_failed() & BIT((offset - FAN1_FAIL))) ? 1 : 0;
+		break;
+	case PWR_BTN_INT ... SLP_INT:
+		regmap_read(drvdata->pl_int, PLREG_INT_GRP5_FLAG, &val);
+		/* Active low */
+		ret = (val & BIT((offset - PWR_BTN_INT) + 16)) ? 0 : 1;
+		break;
+	case  PSU1_INST ... PSU8_INST:
+		ret = (get_psu_inst() & BIT((offset - PSU1_INST))) ? 1 : 0;
+		break;
+	case PSU1_AC ... PSU8_AC:
+		ret = (get_psu_ac() & BIT((offset - PSU1_AC))) ? 1 : 0;
+		break;
+	case PSU1_DC ... PSU8_DC:
+		ret = (get_psu_dc() & BIT((offset - PSU1_DC))) ? 1 : 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_pl_set(struct gpio_chip *chip,
+			    unsigned int offset, int value)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+	switch (offset) {
+	case IOP_LED1 ... IOP_LED8:
+		regmap_update_bits(drvdata->pl_led, 0x00, BIT(offset),
+				   value == 0 ? 0 : BIT(offset));
+		break;
+	case LED_IDENTIFY:
+		regmap_update_bits(drvdata->pl_led, 0x01, BIT(7) | BIT(6),
+				   value == 0 ? BIT(7) : BIT(7) | BIT(6));
+		break;
+	case LED_HEALTH_RED:
+		regmap_update_bits(drvdata->pl_health, 0x0, BIT(7),
+				   value == 0 ? 0 : BIT(7));
+		break;
+	case LED_HEALTH_AMBER:
+		regmap_update_bits(drvdata->pl_health, 0x0, BIT(6),
+				   value == 0 ? 0 : BIT(6));
+		break;
+	default:
+		break;
+	}
+}
+
+static int gxp_gpio_pl_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_pl_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	int ret = -EOPNOTSUPP;
+
+	switch (offset) {
+	case 8 ... 55:
+		ret = 0;
+		break;
+	case 59 ... 65:
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int gxp_gpio_pl_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_pl_set(chip, offset, value);
+		ret = 0;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void gxp_gpio_pl_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+	unsigned int val;
+
+	/* Read latched interrupt for group 5 */
+	regmap_read(drvdata->pl_int, PLREG_INT_GRP5_FLAG, &val);
+	/* Clear latched interrupt */
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP5_FLAG,
+			   0xFF, 0xFF);
+
+	/* Read latched interrupt for group 6 */
+	regmap_read(drvdata->pl_int, PLREG_INT_GRP6_FLAG, &val);
+	/* Clear latched interrupt */
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP6_FLAG,
+			   0xFF, 0xFF);
+}
+
+static void gxp_gpio_pl_irq_set_mask(struct irq_data *d, bool set)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP5_BASE,
+			   BIT(0) | BIT(2), set ? 0 : BIT(0) | BIT(2));
+
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP6_BASE,
+			   BIT(2), set ? 0 : BIT(2));
+}
+
+static void gxp_gpio_pl_irq_mask(struct irq_data *d)
+{
+	gxp_gpio_pl_irq_set_mask(d, false);
+}
+
+static void gxp_gpio_pl_irq_unmask(struct irq_data *d)
+{
+	gxp_gpio_pl_irq_set_mask(d, true);
+}
+
+static int gxp_gpio_irq_init_hw(struct gpio_chip *chip)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP5_BASE,
+			   BIT(0) | BIT(2), 0);
+
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP6_BASE,
+			   BIT(2), 0);
+
+	return 0;
+}
+
+static int gxp_gpio_pl_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_gpio_pl_irq_handle(int irq, void *_drvdata)
+{
+	struct gxp_gpio_drvdata *drvdata = (struct gxp_gpio_drvdata *)_drvdata;
+	unsigned int val, girq, i;
+
+	/* Check group 5 interrupts */
+
+	regmap_read(drvdata->pl_int, PLREG_INT_GRP5_FLAG, &val);
+
+	if (val) {
+		for_each_set_bit(i, (unsigned long *)&val, 3) {
+			girq = irq_find_mapping(drvdata->chip.irq.domain,
+						i + PLREG_INT_GRP5_PIN_BASE);
+			generic_handle_irq(girq);
+		}
+
+		/* Clear latched interrupt */
+		regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP5_FLAG,
+				   0xFF, 0xFF);
+		regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP5_BASE,
+				   BIT(0) | BIT(2), 0);
+	}
+
+	/* Check group 6 interrupts */
+
+	regmap_read(drvdata->pl_int, PLREG_INT_GRP6_FLAG, &val);
+
+	if (val) {
+		for_each_set_bit(i, (unsigned long *)&val, 3) {
+			girq = irq_find_mapping(drvdata->chip.irq.domain,
+						i + PLREG_INT_GRP6_PIN_BASE);
+			generic_handle_irq(girq);
+		}
+
+		/* Clear latched interrupt */
+		regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP6_FLAG,
+				   0xFF, 0xFF);
+		regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP6_BASE,
+				   BIT(2), 0);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static struct irq_chip gxp_gpio_irqchip = {
+	.name		= "gxp_fn2",
+	.irq_ack	= gxp_gpio_fn2_irq_ack,
+	.irq_mask	= gxp_gpio_fn2_irq_mask,
+	.irq_unmask	= gxp_gpio_fn2_irq_unmask,
+	.irq_set_type	= gxp_gpio_fn2_set_type,
+};
+
+static struct gpio_chip common_chip = {
+	.label			= "gxp_gpio",
+	.owner                  = THIS_MODULE,
+	.get                    = gxp_gpio_get,
+	.set                    = gxp_gpio_set,
+	.get_direction		= gxp_gpio_get_direction,
+	.direction_input	= gxp_gpio_direction_input,
+	.direction_output	= gxp_gpio_direction_output,
+	.base = 0,
+};
+
+static struct gpio_chip plreg_chip = {
+	.label			= "gxp_gpio_plreg",
+	.owner			= THIS_MODULE,
+	.get			= gxp_gpio_pl_get,
+	.set			= gxp_gpio_pl_set,
+	.get_direction = gxp_gpio_pl_get_direction,
+	.direction_input = gxp_gpio_pl_direction_input,
+	.direction_output = gxp_gpio_pl_direction_output,
+	.base = -1,
+};
+
+static struct irq_chip gxp_plreg_irqchip = {
+	.name		= "gxp_plreg",
+	.irq_ack	= gxp_gpio_pl_irq_ack,
+	.irq_mask	= gxp_gpio_pl_irq_mask,
+	.irq_unmask	= gxp_gpio_pl_irq_unmask,
+	.irq_set_type	= gxp_gpio_pl_set_type,
+};
+
+static int gxp_gpio_init(struct platform_device *pdev)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+	struct gpio_irq_chip *girq;
+	int ret;
+
+	drvdata->csm_map = gxp_gpio_init_regmap(pdev, "csm", 32);
+	if (IS_ERR(drvdata->csm_map))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->csm_map),
+				     "failed to map csm_handle\n");
+
+	drvdata->fn2_vbtn = devm_platform_ioremap_resource_byname(pdev, "fn2-vbtn");
+	if (IS_ERR(drvdata->fn2_vbtn))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->fn2_vbtn),
+				     "failed to map fn2_vbtn\n");
+
+	drvdata->fn2_stat = gxp_gpio_init_regmap(pdev, "fn2-stat", 32);
+	if (IS_ERR(drvdata->fn2_stat))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->fn2_stat),
+				     "failed to map fn2_stat\n");
+
+	drvdata->vuhc0_map = gxp_gpio_init_regmap(pdev, "vuhc", 32);
+	if (IS_ERR(drvdata->vuhc0_map))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->vuhc0_map),
+				     "failed to map vuhc0_map\n");
+
+	drvdata->vbtn = devm_platform_ioremap_resource_byname(pdev, "vbtn");
+	if (IS_ERR(drvdata->vbtn))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->vbtn),
+				     "failed to map vbtn\n");
+	girq = &drvdata->chip.irq;
+	girq->chip = &gxp_gpio_irqchip;
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_edge_irq;
+
+	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_gpio_fn2_irq_handle,
+			       IRQF_SHARED, "gxp-fn2", drvdata);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "IRQ handler failed - %d\n", ret);
+		return ret;
+	}
+	drvdata->chip = common_chip;
+	drvdata->chip.ngpio = 300;
+
+	drvdata->chip.parent = &pdev->dev;
+	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, NULL);
+	if (ret < 0)
+		dev_err(&pdev->dev,
+			"Could not register gpiochip for fn2, %d\n", ret);
+	dev_info(&pdev->dev, "HPE GXP FN2 driver loaded.\n");
+
+	return 0;
+}
+
+static int gxp_gpio_pl_init(struct platform_device *pdev)
+{
+	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+	struct gpio_irq_chip *girq;
+	int ret;
+	unsigned int val;
+
+	drvdata->pl_led = gxp_gpio_init_regmap(pdev, "pl-led", 8);
+	if (IS_ERR(drvdata->pl_led))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
+				     "failed to map pl_led\n");
+
+	drvdata->pl_health = gxp_gpio_init_regmap(pdev, "pl-health", 8);
+	if (IS_ERR(drvdata->pl_health))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
+				     "failed to map pl_health\n");
+
+	drvdata->pl_int = gxp_gpio_init_regmap(pdev, "pl-int", 8);
+	if (IS_ERR(drvdata->pl_int))
+		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
+				     "failed to map pl_int\n");
+
+	drvdata->chip = plreg_chip;
+	drvdata->chip.ngpio = 100;
+	drvdata->chip.parent = &pdev->dev;
+
+	girq = &drvdata->chip.irq;
+	girq->chip = &gxp_plreg_irqchip;
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_edge_irq;
+
+	girq->init_hw = gxp_gpio_irq_init_hw;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, drvdata);
+	if (ret < 0)
+		dev_err(&pdev->dev, "Could not register gpiochip for plreg, %d\n",
+			ret);
+
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_HI_PRI_EN,
+			   BIT(4) | BIT(5), BIT(4) | BIT(5));
+	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK,
+			   BIT(4) | BIT(5), 0x00);
+	regmap_read(drvdata->pl_int, PLREG_INT_HI_PRI_EN, &val);
+	regmap_read(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK, &val);
+
+	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_gpio_pl_irq_handle,
+			       IRQF_SHARED, "gxp-pl", drvdata);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "IRQ handler failed - %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id gxp_gpio_of_match[] = {
+	{ .compatible = "hpe,gxp-gpio"},
+	{ .compatible = "hpe,gxp-gpio-pl"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, gxp_gpio_of_match);
+
+static int gxp_gpio_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct gxp_gpio_drvdata *drvdata;
+	struct device *dev = &pdev->dev;
+	struct device *parent;
+	const struct of_device_id *match = of_match_device(gxp_gpio_of_match, dev);
+
+	if (!match) {
+		dev_err(dev, "device is not compatible");
+		return -EINVAL;
+	}
+
+	parent = dev->parent;
+	if (!parent) {
+		dev_err(dev, "no parent\n");
+		return -ENODEV;
+	}
+
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_gpio_drvdata),
+			       GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, drvdata);
+
+	if (strcmp(match->compatible, "hpe,gxp-gpio-pl") == 0)
+		ret = gxp_gpio_pl_init(pdev);
+	else
+		ret = gxp_gpio_init(pdev);
+
+	return ret;
+}
+
+static struct platform_driver gxp_gpio_driver = {
+	.driver = {
+		.name = "gxp-gpio",
+		.of_match_table = gxp_gpio_of_match,
+	},
+	.probe = gxp_gpio_probe,
+};
+module_platform_driver(gxp_gpio_driver);
+
+MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
+MODULE_DESCRIPTION("GPIO interface for GXP");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP SoC supports GPIO on multiple interfaces: Host, CPLD and Soc
> pins. The interface from CPLD and Host are interruptable from vic0
> and vic1. This driver allows support for both of these interfaces
> through the use of different compatible bindings.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  drivers/gpio/Kconfig    |    9 +
>  drivers/gpio/Makefile   |    1 +
>  drivers/gpio/gpio-gxp.c | 1056 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1066 insertions(+)
>  create mode 100644 drivers/gpio/gpio-gxp.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..47435307698c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1235,6 +1235,15 @@ config HTC_EGPIO
>  	  several HTC phones.  It provides basic support for input
>  	  pins, output pins, and IRQs.
>  
> +config GPIO_GXP
> +        tristate "GXP GPIO support"
> +        depends on ARCH_HPE_GXP

|| COMPILE_TEST

(...)

> +
> +	drvdata->chip.parent = &pdev->dev;
> +	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, NULL);
> +	if (ret < 0)
> +		dev_err(&pdev->dev,
> +			"Could not register gpiochip for fn2, %d\n", ret);
> +	dev_info(&pdev->dev, "HPE GXP FN2 driver loaded.\n");

Drop non-informative probe successes. They are not needed.


> +
> +	return 0;
> +}
> +
> +static int gxp_gpio_pl_init(struct platform_device *pdev)
> +{
> +	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
> +	struct gpio_irq_chip *girq;
> +	int ret;
> +	unsigned int val;
> +
> +	drvdata->pl_led = gxp_gpio_init_regmap(pdev, "pl-led", 8);
> +	if (IS_ERR(drvdata->pl_led))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_led\n");
> +
> +	drvdata->pl_health = gxp_gpio_init_regmap(pdev, "pl-health", 8);
> +	if (IS_ERR(drvdata->pl_health))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_health\n");
> +
> +	drvdata->pl_int = gxp_gpio_init_regmap(pdev, "pl-int", 8);
> +	if (IS_ERR(drvdata->pl_int))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->pl_int),
> +				     "failed to map pl_int\n");
> +
> +	drvdata->chip = plreg_chip;
> +	drvdata->chip.ngpio = 100;
> +	drvdata->chip.parent = &pdev->dev;
> +
> +	girq = &drvdata->chip.irq;
> +	girq->chip = &gxp_plreg_irqchip;
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_edge_irq;
> +
> +	girq->init_hw = gxp_gpio_irq_init_hw;
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, drvdata);
> +	if (ret < 0)
> +		dev_err(&pdev->dev, "Could not register gpiochip for plreg, %d\n",
> +			ret);
> +
> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_HI_PRI_EN,
> +			   BIT(4) | BIT(5), BIT(4) | BIT(5));
> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK,
> +			   BIT(4) | BIT(5), 0x00);
> +	regmap_read(drvdata->pl_int, PLREG_INT_HI_PRI_EN, &val);
> +	regmap_read(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK, &val);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);
> +		return ret;

return dev_err_probe

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

return dev_err_probe
Actually other applicable places as well...

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gxp_gpio_of_match[] = {
> +	{ .compatible = "hpe,gxp-gpio"},
> +	{ .compatible = "hpe,gxp-gpio-pl"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, gxp_gpio_of_match);
> +
> +static int gxp_gpio_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct gxp_gpio_drvdata *drvdata;
> +	struct device *dev = &pdev->dev;
> +	struct device *parent;
> +	const struct of_device_id *match = of_match_device(gxp_gpio_of_match, dev);
> +
> +	if (!match) {
> +		dev_err(dev, "device is not compatible");

It is not possible.

> +		return -EINVAL;
> +	}
> +
> +	parent = dev->parent;
> +	if (!parent) {
> +		dev_err(dev, "no parent\n");

Why do you need this check?

> +		return -ENODEV;
> +	}
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_gpio_drvdata),

sizeof(*)

> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	if (strcmp(match->compatible, "hpe,gxp-gpio-pl") == 0)

No, use match data. Anyway this is open-coding of OF functions...

> +		ret = gxp_gpio_pl_init(pdev);
> +	else
> +		ret = gxp_gpio_init(pdev);
> +
> +	return ret;

Best regards,
Krzysztof

Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Guenter Roeck]

On 4/18/23 08:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP SoC supports GPIO on multiple interfaces: Host, CPLD and Soc
> pins. The interface from CPLD and Host are interruptable from vic0
> and vic1. This driver allows support for both of these interfaces
> through the use of different compatible bindings.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>   drivers/gpio/Kconfig    |    9 +
>   drivers/gpio/Makefile   |    1 +
>   drivers/gpio/gpio-gxp.c | 1056 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 1066 insertions(+)
>   create mode 100644 drivers/gpio/gpio-gxp.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..47435307698c 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1235,6 +1235,15 @@ config HTC_EGPIO
>   	  several HTC phones.  It provides basic support for input
>   	  pins, output pins, and IRQs.
>   
> +config GPIO_GXP
> +        tristate "GXP GPIO support"
> +        depends on ARCH_HPE_GXP
> +        select GPIOLIB_IRQCHIP
> +        help
> +	  Say Y here to support GXP GPIO controllers. It provides
> +	  support for the multiple GPIO interfaces available to be
> +	  available to the Host.
> +
>   config GPIO_JANZ_TTL
>   	tristate "Janz VMOD-TTL Digital IO Module"
>   	depends on MFD_JANZ_CMODIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c048ba003367..a7ce0ab097aa 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_GPIO_FTGPIO010)		+= gpio-ftgpio010.o
>   obj-$(CONFIG_GPIO_GE_FPGA)		+= gpio-ge.o
>   obj-$(CONFIG_GPIO_GPIO_MM)		+= gpio-gpio-mm.o
>   obj-$(CONFIG_GPIO_GRGPIO)		+= gpio-grgpio.o
> +obj-$(CONFIG_GPIO_GXP)                  += gpio-gxp.o
>   obj-$(CONFIG_GPIO_GW_PLD)		+= gpio-gw-pld.o
>   obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
>   obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
> diff --git a/drivers/gpio/gpio-gxp.c b/drivers/gpio/gpio-gxp.c
> new file mode 100644
> index 000000000000..86f69174434d
> --- /dev/null
> +++ b/drivers/gpio/gpio-gxp.c
> @@ -0,0 +1,1056 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
> +
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define GPIDATL		0x40
> +#define GPIDATH		0x60
> +#define GPODATL		0xb0
> +#define GPODATH		0xb4
> +#define GPODAT2L	0xf8
> +#define GPODAT2H	0xfc
> +#define GPOOWNL		0x110
> +#define GPOOWNH		0x114
> +#define GPOOWN2L	0x118
> +#define GPOOWN2H	0x11c
> +
> +#define GPIO_DIR_OUT	0
> +#define GPIO_DIR_IN	1
> +
> +#define PGOOD_MASK		1
> +
> +#define PLREG_INT_GRP_STAT_MASK	0x8
> +#define PLREG_INT_HI_PRI_EN	0xC
> +#define PLREG_INT_GRP5_BASE	0x31
> +#define PLREG_INT_GRP6_BASE	0x35
> +#define PLREG_INT_GRP5_FLAG	0x30
> +#define PLREG_INT_GRP6_FLAG	0x34
> +#define PLREG_INT_GRP5_PIN_BASE	59
> +#define PLREG_INT_GRP6_PIN_BASE	90
> +
> +enum pl_gpio_pn {
> +	IOP_LED1 = 0,
> +	IOP_LED2,
> +	IOP_LED3,
> +	IOP_LED4,
> +	IOP_LED5,
> +	IOP_LED6,
> +	IOP_LED7,
> +	IOP_LED8,
> +	FAN1_INST = 8,
> +	FAN2_INST,
> +	FAN3_INST,
> +	FAN4_INST,
> +	FAN5_INST,
> +	FAN6_INST,
> +	FAN7_INST,
> +	FAN8_INST,
> +	FAN1_FAIL,
> +	FAN2_FAIL,
> +	FAN3_FAIL,
> +	FAN4_FAIL,
> +	FAN5_FAIL,
> +	FAN6_FAIL,
> +	FAN7_FAIL,
> +	FAN8_FAIL,
> +	LED_IDENTIFY = 56,
> +	LED_HEALTH_RED,
> +	LED_HEALTH_AMBER,
> +	PWR_BTN_INT = 59,
> +	UID_PRESS_INT,
> +	SLP_INT,
> +	ACM_FORCE_OFF = 70,
> +	ACM_REMOVED,
> +	ACM_REQ_N,
> +	PSU1_INST,
> +	PSU2_INST,
> +	PSU3_INST,
> +	PSU4_INST,
> +	PSU5_INST,
> +	PSU6_INST,
> +	PSU7_INST,
> +	PSU8_INST,
> +	PSU1_AC,
> +	PSU2_AC,
> +	PSU3_AC,
> +	PSU4_AC,
> +	PSU5_AC,
> +	PSU6_AC,
> +	PSU7_AC,
> +	PSU8_AC,
> +	PSU1_DC,
> +	PSU2_DC,
> +	PSU3_DC,
> +	PSU4_DC,
> +	PSU5_DC,
> +	PSU6_DC,
> +	PSU7_DC,
> +	PSU8_DC
> +};
> +
> +enum plreg_gpio_pn {
> +	RESET = 192,
> +	NMI_OUT = 193,
> +	VPBTN = 210,
> +	PGOOD,
> +	PERST,
> +	POST_COMPLETE,
> +};
> +
> +struct gxp_gpio_drvdata {
> +	struct regmap *csm_map;
> +	void __iomem *fn2_vbtn;
> +	struct regmap *fn2_stat;
> +	struct regmap *vuhc0_map;
> +	void __iomem *vbtn;
> +	struct regmap *pl_led;
> +	struct regmap *pl_health;
> +	struct regmap *pl_int;
> +	struct gpio_chip chip;
> +	int irq;
> +};
> +
> +extern u8 get_psu_inst(void);
> +extern u8 get_psu_ac(void);
> +extern u8 get_psu_dc(void);
> +extern u8 get_fans_installed(void);
> +extern u8 get_fans_failed(void);
> +

This is not information which should be reported through a gpio driver.
Besides, the functions don't exist at this point in the series,
and there should be no extern declarations in source files.

If you want to model fan or psu information through gpio, drop
the hwmon drivers and implement reading the status here, then use
the existing gpio-fan hwmon driver to report it in the hwmon subsystem.

Guenter

RE: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Hawkins, Nick]

> This is not information which should be reported through a gpio driver.
> Besides, the functions don't exist at this point in the series,
> and there should be no extern declarations in source files.

> If you want to model fan or psu information through gpio, drop
> the hwmon drivers and implement reading the status here, then use
> the existing gpio-fan hwmon driver to report it in the hwmon subsystem.

Thank you for the feedback Guenter,

I see how it is possible to use gpio-fan for the fan. As for the gxp-psu
Hwmon driver can I model the gpio-fan driver to get the necessary
gpio information for power supplies?

-Nick Hawkins

Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Guenter Roeck]

On 4/27/23 07:53, Hawkins, Nick wrote:
> 
> 
> 
>> This is not information which should be reported through a gpio driver.
>> Besides, the functions don't exist at this point in the series,
>> and there should be no extern declarations in source files.
> 
>> If you want to model fan or psu information through gpio, drop
>> the hwmon drivers and implement reading the status here, then use
>> the existing gpio-fan hwmon driver to report it in the hwmon subsystem.
> 
> Thank you for the feedback Guenter,
> 
> I see how it is possible to use gpio-fan for the fan. As for the gxp-psu
> Hwmon driver can I model the gpio-fan driver to get the necessary
> gpio information for power supplies?
> 

Sorry, I don't understand. Looking into the code again, the major problem
I see is that you want to model fan install status and fan fault
status as gpio pins. The same is true for psu information (installed,
ac, dc flags).

If you want to do this, fine, but then get the status from the gpio
driver and don't export anything to the gpio driver. The kernel supports
means to do that (look at gpiod_get and similar functions). It makes the
code more complex, but I assume you know what you are doing.

Guenter

RE: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Hawkins, Nick]

> Sorry, I don't understand. Looking into the code again, the major problem
> I see is that you want to model fan install status and fan fault
> status as gpio pins. The same is true for psu information (installed,
> ac, dc flags).

> If you want to do this, fine, but then get the status from the gpio
> driver and don't export anything to the gpio driver. The kernel supports
> means to do that (look at gpiod_get and similar functions). It makes the
> code more complex, but I assume you know what you are doing.

Greetings Guenter and Andy,

I have pursued this approach and I have found that while the PSU and
FAN drivers can consume the presence and fail info from the GPIO
driver, the host is unable to read the read only GPIOs.
In OpenBMC we have a GPIO consumer that sits and waits for
GPIOs changes then takes action on it. To resolve this issue would
it be acceptable for the GPIO driver to poll the relevant GPIOs and
share the necessary fan presence/failure and psu presence/failure
information via a global shared variable? This would be the alternative
to the drivers consuming GPIOs.

Thanks you for your time and assistance,

-Nick Hawkins

Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[andy.shevchenko]

Tue, Apr 18, 2023 at 10:28:16AM -0500, nick.hawkins@hpe.com kirjoitti:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP SoC supports GPIO on multiple interfaces: Host, CPLD and Soc
> pins. The interface from CPLD and Host are interruptable from vic0
> and vic1. This driver allows support for both of these interfaces
> through the use of different compatible bindings.

Thank you for your contribution. To begin with, I don't believe a simple GPIO
driver needs 1000+ LoCs. But see more comments below.

...

> +config GPIO_GXP
> +        tristate "GXP GPIO support"
> +        depends on ARCH_HPE_GXP
> +        select GPIOLIB_IRQCHIP
> +        help
> +	  Say Y here to support GXP GPIO controllers. It provides
> +	  support for the multiple GPIO interfaces available to be
> +	  available to the Host.

The indentation is rather problematic. Had you run checkpatch.pl?

...

> +#include <linux/gpio.h>

No, new code may not include this header.

...

> +#include <linux/of.h>
> +#include <linux/of_device.h>

Why? I haven't seen much evidence of needing of these. What you need are
rather mod_devicetable.h and property.h (see also below).

...

> +#define GPIDATL		0x40
> +#define GPIDATH		0x60
> +#define GPODATL		0xb0
> +#define GPODATH		0xb4
> +#define GPODAT2L	0xf8
> +#define GPODAT2H	0xfc

Use same size of the values, e.g. 0x0fc

> +#define GPOOWNL		0x110
> +#define GPOOWNH		0x114
> +#define GPOOWN2L	0x118
> +#define GPOOWN2H	0x11c

To me this sounds like a 2 8-lines bank GPIO array. Drop those H and L suffixes
and use proper offset calculation based on the bank number. There are plenty of
existing GPIO drivers that do that way.

Also why gpio-regmap is not in use?

...

> +#define GPIO_DIR_OUT	0
> +#define GPIO_DIR_IN	1

Oh, what is this?! You must not interfere with the GPIO_ namespace.

...

> +#define PGOOD_MASK		1

For mask use BIT() / GENMASK()

> +#define PLREG_INT_GRP_STAT_MASK	0x8

Ditto.

...

> +#define PLREG_INT_HI_PRI_EN	0xC

Is it register offset or value?

> +#define PLREG_INT_GRP5_BASE	0x31
> +#define PLREG_INT_GRP6_BASE	0x35
> +#define PLREG_INT_GRP5_FLAG	0x30
> +#define PLREG_INT_GRP6_FLAG	0x34

These need more expanation what they are for and why these specific values are
being used.

...


> +#define PLREG_INT_GRP5_PIN_BASE	59
> +#define PLREG_INT_GRP6_PIN_BASE	90

What are these for?

...

> +enum plreg_gpio_pn {
> +	RESET = 192,
> +	NMI_OUT = 193,
> +	VPBTN = 210,
> +	PGOOD,
> +	PERST,
> +	POST_COMPLETE,

Why these numbers? If it comes from hardware specification, make _all_ them
explicitly assigned.

> +};

...

> +struct gxp_gpio_drvdata {
> +	struct regmap *csm_map;
> +	void __iomem *fn2_vbtn;
> +	struct regmap *fn2_stat;
> +	struct regmap *vuhc0_map;
> +	void __iomem *vbtn;
> +	struct regmap *pl_led;
> +	struct regmap *pl_health;
> +	struct regmap *pl_int;

> +	struct gpio_chip chip;

Move it to be the first member in the structure. Might save a few bytes in the
code. 

> +	int irq;
> +};

...

> +static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> +	int ret = 0;

> +	switch (offset) {
> +	case 0 ... 31:
> +		regmap_read(drvdata->csm_map, GPIDATL,	&ret);
> +		ret = (ret & BIT(offset)) ? 1 : 0;
> +		break;
> +	case 32 ... 63:
> +		regmap_read(drvdata->csm_map, GPIDATH,	&ret);
> +		ret = (ret & BIT(offset - 32)) ? 1 : 0;
> +		break;
> +	case 64 ... 95:
> +		regmap_read(drvdata->csm_map, GPODATL,	&ret);
> +		ret = (ret & BIT(offset - 64)) ? 1 : 0;
> +		break;
> +	case 96 ... 127:
> +		regmap_read(drvdata->csm_map, GPODATH,	&ret);
> +		ret = (ret & BIT(offset - 96)) ? 1 : 0;
> +		break;
> +	case 128 ...  159:
> +		regmap_read(drvdata->csm_map, GPODAT2L,	&ret);
> +		ret = (ret & BIT(offset - 128)) ? 1 : 0;
> +		break;
> +	case 160 ... 191:
> +		regmap_read(drvdata->csm_map, GPODAT2H,	&ret);
> +		ret = (ret & BIT(offset - 160)) ? 1 : 0;
> +		break;
> +	case 192:
> +		/* SW_RESET */
> +		regmap_read(drvdata->csm_map, 0x5C, &ret);
> +		ret = (ret & BIT(15)) ? 1 : 0;
> +		break;

Besides redundant " ? 1 : 0" parts, see what I wrote above and calculate offset
properly and then apply. Ditto for other functions.

> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}

I stopped here, this driver requires a lot more work to be done before
considering for upstream, sorry.

...

> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_edge_irq;

Should be handle_bad_irq() by default.

...

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

Huh?! No bailing out?

...

> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_HI_PRI_EN,
> +			   BIT(4) | BIT(5), BIT(4) | BIT(5));

GENMASK(), but do it as a defined value with meaningful name.

> +	regmap_update_bits(drvdata->pl_int, PLREG_INT_GRP_STAT_MASK,
> +			   BIT(4) | BIT(5), 0x00);

Ditto.

...

> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {

> +		dev_err(&pdev->dev, "Get irq from platform fail - %d\n", ret);

No. The message is already printed by platform code. You are not supposed to
pollute logs with duplicative noise.

> +		return ret;
> +	}

...

> +static const struct of_device_id gxp_gpio_of_match[] = {
> +	{ .compatible = "hpe,gxp-gpio"},
> +	{ .compatible = "hpe,gxp-gpio-pl"},

Missing spaces in above two lines.

> +	{}
> +};

...

> +	const struct of_device_id *match = of_match_device(gxp_gpio_of_match, dev);

device_get_match_data()

...

> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_gpio_drvdata),
> +			       GFP_KERNEL);

sizeof(*drvdata) and make it one line.

> +	if (!drvdata)
> +		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Hawkins, Nick]

> Thank you for your contribution. To begin with, I don't believe a simple GPIO
> driver needs 1000+ LoCs. But see more comments below.

Andy,

Thank you for your feedback. I will apply all the input you have provided.

I will need to rewrite this code and I am considering the need to
perhaps create two files instead of one to keep code length down. As
implied by the description I was trying to have one file handle two
different compatible strings.

I believe that one file will need to be the regular IO from the host and
memory mapped IO pins of our SoC. The other will need to be the memory
mapped IO pins coming from our CPLD. Both of these sources are interruptible
which does cause some complexity.

Please let me know if what I have described above is not a good approach to
take with GPIO drivers. Any guidance would be greatly appreciated.

Best Regards,
-Nick Hawkins

Re: [PATCH v1 1/9] gpio: gxp: Add HPE GXP GPIO

[Andy Shevchenko]

On Thu, Apr 27, 2023 at 10:01 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > Thank you for your contribution. To begin with, I don't believe a simple GPIO
> > driver needs 1000+ LoCs. But see more comments below.
>
> Andy,
>
> Thank you for your feedback. I will apply all the input you have provided.

You are welcome!

> I will need to rewrite this code and I am considering the need to
> perhaps create two files instead of one to keep code length down. As
> implied by the description I was trying to have one file handle two
> different compatible strings.
>
> I believe that one file will need to be the regular IO from the host and
> memory mapped IO pins of our SoC. The other will need to be the memory
> mapped IO pins coming from our CPLD. Both of these sources are interruptible
> which does cause some complexity.
>
> Please let me know if what I have described above is not a good approach to
> take with GPIO drivers. Any guidance would be greatly appreciated.

I don't know your hardware, otherwise what you wrote above sounds good to me.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data

[nick.hawkins]

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

The fan driver has access to the IO that reports fan presence.
Add the ability for other drivers such as GPIO to call in
to get fan information. Also remove the system power check as
the GPIO driver needs it to report power state.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/hwmon/gxp-fan-ctrl.c | 58 +++++++++++++++---------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
index 0014b8b0fd41..a8fcea98cc55 100644
--- a/drivers/hwmon/gxp-fan-ctrl.c
+++ b/drivers/hwmon/gxp-fan-ctrl.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
 
 #include <linux/bits.h>
 #include <linux/err.h>
@@ -11,15 +11,14 @@
 
 #define OFS_FAN_INST	0 /* Is 0 because plreg base will be set at INST */
 #define OFS_FAN_FAIL	2 /* Is 2 bytes after base */
-#define OFS_SEVSTAT	0 /* Is 0 because fn2 base will be set at SEVSTAT */
-#define POWER_BIT	24
 
 struct gxp_fan_ctrl_drvdata {
 	void __iomem	*base;
 	void __iomem	*plreg;
-	void __iomem	*fn2;
 };
 
+struct gxp_fan_ctrl_drvdata *drvdata;
+
 static bool fan_installed(struct device *dev, int fan)
 {
 	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
@@ -30,6 +29,16 @@ static bool fan_installed(struct device *dev, int fan)
 	return !!(val & BIT(fan));
 }
 
+u8 get_fans_installed(void)
+{
+	static u8 val;
+
+	val = readb(drvdata->plreg + OFS_FAN_INST);
+
+	return val;
+}
+EXPORT_SYMBOL(get_fans_installed);
+
 static long fan_failed(struct device *dev, int fan)
 {
 	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
@@ -40,19 +49,19 @@ static long fan_failed(struct device *dev, int fan)
 	return !!(val & BIT(fan));
 }
 
-static long fan_enabled(struct device *dev, int fan)
+u8 get_fans_failed(void)
 {
-	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
-	u32 val;
+	static u8 val;
 
-	/*
-	 * Check the power status as if the platform is off the value
-	 * reported for the PWM will be incorrect. Report fan as
-	 * disabled.
-	 */
-	val = readl(drvdata->fn2 + OFS_SEVSTAT);
+	val = readb(drvdata->plreg + OFS_FAN_FAIL);
+
+	return val;
+}
+EXPORT_SYMBOL(get_fans_failed);
 
-	return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan));
+static long fan_enabled(struct device *dev, int fan)
+{
+	return !!(fan_installed(dev, fan));
 }
 
 static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
@@ -98,20 +107,8 @@ static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
 static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
-	u32 reg;
-
-	/*
-	 * Check the power status of the platform. If the platform is off
-	 * the value reported for the PWM will be incorrect. In this case
-	 * report a PWM of zero.
-	 */
 
-	reg = readl(drvdata->fn2 + OFS_SEVSTAT);
-
-	if (reg & BIT(POWER_BIT))
-		*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
-	else
-		*val = 0;
+	*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
 
 	return 0;
 }
@@ -198,7 +195,6 @@ static const struct hwmon_chip_info gxp_fan_ctrl_chip_info = {
 
 static int gxp_fan_ctrl_probe(struct platform_device *pdev)
 {
-	struct gxp_fan_ctrl_drvdata *drvdata;
 	struct device *dev = &pdev->dev;
 	struct device *hwmon_dev;
 
@@ -218,12 +214,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(drvdata->plreg),
 				     "failed to map plreg\n");
 
-	drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev,
-							     "fn2");
-	if (IS_ERR(drvdata->fn2))
-		return dev_err_probe(dev, PTR_ERR(drvdata->fn2),
-				     "failed to map fn2\n");
-
 	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
 							 "hpe_gxp_fan_ctrl",
 							 drvdata,
-- 
2.17.1

Re: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data

[Guenter Roeck]

On 4/18/23 08:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The fan driver has access to the IO that reports fan presence.
> Add the ability for other drivers such as GPIO to call in
> to get fan information. Also remove the system power check as
> the GPIO driver needs it to report power state.
> 

Sorry, I am lost right here. Why would "gpio" want to know about
or report fan information ?

Guenter

> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>   drivers/hwmon/gxp-fan-ctrl.c | 58 +++++++++++++++---------------------
>   1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
> index 0014b8b0fd41..a8fcea98cc55 100644
> --- a/drivers/hwmon/gxp-fan-ctrl.c
> +++ b/drivers/hwmon/gxp-fan-ctrl.c
> @@ -1,5 +1,5 @@
>   // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
> +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
>   
>   #include <linux/bits.h>
>   #include <linux/err.h>
> @@ -11,15 +11,14 @@
>   
>   #define OFS_FAN_INST	0 /* Is 0 because plreg base will be set at INST */
>   #define OFS_FAN_FAIL	2 /* Is 2 bytes after base */
> -#define OFS_SEVSTAT	0 /* Is 0 because fn2 base will be set at SEVSTAT */
> -#define POWER_BIT	24
>   
>   struct gxp_fan_ctrl_drvdata {
>   	void __iomem	*base;
>   	void __iomem	*plreg;
> -	void __iomem	*fn2;
>   };
>   
> +struct gxp_fan_ctrl_drvdata *drvdata;
> +
>   static bool fan_installed(struct device *dev, int fan)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> @@ -30,6 +29,16 @@ static bool fan_installed(struct device *dev, int fan)
>   	return !!(val & BIT(fan));
>   }
>   
> +u8 get_fans_installed(void)
> +{
> +	static u8 val;
> +
> +	val = readb(drvdata->plreg + OFS_FAN_INST);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(get_fans_installed);
> +
>   static long fan_failed(struct device *dev, int fan)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> @@ -40,19 +49,19 @@ static long fan_failed(struct device *dev, int fan)
>   	return !!(val & BIT(fan));
>   }
>   
> -static long fan_enabled(struct device *dev, int fan)
> +u8 get_fans_failed(void)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u32 val;
> +	static u8 val;
>   
> -	/*
> -	 * Check the power status as if the platform is off the value
> -	 * reported for the PWM will be incorrect. Report fan as
> -	 * disabled.
> -	 */
> -	val = readl(drvdata->fn2 + OFS_SEVSTAT);
> +	val = readb(drvdata->plreg + OFS_FAN_FAIL);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(get_fans_failed);
>   
> -	return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan));
> +static long fan_enabled(struct device *dev, int fan)
> +{
> +	return !!(fan_installed(dev, fan));
>   }
>   
>   static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
> @@ -98,20 +107,8 @@ static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
>   static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u32 reg;
> -
> -	/*
> -	 * Check the power status of the platform. If the platform is off
> -	 * the value reported for the PWM will be incorrect. In this case
> -	 * report a PWM of zero.
> -	 */
>   
> -	reg = readl(drvdata->fn2 + OFS_SEVSTAT);
> -
> -	if (reg & BIT(POWER_BIT))
> -		*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
> -	else
> -		*val = 0;
> +	*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
>   
>   	return 0;
>   }
> @@ -198,7 +195,6 @@ static const struct hwmon_chip_info gxp_fan_ctrl_chip_info = {
>   
>   static int gxp_fan_ctrl_probe(struct platform_device *pdev)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata;
>   	struct device *dev = &pdev->dev;
>   	struct device *hwmon_dev;
>   
> @@ -218,12 +214,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, PTR_ERR(drvdata->plreg),
>   				     "failed to map plreg\n");
>   
> -	drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev,
> -							     "fn2");
> -	if (IS_ERR(drvdata->fn2))
> -		return dev_err_probe(dev, PTR_ERR(drvdata->fn2),
> -				     "failed to map fn2\n");
> -
>   	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
>   							 "hpe_gxp_fan_ctrl",
>   							 drvdata,

Re: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.3-rc7]
[cannot apply to brgl/gpio/for-next next-20230417]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230418152824.110823-3-nick.hawkins%40hpe.com
patch subject: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230419/202304190119.scX6TsQH-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/be3722d7f32fea1ea375090d05cbfdd3dd4e04d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
        git checkout be3722d7f32fea1ea375090d05cbfdd3dd4e04d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304190119.scX6TsQH-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/gxp-fan-ctrl.c:32:4: warning: no previous prototype for 'get_fans_installed' [-Wmissing-prototypes]
      32 | u8 get_fans_installed(void)
         |    ^~~~~~~~~~~~~~~~~~
>> drivers/hwmon/gxp-fan-ctrl.c:52:4: warning: no previous prototype for 'get_fans_failed' [-Wmissing-prototypes]
      52 | u8 get_fans_failed(void)
         |    ^~~~~~~~~~~~~~~


vim +/get_fans_installed +32 drivers/hwmon/gxp-fan-ctrl.c

    31	
  > 32	u8 get_fans_installed(void)
    33	{
    34		static u8 val;
    35	
    36		val = readb(drvdata->plreg + OFS_FAN_INST);
    37	
    38		return val;
    39	}
    40	EXPORT_SYMBOL(get_fans_installed);
    41	
    42	static long fan_failed(struct device *dev, int fan)
    43	{
    44		struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
    45		u8 val;
    46	
    47		val = readb(drvdata->plreg + OFS_FAN_FAIL);
    48	
    49		return !!(val & BIT(fan));
    50	}
    51	
  > 52	u8 get_fans_failed(void)
    53	{
    54		static u8 val;
    55	
    56		val = readb(drvdata->plreg + OFS_FAN_FAIL);
    57	
    58		return val;
    59	}
    60	EXPORT_SYMBOL(get_fans_failed);
    61	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.3-rc7]
[cannot apply to brgl/gpio/for-next next-20230418]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230418152824.110823-3-nick.hawkins%40hpe.com
patch subject: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data
config: hexagon-randconfig-r045-20230419 (https://download.01.org/0day-ci/archive/20230419/202304192243.9hwJ1Cad-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 437b7602e4a998220871de78afcb020b9c14a661)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/be3722d7f32fea1ea375090d05cbfdd3dd4e04d3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
        git checkout be3722d7f32fea1ea375090d05cbfdd3dd4e04d3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash block/partitions/ drivers/hwmon/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304192243.9hwJ1Cad-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/hwmon/gxp-fan-ctrl.c:7:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/hwmon/gxp-fan-ctrl.c:7:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/hwmon/gxp-fan-ctrl.c:7:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/hwmon/gxp-fan-ctrl.c:32:4: warning: no previous prototype for function 'get_fans_installed' [-Wmissing-prototypes]
   u8 get_fans_installed(void)
      ^
   drivers/hwmon/gxp-fan-ctrl.c:32:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u8 get_fans_installed(void)
   ^
   static 
>> drivers/hwmon/gxp-fan-ctrl.c:52:4: warning: no previous prototype for function 'get_fans_failed' [-Wmissing-prototypes]
   u8 get_fans_failed(void)
      ^
   drivers/hwmon/gxp-fan-ctrl.c:52:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u8 get_fans_failed(void)
   ^
   static 
   8 warnings generated.


vim +/get_fans_installed +32 drivers/hwmon/gxp-fan-ctrl.c

    31	
  > 32	u8 get_fans_installed(void)
    33	{
    34		static u8 val;
    35	
    36		val = readb(drvdata->plreg + OFS_FAN_INST);
    37	
    38		return val;
    39	}
    40	EXPORT_SYMBOL(get_fans_installed);
    41	
    42	static long fan_failed(struct device *dev, int fan)
    43	{
    44		struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
    45		u8 val;
    46	
    47		val = readb(drvdata->plreg + OFS_FAN_FAIL);
    48	
    49		return !!(val & BIT(fan));
    50	}
    51	
  > 52	u8 get_fans_failed(void)
    53	{
    54		static u8 val;
    55	
    56		val = readb(drvdata->plreg + OFS_FAN_FAIL);
    57	
    58		return val;
    59	}
    60	EXPORT_SYMBOL(get_fans_failed);
    61	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v1 3/9] hwmon: (gxp-psu) Add driver to read HPE PSUs

[nick.hawkins]

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

The GXP SoC can support up to 8 power supplies that it can communicate
with via i2c. The GXP PSU driver will provide a method for the GPIO
driver with info it reads to be available to the host.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 drivers/hwmon/Kconfig   |  10 +
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/gxp-psu.c | 773 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 784 insertions(+)
 create mode 100644 drivers/hwmon/gxp-psu.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..3b56690ea089 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -723,6 +723,16 @@ config SENSORS_GXP_FAN_CTRL
 	  The GXP controls fan function via the CPLD through the use of PWM
 	  registers. This driver reports status and pwm setting of the fans.
 
+config SENSORS_GXP_PSU
+	tristate "HPE GXP PSU driver"
+	depends on ARCH_HPE_GXP || COMPILE_TEST
+	depends on I2C
+	help
+	  If you say yes you will get support for GXP psu driver support. The GXP
+	  gets PSU presence and state information from the CPLD. The GXP gets PSU
+	  data via i2c. It provides a method for other drivers to call into get
+	  PSU presence information.
+
 config SENSORS_HIH6130
 	tristate "Honeywell Humidicon HIH-6130 humidity/temperature sensor"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 88712b5031c8..6c84cd52d0d3 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
 obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
+obj-$(CONFIG_SENSORS_GXP_PSU)	+= gxp-psu.o
 obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
 obj-$(CONFIG_SENSORS_I5500)	+= i5500_temp.o
diff --git a/drivers/hwmon/gxp-psu.c b/drivers/hwmon/gxp-psu.c
new file mode 100644
index 000000000000..e4217200c34b
--- /dev/null
+++ b/drivers/hwmon/gxp-psu.c
@@ -0,0 +1,773 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Enterpise Development Company, L.P. */
+
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#define READ_REG_CMD		0x00
+#define READ_FRU_CMD		0x22
+#define REG_IN_VOL		0x10
+#define REG_IN_CUR		0x11
+#define REG_IN_PWR		0x12
+#define REG_OUT_VOL		0x20
+#define REG_OUT_CUR		0x21
+#define REG_OUT_PWR		0x22
+#define REG_FAN_SPEED		0x40
+#define REG_INLET_TEMP		0x42
+#define MAX_PSU			0x08
+#define INPUT_CHAN 0
+
+#define L_IN_POWER		"pin1"
+#define L_OUT_POWER		"pout1"
+#define L_IN_IN			"vin"
+#define L_OUT_IN		"vout1"
+#define L_FAN			"fan1"
+#define L_IN_CURR		"iin"
+#define L_OUT_CURR		"iout1"
+#define L_TEMP			"temp1"
+
+static void __iomem *pl_psu;
+
+struct gxp_psu_drvdata {
+	struct i2c_client *client;
+	u16 input_power;
+	u16 input_voltage;
+	u16 input_current;
+	u16 output_power;
+	u16 output_voltage;
+	u16 output_current;
+	s16 inlet_temp;
+	u16 fan_speed;
+	u8 id;
+	struct dentry *debugfs;
+	u8 spare_part[10];
+	u8 product_name[26];
+	u8 serial_number[14];
+	u8 product_manufacturer[3];
+	bool present; /* psu can be physically removed */
+	struct mutex update_lock;
+	struct device *hwmon_dev;
+};
+
+static u8 psucount;
+struct gxp_psu_drvdata *psus[MAX_PSU] = { NULL };
+
+u8 get_psu_inst(void)
+{
+	if (!pl_psu)
+		return 0;
+
+	return readb(pl_psu);
+}
+EXPORT_SYMBOL(get_psu_inst);
+
+u8 get_psu_ac(void)
+{
+	if (!pl_psu)
+		return 0;
+
+	return readb(pl_psu + 0x02);
+}
+EXPORT_SYMBOL(get_psu_ac);
+
+u8 get_psu_dc(void)
+{
+	if (!pl_psu)
+		return 0;
+
+	return readb(pl_psu + 0x03);
+}
+EXPORT_SYMBOL(get_psu_dc);
+
+void update_presence(u8 id)
+{
+	unsigned int i;
+	unsigned long temp = (unsigned long)readb(pl_psu);
+
+	for_each_set_bit(i, &temp, 8) {
+		if (i == id)
+			psus[id]->present = true;
+	}
+
+	temp = ~temp;
+	for_each_set_bit(i, &temp, 8) {
+		if (i == id)
+			psus[id]->present = false;
+	}
+}
+
+static unsigned char cal_checksum(unsigned char *buf, unsigned long size)
+{
+	unsigned char sum = 0;
+
+	while (size > 0) {
+		sum += (*(buf++));
+		size--;
+	}
+	return ((~sum) + 1);
+}
+
+static unsigned char valid_checksum(unsigned char *buf, unsigned long size)
+{
+	unsigned char sum = 0;
+
+	while (size > 0) {
+		sum += (*(buf++));
+		size--;
+	}
+	return sum;
+}
+
+static int psu_read_fru(struct gxp_psu_drvdata *drvdata,
+			u8 offset, u8 length, u8 *value)
+{
+	struct i2c_client *client = drvdata->client;
+	unsigned char buf_tx[4] = {(client->addr << 1), READ_FRU_CMD, offset, length};
+	unsigned char tx[4] = {0};
+	unsigned char chksum = cal_checksum(buf_tx, 4);
+	int ret = 0;
+	struct i2c_msg msgs[2] = {0};
+
+	update_presence(drvdata->id);
+
+	value[0] = '\0';
+
+	if (!drvdata->present)
+		return -EOPNOTSUPP;
+
+	tx[0] = READ_FRU_CMD;
+	tx[1] = offset;
+	tx[2] = length;
+	tx[3] = chksum;
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].buf = tx;
+	msgs[0].len = 4;
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].buf = value;
+	msgs[1].len = length;
+
+	mutex_lock(&drvdata->update_lock);
+	ret = i2c_transfer(client->adapter, msgs, 2);
+	mutex_unlock(&drvdata->update_lock);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"gxppsu_i2c_tx_fail addr:0x%x offest:0x%x length:0x%x chk:0x%x ret:0x%x\n",
+			client->addr, offset, length, chksum, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int psu_read_reg_word(struct gxp_psu_drvdata *drvdata,
+			     u8 reg, u16 *value)
+{
+	struct i2c_client *client = drvdata->client;
+	unsigned char buf_tx[3] = {(client->addr << 1), READ_REG_CMD, reg};
+	unsigned char buf_rx[3] = {0};
+	unsigned char tx[3] = {0};
+	unsigned char rx[3] = {0};
+	unsigned char chksum = cal_checksum(buf_tx, 3);
+	struct i2c_msg msgs[2] = {0};
+	int ret = 0;
+
+	tx[0] = 0;
+	tx[1] = reg;
+	tx[2] = chksum;
+
+	msgs[0].addr = client->addr;
+	msgs[0].flags = 0;
+	msgs[0].buf = tx;
+	msgs[0].len = 3;
+	msgs[1].addr = client->addr;
+	msgs[1].flags = I2C_M_RD;
+	msgs[1].buf = rx;
+	msgs[1].len = 3;
+	mutex_lock(&drvdata->update_lock);
+	ret = i2c_transfer(client->adapter, msgs, 2);
+	mutex_unlock(&drvdata->update_lock);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"gxppsu_i2c_tx_fail addr:0x%x reg:0x%x chk:0x%x ret:0x%x\n",
+			client->addr, reg, chksum, ret);
+		return ret;
+	}
+
+	buf_rx[0] = rx[0];
+	buf_rx[1] = rx[1];
+	buf_rx[2] = rx[2];
+	if (valid_checksum(buf_rx, 3) != 0) {
+		dev_err(&client->dev,
+			"gxppsu_checksum_fail addr:0x%x reg:0x%x, data:%x %x %x\n",
+			client->addr, reg, rx[0], rx[1], rx[2]);
+		return -EAGAIN;
+	}
+
+	*value = rx[0] + (rx[1] << 8);
+
+	return ret;
+}
+
+static int gxppsu_update_client(struct device *dev, u8 reg)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+
+	update_presence(drvdata->id);
+
+	if (!drvdata->present)
+		return -EOPNOTSUPP;
+
+	switch (reg) {
+	case REG_IN_PWR:
+		ret = psu_read_reg_word(drvdata, REG_IN_PWR,
+					&drvdata->input_power);
+		break;
+	case REG_IN_VOL:
+		ret = psu_read_reg_word(drvdata, REG_IN_VOL,
+					&drvdata->input_voltage);
+		break;
+	case REG_IN_CUR:
+		ret = psu_read_reg_word(drvdata, REG_IN_CUR,
+					&drvdata->input_current);
+		break;
+	case REG_OUT_PWR:
+		ret = psu_read_reg_word(drvdata, REG_OUT_PWR,
+					&drvdata->output_power);
+		break;
+	case REG_OUT_VOL:
+		ret = psu_read_reg_word(drvdata, REG_OUT_VOL,
+					&drvdata->output_voltage);
+		break;
+	case REG_OUT_CUR:
+		ret = psu_read_reg_word(drvdata, REG_OUT_CUR,
+					&drvdata->output_current);
+		break;
+	case REG_INLET_TEMP:
+		ret = psu_read_reg_word(drvdata, REG_INLET_TEMP,
+					&drvdata->inlet_temp);
+		break;
+	case REG_FAN_SPEED:
+		ret = psu_read_reg_word(drvdata, REG_FAN_SPEED,
+					&drvdata->fan_speed);
+		break;
+	default:
+		dev_err(&drvdata->client->dev, "gxppsu_error_reg 0x%x\n", reg);
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static int gxp_psu_get_power_input(struct device *dev, u32 attr, int channel,
+				   long *val)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg;
+
+	if (channel == INPUT_CHAN)
+		reg = REG_IN_PWR;
+	else
+		reg = REG_OUT_PWR;
+
+	ret = gxppsu_update_client(dev, reg);
+	if (ret < 0)
+		return ret;
+
+	if (channel == INPUT_CHAN)
+		*val = drvdata->input_power;
+	else
+		*val = drvdata->output_power;
+
+	return 0;
+}
+
+static int gxp_psu_get_in_input(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg;
+
+	if (channel == INPUT_CHAN)
+		reg = REG_IN_VOL;
+	else
+		reg = REG_OUT_VOL;
+
+	ret = gxppsu_update_client(dev, reg);
+	if (ret < 0)
+		return ret;
+
+	if (channel == INPUT_CHAN)
+		*val = drvdata->input_voltage;
+	else
+		*val = drvdata->output_voltage;
+
+	return 0;
+}
+
+static int gxp_psu_get_curr_input(struct device *dev, u32 attr, int channel,
+				  long *val)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg;
+
+	if (channel == INPUT_CHAN)
+		reg = REG_IN_CUR;
+	else
+		reg = REG_OUT_CUR;
+
+	ret = gxppsu_update_client(dev, reg);
+
+	if (ret < 0)
+		return ret;
+
+	if (channel == INPUT_CHAN)
+		*val = drvdata->input_current;
+	else
+		*val = drvdata->output_current;
+
+	return 0;
+}
+
+static int gxp_psu_get_temp_input(struct device *dev, u32 attr, int channel,
+				  long *val)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg = REG_INLET_TEMP;
+
+	ret = gxppsu_update_client(dev, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = drvdata->inlet_temp;
+
+	return 0;
+};
+
+static int gxp_psu_get_fan_input(struct device *dev, u32 attr, int channel,
+				 long *val)
+{
+	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg = REG_FAN_SPEED;
+
+	ret = gxppsu_update_client(dev, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = drvdata->fan_speed;
+
+	return 0;
+}
+
+void swapbytes(void *input, size_t len)
+{
+	unsigned int i;
+	unsigned char *in = (unsigned char *)input, tmp;
+
+	for (i = 0; i < len / 2; i++) {
+		tmp = *(in + i);
+		*(in + i) = *(in + len - i - 1);
+		*(in + len - i - 1) = tmp;
+	}
+}
+
+static const struct hwmon_channel_info *gxp_psu_info[] = {
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT | HWMON_P_LABEL,
+			   HWMON_P_INPUT | HWMON_P_LABEL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_LABEL),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL),
+	NULL
+};
+
+static umode_t gxp_psu_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	umode_t mode = 0;
+
+	switch (type) {
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+		case hwmon_power_label:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+	break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+		case hwmon_in_label:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_label:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+		case hwmon_curr_label:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_label:
+			mode = 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return mode;
+}
+
+static int gxp_psu_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			return gxp_psu_get_power_input(dev, attr, channel,
+							       val);
+		default:
+			break;
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return gxp_psu_get_in_input(dev, attr, channel,
+							    val);
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			return gxp_psu_get_fan_input(dev, attr, channel,
+							     val);
+		default:
+			break;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			return gxp_psu_get_curr_input(dev, attr, channel,
+						val);
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return gxp_psu_get_temp_input(dev, attr, channel,
+						      val);
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return -EOPNOTSUPP;
+}
+
+static int gxp_psu_read_string(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_label:
+			*str = channel ? L_OUT_POWER : L_IN_POWER;
+			return 0;
+		default:
+			break;
+		}
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_label:
+			*str = channel ? L_OUT_IN : L_IN_IN;
+			return 0;
+		default:
+			break;
+		}
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_label:
+			*str = L_FAN;
+			return 0;
+		default:
+			break;
+		}
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_label:
+			*str = channel ? L_OUT_CURR : L_IN_CURR;
+			return 0;
+		default:
+			break;
+		}
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			*str = L_TEMP;
+			return 0;
+		default:
+			break;
+		}
+	default:
+		break;
+	}
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops gxp_psu_ops = {
+	.is_visible = gxp_psu_is_visible,
+	.read = gxp_psu_read,
+	.read_string = gxp_psu_read_string,
+};
+
+static const struct hwmon_chip_info gxp_psu_chip_info = {
+	.ops = &gxp_psu_ops,
+	.info = gxp_psu_info,
+};
+
+#ifdef CONFIG_DEBUG_FS
+
+static int serial_number_show(struct seq_file *seqf, void *unused)
+{
+	struct gxp_psu_drvdata *drvdata = seqf->private;
+	int ret = 0;
+
+	ret = psu_read_fru(drvdata, 91, 14, &drvdata->serial_number[0]);
+	if (ret < 0) {
+		dev_err(&drvdata->client->dev, "Unknown product serial number %d", ret);
+		seq_puts(seqf, "unknown\n");
+		return 0;
+	}
+
+	seq_printf(seqf, "%s\n", drvdata->serial_number);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(serial_number);
+
+static int manufacturer_show(struct seq_file *seqf, void *unused)
+{
+	struct gxp_psu_drvdata *drvdata = seqf->private;
+	int ret = 0;
+
+	ret = psu_read_fru(drvdata, 197, 3, &drvdata->product_manufacturer[0]);
+	if (ret < 0) {
+		dev_err(&drvdata->client->dev, "Unknown product manufacturer %d", ret);
+		seq_puts(seqf, "unknown\n");
+		return 0;
+	}
+
+	swapbytes(&drvdata->product_manufacturer[0], 3);
+	seq_printf(seqf, "%s\n", drvdata->product_manufacturer);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(manufacturer);
+
+static int product_name_show(struct seq_file *seqf, void *unused)
+{
+	struct gxp_psu_drvdata *drvdata = seqf->private;
+	int ret = 0;
+
+	ret = psu_read_fru(drvdata, 50, 26, &drvdata->product_name[0]);
+	if (ret < 0) {
+		dev_err(&drvdata->client->dev, "Unknown product name %d", ret);
+		seq_puts(seqf, "unknown\n");
+		return 0;
+	}
+
+	seq_printf(seqf, "%s\n", drvdata->product_name);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(product_name);
+
+static int spare_show(struct seq_file *seqf, void *unused)
+{
+	struct gxp_psu_drvdata *drvdata = seqf->private;
+	int ret = 0;
+
+	ret = psu_read_fru(drvdata, 18, 10, &drvdata->spare_part[0]);
+	if (ret < 0) {
+		dev_err(&drvdata->client->dev, "Unknown product spare %d", ret);
+		seq_puts(seqf, "unknown\n");
+		return 0;
+	}
+
+	seq_printf(seqf, "%s\n", drvdata->spare_part);
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(spare);
+
+static int present_show(struct seq_file *seqf, void *unused)
+{
+	struct gxp_psu_drvdata *drvdata = seqf->private;
+
+	update_presence(drvdata->id);
+
+	if (drvdata->present)
+		seq_puts(seqf, "yes\n");
+	else
+		seq_puts(seqf, "no\n");
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(present);
+
+static void gxp_psu_debugfs_init(struct gxp_psu_drvdata *drvdata)
+{
+	char name[32];
+
+	scnprintf(name, sizeof(name), "%s_%s-%d", dev_name(drvdata->hwmon_dev),
+		  drvdata->client->name, drvdata->client->addr);
+
+	drvdata->debugfs = debugfs_create_dir(name, NULL);
+
+	debugfs_create_file("serial_number", 0444, drvdata->debugfs, drvdata, &serial_number_fops);
+
+	debugfs_create_file("manufacturer", 0444, drvdata->debugfs, drvdata, &manufacturer_fops);
+
+	debugfs_create_file("product_name", 0444, drvdata->debugfs, drvdata, &product_name_fops);
+
+	debugfs_create_file("spare", 0444, drvdata->debugfs, drvdata, &spare_fops);
+
+	debugfs_create_file("present", 0444, drvdata->debugfs, drvdata, &present_fops);
+
+	if (!debugfs_initialized())
+		dev_err(&drvdata->client->dev, "Debug FS not Init");
+}
+
+#else
+
+static void gxp_psu_debugfs_init(struct gxp_psu_drvdata *drvdata)
+{
+}
+
+#endif
+
+static int gxp_psu_probe(struct i2c_client *client)
+{
+	struct gxp_psu_drvdata *drvdata;
+	struct device *hwmon_dev;
+	struct device_node *np;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL))
+		return -EIO;
+
+	drvdata = devm_kzalloc(&client->dev, sizeof(struct gxp_psu_drvdata),
+			       GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	if (!pl_psu) {
+		np = of_parse_phandle(client->dev.of_node, "hpe,sysreg", 0);
+		if (!np)
+			return -ENODEV;
+
+		pl_psu = of_iomap(np, 0);
+		if (IS_ERR(pl_psu))
+			return dev_err_probe(&client->dev, IS_ERR(pl_psu),
+					     "failed to map pl_psu");
+	}
+
+	drvdata->client = client;
+	i2c_set_clientdata(client, drvdata);
+
+	mutex_init(&drvdata->update_lock);
+	drvdata->hwmon_dev = NULL;
+	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev,
+							 "hpe_gxp_psu",
+							 drvdata,
+							 &gxp_psu_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	drvdata->hwmon_dev = hwmon_dev;
+
+	drvdata->id = psucount;
+
+	psus[psucount] = drvdata;
+
+	update_presence(drvdata->id);
+
+	psucount++;
+
+	gxp_psu_debugfs_init(drvdata);
+
+	return 0;
+}
+
+static const struct of_device_id gxp_psu_of_match[] = {
+	{ .compatible = "hpe,gxp-psu" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gxp_psu_of_match);
+
+static struct i2c_driver gxp_psu_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "gxp-psu",
+		.of_match_table = gxp_psu_of_match,
+	},
+	.probe_new = gxp_psu_probe,
+};
+module_i2c_driver(gxp_psu_driver);
+
+MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
+MODULE_DESCRIPTION("HPE GXP PSU driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v1 3/9] hwmon: (gxp-psu) Add driver to read HPE PSUs

[Guenter Roeck]

On Tue, Apr 18, 2023 at 10:28:18AM -0500, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The GXP SoC can support up to 8 power supplies that it can communicate
> with via i2c. The GXP PSU driver will provide a method for the GPIO
> driver with info it reads to be available to the host.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  drivers/hwmon/Kconfig   |  10 +
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/gxp-psu.c | 773 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 784 insertions(+)
>  create mode 100644 drivers/hwmon/gxp-psu.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..3b56690ea089 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -723,6 +723,16 @@ config SENSORS_GXP_FAN_CTRL
>  	  The GXP controls fan function via the CPLD through the use of PWM
>  	  registers. This driver reports status and pwm setting of the fans.
>  
> +config SENSORS_GXP_PSU
> +	tristate "HPE GXP PSU driver"
> +	depends on ARCH_HPE_GXP || COMPILE_TEST
> +	depends on I2C
> +	help
> +	  If you say yes you will get support for GXP psu driver support. The GXP
> +	  gets PSU presence and state information from the CPLD. The GXP gets PSU
> +	  data via i2c. It provides a method for other drivers to call into get
> +	  PSU presence information.
> +
>  config SENSORS_HIH6130
>  	tristate "Honeywell Humidicon HIH-6130 humidity/temperature sensor"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 88712b5031c8..6c84cd52d0d3 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
>  obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>  obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
> +obj-$(CONFIG_SENSORS_GXP_PSU)	+= gxp-psu.o
>  obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
>  obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
>  obj-$(CONFIG_SENSORS_I5500)	+= i5500_temp.o
> diff --git a/drivers/hwmon/gxp-psu.c b/drivers/hwmon/gxp-psu.c
> new file mode 100644
> index 000000000000..e4217200c34b
> --- /dev/null
> +++ b/drivers/hwmon/gxp-psu.c
> @@ -0,0 +1,773 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2023 Hewlett-Packard Enterpise Development Company, L.P. */
> +
> +#include <linux/err.h>
> +#include <linux/debugfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#define READ_REG_CMD		0x00
> +#define READ_FRU_CMD		0x22
> +#define REG_IN_VOL		0x10
> +#define REG_IN_CUR		0x11
> +#define REG_IN_PWR		0x12
> +#define REG_OUT_VOL		0x20
> +#define REG_OUT_CUR		0x21
> +#define REG_OUT_PWR		0x22
> +#define REG_FAN_SPEED		0x40
> +#define REG_INLET_TEMP		0x42
> +#define MAX_PSU			0x08
> +#define INPUT_CHAN 0
> +
> +#define L_IN_POWER		"pin1"
> +#define L_OUT_POWER		"pout1"
> +#define L_IN_IN			"vin"
> +#define L_OUT_IN		"vout1"
> +#define L_FAN			"fan1"
> +#define L_IN_CURR		"iin"
> +#define L_OUT_CURR		"iout1"
> +#define L_TEMP			"temp1"
> +
> +static void __iomem *pl_psu;
> +
> +struct gxp_psu_drvdata {
> +	struct i2c_client *client;
> +	u16 input_power;
> +	u16 input_voltage;
> +	u16 input_current;
> +	u16 output_power;
> +	u16 output_voltage;
> +	u16 output_current;
> +	s16 inlet_temp;
> +	u16 fan_speed;
> +	u8 id;
> +	struct dentry *debugfs;
> +	u8 spare_part[10];
> +	u8 product_name[26];
> +	u8 serial_number[14];
> +	u8 product_manufacturer[3];
> +	bool present; /* psu can be physically removed */
> +	struct mutex update_lock;
> +	struct device *hwmon_dev;
> +};
> +
> +static u8 psucount;
> +struct gxp_psu_drvdata *psus[MAX_PSU] = { NULL };
> +
> +u8 get_psu_inst(void)
> +{
> +	if (!pl_psu)
> +		return 0;
> +
> +	return readb(pl_psu);
> +}
> +EXPORT_SYMBOL(get_psu_inst);
> +
> +u8 get_psu_ac(void)
> +{
> +	if (!pl_psu)
> +		return 0;
> +
> +	return readb(pl_psu + 0x02);
> +}
> +EXPORT_SYMBOL(get_psu_ac);
> +
> +u8 get_psu_dc(void)
> +{
> +	if (!pl_psu)
> +		return 0;
> +
> +	return readb(pl_psu + 0x03);
> +}
> +EXPORT_SYMBOL(get_psu_dc);
> +

The above is unacceptable. If you want to make the information available
to userspace, use debugfs files. 

> +void update_presence(u8 id)
> +{
> +	unsigned int i;
> +	unsigned long temp = (unsigned long)readb(pl_psu);
> +
> +	for_each_set_bit(i, &temp, 8) {
> +		if (i == id)
> +			psus[id]->present = true;
> +	}
> +
> +	temp = ~temp;
> +	for_each_set_bit(i, &temp, 8) {
> +		if (i == id)
> +			psus[id]->present = false;
> +	}
> +}
> +
> +static unsigned char cal_checksum(unsigned char *buf, unsigned long size)
> +{
> +	unsigned char sum = 0;
> +
> +	while (size > 0) {
> +		sum += (*(buf++));
> +		size--;
> +	}
> +	return ((~sum) + 1);
> +}
> +
> +static unsigned char valid_checksum(unsigned char *buf, unsigned long size)
> +{
> +	unsigned char sum = 0;
> +
> +	while (size > 0) {
> +		sum += (*(buf++));
> +		size--;
> +	}
> +	return sum;
> +}
> +
> +static int psu_read_fru(struct gxp_psu_drvdata *drvdata,
> +			u8 offset, u8 length, u8 *value)
> +{
> +	struct i2c_client *client = drvdata->client;
> +	unsigned char buf_tx[4] = {(client->addr << 1), READ_FRU_CMD, offset, length};
> +	unsigned char tx[4] = {0};
> +	unsigned char chksum = cal_checksum(buf_tx, 4);
> +	int ret = 0;

Initializing ret is unnecessary.

> +	struct i2c_msg msgs[2] = {0};
> +
> +	update_presence(drvdata->id);
> +
> +	value[0] = '\0';
> +
> +	if (!drvdata->present)
> +		return -EOPNOTSUPP;

Wrong return value. The attribute is supported, it just doesn't have any data
associated with it.

> +
> +	tx[0] = READ_FRU_CMD;
> +	tx[1] = offset;
> +	tx[2] = length;
> +	tx[3] = chksum;
> +	msgs[0].addr = client->addr;
> +	msgs[0].flags = 0;
> +	msgs[0].buf = tx;
> +	msgs[0].len = 4;
> +	msgs[1].addr = client->addr;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].buf = value;
> +	msgs[1].len = length;
> +
> +	mutex_lock(&drvdata->update_lock);
> +	ret = i2c_transfer(client->adapter, msgs, 2);
> +	mutex_unlock(&drvdata->update_lock);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"gxppsu_i2c_tx_fail addr:0x%x offest:0x%x length:0x%x chk:0x%x ret:0x%x\n",
> +			client->addr, offset, length, chksum, ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int psu_read_reg_word(struct gxp_psu_drvdata *drvdata,
> +			     u8 reg, u16 *value)
> +{
> +	struct i2c_client *client = drvdata->client;
> +	unsigned char buf_tx[3] = {(client->addr << 1), READ_REG_CMD, reg};
> +	unsigned char buf_rx[3] = {0};
> +	unsigned char tx[3] = {0};
> +	unsigned char rx[3] = {0};
> +	unsigned char chksum = cal_checksum(buf_tx, 3);
> +	struct i2c_msg msgs[2] = {0};
> +	int ret = 0;
> +
> +	tx[0] = 0;
> +	tx[1] = reg;
> +	tx[2] = chksum;
> +
> +	msgs[0].addr = client->addr;
> +	msgs[0].flags = 0;
> +	msgs[0].buf = tx;
> +	msgs[0].len = 3;
> +	msgs[1].addr = client->addr;
> +	msgs[1].flags = I2C_M_RD;
> +	msgs[1].buf = rx;
> +	msgs[1].len = 3;
> +	mutex_lock(&drvdata->update_lock);
> +	ret = i2c_transfer(client->adapter, msgs, 2);
> +	mutex_unlock(&drvdata->update_lock);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"gxppsu_i2c_tx_fail addr:0x%x reg:0x%x chk:0x%x ret:0x%x\n",
> +			client->addr, reg, chksum, ret);
> +		return ret;
> +	}
> +
> +	buf_rx[0] = rx[0];
> +	buf_rx[1] = rx[1];
> +	buf_rx[2] = rx[2];
> +	if (valid_checksum(buf_rx, 3) != 0) {
> +		dev_err(&client->dev,
> +			"gxppsu_checksum_fail addr:0x%x reg:0x%x, data:%x %x %x\n",
> +			client->addr, reg, rx[0], rx[1], rx[2]);
> +		return -EAGAIN;

This is an IO error. If the interface is so unstable that it needs retries,
implement retries, don't expect the kernel or userspace to do it for you,
and limit the number of retries.

> +	}
> +
> +	*value = rx[0] + (rx[1] << 8);

Are those values all in units expected by the hwmon subsystem ?

> +
> +	return ret;
> +}
> +
> +static int gxppsu_update_client(struct device *dev, u8 reg)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	update_presence(drvdata->id);
> +
 +	if (!drvdata->present)
> +		return -EOPNOTSUPP;

Should be -ENODATA but, really, PSUs should not be instantiated to start with
if they are not installed.

> +
> +	switch (reg) {
> +	case REG_IN_PWR:
> +		ret = psu_read_reg_word(drvdata, REG_IN_PWR,
> +					&drvdata->input_power);
> +		break;
> +	case REG_IN_VOL:
> +		ret = psu_read_reg_word(drvdata, REG_IN_VOL,
> +					&drvdata->input_voltage);
> +		break;
> +	case REG_IN_CUR:
> +		ret = psu_read_reg_word(drvdata, REG_IN_CUR,
> +					&drvdata->input_current);
> +		break;
> +	case REG_OUT_PWR:
> +		ret = psu_read_reg_word(drvdata, REG_OUT_PWR,
> +					&drvdata->output_power);
> +		break;
> +	case REG_OUT_VOL:
> +		ret = psu_read_reg_word(drvdata, REG_OUT_VOL,
> +					&drvdata->output_voltage);
> +		break;
> +	case REG_OUT_CUR:
> +		ret = psu_read_reg_word(drvdata, REG_OUT_CUR,
> +					&drvdata->output_current);
> +		break;
> +	case REG_INLET_TEMP:
> +		ret = psu_read_reg_word(drvdata, REG_INLET_TEMP,
> +					&drvdata->inlet_temp);
> +		break;
> +	case REG_FAN_SPEED:
> +		ret = psu_read_reg_word(drvdata, REG_FAN_SPEED,
> +					&drvdata->fan_speed);
> +		break;
> +	default:
> +		dev_err(&drvdata->client->dev, "gxppsu_error_reg 0x%x\n", reg);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +static int gxp_psu_get_power_input(struct device *dev, u32 attr, int channel,
> +				   long *val)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg;
> +
> +	if (channel == INPUT_CHAN)
> +		reg = REG_IN_PWR;
> +	else
> +		reg = REG_OUT_PWR;
> +
> +	ret = gxppsu_update_client(dev, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (channel == INPUT_CHAN)
> +		*val = drvdata->input_power;
> +	else
> +		*val = drvdata->output_power;
> +
> +	return 0;
> +}
> +
> +static int gxp_psu_get_in_input(struct device *dev, u32 attr, int channel,
> +				long *val)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg;
> +
> +	if (channel == INPUT_CHAN)
> +		reg = REG_IN_VOL;
> +	else
> +		reg = REG_OUT_VOL;
> +
> +	ret = gxppsu_update_client(dev, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (channel == INPUT_CHAN)
> +		*val = drvdata->input_voltage;
> +	else
> +		*val = drvdata->output_voltage;
> +
> +	return 0;
> +}
> +
> +static int gxp_psu_get_curr_input(struct device *dev, u32 attr, int channel,
> +				  long *val)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg;
> +
> +	if (channel == INPUT_CHAN)
> +		reg = REG_IN_CUR;
> +	else
> +		reg = REG_OUT_CUR;
> +
> +	ret = gxppsu_update_client(dev, reg);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (channel == INPUT_CHAN)
> +		*val = drvdata->input_current;
> +	else
> +		*val = drvdata->output_current;
> +
> +	return 0;
> +}
> +
> +static int gxp_psu_get_temp_input(struct device *dev, u32 attr, int channel,
> +				  long *val)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg = REG_INLET_TEMP;
> +
> +	ret = gxppsu_update_client(dev, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = drvdata->inlet_temp;
> +
> +	return 0;
> +};
> +
> +static int gxp_psu_get_fan_input(struct device *dev, u32 attr, int channel,
> +				 long *val)
> +{
> +	struct gxp_psu_drvdata *drvdata = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg = REG_FAN_SPEED;
> +
> +	ret = gxppsu_update_client(dev, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = drvdata->fan_speed;
> +

All this code is terribly complex. Why not just use a function passing the
register and returning its value ? Storing interim results in various
variables just to read them a bit later seems like a lot of overly complex
code.

Example:

psu_read_reg_word(drvdata, REG_IN_PWR,
> +                                     &drvdata->input_power);

	u16 fan_speed;

	ret = psu_read_reg_word(dev, REG_FAN_SPEED, &fan_speed);
	if (ret < 0)
		return ret
	
	return fan_speed;

Given that the value parameter of psu_read_reg_word() is a pointer to u16,
it is unnecessary to provide it in the first place, and the return value
could be both an error or a return value. This would reduce this code to

	return psu_read_reg_word(dev, REG_FAN_SPEED);

> +	return 0;
> +}
> +
> +void swapbytes(void *input, size_t len)
> +{
> +	unsigned int i;
> +	unsigned char *in = (unsigned char *)input, tmp;
> +
> +	for (i = 0; i < len / 2; i++) {
> +		tmp = *(in + i);
> +		*(in + i) = *(in + len - i - 1);
> +		*(in + len - i - 1) = tmp;
> +	}
> +}
> +
> +static const struct hwmon_channel_info *gxp_psu_info[] = {
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_LABEL,
> +			   HWMON_P_INPUT | HWMON_P_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static umode_t gxp_psu_is_visible(const void *_data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	umode_t mode = 0;
> +
> +	switch (type) {
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +		case hwmon_power_label:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +	break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_label:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_label:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +		case hwmon_curr_label:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_label:
> +			mode = 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return mode;
> +}
> +
> +static int gxp_psu_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return gxp_psu_get_power_input(dev, attr, channel,
> +							       val);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return gxp_psu_get_in_input(dev, attr, channel,
> +							    val);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			return gxp_psu_get_fan_input(dev, attr, channel,
> +							     val);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return gxp_psu_get_curr_input(dev, attr, channel,
> +						val);
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return gxp_psu_get_temp_input(dev, attr, channel,
> +						      val);
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static int gxp_psu_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_label:
> +			*str = channel ? L_OUT_POWER : L_IN_POWER;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*str = channel ? L_OUT_IN : L_IN_IN;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_label:
> +			*str = L_FAN;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_label:
> +			*str = channel ? L_OUT_CURR : L_IN_CURR;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_label:
> +			*str = L_TEMP;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	default:
> +		break;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops gxp_psu_ops = {
> +	.is_visible = gxp_psu_is_visible,
> +	.read = gxp_psu_read,
> +	.read_string = gxp_psu_read_string,
> +};
> +
> +static const struct hwmon_chip_info gxp_psu_chip_info = {
> +	.ops = &gxp_psu_ops,
> +	.info = gxp_psu_info,
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int serial_number_show(struct seq_file *seqf, void *unused)
> +{
> +	struct gxp_psu_drvdata *drvdata = seqf->private;
> +	int ret = 0;
> +
> +	ret = psu_read_fru(drvdata, 91, 14, &drvdata->serial_number[0]);
> +	if (ret < 0) {
> +		dev_err(&drvdata->client->dev, "Unknown product serial number %d", ret);
> +		seq_puts(seqf, "unknown\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seqf, "%s\n", drvdata->serial_number);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(serial_number);
> +
> +static int manufacturer_show(struct seq_file *seqf, void *unused)
> +{
> +	struct gxp_psu_drvdata *drvdata = seqf->private;
> +	int ret = 0;
> +
> +	ret = psu_read_fru(drvdata, 197, 3, &drvdata->product_manufacturer[0]);
> +	if (ret < 0) {
> +		dev_err(&drvdata->client->dev, "Unknown product manufacturer %d", ret);
> +		seq_puts(seqf, "unknown\n");
> +		return 0;
> +	}
> +
> +	swapbytes(&drvdata->product_manufacturer[0], 3);
> +	seq_printf(seqf, "%s\n", drvdata->product_manufacturer);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(manufacturer);
> +
> +static int product_name_show(struct seq_file *seqf, void *unused)
> +{
> +	struct gxp_psu_drvdata *drvdata = seqf->private;
> +	int ret = 0;
> +
> +	ret = psu_read_fru(drvdata, 50, 26, &drvdata->product_name[0]);
> +	if (ret < 0) {
> +		dev_err(&drvdata->client->dev, "Unknown product name %d", ret);
> +		seq_puts(seqf, "unknown\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seqf, "%s\n", drvdata->product_name);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(product_name);
> +
> +static int spare_show(struct seq_file *seqf, void *unused)
> +{
> +	struct gxp_psu_drvdata *drvdata = seqf->private;
> +	int ret = 0;
> +
> +	ret = psu_read_fru(drvdata, 18, 10, &drvdata->spare_part[0]);
> +	if (ret < 0) {
> +		dev_err(&drvdata->client->dev, "Unknown product spare %d", ret);
> +		seq_puts(seqf, "unknown\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seqf, "%s\n", drvdata->spare_part);
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spare);
> +
> +static int present_show(struct seq_file *seqf, void *unused)
> +{
> +	struct gxp_psu_drvdata *drvdata = seqf->private;
> +
> +	update_presence(drvdata->id);
> +
> +	if (drvdata->present)
> +		seq_puts(seqf, "yes\n");
> +	else
> +		seq_puts(seqf, "no\n");
> +

What happens if the power supply is not present and someone tries 
to read any of its attributes ?

> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(present);
> +
> +static void gxp_psu_debugfs_init(struct gxp_psu_drvdata *drvdata)
> +{
> +	char name[32];
> +
> +	scnprintf(name, sizeof(name), "%s_%s-%d", dev_name(drvdata->hwmon_dev),
> +		  drvdata->client->name, drvdata->client->addr);
> +
> +	drvdata->debugfs = debugfs_create_dir(name, NULL);

drvdata->debugfs is only used here. No point storing the variable in drvdata
unless it is used there (for removal, for example).

I really wonder what happens if the driver is built as module and unloaded.

> +
> +	debugfs_create_file("serial_number", 0444, drvdata->debugfs, drvdata, &serial_number_fops);
> +
> +	debugfs_create_file("manufacturer", 0444, drvdata->debugfs, drvdata, &manufacturer_fops);
> +
> +	debugfs_create_file("product_name", 0444, drvdata->debugfs, drvdata, &product_name_fops);
> +
> +	debugfs_create_file("spare", 0444, drvdata->debugfs, drvdata, &spare_fops);
> +
> +	debugfs_create_file("present", 0444, drvdata->debugfs, drvdata, &present_fops);
> +
> +	if (!debugfs_initialized())
> +		dev_err(&drvdata->client->dev, "Debug FS not Init");

No such error message please. debugfs failures are supposed to be silent.

> +}
> +
> +#else
> +
> +static void gxp_psu_debugfs_init(struct gxp_psu_drvdata *drvdata)
> +{
> +}
> +
> +#endif
> +
> +static int gxp_psu_probe(struct i2c_client *client)
> +{
> +	struct gxp_psu_drvdata *drvdata;
> +	struct device *hwmon_dev;
> +	struct device_node *np;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL))
> +		return -EIO;
> +
> +	drvdata = devm_kzalloc(&client->dev, sizeof(struct gxp_psu_drvdata),
> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	if (!pl_psu) {
> +		np = of_parse_phandle(client->dev.of_node, "hpe,sysreg", 0);
> +		if (!np)
> +			return -ENODEV;
> +
> +		pl_psu = of_iomap(np, 0);
> +		if (IS_ERR(pl_psu))
> +			return dev_err_probe(&client->dev, IS_ERR(pl_psu),
> +					     "failed to map pl_psu");
> +	}
> +
> +	drvdata->client = client;
> +	i2c_set_clientdata(client, drvdata);
> +
> +	mutex_init(&drvdata->update_lock);
> +	drvdata->hwmon_dev = NULL;

What is that for ? drvdata was just allocated with devm_kzalloc().

> +	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev,
> +							 "hpe_gxp_psu",
> +							 drvdata,
> +							 &gxp_psu_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	drvdata->hwmon_dev = hwmon_dev;

drvdata->hwmon_dev is only used in gxp_psu_debugfs_init().
That means it is really pointless to store it in drvdata;
you might as well pass hwmon_dev as parameter to that function.

> +
> +	drvdata->id = psucount;
> +
> +	psus[psucount] = drvdata;
> +
> +	update_presence(drvdata->id);
> +
> +	psucount++;

So what happens if someone removes and reinstantiates this driver ?
Or instantiates more than MAX_PSU instances ?

Overall this looks very fragile. There should not be any such static
variables/arrays. I'd suggest to use a list and the I2C address as
ID, or something similar.

> +
> +	gxp_psu_debugfs_init(drvdata);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gxp_psu_of_match[] = {
> +	{ .compatible = "hpe,gxp-psu" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_psu_of_match);
> +
> +static struct i2c_driver gxp_psu_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "gxp-psu",
> +		.of_match_table = gxp_psu_of_match,
> +	},
> +	.probe_new = gxp_psu_probe,
> +};
> +module_i2c_driver(gxp_psu_driver);
> +
> +MODULE_AUTHOR("Nick Hawkins <nick.hawkins@hpe.com>");
> +MODULE_DESCRIPTION("HPE GXP PSU driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.1
> 

Re: [PATCH v1 3/9] hwmon: (gxp-psu) Add driver to read HPE PSUs

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.3-rc7]
[cannot apply to brgl/gpio/for-next next-20230417]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230418152824.110823-4-nick.hawkins%40hpe.com
patch subject: [PATCH v1 3/9] hwmon: (gxp-psu) Add driver to read HPE PSUs
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230419/202304190301.9jtv8uzG-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4006e868883aedc37a54480167f6f9dc377db1cc
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/gpio-gxp-Add-HPE-GXP-GPIO/20230418-233513
        git checkout 4006e868883aedc37a54480167f6f9dc377db1cc
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304190301.9jtv8uzG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hwmon/gxp-psu.c:62:4: warning: no previous prototype for 'get_psu_inst' [-Wmissing-prototypes]
      62 | u8 get_psu_inst(void)
         |    ^~~~~~~~~~~~
>> drivers/hwmon/gxp-psu.c:71:4: warning: no previous prototype for 'get_psu_ac' [-Wmissing-prototypes]
      71 | u8 get_psu_ac(void)
         |    ^~~~~~~~~~
>> drivers/hwmon/gxp-psu.c:80:4: warning: no previous prototype for 'get_psu_dc' [-Wmissing-prototypes]
      80 | u8 get_psu_dc(void)
         |    ^~~~~~~~~~
>> drivers/hwmon/gxp-psu.c:89:6: warning: no previous prototype for 'update_presence' [-Wmissing-prototypes]
      89 | void update_presence(u8 id)
         |      ^~~~~~~~~~~~~~~
>> drivers/hwmon/gxp-psu.c:376:6: warning: no previous prototype for 'swapbytes' [-Wmissing-prototypes]
     376 | void swapbytes(void *input, size_t len)
         |      ^~~~~~~~~
   drivers/hwmon/gxp-psu.c: In function 'gxp_psu_read_string':
>> drivers/hwmon/gxp-psu.c:528:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
     528 |                 switch (attr) {
         |                 ^~~~~~
   drivers/hwmon/gxp-psu.c:535:9: note: here
     535 |         case hwmon_in:
         |         ^~~~
   drivers/hwmon/gxp-psu.c:536:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
     536 |                 switch (attr) {
         |                 ^~~~~~
   drivers/hwmon/gxp-psu.c:543:9: note: here
     543 |         case hwmon_fan:
         |         ^~~~
   drivers/hwmon/gxp-psu.c:544:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
     544 |                 switch (attr) {
         |                 ^~~~~~
   drivers/hwmon/gxp-psu.c:551:9: note: here
     551 |         case hwmon_curr:
         |         ^~~~
   drivers/hwmon/gxp-psu.c:552:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
     552 |                 switch (attr) {
         |                 ^~~~~~
   drivers/hwmon/gxp-psu.c:559:9: note: here
     559 |         case hwmon_temp:
         |         ^~~~


vim +/get_psu_inst +62 drivers/hwmon/gxp-psu.c

    61	
  > 62	u8 get_psu_inst(void)
    63	{
    64		if (!pl_psu)
    65			return 0;
    66	
    67		return readb(pl_psu);
    68	}
    69	EXPORT_SYMBOL(get_psu_inst);
    70	
  > 71	u8 get_psu_ac(void)
    72	{
    73		if (!pl_psu)
    74			return 0;
    75	
    76		return readb(pl_psu + 0x02);
    77	}
    78	EXPORT_SYMBOL(get_psu_ac);
    79	
  > 80	u8 get_psu_dc(void)
    81	{
    82		if (!pl_psu)
    83			return 0;
    84	
    85		return readb(pl_psu + 0x03);
    86	}
    87	EXPORT_SYMBOL(get_psu_dc);
    88	
  > 89	void update_presence(u8 id)
    90	{
    91		unsigned int i;
    92		unsigned long temp = (unsigned long)readb(pl_psu);
    93	
    94		for_each_set_bit(i, &temp, 8) {
    95			if (i == id)
    96				psus[id]->present = true;
    97		}
    98	
    99		temp = ~temp;
   100		for_each_set_bit(i, &temp, 8) {
   101			if (i == id)
   102				psus[id]->present = false;
   103		}
   104	}
   105	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH v1 4/9] dt-bindings: hwmon: Modify hpe,gxp-fan-ctrl

[nick.hawkins]

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

Remove the fn2 register reference as GPIO will
be using it.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml         | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
index 4a52aac6be72..ee70f06787f6 100644
--- a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
+++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
@@ -21,13 +21,11 @@ properties:
     items:
       - description: Fan controller PWM
       - description: Programmable logic
-      - description: Function 2
 
   reg-names:
     items:
       - const: base
       - const: pl
-      - const: fn2
 
 required:
   - compatible
@@ -40,6 +38,6 @@ examples:
   - |
     fan-controller@1000c00 {
       compatible = "hpe,gxp-fan-ctrl";
-      reg = <0x1000c00 0x200>, <0xd1000000 0xff>, <0x80200000 0x100000>;
-      reg-names = "base", "pl", "fn2";
+      reg = <0x1000c00 0x200>, <0xd1000000 0xff>;
+      reg-names = "base", "pl";
     };
-- 
2.17.1

Re: [PATCH v1 4/9] dt-bindings: hwmon: Modify hpe,gxp-fan-ctrl

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Remove the fn2 register reference as GPIO will
> be using it.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

Subject: everything is modify/update. Be a bit more descriptive what you
are doing here.

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml         | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

BTW, bindings go before drivers using them.

Best regards,
Krzysztof

[PATCH v1 5/9] dt-bindings: gpio: Add HPE GXP GPIO

[nick.hawkins]

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

Provide access to the registers and interrupts for GPIO. The GPIO
will have two driver instances: One for host, the other for CPLD.

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

diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
new file mode 100644
index 000000000000..1cf4cff26d5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
@@ -0,0 +1,137 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP gpio controllers
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+
+description:
+  Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - hpe,gxp-gpio
+              - hpe,gxp-gpio-pl
+
+  reg:
+    minItems: 3
+    maxItems: 6
+
+  reg-names:
+    minItems: 3
+    maxItems: 6
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-line-names:
+    minItems: 1
+    maxItems: 300
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - hpe,gxp-gpio
+    then:
+      properties:
+        reg:
+          items:
+            - description: CSM
+            - description: fn2 virtual button
+            - description: fn2 system status
+            - description: vuhc status
+            - description: external virtual button
+        reg-names:
+          items:
+            - const: csm
+            - const: fn2-vbtn
+            - const: fn2-stat
+            - const: vuhc
+            - const: vbtn
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - hpe,gxp-gpio-pl
+    then:
+      properties:
+        reg:
+          items:
+            - description: Programmable logic led
+            - description: Programmable logic health led
+            - description: Programmable logic interrupt interface
+        reg-names:
+          items:
+            - const: pl-led
+            - const: pl-health
+            - const: pl-int
+
+examples:
+  - |
+        gpio@0 {
+          compatible = "hpe,gxp-gpio";
+          reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>, <0x400064 0x80>, <0x5100030f 0x1>;
+          reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc", "vbtn";
+          gpio-controller;
+          #gpio-cells = <2>;
+          interrupt-parent = <&vic0>;
+          interrupts = <10>;
+          gpio-line-names =
+          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",
+          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
+          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
+          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
+          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
+          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
+          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
+          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
+          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
+          "", "", "", "", "", "", "", "", "", "";
+        };
+
+  - |
+        gpio@51000304 {
+          compatible = "hpe,gxp-gpio-pl";
+          reg = <0x51000304 0x2>, <0x5100030d 0x01>, <0x51000380 0x7f>;
+          reg-names = "pl-led", "pl-health", "pl-int";
+          gpio-controller;
+          #gpio-cells = <2>;
+          interrupt-parent = <&vic0>;
+          interrupts = <24>;
+          gpio-line-names =
+          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",
+          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
+          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
+          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
+          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
+          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
+          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
+          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
+          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
+          "", "", "", "", "", "", "", "", "", "";
+        };
-- 
2.17.1

Re: [PATCH v1 5/9] dt-bindings: gpio: Add HPE GXP GPIO

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide access to the registers and interrupts for GPIO. The GPIO
> will have two driver instances: One for host, the other for CPLD.

Are these different devices? What does it mean here "instance"? What are
the differences?

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/gpio/hpe,gxp-gpio.yaml           | 137 ++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> new file mode 100644
> index 000000000000..1cf4cff26d5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP gpio controllers

s/gpio/GPIO/

> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +description:
> +  Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces.
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop oneOf.

> +      - items:

And items. You do not have here multiple choices and items.

> +          - enum:
> +              - hpe,gxp-gpio
> +              - hpe,gxp-gpio-pl
> +
> +  reg:
> +    minItems: 3
> +    maxItems: 6
> +
> +  reg-names:
> +    minItems: 3
> +    maxItems: 6
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  gpio-line-names:
> +    minItems: 1
> +    maxItems: 300

Hm, shouldn't line-names match all GPIOs? If someone provides just one
name, how do you know for which GPIO is it?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"

Use consistent quotes. Either ' or "

> +
> +additionalProperties: false

Put it after allOf: block.

> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - hpe,gxp-gpio
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: CSM
> +            - description: fn2 virtual button
> +            - description: fn2 system status
> +            - description: vuhc status
> +            - description: external virtual button

I have doubts you describe actual one GPIO controller...

> +        reg-names:
> +          items:
> +            - const: csm
> +            - const: fn2-vbtn
> +            - const: fn2-stat
> +            - const: vuhc
> +            - const: vbtn
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - hpe,gxp-gpio-pl
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: Programmable logic led
> +            - description: Programmable logic health led
> +            - description: Programmable logic interrupt interface
> +        reg-names:
> +          items:
> +            - const: pl-led
> +            - const: pl-health
> +            - const: pl-int
> +
> +examples:
> +  - |
> +        gpio@0 {

Weird indentation. Use 4 spaces for example indentation.

> +          compatible = "hpe,gxp-gpio";
> +          reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>, <0x400064 0x80>, <0x5100030f 0x1>;
> +          reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc", "vbtn";
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          interrupt-parent = <&vic0>;
> +          interrupts = <10>;
> +          gpio-line-names =
> +          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",

Broken indentation and unnecessary line break before.

> +          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
> +          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
> +          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
> +          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
> +          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
> +          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
> +          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
> +          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
> +          "", "", "", "", "", "", "", "", "", "";
> +        };
> +


Best regards,
Krzysztof

[PATCH v1 6/9] dt-bindings: hwmon: Add HPE GXP PSU Support

[nick.hawkins]

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

Provide i2c register information and CPLD register information to the
driver.

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

diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
new file mode 100644
index 000000000000..60ca0f6ace46
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/hpe,gxp-psu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP psu controller
+
+maintainers:
+  - Nicholas Hawkins <nick.hawkins@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp-psu
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  hpe,sysreg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the global status registers shared between each psu
+      controller instance.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        psu@48 {
+            compatible = "hpe,gxp-psu";
+            reg = <0x48>;
+            hpe,sysreg = <&sysreg_system_controller2>;
+        };
+    };
-- 
2.17.1

Re: [PATCH v1 6/9] dt-bindings: hwmon: Add HPE GXP PSU Support

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide i2c register information and CPLD register information to the
> driver.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/hwmon/hpe,gxp-psu.yaml           | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> new file mode 100644
> index 000000000000..60ca0f6ace46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/hpe,gxp-psu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP psu controller
> +
> +maintainers:
> +  - Nicholas Hawkins <nick.hawkins@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-psu

Missing blank line.

> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1

All your bindings are written with different style... reg is second in
your previous patchset, so what order did you choose here?


> +
> +  hpe,sysreg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the global status registers shared between each psu
> +      controller instance.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line.

> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        psu@48 {
> +            compatible = "hpe,gxp-psu";
> +            reg = <0x48>;

Add also interrupts.

Best regards,
Krzysztof

Re: [PATCH v1 6/9] dt-bindings: hwmon: Add HPE GXP PSU Support

[Rob Herring]

On Tue, Apr 18, 2023 at 10:28:21AM -0500, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide i2c register information and CPLD register information to the
> driver.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/hwmon/hpe,gxp-psu.yaml           | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> new file mode 100644
> index 000000000000..60ca0f6ace46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/hpe,gxp-psu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP psu controller
> +
> +maintainers:
> +  - Nicholas Hawkins <nick.hawkins@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-psu
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  hpe,sysreg:

Why is this optional?

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the global status registers shared between each psu
> +      controller instance.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        psu@48 {
> +            compatible = "hpe,gxp-psu";
> +            reg = <0x48>;
> +            hpe,sysreg = <&sysreg_system_controller2>;
> +        };
> +    };
> -- 
> 2.17.1
> 

[PATCH v1 7/9] ARM: dts: gxp: add psu, i2c, gpio

[nick.hawkins]

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

Add support for the GXP I2C, PSU, and GPIO
drivers. As well as adjust register ranges to be
correct.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 161 ++++++++++++++++++
 arch/arm/boot/dts/hpe-gxp.dtsi           | 197 ++++++++++++++++++++---
 2 files changed, 338 insertions(+), 20 deletions(-)

diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
index 3a7382ce40ef..487b6485a832 100644
--- a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -23,4 +23,165 @@
 		device_type = "memory";
 		reg = <0x40000000 0x20000000>;
 	};
+
+	i2cmux@4 {
+		compatible = "i2c-mux-reg";
+		i2c-parent = <&i2c4>;
+		reg = <0xd1000374 0x1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@3 {
+			reg = <3>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@4 {
+			reg = <4>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+
+	i2cmux@6 {
+		compatible = "i2c-mux-reg";
+		i2c-parent = <&i2c6>;
+		reg = <0xd1000376 0x1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@2 {
+			reg = <2>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@3 {
+			reg = <3>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@4 {
+			reg = <4>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@5 {
+			reg = <5>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+};
+
+&i2c0 {
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+};
+
+&i2c2 {
+	status = "okay";
+	eeprom@50 {
+		compatible = "atmel,24c02";
+		pagesize = <8>;
+		reg = <0x50>;
+	};
+};
+
+&i2c3 {
+	status = "okay";
+};
+
+&i2c4 {
+	status = "okay";
+};
+
+&i2c5 {
+	status = "okay";
+};
+
+&i2c6 {
+	status = "okay";
+};
+
+&i2c7 {
+	status = "okay";
+	psu@58 {
+		compatible = "hpe,gxp-psu";
+		reg = <0x58>;
+		hpe,sysreg = <&sysreg_system_controller2>;
+	};
+
+	psu@59 {
+		compatible = "hpe,gxp-psu";
+		reg = <0x59>;
+		hpe,sysreg = <&sysreg_system_controller2>;
+	};
+};
+
+&i2c8 {
+	status = "okay";
+};
+
+&i2c9 {
+	status = "okay";
+};
+
+&gpio {
+	gpio-line-names =
+	"", "", "", "", "", "", "", "", "", "", /*0 - 9*/
+	"", "", "", "", "", "", "", "", "", "", /*10 - 19*/
+	"", "", "", "", "", "", "", "", "", "", /*20 - 29*/
+	"", "", "", "", "", "", "", "", "", "", /*30 - 39*/
+	"", "", "", "", "", "", "", "", "", "", /*40 - 49*/
+	"", "", "", "", "", "", "", "", "", "", /*50 - 59*/
+	"", "", "", "", "", "", "", "", "", "", /*60 - 69*/
+	"", "", "", "", "", "", "", "", "", "", /*70 - 79*/
+	"", "", "", "", "", "", "", "", "", "", /*80 - 89*/
+	"", "", "", "", "", "", "", "", "", "", /*90 - 99*/
+	"", "", "", "", "", "", "", "", "", "", /*100 - 109*/
+	"", "", "", "", "", "", "", "", "", "", /*110 - 119*/
+	"", "", "", "", "", "", "", "", "", "", /*120 - 129*/
+	"", "", "", "", "", "", "", "", "", "", /*130 - 139*/
+	"", "", "", "", "", "", "", "", "", "", /*140 - 149*/
+	"", "", "", "", "", "", "", "", "", "", /*150 - 159*/
+	"", "", "", "", "", "", "", "", "", "", /*160 - 169*/
+	"", "", "", "", "", "", "", "", "", "", /*170 - 179*/
+	"", "", "", "", "", "", "", "", "", "", /*180 - 189*/
+	"", "", "RESET_OUT", "NMI_OUT", "", "", "", "", "", "", /*190 - 199*//*GPIO*/
+	"", "", "", "", "", "", "", "", "", "", /*Vuhc 200-209*/
+	"POWER_OUT", "PS_PWROK", "PCIERST", "POST_COMPLETE", "", "", "", "", "", ""; /*210 - 219*/
+};
+
+&gpio1 {
+	gpio-line-names =
+	"IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7",
+	"IOP_LED8", "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST",
+	"FAN7_INST", "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL",
+	"FAN6_FAIL", "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID",
+	"FAN5_ID", "FAN6_ID", "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER",
+	"POWER_BUTTON", "UID_PRESS", "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5",
+	"SO_ON_CONTROL", "PSU1_INST", "PSU2_INST", "PSU3_INST", "PSU4_INST", "PSU5_INST",
+	"PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC", "PSU2_AC", "PSU3_AC", "PSU4_AC",
+	"PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC", "PSU2_DC", "PSU3_DC", "PSU4_DC",
+	"PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
+	"", "", "", "", "", "", "", "", "", "";
 };
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
index cf735b3c4f35..8a8faf7fbd60 100644
--- a/arch/arm/boot/dts/hpe-gxp.dtsi
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Device Tree file for HPE GXP
+ * Device Tree for HPE GXP
  */
 
 /dts-v1/;
@@ -52,76 +52,233 @@
 			cache-level = <2>;
 		};
 
-		ahb@c0000000 {
+		ahb@80000000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
 			#size-cells = <1>;
-			ranges = <0x0 0xc0000000 0x30000000>;
-			dma-ranges;
+			ranges = <0x0 0x80000000 0x20000000>,
+			<0x40000000 0xc0000000 0x3fff0000>;
 
-			vic0: interrupt-controller@eff0000 {
+			vic0: interrupt-controller@4eff0000 {
 				compatible = "arm,pl192-vic";
-				reg = <0xeff0000 0x1000>;
+				reg = <0x4eff0000 0x1000>;
 				interrupt-controller;
 				#interrupt-cells = <1>;
 			};
 
-			vic1: interrupt-controller@80f00000 {
+			vic1: interrupt-controller@f00000 {
 				compatible = "arm,pl192-vic";
-				reg = <0x80f00000 0x1000>;
+				reg = <0xf00000 0x1000>;
 				interrupt-controller;
 				#interrupt-cells = <1>;
 			};
 
-			uarta: serial@e0 {
+			uarta: serial@400000e0 {
 				compatible = "ns16550a";
-				reg = <0xe0 0x8>;
+				reg = <0x400000e0 0x8>;
 				interrupts = <17>;
 				interrupt-parent = <&vic0>;
 				clock-frequency = <1846153>;
 				reg-shift = <0>;
 			};
 
-			uartb: serial@e8 {
+			uartb: serial@400000e8 {
 				compatible = "ns16550a";
-				reg = <0xe8 0x8>;
+				reg = <0x400000e8 0x8>;
 				interrupts = <18>;
 				interrupt-parent = <&vic0>;
 				clock-frequency = <1846153>;
 				reg-shift = <0>;
 			};
 
-			uartc: serial@f0 {
+			uartc: serial@400000f0 {
 				compatible = "ns16550a";
-				reg = <0xf0 0x8>;
+				reg = <0x400000f0 0x8>;
 				interrupts = <19>;
 				interrupt-parent = <&vic0>;
 				clock-frequency = <1846153>;
 				reg-shift = <0>;
 			};
 
-			usb0: usb@efe0000 {
+			usb0: usb@4efe0000 {
 				compatible = "hpe,gxp-ehci", "generic-ehci";
-				reg = <0xefe0000 0x100>;
+				reg = <0x4efe0000 0x100>;
 				interrupts = <7>;
 				interrupt-parent = <&vic0>;
 			};
 
-			st: timer@80 {
+			st: timer@40000080 {
 				compatible = "hpe,gxp-timer";
-				reg = <0x80 0x16>;
+				reg = <0x40000080 0x16>;
 				interrupts = <0>;
 				interrupt-parent = <&vic0>;
 				clocks = <&iopclk>;
 				clock-names = "iop";
 			};
 
-			usb1: usb@efe0100 {
+			usb1: usb@4efe0100 {
 				compatible = "hpe,gxp-ohci", "generic-ohci";
-				reg = <0xefe0100 0x110>;
+				reg = <0x4efe0100 0x110>;
 				interrupts = <6>;
 				interrupt-parent = <&vic0>;
 			};
+
+			sysreg_system_controller: syscon@400000f8 {
+				compatible = "hpe,gxp-sysreg", "syscon";
+				reg = <0x400000f8 0x8>;
+			};
+
+			sysreg_system_controller2: syscon@51000319 {
+				compatible = "hpe,gxp-sysreg", "syscon";
+				reg = <0x51000319 0x4>;
+			};
+
+			i2c0: i2c@40002000 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002000 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c1: i2c@40002100 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002100 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c2: i2c@40002200 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002200 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c3: i2c@40002300 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002300 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c4: i2c@40002400 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002400 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c5: i2c@40002500 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002500 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c6: i2c@40002600 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002600 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c7: i2c@40002700 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002700 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c8: i2c@40002800 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002800 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			i2c9: i2c@40002900 {
+				compatible = "hpe,gxp-i2c";
+				reg = <0x40002900 0x70>;
+				interrupts = <9>;
+				interrupt-parent = <&vic0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+				hpe,sysreg = <&sysreg_system_controller>;
+				clock-frequency = <100000>;
+			};
+
+			fan-controller@40000c10 { /* 0xc0000c10 */
+				compatible = "hpe,gxp-fan-ctrl";
+				reg = <0x40000c10 0x8>, <0x51000327 0x06>;
+				reg-names = "base", "pl";
+			};
+
+			gpio: gpio@0 {
+				compatible = "hpe,gxp-gpio";
+				reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>,
+				<0x400064 0x80>, <0x5100030f 0x1>;
+				reg-names = "csm", "fn2-vbtn", "fn2-stat", "vuhc", "vbtn";
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <0>;
+				interrupt-parent = <&vic1>;
+			};
+
+			gpio1: gpio@51000304 {
+				compatible = "hpe,gxp-gpio-pl";
+				reg = <0x51000304 0x2>, <0x5100030d 0x01>, <0x51000380 0x7f>;
+				reg-names = "pl-led", "pl-health", "pl-int";
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <26>;
+				interrupt-parent = <&vic0>;
+			};
 		};
 	};
 };
-- 
2.17.1

Re: [PATCH v1 7/9] ARM: dts: gxp: add psu, i2c, gpio

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Add support for the GXP I2C, PSU, and GPIO
> drivers. As well as adjust register ranges to be
> correct.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 161 ++++++++++++++++++
>  arch/arm/boot/dts/hpe-gxp.dtsi           | 197 ++++++++++++++++++++---
>  2 files changed, 338 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> index 3a7382ce40ef..487b6485a832 100644
> --- a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> @@ -23,4 +23,165 @@
>  		device_type = "memory";
>  		reg = <0x40000000 0x20000000>;
>  	};
> +
> +	i2cmux@4 {
> +		compatible = "i2c-mux-reg";
> +		i2c-parent = <&i2c4>;
> +		reg = <0xd1000374 0x1>;

Keep reg as second property. Run dtbs_check W=1, doesn't it scream about
mistake in unit address?

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@3 {
> +			reg = <3>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@4 {
> +			reg = <4>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};
> +
> +	i2cmux@6 {
> +		compatible = "i2c-mux-reg";
> +		i2c-parent = <&i2c6>;
> +		reg = <0xd1000376 0x1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@2 {
> +			reg = <2>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@3 {
> +			reg = <3>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@4 {
> +			reg = <4>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		i2c@5 {
> +			reg = <5>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +	};
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +};
> +
> +&i2c2 {
> +	status = "okay";
> +	eeprom@50 {
> +		compatible = "atmel,24c02";
> +		pagesize = <8>;
> +		reg = <0x50>;

Keep reg as second property. In other places you have it correctly.


> +	};
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +};
> +
> +&i2c4 {
> +	status = "okay";
> +};
> +
> +&i2c5 {
> +	status = "okay";
> +};
> +
> +&i2c6 {
> +	status = "okay";
> +};
> +
> +&i2c7 {
> +	status = "okay";
> +	psu@58 {
> +		compatible = "hpe,gxp-psu";
> +		reg = <0x58>;
> +		hpe,sysreg = <&sysreg_system_controller2>;
> +	};
> +
> +	psu@59 {
> +		compatible = "hpe,gxp-psu";
> +		reg = <0x59>;
> +		hpe,sysreg = <&sysreg_system_controller2>;
> +	};
> +};
> +
> +&i2c8 {
> +	status = "okay";
> +};
> +
> +&i2c9 {
> +	status = "okay";
> +};
> +
> +&gpio {
> +	gpio-line-names =
> +	"", "", "", "", "", "", "", "", "", "", /*0 - 9*/

That's very odd indentation. Usually it is one of:

gpio-line-names = "foo", "bar"
                  "baz", "zab";


gpio-line-names =
	"foo", "bar"
	"baz", "zab";

Where first one is preferred.


> +	"", "", "", "", "", "", "", "", "", "", /*10 - 19*/
> +	"", "", "", "", "", "", "", "", "", "", /*20 - 29*/
> +	"", "", "", "", "", "", "", "", "", "", /*30 - 39*/
> +	"", "", "", "", "", "", "", "", "", "", /*40 - 49*/
> +	"", "", "", "", "", "", "", "", "", "", /*50 - 59*/
> +	"", "", "", "", "", "", "", "", "", "", /*60 - 69*/
> +	"", "", "", "", "", "", "", "", "", "", /*70 - 79*/
> +	"", "", "", "", "", "", "", "", "", "", /*80 - 89*/
> +	"", "", "", "", "", "", "", "", "", "", /*90 - 99*/
> +	"", "", "", "", "", "", "", "", "", "", /*100 - 109*/
> +	"", "", "", "", "", "", "", "", "", "", /*110 - 119*/
> +	"", "", "", "", "", "", "", "", "", "", /*120 - 129*/
> +	"", "", "", "", "", "", "", "", "", "", /*130 - 139*/
> +	"", "", "", "", "", "", "", "", "", "", /*140 - 149*/
> +	"", "", "", "", "", "", "", "", "", "", /*150 - 159*/
> +	"", "", "", "", "", "", "", "", "", "", /*160 - 169*/
> +	"", "", "", "", "", "", "", "", "", "", /*170 - 179*/
> +	"", "", "", "", "", "", "", "", "", "", /*180 - 189*/
> +	"", "", "RESET_OUT", "NMI_OUT", "", "", "", "", "", "", /*190 - 199*//*GPIO*/
> +	"", "", "", "", "", "", "", "", "", "", /*Vuhc 200-209*/
> +	"POWER_OUT", "PS_PWROK", "PCIERST", "POST_COMPLETE", "", "", "", "", "", ""; /*210 - 219*/
> +};
> +
> +&gpio1 {
> +	gpio-line-names =
> +	"IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7",
> +	"IOP_LED8", "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST",
> +	"FAN7_INST", "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL",
> +	"FAN6_FAIL", "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID",
> +	"FAN5_ID", "FAN6_ID", "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER",
> +	"POWER_BUTTON", "UID_PRESS", "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5",
> +	"SO_ON_CONTROL", "PSU1_INST", "PSU2_INST", "PSU3_INST", "PSU4_INST", "PSU5_INST",
> +	"PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC", "PSU2_AC", "PSU3_AC", "PSU4_AC",
> +	"PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC", "PSU2_DC", "PSU3_DC", "PSU4_DC",
> +	"PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
> +	"", "", "", "", "", "", "", "", "", "";
>  };
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> index cf735b3c4f35..8a8faf7fbd60 100644
> --- a/arch/arm/boot/dts/hpe-gxp.dtsi
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Device Tree file for HPE GXP
> + * Device Tree for HPE GXP

Not really related to this change,

>   */
>  
>  /dts-v1/;
> @@ -52,76 +52,233 @@
>  			cache-level = <2>;
>  		};
>  
> -		ahb@c0000000 {
> +		ahb@80000000 {
>  			compatible = "simple-bus";
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> -			ranges = <0x0 0xc0000000 0x30000000>;
> -			dma-ranges;
> +			ranges = <0x0 0x80000000 0x20000000>,
> +			<0x40000000 0xc0000000 0x3fff0000>;

Wrong indentation.

>  
> -			vic0: interrupt-controller@eff0000 {
> +			vic0: interrupt-controller@4eff0000 {

You need to better organize your changes and split some refactorings
from new devices. I don't understand why eff0000 becomes 4eff0000 -
whether this is a bug being fixed, incorrect design etc. Commit msg just
says "to be correct", so this is was a bug. Bugfixes cannot mixed with
new features/code/refactorings. Anyway this is very vague. Explain what
is not correct, why it has to be fixed.

>  				compatible = "arm,pl192-vic";
> -				reg = <0xeff0000 0x1000>;
> +				reg = <0x4eff0000 0x1000>;
>  				interrupt-controller;
>  				#interrupt-cells = <1>;
>  			};
>  
> -			vic1: interrupt-controller@80f00000 {
> +			vic1: interrupt-controller@f00000 {
>  				compatible = "arm,pl192-vic";
> -				reg = <0x80f00000 0x1000>;
> +				reg = <0xf00000 0x1000>;
>  				interrupt-controller;
>  				#interrupt-cells = <1>;
>  			};
>  
> -			uarta: serial@e0 {
> +			uarta: serial@400000e0 {
>  				compatible = "ns16550a";
> -				reg = <0xe0 0x8>;
> +				reg = <0x400000e0 0x8>;
>  				interrupts = <17>;
>  				interrupt-parent = <&vic0>;
>  				clock-frequency = <1846153>;
>  				reg-shift = <0>;
>  			};
>  
> -			uartb: serial@e8 {
> +			uartb: serial@400000e8 {
>  				compatible = "ns16550a";
> -				reg = <0xe8 0x8>;
> +				reg = <0x400000e8 0x8>;
>  				interrupts = <18>;
>  				interrupt-parent = <&vic0>;
>  				clock-frequency = <1846153>;
>  				reg-shift = <0>;
>  			};
>  
> -			uartc: serial@f0 {
> +			uartc: serial@400000f0 {
>  				compatible = "ns16550a";
> -				reg = <0xf0 0x8>;
> +				reg = <0x400000f0 0x8>;
>  				interrupts = <19>;
>  				interrupt-parent = <&vic0>;
>  				clock-frequency = <1846153>;
>  				reg-shift = <0>;
>  			};
>  
> -			usb0: usb@efe0000 {
> +			usb0: usb@4efe0000 {
>  				compatible = "hpe,gxp-ehci", "generic-ehci";
> -				reg = <0xefe0000 0x100>;
> +				reg = <0x4efe0000 0x100>;
>  				interrupts = <7>;
>  				interrupt-parent = <&vic0>;
>  			};
>  
> -			st: timer@80 {
> +			st: timer@40000080 {
>  				compatible = "hpe,gxp-timer";
> -				reg = <0x80 0x16>;
> +				reg = <0x40000080 0x16>;
>  				interrupts = <0>;
>  				interrupt-parent = <&vic0>;
>  				clocks = <&iopclk>;
>  				clock-names = "iop";
>  			};
>  
> -			usb1: usb@efe0100 {
> +			usb1: usb@4efe0100 {
>  				compatible = "hpe,gxp-ohci", "generic-ohci";
> -				reg = <0xefe0100 0x110>;
> +				reg = <0x4efe0100 0x110>;
>  				interrupts = <6>;
>  				interrupt-parent = <&vic0>;
>  			};
> +
> +			sysreg_system_controller: syscon@400000f8 {
> +				compatible = "hpe,gxp-sysreg", "syscon";
> +				reg = <0x400000f8 0x8>;
> +			};
> +
> +			sysreg_system_controller2: syscon@51000319 {
> +				compatible = "hpe,gxp-sysreg", "syscon";
> +				reg = <0x51000319 0x4>;
> +			};
> +
> +			i2c0: i2c@40002000 {
> +				compatible = "hpe,gxp-i2c";
> +				reg = <0x40002000 0x70>;
> +				interrupts = <9>;
> +				interrupt-parent = <&vic0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				status = "disabled";

Status is last property.

> +				hpe,sysreg = <&sysreg_system_controller>;
> +				clock-frequency = <100000>;
> +			};


(...)
> +
> +			fan-controller@40000c10 { /* 0xc0000c10 */
> +				compatible = "hpe,gxp-fan-ctrl";
> +				reg = <0x40000c10 0x8>, <0x51000327 0x06>;
> +				reg-names = "base", "pl";
> +			};
> +
> +			gpio: gpio@0 {
> +				compatible = "hpe,gxp-gpio";
> +				reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>,
> +				<0x400064 0x80>, <0x5100030f 0x1>;

This looks randomly indented...



Best regards,
Krzysztof

RE: [PATCH v1 7/9] ARM: dts: gxp: add psu, i2c, gpio

[Hawkins, Nick]

> You need to better organize your changes and split some refactorings
> from new devices. I don't understand why eff0000 becomes 4eff0000 -
> whether this is a bug being fixed, incorrect design etc. Commit msg just
> says "to be correct", so this is was a bug. Bugfixes cannot mixed with
> new features/code/refactorings. Anyway this is very vague. Explain what
> is not correct, why it has to be fixed.

Thank you for all of the feedback you have provided Krzysztof,

It indeed is a bug that was introduced early on. I attempted
previously to correct this issue separately here:

https://lore.kernel.org/all/20230130220056.14349-2-nick.hawkins@hpe.com/

I see though that I have made some of the mistakes you mentioned above.
I will resubmit.

> > +			gpio: gpio@0 {
> > +				compatible = "hpe,gxp-gpio";
> > +				reg = <0x0 0x400>, <0x200046 0x1>, <0x200070 0x08>,
> > +				<0x400064 0x80>, <0x5100030f 0x1>;

> This looks randomly indented...

Although the design is likely to change I will use a format similar to
gpio-line-names you mentioned previously.

Thanks,

-Nick Hawkins

[PATCH v1 8/9] ARM: multi_v7_defconfig: Add PSU, GPIO, and I2C

[nick.hawkins]

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

Add the CONFIG_I2C_GXP, CONFIG_GPIO_GXP, and CONFIG_SENSORS_GXP_PSU
symbols. Make CONFIG_SENSORS_GXP_FAN_CTRL=y

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

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 084cc612ea23..fcfbcd233fb8 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -405,6 +405,7 @@ CONFIG_I2C_DAVINCI=y
 CONFIG_I2C_DESIGNWARE_PLATFORM=y
 CONFIG_I2C_DIGICOLOR=m
 CONFIG_I2C_EMEV2=m
+CONFIG_I2C_GXP=m
 CONFIG_I2C_IMX=y
 CONFIG_I2C_MESON=y
 CONFIG_I2C_MV64XXX=y
@@ -478,6 +479,7 @@ CONFIG_GPIO_ASPEED_SGPIO=y
 CONFIG_GPIO_DAVINCI=y
 CONFIG_GPIO_DWAPB=y
 CONFIG_GPIO_EM=y
+CONFIG_GPIO_GXP=y
 CONFIG_GPIO_MPC8XXX=y
 CONFIG_GPIO_MXC=y
 CONFIG_GPIO_RCAR=y
@@ -527,7 +529,8 @@ CONFIG_SENSORS_NTC_THERMISTOR=m
 CONFIG_SENSORS_PWM_FAN=m
 CONFIG_SENSORS_RASPBERRYPI_HWMON=m
 CONFIG_SENSORS_INA2XX=m
-CONFIG_SENSORS_GXP_FAN_CTRL=m
+CONFIG_SENSORS_GXP_FAN_CTRL=y
+CONFIG_SENSORS_GXP_PSU=y
 CONFIG_CPU_THERMAL=y
 CONFIG_DEVFREQ_THERMAL=y
 CONFIG_IMX_THERMAL=y
-- 
2.17.1

Re: [PATCH v1 8/9] ARM: multi_v7_defconfig: Add PSU, GPIO, and I2C

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Add the CONFIG_I2C_GXP, CONFIG_GPIO_GXP, and CONFIG_SENSORS_GXP_PSU

Why?


> symbols. Make CONFIG_SENSORS_GXP_FAN_CTRL=y

Why?

Please briefly provide rationale in the commit msg.

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/configs/multi_v7_defconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index 084cc612ea23..fcfbcd233fb8 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -405,6 +405,7 @@ CONFIG_I2C_DAVINCI=y
>  CONFIG_I2C_DESIGNWARE_PLATFORM=y
>  CONFIG_I2C_DIGICOLOR=m
>  CONFIG_I2C_EMEV2=m
> +CONFIG_I2C_GXP=m
>  CONFIG_I2C_IMX=y
>  CONFIG_I2C_MESON=y
>  CONFIG_I2C_MV64XXX=y
> @@ -478,6 +479,7 @@ CONFIG_GPIO_ASPEED_SGPIO=y
>  CONFIG_GPIO_DAVINCI=y
>  CONFIG_GPIO_DWAPB=y
>  CONFIG_GPIO_EM=y
> +CONFIG_GPIO_GXP=y
>  CONFIG_GPIO_MPC8XXX=y
>  CONFIG_GPIO_MXC=y
>  CONFIG_GPIO_RCAR=y
> @@ -527,7 +529,8 @@ CONFIG_SENSORS_NTC_THERMISTOR=m
>  CONFIG_SENSORS_PWM_FAN=m
>  CONFIG_SENSORS_RASPBERRYPI_HWMON=m
>  CONFIG_SENSORS_INA2XX=m
> -CONFIG_SENSORS_GXP_FAN_CTRL=m
> +CONFIG_SENSORS_GXP_FAN_CTRL=y

No, we want it to be module.

> +CONFIG_SENSORS_GXP_PSU=y

Same here.

>  CONFIG_CPU_THERMAL=y
>  CONFIG_DEVFREQ_THERMAL=y
>  CONFIG_IMX_THERMAL=y

Best regards,
Krzysztof

[PATCH v1 9/9] MAINTAINERS: hpe: Add GPIO, PSU

[nick.hawkins]

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

List the files added for GPIO and PSU support.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index a3b14ec33830..6df959ebf523 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2239,7 +2239,9 @@ M:	Nick Hawkins <nick.hawkins@hpe.com>
 S:	Maintained
 F:	Documentation/hwmon/gxp-fan-ctrl.rst
 F:	Documentation/devicetree/bindings/arm/hpe,gxp.yaml
+F:	Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
 F:	Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
+F:	Documentation/devicetree/bindings/hwmon/hpe,gxp-psu.yaml
 F:	Documentation/devicetree/bindings/i2c/hpe,gxp-i2c.yaml
 F:	Documentation/devicetree/bindings/spi/hpe,gxp-spifi.yaml
 F:	Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
@@ -2247,7 +2249,9 @@ 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/gpio/gpio-gxp.c
 F:	drivers/hwmon/gxp-fan-ctrl.c
+F:	drivers/hwmon/gxp-psu.c
 F:	drivers/i2c/busses/i2c-gxp.c
 F:	drivers/spi/spi-gxp.c
 F:	drivers/watchdog/gxp-wdt.c
-- 
2.17.1

Re: [PATCH v1 9/9] MAINTAINERS: hpe: Add GPIO, PSU

[Krzysztof Kozlowski]

On 18/04/2023 17:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> List the files added for GPIO and PSU support.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  MAINTAINERS | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a3b14ec33830..6df959ebf523 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2239,7 +2239,9 @@ M:	Nick Hawkins <nick.hawkins@hpe.com>
>  S:	Maintained
>  F:	Documentation/hwmon/gxp-fan-ctrl.rst
>  F:	Documentation/devicetree/bindings/arm/hpe,gxp.yaml
> +F:	Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml

Since the drivers are going through subsystems, your patchset is not
bisectable.

Squash respective changes with respective patches.

I also suggest do not mix three different subsystems - GPIO, hwmon and
ARM SoC - in one patchset.

Best regards,
Krzysztof