* [PATCH 0/2] Add ADP1050 support
@ 2024-03-18 11:21 Radu Sabau
2024-03-18 11:21 ` [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings Radu Sabau
2024-03-18 11:21 ` [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support Radu Sabau
0 siblings, 2 replies; 13+ messages in thread
From: Radu Sabau @ 2024-03-18 11:21 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, Radu Sabau,
linux-hwmon, devicetree, linux-kernel, linux-doc, linux-i2c
The ADP1050 is an advanced digital controller with a PMBus™
interface targeting high density, high efficiency dc-to-dc power
conversion which can measure input/output voltages, input currents
and temperature.
Radu Sabau (2):
dt-bindings: hwmon: pmbus: adp1050 : add bindings
hwmon: pmbus: adp1050 : Add driver support
.../bindings/hwmon/pmbus/adi,adp1050.yaml | 65 ++++++++++
Documentation/hwmon/adp1050.rst | 69 +++++++++++
Documentation/hwmon/index.rst | 1 +
MAINTAINERS | 8 ++
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++
7 files changed, 265 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
create mode 100644 Documentation/hwmon/adp1050.rst
create mode 100644 drivers/hwmon/pmbus/adp1050.c
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings
2024-03-18 11:21 [PATCH 0/2] Add ADP1050 support Radu Sabau
@ 2024-03-18 11:21 ` Radu Sabau
2024-03-18 13:32 ` Rob Herring
` (2 more replies)
2024-03-18 11:21 ` [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support Radu Sabau
1 sibling, 3 replies; 13+ messages in thread
From: Radu Sabau @ 2024-03-18 11:21 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, Radu Sabau,
linux-hwmon, devicetree, linux-kernel, linux-doc, linux-i2c
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
+
+required:
+ - compatible
+ - reg
+ - vcc-supply
+ - adi,vin-scale-monitor
+ - adi,iin-scale-monitor
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #adress-cells = <1>;
+ #size-cells = <0>;
+ clock-frequency = <100000>;
+ adp1050@70 {
+ #adress-cells = <1>;
+ #size-cells = <0>;
+ compatible = "adi,adp1050";
+ reg = <0x70>;
+ adi,vin-scale-monitor = <0xB030>;
+ adi,iin-scale-monitor = <0x1>;
+ vcc-supply = <&vcc>;
+ };
+...
+
diff --git a/MAINTAINERS b/MAINTAINERS
index f4d7f7cb7577..c90140859988 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -479,6 +479,14 @@ L: linux-wireless@vger.kernel.org
S: Orphan
F: drivers/net/wireless/admtek/adm8211.*
+ADP1050 HARDWARE MONITOR DRIVER
+M: Radu Sabau <radu.sabau@analog.com>
+L: linux-hwmon@vger.kernel.org
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Dcumentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
+F: drivers/hwmon/pmbus/adp1050.c
+
ADP1653 FLASH CONTROLLER DRIVER
M: Sakari Ailus <sakari.ailus@iki.fi>
L: linux-media@vger.kernel.org
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
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 11:21 ` Radu Sabau
2024-03-18 15:06 ` Guenter Roeck
` (3 more replies)
1 sibling, 4 replies; 13+ messages in thread
From: Radu Sabau @ 2024-03-18 11:21 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, Radu Sabau,
linux-hwmon, devicetree, linux-kernel, linux-doc, linux-i2c
Add support for ADP1050 Digital Controller for Isolated Power Supplies
with PMBus interface Voltage, Current and Temperature Monitor.
The ADP1050 implements several features to enable a robust
system of parallel and redundant operation for customers who
require high availability. The device can measure voltage,
current and temperature that can be used in different
techniques to identify and safely shut down an erroneous
power supply in parallel operation mode.
Signed-off-by: Radu Sabau <radu.sabau@analog.com>
---
Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++
Documentation/hwmon/index.rst | 1 +
drivers/hwmon/pmbus/Kconfig | 10 +++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++
5 files changed, 192 insertions(+)
create mode 100644 Documentation/hwmon/adp1050.rst
create mode 100644 drivers/hwmon/pmbus/adp1050.c
diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst
new file mode 100644
index 000000000000..e3e5bb650a51
--- /dev/null
+++ b/Documentation/hwmon/adp1050.rst
@@ -0,0 +1,69 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver adp1050
+=====================
+
+Supported chips:
+
+ * Analog Devices ADP1050
+
+ Prefix: 'adp1050'
+
+ Addresses scanned: I2C 0x70 - 0x77
+
+ Datasheet: https://www.analog.com/media/en/technical-documentation/data-
+sheets/ADP1050.pdf
+
+Authors:
+
+ - Radu Sabau <radu.sabau@analog.com>
+
+
+Description
+-----------
+
+This driver supprts hardware monitoring for Analog Devices ADP1050 Digital
+Controller for Isolated Power Supply with PMBus interface.
+
+The ADP1050 is an advanced digital controller with a PMBus™
+interface targeting high density, high efficiency dc-to-dc power
+conversion used to monitor system temperatures, voltages and currents.
+Through the PMBus interface, the device can monitor input/output voltages,
+input current and temperature.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate
+the devices explicitly.
+Please see Documentation/i2c/instantiating-devices.rst for details.
+
+The vin scale monitor value and iin scale monitor value can be
+configured by a device tree property;see
+Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data.
+
+Sysfs Attributes
+----------------
+
+================= ========================================
+in1_label "vin"
+in1_input Measured input voltage
+in1_alarm Input voltage alarm
+in2_label "vout1"
+in2_input Measured output voltage
+in2_crit Critical maximum output voltage
+in2_crit_alarm Output voltage high alarm
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+curr1_label "iin"
+curr1_input Measured input current.
+curr1_alarm Input current alarm
+temp1_input Measured temperature
+temp1_crit Critical high temperature
+temp1_crit_alarm Chip temperature critical high alarm
+================= ========================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 1ca7a4fe1f8f..9a4fd576e6f6 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers
adm1266
adm1275
adm9240
+ adp1050
ads7828
adt7410
adt7411
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 557ae0c414b0..38e794d83cc3 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -57,6 +57,16 @@ config SENSORS_ADM1275
This driver can also be built as a module. If so, the module will
be called adm1275.
+config SENSORS_ADP1050
+ tristate "Analog Devices ADP1050 digital controller for Power Supplies"
+ help
+ If you say yes here you get hardware monitoring support for Analog
+ Devices ADP1050 digital controller for isolated power supply with
+ PMBus interface.
+
+ This driver can also be built as a module. If so, the module will
+ be called adp1050.
+
config SENSORS_BEL_PFE
tristate "Bel PFE Compatible Power Supplies"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index f14ecf03ad77..95a8dea5e5ed 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
+obj-$(CONFIG_SENSORS_ADP1050) += adp1050.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o
obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c
new file mode 100644
index 000000000000..53198d858156
--- /dev/null
+++ b/drivers/hwmon/pmbus/adp1050.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Hardware monitoring driver for Analog Devices ADP1050
+ *
+ * Copyright (C) 2024 Analog Devices, Inc.
+ */
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include "pmbus.h"
+
+#define ADP1050_CHIP_PASSWORD 0xD7
+
+#define ADP1050_VIN_SCALE_MONITOR 0xD8
+#define ADP1050_IIN_SCALE_MONITOR 0xD9
+
+static struct pmbus_driver_info adp1050_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+ .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
+ | PMBUS_HAVE_STATUS_TEMP,
+};
+
+static int adp1050_probe(struct i2c_client *client)
+{
+ u32 vin_scale_monitor, iin_scale_monitor;
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WRITE_WORD_DATA))
+ return -ENODEV;
+
+ /* Unlock CHIP's password in order to be able to read/write to it's
+ * VIN_SCALE and IIN_SCALE registers.
+ */
+ ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
+ if (ret < 0) {
+ dev_err_probe(&client->dev, "Device can't be unlocked.\n");
+ return ret;
+ }
+
+ ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
+ if (ret < 0) {
+ dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
+ return ret;
+ }
+
+ /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
+ * VIN monitor isn't used, therefore 0 is used as scale in order
+ * for the readings to return 0.
+ */
+ if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
+ &vin_scale_monitor))
+ vin_scale_monitor = 0;
+
+ /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
+ * IIN monitor isn't used, therefore 0 is used as scale in order
+ * for the readings to return 0.
+ */
+ if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
+ &iin_scale_monitor))
+ iin_scale_monitor = 0;
+
+ ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
+ vin_scale_monitor);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
+ iin_scale_monitor);
+ if (ret < 0)
+ return ret;
+
+ return pmbus_do_probe(client, &adp1050_info);
+}
+
+static const struct i2c_device_id adp1050_id[] = {
+ {"adp1050", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, adp1050_id);
+
+static const struct of_device_id adp1050_of_match[] = {
+ { .compatible = "adi,adp1050"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, adp1050_of_match);
+
+static struct i2c_driver adp1050_driver = {
+ .driver = {
+ .name = "adp1050",
+ .of_match_table = of_match_ptr(adp1050_of_match),
+ },
+ .probe = adp1050_probe,
+ .id_table = adp1050_id,
+};
+module_i2c_driver(adp1050_driver);
+
+MODULE_AUTHOR("Radu Sabau <radu.sabau@analog.com>");
+MODULE_DESCRIPTION("Analog Devices ADP1050 HWMON PMBus Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings
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
2024-03-18 16:10 ` Krzysztof Kozlowski
2 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2024-03-18 13:32 UTC (permalink / raw)
To: Radu Sabau
Cc: linux-doc, Delphine CC Chiu, Jean Delvare, linux-kernel,
linux-i2c, Guenter Roeck, Conor Dooley, linux-hwmon, devicetree,
Rob Herring, Krzysztof Kozlowski, Jonathan Corbet
On Mon, 18 Mar 2024 13:21:34 +0200, 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
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Traceback (most recent call last):
File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 909, in resolve_from_url
document = self.store[url]
~~~~~~~~~~^^^^^
File "/usr/local/lib/python3.11/dist-packages/jsonschema/_utils.py", line 28, in __getitem__
return self.store[self.normalize(uri)]
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
KeyError: 'htpps://devicetree.org/meta-schemes/core.yaml'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 912, in resolve_from_url
document = self.resolve_remote(url)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 1018, in resolve_remote
with urlopen(uri) as url:
^^^^^^^^^^^^
File "/usr/lib/python3.11/urllib/request.py", line 216, in urlopen
return opener.open(url, data, timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/urllib/request.py", line 519, in open
response = self._open(req, data)
^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/urllib/request.py", line 541, in _open
return self._call_chain(self.handle_open, 'unknown',
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.11/urllib/request.py", line 496, in _call_chain
result = func(*args)
^^^^^^^^^^^
File "/usr/lib/python3.11/urllib/request.py", line 1419, in unknown_open
raise URLError('unknown url type: %s' % type)
urllib.error.URLError: <urlopen error unknown url type: htpps>
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/usr/local/bin/dt-doc-validate", line 8, in <module>
sys.exit(main())
^^^^^^
File "/usr/local/lib/python3.11/dist-packages/dtschema/doc_validate.py", line 66, in main
ret |= check_doc(f)
^^^^^^^^^^^^
File "/usr/local/lib/python3.11/dist-packages/dtschema/doc_validate.py", line 29, in check_doc
for error in sorted(dtsch.iter_errors(), key=lambda e: e.linecol):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/dist-packages/dtschema/schema.py", line 120, in iter_errors
meta_schema = self.resolver.resolve_from_url(self['$schema'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/dist-packages/jsonschema/validators.py", line 914, in resolve_from_url
raise exceptions.RefResolutionError(exc)
jsonschema.exceptions.RefResolutionError: <urlopen error unknown url type: htpps>
Error: Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.example.dts:33.3-34.1 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240318112140.385244-2-radu.sabau@analog.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
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
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2024-03-18 15:06 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, linux-hwmon,
devicetree, linux-kernel, linux-doc, linux-i2c
On 3/18/24 04:21, Radu Sabau wrote:
> Add support for ADP1050 Digital Controller for Isolated Power Supplies
> with PMBus interface Voltage, Current and Temperature Monitor.
>
> The ADP1050 implements several features to enable a robust
> system of parallel and redundant operation for customers who
> require high availability. The device can measure voltage,
> current and temperature that can be used in different
> techniques to identify and safely shut down an erroneous
> power supply in parallel operation mode.
>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++
> Documentation/hwmon/index.rst | 1 +
> drivers/hwmon/pmbus/Kconfig | 10 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++
> 5 files changed, 192 insertions(+)
> create mode 100644 Documentation/hwmon/adp1050.rst
> create mode 100644 drivers/hwmon/pmbus/adp1050.c
>
> diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst
> new file mode 100644
> index 000000000000..e3e5bb650a51
> --- /dev/null
> +++ b/Documentation/hwmon/adp1050.rst
> @@ -0,0 +1,69 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver adp1050
> +=====================
> +
> +Supported chips:
> +
> + * Analog Devices ADP1050
> +
> + Prefix: 'adp1050'
> +
> + Addresses scanned: I2C 0x70 - 0x77
> +
> + Datasheet: https://www.analog.com/media/en/technical-documentation/data-
> +sheets/ADP1050.pdf
> +
> +Authors:
> +
> + - Radu Sabau <radu.sabau@analog.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for Analog Devices ADP1050 Digital
> +Controller for Isolated Power Supply with PMBus interface.
> +
> +The ADP1050 is an advanced digital controller with a PMBus™
> +interface targeting high density, high efficiency dc-to-dc power
> +conversion used to monitor system temperatures, voltages and currents.
> +Through the PMBus interface, the device can monitor input/output voltages,
> +input current and temperature.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate
> +the devices explicitly.
> +Please see Documentation/i2c/instantiating-devices.rst for details.
> +
> +The vin scale monitor value and iin scale monitor value can be
> +configured by a device tree property;see
> +Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data.
> +
> +Sysfs Attributes
> +----------------
> +
> +================= ========================================
> +in1_label "vin"
> +in1_input Measured input voltage
> +in1_alarm Input voltage alarm
> +in2_label "vout1"
> +in2_input Measured output voltage
> +in2_crit Critical maximum output voltage
> +in2_crit_alarm Output voltage high alarm
> +in2_lcrit Critical minimum output voltage
> +in2_lcrit_alarm Output voltage critical low alarm
> +curr1_label "iin"
> +curr1_input Measured input current.
> +curr1_alarm Input current alarm
> +temp1_input Measured temperature
> +temp1_crit Critical high temperature
> +temp1_crit_alarm Chip temperature critical high alarm
> +================= ========================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1ca7a4fe1f8f..9a4fd576e6f6 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers
> adm1266
> adm1275
> adm9240
> + adp1050
> ads7828
> adt7410
> adt7411
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 557ae0c414b0..38e794d83cc3 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -57,6 +57,16 @@ config SENSORS_ADM1275
> This driver can also be built as a module. If so, the module will
> be called adm1275.
>
> +config SENSORS_ADP1050
> + tristate "Analog Devices ADP1050 digital controller for Power Supplies"
> + help
> + If you say yes here you get hardware monitoring support for Analog
> + Devices ADP1050 digital controller for isolated power supply with
> + PMBus interface.
> +
> + This driver can also be built as a module. If so, the module will
> + be called adp1050.
> +
> config SENSORS_BEL_PFE
> tristate "Bel PFE Compatible Power Supplies"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index f14ecf03ad77..95a8dea5e5ed 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> +obj-$(CONFIG_SENSORS_ADP1050) += adp1050.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o
> obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
> diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c
> new file mode 100644
> index 000000000000..53198d858156
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/adp1050.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Hardware monitoring driver for Analog Devices ADP1050
> + *
> + * Copyright (C) 2024 Analog Devices, Inc.
> + */
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include "pmbus.h"
> +
> +#define ADP1050_CHIP_PASSWORD 0xD7
> +
> +#define ADP1050_VIN_SCALE_MONITOR 0xD8
> +#define ADP1050_IIN_SCALE_MONITOR 0xD9
> +
> +static struct pmbus_driver_info adp1050_info = {
> + .pages = 1,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = linear,
> + .format[PSC_CURRENT_IN] = linear,
> + .format[PSC_TEMPERATURE] = linear,
> + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> + | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> + | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
> + | PMBUS_HAVE_STATUS_TEMP,
> +};
> +
> +static int adp1050_probe(struct i2c_client *client)
> +{
> + u32 vin_scale_monitor, iin_scale_monitor;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
> + return -ENODEV;
> +
> + /* Unlock CHIP's password in order to be able to read/write to it's
> + * VIN_SCALE and IIN_SCALE registers.
> + */
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
> + return ret;
> + }
> +
The datasheet says that the factory default password is 0xffff. What if it isn't ?
Refusing to instantiate the chip because it can't be unlocked seems a bit extreme.
After all, the password _can_ be changed.
> + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
> + * VIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
> + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
> + &vin_scale_monitor))
> + vin_scale_monitor = 0;
> +
> + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
> + * IIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
> + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
> + &iin_scale_monitor))
> + iin_scale_monitor = 0;
> +
I don't think the "-monitor" extensions in those property names
add any value.
> + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
> + vin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
> + iin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
If vin and iin monitoring is disabled on purpose, why still set
PMBUS_HAVE_VIN and PMBUS_HAVE_IIN above ? Reporting input values as 0
if the values are explicitly not monitored does not make sense to me.
I am puzzled about this to start with. Unless I am missing something,
the data sheet has no indication that VIN monitoring and/or IIN monitoring
is actually turned off if iin_scale/vin_scale are set to 0. Why support
the option to disable them ? That doesn't make sense to me. Please explain
how/when this makes sense. Please also explain why chip support is made
to depend on devicetree support. Register values may have been be set by
rommon/bios or (much more likely) by manufacturing, or users could just
want to keep the chip default. Why force-override it or even force-disable
it ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings
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
2024-03-18 16:10 ` Krzysztof Kozlowski
2 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2024-03-18 15:17 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, linux-hwmon,
devicetree, linux-kernel, linux-doc, linux-i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] dt-bindings: hwmon: pmbus: adp1050 : add bindings
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
@ 2024-03-18 16:10 ` Krzysztof Kozlowski
2 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-18 16:10 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
On 18/03/2024 12: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>
Subject: drop space before ':'
> ---
> .../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
> +---
> +
Drop
> +$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
Missing blank line
> +properties:
> + compatbile:
Typo. And you did not test it...
> + 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
Missing blank line.
> + 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
> +
> +required:
> + - compatible
> + - reg
> + - vcc-supply
> + - adi,vin-scale-monitor
> + - adi,iin-scale-monitor
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #adress-cells = <1>;
Totally messed indentation.
Use 4 spaces for example indentation.
> + #size-cells = <0>;
> + clock-frequency = <100000>;
> + adp1050@70 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + #adress-cells = <1>;
> + #size-cells = <0>;
> + compatible = "adi,adp1050";
> + reg = <0x70>;
> + adi,vin-scale-monitor = <0xB030>;
> + adi,iin-scale-monitor = <0x1>;
> + vcc-supply = <&vcc>;
> + };
> +...
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f4d7f7cb7577..c90140859988 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -479,6 +479,14 @@ L: linux-wireless@vger.kernel.org
> S: Orphan
> F: drivers/net/wireless/admtek/adm8211.*
>
> +ADP1050 HARDWARE MONITOR DRIVER
> +M: Radu Sabau <radu.sabau@analog.com>
> +L: linux-hwmon@vger.kernel.org
> +S: Supported
> +W: https://ez.analog.com/linux-software-drivers
> +F: Dcumentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
> +F: drivers/hwmon/pmbus/adp1050.c
There is no such file...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
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-18 16:12 ` Krzysztof Kozlowski
2024-03-18 16:48 ` Guenter Roeck
2024-03-18 21:28 ` kernel test robot
2024-03-19 0:25 ` kernel test robot
3 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-18 16:12 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
On 18/03/2024 12:21, Radu Sabau wrote:
> Add support for ADP1050 Digital Controller for Isolated Power Supplies
> with PMBus interface Voltage, Current and Temperature Monitor.
>
...
> +static int adp1050_probe(struct i2c_client *client)
> +{
> + u32 vin_scale_monitor, iin_scale_monitor;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
> + return -ENODEV;
> +
> + /* Unlock CHIP's password in order to be able to read/write to it's
> + * VIN_SCALE and IIN_SCALE registers.
> + */
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
Syntax is: return dev_err_probe(). Same in other places.
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
> + return ret;
> + }
> +
> + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
> + * VIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
Please use Linux coding style comments. /* and aligned */.
> + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
> + &vin_scale_monitor))
> + vin_scale_monitor = 0;
> +
> + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
> + * IIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
> + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
> + &iin_scale_monitor))
> + iin_scale_monitor = 0;
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
> + vin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
> + iin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
> + return pmbus_do_probe(client, &adp1050_info);
> +}
> +
> +static const struct i2c_device_id adp1050_id[] = {
> + {"adp1050", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, adp1050_id);
> +
> +static const struct of_device_id adp1050_of_match[] = {
> + { .compatible = "adi,adp1050"},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, adp1050_of_match);
> +
> +static struct i2c_driver adp1050_driver = {
> + .driver = {
> + .name = "adp1050",
> + .of_match_table = of_match_ptr(adp1050_of_match),
Drop of_match_ptr, you will have here warnings.
> + },
> + .probe = adp1050_probe,
> + .id_table = adp1050_id,
> +};
> +module_i2c_driver(adp1050_driver);
> +
> +MODULE_AUTHOR("Radu Sabau <radu.sabau@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices ADP1050 HWMON PMBus Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
2024-03-18 16:12 ` Krzysztof Kozlowski
@ 2024-03-18 16:48 ` Guenter Roeck
2024-03-18 18:00 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2024-03-18 16:48 UTC (permalink / raw)
To: Krzysztof Kozlowski, Radu Sabau, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
On 3/18/24 09:12, Krzysztof Kozlowski wrote:
> On 18/03/2024 12:21, Radu Sabau wrote:
>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>> with PMBus interface Voltage, Current and Temperature Monitor.
>>
>
> ...
>
>> +static int adp1050_probe(struct i2c_client *client)
>> +{
>> + u32 vin_scale_monitor, iin_scale_monitor;
>> + int ret;
>> +
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>> + return -ENODEV;
>> +
>> + /* Unlock CHIP's password in order to be able to read/write to it's
>> + * VIN_SCALE and IIN_SCALE registers.
>> + */
>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>> + if (ret < 0) {
>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>
> Syntax is: return dev_err_probe(). Same in other places.
>
dev_err_probe() expects the error number as second parameter, so I don't
really understand how the above even compiles.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
2024-03-18 16:48 ` Guenter Roeck
@ 2024-03-18 18:00 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-18 18:00 UTC (permalink / raw)
To: Guenter Roeck, Radu Sabau, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
On 18/03/2024 17:48, Guenter Roeck wrote:
> On 3/18/24 09:12, Krzysztof Kozlowski wrote:
>> On 18/03/2024 12:21, Radu Sabau wrote:
>>> Add support for ADP1050 Digital Controller for Isolated Power Supplies
>>> with PMBus interface Voltage, Current and Temperature Monitor.
>>>
>>
>> ...
>>
>>> +static int adp1050_probe(struct i2c_client *client)
>>> +{
>>> + u32 vin_scale_monitor, iin_scale_monitor;
>>> + int ret;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + /* Unlock CHIP's password in order to be able to read/write to it's
>>> + * VIN_SCALE and IIN_SCALE registers.
>>> + */
>>> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
>>> + if (ret < 0) {
>>> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
>>
>> Syntax is: return dev_err_probe(). Same in other places.
>>
>
> dev_err_probe() expects the error number as second parameter, so I don't
> really understand how the above even compiles.
I did not explain the arguments, because they are obvious... but if you
need so:
return dev_err_probe(&client->dev, ret, "Device can't be unlocked.\n");
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
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-18 16:12 ` Krzysztof Kozlowski
@ 2024-03-18 21:28 ` kernel test robot
2024-03-19 0:25 ` kernel test robot
3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-03-18 21:28 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
Cc: oe-kbuild-all
Hi Radu,
kernel test robot noticed the following build errors:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on robh/for-next linus/master v6.8 next-20240318]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com
patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190552.U4RHYvqc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403190552.U4RHYvqc-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/hwmon/pmbus/adp1050.c: In function 'adp1050_probe':
>> drivers/hwmon/pmbus/adp1050.c:47:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion]
47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
In file included from include/linux/device.h:15,
from include/linux/acpi.h:14,
from include/linux/i2c.h:13,
from drivers/hwmon/pmbus/adp1050.c:9:
include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *'
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ~~~~^~~
>> drivers/hwmon/pmbus/adp1050.c:47:17: error: too few arguments to function 'dev_err_probe'
47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n");
| ^~~~~~~~~~~~~
include/linux/dev_printk.h:277:20: note: declared here
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ^~~~~~~~~~~~~
drivers/hwmon/pmbus/adp1050.c:53:45: warning: passing argument 2 of 'dev_err_probe' makes integer from pointer without a cast [-Wint-conversion]
53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
include/linux/dev_printk.h:277:64: note: expected 'int' but argument is of type 'char *'
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ~~~~^~~
drivers/hwmon/pmbus/adp1050.c:53:17: error: too few arguments to function 'dev_err_probe'
53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
| ^~~~~~~~~~~~~
include/linux/dev_printk.h:277:20: note: declared here
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ^~~~~~~~~~~~~
vim +/dev_err_probe +47 drivers/hwmon/pmbus/adp1050.c
> 9 #include <linux/i2c.h>
10 #include <linux/init.h>
11 #include <linux/kernel.h>
12 #include <linux/module.h>
13 #include <linux/of.h>
14 #include "pmbus.h"
15
16 #define ADP1050_CHIP_PASSWORD 0xD7
17
18 #define ADP1050_VIN_SCALE_MONITOR 0xD8
19 #define ADP1050_IIN_SCALE_MONITOR 0xD9
20
21 static struct pmbus_driver_info adp1050_info = {
22 .pages = 1,
23 .format[PSC_VOLTAGE_IN] = linear,
24 .format[PSC_VOLTAGE_OUT] = linear,
25 .format[PSC_CURRENT_IN] = linear,
26 .format[PSC_TEMPERATURE] = linear,
27 .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
28 | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
29 | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
30 | PMBUS_HAVE_STATUS_TEMP,
31 };
32
33 static int adp1050_probe(struct i2c_client *client)
34 {
35 u32 vin_scale_monitor, iin_scale_monitor;
36 int ret;
37
38 if (!i2c_check_functionality(client->adapter,
39 I2C_FUNC_SMBUS_WRITE_WORD_DATA))
40 return -ENODEV;
41
42 /* Unlock CHIP's password in order to be able to read/write to it's
43 * VIN_SCALE and IIN_SCALE registers.
44 */
45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
46 if (ret < 0) {
> 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n");
48 return ret;
49 }
50
51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
52 if (ret < 0) {
53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
54 return ret;
55 }
56
57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
58 * VIN monitor isn't used, therefore 0 is used as scale in order
59 * for the readings to return 0.
60 */
61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
62 &vin_scale_monitor))
63 vin_scale_monitor = 0;
64
65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
66 * IIN monitor isn't used, therefore 0 is used as scale in order
67 * for the readings to return 0.
68 */
69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
70 &iin_scale_monitor))
71 iin_scale_monitor = 0;
72
73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
74 vin_scale_monitor);
75 if (ret < 0)
76 return ret;
77
78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
79 iin_scale_monitor);
80 if (ret < 0)
81 return ret;
82
83 return pmbus_do_probe(client, &adp1050_info);
84 }
85
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
2024-03-18 11:21 ` [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support Radu Sabau
` (2 preceding siblings ...)
2024-03-18 21:28 ` kernel test robot
@ 2024-03-19 0:25 ` kernel test robot
3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-03-19 0:25 UTC (permalink / raw)
To: Radu Sabau, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Delphine CC Chiu, linux-hwmon, devicetree, linux-kernel,
linux-doc, linux-i2c
Cc: llvm, oe-kbuild-all
Hi Radu,
kernel test robot noticed the following build errors:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on robh/for-next linus/master v6.8 next-20240318]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radu-Sabau/dt-bindings-hwmon-pmbus-adp1050-add-bindings/20240318-202619
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20240318112140.385244-3-radu.sabau%40analog.com
patch subject: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 8f68022f8e6e54d1aeae4ed301f5a015963089b7)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240319/202403190800.h8cSGROp-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403190800.h8cSGROp-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/hwmon/pmbus/adp1050.c:9:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/hwmon/pmbus/adp1050.c:9:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/hwmon/pmbus/adp1050.c:9:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/hwmon/pmbus/adp1050.c:9:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:20:
In file included from include/linux/mm.h:2188:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/pmbus/adp1050.c:47:60: error: too few arguments to function call, expected at least 3, have 2
47 | dev_err_probe(&client->dev, "Device can't be unlocked.\n");
| ~~~~~~~~~~~~~ ^
include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hwmon/pmbus/adp1050.c:53:63: error: too few arguments to function call, expected at least 3, have 2
53 | dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
| ~~~~~~~~~~~~~ ^
include/linux/dev_printk.h:277:20: note: 'dev_err_probe' declared here
277 | __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7 warnings and 2 errors generated.
vim +47 drivers/hwmon/pmbus/adp1050.c
32
33 static int adp1050_probe(struct i2c_client *client)
34 {
35 u32 vin_scale_monitor, iin_scale_monitor;
36 int ret;
37
38 if (!i2c_check_functionality(client->adapter,
39 I2C_FUNC_SMBUS_WRITE_WORD_DATA))
40 return -ENODEV;
41
42 /* Unlock CHIP's password in order to be able to read/write to it's
43 * VIN_SCALE and IIN_SCALE registers.
44 */
45 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
46 if (ret < 0) {
> 47 dev_err_probe(&client->dev, "Device can't be unlocked.\n");
48 return ret;
49 }
50
51 ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
52 if (ret < 0) {
53 dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
54 return ret;
55 }
56
57 /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
58 * VIN monitor isn't used, therefore 0 is used as scale in order
59 * for the readings to return 0.
60 */
61 if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
62 &vin_scale_monitor))
63 vin_scale_monitor = 0;
64
65 /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
66 * IIN monitor isn't used, therefore 0 is used as scale in order
67 * for the readings to return 0.
68 */
69 if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
70 &iin_scale_monitor))
71 iin_scale_monitor = 0;
72
73 ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
74 vin_scale_monitor);
75 if (ret < 0)
76 return ret;
77
78 ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
79 iin_scale_monitor);
80 if (ret < 0)
81 return ret;
82
83 return pmbus_do_probe(client, &adp1050_info);
84 }
85
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/2] hwmon: pmbus: adp1050 : Add driver support
2024-03-18 15:06 ` Guenter Roeck
@ 2024-03-19 10:46 ` Sabau, Radu bogdan
0 siblings, 0 replies; 13+ messages in thread
From: Sabau, Radu bogdan @ 2024-03-19 10:46 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, Delphine CC Chiu, linux-hwmon,
devicetree, linux-kernel, linux-doc, linux-i2c
-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: Monday, March 18, 2024 5:07 PM
To: Sabau, Radu bogdan <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 2/2] hwmon: pmbus: adp1050 : Add driver support
[External]
On 3/18/24 04:21, Radu Sabau wrote:
> Add support for ADP1050 Digital Controller for Isolated Power Supplies
> with PMBus interface Voltage, Current and Temperature Monitor.
>
> The ADP1050 implements several features to enable a robust
> system of parallel and redundant operation for customers who
> require high availability. The device can measure voltage,
> current and temperature that can be used in different
> techniques to identify and safely shut down an erroneous
> power supply in parallel operation mode.
>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
> Documentation/hwmon/adp1050.rst | 69 ++++++++++++++++++++
> Documentation/hwmon/index.rst | 1 +
> drivers/hwmon/pmbus/Kconfig | 10 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/adp1050.c | 111 ++++++++++++++++++++++++++++++++
> 5 files changed, 192 insertions(+)
> create mode 100644 Documentation/hwmon/adp1050.rst
> create mode 100644 drivers/hwmon/pmbus/adp1050.c
>
> diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst
> new file mode 100644
> index 000000000000..e3e5bb650a51
> --- /dev/null
> +++ b/Documentation/hwmon/adp1050.rst
> @@ -0,0 +1,69 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver adp1050
> +=====================
> +
> +Supported chips:
> +
> + * Analog Devices ADP1050
> +
> + Prefix: 'adp1050'
> +
> + Addresses scanned: I2C 0x70 - 0x77
> +
> + Datasheet: https://www.analog.com/media/en/technical-documentation/data-
> +sheets/ADP1050.pdf
> +
> +Authors:
> +
> + - Radu Sabau <radu.sabau@analog.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for Analog Devices ADP1050 Digital
> +Controller for Isolated Power Supply with PMBus interface.
> +
> +The ADP1050 is an advanced digital controller with a PMBus™
> +interface targeting high density, high efficiency dc-to-dc power
> +conversion used to monitor system temperatures, voltages and currents.
> +Through the PMBus interface, the device can monitor input/output voltages,
> +input current and temperature.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate
> +the devices explicitly.
> +Please see Documentation/i2c/instantiating-devices.rst for details.
> +
> +The vin scale monitor value and iin scale monitor value can be
> +configured by a device tree property;see
> +Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml for details
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data.
> +
> +Sysfs Attributes
> +----------------
> +
> +================= ========================================
> +in1_label "vin"
> +in1_input Measured input voltage
> +in1_alarm Input voltage alarm
> +in2_label "vout1"
> +in2_input Measured output voltage
> +in2_crit Critical maximum output voltage
> +in2_crit_alarm Output voltage high alarm
> +in2_lcrit Critical minimum output voltage
> +in2_lcrit_alarm Output voltage critical low alarm
> +curr1_label "iin"
> +curr1_input Measured input current.
> +curr1_alarm Input current alarm
> +temp1_input Measured temperature
> +temp1_crit Critical high temperature
> +temp1_crit_alarm Chip temperature critical high alarm
> +================= ========================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1ca7a4fe1f8f..9a4fd576e6f6 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -33,6 +33,7 @@ Hardware Monitoring Kernel Drivers
> adm1266
> adm1275
> adm9240
> + adp1050
> ads7828
> adt7410
> adt7411
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 557ae0c414b0..38e794d83cc3 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -57,6 +57,16 @@ config SENSORS_ADM1275
> This driver can also be built as a module. If so, the module will
> be called adm1275.
>
> +config SENSORS_ADP1050
> + tristate "Analog Devices ADP1050 digital controller for Power Supplies"
> + help
> + If you say yes here you get hardware monitoring support for Analog
> + Devices ADP1050 digital controller for isolated power supply with
> + PMBus interface.
> +
> + This driver can also be built as a module. If so, the module will
> + be called adp1050.
> +
> config SENSORS_BEL_PFE
> tristate "Bel PFE Compatible Power Supplies"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index f14ecf03ad77..95a8dea5e5ed 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ACBEL_FSG032) += acbel-fsg032.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> +obj-$(CONFIG_SENSORS_ADP1050) += adp1050.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o
> obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o
> diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c
> new file mode 100644
> index 000000000000..53198d858156
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/adp1050.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Hardware monitoring driver for Analog Devices ADP1050
> + *
> + * Copyright (C) 2024 Analog Devices, Inc.
> + */
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include "pmbus.h"
> +
> +#define ADP1050_CHIP_PASSWORD 0xD7
> +
> +#define ADP1050_VIN_SCALE_MONITOR 0xD8
> +#define ADP1050_IIN_SCALE_MONITOR 0xD9
> +
> +static struct pmbus_driver_info adp1050_info = {
> + .pages = 1,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = linear,
> + .format[PSC_CURRENT_IN] = linear,
> + .format[PSC_TEMPERATURE] = linear,
> + .func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> + | PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> + | PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
> + | PMBUS_HAVE_STATUS_TEMP,
> +};
> +
> +static int adp1050_probe(struct i2c_client *client)
> +{
> + u32 vin_scale_monitor, iin_scale_monitor;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA))
> + return -ENODEV;
> +
> + /* Unlock CHIP's password in order to be able to read/write to it's
> + * VIN_SCALE and IIN_SCALE registers.
> + */
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device can't be unlocked.\n");
> + return ret;
> + }
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_CHIP_PASSWORD, 0xFFFF);
> + if (ret < 0) {
> + dev_err_probe(&client->dev, "Device couldn't be unlocked.\n");
> + return ret;
> + }
> +
The datasheet says that the factory default password is 0xffff. What if it isn't ?
Refusing to instantiate the chip because it can't be unlocked seems a bit extreme.
After all, the password _can_ be changed.
This is indeed a bit extreme, will remove it.
> + /* If adi,vin-scale-monitor isn't set or is set to 0 means that the
> + * VIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
> + if (device_property_read_u32(&client->dev, "adi,vin-scale-monitor",
> + &vin_scale_monitor))
> + vin_scale_monitor = 0;
> +
> + /* If adi,iin-scale-monitor isn't set or is set to 0 means that the
> + * IIN monitor isn't used, therefore 0 is used as scale in order
> + * for the readings to return 0.
> + */
> + if (device_property_read_u32(&client->dev, "adi,iin-scale-monitor",
> + &iin_scale_monitor))
> + iin_scale_monitor = 0;
> +
I don't think the "-monitor" extensions in those property names
add any value.
> + ret = i2c_smbus_write_word_data(client, ADP1050_VIN_SCALE_MONITOR,
> + vin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_write_word_data(client, ADP1050_IIN_SCALE_MONITOR,
> + iin_scale_monitor);
> + if (ret < 0)
> + return ret;
> +
If vin and iin monitoring is disabled on purpose, why still set
PMBUS_HAVE_VIN and PMBUS_HAVE_IIN above ? Reporting input values as 0
if the values are explicitly not monitored does not make sense to me.
I am puzzled about this to start with. Unless I am missing something,
the data sheet has no indication that VIN monitoring and/or IIN monitoring
is actually turned off if iin_scale/vin_scale are set to 0. Why support
the option to disable them ? That doesn't make sense to me. Please explain
how/when this makes sense. Please also explain why chip support is made
to depend on devicetree support. Register values may have been be set by
rommon/bios or (much more likely) by manufacturing, or users could just
want to keep the chip default. Why force-override it or even force-disable
it ?
At first I made the chip support depend on devicetree support because iin_scale
and vin_scale are values that should be calculated depending on external
hardware components. I tested with with ADP1050DC1-EVALZ board and there is
a resistor divider on the VF (VIN) pin. Although now that you said it, chip support
shouldn't depend on devicetree support, I tested the driver in this way and works the
same, in my case I have a resistor divider and have to modify the scale accordingly
in order for correct readings, and from a user point of view I actually found this way
more suitable.
Thanks,
Guenter
Thanks,
Radu
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-03-19 10:46 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.