* [PATCH 0/2] Add support for LTC4282 @ 2023-11-10 15:18 Nuno Sa 2023-11-10 15:18 ` [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings Nuno Sa 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa 0 siblings, 2 replies; 17+ messages in thread From: Nuno Sa @ 2023-11-10 15:18 UTC (permalink / raw) To: linux-hwmon, linux-doc, devicetree Cc: Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Nuno Sa, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley Hi all, The LTC4282 hot swap controller allows a board to be safely inserted and removed from a live backplane. Using one or more external N-channel pass transistors, board supply voltage and inrush current are ramped up at an adjustable rate. An I2C interface and onboard ADC allows for monitoring of board current, voltage, power, energy and fault status. I'm aware that there are ABI in the driver that will surely raise questions. So, I'll try to add some comments about those: - For the fault_log stuff please see the comment in the code. There might be some scenarios where one might really want to latch off the device until a fault is manually cleared. - I also see value in the FET interfaces as they are real faults but maybe the naming is poor. - I'm not so sure about the power1_good and the power1_fault_log. The power1_good is more of a real status bit. If the bit is 0, it does not necessarily means that there's something wrong. If someone removed (on purpose) the "load", then this will be 0 and there's nothing wrong. The fault_log is also not one of those bits that will keep the device to latch on again. However, they might really indicate some misbehave. But, OTOH (again :)), maybe the GPIO support for this is enough... - There's also the handling for the overflow bits. I don't think it makes much sense to export those so I tried to be clever and automatically handle it the driver. The power_average is the thing making the whole thing more complicated. If it was only the energy, we could defer it completely to userspace... - And there's also the rsense as a mandatory property. Designs like this completely depend on the calculated rsense so I have no idea (and if it makes sense) what default should I use if the property is not given. I'm also cc'ing the GPIO folks for the GPIO bits. And I'm also not so sure about it. I'm just treating the pins as if I can set value + direction. However, the only thing that we can do is to PULL_LOW and set the pins in HIGH_Z. So, I dunno I'm doing the right thing. I wonder if I should just give the ability to configure the pins through FW with the .set_config hook and then just allow to read the pin level? The GPIO1 is also odd since is only the one that directly allows you to control the direction but then, again, you can just pull it low or high-z. One last comment is about lines length. I know some maintainer still want the 80 col limit but since I'm not so sure on the policy in hwmon I just went for 100. I'm pretty sure I'll need more iterations to get the driver in, so I'm happy to change it to 80 if required. Nuno Sa (2): dt-bindings: hwmon: Add LTC4282 bindings hwmon: ltc4282: add support for the LTC4282 chip .../bindings/hwmon/adi,ltc4282.yaml | 228 +++ Documentation/hwmon/ltc4282.rst | 101 ++ MAINTAINERS | 8 + drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/ltc4282.c | 1518 +++++++++++++++++ 6 files changed, 1867 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml create mode 100644 Documentation/hwmon/ltc4282.rst create mode 100644 drivers/hwmon/ltc4282.c -- 2.42.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings 2023-11-10 15:18 [PATCH 0/2] Add support for LTC4282 Nuno Sa @ 2023-11-10 15:18 ` Nuno Sa 2023-11-10 18:42 ` Conor Dooley 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa 1 sibling, 1 reply; 17+ messages in thread From: Nuno Sa @ 2023-11-10 15:18 UTC (permalink / raw) To: linux-hwmon, linux-doc, devicetree Cc: Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Nuno Sa, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley Add bindings for the LTC4282 High Current Hot Swap Controller with I2C Compatible Monitoring. Signed-off-by: Nuno Sa <nuno.sa@analog.com> --- .../bindings/hwmon/adi,ltc4282.yaml | 228 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 234 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml new file mode 100644 index 000000000000..0a5d540f014e --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml @@ -0,0 +1,228 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/adi,ltc4282.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C + +maintainers: + - Nuno Sa <nuno.sa@analog.com> + +description: | + Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C. + + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf + + +properties: + compatible: + enum: + - adi,ltc4282 + + reg: + maxItems: 1 + + vdd-supply: true + + clocks: + maxItems: 1 + + adi,clkout-mode: + description: | + Controls in which mode the CLKOUT PIN should work: + 0 - Configures the CLKOUT pin to output the internal system clock + 1 - Configures the CLKOUT pin to output the internal conversion time + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + adi,rsense-nano-ohms: + description: Value of the sense resistor. + + adi,vin-mode-microvolt: + description: + Selects operating range for the Undervoltage, Overvoltage and Foldback + pins. Also for the ADC. Should be set to the nominal input voltage. + enum: [3300000, 5000000, 12000000, 24000000] + default: 12000000 + + adi,fet-bad-timeout-ms: + description: + From the moment a FET bad conditions is present, this property selects the + wait time/timeout for a FET-bad fault to be signaled. Setting this to 0, + disables FET bad faults to be reported. + default: 255 + maximum: 255 + + adi,overvoltage-dividers: + description: | + Select which dividers to use for VDD Overvoltage detection. Note that + when the internal dividers are used the threshold is referenced to VDD. + The percentages in the datasheet are misleading since the actual values + to look for are in the "Absolute Maximum Ratings" table in the + "Comparator Inputs" section. In there there's a line for each of the 5%, + 10% and 15% settings with the actual min, typical and max tolerances. + 0 - Use the external dividers. + 1 - Internal dividers 5% + 2 - Internal dividers 10% + 3 - Internal dividers 15% + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + default: 0 + + adi,undervoltage-dividers: + description: | + Select which dividers to use for VDD Overvoltage detection. Note that + when the internal dividers are used the threshold is referenced to VDD. + The percentages in the datasheet are misleading since the actual values + to look for are in the "Absolute Maximum Ratings" table in the + "Comparator Inputs" section. In there there's a line for each of the 5%, + 10% and 15% settings with the actual min, typical and max tolerances. + 0 - Use the external dividers. + 1 - Internal dividers 5% + 2 - Internal dividers 10% + 3 - Internal dividers 15% + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + default: 0 + + adi,current-limit-microvolt: + description: + The current limit sense voltage of the chip is adjustable between + 12.5mV and 34.4mV in 3.1mV steps. This effectively limits the current + on the load. + enum: [12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375] + default: 25000 + + adi,overcurrent-retry: + description: + If set, enables the chip to auto-retry 256 timer cycles after an + Overcurrent fault. + type: boolean + + adi,overvoltage-retry-disable: + description: + If set, disables the chip to auto-retry 50ms after an Overvoltage fault. + It's enabled by default. + type: boolean + + adi,undervoltage-retry-disable: + description: + If set, disables the chip to auto-retry 50ms after an Undervoltage fault. + It's enabled by default. + type: boolean + + adi,fault-log-enable: + description: + If set, enables the FAULT_LOG and ADC_ALERT_LOG registers to be written + to the EEPROM when a fault bit transitions high and hence, will be + available after a power cycle (the chip loads the contents of + the EE_FAULT_LOG register - the one in EEPROM - into FAULT_LOG at boot). + type: boolean + + adi,gpio-alert: + description: Use the ALERT pin as a GPIO. + type: boolean + + adi,gpio1-mode: + description: | + Defines the function of the Pin. + 0 - GPIO Mode. + 1 - Power Bad. Goes into high-z to indicate that the output power is + no longer good. + 2 - Power Good. Pulls low to indicate that the output power is no + longer good. + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 2 + + adi,gpio2-mode: + description: | + Defines the function of the Pin. + 0 - GPIO Mode. + 1 - Acts as an input pin and it is feeded into the ADC. + 2 - Pulls Low when the MOSFET is dissipating power (stress). + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 2 + + adi,gpio3-mode: + description: | + Defines the function of the Pin. + 0 - GPIO Mode. + 1 - Acts as an input pin and it is feeded into the ADC. + $ref: /schemas/types.yaml#/definitions/uint32 + maximum: 1 + + gpio-controller: + description: + This property applies if some of the pins are used as GPIOs. + + '#gpio-cells': + const: 2 + +required: + - compatible + - reg + - adi,rsense-nano-ohms + +dependencies: + adi,alert-as-gpio: [gpio-controller, '#gpio-cells'] + +allOf: + - if: + required: + - adi,gpio1-mode + then: + allOf: + - if: + properties: + adi,gpio0-mode: + const: 0 + then: + dependencies: + adi,gpio0-mode: [gpio-controller, '#gpio-cells'] + - if: + required: + - adi,gpio2-mode + then: + allOf: + - if: + properties: + adi,gpio1-mode: + const: 0 + then: + dependencies: + adi,gpio1-mode: [gpio-controller, '#gpio-cells'] + - if: + required: + - adi,gpio3-mode + then: + allOf: + - if: + properties: + adi,gpio2-mode: + const: 0 + then: + dependencies: + adi,gpio2-mode: [gpio-controller, '#gpio-cells'] + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + hwmon@50 { + compatible = "adi,ltc4282"; + reg = <0x50>; + adi,rsense-nano-ohms = <500>; + + gpio-controller; + #gpio-cells = <2>; + + adi,gpio1-mode = <2>; + adi,gpio2-mode = <0>; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index 43121073390c..9f9527f6057b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12481,6 +12481,12 @@ S: Maintained F: Documentation/hwmon/ltc4261.rst F: drivers/hwmon/ltc4261.c +LTC4282 HARDWARE MONITOR DRIVER +M: Nuno Sa <nuno.sa@analog.com> +L: linux-hwmon@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml + LTC4306 I2C MULTIPLEXER DRIVER M: Michael Hennerich <michael.hennerich@analog.com> L: linux-i2c@vger.kernel.org -- 2.42.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings 2023-11-10 15:18 ` [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings Nuno Sa @ 2023-11-10 18:42 ` Conor Dooley 2023-11-13 9:32 ` Nuno Sá 0 siblings, 1 reply; 17+ messages in thread From: Conor Dooley @ 2023-11-10 18:42 UTC (permalink / raw) To: Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley [-- Attachment #1: Type: text/plain, Size: 9655 bytes --] Yo, On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote: > Add bindings for the LTC4282 High Current Hot Swap Controller with I2C > Compatible Monitoring. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > .../bindings/hwmon/adi,ltc4282.yaml | 228 ++++++++++++++++++ > MAINTAINERS | 6 + > 2 files changed, 234 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > new file mode 100644 > index 000000000000..0a5d540f014e > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > @@ -0,0 +1,228 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/adi,ltc4282.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C > + > +maintainers: > + - Nuno Sa <nuno.sa@analog.com> > + > +description: | > + Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C. > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf > + > + Extra blank line here FYI. > +properties: > + compatible: > + enum: > + - adi,ltc4282 > + > + reg: > + maxItems: 1 > + > + vdd-supply: true > + > + clocks: > + maxItems: 1 > + > + adi,clkout-mode: > + description: | > + Controls in which mode the CLKOUT PIN should work: > + 0 - Configures the CLKOUT pin to output the internal system clock > + 1 - Configures the CLKOUT pin to output the internal conversion time > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] I really am not a fan of these types of properties. Part of me says that if you're outputting clocks from this device, then you should be a clock controller. How do consumers of this @clkout@ pin get the rate of the clock? I'd kinda be expecting to see a clocks property with a maxItems of 1 and clock-names with two, mutually exclusive, options. The other part says, and it applies in multiple places here, that having integer properties with non-integer meanings is a poor ABI. I'd be vastly happier if the various instances in this file became enums of strings, or $re┤evant-unit so that a dts containing these properties is immediately understandable. Cheers, COnor. > + > + adi,rsense-nano-ohms: > + description: Value of the sense resistor. > + > + adi,vin-mode-microvolt: > + description: > + Selects operating range for the Undervoltage, Overvoltage and Foldback > + pins. Also for the ADC. Should be set to the nominal input voltage. > + enum: [3300000, 5000000, 12000000, 24000000] > + default: 12000000 > + > + adi,fet-bad-timeout-ms: > + description: > + From the moment a FET bad conditions is present, this property selects the > + wait time/timeout for a FET-bad fault to be signaled. Setting this to 0, > + disables FET bad faults to be reported. > + default: 255 > + maximum: 255 > + > + adi,overvoltage-dividers: > + description: | > + Select which dividers to use for VDD Overvoltage detection. Note that > + when the internal dividers are used the threshold is referenced to VDD. > + The percentages in the datasheet are misleading since the actual values > + to look for are in the "Absolute Maximum Ratings" table in the > + "Comparator Inputs" section. In there there's a line for each of the 5%, > + 10% and 15% settings with the actual min, typical and max tolerances. > + 0 - Use the external dividers. > + 1 - Internal dividers 5% > + 2 - Internal dividers 10% > + 3 - Internal dividers 15% > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3] > + default: 0 > + > + adi,undervoltage-dividers: > + description: | > + Select which dividers to use for VDD Overvoltage detection. Note that > + when the internal dividers are used the threshold is referenced to VDD. > + The percentages in the datasheet are misleading since the actual values > + to look for are in the "Absolute Maximum Ratings" table in the > + "Comparator Inputs" section. In there there's a line for each of the 5%, > + 10% and 15% settings with the actual min, typical and max tolerances. > + 0 - Use the external dividers. > + 1 - Internal dividers 5% > + 2 - Internal dividers 10% > + 3 - Internal dividers 15% > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3] > + default: 0 > + > + adi,current-limit-microvolt: > + description: > + The current limit sense voltage of the chip is adjustable between > + 12.5mV and 34.4mV in 3.1mV steps. This effectively limits the current > + on the load. > + enum: [12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375] > + default: 25000 > + > + adi,overcurrent-retry: > + description: > + If set, enables the chip to auto-retry 256 timer cycles after an > + Overcurrent fault. > + type: boolean > + > + adi,overvoltage-retry-disable: > + description: > + If set, disables the chip to auto-retry 50ms after an Overvoltage fault. > + It's enabled by default. > + type: boolean > + > + adi,undervoltage-retry-disable: > + description: > + If set, disables the chip to auto-retry 50ms after an Undervoltage fault. > + It's enabled by default. > + type: boolean > + > + adi,fault-log-enable: > + description: > + If set, enables the FAULT_LOG and ADC_ALERT_LOG registers to be written > + to the EEPROM when a fault bit transitions high and hence, will be > + available after a power cycle (the chip loads the contents of > + the EE_FAULT_LOG register - the one in EEPROM - into FAULT_LOG at boot). > + type: boolean > + > + adi,gpio-alert: > + description: Use the ALERT pin as a GPIO. > + type: boolean > + > + adi,gpio1-mode: > + description: | > + Defines the function of the Pin. > + 0 - GPIO Mode. > + 1 - Power Bad. Goes into high-z to indicate that the output power is > + no longer good. > + 2 - Power Good. Pulls low to indicate that the output power is no > + longer good. > + $ref: /schemas/types.yaml#/definitions/uint32 > + maximum: 2 > + > + adi,gpio2-mode: > + description: | > + Defines the function of the Pin. > + 0 - GPIO Mode. > + 1 - Acts as an input pin and it is feeded into the ADC. > + 2 - Pulls Low when the MOSFET is dissipating power (stress). > + $ref: /schemas/types.yaml#/definitions/uint32 > + maximum: 2 > + > + adi,gpio3-mode: > + description: | > + Defines the function of the Pin. > + 0 - GPIO Mode. > + 1 - Acts as an input pin and it is feeded into the ADC. > + $ref: /schemas/types.yaml#/definitions/uint32 > + maximum: 1 > + > + gpio-controller: > + description: > + This property applies if some of the pins are used as GPIOs. > + > + '#gpio-cells': > + const: 2 > + > +required: > + - compatible > + - reg > + - adi,rsense-nano-ohms > + > +dependencies: > + adi,alert-as-gpio: [gpio-controller, '#gpio-cells'] > + > +allOf: > + - if: > + required: > + - adi,gpio1-mode > + then: > + allOf: > + - if: > + properties: > + adi,gpio0-mode: > + const: 0 > + then: > + dependencies: > + adi,gpio0-mode: [gpio-controller, '#gpio-cells'] > + - if: > + required: > + - adi,gpio2-mode > + then: > + allOf: > + - if: > + properties: > + adi,gpio1-mode: > + const: 0 > + then: > + dependencies: > + adi,gpio1-mode: [gpio-controller, '#gpio-cells'] > + - if: > + required: > + - adi,gpio3-mode > + then: > + allOf: > + - if: > + properties: > + adi,gpio2-mode: > + const: 0 > + then: > + dependencies: > + adi,gpio2-mode: [gpio-controller, '#gpio-cells'] > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + hwmon@50 { > + compatible = "adi,ltc4282"; > + reg = <0x50>; > + adi,rsense-nano-ohms = <500>; > + > + gpio-controller; > + #gpio-cells = <2>; > + > + adi,gpio1-mode = <2>; > + adi,gpio2-mode = <0>; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index 43121073390c..9f9527f6057b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12481,6 +12481,12 @@ S: Maintained > F: Documentation/hwmon/ltc4261.rst > F: drivers/hwmon/ltc4261.c > > +LTC4282 HARDWARE MONITOR DRIVER > +M: Nuno Sa <nuno.sa@analog.com> > +L: linux-hwmon@vger.kernel.org > +S: Supported > +F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > + > LTC4306 I2C MULTIPLEXER DRIVER > M: Michael Hennerich <michael.hennerich@analog.com> > L: linux-i2c@vger.kernel.org > -- > 2.42.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings 2023-11-10 18:42 ` Conor Dooley @ 2023-11-13 9:32 ` Nuno Sá 2023-11-13 20:12 ` Conor Dooley 2023-11-20 15:03 ` Guenter Roeck 0 siblings, 2 replies; 17+ messages in thread From: Nuno Sá @ 2023-11-13 9:32 UTC (permalink / raw) To: Conor Dooley, Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley On Fri, 2023-11-10 at 18:42 +0000, Conor Dooley wrote: > Yo, > > On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote: > > Add bindings for the LTC4282 High Current Hot Swap Controller with I2C > > Compatible Monitoring. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > .../bindings/hwmon/adi,ltc4282.yaml | 228 ++++++++++++++++++ > > MAINTAINERS | 6 + > > 2 files changed, 234 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > new file mode 100644 > > index 000000000000..0a5d540f014e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > @@ -0,0 +1,228 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/hwmon/adi,ltc4282.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C > > + > > +maintainers: > > + - Nuno Sa <nuno.sa@analog.com> > > + > > +description: | > > + Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C. > > + > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf > > + > > + > > Extra blank line here FYI. > Right... > > +properties: > > + compatible: > > + enum: > > + - adi,ltc4282 > > + > > + reg: > > + maxItems: 1 > > + > > + vdd-supply: true > > + > > + clocks: > > + maxItems: 1 > > + > > + adi,clkout-mode: > > + description: | > > + Controls in which mode the CLKOUT PIN should work: > > + 0 - Configures the CLKOUT pin to output the internal system clock > > + 1 - Configures the CLKOUT pin to output the internal conversion > > time > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1] > > I really am not a fan of these types of properties. Part of me says that > if you're outputting clocks from this device, then you should be a clock > controller. How do consumers of this @clkout@ pin get the rate of the > clock? I explained it to Guenter as he also argued about this. I'll wait for more feedback but it's likely this will just turn into a clock provider, yes. > I'd kinda be expecting to see a clocks property with a maxItems of 1 and > clock-names with two, mutually exclusive, options. > > The other part says, and it applies in multiple places here, that having > integer properties with non-integer meanings is a poor ABI. I'd be vastly > happier if the various instances in this file became enums of strings, > or $re┤evant-unit so that a dts containing these properties is > immediately understandable. Well, I think you're mentioning the 'gpio-mode' 'and under/over-voltage- dividers'. I think for both it's clear that having the relevant units is not feasible (at least I'm not seeing a way of properly do it). As for the strings, well, I don't have any much to argue other than: 1) It's pattern seen in a lot of bindings - yes, that's not an excuse to copy bad/wrong things over new bindings - but, honestly, it's the first time I have someone complaining about it so I never thought it was wrong. 2) It makes much more easier to handle the properties in the driver (yeah, I know that, as far as you're concerned, this does not matter to you :)) So yeah, if you insist on it, no strong reasons on my side to not do it. As long as I see some consistency down the road :)). - Nuno Sá ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings 2023-11-13 9:32 ` Nuno Sá @ 2023-11-13 20:12 ` Conor Dooley 2023-11-20 15:03 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Conor Dooley @ 2023-11-13 20:12 UTC (permalink / raw) To: Nuno Sá Cc: Nuno Sa, linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley [-- Attachment #1: Type: text/plain, Size: 3397 bytes --] On Mon, Nov 13, 2023 at 10:32:17AM +0100, Nuno Sá wrote: > On Fri, 2023-11-10 at 18:42 +0000, Conor Dooley wrote: > > On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote: > > > + adi,clkout-mode: > > > + description: | > > > + Controls in which mode the CLKOUT PIN should work: > > > + 0 - Configures the CLKOUT pin to output the internal system clock > > > + 1 - Configures the CLKOUT pin to output the internal conversion > > > time > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1] > > > > I really am not a fan of these types of properties. Part of me says that > > if you're outputting clocks from this device, then you should be a clock > > controller. How do consumers of this @clkout@ pin get the rate of the > > clock? > > I explained it to Guenter as he also argued about this. I'll wait for more > feedback but it's likely this will just turn into a clock provider, yes. > > > I'd kinda be expecting to see a clocks property with a maxItems of 1 and > > clock-names with two, mutually exclusive, options. > > > > The other part says, and it applies in multiple places here, that having > > integer properties with non-integer meanings is a poor ABI. I'd be vastly > > happier if the various instances in this file became enums of strings, > > or $re┤evant-unit so that a dts containing these properties is > > immediately understandable. > > Well, I think you're mentioning the 'gpio-mode' 'and under/over-voltage- > dividers'. I think for both it's clear that having the relevant units is not > feasible (at least I'm not seeing a way of properly do it). As for the strings, > well, I don't have any much to argue other than: Yeah, sorry - I was kinda making a general point there about not liking having integer values mapped to some sort of meaning, when units or some other sort of more meaningful property is possible. I really do not like these sorts of properties that you go and put "gpio-mode = <3>;" or whatever in the devicetree. I know its not quite units, but you could use 5, 10 & 15 as the allowable values for the divider property and I think that'd be fine. Oh and now that I think of it - for the divider property, how does "adi,undervoltage-dividers = 0" differ from omitting the property altogether? It's not entirely apparently from the binding what that actually means. If they do differ, I think you need to mention what the omission of the property means, and if they do not, then that = 0 case should be removed IMO. > 1) It's pattern seen in a lot of bindings - yes, that's not an excuse to copy > bad/wrong things over new bindings - but, honestly, it's the first time I have > someone complaining about it so I never thought it was wrong. > > 2) It makes much more easier to handle the properties in the driver (yeah, I > know that, as far as you're concerned, this does not matter to you :)) Yeah, with one hat on, sure, I don't care. Realistically I am aware that having these integers makes your life a little easier though. > > So yeah, if you insist on it, no strong reasons on my side to not do it. As long > as I see some consistency down the road :)). From me at least, I try to push people away from these sorts of integer properties and will continue to do so. Cheers, Conor. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings 2023-11-13 9:32 ` Nuno Sá 2023-11-13 20:12 ` Conor Dooley @ 2023-11-20 15:03 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2023-11-20 15:03 UTC (permalink / raw) To: Nuno Sá, Conor Dooley, Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley On 11/13/23 01:32, Nuno Sá wrote: >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + adi,clkout-mode: >>> + description: | >>> + Controls in which mode the CLKOUT PIN should work: >>> + 0 - Configures the CLKOUT pin to output the internal system clock >>> + 1 - Configures the CLKOUT pin to output the internal conversion >>> time >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >> >> I really am not a fan of these types of properties. Part of me says that >> if you're outputting clocks from this device, then you should be a clock >> controller. How do consumers of this @clkout@ pin get the rate of the >> clock? > > I explained it to Guenter as he also argued about this. I'll wait for more > feedback but it's likely this will just turn into a clock provider, yes. > That wasn't an argument, it was a request. If the chip acts as clock provider, it needs to register as one if that functionality is used/provided. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 15:18 [PATCH 0/2] Add support for LTC4282 Nuno Sa 2023-11-10 15:18 ` [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings Nuno Sa @ 2023-11-10 15:18 ` Nuno Sa 2023-11-10 16:50 ` Andy Shevchenko ` (3 more replies) 1 sibling, 4 replies; 17+ messages in thread From: Nuno Sa @ 2023-11-10 15:18 UTC (permalink / raw) To: linux-hwmon, linux-doc, devicetree Cc: Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Nuno Sa, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley The LTC4282 hot swap controller allows a board to be safely inserted and removed from a live backplane. Using one or more external N-channel pass transistors, board supply voltage and inrush current are ramped up at an adjustable rate. An I2C interface and onboard ADC allows for monitoring of board current, voltage, power, energy and fault status. Signed-off-by: Nuno Sa <nuno.sa@analog.com> --- Documentation/hwmon/ltc4282.rst | 101 ++ MAINTAINERS | 2 + drivers/hwmon/Kconfig | 11 + drivers/hwmon/Makefile | 1 + drivers/hwmon/ltc4282.c | 1518 +++++++++++++++++++++++++++++++ 5 files changed, 1633 insertions(+) create mode 100644 Documentation/hwmon/ltc4282.rst create mode 100644 drivers/hwmon/ltc4282.c diff --git a/Documentation/hwmon/ltc4282.rst b/Documentation/hwmon/ltc4282.rst new file mode 100644 index 000000000000..2fab5d165aa4 --- /dev/null +++ b/Documentation/hwmon/ltc4282.rst @@ -0,0 +1,101 @@ +Kernel drivers ltc4282 +========================================== + +Supported chips: + + * Analog Devices LTC4282 + + Prefix: 'ltc4282' + + Addresses scanned: - I2C 0x40 - 0x5A (7-bit) + Addresses scanned: - I2C 0x80 - 0xB4 with a step of 2 (8-bit) + + Datasheet: + + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf + +Author: Nuno Sá <nuno.sa@analog.com> + +Description +___________ + +The LTC4282 hot swap controller allows a board to be safely inserted and removed +from a live backplane. Using one or more external N-channel pass transistors, +board supply voltage and inrush current are ramped up at an adjustable rate. An +I2C interface and onboard ADC allows for monitoring of board current, voltage, +power, energy and fault status. The device features analog foldback current +limiting and supply monitoring for applications from 2.9V to 33V. Dual 12V gate +drive allows high power applications to either share safe operating area across +parallel MOSFETs or support a 2-stage start-up that first charges the load +capacitance followed by enabling a low on-resistance path to the load. The +LTC4282 is well suited to high power applications because the precise monitoring +capability and accurate current limiting reduce the extremes in which both loads +and power supplies must safely operate. Non-volatile configuration allows for +flexibility in the autonomous generation of alerts and response to faults. + +Sysfs entries +_____________ + +The following attributes are supported. Limits are read-write and all the other +attributes are read-only. + +======================= ========================================== +in0_input Output voltage (mV). +in0_min Undervoltage threshold +in0_max Overvoltage threshold +in0_lowest Lowest measured voltage +in0_highest Highest measured voltage +in0_min_alarm Undervoltage alarm +in0_max_alarm Overvoltage alarm +in0_label Channel label (VSOURCE) + +in1_input Input voltage (mV). +in1_lcrit_alarm Critical Undervoltage alarm +in1_lcrit_fault_log Undervoltage Fault Log. In some setups, clearing this + fault (writing on the file) might be needed. +in1_crit_alarm Critical Overvoltage alarm +in1_crit_fault_log Overvoltage Fault Log. In some setups, clearing this + fault (writing on the file) might be needed +in1_label Channel label (VDD) + +in2_input GPIO voltage (mV) +in2_min Undervoltage threshold +in2_max Overvoltage threshold +in2_lowest Lowest measured voltage +in2_highest Highest measured voltage +in2_min_alarm Undervoltage alarm +in2_max_alarm Overvoltage alarm +in2_label Channel label (VGPIO) + +curr1_input Sense current (mA) +curr1_min Undercurrent threshold +curr1_max Overcurrent threshold +curr1_lowest Lowest measured current +curr1_highest Highest measured current +curr1_min_alarm Undercurrent alarm +curr1_max_alarm Overcurrent alarm +curr1_crit_alarm Critical Overcurrent alarm +curr1_crit_fault_log Overcurrent Fault Log. In some setups, clearing this + fault (writing on the file) might be needed +curr1_label Channel label (ISENSE) + +power1_input Power (in uW) +power1_min Low power threshold +power1_max High power threshold +power1_average Average Power use +power1_input_lowest Historical minimum power use +power1_input_highest Historical maximum power use +power1_min_alarm Low power alarm +power1_max_alarm High power alarm +power1_good Power considered good +power1_fault_log Power bad Fault log +power1_label Channel label (Power) + +energy1_input Measured energy over time (in microJoule) +energy1_enable Enable/Disable Energy accumulation + +fet_short_fault FET short alarm +fet_short_fault_log FET short Fault log +fet_bad_fault FET bad alarm +fet_bad_fault_log FET bad Fault log +======================= ========================================== diff --git a/MAINTAINERS b/MAINTAINERS index 9f9527f6057b..ba7f0439d8e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12486,6 +12486,8 @@ M: Nuno Sa <nuno.sa@analog.com> L: linux-hwmon@vger.kernel.org S: Supported F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml +F: Documentation/hwmon/ltc4282.rst +F: drivers/hwmon/ltc4282.c LTC4306 I2C MULTIPLEXER DRIVER M: Michael Hennerich <michael.hennerich@analog.com> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index cf27523eed5a..2f1a0cf2e1e4 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1027,6 +1027,17 @@ config SENSORS_LTC4261 This driver can also be built as a module. If so, the module will be called ltc4261. +config SENSORS_LTC4282 + tristate "Analog Devices LTC4282" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for Analog Devices LTC4282 + High Current Hot Swap Controller I2C interface. + + This driver can also be built as a module. If so, the module will + be called ltc4282. + config SENSORS_LTQ_CPUTEMP bool "Lantiq cpu temperature sensor driver" depends on SOC_XWAY diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e84bd9685b5c..cbf22d1d736f 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_LTC4222) += ltc4222.o obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o obj-$(CONFIG_SENSORS_LTC4260) += ltc4260.o obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o +obj-$(CONFIG_SENSORS_LTC4282) += ltc4282.o obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o obj-$(CONFIG_SENSORS_MAX1111) += max1111.o obj-$(CONFIG_SENSORS_MAX127) += max127.o diff --git a/drivers/hwmon/ltc4282.c b/drivers/hwmon/ltc4282.c new file mode 100644 index 000000000000..738312cf212e --- /dev/null +++ b/drivers/hwmon/ltc4282.c @@ -0,0 +1,1518 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C + * + * Copyright 2023 Analog Devices Inc. + */ +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/cleanup.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/driver.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/i2c.h> +#include <linux/math.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/property.h> +#include <linux/units.h> + +#define LTC4282_CTRL_LSB 0x00 + #define LTC4282_CTRL_OV_RETRY_MASK BIT(0) + #define LTC4282_CTRL_UV_RETRY_MASK BIT(1) + #define LTC4282_CTRL_OC_RETRY_MASK BIT(2) + #define LTC4282_CTRL_ON_ACTIVE_LOW_MASK BIT(5) + #define LTC4282_CTRL_ON_DELAY_MASK BIT(6) +#define LTC4282_CTRL_MSB 0x01 + #define LTC4282_CTRL_VIN_MODE_MASK GENMASK(1, 0) + #define LTC4282_CTRL_OV_MODE_MASK GENMASK(3, 2) + #define LTC4282_CTRL_UV_MODE_MASK GENMASK(5, 4) +#define LTC4282_FAULT_LOG 0x04 + #define LTC4282_OV_FAULT_MASK BIT(0) + #define LTC4282_UV_FAULT_MASK BIT(1) + #define LTC4282_OC_FAULT_MASK BIT(2) + #define LTC4282_POWER_BAD_FAULT_MASK BIT(3) + #define LTC4282_FET_SHORT_FAULT_MASK BIT(5) + #define LTC4282_FET_BAD_FAULT_MASK BIT(6) +#define LTC4282_ADC_ALERT_LOG 0x05 + #define LTC4282_GPIO_ALARM_L_MASK BIT(0) + #define LTC4282_GPIO_ALARM_H_MASK BIT(1) + #define LTC4282_VSOURCE_ALARM_L_MASK BIT(2) + #define LTC4282_VSOURCE_ALARM_H_MASK BIT(3) + #define LTC4282_VSENSE_ALARM_L_MASK BIT(4) + #define LTC4282_VSENSE_ALARM_H_MASK BIT(5) + #define LTC4282_POWER_ALARM_L_MASK BIT(6) + #define LTC4282_POWER_ALARM_H_MASK BIT(7) +#define LTC4282_FET_BAD_FAULT_TIMEOUT 0x06 + #define LTC4282_FET_BAD_MAX_TIMEOUT 255 +#define LTC4282_GPIO_CONFIG 0x07 + #define LTC4282_GPIO_2_FET_STRESS_MASK BIT(1) + #define LTC4282_GPIO_1_OUT_MASK BIT(3) + #define LTC4282_GPIO_1_CONFIG_MASK GENMASK(5, 4) + #define LTC4282_GPIO_2_OUT_MASK BIT(6) + #define LTC4282_GPIO_3_OUT_MASK BIT(7) +#define LTC4282_VGPIO_MIN 0x08 +#define LTC4282_VGPIO_MAX 0x09 +#define LTC4282_VSOURCE_MIN 0x0a +#define LTC4282_VSOURCE_MAX 0x0b +#define LTC4282_VSENSE_MIN 0x0c +#define LTC4282_VSENSE_MAX 0x0d +#define LTC4282_POWER_MIN 0x0e +#define LTC4282_POWER_MAX 0x0f +#define LTC4282_CLK_DIV 0x10 + #define LTC4282_CLK_DIV_MASK GENMASK(4, 0) + #define LTC4282_CLKOUT_MASK GENMASK(6, 5) +#define LTC4282_ILIM_ADJUST 0x11 + #define LTC4282_GPIO_MODE_MASK BIT(1) + #define LTC4282_VDD_MONITOR_MASK BIT(2) + #define LTC4282_FOLDBACK_MODE_MASK GENMASK(4, 3) + #define LTC4282_ILIM_ADJUST_MASK GENMASK(7, 5) +#define LTC4282_ENERGY 0x12 +#define LTC4282_TIME_COUNTER 0x18 +#define LTC4282_ALERT_CTRL 0x1C + #define LTC4282_ALERT_OUT_MASK BIT(6) +#define LTC4282_ADC_CTRL 0x1D + #define LTC4282_FAULT_LOG_EN_MASK BIT(2) + #define LTC4282_METER_HALT_MASK BIT(5) + #define LTC4282_METER_RESET_MASK BIT(6) + #define LTC4282_RESET_MASK BIT(7) +#define LTC4282_STATUS_LSB 0x1E + #define LTC4282_OV_STATUS_MASK BIT(0) + #define LTC4282_UV_STATUS_MASK BIT(1) + #define LTC4282_VDD_STATUS_MASK (LTC4282_OV_STATUS_MASK | LTC4282_UV_STATUS_MASK) + #define LTC4282_OC_STATUS_MASK BIT(2) + #define LTC4282_POWER_GOOD_MASK BIT(3) + #define LTC4282_FET_SHORT_MASK BIT(5) + #define LTC4282_FET_BAD_STATUS_MASK BIT(6) +#define LTC4282_STATUS_MSB 0x1F + #define LTC4282_METER_TICK_OVERFLOW_MASK GENMASK(1, 0) + #define LTC4282_ALERT_STATUS_MASK BIT(4) + #define LTC4282_GPIO_1_STATUS_MASK BIT(5) + #define LTC4282_GPIO_2_STATUS_MASK BIT(6) + #define LTC4282_GPIO_3_STATUS_MASK BIT(7) +#define LTC4282_RESERVED_1 0x32 +#define LTC4282_RESERVED_2 0x33 +#define LTC4282_VGPIO 0x34 +#define LTC4282_VGPIO_LOWEST 0x36 +#define LTC4282_VGPIO_HIGHEST 0x38 +#define LTC4282_VSOURCE 0x3a +#define LTC4282_VSOURCE_LOWEST 0x3c +#define LTC4282_VSOURCE_HIGHEST 0x3e +#define LTC4282_VSENSE 0x40 +#define LTC4282_VSENSE_LOWEST 0x42 +#define LTC4282_VSENSE_HIGHEST 0x44 +#define LTC4282_POWER 0x46 +#define LTC4282_POWER_LOWEST 0x48 +#define LTC4282_POWER_HIGHEST 0x4a +#define LTC4282_RESERVED_3 0x50 + +#define LTC4282_CLKIN_MIN (250 * KILO) +#define LTC4282_CLKIN_MAX (15500 * KILO) +/* this assumes 12bit ADC */ +#define LTC4282_TCONV_US 65535 +#define LTC4282_GPIO_NR 4 +/* + * relaxed version of FIELD_PREP() to be used when mask is not a compile time constant + * u32_encode_bits() can't also be used as the compiler needs to be able to evaluate + * mask at compile time. + */ +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) + +struct ltc4282_state { + struct regmap *map; + struct device *dev; + /* Protect against multiple accesses to the device registers */ + struct mutex lock; + struct gpio_chip gc; + u64 saved_energy; + long power_max; + u32 gpio_map[LTC4282_GPIO_NR]; + u32 rsense; + u32 vin_mode; + u16 vfs_out; +}; + +struct ltc4282_gpio { + u32 out_reg; + u32 out_mask; + u32 in_reg; + u32 in_mask; + bool active_high; + u8 n_funcs; +}; + +enum { + LTC4282_VIN_3_3V, + LTC4282_VIN_5V, + LTC4282_VIN_12V, + LTC4282_VIN_24V, +}; + +enum { + LTC4282_CHAN_VSOURCE, + LTC4282_CHAN_VDD, + LTC4282_CHAN_VGPIO, +}; + +enum { + LTC4282_GPIO_1, + LTC4282_GPIO_2, + LTC4282_GPIO_3, + LTC4282_ALERT, +}; + +static const struct ltc4282_gpio ltc4282_gpios[] = { + [LTC4282_GPIO_1] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_1_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_1_OUT_MASK, + .active_high = true, + .n_funcs = 3, + }, + [LTC4282_GPIO_2] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_2_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_2_OUT_MASK, + .n_funcs = 3, + }, + [LTC4282_GPIO_3] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_GPIO_3_STATUS_MASK, + .out_reg = LTC4282_GPIO_CONFIG, + .out_mask = LTC4282_GPIO_3_OUT_MASK, + .n_funcs = 2, + }, + [LTC4282_ALERT] = { + .in_reg = LTC4282_STATUS_MSB, + .in_mask = LTC4282_ALERT_STATUS_MASK, + .out_reg = LTC4282_ALERT_CTRL, + .out_mask = LTC4282_ALERT_OUT_MASK, + }, +}; + +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + u32 gpio_pin = st->gpio_map[offset]; + + /* we can only control this for GPIO_1 */ + if (gpio_pin != LTC4282_GPIO_1) + return 0; + + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK); +} + +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset, int val) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + u32 gpio_pin = st->gpio_map[offset]; + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; + + guard(mutex)(&st->lock); + /* + * Explicitly setting the pin as output can only be done for GPIO_1. For the + * other pins we just pull the line down or high-z. + */ + if (gpio_pin == LTC4282_GPIO_1) { + int ret; + + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK, + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2)); + if (ret) + return ret; + } + + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down the line */ + if (!gpio->active_high) + val = !val; + + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, + LTC4282_FIELD_PREP(gpio->out_mask, val)); +} + +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, int val) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + u32 gpio_pin = st->gpio_map[offset]; + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; + + if (!gpio->active_high) + val = !val; + + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, + LTC4282_FIELD_PREP(gpio->out_mask, val)); +} + +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + struct ltc4282_state *st = gpiochip_get_data(chip); + u32 gpio_pin = st->gpio_map[offset]; + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; + int ret; + u32 val; + + ret = regmap_read(st->map, gpio->in_reg, &val); + if (ret) + return ret; + + return !!(val & gpio->in_mask); +} + +static int ltc4282_read_voltage_word(const struct ltc4282_state *st, u32 reg, u32 fs, long *val) +{ + __be16 in; + int ret; + + ret = regmap_bulk_read(st->map, reg, &in, sizeof(in)); + if (ret) + return ret; + + /* + * This is also used to calculate current in which case fs comes in 10 * uV. + * Hence the ULL usage. + */ + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); + return 0; +} + +static int ltc4282_read_voltage_byte(const struct ltc4282_state *st, u32 reg, u32 fs, long *val) +{ + int ret; + u32 in; + + ret = regmap_read(st->map, reg, &in); + if (ret) + return ret; + + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); + return 0; +} + +static int ltc4282_read_vdd(struct ltc4282_state *st, u32 fs, long *val) +{ + int ret; + + guard(mutex)(&st->lock); + + /* ADC now monitors VDD */ + ret = regmap_clear_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_VDD_MONITOR_MASK); + if (ret) + return ret; + + /* + * Wait for two ADC conversions so we are sure we get one full VDD + * measurement. + */ + msleep(2 * LTC4282_TCONV_US / MILLI); + + ret = ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, val); + if (ret) + return ret; + + /* back to VSOURCE */ + return regmap_set_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_VDD_MONITOR_MASK); +} + +static int ltc4282_read_alarm(struct ltc4282_state *st, u32 reg, u32 mask, long *val) +{ + u32 alarm; + int ret; + + guard(mutex)(&st->lock); + + /* if not status, clear first the alarm by clearing the bit */ + if (reg != LTC4282_STATUS_LSB && reg != LTC4282_FAULT_LOG) { + ret = regmap_clear_bits(st->map, reg, mask); + if (ret) + return ret; + + msleep(LTC4282_TCONV_US / MILLI); + } + + ret = regmap_read(st->map, reg, &alarm); + if (ret) + return ret; + + *val = !!(alarm & mask); + + return 0; +} + +static int ltc4282_read_in(struct device *dev, u32 attr, long *val, u32 channel) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_in_input: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, val); + if (channel == LTC4282_CHAN_VDD) + return ltc4282_read_vdd(st, st->vfs_out, val); + + return ltc4282_read_voltage_word(st, LTC4282_VGPIO, 1280, val); + case hwmon_in_highest: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE_HIGHEST, + st->vfs_out, val); + + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_HIGHEST, 1280, val); + case hwmon_in_lowest: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE_LOWEST, + st->vfs_out, val); + + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_LOWEST, 1280, val); + case hwmon_in_max_alarm: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_VSOURCE_ALARM_H_MASK, val); + + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_GPIO_ALARM_H_MASK, + val); + case hwmon_in_min_alarm: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, + LTC4282_VSOURCE_ALARM_L_MASK, val); + + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_GPIO_ALARM_L_MASK, + val); + case hwmon_in_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, + LTC4282_FET_BAD_STATUS_MASK, val); + case hwmon_in_crit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_OV_STATUS_MASK, val); + case hwmon_in_lcrit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_UV_STATUS_MASK, val); + case hwmon_in_max: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_voltage_byte(st, LTC4282_VSOURCE_MAX, + st->vfs_out, val); + + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MAX, 1280, val); + case hwmon_in_min: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_read_voltage_byte(st, LTC4282_VSOURCE_MIN, + st->vfs_out, val); + + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MIN, 1280, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_read_current_word(const struct ltc4282_state *st, u32 reg, long *val) +{ + long in; + int ret; + + /* + * We pass in full scale in 10 * micro (note that 40 is already millivolt) so we + * have better approximations to calculate current. + */ + ret = ltc4282_read_voltage_word(st, reg, DECA * 40 * MILLI, &in); + if (ret) + return ret; + + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); + + return 0; +} + +static int ltc4282_read_current_byte(const struct ltc4282_state *st, u32 reg, long *val) +{ + long in; + int ret; + + ret = ltc4282_read_voltage_byte(st, reg, DECA * 40 * MILLI, &in); + if (ret) + return ret; + + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); + + return 0; +} + +static int ltc4282_read_curr(struct device *dev, const u32 attr, long *val) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_curr_input: + return ltc4282_read_current_word(st, LTC4282_VSENSE, val); + case hwmon_curr_highest: + return ltc4282_read_current_word(st, LTC4282_VSENSE_HIGHEST, val); + case hwmon_curr_lowest: + return ltc4282_read_current_word(st, LTC4282_VSENSE_LOWEST, val); + case hwmon_curr_max: + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MAX, val); + case hwmon_curr_min: + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MIN, val); + case hwmon_curr_max_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_VSENSE_ALARM_H_MASK, + val); + case hwmon_curr_min_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_VSENSE_ALARM_L_MASK, + val); + case hwmon_curr_crit_alarm: + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_OC_STATUS_MASK, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, long *val) +{ + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; + __be16 raw; + u16 power; + int ret; + + ret = regmap_bulk_read(st->map, reg, &raw, sizeof(raw)); + if (ret) + return ret; + + power = be16_to_cpu(raw); + /* + * Power is given by: + * P = CODE(16b) * 0.040 * Vfs(out) * 2^16 / ((2^16 - 1)^2 * Rsense) + */ + if (check_mul_overflow(power * temp, MICRO, &temp_2)) { + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * (u64)st->rsense); + return 0; + } + + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * int_pow(U16_MAX, 2)); + + return 0; +} + +static int ltc4282_read_power_byte(const struct ltc4282_state *st, u32 reg, long *val) +{ + u32 power; + u64 temp; + int ret; + + ret = regmap_read(st->map, reg, &power); + if (ret) + return ret; + + temp = power * 40 * DECA * st->vfs_out * 256ULL; + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * st->rsense); + + return 0; +} + +static int ltc4282_read_energy(const struct ltc4282_state *st, u64 *val) +{ + u64 temp, energy; + u32 status; + __be64 raw; + int ret; + + ret = regmap_bulk_read(st->map, LTC4282_ENERGY, &raw, 6); + if (ret) + return ret; + + ret = regmap_read(st->map, LTC4282_STATUS_MSB, &status); + if (ret) + return ret; + + if (status & LTC4282_METER_TICK_OVERFLOW_MASK) { + dev_warn(st->dev, "Got overflow in meter/ticker counters\n"); + /* + * This resets the meter and the tick counter and holds them + * reset. + */ + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, + LTC4282_METER_RESET_MASK); + if (ret) + return ret; + + /* start accumulating again */ + ret = regmap_clear_bits(st->map, LTC4282_ADC_CTRL, + LTC4282_METER_RESET_MASK); + if (ret) + return ret; + + /* + * Let the callers know a reset happened. Important when calling + * from power1_average. + */ + ret = 1; + } + + energy = be64_to_cpu(raw) >> 16; + /* + * The formula for energy is given by: + * E = CODE(48b) * 0.040 * Vfs(out) * Tconv * 256 / ((2^16 - 1)^2 * Rsense) + * + * Since we only support 12bit ADC, Tconv = 0.065535s. Passing Vfs(out) and 0.040 to + * mV and Tconv to us, we can simplify the formula to: + * E = CODE(48b) * 40 * Vfs(out) * 256 / (U16_MAX * Rsense) + * + * As Rsense is in tens of micro-ohm, we need to multiply by DECA to get + * microujoule. + */ + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, &temp)) { + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, U16_MAX); + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); + return ret; + } + + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); + + return ret; +} + +static int ltc4282_read_power_average(struct ltc4282_state *st, long *val) +{ + u64 energy, temp; + __be32 raw; + u32 count; + int ret; + + guard(mutex)(&st->lock); + + ret = ltc4282_read_energy(st, &energy); + if (ret < 0) + return ret; + if (ret == 1) { + /* + * reset happened... let's read the new energy value that + * together with the new tick counter should give a sane average + * value. Furthermore, we save whatever value we had accumulated + * so that the next energy read will have it into account. + */ + st->saved_energy = energy; + /* give some time for accumulation... */ + msleep(2 * LTC4282_TCONV_US / MILLI); + ret = ltc4282_read_energy(st, &energy); + if (ret < 0) + return ret; + } + + ret = regmap_bulk_read(st->map, LTC4282_TIME_COUNTER, &raw, sizeof(raw)); + if (ret) + return ret; + + count = be32_to_cpu(raw); + if (!count) { + *val = 0; + return 0; + } + + /* + * AVG = E / (Tconv * counter) + * We get energy in microJoule, hence dividing it by microSeconds gives Watts. Therefore, + * multiplying by MICRO gives us microWatts. + */ + if (check_mul_overflow(energy, MICRO, &temp)) { + temp = DIV_ROUND_CLOSEST_ULL(energy, LTC4282_TCONV_US); + *val = DIV_ROUND_CLOSEST_ULL(temp * MICRO, count); + return 0; + } + + *val = DIV64_U64_ROUND_CLOSEST(temp, LTC4282_TCONV_US * (u64)count); + return 0; +} + +static int ltc4282_read_power(struct device *dev, const u32 attr, long *val) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_power_input: + return ltc4282_read_power_word(st, LTC4282_POWER, val); + case hwmon_power_input_highest: + return ltc4282_read_power_word(st, LTC4282_POWER_HIGHEST, val); + case hwmon_power_input_lowest: + return ltc4282_read_power_word(st, LTC4282_POWER_LOWEST, val); + case hwmon_power_max_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_POWER_ALARM_H_MASK, + val); + case hwmon_power_min_alarm: + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_POWER_ALARM_L_MASK, + val); + case hwmon_power_average: + return ltc4282_read_power_average(st, val); + case hwmon_power_max: + return ltc4282_read_power_byte(st, LTC4282_POWER_MAX, val); + case hwmon_power_min: + return ltc4282_read_power_byte(st, LTC4282_POWER_MIN, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_energy_enable(struct device *dev, long *val) +{ + const struct ltc4282_state *st = dev_get_drvdata(dev); + u32 reg_val; + int ret; + + ret = regmap_read(st->map, LTC4282_ADC_CTRL, ®_val); + if (ret) + return ret; + + *val = !FIELD_GET(LTC4282_METER_HALT_MASK, reg_val); + + return 0; +} + +static int ltc4282_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + switch (type) { + case hwmon_in: + return ltc4282_read_in(dev, attr, val, channel); + case hwmon_curr: + return ltc4282_read_curr(dev, attr, val); + case hwmon_power: + return ltc4282_read_power(dev, attr, val); + case hwmon_energy: + return ltc4282_energy_enable(dev, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_write_power_byte(const struct ltc4282_state *st, u32 reg, long val) +{ + u32 power; + u64 temp; + + if (val > st->power_max) + val = st->power_max; + + temp = val * int_pow(U8_MAX, 2) * st->rsense; + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st->vfs_out * 40); + + return regmap_write(st->map, reg, power); +} + +static int ltc4282_write_power(const struct ltc4282_state *st, u32 attr, + long val) +{ + switch (attr) { + case hwmon_power_max: + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, val); + case hwmon_power_min: + return ltc4282_write_power_byte(st, LTC4282_POWER_MIN, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_write_voltage_byte(const struct ltc4282_state *st, u32 reg, u32 fs, long val) +{ + u32 in; + + if (val >= fs) + in = U8_MAX; + else + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); + + return regmap_write(st->map, reg, in); +} + +static int ltc4282_write_in(const struct ltc4282_state *st, u32 attr, long val, + int channel) +{ + switch (attr) { + case hwmon_in_max: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_write_voltage_byte(st, LTC4282_VSOURCE_MAX, + st->vfs_out, val); + + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MAX, 1280, val); + case hwmon_in_min: + if (channel == LTC4282_CHAN_VSOURCE) + return ltc4282_write_voltage_byte(st, LTC4282_VSOURCE_MIN, + st->vfs_out, val); + + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MIN, 1280, val); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_write_curr(const struct ltc4282_state *st, u32 attr, + long val) +{ + /* need to pass it in millivolt */ + u32 in = DIV_ROUND_CLOSEST_ULL((u64)val * st->rsense, DECA * MICRO); + + switch (attr) { + case hwmon_curr_max: + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40, in); + case hwmon_curr_min: + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MIN, 40, in); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_write(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + const struct ltc4282_state *st = dev_get_drvdata(dev); + + switch (type) { + case hwmon_power: + return ltc4282_write_power(st, attr, val); + case hwmon_in: + return ltc4282_write_in(st, attr, val, channel); + case hwmon_curr: + return ltc4282_write_curr(st, attr, val); + case hwmon_energy: + /* setting the bit halts the meter */ + return regmap_update_bits(st->map, LTC4282_ADC_CTRL, + LTC4282_METER_HALT_MASK, + FIELD_PREP(LTC4282_METER_HALT_MASK, !!!val)); + default: + return -ENOTSUPP; + } +} + +static int ltc4282_in_is_visible(const struct ltc4282_state *st, u32 attr) +{ + switch (attr) { + case hwmon_in_input: + case hwmon_in_highest: + case hwmon_in_lowest: + case hwmon_in_max_alarm: + case hwmon_in_min_alarm: + case hwmon_in_label: + case hwmon_in_lcrit_alarm: + case hwmon_in_crit_alarm: + return 0444; + case hwmon_in_max: + case hwmon_in_min: + return 0644; + default: + return 0; + } +} + +static int ltc4282_curr_is_visible(u32 attr) +{ + switch (attr) { + case hwmon_curr_input: + case hwmon_curr_highest: + case hwmon_curr_lowest: + case hwmon_curr_max_alarm: + case hwmon_curr_min_alarm: + case hwmon_curr_crit_alarm: + case hwmon_curr_label: + return 0444; + case hwmon_curr_max: + case hwmon_curr_min: + return 0644; + default: + return 0; + } +} + +static int ltc4282_power_is_visible(u32 attr) +{ + switch (attr) { + case hwmon_power_input: + case hwmon_power_input_highest: + case hwmon_power_input_lowest: + case hwmon_power_label: + case hwmon_power_max_alarm: + case hwmon_power_min_alarm: + case hwmon_power_average: + return 0444; + case hwmon_power_max: + case hwmon_power_min: + return 0644; + default: + return 0; + } +} + +static umode_t ltc4282_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_in: + return ltc4282_in_is_visible(data, attr); + case hwmon_curr: + return ltc4282_curr_is_visible(attr); + case hwmon_power: + return ltc4282_power_is_visible(attr); + case hwmon_energy: + /* hwmon_energy_enable */ + return 0644; + default: + return -ENOTSUPP; + } +} + +static const char * const ltc4282_in_strs[] = { + "VSOURCE", "VDD", "VGPIO" +}; + +static int ltc4282_read_labels(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + switch (type) { + case hwmon_in: + *str = ltc4282_in_strs[channel]; + return 0; + case hwmon_curr: + *str = "ISENSE"; + return 0; + case hwmon_power: + *str = "Power"; + return 0; + default: + return -ENOTSUPP; + } +} + +static ssize_t ltc4282_show_energy(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + u64 energy; + long en; + int ret; + + guard(mutex)(&st->lock); + + ret = ltc4282_energy_enable(dev, &en); + if (ret) + return ret; + if (!en) + return -ENODATA; + + ret = ltc4282_read_energy(st, &energy); + if (ret < 0) + return ret; + + /* see @ltc4282_read_power_average */ + if (st->saved_energy) { + energy += st->saved_energy; + st->saved_energy = 0; + } + + return sysfs_emit(buf, "%llu\n", energy); +} + +static ssize_t ltc4282_show_fault(struct device *dev, + struct device_attribute *da, + u32 reg, char *buf) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); + long alarm; + int ret; + + ret = ltc4282_read_alarm(st, reg, attr->index, &alarm); + if (ret) + return ret; + + return sysfs_emit(buf, "%lu\n", alarm); +} + +static ssize_t ltc4282_show_status(struct device *dev, + struct device_attribute *da, char *buf) +{ + return ltc4282_show_fault(dev, da, LTC4282_STATUS_LSB, buf); +} + +static ssize_t ltc4282_show_fault_log(struct device *dev, + struct device_attribute *da, char *buf) +{ + return ltc4282_show_fault(dev, da, LTC4282_FAULT_LOG, buf); +} + +static ssize_t ltc4282_clear_fault_log(struct device *dev, + struct device_attribute *da, + const char *buf, size_t len) +{ + struct ltc4282_state *st = dev_get_drvdata(dev); + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); + int ret; + + ret = regmap_clear_bits(st->map, LTC4282_FAULT_LOG, attr->index); + if (ret) + return ret; + + return len; +} + +static int ltc4282_power_on(const struct ltc4282_state *st) +{ + u32 n_tries = 5, reg; + int ret; + + ret = devm_regulator_get_enable(st->dev, "vdd"); + if (ret) + return dev_err_probe(st->dev, ret, "Failed vdd get/enable\n"); + + /* + * Make sure vdd is stable. From the datasheet: + * The state of the UV and OV comparators is indicated by the STATUS register + * bits 0 and 1 and must be stable for at least 50ms to qualify for turn-on. + */ + do { + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg, + reg & LTC4282_VDD_STATUS_MASK, 10000, 50000); + if (!ret) + continue; + if (ret != -ETIMEDOUT) + return dev_err_probe(st->dev, ret, "Failed regmap read\n"); + + /* Alright. We got timeout which means UV and OV are stable for 50ms */ + break; + } while (n_tries--); + + if (!n_tries) + return dev_err_probe(st->dev, -ETIMEDOUT, "VDD not stable\n"); + + return 0; +} + +enum { + LTC4282_CLKOUT_INT, + LTC4282_CLKOUT_TICK, +}; + +static int ltc428_clks_setup(const struct ltc4282_state *st) +{ + unsigned long rate; + struct clk *clkin; + u32 val; + int ret; + + ret = device_property_read_u32(st->dev, "adi,clkout-mode", &val); + if (ret) + return 0; + + if (val > LTC4282_CLKOUT_TICK) + return dev_err_probe(st->dev, -EINVAL, + "Invalid val(%u) for adi,clkout-mode\n", val); + + ret = regmap_update_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLKOUT_MASK, + FIELD_PREP(LTC4282_CLKOUT_MASK, val + 1)); + if (ret) + return ret; + + clkin = devm_clk_get_optional_enabled(st->dev, NULL); + if (IS_ERR(clkin)) + return dev_err_probe(st->dev, PTR_ERR(clkin), "Failed to get clkin"); + if (!clkin) + return 0; + + rate = clk_get_rate(clkin); + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX) + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin range(%lu) [%lu %lu]\n", + rate, LTC4282_CLKIN_MIN, LTC4282_CLKIN_MAX); + + /* + * Clocks faster than 250KHZ should be reduced to 250KHZ. The clock frequency + * is divided by twice the value in the register. + */ + val = rate / (2 * LTC4282_CLKIN_MIN); + + return regmap_update_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLK_DIV_MASK, + FIELD_PREP(LTC4282_CLK_DIV_MASK, val)); +} + +static const int ltc4282_curr_lim_uv[] = { + 12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375 +}; + +static int ltc4282_get_defaults(struct ltc4282_state *st, u32 *curr_lim_uv) +{ + u32 reg_val, ilm_adjust; + int ret; + + ret = regmap_read(st->map, LTC4282_CTRL_MSB, ®_val); + if (ret) + return ret; + + st->vin_mode = FIELD_GET(LTC4282_CTRL_VIN_MODE_MASK, reg_val); + + ret = regmap_read(st->map, LTC4282_ILIM_ADJUST, ®_val); + if (ret) + return ret; + + ilm_adjust = FIELD_GET(LTC4282_ILIM_ADJUST_MASK, reg_val); + *curr_lim_uv = ltc4282_curr_lim_uv[ilm_adjust]; + + return 0; +} + +/* + * Set max limits for ISENSE and Power as that depends on the max voltage on rsense + * that is defined in ILIM_ADJUST. This is specially important for power because for + * some rsense and vfsout values, if we allow the default raw 255 value, that would + * overflow long in 32bit archs when reading back the max power limit. + */ +static int ltc4282_set_max_limits(struct ltc4282_state *st, u32 val_uv) +{ + int ret; + + ret = ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40 * MILLI, val_uv); + if (ret) + return ret; + + /* Power is given by ISENSE * Vout. */ + st->power_max = DIV_ROUND_CLOSEST(val_uv * DECA * MILLI, st->rsense) * st->vfs_out; + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, st->power_max); +} + +/* valid GPIO functions */ +enum { + LTC4282_PIN_GPIO, + /* Power functions only for GPIO_1*/ + LTC4282_PIN_POWER_BAD, + LTC4282_PIN_POWER_GOOD, + /* ADC monitor only for GPIO_2 and 3 */ + LTC4282_PIN_ADC = 2, + /* Only for GPIO_2 */ + LTC4282_PIN_FET_STRESS, +}; + +static int ltc4282_non_gpio_setup(struct ltc4282_state *st, u32 pin, u32 func, bool *adc_in) +{ + if (pin == LTC4282_GPIO_1) { + u32 val = LTC4282_PIN_POWER_BAD; + + if (func == LTC4282_PIN_POWER_GOOD) + val = 0; + + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK, + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, val)); + } + + if (func == LTC4282_PIN_FET_STRESS) + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_2_FET_STRESS_MASK, + FIELD_PREP(LTC4282_GPIO_2_FET_STRESS_MASK, 1)); + + /* + * Then, let's point the given GPIO to the ADC input. We need to ensure that + * function is only given once. + */ + if (*adc_in) + return dev_err_probe(st->dev, -EINVAL, + "Only one gpio can be given to the ADC input\n"); + + *adc_in = true; + + /* setting the bit to 1 cause the ADC to monitor GPIO2 */ + return regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_GPIO_MODE_MASK, + FIELD_PREP(LTC4282_GPIO_MODE_MASK, pin == LTC4282_GPIO_2)); +} + +static const char * const ltc4282_gpio_prop[] = { + "adi,gpio1-mode", "adi,gpio2-mode", "adi,gpio3-mode" +}; + +static int ltc4282_gpio_setup(struct ltc4282_state *st) +{ + struct device *dev = st->dev; + u32 gpio, func, ngpios = 0; + bool adc_in = false; + int ret; + + if (!IS_ENABLED(CONFIG_GPIOLIB)) + return 0; + + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { + ret = device_property_read_u32(dev, ltc4282_gpio_prop[gpio], &func); + if (ret) + continue; + if (func >= ltc4282_gpios[gpio].n_funcs) + return dev_err_probe(dev, ret, "Invalid func(%u >= %u) for gpio%u\n", + func, ltc4282_gpios[gpio].n_funcs, gpio + 1); + if (func == LTC4282_PIN_GPIO) { + st->gpio_map[ngpios++] = gpio; + if (gpio == LTC4282_GPIO_1) { + /* default to input GPIO */ + ret = regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, + LTC4282_GPIO_1_CONFIG_MASK); + if (ret) + return ret; + } + + continue; + } + + ret = ltc4282_non_gpio_setup(st, gpio, func, &adc_in); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,gpio-alert")) + st->gpio_map[ngpios++] = LTC4282_ALERT; + + if (!ngpios) + return 0; + + st->gc.parent = dev; + st->gc.base = -1; + st->gc.ngpio = ngpios; + st->gc.can_sleep = true; + st->gc.label = "ltc4282"; + st->gc.direction_input = ltc4282_gpio_input_set; + st->gc.direction_output = ltc4282_gpio_output_set; + st->gc.set = ltc4282_gpio_set; + st->gc.get = ltc4282_gpio_get; + + return devm_gpiochip_add_data(dev, &st->gc, st); +} + +/* This maps the Vout full scale for the given Vin mode */ +static const u16 ltc4282_vfs_milli[] = { 5540, 8320, 16640, 33280 }; + +enum { + LTC4282_DIV_EXTERNAL, + LTC4282_DIV_5_PERCENT, + LTC4282_DIV_10_PERCENT, + LTC4282_DIV_15_PERCENT, +}; + +static int ltc4282_setup(struct ltc4282_state *st) +{ + struct device *dev = st->dev; + u32 val, curr_lim_uv; + int ret; + + /* The part has an eeprom so let's get the needed defaults from it */ + ret = ltc4282_get_defaults(st, &curr_lim_uv); + if (ret) + return ret; + + ret = device_property_read_u32(dev, "adi,rsense-nano-ohms", &st->rsense); + if (ret) + return dev_err_probe(dev, ret, "Failed to read adi,rsense-nano-ohms\n"); + + /* + * The resolution for rsense is tens of micro which means we need nano in the bindings. + * However, to make things easier to handle (with respect to overflows) we divide it by + * 100 as we don't really need the last two digits. + */ + st->rsense /= CENTI; + + ret = device_property_read_u32(dev, "vin-mode-microvolt", &val); + if (!ret) { + switch (val) { + case 3300000: + st->vin_mode = LTC4282_VIN_3_3V; + break; + case 5000000: + st->vin_mode = LTC4282_VIN_5V; + break; + case 12000000: + st->vin_mode = LTC4282_VIN_12V; + break; + case 24000000: + st->vin_mode = LTC4282_VIN_24V; + break; + default: + return dev_err_probe(dev, -EINVAL, + "Invalid val(%u) for vin-mode-microvolt\n", val); + } + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_VIN_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_VIN_MODE_MASK, st->vin_mode)); + if (ret) + return ret; + + /* Foldback mode should also be set to the input voltage */ + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_FOLDBACK_MODE_MASK, + FIELD_PREP(LTC4282_FOLDBACK_MODE_MASK, st->vin_mode)); + if (ret) + return ret; + } + + st->vfs_out = ltc4282_vfs_milli[st->vin_mode]; + + ret = device_property_read_u32(dev, "adi,current-limit-microvolt", + &curr_lim_uv); + if (!ret) { + int reg_val; + + switch (val) { + case 12500: + reg_val = 0; + break; + case 15625: + reg_val = 1; + break; + case 18750: + reg_val = 2; + break; + case 21875: + reg_val = 3; + break; + case 25000: + reg_val = 4; + break; + case 28125: + reg_val = 5; + break; + case 31250: + reg_val = 6; + break; + case 34375: + reg_val = 7; + break; + default: + return dev_err_probe(dev, -EINVAL, + "Invalid val(%u) for adi,current-limit-microvolt\n", + val); + } + + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_ILIM_ADJUST_MASK, + FIELD_PREP(LTC4282_ILIM_ADJUST_MASK, reg_val)); + if (ret) + return ret; + } + + ret = ltc4282_set_max_limits(st, curr_lim_uv); + if (ret) + return ret; + + ret = device_property_read_u32(st->dev, "adi,overvoltage-dividers", &val); + if (!ret) { + if (val > LTC4282_DIV_15_PERCENT) + return dev_err_probe(st->dev, -EINVAL, + "Invalid val(%u) for adi,overvoltage-divider\n", val); + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_OV_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_OV_MODE_MASK, val)); + } + + ret = device_property_read_u32(st->dev, "adi,undervoltage-dividers", &val); + if (!ret) { + if (val > LTC4282_DIV_15_PERCENT) + return dev_err_probe(st->dev, -EINVAL, + "Invalid val(%u) for adi,undervoltage-divider\n", val); + + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_UV_MODE_MASK, + FIELD_PREP(LTC4282_CTRL_UV_MODE_MASK, val)); + } + + if (device_property_read_bool(dev, "adi,overcurrent-retry")) { + ret = regmap_set_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_OC_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,overvoltage-retry-disable")) { + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_OV_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,undervoltage-retry-disable")) { + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_UV_RETRY_MASK); + if (ret) + return ret; + } + + if (device_property_read_bool(dev, "adi,fault-log-enable")) { + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_FAULT_LOG_EN_MASK); + if (ret) + return ret; + } + + ret = device_property_read_u32(dev, "adi,fet-bad-timeout-ms", &val); + if (!ret) { + if (val > LTC4282_FET_BAD_MAX_TIMEOUT) + return dev_err_probe(dev, -EINVAL, + "Invalid value(%u) for adi,fet-bad-timeout-ms", val); + + ret = regmap_write(st->map, LTC4282_FET_BAD_FAULT_TIMEOUT, val); + if (ret) + return ret; + } + + return ltc4282_gpio_setup(st); +} + +static bool ltc4282_readable_reg(struct device *dev, unsigned int reg) +{ + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) + return false; + + return true; +} + +static bool ltc4282_writable_reg(struct device *dev, unsigned int reg) +{ + if (reg == LTC4282_STATUS_LSB || reg == LTC4282_STATUS_MSB) + return false; + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) + return false; + + return true; +} + +static const struct regmap_config ltc4282_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = LTC4282_RESERVED_3, + .readable_reg = ltc4282_readable_reg, + .writeable_reg = ltc4282_writable_reg, +}; + +static const struct hwmon_channel_info * const ltc4282_info[] = { + HWMON_CHANNEL_INFO(in, + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | + HWMON_I_MAX_ALARM | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LCRIT_ALARM | HWMON_I_CRIT_ALARM | + HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | + HWMON_I_MAX_ALARM | HWMON_I_LABEL), + HWMON_CHANNEL_INFO(curr, + HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST | + HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM | + HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM | + HWMON_C_LABEL), + HWMON_CHANNEL_INFO(power, + HWMON_P_INPUT | HWMON_P_INPUT_LOWEST | + HWMON_P_INPUT_HIGHEST | HWMON_P_MAX | HWMON_P_MIN | + HWMON_P_AVERAGE | HWMON_P_MAX_ALARM | HWMON_P_MIN_ALARM | + HWMON_P_LABEL), + HWMON_CHANNEL_INFO(energy, + HWMON_E_ENABLE), + NULL +}; + +static const struct hwmon_ops ltc4282_hwmon_ops = { + .read = ltc4282_read, + .write = ltc4282_write, + .is_visible = ltc4282_is_visible, + .read_string = ltc4282_read_labels, +}; + +static const struct hwmon_chip_info ltc2947_chip_info = { + .ops = <c4282_hwmon_ops, + .info = ltc4282_info, +}; + +/* energy attributes are 6bytes wide so we need u64 */ +static SENSOR_DEVICE_ATTR(energy1_input, 0444, ltc4282_show_energy, NULL, 0); +/* power1_fault */ +static SENSOR_DEVICE_ATTR(power1_good, 0444, ltc4282_show_status, NULL, + LTC4282_POWER_GOOD_MASK); +/* FET faults */ +static SENSOR_DEVICE_ATTR(fet_short_fault, 0444, ltc4282_show_status, NULL, + LTC4282_FET_SHORT_MASK); +static SENSOR_DEVICE_ATTR(fet_bad_fault, 0444, ltc4282_show_status, NULL, + LTC4282_FET_BAD_STATUS_MASK); +/* + * Fault log failures. These faults might be important in systems where auto-retry is not enabled + * since they will cause the part to latch off until they are cleared. Typically that happens + * when the system admin is close enough so he can check what happened and manually clear the + * faults. Moreover, manually clearing the faults might only matter when ON_FAULT_MASK in the + * CONTROL register is set (which is the default) as in that case, a turn off signal from the + * ON pin won't clear them. + */ +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK); +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK); +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK); +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_POWER_BAD_FAULT_MASK); +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_FET_BAD_FAULT_MASK); +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, ltc4282_show_fault_log, + ltc4282_clear_fault_log, LTC4282_FET_SHORT_FAULT_MASK); + +static struct attribute *ltc4282_attrs[] = { + &sensor_dev_attr_energy1_input.dev_attr.attr, + &sensor_dev_attr_power1_good.dev_attr.attr, + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, + &sensor_dev_attr_fet_short_fault.dev_attr.attr, + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr, + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr, + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr, + &sensor_dev_attr_power1_fault_log.dev_attr.attr, + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr, + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr, + NULL, +}; +ATTRIBUTE_GROUPS(ltc4282); + +static int ltc4282_probe(struct i2c_client *i2c) +{ + struct device *dev = &i2c->dev, *hwmon; + struct ltc4282_state *st; + int ret; + + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); + if (!st) + return dev_err_probe(dev, -ENOMEM, "Failed to allocate memory\n"); + + st->dev = dev; + st->map = devm_regmap_init_i2c(i2c, <c4282_regmap_config); + if (IS_ERR(st->map)) + return dev_err_probe(dev, PTR_ERR(st->map), "failed regmap init\n"); + + ret = ltc4282_power_on(st); + if (ret) + return ret; + + /* Soft reset */ + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_RESET_MASK); + if (ret) + return ret; + + msleep(3200); + + ret = ltc428_clks_setup(st); + if (ret) + return ret; + + ret = ltc4282_setup(st); + if (ret) + return ret; + + mutex_init(&st->lock); + hwmon = devm_hwmon_device_register_with_info(dev, "ltc4282", st, <c2947_chip_info, + ltc4282_groups); + return PTR_ERR_OR_ZERO(hwmon); +} + +static const struct of_device_id ltc4282_of_match[] = { + { .compatible = "adi,ltc4282" }, + {} +}; +MODULE_DEVICE_TABLE(of, ltc4282_of_match); + +static struct i2c_driver ltc4282_driver = { + .driver = { + .name = "ltc4282", + .of_match_table = ltc4282_of_match, + }, + .probe = ltc4282_probe, +}; +module_i2c_driver(ltc4282_driver); + +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); +MODULE_DESCRIPTION("LTC4282 I2C High Current Hot Swap Controller"); +MODULE_LICENSE("GPL"); -- 2.42.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa @ 2023-11-10 16:50 ` Andy Shevchenko 2023-11-13 10:13 ` Nuno Sá 2023-11-11 1:04 ` kernel test robot ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2023-11-10 16:50 UTC (permalink / raw) To: Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Jean Delvare, Conor Dooley On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: > The LTC4282 hot swap controller allows a board to be safely inserted and > removed from a live backplane. Using one or more external N-channel pass > transistors, board supply voltage and inrush current are ramped up at an > adjustable rate. An I2C interface and onboard ADC allows for monitoring > of board current, voltage, power, energy and fault status. There are plenty of ltc42xx in the same folder and a lot of other chips. Have you checked that none of the existing driver is not close enough and brand new driver is indeed required? If so, add a respective paragraph somewhere (cover letter? TBH I haven't read it, it's fine if it's already explained there). ... > +#define LTC4282_STATUS_MSB 0x1F > +#define LTC4282_VSOURCE 0x3a > +#define LTC4282_VSOURCE_LOWEST 0x3c > +#define LTC4282_VSOURCE_HIGHEST 0x3e You have an inconsistency in the capitalization of the hexadecimal values. ... > +/* > + * relaxed version of FIELD_PREP() to be used when mask is not a compile time constant > + * u32_encode_bits() can't also be used as the compiler needs to be able to evaluate > + * mask at compile time. > + */ > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) Can we name it accordingly as done in other places, and TBH it's a time to move it to the header. (At least I know about two more implementations of this). ... > +struct ltc4282_state { > + struct regmap *map; > + struct device *dev; > + /* Protect against multiple accesses to the device registers */ > + struct mutex lock; > + struct gpio_chip gc; > + u64 saved_energy; > + long power_max; > + u32 gpio_map[LTC4282_GPIO_NR]; Why is this needed? The valid_mask is not enough? > + u32 rsense; > + u32 vin_mode; > + u16 vfs_out; > +}; ... > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down the line */ > + if (!gpio->active_high) Hmm... Why do you need a separate flag for this? Shouldn't be described or autodetected somehow? > + val = !val; ... > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); I'm wondering if you can do some trick to "divide" actually to 2^16 so, it will not use division op at all? ... > + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); Ditto. ... > + /* > + * Wait for two ADC conversions so we are sure we get one full VDD > + * measurement. > + */ > + msleep(2 * LTC4282_TCONV_US / MILLI); fsleep() ? ... > +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, long *val) > +{ > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; Too many spaces. "* BIT(16)" / "* BIT_ULL(16)" ? ... > + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * (u64)st->rsense); > + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * int_pow(U16_MAX, 2)); Same Q about possible optimizations. ... > + temp = power * 40 * DECA * st->vfs_out * 256ULL; BIT_ULL(8) ? ... > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * st->rsense); Same Q about possible optimizations. ... > + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, &temp)) { > + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, U16_MAX); > + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); > + return ret; > + } BIT(8) ? ... > + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); Same Q about possible optimizations. ... > + /* > + * reset happened... let's read the new energy value that Reset Let's > + * together with the new tick counter should give a sane average > + * value. Furthermore, we save whatever value we had accumulated > + * so that the next energy read will have it into account. > + */ ... > + /* give some time for accumulation... */ > + msleep(2 * LTC4282_TCONV_US / MILLI); fsleep() ? ... > + /* > + * AVG = E / (Tconv * counter) > + * We get energy in microJoule, hence dividing it by microSeconds gives Watts. Therefore, > + * multiplying by MICRO gives us microWatts. > + */ Here you used long lines, somewhere else, much shorter. Please, choose one limit and be consistent with it. ... > + temp = val * int_pow(U8_MAX, 2) * st->rsense; > + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st->vfs_out * 40); As per above. ... > + if (val >= fs) > + in = U8_MAX; > + else > + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); Can clamp() be used here? ... > + /* > + * Make sure vdd is stable. From the datasheet: > + * The state of the UV and OV comparators is indicated by the STATUS register > + * bits 0 and 1 and must be stable for at least 50ms to qualify for turn-on. > + */ > + do { > + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg, > + reg & LTC4282_VDD_STATUS_MASK, 10000, 50000); > + if (!ret) > + continue; > + if (ret != -ETIMEDOUT) > + return dev_err_probe(st->dev, ret, "Failed regmap read\n"); I would do it a bit differently. > + break; > + } while (n_tries--); } while (--n_tries); ? Altogether: do { ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg, reg & LTC4282_VDD_STATUS_MASK, 10000, 50000); /* Check if we got timeout which means UV and OV are stable for 50ms */ if (ret == -ETIMEDOUT) break; if (ret) return dev_err_probe(st->dev, ret, "Failed regmap read\n"); } while (--n_tries); ... > + if (val > LTC4282_CLKOUT_TICK) > + return dev_err_probe(st->dev, -EINVAL, > + "Invalid val(%u) for adi,clkout-mode\n", val); ERANGE? ... > + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX) in_range() ? > + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin range(%lu) [%lu %lu]\n", > + rate, LTC4282_CLKIN_MIN, LTC4282_CLKIN_MAX); ... > + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { Is 0 also being defined? > + ret = device_property_read_u32(dev, ltc4282_gpio_prop[gpio], &func); > + if (ret) > + continue; > + if (func >= ltc4282_gpios[gpio].n_funcs) > + return dev_err_probe(dev, ret, "Invalid func(%u >= %u) for gpio%u\n", > + func, ltc4282_gpios[gpio].n_funcs, gpio + 1); > + if (func == LTC4282_PIN_GPIO) { > + st->gpio_map[ngpios++] = gpio; > + if (gpio == LTC4282_GPIO_1) { > + /* default to input GPIO */ > + ret = regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, > + LTC4282_GPIO_1_CONFIG_MASK); > + if (ret) > + return ret; > + } > + > + continue; > + } ... > + switch (val) { > + case 3300000: Hmm... Do we have MICROVOLTS_IN_VOLTS or so? > + st->vin_mode = LTC4282_VIN_3_3V; > + break; > + case 5000000: > + st->vin_mode = LTC4282_VIN_5V; > + break; > + case 12000000: > + st->vin_mode = LTC4282_VIN_12V; > + break; > + case 24000000: > + st->vin_mode = LTC4282_VIN_24V; > + break; > + default: > + return dev_err_probe(dev, -EINVAL, > + "Invalid val(%u) for vin-mode-microvolt\n", val); > + } ... > +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_POWER_BAD_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_FET_BAD_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_FET_SHORT_FAULT_MASK); SENSOR_DEVICE_ATTR_RO() / _RW() ? ... > +static struct attribute *ltc4282_attrs[] = { > + &sensor_dev_attr_energy1_input.dev_attr.attr, > + &sensor_dev_attr_power1_good.dev_attr.attr, > + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, > + &sensor_dev_attr_fet_short_fault.dev_attr.attr, > + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr, > + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr, > + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr, > + &sensor_dev_attr_power1_fault_log.dev_attr.attr, > + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr, > + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr, > + NULL, No comma for the terminator line. > +}; ... > + msleep(3200); Not a single letter to comment such a huge delay :-( -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 16:50 ` Andy Shevchenko @ 2023-11-13 10:13 ` Nuno Sá 2023-11-13 16:31 ` Andy Shevchenko 2023-11-20 15:00 ` Guenter Roeck 0 siblings, 2 replies; 17+ messages in thread From: Nuno Sá @ 2023-11-13 10:13 UTC (permalink / raw) To: Andy Shevchenko, Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Jean Delvare, Conor Dooley On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote: > On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: > > The LTC4282 hot swap controller allows a board to be safely inserted and > > removed from a live backplane. Using one or more external N-channel pass > > transistors, board supply voltage and inrush current are ramped up at an > > adjustable rate. An I2C interface and onboard ADC allows for monitoring > > of board current, voltage, power, energy and fault status. > > There are plenty of ltc42xx in the same folder and a lot of other chips. > Have you checked that none of the existing driver is not close enough and > brand new driver is indeed required? If so, add a respective paragraph > somewhere (cover letter? TBH I haven't read it, it's fine if it's already > explained there). > I can double check but not the last time I looked into those drivers > ... > > > +#define LTC4282_STATUS_MSB 0x1F > > > +#define LTC4282_VSOURCE 0x3a > > +#define LTC4282_VSOURCE_LOWEST 0x3c > > +#define LTC4282_VSOURCE_HIGHEST 0x3e > > You have an inconsistency in the capitalization of the hexadecimal values. > right... > ... > > > +/* > > + * relaxed version of FIELD_PREP() to be used when mask is not a compile > > time constant > > + * u32_encode_bits() can't also be used as the compiler needs to be able to > > evaluate > > + * mask at compile time. > > + */ > > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > > Can we name it accordingly as done in other places, and TBH it's a time to > move > it to the header. (At least I know about two more implementations of this). > Not sure what you mean? Is there some other drivers doing it already? I'll, anyways, wait on more feedback for the GPIO stuff because we might end up not needing it... > ... > > > +struct ltc4282_state { > > + struct regmap *map; > > + struct device *dev; > > + /* Protect against multiple accesses to the device registers */ > > + struct mutex lock; > > + struct gpio_chip gc; > > + u64 saved_energy; > > + long power_max; > > > + u32 gpio_map[LTC4282_GPIO_NR]; > > Why is this needed? The valid_mask is not enough? > Hmm I just looked at the valid_mask. So, I can definitely make it work with it. The only thing I see is that we might end up wasting memory in case we are not using the 4 available pins. Worst case, we allocate 3 'struct gpio_desc' that will never be used but I guess that's not a real issue... So yeah, I'll use the mask stuff. > > + u32 rsense; > > + u32 vin_mode; > > + u16 vfs_out; > > +}; > > ... > > > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull > > down the line */ > > + if (!gpio->active_high) > > Hmm... Why do you need a separate flag for this? Shouldn't be described or > autodetected somehow? Well, if a consumer as an active high gpio, it expects to call gpiod_set_value(..., 1) and the line to assert, right? To have that, we need to write 0 on the device register for some of the pins. And the same story is true for active low. gpiod_set_value(..., 0) will have the gpiolib to automatically invert the value and we get 1 in the callback. > > > + val = !val; > > ... > > > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); > > I'm wondering if you can do some trick to "divide" actually to 2^16 so, it > will > not use division op at all? Hmm, not sure if it will be obvious but you mean something like: *val = (be16_to_cpu(in) * (u64)fs) >> 16; Is this what you mean? If so, we`ll loose the "CLOSEST" handling... Not so sure if we need to be "that" performant in such a code path. But Guenter can also share his opinion... > > ... > > > + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); > > Ditto. > > ... > > > + /* > > + * Wait for two ADC conversions so we are sure we get one full VDD > > + * measurement. > > + */ > > + msleep(2 * LTC4282_TCONV_US / MILLI); > > fsleep() ? > Could be but this is going away anyways... > ... > > > +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, > > long *val) > > +{ > > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > > Too many spaces. > > "* BIT(16)" / "* BIT_ULL(16)" ? Well, I can just place the number as in the formula. Not too keen on the BIT() macros as this is not really a mask. > > ... > > > + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); > > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * > > (u64)st->rsense); > > > + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * > > int_pow(U16_MAX, 2)); > > Same Q about possible optimizations. > > ... > > > + temp = power * 40 * DECA * st->vfs_out * 256ULL; > > BIT_ULL(8) ? Same, not a mask so I would prefer the plain 256. No strong feeling though... > > ... > > > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * > > st->rsense); > > Same Q about possible optimizations. > > ... > > > + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, > > &temp)) { > > + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, > > U16_MAX); > > + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); > > + return ret; > > + } > > BIT(8) ? > > ... > > > + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); > > Same Q about possible optimizations. > > ... > > > + /* > > + * reset happened... let's read the new energy value that > > Reset > Let's > > > + * together with the new tick counter should give a sane > > average > > + * value. Furthermore, we save whatever value we had > > accumulated > > + * so that the next energy read will have it into account. > > + */ > > ... > > > + /* give some time for accumulation... */ > > + msleep(2 * LTC4282_TCONV_US / MILLI); > > fsleep() ? > > ... > > > + /* > > + * AVG = E / (Tconv * counter) > > + * We get energy in microJoule, hence dividing it by microSeconds > > gives Watts. Therefore, > > + * multiplying by MICRO gives us microWatts. > > + */ > > Here you used long lines, somewhere else, much shorter. Please, choose one > limit and be consistent with it. > Yeah, maybe not too consistent. I'm still not sure what's the convention in hwmon (I mention it in the cover). > ... > > > + temp = val * int_pow(U8_MAX, 2) * st->rsense; > > + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st- > > >vfs_out * 40); > > As per above. > > ... > > > + if (val >= fs) > > + in = U8_MAX; > > + else > > + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); > > Can clamp() be used here? > Yes, I guess it can... > ... > > > + /* > > + * Make sure vdd is stable. From the datasheet: > > + * The state of the UV and OV comparators is indicated by the > > STATUS register > > + * bits 0 and 1 and must be stable for at least 50ms to qualify for > > turn-on. > > + */ > > + do { > > + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, > > reg, > > + reg & > > LTC4282_VDD_STATUS_MASK, 10000, 50000); > > + if (!ret) > > + continue; > > + if (ret != -ETIMEDOUT) > > + return dev_err_probe(st->dev, ret, "Failed regmap > > read\n"); > > I would do it a bit differently. > > > + break; > > > + } while (n_tries--); > > } while (--n_tries); ? > > Altogether: > > do { > ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, > reg, > reg & LTC4282_VDD_STATUS_MASK, > 10000, 50000); > /* Check if we got timeout which means UV and OV are stable > for 50ms */ > if (ret == -ETIMEDOUT) > break; > if (ret) > return dev_err_probe(st->dev, ret, "Failed regmap > read\n"); > } while (--n_tries); > > being dropped... > ... > > > + if (val > LTC4282_CLKOUT_TICK) > > + return dev_err_probe(st->dev, -EINVAL, > > + "Invalid val(%u) for adi,clkout- > > mode\n", val); > > ERANGE? Yeah, probably more appropriate. > > ... > > > + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX) > > in_range() ? sure... > > > + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin > > range(%lu) [%lu %lu]\n", > > + rate, LTC4282_CLKIN_MIN, > > LTC4282_CLKIN_MAX); > > ... > > > + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { > > Is 0 also being defined? > Hmm, I guess ARRAY_SIZE(ltc4282_gpio_prop) is better. Not sure why I did not used it... > > + ret = device_property_read_u32(dev, > > ltc4282_gpio_prop[gpio], &func); > > + if (ret) > > + continue; > > + if (func >= ltc4282_gpios[gpio].n_funcs) > > + return dev_err_probe(dev, ret, "Invalid func(%u >= > > %u) for gpio%u\n", > > + func, > > ltc4282_gpios[gpio].n_funcs, gpio + 1); > > + if (func == LTC4282_PIN_GPIO) { > > + st->gpio_map[ngpios++] = gpio; > > + if (gpio == LTC4282_GPIO_1) { > > + /* default to input GPIO */ > > + ret = regmap_set_bits(st->map, > > LTC4282_GPIO_CONFIG, > > + > > LTC4282_GPIO_1_CONFIG_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + continue; > > + } > > ... > > > + switch (val) { > > + case 3300000: > > Hmm... Do we have MICROVOLTS_IN_VOLTS or so? > Not sure, can look for it... > > + st->vin_mode = LTC4282_VIN_3_3V; > > + break; > > + case 5000000: > > + st->vin_mode = LTC4282_VIN_5V; > > + break; > > + case 12000000: > > + st->vin_mode = LTC4282_VIN_12V; > > + break; > > + case 24000000: > > + st->vin_mode = LTC4282_VIN_24V; > > + break; > > + default: > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid val(%u) for vin-mode- > > microvolt\n", val); > > + } > > ... > > > +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_POWER_BAD_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_FET_BAD_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_FET_SHORT_FAULT_MASK); > > SENSOR_DEVICE_ATTR_RO() / _RW() ? will check.. > > ... > > > +static struct attribute *ltc4282_attrs[] = { > > + &sensor_dev_attr_energy1_input.dev_attr.attr, > > + &sensor_dev_attr_power1_good.dev_attr.attr, > > + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, > > + &sensor_dev_attr_fet_short_fault.dev_attr.attr, > > + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_power1_fault_log.dev_attr.attr, > > + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr, > > + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr, > > + NULL, > > No comma for the terminator line. > sure. > > +}; > > ... > > > + msleep(3200); > > Not a single letter to comment such a huge delay :-( > Well, it's after doing a reset so it should be pretty obvious is the number given in the DS. But I'll put a comment on it. - Nuno Sá ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-13 10:13 ` Nuno Sá @ 2023-11-13 16:31 ` Andy Shevchenko 2023-11-14 8:36 ` Nuno Sá 2023-11-20 15:00 ` Guenter Roeck 1 sibling, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2023-11-13 16:31 UTC (permalink / raw) To: Nuno Sá Cc: Nuno Sa, linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Jean Delvare, Conor Dooley On Mon, Nov 13, 2023 at 11:13:44AM +0100, Nuno Sá wrote: > On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote: > > On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: ... > > > +/* > > > + * relaxed version of FIELD_PREP() to be used when mask is not a compile > > > time constant > > > + * u32_encode_bits() can't also be used as the compiler needs to be able to > > > evaluate > > > + * mask at compile time. > > > + */ > > > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > > > > Can we name it accordingly as done in other places, and TBH it's a time to > > move > > it to the header. (At least I know about two more implementations of this). > > Not sure what you mean? Is there some other drivers doing it already? I'll, > anyways, wait on more feedback for the GPIO stuff because we might end up not > needing it... $ git grep -n 'define field_prep' ... > > > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull > > > down the line */ > > > + if (!gpio->active_high) > > > > Hmm... Why do you need a separate flag for this? Shouldn't be described or > > autodetected somehow? > > Well, if a consumer as an active high gpio, it expects to call > gpiod_set_value(..., 1) and the line to assert, right? To have that, we need to > write 0 on the device register for some of the pins. It doesn't matter, the GPIO (not _raw) APIs are using logical levels, 1 — activate, 0 — deactivate. > And the same story is true for active low. gpiod_set_value(..., 0) will have the > gpiolib to automatically invert the value and we get 1 in the callback. Yes, but why do you have that flag in the structure? > > > + val = !val; ... > > > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); > > > > I'm wondering if you can do some trick to "divide" actually to 2^16 so, it > > will > > not use division op at all? > > Hmm, not sure if it will be obvious but you mean something like: > > *val = (be16_to_cpu(in) * (u64)fs) >> 16; > > Is this what you mean? If so, we`ll loose the "CLOSEST" handling... Not so sure > if we need to be "that" performant in such a code path. But Guenter can also > share his opinion... *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs + (BIT(16) - 1), BIT(16)); will give the same result without division, no? What you need is to make sure that the multiplication won't get closer to U64_MAX, which seems not the case here (max 48-bit number). Ditto for all other similar cases which I already pointed out. ... > > > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > > > > "* BIT(16)" / "* BIT_ULL(16)" ? > > Well, I can just place the number as in the formula. Not too keen on the BIT() > macros as this is not really a mask. I'm not sure I got this. The << 16 neither a plain number and BIT() is equally good. With power of two it's most likely that this is due to internal implementation of the firmware or hardware, so again BIT() can be still good enough to show that. ... > > > + msleep(3200); > > > > Not a single letter to comment such a huge delay :-( > > Well, it's after doing a reset so it should be pretty obvious is the number > given in the DS. But I'll put a comment on it. Please do. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-13 16:31 ` Andy Shevchenko @ 2023-11-14 8:36 ` Nuno Sá 0 siblings, 0 replies; 17+ messages in thread From: Nuno Sá @ 2023-11-14 8:36 UTC (permalink / raw) To: Andy Shevchenko Cc: Nuno Sa, linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Rob Herring, Jean Delvare, Conor Dooley On Mon, 2023-11-13 at 18:31 +0200, Andy Shevchenko wrote: > On Mon, Nov 13, 2023 at 11:13:44AM +0100, Nuno Sá wrote: > > On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote: > > > On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: > > ... > > > > > +/* > > > > + * relaxed version of FIELD_PREP() to be used when mask is not a > > > > compile > > > > time constant > > > > + * u32_encode_bits() can't also be used as the compiler needs to be > > > > able to > > > > evaluate > > > > + * mask at compile time. > > > > + */ > > > > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > > > > > > Can we name it accordingly as done in other places, and TBH it's a time to > > > move > > > it to the header. (At least I know about two more implementations of > > > this). > > > > Not sure what you mean? Is there some other drivers doing it already? I'll, > > anyways, wait on more feedback for the GPIO stuff because we might end up > > not > > needing it... > > $ git grep -n 'define field_prep' > > ... > > > > > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to > > > > pull > > > > down the line */ > > > > + if (!gpio->active_high) > > > > > > Hmm... Why do you need a separate flag for this? Shouldn't be described or > > > autodetected somehow? > > > > Well, if a consumer as an active high gpio, it expects to call > > gpiod_set_value(..., 1) and the line to assert, right? To have that, we need > > to > > write 0 on the device register for some of the pins. > > It doesn't matter, the GPIO (not _raw) APIs are using logical levels, 1 — > activate, > 0 — deactivate. > > > And the same story is true for active low. gpiod_set_value(..., 0) will have > > the > > gpiolib to automatically invert the value and we get 1 in the callback. > > Yes, but why do you have that flag in the structure? Because one of the pins (GPIO_1) has the opposite behavior... > > > > > + val = !val; > > ... > > > > > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, > > > > U16_MAX); > > > > > > I'm wondering if you can do some trick to "divide" actually to 2^16 so, it > > > will > > > not use division op at all? > > > > Hmm, not sure if it will be obvious but you mean something like: > > > > *val = (be16_to_cpu(in) * (u64)fs) >> 16; > > > > Is this what you mean? If so, we`ll loose the "CLOSEST" handling... Not so > > sure > > if we need to be "that" performant in such a code path. But Guenter can also > > share his opinion... > > *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs + (BIT(16) - > 1), BIT(16)); > > will give the same result without division, no? > What you need is to make sure that the multiplication won't get closer to > U64_MAX, which seems not the case here (max 48-bit number). Hmm, I must be missing something but you're still using DIV_ROUND_CLOSEST_ULL(). So, I guess you're rely on some formula optimization that removes the division (I'm honestly seeing it) but the result won't be exactly the same (off by 1). Again, this is not a fast path (AFAIK) and this is a typical formula to get a value from an ADC so I'm not sure making any super "smart" tricks to make this run faster beats readability. But, I'm still not seeing what you mean so I might change my mind... > > Ditto for all other similar cases which I already pointed out. > > ... > > > > > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > > > > > > > "* BIT(16)" / "* BIT_ULL(16)" ? > > > > Well, I can just place the number as in the formula. Not too keen on the > > BIT() > > macros as this is not really a mask. > > I'm not sure I got this. The << 16 neither a plain number and BIT() is equally Well, I do agree with << 16 part... > good. With power of two it's most likely that this is due to internal > implementation of the firmware or hardware, so again BIT() can be still good > enough to show that. > I'm still not convinced honestly... I see plain numbers to be a good fit and they match exactly with the DS. I just see things like BIT(), GENMASK, BITMAP and the likes to be used on masks. But I don't really care so unless Guenter has some opinion I can make as you suggest... - Nuno Sá ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-13 10:13 ` Nuno Sá 2023-11-13 16:31 ` Andy Shevchenko @ 2023-11-20 15:00 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2023-11-20 15:00 UTC (permalink / raw) To: Nuno Sá, Andy Shevchenko, Nuno Sa Cc: linux-hwmon, linux-doc, devicetree, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Rob Herring, Jean Delvare, Conor Dooley On 11/13/23 02:13, Nuno Sá wrote: > On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote: >> On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote: >>> The LTC4282 hot swap controller allows a board to be safely inserted and >>> removed from a live backplane. Using one or more external N-channel pass >>> transistors, board supply voltage and inrush current are ramped up at an >>> adjustable rate. An I2C interface and onboard ADC allows for monitoring >>> of board current, voltage, power, energy and fault status. >> >> There are plenty of ltc42xx in the same folder and a lot of other chips. >> Have you checked that none of the existing driver is not close enough and >> brand new driver is indeed required? If so, add a respective paragraph >> somewhere (cover letter? TBH I haven't read it, it's fine if it's already >> explained there). >> > > I can double check but not the last time I looked into those drivers They are all different. >> >>> + /* >>> + * AVG = E / (Tconv * counter) >>> + * We get energy in microJoule, hence dividing it by microSeconds >>> gives Watts. Therefore, >>> + * multiplying by MICRO gives us microWatts. >>> + */ >> >> Here you used long lines, somewhere else, much shorter. Please, choose one >> limit and be consistent with it. >> > > Yeah, maybe not too consistent. I'm still not sure what's the convention in > hwmon (I mention it in the cover). > Patches should pass "checkpatch --strict". Other than that, I'd expect consistency and common sense, but that isn't exactly spelled out anywhere and would be difficult to define. Personally, I would suggest something like "use limit of 80 columns for comments and otherwise try to stick with 80 columns unless it results in odd line breaks. In that case, feel free to go up to 100 columns". Do I need to spell that out in Documentation/hwmon/submitting-patches.rst ? Not that anyone seems to read that document :-(. Sorry, frustrated maintainer speaking. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa 2023-11-10 16:50 ` Andy Shevchenko @ 2023-11-11 1:04 ` kernel test robot 2023-11-11 17:22 ` Guenter Roeck 2023-11-20 12:10 ` Dan Carpenter 3 siblings, 0 replies; 17+ messages in thread From: kernel test robot @ 2023-11-11 1:04 UTC (permalink / raw) To: Nuno Sa, linux-hwmon, linux-doc, devicetree Cc: oe-kbuild-all, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Nuno Sa, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley Hi Nuno, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on linus/master v6.6 next-20231110] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa/dt-bindings-hwmon-Add-LTC4282-bindings/20231110-232017 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231110151905.1659873-3-nuno.sa%40analog.com patch subject: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip reproduce: (https://download.01.org/0day-ci/archive/20231111/202311110850.BINBPMG5-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311110850.BINBPMG5-lkp@intel.com/ All warnings (new ones prefixed by >>): >> Documentation/hwmon/ltc4282.rst: WARNING: document isn't included in any toctree -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa 2023-11-10 16:50 ` Andy Shevchenko 2023-11-11 1:04 ` kernel test robot @ 2023-11-11 17:22 ` Guenter Roeck 2023-11-13 9:24 ` Nuno Sá 2023-11-20 12:10 ` Dan Carpenter 3 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2023-11-11 17:22 UTC (permalink / raw) To: Nuno Sa, linux-hwmon, linux-doc, devicetree Cc: Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley On 11/10/23 07:18, Nuno Sa wrote: > The LTC4282 hot swap controller allows a board to be safely inserted and > removed from a live backplane. Using one or more external N-channel pass > transistors, board supply voltage and inrush current are ramped up at an > adjustable rate. An I2C interface and onboard ADC allows for monitoring > of board current, voltage, power, energy and fault status. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > Documentation/hwmon/ltc4282.rst | 101 ++ As reported by 0-day, this must be added to index.rst. Anyway, partial review only. Also, since it registers a gpio driver, it will need to be reviewed by a gpio maintainer. I am not comfortable with some of the gpio code. > MAINTAINERS | 2 + > drivers/hwmon/Kconfig | 11 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/ltc4282.c | 1518 +++++++++++++++++++++++++++++++ > 5 files changed, 1633 insertions(+) > create mode 100644 Documentation/hwmon/ltc4282.rst > create mode 100644 drivers/hwmon/ltc4282.c > > diff --git a/Documentation/hwmon/ltc4282.rst b/Documentation/hwmon/ltc4282.rst > new file mode 100644 > index 000000000000..2fab5d165aa4 > --- /dev/null > +++ b/Documentation/hwmon/ltc4282.rst > @@ -0,0 +1,101 @@ > +Kernel drivers ltc4282 > +========================================== > + > +Supported chips: > + > + * Analog Devices LTC4282 > + > + Prefix: 'ltc4282' > + > + Addresses scanned: - I2C 0x40 - 0x5A (7-bit) > + Addresses scanned: - I2C 0x80 - 0xB4 with a step of 2 (8-bit) > + > + Datasheet: > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf > + > +Author: Nuno Sá <nuno.sa@analog.com> > + > +Description > +___________ > + > +The LTC4282 hot swap controller allows a board to be safely inserted and removed > +from a live backplane. Using one or more external N-channel pass transistors, > +board supply voltage and inrush current are ramped up at an adjustable rate. An > +I2C interface and onboard ADC allows for monitoring of board current, voltage, > +power, energy and fault status. The device features analog foldback current > +limiting and supply monitoring for applications from 2.9V to 33V. Dual 12V gate > +drive allows high power applications to either share safe operating area across > +parallel MOSFETs or support a 2-stage start-up that first charges the load > +capacitance followed by enabling a low on-resistance path to the load. The > +LTC4282 is well suited to high power applications because the precise monitoring > +capability and accurate current limiting reduce the extremes in which both loads > +and power supplies must safely operate. Non-volatile configuration allows for > +flexibility in the autonomous generation of alerts and response to faults. > + > +Sysfs entries > +_____________ > + > +The following attributes are supported. Limits are read-write and all the other > +attributes are read-only. > + > +======================= ========================================== > +in0_input Output voltage (mV). > +in0_min Undervoltage threshold > +in0_max Overvoltage threshold > +in0_lowest Lowest measured voltage > +in0_highest Highest measured voltage > +in0_min_alarm Undervoltage alarm > +in0_max_alarm Overvoltage alarm > +in0_label Channel label (VSOURCE) > + > +in1_input Input voltage (mV). > +in1_lcrit_alarm Critical Undervoltage alarm > +in1_lcrit_fault_log Undervoltage Fault Log. In some setups, clearing this > + fault (writing on the file) might be needed. > +in1_crit_alarm Critical Overvoltage alarm > +in1_crit_fault_log Overvoltage Fault Log. In some setups, clearing this > + fault (writing on the file) might be needed > +in1_label Channel label (VDD) > + > +in2_input GPIO voltage (mV) > +in2_min Undervoltage threshold > +in2_max Overvoltage threshold > +in2_lowest Lowest measured voltage > +in2_highest Highest measured voltage > +in2_min_alarm Undervoltage alarm > +in2_max_alarm Overvoltage alarm > +in2_label Channel label (VGPIO) > + > +curr1_input Sense current (mA) > +curr1_min Undercurrent threshold > +curr1_max Overcurrent threshold > +curr1_lowest Lowest measured current > +curr1_highest Highest measured current > +curr1_min_alarm Undercurrent alarm > +curr1_max_alarm Overcurrent alarm > +curr1_crit_alarm Critical Overcurrent alarm > +curr1_crit_fault_log Overcurrent Fault Log. In some setups, clearing this > + fault (writing on the file) might be needed > +curr1_label Channel label (ISENSE) > + > +power1_input Power (in uW) > +power1_min Low power threshold > +power1_max High power threshold > +power1_average Average Power use > +power1_input_lowest Historical minimum power use > +power1_input_highest Historical maximum power use > +power1_min_alarm Low power alarm > +power1_max_alarm High power alarm > +power1_good Power considered good > +power1_fault_log Power bad Fault log > +power1_label Channel label (Power) > + > +energy1_input Measured energy over time (in microJoule) > +energy1_enable Enable/Disable Energy accumulation > + > +fet_short_fault FET short alarm > +fet_short_fault_log FET short Fault log > +fet_bad_fault FET bad alarm > +fet_bad_fault_log FET bad Fault log > +======================= ========================================== > diff --git a/MAINTAINERS b/MAINTAINERS > index 9f9527f6057b..ba7f0439d8e8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12486,6 +12486,8 @@ M: Nuno Sa <nuno.sa@analog.com> > L: linux-hwmon@vger.kernel.org > S: Supported > F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > +F: Documentation/hwmon/ltc4282.rst > +F: drivers/hwmon/ltc4282.c > > LTC4306 I2C MULTIPLEXER DRIVER > M: Michael Hennerich <michael.hennerich@analog.com> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index cf27523eed5a..2f1a0cf2e1e4 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1027,6 +1027,17 @@ config SENSORS_LTC4261 > This driver can also be built as a module. If so, the module will > be called ltc4261. > > +config SENSORS_LTC4282 > + tristate "Analog Devices LTC4282" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get support for Analog Devices LTC4282 > + High Current Hot Swap Controller I2C interface. > + > + This driver can also be built as a module. If so, the module will > + be called ltc4282. > + > config SENSORS_LTQ_CPUTEMP > bool "Lantiq cpu temperature sensor driver" > depends on SOC_XWAY > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index e84bd9685b5c..cbf22d1d736f 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_LTC4222) += ltc4222.o > obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o > obj-$(CONFIG_SENSORS_LTC4260) += ltc4260.o > obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o > +obj-$(CONFIG_SENSORS_LTC4282) += ltc4282.o > obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o > obj-$(CONFIG_SENSORS_MAX1111) += max1111.o > obj-$(CONFIG_SENSORS_MAX127) += max127.o > diff --git a/drivers/hwmon/ltc4282.c b/drivers/hwmon/ltc4282.c > new file mode 100644 > index 000000000000..738312cf212e > --- /dev/null > +++ b/drivers/hwmon/ltc4282.c > @@ -0,0 +1,1518 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C > + * > + * Copyright 2023 Analog Devices Inc. > + */ > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/cleanup.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/gpio/driver.h> > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/i2c.h> > +#include <linux/math.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/mutex.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/property.h> > +#include <linux/units.h> > + > +#define LTC4282_CTRL_LSB 0x00 > + #define LTC4282_CTRL_OV_RETRY_MASK BIT(0) > + #define LTC4282_CTRL_UV_RETRY_MASK BIT(1) > + #define LTC4282_CTRL_OC_RETRY_MASK BIT(2) > + #define LTC4282_CTRL_ON_ACTIVE_LOW_MASK BIT(5) > + #define LTC4282_CTRL_ON_DELAY_MASK BIT(6) > +#define LTC4282_CTRL_MSB 0x01 > + #define LTC4282_CTRL_VIN_MODE_MASK GENMASK(1, 0) > + #define LTC4282_CTRL_OV_MODE_MASK GENMASK(3, 2) > + #define LTC4282_CTRL_UV_MODE_MASK GENMASK(5, 4) > +#define LTC4282_FAULT_LOG 0x04 > + #define LTC4282_OV_FAULT_MASK BIT(0) > + #define LTC4282_UV_FAULT_MASK BIT(1) > + #define LTC4282_OC_FAULT_MASK BIT(2) > + #define LTC4282_POWER_BAD_FAULT_MASK BIT(3) > + #define LTC4282_FET_SHORT_FAULT_MASK BIT(5) > + #define LTC4282_FET_BAD_FAULT_MASK BIT(6) > +#define LTC4282_ADC_ALERT_LOG 0x05 > + #define LTC4282_GPIO_ALARM_L_MASK BIT(0) > + #define LTC4282_GPIO_ALARM_H_MASK BIT(1) > + #define LTC4282_VSOURCE_ALARM_L_MASK BIT(2) > + #define LTC4282_VSOURCE_ALARM_H_MASK BIT(3) > + #define LTC4282_VSENSE_ALARM_L_MASK BIT(4) > + #define LTC4282_VSENSE_ALARM_H_MASK BIT(5) > + #define LTC4282_POWER_ALARM_L_MASK BIT(6) > + #define LTC4282_POWER_ALARM_H_MASK BIT(7) > +#define LTC4282_FET_BAD_FAULT_TIMEOUT 0x06 > + #define LTC4282_FET_BAD_MAX_TIMEOUT 255 > +#define LTC4282_GPIO_CONFIG 0x07 > + #define LTC4282_GPIO_2_FET_STRESS_MASK BIT(1) > + #define LTC4282_GPIO_1_OUT_MASK BIT(3) > + #define LTC4282_GPIO_1_CONFIG_MASK GENMASK(5, 4) > + #define LTC4282_GPIO_2_OUT_MASK BIT(6) > + #define LTC4282_GPIO_3_OUT_MASK BIT(7) > +#define LTC4282_VGPIO_MIN 0x08 > +#define LTC4282_VGPIO_MAX 0x09 > +#define LTC4282_VSOURCE_MIN 0x0a > +#define LTC4282_VSOURCE_MAX 0x0b > +#define LTC4282_VSENSE_MIN 0x0c > +#define LTC4282_VSENSE_MAX 0x0d > +#define LTC4282_POWER_MIN 0x0e > +#define LTC4282_POWER_MAX 0x0f > +#define LTC4282_CLK_DIV 0x10 > + #define LTC4282_CLK_DIV_MASK GENMASK(4, 0) > + #define LTC4282_CLKOUT_MASK GENMASK(6, 5) > +#define LTC4282_ILIM_ADJUST 0x11 > + #define LTC4282_GPIO_MODE_MASK BIT(1) > + #define LTC4282_VDD_MONITOR_MASK BIT(2) > + #define LTC4282_FOLDBACK_MODE_MASK GENMASK(4, 3) > + #define LTC4282_ILIM_ADJUST_MASK GENMASK(7, 5) > +#define LTC4282_ENERGY 0x12 > +#define LTC4282_TIME_COUNTER 0x18 > +#define LTC4282_ALERT_CTRL 0x1C > + #define LTC4282_ALERT_OUT_MASK BIT(6) > +#define LTC4282_ADC_CTRL 0x1D > + #define LTC4282_FAULT_LOG_EN_MASK BIT(2) > + #define LTC4282_METER_HALT_MASK BIT(5) > + #define LTC4282_METER_RESET_MASK BIT(6) > + #define LTC4282_RESET_MASK BIT(7) > +#define LTC4282_STATUS_LSB 0x1E > + #define LTC4282_OV_STATUS_MASK BIT(0) > + #define LTC4282_UV_STATUS_MASK BIT(1) > + #define LTC4282_VDD_STATUS_MASK (LTC4282_OV_STATUS_MASK | LTC4282_UV_STATUS_MASK) > + #define LTC4282_OC_STATUS_MASK BIT(2) > + #define LTC4282_POWER_GOOD_MASK BIT(3) > + #define LTC4282_FET_SHORT_MASK BIT(5) > + #define LTC4282_FET_BAD_STATUS_MASK BIT(6) > +#define LTC4282_STATUS_MSB 0x1F > + #define LTC4282_METER_TICK_OVERFLOW_MASK GENMASK(1, 0) > + #define LTC4282_ALERT_STATUS_MASK BIT(4) > + #define LTC4282_GPIO_1_STATUS_MASK BIT(5) > + #define LTC4282_GPIO_2_STATUS_MASK BIT(6) > + #define LTC4282_GPIO_3_STATUS_MASK BIT(7) > +#define LTC4282_RESERVED_1 0x32 > +#define LTC4282_RESERVED_2 0x33 > +#define LTC4282_VGPIO 0x34 > +#define LTC4282_VGPIO_LOWEST 0x36 > +#define LTC4282_VGPIO_HIGHEST 0x38 > +#define LTC4282_VSOURCE 0x3a > +#define LTC4282_VSOURCE_LOWEST 0x3c > +#define LTC4282_VSOURCE_HIGHEST 0x3e > +#define LTC4282_VSENSE 0x40 > +#define LTC4282_VSENSE_LOWEST 0x42 > +#define LTC4282_VSENSE_HIGHEST 0x44 > +#define LTC4282_POWER 0x46 > +#define LTC4282_POWER_LOWEST 0x48 > +#define LTC4282_POWER_HIGHEST 0x4a > +#define LTC4282_RESERVED_3 0x50 > + > +#define LTC4282_CLKIN_MIN (250 * KILO) > +#define LTC4282_CLKIN_MAX (15500 * KILO) > +/* this assumes 12bit ADC */ > +#define LTC4282_TCONV_US 65535 > +#define LTC4282_GPIO_NR 4 > +/* > + * relaxed version of FIELD_PREP() to be used when mask is not a compile time constant > + * u32_encode_bits() can't also be used as the compiler needs to be able to evaluate > + * mask at compile time. > + */ > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > + > +struct ltc4282_state { > + struct regmap *map; > + struct device *dev; This is only used for an inappropriate warning message and during probe. Pass it to initialization functions where needed. > + /* Protect against multiple accesses to the device registers */ > + struct mutex lock; > + struct gpio_chip gc; > + u64 saved_energy; > + long power_max; I do not understand how this is used in the code. It seems to avoid overflows due to unreasonable values of rsense. That adds a lot of complexist to the code and makes it hard to understand. > + u32 gpio_map[LTC4282_GPIO_NR]; > + u32 rsense; > + u32 vin_mode; > + u16 vfs_out; > +}; > + > +struct ltc4282_gpio { > + u32 out_reg; > + u32 out_mask; > + u32 in_reg; > + u32 in_mask; > + bool active_high; > + u8 n_funcs; > +}; > + > +enum { > + LTC4282_VIN_3_3V, > + LTC4282_VIN_5V, > + LTC4282_VIN_12V, > + LTC4282_VIN_24V, > +}; > + > +enum { > + LTC4282_CHAN_VSOURCE, > + LTC4282_CHAN_VDD, > + LTC4282_CHAN_VGPIO, > +}; > + > +enum { > + LTC4282_GPIO_1, > + LTC4282_GPIO_2, > + LTC4282_GPIO_3, > + LTC4282_ALERT, > +}; > + > +static const struct ltc4282_gpio ltc4282_gpios[] = { > + [LTC4282_GPIO_1] = { > + .in_reg = LTC4282_STATUS_MSB, > + .in_mask = LTC4282_GPIO_1_STATUS_MASK, > + .out_reg = LTC4282_GPIO_CONFIG, > + .out_mask = LTC4282_GPIO_1_OUT_MASK, > + .active_high = true, > + .n_funcs = 3, > + }, > + [LTC4282_GPIO_2] = { > + .in_reg = LTC4282_STATUS_MSB, > + .in_mask = LTC4282_GPIO_2_STATUS_MASK, > + .out_reg = LTC4282_GPIO_CONFIG, > + .out_mask = LTC4282_GPIO_2_OUT_MASK, > + .n_funcs = 3, > + }, > + [LTC4282_GPIO_3] = { > + .in_reg = LTC4282_STATUS_MSB, > + .in_mask = LTC4282_GPIO_3_STATUS_MASK, > + .out_reg = LTC4282_GPIO_CONFIG, > + .out_mask = LTC4282_GPIO_3_OUT_MASK, > + .n_funcs = 2, > + }, > + [LTC4282_ALERT] = { > + .in_reg = LTC4282_STATUS_MSB, > + .in_mask = LTC4282_ALERT_STATUS_MASK, > + .out_reg = LTC4282_ALERT_CTRL, > + .out_mask = LTC4282_ALERT_OUT_MASK, > + }, > +}; > + > +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int offset) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + u32 gpio_pin = st->gpio_map[offset]; > + > + /* we can only control this for GPIO_1 */ > + if (gpio_pin != LTC4282_GPIO_1) > + return 0; > + > + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK); > +} > + > +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int offset, int val) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + u32 gpio_pin = st->gpio_map[offset]; > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > + > + guard(mutex)(&st->lock); > + /* > + * Explicitly setting the pin as output can only be done for GPIO_1. For the > + * other pins we just pull the line down or high-z. > + */ > + if (gpio_pin == LTC4282_GPIO_1) { > + int ret; > + > + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK, > + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2)); > + if (ret) > + return ret; > + } > + > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down the line */ > + if (!gpio->active_high) > + val = !val; > + > + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > + LTC4282_FIELD_PREP(gpio->out_mask, val)); > +} > + > +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, int val) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + u32 gpio_pin = st->gpio_map[offset]; > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > + > + if (!gpio->active_high) > + val = !val; > + > + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > + LTC4282_FIELD_PREP(gpio->out_mask, val)); > +} > + > +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct ltc4282_state *st = gpiochip_get_data(chip); > + u32 gpio_pin = st->gpio_map[offset]; > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > + int ret; > + u32 val; > + > + ret = regmap_read(st->map, gpio->in_reg, &val); > + if (ret) > + return ret; > + > + return !!(val & gpio->in_mask); > +} > + > +static int ltc4282_read_voltage_word(const struct ltc4282_state *st, u32 reg, u32 fs, long *val) > +{ > + __be16 in; > + int ret; > + > + ret = regmap_bulk_read(st->map, reg, &in, sizeof(in)); > + if (ret) > + return ret; > + > + /* > + * This is also used to calculate current in which case fs comes in 10 * uV. > + * Hence the ULL usage. > + */ > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); > + return 0; > +} > + > +static int ltc4282_read_voltage_byte(const struct ltc4282_state *st, u32 reg, u32 fs, long *val) > +{ > + int ret; > + u32 in; > + > + ret = regmap_read(st->map, reg, &in); > + if (ret) > + return ret; > + > + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); > + return 0; > +} > + > +static int ltc4282_read_vdd(struct ltc4282_state *st, u32 fs, long *val) > +{ > + int ret; > + > + guard(mutex)(&st->lock); > + > + /* ADC now monitors VDD */ > + ret = regmap_clear_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_VDD_MONITOR_MASK); > + if (ret) > + return ret; > + > + /* > + * Wait for two ADC conversions so we are sure we get one full VDD > + * measurement. > + */ > + msleep(2 * LTC4282_TCONV_US / MILLI); > + > + ret = ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, val); > + if (ret) > + return ret; > + > + /* back to VSOURCE */ > + return regmap_set_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_VDD_MONITOR_MASK); Handle this with _enable attributes if the chip only reports one measurement. Sleeping 130+ ms for each reading is unacceptable. Besides, subsequent VSOURCE readings would have to be delayed as well. I'll have to read the datasheet in lore detail to understand how this translates into limits. I see that the chip has limit registers for the various voltages, which doesn't really make sense if the chip doesn't measure those voltages. > +} > + > +static int ltc4282_read_alarm(struct ltc4282_state *st, u32 reg, u32 mask, long *val) > +{ > + u32 alarm; > + int ret; > + > + guard(mutex)(&st->lock); > + > + /* if not status, clear first the alarm by clearing the bit */ > + if (reg != LTC4282_STATUS_LSB && reg != LTC4282_FAULT_LOG) { > + ret = regmap_clear_bits(st->map, reg, mask); > + if (ret) > + return ret; > + > + msleep(LTC4282_TCONV_US / MILLI); > + } Normally one would want to have historic alarms reported at least once, even if the alarm since went away. This code seems to clear alarm bits prior to reading them, which will result in under-reporting. This will need to be explained, especially since it results in a 65+ ms delay for each alarm report if alarms are active which seems excessive and unnceessary. > + > + ret = regmap_read(st->map, reg, &alarm); > + if (ret) > + return ret; > + > + *val = !!(alarm & mask); > + > + return 0; > +} > + > +static int ltc4282_read_in(struct device *dev, u32 attr, long *val, u32 channel) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_in_input: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, val); > + if (channel == LTC4282_CHAN_VDD) > + return ltc4282_read_vdd(st, st->vfs_out, val); > + > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO, 1280, val); > + case hwmon_in_highest: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE_HIGHEST, > + st->vfs_out, val); > + > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_HIGHEST, 1280, val); > + case hwmon_in_lowest: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_voltage_word(st, LTC4282_VSOURCE_LOWEST, > + st->vfs_out, val); > + > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_LOWEST, 1280, val); > + case hwmon_in_max_alarm: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > + LTC4282_VSOURCE_ALARM_H_MASK, val); > + > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_GPIO_ALARM_H_MASK, > + val); > + case hwmon_in_min_alarm: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > + LTC4282_VSOURCE_ALARM_L_MASK, val); > + > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_GPIO_ALARM_L_MASK, > + val); > + case hwmon_in_alarm: > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, > + LTC4282_FET_BAD_STATUS_MASK, val); > + case hwmon_in_crit_alarm: > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_OV_STATUS_MASK, val); > + case hwmon_in_lcrit_alarm: > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_UV_STATUS_MASK, val); > + case hwmon_in_max: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_voltage_byte(st, LTC4282_VSOURCE_MAX, > + st->vfs_out, val); > + > + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MAX, 1280, val); > + case hwmon_in_min: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_read_voltage_byte(st, LTC4282_VSOURCE_MIN, > + st->vfs_out, val); > + > + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MIN, 1280, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_read_current_word(const struct ltc4282_state *st, u32 reg, long *val) > +{ > + long in; > + int ret; > + > + /* > + * We pass in full scale in 10 * micro (note that 40 is already millivolt) so we > + * have better approximations to calculate current. > + */ > + ret = ltc4282_read_voltage_word(st, reg, DECA * 40 * MILLI, &in); > + if (ret) > + return ret; > + > + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); > + > + return 0; > +} > + > +static int ltc4282_read_current_byte(const struct ltc4282_state *st, u32 reg, long *val) > +{ > + long in; > + int ret; > + > + ret = ltc4282_read_voltage_byte(st, reg, DECA * 40 * MILLI, &in); > + if (ret) > + return ret; > + > + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); > + > + return 0; > +} > + > +static int ltc4282_read_curr(struct device *dev, const u32 attr, long *val) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_curr_input: > + return ltc4282_read_current_word(st, LTC4282_VSENSE, val); > + case hwmon_curr_highest: > + return ltc4282_read_current_word(st, LTC4282_VSENSE_HIGHEST, val); > + case hwmon_curr_lowest: > + return ltc4282_read_current_word(st, LTC4282_VSENSE_LOWEST, val); > + case hwmon_curr_max: > + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MAX, val); > + case hwmon_curr_min: > + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MIN, val); > + case hwmon_curr_max_alarm: > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_VSENSE_ALARM_H_MASK, > + val); > + case hwmon_curr_min_alarm: > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_VSENSE_ALARM_L_MASK, > + val); > + case hwmon_curr_crit_alarm: > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, LTC4282_OC_STATUS_MASK, val); > + default: > + return -ENOTSUPP; checkpatch: WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP Please run checkpatch --strict on your patches. > + } > +} > + > +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, long *val) > +{ > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > + __be16 raw; > + u16 power; > + int ret; > + > + ret = regmap_bulk_read(st->map, reg, &raw, sizeof(raw)); > + if (ret) > + return ret; > + > + power = be16_to_cpu(raw); > + /* > + * Power is given by: > + * P = CODE(16b) * 0.040 * Vfs(out) * 2^16 / ((2^16 - 1)^2 * Rsense) > + */ > + if (check_mul_overflow(power * temp, MICRO, &temp_2)) { > + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * (u64)st->rsense); > + return 0; > + } > + > + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * int_pow(U16_MAX, 2)); > + > + return 0; > +} > + > +static int ltc4282_read_power_byte(const struct ltc4282_state *st, u32 reg, long *val) > +{ > + u32 power; > + u64 temp; > + int ret; > + > + ret = regmap_read(st->map, reg, &power); > + if (ret) > + return ret; > + > + temp = power * 40 * DECA * st->vfs_out * 256ULL; > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * st->rsense); > + > + return 0; > +} > + > +static int ltc4282_read_energy(const struct ltc4282_state *st, u64 *val) > +{ > + u64 temp, energy; > + u32 status; > + __be64 raw; > + int ret; > + > + ret = regmap_bulk_read(st->map, LTC4282_ENERGY, &raw, 6); > + if (ret) > + return ret; > + > + ret = regmap_read(st->map, LTC4282_STATUS_MSB, &status); > + if (ret) > + return ret; > + > + if (status & LTC4282_METER_TICK_OVERFLOW_MASK) { > + dev_warn(st->dev, "Got overflow in meter/ticker counters\n"); This is a big no-go. A normal operation must not generate any messages, much less warnings. > + /* > + * This resets the meter and the tick counter and holds them > + * reset. > + */ > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, > + LTC4282_METER_RESET_MASK); > + if (ret) > + return ret; > + > + /* start accumulating again */ > + ret = regmap_clear_bits(st->map, LTC4282_ADC_CTRL, > + LTC4282_METER_RESET_MASK); > + if (ret) > + return ret; > + > + /* > + * Let the callers know a reset happened. Important when calling > + * from power1_average. > + */ Only I won't accept in-kernel calculated power average value attributes. If the chip does not report averages, they must be calculated in userspace. > + ret = 1; > + } > + > + energy = be64_to_cpu(raw) >> 16; > + /* > + * The formula for energy is given by: > + * E = CODE(48b) * 0.040 * Vfs(out) * Tconv * 256 / ((2^16 - 1)^2 * Rsense) > + * > + * Since we only support 12bit ADC, Tconv = 0.065535s. Passing Vfs(out) and 0.040 to > + * mV and Tconv to us, we can simplify the formula to: > + * E = CODE(48b) * 40 * Vfs(out) * 256 / (U16_MAX * Rsense) > + * > + * As Rsense is in tens of micro-ohm, we need to multiply by DECA to get > + * microujoule. > + */ > + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, &temp)) { > + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, U16_MAX); > + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); > + return ret; return 0 > + } > + > + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); > + > + return ret; return 0 > +} > + > +static int ltc4282_read_power_average(struct ltc4282_state *st, long *val) > +{ > + u64 energy, temp; > + __be32 raw; > + u32 count; > + int ret; > + > + guard(mutex)(&st->lock); > + > + ret = ltc4282_read_energy(st, &energy); > + if (ret < 0) > + return ret; > + if (ret == 1) { > + /* > + * reset happened... let's read the new energy value that > + * together with the new tick counter should give a sane average > + * value. Furthermore, we save whatever value we had accumulated > + * so that the next energy read will have it into account. > + */ > + st->saved_energy = energy; > + /* give some time for accumulation... */ > + msleep(2 * LTC4282_TCONV_US / MILLI); > + ret = ltc4282_read_energy(st, &energy); > + if (ret < 0) > + return ret; > + } > + > + ret = regmap_bulk_read(st->map, LTC4282_TIME_COUNTER, &raw, sizeof(raw)); > + if (ret) > + return ret; > + > + count = be32_to_cpu(raw); > + if (!count) { > + *val = 0; > + return 0; > + } > + > + /* > + * AVG = E / (Tconv * counter) > + * We get energy in microJoule, hence dividing it by microSeconds gives Watts. Therefore, > + * multiplying by MICRO gives us microWatts. > + */ > + if (check_mul_overflow(energy, MICRO, &temp)) { > + temp = DIV_ROUND_CLOSEST_ULL(energy, LTC4282_TCONV_US); > + *val = DIV_ROUND_CLOSEST_ULL(temp * MICRO, count); > + return 0; > + } > + > + *val = DIV64_U64_ROUND_CLOSEST(temp, LTC4282_TCONV_US * (u64)count); > + return 0; > +} No. If the chip doesn't report power averages, calculate it in userspace. > + > +static int ltc4282_read_power(struct device *dev, const u32 attr, long *val) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_power_input: > + return ltc4282_read_power_word(st, LTC4282_POWER, val); > + case hwmon_power_input_highest: > + return ltc4282_read_power_word(st, LTC4282_POWER_HIGHEST, val); > + case hwmon_power_input_lowest: > + return ltc4282_read_power_word(st, LTC4282_POWER_LOWEST, val); > + case hwmon_power_max_alarm: > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_POWER_ALARM_H_MASK, > + val); > + case hwmon_power_min_alarm: > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, LTC4282_POWER_ALARM_L_MASK, > + val); > + case hwmon_power_average: > + return ltc4282_read_power_average(st, val); > + case hwmon_power_max: > + return ltc4282_read_power_byte(st, LTC4282_POWER_MAX, val); > + case hwmon_power_min: > + return ltc4282_read_power_byte(st, LTC4282_POWER_MIN, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_energy_enable(struct device *dev, long *val) > +{ > + const struct ltc4282_state *st = dev_get_drvdata(dev); > + u32 reg_val; > + int ret; > + > + ret = regmap_read(st->map, LTC4282_ADC_CTRL, ®_val); > + if (ret) > + return ret; > + > + *val = !FIELD_GET(LTC4282_METER_HALT_MASK, reg_val); > + > + return 0; > +} > + > +static int ltc4282_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + switch (type) { > + case hwmon_in: > + return ltc4282_read_in(dev, attr, val, channel); > + case hwmon_curr: > + return ltc4282_read_curr(dev, attr, val); > + case hwmon_power: > + return ltc4282_read_power(dev, attr, val); > + case hwmon_energy: > + return ltc4282_energy_enable(dev, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_write_power_byte(const struct ltc4282_state *st, u32 reg, long val) > +{ > + u32 power; > + u64 temp; > + > + if (val > st->power_max) > + val = st->power_max; > + > + temp = val * int_pow(U8_MAX, 2) * st->rsense; > + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st->vfs_out * 40); > + > + return regmap_write(st->map, reg, power); > +} > + > +static int ltc4282_write_power(const struct ltc4282_state *st, u32 attr, > + long val) > +{ > + switch (attr) { > + case hwmon_power_max: > + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, val); > + case hwmon_power_min: > + return ltc4282_write_power_byte(st, LTC4282_POWER_MIN, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_write_voltage_byte(const struct ltc4282_state *st, u32 reg, u32 fs, long val) > +{ > + u32 in; > + > + if (val >= fs) > + in = U8_MAX; > + else > + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); > + > + return regmap_write(st->map, reg, in); > +} > + > +static int ltc4282_write_in(const struct ltc4282_state *st, u32 attr, long val, > + int channel) > +{ > + switch (attr) { > + case hwmon_in_max: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_write_voltage_byte(st, LTC4282_VSOURCE_MAX, > + st->vfs_out, val); > + > + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MAX, 1280, val); > + case hwmon_in_min: > + if (channel == LTC4282_CHAN_VSOURCE) > + return ltc4282_write_voltage_byte(st, LTC4282_VSOURCE_MIN, > + st->vfs_out, val); > + > + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MIN, 1280, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_write_curr(const struct ltc4282_state *st, u32 attr, > + long val) > +{ > + /* need to pass it in millivolt */ > + u32 in = DIV_ROUND_CLOSEST_ULL((u64)val * st->rsense, DECA * MICRO); > + > + switch (attr) { > + case hwmon_curr_max: > + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40, in); > + case hwmon_curr_min: > + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MIN, 40, in); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_write(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + const struct ltc4282_state *st = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_power: > + return ltc4282_write_power(st, attr, val); > + case hwmon_in: > + return ltc4282_write_in(st, attr, val, channel); > + case hwmon_curr: > + return ltc4282_write_curr(st, attr, val); > + case hwmon_energy: > + /* setting the bit halts the meter */ > + return regmap_update_bits(st->map, LTC4282_ADC_CTRL, > + LTC4282_METER_HALT_MASK, > + FIELD_PREP(LTC4282_METER_HALT_MASK, !!!val)); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int ltc4282_in_is_visible(const struct ltc4282_state *st, u32 attr) > +{ > + switch (attr) { > + case hwmon_in_input: > + case hwmon_in_highest: > + case hwmon_in_lowest: > + case hwmon_in_max_alarm: > + case hwmon_in_min_alarm: > + case hwmon_in_label: > + case hwmon_in_lcrit_alarm: > + case hwmon_in_crit_alarm: > + return 0444; > + case hwmon_in_max: > + case hwmon_in_min: > + return 0644; > + default: > + return 0; > + } > +} > + > +static int ltc4282_curr_is_visible(u32 attr) > +{ > + switch (attr) { > + case hwmon_curr_input: > + case hwmon_curr_highest: > + case hwmon_curr_lowest: > + case hwmon_curr_max_alarm: > + case hwmon_curr_min_alarm: > + case hwmon_curr_crit_alarm: > + case hwmon_curr_label: > + return 0444; > + case hwmon_curr_max: > + case hwmon_curr_min: > + return 0644; > + default: > + return 0; > + } > +} > + > +static int ltc4282_power_is_visible(u32 attr) > +{ > + switch (attr) { > + case hwmon_power_input: > + case hwmon_power_input_highest: > + case hwmon_power_input_lowest: > + case hwmon_power_label: > + case hwmon_power_max_alarm: > + case hwmon_power_min_alarm: > + case hwmon_power_average: > + return 0444; > + case hwmon_power_max: > + case hwmon_power_min: > + return 0644; > + default: > + return 0; > + } > +} > + > +static umode_t ltc4282_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_in: > + return ltc4282_in_is_visible(data, attr); > + case hwmon_curr: > + return ltc4282_curr_is_visible(attr); > + case hwmon_power: > + return ltc4282_power_is_visible(attr); > + case hwmon_energy: > + /* hwmon_energy_enable */ > + return 0644; > + default: > + return -ENOTSUPP; > + } > +} > + > +static const char * const ltc4282_in_strs[] = { > + "VSOURCE", "VDD", "VGPIO" > +}; > + > +static int ltc4282_read_labels(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + switch (type) { > + case hwmon_in: > + *str = ltc4282_in_strs[channel]; > + return 0; > + case hwmon_curr: > + *str = "ISENSE"; > + return 0; > + case hwmon_power: > + *str = "Power"; > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static ssize_t ltc4282_show_energy(struct device *dev, > + struct device_attribute *da, char *buf) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + u64 energy; > + long en; > + int ret; > + > + guard(mutex)(&st->lock); > + > + ret = ltc4282_energy_enable(dev, &en); > + if (ret) > + return ret; > + if (!en) > + return -ENODATA; > + This should be stored in a flag. Reading it from the chip for each energy reading is too expensive. > + ret = ltc4282_read_energy(st, &energy); > + if (ret < 0) > + return ret; > + > + /* see @ltc4282_read_power_average */ > + if (st->saved_energy) { > + energy += st->saved_energy; > + st->saved_energy = 0; > + } > + > + return sysfs_emit(buf, "%llu\n", energy); > +} > + > +static ssize_t ltc4282_show_fault(struct device *dev, > + struct device_attribute *da, > + u32 reg, char *buf) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); > + long alarm; > + int ret; > + > + ret = ltc4282_read_alarm(st, reg, attr->index, &alarm); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%lu\n", alarm); > +} > + > +static ssize_t ltc4282_show_status(struct device *dev, > + struct device_attribute *da, char *buf) > +{ > + return ltc4282_show_fault(dev, da, LTC4282_STATUS_LSB, buf); > +} > + > +static ssize_t ltc4282_show_fault_log(struct device *dev, > + struct device_attribute *da, char *buf) > +{ > + return ltc4282_show_fault(dev, da, LTC4282_FAULT_LOG, buf); > +} > + > +static ssize_t ltc4282_clear_fault_log(struct device *dev, > + struct device_attribute *da, > + const char *buf, size_t len) > +{ > + struct ltc4282_state *st = dev_get_drvdata(dev); > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); > + int ret; > + > + ret = regmap_clear_bits(st->map, LTC4282_FAULT_LOG, attr->index); > + if (ret) > + return ret; > + > + return len; > +} > + > +static int ltc4282_power_on(const struct ltc4282_state *st) > +{ > + u32 n_tries = 5, reg; > + int ret; > + > + ret = devm_regulator_get_enable(st->dev, "vdd"); > + if (ret) > + return dev_err_probe(st->dev, ret, "Failed vdd get/enable\n"); > + > + /* > + * Make sure vdd is stable. From the datasheet: > + * The state of the UV and OV comparators is indicated by the STATUS register > + * bits 0 and 1 and must be stable for at least 50ms to qualify for turn-on. > + */ > + do { > + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg, > + reg & LTC4282_VDD_STATUS_MASK, 10000, 50000); > + if (!ret) > + continue; > + if (ret != -ETIMEDOUT) > + return dev_err_probe(st->dev, ret, "Failed regmap read\n"); > + > + /* Alright. We got timeout which means UV and OV are stable for 50ms */ > + break; > + } while (n_tries--); > + > + if (!n_tries) > + return dev_err_probe(st->dev, -ETIMEDOUT, "VDD not stable\n"); > + > + return 0; > +} This is a hot swap controller. Turning it on in its hardware monitoring driver is highly inappropriate. I don't know where and how it is supposed to be turned on, but that can not be done here. Also, turning it off when unloading the driver (presumably that will happen since it is turned on with a device managed function) is even more inappropriate. It is even more inappropriate to delay the probe function by at least 50 ms just to check if two status register bits changed state. > + > +enum { > + LTC4282_CLKOUT_INT, > + LTC4282_CLKOUT_TICK, > +}; > + > +static int ltc428_clks_setup(const struct ltc4282_state *st) > +{ > + unsigned long rate; > + struct clk *clkin; > + u32 val; > + int ret; > + > + ret = device_property_read_u32(st->dev, "adi,clkout-mode", &val); > + if (ret) > + return 0; > + > + if (val > LTC4282_CLKOUT_TICK) > + return dev_err_probe(st->dev, -EINVAL, > + "Invalid val(%u) for adi,clkout-mode\n", val); > + > + ret = regmap_update_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLKOUT_MASK, > + FIELD_PREP(LTC4282_CLKOUT_MASK, val + 1)); > + if (ret) > + return ret; > + If the chip generates a clock output, the driver needs to register a clock device. > + clkin = devm_clk_get_optional_enabled(st->dev, NULL); > + if (IS_ERR(clkin)) > + return dev_err_probe(st->dev, PTR_ERR(clkin), "Failed to get clkin"); > + if (!clkin) > + return 0; > + > + rate = clk_get_rate(clkin); > + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX) > + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin range(%lu) [%lu %lu]\n", > + rate, LTC4282_CLKIN_MIN, LTC4282_CLKIN_MAX); > + > + /* > + * Clocks faster than 250KHZ should be reduced to 250KHZ. The clock frequency > + * is divided by twice the value in the register. > + */ > + val = rate / (2 * LTC4282_CLKIN_MIN); > + > + return regmap_update_bits(st->map, LTC4282_CLK_DIV, LTC4282_CLK_DIV_MASK, > + FIELD_PREP(LTC4282_CLK_DIV_MASK, val)); > +} > + > +static const int ltc4282_curr_lim_uv[] = { > + 12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375 > +}; > + > +static int ltc4282_get_defaults(struct ltc4282_state *st, u32 *curr_lim_uv) > +{ > + u32 reg_val, ilm_adjust; > + int ret; > + > + ret = regmap_read(st->map, LTC4282_CTRL_MSB, ®_val); > + if (ret) > + return ret; > + > + st->vin_mode = FIELD_GET(LTC4282_CTRL_VIN_MODE_MASK, reg_val); > + > + ret = regmap_read(st->map, LTC4282_ILIM_ADJUST, ®_val); > + if (ret) > + return ret; > + > + ilm_adjust = FIELD_GET(LTC4282_ILIM_ADJUST_MASK, reg_val); > + *curr_lim_uv = ltc4282_curr_lim_uv[ilm_adjust]; > + > + return 0; > +} > + > +/* > + * Set max limits for ISENSE and Power as that depends on the max voltage on rsense > + * that is defined in ILIM_ADJUST. This is specially important for power because for > + * some rsense and vfsout values, if we allow the default raw 255 value, that would > + * overflow long in 32bit archs when reading back the max power limit. > + */ Wouldn't it make more sense to limit rsense to reasonable values instead ? > +static int ltc4282_set_max_limits(struct ltc4282_state *st, u32 val_uv) > +{ > + int ret; > + > + ret = ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40 * MILLI, val_uv); > + if (ret) > + return ret; > + > + /* Power is given by ISENSE * Vout. */ > + st->power_max = DIV_ROUND_CLOSEST(val_uv * DECA * MILLI, st->rsense) * st->vfs_out; > + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, st->power_max); > +} > + > +/* valid GPIO functions */ > +enum { > + LTC4282_PIN_GPIO, > + /* Power functions only for GPIO_1*/ > + LTC4282_PIN_POWER_BAD, > + LTC4282_PIN_POWER_GOOD, > + /* ADC monitor only for GPIO_2 and 3 */ > + LTC4282_PIN_ADC = 2, > + /* Only for GPIO_2 */ > + LTC4282_PIN_FET_STRESS, > +}; > + > +static int ltc4282_non_gpio_setup(struct ltc4282_state *st, u32 pin, u32 func, bool *adc_in) > +{ > + if (pin == LTC4282_GPIO_1) { > + u32 val = LTC4282_PIN_POWER_BAD; > + > + if (func == LTC4282_PIN_POWER_GOOD) > + val = 0; > + > + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, LTC4282_GPIO_1_CONFIG_MASK, > + FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, val)); > + } > + > + if (func == LTC4282_PIN_FET_STRESS) > + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > + LTC4282_GPIO_2_FET_STRESS_MASK, > + FIELD_PREP(LTC4282_GPIO_2_FET_STRESS_MASK, 1)); > + > + /* > + * Then, let's point the given GPIO to the ADC input. We need to ensure that > + * function is only given once. > + */ > + if (*adc_in) > + return dev_err_probe(st->dev, -EINVAL, > + "Only one gpio can be given to the ADC input\n"); > + > + *adc_in = true; > + > + /* setting the bit to 1 cause the ADC to monitor GPIO2 */ > + return regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_GPIO_MODE_MASK, > + FIELD_PREP(LTC4282_GPIO_MODE_MASK, pin == LTC4282_GPIO_2)); > +} > + > +static const char * const ltc4282_gpio_prop[] = { > + "adi,gpio1-mode", "adi,gpio2-mode", "adi,gpio3-mode" > +}; > + > +static int ltc4282_gpio_setup(struct ltc4282_state *st) > +{ > + struct device *dev = st->dev; > + u32 gpio, func, ngpios = 0; > + bool adc_in = false; > + int ret; > + > + if (!IS_ENABLED(CONFIG_GPIOLIB)) > + return 0; > + > + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { > + ret = device_property_read_u32(dev, ltc4282_gpio_prop[gpio], &func); > + if (ret) > + continue; > + if (func >= ltc4282_gpios[gpio].n_funcs) > + return dev_err_probe(dev, ret, "Invalid func(%u >= %u) for gpio%u\n", > + func, ltc4282_gpios[gpio].n_funcs, gpio + 1); > + if (func == LTC4282_PIN_GPIO) { > + st->gpio_map[ngpios++] = gpio; > + if (gpio == LTC4282_GPIO_1) { > + /* default to input GPIO */ > + ret = regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, > + LTC4282_GPIO_1_CONFIG_MASK); > + if (ret) > + return ret; > + } > + > + continue; > + } > + > + ret = ltc4282_non_gpio_setup(st, gpio, func, &adc_in); > + if (ret) > + return ret; > + } > + > + if (device_property_read_bool(dev, "adi,gpio-alert")) > + st->gpio_map[ngpios++] = LTC4282_ALERT; > + > + if (!ngpios) > + return 0; > + > + st->gc.parent = dev; > + st->gc.base = -1; > + st->gc.ngpio = ngpios; > + st->gc.can_sleep = true; > + st->gc.label = "ltc4282"; > + st->gc.direction_input = ltc4282_gpio_input_set; > + st->gc.direction_output = ltc4282_gpio_output_set; > + st->gc.set = ltc4282_gpio_set; > + st->gc.get = ltc4282_gpio_get; > + > + return devm_gpiochip_add_data(dev, &st->gc, st); > +} > + > +/* This maps the Vout full scale for the given Vin mode */ > +static const u16 ltc4282_vfs_milli[] = { 5540, 8320, 16640, 33280 }; > + > +enum { > + LTC4282_DIV_EXTERNAL, > + LTC4282_DIV_5_PERCENT, > + LTC4282_DIV_10_PERCENT, > + LTC4282_DIV_15_PERCENT, > +}; > + > +static int ltc4282_setup(struct ltc4282_state *st) > +{ > + struct device *dev = st->dev; > + u32 val, curr_lim_uv; > + int ret; > + > + /* The part has an eeprom so let's get the needed defaults from it */ > + ret = ltc4282_get_defaults(st, &curr_lim_uv); > + if (ret) > + return ret; > + > + ret = device_property_read_u32(dev, "adi,rsense-nano-ohms", &st->rsense); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to read adi,rsense-nano-ohms\n"); > + > + /* > + * The resolution for rsense is tens of micro which means we need nano in the bindings. Assuming this is supposed to mean "tenths of micro", not "tens of micro", do such sense resistors really exist in practice ? The smallest I was able to find has 25 micro-ohm. > + * However, to make things easier to handle (with respect to overflows) we divide it by > + * 100 as we don't really need the last two digits. > + */ > + st->rsense /= CENTI; > + This blindly accepts rsense values of < 100 which results in st->rsense == 0 and division by zero crashes. > + ret = device_property_read_u32(dev, "vin-mode-microvolt", &val); No adi, prefix ? Anyway, this is really the input voltage range, or rather the maximum accepted input voltage. > + if (!ret) { > + switch (val) { > + case 3300000: > + st->vin_mode = LTC4282_VIN_3_3V; > + break; > + case 5000000: > + st->vin_mode = LTC4282_VIN_5V; > + break; > + case 12000000: > + st->vin_mode = LTC4282_VIN_12V; > + break; > + case 24000000: > + st->vin_mode = LTC4282_VIN_24V; > + break; > + default: > + return dev_err_probe(dev, -EINVAL, > + "Invalid val(%u) for vin-mode-microvolt\n", val); > + } > + > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_VIN_MODE_MASK, > + FIELD_PREP(LTC4282_CTRL_VIN_MODE_MASK, st->vin_mode)); > + if (ret) > + return ret; > + > + /* Foldback mode should also be set to the input voltage */ > + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_FOLDBACK_MODE_MASK, > + FIELD_PREP(LTC4282_FOLDBACK_MODE_MASK, st->vin_mode)); > + if (ret) > + return ret; > + } > + > + st->vfs_out = ltc4282_vfs_milli[st->vin_mode]; > + > + ret = device_property_read_u32(dev, "adi,current-limit-microvolt", > + &curr_lim_uv); Current is measured in Ampere. Yes, that translates to volt through the sense resistor, but it is still Ampere. No idea how that is normally handled in devicetree. > + if (!ret) { > + int reg_val; > + > + switch (val) { > + case 12500: > + reg_val = 0; > + break; > + case 15625: > + reg_val = 1; > + break; > + case 18750: > + reg_val = 2; > + break; > + case 21875: > + reg_val = 3; > + break; > + case 25000: > + reg_val = 4; > + break; > + case 28125: > + reg_val = 5; > + break; > + case 31250: > + reg_val = 6; > + break; > + case 34375: > + reg_val = 7; > + break; > + default: > + return dev_err_probe(dev, -EINVAL, > + "Invalid val(%u) for adi,current-limit-microvolt\n", > + val); > + } > + > + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, LTC4282_ILIM_ADJUST_MASK, > + FIELD_PREP(LTC4282_ILIM_ADJUST_MASK, reg_val)); > + if (ret) > + return ret; > + } > + > + ret = ltc4282_set_max_limits(st, curr_lim_uv); > + if (ret) > + return ret; > + > + ret = device_property_read_u32(st->dev, "adi,overvoltage-dividers", &val); > + if (!ret) { > + if (val > LTC4282_DIV_15_PERCENT) > + return dev_err_probe(st->dev, -EINVAL, > + "Invalid val(%u) for adi,overvoltage-divider\n", val); > + > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_OV_MODE_MASK, > + FIELD_PREP(LTC4282_CTRL_OV_MODE_MASK, val)); > + } > + > + ret = device_property_read_u32(st->dev, "adi,undervoltage-dividers", &val); > + if (!ret) { > + if (val > LTC4282_DIV_15_PERCENT) > + return dev_err_probe(st->dev, -EINVAL, > + "Invalid val(%u) for adi,undervoltage-divider\n", val); > + > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, LTC4282_CTRL_UV_MODE_MASK, > + FIELD_PREP(LTC4282_CTRL_UV_MODE_MASK, val)); > + } > + > + if (device_property_read_bool(dev, "adi,overcurrent-retry")) { > + ret = regmap_set_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_OC_RETRY_MASK); > + if (ret) > + return ret; > + } > + > + if (device_property_read_bool(dev, "adi,overvoltage-retry-disable")) { > + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_OV_RETRY_MASK); > + if (ret) > + return ret; > + } > + > + if (device_property_read_bool(dev, "adi,undervoltage-retry-disable")) { > + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, LTC4282_CTRL_UV_RETRY_MASK); > + if (ret) > + return ret; > + } > + > + if (device_property_read_bool(dev, "adi,fault-log-enable")) { > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_FAULT_LOG_EN_MASK); > + if (ret) > + return ret; > + } > + > + ret = device_property_read_u32(dev, "adi,fet-bad-timeout-ms", &val); > + if (!ret) { > + if (val > LTC4282_FET_BAD_MAX_TIMEOUT) > + return dev_err_probe(dev, -EINVAL, > + "Invalid value(%u) for adi,fet-bad-timeout-ms", val); > + > + ret = regmap_write(st->map, LTC4282_FET_BAD_FAULT_TIMEOUT, val); > + if (ret) > + return ret; > + } > + > + return ltc4282_gpio_setup(st); > +} > + > +static bool ltc4282_readable_reg(struct device *dev, unsigned int reg) > +{ > + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) > + return false; > + > + return true; > +} > + > +static bool ltc4282_writable_reg(struct device *dev, unsigned int reg) > +{ > + if (reg == LTC4282_STATUS_LSB || reg == LTC4282_STATUS_MSB) > + return false; > + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) > + return false; > + > + return true; > +} > + > +static const struct regmap_config ltc4282_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = LTC4282_RESERVED_3, > + .readable_reg = ltc4282_readable_reg, > + .writeable_reg = ltc4282_writable_reg, > +}; > + > +static const struct hwmon_channel_info * const ltc4282_info[] = { > + HWMON_CHANNEL_INFO(in, > + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | > + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | > + HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_LCRIT_ALARM | HWMON_I_CRIT_ALARM | > + HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST | > + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | > + HWMON_I_MAX_ALARM | HWMON_I_LABEL), > + HWMON_CHANNEL_INFO(curr, > + HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST | > + HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM | > + HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM | > + HWMON_C_LABEL), > + HWMON_CHANNEL_INFO(power, > + HWMON_P_INPUT | HWMON_P_INPUT_LOWEST | > + HWMON_P_INPUT_HIGHEST | HWMON_P_MAX | HWMON_P_MIN | > + HWMON_P_AVERAGE | HWMON_P_MAX_ALARM | HWMON_P_MIN_ALARM | > + HWMON_P_LABEL), > + HWMON_CHANNEL_INFO(energy, > + HWMON_E_ENABLE), > + NULL > +}; > + > +static const struct hwmon_ops ltc4282_hwmon_ops = { > + .read = ltc4282_read, > + .write = ltc4282_write, > + .is_visible = ltc4282_is_visible, > + .read_string = ltc4282_read_labels, > +}; > + > +static const struct hwmon_chip_info ltc2947_chip_info = { > + .ops = <c4282_hwmon_ops, > + .info = ltc4282_info, > +}; > + > +/* energy attributes are 6bytes wide so we need u64 */ > +static SENSOR_DEVICE_ATTR(energy1_input, 0444, ltc4282_show_energy, NULL, 0); > +/* power1_fault */ > +static SENSOR_DEVICE_ATTR(power1_good, 0444, ltc4282_show_status, NULL, > + LTC4282_POWER_GOOD_MASK); > +/* FET faults */ > +static SENSOR_DEVICE_ATTR(fet_short_fault, 0444, ltc4282_show_status, NULL, > + LTC4282_FET_SHORT_MASK); > +static SENSOR_DEVICE_ATTR(fet_bad_fault, 0444, ltc4282_show_status, NULL, > + LTC4282_FET_BAD_STATUS_MASK); > +/* > + * Fault log failures. These faults might be important in systems where auto-retry is not enabled > + * since they will cause the part to latch off until they are cleared. Typically that happens > + * when the system admin is close enough so he can check what happened and manually clear the > + * faults. Moreover, manually clearing the faults might only matter when ON_FAULT_MASK in the > + * CONTROL register is set (which is the default) as in that case, a turn off signal from the > + * ON pin won't clear them. > + */ I'd argue that such an operator would only be interested in clearing all faults at the same time. Your call, but all this does is making life hard for operators. > +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_POWER_BAD_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_FET_BAD_FAULT_MASK); > +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, ltc4282_show_fault_log, > + ltc4282_clear_fault_log, LTC4282_FET_SHORT_FAULT_MASK); > + > +static struct attribute *ltc4282_attrs[] = { > + &sensor_dev_attr_energy1_input.dev_attr.attr, > + &sensor_dev_attr_power1_good.dev_attr.attr, > + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, > + &sensor_dev_attr_fet_short_fault.dev_attr.attr, > + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr, > + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr, > + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr, > + &sensor_dev_attr_power1_fault_log.dev_attr.attr, > + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr, > + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(ltc4282); > + Move all those except for energy1_input to debugfs. > +static int ltc4282_probe(struct i2c_client *i2c) > +{ > + struct device *dev = &i2c->dev, *hwmon; > + struct ltc4282_state *st; > + int ret; > + > + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); > + if (!st) > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate memory\n"); > + > + st->dev = dev; > + st->map = devm_regmap_init_i2c(i2c, <c4282_regmap_config); > + if (IS_ERR(st->map)) > + return dev_err_probe(dev, PTR_ERR(st->map), "failed regmap init\n"); > + > + ret = ltc4282_power_on(st); > + if (ret) > + return ret; > + > + /* Soft reset */ > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_RESET_MASK); > + if (ret) > + return ret; > + > + msleep(3200); > + > + ret = ltc428_clks_setup(st); > + if (ret) > + return ret; > + > + ret = ltc4282_setup(st); > + if (ret) > + return ret; > + > + mutex_init(&st->lock); > + hwmon = devm_hwmon_device_register_with_info(dev, "ltc4282", st, <c2947_chip_info, > + ltc4282_groups); > + return PTR_ERR_OR_ZERO(hwmon); > +} > + > +static const struct of_device_id ltc4282_of_match[] = { > + { .compatible = "adi,ltc4282" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ltc4282_of_match); > + > +static struct i2c_driver ltc4282_driver = { > + .driver = { > + .name = "ltc4282", > + .of_match_table = ltc4282_of_match, > + }, > + .probe = ltc4282_probe, > +}; > +module_i2c_driver(ltc4282_driver); > + > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > +MODULE_DESCRIPTION("LTC4282 I2C High Current Hot Swap Controller"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-11 17:22 ` Guenter Roeck @ 2023-11-13 9:24 ` Nuno Sá 0 siblings, 0 replies; 17+ messages in thread From: Nuno Sá @ 2023-11-13 9:24 UTC (permalink / raw) To: Guenter Roeck, Nuno Sa, linux-hwmon, linux-doc, devicetree Cc: Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley On Sat, 2023-11-11 at 09:22 -0800, Guenter Roeck wrote: > On 11/10/23 07:18, Nuno Sa wrote: > > The LTC4282 hot swap controller allows a board to be safely inserted and > > removed from a live backplane. Using one or more external N-channel pass > > transistors, board supply voltage and inrush current are ramped up at an > > adjustable rate. An I2C interface and onboard ADC allows for monitoring > > of board current, voltage, power, energy and fault status. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > Documentation/hwmon/ltc4282.rst | 101 ++ > > As reported by 0-day, this must be added to index.rst. > ack... > > Anyway, partial review only. Also, since it registers a gpio driver, > it will need to be reviewed by a gpio maintainer. I am not comfortable > with some of the gpio code. > Yeah, I'm also far from being sure on the GPIO stuff (I have some words regarding that in the cover) and I did already CC'ed the maintainers + Andy which is a reviewer. > > MAINTAINERS | 2 + > > drivers/hwmon/Kconfig | 11 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/ltc4282.c | 1518 +++++++++++++++++++++++++++++++ > > 5 files changed, 1633 insertions(+) > > create mode 100644 Documentation/hwmon/ltc4282.rst > > create mode 100644 drivers/hwmon/ltc4282.c > > > > diff --git a/Documentation/hwmon/ltc4282.rst > > b/Documentation/hwmon/ltc4282.rst > > new file mode 100644 > > index 000000000000..2fab5d165aa4 > > --- /dev/null > > +++ b/Documentation/hwmon/ltc4282.rst > > @@ -0,0 +1,101 @@ > > +Kernel drivers ltc4282 > > +========================================== > > + > > +Supported chips: > > + > > + * Analog Devices LTC4282 > > + > > + Prefix: 'ltc4282' > > + > > + Addresses scanned: - I2C 0x40 - 0x5A (7-bit) > > + Addresses scanned: - I2C 0x80 - 0xB4 with a step of 2 (8-bit) > > + > > + Datasheet: > > + > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf > > + > > +Author: Nuno Sá <nuno.sa@analog.com> > > + > > +Description > > +___________ > > + > > +The LTC4282 hot swap controller allows a board to be safely inserted and > > removed > > +from a live backplane. Using one or more external N-channel pass > > transistors, > > +board supply voltage and inrush current are ramped up at an adjustable > > rate. An > > +I2C interface and onboard ADC allows for monitoring of board current, > > voltage, > > +power, energy and fault status. The device features analog foldback current > > +limiting and supply monitoring for applications from 2.9V to 33V. Dual 12V > > gate > > +drive allows high power applications to either share safe operating area > > across > > +parallel MOSFETs or support a 2-stage start-up that first charges the load > > +capacitance followed by enabling a low on-resistance path to the load. The > > +LTC4282 is well suited to high power applications because the precise > > monitoring > > +capability and accurate current limiting reduce the extremes in which both > > loads > > +and power supplies must safely operate. Non-volatile configuration allows > > for > > +flexibility in the autonomous generation of alerts and response to faults. > > + > > +Sysfs entries > > +_____________ > > + > > +The following attributes are supported. Limits are read-write and all the > > other > > +attributes are read-only. > > + > > +======================= ========================================== > > +in0_input Output voltage (mV). > > +in0_min Undervoltage threshold > > +in0_max Overvoltage threshold > > +in0_lowest Lowest measured voltage > > +in0_highest Highest measured voltage > > +in0_min_alarm Undervoltage alarm > > +in0_max_alarm Overvoltage alarm > > +in0_label Channel label (VSOURCE) > > + > > +in1_input Input voltage (mV). > > +in1_lcrit_alarm Critical Undervoltage alarm > > +in1_lcrit_fault_log Undervoltage Fault Log. In some setups, clearing > > this > > + fault (writing on the file) might be needed. > > +in1_crit_alarm Critical Overvoltage alarm > > +in1_crit_fault_log Overvoltage Fault Log. In some setups, clearing > > this > > + fault (writing on the file) might be needed > > +in1_label Channel label (VDD) > > + > > +in2_input GPIO voltage (mV) > > +in2_min Undervoltage threshold > > +in2_max Overvoltage threshold > > +in2_lowest Lowest measured voltage > > +in2_highest Highest measured voltage > > +in2_min_alarm Undervoltage alarm > > +in2_max_alarm Overvoltage alarm > > +in2_label Channel label (VGPIO) > > + > > +curr1_input Sense current (mA) > > +curr1_min Undercurrent threshold > > +curr1_max Overcurrent threshold > > +curr1_lowest Lowest measured current > > +curr1_highest Highest measured current > > +curr1_min_alarm Undercurrent alarm > > +curr1_max_alarm Overcurrent alarm > > +curr1_crit_alarm Critical Overcurrent alarm > > +curr1_crit_fault_log Overcurrent Fault Log. In some setups, clearing > > this > > + fault (writing on the file) might be needed > > +curr1_label Channel label (ISENSE) > > + > > +power1_input Power (in uW) > > +power1_min Low power threshold > > +power1_max High power threshold > > +power1_average Average Power use > > +power1_input_lowest Historical minimum power use > > +power1_input_highest Historical maximum power use > > +power1_min_alarm Low power alarm > > +power1_max_alarm High power alarm > > +power1_good Power considered good > > +power1_fault_log Power bad Fault log > > +power1_label Channel label (Power) > > + > > +energy1_input Measured energy over time (in microJoule) > > +energy1_enable Enable/Disable Energy accumulation > > + > > +fet_short_fault FET short alarm > > +fet_short_fault_log FET short Fault log > > +fet_bad_fault FET bad alarm > > +fet_bad_fault_log FET bad Fault log > > +======================= ========================================== > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 9f9527f6057b..ba7f0439d8e8 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -12486,6 +12486,8 @@ M: Nuno Sa <nuno.sa@analog.com> > > L: linux-hwmon@vger.kernel.org > > S: Supported > > F: Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml > > +F: Documentation/hwmon/ltc4282.rst > > +F: drivers/hwmon/ltc4282.c > > > > LTC4306 I2C MULTIPLEXER DRIVER > > M: Michael Hennerich <michael.hennerich@analog.com> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index cf27523eed5a..2f1a0cf2e1e4 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1027,6 +1027,17 @@ config SENSORS_LTC4261 > > This driver can also be built as a module. If so, the module will > > be called ltc4261. > > > > +config SENSORS_LTC4282 > > + tristate "Analog Devices LTC4282" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + If you say yes here you get support for Analog Devices LTC4282 > > + High Current Hot Swap Controller I2C interface. > > + > > + This driver can also be built as a module. If so, the module will > > + be called ltc4282. > > + > > config SENSORS_LTQ_CPUTEMP > > bool "Lantiq cpu temperature sensor driver" > > depends on SOC_XWAY > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index e84bd9685b5c..cbf22d1d736f 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_LTC4222) += ltc4222.o > > obj-$(CONFIG_SENSORS_LTC4245) += ltc4245.o > > obj-$(CONFIG_SENSORS_LTC4260) += ltc4260.o > > obj-$(CONFIG_SENSORS_LTC4261) += ltc4261.o > > +obj-$(CONFIG_SENSORS_LTC4282) += ltc4282.o > > obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o > > obj-$(CONFIG_SENSORS_MAX1111) += max1111.o > > obj-$(CONFIG_SENSORS_MAX127) += max127.o > > diff --git a/drivers/hwmon/ltc4282.c b/drivers/hwmon/ltc4282.c > > new file mode 100644 > > index 000000000000..738312cf212e > > --- /dev/null > > +++ b/drivers/hwmon/ltc4282.c > > @@ -0,0 +1,1518 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C > > + * > > + * Copyright 2023 Analog Devices Inc. > > + */ > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/cleanup.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/hwmon.h> > > +#include <linux/hwmon-sysfs.h> > > +#include <linux/i2c.h> > > +#include <linux/math.h> > > +#include <linux/module.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/mutex.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/property.h> > > +#include <linux/units.h> > > + > > +#define LTC4282_CTRL_LSB 0x00 > > + #define LTC4282_CTRL_OV_RETRY_MASK BIT(0) > > + #define LTC4282_CTRL_UV_RETRY_MASK BIT(1) > > + #define LTC4282_CTRL_OC_RETRY_MASK BIT(2) > > + #define LTC4282_CTRL_ON_ACTIVE_LOW_MASK BIT(5) > > + #define LTC4282_CTRL_ON_DELAY_MASK BIT(6) > > +#define LTC4282_CTRL_MSB 0x01 > > + #define LTC4282_CTRL_VIN_MODE_MASK GENMASK(1, 0) > > + #define LTC4282_CTRL_OV_MODE_MASK GENMASK(3, 2) > > + #define LTC4282_CTRL_UV_MODE_MASK GENMASK(5, 4) > > +#define LTC4282_FAULT_LOG 0x04 > > + #define LTC4282_OV_FAULT_MASK BIT(0) > > + #define LTC4282_UV_FAULT_MASK BIT(1) > > + #define LTC4282_OC_FAULT_MASK BIT(2) > > + #define LTC4282_POWER_BAD_FAULT_MASK BIT(3) > > + #define LTC4282_FET_SHORT_FAULT_MASK BIT(5) > > + #define LTC4282_FET_BAD_FAULT_MASK BIT(6) > > +#define LTC4282_ADC_ALERT_LOG 0x05 > > + #define LTC4282_GPIO_ALARM_L_MASK BIT(0) > > + #define LTC4282_GPIO_ALARM_H_MASK BIT(1) > > + #define LTC4282_VSOURCE_ALARM_L_MASK BIT(2) > > + #define LTC4282_VSOURCE_ALARM_H_MASK BIT(3) > > + #define LTC4282_VSENSE_ALARM_L_MASK BIT(4) > > + #define LTC4282_VSENSE_ALARM_H_MASK BIT(5) > > + #define LTC4282_POWER_ALARM_L_MASK BIT(6) > > + #define LTC4282_POWER_ALARM_H_MASK BIT(7) > > +#define LTC4282_FET_BAD_FAULT_TIMEOUT 0x06 > > + #define LTC4282_FET_BAD_MAX_TIMEOUT 255 > > +#define LTC4282_GPIO_CONFIG 0x07 > > + #define LTC4282_GPIO_2_FET_STRESS_MASK BIT(1) > > + #define LTC4282_GPIO_1_OUT_MASK BIT(3) > > + #define LTC4282_GPIO_1_CONFIG_MASK GENMASK(5, 4) > > + #define LTC4282_GPIO_2_OUT_MASK BIT(6) > > + #define LTC4282_GPIO_3_OUT_MASK BIT(7) > > +#define LTC4282_VGPIO_MIN 0x08 > > +#define LTC4282_VGPIO_MAX 0x09 > > +#define LTC4282_VSOURCE_MIN 0x0a > > +#define LTC4282_VSOURCE_MAX 0x0b > > +#define LTC4282_VSENSE_MIN 0x0c > > +#define LTC4282_VSENSE_MAX 0x0d > > +#define LTC4282_POWER_MIN 0x0e > > +#define LTC4282_POWER_MAX 0x0f > > +#define LTC4282_CLK_DIV 0x10 > > + #define LTC4282_CLK_DIV_MASK GENMASK(4, 0) > > + #define LTC4282_CLKOUT_MASK GENMASK(6, 5) > > +#define LTC4282_ILIM_ADJUST 0x11 > > + #define LTC4282_GPIO_MODE_MASK BIT(1) > > + #define LTC4282_VDD_MONITOR_MASK BIT(2) > > + #define LTC4282_FOLDBACK_MODE_MASK GENMASK(4, 3) > > + #define LTC4282_ILIM_ADJUST_MASK GENMASK(7, 5) > > +#define LTC4282_ENERGY 0x12 > > +#define LTC4282_TIME_COUNTER 0x18 > > +#define LTC4282_ALERT_CTRL 0x1C > > + #define LTC4282_ALERT_OUT_MASK BIT(6) > > +#define LTC4282_ADC_CTRL 0x1D > > + #define LTC4282_FAULT_LOG_EN_MASK BIT(2) > > + #define LTC4282_METER_HALT_MASK BIT(5) > > + #define LTC4282_METER_RESET_MASK BIT(6) > > + #define LTC4282_RESET_MASK BIT(7) > > +#define LTC4282_STATUS_LSB 0x1E > > + #define LTC4282_OV_STATUS_MASK BIT(0) > > + #define LTC4282_UV_STATUS_MASK BIT(1) > > + #define LTC4282_VDD_STATUS_MASK (LTC4282_OV_STATUS_MASK | > > LTC4282_UV_STATUS_MASK) > > + #define LTC4282_OC_STATUS_MASK BIT(2) > > + #define LTC4282_POWER_GOOD_MASK BIT(3) > > + #define LTC4282_FET_SHORT_MASK BIT(5) > > + #define LTC4282_FET_BAD_STATUS_MASK BIT(6) > > +#define LTC4282_STATUS_MSB 0x1F > > + #define LTC4282_METER_TICK_OVERFLOW_MASK GENMASK(1, 0) > > + #define LTC4282_ALERT_STATUS_MASK BIT(4) > > + #define LTC4282_GPIO_1_STATUS_MASK BIT(5) > > + #define LTC4282_GPIO_2_STATUS_MASK BIT(6) > > + #define LTC4282_GPIO_3_STATUS_MASK BIT(7) > > +#define LTC4282_RESERVED_1 0x32 > > +#define LTC4282_RESERVED_2 0x33 > > +#define LTC4282_VGPIO 0x34 > > +#define LTC4282_VGPIO_LOWEST 0x36 > > +#define LTC4282_VGPIO_HIGHEST 0x38 > > +#define LTC4282_VSOURCE 0x3a > > +#define LTC4282_VSOURCE_LOWEST 0x3c > > +#define LTC4282_VSOURCE_HIGHEST 0x3e > > +#define LTC4282_VSENSE 0x40 > > +#define LTC4282_VSENSE_LOWEST 0x42 > > +#define LTC4282_VSENSE_HIGHEST 0x44 > > +#define LTC4282_POWER 0x46 > > +#define LTC4282_POWER_LOWEST 0x48 > > +#define LTC4282_POWER_HIGHEST 0x4a > > +#define LTC4282_RESERVED_3 0x50 > > + > > +#define LTC4282_CLKIN_MIN (250 * KILO) > > +#define LTC4282_CLKIN_MAX (15500 * KILO) > > +/* this assumes 12bit ADC */ > > +#define LTC4282_TCONV_US 65535 > > +#define LTC4282_GPIO_NR 4 > > +/* > > + * relaxed version of FIELD_PREP() to be used when mask is not a compile > > time constant > > + * u32_encode_bits() can't also be used as the compiler needs to be able to > > evaluate > > + * mask at compile time. > > + */ > > +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m)) > > + > > +struct ltc4282_state { > > + struct regmap *map; > > + struct device *dev; > > This is only used for an inappropriate warning message and during probe. > Pass it to initialization functions where needed. > will do... > > + /* Protect against multiple accesses to the device registers */ > > + struct mutex lock; > > + struct gpio_chip gc; > > + u64 saved_energy; > > + long power_max; > > I do not understand how this is used in the code. It seems to avoid overflows > due to unreasonable values of rsense. That adds a lot of complexist to the > code > and makes it hard to understand. > It's not about unreasonable rsense... More details below in your other comment about this... > > + u32 gpio_map[LTC4282_GPIO_NR]; > > + u32 rsense; > > + u32 vin_mode; > > + u16 vfs_out; > > +}; > > + > > +struct ltc4282_gpio { > > + u32 out_reg; > > + u32 out_mask; > > + u32 in_reg; > > + u32 in_mask; > > + bool active_high; > > + u8 n_funcs; > > +}; > > + > > +enum { > > + LTC4282_VIN_3_3V, > > + LTC4282_VIN_5V, > > + LTC4282_VIN_12V, > > + LTC4282_VIN_24V, > > +}; > > + > > +enum { > > + LTC4282_CHAN_VSOURCE, > > + LTC4282_CHAN_VDD, > > + LTC4282_CHAN_VGPIO, > > +}; > > + > > +enum { > > + LTC4282_GPIO_1, > > + LTC4282_GPIO_2, > > + LTC4282_GPIO_3, > > + LTC4282_ALERT, > > +}; > > + > > +static const struct ltc4282_gpio ltc4282_gpios[] = { > > + [LTC4282_GPIO_1] = { > > + .in_reg = LTC4282_STATUS_MSB, > > + .in_mask = LTC4282_GPIO_1_STATUS_MASK, > > + .out_reg = LTC4282_GPIO_CONFIG, > > + .out_mask = LTC4282_GPIO_1_OUT_MASK, > > + .active_high = true, > > + .n_funcs = 3, > > + }, > > + [LTC4282_GPIO_2] = { > > + .in_reg = LTC4282_STATUS_MSB, > > + .in_mask = LTC4282_GPIO_2_STATUS_MASK, > > + .out_reg = LTC4282_GPIO_CONFIG, > > + .out_mask = LTC4282_GPIO_2_OUT_MASK, > > + .n_funcs = 3, > > + }, > > + [LTC4282_GPIO_3] = { > > + .in_reg = LTC4282_STATUS_MSB, > > + .in_mask = LTC4282_GPIO_3_STATUS_MASK, > > + .out_reg = LTC4282_GPIO_CONFIG, > > + .out_mask = LTC4282_GPIO_3_OUT_MASK, > > + .n_funcs = 2, > > + }, > > + [LTC4282_ALERT] = { > > + .in_reg = LTC4282_STATUS_MSB, > > + .in_mask = LTC4282_ALERT_STATUS_MASK, > > + .out_reg = LTC4282_ALERT_CTRL, > > + .out_mask = LTC4282_ALERT_OUT_MASK, > > + }, > > +}; > > + > > +static int ltc4282_gpio_input_set(struct gpio_chip *chip, unsigned int > > offset) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + u32 gpio_pin = st->gpio_map[offset]; > > + > > + /* we can only control this for GPIO_1 */ > > + if (gpio_pin != LTC4282_GPIO_1) > > + return 0; > > + > > + return regmap_set_bits(st->map, LTC4282_GPIO_CONFIG, > > LTC4282_GPIO_1_CONFIG_MASK); > > +} > > + > > +static int ltc4282_gpio_output_set(struct gpio_chip *chip, unsigned int > > offset, int val) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + u32 gpio_pin = st->gpio_map[offset]; > > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > > + > > + guard(mutex)(&st->lock); > > + /* > > + * Explicitly setting the pin as output can only be done for > > GPIO_1. For the > > + * other pins we just pull the line down or high-z. > > + */ > > + if (gpio_pin == LTC4282_GPIO_1) { > > + int ret; > > + > > + ret = regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > > LTC4282_GPIO_1_CONFIG_MASK, > > + > > FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, 2)); > > + if (ret) > > + return ret; > > + } > > + > > + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull > > down the line */ > > + if (!gpio->active_high) > > + val = !val; > > + > > + return regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > > + LTC4282_FIELD_PREP(gpio->out_mask, val)); > > +} > > + > > +static void ltc4282_gpio_set(struct gpio_chip *chip, unsigned int offset, > > int val) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + u32 gpio_pin = st->gpio_map[offset]; > > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > > + > > + if (!gpio->active_high) > > + val = !val; > > + > > + regmap_update_bits(st->map, gpio->out_reg, gpio->out_mask, > > + LTC4282_FIELD_PREP(gpio->out_mask, val)); > > +} > > + > > +static int ltc4282_gpio_get(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct ltc4282_state *st = gpiochip_get_data(chip); > > + u32 gpio_pin = st->gpio_map[offset]; > > + const struct ltc4282_gpio *gpio = <c4282_gpios[gpio_pin]; > > + int ret; > > + u32 val; > > + > > + ret = regmap_read(st->map, gpio->in_reg, &val); > > + if (ret) > > + return ret; > > + > > + return !!(val & gpio->in_mask); > > +} > > + > > +static int ltc4282_read_voltage_word(const struct ltc4282_state *st, u32 > > reg, u32 fs, long *val) > > +{ > > + __be16 in; > > + int ret; > > + > > + ret = regmap_bulk_read(st->map, reg, &in, sizeof(in)); > > + if (ret) > > + return ret; > > + > > + /* > > + * This is also used to calculate current in which case fs comes in > > 10 * uV. > > + * Hence the ULL usage. > > + */ > > + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX); > > + return 0; > > +} > > + > > +static int ltc4282_read_voltage_byte(const struct ltc4282_state *st, u32 > > reg, u32 fs, long *val) > > +{ > > + int ret; > > + u32 in; > > + > > + ret = regmap_read(st->map, reg, &in); > > + if (ret) > > + return ret; > > + > > + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX); > > + return 0; > > +} > > + > > +static int ltc4282_read_vdd(struct ltc4282_state *st, u32 fs, long *val) > > +{ > > + int ret; > > + > > + guard(mutex)(&st->lock); > > + > > + /* ADC now monitors VDD */ > > + ret = regmap_clear_bits(st->map, LTC4282_ILIM_ADJUST, > > LTC4282_VDD_MONITOR_MASK); > > + if (ret) > > + return ret; > > + > > + /* > > + * Wait for two ADC conversions so we are sure we get one full VDD > > + * measurement. > > + */ > > + msleep(2 * LTC4282_TCONV_US / MILLI); > > + > > + ret = ltc4282_read_voltage_word(st, LTC4282_VSOURCE, st->vfs_out, > > val); > > + if (ret) > > + return ret; > > + > > + /* back to VSOURCE */ > > + return regmap_set_bits(st->map, LTC4282_ILIM_ADJUST, > > LTC4282_VDD_MONITOR_MASK); > > Handle this with _enable attributes if the chip only reports one measurement. > Sleeping 130+ ms for each reading is unacceptable. Besides, subsequent VSOURCE > readings would have to be delayed as well. > > I'll have to read the datasheet in lore detail to understand how this > translates > into limits. I see that the chip has limit registers for the various voltages, > which doesn't really make sense if the chip doesn't measure those voltages. > I see... This together with the sleeps in alarms make, for example, the 'sensors' command to run really slow. The _enable is indeed a good idea and something that I did not remembered. There's only one thing that is not really optimal with that: We have 2 ADCs, one where we can mux between VSOURCE/VDD and the other is for GPIO2/GPIO3. ON top of that we only have one set of registers for VSOURCE/VDD. For the min/max alarms is not an issue as userspace could easily handle them when changing between VDD/VSOURCE. Hmm, and actually when I was just writting this email, I remembered to check the datasheet and just realized that the highest/lowest registers are R/W. So, what comes to mind is that I can support '_reset_history' and then userspace can easily handle the switch between VDD/VSOURCE. I guess the driver could even automatically clear the history when changing between source. Would that be appropriate? Or do you think it's better to be done explicitly by userspace? This also brings me the question if I should have the same handling for changing between GPIO2/GPIO3. Right now, that choice is done via dts because this kind of stuff typically means that you really need to have some kind of HW plugged in the PIN. But , yeah, nowadays things are more and more "dynamic" so, not sure... > > +} > > + > > +static int ltc4282_read_alarm(struct ltc4282_state *st, u32 reg, u32 mask, > > long *val) > > +{ > > + u32 alarm; > > + int ret; > > + > > + guard(mutex)(&st->lock); > > + > > + /* if not status, clear first the alarm by clearing the bit */ > > + if (reg != LTC4282_STATUS_LSB && reg != LTC4282_FAULT_LOG) { > > + ret = regmap_clear_bits(st->map, reg, mask); > > + if (ret) > > + return ret; > > + > > + msleep(LTC4282_TCONV_US / MILLI); > > + } > > Normally one would want to have historic alarms reported at least once, > even if the alarm since went away. This code seems to clear alarm bits > prior to reading them, which will result in under-reporting. This will > need to be explained, especially since it results in a 65+ ms delay for > each alarm report if alarms are active which seems excessive and > unnceessary. Hmm, I see. Well, my thinking for this was really to not see those "historic" alarms and only see things that are still present. But I see now that's not the expectation. I'll move this to after reading the alarms... > > > + > > + ret = regmap_read(st->map, reg, &alarm); > > + if (ret) > > + return ret; > > + > > + *val = !!(alarm & mask); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read_in(struct device *dev, u32 attr, long *val, u32 > > channel) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_in_input: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_voltage_word(st, > > LTC4282_VSOURCE, st->vfs_out, val); > > + if (channel == LTC4282_CHAN_VDD) > > + return ltc4282_read_vdd(st, st->vfs_out, val); > > + > > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO, 1280, > > val); > > + case hwmon_in_highest: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_voltage_word(st, > > LTC4282_VSOURCE_HIGHEST, > > + st->vfs_out, val); > > + > > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_HIGHEST, > > 1280, val); > > + case hwmon_in_lowest: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_voltage_word(st, > > LTC4282_VSOURCE_LOWEST, > > + st->vfs_out, val); > > + > > + return ltc4282_read_voltage_word(st, LTC4282_VGPIO_LOWEST, > > 1280, val); > > + case hwmon_in_max_alarm: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_alarm(st, > > LTC4282_ADC_ALERT_LOG, > > + > > LTC4282_VSOURCE_ALARM_H_MASK, val); > > + > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_GPIO_ALARM_H_MASK, > > + val); > > + case hwmon_in_min_alarm: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_alarm(st, > > LTC4282_ADC_ALERT_LOG, > > + > > LTC4282_VSOURCE_ALARM_L_MASK, val); > > + > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_GPIO_ALARM_L_MASK, > > + val); > > + case hwmon_in_alarm: > > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, > > + LTC4282_FET_BAD_STATUS_MASK, > > val); > > + case hwmon_in_crit_alarm: > > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, > > LTC4282_OV_STATUS_MASK, val); > > + case hwmon_in_lcrit_alarm: > > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, > > LTC4282_UV_STATUS_MASK, val); > > + case hwmon_in_max: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_voltage_byte(st, > > LTC4282_VSOURCE_MAX, > > + st->vfs_out, val); > > + > > + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MAX, > > 1280, val); > > + case hwmon_in_min: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_read_voltage_byte(st, > > LTC4282_VSOURCE_MIN, > > + st->vfs_out, val); > > + > > + return ltc4282_read_voltage_byte(st, LTC4282_VGPIO_MIN, > > 1280, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_read_current_word(const struct ltc4282_state *st, u32 > > reg, long *val) > > +{ > > + long in; > > + int ret; > > + > > + /* > > + * We pass in full scale in 10 * micro (note that 40 is already > > millivolt) so we > > + * have better approximations to calculate current. > > + */ > > + ret = ltc4282_read_voltage_word(st, reg, DECA * 40 * MILLI, &in); > > + if (ret) > > + return ret; > > + > > + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read_current_byte(const struct ltc4282_state *st, u32 > > reg, long *val) > > +{ > > + long in; > > + int ret; > > + > > + ret = ltc4282_read_voltage_byte(st, reg, DECA * 40 * MILLI, &in); > > + if (ret) > > + return ret; > > + > > + *val = DIV_ROUND_CLOSEST(in * MILLI, st->rsense); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read_curr(struct device *dev, const u32 attr, long *val) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_curr_input: > > + return ltc4282_read_current_word(st, LTC4282_VSENSE, val); > > + case hwmon_curr_highest: > > + return ltc4282_read_current_word(st, > > LTC4282_VSENSE_HIGHEST, val); > > + case hwmon_curr_lowest: > > + return ltc4282_read_current_word(st, LTC4282_VSENSE_LOWEST, > > val); > > + case hwmon_curr_max: > > + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MAX, > > val); > > + case hwmon_curr_min: > > + return ltc4282_read_current_byte(st, LTC4282_VSENSE_MIN, > > val); > > + case hwmon_curr_max_alarm: > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_VSENSE_ALARM_H_MASK, > > + val); > > + case hwmon_curr_min_alarm: > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_VSENSE_ALARM_L_MASK, > > + val); > > + case hwmon_curr_crit_alarm: > > + return ltc4282_read_alarm(st, LTC4282_STATUS_LSB, > > LTC4282_OC_STATUS_MASK, val); > > + default: > > + return -ENOTSUPP; > > checkpatch: > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP > > Please run checkpatch --strict on your patches. > Bahh, I'm pretty sure I ran it but forgot --strict > > + } > > +} > > + > > +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, > > long *val) > > +{ > > + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2; > > + __be16 raw; > > + u16 power; > > + int ret; > > + > > + ret = regmap_bulk_read(st->map, reg, &raw, sizeof(raw)); > > + if (ret) > > + return ret; > > + > > + power = be16_to_cpu(raw); > > + /* > > + * Power is given by: > > + * P = CODE(16b) * 0.040 * Vfs(out) * 2^16 / ((2^16 - 1)^2 * > > Rsense) > > + */ > > + if (check_mul_overflow(power * temp, MICRO, &temp_2)) { > > + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX); > > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * > > (u64)st->rsense); > > + return 0; > > + } > > + > > + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * > > int_pow(U16_MAX, 2)); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read_power_byte(const struct ltc4282_state *st, u32 reg, > > long *val) > > +{ > > + u32 power; > > + u64 temp; > > + int ret; > > + > > + ret = regmap_read(st->map, reg, &power); > > + if (ret) > > + return ret; > > + > > + temp = power * 40 * DECA * st->vfs_out * 256ULL; > > + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * > > st->rsense); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read_energy(const struct ltc4282_state *st, u64 *val) > > +{ > > + u64 temp, energy; > > + u32 status; > > + __be64 raw; > > + int ret; > > + > > + ret = regmap_bulk_read(st->map, LTC4282_ENERGY, &raw, 6); > > + if (ret) > > + return ret; > > + > > + ret = regmap_read(st->map, LTC4282_STATUS_MSB, &status); > > + if (ret) > > + return ret; > > + > > + if (status & LTC4282_METER_TICK_OVERFLOW_MASK) { > > + dev_warn(st->dev, "Got overflow in meter/ticker > > counters\n"); > > This is a big no-go. A normal operation must not generate any messages, > much less warnings. > > > + /* > > + * This resets the meter and the tick counter and holds > > them > > + * reset. > > + */ > > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, > > + LTC4282_METER_RESET_MASK); > > + if (ret) > > + return ret; > > + > > + /* start accumulating again */ > > + ret = regmap_clear_bits(st->map, LTC4282_ADC_CTRL, > > + LTC4282_METER_RESET_MASK); > > + if (ret) > > + return ret; > > + > > + /* > > + * Let the callers know a reset happened. Important when > > calling > > + * from power1_average. > > + */ > > Only I won't accept in-kernel calculated power average value attributes. > If the chip does not report averages, they must be calculated in > userspace. I see... So in userspace we won't have the "TIME COUNTER" value used for the average calculation (as per DS). You mean, userspace could just do simple averaging on the power values read? Anyways, I can drop this and with that (as mentioned in the cover) I can also drop the overflow handling. It will make things much simpler and is trivial for userspace to detect an energy overflow (the chip automatically rolls over the meter counter). > > > + ret = 1; > > + } > > + > > + energy = be64_to_cpu(raw) >> 16; > > + /* > > + * The formula for energy is given by: > > + * E = CODE(48b) * 0.040 * Vfs(out) * Tconv * 256 / ((2^16 - > > 1)^2 * Rsense) > > + * > > + * Since we only support 12bit ADC, Tconv = 0.065535s. Passing > > Vfs(out) and 0.040 to > > + * mV and Tconv to us, we can simplify the formula to: > > + * E = CODE(48b) * 40 * Vfs(out) * 256 / (U16_MAX * Rsense) > > + * > > + * As Rsense is in tens of micro-ohm, we need to multiply by DECA > > to get > > + * microujoule. > > + */ > > + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, > > &temp)) { > > + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, > > U16_MAX); > > + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense); > > + return ret; > > return 0 > > > + } > > + > > + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense); > > + > > + return ret; > > return 0 > > > +} > > + > > +static int ltc4282_read_power_average(struct ltc4282_state *st, long *val) > > +{ > > + u64 energy, temp;+static int ltc4282_probe(struct i2c_client *i2c) > > +{ > > + struct device *dev = &i2c->dev, *hwmon; > > + struct ltc4282_state *st; > > + int ret; > > + > > + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); > > + if (!st) > > + return dev_err_probe(dev, -ENOMEM, "Failed to allocate memory\n"); > > + > > + st->dev = dev; > > + st->map = devm_regmap_init_i2c(i2c, <c4282_regmap_config); > > + if (IS_ERR(st->map)) > > + return dev_err_probe(dev, PTR_ERR(st->map), "failed regmap init\n"); > > + > > + ret = ltc4282_power_on(st); > > + if (ret) > > + return ret; > > + > > + /* Soft reset */ > > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, LTC4282_RESET_MASK); > > + if (ret) > > + return ret; > > + > > + msleep(3200); > > + > > + ret = ltc428_clks_setup(st); > > + if (ret) > > + return ret; > > + > > + ret = ltc4282_setup(st); > > + if (ret) > > + return ret; > > + > > + mutex_init(&st->lock); > > + hwmon = devm_hwmon_device_register_with_info(dev, "ltc4282", st, > > <c2947_chip_info, > > + ltc4282_groups); > > + return PTR_ERR_OR_ZERO(hwmon); > > +} > > + > > +static const struct of_device_id ltc4282_of_match[] = { > > + { .compatible = "adi,ltc4282" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, ltc4282_of_match); > > + > > +static struct i2c_driver ltc4282_driver = { > > + .driver = { > > + .name = "ltc4282", > > + .of_match_table = ltc4282_of_match, > > + }, > > + .probe = ltc4282_probe, > > +}; > > +module_i2c_driver(ltc4282_driver); > > + > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > > +MODULE_DESCRIPTION("LTC4282 I2C High Current Hot Swap Controller"); > > +MODULE_LICENSE("GPL"); > > > + __be32 raw; > > + u32 count; > > + int ret; > > + > > + guard(mutex)(&st->lock); > > + > > + ret = ltc4282_read_energy(st, &energy); > > + if (ret < 0) > > + return ret; > > + if (ret == 1) { > > + /* > > + * reset happened... let's read the new energy value that > > + * together with the new tick counter should give a sane > > average > > + * value. Furthermore, we save whatever value we had > > accumulated > > + * so that the next energy read will have it into account. > > + */ > > + st->saved_energy = energy; > > + /* give some time for accumulation... */ > > + msleep(2 * LTC4282_TCONV_US / MILLI); > > + ret = ltc4282_read_energy(st, &energy); > > + if (ret < 0) > > + return ret; > > + } > > + > > + ret = regmap_bulk_read(st->map, LTC4282_TIME_COUNTER, &raw, > > sizeof(raw)); > > + if (ret) > > + return ret; > > + > > + count = be32_to_cpu(raw); > > + if (!count) { > > + *val = 0; > > + return 0; > > + } > > + > > + /* > > + * AVG = E / (Tconv * counter) > > + * We get energy in microJoule, hence dividing it by microSeconds > > gives Watts. Therefore, > > + * multiplying by MICRO gives us microWatts. > > + */ > > + if (check_mul_overflow(energy, MICRO, &temp)) { > > + temp = DIV_ROUND_CLOSEST_ULL(energy, LTC4282_TCONV_US); > > + *val = DIV_ROUND_CLOSEST_ULL(temp * MICRO, count); > > + return 0; > > + } > > + > > + *val = DIV64_U64_ROUND_CLOSEST(temp, LTC4282_TCONV_US * > > (u64)count); > > + return 0; > > +} > > No. If the chip doesn't report power averages, calculate it in userspace. > > > + > > +static int ltc4282_read_power(struct device *dev, const u32 attr, long > > *val) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_power_input: > > + return ltc4282_read_power_word(st, LTC4282_POWER, val); > > + case hwmon_power_input_highest: > > + return ltc4282_read_power_word(st, LTC4282_POWER_HIGHEST, > > val); > > + case hwmon_power_input_lowest: > > + return ltc4282_read_power_word(st, LTC4282_POWER_LOWEST, > > val); > > + case hwmon_power_max_alarm: > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_POWER_ALARM_H_MASK, > > + val); > > + case hwmon_power_min_alarm: > > + return ltc4282_read_alarm(st, LTC4282_ADC_ALERT_LOG, > > LTC4282_POWER_ALARM_L_MASK, > > + val); > > + case hwmon_power_average: > > + return ltc4282_read_power_average(st, val); > > + case hwmon_power_max: > > + return ltc4282_read_power_byte(st, LTC4282_POWER_MAX, val); > > + case hwmon_power_min: > > + return ltc4282_read_power_byte(st, LTC4282_POWER_MIN, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_energy_enable(struct device *dev, long *val) > > +{ > > + const struct ltc4282_state *st = dev_get_drvdata(dev); > > + u32 reg_val; > > + int ret; > > + > > + ret = regmap_read(st->map, LTC4282_ADC_CTRL, ®_val); > > + if (ret) > > + return ret; > > + > > + *val = !FIELD_GET(LTC4282_METER_HALT_MASK, reg_val); > > + > > + return 0; > > +} > > + > > +static int ltc4282_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_in: > > + return ltc4282_read_in(dev, attr, val, channel); > > + case hwmon_curr: > > + return ltc4282_read_curr(dev, attr, val); > > + case hwmon_power: > > + return ltc4282_read_power(dev, attr, val); > > + case hwmon_energy: > > + return ltc4282_energy_enable(dev, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_write_power_byte(const struct ltc4282_state *st, u32 > > reg, long val) > > +{ > > + u32 power; > > + u64 temp; > > + > > + if (val > st->power_max) > > + val = st->power_max; > > + > > + temp = val * int_pow(U8_MAX, 2) * st->rsense; > > + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st- > > >vfs_out * 40); > > + > > + return regmap_write(st->map, reg, power); > > +} > > + > > +static int ltc4282_write_power(const struct ltc4282_state *st, u32 attr, > > + long val) > > +{ > > + switch (attr) { > > + case hwmon_power_max: > > + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, > > val); > > + case hwmon_power_min: > > + return ltc4282_write_power_byte(st, LTC4282_POWER_MIN, > > val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_write_voltage_byte(const struct ltc4282_state *st, u32 > > reg, u32 fs, long val) > > +{ > > + u32 in; > > + > > + if (val >= fs) > > + in = U8_MAX; > > + else > > + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs); > > + > > + return regmap_write(st->map, reg, in); > > +} > > + > > +static int ltc4282_write_in(const struct ltc4282_state *st, u32 attr, long > > val, > > + int channel) > > +{ > > + switch (attr) { > > + case hwmon_in_max: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_write_voltage_byte(st, > > LTC4282_VSOURCE_MAX, > > + st->vfs_out, > > val); > > + > > + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MAX, > > 1280, val); > > + case hwmon_in_min: > > + if (channel == LTC4282_CHAN_VSOURCE) > > + return ltc4282_write_voltage_byte(st, > > LTC4282_VSOURCE_MIN, > > + st->vfs_out, > > val); > > + > > + return ltc4282_write_voltage_byte(st, LTC4282_VGPIO_MIN, > > 1280, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_write_curr(const struct ltc4282_state *st, u32 attr, > > + long val) > > +{ > > + /* need to pass it in millivolt */ > > + u32 in = DIV_ROUND_CLOSEST_ULL((u64)val * st->rsense, DECA * > > MICRO); > > + > > + switch (attr) { > > + case hwmon_curr_max: > > + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, > > 40, in); > > + case hwmon_curr_min: > > + return ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MIN, > > 40, in); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_write(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + const struct ltc4282_state *st = dev_get_drvdata(dev); > > + > > + switch (type) { > > + case hwmon_power: > > + return ltc4282_write_power(st, attr, val); > > + case hwmon_in: > > + return ltc4282_write_in(st, attr, val, channel); > > + case hwmon_curr: > > + return ltc4282_write_curr(st, attr, val); > > + case hwmon_energy: > > + /* setting the bit halts the meter */ > > + return regmap_update_bits(st->map, LTC4282_ADC_CTRL, > > + LTC4282_METER_HALT_MASK, > > + > > FIELD_PREP(LTC4282_METER_HALT_MASK, !!!val)); > > > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int ltc4282_in_is_visible(const struct ltc4282_state *st, u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_in_input: > > + case hwmon_in_highest: > > + case hwmon_in_lowest: > > + case hwmon_in_max_alarm: > > + case hwmon_in_min_alarm: > > + case hwmon_in_label: > > + case hwmon_in_lcrit_alarm: > > + case hwmon_in_crit_alarm: > > + return 0444; > > + case hwmon_in_max: > > + case hwmon_in_min: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static int ltc4282_curr_is_visible(u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_curr_input: > > + case hwmon_curr_highest: > > + case hwmon_curr_lowest: > > + case hwmon_curr_max_alarm: > > + case hwmon_curr_min_alarm: > > + case hwmon_curr_crit_alarm: > > + case hwmon_curr_label: > > + return 0444; > > + case hwmon_curr_max: > > + case hwmon_curr_min: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static int ltc4282_power_is_visible(u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_power_input: > > + case hwmon_power_input_highest: > > + case hwmon_power_input_lowest: > > + case hwmon_power_label: > > + case hwmon_power_max_alarm: > > + case hwmon_power_min_alarm: > > + case hwmon_power_average: > > + return 0444; > > + case hwmon_power_max: > > + case hwmon_power_min: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t ltc4282_is_visible(const void *data, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_in: > > + return ltc4282_in_is_visible(data, attr); > > + case hwmon_curr: > > + return ltc4282_curr_is_visible(attr); > > + case hwmon_power: > > + return ltc4282_power_is_visible(attr); > > + case hwmon_energy: > > + /* hwmon_energy_enable */ > > + return 0644; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static const char * const ltc4282_in_strs[] = { > > + "VSOURCE", "VDD", "VGPIO" > > +}; > > + > > +static int ltc4282_read_labels(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, const char **str) > > +{ > > + switch (type) { > > + case hwmon_in: > > + *str = ltc4282_in_strs[channel]; > > + return 0; > > + case hwmon_curr: > > + *str = "ISENSE"; > > + return 0; > > + case hwmon_power: > > + *str = "Power"; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static ssize_t ltc4282_show_energy(struct device *dev, > > + struct device_attribute *da, char *buf) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + u64 energy; > > + long en; > > + int ret; > > + > > + guard(mutex)(&st->lock); > > + > > + ret = ltc4282_energy_enable(dev, &en); > > + if (ret) > > + return ret; > > + if (!en) > > + return -ENODATA; > > + > > This should be stored in a flag. Reading it from the chip for each > energy reading is too expensive. will do... > > > + ret = ltc4282_read_energy(st, &energy); > > + if (ret < 0) > > + return ret; > > + > > + /* see @ltc4282_read_power_average */ > > + if (st->saved_energy) { > > + energy += st->saved_energy; > > + st->saved_energy = 0; > > + } > > + > > + return sysfs_emit(buf, "%llu\n", energy); > > +} > > + > > +static ssize_t ltc4282_show_fault(struct device *dev, > > + struct device_attribute *da, > > + u32 reg, char *buf) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); > > + long alarm; > > + int ret; > > + > > + ret = ltc4282_read_alarm(st, reg, attr->index, &alarm); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%lu\n", alarm); > > +} > > + > > +static ssize_t ltc4282_show_status(struct device *dev, > > + struct device_attribute *da, char *buf) > > +{ > > + return ltc4282_show_fault(dev, da, LTC4282_STATUS_LSB, buf); > > +} > > + > > +static ssize_t ltc4282_show_fault_log(struct device *dev, > > + struct device_attribute *da, char > > *buf) > > +{ > > + return ltc4282_show_fault(dev, da, LTC4282_FAULT_LOG, buf); > > +} > > + > > +static ssize_t ltc4282_clear_fault_log(struct device *dev, > > + struct device_attribute *da, > > + const char *buf, size_t len) > > +{ > > + struct ltc4282_state *st = dev_get_drvdata(dev); > > + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); > > + int ret; > > + > > + ret = regmap_clear_bits(st->map, LTC4282_FAULT_LOG, attr->index); > > + if (ret) > > + return ret; > > + > > + return len; > > +} > > + > > +static int ltc4282_power_on(const struct ltc4282_state *st) > > +{ > > + u32 n_tries = 5, reg; > > + int ret; > > + > > + ret = devm_regulator_get_enable(st->dev, "vdd"); > > + if (ret) > > + return dev_err_probe(st->dev, ret, "Failed vdd > > get/enable\n"); > > + > > + /* > > + * Make sure vdd is stable. From the datasheet: > > + * The state of the UV and OV comparators is indicated by the > > STATUS register > > + * bits 0 and 1 and must be stable for at least 50ms to qualify for > > turn-on. > > + */ > > + do { > > + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, > > reg, > > + reg & > > LTC4282_VDD_STATUS_MASK, 10000, 50000); > > + if (!ret) > > + continue; > > + if (ret != -ETIMEDOUT) > > + return dev_err_probe(st->dev, ret, "Failed regmap > > read\n"); > > + > > + /* Alright. We got timeout which means UV and OV are stable > > for 50ms */ > > + break; > > + } while (n_tries--); > > + > > + if (!n_tries) > > + return dev_err_probe(st->dev, -ETIMEDOUT, "VDD not > > stable\n"); > > + > > + return 0; > > +} > > > This is a hot swap controller. Turning it on in its hardware monitoring > driver is highly inappropriate. I don't know where and how it is supposed > to be turned on, but that can not be done here. Also, turning it off when > unloading the driver (presumably that will happen since it is turned on > with a device managed function) is even more inappropriate. > > It is even more inappropriate to delay the probe function by at least 50 ms > just to check if two status register bits changed state. > Ohh yeah... I see now its completely inappropriate even more when unloading the driver. Will drop this. > > + > > +enum { > > + LTC4282_CLKOUT_INT, > > + LTC4282_CLKOUT_TICK, > > +}; > > + > > +static int ltc428_clks_setup(const struct ltc4282_state *st) > > +{ > > + unsigned long rate; > > + struct clk *clkin; > > + u32 val; > > + int ret; > > + > > + ret = device_property_read_u32(st->dev, "adi,clkout-mode", &val); > > + if (ret) > > + return 0; > > + > > + if (val > LTC4282_CLKOUT_TICK) > > + return dev_err_probe(st->dev, -EINVAL, > > + "Invalid val(%u) for adi,clkout- > > mode\n", val); > > + > > + ret = regmap_update_bits(st->map, LTC4282_CLK_DIV, > > LTC4282_CLKOUT_MASK, > > + FIELD_PREP(LTC4282_CLKOUT_MASK, val + 1)); > > + if (ret) > > + return ret; > > + > > If the chip generates a clock output, the driver needs to register a clock > device. Ohh, I forgot to mention this in the cover. I did thought about it but the chip is only capable of outputting either the conversion time or the internal system clock. So, I figured this would be to specific to matter to any in kernel consumers. Anyways, I can add support for it and maybe always round to the closest frequency someone asks for (again, the chip only supports outputting two frequencies). Or maybe keep the attr and have a kind of "read only" clk? > > > + clkin = devm_clk_get_optional_enabled(st->dev, NULL); > > + if (IS_ERR(clkin)) > > + return dev_err_probe(st->dev, PTR_ERR(clkin), "Failed to > > get clkin"); > > + if (!clkin) > > + return 0; > > + > > + rate = clk_get_rate(clkin); > > + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX) > > + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin > > range(%lu) [%lu %lu]\n", > > + rate, LTC4282_CLKIN_MIN, > > LTC4282_CLKIN_MAX); > > + > > + /* > > + * Clocks faster than 250KHZ should be reduced to 250KHZ. The clock > > frequency > > + * is divided by twice the value in the register. > > + */ > > + val = rate / (2 * LTC4282_CLKIN_MIN); > > + > > + return regmap_update_bits(st->map, LTC4282_CLK_DIV, > > LTC4282_CLK_DIV_MASK, > > + FIELD_PREP(LTC4282_CLK_DIV_MASK, val)); > > +} > > + > > +static const int ltc4282_curr_lim_uv[] = { > > + 12500, 15625, 18750, 21875, 25000, 28125, 31250, 34375 > > +}; > > + > > +static int ltc4282_get_defaults(struct ltc4282_state *st, u32 *curr_lim_uv) > > +{ > > + u32 reg_val, ilm_adjust; > > + int ret; > > + > > + ret = regmap_read(st->map, LTC4282_CTRL_MSB, ®_val); > > + if (ret) > > + return ret; > > + > > + st->vin_mode = FIELD_GET(LTC4282_CTRL_VIN_MODE_MASK, reg_val); > > + > > + ret = regmap_read(st->map, LTC4282_ILIM_ADJUST, ®_val); > > + if (ret) > > + return ret; > > + > > + ilm_adjust = FIELD_GET(LTC4282_ILIM_ADJUST_MASK, reg_val); > > + *curr_lim_uv = ltc4282_curr_lim_uv[ilm_adjust]; > > + > > + return 0; > > +} > > + > > +/* > > + * Set max limits for ISENSE and Power as that depends on the max voltage > > on rsense > > + * that is defined in ILIM_ADJUST. This is specially important for power > > because for > > + * some rsense and vfsout values, if we allow the default raw 255 value, > > that would > > + * overflow long in 32bit archs when reading back the max power limit. > > + */ > > Wouldn't it make more sense to limit rsense to reasonable values instead ? The thing is, what would be that reasonable value? I honestly have no idea. The problem with the overflow is that the default value in the max_alarm register is 255. And with that, even for a reasonable value of rsense, we could get an overflow. Given the above, we could just defer handling for usersapce (meaning, they should write a reasonable value in max_alarm) or keep the below. And the below sounded like a good idea because, at this stage, we do have all the info to calculate the theoretical max_power (we have the max output voltage and current). > > > +static int ltc4282_set_max_limits(struct ltc4282_state *st, u32 val_uv) > > +{ > > + int ret; > > + > > + ret = ltc4282_write_voltage_byte(st, LTC4282_VSENSE_MAX, 40 * > > MILLI, val_uv); > > + if (ret) > > + return ret; > > + > > + /* Power is given by ISENSE * Vout. */ > > + st->power_max = DIV_ROUND_CLOSEST(val_uv * DECA * MILLI, st- > > >rsense) * st->vfs_out; > > + return ltc4282_write_power_byte(st, LTC4282_POWER_MAX, st- > > >power_max); > > +} > > + > > +/* valid GPIO functions */ > > +enum { > > + LTC4282_PIN_GPIO, > > + /* Power functions only for GPIO_1*/ > > + LTC4282_PIN_POWER_BAD, > > + LTC4282_PIN_POWER_GOOD, > > + /* ADC monitor only for GPIO_2 and 3 */ > > + LTC4282_PIN_ADC = 2, > > + /* Only for GPIO_2 */ > > + LTC4282_PIN_FET_STRESS, > > +}; > > + > > +static int ltc4282_non_gpio_setup(struct ltc4282_state *st, u32 pin, u32 > > func, bool *adc_in) > > +{ > > + if (pin == LTC4282_GPIO_1) { > > + u32 val = LTC4282_PIN_POWER_BAD; > > + > > + if (func == LTC4282_PIN_POWER_GOOD) > > + val = 0; > > + > > + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > > LTC4282_GPIO_1_CONFIG_MASK, > > + > > FIELD_PREP(LTC4282_GPIO_1_CONFIG_MASK, val)); > > + } > > + > > + if (func == LTC4282_PIN_FET_STRESS) > > + return regmap_update_bits(st->map, LTC4282_GPIO_CONFIG, > > + LTC4282_GPIO_2_FET_STRESS_MASK, > > + > > FIELD_PREP(LTC4282_GPIO_2_FET_STRESS_MASK, 1)); > > + > > + /* > > + * Then, let's point the given GPIO to the ADC input. We need to > > ensure that > > + * function is only given once. > > + */ > > + if (*adc_in) > > + return dev_err_probe(st->dev, -EINVAL, > > + "Only one gpio can be given to the ADC > > input\n"); > > + > > + *adc_in = true; > > + > > + /* setting the bit to 1 cause the ADC to monitor GPIO2 */ > > + return regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, > > LTC4282_GPIO_MODE_MASK, > > + FIELD_PREP(LTC4282_GPIO_MODE_MASK, pin == > > LTC4282_GPIO_2)); > > +} > > + > > +static const char * const ltc4282_gpio_prop[] = { > > + "adi,gpio1-mode", "adi,gpio2-mode", "adi,gpio3-mode" > > +}; > > + > > +static int ltc4282_gpio_setup(struct ltc4282_state *st) > > +{ > > + struct device *dev = st->dev; > > + u32 gpio, func, ngpios = 0; > > + bool adc_in = false; > > + int ret; > > + > > + if (!IS_ENABLED(CONFIG_GPIOLIB)) > > + return 0; > > + > > + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) { > > + ret = device_property_read_u32(dev, > > ltc4282_gpio_prop[gpio], &func); > > + if (ret) > > + continue; > > + if (func >= ltc4282_gpios[gpio].n_funcs) > > + return dev_err_probe(dev, ret, "Invalid func(%u >= > > %u) for gpio%u\n", > > + func, > > ltc4282_gpios[gpio].n_funcs, gpio + 1); > > + if (func == LTC4282_PIN_GPIO) { > > + st->gpio_map[ngpios++] = gpio; > > + if (gpio == LTC4282_GPIO_1) { > > + /* default to input GPIO */ > > + ret = regmap_set_bits(st->map, > > LTC4282_GPIO_CONFIG, > > + > > LTC4282_GPIO_1_CONFIG_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + continue; > > + } > > + > > + ret = ltc4282_non_gpio_setup(st, gpio, func, &adc_in); > > + if (ret) > > + return ret; > > + } > > + > > + if (device_property_read_bool(dev, "adi,gpio-alert")) > > + st->gpio_map[ngpios++] = LTC4282_ALERT; > > + > > + if (!ngpios) > > + return 0; > > + > > + st->gc.parent = dev; > > + st->gc.base = -1; > > + st->gc.ngpio = ngpios; > > + st->gc.can_sleep = true; > > + st->gc.label = "ltc4282"; > > + st->gc.direction_input = ltc4282_gpio_input_set; > > + st->gc.direction_output = ltc4282_gpio_output_set; > > + st->gc.set = ltc4282_gpio_set; > > + st->gc.get = ltc4282_gpio_get; > > + > > + return devm_gpiochip_add_data(dev, &st->gc, st); > > +} > > + > > +/* This maps the Vout full scale for the given Vin mode */ > > +static const u16 ltc4282_vfs_milli[] = { 5540, 8320, 16640, 33280 }; > > + > > +enum { > > + LTC4282_DIV_EXTERNAL, > > + LTC4282_DIV_5_PERCENT, > > + LTC4282_DIV_10_PERCENT, > > + LTC4282_DIV_15_PERCENT, > > +}; > > + > > +static int ltc4282_setup(struct ltc4282_state *st) > > +{ > > + struct device *dev = st->dev; > > + u32 val, curr_lim_uv; > > + int ret; > > + > > + /* The part has an eeprom so let's get the needed defaults from it > > */ > > + ret = ltc4282_get_defaults(st, &curr_lim_uv); > > + if (ret) > > + return ret; > > + > > + ret = device_property_read_u32(dev, "adi,rsense-nano-ohms", &st- > > >rsense); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to read adi,rsense- > > nano-ohms\n"); > > + > > + /* > > + * The resolution for rsense is tens of micro which means we need > > nano in the bindings. > > Assuming this is supposed to mean "tenths of micro", not "tens of micro", do > such sense > resistors really exist in practice ? The smallest I was able to find has 25 > micro-ohm. > Oh my bad. What I mean is that we can have values like 62.5 micro-ohm and approximating it for these systems adds reasonable approx errors. > > + * However, to make things easier to handle (with respect to > > overflows) we divide it by > > + * 100 as we don't really need the last two digits. > > + */ > > + st->rsense /= CENTI; > > + > > This blindly accepts rsense values of < 100 which results in st->rsense == 0 > and > division by zero crashes. Right... > > > + ret = device_property_read_u32(dev, "vin-mode-microvolt", &val); > > No adi, prefix ? will fix > Anyway, this is really the input voltage range, or rather the maximum accepted > input voltage. > Hmm so is this not a valid property? Should I just user regulators to get VDD? I know already that I should not enable/disable it :) > > > + if (!ret) { > > + switch (val) { > > + case 3300000: > > + st->vin_mode = LTC4282_VIN_3_3V; > > + break; > > + case 5000000: > > + st->vin_mode = LTC4282_VIN_5V; > > + break; > > + case 12000000: > > + st->vin_mode = LTC4282_VIN_12V; > > + break; > > + case 24000000: > > + st->vin_mode = LTC4282_VIN_24V; > > + break; > > + default: > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid val(%u) for vin-mode- > > microvolt\n", val); > > + } > > + > > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, > > LTC4282_CTRL_VIN_MODE_MASK, > > + > > FIELD_PREP(LTC4282_CTRL_VIN_MODE_MASK, st->vin_mode)); > > + if (ret) > > + return ret; > > + > > + /* Foldback mode should also be set to the input voltage */ > > + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, > > LTC4282_FOLDBACK_MODE_MASK, > > + > > FIELD_PREP(LTC4282_FOLDBACK_MODE_MASK, st->vin_mode)); > > + if (ret) > > + return ret; > > + } > > + > > + st->vfs_out = ltc4282_vfs_milli[st->vin_mode]; > > + > > + ret = device_property_read_u32(dev, "adi,current-limit-microvolt", > > + &curr_lim_uv); > > Current is measured in Ampere. Yes, that translates to volt through the sense > resistor, > but it is still Ampere. No idea how that is normally handled in devicetree. Yes, but this translates directly from the datasheet and what's used in the device. Otherwise, to give this in current, I would need rsense to translate in voltage. I could change the name but this uses the terminology used the datasheet. I mean, there we have "current limit sense voltage". So maybe, 'adi,current-limit-sense-microvolt'? > > > + if (!ret) { > > + int reg_val; > > + > > + switch (val) { > > + case 12500: > > + reg_val = 0; > > + break; > > + case 15625: > > + reg_val = 1; > > + break; > > + case 18750: > > + reg_val = 2; > > + break; > > + case 21875: > > + reg_val = 3; > > + break; > > + case 25000: > > + reg_val = 4; > > + break; > > + case 28125: > > + reg_val = 5; > > + break; > > + case 31250: > > + reg_val = 6; > > + break; > > + case 34375: > > + reg_val = 7; > > + break; > > + default: > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid val(%u) for > > adi,current-limit-microvolt\n", > > + val); > > + } > > + > > + ret = regmap_update_bits(st->map, LTC4282_ILIM_ADJUST, > > LTC4282_ILIM_ADJUST_MASK, > > + > > FIELD_PREP(LTC4282_ILIM_ADJUST_MASK, reg_val)); > > + if (ret) > > + return ret; > > + } > > + > > + ret = ltc4282_set_max_limits(st, curr_lim_uv); > > + if (ret) > > + return ret; > > + > > + ret = device_property_read_u32(st->dev, "adi,overvoltage-dividers", > > &val); > > + if (!ret) { > > + if (val > LTC4282_DIV_15_PERCENT) > > + return dev_err_probe(st->dev, -EINVAL, > > + "Invalid val(%u) for > > adi,overvoltage-divider\n", val); > > + > > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, > > LTC4282_CTRL_OV_MODE_MASK, > > + > > FIELD_PREP(LTC4282_CTRL_OV_MODE_MASK, val)); > > + } > > + > > + ret = device_property_read_u32(st->dev, "adi,undervoltage- > > dividers", &val); > > + if (!ret) { > > + if (val > LTC4282_DIV_15_PERCENT) > > + return dev_err_probe(st->dev, -EINVAL, > > + "Invalid val(%u) for > > adi,undervoltage-divider\n", val); > > + > > + ret = regmap_update_bits(st->map, LTC4282_CTRL_MSB, > > LTC4282_CTRL_UV_MODE_MASK, > > + > > FIELD_PREP(LTC4282_CTRL_UV_MODE_MASK, val)); > > + } > > + > > + if (device_property_read_bool(dev, "adi,overcurrent-retry")) { > > + ret = regmap_set_bits(st->map, LTC4282_CTRL_LSB, > > LTC4282_CTRL_OC_RETRY_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + if (device_property_read_bool(dev, "adi,overvoltage-retry- > > disable")) { > > + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, > > LTC4282_CTRL_OV_RETRY_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + if (device_property_read_bool(dev, "adi,undervoltage-retry- > > disable")) { > > + ret = regmap_clear_bits(st->map, LTC4282_CTRL_LSB, > > LTC4282_CTRL_UV_RETRY_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + if (device_property_read_bool(dev, "adi,fault-log-enable")) { > > + ret = regmap_set_bits(st->map, LTC4282_ADC_CTRL, > > LTC4282_FAULT_LOG_EN_MASK); > > + if (ret) > > + return ret; > > + } > > + > > + ret = device_property_read_u32(dev, "adi,fet-bad-timeout-ms", > > &val); > > + if (!ret) { > > + if (val > LTC4282_FET_BAD_MAX_TIMEOUT) > > + return dev_err_probe(dev, -EINVAL, > > + "Invalid value(%u) for > > adi,fet-bad-timeout-ms", val); > > + > > + ret = regmap_write(st->map, LTC4282_FET_BAD_FAULT_TIMEOUT, > > val); > > + if (ret) > > + return ret; > > + } > > + > > + return ltc4282_gpio_setup(st); > > +} > > + > > +static bool ltc4282_readable_reg(struct device *dev, unsigned int reg) > > +{ > > + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) > > + return false; > > + > > + return true; > > +} > > + > > +static bool ltc4282_writable_reg(struct device *dev, unsigned int reg) > > +{ > > + if (reg == LTC4282_STATUS_LSB || reg == LTC4282_STATUS_MSB) > > + return false; > > + if (reg == LTC4282_RESERVED_1 || reg == LTC4282_RESERVED_2) > > + return false; > > + > > + return true; > > +} > > + > > +static const struct regmap_config ltc4282_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = LTC4282_RESERVED_3, > > + .readable_reg = ltc4282_readable_reg, > > + .writeable_reg = ltc4282_writable_reg, > > +}; > > + > > +static const struct hwmon_channel_info * const ltc4282_info[] = { > > + HWMON_CHANNEL_INFO(in, > > + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST > > | > > + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | > > + HWMON_I_MAX_ALARM | HWMON_I_LABEL, > > + HWMON_I_INPUT | HWMON_I_LCRIT_ALARM | > > HWMON_I_CRIT_ALARM | > > + HWMON_I_LABEL, > > + HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_HIGHEST > > | > > + HWMON_I_MAX | HWMON_I_MIN | HWMON_I_MIN_ALARM | > > + HWMON_I_MAX_ALARM | HWMON_I_LABEL), > > + HWMON_CHANNEL_INFO(curr, > > + HWMON_C_INPUT | HWMON_C_LOWEST | HWMON_C_HIGHEST > > | > > + HWMON_C_MAX | HWMON_C_MIN | HWMON_C_MIN_ALARM | > > + HWMON_C_MAX_ALARM | HWMON_C_CRIT_ALARM | > > + HWMON_C_LABEL), > > + HWMON_CHANNEL_INFO(power, > > + HWMON_P_INPUT | HWMON_P_INPUT_LOWEST | > > + HWMON_P_INPUT_HIGHEST | HWMON_P_MAX | > > HWMON_P_MIN | > > + HWMON_P_AVERAGE | HWMON_P_MAX_ALARM | > > HWMON_P_MIN_ALARM | > > + HWMON_P_LABEL), > > + HWMON_CHANNEL_INFO(energy, > > + HWMON_E_ENABLE), > > + NULL > > +}; > > + > > +static const struct hwmon_ops ltc4282_hwmon_ops = { > > + .read = ltc4282_read, > > + .write = ltc4282_write, > > + .is_visible = ltc4282_is_visible, > > + .read_string = ltc4282_read_labels, > > +}; > > + > > +static const struct hwmon_chip_info ltc2947_chip_info = { > > + .ops = <c4282_hwmon_ops, > > + .info = ltc4282_info, > > +}; > > + > > +/* energy attributes are 6bytes wide so we need u64 */ > > +static SENSOR_DEVICE_ATTR(energy1_input, 0444, ltc4282_show_energy, NULL, > > 0); > > +/* power1_fault */ > > +static SENSOR_DEVICE_ATTR(power1_good, 0444, ltc4282_show_status, NULL, > > + LTC4282_POWER_GOOD_MASK); > > +/* FET faults */ > > +static SENSOR_DEVICE_ATTR(fet_short_fault, 0444, ltc4282_show_status, NULL, > > + LTC4282_FET_SHORT_MASK); > > +static SENSOR_DEVICE_ATTR(fet_bad_fault, 0444, ltc4282_show_status, NULL, > > + LTC4282_FET_BAD_STATUS_MASK); > > +/* > > + * Fault log failures. These faults might be important in systems where > > auto-retry is not enabled > > + * since they will cause the part to latch off until they are cleared. > > Typically that happens > > + * when the system admin is close enough so he can check what happened and > > manually clear the > > + * faults. Moreover, manually clearing the faults might only matter when > > ON_FAULT_MASK in the > > + * CONTROL register is set (which is the default) as in that case, a turn > > off signal from the > > + * ON pin won't clear them. > > + */ > > I'd argue that such an operator would only be interested in clearing all > faults > at the same time. Your call, but all this does is making life hard for > operators. Hmm, So you mean having a single attr like 'fault_logs_reset? And having the single logs in debugFS? > > > +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_POWER_BAD_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_FET_BAD_FAULT_MASK); > > +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, > > ltc4282_show_fault_log, > > + ltc4282_clear_fault_log, > > LTC4282_FET_SHORT_FAULT_MASK); > > + > > +static struct attribute *ltc4282_attrs[] = { > > + &sensor_dev_attr_energy1_input.dev_attr.attr, > > + &sensor_dev_attr_power1_good.dev_attr.attr, > > + &sensor_dev_attr_fet_bad_fault.dev_attr.attr, > > + &sensor_dev_attr_fet_short_fault.dev_attr.attr, > > + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr, > > + &sensor_dev_attr_power1_fault_log.dev_attr.attr, > > + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr, > > + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr, > > + NULL, > > +}; > > +ATTRIBUTE_GROUPS(ltc4282); > > + > > Move all those except for energy1_input to debugfs. > I want only to mention 'power1_good'. Is not a fault log but a status bit. I mention it in the cover as I'm not so sure about is "added value". I even though about exporting it as 'power1_fault' but that's not ok because that bit being is 0 does not necessarily means something is wrong. - Nuno Sá > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa ` (2 preceding siblings ...) 2023-11-11 17:22 ` Guenter Roeck @ 2023-11-20 12:10 ` Dan Carpenter 2023-11-20 14:52 ` Guenter Roeck 3 siblings, 1 reply; 17+ messages in thread From: Dan Carpenter @ 2023-11-20 12:10 UTC (permalink / raw) To: oe-kbuild, Nuno Sa, linux-hwmon, linux-doc, devicetree Cc: lkp, oe-kbuild-all, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Guenter Roeck, Nuno Sa, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley Hi Nuno, kernel test robot noticed the following build warnings: https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa/dt-bindings-hwmon-Add-LTC4282-bindings/20231110-232017 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231110151905.1659873-3-nuno.sa%40analog.com patch subject: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip config: i386-randconfig-141-20231112 (https://download.01.org/0day-ci/archive/20231113/202311130219.YBiF8Xu3-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20231113/202311130219.YBiF8Xu3-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Reported-by: Dan Carpenter <error27@gmail.com> | Closes: https://lore.kernel.org/r/202311130219.YBiF8Xu3-lkp@intel.com/ smatch warnings: drivers/hwmon/ltc4282.c:859 ltc4282_is_visible() warn: signedness bug returning '(-524)' drivers/hwmon/ltc4282.c:1143 ltc4282_gpio_setup() warn: passing zero to 'dev_err_probe' vim +859 drivers/hwmon/ltc4282.c 31ea069ddf481b Nuno Sa 2023-11-10 844 static umode_t ltc4282_is_visible(const void *data, 31ea069ddf481b Nuno Sa 2023-11-10 845 enum hwmon_sensor_types type, 31ea069ddf481b Nuno Sa 2023-11-10 846 u32 attr, int channel) 31ea069ddf481b Nuno Sa 2023-11-10 847 { 31ea069ddf481b Nuno Sa 2023-11-10 848 switch (type) { 31ea069ddf481b Nuno Sa 2023-11-10 849 case hwmon_in: 31ea069ddf481b Nuno Sa 2023-11-10 850 return ltc4282_in_is_visible(data, attr); 31ea069ddf481b Nuno Sa 2023-11-10 851 case hwmon_curr: 31ea069ddf481b Nuno Sa 2023-11-10 852 return ltc4282_curr_is_visible(attr); 31ea069ddf481b Nuno Sa 2023-11-10 853 case hwmon_power: 31ea069ddf481b Nuno Sa 2023-11-10 854 return ltc4282_power_is_visible(attr); 31ea069ddf481b Nuno Sa 2023-11-10 855 case hwmon_energy: 31ea069ddf481b Nuno Sa 2023-11-10 856 /* hwmon_energy_enable */ 31ea069ddf481b Nuno Sa 2023-11-10 857 return 0644; 31ea069ddf481b Nuno Sa 2023-11-10 858 default: 31ea069ddf481b Nuno Sa 2023-11-10 @859 return -ENOTSUPP; This function returns umode_t (which must be unsigned and smaller than int for this warning to trigger). 31ea069ddf481b Nuno Sa 2023-11-10 860 } 31ea069ddf481b Nuno Sa 2023-11-10 861 } -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip 2023-11-20 12:10 ` Dan Carpenter @ 2023-11-20 14:52 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2023-11-20 14:52 UTC (permalink / raw) To: Dan Carpenter, oe-kbuild, Nuno Sa, linux-hwmon, linux-doc, devicetree Cc: lkp, oe-kbuild-all, Bartosz Golaszewski, Jonathan Corbet, Krzysztof Kozlowski, Linus Walleij, Rob Herring, Andy Shevchenko, Jean Delvare, Conor Dooley On 11/20/23 04:10, Dan Carpenter wrote: > Hi Nuno, > > kernel test robot noticed the following build warnings: > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa/dt-bindings-hwmon-Add-LTC4282-bindings/20231110-232017 > base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next > patch link: https://lore.kernel.org/r/20231110151905.1659873-3-nuno.sa%40analog.com > patch subject: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip > config: i386-randconfig-141-20231112 (https://download.01.org/0day-ci/archive/20231113/202311130219.YBiF8Xu3-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce: (https://download.01.org/0day-ci/archive/20231113/202311130219.YBiF8Xu3-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <error27@gmail.com> > | Closes: https://lore.kernel.org/r/202311130219.YBiF8Xu3-lkp@intel.com/ > > smatch warnings: > drivers/hwmon/ltc4282.c:859 ltc4282_is_visible() warn: signedness bug returning '(-524)' > drivers/hwmon/ltc4282.c:1143 ltc4282_gpio_setup() warn: passing zero to 'dev_err_probe' > > vim +859 drivers/hwmon/ltc4282.c > > 31ea069ddf481b Nuno Sa 2023-11-10 844 static umode_t ltc4282_is_visible(const void *data, > 31ea069ddf481b Nuno Sa 2023-11-10 845 enum hwmon_sensor_types type, > 31ea069ddf481b Nuno Sa 2023-11-10 846 u32 attr, int channel) > 31ea069ddf481b Nuno Sa 2023-11-10 847 { > 31ea069ddf481b Nuno Sa 2023-11-10 848 switch (type) { > 31ea069ddf481b Nuno Sa 2023-11-10 849 case hwmon_in: > 31ea069ddf481b Nuno Sa 2023-11-10 850 return ltc4282_in_is_visible(data, attr); > 31ea069ddf481b Nuno Sa 2023-11-10 851 case hwmon_curr: > 31ea069ddf481b Nuno Sa 2023-11-10 852 return ltc4282_curr_is_visible(attr); > 31ea069ddf481b Nuno Sa 2023-11-10 853 case hwmon_power: > 31ea069ddf481b Nuno Sa 2023-11-10 854 return ltc4282_power_is_visible(attr); > 31ea069ddf481b Nuno Sa 2023-11-10 855 case hwmon_energy: > 31ea069ddf481b Nuno Sa 2023-11-10 856 /* hwmon_energy_enable */ > 31ea069ddf481b Nuno Sa 2023-11-10 857 return 0644; > 31ea069ddf481b Nuno Sa 2023-11-10 858 default: > 31ea069ddf481b Nuno Sa 2023-11-10 @859 return -ENOTSUPP; > > This function returns umode_t (which must be unsigned and smaller than > int for this warning to trigger). > Yes, this is an is_visible() function which should return 0 if the attribute isn't visible. Guenter ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-11-20 15:03 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-10 15:18 [PATCH 0/2] Add support for LTC4282 Nuno Sa 2023-11-10 15:18 ` [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings Nuno Sa 2023-11-10 18:42 ` Conor Dooley 2023-11-13 9:32 ` Nuno Sá 2023-11-13 20:12 ` Conor Dooley 2023-11-20 15:03 ` Guenter Roeck 2023-11-10 15:18 ` [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip Nuno Sa 2023-11-10 16:50 ` Andy Shevchenko 2023-11-13 10:13 ` Nuno Sá 2023-11-13 16:31 ` Andy Shevchenko 2023-11-14 8:36 ` Nuno Sá 2023-11-20 15:00 ` Guenter Roeck 2023-11-11 1:04 ` kernel test robot 2023-11-11 17:22 ` Guenter Roeck 2023-11-13 9:24 ` Nuno Sá 2023-11-20 12:10 ` Dan Carpenter 2023-11-20 14:52 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).