[RFC 0/4] Add support for the Gateworks System Controller

[RFC 0/4] Add support for the Gateworks System Controller

[Tim Harvey]

This series adds support for the Gateworks System Controller used on Gateworks
Laguna, Ventana, and Newport product families.

The GSC is an MSP430 I2C slave controller whose firmware embeds the following
features:
 - I/O expander (16 GPIO's emulating a PCA955x)
 - EEPROM (enumating AT24)
 - RTC (enumating DS1672)
 - HWMON
 - Interrupt controller with tamper detect, user pushbotton
 - Watchdog controller capable of full board power-cycle
 - Power Control capable of full board power-cycle

see http://trac.gateworks.com/wiki/gsc for more details

Tim Harvey (4):
  dt-bindings: mfd: Add Gateworks System Controller bindings
  mfd: add Gateworks System Controller core driver
  hwmon: add Gateworks System Controller support
  input: misc: Add Gateworks System Controller support

 Documentation/devicetree/bindings/mfd/gsc.txt |  69 ++++++
 drivers/hwmon/Kconfig                         |   6 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/gsc-hwmon.c                     | 299 +++++++++++++++++++++++
 drivers/input/misc/Kconfig                    |   6 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/gsc-input.c                | 196 +++++++++++++++
 drivers/mfd/Kconfig                           |  10 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/gsc.c                             | 330 ++++++++++++++++++++++++++
 include/linux/mfd/gsc.h                       |  79 ++++++
 11 files changed, 998 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
 create mode 100644 drivers/hwmon/gsc-hwmon.c
 create mode 100644 drivers/input/misc/gsc-input.c
 create mode 100644 drivers/mfd/gsc.c
 create mode 100644 include/linux/mfd/gsc.h

-- 
2.7.4

[RFC 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings

[Tim Harvey]

This patch adds documentation of device-tree bindings for the
Gateworks System Controller (GSC).

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 Documentation/devicetree/bindings/mfd/gsc.txt | 69 +++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt

diff --git a/Documentation/devicetree/bindings/mfd/gsc.txt b/Documentation/devicetree/bindings/mfd/gsc.txt
new file mode 100644
index 0000000..7671347
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/gsc.txt
@@ -0,0 +1,159 @@
+Gateworks System Controller multi-function device
+
+The GSC is a Multifunction I2C slave device with the following submodules:
+- WDT
+- GPIO
+- Pushbutton controller
+- HWMON
+
+Required properties:
+- compatible : Must be "gw,gsc_v1", "gw,gsc_v2", "gw,gsc_v3"
+- reg: I2C address of the device
+- interrupts: interrupt triggered by GSC_IRQ# signal
+- interrupt-parent: Interrupt controller GSC is connected to
+- #interrupt-cells: should be <1>, index of the interrupt within the
+  controller, in accordance with the "one cell" variant of
+  <devicetree/bindings/interrupt-controller/interrupt.txt>
+
+Optional nodes:
+* watchdog:
+The GSC provides a Watchdog monitor which can power cycle the board's
+primary power supply on most board models when tripped.
+
+Required properties:
+- compatible: must be "gw,gsc-watchdog"
+
+* input:
+The GSC provides an input device capable of dispatching Linux Input events
+for user pushbutton events, tamper switch events, etc.
+
+Required properties:
+- compatible: must be "gw,gsc-input"
+
+* hwmon:
+The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
+temperature and/or voltage monitoring.
+
+Required properties:
+- compatible: must be "gw,gsc-hwmon"
+
+Example:
+
+	gsc: gsc@20 {
+		compatible = "gw,gsc_v2";
+		reg = <0x20>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <4 GPIO_ACTIVE_LOW>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		gsc_input {
+			compatible = "gw,gsc-input";
+		};
+
+		gsc_watchdog {
+			compatible = "gw,gsc-watchdog";
+		};
+
+		gsc_hwmon {
+			compatible = "gw,gsc-hwmon";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hwmon@0 { /* A0: Board Temperature */
+				type = <0>;
+				reg = <0x00>;
+				label = "temp";
+			};
+
+			hwmon@1 { /* A1: Input Voltage */
+				type = <1>;
+				reg = <0x02>;
+				label = "Vin";
+			};
+
+			hwmon@2 { /* A2: 5P0 */
+				type = <1>;
+				reg = <0x0b>;
+				label = "5P0";
+			};
+
+			hwmon@4 { /* A4: 0-5V input */
+				type = <1>;
+				reg = <0x14>;
+				label = "ANL0";
+			};
+
+			hwmon@5 { /* A5: 2P5 PCIe/GigE */
+				type = <1>;
+				reg = <0x23>;
+				label = "2P5";
+			};
+
+			hwmon@6 { /* A6: 1P8 Aud/Vid */
+				type = <1>;
+				reg = <0x1d>;
+				label = "1P8";
+			};
+
+			hwmon@7 { /* A7: GPS */
+				type = <1>;
+				reg = <0x26>;
+				label = "GPS";
+			};
+
+			hwmon@12 { /* A12: VDD_CORE */
+				type = <1>;
+				reg = <0x3>;
+				label = "VDD_CORE";
+			};
+
+			hwmon@13 { /* A13: VDD_SOC */
+				type = <1>;
+				reg = <0x11>;
+				label = "VDD_SOC";
+			};
+
+			hwmon@14 { /* A14: 1P0 PCIe SW */
+				type = <1>;
+				reg = <0x20>;
+				label = "1P0";
+			};
+
+			hwmon@15 { /* fan0 */
+				type = <2>;
+				reg = <0x2c>;
+				label = "fan_50p";
+			};
+
+			hwmon@16 { /* fan1 */
+				type = <2>;
+				reg = <0x2e>;
+				label = "fan_60p";
+			};
+
+			hwmon@17 { /* fan2 */
+				type = <2>;
+				reg = <0x30>;
+				label = "fan_70p";
+			};
+
+			hwmon@18 { /* fan3 */
+				type = <2>;
+				reg = <0x32>;
+				label = "fan_80p";
+			};
+
+			hwmon@19 { /* fan4 */
+				type = <2>;
+				reg = <0x34>;
+				label = "fan_90p";
+			};
+
+			hwmon@20 { /* fan5 */
+				type = <2>;
+				reg = <0x36>;
+				label = "fan_100p";
+			};
+		};
+	};
-- 
2.7.4

[RFC 2/4] mfd: add Gateworks System Controller core driver

[Tim Harvey]

The Gateworks System Controller (GSC) is an I2C slave controller
implemented with an MSP430 micro-controller whose firmware embeds the
following features:
 - I/O expander (16 GPIO's) using PCA955x protocol
 - Real Time Clock using DS1672 protocol
 - User EEPROM using AT24 protocol
 - HWMON using custom protocol
 - Interrupt controller with tamper detect, user pushbotton
 - Watchdog controller capable of full board power-cycle
 - Power Control capable of full board power-cycle

see http://trac.gateworks.com/wiki/gsc for more details

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/mfd/Kconfig     |  10 ++
 drivers/mfd/Makefile    |   1 +
 drivers/mfd/gsc.c       | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/gsc.h |  79 ++++++++++++
 4 files changed, 420 insertions(+)
 create mode 100644 drivers/mfd/gsc.c
 create mode 100644 include/linux/mfd/gsc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1d20a80..16dd486 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
 	  Select this option to enable support for Samsung Exynos Low Power
 	  Audio Subsystem.
 
+config MFD_GSC
+	tristate "Gateworks System Controller"
+	depends on (I2C && OF) || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	help
+	  Enable support for the Gateworks System Controller found
+	  on Gateworks Single Board Computers.
+
 config MFD_MC13XXX
 	tristate
 	depends on (SPI_MASTER || I2C)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index d9474ad..aede0bc 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec_core.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
+obj-$(CONFIG_MFD_GSC)		+= gsc.o
 
 rtsx_pci-objs			:= rtsx_pcr.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
 obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
diff --git a/drivers/mfd/gsc.c b/drivers/mfd/gsc.c
new file mode 100644
index 0000000..2fe4174
--- /dev/null
+++ b/drivers/mfd/gsc.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ *
+ * The Gateworks System Controller (GSC) is a family of a multi-function
+ * "Power Management and System Companion Device" chips originally designed for
+ * use in Gateworks Single Board Computers. The control interface is I2C,
+ * at 100kbps, with an interrupt.
+ *
+ */
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/gsc.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/*
+ * The GSC suffers from an errata where occasionally during
+ * ADC cycles the chip can NAK i2c transactions. To ensure we have reliable
+ * register access we place retries around register access.
+ */
+#define I2C_RETRIES	3
+
+static int gsc_regmap_regwrite(void *context, unsigned int reg,
+			       unsigned int val)
+{
+	struct i2c_client *client = context;
+	int retry, ret;
+
+	for (retry = 0; retry < I2C_RETRIES; retry++) {
+		ret = i2c_smbus_write_byte_data(client, reg, val);
+		/*
+		 * -EAGAIN returned when the i2c host controller is busy
+		 * -EIO returned when i2c device is busy
+		 */
+		if (ret != -EAGAIN && ret != -EIO)
+			break;
+	}
+	if (ret < 0) {
+		dev_err(&client->dev, ">> 0x%02x %d\n", reg, ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, ">> 0x%02x=0x%02x (%d)\n", reg, val, retry);
+
+        return 0;
+}
+
+static int gsc_regmap_regread(void *context, unsigned int reg,
+                                  unsigned int *val)
+{
+	struct i2c_client *client = context;
+	int retry, ret;
+
+	for (retry = 0; retry < I2C_RETRIES; retry++) {
+		ret = i2c_smbus_read_byte_data(client, reg);
+		/*
+		 * -EAGAIN returned when the i2c host controller is busy
+		 * -EIO returned when i2c device is busy
+		 */
+		if (ret != -EAGAIN && ret != -EIO)
+			break;
+	}
+	if (ret < 0) {
+		dev_err(&client->dev, "<< 0x%02x %d\n", reg, ret);
+		return ret;
+	}
+
+	*val = ret & 0xff;
+	dev_dbg(&client->dev, "<< 0x%02x=0x%02x (%d)\n", reg, *val, retry);
+
+        return 0;
+}
+
+static struct regmap_bus regmap_gsc = {
+        .reg_write = gsc_regmap_regwrite,
+        .reg_read = gsc_regmap_regread,
+};
+
+/*
+ * gsc_powerdown - API to use GSC to power down board for a specific time
+ *
+ * secs - number of seconds to remain powered off
+ */
+static int gsc_powerdown(struct gsc_dev *gsc, unsigned long secs)
+{
+	int ret;
+	unsigned char regs[4];
+
+	dev_info(&gsc->i2c->dev, "GSC powerdown for %ld seconds\n",
+		 secs);
+	regs[0] = secs & 0xff;
+	regs[1] = (secs >> 8) & 0xff;
+	regs[2] = (secs >> 16) & 0xff;
+	regs[3] = (secs >> 24) & 0xff;
+	ret = regmap_bulk_write(gsc->regmap, GSC_TIME_ADD, regs, 4);
+
+	return ret;
+}
+
+/*
+ * sysfs hooks
+ */
+static ssize_t gsc_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(dev);
+	const char *name = attr->attr.name;
+	int rz = 0;
+
+	if (strcasecmp(name, "fw_version") == 0)
+		rz = sprintf(buf, "%d\n", gsc->fwver);
+	else if (strcasecmp(name, "fw_crc") == 0)
+		rz = sprintf(buf, "0x%04x\n", gsc->fwcrc);
+
+	return rz;
+}
+
+static ssize_t gsc_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(dev);
+	const char *name = attr->attr.name;
+	int ret;
+
+	if (strcasecmp(name, "powerdown") == 0) {
+		long value;
+
+		ret = kstrtol(buf, 0, &value);
+		if (ret == 0)
+			gsc_powerdown(gsc, value);
+	} else
+		printk(KERN_ERR "invalid name '%s\n", name);
+
+	return count;
+}
+
+
+/*
+ * Create a group of attributes so that we can create and destroy them all
+ * at once.
+ */
+static struct device_attribute attr_fwver =
+	__ATTR(fw_version, 0440, gsc_show, NULL);
+static struct device_attribute attr_fwcrc =
+	__ATTR(fw_crc, 0440, gsc_show, NULL);
+static struct device_attribute attr_pwrdown =
+	__ATTR(powerdown, 0220, NULL, gsc_store);
+
+static struct attribute *gsc_attrs[] = {
+	&attr_fwver.attr,
+	&attr_fwcrc.attr,
+	&attr_pwrdown.attr,
+	NULL,
+};
+
+static struct attribute_group attr_group = {
+	.attrs = gsc_attrs,
+};
+
+static const struct i2c_device_id gsc_i2c_ids[] = {
+	{ "gsc_v1", gsc_v1 },
+	{ "gsc_v2", gsc_v2 },
+	{ "gsc_v3", gsc_v3 },
+	{ },
+};
+
+static const struct of_device_id gsc_of_match[] = {
+	{ .compatible = "gw,gsc_v1", .data = (void *)gsc_v1 },
+	{ .compatible = "gw,gsc_v2", .data = (void *)gsc_v2 },
+	{ .compatible = "gw,gsc_v3", .data = (void *)gsc_v3 },
+	{ }
+};
+
+static const struct regmap_config gsc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_NONE,
+	.max_register = 0xf,
+};
+
+static const struct regmap_config gsc_regmap_hwmon_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_NONE,
+	.max_register = 0x37,
+};
+
+static const struct regmap_irq gsc_irqs[] = {
+	REGMAP_IRQ_REG(GSC_IRQ_PB, 0, BIT(GSC_IRQ_PB)),
+	REGMAP_IRQ_REG(GSC_IRQ_KEY_ERASED, 0, BIT(GSC_IRQ_KEY_ERASED)),
+	REGMAP_IRQ_REG(GSC_IRQ_EEPROM_WP, 0, BIT(GSC_IRQ_EEPROM_WP)),
+	REGMAP_IRQ_REG(GSC_IRQ_RESV, 0, BIT(GSC_IRQ_RESV)),
+	REGMAP_IRQ_REG(GSC_IRQ_GPIO, 0, BIT(GSC_IRQ_GPIO)),
+	REGMAP_IRQ_REG(GSC_IRQ_TAMPER, 0, BIT(GSC_IRQ_TAMPER)),
+	REGMAP_IRQ_REG(GSC_IRQ_WDT_TIMEOUT, 0, BIT(GSC_IRQ_WDT_TIMEOUT)),
+	REGMAP_IRQ_REG(GSC_IRQ_SWITCH_HOLD, 0, BIT(GSC_IRQ_SWITCH_HOLD)),
+};
+
+static const struct regmap_irq_chip gsc_irq_chip = {
+	.name = KBUILD_MODNAME,
+	.irqs = gsc_irqs,
+	.num_irqs = ARRAY_SIZE(gsc_irqs),
+	.num_regs = 1,
+	.status_base = GSC_IRQ_STATUS,
+	.mask_base = GSC_IRQ_ENABLE,
+	.mask_invert = true,
+	.ack_base = GSC_IRQ_STATUS,
+	.ack_invert = true,
+};
+
+static int gsc_of_probe(struct device_node *np, struct gsc_dev *gsc)
+{
+	const struct of_device_id *of_id;
+
+	if (!np)
+		return -ENODEV;
+
+	of_id = of_match_device(gsc_of_match, gsc->dev);
+	if (of_id)
+		gsc->type = (enum gsc_type)of_id->data;
+
+	return 0;
+}
+
+static int
+gsc_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct gsc_dev *gsc;
+	int ret;
+	unsigned int reg;
+
+	gsc = devm_kzalloc(dev, sizeof(*gsc), GFP_KERNEL);
+	if (!gsc)
+		return -ENOMEM;
+
+	gsc->dev = &client->dev;
+	gsc->i2c = client;
+	gsc->irq = client->irq;
+	i2c_set_clientdata(client, gsc);
+
+	gsc->regmap = devm_regmap_init(dev, &regmap_gsc, client,
+				       &gsc_regmap_config);
+	if (IS_ERR(gsc->regmap))
+		return PTR_ERR(gsc->regmap);
+
+	ret = gsc_of_probe(dev->of_node, gsc);
+	if (reg < 0)
+		return ret;
+
+	if (regmap_read(gsc->regmap, GSC_FW_VER, &reg))
+		return -EIO;
+	gsc->fwver = reg;
+	regmap_read(gsc->regmap, GSC_FW_CRC, &reg);
+	gsc->fwcrc = reg;
+	regmap_read(gsc->regmap, GSC_FW_CRC + 1, &reg);
+	gsc->fwcrc |= reg << 8;
+
+	gsc->i2c_hwmon = i2c_new_dummy(client->adapter, GSC_HWMON);
+	if (!gsc->i2c_hwmon) {
+		dev_err(dev, "Failed to allocate I2C device for HWMON\n");
+		return -ENODEV;
+	}
+	i2c_set_clientdata(gsc->i2c_hwmon, gsc);
+
+	gsc->regmap_hwmon = devm_regmap_init(dev, &regmap_gsc, gsc->i2c_hwmon,
+				       &gsc_regmap_hwmon_config);
+	if (IS_ERR(gsc->regmap_hwmon)) {
+		ret = PTR_ERR(gsc->regmap_hwmon);
+		dev_err(dev, "failed to allocate register map: %d\n", ret);
+		goto err_regmap;
+	}
+
+	ret = devm_regmap_add_irq_chip(dev, gsc->regmap, gsc->irq,
+				       IRQF_ONESHOT | IRQF_SHARED |
+				       IRQF_TRIGGER_FALLING, 0,
+				       &gsc_irq_chip, &gsc->irq_chip_data);
+	if (ret)
+		goto err_regmap;
+
+	dev_info(dev, "Gateworks System Controller v%d: fw v%02d 0x%04x\n",
+		 gsc->type, gsc->fwver, gsc->fwcrc);
+
+	/* sysfs hooks */
+	ret = sysfs_create_group(&dev->kobj, &attr_group);
+	if (ret)
+		dev_err(dev, "failed to create sysfs attrs\n");
+
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		goto err_sysfs;
+
+	return 0;
+
+err_sysfs:
+	sysfs_remove_group(&dev->kobj, &attr_group);
+err_regmap:
+	i2c_unregister_device(gsc->i2c_hwmon);
+
+	return ret;
+}
+
+static int gsc_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &attr_group);
+
+	return 0;
+}
+
+static struct i2c_driver gsc_driver = {
+	.driver = {
+		.name	= KBUILD_MODNAME,
+		.of_match_table = of_match_ptr(gsc_of_match),
+	},
+	.probe		= gsc_probe,
+	.remove		= gsc_remove,
+	.id_table	= gsc_i2c_ids,
+};
+
+module_i2c_driver(gsc_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("I2C Core interface for GSC");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/gsc.h b/include/linux/mfd/gsc.h
new file mode 100644
index 0000000..2a55e0d
--- /dev/null
+++ b/include/linux/mfd/gsc.h
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#ifndef __LINUX_MFD_GSC_H_
+#define __LINUX_MFD_GSC_H_
+
+/* Device Addresses */
+#define GSC_MISC	0x20
+#define GSC_UPDATE	0x21
+#define GSC_GPIO	0x23
+#define GSC_HWMON	0x29
+#define GSC_EEPROM0	0x50
+#define GSC_EEPROM1	0x51
+#define GSC_EEPROM2	0x52
+#define GSC_EEPROM3	0x53
+#define GSC_RTC		0x68
+
+/* Register offsets */
+#define GSC_CTRL_0	0x00
+#define GSC_CTRL_1	0x01
+#define GSC_TIME	0x02
+#define GSC_TIME_ADD	0x06
+#define GSC_IRQ_STATUS	0x0A
+#define GSC_IRQ_ENABLE	0x0B
+#define GSC_FW_CRC	0x0C
+#define GSC_FW_VER	0x0E
+#define GSC_WP		0x0F
+
+/* Bit definitions */
+#define GSC_CTRL_0_PB_HARD_RESET	0
+#define GSC_CTRL_0_PB_CLEAR_SECURE_KEY	1
+#define GSC_CTRL_0_PB_SOFT_POWER_DOWN	2
+#define GSC_CTRL_0_PB_BOOT_ALTERNATE	3
+#define GSC_CTRL_0_PERFORM_CRC		4
+#define GSC_CTRL_0_TAMPER_DETECT	5
+#define GSC_CTRL_0_SWITCH_HOLD		6
+
+#define GSC_CTRL_1_SLEEP_ENABLE		0
+#define GSC_CTRL_1_ACTIVATE_SLEEP	1
+#define GSC_CTRL_1_LATCH_SLEEP_ADD	2
+#define GSC_CTRL_1_SLEEP_NOWAKEPB	3
+#define GSC_CTRL_1_WDT_TIME		4
+#define GSC_CTRL_1_WDT_ENABLE		5
+#define GSC_CTRL_1_SWITCH_BOOT_ENABLE	6
+#define GSC_CTRL_1_SWITCH_BOOT_CLEAR	7
+
+#define GSC_IRQ_PB			0
+#define GSC_IRQ_KEY_ERASED		1
+#define GSC_IRQ_EEPROM_WP		2
+#define GSC_IRQ_RESV			3
+#define GSC_IRQ_GPIO			4
+#define GSC_IRQ_TAMPER			5
+#define GSC_IRQ_WDT_TIMEOUT		6
+#define GSC_IRQ_SWITCH_HOLD		7
+
+enum gsc_type {
+	gsc_v1 = 1,
+	gsc_v2 = 2,
+	gsc_v3 = 3,
+};
+
+struct gsc_dev {
+	struct device *dev;
+
+	struct i2c_client *i2c;		/* 0x20: interrupt controller, WDT */
+	struct i2c_client *i2c_hwmon;	/* 0x29: hwmon, fan controller */
+
+	struct regmap *regmap;
+	struct regmap *regmap_hwmon;
+	struct regmap_irq_chip_data *irq_chip_data;
+
+	int irq;
+	enum gsc_type type;
+	unsigned int fwver;
+	unsigned short fwcrc;
+};
+
+#endif /* __LINUX_MFD_GSC_H_ */
-- 
2.7.4

[RFC 3/4] hwmon: add Gateworks System Controller support

[Tim Harvey]

The Gateworks System Controller has a hwmon sub-component that exposes
up to 16 ADC's, some of which are temperature sensors, others which are
voltage inputs. The ADC configuration (register mapping and name) is
configured via device-tree and varies board to board.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/hwmon/Kconfig     |   6 +
 drivers/hwmon/Makefile    |   1 +
 drivers/hwmon/gsc-hwmon.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 306 insertions(+)
 create mode 100644 drivers/hwmon/gsc-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ad0176..9cdc3cb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -475,6 +475,12 @@ config SENSORS_F75375S
 	  This driver can also be built as a module.  If so, the module
 	  will be called f75375s.
 
+config SENSORS_GSC
+        tristate "Gateworks System Controller ADC"
+        depends on MFD_GSC
+        help
+          Support for the Gateworks System Controller A/D converters.
+
 config SENSORS_MC13783_ADC
         tristate "Freescale MC13783/MC13892 ADC"
         depends on MFD_MC13XXX
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0fe489f..835a536 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
 obj-$(CONFIG_SENSORS_G762)	+= g762.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
+obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
 obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
new file mode 100644
index 0000000..3e14bea
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#define DEBUG
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mfd/gsc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* map channel to channel info */
+struct gsc_hwmon_ch {
+	u8 reg;
+	char name[32];
+};
+static struct gsc_hwmon_ch gsc_temp_ch[16];
+static struct gsc_hwmon_ch gsc_in_ch[16];
+static struct gsc_hwmon_ch gsc_fan_ch[5];
+
+static int
+gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	       int ch, long *val)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(dev);
+	int sz, ret;
+	u8 reg;
+	u8 buf[3];
+
+	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+	switch (type) {
+	case hwmon_in:
+		sz = 3;
+		reg = gsc_in_ch[ch].reg;
+		break;
+	case hwmon_temp:
+		sz = 2;
+		reg = gsc_temp_ch[ch].reg;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
+	if (!ret) {
+		*val = 0;
+		while (sz-- > 0)
+			*val |= (buf[sz] << (8*sz));
+		if ((type == hwmon_temp) && *val > 0x8000)
+			*val -= 0xffff;
+	}
+
+	return ret;
+}
+
+static int
+gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+		      u32 attr, int ch, const char **buf)
+{
+	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+	switch (type) {
+	case hwmon_in:
+	case hwmon_temp:
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_in_label:
+			*buf = gsc_in_ch[ch].name;
+			return 0;
+			break;
+		case hwmon_temp_label:
+			*buf = gsc_temp_ch[ch].name;
+			return 0;
+			break;
+		case hwmon_fan_label:
+			*buf = gsc_fan_ch[ch].name;
+			return 0;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -ENOTSUPP;
+}
+
+static int
+gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	       int ch, long val)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(dev);
+	int ret;
+	u8 reg;
+	u8 buf[3];
+
+	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+	switch (type) {
+	case hwmon_fan:
+		buf[0] = val & 0xff;
+		buf[1] = (val >> 8) & 0xff;
+		reg = gsc_fan_ch[ch].reg;
+		ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	return ret;
+}
+
+static umode_t
+gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
+		     int ch)
+{
+	const struct gsc_dev *gsc = _data;
+	struct device *dev = gsc->dev;
+	umode_t mode = 0;
+
+	switch (type) {
+	case hwmon_fan:
+		if (attr == hwmon_fan_input)
+			mode = (S_IRUGO | S_IWUSR);
+		break;
+	case hwmon_temp:
+	case hwmon_in:
+		mode = S_IRUGO;
+		break;
+	default:
+		break;
+	}
+	dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
+		attr, ch, mode);
+
+	return mode;
+}
+
+static u32 gsc_in_config[] = {
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	0
+};
+static const struct hwmon_channel_info gsc_in = {
+	.type = hwmon_in,
+	.config = gsc_in_config,
+};
+
+static u32 gsc_temp_config[] = {
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	0
+};
+static const struct hwmon_channel_info gsc_temp = {
+	.type = hwmon_temp,
+	.config = gsc_temp_config,
+};
+
+static u32 gsc_fan_config[] = {
+	HWMON_F_INPUT,
+	HWMON_F_INPUT,
+	HWMON_F_INPUT,
+	HWMON_F_INPUT,
+	HWMON_F_INPUT,
+	HWMON_F_INPUT,
+	0,
+};
+static const struct hwmon_channel_info gsc_fan = {
+	.type = hwmon_fan,
+	.config = gsc_fan_config,
+};
+
+static const struct hwmon_channel_info *gsc_info[] = {
+	&gsc_temp,
+	&gsc_in,
+	&gsc_fan,
+	NULL
+};
+
+static const struct hwmon_ops gsc_hwmon_ops = {
+	.is_visible = gsc_hwmon_is_visible,
+	.read = gsc_hwmon_read,
+	.read_string = gsc_hwmon_read_string,
+	.write = gsc_hwmon_write,
+};
+
+static const struct hwmon_chip_info gsc_chip_info = {
+	.ops = &gsc_hwmon_ops,
+	.info = gsc_info,
+};
+
+static int gsc_hwmon_probe(struct platform_device *pdev)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+	struct device_node *np;
+	struct device *hwmon;
+	int temp_count = 0;
+	int in_count = 0;
+	int fan_count = 0;
+	int ret;
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+	np = of_get_next_child(pdev->dev.of_node, NULL);
+	while (np) {
+		u32 reg, type;
+		const char *label;
+
+		of_property_read_u32(np, "reg", &reg);
+		of_property_read_u32(np, "type", &type);
+		label = of_get_property(np, "label", NULL);
+		switch(type) {
+		case 0: /* temperature sensor */
+			gsc_temp_config[temp_count] = HWMON_T_INPUT |
+						      HWMON_T_LABEL;
+			gsc_temp_ch[temp_count].reg = reg;
+			strncpy(gsc_temp_ch[temp_count].name, label, 32);
+			if (temp_count < ARRAY_SIZE(gsc_temp_config))
+				temp_count++;
+			break;
+		case 1: /* voltage sensor */
+			gsc_in_config[in_count] = HWMON_I_INPUT |
+						  HWMON_I_LABEL;
+			gsc_in_ch[in_count].reg = reg;
+			strncpy(gsc_in_ch[in_count].name, label, 32);
+			if (in_count < ARRAY_SIZE(gsc_in_config))
+				in_count++;
+			break;
+		case 2: /* fan controller setpoint */
+			gsc_fan_config[fan_count] = HWMON_F_INPUT |
+						    HWMON_F_LABEL;
+			gsc_fan_ch[fan_count].reg = reg;
+			strncpy(gsc_fan_ch[fan_count].name, label, 32);
+			if (fan_count < ARRAY_SIZE(gsc_fan_config))
+				fan_count++;
+			break;
+		}
+		np = of_get_next_child(pdev->dev.of_node, np);
+	}
+	/* terminate list */
+	gsc_in_config[in_count] = 0;
+	gsc_temp_config[temp_count] = 0;
+	gsc_fan_config[fan_count] = 0;
+
+	hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
+						     KBUILD_MODNAME, gsc,
+						     &gsc_chip_info, NULL);
+	if (IS_ERR(hwmon)) {
+		ret = PTR_ERR(hwmon);
+		dev_err(&pdev->dev, "Unable to register hwmon device: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id gsc_hwmon_of_match[] = {
+	{ .compatible = "gw,gsc-hwmon", },
+	{}
+};
+
+static struct platform_driver gsc_hwmon_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = gsc_hwmon_of_match,
+	},
+	.probe = gsc_hwmon_probe,
+};
+
+module_platform_driver(gsc_hwmon_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("GSC hardware monitor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[RFC 4/4] input: misc: Add Gateworks System Controller support

[Tim Harvey]

Add support for dispatching Linux Input events for the various interrupts
the Gateworks System Controller provides.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/input/misc/Kconfig     |   6 ++
 drivers/input/misc/Makefile    |   1 +
 drivers/input/misc/gsc-input.c | 196 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/input/misc/gsc-input.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 9f082a3..3d18a0e 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
 	  To compile this driver as a module, choose M here: the
 	  module will be called e3x0_button.
 
+config INPUT_GSC
+	tristate "Gateworks System Controller input support"
+	depends on MFD_GSC
+	help
+	  Say Y here if you want Gateworks System Controller input support.
+
 config INPUT_PCSPKR
 	tristate "PC Speaker support"
 	depends on PCSPKR_PLATFORM
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4b6118d..969debe 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
 obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
+obj-$(CONFIG_INPUT_GSC)			+= gsc-input.o
 obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o
diff --git a/drivers/input/misc/gsc-input.c b/drivers/input/misc/gsc-input.c
new file mode 100644
index 0000000..7cf217c
--- /dev/null
+++ b/drivers/input/misc/gsc-input.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#define DEBUG
+
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/gsc.h>
+
+struct gsc_irq {
+	unsigned int irq;
+	const char *name;
+	unsigned int virq;
+};
+
+static struct gsc_irq gsc_irqs[] = {
+	{ GSC_IRQ_PB,		"button" },
+	{ GSC_IRQ_KEY_ERASED,	"key-erased" },
+	{ GSC_IRQ_EEPROM_WP,	"eeprom-wp" },
+	{ GSC_IRQ_TAMPER,	"tamper" },
+	{ GSC_IRQ_SWITCH_HOLD,	"button-held" },
+};
+
+struct gsc_input_info {
+	struct device *dev;
+	struct gsc_dev *gsc;
+	struct input_dev *input;
+
+	int irq;
+	struct work_struct irq_work;
+	struct mutex mutex;
+};
+
+static void gsc_input_irq_work(struct work_struct *work)
+{
+	struct gsc_input_info *info = container_of(work, struct gsc_input_info,
+						   irq_work);
+	struct gsc_dev *gsc = info->gsc;
+	int i, ret = 0;
+	int key, sts;
+	struct gsc_irq *gsc_irq = NULL;
+
+	dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
+	mutex_lock(&info->mutex);
+
+	for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
+		if (info->irq == gsc_irqs[i].virq)
+			gsc_irq = &gsc_irqs[i];
+	if (!gsc_irq) {
+		dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, &sts);
+	if (ret) {
+		dev_err(info->dev, "failed to read status register\n");
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	key = -1;
+	switch (gsc_irq->virq) {
+	case GSC_IRQ_PB:
+		key = BTN_0;
+		break;
+	case GSC_IRQ_KEY_ERASED:
+		key = BTN_1;
+		break;
+	case GSC_IRQ_EEPROM_WP:
+		key = BTN_2;
+		break;
+	case GSC_IRQ_GPIO:
+		key = BTN_3;
+		break;
+	case GSC_IRQ_TAMPER:
+		key = BTN_4;
+		break;
+	case GSC_IRQ_SWITCH_HOLD:
+		key = BTN_5;
+		break;
+	}
+
+	if (key != -1) {
+		dev_dbg(&info->input->dev, "bit%d: key=0x%03x %s\n",
+			gsc_irq->virq, key, gsc_irq->name);
+		input_report_key(info->input, key, 1);
+		input_report_key(info->input, key, 0);
+		input_sync(info->input);
+	}
+
+	mutex_unlock(&info->mutex);
+}
+
+static irqreturn_t gsc_input_irq(int irq, void *data)
+{
+	struct gsc_input_info *info = data;
+
+	dev_dbg(info->gsc->dev, "%s irq%d\n", __func__, irq);
+	info->irq = irq;
+	schedule_work(&info->irq_work);
+
+	return IRQ_HANDLED;
+}
+
+static int gsc_input_probe(struct platform_device *pdev)
+{
+	struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+	struct gsc_input_info *info;
+	struct input_dev *input;
+	int ret, i;
+
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+	info = devm_kzalloc(&pdev->dev, sizeof(struct gsc_input_info),
+			    GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	info->dev = &pdev->dev;
+	info->gsc = gsc;
+
+	/* Register input device */
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input) {
+		dev_err(&pdev->dev, "Can't allocate input device\n");
+		return -ENOMEM;
+	}
+	info->input = input;
+
+	input->name = KBUILD_MODNAME;
+	input->dev.parent = &pdev->dev;
+
+	input_set_capability(input, EV_KEY, BTN_0); /* button */
+	input_set_capability(input, EV_KEY, BTN_1); /* key erased */
+	input_set_capability(input, EV_KEY, BTN_2); /* ee wp */
+	input_set_capability(input, EV_KEY, BTN_3); /* gpio */
+	input_set_capability(input, EV_KEY, BTN_4); /* tamper */
+	input_set_capability(input, EV_KEY, BTN_5); /* button held */
+
+	ret = input_register_device(input);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, gsc);
+	mutex_init(&info->mutex);
+	INIT_WORK(&info->irq_work, gsc_input_irq_work);
+
+	/* Support irq domain */
+	for (i = 0; i < ARRAY_SIZE(gsc_irqs); i++) {
+		struct gsc_irq *gsc_irq = &gsc_irqs[i];
+		int virq;
+
+		virq = regmap_irq_get_virq(gsc->irq_chip_data, gsc_irq->irq);
+		if (virq <= 0)
+			return -EINVAL;
+		gsc_irq->virq = virq;
+
+		ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
+						gsc_input_irq, 0,
+						gsc_irq->name, info);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed: irq request (IRQ: %d, error: %d\n)",
+				gsc_irq->irq, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id gsc_input_dt_ids[] = {
+	{ .compatible = "gw,gsc-input", },
+	{}
+};
+
+static struct platform_driver gsc_input_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = gsc_input_dt_ids,
+	},
+	.probe = gsc_input_probe,
+};
+
+module_platform_driver(gsc_input_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("GSC input driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [RFC 2/4] mfd: add Gateworks System Controller core driver

[Randy Dunlap]

On 02/27/2018 05:21 PM, Tim Harvey wrote:
> The Gateworks System Controller (GSC) is an I2C slave controller
> implemented with an MSP430 micro-controller whose firmware embeds the
> following features:
>  - I/O expander (16 GPIO's) using PCA955x protocol
>  - Real Time Clock using DS1672 protocol
>  - User EEPROM using AT24 protocol
>  - HWMON using custom protocol
>  - Interrupt controller with tamper detect, user pushbotton
>  - Watchdog controller capable of full board power-cycle
>  - Power Control capable of full board power-cycle
> 
> see http://trac.gateworks.com/wiki/gsc for more details
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/mfd/Kconfig     |  10 ++
>  drivers/mfd/Makefile    |   1 +
>  drivers/mfd/gsc.c       | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/gsc.h |  79 ++++++++++++
>  4 files changed, 420 insertions(+)
>  create mode 100644 drivers/mfd/gsc.c
>  create mode 100644 include/linux/mfd/gsc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1d20a80..16dd486 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
>  	  Select this option to enable support for Samsung Exynos Low Power
>  	  Audio Subsystem.
>  
> +config MFD_GSC
> +	tristate "Gateworks System Controller"
> +	depends on (I2C && OF) || COMPILE_TEST

Do both I2C and OF have stubs so that a driver will build when they are
both disabled?  I.e., only COMPILE_TEST is enabled?


> +	select MFD_CORE
> +	select REGMAP_I2C
> +	select REGMAP_IRQ
> +	help
> +	  Enable support for the Gateworks System Controller found
> +	  on Gateworks Single Board Computers.
> +
>  config MFD_MC13XXX
>  	tristate
>  	depends on (SPI_MASTER || I2C)
thanks,
-- 
~Randy

Re: [RFC 3/4] hwmon: add Gateworks System Controller support

[Guenter Roeck]

On 02/27/2018 05:21 PM, Tim Harvey wrote:
> The Gateworks System Controller has a hwmon sub-component that exposes
> up to 16 ADC's, some of which are temperature sensors, others which are
> voltage inputs. The ADC configuration (register mapping and name) is
> configured via device-tree and varies board to board.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>   drivers/hwmon/Kconfig     |   6 +
>   drivers/hwmon/Makefile    |   1 +
>   drivers/hwmon/gsc-hwmon.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 306 insertions(+)
>   create mode 100644 drivers/hwmon/gsc-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ad0176..9cdc3cb 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -475,6 +475,12 @@ config SENSORS_F75375S
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called f75375s.
>   
> +config SENSORS_GSC
> +        tristate "Gateworks System Controller ADC"
> +        depends on MFD_GSC
> +        help
> +          Support for the Gateworks System Controller A/D converters.
> +
>   config SENSORS_MC13783_ADC
>           tristate "Freescale MC13783/MC13892 ADC"
>           depends on MFD_MC13XXX
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 0fe489f..835a536 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)	+= g760a.o
>   obj-$(CONFIG_SENSORS_G762)	+= g762.o
>   obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
>   obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
> +obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
>   obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>   obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
>   obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
> new file mode 100644
> index 0000000..3e14bea
> --- /dev/null
> +++ b/drivers/hwmon/gsc-hwmon.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Gateworks Corporation
> + */
> +#define DEBUG

Please no.

> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/gsc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* map channel to channel info */
> +struct gsc_hwmon_ch {
> +	u8 reg;
> +	char name[32];
> +};
> +static struct gsc_hwmon_ch gsc_temp_ch[16];

16 temperature channels ...

> +static struct gsc_hwmon_ch gsc_in_ch[16];
> +static struct gsc_hwmon_ch gsc_fan_ch[5];
> +
> +static int
> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	       int ch, long *val)
> +{
> +	struct gsc_dev *gsc = dev_get_drvdata(dev);
> +	int sz, ret;
> +	u8 reg;
> +	u8 buf[3];
> +
> +	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
> +	switch (type) {
> +	case hwmon_in:
> +		sz = 3;
> +		reg = gsc_in_ch[ch].reg;
> +		break;
> +	case hwmon_temp:
> +		sz = 2;
> +		reg = gsc_temp_ch[ch].reg;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
> +	if (!ret) {
> +		*val = 0;
> +		while (sz-- > 0)
> +			*val |= (buf[sz] << (8*sz));
> +		if ((type == hwmon_temp) && *val > 0x8000)

Excessive [and inconsistent] ( )

Please make this
	if (ret)
		return ret;
	...
	return 0;

> +			*val -= 0xffff;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int ch, const char **buf)
> +{
> +	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
> +	switch (type) {
> +	case hwmon_in:
> +	case hwmon_temp:
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_in_label:
> +			*buf = gsc_in_ch[ch].name;
> +			return 0;
> +			break;
> +		case hwmon_temp_label:
> +			*buf = gsc_temp_ch[ch].name;
> +			return 0;
> +			break;
> +		case hwmon_fan_label:
> +			*buf = gsc_fan_ch[ch].name;
> +			return 0;
> +			break;

return followed by break doesn't make sense.

> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -ENOTSUPP;
> +}
> +
> +static int
> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	       int ch, long val)
> +{
> +	struct gsc_dev *gsc = dev_get_drvdata(dev);
> +	int ret;
> +	u8 reg;
> +	u8 buf[3];
> +
> +	dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
> +	switch (type) {
> +	case hwmon_fan:
> +		buf[0] = val & 0xff;
> +		buf[1] = (val >> 8) & 0xff;
> +		reg = gsc_fan_ch[ch].reg;
> +		ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);

	&buf -> buf

> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static umode_t
> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> +		     int ch)
> +{
> +	const struct gsc_dev *gsc = _data;
> +	struct device *dev = gsc->dev;
> +	umode_t mode = 0;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		if (attr == hwmon_fan_input)
> +			mode = (S_IRUGO | S_IWUSR);

Unnecessary ( )

> +		break;
> +	case hwmon_temp:
> +	case hwmon_in:
> +		mode = S_IRUGO;
> +		break;
> +	default:
> +		break;
> +	}
> +	dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
> +		attr, ch, mode);
> +
> +	return mode;
> +}
> +
> +static u32 gsc_in_config[] = {
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	0
> +};
> +static const struct hwmon_channel_info gsc_in = {
> +	.type = hwmon_in,
> +	.config = gsc_in_config,
> +};
> +
> +static u32 gsc_temp_config[] = {
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	0

... but this array only has 8+1 elements. This seems inconsistent.
How about using some defines for array sizes ?

Also, why initialize those arrays ? You are overwriting them below.
You could just use a static size array instead.

I assume it is guaranteed that there is only exactly one instance
of this device in the system. Have you tried what happens if you
declare two instances anyway ? The result must be interesting,
with all those static variables.

> +};
> +static const struct hwmon_channel_info gsc_temp = {
> +	.type = hwmon_temp,
> +	.config = gsc_temp_config,
> +};
> +
> +static u32 gsc_fan_config[] = {
> +	HWMON_F_INPUT,
> +	HWMON_F_INPUT,
> +	HWMON_F_INPUT,
> +	HWMON_F_INPUT,
> +	HWMON_F_INPUT,
> +	HWMON_F_INPUT,
> +	0,
> +};

The matching gsc_fan_ch has only 5 entries.

> +static const struct hwmon_channel_info gsc_fan = {
> +	.type = hwmon_fan,
> +	.config = gsc_fan_config,
> +};
> +
> +static const struct hwmon_channel_info *gsc_info[] = {
> +	&gsc_temp,
> +	&gsc_in,
> +	&gsc_fan,
> +	NULL
> +};
> +
> +static const struct hwmon_ops gsc_hwmon_ops = {
> +	.is_visible = gsc_hwmon_is_visible,
> +	.read = gsc_hwmon_read,
> +	.read_string = gsc_hwmon_read_string,
> +	.write = gsc_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info gsc_chip_info = {
> +	.ops = &gsc_hwmon_ops,
> +	.info = gsc_info,
> +};
> +
> +static int gsc_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *np;
> +	struct device *hwmon;
> +	int temp_count = 0;
> +	int in_count = 0;
> +	int fan_count = 0;
> +	int ret;
> +
> +	dev_dbg(&pdev->dev, "%s\n", __func__);

You declare local 'dev' variables all over the place, except here,
where it would actually be used multiple times.

Please either declare one here as well, or drop all the others.

> +	np = of_get_next_child(pdev->dev.of_node, NULL);
> +	while (np) {
> +		u32 reg, type;
> +		const char *label;
> +
> +		of_property_read_u32(np, "reg", &reg);
> +		of_property_read_u32(np, "type", &type);
> +		label = of_get_property(np, "label", NULL);

It must be interesting to see what happens if no 'label' property
is provided. Have you tried ? Also, no validation of 'reg' and 'type' ?
Are you sure ?

> +		switch(type) {
> +		case 0: /* temperature sensor */
> +			gsc_temp_config[temp_count] = HWMON_T_INPUT |
> +						      HWMON_T_LABEL;
> +			gsc_temp_ch[temp_count].reg = reg;
> +			strncpy(gsc_temp_ch[temp_count].name, label, 32);

This leaves the string unterminated if it is too long. Have you tested
what happens in this situation ? Consider using strlcpy instead.

Also please use sizeof() instead of '32'.

> +			if (temp_count < ARRAY_SIZE(gsc_temp_config))
> +				temp_count++;

I would suggest to abort with EINVAL if this happens. Otherwise the last entry
is overwritten, which doesn't make much sense. Also, this accepts up to ARRAY_SIZE()
entries, leaving no termination.

> +			break;
> +		case 1: /* voltage sensor */
> +			gsc_in_config[in_count] = HWMON_I_INPUT |
> +						  HWMON_I_LABEL;
> +			gsc_in_ch[in_count].reg = reg;

So a reg value of 0xXXyy is auto-converted to 0xYY ?

> +			strncpy(gsc_in_ch[in_count].name, label, 32);
> +			if (in_count < ARRAY_SIZE(gsc_in_config))
> +				in_count++;
> +			break;
> +		case 2: /* fan controller setpoint */
> +			gsc_fan_config[fan_count] = HWMON_F_INPUT |
> +						    HWMON_F_LABEL;
> +			gsc_fan_ch[fan_count].reg = reg;

It is going to be interesting to see what happens if there are more than
5 such entries.

> +			strncpy(gsc_fan_ch[fan_count].name, label, 32);
> +			if (fan_count < ARRAY_SIZE(gsc_fan_config))
> +				fan_count++;
> +			break;

All other types are silently ignored ?

> +		}
> +		np = of_get_next_child(pdev->dev.of_node, np);
> +	}
> +	/* terminate list */
> +	gsc_in_config[in_count] = 0;
> +	gsc_temp_config[temp_count] = 0;
> +	gsc_fan_config[fan_count] = 0;
> +
I would suggest to move above code into a separate function.

> +	hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
> +						     KBUILD_MODNAME, gsc,
> +						     &gsc_chip_info, NULL);
> +	if (IS_ERR(hwmon)) {
> +		ret = PTR_ERR(hwmon);
> +		dev_err(&pdev->dev, "Unable to register hwmon device: %d\n",
> +			ret);
> +		return ret;
> +	}

The error would be ENOMEM. Is it necessary to report that again ?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gsc_hwmon_of_match[] = {
> +	{ .compatible = "gw,gsc-hwmon", },
> +	{}
> +};
> +
> +static struct platform_driver gsc_hwmon_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = gsc_hwmon_of_match,
> +	},
> +	.probe = gsc_hwmon_probe,
> +};
> +
> +module_platform_driver(gsc_hwmon_driver);
> +
> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
> +MODULE_DESCRIPTION("GSC hardware monitor driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC 4/4] input: misc: Add Gateworks System Controller support

[Dmitry Torokhov]

Hi Tim,

On Tue, Feb 27, 2018 at 05:21:14PM -0800, Tim Harvey wrote:
> Add support for dispatching Linux Input events for the various interrupts
> the Gateworks System Controller provides.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/input/misc/Kconfig     |   6 ++
>  drivers/input/misc/Makefile    |   1 +
>  drivers/input/misc/gsc-input.c | 196 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/input/misc/gsc-input.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 9f082a3..3d18a0e 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called e3x0_button.
>  
> +config INPUT_GSC
> +	tristate "Gateworks System Controller input support"
> +	depends on MFD_GSC
> +	help
> +	  Say Y here if you want Gateworks System Controller input support.
> +

"To compile this driver as a module..."

>  config INPUT_PCSPKR
>  	tristate "PC Speaker support"
>  	depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 4b6118d..969debe 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
>  obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
>  obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
>  obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
> +obj-$(CONFIG_INPUT_GSC)			+= gsc-input.o
>  obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
>  obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
>  obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o
> diff --git a/drivers/input/misc/gsc-input.c b/drivers/input/misc/gsc-input.c
> new file mode 100644
> index 0000000..7cf217c
> --- /dev/null
> +++ b/drivers/input/misc/gsc-input.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Gateworks Corporation
> + */

Let's keep the same // comment block fir the copyright notice as well.
An one-line describing what this driver is would be appreciated too.

> +#define DEBUG

Please no.

> +
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/gsc.h>
> +
> +struct gsc_irq {
> +	unsigned int irq;
> +	const char *name;
> +	unsigned int virq;
> +};
> +
> +static struct gsc_irq gsc_irqs[] = {
> +	{ GSC_IRQ_PB,		"button" },
> +	{ GSC_IRQ_KEY_ERASED,	"key-erased" },
> +	{ GSC_IRQ_EEPROM_WP,	"eeprom-wp" },
> +	{ GSC_IRQ_TAMPER,	"tamper" },
> +	{ GSC_IRQ_SWITCH_HOLD,	"button-held" },
> +};
> +
> +struct gsc_input_info {
> +	struct device *dev;
> +	struct gsc_dev *gsc;
> +	struct input_dev *input;
> +
> +	int irq;
> +	struct work_struct irq_work;
> +	struct mutex mutex;
> +};
> +
> +static void gsc_input_irq_work(struct work_struct *work)
> +{
> +	struct gsc_input_info *info = container_of(work, struct gsc_input_info,
> +						   irq_work);
> +	struct gsc_dev *gsc = info->gsc;
> +	int i, ret = 0;
> +	int key, sts;
> +	struct gsc_irq *gsc_irq = NULL;
> +
> +	dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
> +	mutex_lock(&info->mutex);
> +
> +	for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
> +		if (info->irq == gsc_irqs[i].virq)
> +			gsc_irq = &gsc_irqs[i];
> +	if (!gsc_irq) {
> +		dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, &sts);

Why is this needed? To clear irq? What happens if several events happen
at the same time? Do we lose one of them?

> +	if (ret) {
> +		dev_err(info->dev, "failed to read status register\n");
> +		mutex_unlock(&info->mutex);
> +		return;
> +	}
> +
> +	key = -1;
> +	switch (gsc_irq->virq) {
> +	case GSC_IRQ_PB:
> +		key = BTN_0;
> +		break;
> +	case GSC_IRQ_KEY_ERASED:
> +		key = BTN_1;
> +		break;
> +	case GSC_IRQ_EEPROM_WP:
> +		key = BTN_2;
> +		break;
> +	case GSC_IRQ_GPIO:
> +		key = BTN_3;
> +		break;
> +	case GSC_IRQ_TAMPER:
> +		key = BTN_4;
> +		break;
> +	case GSC_IRQ_SWITCH_HOLD:
> +		key = BTN_5;

Could we provide the mapping in DTS instead of hard-coding them?

> +		break;
> +	}
> +
> +	if (key != -1) {
> +		dev_dbg(&info->input->dev, "bit%d: key=0x%03x %s\n",
> +			gsc_irq->virq, key, gsc_irq->name);
> +		input_report_key(info->input, key, 1);

		input_sync();

> +		input_report_key(info->input, key, 0);
> +		input_sync(info->input);
> +	}
> +
> +	mutex_unlock(&info->mutex);
> +}
> +
> +static irqreturn_t gsc_input_irq(int irq, void *data)
> +{
> +	struct gsc_input_info *info = data;
> +
> +	dev_dbg(info->gsc->dev, "%s irq%d\n", __func__, irq);
> +	info->irq = irq;
> +	schedule_work(&info->irq_work);

Why not use threaded interrupt?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int gsc_input_probe(struct platform_device *pdev)
> +{
> +	struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> +	struct gsc_input_info *info;
> +	struct input_dev *input;
> +	int ret, i;
> +
> +	dev_dbg(&pdev->dev, "%s\n", __func__);
> +	info = devm_kzalloc(&pdev->dev, sizeof(struct gsc_input_info),
> +			    GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +	info->dev = &pdev->dev;
> +	info->gsc = gsc;
> +
> +	/* Register input device */
> +	input = devm_input_allocate_device(&pdev->dev);
> +	if (!input) {
> +		dev_err(&pdev->dev, "Can't allocate input device\n");
> +		return -ENOMEM;
> +	}
> +	info->input = input;
> +
> +	input->name = KBUILD_MODNAME;
> +	input->dev.parent = &pdev->dev;

Not needed - it is set by devm_input_allocate_device().

> +
> +	input_set_capability(input, EV_KEY, BTN_0); /* button */
> +	input_set_capability(input, EV_KEY, BTN_1); /* key erased */
> +	input_set_capability(input, EV_KEY, BTN_2); /* ee wp */
> +	input_set_capability(input, EV_KEY, BTN_3); /* gpio */
> +	input_set_capability(input, EV_KEY, BTN_4); /* tamper */
> +	input_set_capability(input, EV_KEY, BTN_5); /* button held */
> +
> +	ret = input_register_device(input);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, gsc);
> +	mutex_init(&info->mutex);
> +	INIT_WORK(&info->irq_work, gsc_input_irq_work);
> +
> +	/* Support irq domain */
> +	for (i = 0; i < ARRAY_SIZE(gsc_irqs); i++) {
> +		struct gsc_irq *gsc_irq = &gsc_irqs[i];
> +		int virq;
> +
> +		virq = regmap_irq_get_virq(gsc->irq_chip_data, gsc_irq->irq);
> +		if (virq <= 0)
> +			return -EINVAL;
> +		gsc_irq->virq = virq;

I'd say mapping should be done by MFD piece. You can add interrupts as
resources and fetch them here.

> +
> +		ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
> +						gsc_input_irq, 0,
> +						gsc_irq->name, info);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed: irq request (IRQ: %d, error: %d\n)",
> +				gsc_irq->irq, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gsc_input_dt_ids[] = {
> +	{ .compatible = "gw,gsc-input", },
> +	{}
> +};
> +
> +static struct platform_driver gsc_input_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = gsc_input_dt_ids,
> +	},
> +	.probe = gsc_input_probe,
> +};
> +
> +module_platform_driver(gsc_input_driver);
> +
> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
> +MODULE_DESCRIPTION("GSC input driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [RFC 0/4] Add support for the Gateworks System Controller

[Andrew Lunn]

On Tue, Feb 27, 2018 at 05:21:10PM -0800, Tim Harvey wrote:
> This series adds support for the Gateworks System Controller used on Gateworks
> Laguna, Ventana, and Newport product families.
> 
> The GSC is an MSP430 I2C slave controller whose firmware embeds the following
> features:
>  - I/O expander (16 GPIO's emulating a PCA955x)
>  - EEPROM (enumating AT24)
>  - RTC (enumating DS1672)

Hi Tim

Maybe it is in these patches, and i missed it....

How do these emulated devices work? Does the controller respond to
different addresses for these different emulated devices? Or is it an
I2c bus mux?

    Andrew

Re: [RFC 0/4] Add support for the Gateworks System Controller

[Tim Harvey]

On Wed, Feb 28, 2018 at 6:44 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Feb 27, 2018 at 05:21:10PM -0800, Tim Harvey wrote:
>> This series adds support for the Gateworks System Controller used on Gateworks
>> Laguna, Ventana, and Newport product families.
>>
>> The GSC is an MSP430 I2C slave controller whose firmware embeds the following
>> features:
>>  - I/O expander (16 GPIO's emulating a PCA955x)
>>  - EEPROM (enumating AT24)
>>  - RTC (enumating DS1672)
>
> Hi Tim
>
> Maybe it is in these patches, and i missed it....
>
> How do these emulated devices work? Does the controller respond to
> different addresses for these different emulated devices? Or is it an
> I2c bus mux?
>

Andrew,

You didn't miss it - I probably need to explain it better.

The 'emulated devices' do respond on different slave addresses (which
match one of or the only the slave addresses those parts support). For
example the device-tree for the GW54xx has the following which are all
from the GSC which is the only thing on i2c1:

&i2c1 {
        clock-frequency = <100000>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_i2c1>;
        status = "okay";

        gsc: gsc@20 {
                compatible = "gw,gsc_v2";
                reg = <0x20>;
                interrupt-parent = <&gpio1>;
                interrupts = <4 GPIO_ACTIVE_LOW>;
                interrupt-controller;
                #interrupt-cells = <1>;

                gsc_input {
                        compatible = "gw,gsc-input";
                };

                gsc_hwmon {
                        compatible = "gw,gsc-hwmon";
                        #address-cells = <1>;
                        #size-cells = <0>;

                        hwmon@0 { /* A0: Board Temperature */
                                type = <0>;
                                reg = <0x00>;
                                label = "temp";
                                /* lookup table */
                        };

                        hwmon@1 { /* A1: Input Voltage */
                                type = <1>;
                                reg = <0x02>;
                                label = "Vin";
                                gw,voltage-divider = <22100 1000>;
                                gw,offset = <800>;
                        };

                        hwmon@2 { /* A2: 5P0 */
                                type = <1>;
                                reg = <0x0b>;
                                label = "5P0";
                                gw,voltage-divider = <22100 1000>;
                        };

                        hwmon@4 { /* A4: 0-5V input */
                                type = <1>;
                                reg = <0x14>;
                                label = "ANL0";
                                gw,voltage-divider = <10000 10000>;
                        };

                        hwmon@5 { /* A5: 2P5 PCIe/GigE */
                                type = <1>;
                                reg = <0x23>;
                                label = "2P5";
                                gw,voltage-divider = <10000 10000>;
                        };

                        hwmon@6 { /* A6: 1P8 Aud/Vid */
                                type = <1>;
                                reg = <0x1d>;
                                label = "1P8";
                        };

                        hwmon@7 { /* A7: GPS */
                                type = <1>;
                                reg = <0x26>;
                                label = "GPS";
                                gw,voltage-divider = <4990 10000>;
                        };

                        hwmon@12 { /* A12: VDD_CORE */
                                type = <1>;
                                reg = <0x3>;
                                label = "VDD_CORE";
                        };

                        hwmon@13 { /* A13: VDD_SOC */
                                type = <1>;
                                reg = <0x11>;
                                label = "VDD_SOC";
                        };

                        hwmon@14 { /* A14: 1P0 PCIe SW */
                                type = <1>;
                                reg = <0x20>;
                                label = "1P0";
                        };

                        hwmon@15 { /* fan0 */
                                type = <2>;
                                reg = <0x2c>;
                                label = "fan_50p";
                        };

                        hwmon@16 { /* fan1 */
                                type = <2>;
                                reg = <0x2e>;
                                label = "fan_60p";
                        };

                        hwmon@17 { /* fan2 */
                                type = <2>;
                                reg = <0x30>;
                                label = "fan_70p";
                        };

                        hwmon@18 { /* fan3 */
                                type = <2>;
                                reg = <0x32>;
                                label = "fan_80p";
                        };

                        hwmon@19 { /* fan4 */
                                type = <2>;
                                reg = <0x34>;
                                label = "fan_90p";
                        };

                        hwmon@20 { /* fan5 */
                                type = <2>;
                                reg = <0x36>;
                                label = "fan_100p";
                        };
                };
        };

        gsc_gpio: pca9555@23 {
                compatible = "nxp,pca9555";
                reg = <0x23>;
                gpio-controller;
                #gpio-cells = <2>;
                interrupt-parent = <&gsc>;
                interrupts = <4>;
        };

        eeprom1: eeprom@50 {
                compatible = "atmel,24c02";
                reg = <0x50>;
                pagesize = <16>;
        };

        eeprom2: eeprom@51 {
                compatible = "atmel,24c02";
                reg = <0x51>;
                pagesize = <16>;
        };

        eeprom3: eeprom@52 {
                compatible = "atmel,24c02";
                reg = <0x52>;
                pagesize = <16>;
        };

        eeprom4: eeprom@53 {
                compatible = "atmel,24c02";
                reg = <0x53>;
                pagesize = <16>;
        };

        rtc: ds1672@68 {
                compatible = "dallas,ds1672";
                reg = <0x68>;
        };
};


One issue I'm trying to figure out the best way to deal with is the
fact that the GSC can 'NAK' transactions occasionally which is why I
override the regmap read/write functions and provide retries. This
resolves the issue for the mfd core driver and sub-module drivers but
doesn't resolve the issue with these 'emulated devices' which have
their own stand-alone drivers. I'm not sure how to best deal with that
yet. I tried to add retires to the i2c adapter but that wasn't
accepted upstream because it was too generic and I was told I need to
work around it in device-drivers. I'm guessing I need to write my own
sub-module drivers that will largely duplicate whats in the
stand-alone drivers (ds1672, at24, pca9553x).

Regards,

Tim

Re: [RFC 0/4] Add support for the Gateworks System Controller

[Andrew Lunn]

Hi Tim

Cool. I would say this is done right.

> One issue I'm trying to figure out the best way to deal with is the
> fact that the GSC can 'NAK' transactions occasionally which is why I
> override the regmap read/write functions and provide retries. This
> resolves the issue for the mfd core driver and sub-module drivers but
> doesn't resolve the issue with these 'emulated devices' which have
> their own stand-alone drivers. I'm not sure how to best deal with that
> yet. I tried to add retires to the i2c adapter but that wasn't
> accepted upstream because it was too generic and I was told I need to
> work around it in device-drivers.

How about writing an i2c bus driver which sits directly on top of
another i2c bus? Basically a one port i2c mux.

The current mux code does not seem to directly allow it, since it
calls i2c_transfer() directly on the parent, where as you want it to
call your own i2c_transfer function. But maybe you could expended the
core mux code to allow the i2c_mux_core structure to contain a transfer
function?

      Andrew

Re: [RFC 2/4] mfd: add Gateworks System Controller core driver

[Andrew Lunn]

> +		dev_err(&client->dev, ">> 0x%02x %d\n", reg, ret);
> +		return ret;
> +	}
> +	dev_dbg(&client->dev, ">> 0x%02x=0x%02x (%d)\n", reg, val, retry);
> +
> +        return 0;

Hi Tim

There appears to be a few spaces vs tabs issues in this file.

      Andrew
 

Re: [RFC 4/4] input: misc: Add Gateworks System Controller support

[Tim Harvey]

On Tue, Feb 27, 2018 at 8:54 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Tim,

Hi Dmitry - thanks for the review!

>
> On Tue, Feb 27, 2018 at 05:21:14PM -0800, Tim Harvey wrote:
>> Add support for dispatching Linux Input events for the various interrupts
>> the Gateworks System Controller provides.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  drivers/input/misc/Kconfig     |   6 ++
>>  drivers/input/misc/Makefile    |   1 +
>>  drivers/input/misc/gsc-input.c | 196 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 203 insertions(+)
>>  create mode 100644 drivers/input/misc/gsc-input.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 9f082a3..3d18a0e 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
>>         To compile this driver as a module, choose M here: the
>>         module will be called e3x0_button.
>>
>> +config INPUT_GSC
>> +     tristate "Gateworks System Controller input support"
>> +     depends on MFD_GSC
>> +     help
>> +       Say Y here if you want Gateworks System Controller input support.
>> +
>
> "To compile this driver as a module..."
>

ok

>>  config INPUT_PCSPKR
>>       tristate "PC Speaker support"
>>       depends on PCSPKR_PLATFORM
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index 4b6118d..969debe 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GP2A)            += gp2ap002a00f.o
>>  obj-$(CONFIG_INPUT_GPIO_BEEPER)              += gpio-beeper.o
>>  obj-$(CONFIG_INPUT_GPIO_TILT_POLLED) += gpio_tilt_polled.o
>>  obj-$(CONFIG_INPUT_GPIO_DECODER)     += gpio_decoder.o
>> +obj-$(CONFIG_INPUT_GSC)                      += gsc-input.o
>>  obj-$(CONFIG_INPUT_HISI_POWERKEY)    += hisi_powerkey.o
>>  obj-$(CONFIG_HP_SDC_RTC)             += hp_sdc_rtc.o
>>  obj-$(CONFIG_INPUT_IMS_PCU)          += ims-pcu.o
>> diff --git a/drivers/input/misc/gsc-input.c b/drivers/input/misc/gsc-input.c
>> new file mode 100644
>> index 0000000..7cf217c
>> --- /dev/null
>> +++ b/drivers/input/misc/gsc-input.c
>> @@ -0,0 +1,196 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Gateworks Corporation
>> + */
>
> Let's keep the same // comment block fir the copyright notice as well.
> An one-line describing what this driver is would be appreciated too.

ok - will have this in v2:

// SPDX-License-Identifier: GPL-2.0
//
// Copyright (C) 2018 Gateworks Corporation
//
// This driver dispatches Linux input events for GSC interrupt events
//


>
>> +#define DEBUG
>
> Please no.
>

oops, did not mean to submit that

>> +
>> +#include <linux/input.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/mfd/gsc.h>
>> +
>> +struct gsc_irq {
>> +     unsigned int irq;
>> +     const char *name;
>> +     unsigned int virq;
>> +};
>> +
>> +static struct gsc_irq gsc_irqs[] = {
>> +     { GSC_IRQ_PB,           "button" },
>> +     { GSC_IRQ_KEY_ERASED,   "key-erased" },
>> +     { GSC_IRQ_EEPROM_WP,    "eeprom-wp" },
>> +     { GSC_IRQ_TAMPER,       "tamper" },
>> +     { GSC_IRQ_SWITCH_HOLD,  "button-held" },
>> +};
>> +
>> +struct gsc_input_info {
>> +     struct device *dev;
>> +     struct gsc_dev *gsc;
>> +     struct input_dev *input;
>> +
>> +     int irq;
>> +     struct work_struct irq_work;
>> +     struct mutex mutex;
>> +};
>> +
>> +static void gsc_input_irq_work(struct work_struct *work)
>> +{
>> +     struct gsc_input_info *info = container_of(work, struct gsc_input_info,
>> +                                                irq_work);
>> +     struct gsc_dev *gsc = info->gsc;
>> +     int i, ret = 0;
>> +     int key, sts;
>> +     struct gsc_irq *gsc_irq = NULL;
>> +
>> +     dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
>> +     mutex_lock(&info->mutex);
>> +
>> +     for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
>> +             if (info->irq == gsc_irqs[i].virq)
>> +                     gsc_irq = &gsc_irqs[i];
>> +     if (!gsc_irq) {
>> +             dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
>> +             mutex_unlock(&info->mutex);
>> +             return;
>> +     }
>> +
>> +     ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, &sts);
>
> Why is this needed? To clear irq? What happens if several events happen
> at the same time? Do we lose one of them?

it was for original debugging and not needed - will remove

>
>> +     if (ret) {
>> +             dev_err(info->dev, "failed to read status register\n");
>> +             mutex_unlock(&info->mutex);
>> +             return;
>> +     }
>> +
>> +     key = -1;
>> +     switch (gsc_irq->virq) {
>> +     case GSC_IRQ_PB:
>> +             key = BTN_0;
>> +             break;
>> +     case GSC_IRQ_KEY_ERASED:
>> +             key = BTN_1;
>> +             break;
>> +     case GSC_IRQ_EEPROM_WP:
>> +             key = BTN_2;
>> +             break;
>> +     case GSC_IRQ_GPIO:
>> +             key = BTN_3;
>> +             break;
>> +     case GSC_IRQ_TAMPER:
>> +             key = BTN_4;
>> +             break;
>> +     case GSC_IRQ_SWITCH_HOLD:
>> +             key = BTN_5;
>
> Could we provide the mapping in DTS instead of hard-coding them?

Yes, that makes sense. I'll propose something like the following in v2:

gsc_input {
   compatible = "gw,gsc-input";

   button {
      label = "user pushbutton";
      linux,code = <256>;
      interrupts = <0>
   };

   key-erased {
      label = "key erased";
      linux,code = <257>;
      interrupts = <1>
   };

   ...
};


>
>> +             break;
>> +     }
>> +
>> +     if (key != -1) {
>> +             dev_dbg(&info->input->dev, "bit%d: key=0x%03x %s\n",
>> +                     gsc_irq->virq, key, gsc_irq->name);
>> +             input_report_key(info->input, key, 1);
>
>                 input_sync();

right - thanks!

>
>> +             input_report_key(info->input, key, 0);
>> +             input_sync(info->input);
>> +     }
>> +
>> +     mutex_unlock(&info->mutex);
>> +}
>> +
>> +static irqreturn_t gsc_input_irq(int irq, void *data)
>> +{
>> +     struct gsc_input_info *info = data;
>> +
>> +     dev_dbg(info->gsc->dev, "%s irq%d\n", __func__, irq);
>> +     info->irq = irq;
>> +     schedule_work(&info->irq_work);
>
> Why not use threaded interrupt?

I am using request_threaded_irq with thread_fn with thread_fn (vs handler).

Do you mean why use a work procedure? I guess I don't need that and
can call input_report_key directly from the irq.

>
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int gsc_input_probe(struct platform_device *pdev)
>> +{
>> +     struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> +     struct gsc_input_info *info;
>> +     struct input_dev *input;
>> +     int ret, i;
>> +
>> +     dev_dbg(&pdev->dev, "%s\n", __func__);
>> +     info = devm_kzalloc(&pdev->dev, sizeof(struct gsc_input_info),
>> +                         GFP_KERNEL);
>> +     if (!info)
>> +             return -ENOMEM;
>> +     info->dev = &pdev->dev;
>> +     info->gsc = gsc;
>> +
>> +     /* Register input device */
>> +     input = devm_input_allocate_device(&pdev->dev);
>> +     if (!input) {
>> +             dev_err(&pdev->dev, "Can't allocate input device\n");
>> +             return -ENOMEM;
>> +     }
>> +     info->input = input;
>> +
>> +     input->name = KBUILD_MODNAME;
>> +     input->dev.parent = &pdev->dev;
>
> Not needed - it is set by devm_input_allocate_device().

ok

>
>> +
>> +     input_set_capability(input, EV_KEY, BTN_0); /* button */
>> +     input_set_capability(input, EV_KEY, BTN_1); /* key erased */
>> +     input_set_capability(input, EV_KEY, BTN_2); /* ee wp */
>> +     input_set_capability(input, EV_KEY, BTN_3); /* gpio */
>> +     input_set_capability(input, EV_KEY, BTN_4); /* tamper */
>> +     input_set_capability(input, EV_KEY, BTN_5); /* button held */
>> +
>> +     ret = input_register_device(input);
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     platform_set_drvdata(pdev, gsc);
>> +     mutex_init(&info->mutex);
>> +     INIT_WORK(&info->irq_work, gsc_input_irq_work);
>> +
>> +     /* Support irq domain */
>> +     for (i = 0; i < ARRAY_SIZE(gsc_irqs); i++) {
>> +             struct gsc_irq *gsc_irq = &gsc_irqs[i];
>> +             int virq;
>> +
>> +             virq = regmap_irq_get_virq(gsc->irq_chip_data, gsc_irq->irq);
>> +             if (virq <= 0)
>> +                     return -EINVAL;
>> +             gsc_irq->virq = virq;
>
> I'd say mapping should be done by MFD piece. You can add interrupts as
> resources and fetch them here.

can you point me to an example dts/driver?

Tim

>
>> +
>> +             ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
>> +                                             gsc_input_irq, 0,
>> +                                             gsc_irq->name, info);
>> +             if (ret) {
>> +                     dev_err(&pdev->dev,
>> +                             "failed: irq request (IRQ: %d, error: %d\n)",
>> +                             gsc_irq->irq, ret);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id gsc_input_dt_ids[] = {
>> +     { .compatible = "gw,gsc-input", },
>> +     {}
>> +};
>> +
>> +static struct platform_driver gsc_input_driver = {
>> +     .driver = {
>> +             .name = KBUILD_MODNAME,
>> +             .of_match_table = gsc_input_dt_ids,
>> +     },
>> +     .probe = gsc_input_probe,
>> +};
>> +
>> +module_platform_driver(gsc_input_driver);
>> +
>> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
>> +MODULE_DESCRIPTION("GSC input driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.7.4
>>
>
> Thanks.
>
> --
> Dmitry

Re: [RFC 2/4] mfd: add Gateworks System Controller core driver

[Tim Harvey]

On Tue, Feb 27, 2018 at 6:00 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 02/27/2018 05:21 PM, Tim Harvey wrote:
>> The Gateworks System Controller (GSC) is an I2C slave controller
>> implemented with an MSP430 micro-controller whose firmware embeds the
>> following features:
>>  - I/O expander (16 GPIO's) using PCA955x protocol
>>  - Real Time Clock using DS1672 protocol
>>  - User EEPROM using AT24 protocol
>>  - HWMON using custom protocol
>>  - Interrupt controller with tamper detect, user pushbotton
>>  - Watchdog controller capable of full board power-cycle
>>  - Power Control capable of full board power-cycle
>>
>> see http://trac.gateworks.com/wiki/gsc for more details
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  drivers/mfd/Kconfig     |  10 ++
>>  drivers/mfd/Makefile    |   1 +
>>  drivers/mfd/gsc.c       | 330 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/gsc.h |  79 ++++++++++++
>>  4 files changed, 420 insertions(+)
>>  create mode 100644 drivers/mfd/gsc.c
>>  create mode 100644 include/linux/mfd/gsc.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 1d20a80..16dd486 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
>>         Select this option to enable support for Samsung Exynos Low Power
>>         Audio Subsystem.
>>
>> +config MFD_GSC
>> +     tristate "Gateworks System Controller"
>> +     depends on (I2C && OF) || COMPILE_TEST
>
> Do both I2C and OF have stubs so that a driver will build when they are
> both disabled?  I.e., only COMPILE_TEST is enabled?

Randy,

No, at this point it requires both I2C and OF. I may add platform data
to support an older non-device-tree family of boards but it still
would require I2C.

I will remove the || COMPILE_TEST

Thanks for catching that.

Tim

Re: [RFC 2/4] mfd: add Gateworks System Controller core driver

[Tim Harvey]

On Wed, Feb 28, 2018 at 10:53 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> +             dev_err(&client->dev, ">> 0x%02x %d\n", reg, ret);
>> +             return ret;
>> +     }
>> +     dev_dbg(&client->dev, ">> 0x%02x=0x%02x (%d)\n", reg, val, retry);
>> +
>> +        return 0;
>
> Hi Tim
>
> There appears to be a few spaces vs tabs issues in this file.
>
>       Andrew
>

Thanks. I'll run through checkpatch prior to v2.

Tim

Re: [RFC 3/4] hwmon: add Gateworks System Controller support

[Tim Harvey]

On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/27/2018 05:21 PM, Tim Harvey wrote:
>>
>> The Gateworks System Controller has a hwmon sub-component that exposes
>> up to 16 ADC's, some of which are temperature sensors, others which are
>> voltage inputs. The ADC configuration (register mapping and name) is
>> configured via device-tree and varies board to board.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>   drivers/hwmon/Kconfig     |   6 +
>>   drivers/hwmon/Makefile    |   1 +
>>   drivers/hwmon/gsc-hwmon.c | 299
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 306 insertions(+)
>>   create mode 100644 drivers/hwmon/gsc-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 7ad0176..9cdc3cb 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -475,6 +475,12 @@ config SENSORS_F75375S
>>           This driver can also be built as a module.  If so, the module
>>           will be called f75375s.
>>   +config SENSORS_GSC
>> +        tristate "Gateworks System Controller ADC"
>> +        depends on MFD_GSC
>> +        help
>> +          Support for the Gateworks System Controller A/D converters.
>> +
>>   config SENSORS_MC13783_ADC
>>           tristate "Freescale MC13783/MC13892 ADC"
>>           depends on MFD_MC13XXX
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 0fe489f..835a536 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)   += g760a.o
>>   obj-$(CONFIG_SENSORS_G762)    += g762.o
>>   obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
>>   obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
>> +obj-$(CONFIG_SENSORS_GSC)      += gsc-hwmon.o
>>   obj-$(CONFIG_SENSORS_GPIO_FAN)        += gpio-fan.o
>>   obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
>>   obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
>> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
>> new file mode 100644
>> index 0000000..3e14bea
>> --- /dev/null
>> +++ b/drivers/hwmon/gsc-hwmon.c
>> @@ -0,0 +1,299 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Gateworks Corporation
>> + */
>> +#define DEBUG

Guenter,

Thanks for the review!

>
>
> Please no.

oops - left that in by mistake.

>
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/mfd/gsc.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +/* map channel to channel info */
>> +struct gsc_hwmon_ch {
>> +       u8 reg;
>> +       char name[32];
>> +};
>> +static struct gsc_hwmon_ch gsc_temp_ch[16];
>
>
> 16 temperature channels ...

It has 16x ADC channels where some can be temperatures and others can
be voltage inputs (based on device tree).

>
>
>> +static struct gsc_hwmon_ch gsc_in_ch[16];
>> +static struct gsc_hwmon_ch gsc_fan_ch[5];
>> +
>> +static int
>> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32
>> attr,
>> +              int ch, long *val)
>> +{
>> +       struct gsc_dev *gsc = dev_get_drvdata(dev);
>> +       int sz, ret;
>> +       u8 reg;
>> +       u8 buf[3];
>> +
>> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> +       switch (type) {
>> +       case hwmon_in:
>> +               sz = 3;
>> +               reg = gsc_in_ch[ch].reg;
>> +               break;
>> +       case hwmon_temp:
>> +               sz = 2;
>> +               reg = gsc_temp_ch[ch].reg;
>> +               break;
>> +       default:
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
>> +       if (!ret) {
>> +               *val = 0;
>> +               while (sz-- > 0)
>> +                       *val |= (buf[sz] << (8*sz));
>> +               if ((type == hwmon_temp) && *val > 0x8000)
>
>
> Excessive [and inconsistent] ( )
>
> Please make this
>         if (ret)
>                 return ret;
>         ...
>         return 0;

understood - a much cleaner pattern

>
>
>> +                       *val -= 0xffff;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int
>> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>> +                     u32 attr, int ch, const char **buf)
>> +{
>> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> +       switch (type) {
>> +       case hwmon_in:
>> +       case hwmon_temp:
>> +       case hwmon_fan:
>> +               switch (attr) {
>> +               case hwmon_in_label:
>> +                       *buf = gsc_in_ch[ch].name;
>> +                       return 0;
>> +                       break;
>> +               case hwmon_temp_label:
>> +                       *buf = gsc_temp_ch[ch].name;
>> +                       return 0;
>> +                       break;
>> +               case hwmon_fan_label:
>> +                       *buf = gsc_fan_ch[ch].name;
>> +                       return 0;
>> +                       break;
>
>
> return followed by break doesn't make sense.

right - removed

>
>
>> +               default:
>> +                       break;
>> +               }
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return -ENOTSUPP;
>> +}
>> +
>> +static int
>> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32
>> attr,
>> +              int ch, long val)
>> +{
>> +       struct gsc_dev *gsc = dev_get_drvdata(dev);
>> +       int ret;
>> +       u8 reg;
>> +       u8 buf[3];
>> +
>> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
>> ch);
>> +       switch (type) {
>> +       case hwmon_fan:
>> +               buf[0] = val & 0xff;
>> +               buf[1] = (val >> 8) & 0xff;
>> +               reg = gsc_fan_ch[ch].reg;
>> +               ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);
>
>
>         &buf -> buf

yikes - thanks for catching that

>
>> +               break;
>> +       default:
>> +               ret = -EOPNOTSUPP;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static umode_t
>> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32
>> attr,
>> +                    int ch)
>> +{
>> +       const struct gsc_dev *gsc = _data;
>> +       struct device *dev = gsc->dev;
>> +       umode_t mode = 0;
>> +
>> +       switch (type) {
>> +       case hwmon_fan:
>> +               if (attr == hwmon_fan_input)
>> +                       mode = (S_IRUGO | S_IWUSR);
>
>
> Unnecessary ( )

ok

>
>
>> +               break;
>> +       case hwmon_temp:
>> +       case hwmon_in:
>> +               mode = S_IRUGO;
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__,
>> type,
>> +               attr, ch, mode);
>> +
>> +       return mode;
>> +}
>> +
>> +static u32 gsc_in_config[] = {
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       HWMON_I_INPUT,
>> +       0
>> +};
>> +static const struct hwmon_channel_info gsc_in = {
>> +       .type = hwmon_in,
>> +       .config = gsc_in_config,
>> +};
>> +
>> +static u32 gsc_temp_config[] = {
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       HWMON_T_INPUT,
>> +       0
>
>
> ... but this array only has 8+1 elements. This seems inconsistent.
> How about using some defines for array sizes ?
>
> Also, why initialize those arrays ? You are overwriting them below.
> You could just use a static size array instead.
>
> I assume it is guaranteed that there is only exactly one instance
> of this device in the system. Have you tried what happens if you
> declare two instances anyway ? The result must be interesting,
> with all those static variables.

yes, that static arrays are not very forward-thinking and yes my
arrays are not consistent. I'll convert to dynamically allocating the
channels for v2

>
>> +};
>> +static const struct hwmon_channel_info gsc_temp = {
>> +       .type = hwmon_temp,
>> +       .config = gsc_temp_config,
>> +};
>> +
>> +static u32 gsc_fan_config[] = {
>> +       HWMON_F_INPUT,
>> +       HWMON_F_INPUT,
>> +       HWMON_F_INPUT,
>> +       HWMON_F_INPUT,
>> +       HWMON_F_INPUT,
>> +       HWMON_F_INPUT,
>> +       0,
>> +};
>
>
> The matching gsc_fan_ch has only 5 entries.

right - certainly an issue

>
>
>> +static const struct hwmon_channel_info gsc_fan = {
>> +       .type = hwmon_fan,
>> +       .config = gsc_fan_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *gsc_info[] = {
>> +       &gsc_temp,
>> +       &gsc_in,
>> +       &gsc_fan,
>> +       NULL
>> +};
>> +
>> +static const struct hwmon_ops gsc_hwmon_ops = {
>> +       .is_visible = gsc_hwmon_is_visible,
>> +       .read = gsc_hwmon_read,
>> +       .read_string = gsc_hwmon_read_string,
>> +       .write = gsc_hwmon_write,
>> +};
>> +
>> +static const struct hwmon_chip_info gsc_chip_info = {
>> +       .ops = &gsc_hwmon_ops,
>> +       .info = gsc_info,
>> +};
>> +
>> +static int gsc_hwmon_probe(struct platform_device *pdev)
>> +{
>> +       struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> +       struct device_node *np;
>> +       struct device *hwmon;
>> +       int temp_count = 0;
>> +       int in_count = 0;
>> +       int fan_count = 0;
>> +       int ret;
>> +
>> +       dev_dbg(&pdev->dev, "%s\n", __func__);
>
>
> You declare local 'dev' variables all over the place, except here,
> where it would actually be used multiple times.
>
> Please either declare one here as well, or drop all the others.

will do

>
>> +       np = of_get_next_child(pdev->dev.of_node, NULL);
>> +       while (np) {
>> +               u32 reg, type;
>> +               const char *label;
>> +
>> +               of_property_read_u32(np, "reg", &reg);
>> +               of_property_read_u32(np, "type", &type);
>> +               label = of_get_property(np, "label", NULL);
>
>
> It must be interesting to see what happens if no 'label' property
> is provided. Have you tried ? Also, no validation of 'reg' and 'type' ?
> Are you sure ?

will add validation

>
>> +               switch(type) {
>> +               case 0: /* temperature sensor */
>> +                       gsc_temp_config[temp_count] = HWMON_T_INPUT |
>> +                                                     HWMON_T_LABEL;
>> +                       gsc_temp_ch[temp_count].reg = reg;
>> +                       strncpy(gsc_temp_ch[temp_count].name, label, 32);
>
>
> This leaves the string unterminated if it is too long. Have you tested
> what happens in this situation ? Consider using strlcpy instead.
>
> Also please use sizeof() instead of '32'.

ok

>
>> +                       if (temp_count < ARRAY_SIZE(gsc_temp_config))
>> +                               temp_count++;
>
>
> I would suggest to abort with EINVAL if this happens. Otherwise the last
> entry
> is overwritten, which doesn't make much sense. Also, this accepts up to
> ARRAY_SIZE()
> entries, leaving no termination.
>
>> +                       break;
>> +               case 1: /* voltage sensor */
>> +                       gsc_in_config[in_count] = HWMON_I_INPUT |
>> +                                                 HWMON_I_LABEL;
>> +                       gsc_in_ch[in_count].reg = reg;
>
>
> So a reg value of 0xXXyy is auto-converted to 0xYY ?

Do you mean stuffing a u32 into a u8?

>
>> +                       strncpy(gsc_in_ch[in_count].name, label, 32);
>> +                       if (in_count < ARRAY_SIZE(gsc_in_config))
>> +                               in_count++;
>> +                       break;
>> +               case 2: /* fan controller setpoint */
>> +                       gsc_fan_config[fan_count] = HWMON_F_INPUT |
>> +                                                   HWMON_F_LABEL;
>> +                       gsc_fan_ch[fan_count].reg = reg;
>
>
> It is going to be interesting to see what happens if there are more than
> 5 such entries.

will fix

>
>> +                       strncpy(gsc_fan_ch[fan_count].name, label, 32);
>> +                       if (fan_count < ARRAY_SIZE(gsc_fan_config))
>> +                               fan_count++;
>> +                       break;
>
>
> All other types are silently ignored ?

will fix

>
>> +               }
>> +               np = of_get_next_child(pdev->dev.of_node, np);
>> +       }
>> +       /* terminate list */
>> +       gsc_in_config[in_count] = 0;
>> +       gsc_temp_config[temp_count] = 0;
>> +       gsc_fan_config[fan_count] = 0;
>> +
>
> I would suggest to move above code into a separate function.

will do

>
>> +       hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
>> +                                                    KBUILD_MODNAME, gsc,
>> +                                                    &gsc_chip_info,
>> NULL);
>> +       if (IS_ERR(hwmon)) {
>> +               ret = PTR_ERR(hwmon);
>> +               dev_err(&pdev->dev, "Unable to register hwmon device:
>> %d\n",
>> +                       ret);
>> +               return ret;
>> +       }
>
>
> The error would be ENOMEM. Is it necessary to report that again ?

could also return -EINVAL but not with the args I'm passing in so I'll
change it to:
return PTR_ERR_OR_ZERO(hwmon);

Thanks!

Tim

Re: [RFC 3/4] hwmon: add Gateworks System Controller support

[Guenter Roeck]

On Wed, Feb 28, 2018 at 01:44:36PM -0800, Tim Harvey wrote:
> On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 02/27/2018 05:21 PM, Tim Harvey wrote:
> >>
> >> The Gateworks System Controller has a hwmon sub-component that exposes
> >> up to 16 ADC's, some of which are temperature sensors, others which are
> >> voltage inputs. The ADC configuration (register mapping and name) is
> >> configured via device-tree and varies board to board.
> >>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >>   drivers/hwmon/Kconfig     |   6 +
> >>   drivers/hwmon/Makefile    |   1 +
> >>   drivers/hwmon/gsc-hwmon.c | 299
> >> ++++++++++++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 306 insertions(+)
> >>   create mode 100644 drivers/hwmon/gsc-hwmon.c
> >>
> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >> index 7ad0176..9cdc3cb 100644
> >> --- a/drivers/hwmon/Kconfig
> >> +++ b/drivers/hwmon/Kconfig
> >> @@ -475,6 +475,12 @@ config SENSORS_F75375S
> >>           This driver can also be built as a module.  If so, the module
> >>           will be called f75375s.
> >>   +config SENSORS_GSC
> >> +        tristate "Gateworks System Controller ADC"
> >> +        depends on MFD_GSC
> >> +        help
> >> +          Support for the Gateworks System Controller A/D converters.
> >> +
> >>   config SENSORS_MC13783_ADC
> >>           tristate "Freescale MC13783/MC13892 ADC"
> >>           depends on MFD_MC13XXX
> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >> index 0fe489f..835a536 100644
> >> --- a/drivers/hwmon/Makefile
> >> +++ b/drivers/hwmon/Makefile
> >> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)   += g760a.o
> >>   obj-$(CONFIG_SENSORS_G762)    += g762.o
> >>   obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> >>   obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> >> +obj-$(CONFIG_SENSORS_GSC)      += gsc-hwmon.o
> >>   obj-$(CONFIG_SENSORS_GPIO_FAN)        += gpio-fan.o
> >>   obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
> >>   obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> >> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
> >> new file mode 100644
> >> index 0000000..3e14bea
> >> --- /dev/null
> >> +++ b/drivers/hwmon/gsc-hwmon.c
> >> @@ -0,0 +1,299 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2018 Gateworks Corporation
> >> + */
> >> +#define DEBUG
> 
> Guenter,
> 
> Thanks for the review!
> 
> >
> >
> > Please no.
> 
> oops - left that in by mistake.
> 
> >
> >> +
> >> +#include <linux/hwmon.h>
> >> +#include <linux/hwmon-sysfs.h>
> >> +#include <linux/mfd/gsc.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/slab.h>
> >> +
> >> +/* map channel to channel info */
> >> +struct gsc_hwmon_ch {
> >> +       u8 reg;
> >> +       char name[32];
> >> +};
> >> +static struct gsc_hwmon_ch gsc_temp_ch[16];
> >
> >
> > 16 temperature channels ...
> 
> It has 16x ADC channels where some can be temperatures and others can
> be voltage inputs (based on device tree).
> 
That was just to point out that you use smaller arrays later on.

> >
> >
> >> +static struct gsc_hwmon_ch gsc_in_ch[16];
> >> +static struct gsc_hwmon_ch gsc_fan_ch[5];
> >> +
> >> +static int
> >> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32
> >> attr,
> >> +              int ch, long *val)
> >> +{
> >> +       struct gsc_dev *gsc = dev_get_drvdata(dev);
> >> +       int sz, ret;
> >> +       u8 reg;
> >> +       u8 buf[3];
> >> +
> >> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> +       switch (type) {
> >> +       case hwmon_in:
> >> +               sz = 3;
> >> +               reg = gsc_in_ch[ch].reg;
> >> +               break;
> >> +       case hwmon_temp:
> >> +               sz = 2;
> >> +               reg = gsc_temp_ch[ch].reg;
> >> +               break;
> >> +       default:
> >> +               return -EOPNOTSUPP;
> >> +       }
> >> +
> >> +       ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz);
> >> +       if (!ret) {
> >> +               *val = 0;
> >> +               while (sz-- > 0)
> >> +                       *val |= (buf[sz] << (8*sz));
> >> +               if ((type == hwmon_temp) && *val > 0x8000)
> >
> >
> > Excessive [and inconsistent] ( )
> >
> > Please make this
> >         if (ret)
> >                 return ret;
> >         ...
> >         return 0;
> 
> understood - a much cleaner pattern
> 
> >
> >
> >> +                       *val -= 0xffff;
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> >> +                     u32 attr, int ch, const char **buf)
> >> +{
> >> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> +       switch (type) {
> >> +       case hwmon_in:
> >> +       case hwmon_temp:
> >> +       case hwmon_fan:
> >> +               switch (attr) {
> >> +               case hwmon_in_label:
> >> +                       *buf = gsc_in_ch[ch].name;
> >> +                       return 0;
> >> +                       break;
> >> +               case hwmon_temp_label:
> >> +                       *buf = gsc_temp_ch[ch].name;
> >> +                       return 0;
> >> +                       break;
> >> +               case hwmon_fan_label:
> >> +                       *buf = gsc_fan_ch[ch].name;
> >> +                       return 0;
> >> +                       break;
> >
> >
> > return followed by break doesn't make sense.
> 
> right - removed
> 
> >
> >
> >> +               default:
> >> +                       break;
> >> +               }
> >> +               break;
> >> +       default:
> >> +               break;
> >> +       }
> >> +
> >> +       return -ENOTSUPP;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32
> >> attr,
> >> +              int ch, long val)
> >> +{
> >> +       struct gsc_dev *gsc = dev_get_drvdata(dev);
> >> +       int ret;
> >> +       u8 reg;
> >> +       u8 buf[3];
> >> +
> >> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr,
> >> ch);
> >> +       switch (type) {
> >> +       case hwmon_fan:
> >> +               buf[0] = val & 0xff;
> >> +               buf[1] = (val >> 8) & 0xff;
> >> +               reg = gsc_fan_ch[ch].reg;
> >> +               ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2);
> >
> >
> >         &buf -> buf
> 
> yikes - thanks for catching that
> 
> >
> >> +               break;
> >> +       default:
> >> +               ret = -EOPNOTSUPP;
> >> +               break;
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static umode_t
> >> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32
> >> attr,
> >> +                    int ch)
> >> +{
> >> +       const struct gsc_dev *gsc = _data;
> >> +       struct device *dev = gsc->dev;
> >> +       umode_t mode = 0;
> >> +
> >> +       switch (type) {
> >> +       case hwmon_fan:
> >> +               if (attr == hwmon_fan_input)
> >> +                       mode = (S_IRUGO | S_IWUSR);
> >
> >
> > Unnecessary ( )
> 
> ok
> 
> >
> >
> >> +               break;
> >> +       case hwmon_temp:
> >> +       case hwmon_in:
> >> +               mode = S_IRUGO;
> >> +               break;
> >> +       default:
> >> +               break;
> >> +       }
> >> +       dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__,
> >> type,
> >> +               attr, ch, mode);
> >> +
> >> +       return mode;
> >> +}
> >> +
> >> +static u32 gsc_in_config[] = {
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       HWMON_I_INPUT,
> >> +       0
> >> +};
> >> +static const struct hwmon_channel_info gsc_in = {
> >> +       .type = hwmon_in,
> >> +       .config = gsc_in_config,
> >> +};
> >> +
> >> +static u32 gsc_temp_config[] = {
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       HWMON_T_INPUT,
> >> +       0
> >
> >
> > ... but this array only has 8+1 elements. This seems inconsistent.
> > How about using some defines for array sizes ?
> >
> > Also, why initialize those arrays ? You are overwriting them below.
> > You could just use a static size array instead.
> >
> > I assume it is guaranteed that there is only exactly one instance
> > of this device in the system. Have you tried what happens if you
> > declare two instances anyway ? The result must be interesting,
> > with all those static variables.
> 
> yes, that static arrays are not very forward-thinking and yes my
> arrays are not consistent. I'll convert to dynamically allocating the
> channels for v2
> 
> >
> >> +};
> >> +static const struct hwmon_channel_info gsc_temp = {
> >> +       .type = hwmon_temp,
> >> +       .config = gsc_temp_config,
> >> +};
> >> +
> >> +static u32 gsc_fan_config[] = {
> >> +       HWMON_F_INPUT,
> >> +       HWMON_F_INPUT,
> >> +       HWMON_F_INPUT,
> >> +       HWMON_F_INPUT,
> >> +       HWMON_F_INPUT,
> >> +       HWMON_F_INPUT,
> >> +       0,
> >> +};
> >
> >
> > The matching gsc_fan_ch has only 5 entries.
> 
> right - certainly an issue
> 
> >
> >
> >> +static const struct hwmon_channel_info gsc_fan = {
> >> +       .type = hwmon_fan,
> >> +       .config = gsc_fan_config,
> >> +};
> >> +
> >> +static const struct hwmon_channel_info *gsc_info[] = {
> >> +       &gsc_temp,
> >> +       &gsc_in,
> >> +       &gsc_fan,
> >> +       NULL
> >> +};
> >> +
> >> +static const struct hwmon_ops gsc_hwmon_ops = {
> >> +       .is_visible = gsc_hwmon_is_visible,
> >> +       .read = gsc_hwmon_read,
> >> +       .read_string = gsc_hwmon_read_string,
> >> +       .write = gsc_hwmon_write,
> >> +};
> >> +
> >> +static const struct hwmon_chip_info gsc_chip_info = {
> >> +       .ops = &gsc_hwmon_ops,
> >> +       .info = gsc_info,
> >> +};
> >> +
> >> +static int gsc_hwmon_probe(struct platform_device *pdev)
> >> +{
> >> +       struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> >> +       struct device_node *np;
> >> +       struct device *hwmon;
> >> +       int temp_count = 0;
> >> +       int in_count = 0;
> >> +       int fan_count = 0;
> >> +       int ret;
> >> +
> >> +       dev_dbg(&pdev->dev, "%s\n", __func__);
> >
> >
> > You declare local 'dev' variables all over the place, except here,
> > where it would actually be used multiple times.
> >
> > Please either declare one here as well, or drop all the others.
> 
> will do
> 
> >
> >> +       np = of_get_next_child(pdev->dev.of_node, NULL);
> >> +       while (np) {
> >> +               u32 reg, type;
> >> +               const char *label;
> >> +
> >> +               of_property_read_u32(np, "reg", &reg);
> >> +               of_property_read_u32(np, "type", &type);
> >> +               label = of_get_property(np, "label", NULL);
> >
> >
> > It must be interesting to see what happens if no 'label' property
> > is provided. Have you tried ? Also, no validation of 'reg' and 'type' ?
> > Are you sure ?
> 
> will add validation
> 
> >
> >> +               switch(type) {
> >> +               case 0: /* temperature sensor */
> >> +                       gsc_temp_config[temp_count] = HWMON_T_INPUT |
> >> +                                                     HWMON_T_LABEL;
> >> +                       gsc_temp_ch[temp_count].reg = reg;
> >> +                       strncpy(gsc_temp_ch[temp_count].name, label, 32);
> >
> >
> > This leaves the string unterminated if it is too long. Have you tested
> > what happens in this situation ? Consider using strlcpy instead.
> >
> > Also please use sizeof() instead of '32'.
> 
> ok
> 
> >
> >> +                       if (temp_count < ARRAY_SIZE(gsc_temp_config))
> >> +                               temp_count++;
> >
> >
> > I would suggest to abort with EINVAL if this happens. Otherwise the last
> > entry
> > is overwritten, which doesn't make much sense. Also, this accepts up to
> > ARRAY_SIZE()
> > entries, leaving no termination.
> >
> >> +                       break;
> >> +               case 1: /* voltage sensor */
> >> +                       gsc_in_config[in_count] = HWMON_I_INPUT |
> >> +                                                 HWMON_I_LABEL;
> >> +                       gsc_in_ch[in_count].reg = reg;
> >
> >
> > So a reg value of 0xXXyy is auto-converted to 0xYY ?
> 
> Do you mean stuffing a u32 into a u8?
> 

That is what you do, isn't it ? So reg=0xffff and reg=0xfeff will both
map to 0xff. Or, in other word, the code happily accepts invalid values
and converts them into something else.

> >
> >> +                       strncpy(gsc_in_ch[in_count].name, label, 32);
> >> +                       if (in_count < ARRAY_SIZE(gsc_in_config))
> >> +                               in_count++;
> >> +                       break;
> >> +               case 2: /* fan controller setpoint */
> >> +                       gsc_fan_config[fan_count] = HWMON_F_INPUT |
> >> +                                                   HWMON_F_LABEL;
> >> +                       gsc_fan_ch[fan_count].reg = reg;
> >
> >
> > It is going to be interesting to see what happens if there are more than
> > 5 such entries.
> 
> will fix
> 
> >
> >> +                       strncpy(gsc_fan_ch[fan_count].name, label, 32);
> >> +                       if (fan_count < ARRAY_SIZE(gsc_fan_config))
> >> +                               fan_count++;
> >> +                       break;
> >
> >
> > All other types are silently ignored ?
> 
> will fix
> 
> >
> >> +               }
> >> +               np = of_get_next_child(pdev->dev.of_node, np);
> >> +       }
> >> +       /* terminate list */
> >> +       gsc_in_config[in_count] = 0;
> >> +       gsc_temp_config[temp_count] = 0;
> >> +       gsc_fan_config[fan_count] = 0;
> >> +
> >
> > I would suggest to move above code into a separate function.
> 
> will do
> 
> >
> >> +       hwmon = devm_hwmon_device_register_with_info(&pdev->dev,
> >> +                                                    KBUILD_MODNAME, gsc,
> >> +                                                    &gsc_chip_info,
> >> NULL);
> >> +       if (IS_ERR(hwmon)) {
> >> +               ret = PTR_ERR(hwmon);
> >> +               dev_err(&pdev->dev, "Unable to register hwmon device:
> >> %d\n",
> >> +                       ret);
> >> +               return ret;
> >> +       }
> >
> >
> > The error would be ENOMEM. Is it necessary to report that again ?
> 
> could also return -EINVAL but not with the args I'm passing in so I'll
> change it to:
> return PTR_ERR_OR_ZERO(hwmon);
> 
Much preferred.

Thanks,
Guenter