From: Guenter Roeck <linux@roeck-us.net>
To: Radu Sabau <radu.sabau@analog.com>,
Jean Delvare <jdelvare@suse.com>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Jonathan Corbet <corbet@lwn.net>,
Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>,
linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-i2c@vger.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings
Date: Mon, 18 Mar 2024 08:17:46 -0700 [thread overview]
Message-ID: <e2caf1f3-97b8-4335-bb8c-be7fa2b40094@roeck-us.net> (raw)
In-Reply-To: <20240318112140.385244-2-radu.sabau@analog.com>
On 3/18/24 04:21, Radu Sabau wrote:
> Add dt-bindings for adp1050 digital controller for isolated power supply
> with pmbus interface voltage, current and temperature monitor.
>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> .../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 +++++++++++++++++++
> MAINTAINERS | 8 +++
> 2 files changed, 73 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> new file mode 100644
> index 000000000000..e3162d0df0e2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: htpps://devicetree.org/schemas/hwmon/pmbus/adi,adp1050.yaml#
> +$schema: htpps://devicetree.org/meta-schemes/core.yaml#
> +
> +title: Analog Devices ADP1050 digital controller with PMBus interface
> +
> +maintainers:
> + - Radu Sabau <radu.sabau@analog.com>
> +
> +description: |
> + The ADP1050 is used to monitor system voltages, currents and temperatures.
> + Through the PMBus interface, the ADP1050 targets isolated power supplies
> + and has four individual monitors for input/output voltage, input current
> + and temperature.
> + Datasheet:
> + https://www.analog.com/en/products/adp1050.html
> +properties:
> + compatbile:
> + const: adi,adp1050
> +
> + reg:
> + maxItems: 1
> +
> + vcc-supply: true
> +
> + adi,vin-scale-monitor:
> + description:
> + The value of the input voltage scale used by the internal ADP1050 ADC in
> + order to read correct voltage values.
> + $ref: /schemas/typees.yaml#/definitions/uint16
> + adi,iin-scale-monitor:
> + description:
> + The value of the input current scale used by the internal ADP1050 ADC in
> + order to read carrect current values.
> + $ref: /schemas/typees.yaml#/definitions/uint16
> +
I don't see value in "-monitor" in those property names.
Also, I don't see why those properties should be mandatory since the chip
has the ability to store its configuration in eeprom.
The proposed driver code disables vin and iin monitoring if the properties
are set to 0 or not provided. I disagree that this should be possible in
the first place (I don't see its value), but if disabling vin and iin
monitoring is supported it should be documented.
Last but not least, I think those values should be abstracted with some
scale instead of reflecting raw (and unexplained) register values.
Thanks,
Guenter
next prev parent reply other threads:[~2024-03-18 15:17 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 11:21 [PATCH 0/2] Add ADP1050 support Radu Sabau
2024-03-18 11:21 ` [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings Radu Sabau
2024-03-18 13:32 ` Rob Herring
2024-03-18 15:17 ` Guenter Roeck [this message]
2024-03-18 16:10 ` Krzysztof Kozlowski
2024-03-18 11:21 ` [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support Radu Sabau
2024-03-18 15:06 ` Guenter Roeck
2024-03-19 10:46 ` Sabau, Radu bogdan
2024-03-18 16:12 ` Krzysztof Kozlowski
2024-03-18 16:48 ` Guenter Roeck
2024-03-18 18:00 ` Krzysztof Kozlowski
2024-03-18 21:28 ` kernel test robot
2024-03-19 0:25 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e2caf1f3-97b8-4335-bb8c-be7fa2b40094@roeck-us.net \
--to=linux@roeck-us.net \
--cc=Delphine_CC_Chiu@Wiwynn.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=radu.sabau@analog.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).