| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d364524bad53f5c665071287f55a96e28dc9b231.camel@microchip.com>
Date: Tue, 10 Jun 2025 14:46:08 +0000
From: <Marius.Cristea@...rochip.com>
To: <jic23@...nel.org>, <nuno.sa@...log.com>, <dlechner@...libre.com>,
<andy@...nel.org>
CC: <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<broonie@...nel.org>, <devicetree@...r.kernel.org>,
<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X
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@...rochip.com wrote:
> > From: Marius Cristea <marius.cristea@...rochip.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@...rochip.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@...rochip.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;
> > + };
> > + };
> > + };
> > +
> > +...
Powered by blists - more mailing lists