* [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* 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 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
* [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* 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 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
* [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 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