* [PATCH v8 0/3] Add support for the Gateworks System Controller
@ 2020-03-27 20:33 Tim Harvey
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Tim Harvey @ 2020-03-27 20:33 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
---
v8:
- mfd: whitespace fixes
- mfd: describe sub-devices in Kconfig
- mfd: add error print for invalid command
- mfd: update copyright
- mfd: use devm_of_platform_populate
- mfd: use probe_new
- mfd: move hwmon's regmap init to hwmon
- hwmon: move hwmon's regmap init to hwmon
- dt-bindings: add register to fan-controller node name
v7:
- dt-bindings: change divider from mili-ohms to ohms
- dt-bindings: add constraints for voltage divider and offset
- dt-bindings: remove unnecessary ref for offset
- dt-bindings: renamed fan to fan-controller and changed base prop to reg
- mfd: remove irq from private data struct
- hwmon: fix whitespace in Kconfig
- hwmon: remove unnecessary device pointer in private data
- hwmon: change divider from mili-ohms to ohms
- hwmon: move fan base property to reg
v6:
- hwmon: fix size of info field
- hwmon: improve pwm output control documentation
- hwmon: include unit suffix in divider and offset
- hwmon: change subnode name to gsc-adc
- hwmon: change to fan subnode
- hwmon: fix voltage offset
- dt-bindings: fix commit message typo
- dt-bindings: drop invalid description from #interrupt-cells property
- dt-bindings: fix adc pattern property
- dt-bindings: add unit suffix
- dt-bindings: replace hwmon/adc with adc/channel
- dt-bindings: changed adc type to gw,mode
- dt-bindings: add unit suffix and drop ref for voltage-divider
- dt-bindings: add unit suffix for voltage-offset
- dt-bindings: moved fan to its own subnode with base register
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 | 194 +++++++++++
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 | 385 +++++++++++++++++++++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 288 +++++++++++++++
include/linux/mfd/gsc.h | 73 ++++
include/linux/platform_data/gsc_hwmon.h | 44 +++
12 files changed, 1070 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] 15+ messages in thread
* [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-27 20:33 [PATCH v8 0/3] Add support for the Gateworks System Controller Tim Harvey
@ 2020-03-27 20:33 ` Tim Harvey
2020-03-30 16:03 ` Rob Herring
2020-04-17 9:58 ` Lee Jones
2020-03-27 20:33 ` [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-03-27 20:33 ` [PATCH v8 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2 siblings, 2 replies; 15+ messages in thread
From: Tim Harvey @ 2020-03-27 20:33 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>
---
v8:
- add register to fan-controller node name
v7:
- change divider from mili-ohms to ohms
- add constraints for voltage divider and offset
- remove unnecessary ref for offset
- renamed fan to fan-controller and changed base prop to reg
v6:
- fix typo
- drop invalid description from #interrupt-cells property
- fix adc pattern property
- add unit suffix
- replace hwmon/adc with adc/channel
- changed adc type to mode and enum int
- add unit suffix and drop ref for voltage-divider
- moved fan to its own subnode with base register
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 | 194 +++++++++++++++++++++
1 file changed, 194 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..a96751c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
@@ -0,0 +1,194 @@
+# 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 Monitor 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
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ adc:
+ type: object
+ description: Optional Hardware Monitoring module
+
+ properties:
+ compatible:
+ const: gw,gsc-adc
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ patternProperties:
+ "^channel@[0-9]+$":
+ 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
+
+ gw,mode:
+ description: |
+ conversion mode:
+ 0 - temperature, in C*10
+ 1 - pre-scaled voltage value
+ 2 - scaled voltage based on an optional resistor divider
+ and optional offset
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2]
+
+ gw,voltage-divider-ohms:
+ description: values of resistors for divider on raw ADC input
+ maxItems: 2
+ items:
+ minimum: 1000
+ maximum: 1000000
+
+ gw,voltage-offset-microvolt:
+ description: |
+ A positive voltage offset to apply to a raw ADC
+ (ie to compensate for a diode drop).
+ minimum: 0
+ maximum: 1000000
+
+ required:
+ - gw,mode
+ - reg
+ - label
+
+ required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+
+patternProperties:
+ "^fan-controller@[0-9a-f]+$":
+ type: object
+ description: Optional FAN controller
+
+ properties:
+ compatible:
+ const: gw,gsc-fan
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ reg:
+ description: The fan controller base address
+ maxItems: 1
+
+ required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+ - "#address-cells"
+ - "#size-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>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc {
+ compatible = "gw,gsc-adc";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 { /* A0: Board Temperature */
+ reg = <0x00>;
+ label = "temp";
+ gw,mode = <0>;
+ };
+
+ channel@2 { /* A1: Input Voltage (raw ADC) */
+ reg = <0x02>;
+ label = "vdd_vin";
+ gw,mode = <1>;
+ gw,voltage-divider-ohms = <22100 1000>;
+ gw,voltage-offset-microvolt = <800000>;
+ };
+
+ channel@b { /* A2: Battery voltage */
+ reg = <0x0b>;
+ label = "vdd_bat";
+ gw,mode = <1>;
+ };
+ };
+
+ fan-controller@2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "gw,gsc-fan";
+ reg = <0x2c>;
+ };
+ };
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-03-27 20:33 [PATCH v8 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
@ 2020-03-27 20:33 ` Tim Harvey
2020-04-08 15:36 ` Tim Harvey
2020-04-28 9:44 ` Lee Jones
2020-03-27 20:33 ` [PATCH v8 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2 siblings, 2 replies; 15+ messages in thread
From: Tim Harvey @ 2020-03-27 20:33 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>
---
v8:
- whitespace fixes
- describe sub-devices in Kconfig
- add error print for invalid command
- update copyright
- use devm_of_platform_populate
- use probe_new
- move hwmon's regmap init to hwmon
v7:
- remove irq from private data struct
v6:
- remove duplicate signature and fix commit log
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
---
MAINTAINERS | 8 ++
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 288 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/gsc.h | 73 +++++++++++
5 files changed, 380 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..020e1e1
--- /dev/null
+++ b/drivers/mfd/gateworks-gsc.c
@@ -0,0 +1,288 @@
+// 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;
+}
+
+/*
+ * 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);
+ else
+ dev_err(dev, "invalid command: '%s'\n", name);
+
+ 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 command: '%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", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, gsc_of_match);
+
+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_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)
+{
+ 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;
+ i2c_set_clientdata(client, gsc);
+
+ gsc->bus.reg_write = gsc_regmap_regwrite;
+ gsc->bus.reg_read = gsc_regmap_regread;
+ gsc->regmap = devm_regmap_init(dev, &gsc->bus, 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);
+
+ ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->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 = devm_of_platform_populate(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 const struct i2c_device_id gsc_id_table[] = {
+ {"gsc", GSC_MISC },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, gsc_id_table);
+
+static struct i2c_driver gsc_driver = {
+ .driver = {
+ .name = "gateworks-gsc",
+ .of_match_table = gsc_of_match,
+ },
+ .id_table = gsc_id_table,
+ .probe_new = 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..fc33647
--- /dev/null
+++ b/include/linux/mfd/gsc.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2020 Gateworks Corporation
+ */
+#ifndef __LINUX_MFD_GSC_H_
+#define __LINUX_MFD_GSC_H_
+
+#include <linux/regmap.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_bus bus;
+ struct regmap_irq_chip_data *irq_chip_data;
+
+ unsigned int fwver;
+ unsigned short fwcrc;
+};
+
+#endif /* __LINUX_MFD_GSC_H_ */
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 3/3] hwmon: add Gateworks System Controller support
2020-03-27 20:33 [PATCH v8 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-03-27 20:33 ` [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
@ 2020-03-27 20:33 ` Tim Harvey
2020-03-31 15:49 ` Guenter Roeck
2 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2020-03-27 20:33 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>
---
v8:
- move regmap init to hwmon
v7:
- fix whitespace in Kconfig
- remove unnecessary device pointer in private data
- change divider from mili-ohms to ohms
- move fan base property to reg
v6:
- fix size of info field
- improve pwm output control documentation
- include unit suffix in divider and offset
- change subnode name to gsc-adc
- change to fan subnode
- fix voltage offset
v5:
- fix various checkpatch issues
- correct gsc-hwmon.rst in MAINTAINERS
- encorporate Gunter's feedback:
- 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 | 385 ++++++++++++++++++++++++++++++++
include/linux/platform_data/gsc_hwmon.h | 44 ++++
7 files changed, 496 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..ffac392
--- /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_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..47b8761 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..d5cdd92
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,385 @@
+// 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 gsc_hwmon_platform_data *pdata;
+ struct regmap *regmap;
+ 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[3];
+ struct hwmon_chip_info chip;
+};
+
+static const struct regmap_config gsc_hwmon_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .cache_type = REGCACHE_NONE,
+};
+
+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->regmap, 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->regmap, 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->mode == mode_voltage) ? 3 : 2;
+ ret = regmap_bulk_read(hwmon->regmap, ch->reg, buf, sz);
+ if (ret)
+ return ret;
+
+ tmp = 0;
+ while (sz-- > 0)
+ tmp |= (buf[sz] << (8 * sz));
+
+ switch (ch->mode) {
+ case mode_temperature:
+ if (tmp > 0x8000)
+ tmp -= 0xffff;
+ break;
+ case mode_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 mode_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;
+ struct device_node *fan;
+ 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;
+
+ /* fan controller base address */
+ fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan");
+ if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) {
+ dev_err(dev, "fan node without base\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ /* 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_u32(child, "gw,mode", &ch->mode)) {
+ dev_err(dev, "channel without mode\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (ch->mode > mode_max) {
+ dev_err(dev, "invalid channel mode\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (!fwnode_property_read_u32(child,
+ "gw,voltage-offset-microvolt",
+ &ch->mvoffset))
+ ch->mvoffset /= 1000;
+ fwnode_property_read_u32_array(child,
+ "gw,voltage-divider-ohms",
+ 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 device *hwmon_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;
+
+ hwmon->regmap = devm_regmap_init(dev, &gsc->bus,
+ gsc->i2c_hwmon,
+ &gsc_hwmon_regmap_config);
+ if (IS_ERR(hwmon->regmap))
+ return PTR_ERR(hwmon->regmap);
+
+ 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->mode) {
+ case mode_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 mode_voltage:
+ case mode_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 mode: %d\n", ch->mode);
+ 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-adc", },
+ {}
+};
+
+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..ec1611a
--- /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_mode {
+ mode_temperature,
+ mode_voltage,
+ mode_voltage_raw,
+ mode_max,
+};
+
+/**
+ * struct gsc_hwmon_channel - configuration parameters
+ * @reg: I2C register offset
+ * @mode: channel mode
+ * @name: channel name
+ * @mvoffset: voltage offset
+ * @vdiv: voltage divider array (2 resistor values in milli-ohms)
+ */
+struct gsc_hwmon_channel {
+ unsigned int reg;
+ unsigned int mode;
+ 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] 15+ messages in thread
* Re: [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
@ 2020-03-30 16:03 ` Rob Herring
2020-04-17 9:58 ` Lee Jones
1 sibling, 0 replies; 15+ messages in thread
From: Rob Herring @ 2020-03-30 16:03 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Jean Delvare, Guenter Roeck, linux-hwmon,
Frank Rowand, devicetree, linux-kernel, Robert Jones
On Fri, Mar 27, 2020 at 01:33:32PM -0700, 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>
> ---
> v8:
> - add register to fan-controller node name
>
> v7:
> - change divider from mili-ohms to ohms
> - add constraints for voltage divider and offset
> - remove unnecessary ref for offset
> - renamed fan to fan-controller and changed base prop to reg
>
> v6:
> - fix typo
> - drop invalid description from #interrupt-cells property
> - fix adc pattern property
> - add unit suffix
> - replace hwmon/adc with adc/channel
> - changed adc type to mode and enum int
> - add unit suffix and drop ref for voltage-divider
> - moved fan to its own subnode with base register
>
> 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 | 194 +++++++++++++++++++++
> 1 file changed, 194 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 3/3] hwmon: add Gateworks System Controller support
2020-03-27 20:33 ` [PATCH v8 3/3] hwmon: add Gateworks System Controller support Tim Harvey
@ 2020-03-31 15:49 ` Guenter Roeck
0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2020-03-31 15:49 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Jean Delvare, linux-hwmon, Rob Herring, Frank Rowand,
devicetree, linux-kernel, Robert Jones
On Fri, Mar 27, 2020 at 01:33:34PM -0700, 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>
For my own reference:
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Guenter
> ---
> v8:
> - move regmap init to hwmon
>
> v7:
> - fix whitespace in Kconfig
> - remove unnecessary device pointer in private data
> - change divider from mili-ohms to ohms
> - move fan base property to reg
>
> v6:
> - fix size of info field
> - improve pwm output control documentation
> - include unit suffix in divider and offset
> - change subnode name to gsc-adc
> - change to fan subnode
> - fix voltage offset
>
> v5:
> - fix various checkpatch issues
> - correct gsc-hwmon.rst in MAINTAINERS
> - encorporate Gunter's feedback:
> - 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 | 385 ++++++++++++++++++++++++++++++++
> include/linux/platform_data/gsc_hwmon.h | 44 ++++
> 7 files changed, 496 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..ffac392
> --- /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_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..47b8761 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..d5cdd92
> --- /dev/null
> +++ b/drivers/hwmon/gsc-hwmon.c
> @@ -0,0 +1,385 @@
> +// 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 gsc_hwmon_platform_data *pdata;
> + struct regmap *regmap;
> + 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[3];
> + struct hwmon_chip_info chip;
> +};
> +
> +static const struct regmap_config gsc_hwmon_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +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->regmap, 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->regmap, 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->mode == mode_voltage) ? 3 : 2;
> + ret = regmap_bulk_read(hwmon->regmap, ch->reg, buf, sz);
> + if (ret)
> + return ret;
> +
> + tmp = 0;
> + while (sz-- > 0)
> + tmp |= (buf[sz] << (8 * sz));
> +
> + switch (ch->mode) {
> + case mode_temperature:
> + if (tmp > 0x8000)
> + tmp -= 0xffff;
> + break;
> + case mode_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 mode_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;
> + struct device_node *fan;
> + 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;
> +
> + /* fan controller base address */
> + fan = of_find_compatible_node(dev->parent->of_node, NULL, "gw,gsc-fan");
> + if (fan && of_property_read_u32(fan, "reg", &pdata->fan_base)) {
> + dev_err(dev, "fan node without base\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + /* 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_u32(child, "gw,mode", &ch->mode)) {
> + dev_err(dev, "channel without mode\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (ch->mode > mode_max) {
> + dev_err(dev, "invalid channel mode\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (!fwnode_property_read_u32(child,
> + "gw,voltage-offset-microvolt",
> + &ch->mvoffset))
> + ch->mvoffset /= 1000;
> + fwnode_property_read_u32_array(child,
> + "gw,voltage-divider-ohms",
> + 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 device *hwmon_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;
> +
> + hwmon->regmap = devm_regmap_init(dev, &gsc->bus,
> + gsc->i2c_hwmon,
> + &gsc_hwmon_regmap_config);
> + if (IS_ERR(hwmon->regmap))
> + return PTR_ERR(hwmon->regmap);
> +
> + 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->mode) {
> + case mode_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 mode_voltage:
> + case mode_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 mode: %d\n", ch->mode);
> + 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-adc", },
> + {}
> +};
> +
> +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..ec1611a
> --- /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_mode {
> + mode_temperature,
> + mode_voltage,
> + mode_voltage_raw,
> + mode_max,
> +};
> +
> +/**
> + * struct gsc_hwmon_channel - configuration parameters
> + * @reg: I2C register offset
> + * @mode: channel mode
> + * @name: channel name
> + * @mvoffset: voltage offset
> + * @vdiv: voltage divider array (2 resistor values in milli-ohms)
> + */
> +struct gsc_hwmon_channel {
> + unsigned int reg;
> + unsigned int mode;
> + 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] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-03-27 20:33 ` [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
@ 2020-04-08 15:36 ` Tim Harvey
2020-04-14 8:09 ` Lee Jones
2020-04-28 9:44 ` Lee Jones
1 sibling, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2020-04-08 15:36 UTC (permalink / raw)
To: Lee Jones
Cc: Linux HWMON List, Robert Jones, Rob Herring, Frank Rowand,
Device Tree Mailing List, open list, Jean Delvare, Guenter Roeck
On Fri, Mar 27, 2020 at 1:33 PM Tim Harvey <tharvey@gateworks.com> 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
>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v8:
> - whitespace fixes
> - describe sub-devices in Kconfig
> - add error print for invalid command
> - update copyright
> - use devm_of_platform_populate
> - use probe_new
> - move hwmon's regmap init to hwmon
>
> v7:
> - remove irq from private data struct
>
> v6:
> - remove duplicate signature and fix commit log
>
> 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
> ---
> MAINTAINERS | 8 ++
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/gateworks-gsc.c | 288 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/gsc.h | 73 +++++++++++
> 5 files changed, 380 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..020e1e1
> --- /dev/null
> +++ b/drivers/mfd/gateworks-gsc.c
> @@ -0,0 +1,288 @@
> +// 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;
> +}
> +
> +/*
> + * 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);
> + else
> + dev_err(dev, "invalid command: '%s'\n", name);
> +
> + 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 command: '%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", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gsc_of_match);
> +
> +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_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)
> +{
> + 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;
> + i2c_set_clientdata(client, gsc);
> +
> + gsc->bus.reg_write = gsc_regmap_regwrite;
> + gsc->bus.reg_read = gsc_regmap_regread;
> + gsc->regmap = devm_regmap_init(dev, &gsc->bus, 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);
> +
> + ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->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 = devm_of_platform_populate(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 const struct i2c_device_id gsc_id_table[] = {
> + {"gsc", GSC_MISC },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, gsc_id_table);
> +
> +static struct i2c_driver gsc_driver = {
> + .driver = {
> + .name = "gateworks-gsc",
> + .of_match_table = gsc_of_match,
> + },
> + .id_table = gsc_id_table,
> + .probe_new = 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..fc33647
> --- /dev/null
> +++ b/include/linux/mfd/gsc.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2020 Gateworks Corporation
> + */
> +#ifndef __LINUX_MFD_GSC_H_
> +#define __LINUX_MFD_GSC_H_
> +
> +#include <linux/regmap.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_bus bus;
> + struct regmap_irq_chip_data *irq_chip_data;
> +
> + unsigned int fwver;
> + unsigned short fwcrc;
> +};
> +
> +#endif /* __LINUX_MFD_GSC_H_ */
> --
> 2.7.4
>
Lee,
Do you have time to look at this? Rob has reviewed the dt-bindings and
Guenter has reviewed the hwmon driver so this is the only patch
remaining in the series that needs review/sign-off.
Best Regards,
Tim
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-04-08 15:36 ` Tim Harvey
@ 2020-04-14 8:09 ` Lee Jones
0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2020-04-14 8:09 UTC (permalink / raw)
To: Tim Harvey
Cc: Linux HWMON List, Robert Jones, Rob Herring, Frank Rowand,
Device Tree Mailing List, open list, Jean Delvare, Guenter Roeck
On Wed, 08 Apr 2020, Tim Harvey wrote:
> On Fri, Mar 27, 2020 at 1:33 PM Tim Harvey <tharvey@gateworks.com> 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
> >
> > Cc: Randy Dunlap <rdunlap@infradead.org>
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> > MAINTAINERS | 8 ++
> > drivers/mfd/Kconfig | 10 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/gateworks-gsc.c | 288 ++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/gsc.h | 73 +++++++++++
> > 5 files changed, 380 insertions(+)
> > create mode 100644 drivers/mfd/gateworks-gsc.c
> > create mode 100644 include/linux/mfd/gsc.h
[...]
Please clip your responses.
> Lee,
>
> Do you have time to look at this? Rob has reviewed the dt-bindings and
> Guenter has reviewed the hwmon driver so this is the only patch
> remaining in the series that needs review/sign-off.
It's on the list.
Please bear in mind that the merge-window has been open for the past 2
weeks. Usually reviewing cadence slows down during this period. It
should be ramping up again this week.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-03-30 16:03 ` Rob Herring
@ 2020-04-17 9:58 ` Lee Jones
2020-04-20 15:28 ` Tim Harvey
1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2020-04-17 9:58 UTC (permalink / raw)
To: Tim Harvey
Cc: Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones
On Fri, 27 Mar 2020, 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>
> ---
> v8:
> - add register to fan-controller node name
>
> v7:
> - change divider from mili-ohms to ohms
> - add constraints for voltage divider and offset
> - remove unnecessary ref for offset
> - renamed fan to fan-controller and changed base prop to reg
>
> v6:
> - fix typo
> - drop invalid description from #interrupt-cells property
> - fix adc pattern property
> - add unit suffix
> - replace hwmon/adc with adc/channel
> - changed adc type to mode and enum int
> - add unit suffix and drop ref for voltage-divider
> - moved fan to its own subnode with base register
>
> 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 | 194 +++++++++++++++++++++
> 1 file changed, 194 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..a96751c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> @@ -0,0 +1,194 @@
> +# 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
I'd prefer if you didn't use Linuxisums in DT docs.
A 'multi-function device' isn't a thing - we made it up.
Nowhere in the documentation [0] is the Gateworks System Controller
described as a multi-function device.
[0] http://trac.gateworks.com/wiki/gsc
> +description: |
> + The GSC is a Multifunction I2C slave device with the following submodules:
No it isn't. It's a:
"The Gateworks System Controller (GSC) is a device present across
various Gateworks product families that provides a set of system
related feature such as the following (refer to the board hardware
user manuals to see what features are present)"
> + - Watchdog Timer
> + - GPIO
> + - Pushbutton controller
> + - Hardware Monitor with ADC's for temperature and voltage rails and
> + fan controller
Why is "Monitor" capitalised, but "controller" is not?
I would s/Monitor/monitor/ here.
> +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
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + adc:
> + type: object
> + description: Optional Hardware Monitoring module
Again, an odd thing to capitalise.
> + properties:
> + compatible:
> + const: gw,gsc-adc
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + patternProperties:
> + "^channel@[0-9]+$":
> + 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).
/ie/i.e./
> + properties:
> + reg:
> + description: Register of the ADC
> + maxItems: 1
> +
> + label:
> + description: Name of the ADC input
> +
> + gw,mode:
> + description: |
> + conversion mode:
> + 0 - temperature, in C*10
> + 1 - pre-scaled voltage value
> + 2 - scaled voltage based on an optional resistor divider
> + and optional offset
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
Rob just submitted a patch-set to remove 'allOf's from '$ref'
properties.
> + enum: [0, 1, 2]
> +
> + gw,voltage-divider-ohms:
> + description: values of resistors for divider on raw ADC input
s/values/Values/
> + maxItems: 2
> + items:
> + minimum: 1000
> + maximum: 1000000
> +
> + gw,voltage-offset-microvolt:
> + description: |
> + A positive voltage offset to apply to a raw ADC
> + (ie to compensate for a diode drop).
s/ie/i.e/
> + minimum: 0
> + maximum: 1000000
> +
> + required:
> + - gw,mode
> + - reg
> + - label
> +
> + required:
> + - compatible
> + - "#address-cells"
> + - "#size-cells"
> +
> +patternProperties:
> + "^fan-controller@[0-9a-f]+$":
> + type: object
> + description: Optional FAN controller
"Fan"
> + properties:
> + compatible:
> + const: gw,gsc-fan
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + reg:
> + description: The fan controller base address
> + maxItems: 1
> +
> + required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> + - "#address-cells"
> + - "#size-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>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + adc {
> + compatible = "gw,gsc-adc";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 { /* A0: Board Temperature */
> + reg = <0x00>;
> + label = "temp";
> + gw,mode = <0>;
> + };
> +
> + channel@2 { /* A1: Input Voltage (raw ADC) */
> + reg = <0x02>;
> + label = "vdd_vin";
> + gw,mode = <1>;
> + gw,voltage-divider-ohms = <22100 1000>;
> + gw,voltage-offset-microvolt = <800000>;
> + };
> +
> + channel@b { /* A2: Battery voltage */
> + reg = <0x0b>;
> + label = "vdd_bat";
> + gw,mode = <1>;
> + };
> + };
> +
> + fan-controller@2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "gw,gsc-fan";
> + reg = <0x2c>;
> + };
> + };
> + };
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
2020-04-17 9:58 ` Lee Jones
@ 2020-04-20 15:28 ` Tim Harvey
0 siblings, 0 replies; 15+ messages in thread
From: Tim Harvey @ 2020-04-20 15:28 UTC (permalink / raw)
To: Lee Jones
Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, Rob Herring,
Frank Rowand, Device Tree Mailing List, open list, Robert Jones
On Fri, Apr 17, 2020 at 2:57 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 27 Mar 2020, 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>
<snip>
> > ---
> > .../devicetree/bindings/mfd/gateworks-gsc.yaml | 194 +++++++++++++++++++++
> > 1 file changed, 194 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..a96751c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > @@ -0,0 +1,194 @@
> > +# 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
>
> I'd prefer if you didn't use Linuxisums in DT docs.
>
> A 'multi-function device' isn't a thing - we made it up.
>
> Nowhere in the documentation [0] is the Gateworks System Controller
> described as a multi-function device.
>
> [0] http://trac.gateworks.com/wiki/gsc
>
> > +description: |
> > + The GSC is a Multifunction I2C slave device with the following submodules:
>
> No it isn't. It's a:
>
> "The Gateworks System Controller (GSC) is a device present across
> various Gateworks product families that provides a set of system
> related feature such as the following (refer to the board hardware
> user manuals to see what features are present)"
>
> > + - Watchdog Timer
> > + - GPIO
> > + - Pushbutton controller
> > + - Hardware Monitor with ADC's for temperature and voltage rails and
> > + fan controller
>
> Why is "Monitor" capitalised, but "controller" is not?
>
> I would s/Monitor/monitor/ here.
>
> > +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
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + adc:
> > + type: object
> > + description: Optional Hardware Monitoring module
>
> Again, an odd thing to capitalise.
>
> > + properties:
> > + compatible:
> > + const: gw,gsc-adc
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + patternProperties:
> > + "^channel@[0-9]+$":
> > + 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).
>
> /ie/i.e./
>
> > + properties:
> > + reg:
> > + description: Register of the ADC
> > + maxItems: 1
> > +
> > + label:
> > + description: Name of the ADC input
> > +
> > + gw,mode:
> > + description: |
> > + conversion mode:
> > + 0 - temperature, in C*10
> > + 1 - pre-scaled voltage value
> > + 2 - scaled voltage based on an optional resistor divider
> > + and optional offset
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32
>
> Rob just submitted a patch-set to remove 'allOf's from '$ref'
> properties.
>
> > + enum: [0, 1, 2]
> > +
> > + gw,voltage-divider-ohms:
> > + description: values of resistors for divider on raw ADC input
>
> s/values/Values/
>
> > + maxItems: 2
> > + items:
> > + minimum: 1000
> > + maximum: 1000000
> > +
> > + gw,voltage-offset-microvolt:
> > + description: |
> > + A positive voltage offset to apply to a raw ADC
> > + (ie to compensate for a diode drop).
>
> s/ie/i.e/
>
> > + minimum: 0
> > + maximum: 1000000
> > +
> > + required:
> > + - gw,mode
> > + - reg
> > + - label
> > +
> > + required:
> > + - compatible
> > + - "#address-cells"
> > + - "#size-cells"
> > +
> > +patternProperties:
> > + "^fan-controller@[0-9a-f]+$":
> > + type: object
> > + description: Optional FAN controller
>
> "Fan"
>
> > + properties:
> > + compatible:
> > + const: gw,gsc-fan
> > +
> > + "#address-cells":
> > + const: 1
> > +
> > + "#size-cells":
> > + const: 0
> > +
> > + reg:
> > + description: The fan controller base address
> > + maxItems: 1
> > +
> > + required:
> > + - compatible
> > + - reg
> > + - "#address-cells"
> > + - "#size-cells"
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - interrupt-controller
> > + - "#interrupt-cells"
> > + - "#address-cells"
> > + - "#size-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>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + adc {
> > + compatible = "gw,gsc-adc";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + channel@0 { /* A0: Board Temperature */
> > + reg = <0x00>;
> > + label = "temp";
> > + gw,mode = <0>;
> > + };
> > +
> > + channel@2 { /* A1: Input Voltage (raw ADC) */
> > + reg = <0x02>;
> > + label = "vdd_vin";
> > + gw,mode = <1>;
> > + gw,voltage-divider-ohms = <22100 1000>;
> > + gw,voltage-offset-microvolt = <800000>;
> > + };
> > +
> > + channel@b { /* A2: Battery voltage */
> > + reg = <0x0b>;
> > + label = "vdd_bat";
> > + gw,mode = <1>;
> > + };
> > + };
> > +
> > + fan-controller@2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "gw,gsc-fan";
> > + reg = <0x2c>;
> > + };
> > + };
> > + };
>
Lee,
Thanks for the review. I will send a v9 once you have had time to
review the mfd driver patch in this series.
Best Regards,
Tim
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-03-27 20:33 ` [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-04-08 15:36 ` Tim Harvey
@ 2020-04-28 9:44 ` Lee Jones
2020-04-28 19:21 ` Tim Harvey
1 sibling, 1 reply; 15+ messages in thread
From: Lee Jones @ 2020-04-28 9:44 UTC (permalink / raw)
To: Tim Harvey
Cc: Jean Delvare, Guenter Roeck, linux-hwmon, Rob Herring,
Frank Rowand, devicetree, linux-kernel, Robert Jones,
Randy Dunlap
On Fri, 27 Mar 2020, 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
>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v8:
> - whitespace fixes
> - describe sub-devices in Kconfig
> - add error print for invalid command
> - update copyright
> - use devm_of_platform_populate
> - use probe_new
> - move hwmon's regmap init to hwmon
>
> v7:
> - remove irq from private data struct
>
> v6:
> - remove duplicate signature and fix commit log
>
> 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
> ---
> MAINTAINERS | 8 ++
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/gateworks-gsc.c | 288 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/gsc.h | 73 +++++++++++
> 5 files changed, 380 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.
Please expand on the help section some more. What is the purpose of
the device? What sub-devices make up the GSC? What does this device
driver offer/provide?
> 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..020e1e1
> --- /dev/null
> +++ b/drivers/mfd/gateworks-gsc.c
> @@ -0,0 +1,288 @@
> +// 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.
Looks like good information for the help.
> + * Copyright (C) 2020 Gateworks Corporation
> + */
Nit: '\n' here.
> +#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;
> +}
> +
> +/*
> + * 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;
Would put_unaligned_{b,l}e32() make sense here?
> + ret = regmap_bulk_write(gsc->regmap, GSC_TIME_ADD, regs, 4);
> + if (ret)
> + return ret;
> +
> + regs[0] = 1 << GSC_CTRL_1_LATCH_SLEEP_ADD;
BIT()
Why does this value need to be loaded into a variable?
> + 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);
BIT()
> + 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);
> + else
> + dev_err(dev, "invalid command: '%s'\n", name);
> +
> + 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 command: '%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", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, gsc_of_match);
> +
> +static const struct regmap_config gsc_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_NONE,
> + .max_register = 0xf,
Nicer if this was defined as part of an enum of registers I think.
> +};
> +
> +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)
> +{
> + 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;
> + i2c_set_clientdata(client, gsc);
> +
> + gsc->bus.reg_write = gsc_regmap_regwrite;
> + gsc->bus.reg_read = gsc_regmap_regread;
Why do you need to store these in ddata?
> + gsc->regmap = devm_regmap_init(dev, &gsc->bus, 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);
You already have this set.
Just 'get' it from client->dev.parent (or whatever it is).
> + ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->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 = devm_of_platform_populate(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);
If you use devm_* you can omit this line.
> + 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);
If you use devm_* you can omit this line.
> + return 0;
> +}
> +
> +static const struct i2c_device_id gsc_id_table[] = {
> + {"gsc", GSC_MISC },
Space after the '{'.
> + { /* sentinel */ }
No need for this comment.
> +};
> +MODULE_DEVICE_TABLE(i2c, gsc_id_table);
> +
> +static struct i2c_driver gsc_driver = {
> + .driver = {
> + .name = "gateworks-gsc",
> + .of_match_table = gsc_of_match,
> + },
> + .id_table = gsc_id_table,
> + .probe_new = gsc_probe,
> + .remove = gsc_remove,
> +};
> +
Nit: Superfluous '\n'.
> +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..fc33647
> --- /dev/null
> +++ b/include/linux/mfd/gsc.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2020 Gateworks Corporation
> + */
> +#ifndef __LINUX_MFD_GSC_H_
> +#define __LINUX_MFD_GSC_H_
> +
> +#include <linux/regmap.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_bus bus;
> + struct regmap_irq_chip_data *irq_chip_data;
What is this used by?
> +
> + unsigned int fwver;
> + unsigned short fwcrc;
> +};
Please ensure all of these properties are used outside of .probe().
If they're not, please remove them.
> +#endif /* __LINUX_MFD_GSC_H_ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-04-28 9:44 ` Lee Jones
@ 2020-04-28 19:21 ` Tim Harvey
2020-04-29 6:33 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2020-04-28 19:21 UTC (permalink / raw)
To: Lee Jones
Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, Rob Herring,
Frank Rowand, Device Tree Mailing List, open list, Robert Jones,
Randy Dunlap
On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
>
<snip>
> > +
> > +static int gsc_probe(struct i2c_client *client)
> > +{
> > + 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;
> > + i2c_set_clientdata(client, gsc);
> > +
> > + gsc->bus.reg_write = gsc_regmap_regwrite;
> > + gsc->bus.reg_read = gsc_regmap_regread;
>
> Why do you need to store these in ddata?
Lee,
Thanks for the review!
I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
&gsc_hwmon_regmap_config);
Is there something easier I'm missing?
Thanks,
Tim
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-04-28 19:21 ` Tim Harvey
@ 2020-04-29 6:33 ` Lee Jones
2020-04-29 14:50 ` Tim Harvey
0 siblings, 1 reply; 15+ messages in thread
From: Lee Jones @ 2020-04-29 6:33 UTC (permalink / raw)
To: Tim Harvey
Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, Rob Herring,
Frank Rowand, Device Tree Mailing List, open list, Robert Jones,
Randy Dunlap
On Tue, 28 Apr 2020, Tim Harvey wrote:
> On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> <snip>
> > > +
> > > +static int gsc_probe(struct i2c_client *client)
> > > +{
> > > + 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;
> > > + i2c_set_clientdata(client, gsc);
> > > +
> > > + gsc->bus.reg_write = gsc_regmap_regwrite;
> > > + gsc->bus.reg_read = gsc_regmap_regread;
> >
> > Why do you need to store these in ddata?
>
> Lee,
>
> Thanks for the review!
>
> I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
>
> hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> &gsc_hwmon_regmap_config);
>
> Is there something easier I'm missing?
This is an odd setup. I haven't seen one driver registering another
driver's Regmap call-backs before, related or otherwise. Normally the
Regmap is setup (initialised) in the parent driver and child drivers
just make use of it. Here it looks like you are registering 2
separate Regmaps, but using the same call-backs for both, which seems
wrong to me.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-04-29 6:33 ` Lee Jones
@ 2020-04-29 14:50 ` Tim Harvey
2020-04-30 8:28 ` Lee Jones
0 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2020-04-29 14:50 UTC (permalink / raw)
To: Lee Jones
Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, Rob Herring,
Frank Rowand, Device Tree Mailing List, open list, Robert Jones,
Randy Dunlap
On Tue, Apr 28, 2020 at 11:33 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 28 Apr 2020, Tim Harvey wrote:
>
> > On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > <snip>
> > > > +
> > > > +static int gsc_probe(struct i2c_client *client)
> > > > +{
> > > > + 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;
> > > > + i2c_set_clientdata(client, gsc);
> > > > +
> > > > + gsc->bus.reg_write = gsc_regmap_regwrite;
> > > > + gsc->bus.reg_read = gsc_regmap_regread;
> > >
> > > Why do you need to store these in ddata?
> >
> > Lee,
> >
> > Thanks for the review!
> >
> > I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
> >
> > hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> > &gsc_hwmon_regmap_config);
> >
> > Is there something easier I'm missing?
>
> This is an odd setup. I haven't seen one driver registering another
> driver's Regmap call-backs before, related or otherwise. Normally the
> Regmap is setup (initialised) in the parent driver and child drivers
> just make use of it. Here it looks like you are registering 2
> separate Regmaps, but using the same call-backs for both, which seems
> wrong to me.
>
Lee,
It is perhaps an odd setup. The hwmon sub-device is at a different i2c
slave address than the other sub-devices. The same callbacks are used
for reg read/write to take advantage of the retries due to the errata
resulting in occasional NAK'd register reads.
Tim
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver
2020-04-29 14:50 ` Tim Harvey
@ 2020-04-30 8:28 ` Lee Jones
0 siblings, 0 replies; 15+ messages in thread
From: Lee Jones @ 2020-04-30 8:28 UTC (permalink / raw)
To: Tim Harvey
Cc: Jean Delvare, Guenter Roeck, Linux HWMON List, Rob Herring,
Frank Rowand, Device Tree Mailing List, open list, Robert Jones,
Randy Dunlap
On Wed, 29 Apr 2020, Tim Harvey wrote:
> On Tue, Apr 28, 2020 at 11:33 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 28 Apr 2020, Tim Harvey wrote:
> >
> > > On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > <snip>
> > > > > +
> > > > > +static int gsc_probe(struct i2c_client *client)
> > > > > +{
> > > > > + 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;
> > > > > + i2c_set_clientdata(client, gsc);
> > > > > +
> > > > > + gsc->bus.reg_write = gsc_regmap_regwrite;
> > > > > + gsc->bus.reg_read = gsc_regmap_regread;
> > > >
> > > > Why do you need to store these in ddata?
> > >
> > > Lee,
> > >
> > > Thanks for the review!
> > >
> > > I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
> > >
> > > hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> > > &gsc_hwmon_regmap_config);
> > >
> > > Is there something easier I'm missing?
> >
> > This is an odd setup. I haven't seen one driver registering another
> > driver's Regmap call-backs before, related or otherwise. Normally the
> > Regmap is setup (initialised) in the parent driver and child drivers
> > just make use of it. Here it looks like you are registering 2
> > separate Regmaps, but using the same call-backs for both, which seems
> > wrong to me.
> >
>
> Lee,
>
> It is perhaps an odd setup. The hwmon sub-device is at a different i2c
> slave address than the other sub-devices. The same callbacks are used
> for reg read/write to take advantage of the retries due to the errata
> resulting in occasional NAK'd register reads.
Then I suggest putting them somewhere shared or exporting them.
Passing pointers to the via ddata sounds a bit batty.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-04-30 8:28 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 20:33 [PATCH v8 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-03-27 20:33 ` [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-03-30 16:03 ` Rob Herring
2020-04-17 9:58 ` Lee Jones
2020-04-20 15:28 ` Tim Harvey
2020-03-27 20:33 ` [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-04-08 15:36 ` Tim Harvey
2020-04-14 8:09 ` Lee Jones
2020-04-28 9:44 ` Lee Jones
2020-04-28 19:21 ` Tim Harvey
2020-04-29 6:33 ` Lee Jones
2020-04-29 14:50 ` Tim Harvey
2020-04-30 8:28 ` Lee Jones
2020-03-27 20:33 ` [PATCH v8 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2020-03-31 15:49 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).