* [PATCH v5 0/3] Add support for the Gateworks System Controller
@ 2020-02-24 20:54 Tim Harvey
2020-02-24 20:54 ` [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Tim Harvey @ 2020-02-24 20:54 UTC (permalink / raw)
To: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones
Cc: 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
---
v5:
- fix checkpatch issues
- fix dt_binding_check issues
- address feedback from v4
v4:
- hwmon: move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
- hwmon: remove unncessary resolution/scaling properties for ADCs
- bindings: update to yaml Documentation
- removed watchdog driver
v3:
- removed unnecessary input driver
- added wdt driver
- bindings: encorporated feedback from mailng list
- hwmon:
- encoroprated feedback from mailng list
- added support for raw ADC voltage input used in newer GSC firmware
v2:
- change license comment block style
- remove COMPILE_TEST
- fixed whitespace issues
- replaced a printk with dev_err
- remove DEBUG
- simplify regmap_bulk_read err check
- remove break after returns in switch statement
- fix fan setpoint buffer address
- remove unnecessary parens
- consistently use struct device *dev pointer
- add validation for hwmon child node props
- move parsing of of to own function
- use strlcpy to ensure null termination
- fix static array sizes and removed unnecessary initializers
- dynamically allocate channels
- fix fan input label
- support platform data
Tim Harvey (3):
dt-bindings: mfd: Add Gateworks System Controller bindings
mfd: add Gateworks System Controller core driver
hwmon: add Gateworks System Controller support
.../devicetree/bindings/mfd/gateworks-gsc.yaml | 158 +++++++++
Documentation/hwmon/gsc-hwmon.rst | 53 +++
Documentation/hwmon/index.rst | 1 +
MAINTAINERS | 11 +
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gsc-hwmon.c | 371 +++++++++++++++++++++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 292 ++++++++++++++++
include/linux/mfd/gsc.h | 72 ++++
include/linux/platform_data/gsc_hwmon.h | 44 +++
12 files changed, 1023 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
create mode 100644 Documentation/hwmon/gsc-hwmon.rst
create mode 100644 drivers/hwmon/gsc-hwmon.c
create mode 100644 drivers/mfd/gateworks-gsc.c
create mode 100644 include/linux/mfd/gsc.h
create mode 100644 include/linux/platform_data/gsc_hwmon.h
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-02-24 20:54 [PATCH v5 0/3] Add support for the Gateworks System Controller Tim Harvey
@ 2020-02-24 20:54 ` Tim Harvey
2020-03-02 20:49 ` Rob Herring
2020-02-24 20:54 ` [PATCH v5 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-02-24 20:54 ` [PATCH v5 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2 siblings, 1 reply; 10+ messages in thread
From: Tim Harvey @ 2020-02-24 20:54 UTC (permalink / raw)
To: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones
Cc: Tim Harvey
This patch adds documentation of device-tree bindings for the
Gateworks System Controller (GSC).
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v5:
- resolve dt_binding_check issues
v4:
- move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
- remove unncessary resolution/scaling properties for ADCs
- update to yaml
- remove watchdog
v3:
- replaced _ with -
- remove input bindings
- added full description of hwmon
- fix unit address of hwmon child nodes
---
.../devicetree/bindings/mfd/gateworks-gsc.yaml | 158 +++++++++++++++++++++
1 file changed, 158 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
new file mode 100644
index 00000000..f7c1a05
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
@@ -0,0 +1,158 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Gateworks System Controller multi-function device
+
+description: |
+ The GSC is a Multifunction I2C slave device with the following submodules:
+ - Watchdog Timer
+ - GPIO
+ - Pushbutton controller
+ - Hardware Monitore with ADC's for temperature and voltage rails and
+ fan controller
+
+maintainers:
+ - Tim Harvey <tharvey@gateworks.com>
+ - Robert Jones <rjones@gateworks.com>
+
+properties:
+ $nodename:
+ pattern: "gsc@[0-9a-f]{1,2}"
+ compatible:
+ const: gw,gsc
+
+ reg:
+ description: I2C device address
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 1
+ description: The IRQ number
+
+ hwmon:
+ type: object
+ description: Optional Hardware Monitoring module
+
+ properties:
+ compatible:
+ const: gw,gsc-hwmon
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ gw,fan-base:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: The fan controller base address
+
+ patternProperties:
+ "^adc@[0-2]$":
+ type: object
+ description: |
+ Properties for a single ADC which can report cooked values
+ (ie temperature sensor based on thermister), raw values
+ (ie voltage rail with a pre-scaling resistor divider).
+
+ properties:
+ reg:
+ description: Register of the ADC
+ maxItems: 1
+
+ label:
+ description: Name of the ADC input
+
+ type:
+ description: |
+ temperature in C*10 (temperature),
+ pre-scaled voltage value (voltage),
+ or scaled based on an optional resistor divider and optional
+ offset (voltage-raw)
+ enum:
+ - temperature
+ - voltage
+ - voltage-raw
+
+ gw,voltage-divider:
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: values of resistors for divider on raw ADC input
+ items:
+ - description: R1
+ - description: R2
+
+ gw,voltage-offset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ A positive uV voltage offset to apply to a raw ADC
+ (ie to compensate for a diode drop).
+
+ required:
+ - type
+ - reg
+ - label
+
+ required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ gsc@20 {
+ compatible = "gw,gsc";
+ reg = <0x20>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <4 GPIO_ACTIVE_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ hwmon {
+ compatible = "gw,gsc-hwmon";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ gw,fan-base = <0x2c>;
+
+ adc@0 { /* A0: Board Temperature */
+ type = "temperature";
+ reg = <0x00>;
+ label = "temp";
+ };
+
+ adc@2 { /* A1: Input Voltage (raw ADC) */
+ type = "voltage-raw";
+ reg = <0x02>;
+ label = "vdd_vin";
+ gw,voltage-divider = <22100 1000>;
+ gw,voltage-offset = <800000>;
+ };
+
+ adc@b { /* A2: Battery voltage */
+ type = "voltage";
+ reg = <0x0b>;
+ label = "vdd_bat";
+ };
+ };
+ };
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/3] mfd: add Gateworks System Controller core driver
2020-02-24 20:54 [PATCH v5 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-02-24 20:54 ` [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
@ 2020-02-24 20:54 ` Tim Harvey
2020-02-24 20:54 ` [PATCH v5 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2 siblings, 0 replies; 10+ messages in thread
From: Tim Harvey @ 2020-02-24 20:54 UTC (permalink / raw)
To: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones
Cc: Tim Harvey, Randy Dunlap
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
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
v5:
- simplify powerdown function
v4:
- remove hwmon max reg check/define
- fix powerdown function
v3:
- rename gsc->gateworks-gsc
- remove uncecessary include for linux/mfd/core.h
- upercase I2C in comments
- remove i2c debug
- remove uncecessary comments
- don't use KBUILD_MODNAME for name
- remove unnecessary v1/v2/v3 tracking
- unregister hwmon i2c adapter on remove
v2:
- change license comment block style
- remove COMPILE_TEST (Randy)
- fixed whitespace issues
- replaced a printk with dev_err
merge with mfd
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
MAINTAINERS | 8 ++
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 292 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/gsc.h | 72 +++++++++++
5 files changed, 383 insertions(+)
create mode 100644 drivers/mfd/gateworks-gsc.c
create mode 100644 include/linux/mfd/gsc.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 56765f5..bb79b60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6839,6 +6839,14 @@ F: tools/testing/selftests/futex/
F: tools/perf/bench/futex*
F: Documentation/*futex*
+GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER
+M: Tim Harvey <tharvey@gateworks.com>
+M: Robert Jones <rjones@gateworks.com>
+S: Maintained
+F: Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
+F: drivers/mfd/gateworks-gsc.c
+F: include/linux/mfd/gsc.h
+
GCC PLUGINS
M: Kees Cook <keescook@chromium.org>
R: Emese Revfy <re.emese@gmail.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4209008..d84725a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -407,6 +407,16 @@ config MFD_EXYNOS_LPASS
Select this option to enable support for Samsung Exynos Low Power
Audio Subsystem.
+config MFD_GATEWORKS_GSC
+ tristate "Gateworks System Controller"
+ depends on (I2C && OF)
+ 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 aed99f0..c82b442 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o
obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
+obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o
obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o
diff --git a/drivers/mfd/gateworks-gsc.c b/drivers/mfd/gateworks-gsc.c
new file mode 100644
index 00000000..f22fa63
--- /dev/null
+++ b/drivers/mfd/gateworks-gsc.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The Gateworks System Controller (GSC) is a multi-function
+ * device designed for use in Gateworks Single Board Computers.
+ * The control interface is I2C, with an interrupt. The device supports
+ * system functions such as pushbutton monitoring, multiple ADC's for
+ * voltage and temperature, fan controller, and watchdog monitor.
+ *
+ * Copyright (C) 2020 Gateworks Corporation
+ *
+ */
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.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;
+ }
+
+ 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;
+ }
+ *val = ret & 0xff;
+
+ 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);
+ if (ret)
+ return ret;
+ regs[0] = 1 << GSC_CTRL_1_LATCH_SLEEP_ADD;
+ ret = regmap_update_bits(gsc->regmap, GSC_CTRL_1, regs[0], regs[0]);
+ if (ret)
+ return ret;
+ regs[0] = (1 << GSC_CTRL_1_ACTIVATE_SLEEP) |
+ (1 << GSC_CTRL_1_SLEEP_ENABLE);
+ ret = regmap_update_bits(gsc->regmap, GSC_CTRL_1, regs[0], regs[0]);
+
+ return ret;
+}
+
+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;
+ long value;
+
+ if (strcasecmp(name, "powerdown") == 0) {
+ if (kstrtol(buf, 0, &value) == 0)
+ gsc_powerdown(gsc, value);
+ } else {
+ dev_err(dev, "invalid name '%s\n", name);
+ }
+
+ return count;
+}
+
+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 of_device_id gsc_of_match[] = {
+ { .compatible = "gw,gsc", },
+ { }
+};
+
+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,
+};
+
+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 = "gateworks-gsc",
+ .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_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, ®map_gsc, client,
+ &gsc_regmap_config);
+ if (IS_ERR(gsc->regmap))
+ return PTR_ERR(gsc->regmap);
+
+ if (regmap_read(gsc->regmap, GSC_FW_VER, ®))
+ return -EIO;
+ gsc->fwver = reg;
+
+ regmap_read(gsc->regmap, GSC_FW_CRC, ®);
+ gsc->fwcrc = reg;
+ regmap_read(gsc->regmap, GSC_FW_CRC + 1, ®);
+ gsc->fwcrc |= reg << 8;
+
+ gsc->i2c_hwmon = i2c_new_dummy_device(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, ®map_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 0x%04x\n",
+ gsc->fwver, gsc->fwcrc);
+
+ 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)
+{
+ struct gsc_dev *gsc = i2c_get_clientdata(client);
+
+ sysfs_remove_group(&client->dev.kobj, &attr_group);
+ i2c_unregister_device(gsc->i2c_hwmon);
+
+ return 0;
+}
+
+static struct i2c_driver gsc_driver = {
+ .driver = {
+ .name = "gateworks-gsc",
+ .of_match_table = of_match_ptr(gsc_of_match),
+ },
+ .probe = gsc_probe,
+ .remove = gsc_remove,
+};
+
+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 00000000..ed9e92d
--- /dev/null
+++ b/include/linux/mfd/gsc.h
@@ -0,0 +1,72 @@
+/* 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
+
+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;
+ unsigned int fwver;
+ unsigned short fwcrc;
+};
+
+#endif /* __LINUX_MFD_GSC_H_ */
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/3] hwmon: add Gateworks System Controller support
2020-02-24 20:54 [PATCH v5 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-02-24 20:54 ` [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-02-24 20:54 ` [PATCH v5 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
@ 2020-02-24 20:54 ` Tim Harvey
2020-03-03 20:54 ` Guenter Roeck
2 siblings, 1 reply; 10+ messages in thread
From: Tim Harvey @ 2020-02-24 20:54 UTC (permalink / raw)
To: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones
Cc: 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.
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
v5:
- fix various checkpatch issues
- correct gsc-hwmon.rst in MAINTAINERS
- switch to SENSOR_DEVICE_ATTR_{RW,RO}
- use tmp value to avoid excessive pointer deference
- simplify shift operation
- scale voffset once
- simplify is_visible function
- remove empty line at end of file
v4:
- adjust for uV offset from device-tree
- remove unnecessary optional write function
- remove register range check
- change dev_err prints to use gsc dev
- hard-code resolution/scaling for raw adcs
- describe units of ADC resolution
- move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
- ensure space before/after operators
- remove unnecessary parens
- remove more debugging
- add default case and comment for type_voltage
- remove unnecessary index bounds checks for channel
- remove unnecessary clearing of struct fields
- added Documentation/hwmon/gsc-hwmon.rst
v3:
- add voltage_raw input type and supporting fields
- add channel validation to is_visible function
- remove unnecessary channel validation from read/write functions
v2:
- change license comment style
- remove DEBUG
- simplify regmap_bulk_read err check
- remove break after returns in switch statement
- fix fan setpoint buffer address
- remove unnecessary parens
- consistently use struct device *dev pointer
- change license/comment block
- add validation for hwmon child node props
- move parsing of of to own function
- use strlcpy to ensure null termination
- fix static array sizes and removed unnecessary initializers
- dynamically allocate channels
- fix fan input label
- support platform data
- fixed whitespace issues
---
Documentation/hwmon/gsc-hwmon.rst | 53 +++++
Documentation/hwmon/index.rst | 1 +
MAINTAINERS | 3 +
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gsc-hwmon.c | 371 ++++++++++++++++++++++++++++++++
include/linux/platform_data/gsc_hwmon.h | 44 ++++
7 files changed, 482 insertions(+)
create mode 100644 Documentation/hwmon/gsc-hwmon.rst
create mode 100644 drivers/hwmon/gsc-hwmon.c
create mode 100644 include/linux/platform_data/gsc_hwmon.h
diff --git a/Documentation/hwmon/gsc-hwmon.rst b/Documentation/hwmon/gsc-hwmon.rst
new file mode 100644
index 00000000..d34e508
--- /dev/null
+++ b/Documentation/hwmon/gsc-hwmon.rst
@@ -0,0 +1,53 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver gsc-hwmon
+=======================
+
+Supported chips: Gateworks GSC
+Datasheet: http://trac.gateworks.com/wiki/gsc
+Author: Tim Harvey <tharvey@gateworks.com>
+
+Description:
+------------
+
+This driver supports hardware monitoring for the temperature sensor,
+various ADC's connected to the GSC, and optional FAN controller available
+on some boards.
+
+
+Voltage Monitoring
+------------------
+
+The voltage inputs are scaled either internally or by the driver depending
+on the GSC version and firmware. The values returned by the driver do not need
+further scaling. The voltage input labels provide the voltage rail name:
+
+inX_input Measured voltage (mV).
+inX_label Name of voltage rail.
+
+
+Temperature Monitoring
+----------------------
+
+Temperatures are measured with 12-bit or 10-bit resolution and are scaled
+either internally or by the driver depending on the GSC version and firmware.
+The values returned by the driver reflect millidegree Celcius:
+
+tempX_input Measured temperature.
+tempX_label Name of temperature input.
+
+
+PWM Output Control
+------------------
+
+The GSC features 1 PWM output that operates in automatic mode where the
+PWM value will be scalled depending on 6 temperature boundaries.
+The tempeature boundaries are read-write and in millidegree Celcius and the
+read-only PWM values range from 0 (off) to 255 (full speed).
+Fan speed will be set to minimum (off) when the temperature sensor reads
+less than pwm1_auto_point1_temp and maximum when the temperature sensor
+equals or exceeds pwm1_auto_point6_temp.
+
+pwm1_auto_point1_pwm PWM value.
+pwm1_auto_point1_temp Temperature boundary.
+
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 43cc605..a4fab69 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -58,6 +58,7 @@ Hardware Monitoring Kernel Drivers
ftsteutates
g760a
g762
+ gsc-hwmon
gl518sm
hih6130
ibmaem
diff --git a/MAINTAINERS b/MAINTAINERS
index bb79b60..3f15542 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6846,6 +6846,9 @@ S: Maintained
F: Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
F: drivers/mfd/gateworks-gsc.c
F: include/linux/mfd/gsc.h
+F: Documentation/hwmon/gsc-hwmon.rst
+F: drivers/hwmon/gsc-hwmon.c
+F: include/linux/platform_data/gsc_hwmon.h
GCC PLUGINS
M: Kees Cook <keescook@chromium.org>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 23dfe84..99dae13 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -494,6 +494,15 @@ 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_GATEWORKS_GSC
+ help
+ Support for the Gateworks System Controller A/D converters.
+
+ To compile this driver as a module, choose M here:
+ the module will be called gsc-hwmon.
+
config SENSORS_MC13783_ADC
tristate "Freescale MC13783/MC13892 ADC"
depends on MFD_MC13XXX
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 6db5db9..259cba7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -71,6 +71,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 00000000..d9164a6
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Gateworks System Controller Hardware Monitor module
+ *
+ * Copyright (C) 2020 Gateworks Corporation
+ */
+#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>
+
+#include <linux/platform_data/gsc_hwmon.h>
+
+#define GSC_HWMON_MAX_TEMP_CH 16
+#define GSC_HWMON_MAX_IN_CH 16
+
+#define GSC_HWMON_RESOLUTION 12
+#define GSC_HWMON_VREF 2500
+
+struct gsc_hwmon_data {
+ struct gsc_dev *gsc;
+ struct device *dev;
+ struct gsc_hwmon_platform_data *pdata;
+ const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH];
+ const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH];
+ u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1];
+ u32 in_config[GSC_HWMON_MAX_IN_CH + 1];
+ struct hwmon_channel_info temp_info;
+ struct hwmon_channel_info in_info;
+ const struct hwmon_channel_info *info[4];
+ struct hwmon_chip_info chip;
+};
+
+static ssize_t pwm_auto_point_temp_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ u8 reg = hwmon->pdata->fan_base + (2 * attr->index);
+ u8 regs[2];
+ int ret;
+
+ ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, reg, regs, 2);
+ if (ret)
+ return ret;
+
+ ret = regs[0] | regs[1] << 8;
+ return sprintf(buf, "%d\n", ret * 10);
+}
+
+static ssize_t pwm_auto_point_temp_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ u8 reg = hwmon->pdata->fan_base + (2 * attr->index);
+ u8 regs[2];
+ long temp;
+ int err;
+
+ if (kstrtol(buf, 10, &temp))
+ return -EINVAL;
+
+ temp = clamp_val(temp, 0, 10000);
+ temp = DIV_ROUND_CLOSEST(temp, 10);
+
+ regs[0] = temp & 0xff;
+ regs[1] = (temp >> 8) & 0xff;
+ err = regmap_bulk_write(hwmon->gsc->regmap_hwmon, reg, regs, 2);
+ if (err)
+ return err;
+
+ return count;
+}
+
+static ssize_t pwm_auto_point_pwm_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ return sprintf(buf, "%d\n", 255 * (50 + (attr->index * 10)) / 100);
+}
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point1_pwm, pwm_auto_point_pwm, 0);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_temp, pwm_auto_point_temp, 0);
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point2_pwm, pwm_auto_point_pwm, 1);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_temp, pwm_auto_point_temp, 1);
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point3_pwm, pwm_auto_point_pwm, 2);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point3_temp, pwm_auto_point_temp, 2);
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point4_pwm, pwm_auto_point_pwm, 3);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point4_temp, pwm_auto_point_temp, 3);
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point5_pwm, pwm_auto_point_pwm, 4);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point5_temp, pwm_auto_point_temp, 4);
+
+static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point6_pwm, pwm_auto_point_pwm, 5);
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point6_temp, pwm_auto_point_temp, 5);
+
+static struct attribute *gsc_hwmon_attributes[] = {
+ &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group gsc_hwmon_group = {
+ .attrs = gsc_hwmon_attributes,
+};
+__ATTRIBUTE_GROUPS(gsc_hwmon);
+
+static int
+gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+ const struct gsc_hwmon_channel *ch;
+ int sz, ret;
+ long tmp;
+ u8 buf[3];
+
+ switch (type) {
+ case hwmon_in:
+ ch = hwmon->in_ch[channel];
+ break;
+ case hwmon_temp:
+ ch = hwmon->temp_ch[channel];
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ sz = (ch->type == type_voltage) ? 3 : 2;
+ ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
+ if (ret)
+ return ret;
+
+ tmp = 0;
+ while (sz-- > 0)
+ tmp |= (buf[sz] << (8 * sz));
+
+ switch (ch->type) {
+ case type_temperature:
+ if (tmp > 0x8000)
+ tmp -= 0xffff;
+ break;
+ case type_voltage_raw:
+ tmp = clamp_val(tmp, 0, BIT(GSC_HWMON_RESOLUTION));
+ /* scale based on ref voltage and ADC resolution */
+ tmp *= GSC_HWMON_VREF;
+ tmp >>= GSC_HWMON_RESOLUTION;
+ /* scale based on optional voltage divider */
+ if (ch->vdiv[0] && ch->vdiv[1]) {
+ tmp *= (ch->vdiv[0] + ch->vdiv[1]);
+ tmp /= ch->vdiv[1];
+ }
+ /* adjust by uV offset */
+ tmp += ch->mvoffset;
+ break;
+ case type_voltage:
+ /* no adjustment needed */
+ break;
+ }
+
+ *val = tmp;
+
+ return 0;
+}
+
+static int
+gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **buf)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_in:
+ *buf = hwmon->in_ch[channel]->name;
+ break;
+ case hwmon_temp:
+ *buf = hwmon->temp_ch[channel]->name;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ return 0;
+}
+
+static umode_t
+gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
+ int ch)
+{
+ return 0444;
+}
+
+static const struct hwmon_ops gsc_hwmon_ops = {
+ .is_visible = gsc_hwmon_is_visible,
+ .read = gsc_hwmon_read,
+ .read_string = gsc_hwmon_read_string,
+};
+
+static struct gsc_hwmon_platform_data *
+gsc_hwmon_get_devtree_pdata(struct device *dev)
+{
+ struct gsc_hwmon_platform_data *pdata;
+ struct gsc_hwmon_channel *ch;
+ struct fwnode_handle *child;
+ const char *type;
+ int nchannels;
+
+ nchannels = device_get_child_node_count(dev);
+ if (nchannels == 0)
+ return ERR_PTR(-ENODEV);
+
+ pdata = devm_kzalloc(dev,
+ sizeof(*pdata) + nchannels * sizeof(*ch),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+ ch = (struct gsc_hwmon_channel *)(pdata + 1);
+ pdata->channels = ch;
+ pdata->nchannels = nchannels;
+
+ device_property_read_u32(dev, "gw,fan-base", &pdata->fan_base);
+
+ /* allocate structures for channels and count instances of each type */
+ device_for_each_child_node(dev, child) {
+ if (fwnode_property_read_string(child, "label", &ch->name)) {
+ dev_err(dev, "channel without label\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
+ dev_err(dev, "channel without reg\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (fwnode_property_read_string(child, "type", &type)) {
+ dev_err(dev, "channel without type\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (!strcasecmp(type, "temperature")) {
+ ch->type = type_temperature;
+ } else if (!strcasecmp(type, "voltage")) {
+ ch->type = type_voltage;
+ } else if (!strcasecmp(type, "voltage-raw")) {
+ ch->type = type_voltage_raw;
+ } else {
+ dev_err(dev, "channel without type\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (fwnode_property_read_u32(child, "gw,voltage-offset",
+ &ch->mvoffset))
+ ch->mvoffset /= 1000;
+ fwnode_property_read_u32_array(child, "gw,voltage-divider",
+ ch->vdiv, ARRAY_SIZE(ch->vdiv));
+ ch++;
+ }
+
+ return pdata;
+}
+
+static int gsc_hwmon_probe(struct platform_device *pdev)
+{
+ struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
+ struct gsc_hwmon_data *hwmon;
+ const struct attribute_group **groups;
+ int i, i_in, i_temp;
+
+ if (!pdata) {
+ pdata = gsc_hwmon_get_devtree_pdata(dev);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+ }
+
+ hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return -ENOMEM;
+ hwmon->gsc = gsc;
+ hwmon->pdata = pdata;
+
+ for (i = 0, i_in = 0, i_temp = 0; i < hwmon->pdata->nchannels; i++) {
+ const struct gsc_hwmon_channel *ch = &pdata->channels[i];
+
+ switch (ch->type) {
+ case type_temperature:
+ if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
+ dev_err(gsc->dev, "too many temp channels\n");
+ return -EINVAL;
+ }
+ hwmon->temp_ch[i_temp] = ch;
+ hwmon->temp_config[i_temp] = HWMON_T_INPUT |
+ HWMON_T_LABEL;
+ i_temp++;
+ break;
+ case type_voltage:
+ case type_voltage_raw:
+ if (i_in == GSC_HWMON_MAX_IN_CH) {
+ dev_err(gsc->dev, "too many input channels\n");
+ return -EINVAL;
+ }
+ hwmon->in_ch[i_in] = ch;
+ hwmon->in_config[i_in] =
+ HWMON_I_INPUT | HWMON_I_LABEL;
+ i_in++;
+ break;
+ default:
+ dev_err(gsc->dev, "invalid type: %d\n", ch->type);
+ return -EINVAL;
+ }
+ }
+
+ /* setup config structures */
+ hwmon->chip.ops = &gsc_hwmon_ops;
+ hwmon->chip.info = hwmon->info;
+ hwmon->info[0] = &hwmon->temp_info;
+ hwmon->info[1] = &hwmon->in_info;
+ hwmon->temp_info.type = hwmon_temp;
+ hwmon->temp_info.config = hwmon->temp_config;
+ hwmon->in_info.type = hwmon_in;
+ hwmon->in_info.config = hwmon->in_config;
+
+ groups = pdata->fan_base ? gsc_hwmon_groups : NULL;
+ hwmon->dev = devm_hwmon_device_register_with_info(dev,
+ KBUILD_MODNAME, hwmon,
+ &hwmon->chip, groups);
+ return PTR_ERR_OR_ZERO(hwmon->dev);
+}
+
+static const struct of_device_id gsc_hwmon_of_match[] = {
+ { .compatible = "gw,gsc-hwmon", },
+ {}
+};
+
+static struct platform_driver gsc_hwmon_driver = {
+ .driver = {
+ .name = "gsc-hwmon",
+ .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");
diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
new file mode 100644
index 00000000..09465f8
--- /dev/null
+++ b/include/linux/platform_data/gsc_hwmon.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _GSC_HWMON_H
+#define _GSC_HWMON_H
+
+enum gsc_hwmon_type {
+ type_temperature,
+ type_voltage,
+ type_voltage_raw,
+ type_fan,
+};
+
+/**
+ * struct gsc_hwmon_channel - configuration parameters
+ * @reg: I2C register offset
+ * @type: channel type
+ * @name: channel name
+ * @mvoffset: voltage offset
+ * @vdiv: voltage divider array (2 resistor values in ohms)
+ */
+struct gsc_hwmon_channel {
+ unsigned int reg;
+ unsigned int type;
+ const char *name;
+ unsigned int mvoffset;
+ unsigned int vdiv[2];
+};
+
+/**
+ * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
+ * @channels: pointer to array of gsc_hwmon_channel structures
+ * describing channels
+ * @nchannels: number of elements in @channels array
+ * @vreference: voltage reference (mV)
+ * @resolution: ADC bit resolution
+ * @fan_base: register base for FAN controller
+ */
+struct gsc_hwmon_platform_data {
+ const struct gsc_hwmon_channel *channels;
+ int nchannels;
+ unsigned int resolution;
+ unsigned int vreference;
+ unsigned int fan_base;
+};
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-02-24 20:54 ` [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
@ 2020-03-02 20:49 ` Rob Herring
2020-03-06 15:58 ` Tim Harvey
0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2020-03-02 20:49 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon,
Frank Rowand, devicetree, linux-kernel, Robert Jones
On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> This patch adds documentation of device-tree bindings for the
> Gateworks System Controller (GSC).
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v5:
> - resolve dt_binding_check issues
>
> v4:
> - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> - remove unncessary resolution/scaling properties for ADCs
> - update to yaml
> - remove watchdog
>
> v3:
> - replaced _ with -
> - remove input bindings
> - added full description of hwmon
> - fix unit address of hwmon child nodes
> ---
> .../devicetree/bindings/mfd/gateworks-gsc.yaml | 158 +++++++++++++++++++++
> 1 file changed, 158 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> new file mode 100644
> index 00000000..f7c1a05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Gateworks System Controller multi-function device
> +
> +description: |
> + The GSC is a Multifunction I2C slave device with the following submodules:
> + - Watchdog Timer
> + - GPIO
> + - Pushbutton controller
> + - Hardware Monitore with ADC's for temperature and voltage rails and
typo
> + fan controller
> +
> +maintainers:
> + - Tim Harvey <tharvey@gateworks.com>
> + - Robert Jones <rjones@gateworks.com>
> +
> +properties:
> + $nodename:
> + pattern: "gsc@[0-9a-f]{1,2}"
> + compatible:
> + const: gw,gsc
That's not very specific.
> +
> + reg:
> + description: I2C device address
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 1
> + description: The IRQ number
description is wrong. You can just drop it.
> +
> + hwmon:
'hwmon' is a Linux thing. I'm suspicious...
> + type: object
> + description: Optional Hardware Monitoring module
> +
> + properties:
> + compatible:
> + const: gw,gsc-hwmon
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + gw,fan-base:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: The fan controller base address
Shouldn't this be described as a node in the DT or be implied by the
compatible?
> +
> + patternProperties:
> + "^adc@[0-2]$":
There's only one number space at any level. So if you ever need anything
else at this level, it can't have an address. Just something to
consider.
> + type: object
> + description: |
> + Properties for a single ADC which can report cooked values
> + (ie temperature sensor based on thermister), raw values
> + (ie voltage rail with a pre-scaling resistor divider).
> +
> + properties:
> + reg:
> + description: Register of the ADC
> + maxItems: 1
> +
> + label:
> + description: Name of the ADC input
> +
> + type:
Very generic property name, but it's not generic. Needs a vendor prefix
at least.
> + description: |
> + temperature in C*10 (temperature),
> + pre-scaled voltage value (voltage),
> + or scaled based on an optional resistor divider and optional
> + offset (voltage-raw)
> + enum:
> + - temperature
> + - voltage
> + - voltage-raw
> +
> + gw,voltage-divider:
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: values of resistors for divider on raw ADC input
> + items:
> + - description: R1
> + - description: R2
Needs a standard unit suffix. With that, you can drop the type
reference.
> +
> + gw,voltage-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + A positive uV voltage offset to apply to a raw ADC
> + (ie to compensate for a diode drop).
Needs a unit suffix.
> +
> + required:
> + - type
> + - reg
> + - label
> +
> + required:
> + - compatible
> + - "#address-cells"
> + - "#size-cells"
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + gsc@20 {
> + compatible = "gw,gsc";
> + reg = <0x20>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <4 GPIO_ACTIVE_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + hwmon {
> + compatible = "gw,gsc-hwmon";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + gw,fan-base = <0x2c>;
> +
> + adc@0 { /* A0: Board Temperature */
> + type = "temperature";
> + reg = <0x00>;
> + label = "temp";
> + };
> +
> + adc@2 { /* A1: Input Voltage (raw ADC) */
> + type = "voltage-raw";
> + reg = <0x02>;
> + label = "vdd_vin";
> + gw,voltage-divider = <22100 1000>;
> + gw,voltage-offset = <800000>;
> + };
> +
> + adc@b { /* A2: Battery voltage */
> + type = "voltage";
> + reg = <0x0b>;
> + label = "vdd_bat";
> + };
> + };
> + };
> + };
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] hwmon: add Gateworks System Controller support
2020-02-24 20:54 ` [PATCH v5 3/3] hwmon: add Gateworks System Controller support Tim Harvey
@ 2020-03-03 20:54 ` Guenter Roeck
2020-03-06 16:07 ` Tim Harvey
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2020-03-03 20:54 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Jean Delvare, linux-hwmon, Rob Herring, Frank Rowand,
devicetree, linux-kernel, Robert Jones
On Mon, Feb 24, 2020 at 12:54:25PM -0800, 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.
>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
Couple of minor comments, otherwise looks good from my perspective.
>
> v5:
> - fix various checkpatch issues
> - correct gsc-hwmon.rst in MAINTAINERS
> - switch to SENSOR_DEVICE_ATTR_{RW,RO}
> - use tmp value to avoid excessive pointer deference
> - simplify shift operation
> - scale voffset once
> - simplify is_visible function
> - remove empty line at end of file
>
> v4:
> - adjust for uV offset from device-tree
> - remove unnecessary optional write function
> - remove register range check
> - change dev_err prints to use gsc dev
> - hard-code resolution/scaling for raw adcs
> - describe units of ADC resolution
> - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> - ensure space before/after operators
> - remove unnecessary parens
> - remove more debugging
> - add default case and comment for type_voltage
> - remove unnecessary index bounds checks for channel
> - remove unnecessary clearing of struct fields
> - added Documentation/hwmon/gsc-hwmon.rst
>
> v3:
> - add voltage_raw input type and supporting fields
> - add channel validation to is_visible function
> - remove unnecessary channel validation from read/write functions
>
> v2:
> - change license comment style
> - remove DEBUG
> - simplify regmap_bulk_read err check
> - remove break after returns in switch statement
> - fix fan setpoint buffer address
> - remove unnecessary parens
> - consistently use struct device *dev pointer
> - change license/comment block
> - add validation for hwmon child node props
> - move parsing of of to own function
> - use strlcpy to ensure null termination
> - fix static array sizes and removed unnecessary initializers
> - dynamically allocate channels
> - fix fan input label
> - support platform data
> - fixed whitespace issues
> ---
> Documentation/hwmon/gsc-hwmon.rst | 53 +++++
> Documentation/hwmon/index.rst | 1 +
> MAINTAINERS | 3 +
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gsc-hwmon.c | 371 ++++++++++++++++++++++++++++++++
> include/linux/platform_data/gsc_hwmon.h | 44 ++++
> 7 files changed, 482 insertions(+)
> create mode 100644 Documentation/hwmon/gsc-hwmon.rst
> create mode 100644 drivers/hwmon/gsc-hwmon.c
> create mode 100644 include/linux/platform_data/gsc_hwmon.h
>
> diff --git a/Documentation/hwmon/gsc-hwmon.rst b/Documentation/hwmon/gsc-hwmon.rst
> new file mode 100644
> index 00000000..d34e508
> --- /dev/null
> +++ b/Documentation/hwmon/gsc-hwmon.rst
> @@ -0,0 +1,53 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver gsc-hwmon
> +=======================
> +
> +Supported chips: Gateworks GSC
> +Datasheet: http://trac.gateworks.com/wiki/gsc
> +Author: Tim Harvey <tharvey@gateworks.com>
> +
> +Description:
> +------------
> +
> +This driver supports hardware monitoring for the temperature sensor,
> +various ADC's connected to the GSC, and optional FAN controller available
> +on some boards.
> +
> +
> +Voltage Monitoring
> +------------------
> +
> +The voltage inputs are scaled either internally or by the driver depending
> +on the GSC version and firmware. The values returned by the driver do not need
> +further scaling. The voltage input labels provide the voltage rail name:
> +
> +inX_input Measured voltage (mV).
> +inX_label Name of voltage rail.
> +
> +
> +Temperature Monitoring
> +----------------------
> +
> +Temperatures are measured with 12-bit or 10-bit resolution and are scaled
> +either internally or by the driver depending on the GSC version and firmware.
> +The values returned by the driver reflect millidegree Celcius:
> +
> +tempX_input Measured temperature.
> +tempX_label Name of temperature input.
> +
> +
> +PWM Output Control
> +------------------
> +
> +The GSC features 1 PWM output that operates in automatic mode where the
> +PWM value will be scalled depending on 6 temperature boundaries.
> +The tempeature boundaries are read-write and in millidegree Celcius and the
> +read-only PWM values range from 0 (off) to 255 (full speed).
> +Fan speed will be set to minimum (off) when the temperature sensor reads
> +less than pwm1_auto_point1_temp and maximum when the temperature sensor
> +equals or exceeds pwm1_auto_point6_temp.
> +
> +pwm1_auto_point1_pwm PWM value.
> +pwm1_auto_point1_temp Temperature boundary.
Should be something like
pwm1_auto_point[1-6]_pwm PWM value.
pwm1_auto_point[1-6]_temp Temperature boundary.
> +
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 43cc605..a4fab69 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -58,6 +58,7 @@ Hardware Monitoring Kernel Drivers
> ftsteutates
> g760a
> g762
> + gsc-hwmon
> gl518sm
> hih6130
> ibmaem
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bb79b60..3f15542 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6846,6 +6846,9 @@ S: Maintained
> F: Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> F: drivers/mfd/gateworks-gsc.c
> F: include/linux/mfd/gsc.h
> +F: Documentation/hwmon/gsc-hwmon.rst
> +F: drivers/hwmon/gsc-hwmon.c
> +F: include/linux/platform_data/gsc_hwmon.h
>
> GCC PLUGINS
> M: Kees Cook <keescook@chromium.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 23dfe84..99dae13 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -494,6 +494,15 @@ 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_GATEWORKS_GSC
> + help
> + Support for the Gateworks System Controller A/D converters.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called gsc-hwmon.
> +
> config SENSORS_MC13783_ADC
> tristate "Freescale MC13783/MC13892 ADC"
> depends on MFD_MC13XXX
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 6db5db9..259cba7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -71,6 +71,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 00000000..d9164a6
> --- /dev/null
> +++ b/drivers/hwmon/gsc-hwmon.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Gateworks System Controller Hardware Monitor module
> + *
> + * Copyright (C) 2020 Gateworks Corporation
> + */
> +#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>
> +
> +#include <linux/platform_data/gsc_hwmon.h>
> +
> +#define GSC_HWMON_MAX_TEMP_CH 16
> +#define GSC_HWMON_MAX_IN_CH 16
> +
> +#define GSC_HWMON_RESOLUTION 12
> +#define GSC_HWMON_VREF 2500
> +
> +struct gsc_hwmon_data {
> + struct gsc_dev *gsc;
> + struct device *dev;
> + struct gsc_hwmon_platform_data *pdata;
> + const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH];
> + const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH];
> + u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1];
> + u32 in_config[GSC_HWMON_MAX_IN_CH + 1];
> + struct hwmon_channel_info temp_info;
> + struct hwmon_channel_info in_info;
> + const struct hwmon_channel_info *info[4];
Unless I am missing something, info[3] should be sufficient.
> + struct hwmon_chip_info chip;
> +};
> +
> +static ssize_t pwm_auto_point_temp_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + u8 reg = hwmon->pdata->fan_base + (2 * attr->index);
> + u8 regs[2];
> + int ret;
> +
> + ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, reg, regs, 2);
> + if (ret)
> + return ret;
> +
> + ret = regs[0] | regs[1] << 8;
> + return sprintf(buf, "%d\n", ret * 10);
> +}
> +
> +static ssize_t pwm_auto_point_temp_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + u8 reg = hwmon->pdata->fan_base + (2 * attr->index);
> + u8 regs[2];
> + long temp;
> + int err;
> +
> + if (kstrtol(buf, 10, &temp))
> + return -EINVAL;
> +
> + temp = clamp_val(temp, 0, 10000);
> + temp = DIV_ROUND_CLOSEST(temp, 10);
> +
> + regs[0] = temp & 0xff;
> + regs[1] = (temp >> 8) & 0xff;
> + err = regmap_bulk_write(hwmon->gsc->regmap_hwmon, reg, regs, 2);
> + if (err)
> + return err;
> +
> + return count;
> +}
> +
> +static ssize_t pwm_auto_point_pwm_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%d\n", 255 * (50 + (attr->index * 10)) / 100);
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point1_pwm, pwm_auto_point_pwm, 0);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_temp, pwm_auto_point_temp, 0);
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point2_pwm, pwm_auto_point_pwm, 1);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_temp, pwm_auto_point_temp, 1);
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point3_pwm, pwm_auto_point_pwm, 2);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point3_temp, pwm_auto_point_temp, 2);
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point4_pwm, pwm_auto_point_pwm, 3);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point4_temp, pwm_auto_point_temp, 3);
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point5_pwm, pwm_auto_point_pwm, 4);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point5_temp, pwm_auto_point_temp, 4);
> +
> +static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point6_pwm, pwm_auto_point_pwm, 5);
> +static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point6_temp, pwm_auto_point_temp, 5);
> +
> +static struct attribute *gsc_hwmon_attributes[] = {
> + &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point4_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point5_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point6_pwm.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group gsc_hwmon_group = {
> + .attrs = gsc_hwmon_attributes,
> +};
> +__ATTRIBUTE_GROUPS(gsc_hwmon);
> +
> +static int
> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> + const struct gsc_hwmon_channel *ch;
> + int sz, ret;
> + long tmp;
> + u8 buf[3];
> +
> + switch (type) {
> + case hwmon_in:
> + ch = hwmon->in_ch[channel];
> + break;
> + case hwmon_temp:
> + ch = hwmon->temp_ch[channel];
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + sz = (ch->type == type_voltage) ? 3 : 2;
> + ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
> + if (ret)
> + return ret;
> +
> + tmp = 0;
> + while (sz-- > 0)
> + tmp |= (buf[sz] << (8 * sz));
> +
> + switch (ch->type) {
> + case type_temperature:
> + if (tmp > 0x8000)
> + tmp -= 0xffff;
> + break;
> + case type_voltage_raw:
> + tmp = clamp_val(tmp, 0, BIT(GSC_HWMON_RESOLUTION));
> + /* scale based on ref voltage and ADC resolution */
> + tmp *= GSC_HWMON_VREF;
> + tmp >>= GSC_HWMON_RESOLUTION;
> + /* scale based on optional voltage divider */
> + if (ch->vdiv[0] && ch->vdiv[1]) {
> + tmp *= (ch->vdiv[0] + ch->vdiv[1]);
> + tmp /= ch->vdiv[1];
> + }
> + /* adjust by uV offset */
> + tmp += ch->mvoffset;
> + break;
> + case type_voltage:
> + /* no adjustment needed */
> + break;
> + }
> +
> + *val = tmp;
> +
> + return 0;
> +}
> +
> +static int
> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **buf)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_in:
> + *buf = hwmon->in_ch[channel]->name;
> + break;
> + case hwmon_temp:
> + *buf = hwmon->temp_ch[channel]->name;
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static umode_t
> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> + int ch)
> +{
> + return 0444;
> +}
> +
> +static const struct hwmon_ops gsc_hwmon_ops = {
> + .is_visible = gsc_hwmon_is_visible,
> + .read = gsc_hwmon_read,
> + .read_string = gsc_hwmon_read_string,
> +};
> +
> +static struct gsc_hwmon_platform_data *
> +gsc_hwmon_get_devtree_pdata(struct device *dev)
> +{
> + struct gsc_hwmon_platform_data *pdata;
> + struct gsc_hwmon_channel *ch;
> + struct fwnode_handle *child;
> + const char *type;
> + int nchannels;
> +
> + nchannels = device_get_child_node_count(dev);
> + if (nchannels == 0)
> + return ERR_PTR(-ENODEV);
> +
> + pdata = devm_kzalloc(dev,
> + sizeof(*pdata) + nchannels * sizeof(*ch),
> + GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> + ch = (struct gsc_hwmon_channel *)(pdata + 1);
> + pdata->channels = ch;
> + pdata->nchannels = nchannels;
> +
> + device_property_read_u32(dev, "gw,fan-base", &pdata->fan_base);
> +
> + /* allocate structures for channels and count instances of each type */
> + device_for_each_child_node(dev, child) {
> + if (fwnode_property_read_string(child, "label", &ch->name)) {
> + dev_err(dev, "channel without label\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
> + dev_err(dev, "channel without reg\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (fwnode_property_read_string(child, "type", &type)) {
> + dev_err(dev, "channel without type\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (!strcasecmp(type, "temperature")) {
> + ch->type = type_temperature;
> + } else if (!strcasecmp(type, "voltage")) {
> + ch->type = type_voltage;
> + } else if (!strcasecmp(type, "voltage-raw")) {
> + ch->type = type_voltage_raw;
> + } else {
> + dev_err(dev, "channel without type\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (fwnode_property_read_u32(child, "gw,voltage-offset",
> + &ch->mvoffset))
> + ch->mvoffset /= 1000;
> + fwnode_property_read_u32_array(child, "gw,voltage-divider",
> + ch->vdiv, ARRAY_SIZE(ch->vdiv));
> + ch++;
> + }
> +
> + return pdata;
> +}
> +
> +static int gsc_hwmon_probe(struct platform_device *pdev)
> +{
> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
> + struct gsc_hwmon_data *hwmon;
> + const struct attribute_group **groups;
> + int i, i_in, i_temp;
> +
> + if (!pdata) {
> + pdata = gsc_hwmon_get_devtree_pdata(dev);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> + }
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> + hwmon->gsc = gsc;
> + hwmon->pdata = pdata;
> +
> + for (i = 0, i_in = 0, i_temp = 0; i < hwmon->pdata->nchannels; i++) {
> + const struct gsc_hwmon_channel *ch = &pdata->channels[i];
> +
> + switch (ch->type) {
> + case type_temperature:
> + if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
> + dev_err(gsc->dev, "too many temp channels\n");
> + return -EINVAL;
> + }
> + hwmon->temp_ch[i_temp] = ch;
> + hwmon->temp_config[i_temp] = HWMON_T_INPUT |
> + HWMON_T_LABEL;
> + i_temp++;
> + break;
> + case type_voltage:
> + case type_voltage_raw:
> + if (i_in == GSC_HWMON_MAX_IN_CH) {
> + dev_err(gsc->dev, "too many input channels\n");
> + return -EINVAL;
> + }
> + hwmon->in_ch[i_in] = ch;
> + hwmon->in_config[i_in] =
> + HWMON_I_INPUT | HWMON_I_LABEL;
> + i_in++;
> + break;
> + default:
> + dev_err(gsc->dev, "invalid type: %d\n", ch->type);
> + return -EINVAL;
> + }
> + }
> +
> + /* setup config structures */
> + hwmon->chip.ops = &gsc_hwmon_ops;
> + hwmon->chip.info = hwmon->info;
> + hwmon->info[0] = &hwmon->temp_info;
> + hwmon->info[1] = &hwmon->in_info;
> + hwmon->temp_info.type = hwmon_temp;
> + hwmon->temp_info.config = hwmon->temp_config;
> + hwmon->in_info.type = hwmon_in;
> + hwmon->in_info.config = hwmon->in_config;
> +
> + groups = pdata->fan_base ? gsc_hwmon_groups : NULL;
> + hwmon->dev = devm_hwmon_device_register_with_info(dev,
> + KBUILD_MODNAME, hwmon,
> + &hwmon->chip, groups);
> + return PTR_ERR_OR_ZERO(hwmon->dev);
> +}
> +
> +static const struct of_device_id gsc_hwmon_of_match[] = {
> + { .compatible = "gw,gsc-hwmon", },
> + {}
> +};
> +
> +static struct platform_driver gsc_hwmon_driver = {
> + .driver = {
> + .name = "gsc-hwmon",
> + .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");
> diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
> new file mode 100644
> index 00000000..09465f8
> --- /dev/null
> +++ b/include/linux/platform_data/gsc_hwmon.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _GSC_HWMON_H
> +#define _GSC_HWMON_H
> +
> +enum gsc_hwmon_type {
> + type_temperature,
> + type_voltage,
> + type_voltage_raw,
> + type_fan,
> +};
> +
> +/**
> + * struct gsc_hwmon_channel - configuration parameters
> + * @reg: I2C register offset
> + * @type: channel type
> + * @name: channel name
> + * @mvoffset: voltage offset
> + * @vdiv: voltage divider array (2 resistor values in ohms)
> + */
> +struct gsc_hwmon_channel {
> + unsigned int reg;
> + unsigned int type;
> + const char *name;
> + unsigned int mvoffset;
> + unsigned int vdiv[2];
> +};
> +
> +/**
> + * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
> + * @channels: pointer to array of gsc_hwmon_channel structures
> + * describing channels
> + * @nchannels: number of elements in @channels array
> + * @vreference: voltage reference (mV)
> + * @resolution: ADC bit resolution
> + * @fan_base: register base for FAN controller
> + */
> +struct gsc_hwmon_platform_data {
> + const struct gsc_hwmon_channel *channels;
> + int nchannels;
> + unsigned int resolution;
> + unsigned int vreference;
> + unsigned int fan_base;
> +};
> +#endif
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-02 20:49 ` Rob Herring
@ 2020-03-06 15:58 ` Tim Harvey
2020-03-06 17:50 ` Rob Herring
0 siblings, 1 reply; 10+ messages in thread
From: Tim Harvey @ 2020-03-06 15:58 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon,
Frank Rowand, devicetree, open list, Robert Jones
On Mon, Mar 2, 2020 at 12:49 PM Rob Herring <robh@kernel.org> wrote:
>
Rob,
Thanks for the review! Some questions below:
> On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> > This patch adds documentation of device-tree bindings for the
> > Gateworks System Controller (GSC).
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> > v5:
> > - resolve dt_binding_check issues
> >
> > v4:
> > - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> > - remove unncessary resolution/scaling properties for ADCs
> > - update to yaml
> > - remove watchdog
> >
> > v3:
> > - replaced _ with -
> > - remove input bindings
> > - added full description of hwmon
> > - fix unit address of hwmon child nodes
> > ---
> > .../devicetree/bindings/mfd/gateworks-gsc.yaml | 158 +++++++++++++++++++++
> > 1 file changed, 158 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > new file mode 100644
> > index 00000000..f7c1a05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > @@ -0,0 +1,158 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Gateworks System Controller multi-function device
> > +
> > +description: |
> > + The GSC is a Multifunction I2C slave device with the following submodules:
> > + - Watchdog Timer
> > + - GPIO
> > + - Pushbutton controller
> > + - Hardware Monitore with ADC's for temperature and voltage rails and
>
> typo
will fix
>
> > + fan controller
> > +
> > +maintainers:
> > + - Tim Harvey <tharvey@gateworks.com>
> > + - Robert Jones <rjones@gateworks.com>
> > +
> > +properties:
> > + $nodename:
> > + pattern: "gsc@[0-9a-f]{1,2}"
> > + compatible:
> > + const: gw,gsc
>
> That's not very specific.
>
Do you mean something like 'gw,system-controller' would be better
instead of the gsc abbreviation for 'Gateworks System Controller'?
> +
> > + reg:
> > + description: I2C device address
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + interrupt-controller: true
> > +
> > + "#interrupt-cells":
> > + const: 1
> > + description: The IRQ number
>
> description is wrong. You can just drop it.
>
ok
> > +
> > + hwmon:
>
> 'hwmon' is a Linux thing. I'm suspicious...
>
Yes, we've discussed this before and I understand that DT shouldn't
use terminology that is Linux specific (which is why I replaced
'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
in them.
Perhaps this makes more sense?
adc {
compatible = "gw,gsc-adc";
#address-cells = <1>;
#size-cells = <0>;
channel@6 {
type = "gw,hwmon-temperature";
reg = <0x06>;
label = "temp";
};
...
};
> > + type: object
> > + description: Optional Hardware Monitoring module
> > +
> > + properties:
> > + compatible:
> > + const: gw,gsc-hwmon
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + gw,fan-base:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: The fan controller base address
>
> Shouldn't this be described as a node in the DT or be implied by the
> compatible?
It does look out of place there. Would adding another subnode outside
of the (perhaps misnamed) 'hwmon' node make more sense?:
fan:
properties:
compatible: gw,gsc-fancontroller
reg:
description: address of the fan controller base register
maxItems: 1
>
> > +
> > + patternProperties:
> > + "^adc@[0-2]$":
>
> There's only one number space at any level. So if you ever need anything
> else at this level, it can't have an address. Just something to
> consider.
>
yes, one number space is ok if I understand what you mean but I meant
this to be "^adc@[0-9]+$" to support the number of ADC pins the part
supports.
> > + type: object
> > + description: |
> > + Properties for a single ADC which can report cooked values
> > + (ie temperature sensor based on thermister), raw values
> > + (ie voltage rail with a pre-scaling resistor divider).
> > +
> > + properties:
> > + reg:
> > + description: Register of the ADC
> > + maxItems: 1
> > +
> > + label:
> > + description: Name of the ADC input
> > +
> > + type:
>
> Very generic property name, but it's not generic. Needs a vendor prefix
> at least.
You mean the property name of 'type' is fine, but the values will need
to be vendor specific like 'gw,temperature', 'gw,voltage',
'gw,voltage-raw' or is it inappropriate to use 'type'?
>
> > + description: |
> > + temperature in C*10 (temperature),
> > + pre-scaled voltage value (voltage),
> > + or scaled based on an optional resistor divider and optional
> > + offset (voltage-raw)
> > + enum:
> > + - temperature
> > + - voltage
> > + - voltage-raw
> > +
> > + gw,voltage-divider:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > + description: values of resistors for divider on raw ADC input
> > + items:
> > + - description: R1
> > + - description: R2
>
> Needs a standard unit suffix. With that, you can drop the type
> reference.
I understand the unit suffix but not sure what you mean by type
reference. Do you mean:
gw,voltage-divider-milli-ohms:
description: values of resistors for divider on raw ADC input
items:
- description: R1
- description: R2
>
> > +
> > + gw,voltage-offset:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + A positive uV voltage offset to apply to a raw ADC
> > + (ie to compensate for a diode drop).
>
> Needs a unit suffix.
ok
Thanks!
Tim
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] hwmon: add Gateworks System Controller support
2020-03-03 20:54 ` Guenter Roeck
@ 2020-03-06 16:07 ` Tim Harvey
0 siblings, 0 replies; 10+ messages in thread
From: Tim Harvey @ 2020-03-06 16:07 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lee Jones, Jean Delvare, linux-hwmon, Rob Herring, Frank Rowand,
devicetree, open list, Robert Jones
On Tue, Mar 3, 2020 at 12:54 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Feb 24, 2020 at 12:54:25PM -0800, 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.
> >
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
> Couple of minor comments, otherwise looks good from my perspective.
>
Guenter,
Thank you for the review! I will post a v6 as soon as I get the dt
bindings worked through.
Tim
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-06 15:58 ` Tim Harvey
@ 2020-03-06 17:50 ` Rob Herring
2020-03-06 20:02 ` Tim Harvey
0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2020-03-06 17:50 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Jean Delvare, Guenter Roeck, Linux HWMON List,
Frank Rowand, devicetree, open list, Robert Jones
On Fri, Mar 6, 2020 at 9:58 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 2, 2020 at 12:49 PM Rob Herring <robh@kernel.org> wrote:
> >
>
> Rob,
>
> Thanks for the review! Some questions below:
>
> > On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> > > This patch adds documentation of device-tree bindings for the
> > > Gateworks System Controller (GSC).
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > ---
> > > v5:
> > > - resolve dt_binding_check issues
> > >
> > > v4:
> > > - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> > > - remove unncessary resolution/scaling properties for ADCs
> > > - update to yaml
> > > - remove watchdog
> > >
> > > v3:
> > > - replaced _ with -
> > > - remove input bindings
> > > - added full description of hwmon
> > > - fix unit address of hwmon child nodes
> > > ---
> > > .../devicetree/bindings/mfd/gateworks-gsc.yaml | 158 +++++++++++++++++++++
> > > 1 file changed, 158 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > > new file mode 100644
> > > index 00000000..f7c1a05
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > > @@ -0,0 +1,158 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Gateworks System Controller multi-function device
> > > +
> > > +description: |
> > > + The GSC is a Multifunction I2C slave device with the following submodules:
> > > + - Watchdog Timer
> > > + - GPIO
> > > + - Pushbutton controller
> > > + - Hardware Monitore with ADC's for temperature and voltage rails and
> >
> > typo
>
> will fix
>
> >
> > > + fan controller
> > > +
> > > +maintainers:
> > > + - Tim Harvey <tharvey@gateworks.com>
> > > + - Robert Jones <rjones@gateworks.com>
> > > +
> > > +properties:
> > > + $nodename:
> > > + pattern: "gsc@[0-9a-f]{1,2}"
> > > + compatible:
> > > + const: gw,gsc
> >
> > That's not very specific.
> >
>
> Do you mean something like 'gw,system-controller' would be better
> instead of the gsc abbreviation for 'Gateworks System Controller'?
No, I mean is there or will there be only one version of this?
>
> > +
> > > + reg:
> > > + description: I2C device address
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + maxItems: 1
> > > +
> > > + interrupt-controller: true
> > > +
> > > + "#interrupt-cells":
> > > + const: 1
> > > + description: The IRQ number
> >
> > description is wrong. You can just drop it.
> >
>
> ok
>
> > > +
> > > + hwmon:
> >
> > 'hwmon' is a Linux thing. I'm suspicious...
> >
>
> Yes, we've discussed this before and I understand that DT shouldn't
> use terminology that is Linux specific (which is why I replaced
> 'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
> dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
> in them.
>
> Perhaps this makes more sense?
Yes, that's more aligned with IIO ADC bindings. Yes, IIO is again a
Linuxism, but I think the ADC bindings are fairly independent other
than the directory name.
>
> adc {
> compatible = "gw,gsc-adc";
> #address-cells = <1>;
> #size-cells = <0>;
>
> channel@6 {
> type = "gw,hwmon-temperature";
> reg = <0x06>;
> label = "temp";
> };
> ...
> };
>
>
> > > + type: object
> > > + description: Optional Hardware Monitoring module
> > > +
> > > + properties:
> > > + compatible:
> > > + const: gw,gsc-hwmon
> > > +
> > > + "#address-cells":
> > > + const: 1
> > > +
> > > + "#size-cells":
> > > + const: 0
> > > +
> > > + gw,fan-base:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description: The fan controller base address
> >
> > Shouldn't this be described as a node in the DT or be implied by the
> > compatible?
>
> It does look out of place there. Would adding another subnode outside
> of the (perhaps misnamed) 'hwmon' node make more sense?:
>
> fan:
> properties:
> compatible: gw,gsc-fancontroller
> reg:
> description: address of the fan controller base register
> maxItems: 1
Seems somewhat better location in that the first level is
sub-functions of this chip.
But now you have 'adc' with no address and 'fan' (w/ reg should be
fan@...) with an address, so that's not consistent.
Also, I think fan controllers and fans need to have separate nodes as
there are different types of fans such as with and without tach
signals. I've tried to steer other fan bindings that way. Depends how
complex the fan controller is whether that's necessary.
> > > + patternProperties:
> > > + "^adc@[0-2]$":
> >
> > There's only one number space at any level. So if you ever need anything
> > else at this level, it can't have an address. Just something to
> > consider.
> >
>
> yes, one number space is ok if I understand what you mean but I meant
> this to be "^adc@[0-9]+$" to support the number of ADC pins the part
> supports.
>
> > > + type: object
> > > + description: |
> > > + Properties for a single ADC which can report cooked values
> > > + (ie temperature sensor based on thermister), raw values
> > > + (ie voltage rail with a pre-scaling resistor divider).
> > > +
> > > + properties:
> > > + reg:
> > > + description: Register of the ADC
> > > + maxItems: 1
> > > +
> > > + label:
> > > + description: Name of the ADC input
> > > +
> > > + type:
> >
> > Very generic property name, but it's not generic. Needs a vendor prefix
> > at least.
>
> You mean the property name of 'type' is fine, but the values will need
> to be vendor specific like 'gw,temperature', 'gw,voltage',
> 'gw,voltage-raw' or is it inappropriate to use 'type'?
Don't use 'type'.
Is this for 'how to setup/program the adc' or 'what am I measuring'?
For example, configure the adc for temperature readings vs. measure
CPU temperature. Seems like a common thing needed for ADC. 'label'
already covers the latter case.
> > > + description: |
> > > + temperature in C*10 (temperature),
> > > + pre-scaled voltage value (voltage),
> > > + or scaled based on an optional resistor divider and optional
> > > + offset (voltage-raw)
> > > + enum:
> > > + - temperature
> > > + - voltage
> > > + - voltage-raw
> > > +
> > > + gw,voltage-divider:
> > > + allOf:
> > > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > > + description: values of resistors for divider on raw ADC input
> > > + items:
> > > + - description: R1
> > > + - description: R2
> >
> > Needs a standard unit suffix. With that, you can drop the type
> > reference.
>
> I understand the unit suffix but not sure what you mean by type
> reference. Do you mean:
>
> gw,voltage-divider-milli-ohms:
> description: values of resistors for divider on raw ADC input
> items:
> - description: R1
> - description: R2
Yes, drop the '$ref'.
Rob
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-06 17:50 ` Rob Herring
@ 2020-03-06 20:02 ` Tim Harvey
0 siblings, 0 replies; 10+ messages in thread
From: Tim Harvey @ 2020-03-06 20:02 UTC (permalink / raw)
To: Rob Herring
Cc: Lee Jones, Jean Delvare, Guenter Roeck, Linux HWMON List,
Frank Rowand, devicetree, open list, Robert Jones
On Fri, Mar 6, 2020 at 9:50 AM Rob Herring <robh@kernel.org> wrote:
>
<snip>
> > > > +properties:
> > > > + $nodename:
> > > > + pattern: "gsc@[0-9a-f]{1,2}"
> > > > + compatible:
> > > > + const: gw,gsc
> > >
> > > That's not very specific.
> > >
> >
> > Do you mean something like 'gw,system-controller' would be better
> > instead of the gsc abbreviation for 'Gateworks System Controller'?
>
> No, I mean is there or will there be only one version of this?
>
currently just 1 version is enough
> >
<snip>
> >
> > > > +
> > > > + hwmon:
> > >
> > > 'hwmon' is a Linux thing. I'm suspicious...
> > >
> >
> > Yes, we've discussed this before and I understand that DT shouldn't
> > use terminology that is Linux specific (which is why I replaced
> > 'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
> > dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
> > in them.
> >
> > Perhaps this makes more sense?
>
> Yes, that's more aligned with IIO ADC bindings. Yes, IIO is again a
> Linuxism, but I think the ADC bindings are fairly independent other
> than the directory name.
>
> >
> > adc {
> > compatible = "gw,gsc-adc";
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > channel@6 {
> > type = "gw,hwmon-temperature";
> > reg = <0x06>;
> > label = "temp";
> > };
> > ...
> > };
> >
ok, will use adc/channel instead of hwmon/adc and change compatible to
'gw,gsc-adc'
> >
> > > > + type: object
> > > > + description: Optional Hardware Monitoring module
> > > > +
> > > > + properties:
> > > > + compatible:
> > > > + const: gw,gsc-hwmon
> > > > +
> > > > + "#address-cells":
> > > > + const: 1
> > > > +
> > > > + "#size-cells":
> > > > + const: 0
> > > > +
> > > > + gw,fan-base:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + description: The fan controller base address
> > >
> > > Shouldn't this be described as a node in the DT or be implied by the
> > > compatible?
> >
> > It does look out of place there. Would adding another subnode outside
> > of the (perhaps misnamed) 'hwmon' node make more sense?:
> >
> > fan:
> > properties:
> > compatible: gw,gsc-fancontroller
> > reg:
> > description: address of the fan controller base register
> > maxItems: 1
>
> Seems somewhat better location in that the first level is
> sub-functions of this chip.
>
> But now you have 'adc' with no address and 'fan' (w/ reg should be
> fan@...) with an address, so that's not consistent.
>
> Also, I think fan controllers and fans need to have separate nodes as
> there are different types of fans such as with and without tach
> signals. I've tried to steer other fan bindings that way. Depends how
> complex the fan controller is whether that's necessary.
>
The fan controller does now support a tach signal reported via one of
the ADC channels (which I've neglected to cover) so I can represent
that as well in a new first level node such as:
fan:
type: object
description: Optional FAN controller
properties:
compatible:
const: gw,gsc-fan
reg:
description: The fan controller base address
maxItems: 1
gw,fan-tach-ch:
description: The fan tach ADC channel
maxItems: 1
required:
- compatible
- reg
fan {
compatible = "gw,gsc-pwm-fan";
reg = <0x2c>;
gw,fan-tach-ch = <0x16>;
};
<snip>
> >
> > > > + type: object
> > > > + description: |
> > > > + Properties for a single ADC which can report cooked values
> > > > + (ie temperature sensor based on thermister), raw values
> > > > + (ie voltage rail with a pre-scaling resistor divider).
> > > > +
> > > > + properties:
> > > > + reg:
> > > > + description: Register of the ADC
> > > > + maxItems: 1
> > > > +
> > > > + label:
> > > > + description: Name of the ADC input
> > > > +
> > > > + type:
> > >
> > > Very generic property name, but it's not generic. Needs a vendor prefix
> > > at least.
> >
> > You mean the property name of 'type' is fine, but the values will need
> > to be vendor specific like 'gw,temperature', 'gw,voltage',
> > 'gw,voltage-raw' or is it inappropriate to use 'type'?
>
> Don't use 'type'.
>
> Is this for 'how to setup/program the adc' or 'what am I measuring'?
> For example, configure the adc for temperature readings vs. measure
> CPU temperature. Seems like a common thing needed for ADC. 'label'
> already covers the latter case.
This is for translation of the raw ADC to a cooked value. An earlier
version of the GSC reported cooked values (doing the scaling in the
GSC firmware) and later versions report raw values which need to be
scaled depending on optional voltage divider so you can consider that
'setup'. Instead of handling this via a 'version' of the GSC I elected
to describe the difference in ADC channel type as I already had on
that reported millidegree celcius vs millivolts. I could just move
them to properties such as:
gw,temperature
gw,voltage
gw,voltage-raw
Only one of the above is allowed and am not sure how to represent that
in the yaml.
Alternatively I could call this property name 'gw,conversion' and
leave the three type enum?
>
> > > > + description: |
> > > > + temperature in C*10 (temperature),
> > > > + pre-scaled voltage value (voltage),
> > > > + or scaled based on an optional resistor divider and optional
> > > > + offset (voltage-raw)
> > > > + enum:
> > > > + - temperature
> > > > + - voltage
> > > > + - voltage-raw
> > > > +
> > > > + gw,voltage-divider:
> > > > + allOf:
> > > > + - $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > + description: values of resistors for divider on raw ADC input
> > > > + items:
> > > > + - description: R1
> > > > + - description: R2
> > >
> > > Needs a standard unit suffix. With that, you can drop the type
> > > reference.
> >
> > I understand the unit suffix but not sure what you mean by type
> > reference. Do you mean:
> >
> > gw,voltage-divider-milli-ohms:
> > description: values of resistors for divider on raw ADC input
> > items:
> > - description: R1
> > - description: R2
>
> Yes, drop the '$ref'.
>
ok,
Thanks,
Tim
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-06 20:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24 20:54 [PATCH v5 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-02-24 20:54 ` [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-03-02 20:49 ` Rob Herring
2020-03-06 15:58 ` Tim Harvey
2020-03-06 17:50 ` Rob Herring
2020-03-06 20:02 ` Tim Harvey
2020-02-24 20:54 ` [PATCH v5 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-02-24 20:54 ` [PATCH v5 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2020-03-03 20:54 ` Guenter Roeck
2020-03-06 16:07 ` Tim Harvey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.