Re: [PATCH v1 4/4] pmbus: Add support for bcm6123 Bus Converter
From: Guenter Roeck
Date: Tue Jan 18 2022 - 12:52:11 EST
On 1/18/22 8:47 AM, sylv wrote:
On Mon, 2022-01-17 at 09:21 -0800, Guenter Roeck wrote:
On 1/17/22 9:01 AM, Guenter Roeck wrote:
On 1/17/22 8:12 AM, Marcello Sylvester Bauer wrote:
From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
BCM6123 is an Fixed-Ratio DC-DC Converter.
Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
Signed-off-by: Marcello Sylvester Bauer <sylv@xxxxxxx>
---
drivers/hwmon/pmbus/Kconfig | 9 ++++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/bcm6123.c | 90 +++++++++++++++++++++++++++++++++++
Documentation/hwmon/bcm6123 is missing.
3 files changed, 100 insertions(+)
create mode 100644 drivers/hwmon/pmbus/bcm6123.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index c96f7b7338bd..62dac90631c5 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -48,6 +48,15 @@ config SENSORS_ADM1275
This driver can also be built as a module. If so, the module will
be called adm1275.
+config SENSORS_BCM6123
+ tristate "Vicor BCM6123 Compatible Power Supplies"
Is this a power supply or a chip ? It can't be both.
+ help
+ If you say yes here you get hardware monitoring support for Vicor
+ BCM6123 Power Supplies.
+
+ This driver can also be built as a module. If so, the module will
+ be called bcm6123.
+
config SENSORS_BEL_PFE
tristate "Bel PFE Compatible Power Supplies"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index e5935f70c9e0..2918c2ea7bc5 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
+obj-$(CONFIG_SENSORS_BCM6123) += bcm6123.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/bcm6123.c b/drivers/hwmon/pmbus/bcm6123.c
new file mode 100644
index 000000000000..78fc259bc40f
--- /dev/null
+++ b/drivers/hwmon/pmbus/bcm6123.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Infineon bcm6123
+ *
Infineon ?
+ * Copyright (c) 2021 9elements GmbH
+ *
+ * VOUT_MODE is not supported by the device. The driver fakes VOUT linear16
+ * mode with exponent value -8 as direct mode with m=256/b=0/R=0.
+ *
Does it not ? The datasheet doesn't say, and the code below doesn't match
this description.
+ * The device supports VOUT_PEAK, IOUT_PEAK, and TEMPERATURE_PEAK, however
+ * this driver does not currently support them.
Does it ? There is no reference for this in the datasheet.
Overall it seems like there is a lot of cut-and-paste. Please clean this up.
Sorry. Looks like it is not related to this driver and should be
removed.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include "pmbus.h"
+
+static struct pmbus_platform_data bcm6123_plat_data = {
+ .flags = PMBUS_NO_CAPABILITY,
+};
We should only set this flag if it is really needed.
The capability command did not return valid information during testing.
e.g. it claimed PEC is supported and caused therefore unexpected
issues. I'll test it again to see if it's necessary for the driver to
work.
+
+static struct pmbus_driver_info bcm6123_info = {
+ .pages = 2,
+ .format[PSC_VOLTAGE_IN] = direct,
+ .format[PSC_VOLTAGE_OUT] = direct,
+ .format[PSC_CURRENT_IN] = direct,
+ .format[PSC_CURRENT_OUT] = direct,
+ .format[PSC_POWER] = linear,
+ .format[PSC_TEMPERATURE] = linear,
+ .m[PSC_VOLTAGE_IN] = 1,
+ .b[PSC_VOLTAGE_IN] = 0,
+ .R[PSC_VOLTAGE_IN] = 1,
+ .m[PSC_VOLTAGE_OUT] = 1,
+ .b[PSC_VOLTAGE_OUT] = 0,
+ .R[PSC_VOLTAGE_OUT] = 1,
+ .m[PSC_CURRENT_IN] = 1,
+ .b[PSC_CURRENT_IN] = 0,
+ .R[PSC_CURRENT_IN] = 3,
+ .m[PSC_CURRENT_OUT] = 1,
+ .b[PSC_CURRENT_OUT] = 0,
+ .R[PSC_CURRENT_OUT] = 2,
+ .func[0] = 0, /* Summing page without voltage readings */
This needs further explanation. The public datasheet doesn't say anything
about multiple pages, and it doesn't really make much sense to have an
"empty" page with no information in it.
Digging further, there is actually a digital supervisor (D44TL1A0) which is used
to access BCM6123, and there can be up to 4 BCM6123 connected to D44TL1A0.
Page 0 is the supervisor, and pages 1..4 provide data for the connected BCM6123s.
Page 0 does report various telemetry values, which should be supported. Pages
2..4 should be supported as well.
Question is if the driver should be named after the supervisor or after
the voltage converter; I'll leave that up to you. Either case it needs
to support all five pages.
Guenter
After Digging further ourselves, it looks like our hardware also
contains a digital supervisor IC (PLI1209BC), which is connected to one
BCM6123 only. Unfortunately, we were not able to find the data sheet
before:
https://www.vicorpower.com/documents/datasheets/ds-PLI1209BCxyzz-VICOR.pdf
This explains why there are two pages in our case. Since there are
different hardware configurations it would make more sense to name this
driver after the supervisor.
I'll rework this patch set and implement the PLI1209BC properly.
Apparently there are (at least) two different supervisor chips,
PLI1209BC and D44TL1A0. Given that, it is ok if you only implement
support for PLI1209BC is that is what you use, but the driver name
should indeed reflect the supervisor chip to ensure that support
for the other supervisor chip can be added at a later point.
Thanks,
Guenter
Marcello
+ .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+ | PMBUS_HAVE_IIN | PMBUS_HAVE_POUT,
+};
+
+static int bcm6123_probe(struct i2c_client *client)
+{
+ client->dev.platform_data = &bcm6123_plat_data;
+
+ return pmbus_do_probe(client, &bcm6123_info);
+}
+
+static const struct i2c_device_id bcm6123_id[] = {
+ {"bcm6123", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, bcm6123_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id bcm6123_of_match[] = {
+ { .compatible = "vicor,bcm6123" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, bcm6123_of_match);
+#endif
+
+/* This is the driver that will be inserted */
+static struct i2c_driver bcm6123_driver = {
+ .driver = {
+ .name = "bcm6123",
+ .of_match_table = of_match_ptr(bcm6123_of_match),
+ },
+ .probe_new = bcm6123_probe,
+ .id_table = bcm6123_id,
+};
+
+module_i2c_driver(bcm6123_driver);
+
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("PMBus driver for Vicor bcm6123");
+MODULE_LICENSE("GPL");