* [PATCH v3 0/2] adding support for Microchip PAC194X Power Monitor
@ 2025-06-06 9:39 marius.cristea
2025-06-06 9:39 ` [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X marius.cristea
2025-06-06 9:39 ` [PATCH v3 2/2] " marius.cristea
0 siblings, 2 replies; 18+ messages in thread
From: marius.cristea @ 2025-06-06 9:39 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy
Cc: robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree,
linux-kernel, marius.cristea
From: Marius Cristea <marius.cristea@microchip.com>
Adding support for Microchip PAC194X and PAC195X series of Power Monitor
with Accumulator chip family. This driver covers the following part
numbers:
- PAC1941, PAC1941_2, PAC1942, PAC1942_2, PAC1943, PAC1944
- PAC1951, PAC1951_2, PAC1952, PAC1952_2, PAC1953, PAC1954
The PAC194X family supports 9V Full-Scale Range and the PAC195X supports
32V Full-Scale Range.
There are two versions of the PAC194X/5X: the PAC194X-1/5X-1 devices are
for high-side current sensing and the PAC194X/5X-2 devices are for low-side
current sensing or floating VBUS applications. The PAC194X/5X-1 is named
shortly PAC194X/5X.
Differences related to previous patch:
v3:
- fix review comments device tree binding:
rewrite commit message
change the way full scale for voltage and current is set. Add a
properties to describe if the input is bipolar or not.
fix the "dtschema/dtc warnings/errors"
- fix review comments driver:
simplify the driver to include just the basic functionality. More
features will be added later.
fix coding style issues
change to lower case the "to_pac1944_chip_info()" to be more like
other container_of
remove the PAC1944_DEV_ATTR() macro
drop the __func__ bit from error messages
remove unneeded casts
change the logic to reset the accumulator when is enabled
change from {} to { }
remove unreachable() from the code
rewrite the code to keep the error paths out of line
replace scoped_guard() with guard(), where was possible
use to_delayed_work() to get from work to delayed work
remove active_channels[] array
print info message in case FW disagrees with what is found on the bus
v2:
- fix review comments device tree binding:
remove underscore from names
add names to the interrupts and list them
add a better description for new properties
- fix review comments driver:
fix coding style issues
use bitmap for checking the active channels
keep the "pac1944_get_unaligned_be56" here because the change wasn't acceted
into the asm-generic.
document new added attributes
remove the "scan" part till we support buffered capture
remove "unlikely" marking
add masks up in some array of const structures to avoid some case statements
remove pac1944_mutex_destroy function
replace some functions with a macro (just for testing)
replace dev_err with dev_err_probe in functions used in pac1944_probe
v1:
- first version committed to review
Marius Cristea (2):
dt-bindings: iio: adc: adding support for PAC194X
iio: adc: adding support for PAC194X
.../ABI/testing/sysfs-bus-iio-adc-pac1944 | 17 +
.../bindings/iio/adc/microchip,pac1944.yaml | 204 ++
MAINTAINERS | 7 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/pac1944.c | 2841 +++++++++++++++++
6 files changed, 3082 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944
create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml
create mode 100644 drivers/iio/adc/pac1944.c
base-commit: 0c86e33819785fe50616b6ee3fb35c1e4be406d5
--
2.48.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-06 9:39 [PATCH v3 0/2] adding support for Microchip PAC194X Power Monitor marius.cristea @ 2025-06-06 9:39 ` marius.cristea 2025-06-06 15:53 ` David Lechner 2025-06-06 16:24 ` Conor Dooley 2025-06-06 9:39 ` [PATCH v3 2/2] " marius.cristea 1 sibling, 2 replies; 18+ messages in thread From: marius.cristea @ 2025-06-06 9:39 UTC (permalink / raw) To: jic23, dlechner, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel, marius.cristea From: Marius Cristea <marius.cristea@microchip.com> This is the device tree schema for iio driver for Microchip PAC194X and PAC195X series of Power Monitors with Accumulator. The PAC194X family supports 9V Full-Scale Range and the PAC195X supports 32V Full-Scale Range. There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices are for high-side current sensing and the PAC194X/5X-2 devices are for low-side current sensing or floating VBUS applications. The PAC194X/5X-1 is named shortly PAC194X/5X. Signed-off-by: Marius Cristea <marius.cristea@microchip.com> --- .../bindings/iio/adc/microchip,pac1944.yaml | 204 ++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml new file mode 100644 index 000000000000..4a2cf6b64055 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml @@ -0,0 +1,204 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1944.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip PAC1944 and PAC1954 Power Monitors with Accumulator + +maintainers: + - Marius Cristea <marius.cristea@microchip.com> + +description: | + This device is part of the Microchip family of Power Monitors with + Accumulator. The datasheet for PAC1941-1, PAC1941-1, PAC1942-1, PAC1942-2, + PAC1943-1 and PAC1944-1 can be found here: + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf + The datasheet for PAC1951-1, PAC1951-1, PAC1952-1, PAC1952-2, PAC1953-1 and + PAC1954-1 can be found here: + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf + +properties: + compatible: + enum: + - microchip,pac1941 + - microchip,pac19412 + - microchip,pac1942 + - microchip,pac19422 + - microchip,pac1943 + - microchip,pac1944 + - microchip,pac1951 + - microchip,pac19512 + - microchip,pac1952 + - microchip,pac19522 + - microchip,pac1953 + - microchip,pac1954 + + reg: + maxItems: 1 + + vdd-supply: true + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + interrupts: + maxItems: 2 + + interrupt-names: + description: + alert1 indicates a HIGH or LOW limit was exceeded. + alert2 indicates a THERM limit was exceeded. + items: + - const: alert1 + - const: alert2 + +patternProperties: + "^channel@[1-4]+$": + type: object + $ref: adc.yaml + description: + Represents the external channels which are connected to the ADC. + + properties: + reg: + items: + minimum: 1 + maximum: 4 + + shunt-resistor-micro-ohms: + description: + Value in micro Ohms of the shunt resistor connected between + the SENSE+ and SENSE- inputs, across which the current is measured. + Value is needed to compute the scaling of the measured current. + + microchip,vbus-half-range: + $ref: /schemas/types.yaml#/definitions/flag + description: | + In order to increase measurement resolution and keeping the same + number the of bits the device has a configurable VBUS full range scale + (FSR). The range should be set by hardware design and it should not be + changed during runtime. The bipolar capability for VBUS enables + accurate offset measurement and correction. + The VBUS could be configured into the following full scale range: + - VBUS has unipolar 0V to 32V FSR (default) for PAC195X or 0V to 9V + (default) for PAC194X. + - VBUS has bipolar -32V to 32V FSR for PAC195X or -9V to 9V for + PAC194X. The actual range is limited to about -200 mV due to the + impact of the ESD structures. + - VBUS has bipolar -16V to 16V FSR for PAC195X or -4.5V to 4.5V for + PAC194X. The actual range is limited to about -200 mV due to the + impact of the ESD structures. + + microchip,vbus-bipolar: + $ref: /schemas/types.yaml#/definitions/flag + description: + If provided, the channel is to be used in bipolar mode. The + actual range is limited to about -200 mV due to the impact of the ESD + structures. + + microchip,vsense-half-range: + $ref: /schemas/types.yaml#/definitions/flag + description: | + In order to decrease the power dissipation on the shunt resistor and + in the same time to increase measurement resolution by keeping the + same number the of bits the device has a configurable VSENSE full + range scale (FSR). The range should be set by hardware design and it + should not be changed during runtime. + The VSENSE could be configured into the following full scale range: + - VSENSE has unipolar 0V to 100 mV FSR (default) + - VSENSE has bipolar -100 mV to 100 mV FSR + - VSENSE has bipolar -50 mV to 50 mV FSR + + microchip,vsense-bipolar: + $ref: /schemas/types.yaml#/definitions/flag + description: + If provided, the channel is to be used in bipolar mode. + + microchip,accumulation-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + The Hardware Accumulator may be used to accumulate VPOWER, VSENSE or + VBUS values for any channel. By setting the accumulator for a channel + to accumulate the VPOWER values gives a measure of accumulated power + into a time period, which is equivalent to energy. Setting the + accumulator for a channel to accumulate VSENSE values gives a measure + of accumulated current, which is equivalent to charge. This allows the + accumulator to be used as a coulomb counter. For either VSENSE or + VBUS, many samples may be accumulated on chip and the result collected + by the host and divided by the accumulator counter count value to + yield an average value with a very long integration time to reduce + noise. This feature is also very useful for system calibration, + allowing many averages to be accumulated for fast averaging/noise + reduction. + This functionality needs to be setup once and must not be changed + during the runtime, just in case the user wants to measure the charge + or the energy consumed from board power up till the user has control + or during a reboot of the system. + The Hardware Accumulator could be configured to accumulate + VPOWER, VSENSE or VBUS + <0> - Accumulator accumulates VPOWER (default) + <1> - Accumulator accumulates VSENSE + <2> - Accumulator accumulates VBUS + maximum: 2 + default: 0 + + required: + - reg + - shunt-resistor-micro-ohms + + unevaluatedProperties: false + +required: + - compatible + - reg + - vdd-supply + - "#address-cells" + - "#size-cells" + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + power-monitor@10 { + compatible = "microchip,pac1954"; + reg = <0x10>; + vdd-supply = <&vdd>; + + #address-cells = <1>; + #size-cells = <0>; + + channel@1 { + reg = <0x1>; + shunt-resistor-micro-ohms = <24900>; + label = "CPU"; + microchip,vsense-half-range; + microchip,vsense-bipolar; + }; + + channel@3 { + reg = <0x3>; + shunt-resistor-micro-ohms = <75000>; + label = "MEM"; + microchip,vbus-half-range; + microchip,vbus-bipolar; + microchip,vsense-half-range; + }; + + channel@4 { + reg = <0x4>; + shunt-resistor-micro-ohms = <100000>; + label = "NET"; + microchip,vbus-bipolar; + }; + }; + }; + +... -- 2.48.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-06 9:39 ` [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X marius.cristea @ 2025-06-06 15:53 ` David Lechner 2025-06-10 14:46 ` Marius.Cristea 2025-06-06 16:24 ` Conor Dooley 1 sibling, 1 reply; 18+ messages in thread From: David Lechner @ 2025-06-06 15:53 UTC (permalink / raw) To: marius.cristea, jic23, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel On 6/6/25 4:39 AM, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the device tree schema for iio driver for Microchip PAC194X > and PAC195X series of Power Monitors with Accumulator. The PAC194X > family supports 9V Full-Scale Range and the PAC195X supports 32V > Full-Scale Range. > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > are for high-side current sensing and the PAC194X/5X-2 devices are > for low-side current sensing or floating VBUS applications. > > The PAC194X/5X-1 is named shortly PAC194X/5X. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- > .../bindings/iio/adc/microchip,pac1944.yaml | 204 ++++++++++++++++++ > 1 file changed, 204 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > new file mode 100644 > index 000000000000..4a2cf6b64055 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > @@ -0,0 +1,204 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1944.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip PAC1944 and PAC1954 Power Monitors with Accumulator > + > +maintainers: > + - Marius Cristea <marius.cristea@microchip.com> > + > +description: | > + This device is part of the Microchip family of Power Monitors with > + Accumulator. The datasheet for PAC1941-1, PAC1941-1, PAC1942-1, PAC1942-2, > + PAC1943-1 and PAC1944-1 can be found here: > + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf > + The datasheet for PAC1951-1, PAC1951-1, PAC1952-1, PAC1952-2, PAC1953-1 and > + PAC1954-1 can be found here: > + https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf > + > +properties: > + compatible: > + enum: > + - microchip,pac1941 > + - microchip,pac19412 > + - microchip,pac1942 > + - microchip,pac19422 > + - microchip,pac1943 > + - microchip,pac1944 > + - microchip,pac1951 > + - microchip,pac19512 > + - microchip,pac1952 > + - microchip,pac19522 > + - microchip,pac1953 > + - microchip,pac1954 > + > + reg: > + maxItems: 1 > + > + vdd-supply: true > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + interrupts: > + maxItems: 2 > + > + interrupt-names: Needs minItems: 1 if we want to allow a single named interrupt. > + description: > + alert1 indicates a HIGH or LOW limit was exceeded. > + alert2 indicates a THERM limit was exceeded. > + items: > + - const: alert1 > + - const: alert2 > + I am having deja vu. I just commented on an identical interrupts binding in a different series [1]. In this case though, alert1 and alert2 are the actual pin names, so that is fine. But each pin can be programmed to indicate lots of different things, so drop descriptions or change them to describe the pin, not an arbitrary function. I don't even see THERM in the datasheet, so I'm guessing that was just a copy/ paste from something else anyway. [1]: https://lore.kernel.org/linux-iio/0f68e3f9-cba5-4df3-8e56-2cccbccf35ce@baylibre.com/ --- Even if the driver doesn't use them (yet), we could consider adding gpio-controller and #gpio-cells properties since these chips have pins that can operate as GPIOs. And we could add a powerdown-gpios property for the /PWRDN pin. We want to try to make the bindings as complete as possible, if we can [2]. [2]: https://docs.kernel.org/devicetree/bindings/writing-bindings.html > +patternProperties: > + "^channel@[1-4]+$": Drop the +. Only 1 to 4 are allowed, not 11, 111, etc. Also, we could further restrict things based on the actual number of channels on a chip like this: allOf: - if: properties: compatible: pattern: "^pac19[45]1" then: properties: channel@1: reg: items: maximum: 1 patternProperties: ^channel@[2-4]$": false - if: properties: compatible: pattern: "^pac19[45]2" then: patternProperties: ^channel@[1-2]$": reg: items: maximum: 2 patternProperties: ^channel@[3-4]$": false - if: properties: compatible: pattern: "^pac19[45]3" then: patternProperties: ^channel@[1-3]$": reg: items: maximum: 3 properties: channel@4: false > + type: object > + $ref: adc.yaml > + description: > + Represents the external channels which are connected to the ADC. > + > + properties: > + reg: > + items: > + minimum: 1 > + maximum: 4 > + > + shunt-resistor-micro-ohms: > + description: > + Value in micro Ohms of the shunt resistor connected between > + the SENSE+ and SENSE- inputs, across which the current is measured. > + Value is needed to compute the scaling of the measured current. > + > + microchip,vbus-half-range: > + $ref: /schemas/types.yaml#/definitions/flag > + description: | > + In order to increase measurement resolution and keeping the same > + number the of bits the device has a configurable VBUS full range scale > + (FSR). The range should be set by hardware design and it should not be > + changed during runtime. The bipolar capability for VBUS enables > + accurate offset measurement and correction. > + The VBUS could be configured into the following full scale range: > + - VBUS has unipolar 0V to 32V FSR (default) for PAC195X or 0V to 9V > + (default) for PAC194X. > + - VBUS has bipolar -32V to 32V FSR for PAC195X or -9V to 9V for > + PAC194X. The actual range is limited to about -200 mV due to the > + impact of the ESD structures. > + - VBUS has bipolar -16V to 16V FSR for PAC195X or -4.5V to 4.5V for > + PAC194X. The actual range is limited to about -200 mV due to the > + impact of the ESD structures. > + > + microchip,vbus-bipolar: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If provided, the channel is to be used in bipolar mode. The > + actual range is limited to about -200 mV due to the impact of the ESD > + structures. > + Using Jonathan's suggestion from v2 to just have a single property with 3 different ranges to chose from seems simpler that this. It would only require one property and would be self-documenting. The description could be shortened to just a couple of lines. Otherwise, we also need to add: - if: required: microchip,vbus-half-range then: required: microchip,vbus-bipolar to validate that that there are only 3 possibilities. Also, swapping the word order to range-half would be more consistent with the existing adi,range-double property that serves a similar purpose. Same applies to vsense. > + microchip,vsense-half-range: > + $ref: /schemas/types.yaml#/definitions/flag > + description: | > + In order to decrease the power dissipation on the shunt resistor and > + in the same time to increase measurement resolution by keeping the > + same number the of bits the device has a configurable VSENSE full > + range scale (FSR). The range should be set by hardware design and it > + should not be changed during runtime. > + The VSENSE could be configured into the following full scale range: > + - VSENSE has unipolar 0V to 100 mV FSR (default) > + - VSENSE has bipolar -100 mV to 100 mV FSR > + - VSENSE has bipolar -50 mV to 50 mV FSR > + > + microchip,vsense-bipolar: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + If provided, the channel is to be used in bipolar mode. > + > + microchip,accumulation-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + The Hardware Accumulator may be used to accumulate VPOWER, VSENSE or > + VBUS values for any channel. By setting the accumulator for a channel > + to accumulate the VPOWER values gives a measure of accumulated power > + into a time period, which is equivalent to energy. Setting the > + accumulator for a channel to accumulate VSENSE values gives a measure > + of accumulated current, which is equivalent to charge. This allows the > + accumulator to be used as a coulomb counter. For either VSENSE or > + VBUS, many samples may be accumulated on chip and the result collected > + by the host and divided by the accumulator counter count value to > + yield an average value with a very long integration time to reduce > + noise. This feature is also very useful for system calibration, > + allowing many averages to be accumulated for fast averaging/noise > + reduction. > + This functionality needs to be setup once and must not be changed > + during the runtime, Why not? The datasheet says there is a REFRESH command to allow changing it at runtime. > just in case the user wants to measure the charge > + or the energy consumed from board power up till the user has control > + or during a reboot of the system. > + The Hardware Accumulator could be configured to accumulate > + VPOWER, VSENSE or VBUS > + <0> - Accumulator accumulates VPOWER (default) > + <1> - Accumulator accumulates VSENSE > + <2> - Accumulator accumulates VBUS > + maximum: 2 > + default: 0 > + > + required: > + - reg > + - shunt-resistor-micro-ohms > + > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - vdd-supply > + - "#address-cells" > + - "#size-cells" > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + power-monitor@10 { > + compatible = "microchip,pac1954"; > + reg = <0x10>; > + vdd-supply = <&vdd>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + channel@1 { > + reg = <0x1>; > + shunt-resistor-micro-ohms = <24900>; > + label = "CPU"; > + microchip,vsense-half-range; > + microchip,vsense-bipolar; > + }; Seems odd to leave a channel unconfigured since the shunt resistor value is required and there is a 3 channel version of the chip that could be used if only 3 channels were wired up. > + > + channel@3 { > + reg = <0x3>; > + shunt-resistor-micro-ohms = <75000>; > + label = "MEM"; > + microchip,vbus-half-range; > + microchip,vbus-bipolar; > + microchip,vsense-half-range; > + }; > + > + channel@4 { > + reg = <0x4>; > + shunt-resistor-micro-ohms = <100000>; > + label = "NET"; > + microchip,vbus-bipolar; > + }; > + }; > + }; > + > +... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-06 15:53 ` David Lechner @ 2025-06-10 14:46 ` Marius.Cristea 2025-06-10 15:22 ` David Lechner 0 siblings, 1 reply; 18+ messages in thread From: Marius.Cristea @ 2025-06-10 14:46 UTC (permalink / raw) To: jic23, nuno.sa, dlechner, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel Hi David, Thank you for the feedback. Please see my comments below... On Fri, 2025-06-06 at 10:53 -0500, David Lechner wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 6/6/25 4:39 AM, marius.cristea@microchip.com wrote: > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the device tree schema for iio driver for Microchip PAC194X > > and PAC195X series of Power Monitors with Accumulator. The PAC194X > > family supports 9V Full-Scale Range and the PAC195X supports 32V > > Full-Scale Range. > > > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > > are for high-side current sensing and the PAC194X/5X-2 devices are > > for low-side current sensing or floating VBUS applications. > > > > The PAC194X/5X-1 is named shortly PAC194X/5X. > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > --- > > .../bindings/iio/adc/microchip,pac1944.yaml | 204 > > ++++++++++++++++++ > > 1 file changed, 204 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > > new file mode 100644 > > index 000000000000..4a2cf6b64055 > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > > @@ -0,0 +1,204 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/adc/microchip,pac1944.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Microchip PAC1944 and PAC1954 Power Monitors with > > Accumulator > > + > > +maintainers: > > + - Marius Cristea <marius.cristea@microchip.com> > > + > > +description: | > > + This device is part of the Microchip family of Power Monitors > > with > > + Accumulator. The datasheet for PAC1941-1, PAC1941-1, PAC1942-1, > > PAC1942-2, > > + PAC1943-1 and PAC1944-1 can be found here: > > + > > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf > > + The datasheet for PAC1951-1, PAC1951-1, PAC1952-1, PAC1952-2, > > PAC1953-1 and > > + PAC1954-1 can be found here: > > + > > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf > > + > > +properties: > > + compatible: > > + enum: > > + - microchip,pac1941 > > + - microchip,pac19412 > > + - microchip,pac1942 > > + - microchip,pac19422 > > + - microchip,pac1943 > > + - microchip,pac1944 > > + - microchip,pac1951 > > + - microchip,pac19512 > > + - microchip,pac1952 > > + - microchip,pac19522 > > + - microchip,pac1953 > > + - microchip,pac1954 > > + > > + reg: > > + maxItems: 1 > > + > > + vdd-supply: true > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 0 > > + > > + interrupts: > > + maxItems: 2 > > + > > + interrupt-names: > > Needs minItems: 1 if we want to allow a single named interrupt. > the driver as it is right now it doesn't support any interrupt. I was thinking to add them here, just in case there will be a request to be added later. > > + description: > > + alert1 indicates a HIGH or LOW limit was exceeded. > > + alert2 indicates a THERM limit was exceeded. > > + items: > > + - const: alert1 > > + - const: alert2 > > + > > I am having deja vu. I just commented on an identical interrupts > binding > in a different series [1]. In this case though, alert1 and alert2 are > the actual pin names, so that is fine. But each pin can be programmed > to indicate lots of different things, so drop descriptions or change > them to describe the pin, not an arbitrary function. I don't even > see THERM in the datasheet, so I'm guessing that was just a copy/ > paste from something else anyway. > sorry here, I will fix it. > [1]: > https://lore.kernel.org/linux-iio/0f68e3f9-cba5-4df3-8e56-2cccbccf35ce@baylibre.com/ > > > --- > > Even if the driver doesn't use them (yet), we could consider adding > gpio-controller and #gpio-cells properties since these chips have > pins > that can operate as GPIOs. > > And we could add a powerdown-gpios property for the /PWRDN pin. > > We want to try to make the bindings as complete as possible, if we > can [2]. > > [2]: > https://docs.kernel.org/devicetree/bindings/writing-bindings.html > > > +patternProperties: > > + "^channel@[1-4]+$": > > Drop the +. Only 1 to 4 are allowed, not 11, 111, etc. > > Also, we could further restrict things based on the actual number of > channels on a chip like this: > > allOf: > - if: > properties: > compatible: > pattern: "^pac19[45]1" > then: > properties: > channel@1: > reg: > items: > maximum: 1 > patternProperties: > ^channel@[2-4]$": false > - if: > properties: > compatible: > pattern: "^pac19[45]2" > then: > patternProperties: > ^channel@[1-2]$": > reg: > items: > maximum: 2 > patternProperties: > ^channel@[3-4]$": false > - if: > properties: > compatible: > pattern: "^pac19[45]3" > then: > patternProperties: > ^channel@[1-3]$": > reg: > items: > maximum: 3 > properties: > channel@4: false > > > > > + type: object > > + $ref: adc.yaml > > + description: > > + Represents the external channels which are connected to the > > ADC. > > + > > + properties: > > + reg: > > + items: > > + minimum: 1 > > + maximum: 4 > > + > > + shunt-resistor-micro-ohms: > > + description: > > + Value in micro Ohms of the shunt resistor connected > > between > > + the SENSE+ and SENSE- inputs, across which the current > > is measured. > > + Value is needed to compute the scaling of the measured > > current. > > + > > + microchip,vbus-half-range: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: | > > + In order to increase measurement resolution and keeping > > the same > > + number the of bits the device has a configurable VBUS > > full range scale > > + (FSR). The range should be set by hardware design and it > > should not be > > + changed during runtime. The bipolar capability for VBUS > > enables > > + accurate offset measurement and correction. > > + The VBUS could be configured into the following full > > scale range: > > + - VBUS has unipolar 0V to 32V FSR (default) for > > PAC195X or 0V to 9V > > + (default) for PAC194X. > > + - VBUS has bipolar -32V to 32V FSR for PAC195X or -9V > > to 9V for > > + PAC194X. The actual range is limited to about -200 > > mV due to the > > + impact of the ESD structures. > > + - VBUS has bipolar -16V to 16V FSR for PAC195X or - > > 4.5V to 4.5V for > > + PAC194X. The actual range is limited to about -200 > > mV due to the > > + impact of the ESD structures. > > + > > + microchip,vbus-bipolar: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > + If provided, the channel is to be used in bipolar mode. > > The > > + actual range is limited to about -200 mV due to the > > impact of the ESD > > + structures. > > + > > Using Jonathan's suggestion from v2 to just have a single property > with 3 different > ranges to chose from seems simpler that this. It would only require > one property > and would be self-documenting. The description could be shortened to > just a couple > of lines. I was thinking to add the range for this property, but it looks (for me at least) more complicated from the checking point of view. The driver is supporting two family of devices that has, each, 3 different voltage range as an input. > > Otherwise, we also need to add: > > - if: > required: > microchip,vbus-half-range > then: > required: > microchip,vbus-bipolar > > to validate that that there are only 3 possibilities. Those properties was just an alternative solution to the range. If we agree that this is the way forward, I will add the check. > > Also, swapping the word order to range-half would be more consistent > with > the existing adi,range-double property that serves a similar purpose. > > Same applies to vsense. > > > + microchip,vsense-half-range: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: | > > + In order to decrease the power dissipation on the shunt > > resistor and > > + in the same time to increase measurement resolution by > > keeping the > > + same number the of bits the device has a configurable > > VSENSE full > > + range scale (FSR). The range should be set by hardware > > design and it > > + should not be changed during runtime. > > + The VSENSE could be configured into the following full > > scale range: > > + - VSENSE has unipolar 0V to 100 mV FSR (default) > > + - VSENSE has bipolar -100 mV to 100 mV FSR > > + - VSENSE has bipolar -50 mV to 50 mV FSR > > + > > + microchip,vsense-bipolar: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > + If provided, the channel is to be used in bipolar mode. > > + > > + microchip,accumulation-mode: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + The Hardware Accumulator may be used to accumulate > > VPOWER, VSENSE or > > + VBUS values for any channel. By setting the accumulator > > for a channel > > + to accumulate the VPOWER values gives a measure of > > accumulated power > > + into a time period, which is equivalent to energy. > > Setting the > > + accumulator for a channel to accumulate VSENSE values > > gives a measure > > + of accumulated current, which is equivalent to charge. > > This allows the > > + accumulator to be used as a coulomb counter. For either > > VSENSE or > > + VBUS, many samples may be accumulated on chip and the > > result collected > > + by the host and divided by the accumulator counter count > > value to > > + yield an average value with a very long integration time > > to reduce > > + noise. This feature is also very useful for system > > calibration, > > + allowing many averages to be accumulated for fast > > averaging/noise > > + reduction. > > + This functionality needs to be setup once and must not > > be changed > > + during the runtime, > > Why not? The datasheet says there is a REFRESH command to allow > changing it > at runtime. Yes sure, from the data sheet point of view it is possible to change the accumulation mode during the runtime. From the customer point of view once the channel has a certain functionality the operation mode will not be changed. E.g. once a channel is used as a coulomb counter nobody will change this operation mode in order not to loose any charging or discharging current for that channel. Also being only one hardware register per channel it will be hard to have enabled/available into the sysfs all the functionality and to "guess" the one that is working at a point in time. > > > just in case the user wants to measure the charge > > + or the energy consumed from board power up till the user > > has control > > + or during a reboot of the system. > > + The Hardware Accumulator could be configured to > > accumulate > > + VPOWER, VSENSE or VBUS > > + <0> - Accumulator accumulates VPOWER (default) > > + <1> - Accumulator accumulates VSENSE > > + <2> - Accumulator accumulates VBUS > > + maximum: 2 > > + default: 0 > > + > > + required: > > + - reg > > + - shunt-resistor-micro-ohms > > + > > + unevaluatedProperties: false > > + > > +required: > > + - compatible > > + - reg > > + - vdd-supply > > + - "#address-cells" > > + - "#size-cells" > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + power-monitor@10 { > > + compatible = "microchip,pac1954"; > > + reg = <0x10>; > > + vdd-supply = <&vdd>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + channel@1 { > > + reg = <0x1>; > > + shunt-resistor-micro-ohms = <24900>; > > + label = "CPU"; > > + microchip,vsense-half-range; > > + microchip,vsense-bipolar; > > + }; > > Seems odd to leave a channel unconfigured since the shunt resistor > value is required and there is a 3 channel version of the chip that > could be used if only 3 channels were wired up. Yes, indeed. The reason I was given this example was because, sometimes the client will use a chip with more channels that are needed in order to develop some other functions later but on the same hardware. E.g. a board that could monitor a battery charge/discharge current, but the same hardware/board on a more expensive product could also support a second battery that has a second charging monitoring channel. The parts are pin compatible and sometime the clients will buy the part with more channels and use only part of those channels. In order to save some power each channel could be enabled or disabled. > > > + > > + channel@3 { > > + reg = <0x3>; > > + shunt-resistor-micro-ohms = <75000>; > > + label = "MEM"; > > + microchip,vbus-half-range; > > + microchip,vbus-bipolar; > > + microchip,vsense-half-range; > > + }; > > + > > + channel@4 { > > + reg = <0x4>; > > + shunt-resistor-micro-ohms = <100000>; > > + label = "NET"; > > + microchip,vbus-bipolar; > > + }; > > + }; > > + }; > > + > > +... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-10 14:46 ` Marius.Cristea @ 2025-06-10 15:22 ` David Lechner 2025-06-10 16:04 ` Marius.Cristea 0 siblings, 1 reply; 18+ messages in thread From: David Lechner @ 2025-06-10 15:22 UTC (permalink / raw) To: Marius.Cristea, jic23, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On 6/10/25 9:46 AM, Marius.Cristea@microchip.com wrote: > Hi David, > > Thank you for the feedback. Please see my comments below... > ... >>> + interrupts: >>> + maxItems: 2 >>> + >>> + interrupt-names: >> >> Needs minItems: 1 if we want to allow a single named interrupt. >> > the driver as it is right now it doesn't support any interrupt. I was > thinking to add them here, just in case there will be a request to be > added later. > Making the bindings complete even if the driver isn't using it yet is the right thing to do. :-) I meant allowing just a single interrupt wired up though. So it doesn't matter how the driver would handle it. > >>> + description: >>> + alert1 indicates a HIGH or LOW limit was exceeded. >>> + alert2 indicates a THERM limit was exceeded. >>> + items: >>> + - const: alert1 >>> + - const: alert2 >>> + >> ... >>> + >>> + microchip,vbus-half-range: >>> + $ref: /schemas/types.yaml#/definitions/flag >>> + description: | >>> + In order to increase measurement resolution and keeping >>> the same >>> + number the of bits the device has a configurable VBUS >>> full range scale >>> + (FSR). The range should be set by hardware design and it >>> should not be >>> + changed during runtime. The bipolar capability for VBUS >>> enables >>> + accurate offset measurement and correction. >>> + The VBUS could be configured into the following full >>> scale range: >>> + - VBUS has unipolar 0V to 32V FSR (default) for >>> PAC195X or 0V to 9V >>> + (default) for PAC194X. >>> + - VBUS has bipolar -32V to 32V FSR for PAC195X or -9V >>> to 9V for >>> + PAC194X. The actual range is limited to about -200 >>> mV due to the >>> + impact of the ESD structures. >>> + - VBUS has bipolar -16V to 16V FSR for PAC195X or - >>> 4.5V to 4.5V for >>> + PAC194X. The actual range is limited to about -200 >>> mV due to the >>> + impact of the ESD structures. >>> + >>> + microchip,vbus-bipolar: >>> + $ref: /schemas/types.yaml#/definitions/flag >>> + description: >>> + If provided, the channel is to be used in bipolar mode. >>> The >>> + actual range is limited to about -200 mV due to the >>> impact of the ESD >>> + structures. >>> + >> >> Using Jonathan's suggestion from v2 to just have a single property >> with 3 different >> ranges to chose from seems simpler that this. It would only require >> one property >> and would be self-documenting. The description could be shortened to >> just a couple >> of lines. > > I was thinking to add the range for this property, but it looks (for me > at least) more complicated from the checking point of view. The driver > is supporting two family of devices that has, each, 3 different voltage > range as an input. > Usually, having a consistent binding for the same thing among similar devices is more important than how easy it is to implement in the driver. Since this seems to be a common pattern, we could probably justify an iio_property_match_ranges() helper function that would simplify the implementation in drivers that would need to use such a property. Then in each driver it would just be a matter of making a static const array lookup table of ranges for each device and calling the helper function. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-10 15:22 ` David Lechner @ 2025-06-10 16:04 ` Marius.Cristea 2025-06-10 16:35 ` David Lechner 0 siblings, 1 reply; 18+ messages in thread From: Marius.Cristea @ 2025-06-10 16:04 UTC (permalink / raw) To: jic23, nuno.sa, dlechner, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On Tue, 2025-06-10 at 10:22 -0500, David Lechner wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 6/10/25 9:46 AM, Marius.Cristea@microchip.com wrote: > > Hi David, > > > > Thank you for the feedback. Please see my comments below... > > > > ... > ... > > > > > + > > > > + microchip,vbus-half-range: > > > > + $ref: /schemas/types.yaml#/definitions/flag > > > > + description: | > > > > + In order to increase measurement resolution and > > > > keeping > > > > the same > > > > + number the of bits the device has a configurable > > > > VBUS > > > > full range scale > > > > + (FSR). The range should be set by hardware design > > > > and it > > > > should not be > > > > + changed during runtime. The bipolar capability for > > > > VBUS > > > > enables > > > > + accurate offset measurement and correction. > > > > + The VBUS could be configured into the following full > > > > scale range: > > > > + - VBUS has unipolar 0V to 32V FSR (default) for > > > > PAC195X or 0V to 9V > > > > + (default) for PAC194X. > > > > + - VBUS has bipolar -32V to 32V FSR for PAC195X or > > > > -9V > > > > to 9V for > > > > + PAC194X. The actual range is limited to about - > > > > 200 > > > > mV due to the > > > > + impact of the ESD structures. > > > > + - VBUS has bipolar -16V to 16V FSR for PAC195X or > > > > - > > > > 4.5V to 4.5V for > > > > + PAC194X. The actual range is limited to about - > > > > 200 > > > > mV due to the > > > > + impact of the ESD structures. > > > > + > > > > + microchip,vbus-bipolar: > > > > + $ref: /schemas/types.yaml#/definitions/flag > > > > + description: > > > > + If provided, the channel is to be used in bipolar > > > > mode. > > > > The > > > > + actual range is limited to about -200 mV due to the > > > > impact of the ESD > > > > + structures. > > > > + > > > > > > Using Jonathan's suggestion from v2 to just have a single > > > property > > > with 3 different > > > ranges to chose from seems simpler that this. It would only > > > require > > > one property > > > and would be self-documenting. The description could be shortened > > > to > > > just a couple > > > of lines. > > > > I was thinking to add the range for this property, but it looks > > (for me > > at least) more complicated from the checking point of view. The > > driver > > is supporting two family of devices that has, each, 3 different > > voltage > > range as an input. > > > > Usually, having a consistent binding for the same thing among similar > devices is more important than how easy it is to implement in the > driver. > > Since this seems to be a common pattern, we could probably justify an > iio_property_match_ranges() helper function that would simplify the > implementation in drivers that would need to use such a property. > Then > in each driver it would just be a matter of making a static const > array > lookup table of ranges for each device and calling the helper > function. Sorry for not explaining very well. I have implemented the range into the driver and I was working well, but I had issues defining the range into the device binding and the checker was failing. That was the reason that I've dropped the range from the binding. Also I had some issues enforcing a certain "available" ranges for a particular part into the binding. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-10 16:04 ` Marius.Cristea @ 2025-06-10 16:35 ` David Lechner 2025-06-17 15:21 ` Marius.Cristea 0 siblings, 1 reply; 18+ messages in thread From: David Lechner @ 2025-06-10 16:35 UTC (permalink / raw) To: Marius.Cristea, jic23, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On 6/10/25 11:04 AM, Marius.Cristea@microchip.com wrote: > On Tue, 2025-06-10 at 10:22 -0500, David Lechner wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you >> know the content is safe >> >> On 6/10/25 9:46 AM, Marius.Cristea@microchip.com wrote: >>> Hi David, >>> >>> Thank you for the feedback. Please see my comments below... >>> ... >>>> >>>> Using Jonathan's suggestion from v2 to just have a single >>>> property >>>> with 3 different >>>> ranges to chose from seems simpler that this. It would only >>>> require >>>> one property >>>> and would be self-documenting. The description could be shortened >>>> to >>>> just a couple >>>> of lines. >>> >>> I was thinking to add the range for this property, but it looks >>> (for me >>> at least) more complicated from the checking point of view. The >>> driver >>> is supporting two family of devices that has, each, 3 different >>> voltage >>> range as an input. >>> >> >> Usually, having a consistent binding for the same thing among similar >> devices is more important than how easy it is to implement in the >> driver. >> >> Since this seems to be a common pattern, we could probably justify an >> iio_property_match_ranges() helper function that would simplify the >> implementation in drivers that would need to use such a property. >> Then >> in each driver it would just be a matter of making a static const >> array >> lookup table of ranges for each device and calling the helper >> function. > > Sorry for not explaining very well. I have implemented the range into > the driver and I was working well, but I had issues defining the range > into the device binding and the checker was failing. That was the > reason that I've dropped the range from the binding. Also I had some > issues enforcing a certain "available" ranges for a particular part > into the binding. What did you try? The usual way is to define all possibilities and then limit it by compatible. I think something like this should work: patternProperties: "^channel@[1-4]$": properties: microchip,input-range-microvolt: items: - enum: [-32000000, -16000000, -9000000, -4500000, 0] - enum: [4500000, 9000000, 16000000, 32000000] allOf: - if: properties: compatible: pattern: "^microchip,pac194" then: patternProperties: "^channel@[1-4]$": properties: microchip,input-range-microvolt: oneOf: - items: - const: 0 - const: 9000000 - items: - const: -9000000 - const: 9000000 - items: - const: -4500000 - const: 4500000 default: items: - const: 0 - const: 9000000 - if: properties: compatible: pattern: "^microchip,pac195" then: patternProperties: "^channel@[1-4]$": properties: microchip,input-range-microvolt: oneOf: - items: - const: 0 - const: 32000000 - items: - const: -32000000 - const: 32000000 - items: - const: -16000000 - const: 16000000 default: items: - const: 0 - const: 32000000 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-10 16:35 ` David Lechner @ 2025-06-17 15:21 ` Marius.Cristea 0 siblings, 0 replies; 18+ messages in thread From: Marius.Cristea @ 2025-06-17 15:21 UTC (permalink / raw) To: jic23, nuno.sa, dlechner, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel Hi David, Thanks for the suggestion. //Marius On Tue, 2025-06-10 at 11:35 -0500, David Lechner wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 6/10/25 11:04 AM, Marius.Cristea@microchip.com wrote: > > On Tue, 2025-06-10 at 10:22 -0500, David Lechner wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > On 6/10/25 9:46 AM, Marius.Cristea@microchip.com wrote: > > > > Hi David, > > > > > > > > Thank you for the feedback. Please see my comments below... > > > > > > ... > > > > > > > > > > > Using Jonathan's suggestion from v2 to just have a single > > > > > property > > > > > with 3 different > > > > > ranges to chose from seems simpler that this. It would only > > > > > require > > > > > one property > > > > > and would be self-documenting. The description could be > > > > > shortened > > > > > to > > > > > just a couple > > > > > of lines. > > > > > > > > I was thinking to add the range for this property, but it looks > > > > (for me > > > > at least) more complicated from the checking point of view. The > > > > driver > > > > is supporting two family of devices that has, each, 3 different > > > > voltage > > > > range as an input. > > > > > > > > > > Usually, having a consistent binding for the same thing among > > > similar > > > devices is more important than how easy it is to implement in the > > > driver. > > > > > > Since this seems to be a common pattern, we could probably > > > justify an > > > iio_property_match_ranges() helper function that would simplify > > > the > > > implementation in drivers that would need to use such a property. > > > Then > > > in each driver it would just be a matter of making a static const > > > array > > > lookup table of ranges for each device and calling the helper > > > function. > > > > Sorry for not explaining very well. I have implemented the range > > into > > the driver and I was working well, but I had issues defining the > > range > > into the device binding and the checker was failing. That was the > > reason that I've dropped the range from the binding. Also I had > > some > > issues enforcing a certain "available" ranges for a particular part > > into the binding. > > What did you try? > > The usual way is to define all possibilities and then limit it by > compatible. > I think something like this should work: > > patternProperties: > "^channel@[1-4]$": > properties: > microchip,input-range-microvolt: > items: > - enum: [-32000000, -16000000, -9000000, -4500000, 0] > - enum: [4500000, 9000000, 16000000, 32000000] > > > allOf: > - if: > properties: > compatible: > pattern: "^microchip,pac194" > then: > patternProperties: > "^channel@[1-4]$": > properties: > microchip,input-range-microvolt: > oneOf: > - items: > - const: 0 > - const: 9000000 > - items: > - const: -9000000 > - const: 9000000 > - items: > - const: -4500000 > - const: 4500000 > default: > items: > - const: 0 > - const: 9000000 > - if: > properties: > compatible: > pattern: "^microchip,pac195" > then: > patternProperties: > "^channel@[1-4]$": > properties: > microchip,input-range-microvolt: > oneOf: > - items: > - const: 0 > - const: 32000000 > - items: > - const: -32000000 > - const: 32000000 > - items: > - const: -16000000 > - const: 16000000 > default: > items: > - const: 0 > - const: 32000000 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X 2025-06-06 9:39 ` [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X marius.cristea 2025-06-06 15:53 ` David Lechner @ 2025-06-06 16:24 ` Conor Dooley 1 sibling, 0 replies; 18+ messages in thread From: Conor Dooley @ 2025-06-06 16:24 UTC (permalink / raw) To: marius.cristea Cc: jic23, dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 770 bytes --] On Fri, Jun 06, 2025 at 12:39:28PM +0300, marius.cristea@microchip.com wrote: David had a bunch of comments, so much to say, but.. > + interrupt-names: > + description: > + alert1 indicates a HIGH or LOW limit was exceeded. > + alert2 indicates a THERM limit was exceeded. You should be able to merge this description into the items list, > + items: > + - const: alert1 by adding description: into these > + - const: alert2 e.g. items: - const: sdmmc-3v3 description: pad configuration for 3.3 V - const: sdmmc-1v8 description: pad configuration for 1.8 V - const: sdmmc-3v3-drv description: pull-up/down configuration for 3.3 V - const: sdmmc-1v8-drv description: pull-up/down configuration for 1.8 V [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-06 9:39 [PATCH v3 0/2] adding support for Microchip PAC194X Power Monitor marius.cristea 2025-06-06 9:39 ` [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X marius.cristea @ 2025-06-06 9:39 ` marius.cristea 2025-06-06 17:02 ` David Lechner 2025-06-07 15:27 ` Jonathan Cameron 1 sibling, 2 replies; 18+ messages in thread From: marius.cristea @ 2025-06-06 9:39 UTC (permalink / raw) To: jic23, dlechner, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel, marius.cristea From: Marius Cristea <marius.cristea@microchip.com> This is the iio driver for Microchip PAC194X and PAC195X series of Power Monitors with Accumulator. The PAC194X family supports 9V Full-Scale Range and the PAC195X supports 32V Full-Scale Range. There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices are for high-side current sensing and the PAC194X/5X-2 devices are for low-side current sensing or floating VBUS applications. The PAC194X/5X-1 is named shortly PAC194X/5X. Signed-off-by: Marius Cristea <marius.cristea@microchip.com> --- .../ABI/testing/sysfs-bus-iio-adc-pac1944 | 17 + MAINTAINERS | 7 + drivers/iio/adc/Kconfig | 12 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/pac1944.c | 2841 +++++++++++++++++ 5 files changed, 2878 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 create mode 100644 drivers/iio/adc/pac1944.c diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 new file mode 100644 index 000000000000..ae88eac354a4 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 @@ -0,0 +1,17 @@ +What: /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg +KernelVersion: 6.16 +Contact: linux-iio@vger.kernel.org +Description: + A read/write property used to route, inside the PAC device, a specific ALERT + signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin must be configured for the + ALERT function in order to control the device hardware pin (this is the default + functionality of the device hardware pin). + +What: /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg +KernelVersion: 6.16 +Contact: linux-iio@vger.kernel.org +Description: + A read/write property used to route, inside the PAC device, a specific ALERT + signal to the GPIO/ALERT2 hardware pin. The GPIO/ALERT2 pin must be configured + for ALERT function in order to control the device hardware pin (this is the + default functionality of the device hardware pin). diff --git a/MAINTAINERS b/MAINTAINERS index 86a2045ba62e..240e84893ad9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15920,6 +15920,13 @@ S: Supported F: Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml F: drivers/iio/adc/pac1934.c +MICROCHIP PAC1944 ADC DRIVER +M: Marius Cristea <marius.cristea@microchip.com> +L: linux-iio@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml +F: drivers/iio/adc/pac1944.c + MICROCHIP PCI1XXXX GP DRIVER M: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com> M: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 186a453af56c..a608aa6cb9c7 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1148,6 +1148,18 @@ config PAC1934 This driver can also be built as a module. If so, the module will be called pac1934. +config PAC1944 + tristate "Microchip Technology PAC1944/PAC1954 driver" + depends on I2C + help + Say yes here to build support for Microchip Technology's PAC1941, + PAC1941-2, PAC1942, PAC1942-2, PAC1943, PAC1944, PAC1951, + PAC1951-2, PAC1952, PAC1952-2, PAC1953, PAC1954 + Single/Multi-Channel Power Monitor with Accumulator. + + This driver can also be built as a module. If so, the module + will be called pac1944. + config PALMAS_GPADC tristate "TI Palmas General Purpose ADC" depends on MFD_PALMAS diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 09ae6edb2650..ee47d880babf 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.o +obj-$(CONFIG_PAC1944) += pac1944.o obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c new file mode 100644 index 000000000000..ce09334b076a --- /dev/null +++ b/drivers/iio/adc/pac1944.c @@ -0,0 +1,2841 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * IIO driver for PAC194X and PAC195X series chips + * + * Copyright (C) 2022-2025 Microchip Technology Inc. and its subsidiaries + * + * Author: Marius Cristea marius.cristea@microchip.com + * + * Datasheet for PAC1941, PAC1942, PAC1943 and PAC1944 can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf + * Datasheet for PAC1951, PAC1952, PAC1953 and PAC1954 can be found here: + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf + */ + +#include <linux/acpi.h> +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/iio/iio.h> +#include <linux/iio/events.h> +#include <linux/iio/sysfs.h> +#include <linux/unaligned.h> + +/* + * Maximum (1092 * 60 * 1000), around 1092 minutes@1024 sps + * We will keep the refresh lower + */ +#define PAC1944_MAX_RFSH_LIMIT_MS 300000 + +/* 50msec is the timeout for validity of the cached registers */ +#define PAC1944_MIN_POLLING_TIME_MS 50 +/* + * 1000usec is the minimum wait time for normal conversions when sample + * rate doesn't change + */ +#define PAC1944_MIN_UPDATE_WAIT_TIME_US 1000 + +#define PAC1944_SHUNT_UOHMS_DEFAULT 100000 + +/* 32000mV */ +#define PAC195X_VOLTAGE_MILLIVOLTS_MAX 32000 +/* 9000mV */ +#define PAC194X_VOLTAGE_MILLIVOLTS_MAX 9000 + +/* + * Voltage bits resolution when set for unsigned values and + * HALF FSR signed values + */ +#define PAC1944_VOLTAGE_16B_RES 16 +/* Voltage bits resolution when set for signed values */ +#define PAC1944_VOLTAGE_15B_RES 15 + +/* 100mV maximum voltage drop over the sense resistors */ +#define PAC1944_VSENSE_MILLIVOLTS_MAX 100 + +/* + * Current bits resolution when set for unsigned values and + * HALF FSR signed values + */ +#define PAC1944_CURRENT_16B_RES 16 + +/* Current bits resolution when set for signed values */ +#define PAC1944_CURRENT_15B_RES 15 + +/* Power resolution is 30 bits when unsigned and HALF FSR signed values */ +#define PAC1944_POWER_30B_RES 30 + +/* Power resolution is 29 bits when signed */ +#define PAC1944_POWER_29B_RES 29 + +/* Accumulation register is 56 bits long for unipolar range */ +#define PAC1944_ENERGY_56B_RES 56 + +/* Accumulation register is 56 bits long for bipolar range */ +#define PAC1944_ENERGY_55B_RES 55 + +/* Maximum power-product value - 32 V * 0.1 V */ +#define PAC195X_PRODUCT_VOLTAGE_PV_FSR 3200000000000UL + +/* Maximum power-product value - 9 V * 0.1 V */ +#define PAC194X_PRODUCT_VOLTAGE_PV_FSR 900000000000UL + +#define PAC1944_MEAS_REG_SNAPSHOT_LEN 80 +#define PAC1944_CTRL_REG_SNAPSHOT_LEN 24 + +#define PAC1944_DEFAULT_CHIP_SAMP_SPEED_HZ 1024 + +/* Device register address map */ +#define PAC1944_REFRESH_REG_ADDR 0x00 +#define PAC1944_CTRL_REG_ADDR 0x01 +#define PAC1944_ACC_COUNT_REG_ADDR 0x02 +#define PAC1944_VACC_1_REG_ADDR 0x03 +#define PAC1944_VACC_2_REG_ADDR 0x04 +#define PAC1944_VACC_3_REG_ADDR 0x05 +#define PAC1944_VACC_4_REG_ADDR 0x06 +#define PAC1944_VBUS_1_ADDR 0x07 +#define PAC1944_VBUS_2_ADDR 0x08 +#define PAC1944_VBUS_3_ADDR 0x09 +#define PAC1944_VBUS_4_ADDR 0x0A +#define PAC1944_VSENSE_1_ADDR 0x0B +#define PAC1944_VSENSE_2_ADDR 0x0C +#define PAC1944_VSENSE_3_ADDR 0x0D +#define PAC1944_VSENSE_4_ADDR 0x0E +#define PAC1944_VBUS_AVG_1_ADDR 0x0F +#define PAC1944_VBUS_AVG_2_ADDR 0x10 +#define PAC1944_VBUS_AVG_3_ADDR 0x11 +#define PAC1944_VBUS_AVG_4_ADDR 0x12 +#define PAC1944_VSENSE_AVG_1_ADDR 0x13 +#define PAC1944_VSENSE_AVG_2_ADDR 0x14 +#define PAC1944_VSENSE_AVG_3_ADDR 0x15 +#define PAC1944_VSENSE_AVG_4_ADDR 0x16 +#define PAC1944_VPOWER_1_ADDR 0x17 +#define PAC1944_VPOWER_2_ADDR 0x18 +#define PAC1944_VPOWER_3_ADDR 0x19 +#define PAC1944_VPOWER_4_ADDR 0x1A + +/* Start of configurations registers */ +#define PAC1944_SMBUS_SETTINGS_REGS_ADDR 0x1C +#define PAC1944_NEG_PWR_FSR_REG_ADDR 0x1D +#define PAC1944_REFRESG_V_REG_ADDR 0x1E +#define PAC1944_REFRESH_V_REG_ADDR 0x1F +#define PAC1944_SLOW_REG_ADDR 0x20 +#define PAC1944_CTRL_ACT_REG_ADDR 0x21 +#define PAC1944_CTRL_LAT_REG_ADDR 0x23 +#define PAC1944_NEG_PWR_FSR_LAT_REG_ADDR 0x24 +#define PAC1944_ACCUM_CFG_REG_ADDR 0x25 + +/* + * Registers related to alert functionality + */ +#define PAC1944_ALERT_STATUS_REG_ADDR 0x26 +#define PAC1944_SLOW_ALERT1_REG_ADDR 0x27 +#define PAC1944_GPIO_ALERT2_REG_ADDR 0x28 +#define PAC1944_ACC_FULLNESS_LIMITS_REG_ADDR 0x29 +#define PAC1944_OC_LIMIT_REG_ADDR 0x30 +#define PAC1944_UC_LIMIT_REG_ADDR 0x34 +#define PAC1944_OP_LIMIT_REG_ADDR 0x38 +#define PAC1944_OV_LIMIT_REG_ADDR 0x3C +#define PAC1944_UV_LIMIT_REG_ADDR 0x40 +#define PAC1944_OC_LIMIT_NSAMPLES_REG_ADDR 0x44 +#define PAC1944_UC_LIMIT_NSAMPLES_REG_ADDR 0x45 +#define PAC1944_OP_LIMIT_NSAMPLES_REG_ADDR 0x46 +#define PAC1944_OV_LIMIT_NSAMPLES_REG_ADDR 0x47 +#define PAC1944_UV_LIMIT_NSAMPLES_REG_ADDR 0x48 +#define PAC1944_ALERT_ENABLE_REG_ADDR 0x49 + +#define PAC1944_ALERT_ENABLE_REG_LEN 3 +#define PAC1944_ALERTS_REG_LEN 63 + +#define PAC1944_PID_REG_ADDR 0xFD + +/* Alert Enable register */ +#define PAC1944_OC_MASK GENMASK(23, 20) +#define PAC1944_CH01OC_MASK BIT(23) +#define PAC1944_CH01OC_SET BIT(23) +#define PAC1944_CH02OC_MASK BIT(22) +#define PAC1944_CH02OC_SET BIT(22) +#define PAC1944_CH03OC_MASK BIT(21) +#define PAC1944_CH03OC_SET BIT(21) +#define PAC1944_CH04OC_MASK BIT(20) +#define PAC1944_CH04OC_SET BIT(20) + +#define PAC1944_UC_MASK GENMASK(19, 16) +#define PAC1944_CH01UC_MASK BIT(19) +#define PAC1944_CH01UC_SET BIT(19) +#define PAC1944_CH02UC_MASK BIT(18) +#define PAC1944_CH02UC_SET BIT(18) +#define PAC1944_CH03UC_MASK BIT(17) +#define PAC1944_CH03UC_SET BIT(17) +#define PAC1944_CH04UC_MASK BIT(16) +#define PAC1944_CH04UC_SET BIT(16) + +#define PAC1944_OV_MASK GENMASK(15, 12) +#define PAC1944_CH01OV_MASK BIT(15) +#define PAC1944_CH01OV_SET BIT(15) +#define PAC1944_CH02OV_MASK BIT(14) +#define PAC1944_CH02OV_SET BIT(14) +#define PAC1944_CH03OV_MASK BIT(13) +#define PAC1944_CH03OV_SET BIT(13) +#define PAC1944_CH04OV_MASK BIT(12) +#define PAC1944_CH04OV_SET BIT(12) + +#define PAC1944_UV_MASK GENMASK(11, 8) +#define PAC1944_CH01UV_MASK BIT(11) +#define PAC1944_CH01UV_SET BIT(11) +#define PAC1944_CH02UV_MASK BIT(10) +#define PAC1944_CH02UV_SET BIT(10) +#define PAC1944_CH03UV_MASK BIT(9) +#define PAC1944_CH03UV_SET BIT(9) +#define PAC1944_CH04UV_MASK BIT(8) +#define PAC1944_CH04UV_SET BIT(8) + +#define PAC1944_OP_MASK GENMASK(7, 4) +#define PAC1944_CH01OP_MASK BIT(7) +#define PAC1944_CH01OP_SET BIT(7) +#define PAC1944_CH02OP_MASK BIT(6) +#define PAC1944_CH02OP_SET BIT(6) +#define PAC1944_CH03OP_MASK BIT(5) +#define PAC1944_CH03OP_SET BIT(5) +#define PAC1944_CH04OP_MASK BIT(4) +#define PAC1944_CH04OP_SET BIT(4) + +#define PAC1944_ACC_OVF_MASK BIT(3) +#define PAC1944_ACC_OVF_SET BIT(3) + +#define PAC1944_ACC_COUNT_MASK BIT(2) +#define PAC1944_ACC_COUNT_SET BIT(2) + +#define PAC1944_ALERT_CC1_MASK BIT(1) +#define PAC1944_ALERT_CC1_SET BIT(1) + +#define PAC1944_ACC_REG_LEN 4 +#define PAC1944_VACC_REG_LEN 7 +#define PAC1944_VBUS_SENSE_REG_LEN 2 +#define PAC1944_VPOWER_REG_LEN 4 +#define PAC1944_CTRL_ACT_REG_LEN 2 +#define PAC1944_CTRL_LAT_REG_LEN 2 +#define PAC1944_MAX_REGISTER_LEN 6 + +#define PAC1944_COMMON_DEVATTR 1 +#define PAC1944_ACC_DEVATTR 3 +#define PAC1944_SHARED_DEVATTRS_COUNT 2 + +#define PAC1944_MID 0x5D +#define PAC194x54_PID 0x5B +#define PAC194x53_PID 0x5A +#define PAC194x52_PID 0x59 +#define PAC194x51_PID 0x58 + +#define PAC1944_MAX_CH 4 + +/* PAC194X family */ +#define PAC_PRODUCT_ID_1941 0x68 +#define PAC_PRODUCT_ID_1942 0x69 +#define PAC_PRODUCT_ID_1943 0x6A +#define PAC_PRODUCT_ID_1944 0x6B +#define PAC_PRODUCT_ID_1941_2 0x6C +#define PAC_PRODUCT_ID_1942_2 0x6D +/* PAC195x family */ +#define PAC_PRODUCT_ID_1951 0x78 +#define PAC_PRODUCT_ID_1952 0x79 +#define PAC_PRODUCT_ID_1953 0x7A +#define PAC_PRODUCT_ID_1954 0x7B +#define PAC_PRODUCT_ID_1951_2 0x7C +#define PAC_PRODUCT_ID_1952_2 0x7D + +#define PAC1944_ALERT 0x00 +#define PAC1944_GPIO_INPUT 0x01 +#define PAC1944_GPIO_OUTPUT 0x02 +#define PAC1944_SLOW 0x03 + +#define PAC1944_CTRL_SAMPLE_MASK GENMASK(15, 12) +#define PAC1944_CTRL_GPIO_ALERT2_MASK GENMASK(11, 10) +#define PAC1944_CTRL_SLOW_ALERT1_MASK GENMASK(9, 8) +#define PAC1944_CTRL_CH_1_OFF_MASK BIT(7) +#define PAC1944_CTRL_CH_2_OFF_MASK BIT(6) +#define PAC1944_CTRL_CH_3_OFF_MASK BIT(5) +#define PAC1944_CTRL_CH_4_OFF_MASK BIT(4) + +#define PAC1944_NEG_PWR_CFG_VS1_MASK GENMASK(15, 14) +#define PAC1944_NEG_PWR_CFG_VS2_MASK GENMASK(13, 12) +#define PAC1944_NEG_PWR_CFG_VS3_MASK GENMASK(11, 10) +#define PAC1944_NEG_PWR_CFG_VS4_MASK GENMASK(9, 8) +#define PAC1944_NEG_PWR_CFG_VB1_MASK GENMASK(7, 6) +#define PAC1944_NEG_PWR_CFG_VB2_MASK GENMASK(5, 4) +#define PAC1944_NEG_PWR_CFG_VB3_MASK GENMASK(3, 2) +#define PAC1944_NEG_PWR_CFG_VB4_MASK GENMASK(1, 0) + +#define PAC1944_CFG_ACC4_SHIFT 0 +#define PAC1944_CFG_ACC3_SHIFT 2 +#define PAC1944_CFG_ACC2_SHIFT 4 +#define PAC1944_CFG_ACC1_SHIFT 6 + +#define PAC1944_ACPI_GET_NAMES 1 +#define PAC1944_ACPI_GET_UOHMS_VALS 2 +#define PAC1944_ACPI_GET_BIPOLAR_SETTINGS 4 +#define PAC1944_ACPI_GET_SAMP 5 + +#define ACCUM_REG(acc1_cfg, acc2_cfg, acc3_cfg, acc4_cfg) \ + ((((acc1_cfg) & 0x03) << PAC1944_CFG_ACC1_SHIFT) | \ + (((acc2_cfg) & 0x03) << PAC1944_CFG_ACC2_SHIFT) | \ + (((acc3_cfg) & 0x03) << PAC1944_CFG_ACC3_SHIFT) | \ + (((acc4_cfg) & 0x03) << PAC1944_CFG_ACC4_SHIFT)) + +/* + * Accumulated power/energy formula (in mW-seconds): + * Energy = (Vacc/10^9)*[(10^9/2^30)*2^9]*3.2*10^3/Rsense + * Vacc - is the accumulated value per second + * Rsense - value of the shunt resistor in microOhms + * + * PAC195X_MAX_VPOWER_RSHIFTED_BY_29B = 3.2*((10^9)/(2^29))*10^9 + * will be used to calculate the scale for accumulated power/energy + */ +#define PAC195X_MAX_VPOWER_RSHIFTED_BY_29B 5960464478UL + +/* + * PAC194X_MAX_VPOWER_RSHIFTED_BY_29B = 0.9*((10^9)/(2^29))*10^9 + * will be used to calculate the scale for accumulated power/energy + */ +#define PAC194X_MAX_VPOWER_RSHIFTED_BY_29B 1676380634UL + +/* (100mV * 1000000) / (2^15) used to calculate the scale for current */ +#define PAC1944_MAX_VSENSE_RSHIFTED_BY_15B 3052 + +/* + * [(100mV * 1000000) / (2^15)]*10^9 used to calculate the scale + * for accumulated current/Coulomb counter + */ +#define PAC1944_MAX_VSENSE_NANO 3051757812500UL + +#define to_pac1944_chip_info(d) container_of(d, struct pac1944_chip_info, work_chip_rfsh) + +/* + * these indexes are exactly describing the element order within a single + * PAC1944/54 phys channel IIO channel descriptor; see the static const struct + * iio_chan_spec pac1944_single_channel[] declaration + */ +enum pac1944_ch_idx { + PAC1944_CH_POWER, + PAC1944_CH_VOLTAGE, + PAC1944_CH_CURRENT, + PAC1944_CH_VOLTAGE_AVERAGE, + PAC1944_CH_CURRENT_AVERAGE, +}; + +/* IDs ordered based on Product ID */ +enum pac1944_ids { + PAC1941, + PAC1942, + PAC1943, + PAC1944, + PAC1941_2, + PAC1942_2, + PAC1951, + PAC1952, + PAC1953, + PAC1954, + PAC1951_2, + PAC1952_2, +}; + +enum pac1944_acc_mode { + PAC1944_ACCMODE_VPOWER, + PAC1944_ACCMODE_VSENSE, + PAC1944_ACCMODE_VBUS, +}; + +enum pac1944_ { + PAC1944_UNIPOLAR_FSR_CFG, + PAC1944_BIPOLAR_FSR_CFG, + PAC1944_BIPOLAR_HALF_FSR_CFG, +}; + +enum pac1944_samps { + PAC1944_SAMP_1024SPS_ADAPT, + PAC1944_SAMP_256SPS_ADAPT, + PAC1944_SAMP_64SPS_ADAPT, + PAC1944_SAMP_8SPS_ADAPT, + + PAC1944_SAMP_1024SPS, + PAC1944_SAMP_256SPS, + PAC1944_SAMP_64SPS, + PAC1944_SAMP_8SPS, + + PAC1944_SAMP_SINGLE_SHOT, + PAC1944_SAMP_SINGLE_SHOT_8X, + PAC1944_SAMP_FAST_MODE, + PAC1944_SAMP_BURST_MODE, + + PAC1944_RESERVED1, + PAC1944_RESERVED2, + PAC1944_RESERVED3, + PAC1944_RESERVED4, +}; + +enum pac1944_number_of_active_channels { + PAC1944_1_CHANNEL_ACTIVE, + PAC1944_2_CHANNELS_ACTIVE, + PAC1944_3_CHANNELS_ACTIVE, + PAC1944_4_CHANNELS_ACTIVE, +}; + +/* + * The PAC195X has a feature called Adaptive Accumulator mode (APAPT). In this + * mode, sampling is programmed at one of the valid sample rates and samples are + * accumulated. If the SLOW pin is asserted and the device begins sampling at + * 8 SPS, these samples are shifted by 7 bits to the left and accumulated so as + * to simulate sampling at the maximum sampling rate, 1024 SPS, and the + * accumulator count is also incremented by 128 for each sample in Slow mode + * (when using the Adaptive Accumulator mode) to simulate samples being + * accumulated at the maximum sampling rate. + * This offers a big reduction in host overhead and bus traffic for systems that + * need to use the SLOW pin for lower power operation during certain times and + * want to have continuous accurate energy monitoring for both the maximum + * sampling rate and the SLOW sampling rate. + */ +static const unsigned int pac1944_samp_rate_map_tbl[] = { + [PAC1944_SAMP_1024SPS_ADAPT] = 1024, + [PAC1944_SAMP_256SPS_ADAPT] = 256, + [PAC1944_SAMP_64SPS_ADAPT] = 64, + [PAC1944_SAMP_8SPS_ADAPT] = 8, + [PAC1944_SAMP_1024SPS] = 1024, + [PAC1944_SAMP_256SPS] = 256, + [PAC1944_SAMP_64SPS] = 64, + [PAC1944_SAMP_8SPS] = 8, + [PAC1944_SAMP_SINGLE_SHOT] = 1, + [PAC1944_SAMP_SINGLE_SHOT_8X] = 8, + [PAC1944_SAMP_FAST_MODE] = 0xff, + [PAC1944_SAMP_BURST_MODE] = 0xff, + [PAC1944_RESERVED1] = 0xff, + [PAC1944_RESERVED2] = 0xff, + [PAC1944_RESERVED3] = 0xff, + [PAC1944_RESERVED4] = 0xff, +}; + +static const unsigned int shift_map_tbl[] = { + [PAC1944_SAMP_1024SPS_ADAPT] = 10, + [PAC1944_SAMP_256SPS_ADAPT] = 10, + [PAC1944_SAMP_64SPS_ADAPT] = 10, + [PAC1944_SAMP_8SPS_ADAPT] = 10, + [PAC1944_SAMP_1024SPS] = 10, + [PAC1944_SAMP_256SPS] = 8, + [PAC1944_SAMP_64SPS] = 6, + [PAC1944_SAMP_8SPS] = 3, + [PAC1944_SAMP_SINGLE_SHOT] = 10, + [PAC1944_SAMP_SINGLE_SHOT_8X] = 8, + [PAC1944_SAMP_FAST_MODE] = 0xff, + [PAC1944_SAMP_BURST_MODE] = 0xff, + [PAC1944_RESERVED1] = 0xff, + [PAC1944_RESERVED2] = 0xff, + [PAC1944_RESERVED3] = 0xff, + [PAC1944_RESERVED4] = 0xff, +}; + +static const unsigned int samp_rate_burst_mode_tbl[] = { + [PAC1944_1_CHANNEL_ACTIVE] = 5120, + [PAC1944_2_CHANNELS_ACTIVE] = 2560, + [PAC1944_3_CHANNELS_ACTIVE] = 1706, + [PAC1944_4_CHANNELS_ACTIVE] = 1280, +}; + +static const unsigned int samp_rate_fast_mode_tbl[] = { + [PAC1944_1_CHANNEL_ACTIVE] = 2560, + [PAC1944_2_CHANNELS_ACTIVE] = 1707, + [PAC1944_3_CHANNELS_ACTIVE] = 1280, + [PAC1944_4_CHANNELS_ACTIVE] = 1024, +}; + +/* Available Sample Modes */ +static const char * const pac1944_frequency_avail[] = { + "1024_ADAP", + "256_ADAP", + "64_ADAP", + "8_ADAP", + "1024", + "256", + "64", + "8", + "single_shot_1x", + "single_shot_8x", + "fast", + "burst", +}; + +static const unsigned int pac1944_overvoltage_mask_tbl[] = { + PAC1944_CH01OV_MASK, + PAC1944_CH02OV_MASK, + PAC1944_CH03OV_MASK, + PAC1944_CH04OV_MASK, +}; + +static const unsigned int pac1944_undervoltage_mask_tbl[] = { + PAC1944_CH01UV_MASK, + PAC1944_CH02UV_MASK, + PAC1944_CH03UV_MASK, + PAC1944_CH04UV_MASK, +}; + +static const unsigned int pac1944_overcurrent_mask_tbl[] = { + PAC1944_CH01OC_MASK, + PAC1944_CH02OC_MASK, + PAC1944_CH03OC_MASK, + PAC1944_CH04OC_MASK, +}; + +static const unsigned int pac1944_undercurrent_mask_tbl[] = { + PAC1944_CH01UC_MASK, + PAC1944_CH02OC_MASK, + PAC1944_CH03OC_MASK, + PAC1944_CH04OC_MASK, +}; + +static const unsigned int pac1944_overpower_mask_tbl[] = { + PAC1944_CH01OP_MASK, + PAC1944_CH02OP_MASK, + PAC1944_CH03OP_MASK, + PAC1944_CH04OP_MASK, +}; + +/** + * struct reg_data - data from the registers + * @vsense_mode:array of values, Full Scale Range (FSR) mode for V Sense + * @vbus_mode: array of values, Full Scale Range (FSR) mode for V Bus + * @accumulation_mode: array of values, accumulation mode for hardware accumulator + * @meas_regs: snapshot of raw measurements registers + * @ctrl_act_reg: snapshot of the ctrl_act register + * @ctrl_lat_reg: snapshot of the ctrl_lat register + * @acc_count: snapshot of the acc_count register + * @total_samples_nr: accumulated values for acc_count (total number of samples) + * @acc_val: accumulated values per second + * @vacc: accumulated vpower values + * @vpower: snapshot of vpower registers + * @vbus: snapshot of vbus registers + * @vbus_avg: averages of vbus registers + * @vsense: snapshot of vsense registers + * @vsense_avg: averages of vsense registers + * @jiffies_tstamp: chip's uptime + */ +struct reg_data { + u8 vbus_mode[PAC1944_MAX_CH]; + u8 vsense_mode[PAC1944_MAX_CH]; + u8 accumulation_mode[PAC1944_MAX_CH]; + u8 meas_regs[PAC1944_MEAS_REG_SNAPSHOT_LEN]; + u16 ctrl_act_reg; + u16 ctrl_lat_reg; + u32 acc_count; + u32 total_samples_nr[PAC1944_MAX_CH]; + s64 acc_val[PAC1944_MAX_CH]; + s64 vacc[PAC1944_MAX_CH]; + s32 vpower[PAC1944_MAX_CH]; + s32 vbus[PAC1944_MAX_CH]; + s32 vbus_avg[PAC1944_MAX_CH]; + s32 vsense[PAC1944_MAX_CH]; + s32 vsense_avg[PAC1944_MAX_CH]; + unsigned long jiffies_tstamp; +}; + +/** + * struct pac1944_chip_info - chip configuration + * @channels: array of values, true means that channel is active + * @iio_info: pointer to iio_info structure + * @client: a pointer to the i2c client associated with the device + * @lock: lock to prevent concurrent reads/writes + * @work_chip_rfsh: chip refresh workqueue implementation + * @active_channels_mask: active channels + * @phys_channels: number of physical channels for the device + * @chip_variant: stores the type of the device + * @chip_revision: store the silicon revision version of the device + * @shunts: array of values, shunt resistor values + * @chip_reg_data: pointer to structure, containing data from the device registers + * @sample_rate_value: sampling frequency + * @labels: array of string, name of each channel + * @is_pac195x_family: true if device is part of the PAC195x family + * @sampling_mode: sampling mode used by the device + * @num_enabled_channels: count of how many chip channels are currently enabled + * @slow_alert1: snapshot of slow/alert register + * @gpio_alert2: snapshot of gpio/alert register + * @acc_fullness: snapshot of accumulator fullness limit register + * @overcurrent: array of values, overcurrent limit + * @undercurrent: array of values, undercurrent limit + * @overpower: array of values, overpower limit + * @overvoltage: array of values, overvoltage limit + * @undervoltage: array of values, undervoltage limit + * @oc_limit_nsamples: number of consecutive samples exceeding the overcurrent limit + * @uc_limit_nsamples: number of consecutive samples exceeding the undercurrent limit + * @op_limit_nsamples: number of consecutive samples exceeding the overpower limit + * @ov_limit_nsamples: number of consecutive samples exceeding the overvoltage limit + * @uv_limit_nsamples: number of consecutive samples exceeding the undervoltage limit + * @alert_enable: snapshot of alert enable register + * @enable_acc: array of values, true means that accumulation channel is measured + */ +struct pac1944_chip_info { + const struct iio_chan_spec *channels; + struct iio_info iio_info; + struct i2c_client *client; + struct mutex lock; /* lock to prevent concurrent reads/writes */ + struct delayed_work work_chip_rfsh; + unsigned long active_channels_mask; + u8 phys_channels; + u8 chip_variant; + u8 chip_revision; + u32 shunts[PAC1944_MAX_CH]; + struct reg_data chip_reg_data; + s32 sample_rate_value; + char *labels[PAC1944_MAX_CH]; + bool is_pac195x_family; + u8 sampling_mode; + u8 num_enabled_channels; + u32 slow_alert1; + u32 gpio_alert2; + u16 acc_fullness; + u16 overcurrent[PAC1944_MAX_CH]; + u16 undercurrent[PAC1944_MAX_CH]; + u32 overpower[PAC1944_MAX_CH]; + u16 overvoltage[PAC1944_MAX_CH]; + u16 undervoltage[PAC1944_MAX_CH]; + u8 oc_limit_nsamples[PAC1944_MAX_CH]; + u8 uc_limit_nsamples[PAC1944_MAX_CH]; + u8 op_limit_nsamples[PAC1944_MAX_CH]; + u8 ov_limit_nsamples[PAC1944_MAX_CH]; + u8 uv_limit_nsamples[PAC1944_MAX_CH]; + u32 alert_enable; + bool enable_acc[PAC1944_MAX_CH]; +}; + +/** + * struct pac1944_features - features of a pac194x instance + * @phys_channels: number of physical channels supported by the chip + * @prod_id: hardware ID + * @name: chip's name + */ +struct pac1944_features { + u8 phys_channels; + u8 prod_id; + const char *name; +}; + +static const struct pac1944_features pac1944_chip_config[] = { + /* PAC194X Family */ + [PAC1941] = { + .phys_channels = 1, + .prod_id = PAC_PRODUCT_ID_1941, + .name = "pac1941", + }, + [PAC1942] = { + .phys_channels = 2, + .prod_id = PAC_PRODUCT_ID_1942, + .name = "pac1942", + }, + [PAC1943] = { + .phys_channels = 3, + .prod_id = PAC_PRODUCT_ID_1943, + .name = "pac1943", + }, + [PAC1944] = { + .phys_channels = 4, + .prod_id = PAC_PRODUCT_ID_1944, + .name = "pac1944", + }, + [PAC1941_2] = { + .phys_channels = 1, + .prod_id = PAC_PRODUCT_ID_1941_2, + .name = "pac1941_2", + }, + [PAC1942_2] = { + .phys_channels = 2, + .prod_id = PAC_PRODUCT_ID_1942_2, + .name = "pac1942_2", + }, + /* PAC195X Family */ + [PAC1951] = { + .phys_channels = 1, + .prod_id = PAC_PRODUCT_ID_1951, + .name = "pac1951", + }, + [PAC1952] = { + .phys_channels = 2, + .prod_id = PAC_PRODUCT_ID_1952, + .name = "pac1952_1", + }, + [PAC1953] = { + .phys_channels = 3, + .prod_id = PAC_PRODUCT_ID_1953, + .name = "pac1953", + }, + [PAC1954] = { + .phys_channels = 4, + .prod_id = PAC_PRODUCT_ID_1954, + .name = "pac1954", + }, + [PAC1951_2] = { + .phys_channels = 1, + .prod_id = PAC_PRODUCT_ID_1951_2, + .name = "pac1951_2", + }, + [PAC1952_2] = { + .phys_channels = 2, + .prod_id = PAC_PRODUCT_ID_1952_2, + .name = "pac1952_2", + }, +}; + +static inline u64 pac1944_get_unaligned_be56(const u8 *p) +{ + return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 | + (u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6]; +} + +/* Low-level I2c functions used to transfer more then 32 bytes at once */ +static int pac1944_i2c_read(struct i2c_client *client, u8 reg_addr, + void *databuf, u8 len) +{ + struct i2c_msg msgs[2] = { + { .addr = client->addr, + .len = 1, + .buf = (u8 *)®_addr, + .flags = 0 + }, + { .addr = client->addr, + .len = len, + .buf = databuf, + .flags = I2C_M_RD + } + }; + + return i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); +} + +static int pac1944_disable_alert_reg(struct device *dev, u32 mask, u8 *status) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + u32 val; + u8 buf[PAC1944_ALERT_ENABLE_REG_LEN]; + + ret = i2c_smbus_read_i2c_block_data(client, + PAC1944_ALERT_ENABLE_REG_ADDR, + PAC1944_ALERT_ENABLE_REG_LEN, + status); + if (ret < 0) { + dev_err(dev, "failing to disable allert reg\n"); + return ret; + } + + val = get_unaligned_be24(status); + val = val & (~mask); + put_unaligned_be24(val, &buf[0]); + + /* disable appropriate bit from the Alert enable register */ + return i2c_smbus_write_block_data(client, PAC1944_ALERT_ENABLE_REG_ADDR, + PAC1944_ALERT_ENABLE_REG_LEN, + (u8 *)&buf[0]); +} + +static int pac1944_restore_alert_reg(struct iio_dev *indio_dev, u8 *status) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + + /* restoring the Alert enable register */ + ret = i2c_smbus_write_block_data(client, PAC1944_ALERT_ENABLE_REG_ADDR, + PAC1944_ALERT_ENABLE_REG_LEN, status); + if (ret) { + dev_err(&client->dev, "failing to write %s\n", __func__); + return ret; + } + + /* Sending a REFRESH_V to the chip, so the new settings take place */ + ret = i2c_smbus_write_byte(info->client, PAC1944_REFRESH_V_REG_ADDR); + if (ret) + dev_err(&client->dev, "cannot send REFRESH_V\n"); + + return ret; +} + +static int pac1944_update_alert_16b(struct device *dev, u8 addr, u32 mask, + u16 value) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + __be16 tmp_be16; + u8 status[PAC1944_ALERT_ENABLE_REG_LEN]; + + ret = pac1944_disable_alert_reg(dev, mask, &status[0]); + if (ret) { + dev_err(dev, "failed to disable alert reg\n"); + return ret; + } + + tmp_be16 = cpu_to_be16(value); + + ret = i2c_smbus_write_word_data(client, addr, tmp_be16); + if (ret) { + dev_err(dev, "failing to write at 0x%x\n", addr); + return ret; + } + + return pac1944_restore_alert_reg(indio_dev, &status[0]); +} + +static int pac1944_update_alert_24b(struct device *dev, u8 addr, u32 mask, + u32 value) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + u8 status[PAC1944_ALERT_ENABLE_REG_LEN], tmp[3]; + + ret = pac1944_disable_alert_reg(dev, mask, &status[0]); + if (ret) { + dev_err(dev, "failed to disable alert reg\n"); + return ret; + } + + put_unaligned_be24(value, &tmp[0]); + + ret = i2c_smbus_write_block_data(client, addr, ARRAY_SIZE(tmp), (u8 *)&tmp[0]); + if (ret) { + dev_err(dev, "failing to write at 0x%x\n", addr); + return ret; + } + + return pac1944_restore_alert_reg(indio_dev, &status[0]); +} + +/* Custom IIO Device Attributes */ +static ssize_t pac1944_shunt_value_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + + return sysfs_emit(buf, "%u\n", info->shunts[this_attr->address]); +} + +static ssize_t pac1944_shunt_value_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int sh_val; + + if (kstrtouint(buf, 10, &sh_val)) { + dev_err(dev, "Shunt value is not valid\n"); + return -EINVAL; + } + + scoped_guard(mutex, &info->lock) + info->shunts[this_attr->address] = sh_val; + + return count; +} + +static ssize_t pac1944_slow_alert1_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + u32 val; + u8 tmp[3]; + + if (kstrtoint(buf, 10, &val)) + return -EINVAL; + + put_unaligned_be24(val, &tmp[0]); + + scoped_guard(mutex, &info->lock) { + ret = i2c_smbus_write_block_data(client, PAC1944_SLOW_ALERT1_REG_ADDR, + ARRAY_SIZE(tmp), (u8 *)&tmp[0]); + if (ret) { + dev_err(dev, "failing to write %s\n", __func__); + return ret; + } + + info->slow_alert1 = val; + } + + return count; +} + +#define PAC1944_SLOW_ALERT_SHOW(alert_name) \ +static ssize_t pac1944_##alert_name##_show(struct device *dev, \ + struct device_attribute *attr, \ + char *buf) \ + { \ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); \ + struct pac1944_chip_info *info = iio_priv(indio_dev); \ + \ + return sysfs_emit(buf, "%u\n", info->alert_name); \ + } + +PAC1944_SLOW_ALERT_SHOW(slow_alert1) +PAC1944_SLOW_ALERT_SHOW(gpio_alert2) + +static ssize_t pac1944_gpio_alert2_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + u32 val; + u8 tmp[3]; + + if (kstrtoint(buf, 10, &val)) + return -EINVAL; + + put_unaligned_be24(val, &tmp[0]); + + scoped_guard(mutex, &info->lock) { + ret = i2c_smbus_write_block_data(client, PAC1944_GPIO_ALERT2_REG_ADDR, + ARRAY_SIZE(tmp), (u8 *)&tmp[0]); + if (ret) { + dev_err(dev, "failing to write %s\n", __func__); + return ret; + } + + info->gpio_alert2 = val; + } + + return count; +} + +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644, + pac1944_shunt_value_show, pac1944_shunt_value_store, 0); +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644, + pac1944_shunt_value_show, pac1944_shunt_value_store, 1); +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644, + pac1944_shunt_value_show, pac1944_shunt_value_store, 2); +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644, + pac1944_shunt_value_show, pac1944_shunt_value_store, 3); + +static IIO_DEVICE_ATTR(slow_alert1_cfg, 0644, pac1944_slow_alert1_show, + pac1944_slow_alert1_store, 0); +static IIO_DEVICE_ATTR(gpio_alert2_cfg, 0644, pac1944_gpio_alert2_show, + pac1944_gpio_alert2_store, 0); + +static struct attribute *pac1944_all_attrs[] = { + &iio_dev_attr_in_current1_shunt_resistor.dev_attr.attr, + &iio_dev_attr_in_current2_shunt_resistor.dev_attr.attr, + &iio_dev_attr_in_current3_shunt_resistor.dev_attr.attr, + &iio_dev_attr_in_current4_shunt_resistor.dev_attr.attr, + &iio_dev_attr_slow_alert1_cfg.dev_attr.attr, + &iio_dev_attr_gpio_alert2_cfg.dev_attr.attr, + NULL +}; + +static const struct iio_event_spec pac1944_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, +}; + +static const struct iio_event_spec pac1944_single_event[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, +}; + +#define PAC1944_VBUS_CHANNEL(_index, _address, _ev_spec, _num_ev_spec) {\ + .type = IIO_VOLTAGE, \ + .address = (_address), \ + .indexed = 1, \ + .channel = (_index) + 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .event_spec = (_ev_spec), \ + .num_event_specs = (_num_ev_spec), \ + .ext_info = pac1944_ext_info \ +} + +#define PAC1944_VBUS_AVG_CHANNEL(_index, _address) {\ + .type = IIO_VOLTAGE, \ + .address = (_address), \ + .indexed = 1, \ + .channel = (_index) + 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = pac1944_ext_info \ +} + +#define PAC1944_VSENSE_CHANNEL(_index, _address, _ev_spec, _num_ev_spec) {\ + .type = IIO_CURRENT, \ + .address = (_address), \ + .indexed = 1, \ + .channel = (_index) + 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .event_spec = (_ev_spec), \ + .num_event_specs = (_num_ev_spec), \ + .ext_info = pac1944_ext_info \ +} + +#define PAC1944_VSENSE_AVG_CHANNEL(_index, _address) { \ + .type = IIO_CURRENT, \ + .address = (_address), \ + .indexed = 1, \ + .channel = (_index) + 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = pac1944_ext_info \ +} + +#define PAC1944_VPOWER_CHANNEL(_index, _address, _ev_spec, _num_ev_spec) {\ + .type = IIO_POWER, \ + .address = (_address), \ + .indexed = 1, \ + .channel = (_index) + 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .event_spec = (_ev_spec), \ + .num_event_specs = (_num_ev_spec), \ + .ext_info = pac1944_ext_info \ +} + +static int pac1944_send_refresh(struct pac1944_chip_info *info, u8 refresh_cmd, + u32 wait_time) +{ + struct i2c_client *client = info->client; + int ret; + + /* Writing a REFRESH or a REFRESH_V command */ + ret = i2c_smbus_write_byte(client, refresh_cmd); + if (ret) { + dev_err(&client->dev, "%s - cannot send Refresh cmd (0x%02X) to PAC1944\n", + __func__, refresh_cmd); + return ret; + } + + /* Register data retrieval timestamp */ + info->chip_reg_data.jiffies_tstamp = jiffies; + /* Wait till the data is available */ + usleep_range(wait_time, wait_time + 100); + + return 0; +} + +static int pac1944_reg_snapshot(struct pac1944_chip_info *info, bool do_refresh, + u8 refresh_cmd, u32 wait_time) +{ + struct i2c_client *client = info->client; + struct device *dev = &client->dev; + u8 shift, idx; + u8 *offset_reg_data_p; + int cnt, ret; + u32 count, inc_count; + u32 fs = 0; + s64 stored_value, tmp_s64; + s64 inc = 0; + __be16 tmp_be16; + u16 smpl_mode; + bool is_unipolar; + + guard(mutex)(&info->lock); + + if (do_refresh) { + ret = pac1944_send_refresh(info, refresh_cmd, wait_time); + if (ret < 0) { + dev_err(dev, "cannot send refresh towards PAC1944\n"); + return ret; + } + } + + /* Read the ctrl/status registers for this snapshot */ + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_ACT_REG_ADDR, + sizeof(tmp_be16), (u8 *)&tmp_be16); + if (ret < 0) { + dev_err(dev, "cannot read PAC1944 regs from 0x%02X\n", PAC1944_CTRL_ACT_REG_ADDR); + return ret; + } + + info->chip_reg_data.ctrl_act_reg = be16_to_cpu(tmp_be16); + + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_LAT_REG_ADDR, + sizeof(tmp_be16), (u8 *)&tmp_be16); + if (ret < 0) { + dev_err(dev, "cannot read PAC1944 regs from 0x%02X\n", PAC1944_CTRL_LAT_REG_ADDR); + return ret; + } + + info->chip_reg_data.ctrl_lat_reg = be16_to_cpu(tmp_be16); + + /* Read the data registers */ + ret = pac1944_i2c_read(client, PAC1944_ACC_COUNT_REG_ADDR, + (u8 *)info->chip_reg_data.meas_regs, + PAC1944_MEAS_REG_SNAPSHOT_LEN); + if (ret < 0) { + dev_err(dev, "cannot read PAC1944 regs from 0x%02X\n", PAC1944_ACC_COUNT_REG_ADDR); + return ret; + } + + offset_reg_data_p = &info->chip_reg_data.meas_regs[0]; + + info->chip_reg_data.acc_count = get_unaligned_be32(offset_reg_data_p); + + offset_reg_data_p += PAC1944_ACC_REG_LEN; + + /* + * Check if the channel is active (within the data read from + * the chip), skip all fields if disabled + */ + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + /* skip if the energy accumulation is disabled */ + if (!info->enable_acc[cnt]) { + offset_reg_data_p += PAC1944_VACC_REG_LEN; + continue; + } + + stored_value = info->chip_reg_data.acc_val[cnt]; + + info->chip_reg_data.vacc[cnt] = pac1944_get_unaligned_be56(offset_reg_data_p); + + is_unipolar = true; + switch (info->chip_reg_data.accumulation_mode[cnt]) { + case PAC1944_ACCMODE_VPOWER: + if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG || + info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + is_unipolar = false; + break; + case PAC1944_ACCMODE_VBUS: + if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + is_unipolar = false; + break; + case PAC1944_ACCMODE_VSENSE: + if (info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + is_unipolar = false; + break; + } + + if (!is_unipolar) + info->chip_reg_data.vacc[cnt] = + sign_extend64(info->chip_reg_data.vacc[cnt], 55); + + if (info->chip_reg_data.accumulation_mode[cnt] != PAC1944_ACCMODE_VBUS) { + /* + * Integrate the accumulated power or current over + * the elapsed interval. + */ + smpl_mode = info->chip_reg_data.ctrl_lat_reg >> 12; + + tmp_s64 = info->chip_reg_data.vacc[cnt]; + if (smpl_mode < PAC1944_SAMP_FAST_MODE) { + /* + * Find how much shift is required by the sample rate + * The chip's sampling rate is 2^shift samples/sec + */ + shift = shift_map_tbl[smpl_mode]; + inc = tmp_s64 >> shift; + } else if (smpl_mode <= PAC1944_SAMP_BURST_MODE) { + idx = info->num_enabled_channels - 1; + + if (smpl_mode == PAC1944_SAMP_FAST_MODE) + fs = samp_rate_fast_mode_tbl[idx]; + else + /* smpl_mode == PAC1944_SAMP_BURST_MODE) */ + fs = samp_rate_burst_mode_tbl[idx]; + + inc = div_u64(abs(tmp_s64), fs); + if (tmp_s64 < 0) + inc = -inc; + } else { + dev_err(dev, "Invalid sample rate index: %d!\n", smpl_mode); + return -EIO; + } + } else { + count = info->chip_reg_data.total_samples_nr[cnt]; + inc_count = info->chip_reg_data.acc_count; + + /* Check if total number of samples will overflow */ + if (check_add_overflow(count, inc_count, &count)) { + dev_err(dev, "Number of samples on channel [%d] overflow!\n", + cnt + 1); + info->chip_reg_data.total_samples_nr[cnt] = 0; + info->chip_reg_data.acc_val[cnt] = 0; + } + + info->chip_reg_data.total_samples_nr[cnt] += inc_count; + + inc = info->chip_reg_data.vacc[cnt]; + } + + if (check_add_overflow(stored_value, inc, &stored_value)) { + if (is_negative(stored_value)) + info->chip_reg_data.acc_val[cnt] = S64_MIN; + else + info->chip_reg_data.acc_val[cnt] = S64_MAX; + + dev_err(dev, "Overflow detected on channel [%d]!\n", cnt + 1); + } else { + info->chip_reg_data.acc_val[cnt] += inc; + } + + offset_reg_data_p += PAC1944_VACC_REG_LEN; + } + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + info->chip_reg_data.vbus[cnt] = get_unaligned_be16(offset_reg_data_p); + + if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + info->chip_reg_data.vbus[cnt] = + sign_extend32(info->chip_reg_data.vbus[cnt], 15); + + offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN; + } + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + info->chip_reg_data.vsense[cnt] = get_unaligned_be16(offset_reg_data_p); + + if (info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + info->chip_reg_data.vsense[cnt] = + sign_extend32(info->chip_reg_data.vsense[cnt], 15); + + offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN; + } + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + info->chip_reg_data.vbus_avg[cnt] = get_unaligned_be16(offset_reg_data_p); + + if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + info->chip_reg_data.vbus_avg[cnt] = + sign_extend32(info->chip_reg_data.vbus_avg[cnt], 15); + + offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN; + } + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + info->chip_reg_data.vsense_avg[cnt] = get_unaligned_be16(offset_reg_data_p); + + if (info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + info->chip_reg_data.vsense_avg[cnt] = + sign_extend32(info->chip_reg_data.vsense_avg[cnt], 15); + + offset_reg_data_p += PAC1944_VBUS_SENSE_REG_LEN; + } + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + info->chip_reg_data.vpower[cnt] = get_unaligned_be32(offset_reg_data_p) >> 2; + + if (info->chip_reg_data.vbus_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG || + info->chip_reg_data.vsense_mode[cnt] != PAC1944_UNIPOLAR_FSR_CFG) + info->chip_reg_data.vpower[cnt] = + sign_extend32(info->chip_reg_data.vpower[cnt], 29); + + offset_reg_data_p += PAC1944_VPOWER_REG_LEN; + } + + return 0; +} + +static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time) +{ + int ret = 0; + + /* + * Check if the minimal elapsed time has passed and if so, + * re-read the chip, otherwise the cached info is just fine + */ + if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp + + msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) { + /* + * We need to re-read the chip values + * call the pac1944_reg_snapshot + */ + ret = pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR, + wait_time); + /* + * Re-schedule the work for the read registers timeout + * (to prevent chip regs saturation) + */ + cancel_delayed_work_sync(&info->work_chip_rfsh); + schedule_delayed_work(&info->work_chip_rfsh, + msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS)); + } + + return ret; +} + +static ssize_t pac1944_in_power_acc_raw_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int ret; + s64 curr_energy, int_part; + int rem; + + ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); + if (ret < 0) + return 0; + + /* + * Expresses the 64 bit energy value as a + * 64 bit integer and a 32 bit nano value + */ + curr_energy = info->chip_reg_data.acc_val[this_attr->address]; + int_part = div_s64_rem(curr_energy, 1000000000, &rem); + + if (rem < 0) + return sysfs_emit(buf, "-%lld.%09u\n", abs(int_part), -rem); + else + return sysfs_emit(buf, "%lld.%09u\n", int_part, abs(rem)); +} + +static ssize_t pac1944_in_power_acc_scale_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + unsigned int shunt, rem; + u64 tmp, ref; + + if (info->is_pac195x_family) + ref = PAC195X_MAX_VPOWER_RSHIFTED_BY_29B; + else + ref = PAC194X_MAX_VPOWER_RSHIFTED_BY_29B; + + if ((info->chip_reg_data.vbus_mode[this_attr->address] == PAC1944_UNIPOLAR_FSR_CFG && + info->chip_reg_data.vsense_mode[this_attr->address] == PAC1944_UNIPOLAR_FSR_CFG) || + info->chip_reg_data.vbus_mode[this_attr->address] == PAC1944_BIPOLAR_HALF_FSR_CFG || + info->chip_reg_data.vsense_mode[this_attr->address] == PAC1944_BIPOLAR_HALF_FSR_CFG) + ref = ref >> 1; + + shunt = info->shunts[this_attr->address]; + + tmp = div_u64(ref * 1000000000LL, shunt); + rem = do_div(tmp, 1000000000LL); + + return sysfs_emit(buf, "%lld.%09u\n", tmp, rem); +} + +static ssize_t pac1944_in_enable_acc_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + + return sysfs_emit(buf, "%d\n", info->enable_acc[this_attr->address]); +} + +static ssize_t pac1944_in_enable_acc_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int val; + + if (kstrtouint(buf, 10, &val)) { + dev_err(dev, "Value is not valid\n"); + return -EINVAL; + } + + scoped_guard(mutex, &info->lock) { + info->enable_acc[this_attr->address] = val ? true : false; + if (val) { + info->chip_reg_data.acc_val[this_attr->address] = 0; + info->chip_reg_data.total_samples_nr[this_attr->address] = 0; + } + } + + return count; +} + +static ssize_t pac1944_in_current_acc_raw_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int ret; + + ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); + if (ret < 0) + return ret; + + return sysfs_emit(buf, "%lld\n", info->chip_reg_data.acc_val[this_attr->address]); +} + +static ssize_t pac1944_in_current_acc_scale_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int shunt, rem; + u64 tmp_u64, ref; + + /* + * Currents - scale for mA - depends on the channel's shunt value + * (100mV * 1000000) / (2^16 * shunt(uOhm)) + */ + ref = PAC1944_MAX_VSENSE_NANO; + + switch (info->chip_reg_data.vsense_mode[this_attr->address]) { + case PAC1944_UNIPOLAR_FSR_CFG: + case PAC1944_BIPOLAR_HALF_FSR_CFG: + shunt = info->shunts[this_attr->address]; + break; + case PAC1944_BIPOLAR_FSR_CFG: + ref = ref << 1; + shunt = info->shunts[this_attr->address]; + break; + default: + return -EINVAL; + } + + /* + * Increasing precision + * (100mV * 1000000 * 1000000000) / 2^16 ) + */ + tmp_u64 = div_u64(ref, shunt); + rem = do_div(tmp_u64, 1000000000LL); + + return sysfs_emit(buf, "%lld.%09u\n", tmp_u64, rem); +} + +static ssize_t pac1944_in_voltage_acc_raw_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int ret; + s64 acc_voltage; + u32 samples_count; + u64 tmp_u64; + + ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); + if (ret < 0) + return ret; + + acc_voltage = info->chip_reg_data.acc_val[this_attr->address]; + samples_count = info->chip_reg_data.total_samples_nr[this_attr->address]; + + tmp_u64 = div_u64(abs(acc_voltage), samples_count); + + if (is_negative(acc_voltage)) + return sysfs_emit(buf, "-%lld\n", tmp_u64); + else + return sysfs_emit(buf, "%lld\n", tmp_u64); +} + +static ssize_t pac1944_in_voltage_acc_scale_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); + int vals[2]; + unsigned long long tmp; + + if (info->is_pac195x_family) + vals[0] = PAC195X_VOLTAGE_MILLIVOLTS_MAX; + else + vals[0] = PAC194X_VOLTAGE_MILLIVOLTS_MAX; + + if (info->chip_reg_data.vbus_mode[this_attr->address] != PAC1944_BIPOLAR_FSR_CFG) + /* PAC1944_UNIPOLAR_FSR_CFG or PAC1944_BIPOLAR_HALF_FSR_CFG */ + vals[1] = PAC1944_VOLTAGE_16B_RES; + else + vals[1] = PAC1944_VOLTAGE_15B_RES; + + tmp = (s64)vals[0] * 1000000000LL >> vals[1]; + vals[1] = do_div(tmp, 1000000000LL); + vals[0] = tmp; + + return sysfs_emit(buf, "%d.%09u\n", vals[0], vals[1]); +} + +static IIO_DEVICE_ATTR(in_energy1_raw, 0444, pac1944_in_power_acc_raw_show, NULL, 0); +static IIO_DEVICE_ATTR(in_energy2_raw, 0444, pac1944_in_power_acc_raw_show, NULL, 1); +static IIO_DEVICE_ATTR(in_energy3_raw, 0444, pac1944_in_power_acc_raw_show, NULL, 2); +static IIO_DEVICE_ATTR(in_energy4_raw, 0444, pac1944_in_power_acc_raw_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_energy1_scale, 0444, pac1944_in_power_acc_scale_show, NULL, 0); +static IIO_DEVICE_ATTR(in_energy2_scale, 0444, pac1944_in_power_acc_scale_show, NULL, 1); +static IIO_DEVICE_ATTR(in_energy3_scale, 0444, pac1944_in_power_acc_scale_show, NULL, 2); +static IIO_DEVICE_ATTR(in_energy4_scale, 0444, pac1944_in_power_acc_scale_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_energy1_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 0); +static IIO_DEVICE_ATTR(in_energy2_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 1); +static IIO_DEVICE_ATTR(in_energy3_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 2); +static IIO_DEVICE_ATTR(in_energy4_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 3); + +static IIO_DEVICE_ATTR(in_current_acc1_raw, 0444, pac1944_in_current_acc_raw_show, NULL, 0); +static IIO_DEVICE_ATTR(in_current_acc2_raw, 0444, pac1944_in_current_acc_raw_show, NULL, 1); +static IIO_DEVICE_ATTR(in_current_acc3_raw, 0444, pac1944_in_current_acc_raw_show, NULL, 2); +static IIO_DEVICE_ATTR(in_current_acc4_raw, 0444, pac1944_in_current_acc_raw_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_current_acc1_scale, 0444, pac1944_in_current_acc_scale_show, NULL, 0); +static IIO_DEVICE_ATTR(in_current_acc2_scale, 0444, pac1944_in_current_acc_scale_show, NULL, 1); +static IIO_DEVICE_ATTR(in_current_acc3_scale, 0444, pac1944_in_current_acc_scale_show, NULL, 2); +static IIO_DEVICE_ATTR(in_current_acc4_scale, 0444, pac1944_in_current_acc_scale_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_current_acc1_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 0); +static IIO_DEVICE_ATTR(in_current_acc2_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 1); +static IIO_DEVICE_ATTR(in_current_acc3_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 2); +static IIO_DEVICE_ATTR(in_current_acc4_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 3); + +static IIO_DEVICE_ATTR(in_voltage_acc1_raw, 0444, pac1944_in_voltage_acc_raw_show, NULL, 0); +static IIO_DEVICE_ATTR(in_voltage_acc2_raw, 0444, pac1944_in_voltage_acc_raw_show, NULL, 1); +static IIO_DEVICE_ATTR(in_voltage_acc3_raw, 0444, pac1944_in_voltage_acc_raw_show, NULL, 2); +static IIO_DEVICE_ATTR(in_voltage_acc4_raw, 0444, pac1944_in_voltage_acc_raw_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_voltage_acc1_scale, 0444, pac1944_in_voltage_acc_scale_show, NULL, 0); +static IIO_DEVICE_ATTR(in_voltage_acc2_scale, 0444, pac1944_in_voltage_acc_scale_show, NULL, 1); +static IIO_DEVICE_ATTR(in_voltage_acc3_scale, 0444, pac1944_in_voltage_acc_scale_show, NULL, 2); +static IIO_DEVICE_ATTR(in_voltage_acc4_scale, 0444, pac1944_in_voltage_acc_scale_show, NULL, 3); + +static IIO_DEVICE_ATTR(in_voltage_acc1_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 0); +static IIO_DEVICE_ATTR(in_voltage_acc2_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 1); +static IIO_DEVICE_ATTR(in_voltage_acc3_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 2); +static IIO_DEVICE_ATTR(in_voltage_acc4_en, 0644, pac1944_in_enable_acc_show, + pac1944_in_enable_acc_store, 3); + +static struct attribute *pac1944_power_acc_attr[] = { + &iio_dev_attr_in_energy1_raw.dev_attr.attr, + &iio_dev_attr_in_energy2_raw.dev_attr.attr, + &iio_dev_attr_in_energy3_raw.dev_attr.attr, + &iio_dev_attr_in_energy4_raw.dev_attr.attr, + &iio_dev_attr_in_energy1_scale.dev_attr.attr, + &iio_dev_attr_in_energy2_scale.dev_attr.attr, + &iio_dev_attr_in_energy3_scale.dev_attr.attr, + &iio_dev_attr_in_energy4_scale.dev_attr.attr, + &iio_dev_attr_in_energy1_en.dev_attr.attr, + &iio_dev_attr_in_energy2_en.dev_attr.attr, + &iio_dev_attr_in_energy3_en.dev_attr.attr, + &iio_dev_attr_in_energy4_en.dev_attr.attr, + NULL +}; + +static struct attribute *pac1944_current_acc_attr[] = { + &iio_dev_attr_in_current_acc1_raw.dev_attr.attr, + &iio_dev_attr_in_current_acc2_raw.dev_attr.attr, + &iio_dev_attr_in_current_acc3_raw.dev_attr.attr, + &iio_dev_attr_in_current_acc4_raw.dev_attr.attr, + &iio_dev_attr_in_current_acc1_scale.dev_attr.attr, + &iio_dev_attr_in_current_acc2_scale.dev_attr.attr, + &iio_dev_attr_in_current_acc3_scale.dev_attr.attr, + &iio_dev_attr_in_current_acc4_scale.dev_attr.attr, + &iio_dev_attr_in_current_acc1_en.dev_attr.attr, + &iio_dev_attr_in_current_acc2_en.dev_attr.attr, + &iio_dev_attr_in_current_acc3_en.dev_attr.attr, + &iio_dev_attr_in_current_acc4_en.dev_attr.attr, + NULL +}; + +static struct attribute *pac1944_voltage_acc_attr[] = { + &iio_dev_attr_in_voltage_acc1_raw.dev_attr.attr, + &iio_dev_attr_in_voltage_acc2_raw.dev_attr.attr, + &iio_dev_attr_in_voltage_acc3_raw.dev_attr.attr, + &iio_dev_attr_in_voltage_acc4_raw.dev_attr.attr, + &iio_dev_attr_in_voltage_acc1_scale.dev_attr.attr, + &iio_dev_attr_in_voltage_acc2_scale.dev_attr.attr, + &iio_dev_attr_in_voltage_acc3_scale.dev_attr.attr, + &iio_dev_attr_in_voltage_acc4_scale.dev_attr.attr, + &iio_dev_attr_in_voltage_acc1_en.dev_attr.attr, + &iio_dev_attr_in_voltage_acc2_en.dev_attr.attr, + &iio_dev_attr_in_voltage_acc3_en.dev_attr.attr, + &iio_dev_attr_in_voltage_acc4_en.dev_attr.attr, + NULL +}; + +static int pac1944_prep_custom_attributes(struct pac1944_chip_info *info, + struct iio_dev *indio_dev) +{ + int ch, i, j; + int active_channels_count = 0; + struct attribute **pac1944_custom_attrs, **tmp_attr; + struct attribute_group *pac1944_group; + int custom_attr_cnt; + struct i2c_client *client = info->client; + + active_channels_count = info->num_enabled_channels; + + pac1944_group = devm_kzalloc(&client->dev, sizeof(*pac1944_group), GFP_KERNEL); + if (!pac1944_group) + return -ENOMEM; + + /* + * Attributes for channel X: + * - in_shunt_value_X + * - one of pair attributes: + * - in_power_accX_raw and in_power_accX_scale + * - in_current_accX_raw and in_current_accX_scale + * - in_voltage_accX_raw and in_voltage_accX_scale + * Shared attributes: + * - slow_alert1_cfg + * - gpio_alert2_cfg + * NULL + */ + custom_attr_cnt = PAC1944_COMMON_DEVATTR * active_channels_count; + custom_attr_cnt += PAC1944_ACC_DEVATTR * active_channels_count; + custom_attr_cnt += PAC1944_SHARED_DEVATTRS_COUNT; + + pac1944_custom_attrs = devm_kzalloc(&client->dev, custom_attr_cnt * + sizeof(*pac1944_group) + 1, GFP_KERNEL); + if (!pac1944_custom_attrs) + return -ENOMEM; + + j = 0; + + for_each_set_bit(ch, &info->active_channels_mask, info->phys_channels) { + for (i = 0; i < PAC1944_COMMON_DEVATTR; i++) + pac1944_custom_attrs[j++] = + pac1944_all_attrs[PAC1944_COMMON_DEVATTR * ch + i]; + + switch (info->chip_reg_data.accumulation_mode[ch]) { + case PAC1944_ACCMODE_VPOWER: + tmp_attr = pac1944_power_acc_attr; + break; + case PAC1944_ACCMODE_VSENSE: + tmp_attr = pac1944_current_acc_attr; + break; + case PAC1944_ACCMODE_VBUS: + tmp_attr = pac1944_voltage_acc_attr; + break; + default: + return -EINVAL; + } + + pac1944_custom_attrs[j++] = tmp_attr[ch]; + pac1944_custom_attrs[j++] = pac1944_power_acc_attr[PAC1944_MAX_CH + ch]; + pac1944_custom_attrs[j++] = pac1944_power_acc_attr[2 * PAC1944_MAX_CH + ch]; + } + + for (i = 0; i < PAC1944_SHARED_DEVATTRS_COUNT; i++) + pac1944_custom_attrs[j++] = + pac1944_all_attrs[PAC1944_COMMON_DEVATTR * PAC1944_MAX_CH + i]; + + pac1944_group->attrs = pac1944_custom_attrs; + info->iio_info.attrs = pac1944_group; + + return 0; +} + +static int pac1944_frequency_set(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int mode) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int ret; + u16 tmp_u16; + __be16 tmp_be16; + + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_ACT_REG_ADDR, + sizeof(tmp_u16), (u8 *)&tmp_be16); + if (ret < 0) { + dev_err(&client->dev, "cannot read PAC1944 regs from 0x%02X\n", + PAC1944_CTRL_ACT_REG_ADDR); + return ret; + } + + tmp_u16 = be16_to_cpu(tmp_be16); + tmp_u16 &= ~PAC1944_CTRL_SAMPLE_MASK; + tmp_u16 |= FIELD_PREP(PAC1944_CTRL_SAMPLE_MASK, mode); + tmp_be16 = cpu_to_be16(tmp_u16); + + scoped_guard(mutex, &info->lock) { + ret = i2c_smbus_write_word_data(client, PAC1944_CTRL_REG_ADDR, tmp_be16); + if (ret < 0) { + dev_err(&client->dev, "Failed to configure sampling mode\n"); + return ret; + } + + info->sampling_mode = mode; + info->chip_reg_data.ctrl_act_reg = tmp_u16; + } + + return pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); +} + +static int pac1944_frequency_get(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + + return info->sampling_mode; +} + +static const struct iio_enum sampling_mode_enum = { + .items = pac1944_frequency_avail, + .num_items = ARRAY_SIZE(pac1944_frequency_avail), + .set = pac1944_frequency_set, + .get = pac1944_frequency_get, +}; + +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = { + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, &sampling_mode_enum), + { + .name = "sampling_frequency_available", + .shared = IIO_SHARED_BY_ALL, + .read = iio_enum_available_read, + .private = (uintptr_t)&sampling_mode_enum, + }, + { } +}; + +static int pac1944_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + int ret, idx; + u64 tmp; + + ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); + if (ret < 0) + return ret; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + /* + * For AVG the index should be between 5 to 8. To calculate + * PAC1944_CH_VOLTAGE_AVERAGE and PAC1944_CH_CURRENT_AVERAGE real index, + * we need to remove the added offset (PAC1944_MAX_CH). + */ + if (idx >= PAC1944_MAX_CH) + idx = idx - PAC1944_MAX_CH; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_VOLTAGE: + *val = info->chip_reg_data.vbus[idx]; + return IIO_VAL_INT; + case IIO_CURRENT: + *val = info->chip_reg_data.vsense[idx]; + return IIO_VAL_INT; + case IIO_POWER: + *val = info->chip_reg_data.vpower[idx]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_AVERAGE_RAW: + switch (chan->type) { + case IIO_VOLTAGE: + *val = info->chip_reg_data.vbus_avg[idx]; + return IIO_VAL_INT; + case IIO_CURRENT: + *val = info->chip_reg_data.vsense_avg[idx]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->address) { + case PAC1944_VBUS_1_ADDR: + case PAC1944_VBUS_2_ADDR: + case PAC1944_VBUS_3_ADDR: + case PAC1944_VBUS_4_ADDR: + case PAC1944_VBUS_AVG_1_ADDR: + case PAC1944_VBUS_AVG_2_ADDR: + case PAC1944_VBUS_AVG_3_ADDR: + case PAC1944_VBUS_AVG_4_ADDR: + if (info->is_pac195x_family) + *val = PAC195X_VOLTAGE_MILLIVOLTS_MAX; + else + *val = PAC194X_VOLTAGE_MILLIVOLTS_MAX; + + switch (info->chip_reg_data.vbus_mode[idx]) { + case PAC1944_UNIPOLAR_FSR_CFG: + case PAC1944_BIPOLAR_HALF_FSR_CFG: + *val2 = PAC1944_VOLTAGE_16B_RES; + break; + case PAC1944_BIPOLAR_FSR_CFG: + *val2 = PAC1944_VOLTAGE_15B_RES; + break; + default: + return -EINVAL; + } + return IIO_VAL_FRACTIONAL_LOG2; + /* + * Currents - scale for mA - depends on the + * channel's shunt value + * (100mV * 1000000) / (2^16 * shunt(microOhm)) + */ + case PAC1944_VSENSE_1_ADDR: + case PAC1944_VSENSE_2_ADDR: + case PAC1944_VSENSE_3_ADDR: + case PAC1944_VSENSE_4_ADDR: + case PAC1944_VSENSE_AVG_1_ADDR: + case PAC1944_VSENSE_AVG_2_ADDR: + case PAC1944_VSENSE_AVG_3_ADDR: + case PAC1944_VSENSE_AVG_4_ADDR: + *val = PAC1944_MAX_VSENSE_RSHIFTED_BY_15B; + switch (info->chip_reg_data.vsense_mode[idx]) { + case PAC1944_UNIPOLAR_FSR_CFG: + case PAC1944_BIPOLAR_HALF_FSR_CFG: + *val = *val >> 1; + *val2 = info->shunts[idx]; + break; + case PAC1944_BIPOLAR_FSR_CFG: + *val2 = info->shunts[idx]; + break; + default: + return -EINVAL; + } + return IIO_VAL_FRACTIONAL; + /* + * Power - mW - it will use the combined scale + * for current and voltage + * current(mA) * voltage(mV) = power (uW) + */ + case PAC1944_VPOWER_1_ADDR: + case PAC1944_VPOWER_2_ADDR: + case PAC1944_VPOWER_3_ADDR: + case PAC1944_VPOWER_4_ADDR: + if (info->is_pac195x_family) + tmp = PAC195X_PRODUCT_VOLTAGE_PV_FSR; + else + tmp = PAC194X_PRODUCT_VOLTAGE_PV_FSR; + + do_div(tmp, info->shunts[idx]); + *val = (int)tmp; + if ((info->chip_reg_data.vbus_mode[idx] == PAC1944_UNIPOLAR_FSR_CFG && + info->chip_reg_data.vsense_mode[idx] == PAC1944_UNIPOLAR_FSR_CFG) || + info->chip_reg_data.vbus_mode[idx] == PAC1944_BIPOLAR_HALF_FSR_CFG || + info->chip_reg_data.vsense_mode[idx] == PAC1944_BIPOLAR_HALF_FSR_CFG) + *val2 = PAC1944_POWER_30B_RES; + else + *val2 = PAC1944_POWER_29B_RES; + + return IIO_VAL_FRACTIONAL_LOG2; + } + } + + return -EINVAL; +} + +static int pac1944_read_label(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, char *label) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + int idx; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + /* + * For AVG the index should be between 5 to 8. + * To calculate PAC1944_CH_VOLTAGE_AVERAGE and + * PAC1944_CH_CURRENT_AVERAGE real index, we need + * to remove the added offset (PAC1944_MAX_CH). + */ + if (idx >= PAC1944_MAX_CH) + idx = idx - PAC1944_MAX_CH; + + switch (chan->address) { + case PAC1944_VBUS_1_ADDR: + case PAC1944_VBUS_2_ADDR: + case PAC1944_VBUS_3_ADDR: + case PAC1944_VBUS_4_ADDR: + if (info->labels[idx]) + return sysfs_emit(label, "%s_VBUS_%d\n", info->labels[idx], idx + 1); + else + return sysfs_emit(label, "VBUS_%d\n", idx + 1); + case PAC1944_VBUS_AVG_1_ADDR: + case PAC1944_VBUS_AVG_2_ADDR: + case PAC1944_VBUS_AVG_3_ADDR: + case PAC1944_VBUS_AVG_4_ADDR: + if (info->labels[idx]) + return sysfs_emit(label, "%s_VBUS_AVG_%d\n", info->labels[idx], idx + 1); + else + return sysfs_emit(label, "VBUS_AVG_%d\n", idx + 1); + case PAC1944_VSENSE_1_ADDR: + case PAC1944_VSENSE_2_ADDR: + case PAC1944_VSENSE_3_ADDR: + case PAC1944_VSENSE_4_ADDR: + if (info->labels[idx]) + return sysfs_emit(label, "%s_IBUS_%d\n", info->labels[idx], idx + 1); + else + return sysfs_emit(label, "IBUS_%d\n", idx + 1); + case PAC1944_VSENSE_AVG_1_ADDR: + case PAC1944_VSENSE_AVG_2_ADDR: + case PAC1944_VSENSE_AVG_3_ADDR: + case PAC1944_VSENSE_AVG_4_ADDR: + if (info->labels[idx]) + return sysfs_emit(label, "%s_IBUS_AVG_%d\n", info->labels[idx], idx + 1); + else + return sysfs_emit(label, "IBUS_AVG_%d\n", idx + 1); + case PAC1944_VPOWER_1_ADDR: + case PAC1944_VPOWER_2_ADDR: + case PAC1944_VPOWER_3_ADDR: + case PAC1944_VPOWER_4_ADDR: + if (info->labels[idx]) + return sysfs_emit(label, "%s_POWER_%d\n", info->labels[idx], idx + 1); + else + return sysfs_emit(label, "POWER_%d\n", idx + 1); + } + + return 0; +} + +static int pac1944_read_thresh(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, enum iio_event_type type, + enum iio_event_direction dir, enum iio_event_info info, + int *val, int *val2) +{ + struct pac1944_chip_info *chip_info = iio_priv(indio_dev); + int idx; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + guard(mutex)(&chip_info->lock); + + switch (chan->type) { + case IIO_VOLTAGE: + switch (dir) { + case IIO_EV_DIR_RISING: + *val = chip_info->overvoltage[idx]; + return IIO_VAL_INT; + case IIO_EV_DIR_FALLING: + *val = chip_info->undervoltage[idx]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CURRENT: + switch (dir) { + case IIO_EV_DIR_RISING: + *val = chip_info->overcurrent[idx]; + return IIO_VAL_INT; + case IIO_EV_DIR_FALLING: + *val = chip_info->undercurrent[idx]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_POWER: + switch (dir) { + case IIO_EV_DIR_RISING: + *val = chip_info->overpower[idx]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int pac1944_write_thresh(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, enum iio_event_type type, + enum iio_event_direction dir, enum iio_event_info info, + int val, int val2) +{ + struct pac1944_chip_info *chip_info = iio_priv(indio_dev); + int idx, ret; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + guard(mutex)(&chip_info->lock); + + switch (chan->type) { + case IIO_VOLTAGE: + switch (dir) { + case IIO_EV_DIR_RISING: + ret = pac1944_update_alert_16b(&indio_dev->dev, + PAC1944_OV_LIMIT_REG_ADDR + idx, + pac1944_overvoltage_mask_tbl[idx], val); + if (ret) + return ret; + + chip_info->overvoltage[idx] = val; + return 0; + case IIO_EV_DIR_FALLING: + ret = pac1944_update_alert_16b(&indio_dev->dev, + PAC1944_UV_LIMIT_REG_ADDR + idx, + pac1944_undervoltage_mask_tbl[idx], val); + if (ret) + return ret; + + chip_info->undervoltage[idx] = val; + return 0; + default: + return -EINVAL; + } + case IIO_CURRENT: + switch (dir) { + case IIO_EV_DIR_RISING: + ret = pac1944_update_alert_16b(&indio_dev->dev, + PAC1944_OC_LIMIT_REG_ADDR + idx, + pac1944_overcurrent_mask_tbl[idx], val); + if (ret) + return ret; + + chip_info->overcurrent[idx] = val; + return ret; + case IIO_EV_DIR_FALLING: + ret = pac1944_update_alert_16b(&indio_dev->dev, + PAC1944_UC_LIMIT_REG_ADDR + idx, + pac1944_undercurrent_mask_tbl[idx], val); + if (ret) + return ret; + + chip_info->undercurrent[idx] = val; + return 0; + default: + return -EINVAL; + } + case IIO_POWER: + switch (dir) { + case IIO_EV_DIR_RISING: + ret = pac1944_update_alert_24b(&indio_dev->dev, + PAC1944_OP_LIMIT_REG_ADDR + idx, + pac1944_overpower_mask_tbl[idx], + val); + if (ret) + return ret; + + chip_info->overpower[idx] = val; + return 0; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int pac1944_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + int idx; + u32 tmp; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + scoped_guard(mutex, &info->lock) { + switch (chan->type) { + case IIO_VOLTAGE: + switch (dir) { + case IIO_EV_DIR_RISING: + tmp = FIELD_GET(PAC1944_OV_MASK, info->alert_enable); + break; + case IIO_EV_DIR_FALLING: + tmp = FIELD_GET(PAC1944_UV_MASK, info->alert_enable); + break; + default: + return -EINVAL; + } + return -EINVAL; + case IIO_CURRENT: + switch (dir) { + case IIO_EV_DIR_RISING: + tmp = FIELD_GET(PAC1944_OC_MASK, info->alert_enable); + break; + case IIO_EV_DIR_FALLING: + tmp = FIELD_GET(PAC1944_UC_MASK, info->alert_enable); + break; + default: + return -EINVAL; + } + return -EINVAL; + case IIO_POWER: + if (dir == IIO_EV_DIR_RISING) + tmp = FIELD_GET(PAC1944_OP_MASK, info->alert_enable); + else + return -EINVAL; + break; + default: + return -EINVAL; + } + } + + tmp = tmp >> (3 - idx); + + return tmp & 0x01; +} + +static int pac1944_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + bool state) +{ + struct pac1944_chip_info *info = iio_priv(indio_dev); + struct i2c_client *client = info->client; + int idx, val, mask, ret; + bool update = false; + u8 tmp[PAC1944_ALERT_ENABLE_REG_LEN]; + + /* into the datasheet channels are noted from 1 to 4 */ + idx = chan->channel - 1; + + switch (chan->type) { + case IIO_VOLTAGE: + switch (dir) { + case IIO_EV_DIR_RISING: + mask = pac1944_overvoltage_mask_tbl[idx]; + break; + case IIO_EV_DIR_FALLING: + mask = pac1944_undervoltage_mask_tbl[idx]; + break; + default: + return -EINVAL; + } + break; + case IIO_CURRENT: + switch (dir) { + case IIO_EV_DIR_RISING: + mask = pac1944_overcurrent_mask_tbl[idx]; + break; + case IIO_EV_DIR_FALLING: + mask = pac1944_undercurrent_mask_tbl[idx]; + break; + default: + return -EINVAL; + } + break; + case IIO_POWER: + if (dir != IIO_EV_DIR_RISING) + return -EINVAL; + + mask = pac1944_overpower_mask_tbl[idx]; + break; + default: + return -EINVAL; + } + + guard(mutex)(&info->lock); + + val = info->alert_enable & mask; + if (state && !val) { + /* enable the event in hardware */ + info->alert_enable |= mask; + update = true; + } else if (!state && val) { + /* disable the event in hardware */ + info->alert_enable &= ~mask; + update = true; + } + + /* do not update if not needed */ + if (update) { + put_unaligned_be24(info->alert_enable, &tmp[0]); + + /* update the Alert enable register */ + ret = pac1944_restore_alert_reg(indio_dev, &tmp[0]); + if (ret) { + dev_err(&client->dev, "failing to restore alert reg\n"); + return ret; + } + } + + return 0; +} + +static void pac1944_work_periodic_rfsh(struct work_struct *work) +{ + struct pac1944_chip_info *info = to_pac1944_chip_info(to_delayed_work(work)); + struct i2c_client *client = info->client; + + dev_dbg(&client->dev, "Periodic refresh\n"); + + pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR, + PAC1944_MIN_UPDATE_WAIT_TIME_US); + + schedule_delayed_work(&info->work_chip_rfsh, + msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS)); +} + +/* + * Universal Unique Identifier (UUID), + * 721F1534-5D27-4B60-9DF4-41A3C4B7DA3A, + * is reserved to Microchip for the PAC194x and PAC195x. + */ +#define PAC1944_DSM_UUID "721F1534-5D27-4B60-9DF4-41A3C4B7DA3A" + +/* + * documentation related to the ACPI device definition + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/UserGuides/PAC194X_5X-UEFI-BIOS-Integration-and-Microsoft-Windows-10-and-Windows-11-Energy-Meter-Interface-Device-Driver-Users-Guide-DS50003155.pdf + */ +static int pac1944_acpi_parse_channel_config(struct i2c_client *client, + struct pac1944_chip_info *info) +{ + acpi_handle handle; + union acpi_object *rez; + struct device *dev = &client->dev; + unsigned short bi_dir_mask; + int i; + guid_t guid; + const struct acpi_device_id *id; + + handle = ACPI_HANDLE(dev); + + id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!id) + return -ENODEV; + + guid_parse(PAC1944_DSM_UUID, &guid); + + rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1944_ACPI_GET_NAMES, NULL); + if (!rez) + return -EINVAL; + + for (i = 0; i < rez->package.count; i++) { + info->labels[i] = devm_kmemdup(dev, rez->package.elements[i].string.pointer, + (size_t)rez->package.elements[i].string.length + 1, + GFP_KERNEL); + info->labels[i][rez->package.elements[i].string.length] = '\0'; + } + + ACPI_FREE(rez); + + rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1944_ACPI_GET_UOHMS_VALS, NULL); + if (!rez) + return -EINVAL; + + for (i = 0; i < rez->package.count; i++) { + info->shunts[i] = rez->package.elements[i].integer.value; + if (info->shunts[i]) + set_bit(i, &info->active_channels_mask); + } + + ACPI_FREE(rez); + + rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1944_ACPI_GET_BIPOLAR_SETTINGS, NULL); + if (!rez) + return -EINVAL; + + for_each_set_bit(i, &info->active_channels_mask, info->phys_channels) { + bi_dir_mask = rez->package.elements[i].integer.value; + + if (bi_dir_mask == PAC1944_UNIPOLAR_FSR_CFG || + bi_dir_mask == PAC1944_BIPOLAR_FSR_CFG || + bi_dir_mask == PAC1944_BIPOLAR_HALF_FSR_CFG) { + dev_dbg(dev, "VBUS{%d} mode set to: %d\n", i, bi_dir_mask); + info->chip_reg_data.vbus_mode[i] = bi_dir_mask; + } else { + return dev_err_probe(dev, -EINVAL, "invalid vbus-mode value on %i\n", i); + } + + bi_dir_mask = rez->package.elements[i + PAC1944_MAX_CH].integer.value; + + if (bi_dir_mask == PAC1944_UNIPOLAR_FSR_CFG || + bi_dir_mask == PAC1944_BIPOLAR_FSR_CFG || + bi_dir_mask == PAC1944_BIPOLAR_HALF_FSR_CFG) { + dev_dbg(dev, "VSENSE{%d} mode set to: %d\n", i, bi_dir_mask); + info->chip_reg_data.vsense_mode[i] = bi_dir_mask; + } else { + return dev_err_probe(dev, -EINVAL, "invalid vsense-mode value on %i\n", i); + } + } + + ACPI_FREE(rez); + + rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1944_ACPI_GET_SAMP, NULL); + if (!rez) + return -EINVAL; + + info->sample_rate_value = rez->package.elements[0].integer.value; + + ACPI_FREE(rez); + + return 0; +} + +static int pac1944_of_parse_channel_config(struct i2c_client *client, + struct pac1944_chip_info *info) +{ + unsigned int current_channel; + struct device *dev = &client->dev; + int idx, ret, temp; + bool is_bipolar, is_half_fsr; + + current_channel = 1; + + device_for_each_child_node_scoped(dev, child) { + ret = fwnode_property_read_u32(child, "reg", &idx); + if (ret) + return dev_err_probe(dev, ret, "reading invalid channel index\n"); + + /* adjust idx to match channel index (1 to 4) from the datasheet */ + idx--; + + if (current_channel >= (info->phys_channels + 1) || + idx >= info->phys_channels || idx < 0) + return dev_err_probe(dev, -EINVAL, "invalid channel index %d value\n", + idx + 1); + + /* enable channel */ + set_bit(idx, &info->active_channels_mask); + + ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", + &info->shunts[idx]); + if (ret) + return dev_err_probe(dev, ret, "%s: invalid shunt-resistor value: %d\n", + fwnode_get_name(child), info->shunts[idx]); + + if (fwnode_property_present(child, "label")) + fwnode_property_read_string(child, "label", + (const char **)&info->labels[idx]); + + is_bipolar = false; + if (fwnode_property_present(child, "microchip,vbus-bipolar")) + is_bipolar = true; + + is_half_fsr = false; + if (fwnode_property_present(child, "microchip,vbus-half-range")) + is_half_fsr = true; + + /* default value is unipolar and Full Scale Range */ + info->chip_reg_data.vbus_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; + if (is_half_fsr) + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; + else if (is_bipolar) + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; + + is_bipolar = false; + if (fwnode_property_present(child, "microchip,vsense-bipolar")) + is_bipolar = true; + + is_half_fsr = false; + if (fwnode_property_present(child, "microchip,vsense-half-range")) + is_half_fsr = true; + + /* default value is unipolar and Full Scale Range */ + info->chip_reg_data.vsense_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; + if (is_half_fsr) + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; + else if (is_bipolar) + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; + + ret = fwnode_property_read_u32(child, "microchip,accumulation-mode", &temp); + if (ret) + return dev_err_probe(dev, ret, "invalid accumulation-mode value on %s\n", + fwnode_get_name(child)); + if (temp == PAC1944_ACCMODE_VPOWER || + temp == PAC1944_ACCMODE_VSENSE || + temp == PAC1944_ACCMODE_VBUS) { + dev_dbg(dev, "Accumulation{%d} mode set to: %d\n", idx, temp); + info->chip_reg_data.accumulation_mode[idx] = temp; + } else { + return dev_err_probe(dev, -EINVAL, + "invalid mode for accumulator value on %s\n", + fwnode_get_name(child)); + } + current_channel++; + } + + return 0; +} + +static void pac1944_cancel_delayed_work(void *dwork) +{ + cancel_delayed_work_sync(dwork); +} + +static int pac1944_chip_identify(struct pac1944_chip_info *info) +{ + int ret = 0; + struct i2c_client *client = info->client; + u8 chip_rev_info[3]; + + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_PID_REG_ADDR, + sizeof(chip_rev_info), + chip_rev_info); + if (ret < 0) + return dev_err_probe(&client->dev, ret, "cannot read revision\n"); + + dev_info(&client->dev, "Chip revision: 0x%02X\n", chip_rev_info[2]); + info->chip_revision = chip_rev_info[2]; + info->chip_variant = chip_rev_info[0]; + + switch (chip_rev_info[0]) { + case PAC_PRODUCT_ID_1941: + case PAC_PRODUCT_ID_1942: + case PAC_PRODUCT_ID_1943: + case PAC_PRODUCT_ID_1944: + case PAC_PRODUCT_ID_1941_2: + case PAC_PRODUCT_ID_1942_2: + info->is_pac195x_family = false; + return chip_rev_info[0] - PAC_PRODUCT_ID_1941; + case PAC_PRODUCT_ID_1951: + case PAC_PRODUCT_ID_1952: + case PAC_PRODUCT_ID_1953: + case PAC_PRODUCT_ID_1954: + case PAC_PRODUCT_ID_1951_2: + case PAC_PRODUCT_ID_1952_2: + info->is_pac195x_family = true; + return (chip_rev_info[0] - PAC_PRODUCT_ID_1951) + + (PAC_PRODUCT_ID_1942_2 - PAC_PRODUCT_ID_1941) + 1; + default: + dev_err(&client->dev, + "product ID (0x%02X, 0x%02X, 0x%02X) for this part doesn't match\n", + chip_rev_info[0], chip_rev_info[1], chip_rev_info[2]); + return -EINVAL; + } +} + +static int pac1944_chip_configure(struct pac1944_chip_info *info) +{ + int cnt, ret; + struct i2c_client *client = info->client; + u8 regs[PAC1944_ALERTS_REG_LEN]; + u8 *offset_p; + u32 wait_time; + u8 tmp_u8; + __be16 tmp_be16; + u16 cfg; + + /* + * Counting how many channels are enabled and store + * this information within the driver data + */ + info->num_enabled_channels = hweight_long(info->active_channels_mask); + + /* get sampling rate from PAC */ + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_REG_ADDR, + sizeof(tmp_be16), (u8 *)&tmp_be16); + if (ret < 0) + return dev_err_probe(&client->dev, ret, "cannot read CTRL reg\n"); + + info->sampling_mode = FIELD_GET(PAC1944_CTRL_SAMPLE_MASK, be16_to_cpu(tmp_be16)); + + /* + * The current/voltage can be measured unidirectional, bidirectional or half FSR + * no SLOW triggered REFRESH, clear POR + */ + cfg = FIELD_PREP(PAC1944_NEG_PWR_CFG_VS1_MASK, info->chip_reg_data.vsense_mode[0]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS2_MASK, info->chip_reg_data.vsense_mode[1]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS3_MASK, info->chip_reg_data.vsense_mode[2]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS4_MASK, info->chip_reg_data.vsense_mode[3]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB1_MASK, info->chip_reg_data.vbus_mode[0]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB2_MASK, info->chip_reg_data.vbus_mode[1]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB3_MASK, info->chip_reg_data.vbus_mode[2]) | + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB4_MASK, info->chip_reg_data.vbus_mode[3]); + + ret = i2c_smbus_write_word_data(client, PAC1944_NEG_PWR_FSR_REG_ADDR, cpu_to_be16(cfg)); + if (ret) + return dev_err_probe(&client->dev, ret, "cannot write NEG_PWR_FSR reg\n"); + + ret = i2c_smbus_write_word_data(client, PAC1944_SLOW_REG_ADDR, 0); + if (ret) + return dev_err_probe(&client->dev, ret, "cannot write SLOW reg\n"); + + /* Write the CHANNEL_N_OFF from CTRL REGISTER */ + cfg = FIELD_PREP(PAC1944_CTRL_SAMPLE_MASK, info->sampling_mode) | + FIELD_PREP(PAC1944_CTRL_GPIO_ALERT2_MASK, 0) | + FIELD_PREP(PAC1944_CTRL_SLOW_ALERT1_MASK, 0) | + FIELD_PREP(PAC1944_CTRL_CH_1_OFF_MASK, !test_bit(0, &info->active_channels_mask)) | + FIELD_PREP(PAC1944_CTRL_CH_2_OFF_MASK, !test_bit(1, &info->active_channels_mask)) | + FIELD_PREP(PAC1944_CTRL_CH_3_OFF_MASK, !test_bit(2, &info->active_channels_mask)) | + FIELD_PREP(PAC1944_CTRL_CH_4_OFF_MASK, !test_bit(3, &info->active_channels_mask)); + + ret = i2c_smbus_write_word_data(client, PAC1944_CTRL_REG_ADDR, cpu_to_be16(cfg)); + if (ret) + return dev_err_probe(&client->dev, ret, "cannot write CTRL reg\n"); + + tmp_u8 = ACCUM_REG(info->chip_reg_data.accumulation_mode[0], + info->chip_reg_data.accumulation_mode[1], + info->chip_reg_data.accumulation_mode[2], + info->chip_reg_data.accumulation_mode[3]); + + ret = i2c_smbus_write_byte_data(client, PAC1944_ACCUM_CFG_REG_ADDR, tmp_u8); + if (ret) + return dev_err_probe(&client->dev, ret, "cannot write ACCUM_CFG reg\n"); + + /* reading all alerts, status and limits related registers */ + ret = pac1944_i2c_read(client, PAC1944_ALERT_STATUS_REG_ADDR, regs, sizeof(regs)); + if (ret < 0) + return dev_err_probe(&client->dev, ret, "cannot read ALERT_STATUS reg\n"); + + offset_p = ®s[0]; + + /* skip alert_status register*/ + offset_p += 3; + + info->slow_alert1 = get_unaligned_be24(offset_p); + offset_p += 3; + + info->gpio_alert2 = get_unaligned_be24(offset_p); + offset_p += 3; + + info->acc_fullness = get_unaligned_be16(offset_p); + offset_p += 2; + + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) { + info->overcurrent[cnt] = get_unaligned_be16(offset_p); + offset_p += 2; + } + + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) { + info->undercurrent[cnt] = get_unaligned_be16(offset_p); + offset_p += 2; + } + + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) { + info->overpower[cnt] = get_unaligned_be24(offset_p); + offset_p += 3; + } + + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) { + info->overvoltage[cnt] = get_unaligned_be16(offset_p); + offset_p += 2; + } + + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) { + info->undervoltage[cnt] = get_unaligned_be16(offset_p); + offset_p += 2; + } + + offset_p += 1; + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) + info->oc_limit_nsamples[cnt] = (*offset_p >> (2 * cnt)) & 0x03; + + offset_p += 1; + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) + info->uc_limit_nsamples[cnt] = (*offset_p >> (2 * cnt)) & 0x03; + + offset_p += 1; + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) + info->op_limit_nsamples[cnt] = (*offset_p >> (2 * cnt)) & 0x03; + + offset_p += 1; + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) + info->ov_limit_nsamples[cnt] = (*offset_p >> (2 * cnt)) & 0x03; + + offset_p += 1; + for (cnt = 0; cnt < PAC1944_MAX_CH; cnt++) + info->uv_limit_nsamples[cnt] = (*offset_p >> (2 * cnt)) & 0x03; + + offset_p += 1; + info->alert_enable = get_unaligned_be24(offset_p); + + /* + * Sending a REFRESH to the chip, so the new settings take place + * as well as resetting the accumulators + */ + ret = i2c_smbus_write_byte(client, PAC1944_REFRESH_REG_ADDR); + if (ret) + return dev_err_probe(&client->dev, ret, "cannot write REFRESH reg\n"); + + /* + * Get the current (in the chip) sampling speed and compute the + * required timeout based on its value the timeout is 1/sampling_speed + * wait the maximum amount of time to be on the safe side - the + * maximum wait time is for 8sps + */ + wait_time = (1024 / pac1944_samp_rate_map_tbl[info->sampling_mode]) * 1000; + usleep_range(wait_time, wait_time + 100); + + INIT_DELAYED_WORK(&info->work_chip_rfsh, pac1944_work_periodic_rfsh); + /* Setup the latest moment for reading the regs before saturation */ + schedule_delayed_work(&info->work_chip_rfsh, + msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS)); + + return devm_add_action_or_reset(&client->dev, pac1944_cancel_delayed_work, + &info->work_chip_rfsh); +} + +static const struct iio_chan_spec pac1944_single_channel[] = { + PAC1944_VPOWER_CHANNEL(0, PAC1944_VPOWER_1_ADDR, pac1944_single_event, + ARRAY_SIZE(pac1944_single_event)), + PAC1944_VBUS_CHANNEL(0, PAC1944_VBUS_1_ADDR, pac1944_events, + ARRAY_SIZE(pac1944_events)), + PAC1944_VSENSE_CHANNEL(0, PAC1944_VSENSE_1_ADDR, pac1944_events, + ARRAY_SIZE(pac1944_events)), + PAC1944_VBUS_AVG_CHANNEL(0, PAC1944_VBUS_AVG_1_ADDR), + PAC1944_VSENSE_AVG_CHANNEL(0, PAC1944_VSENSE_AVG_1_ADDR), +}; + +static int pac1944_prep_iio_channels(struct pac1944_chip_info *info, + struct iio_dev *indio_dev) +{ + struct device *dev = &info->client->dev; + struct iio_chan_spec *ch_sp; + int channel_size, attribute_count, cnt; + + /* Finding out dynamically how many IIO channels we need */ + attribute_count = 0; + channel_size = 0; + + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + /* add the size of the properties of one chip physical channel */ + channel_size += sizeof(pac1944_single_channel); + /* count how many enabled channels we have */ + attribute_count += ARRAY_SIZE(pac1944_single_channel); + dev_dbg(dev, ":%s: Channel %d active\n", __func__, cnt + 1); + } + + ch_sp = devm_kzalloc(dev, channel_size, GFP_KERNEL); + if (!ch_sp) + return -ENOMEM; + + /* + * Send the updated dynamic channel structure information towards IIO + * prepare the required field for IIO class registration + */ + indio_dev->num_channels = attribute_count; + indio_dev->channels = ch_sp; + + /* Populate the dynamic channels and make all the adjustments */ + for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) { + memcpy(ch_sp, pac1944_single_channel, sizeof(pac1944_single_channel)); + /* + * Into the datasheet channels are noted from 1 to 4 so we will adjust + * the channel to match channel index (1 to 4) from the datasheet + */ + ch_sp[PAC1944_CH_POWER].channel = cnt + 1; + ch_sp[PAC1944_CH_POWER].address = cnt + PAC1944_VPOWER_1_ADDR; + ch_sp[PAC1944_CH_VOLTAGE].channel = cnt + 1; + ch_sp[PAC1944_CH_VOLTAGE].address = cnt + PAC1944_VBUS_1_ADDR; + ch_sp[PAC1944_CH_CURRENT].channel = cnt + 1; + ch_sp[PAC1944_CH_CURRENT].address = cnt + PAC1944_VSENSE_1_ADDR; + /* + * In order to be able to use labels for PAC1944_CH_VOLTAGE and + * PAC1944_CH_VOLTAGE_AVERAGE, respectively PAC1944_CH_CURRENT + * and PAC1944_CH_CURRENT_AVERAGE we need to use different channel numbers. + * We will add +5 (+1 to maximum PAC channels). + */ + ch_sp[PAC1944_CH_VOLTAGE_AVERAGE].channel = cnt + PAC1944_MAX_CH + 1; + ch_sp[PAC1944_CH_VOLTAGE_AVERAGE].address = cnt + PAC1944_VBUS_AVG_1_ADDR; + ch_sp[PAC1944_CH_CURRENT_AVERAGE].channel = cnt + PAC1944_MAX_CH + 1; + ch_sp[PAC1944_CH_CURRENT_AVERAGE].address = cnt + PAC1944_VSENSE_AVG_1_ADDR; + + /* advance the pointer */ + ch_sp += ARRAY_SIZE(pac1944_single_channel); + } + + return 0; +} + +static const struct iio_info pac1944_info = { + .read_raw = pac1944_read_raw, + .read_label = pac1944_read_label, + .read_event_value = pac1944_read_thresh, + .write_event_value = pac1944_write_thresh, + .read_event_config = pac1944_read_event_config, + .write_event_config = pac1944_write_event_config, +}; + +static int pac1944_probe(struct i2c_client *client) +{ + struct pac1944_chip_info *info; + struct iio_dev *indio_dev; + const struct pac1944_features *chip; + int cnt, ret; + struct device *dev = &client->dev; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*info)); + if (!indio_dev) + return -ENOMEM; + + info = iio_priv(indio_dev); + info->client = client; + + ret = pac1944_chip_identify(info); + if (ret < 0) { + dev_dbg(dev, "Failed to identify the device based on HW ID\n"); + /* + * If failed to identify the hardware based on internal registers, + * try using fallback compatible in device tree to deal with + * some newer part number. + */ + chip = i2c_get_match_data(client); + if (!chip) + return -EINVAL; + + info->chip_variant = chip->prod_id; + info->phys_channels = chip->phys_channels; + indio_dev->name = chip->name; + } else { + info->phys_channels = pac1944_chip_config[ret].phys_channels; + indio_dev->name = pac1944_chip_config[ret].name; + } + + for (cnt = 0; cnt < info->phys_channels; cnt++) { + /* always start with accumulation channels enabled */ + info->enable_acc[cnt] = true; + } + + if (ACPI_HANDLE(dev)) + ret = pac1944_acpi_parse_channel_config(client, info); + else + ret = pac1944_of_parse_channel_config(client, info); + + if (ret) + return dev_err_probe(dev, ret, "parameter parsing returned an error\n"); + + ret = devm_mutex_init(dev, &info->lock); + if (ret < 0) + return ret; + + ret = pac1944_chip_configure(info); + if (ret < 0) + return ret; + + ret = pac1944_prep_iio_channels(info, indio_dev); + if (ret < 0) + return ret; + + info->iio_info = pac1944_info; + indio_dev->info = &info->iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = pac1944_prep_custom_attributes(info, indio_dev); + if (ret < 0) + return dev_err_probe(dev, ret, "Can't configure custom attributes for device\n"); + + ret = pac1944_reg_snapshot(info, true, false, + PAC1944_MIN_UPDATE_WAIT_TIME_US); + if (ret < 0) + return ret; + + ret = devm_iio_device_register(dev, indio_dev); + if (ret < 0) + return dev_err_probe(dev, ret, "Can't register IIO device\n"); + + return 0; +} + +static const struct i2c_device_id pac1944_id[] = { + { .name = "pac1941", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941] }, + { .name = "pac19412", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941_2] }, + { .name = "pac1942", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942] }, + { .name = "pac19422", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942_2] }, + { .name = "pac1943", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1943] }, + { .name = "pac1944", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] }, + { .name = "pac1951", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951] }, + { .name = "pac19512", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951_2] }, + { .name = "pac1952", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952] }, + { .name = "pac19522", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952_2] }, + { .name = "pac1953", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1953] }, + { .name = "pac1954", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1954] }, + { } +}; +MODULE_DEVICE_TABLE(i2c, pac1944_id); + +static const struct of_device_id pac1944_of_match[] = { + { + .compatible = "microchip,pac1941", + .data = (void *)&pac1944_chip_config[PAC1941] + }, + { + .compatible = "microchip,pac19412", + .data = (void *)&pac1944_chip_config[PAC1941_2] + }, + { + .compatible = "microchip,pac1942", + .data = (void *)&pac1944_chip_config[PAC1942] + }, + { + .compatible = "microchip,pac19422", + .data = (void *)&pac1944_chip_config[PAC1942_2] + }, + { + .compatible = "microchip,pac1943", + .data = (void *)&pac1944_chip_config[PAC1943] + }, + { + .compatible = "microchip,pac1944", + .data = (void *)&pac1944_chip_config[PAC1944] + }, + { + .compatible = "microchip,pac1951", + .data = (void *)&pac1944_chip_config[PAC1951] + }, + { + .compatible = "microchip,pac19512", + .data = (void *)&pac1944_chip_config[PAC1951_2] + }, + { + .compatible = "microchip,pac1952", + .data = (void *)&pac1944_chip_config[PAC1952] + }, + { + .compatible = "microchip,pac19522", + .data = (void *)&pac1944_chip_config[PAC1952_2] + }, + { + .compatible = "microchip,pac1953", + .data = (void *)&pac1944_chip_config[PAC1953] + }, + { + .compatible = "microchip,pac1954", + .data = (void *)&pac1944_chip_config[PAC1954] + }, + { } +}; +MODULE_DEVICE_TABLE(of, pac1944_of_match); + +static const struct acpi_device_id pac1944_acpi_match[] = { + { "MCHP1940", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] }, + { } +}; +MODULE_DEVICE_TABLE(acpi, pac1944_acpi_match); + +static struct i2c_driver pac1944_driver = { + .driver = { + .name = "pac1944", + .of_match_table = pac1944_of_match, + .acpi_match_table = pac1944_acpi_match + }, + .probe = pac1944_probe, + .id_table = pac1944_id, +}; +module_i2c_driver(pac1944_driver); + +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>"); +MODULE_DESCRIPTION("Microchip PAC194X and PAC195X Power Monitor"); +MODULE_LICENSE("GPL"); -- 2.48.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-06 9:39 ` [PATCH v3 2/2] " marius.cristea @ 2025-06-06 17:02 ` David Lechner 2025-06-10 15:07 ` Marius.Cristea 2025-06-07 15:27 ` Jonathan Cameron 1 sibling, 1 reply; 18+ messages in thread From: David Lechner @ 2025-06-06 17:02 UTC (permalink / raw) To: marius.cristea, jic23, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel On 6/6/25 4:39 AM, marius.cristea@microchip.com wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the iio driver for Microchip PAC194X and PAC195X series of > Power Monitors with Accumulator. The PAC194X family supports 9V > Full-Scale Range and the PAC195X supports 32V Full-Scale Range. > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > are for high-side current sensing and the PAC194X/5X-2 devices are > for low-side current sensing or floating VBUS applications. The > PAC194X/5X-1 is named shortly PAC194X/5X. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > --- > .../ABI/testing/sysfs-bus-iio-adc-pac1944 | 17 + > MAINTAINERS | 7 + > drivers/iio/adc/Kconfig | 12 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/pac1944.c | 2841 +++++++++++++++++ Oof. Very hard to make a review of something this big. Is it possible to break it down into more patches? e.g. start with just the basic functionality, then add the accumulator in a separate patch, add events in a separate patch, etc. It's not always possible, but 500 lines per patch is a nice number to aim for. > 5 files changed, 2878 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > create mode 100644 drivers/iio/adc/pac1944.c > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > new file mode 100644 > index 000000000000..ae88eac354a4 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > @@ -0,0 +1,17 @@ > +What: /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg > +KernelVersion: 6.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + A read/write property used to route, inside the PAC device, a specific ALERT > + signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin must be configured for the > + ALERT function in order to control the device hardware pin (this is the default > + functionality of the device hardware pin). > + > +What: /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg > +KernelVersion: 6.16 > +Contact: linux-iio@vger.kernel.org > +Description: > + A read/write property used to route, inside the PAC device, a specific ALERT > + signal to the GPIO/ALERT2 hardware pin. The GPIO/ALERT2 pin must be configured > + for ALERT function in order to control the device hardware pin (this is the > + default functionality of the device hardware pin). What is the use case for needing these? In otherwords, why can't the driver just make best use of available resources as it sees fit? > diff --git a/MAINTAINERS b/MAINTAINERS > index 86a2045ba62e..240e84893ad9 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15920,6 +15920,13 @@ S: Supported > F: Documentation/devicetree/bindings/iio/adc/microchip,pac1934.yaml > F: drivers/iio/adc/pac1934.c > > +MICROCHIP PAC1944 ADC DRIVER > +M: Marius Cristea <marius.cristea@microchip.com> > +L: linux-iio@vger.kernel.org > +S: Supported Missing the ABI docs, but hopefuly we can get rid of that. > +F: Documentation/devicetree/bindings/iio/adc/microchip,pac1944.yaml > +F: drivers/iio/adc/pac1944.c > + > MICROCHIP PCI1XXXX GP DRIVER > M: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com> > M: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 186a453af56c..a608aa6cb9c7 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -1148,6 +1148,18 @@ config PAC1934 > This driver can also be built as a module. If so, the module > will be called pac1934. > > +config PAC1944 > + tristate "Microchip Technology PAC1944/PAC1954 driver" > + depends on I2C > + help > + Say yes here to build support for Microchip Technology's PAC1941, > + PAC1941-2, PAC1942, PAC1942-2, PAC1943, PAC1944, PAC1951, > + PAC1951-2, PAC1952, PAC1952-2, PAC1953, PAC1954 > + Single/Multi-Channel Power Monitor with Accumulator. > + > + This driver can also be built as a module. If so, the module > + will be called pac1944. > + > config PALMAS_GPADC > tristate "TI Palmas General Purpose ADC" > depends on MFD_PALMAS > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 09ae6edb2650..ee47d880babf 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o > obj-$(CONFIG_NPCM_ADC) += npcm_adc.o > obj-$(CONFIG_PAC1921) += pac1921.o > obj-$(CONFIG_PAC1934) += pac1934.o > +obj-$(CONFIG_PAC1944) += pac1944.o > obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o > obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o > diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c > new file mode 100644 > index 000000000000..ce09334b076a > --- /dev/null > +++ b/drivers/iio/adc/pac1944.c > @@ -0,0 +1,2841 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * IIO driver for PAC194X and PAC195X series chips > + * > + * Copyright (C) 2022-2025 Microchip Technology Inc. and its subsidiaries > + * > + * Author: Marius Cristea marius.cristea@microchip.com > + * > + * Datasheet for PAC1941, PAC1942, PAC1943 and PAC1944 can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf > + * Datasheet for PAC1951, PAC1952, PAC1953 and PAC1954 can be found here: > + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf > + */ > + > +#include <linux/acpi.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/events.h> > +#include <linux/iio/sysfs.h> > +#include <linux/unaligned.h> This seems incomplete. Expecting at least linux/module.h, linux/property.h, etc. > + > +static const struct pac1944_features pac1944_chip_config[] = { I know there are lots of existing drivers that use arrays like this, but it just makes lots of extra code to read without any practial benifits. Better is to just make standalone structs for each chip. > + /* PAC194X Family */ > + [PAC1941] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1941, > + .name = "pac1941", > + }, > + [PAC1942] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1942, > + .name = "pac1942", > + }, > + [PAC1943] = { > + .phys_channels = 3, > + .prod_id = PAC_PRODUCT_ID_1943, > + .name = "pac1943", > + }, > + [PAC1944] = { > + .phys_channels = 4, > + .prod_id = PAC_PRODUCT_ID_1944, > + .name = "pac1944", > + }, > + [PAC1941_2] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1941_2, > + .name = "pac1941_2", > + }, > + [PAC1942_2] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1942_2, > + .name = "pac1942_2", > + }, > + /* PAC195X Family */ > + [PAC1951] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1951, > + .name = "pac1951", > + }, > + [PAC1952] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1952, > + .name = "pac1952_1", Odd that this is the only one to have _1. > + }, > + [PAC1953] = { > + .phys_channels = 3, > + .prod_id = PAC_PRODUCT_ID_1953, > + .name = "pac1953", > + }, > + [PAC1954] = { > + .phys_channels = 4, > + .prod_id = PAC_PRODUCT_ID_1954, > + .name = "pac1954", > + }, > + [PAC1951_2] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1951_2, > + .name = "pac1951_2", > + }, > + [PAC1952_2] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1952_2, > + .name = "pac1952_2", > + }, > +}; > + ... > +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644, > + pac1944_shunt_value_show, pac1944_shunt_value_store, 0); > +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644, > + pac1944_shunt_value_show, pac1944_shunt_value_store, 1); > +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644, > + pac1944_shunt_value_show, pac1944_shunt_value_store, 2); > +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644, > + pac1944_shunt_value_show, pac1944_shunt_value_store, 3); These are specified in the devicetree. Why are there also sysfs attribtes? > + > +static IIO_DEVICE_ATTR(slow_alert1_cfg, 0644, pac1944_slow_alert1_show, > + pac1944_slow_alert1_store, 0); > +static IIO_DEVICE_ATTR(gpio_alert2_cfg, 0644, pac1944_gpio_alert2_show, > + pac1944_gpio_alert2_store, 0); > + ... > +static int pac1944_frequency_get(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + > + return info->sampling_mode; > +} Pulled this out of order for context: > +/* Available Sample Modes */ > +static const char * const pac1944_frequency_avail[] = { > + "1024_ADAP", > + "256_ADAP", > + "64_ADAP", > + "8_ADAP", > + "1024", > + "256", > + "64", > + "8", > + "single_shot_1x", > + "single_shot_8x", > + "fast", > + "burst", > +}; > + > +static const struct iio_enum sampling_mode_enum = { > + .items = pac1944_frequency_avail, > + .num_items = ARRAY_SIZE(pac1944_frequency_avail), > + .set = pac1944_frequency_set, > + .get = pac1944_frequency_get, > +}; > + > +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = { > + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, &sampling_mode_enum), > + { > + .name = "sampling_frequency_available", > + .shared = IIO_SHARED_BY_ALL, > + .read = iio_enum_available_read, > + .private = (uintptr_t)&sampling_mode_enum, > + }, > + { } > +}; sampling_frequency{_avialable} are already standard attributes in IIO and is defined to be a number in Hz. So we will need to find a way to make this work with the standard attribute (can use IIO_CHAN_INFO_SAMPLE_FREQ, by the way). And figure out how the other parts fit into other existing IIO features. > + > +static int pac1944_of_parse_channel_config(struct i2c_client *client, > + struct pac1944_chip_info *info) > +{ > + unsigned int current_channel; > + struct device *dev = &client->dev; > + int idx, ret, temp; > + bool is_bipolar, is_half_fsr; > + > + current_channel = 1; > + > + device_for_each_child_node_scoped(dev, child) { > + ret = fwnode_property_read_u32(child, "reg", &idx); > + if (ret) > + return dev_err_probe(dev, ret, "reading invalid channel index\n"); > + > + /* adjust idx to match channel index (1 to 4) from the datasheet */ > + idx--; > + > + if (current_channel >= (info->phys_channels + 1) || > + idx >= info->phys_channels || idx < 0) > + return dev_err_probe(dev, -EINVAL, "invalid channel index %d value\n", > + idx + 1); > + > + /* enable channel */ > + set_bit(idx, &info->active_channels_mask); > + > + ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", > + &info->shunts[idx]); > + if (ret) > + return dev_err_probe(dev, ret, "%s: invalid shunt-resistor value: %d\n", > + fwnode_get_name(child), info->shunts[idx]); If there is an error, then info->shunts[idx] is not set, so doesn't make sense to include it in the error message. > + > + if (fwnode_property_present(child, "label")) No point in checking if property is present if not checking the return value of read_string() > + fwnode_property_read_string(child, "label", > + (const char **)&info->labels[idx]); > + > + is_bipolar = false; > + if (fwnode_property_present(child, "microchip,vbus-bipolar")) > + is_bipolar = true; Just do: is_bipolar = fwnode_property_read_bool(child, "microchip,vbus-bipolar"); > + > + is_half_fsr = false; > + if (fwnode_property_present(child, "microchip,vbus-half-range")) > + is_half_fsr = true; ditto > + > + /* default value is unipolar and Full Scale Range */ > + info->chip_reg_data.vbus_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; > + if (is_half_fsr) > + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; > + else if (is_bipolar) > + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; > + > + is_bipolar = false; > + if (fwnode_property_present(child, "microchip,vsense-bipolar")) > + is_bipolar = true; > + > + is_half_fsr = false; > + if (fwnode_property_present(child, "microchip,vsense-half-range")) > + is_half_fsr = true; > + > + /* default value is unipolar and Full Scale Range */ > + info->chip_reg_data.vsense_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; > + if (is_half_fsr) > + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; > + else if (is_bipolar) > + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; > + > + ret = fwnode_property_read_u32(child, "microchip,accumulation-mode", &temp); > + if (ret) > + return dev_err_probe(dev, ret, "invalid accumulation-mode value on %s\n", > + fwnode_get_name(child)); > + if (temp == PAC1944_ACCMODE_VPOWER || > + temp == PAC1944_ACCMODE_VSENSE || > + temp == PAC1944_ACCMODE_VBUS) { > + dev_dbg(dev, "Accumulation{%d} mode set to: %d\n", idx, temp); > + info->chip_reg_data.accumulation_mode[idx] = temp; > + } else { > + return dev_err_probe(dev, -EINVAL, > + "invalid mode for accumulator value on %s\n", > + fwnode_get_name(child)); > + } > + current_channel++; > + } > + > + return 0; > +} ... > + > +static const struct i2c_device_id pac1944_id[] = { > + { .name = "pac1941", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941] }, > + { .name = "pac19412", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941_2] }, > + { .name = "pac1942", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942] }, > + { .name = "pac19422", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942_2] }, > + { .name = "pac1943", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1943] }, > + { .name = "pac1944", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] }, > + { .name = "pac1951", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951] }, > + { .name = "pac19512", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951_2] }, > + { .name = "pac1952", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952] }, > + { .name = "pac19522", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952_2] }, > + { .name = "pac1953", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1953] }, > + { .name = "pac1954", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1954] }, Drop the .name and .driver_data here. Thre is an effort to do this in all drivers so that we can eventually get rid of the (kernel_ulong_t) casts. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, pac1944_id); > + ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-06 17:02 ` David Lechner @ 2025-06-10 15:07 ` Marius.Cristea 2025-06-10 16:11 ` David Lechner 2025-06-10 20:20 ` Andy Shevchenko 0 siblings, 2 replies; 18+ messages in thread From: Marius.Cristea @ 2025-06-10 15:07 UTC (permalink / raw) To: jic23, nuno.sa, dlechner, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 6/6/25 4:39 AM, marius.cristea@microchip.com wrote: > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the iio driver for Microchip PAC194X and PAC195X series of > > Power Monitors with Accumulator. The PAC194X family supports 9V > > Full-Scale Range and the PAC195X supports 32V Full-Scale Range. > > > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > > are for high-side current sensing and the PAC194X/5X-2 devices are > > for low-side current sensing or floating VBUS applications. The > > PAC194X/5X-1 is named shortly PAC194X/5X. > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > --- > > .../ABI/testing/sysfs-bus-iio-adc-pac1944 | 17 + > > MAINTAINERS | 7 + > > drivers/iio/adc/Kconfig | 12 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/pac1944.c | 2841 > > +++++++++++++++++ > > Oof. Very hard to make a review of something this big. Is it possible > to break it down into more patches? e.g. start with just the basic > functionality, then add the accumulator in a separate patch, add > events > in a separate patch, etc. It's not always possible, but 500 lines per > patch is a nice number to aim for. > The first 2 version of the driver was much bigger. I will try make it smaller and build on top of that. > > > 5 files changed, 2878 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc- > > pac1944 > > create mode 100644 drivers/iio/adc/pac1944.c > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > > new file mode 100644 > > index 000000000000..ae88eac354a4 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > > @@ -0,0 +1,17 @@ > > +What: > > /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg > > +KernelVersion: 6.16 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + A read/write property used to route, inside the PAC > > device, a specific ALERT > > + signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin > > must be configured for the > > + ALERT function in order to control the device > > hardware pin (this is the default > > + functionality of the device hardware pin). > > + > > +What: > > /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg > > +KernelVersion: 6.16 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + A read/write property used to route, inside the PAC > > device, a specific ALERT > > + signal to the GPIO/ALERT2 hardware pin. The > > GPIO/ALERT2 pin must be configured > > + for ALERT function in order to control the device > > hardware pin (this is the > > + default functionality of the device hardware pin). > > > What is the use case for needing these? In otherwords, why can't the > driver just > make best use of available resources as it sees fit? > Here inside the PAC the user could choose what limit to be routeed outside the chip. For sure, all of the limits could be routed to the same hardware pin, but there are some use cases where the user will want to connect that output pin to a safety hardware (e.g. over-current protection or over-voltage and over-current) and in this case we need a way to allow the user to do the setup. > > > subsidiaries > > + * > > + * Author: Marius Cristea marius.cristea@microchip.com > > + * > > + * Datasheet for PAC1941, PAC1942, PAC1943 and PAC1944 can be > > found here: > > + * > > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf > > + * Datasheet for PAC1951, PAC1952, PAC1953 and PAC1954 can be > > found here: > > + * > > https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/bitfield.h> > > +#include <linux/delay.h> > > +#include <linux/err.h> > > +#include <linux/i2c.h> > > +#include <linux/iio/iio.h> > > +#include <linux/iio/events.h> > > +#include <linux/iio/sysfs.h> > > +#include <linux/unaligned.h> > > This seems incomplete. Expecting at least linux/module.h, > linux/property.h, etc. I will double check. Most probably those module are already included by a header. > > ... > > > +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644, > > + pac1944_shunt_value_show, > > pac1944_shunt_value_store, 0); > > +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644, > > + pac1944_shunt_value_show, > > pac1944_shunt_value_store, 1); > > +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644, > > + pac1944_shunt_value_show, > > pac1944_shunt_value_store, 2); > > +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644, > > + pac1944_shunt_value_show, > > pac1944_shunt_value_store, 3); > > These are specified in the devicetree. Why are there also sysfs > attribtes? Yes, you could put a generic shunt resistor into the device tree but this resistor will have a tolerance. Because the end user could calibrate the system, it could also save the calculated/calibrated shunt resistor somewhere and restore that calibrated value each time the driver is loaded. > > > > +/* Available Sample Modes */ > > +static const char * const pac1944_frequency_avail[] = { > > + "1024_ADAP", > > + "256_ADAP", > > + "64_ADAP", > > + "8_ADAP", > > + "1024", > > + "256", > > + "64", > > + "8", > > + "single_shot_1x", > > + "single_shot_8x", > > + "fast", > > + "burst", > > +}; > > > ... > > + > > +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = { > > + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, > > &sampling_mode_enum), > > + { > > + .name = "sampling_frequency_available", > > + .shared = IIO_SHARED_BY_ALL, > > + .read = iio_enum_available_read, > > + .private = (uintptr_t)&sampling_mode_enum, > > + }, > > + { } > > +}; > > sampling_frequency{_avialable} are already standard attributes in IIO > and is > defined to be a number in Hz. So we will need to find a way to make > this > work with the standard attribute (can use IIO_CHAN_INFO_SAMPLE_FREQ, > by the way). > And figure out how the other parts fit into other existing IIO > features. I can change to the standard attributes but I still have some question related to how to handle the ADAPTIVE sampling frequency that the chip supports and that it could be used to lower the power consumption of the chip. > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-10 15:07 ` Marius.Cristea @ 2025-06-10 16:11 ` David Lechner 2025-06-11 15:59 ` Jonathan Cameron 2025-06-10 20:20 ` Andy Shevchenko 1 sibling, 1 reply; 18+ messages in thread From: David Lechner @ 2025-06-10 16:11 UTC (permalink / raw) To: Marius.Cristea, jic23, nuno.sa, andy Cc: robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On 6/10/25 10:07 AM, Marius.Cristea@microchip.com wrote: > On Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote: ... >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 >>> b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 >>> new file mode 100644 >>> index 000000000000..ae88eac354a4 >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 >>> @@ -0,0 +1,17 @@ >>> +What: >>> /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg >>> +KernelVersion: 6.16 >>> +Contact: linux-iio@vger.kernel.org >>> +Description: >>> + A read/write property used to route, inside the PAC >>> device, a specific ALERT >>> + signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin >>> must be configured for the >>> + ALERT function in order to control the device >>> hardware pin (this is the default >>> + functionality of the device hardware pin). >>> + >>> +What: >>> /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg >>> +KernelVersion: 6.16 >>> +Contact: linux-iio@vger.kernel.org >>> +Description: >>> + A read/write property used to route, inside the PAC >>> device, a specific ALERT >>> + signal to the GPIO/ALERT2 hardware pin. The >>> GPIO/ALERT2 pin must be configured >>> + for ALERT function in order to control the device >>> hardware pin (this is the >>> + default functionality of the device hardware pin). >> >> >> What is the use case for needing these? In otherwords, why can't the >> driver just >> make best use of available resources as it sees fit? >> > > Here inside the PAC the user could choose what limit to be routeed > outside the chip. For sure, all of the limits could be routed to the > same hardware pin, but there are some use cases where the user will > want to connect that output pin to a safety hardware (e.g. over-current > protection or over-voltage and over-current) and in this case we need a > way to allow the user to do the setup. > This sounds like it depends on what is wired to the alert pin, so sounds like something that should be specified in the devicetree. I.e. in the devicetree, have a bool property microchip,alert1-is-safety to indicate the ALERT1 pin is wired to the safety hardware. (It could still be also wired as an interrupt input at the same time - or not, doesn't really matter.) Then, on the event attributes add a boolean "safety" attribute to allow routing the signal to either the pin that was flagged as the safety pin or not. This would allow the user to chose which signals control the safety hardware at runtime without them having to know how the hardware is actually wired up. >> >>> subsidiaries >>> + * >>> + * Author: Marius Cristea marius.cristea@microchip.com >>> + * >>> + * Datasheet for PAC1941, PAC1942, PAC1943 and PAC1944 can be >>> found here: >>> + * >>> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC194X-Family-Data-Sheet-DS20006543.pdf >>> + * Datasheet for PAC1951, PAC1952, PAC1953 and PAC1954 can be >>> found here: >>> + * >>> https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/PAC195X-Family-Data-Sheet-DS20006539.pdf >>> + */ >>> + >>> +#include <linux/acpi.h> >>> +#include <linux/bitfield.h> >>> +#include <linux/delay.h> >>> +#include <linux/err.h> >>> +#include <linux/i2c.h> >>> +#include <linux/iio/iio.h> >>> +#include <linux/iio/events.h> >>> +#include <linux/iio/sysfs.h> >>> +#include <linux/unaligned.h> >> >> This seems incomplete. Expecting at least linux/module.h, >> linux/property.h, etc. > > I will double check. Most probably those module are already included by > a header. > Even if some things happens to be indirectly included by another header, we still prefer to have the "primary" header included for everything that is actually used in this file. E.g. since this file uses the MODULE_DESCRIPTION() macro, we expect to see linux/module.h even if another header already includes that indirectly. Usually the "primary" header is the one where a symbol/macro is defined, but not always, e.g. if there is a linux/something.h that incldues asm/something.h, we would prefer the linux/ one rather than including the asm/ one directly. >> >> ... >> >>> +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644, >>> + pac1944_shunt_value_show, >>> pac1944_shunt_value_store, 0); >>> +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644, >>> + pac1944_shunt_value_show, >>> pac1944_shunt_value_store, 1); >>> +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644, >>> + pac1944_shunt_value_show, >>> pac1944_shunt_value_store, 2); >>> +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644, >>> + pac1944_shunt_value_show, >>> pac1944_shunt_value_store, 3); >> >> These are specified in the devicetree. Why are there also sysfs >> attribtes? > > Yes, you could put a generic shunt resistor into the device tree but > this resistor will have a tolerance. Because the end user could > calibrate the system, it could also save the calculated/calibrated > shunt resistor somewhere and restore that calibrated value each time > the driver is loaded. > If changing the resistor value changes the measured raw value, we could probably use one of the existing standard calibration attributes instead, like calibbias or calibscale. > >> >> >>> +/* Available Sample Modes */ >>> +static const char * const pac1944_frequency_avail[] = { >>> + "1024_ADAP", >>> + "256_ADAP", >>> + "64_ADAP", >>> + "8_ADAP", >>> + "1024", >>> + "256", >>> + "64", >>> + "8", >>> + "single_shot_1x", >>> + "single_shot_8x", >>> + "fast", >>> + "burst", >>> +}; >> >>> > ... >>> + >>> +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = { >>> + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, >>> &sampling_mode_enum), >>> + { >>> + .name = "sampling_frequency_available", >>> + .shared = IIO_SHARED_BY_ALL, >>> + .read = iio_enum_available_read, >>> + .private = (uintptr_t)&sampling_mode_enum, >>> + }, >>> + { } >>> +}; >> >> sampling_frequency{_avialable} are already standard attributes in IIO >> and is >> defined to be a number in Hz. So we will need to find a way to make >> this >> work with the standard attribute (can use IIO_CHAN_INFO_SAMPLE_FREQ, >> by the way). >> And figure out how the other parts fit into other existing IIO >> features. > > I can change to the standard attributes but I still have some question > related to how to handle the ADAPTIVE sampling frequency that the chip > supports and that it could be used to lower the power consumption of > the chip. > >> From a quick look at one of the datasheets, it sounds like this "adaptive" mode only applies when using an accumulator. And it doesn't actually change the sample rate, but rather other factors, like scale and the accumulator counter incitement. So it seems like it would be a separate custom boolean attribute. Also, I noticed that the fast mode and burst mode make the sampling frequency dependent on the number of enabled channels. So to handle this, normally, that would mean that IIO_CHAN_INFO_SAMP_FREQ would need to be IIO_SEPARATE rather than IIO_SHARED_BY_ALL. But since these chips support can work both ways (there are modes where sample rate doesn't depend on the number of channels enabled and there are modes where it does), I'm not sure what the right way to handle that would be here. Maybe Jonathan will have some suggestion? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-10 16:11 ` David Lechner @ 2025-06-11 15:59 ` Jonathan Cameron 0 siblings, 0 replies; 18+ messages in thread From: Jonathan Cameron @ 2025-06-11 15:59 UTC (permalink / raw) To: David Lechner Cc: Marius.Cristea, nuno.sa, andy, robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On Tue, 10 Jun 2025 11:11:32 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 6/10/25 10:07 AM, Marius.Cristea@microchip.com wrote: > > On Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote: > > ... > > >>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > >>> b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > >>> new file mode 100644 > >>> index 000000000000..ae88eac354a4 > >>> --- /dev/null > >>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-pac1944 > >>> @@ -0,0 +1,17 @@ > >>> +What: > >>> /sys/bus/iio/devices/iio:deviceX/slow_alert1_cfg > >>> +KernelVersion: 6.16 > >>> +Contact: linux-iio@vger.kernel.org > >>> +Description: > >>> + A read/write property used to route, inside the PAC > >>> device, a specific ALERT > >>> + signal to the SLOW/ALERT1 pin. The SLOW/ALERT1 pin > >>> must be configured for the > >>> + ALERT function in order to control the device > >>> hardware pin (this is the default > >>> + functionality of the device hardware pin). > >>> + > >>> +What: > >>> /sys/bus/iio/devices/iio:deviceX/gpio_alert2_cfg > >>> +KernelVersion: 6.16 > >>> +Contact: linux-iio@vger.kernel.org > >>> +Description: > >>> + A read/write property used to route, inside the PAC > >>> device, a specific ALERT > >>> + signal to the GPIO/ALERT2 hardware pin. The > >>> GPIO/ALERT2 pin must be configured > >>> + for ALERT function in order to control the device > >>> hardware pin (this is the > >>> + default functionality of the device hardware pin). > >> > >> > >> What is the use case for needing these? In otherwords, why can't the > >> driver just > >> make best use of available resources as it sees fit? > >> > > > > Here inside the PAC the user could choose what limit to be routeed > > outside the chip. For sure, all of the limits could be routed to the > > same hardware pin, but there are some use cases where the user will > > want to connect that output pin to a safety hardware (e.g. over-current > > protection or over-voltage and over-current) and in this case we need a > > way to allow the user to do the setup. > > > > This sounds like it depends on what is wired to the alert pin, so sounds > like something that should be specified in the devicetree. Absolutely agree. These corners tend to be tricky to handle cleanly in either DT or userspace interfaces though and sometimes we've just decided not to be fully flexible to keep that sane. > > I.e. in the devicetree, have a bool property microchip,alert1-is-safety > to indicate the ALERT1 pin is wired to the safety hardware. (It could > still be also wired as an interrupt input at the same time - or not, > doesn't really matter.) > > Then, on the event attributes add a boolean "safety" attribute to allow > routing the signal to either the pin that was flagged as the safety pin > or not. This would allow the user to chose which signals control the > safety hardware at runtime without them having to know how the hardware > is actually wired up. I'd question if the use cases are such that it makes sense to expose the option to user space. Generally if you have a way to trigger a safety circuit it's there for a reason and you want it always on for that reason (i.e. you might tweak exactly when a cut off on power usage fires, but you always do it on power usage). We probably want to propose a binding to he DT folk + take a look at what the ABI David suggests looks like in the driver. That's messy if it isn't double wired to the interrupt controller though as we then have events that never fire. > >> > >> ... > >> > >>> +static IIO_DEVICE_ATTR(in_current1_shunt_resistor, 0644, > >>> + pac1944_shunt_value_show, > >>> pac1944_shunt_value_store, 0); > >>> +static IIO_DEVICE_ATTR(in_current2_shunt_resistor, 0644, > >>> + pac1944_shunt_value_show, > >>> pac1944_shunt_value_store, 1); > >>> +static IIO_DEVICE_ATTR(in_current3_shunt_resistor, 0644, > >>> + pac1944_shunt_value_show, > >>> pac1944_shunt_value_store, 2); > >>> +static IIO_DEVICE_ATTR(in_current4_shunt_resistor, 0644, > >>> + pac1944_shunt_value_show, > >>> pac1944_shunt_value_store, 3); > >> > >> These are specified in the devicetree. Why are there also sysfs > >> attribtes? > > > > Yes, you could put a generic shunt resistor into the device tree but > > this resistor will have a tolerance. Because the end user could > > calibrate the system, it could also save the calculated/calibrated > > shunt resistor somewhere and restore that calibrated value each time > > the driver is loaded. > > > > If changing the resistor value changes the measured raw value, we > could probably use one of the existing standard calibration attributes > instead, like calibbias or calibscale. We have precedence for shunt resistor ABI for this reason, so I agree with David if we were starting from scratch (and it's a viable option to use the calibration attributes) but we can't drop the existing ABI and it does limited harm to carry on using it. > > > > >> > >> > >>> +/* Available Sample Modes */ > >>> +static const char * const pac1944_frequency_avail[] = { > >>> + "1024_ADAP", > >>> + "256_ADAP", > >>> + "64_ADAP", > >>> + "8_ADAP", > >>> + "1024", > >>> + "256", > >>> + "64", > >>> + "8", > >>> + "single_shot_1x", > >>> + "single_shot_8x", > >>> + "fast", > >>> + "burst", > >>> +}; > >> > >>> > > ... > >>> + > >>> +static const struct iio_chan_spec_ext_info pac1944_ext_info[] = { > >>> + IIO_ENUM("sampling_frequency", IIO_SHARED_BY_ALL, > >>> &sampling_mode_enum), > >>> + { > >>> + .name = "sampling_frequency_available", > >>> + .shared = IIO_SHARED_BY_ALL, > >>> + .read = iio_enum_available_read, > >>> + .private = (uintptr_t)&sampling_mode_enum, > >>> + }, > >>> + { } > >>> +}; > >> > >> sampling_frequency{_avialable} are already standard attributes in IIO > >> and is > >> defined to be a number in Hz. So we will need to find a way to make > >> this > >> work with the standard attribute (can use IIO_CHAN_INFO_SAMPLE_FREQ, > >> by the way). > >> And figure out how the other parts fit into other existing IIO > >> features. > > > > I can change to the standard attributes but I still have some question > > related to how to handle the ADAPTIVE sampling frequency that the chip > > supports and that it could be used to lower the power consumption of > > the chip. Gah. Second device to do something like this that we've seen this month. (ADXL313 does something similar when enough activity detection modes are turned on). We don't have a good answer yet :( > > > >> > > From a quick look at one of the datasheets, it sounds like this > "adaptive" mode only applies when using an accumulator. And it doesn't > actually change the sample rate, but rather other factors, like scale > and the accumulator counter incitement. So it seems like it would be > a separate custom boolean attribute. Generally when I see this sort of thing my first instinct is hide it. What use is it for userspace to see that the frequency is changing? Maybe we just report the highest (or lowest?) frequency that might be going on? Here it doesn't seem to be coupled to anything else though (unlikely the accelerometer) so I'm not sure how the driver would guess it makes sense to enable it? > > Also, I noticed that the fast mode and burst mode make the sampling > frequency dependent on the number of enabled channels. So to handle > this, normally, that would mean that IIO_CHAN_INFO_SAMP_FREQ would > need to be IIO_SEPARATE rather than IIO_SHARED_BY_ALL. > > But since these chips support can work both ways (there are modes > where sample rate doesn't depend on the number of channels enabled > and there are modes where it does), I'm not sure what the right way > to handle that would be here. Maybe Jonathan will have some suggestion? Oh goody a new way to stretch that already much stretched interface. It's actually more fun because it's not the sum of the number of channels for fast mode but 1 more than that. Given we can't do both ways of reporting it at the same time we will probably have to just do IIO_SHARED_BY_ALL. We can only then report the effective sampling frequency as it is fixed in these modes (so we can't attempt to match a userspace request by cranking it down if there are fewer channels enabled or similar). So we need sampling_frequency_available where the fastest frequency presented changes depending on the enabled channels. Lower values match the 8/64/256/1024 values however many are enabled. if sampling_frequency == top value then we just change it if more channels are enabled. ABI always allows for other attributes to change 'randomly' when you touch a different one. That flexibility is a pain for users but necessary for cases like this :( I'm not sure how we pick between burst and fast mode though as seems to be about auto offset stuff. Jonathan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-10 15:07 ` Marius.Cristea 2025-06-10 16:11 ` David Lechner @ 2025-06-10 20:20 ` Andy Shevchenko 1 sibling, 0 replies; 18+ messages in thread From: Andy Shevchenko @ 2025-06-10 20:20 UTC (permalink / raw) To: Marius.Cristea Cc: jic23, nuno.sa, dlechner, robh, krzk+dt, conor+dt, broonie, devicetree, linux-iio, linux-kernel On Tue, Jun 10, 2025 at 03:07:34PM +0000, Marius.Cristea@microchip.com wrote: > On Fri, 2025-06-06 at 12:02 -0500, David Lechner wrote: > > On 6/6/25 4:39 AM, marius.cristea@microchip.com wrote: ... > > > +#include <linux/acpi.h> > > > +#include <linux/bitfield.h> > > > +#include <linux/delay.h> > > > +#include <linux/err.h> > > > +#include <linux/i2c.h> > > > +#include <linux/iio/iio.h> > > > +#include <linux/iio/events.h> > > > +#include <linux/iio/sysfs.h> > > > +#include <linux/unaligned.h> > > > > This seems incomplete. Expecting at least linux/module.h, > > linux/property.h, etc. > > I will double check. Most probably those module are already included by > a header. And this is exactly the problem needs to be addressed. Read about IFYU principle. That's what we want. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-06 9:39 ` [PATCH v3 2/2] " marius.cristea 2025-06-06 17:02 ` David Lechner @ 2025-06-07 15:27 ` Jonathan Cameron 2025-06-10 15:58 ` Marius.Cristea 1 sibling, 1 reply; 18+ messages in thread From: Jonathan Cameron @ 2025-06-07 15:27 UTC (permalink / raw) To: marius.cristea Cc: dlechner, nuno.sa, andy, robh, krzk+dt, conor+dt, broonie, linux-iio, devicetree, linux-kernel On Fri, 6 Jun 2025 12:39:29 +0300 <marius.cristea@microchip.com> wrote: > From: Marius Cristea <marius.cristea@microchip.com> > > This is the iio driver for Microchip PAC194X and PAC195X series of > Power Monitors with Accumulator. The PAC194X family supports 9V > Full-Scale Range and the PAC195X supports 32V Full-Scale Range. > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > are for high-side current sensing and the PAC194X/5X-2 devices are > for low-side current sensing or floating VBUS applications. The > PAC194X/5X-1 is named shortly PAC194X/5X. > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> Hi Marius, I entirely agree with David on this wanting splitting up into a base driver + patches that add features to that in order to make it easier to review. That also potentially allows me to pick up the basic support whilst any more controversial parts are still under discussion. Jonathan > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 09ae6edb2650..ee47d880babf 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o > obj-$(CONFIG_NPCM_ADC) += npcm_adc.o > obj-$(CONFIG_PAC1921) += pac1921.o > obj-$(CONFIG_PAC1934) += pac1934.o > +obj-$(CONFIG_PAC1944) += pac1944.o > obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o > obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o > diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c > new file mode 100644 > index 000000000000..ce09334b076a > --- /dev/null > +++ b/drivers/iio/adc/pac1944.c > @@ -0,0 +1,2841 @@ > +/* Available Sample Modes */ > +static const char * const pac1944_frequency_avail[] = { > + "1024_ADAP", > + "256_ADAP", This adaptive mode shouldn't be controlled via this standard ABI. That needs to be considered separately. > + "64_ADAP", > + "8_ADAP", > + "1024", > + "256", > + "64", > + "8", This does not look even close to ABI complaint. The numbers cases are fine. The others not really. > + "single_shot_1x", That has nothing directly to do with the sampling frequency. Some others look suspicious. I'd stick to normal sampling_frequency handling and have the discussion about the other modes in here at a later date. > + "single_shot_8x", > + "fast", > + "burst", > +}; > > + > +static const struct pac1944_features pac1944_chip_config[] = { > + /* PAC194X Family */ > + [PAC1941] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1941, > + .name = "pac1941", > + }, > + [PAC1942] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1942, > + .name = "pac1942", > + }, > + [PAC1943] = { > + .phys_channels = 3, > + .prod_id = PAC_PRODUCT_ID_1943, > + .name = "pac1943", > + }, > + [PAC1944] = { > + .phys_channels = 4, > + .prod_id = PAC_PRODUCT_ID_1944, > + .name = "pac1944", > + }, > + [PAC1941_2] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1941_2, > + .name = "pac1941_2", > + }, > + [PAC1942_2] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1942_2, > + .name = "pac1942_2", > + }, > + /* PAC195X Family */ As mentioned below - just add the family in here as well so no function is needed to look it up. > + [PAC1951] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1951, > + .name = "pac1951", > + }, > + [PAC1952] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1952, > + .name = "pac1952_1", > + }, > + [PAC1953] = { > + .phys_channels = 3, > + .prod_id = PAC_PRODUCT_ID_1953, > + .name = "pac1953", > + }, > + [PAC1954] = { > + .phys_channels = 4, > + .prod_id = PAC_PRODUCT_ID_1954, > + .name = "pac1954", > + }, > + [PAC1951_2] = { > + .phys_channels = 1, > + .prod_id = PAC_PRODUCT_ID_1951_2, > + .name = "pac1951_2", > + }, > + [PAC1952_2] = { > + .phys_channels = 2, > + .prod_id = PAC_PRODUCT_ID_1952_2, > + .name = "pac1952_2", > + }, > +}; > + > +static inline u64 pac1944_get_unaligned_be56(const u8 *p) > +{ > + return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 | > + (u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6]; > +} > + > +/* Low-level I2c functions used to transfer more then 32 bytes at once */ > +static int pac1944_i2c_read(struct i2c_client *client, u8 reg_addr, > + void *databuf, u8 len) > +{ > + struct i2c_msg msgs[2] = { > + { .addr = client->addr, { .addr = ... etc > + .len = 1, > + .buf = (u8 *)®_addr, > + .flags = 0 > + }, > + { .addr = client->addr, > + .len = len, > + .buf = databuf, > + .flags = I2C_M_RD > + } > + }; > + > + return i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > +} > + > +static int pac1944_disable_alert_reg(struct device *dev, u32 mask, u8 *status) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + struct i2c_client *client = info->client; > + int ret; > + u32 val; > + u8 buf[PAC1944_ALERT_ENABLE_REG_LEN]; > + > + ret = i2c_smbus_read_i2c_block_data(client, > + PAC1944_ALERT_ENABLE_REG_ADDR, > + PAC1944_ALERT_ENABLE_REG_LEN, > + status); > + if (ret < 0) { > + dev_err(dev, "failing to disable allert reg\n"); > + return ret; > + } > + > + val = get_unaligned_be24(status); > + val = val & (~mask); > + put_unaligned_be24(val, &buf[0]); > + > + /* disable appropriate bit from the Alert enable register */ > + return i2c_smbus_write_block_data(client, PAC1944_ALERT_ENABLE_REG_ADDR, > + PAC1944_ALERT_ENABLE_REG_LEN, > + (u8 *)&buf[0]); I'd just use buf instead of that complex casting and array index (it's a u8 * anyway so definitely no casting! > +} > +static int pac1944_update_alert_16b(struct device *dev, u8 addr, u32 mask, > + u16 value) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + struct i2c_client *client = info->client; > + int ret; > + __be16 tmp_be16; > + u8 status[PAC1944_ALERT_ENABLE_REG_LEN]; > + > + ret = pac1944_disable_alert_reg(dev, mask, &status[0]); > + if (ret) { > + dev_err(dev, "failed to disable alert reg\n"); > + return ret; > + } > + > + tmp_be16 = cpu_to_be16(value); As below - I think this wants to be using i2c_smbus_write_word_swapped() > + > + ret = i2c_smbus_write_word_data(client, addr, tmp_be16); > + if (ret) { > + dev_err(dev, "failing to write at 0x%x\n", addr); > + return ret; > + } > + > + return pac1944_restore_alert_reg(indio_dev, &status[0]); > +} > + > +static int pac1944_update_alert_24b(struct device *dev, u8 addr, u32 mask, > + u32 value) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + struct i2c_client *client = info->client; > + int ret; > + u8 status[PAC1944_ALERT_ENABLE_REG_LEN], tmp[3]; > + > + ret = pac1944_disable_alert_reg(dev, mask, &status[0]); If you are going to parse an array just use status as the parameter, not &status[0] > + if (ret) { > + dev_err(dev, "failed to disable alert reg\n"); > + return ret; > + } > + > + put_unaligned_be24(value, &tmp[0]); , tmp) is fine here. > + > + ret = i2c_smbus_write_block_data(client, addr, ARRAY_SIZE(tmp), (u8 *)&tmp[0]); > + if (ret) { > + dev_err(dev, "failing to write at 0x%x\n", addr); > + return ret; > + } > + > + return pac1944_restore_alert_reg(indio_dev, &status[0]); , status); probably appropriate choice here as well. > +} > + > > +static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time) > +{ > + int ret = 0; > + > + /* > + * Check if the minimal elapsed time has passed and if so, > + * re-read the chip, otherwise the cached info is just fine > + */ > + if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp + > + msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS))) { If you can exit early like here, it is worth flipping the logic. if (!time_after(jiff.....)) return 0; ret = ... > + /* > + * We need to re-read the chip values > + * call the pac1944_reg_snapshot > + */ > + ret = pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR, > + wait_time); > + /* > + * Re-schedule the work for the read registers timeout > + * (to prevent chip regs saturation) > + */ > + cancel_delayed_work_sync(&info->work_chip_rfsh); > + schedule_delayed_work(&info->work_chip_rfsh, > + msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS)); > + } > + > + return ret; > +} > +static struct attribute *pac1944_power_acc_attr[] = { > + &iio_dev_attr_in_energy1_raw.dev_attr.attr, > + &iio_dev_attr_in_energy2_raw.dev_attr.attr, > + &iio_dev_attr_in_energy3_raw.dev_attr.attr, > + &iio_dev_attr_in_energy4_raw.dev_attr.attr, > + &iio_dev_attr_in_energy1_scale.dev_attr.attr, > + &iio_dev_attr_in_energy2_scale.dev_attr.attr, > + &iio_dev_attr_in_energy3_scale.dev_attr.attr, > + &iio_dev_attr_in_energy4_scale.dev_attr.attr, These look like standard read_raw / info_mask based attributes will work. So do that, not custom attributes that are both harder to review and don't work with in kernel consumers (which we may well see for a power monitoring chip). > + &iio_dev_attr_in_energy1_en.dev_attr.attr, > + &iio_dev_attr_in_energy2_en.dev_attr.attr, > + &iio_dev_attr_in_energy3_en.dev_attr.attr, > + &iio_dev_attr_in_energy4_en.dev_attr.attr, > + NULL > +}; > + > +static struct attribute *pac1944_current_acc_attr[] = { > + &iio_dev_attr_in_current_acc1_raw.dev_attr.attr, > + &iio_dev_attr_in_current_acc2_raw.dev_attr.attr, > + &iio_dev_attr_in_current_acc3_raw.dev_attr.attr, > + &iio_dev_attr_in_current_acc4_raw.dev_attr.attr, > + &iio_dev_attr_in_current_acc1_scale.dev_attr.attr, > + &iio_dev_attr_in_current_acc2_scale.dev_attr.attr, > + &iio_dev_attr_in_current_acc3_scale.dev_attr.attr, > + &iio_dev_attr_in_current_acc4_scale.dev_attr.attr, > + &iio_dev_attr_in_current_acc1_en.dev_attr.attr, > + &iio_dev_attr_in_current_acc2_en.dev_attr.attr, > + &iio_dev_attr_in_current_acc3_en.dev_attr.attr, > + &iio_dev_attr_in_current_acc4_en.dev_attr.attr, > + NULL > +}; > + > +static struct attribute *pac1944_voltage_acc_attr[] = { > + &iio_dev_attr_in_voltage_acc1_raw.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc2_raw.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc3_raw.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc4_raw.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc1_scale.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc2_scale.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc3_scale.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc4_scale.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc1_en.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc2_en.dev_attr.attr, > + &iio_dev_attr_in_voltage_acc3_en.dev_attr.attr, Where is this ABI documented? > + &iio_dev_attr_in_voltage_acc4_en.dev_attr.attr, > + NULL > +}; > + > +static int pac1944_prep_custom_attributes(struct pac1944_chip_info *info, > + struct iio_dev *indio_dev) > +{ > + int ch, i, j; > + int active_channels_count = 0; > + struct attribute **pac1944_custom_attrs, **tmp_attr; > + struct attribute_group *pac1944_group; > + int custom_attr_cnt; > + struct i2c_client *client = info->client; > + > + active_channels_count = info->num_enabled_channels; > + > + pac1944_group = devm_kzalloc(&client->dev, sizeof(*pac1944_group), GFP_KERNEL); > + if (!pac1944_group) > + return -ENOMEM; > + > + /* > + * Attributes for channel X: > + * - in_shunt_value_X > + * - one of pair attributes: > + * - in_power_accX_raw and in_power_accX_scale > + * - in_current_accX_raw and in_current_accX_scale > + * - in_voltage_accX_raw and in_voltage_accX_scale > + * Shared attributes: > + * - slow_alert1_cfg > + * - gpio_alert2_cfg > + * NULL > + */ > + custom_attr_cnt = PAC1944_COMMON_DEVATTR * active_channels_count; > + custom_attr_cnt += PAC1944_ACC_DEVATTR * active_channels_count; > + custom_attr_cnt += PAC1944_SHARED_DEVATTRS_COUNT; > + > + pac1944_custom_attrs = devm_kzalloc(&client->dev, custom_attr_cnt * > + sizeof(*pac1944_group) + 1, GFP_KERNEL); > + if (!pac1944_custom_attrs) > + return -ENOMEM; > + > + j = 0; > + > + for_each_set_bit(ch, &info->active_channels_mask, info->phys_channels) { > + for (i = 0; i < PAC1944_COMMON_DEVATTR; i++) > + pac1944_custom_attrs[j++] = > + pac1944_all_attrs[PAC1944_COMMON_DEVATTR * ch + i]; > + > + switch (info->chip_reg_data.accumulation_mode[ch]) { > + case PAC1944_ACCMODE_VPOWER: > + tmp_attr = pac1944_power_acc_attr; > + break; > + case PAC1944_ACCMODE_VSENSE: > + tmp_attr = pac1944_current_acc_attr; > + break; > + case PAC1944_ACCMODE_VBUS: > + tmp_attr = pac1944_voltage_acc_attr; > + break; > + default: > + return -EINVAL; > + } > + > + pac1944_custom_attrs[j++] = tmp_attr[ch]; > + pac1944_custom_attrs[j++] = pac1944_power_acc_attr[PAC1944_MAX_CH + ch]; > + pac1944_custom_attrs[j++] = pac1944_power_acc_attr[2 * PAC1944_MAX_CH + ch]; > + } > + > + for (i = 0; i < PAC1944_SHARED_DEVATTRS_COUNT; i++) > + pac1944_custom_attrs[j++] = > + pac1944_all_attrs[PAC1944_COMMON_DEVATTR * PAC1944_MAX_CH + i]; > + > + pac1944_group->attrs = pac1944_custom_attrs; > + info->iio_info.attrs = pac1944_group; > + > + return 0; > +} > + > +static int pac1944_frequency_set(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + unsigned int mode) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + struct i2c_client *client = info->client; > + int ret; > + u16 tmp_u16; > + __be16 tmp_be16; > + > + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_ACT_REG_ADDR, > + sizeof(tmp_u16), (u8 *)&tmp_be16); > + if (ret < 0) { > + dev_err(&client->dev, "cannot read PAC1944 regs from 0x%02X\n", > + PAC1944_CTRL_ACT_REG_ADDR); > + return ret; > + } > + > + tmp_u16 = be16_to_cpu(tmp_be16); > + tmp_u16 &= ~PAC1944_CTRL_SAMPLE_MASK; > + tmp_u16 |= FIELD_PREP(PAC1944_CTRL_SAMPLE_MASK, mode); > + tmp_be16 = cpu_to_be16(tmp_u16); Do that cpu_to_be16() inline below. > + > + scoped_guard(mutex, &info->lock) { > + ret = i2c_smbus_write_word_data(client, PAC1944_CTRL_REG_ADDR, tmp_be16); > + if (ret < 0) { > + dev_err(&client->dev, "Failed to configure sampling mode\n"); > + return ret; > + } > + > + info->sampling_mode = mode; > + info->chip_reg_data.ctrl_act_reg = tmp_u16; > + } > + > + return pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US); > +} > + > +static int pac1944_frequency_get(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + > + return info->sampling_mode; > +} > + > +static const struct iio_enum sampling_mode_enum = { > + .items = pac1944_frequency_avail, > + .num_items = ARRAY_SIZE(pac1944_frequency_avail), > + .set = pac1944_frequency_set, > + .get = pac1944_frequency_get, > +}; Use the standard read_raw, and info_mask_shared, read_avail etc for this not the path meant for custom ABI. > + > +static int pac1944_read_label(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, char *label) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + int idx; > + > + /* into the datasheet channels are noted from 1 to 4 */ > + idx = chan->channel - 1; Comment as suggested below would read better. > + > + /* > + * For AVG the index should be between 5 to 8. > + * To calculate PAC1944_CH_VOLTAGE_AVERAGE and > + * PAC1944_CH_CURRENT_AVERAGE real index, we need > + * to remove the added offset (PAC1944_MAX_CH). Wrap to 80 chars > + */ > + if (idx >= PAC1944_MAX_CH) > + idx = idx - PAC1944_MAX_CH; Is this same as idx %= PA1944_MAX_CH; or maybe keep the more obvious single subtraction version. if (idx >= PAC1944_MAX_CH) idx -= PAC1944_MAX_CH; Various repeats of this occur elsewhere so apply an changes to all of them. > + > + switch (chan->address) { > + case PAC1944_VBUS_1_ADDR: > + case PAC1944_VBUS_2_ADDR: > + case PAC1944_VBUS_3_ADDR: > + case PAC1944_VBUS_4_ADDR: > + if (info->labels[idx]) > + return sysfs_emit(label, "%s_VBUS_%d\n", info->labels[idx], idx + 1); > + else Technically else not needed as returned in other path. I don't mind keeping it if you strong prefer though. > + return sysfs_emit(label, "VBUS_%d\n", idx + 1); > + case PAC1944_VBUS_AVG_1_ADDR: > + case PAC1944_VBUS_AVG_2_ADDR: > + case PAC1944_VBUS_AVG_3_ADDR: > + case PAC1944_VBUS_AVG_4_ADDR: > + if (info->labels[idx]) > + return sysfs_emit(label, "%s_VBUS_AVG_%d\n", info->labels[idx], idx + 1); > + else > + return sysfs_emit(label, "VBUS_AVG_%d\n", idx + 1); > + case PAC1944_VSENSE_1_ADDR: > + case PAC1944_VSENSE_2_ADDR: > + case PAC1944_VSENSE_3_ADDR: > + case PAC1944_VSENSE_4_ADDR: > + if (info->labels[idx]) > + return sysfs_emit(label, "%s_IBUS_%d\n", info->labels[idx], idx + 1); > + else > + return sysfs_emit(label, "IBUS_%d\n", idx + 1); > + case PAC1944_VSENSE_AVG_1_ADDR: > + case PAC1944_VSENSE_AVG_2_ADDR: > + case PAC1944_VSENSE_AVG_3_ADDR: > + case PAC1944_VSENSE_AVG_4_ADDR: > + if (info->labels[idx]) > + return sysfs_emit(label, "%s_IBUS_AVG_%d\n", info->labels[idx], idx + 1); > + else > + return sysfs_emit(label, "IBUS_AVG_%d\n", idx + 1); > + case PAC1944_VPOWER_1_ADDR: > + case PAC1944_VPOWER_2_ADDR: > + case PAC1944_VPOWER_3_ADDR: > + case PAC1944_VPOWER_4_ADDR: > + if (info->labels[idx]) > + return sysfs_emit(label, "%s_POWER_%d\n", info->labels[idx], idx + 1); > + else > + return sysfs_emit(label, "POWER_%d\n", idx + 1); > + } > + > + return 0; > +} > +static int pac1944_write_thresh(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, enum iio_event_type type, > + enum iio_event_direction dir, enum iio_event_info info, > + int val, int val2) > +{ > + struct pac1944_chip_info *chip_info = iio_priv(indio_dev); > + int idx, ret; > + > + /* into the datasheet channels are noted from 1 to 4 */ > + idx = chan->channel - 1; > + > + guard(mutex)(&chip_info->lock); > + > + switch (chan->type) { > + case IIO_VOLTAGE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = pac1944_update_alert_16b(&indio_dev->dev, > + PAC1944_OV_LIMIT_REG_ADDR + idx, > + pac1944_overvoltage_mask_tbl[idx], val); > + if (ret) > + return ret; > + > + chip_info->overvoltage[idx] = val; > + return 0; > + case IIO_EV_DIR_FALLING: > + ret = pac1944_update_alert_16b(&indio_dev->dev, > + PAC1944_UV_LIMIT_REG_ADDR + idx, > + pac1944_undervoltage_mask_tbl[idx], val); > + if (ret) > + return ret; > + > + chip_info->undervoltage[idx] = val; > + return 0; > + default: > + return -EINVAL; > + } > + case IIO_CURRENT: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = pac1944_update_alert_16b(&indio_dev->dev, > + PAC1944_OC_LIMIT_REG_ADDR + idx, > + pac1944_overcurrent_mask_tbl[idx], val); > + if (ret) > + return ret; > + > + chip_info->overcurrent[idx] = val; > + return ret; return 0; > + case IIO_EV_DIR_FALLING: > + ret = pac1944_update_alert_16b(&indio_dev->dev, > + PAC1944_UC_LIMIT_REG_ADDR + idx, > + pac1944_undercurrent_mask_tbl[idx], val); > + if (ret) > + return ret; > + > + chip_info->undercurrent[idx] = val; > + return 0; > + default: > + return -EINVAL; > + } > + case IIO_POWER: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = pac1944_update_alert_24b(&indio_dev->dev, > + PAC1944_OP_LIMIT_REG_ADDR + idx, > + pac1944_overpower_mask_tbl[idx], > + val); > + if (ret) > + return ret; > + > + chip_info->overpower[idx] = val; > + return 0; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int pac1944_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + int idx; > + u32 tmp; > + > + /* into the datasheet channels are noted from 1 to 4 */ > + idx = chan->channel - 1; > + > + scoped_guard(mutex, &info->lock) { guard() is fine and will reduce indent which will slightly help readability. The tiny bit of maths at the end isn't significant enough to care if we release the lock before it or not. > + switch (chan->type) { > + case IIO_VOLTAGE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + tmp = FIELD_GET(PAC1944_OV_MASK, info->alert_enable); > + break; > + case IIO_EV_DIR_FALLING: > + tmp = FIELD_GET(PAC1944_UV_MASK, info->alert_enable); > + break; > + default: > + return -EINVAL; > + } > + return -EINVAL; > + case IIO_CURRENT: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + tmp = FIELD_GET(PAC1944_OC_MASK, info->alert_enable); > + break; > + case IIO_EV_DIR_FALLING: > + tmp = FIELD_GET(PAC1944_UC_MASK, info->alert_enable); > + break; > + default: > + return -EINVAL; > + } > + return -EINVAL; > + case IIO_POWER: > + if (dir == IIO_EV_DIR_RISING) > + tmp = FIELD_GET(PAC1944_OP_MASK, info->alert_enable); > + else > + return -EINVAL; Flip logic to simplify. if (dir != IIO_EV_DIR_RISING) return -EINVal tmp = FIELD_GET(... Of just use a switch to keep it looking like the other code. > + break; > + default: > + return -EINVAL; > + } > + } > + > + tmp = tmp >> (3 - idx); > + > + return tmp & 0x01; > +} > + > +static int pac1944_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + bool state) > +{ > + struct pac1944_chip_info *info = iio_priv(indio_dev); > + struct i2c_client *client = info->client; > + int idx, val, mask, ret; > + bool update = false; > + u8 tmp[PAC1944_ALERT_ENABLE_REG_LEN]; > + > + /* into the datasheet channels are noted from 1 to 4 */ In the datasheet channels are referred to as 1 to 4 > + idx = chan->channel - 1; > + > + switch (chan->type) { > + case IIO_VOLTAGE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + mask = pac1944_overvoltage_mask_tbl[idx]; > + break; > + case IIO_EV_DIR_FALLING: > + mask = pac1944_undervoltage_mask_tbl[idx]; > + break; > + default: > + return -EINVAL; > + } > + break; > + case IIO_CURRENT: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + mask = pac1944_overcurrent_mask_tbl[idx]; > + break; > + case IIO_EV_DIR_FALLING: > + mask = pac1944_undercurrent_mask_tbl[idx]; > + break; > + default: > + return -EINVAL; > + } > + break; > + case IIO_POWER: > + if (dir != IIO_EV_DIR_RISING) > + return -EINVAL; > + > + mask = pac1944_overpower_mask_tbl[idx]; > + break; > + default: > + return -EINVAL; > + } > + > + guard(mutex)(&info->lock); > + > + val = info->alert_enable & mask; > + if (state && !val) { > + /* enable the event in hardware */ > + info->alert_enable |= mask; > + update = true; > + } else if (!state && val) { > + /* disable the event in hardware */ > + info->alert_enable &= ~mask; > + update = true; > + } > + > + /* do not update if not needed */ > + if (update) { > + put_unaligned_be24(info->alert_enable, &tmp[0]); > + > + /* update the Alert enable register */ > + ret = pac1944_restore_alert_reg(indio_dev, &tmp[0]); > + if (ret) { > + dev_err(&client->dev, "failing to restore alert reg\n"); > + return ret; > + } > + } > + > + return 0; > +} > + > +static int pac1944_of_parse_channel_config(struct i2c_client *client, > + struct pac1944_chip_info *info) > +{ > + unsigned int current_channel; > + struct device *dev = &client->dev; > + int idx, ret, temp; > + bool is_bipolar, is_half_fsr; > + > + current_channel = 1; > + > + device_for_each_child_node_scoped(dev, child) { Perhaps it is worth factoring out the parsing of a single channel as a helper function. > + ret = fwnode_property_read_u32(child, "reg", &idx); > + if (ret) > + return dev_err_probe(dev, ret, "reading invalid channel index\n"); > + > + /* adjust idx to match channel index (1 to 4) from the datasheet */ > + idx--; > + > + if (current_channel >= (info->phys_channels + 1) || > + idx >= info->phys_channels || idx < 0) > + return dev_err_probe(dev, -EINVAL, "invalid channel index %d value\n", > + idx + 1); > + > + /* enable channel */ > + set_bit(idx, &info->active_channels_mask); > + > + ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", > + &info->shunts[idx]); > + if (ret) > + return dev_err_probe(dev, ret, "%s: invalid shunt-resistor value: %d\n", > + fwnode_get_name(child), info->shunts[idx]); > + > + if (fwnode_property_present(child, "label")) > + fwnode_property_read_string(child, "label", > + (const char **)&info->labels[idx]); Given you don't check for errors for fwnode_property_read_string() is this not the same after dropping the fwnode_property_present() check? > + > + is_bipolar = false; > + if (fwnode_property_present(child, "microchip,vbus-bipolar")) > + is_bipolar = true; is_bipolar = fwnode_property_read_bool() probably appropriate here. > + > + is_half_fsr = false; > + if (fwnode_property_present(child, "microchip,vbus-half-range")) > + is_half_fsr = true; same here and the other cases below. > + > + /* default value is unipolar and Full Scale Range */ > + info->chip_reg_data.vbus_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; > + if (is_half_fsr) > + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; > + else if (is_bipolar) > + info->chip_reg_data.vbus_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; > + > + is_bipolar = false; > + if (fwnode_property_present(child, "microchip,vsense-bipolar")) > + is_bipolar = true; > + > + is_half_fsr = false; > + if (fwnode_property_present(child, "microchip,vsense-half-range")) > + is_half_fsr = true; > + > + /* default value is unipolar and Full Scale Range */ > + info->chip_reg_data.vsense_mode[idx] = PAC1944_UNIPOLAR_FSR_CFG; > + if (is_half_fsr) > + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_HALF_FSR_CFG; > + else if (is_bipolar) > + info->chip_reg_data.vsense_mode[idx] = PAC1944_BIPOLAR_FSR_CFG; > + > + ret = fwnode_property_read_u32(child, "microchip,accumulation-mode", &temp); Given temp is only used for this, give it a meaninful name. acc_mode or something like that. > + if (ret) > + return dev_err_probe(dev, ret, "invalid accumulation-mode value on %s\n", > + fwnode_get_name(child)); > + if (temp == PAC1944_ACCMODE_VPOWER || > + temp == PAC1944_ACCMODE_VSENSE || > + temp == PAC1944_ACCMODE_VBUS) { > + dev_dbg(dev, "Accumulation{%d} mode set to: %d\n", idx, temp); > + info->chip_reg_data.accumulation_mode[idx] = temp; > + } else { > + return dev_err_probe(dev, -EINVAL, > + "invalid mode for accumulator value on %s\n", > + fwnode_get_name(child)); > + } > + current_channel++; > + } > + > + return 0; > +} > + > +static void pac1944_cancel_delayed_work(void *dwork) > +{ > + cancel_delayed_work_sync(dwork); > +} > + > +static int pac1944_chip_identify(struct pac1944_chip_info *info) > +{ > + int ret = 0; Always set below so don't initialize. > + struct i2c_client *client = info->client; > + u8 chip_rev_info[3]; > + > + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_PID_REG_ADDR, > + sizeof(chip_rev_info), > + chip_rev_info); > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, "cannot read revision\n"); > + > + dev_info(&client->dev, "Chip revision: 0x%02X\n", chip_rev_info[2]); > + info->chip_revision = chip_rev_info[2]; Why do we keep a copy? Doesn't seem to be used again. > + info->chip_variant = chip_rev_info[0]; > + > + switch (chip_rev_info[0]) { > + case PAC_PRODUCT_ID_1941: > + case PAC_PRODUCT_ID_1942: > + case PAC_PRODUCT_ID_1943: > + case PAC_PRODUCT_ID_1944: > + case PAC_PRODUCT_ID_1941_2: > + case PAC_PRODUCT_ID_1942_2: > + info->is_pac195x_family = false; This is a little odd. We set one element of info in here, but return the value of ret for the others. I'd be inclined to just set all the stuff in info directly in this function. I'm also not sure why that is_pac194x_family isn't just encoded in the chip specific structures? Why do we need to do it explicitly here. > + return chip_rev_info[0] - PAC_PRODUCT_ID_1941; > + case PAC_PRODUCT_ID_1951: > + case PAC_PRODUCT_ID_1952: > + case PAC_PRODUCT_ID_1953: > + case PAC_PRODUCT_ID_1954: > + case PAC_PRODUCT_ID_1951_2: > + case PAC_PRODUCT_ID_1952_2: > + info->is_pac195x_family = true; > + return (chip_rev_info[0] - PAC_PRODUCT_ID_1951) + > + (PAC_PRODUCT_ID_1942_2 - PAC_PRODUCT_ID_1941) + 1; > + default: > + dev_err(&client->dev, > + "product ID (0x%02X, 0x%02X, 0x%02X) for this part doesn't match\n", > + chip_rev_info[0], chip_rev_info[1], chip_rev_info[2]); > + return -EINVAL; > + } > +} > + > +static int pac1944_chip_configure(struct pac1944_chip_info *info) > +{ > + int cnt, ret; > + struct i2c_client *client = info->client; > + u8 regs[PAC1944_ALERTS_REG_LEN]; > + u8 *offset_p; > + u32 wait_time; > + u8 tmp_u8; > + __be16 tmp_be16; > + u16 cfg; > + > + /* > + * Counting how many channels are enabled and store Count how many... > + * this information within the driver data Wrap at 80 chars. Also, when it's a sentence, nice to add a . at the end. > + */ > + info->num_enabled_channels = hweight_long(info->active_channels_mask); > + > + /* get sampling rate from PAC */ > + ret = i2c_smbus_read_i2c_block_data(client, PAC1944_CTRL_REG_ADDR, > + sizeof(tmp_be16), (u8 *)&tmp_be16); > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, "cannot read CTRL reg\n"); > + > + info->sampling_mode = FIELD_GET(PAC1944_CTRL_SAMPLE_MASK, be16_to_cpu(tmp_be16)); > + > + /* > + * The current/voltage can be measured unidirectional, bidirectional or half FSR > + * no SLOW triggered REFRESH, clear POR > + */ > + cfg = FIELD_PREP(PAC1944_NEG_PWR_CFG_VS1_MASK, info->chip_reg_data.vsense_mode[0]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS2_MASK, info->chip_reg_data.vsense_mode[1]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS3_MASK, info->chip_reg_data.vsense_mode[2]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VS4_MASK, info->chip_reg_data.vsense_mode[3]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB1_MASK, info->chip_reg_data.vbus_mode[0]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB2_MASK, info->chip_reg_data.vbus_mode[1]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB3_MASK, info->chip_reg_data.vbus_mode[2]) | > + FIELD_PREP(PAC1944_NEG_PWR_CFG_VB4_MASK, info->chip_reg_data.vbus_mode[3]); > + > + ret = i2c_smbus_write_word_data(client, PAC1944_NEG_PWR_FSR_REG_ADDR, cpu_to_be16(cfg)); Been a while, but from what I recall smbus has a defined byte ordering. So independent of CPU endianness if a u16 is written it should end up in the same order on the bus. For that reason the swaps for word_data read/write, if needed, should be unconditional. ret = i2c_smbus_write_word_swapped() I'm also a little curious that a device supports smbus_write_word, but not smbus_read_word which would simplify the read above? Looking at the family datasheet I'm not seeing either word protocol as supported. For the block access above, it's a different game as there is no ordering on bus going on (as we don't know the word length) hence the swap is correctly be16_to_cpu(). > + if (ret) > + return dev_err_probe(&client->dev, ret, "cannot write NEG_PWR_FSR reg\n"); > + > + ret = i2c_smbus_write_word_data(client, PAC1944_SLOW_REG_ADDR, 0); > + if (ret) > + return dev_err_probe(&client->dev, ret, "cannot write SLOW reg\n"); > + > + /* Write the CHANNEL_N_OFF from CTRL REGISTER */ > + cfg = FIELD_PREP(PAC1944_CTRL_SAMPLE_MASK, info->sampling_mode) | > + FIELD_PREP(PAC1944_CTRL_GPIO_ALERT2_MASK, 0) | > + FIELD_PREP(PAC1944_CTRL_SLOW_ALERT1_MASK, 0) | > + FIELD_PREP(PAC1944_CTRL_CH_1_OFF_MASK, !test_bit(0, &info->active_channels_mask)) | > + FIELD_PREP(PAC1944_CTRL_CH_2_OFF_MASK, !test_bit(1, &info->active_channels_mask)) | > + FIELD_PREP(PAC1944_CTRL_CH_3_OFF_MASK, !test_bit(2, &info->active_channels_mask)) | > + FIELD_PREP(PAC1944_CTRL_CH_4_OFF_MASK, !test_bit(3, &info->active_channels_mask)); > + > + ret = i2c_smbus_write_word_data(client, PAC1944_CTRL_REG_ADDR, cpu_to_be16(cfg)); > + if (ret) > + return dev_err_probe(&client->dev, ret, "cannot write CTRL reg\n"); > + > + tmp_u8 = ACCUM_REG(info->chip_reg_data.accumulation_mode[0], Might as well give tmp_u8 a more meaningful name as it is only used for this. > + info->chip_reg_data.accumulation_mode[1], > + info->chip_reg_data.accumulation_mode[2], > + info->chip_reg_data.accumulation_mode[3]); > + > + ret = i2c_smbus_write_byte_data(client, PAC1944_ACCUM_CFG_REG_ADDR, tmp_u8); > + if (ret) > + return dev_err_probe(&client->dev, ret, "cannot write ACCUM_CFG reg\n"); > + > + /* reading all alerts, status and limits related registers */ > + ret = pac1944_i2c_read(client, PAC1944_ALERT_STATUS_REG_ADDR, regs, sizeof(regs)); > + if (ret < 0) > + return dev_err_probe(&client->dev, ret, "cannot read ALERT_STATUS reg\n"); > + > + offset_p = ®s[0]; ... > +static int pac1944_probe(struct i2c_client *client) > +{ > + struct pac1944_chip_info *info; > + struct iio_dev *indio_dev; > + const struct pac1944_features *chip; > + int cnt, ret; > + struct device *dev = &client->dev; Where there is no other reason to pick an order for declarations, go with reverse xmas tree. It's not important (to me anyway) but might as well pick a style and keep to it. > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + info = iio_priv(indio_dev); > + info->client = client; > + > + ret = pac1944_chip_identify(info); > + if (ret < 0) { > + dev_dbg(dev, "Failed to identify the device based on HW ID\n"); > + /* > + * If failed to identify the hardware based on internal registers, > + * try using fallback compatible in device tree to deal with > + * some newer part number. > + */ > + chip = i2c_get_match_data(client); > + if (!chip) > + return -EINVAL; > + > + info->chip_variant = chip->prod_id; What is this for? It's set but not I think used. > + info->phys_channels = chip->phys_channels; > + indio_dev->name = chip->name; > + } else { > + info->phys_channels = pac1944_chip_config[ret].phys_channels; > + indio_dev->name = pac1944_chip_config[ret].name; This doesn't handle the the family bit of info or setting chip_variant. So looks like it is missing at least some stuff overthe case where we don't match. > + } > + > + for (cnt = 0; cnt < info->phys_channels; cnt++) { > + /* always start with accumulation channels enabled */ > + info->enable_acc[cnt] = true; > + } > + > + if (ACPI_HANDLE(dev)) > + ret = pac1944_acpi_parse_channel_config(client, info); > + else > + ret = pac1944_of_parse_channel_config(client, info); > + > + if (ret) > + return dev_err_probe(dev, ret, "parameter parsing returned an error\n"); > + > + ret = devm_mutex_init(dev, &info->lock); > + if (ret < 0) > + return ret; > + > + ret = pac1944_chip_configure(info); > + if (ret < 0) > + return ret; > + > + ret = pac1944_prep_iio_channels(info, indio_dev); > + if (ret < 0) > + return ret; > + > + info->iio_info = pac1944_info; > + indio_dev->info = &info->iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = pac1944_prep_custom_attributes(info, indio_dev); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Can't configure custom attributes for device\n"); > + > + ret = pac1944_reg_snapshot(info, true, false, > + PAC1944_MIN_UPDATE_WAIT_TIME_US); > + if (ret < 0) > + return ret; > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Can't register IIO device\n"); > + > + return 0; > +} > + > +static const struct i2c_device_id pac1944_id[] = { > + { .name = "pac1941", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941] }, > + { .name = "pac19412", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1941_2] }, > + { .name = "pac1942", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942] }, > + { .name = "pac19422", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1942_2] }, > + { .name = "pac1943", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1943] }, > + { .name = "pac1944", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] }, > + { .name = "pac1951", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951] }, > + { .name = "pac19512", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1951_2] }, > + { .name = "pac1952", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952] }, > + { .name = "pac19522", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1952_2] }, > + { .name = "pac1953", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1953] }, > + { .name = "pac1954", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1954] }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, pac1944_id); > + > +static const struct of_device_id pac1944_of_match[] = { > + { > + .compatible = "microchip,pac1941", > + .data = (void *)&pac1944_chip_config[PAC1941] Why do we need the cast to a void * ? They should be const pointers anyway so assignment to a const void * should be fine without cast. > + }, > + { > + .compatible = "microchip,pac19412", > + .data = (void *)&pac1944_chip_config[PAC1941_2] > + }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, pac1944_of_match); > + > +static const struct acpi_device_id pac1944_acpi_match[] = { > + { "MCHP1940", .driver_data = (kernel_ulong_t)&pac1944_chip_config[PAC1944] }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, pac1944_acpi_match); > + > +static struct i2c_driver pac1944_driver = { > + .driver = { > + .name = "pac1944", > + .of_match_table = pac1944_of_match, > + .acpi_match_table = pac1944_acpi_match > + }, > + .probe = pac1944_probe, > + .id_table = pac1944_id, > +}; > +module_i2c_driver(pac1944_driver); > + > +MODULE_AUTHOR("Marius Cristea <marius.cristea@microchip.com>"); > +MODULE_DESCRIPTION("Microchip PAC194X and PAC195X Power Monitor"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-07 15:27 ` Jonathan Cameron @ 2025-06-10 15:58 ` Marius.Cristea 2025-06-11 16:12 ` Jonathan Cameron 0 siblings, 1 reply; 18+ messages in thread From: Marius.Cristea @ 2025-06-10 15:58 UTC (permalink / raw) To: jic23 Cc: dlechner, nuno.sa, broonie, linux-iio, robh, linux-kernel, andy, krzk+dt, conor+dt, devicetree On Sat, 2025-06-07 at 16:27 +0100, Jonathan Cameron wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, 6 Jun 2025 12:39:29 +0300 > <marius.cristea@microchip.com> wrote: > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > This is the iio driver for Microchip PAC194X and PAC195X series of > > Power Monitors with Accumulator. The PAC194X family supports 9V > > Full-Scale Range and the PAC195X supports 32V Full-Scale Range. > > > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > > are for high-side current sensing and the PAC194X/5X-2 devices are > > for low-side current sensing or floating VBUS applications. The > > PAC194X/5X-1 is named shortly PAC194X/5X. > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > Hi Marius, > > I entirely agree with David on this wanting splitting up into > a base driver + patches that add features to that in order to > make it easier to review. That also potentially allows me to pick up > the basic support whilst any more controversial parts are still under > discussion. > > Jonathan I will try to make it smaller and build on that. > > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > index 09ae6edb2650..ee47d880babf 100644 > > --- a/drivers/iio/adc/Makefile > > +++ b/drivers/iio/adc/Makefile > > @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o > > obj-$(CONFIG_NPCM_ADC) += npcm_adc.o > > obj-$(CONFIG_PAC1921) += pac1921.o > > obj-$(CONFIG_PAC1934) += pac1934.o > > +obj-$(CONFIG_PAC1944) += pac1944.o > > obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o > > obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > > obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o > > diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c > > new file mode 100644 > > index 000000000000..ce09334b076a > > --- /dev/null > > +++ b/drivers/iio/adc/pac1944.c > > @@ -0,0 +1,2841 @@ > > > > +/* Available Sample Modes */ > > +static const char * const pac1944_frequency_avail[] = { > > + "1024_ADAP", > > + "256_ADAP", > > This adaptive mode shouldn't be controlled via this standard > ABI. That needs to be considered separately. > > > + "64_ADAP", > > + "8_ADAP", > > + "1024", > > + "256", > > + "64", > > + "8", > This does not look even close to ABI complaint. > > The numbers cases are fine. The others not really. > > + "single_shot_1x", > > That has nothing directly to do with the sampling frequency. > Some others look suspicious. I'd stick to normal > sampling_frequency handling and have the discussion about the other > modes in here at a later date. > > > + "single_shot_8x", > > + "fast", > > + "burst", > > +}; The adaptive mode is used to lower the power consumption of the device and keep the "same" calculation into the driver. I will try to look over an ABI to put the device into a low power mode and in this way I could change the frequency to ADAPTIVE. The "single_shot_1x" and "single_shot_8x" is a special case when the user could take only one measurement and that measurement could be triggered/synched. > > > > ... > > > > > > > +static struct attribute *pac1944_power_acc_attr[] = { > > + &iio_dev_attr_in_energy1_raw.dev_attr.attr, > > + &iio_dev_attr_in_energy2_raw.dev_attr.attr, > > + &iio_dev_attr_in_energy3_raw.dev_attr.attr, > > + &iio_dev_attr_in_energy4_raw.dev_attr.attr, > > + &iio_dev_attr_in_energy1_scale.dev_attr.attr, > > + &iio_dev_attr_in_energy2_scale.dev_attr.attr, > > + &iio_dev_attr_in_energy3_scale.dev_attr.attr, > > + &iio_dev_attr_in_energy4_scale.dev_attr.attr, > > These look like standard read_raw / info_mask based attributes will > work. > So do that, not custom attributes that are both harder to review and > don't work with in kernel consumers (which we may well see for a > power > monitoring chip). > Because the device could support different modes of operation for the same accumulator, but only one at a time. I was trying to add to the sysfs only the mode that was set from the device tree. > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] iio: adc: adding support for PAC194X 2025-06-10 15:58 ` Marius.Cristea @ 2025-06-11 16:12 ` Jonathan Cameron 0 siblings, 0 replies; 18+ messages in thread From: Jonathan Cameron @ 2025-06-11 16:12 UTC (permalink / raw) To: Marius.Cristea Cc: dlechner, nuno.sa, broonie, linux-iio, robh, linux-kernel, andy, krzk+dt, conor+dt, devicetree On Tue, 10 Jun 2025 15:58:13 +0000 <Marius.Cristea@microchip.com> wrote: > On Sat, 2025-06-07 at 16:27 +0100, Jonathan Cameron wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Fri, 6 Jun 2025 12:39:29 +0300 > > <marius.cristea@microchip.com> wrote: > > > > > From: Marius Cristea <marius.cristea@microchip.com> > > > > > > This is the iio driver for Microchip PAC194X and PAC195X series of > > > Power Monitors with Accumulator. The PAC194X family supports 9V > > > Full-Scale Range and the PAC195X supports 32V Full-Scale Range. > > > > > > There are two versions of the PAC194X/5X: the PAC194X/5X-1 devices > > > are for high-side current sensing and the PAC194X/5X-2 devices are > > > for low-side current sensing or floating VBUS applications. The > > > PAC194X/5X-1 is named shortly PAC194X/5X. > > > > > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com> > > Hi Marius, > > > > I entirely agree with David on this wanting splitting up into > > a base driver + patches that add features to that in order to > > make it easier to review. That also potentially allows me to pick up > > the basic support whilst any more controversial parts are still under > > discussion. > > > > Jonathan > > I will try to make it smaller and build on that. > > > > > > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > > > index 09ae6edb2650..ee47d880babf 100644 > > > --- a/drivers/iio/adc/Makefile > > > +++ b/drivers/iio/adc/Makefile > > > @@ -103,6 +103,7 @@ obj-$(CONFIG_NCT7201) += nct7201.o > > > obj-$(CONFIG_NPCM_ADC) += npcm_adc.o > > > obj-$(CONFIG_PAC1921) += pac1921.o > > > obj-$(CONFIG_PAC1934) += pac1934.o > > > +obj-$(CONFIG_PAC1944) += pac1944.o > > > obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o > > > obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o > > > obj-$(CONFIG_QCOM_SPMI_ADC5) += qcom-spmi-adc5.o > > > diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c > > > new file mode 100644 > > > index 000000000000..ce09334b076a > > > --- /dev/null > > > +++ b/drivers/iio/adc/pac1944.c > > > @@ -0,0 +1,2841 @@ > > > > > > > +/* Available Sample Modes */ > > > +static const char * const pac1944_frequency_avail[] = { > > > + "1024_ADAP", > > > + "256_ADAP", > > > > This adaptive mode shouldn't be controlled via this standard > > ABI. That needs to be considered separately. > > > > > + "64_ADAP", > > > + "8_ADAP", > > > + "1024", > > > + "256", > > > + "64", > > > + "8", > > This does not look even close to ABI complaint. > > > > The numbers cases are fine. The others not really. > > > + "single_shot_1x", > > > > That has nothing directly to do with the sampling frequency. > > Some others look suspicious. I'd stick to normal > > sampling_frequency handling and have the discussion about the other > > modes in here at a later date. > > > > > + "single_shot_8x", > > > + "fast", > > > + "burst", > > > +}; > > The adaptive mode is used to lower the power consumption of the device > and keep the "same" calculation into the driver. I will try to look > over an ABI to put the device into a low power mode and in this way I > could change the frequency to ADAPTIVE. We deliberately don't support 'mode' type switches for power saving in the ABI. They are just too varied for userspace to 'generically' know when to turn them on. Snag here is that it is a straight trade off between a clever algorithm which will introduce some error and straight forward sampling at high frequency. I don't currently have a good idea on how we'd control that. If we also had a dataready trigger involved (rather than accumulating etc) then maybe we'd do it via trigger selection, but that's not appropriate here given this is about driving the accumulators, not a dataready style readback of data (if we did that, we could just accumulate in user space instead of the device - a very different thing!). > > The "single_shot_1x" and "single_shot_8x" is a special case when the > user could take only one measurement and that measurement could be > triggered/synched. 8x seems to be about oversampling - but on a single trigger. In that sense it is a bit unusual. Looks like we could only do it effectively on an external IIO trigger? e.g. hrtimer or something like that? single shot 1x looks like a similar mode to use only on an external trigger (or a sysfs read). Can we just enable that only if we aren't using this devices own trigger? fast vs burst is an open question but taking just say fast that ends up being just another sampling frequency, be it one that changes with number of enabled channels. > > > > > > > > ... > > > > > > > > > > > > > > +static struct attribute *pac1944_power_acc_attr[] = { > > > + &iio_dev_attr_in_energy1_raw.dev_attr.attr, > > > + &iio_dev_attr_in_energy2_raw.dev_attr.attr, > > > + &iio_dev_attr_in_energy3_raw.dev_attr.attr, > > > + &iio_dev_attr_in_energy4_raw.dev_attr.attr, > > > + &iio_dev_attr_in_energy1_scale.dev_attr.attr, > > > + &iio_dev_attr_in_energy2_scale.dev_attr.attr, > > > + &iio_dev_attr_in_energy3_scale.dev_attr.attr, > > > + &iio_dev_attr_in_energy4_scale.dev_attr.attr, > > > > These look like standard read_raw / info_mask based attributes will > > work. > > So do that, not custom attributes that are both harder to review and > > don't work with in kernel consumers (which we may well see for a > > power > > monitoring chip). > > > > Because the device could support different modes of operation for the > same accumulator, but only one at a time. I was trying to add to the > sysfs only the mode that was set from the device tree. Create the iio_chan_spec dynamically instead of the attributes added. Jonathan > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2025-06-17 15:21 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-06 9:39 [PATCH v3 0/2] adding support for Microchip PAC194X Power Monitor marius.cristea 2025-06-06 9:39 ` [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X marius.cristea 2025-06-06 15:53 ` David Lechner 2025-06-10 14:46 ` Marius.Cristea 2025-06-10 15:22 ` David Lechner 2025-06-10 16:04 ` Marius.Cristea 2025-06-10 16:35 ` David Lechner 2025-06-17 15:21 ` Marius.Cristea 2025-06-06 16:24 ` Conor Dooley 2025-06-06 9:39 ` [PATCH v3 2/2] " marius.cristea 2025-06-06 17:02 ` David Lechner 2025-06-10 15:07 ` Marius.Cristea 2025-06-10 16:11 ` David Lechner 2025-06-11 15:59 ` Jonathan Cameron 2025-06-10 20:20 ` Andy Shevchenko 2025-06-07 15:27 ` Jonathan Cameron 2025-06-10 15:58 ` Marius.Cristea 2025-06-11 16:12 ` Jonathan Cameron
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).