After some testing, I have found out there is a timing issue with this
device. After setting page, the device doesn't immediately react and
gives values from the previous page for some time. This is why there
needs to be a delay between pmbus_set_page and the actual read.
Also, a lot of the standard commands don't work with the devices, so
they are filtered out in the custom read function.
Signed-off-by: Václav Kubernát <[email protected]>
---
drivers/hwmon/pmbus/Kconfig | 9 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 164 +++++++++++++++++++++++++++++++++++
3 files changed, 174 insertions(+)
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..66d1655b6750 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,15 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..7f3c3de3a1e6
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V 0x00
+#define YM2151_PAGE_5V 0x20
+#define YH5151E_PAGE_12V 0x00
+#define YH5151E_PAGE_5V 0x10
+#define YH5151E_PAGE_3V3 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+static int set_page(struct i2c_client *client, int page)
+{
+ int rv;
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+
+ if (rv < 0)
+ return rv;
+
+ if (rv != page) {
+ rv = pmbus_set_page(client, page, 0xff);
+ if (rv < 0)
+ return rv;
+
+ msleep(20);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ if (reg >= PMBUS_VIRT_BASE)
+ return -ENXIO;
+
+ switch (reg) {
+ case PMBUS_OT_WARN_LIMIT:
+ case PMBUS_OT_FAULT_LIMIT:
+ case PMBUS_UT_WARN_LIMIT:
+ case PMBUS_UT_FAULT_LIMIT:
+ case PMBUS_VIN_UV_WARN_LIMIT:
+ case PMBUS_VIN_UV_FAULT_LIMIT:
+ case PMBUS_VIN_OV_FAULT_LIMIT:
+ case PMBUS_VIN_OV_WARN_LIMIT:
+ case PMBUS_IOUT_OC_WARN_LIMIT:
+ case PMBUS_IOUT_UC_FAULT_LIMIT:
+ case PMBUS_IOUT_OC_FAULT_LIMIT:
+ case PMBUS_IIN_OC_WARN_LIMIT:
+ case PMBUS_IIN_OC_FAULT_LIMIT:
+ case PMBUS_VOUT_UV_WARN_LIMIT:
+ case PMBUS_VOUT_UV_FAULT_LIMIT:
+ case PMBUS_VOUT_OV_WARN_LIMIT:
+ case PMBUS_VOUT_OV_FAULT_LIMIT:
+ case PMBUS_MFR_VIN_MIN:
+ case PMBUS_MFR_VIN_MAX:
+ case PMBUS_MFR_IIN_MAX:
+ case PMBUS_MFR_VOUT_MIN:
+ case PMBUS_MFR_VOUT_MAX:
+ case PMBUS_MFR_IOUT_MAX:
+ case PMBUS_MFR_PIN_MAX:
+ case PMBUS_POUT_MAX:
+ case PMBUS_POUT_OP_WARN_LIMIT:
+ case PMBUS_POUT_OP_FAULT_LIMIT:
+ case PMBUS_MFR_MAX_TEMP_1:
+ case PMBUS_MFR_MAX_TEMP_2:
+ case PMBUS_MFR_MAX_TEMP_3:
+ case PMBUS_MFR_POUT_MAX:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 0x21,
+ .func[YM2151_PAGE_12V] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5V] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 0x12,
+ .func[YH5151E_PAGE_12V] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ return pmbus_do_probe(client, &fsp3y_info[id->driver_data]);
+}
+
+static const struct i2c_device_id pmbus_id[] = {
+ {"fsp3y_ym2151e", ym2151e},
+ {"fsp3y_yh5151e", yh5151e},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, pmbus_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe = fsp3y_probe,
+ .id_table = pmbus_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
On 3/29/21 7:38 AM, Václav Kubernát wrote:
> After some testing, I have found out there is a timing issue with this
> device. After setting page, the device doesn't immediately react and
> gives values from the previous page for some time. This is why there
> needs to be a delay between pmbus_set_page and the actual read.
>
> Also, a lot of the standard commands don't work with the devices, so
> they are filtered out in the custom read function.
>
This is not an appropriate patch description. Describe the driver here,
not the workarounds / quirks. The reason for the delay should be a
comment in the patch, not in the patch description.
Also, "don't work" is inappropriate (and, again, does not belong into
the patch description). It is perfectly appropriate for the core
to try those commands to see if they are supported. The only reason
to mask them out would be that the device reacts badly to seeing
them. If that is the case, "don't work" should be replaced with
a more detailed comment in the code. Describe what happens, and why
the commands needs to be caught.
What might be useful is a note indicating if you have a manual for
those power supplies available, or if the driver is based on reverse
engineering.
> Signed-off-by: Václav Kubernát <[email protected]>
> ---
> drivers/hwmon/pmbus/Kconfig | 9 ++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/fsp-3y.c | 164 +++++++++++++++++++++++++++++++++++
Documentation/hwmon/fsp-3y.rst is missing.
> 3 files changed, 174 insertions(+)
> create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 03606d4298a4..66d1655b6750 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -56,6 +56,15 @@ config SENSORS_BEL_PFE
> This driver can also be built as a module. If so, the module will
> be called bel-pfe.
>
> +config SENSORS_FSP_3Y
> + tristate "FSP/3Y-Power power supplies"
> + help
> + If you say yes here you get hardware monitoring support for
> + FSP/3Y-Power hot-swap power supplies.
> +
This should list the supported models - if not here, then at least in the
(missing) documentation.
> + This driver can also be built as a module. If so, the module will
> + be called fsp-3y.
> +
> config SENSORS_IBM_CFFPS
> tristate "IBM Common Form Factor Power Supply"
> depends on LEDS_CLASS
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..bfe218ad898f 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> new file mode 100644
> index 000000000000..7f3c3de3a1e6
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/fsp-3y.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for FSP 3Y-Power PSUs
> + *
> + * Copyright (c) 2021 Václav Kubernát, CESNET
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
Alphabetic include file order, please.
> +#include "pmbus.h"
> +
> +#define YM2151_PAGE_12V 0x00
> +#define YM2151_PAGE_5V 0x20
> +#define YH5151E_PAGE_12V 0x00
> +#define YH5151E_PAGE_5V 0x10
> +#define YH5151E_PAGE_3V3 0x11
> +
> +enum chips {
> + ym2151e,
> + yh5151e
> +};
> +
> +static int set_page(struct i2c_client *client, int page)
> +{
> + int rv;
> +
> + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> +
Please no empty line here.
You might want to consider caching the current page to avoid having to read it
for each access, similar to the code implemented in the pmbus core.
> + if (rv < 0)
> + return rv;
> +
> + if (rv != page) {
> + rv = pmbus_set_page(client, page, 0xff);
> + if (rv < 0)
> + return rv;
> +
> + msleep(20);
Please use usleep_range(), and make sure that this huge delay is actually needed.
> + }
> +
> + return 0;
> +}
> +
> +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> + int rv;
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> + int rv;
> +
> + if (reg >= PMBUS_VIRT_BASE)
> + return -ENXIO;
> +
> + switch (reg) {
> + case PMBUS_OT_WARN_LIMIT:
> + case PMBUS_OT_FAULT_LIMIT:
> + case PMBUS_UT_WARN_LIMIT:
> + case PMBUS_UT_FAULT_LIMIT:
> + case PMBUS_VIN_UV_WARN_LIMIT:
> + case PMBUS_VIN_UV_FAULT_LIMIT:
> + case PMBUS_VIN_OV_FAULT_LIMIT:
> + case PMBUS_VIN_OV_WARN_LIMIT:
> + case PMBUS_IOUT_OC_WARN_LIMIT:
> + case PMBUS_IOUT_UC_FAULT_LIMIT:
> + case PMBUS_IOUT_OC_FAULT_LIMIT:
> + case PMBUS_IIN_OC_WARN_LIMIT:
> + case PMBUS_IIN_OC_FAULT_LIMIT:
> + case PMBUS_VOUT_UV_WARN_LIMIT:
> + case PMBUS_VOUT_UV_FAULT_LIMIT:
> + case PMBUS_VOUT_OV_WARN_LIMIT:
> + case PMBUS_VOUT_OV_FAULT_LIMIT:
> + case PMBUS_MFR_VIN_MIN:
> + case PMBUS_MFR_VIN_MAX:
> + case PMBUS_MFR_IIN_MAX:
> + case PMBUS_MFR_VOUT_MIN:
> + case PMBUS_MFR_VOUT_MAX:
> + case PMBUS_MFR_IOUT_MAX:
> + case PMBUS_MFR_PIN_MAX:
> + case PMBUS_POUT_MAX:
> + case PMBUS_POUT_OP_WARN_LIMIT:
> + case PMBUS_POUT_OP_FAULT_LIMIT:
> + case PMBUS_MFR_MAX_TEMP_1:
> + case PMBUS_MFR_MAX_TEMP_2:
> + case PMBUS_MFR_MAX_TEMP_3:
> + case PMBUS_MFR_POUT_MAX:
> + return -ENXIO;
> + }
If that many commands indeed cause trouble (ie cause the device
to get into a bad state), it might be better to list the _supported_
commands instead of the unsupported ones. There is no guarantee
that the core won't start to send other commands to the device
in the future.
The underlying question is if those commands are indeed not supported,
or if they report values in an unexpected format (ie not linear11).
The data format that is auto-selected below (because it is not specified)
is "linear". Is this what the device actually uses ? If not, just disabling
reading the limits without explanation what exactly "does not work" is
inappropriate.
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_word_data(client, reg);
> +}
> +
> +struct pmbus_driver_info fsp3y_info[] = {
> + [ym2151e] = {
> + .pages = 0x21,
> + .func[YM2151_PAGE_12V] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> + PMBUS_HAVE_FAN12,
> + .func[YM2151_PAGE_5V] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> + PMBUS_HAVE_IIN,
It doesn't really make sense to claim support for 0x21 = 33
pages, especially since the pmbus core (and the pmbus standard)
only supports 32 pages. Since page handling is all local anyway,
I would suggest to claim two pages and map the logical page
to the physical page in the set_page command.
How does this work (compile) anyway ? .func[] array size
is 32, meaning .func[0x20] goes beyond the end of the array.
The compiler should complain about that.
Wait, how does this even instantiate ? The PMBus core
should reject a page count larger than 32, and pmbus_do_probe()
should return -ENODEV. How did you test this code ?
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + },
> + [yh5151e] = {
> + .pages = 0x12,
Same as above.
> + .func[YH5151E_PAGE_12V] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> + .func[YH5151E_PAGE_5V] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .func[YH5151E_PAGE_3V3] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + }
> +};
> +
> +static int fsp3y_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
This vendor sells dozens of different power supplies. Apparently
they do not have compatible PMBus attributes (or at least the pages
are not compatible to each other). Given that, I think there should
be a model validation here.
This is even more important since an earlier discussion suggests that
at least some of the 3Y power supplies use LINEAR11 instead of LINEAR16
for output voltages (eg YH5301-1EAR, FSP550-50ERS). We need to ensure
that affected power supplies are not enabled with this driver, and that
the enabled power supplies have been tested and are not only confirmed
to work and report correct data.
> + return pmbus_do_probe(client, &fsp3y_info[id->driver_data]);
> +}
> +
> +static const struct i2c_device_id pmbus_id[] = {
> + {"fsp3y_ym2151e", ym2151e},
> + {"fsp3y_yh5151e", yh5151e},> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver fsp3y_driver = {
> + .driver = {
> + .name = "fsp3y",
> + },
> + .probe = fsp3y_probe,
Please use the .probe_new callback.
> + .id_table = pmbus_id
> +};
> +
> +module_i2c_driver(fsp3y_driver);
> +
> +MODULE_AUTHOR("Václav Kubernát");
> +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> +MODULE_LICENSE("GPL");
>
Hi "V?clav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.12-rc5 next-20210329]
[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]
url: https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-Add-driver-for-fsp-3y-PSUs-and-PDUs/20210329-224216
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/48af68da782edb21e107a884db98beedfd691e81
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review V-clav-Kubern-t/hwmon-Add-driver-for-fsp-3y-PSUs-and-PDUs/20210329-224216
git checkout 48af68da782edb21e107a884db98beedfd691e81
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/hwmon/pmbus/fsp-3y.c:15:25: error: array index in initializer exceeds array bounds
15 | #define YM2151_PAGE_5V 0x20
| ^~~~
drivers/hwmon/pmbus/fsp-3y.c:114:9: note: in expansion of macro 'YM2151_PAGE_5V'
114 | .func[YM2151_PAGE_5V] =
| ^~~~~~~~~~~~~~
drivers/hwmon/pmbus/fsp-3y.c:15:25: note: (near initialization for 'fsp3y_info[0].func')
15 | #define YM2151_PAGE_5V 0x20
| ^~~~
drivers/hwmon/pmbus/fsp-3y.c:114:9: note: in expansion of macro 'YM2151_PAGE_5V'
114 | .func[YM2151_PAGE_5V] =
| ^~~~~~~~~~~~~~
In file included from include/linux/bits.h:6,
from include/linux/bitops.h:6,
from include/linux/kernel.h:11,
from drivers/hwmon/pmbus/fsp-3y.c:8:
>> include/vdso/bits.h:7:19: warning: initialized field overwritten [-Woverride-init]
7 | #define BIT(nr) (UL(1) << (nr))
| ^
drivers/hwmon/pmbus/pmbus.h:383:26: note: in expansion of macro 'BIT'
383 | #define PMBUS_HAVE_VOUT BIT(2)
| ^~~
drivers/hwmon/pmbus/fsp-3y.c:115:4: note: in expansion of macro 'PMBUS_HAVE_VOUT'
115 | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
| ^~~~~~~~~~~~~~~
include/vdso/bits.h:7:19: note: (near initialization for 'fsp3y_info[0].func[0]')
7 | #define BIT(nr) (UL(1) << (nr))
| ^
drivers/hwmon/pmbus/pmbus.h:383:26: note: in expansion of macro 'BIT'
383 | #define PMBUS_HAVE_VOUT BIT(2)
| ^~~
drivers/hwmon/pmbus/fsp-3y.c:115:4: note: in expansion of macro 'PMBUS_HAVE_VOUT'
115 | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
| ^~~~~~~~~~~~~~~
vim +7 include/vdso/bits.h
3945ff37d2f48d Vincenzo Frascino 2020-03-20 6
3945ff37d2f48d Vincenzo Frascino 2020-03-20 @7 #define BIT(nr) (UL(1) << (nr))
3945ff37d2f48d Vincenzo Frascino 2020-03-20 8
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi "V?clav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v5.12-rc5 next-20210329]
[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]
url: https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-Add-driver-for-fsp-3y-PSUs-and-PDUs/20210329-224216
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/48af68da782edb21e107a884db98beedfd691e81
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review V-clav-Kubern-t/hwmon-Add-driver-for-fsp-3y-PSUs-and-PDUs/20210329-224216
git checkout 48af68da782edb21e107a884db98beedfd691e81
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/hwmon/pmbus/fsp-3y.c:15:25: error: array index in initializer exceeds array bounds
15 | #define YM2151_PAGE_5V 0x20
| ^~~~
drivers/hwmon/pmbus/fsp-3y.c:114:9: note: in expansion of macro 'YM2151_PAGE_5V'
114 | .func[YM2151_PAGE_5V] =
| ^~~~~~~~~~~~~~
drivers/hwmon/pmbus/fsp-3y.c:15:25: note: (near initialization for 'fsp3y_info[0].func')
15 | #define YM2151_PAGE_5V 0x20
| ^~~~
drivers/hwmon/pmbus/fsp-3y.c:114:9: note: in expansion of macro 'YM2151_PAGE_5V'
114 | .func[YM2151_PAGE_5V] =
| ^~~~~~~~~~~~~~
In file included from include/linux/bits.h:6,
from include/linux/bitops.h:6,
from include/linux/kernel.h:11,
from drivers/hwmon/pmbus/fsp-3y.c:8:
include/vdso/bits.h:7:19: warning: initialized field overwritten [-Woverride-init]
7 | #define BIT(nr) (UL(1) << (nr))
| ^
drivers/hwmon/pmbus/pmbus.h:383:26: note: in expansion of macro 'BIT'
383 | #define PMBUS_HAVE_VOUT BIT(2)
| ^~~
drivers/hwmon/pmbus/fsp-3y.c:115:4: note: in expansion of macro 'PMBUS_HAVE_VOUT'
115 | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
| ^~~~~~~~~~~~~~~
include/vdso/bits.h:7:19: note: (near initialization for 'fsp3y_info[0].func[0]')
7 | #define BIT(nr) (UL(1) << (nr))
| ^
drivers/hwmon/pmbus/pmbus.h:383:26: note: in expansion of macro 'BIT'
383 | #define PMBUS_HAVE_VOUT BIT(2)
| ^~~
drivers/hwmon/pmbus/fsp-3y.c:115:4: note: in expansion of macro 'PMBUS_HAVE_VOUT'
115 | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
| ^~~~~~~~~~~~~~~
vim +15 drivers/hwmon/pmbus/fsp-3y.c
13
14 #define YM2151_PAGE_12V 0x00
> 15 #define YM2151_PAGE_5V 0x20
16 #define YH5151E_PAGE_12V 0x00
17 #define YH5151E_PAGE_5V 0x10
18 #define YH5151E_PAGE_3V3 0x11
19
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Guenter,
Thank you for the review.
po 29. 3. 2021 v 19:47 odesílatel Guenter Roeck <[email protected]> napsal:
>
> On 3/29/21 7:38 AM, Václav Kubernát wrote:
> > After some testing, I have found out there is a timing issue with this
> > device. After setting page, the device doesn't immediately react and
> > gives values from the previous page for some time. This is why there
> > needs to be a delay between pmbus_set_page and the actual read.
> >
> > Also, a lot of the standard commands don't work with the devices, so
> > they are filtered out in the custom read function.
> >
>
> This is not an appropriate patch description. Describe the driver here,
> not the workarounds / quirks. The reason for the delay should be a
> comment in the patch, not in the patch description.
>
> Also, "don't work" is inappropriate (and, again, does not belong into
> the patch description). It is perfectly appropriate for the core
> to try those commands to see if they are supported. The only reason
> to mask them out would be that the device reacts badly to seeing
> them. If that is the case, "don't work" should be replaced with
> a more detailed comment in the code. Describe what happens, and why
> the commands needs to be caught.
>
>
> What might be useful is a note indicating if you have a manual for
> those power supplies available, or if the driver is based on reverse
> engineering.
>
I will rework the commit message in a V2 patch.
> > Signed-off-by: Václav Kubernát <[email protected]>
> > ---
> > drivers/hwmon/pmbus/Kconfig | 9 ++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/fsp-3y.c | 164 +++++++++++++++++++++++++++++++++++
>
> Documentation/hwmon/fsp-3y.rst is missing.
>
> > 3 files changed, 174 insertions(+)
> > create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
> >
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 03606d4298a4..66d1655b6750 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -56,6 +56,15 @@ config SENSORS_BEL_PFE
> > This driver can also be built as a module. If so, the module will
> > be called bel-pfe.
> >
> > +config SENSORS_FSP_3Y
> > + tristate "FSP/3Y-Power power supplies"
> > + help
> > + If you say yes here you get hardware monitoring support for
> > + FSP/3Y-Power hot-swap power supplies.
> > +
>
> This should list the supported models - if not here, then at least in the
> (missing) documentation.
>
Okay.
> > + This driver can also be built as a module. If so, the module will
> > + be called fsp-3y.
> > +
> > config SENSORS_IBM_CFFPS
> > tristate "IBM Common Form Factor Power Supply"
> > depends on LEDS_CLASS
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index 6a4ba0fdc1db..bfe218ad898f 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> > obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> > obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> > +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> > obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> > obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> > diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> > new file mode 100644
> > index 000000000000..7f3c3de3a1e6
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/fsp-3y.c
> > @@ -0,0 +1,164 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Hardware monitoring driver for FSP 3Y-Power PSUs
> > + *
> > + * Copyright (c) 2021 Václav Kubernát, CESNET
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
>
> Alphabetic include file order, please.
>
> > +#include "pmbus.h"
> > +
> > +#define YM2151_PAGE_12V 0x00
> > +#define YM2151_PAGE_5V 0x20
> > +#define YH5151E_PAGE_12V 0x00
> > +#define YH5151E_PAGE_5V 0x10
> > +#define YH5151E_PAGE_3V3 0x11
> > +
> > +enum chips {
> > + ym2151e,
> > + yh5151e
> > +};
> > +
> > +static int set_page(struct i2c_client *client, int page)
> > +{
> > + int rv;
> > +
> > + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> > +
> Please no empty line here.
>
> You might want to consider caching the current page to avoid having to read it
> for each access, similar to the code implemented in the pmbus core.
>
This was actually what I wanted to do at first, but I wasn't able to
get i2c_set_clientdata to work. Later I found out that it is because
pmbus_do_probe uses sets it own data. What do you think I should use
to cache the page?
> > + if (rv < 0)
> > + return rv;
> > +
> > + if (rv != page) {
> > + rv = pmbus_set_page(client, page, 0xff);
> > + if (rv < 0)
> > + return rv;
> > +
> > + msleep(20);
>
> Please use usleep_range(), and make sure that this huge delay is actually needed.
>
As is written in the original commit message, the devices have some
kind of timing issues and this delay really is needed. I have tested
smaller delays (10ms), they are compared to no delay, but I would
still sometimes get wrong values. I will move this explanation into
the code.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> > +{
> > + int rv;
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> > +{
> > + int rv;
> > +
> > + if (reg >= PMBUS_VIRT_BASE)
> > + return -ENXIO;
> > +
> > + switch (reg) {
> > + case PMBUS_OT_WARN_LIMIT:
> > + case PMBUS_OT_FAULT_LIMIT:
> > + case PMBUS_UT_WARN_LIMIT:
> > + case PMBUS_UT_FAULT_LIMIT:
> > + case PMBUS_VIN_UV_WARN_LIMIT:
> > + case PMBUS_VIN_UV_FAULT_LIMIT:
> > + case PMBUS_VIN_OV_FAULT_LIMIT:
> > + case PMBUS_VIN_OV_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_WARN_LIMIT:
> > + case PMBUS_IOUT_UC_FAULT_LIMIT:
> > + case PMBUS_IOUT_OC_FAULT_LIMIT:
> > + case PMBUS_IIN_OC_WARN_LIMIT:
> > + case PMBUS_IIN_OC_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_WARN_LIMIT:
> > + case PMBUS_VOUT_UV_FAULT_LIMIT:
> > + case PMBUS_VOUT_OV_WARN_LIMIT:
> > + case PMBUS_VOUT_OV_FAULT_LIMIT:
> > + case PMBUS_MFR_VIN_MIN:
> > + case PMBUS_MFR_VIN_MAX:
> > + case PMBUS_MFR_IIN_MAX:
> > + case PMBUS_MFR_VOUT_MIN:
> > + case PMBUS_MFR_VOUT_MAX:
> > + case PMBUS_MFR_IOUT_MAX:
> > + case PMBUS_MFR_PIN_MAX:
> > + case PMBUS_POUT_MAX:
> > + case PMBUS_POUT_OP_WARN_LIMIT:
> > + case PMBUS_POUT_OP_FAULT_LIMIT:
> > + case PMBUS_MFR_MAX_TEMP_1:
> > + case PMBUS_MFR_MAX_TEMP_2:
> > + case PMBUS_MFR_MAX_TEMP_3:
> > + case PMBUS_MFR_POUT_MAX:
> > + return -ENXIO;
> > + }
>
> If that many commands indeed cause trouble (ie cause the device
> to get into a bad state), it might be better to list the _supported_
> commands instead of the unsupported ones. There is no guarantee
> that the core won't start to send other commands to the device
> in the future.
>
> The underlying question is if those commands are indeed not supported,
> or if they report values in an unexpected format (ie not linear11).
> The data format that is auto-selected below (because it is not specified)
> is "linear". Is this what the device actually uses ? If not, just disabling
> reading the limits without explanation what exactly "does not work" is
> inappropriate.
>
The reason I masked these commands is because when I was reading from
the associated files, I would get weird values (like -500). But it's
not like the commands confuse the device. If you think it isn't a good
idea to mask them like that, I'm fine with removing the masking.
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_word_data(client, reg);
> > +}
> > +
> > +struct pmbus_driver_info fsp3y_info[] = {
> > + [ym2151e] = {
> > + .pages = 0x21,
> > + .func[YM2151_PAGE_12V] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> > + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> > + PMBUS_HAVE_FAN12,
> > + .func[YM2151_PAGE_5V] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> > + PMBUS_HAVE_IIN,
>
> It doesn't really make sense to claim support for 0x21 = 33
> pages, especially since the pmbus core (and the pmbus standard)
> only supports 32 pages. Since page handling is all local anyway,
> I would suggest to claim two pages and map the logical page
> to the physical page in the set_page command.
>
> How does this work (compile) anyway ? .func[] array size
> is 32, meaning .func[0x20] goes beyond the end of the array.
> The compiler should complain about that.
>
> Wait, how does this even instantiate ? The PMBus core
> should reject a page count larger than 32, and pmbus_do_probe()
> should return -ENODEV. How did you test this code ?
>
Sorry, I forgot I was building this patch on top of another patch
(written by my colleague), which increases the page limit. The pmbus
specification does say that pages 0x00 through 0x1F are "reserved
specifically for multiple outputs on a device with a single physical
address", but it does not say that there is a page limit. Anyway, The
PSU really does use the 0x20 page. Either way, I'm fine with doing a
mapping between a logical a page and physical, if you decide you don't
want to change the page limit.
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + },
> > + [yh5151e] = {
> > + .pages = 0x12,
>
> Same as above.
>
> > + .func[YH5151E_PAGE_12V] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> > + .func[YH5151E_PAGE_5V] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .func[YH5151E_PAGE_3V3] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + }
> > +};
> > +
> > +static int fsp3y_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
>
> This vendor sells dozens of different power supplies. Apparently
> they do not have compatible PMBus attributes (or at least the pages
> are not compatible to each other). Given that, I think there should
> be a model validation here.
How should I go about doing model validation? I'm already using
i2c_device_id to differentiate between the PDU and the PSU, but I
suppose, that's not the best thing. Maybe I should use an identify
function in pmbus_driver_info?
>
> This is even more important since an earlier discussion suggests that
> at least some of the 3Y power supplies use LINEAR11 instead of LINEAR16
> for output voltages (eg YH5301-1EAR, FSP550-50ERS). We need to ensure
> that affected power supplies are not enabled with this driver, and that
> the enabled power supplies have been tested and are not only confirmed
> to work and report correct data.
>
> > + return pmbus_do_probe(client, &fsp3y_info[id->driver_data]);
> > +}
> > +
> > +static const struct i2c_device_id pmbus_id[] = {
> > + {"fsp3y_ym2151e", ym2151e},
> > + {"fsp3y_yh5151e", yh5151e},> + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> > +
> > +/* This is the driver that will be inserted */
> > +static struct i2c_driver fsp3y_driver = {
> > + .driver = {
> > + .name = "fsp3y",
> > + },
> > + .probe = fsp3y_probe,
>
> Please use the .probe_new callback.
>
> > + .id_table = pmbus_id
> > +};
> > +
> > +module_i2c_driver(fsp3y_driver);
> > +
> > +MODULE_AUTHOR("Václav Kubernát");
> > +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> > +MODULE_LICENSE("GPL");
> >
>
Václav
On 3/29/21 8:31 PM, Václav Kubernát wrote:
> Hi Guenter,
>
> Thank you for the review.
>
> po 29. 3. 2021 v 19:47 odesílatel Guenter Roeck <[email protected]> napsal:
>>
>> On 3/29/21 7:38 AM, Václav Kubernát wrote:
>>> After some testing, I have found out there is a timing issue with this
>>> device. After setting page, the device doesn't immediately react and
>>> gives values from the previous page for some time. This is why there
>>> needs to be a delay between pmbus_set_page and the actual read.
>>>
>>> Also, a lot of the standard commands don't work with the devices, so
>>> they are filtered out in the custom read function.
>>>
>>
>> This is not an appropriate patch description. Describe the driver here,
>> not the workarounds / quirks. The reason for the delay should be a
>> comment in the patch, not in the patch description.
>>
>> Also, "don't work" is inappropriate (and, again, does not belong into
>> the patch description). It is perfectly appropriate for the core
>> to try those commands to see if they are supported. The only reason
>> to mask them out would be that the device reacts badly to seeing
>> them. If that is the case, "don't work" should be replaced with
>> a more detailed comment in the code. Describe what happens, and why
>> the commands needs to be caught.
>>
>>
>> What might be useful is a note indicating if you have a manual for
>> those power supplies available, or if the driver is based on reverse
>> engineering.
>>
>
> I will rework the commit message in a V2 patch.
>
>>> Signed-off-by: Václav Kubernát <[email protected]>
>>> ---
>>> drivers/hwmon/pmbus/Kconfig | 9 ++
>>> drivers/hwmon/pmbus/Makefile | 1 +
>>> drivers/hwmon/pmbus/fsp-3y.c | 164 +++++++++++++++++++++++++++++++++++
>>
>> Documentation/hwmon/fsp-3y.rst is missing.
>>
>>> 3 files changed, 174 insertions(+)
>>> create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
>>>
>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>> index 03606d4298a4..66d1655b6750 100644
>>> --- a/drivers/hwmon/pmbus/Kconfig
>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>> @@ -56,6 +56,15 @@ config SENSORS_BEL_PFE
>>> This driver can also be built as a module. If so, the module will
>>> be called bel-pfe.
>>>
>>> +config SENSORS_FSP_3Y
>>> + tristate "FSP/3Y-Power power supplies"
>>> + help
>>> + If you say yes here you get hardware monitoring support for
>>> + FSP/3Y-Power hot-swap power supplies.
>>> +
>>
>> This should list the supported models - if not here, then at least in the
>> (missing) documentation.
>>
>
> Okay.
>
>>> + This driver can also be built as a module. If so, the module will
>>> + be called fsp-3y.
>>> +
>>> config SENSORS_IBM_CFFPS
>>> tristate "IBM Common Form Factor Power Supply"
>>> depends on LEDS_CLASS
>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
>>> index 6a4ba0fdc1db..bfe218ad898f 100644
>>> --- a/drivers/hwmon/pmbus/Makefile
>>> +++ b/drivers/hwmon/pmbus/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
>>> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
>>> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
>>> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
>>> +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
>>> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
>>> diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
>>> new file mode 100644
>>> index 000000000000..7f3c3de3a1e6
>>> --- /dev/null
>>> +++ b/drivers/hwmon/pmbus/fsp-3y.c
>>> @@ -0,0 +1,164 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Hardware monitoring driver for FSP 3Y-Power PSUs
>>> + *
>>> + * Copyright (c) 2021 Václav Kubernát, CESNET
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>
>> Alphabetic include file order, please.
>>
>>> +#include "pmbus.h"
>>> +
>>> +#define YM2151_PAGE_12V 0x00
>>> +#define YM2151_PAGE_5V 0x20
>>> +#define YH5151E_PAGE_12V 0x00
>>> +#define YH5151E_PAGE_5V 0x10
>>> +#define YH5151E_PAGE_3V3 0x11
>>> +
>>> +enum chips {
>>> + ym2151e,
>>> + yh5151e
>>> +};
>>> +
>>> +static int set_page(struct i2c_client *client, int page)
>>> +{
>>> + int rv;
>>> +
>>> + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>>> +
>> Please no empty line here.
>>
>> You might want to consider caching the current page to avoid having to read it
>> for each access, similar to the code implemented in the pmbus core.
>>
>
> This was actually what I wanted to do at first, but I wasn't able to
> get i2c_set_clientdata to work. Later I found out that it is because
> pmbus_do_probe uses sets it own data. What do you think I should use
> to cache the page?
>
Several other PMBus drivers use local data. The trick is to put
the pmbus_driver_info data structure into that local data, then use
container_of() to access it.
>>> + if (rv < 0)
>>> + return rv;
>>> +
>>> + if (rv != page) {
>>> + rv = pmbus_set_page(client, page, 0xff);
>>> + if (rv < 0)
>>> + return rv;
>>> +
>>> + msleep(20);
>>
>> Please use usleep_range(), and make sure that this huge delay is actually needed.
>>
>
> As is written in the original commit message, the devices have some
> kind of timing issues and this delay really is needed. I have tested
> smaller delays (10ms), they are compared to no delay, but I would
> still sometimes get wrong values. I will move this explanation into
> the code.
> Ok.
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
>>> +{
>>> + int rv;
>>> +
>>> + rv = set_page(client, page);
>>> + if (rv < 0)
>>> + return rv;
>>> +
>>> + return i2c_smbus_read_byte_data(client, reg);
>>> +}
>>> +
>>> +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
>>> +{
>>> + int rv;
>>> +
>>> + if (reg >= PMBUS_VIRT_BASE)
>>> + return -ENXIO;
>>> +
>>> + switch (reg) {
>>> + case PMBUS_OT_WARN_LIMIT:
>>> + case PMBUS_OT_FAULT_LIMIT:
>>> + case PMBUS_UT_WARN_LIMIT:
>>> + case PMBUS_UT_FAULT_LIMIT:
>>> + case PMBUS_VIN_UV_WARN_LIMIT:
>>> + case PMBUS_VIN_UV_FAULT_LIMIT:
>>> + case PMBUS_VIN_OV_FAULT_LIMIT:
>>> + case PMBUS_VIN_OV_WARN_LIMIT:
>>> + case PMBUS_IOUT_OC_WARN_LIMIT:
>>> + case PMBUS_IOUT_UC_FAULT_LIMIT:
>>> + case PMBUS_IOUT_OC_FAULT_LIMIT:
>>> + case PMBUS_IIN_OC_WARN_LIMIT:
>>> + case PMBUS_IIN_OC_FAULT_LIMIT:
>>> + case PMBUS_VOUT_UV_WARN_LIMIT:
>>> + case PMBUS_VOUT_UV_FAULT_LIMIT:
>>> + case PMBUS_VOUT_OV_WARN_LIMIT:
>>> + case PMBUS_VOUT_OV_FAULT_LIMIT:
>>> + case PMBUS_MFR_VIN_MIN:
>>> + case PMBUS_MFR_VIN_MAX:
>>> + case PMBUS_MFR_IIN_MAX:
>>> + case PMBUS_MFR_VOUT_MIN:
>>> + case PMBUS_MFR_VOUT_MAX:
>>> + case PMBUS_MFR_IOUT_MAX:
>>> + case PMBUS_MFR_PIN_MAX:
>>> + case PMBUS_POUT_MAX:
>>> + case PMBUS_POUT_OP_WARN_LIMIT:
>>> + case PMBUS_POUT_OP_FAULT_LIMIT:
>>> + case PMBUS_MFR_MAX_TEMP_1:
>>> + case PMBUS_MFR_MAX_TEMP_2:
>>> + case PMBUS_MFR_MAX_TEMP_3:
>>> + case PMBUS_MFR_POUT_MAX:
>>> + return -ENXIO;
>>> + }
>>
>> If that many commands indeed cause trouble (ie cause the device
>> to get into a bad state), it might be better to list the _supported_
>> commands instead of the unsupported ones. There is no guarantee
>> that the core won't start to send other commands to the device
>> in the future.
>>
>> The underlying question is if those commands are indeed not supported,
>> or if they report values in an unexpected format (ie not linear11).
>> The data format that is auto-selected below (because it is not specified)
>> is "linear". Is this what the device actually uses ? If not, just disabling
>> reading the limits without explanation what exactly "does not work" is
>> inappropriate.
>>
>
> The reason I masked these commands is because when I was reading from
> the associated files, I would get weird values (like -500). But it's
> not like the commands confuse the device. If you think it isn't a good
> idea to mask them like that, I'm fine with removing the masking.
>
The problem is that the power supplies do support those commands. The question
is what data format they use. Given that we know which pages map which voltage,
it should be possible to figure that out based on the raw data reported on
the multiple pages. Also, another question is if all those commands
are affected. If it is PMBUS_VOUT_xxx, it is probably because they are
(wrongly) reported in LINEAR 11 instead of LINEAR16 format. It may as well be
that other limits are reported as LINEAR16 when it should be LINEAR11.
All that is difficult to determine without seeing the raw data and without
datasheet.
>>> +
>>> + rv = set_page(client, page);
>>> + if (rv < 0)
>>> + return rv;
>>> +
>>> + return i2c_smbus_read_word_data(client, reg);
>>> +}
>>> +
>>> +struct pmbus_driver_info fsp3y_info[] = {
>>> + [ym2151e] = {
>>> + .pages = 0x21,
>>> + .func[YM2151_PAGE_12V] =
>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
>>> + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
>>> + PMBUS_HAVE_FAN12,
>>> + .func[YM2151_PAGE_5V] =
>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
>>> + PMBUS_HAVE_IIN,
>>
>> It doesn't really make sense to claim support for 0x21 = 33
>> pages, especially since the pmbus core (and the pmbus standard)
>> only supports 32 pages. Since page handling is all local anyway,
>> I would suggest to claim two pages and map the logical page
>> to the physical page in the set_page command.
>>
>> How does this work (compile) anyway ? .func[] array size
>> is 32, meaning .func[0x20] goes beyond the end of the array.
>> The compiler should complain about that.
>>
>> Wait, how does this even instantiate ? The PMBus core
>> should reject a page count larger than 32, and pmbus_do_probe()
>> should return -ENODEV. How did you test this code ?
>>
>
> Sorry, I forgot I was building this patch on top of another patch
> (written by my colleague), which increases the page limit. The pmbus
> specification does say that pages 0x00 through 0x1F are "reserved
> specifically for multiple outputs on a device with a single physical
> address", but it does not say that there is a page limit. Anyway, The
Ah yes, sorry, too long ago.
> PSU really does use the 0x20 page. Either way, I'm fine with doing a
> mapping between a logical a page and physical, if you decide you don't
> want to change the page limit.
>
Unless there is a good reason to do so (ie a device with more than 32
legitimate pages) I do not really want to increase that limit since that
would affect all PMBus drivers.
>>> + .read_word_data = fsp3y_read_word_data,
>>> + .read_byte_data = fsp3y_read_byte_data,
>>> + },
>>> + [yh5151e] = {
>>> + .pages = 0x12,
>>
>> Same as above.
>>
>>> + .func[YH5151E_PAGE_12V] =
>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> + PMBUS_HAVE_POUT |
>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
>>> + .func[YH5151E_PAGE_5V] =
>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> + PMBUS_HAVE_POUT,
>>> + .func[YH5151E_PAGE_3V3] =
>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> + PMBUS_HAVE_POUT,
Assuming this is really YH5151-1EBR, doesn't it also support
-12V (possibly on page 0x22) and +5Vsb ?
>>> + .read_word_data = fsp3y_read_word_data,
>>> + .read_byte_data = fsp3y_read_byte_data,
>>> + }
>>> +};
>>> +
>>> +static int fsp3y_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>
>> This vendor sells dozens of different power supplies. Apparently
>> they do not have compatible PMBus attributes (or at least the pages
>> are not compatible to each other). Given that, I think there should
>> be a model validation here.
>
> How should I go about doing model validation? I'm already using
> i2c_device_id to differentiate between the PDU and the PSU, but I
> suppose, that's not the best thing. Maybe I should use an identify
> function in pmbus_driver_info?
>
By reading PMBUS_MFR_ID ? PMBUS_MFR_MODEL ? PMBUS_IC_DEVICE_ID ?
I don't have the manual, and manuals from this manufacturer are not
public, so I have no idea what those power supplies report.
>>
>> This is even more important since an earlier discussion suggests that
>> at least some of the 3Y power supplies use LINEAR11 instead of LINEAR16
>> for output voltages (eg YH5301-1EAR, FSP550-50ERS). We need to ensure
>> that affected power supplies are not enabled with this driver, and that
>> the enabled power supplies have been tested and are not only confirmed
>> to work and report correct data.
>>
>
>
>>> + return pmbus_do_probe(client, &fsp3y_info[id->driver_data]);
>>> +}
>>> +
>>> +static const struct i2c_device_id pmbus_id[] = {
>>> + {"fsp3y_ym2151e", ym2151e},
>>> + {"fsp3y_yh5151e", yh5151e},> + {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
>>> +
>>> +/* This is the driver that will be inserted */
>>> +static struct i2c_driver fsp3y_driver = {
>>> + .driver = {
>>> + .name = "fsp3y",
>>> + },
>>> + .probe = fsp3y_probe,
>>
>> Please use the .probe_new callback.
>>
>>> + .id_table = pmbus_id
>>> +};
>>> +
>>> +module_i2c_driver(fsp3y_driver);
>>> +
>>> +MODULE_AUTHOR("Václav Kubernát");
>>> +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>
> Václav
>
This patch adds support for these devices:
- YH-5151E - the PDU
- YM-2151E - the PSU
The device datasheet says that the devices support PMBus 1.2, but in my
testing, a lot of the commands aren't supported and if they are, they
sometimes behave strangely or inconsistently. For example, writes to the
PAGE command requires using PEC, otherwise the write won't work and the
page won't switch, even though, the standard says that PEC is opiotnal.
On the other hand, writes the SMBALERT don't require PEC. Because of
this, the driver is mostly reverse engineered with the help of a tool
called pmbus_peek written by David Brownell (and later adopted by my
colleague Jan Kundrát).
The device also has some sort of a timing issue when switching pages,
which is explained further in the code.
Because of this, the driver support is limited. It exposes only the
values, that have been tested to work correctly.
Signed-off-by: Václav Kubernát <[email protected]>
---
Documentation/hwmon/fsp-3y.rst | 26 ++++
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 217 +++++++++++++++++++++++++++++++++
4 files changed, 254 insertions(+)
create mode 100644 Documentation/hwmon/fsp-3y.rst
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
new file mode 100644
index 000000000000..68a547021846
--- /dev/null
+++ b/Documentation/hwmon/fsp-3y.rst
@@ -0,0 +1,26 @@
+Kernel driver fsp3y
+======================
+Supported devices:
+ * 3Y POWER YH-5151E
+ * 3Y POWER YM-2151E
+
+Author: Václav Kubernát <[email protected]>
+
+Description
+-----------
+This driver implements limited support for two 3Y POWER devices.
+
+Sysfs entries
+-------------
+in1_input input voltage
+in2_input 12V output voltage
+in3_input 5V output voltage
+curr1_input input current
+curr2_input 12V output current
+curr3_input 5V output current
+fan1_input fan rpm
+temp1_input temperature 1
+temp2_input temperature 2
+temp3_input temperature 3
+power1_input input power
+power2_input output power
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..9d12d446396c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+ Supported models: YH-5151E, YM-2151E
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..2c165e034fa8
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V_LOG 0x00
+#define YM2151_PAGE_12V_REAL 0x00
+#define YM2151_PAGE_5VSB_LOG 0x01
+#define YM2151_PAGE_5VSB_REAL 0x20
+#define YH5151E_PAGE_12V_LOG 0x00
+#define YH5151E_PAGE_12V_REAL 0x00
+#define YH5151E_PAGE_5V_LOG 0x01
+#define YH5151E_PAGE_5V_REAL 0x10
+#define YH5151E_PAGE_3V3_LOG 0x02
+#define YH5151E_PAGE_3V3_REAL 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+struct fsp3y_data {
+ struct pmbus_driver_info info;
+ enum chips chip;
+ int page;
+};
+
+#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
+
+static int page_log_to_page_real(int page_log, enum chips chip)
+{
+ switch (chip) {
+ case ym2151e:
+ switch (page_log) {
+ case YM2151_PAGE_12V_LOG:
+ return YM2151_PAGE_12V_REAL;
+ case YM2151_PAGE_5VSB_LOG:
+ return YM2151_PAGE_5VSB_REAL;
+ }
+ return -1;
+ case yh5151e:
+ switch (page_log) {
+ case YH5151E_PAGE_12V_LOG:
+ return YH5151E_PAGE_12V_REAL;
+ case YH5151E_PAGE_5V_LOG:
+ return YH5151E_PAGE_5V_LOG;
+ case YH5151E_PAGE_3V3_LOG:
+ return YH5151E_PAGE_3V3_REAL;
+ }
+ return -1;
+ }
+
+ return -1;
+}
+
+static int set_page(struct i2c_client *client, int page_log)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct fsp3y_data *data = to_fsp3y_data(info);
+ int rv;
+ int page_real = page_log_to_page_real(page_log, data->chip);
+
+ if (page_log < 0)
+ return 0;
+
+ if (data->page != page_real) {
+ rv = pmbus_set_page(client, page_real, 0xff);
+ if (rv < 0)
+ return rv;
+
+ data->page = page_real;
+
+ /* Testing showed that the device has a timing issue. After
+ * setting a page, it takes a while, before the device actually
+ * gives the correct values from the correct page. 20 ms was
+ * tested to be enough to not give wrong values (15 ms wasn't
+ * enough)
+ */
+ usleep_range(20000, 30000);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ switch (reg) {
+ case PMBUS_READ_FAN_SPEED_1:
+ case PMBUS_READ_IIN:
+ case PMBUS_READ_IOUT:
+ case PMBUS_READ_PIN:
+ case PMBUS_READ_POUT:
+ case PMBUS_READ_TEMPERATURE_1:
+ case PMBUS_READ_TEMPERATURE_2:
+ case PMBUS_READ_TEMPERATURE_3:
+ case PMBUS_READ_VIN:
+ case PMBUS_READ_VOUT:
+ case PMBUS_STATUS_WORD:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 2,
+ .func[YM2151_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5VSB_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 3,
+ .func[YH5151E_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_probe(struct i2c_client *client)
+{
+ struct fsp3y_data *data;
+ int rv;
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+
+ data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ if (rv < 0)
+ return rv;
+ data->page = rv;
+
+ rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+ if (rv < 0)
+ return rv;
+ if (rv != 8)
+ return -ENODEV;
+
+ if (!strncmp(buf, "YM-2151E", strlen("YM-2151E")))
+ data->chip = ym2151e;
+ else if (!strncmp(buf, "YH-5151E", strlen("YH-5151E")))
+ data->chip = yh5151e;
+ else
+ return -ENODEV;
+
+ data->info = fsp3y_info[data->chip];
+
+ return pmbus_do_probe(client, &data->info);
+}
+
+static const struct i2c_device_id pmbus_id[] = {
+ {"fsp3y", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, pmbus_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe_new = fsp3y_probe,
+ .id_table = pmbus_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
On 4/7/21 7:34 PM, Václav Kubernát wrote:
> This patch adds support for these devices:
> - YH-5151E - the PDU
> - YM-2151E - the PSU
>
> The device datasheet says that the devices support PMBus 1.2, but in my
> testing, a lot of the commands aren't supported and if they are, they
> sometimes behave strangely or inconsistently. For example, writes to the
> PAGE command requires using PEC, otherwise the write won't work and the
> page won't switch, even though, the standard says that PEC is opiotnal.
> On the other hand, writes the SMBALERT don't require PEC. Because of
> this, the driver is mostly reverse engineered with the help of a tool
> called pmbus_peek written by David Brownell (and later adopted by my
> colleague Jan Kundrát).
>
> The device also has some sort of a timing issue when switching pages,
> which is explained further in the code.
>
> Because of this, the driver support is limited. It exposes only the
> values, that have been tested to work correctly.
>
> Signed-off-by: Václav Kubernát <[email protected]>
> ---
> Documentation/hwmon/fsp-3y.rst | 26 ++++
> drivers/hwmon/pmbus/Kconfig | 10 ++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/fsp-3y.c | 217 +++++++++++++++++++++++++++++++++
> 4 files changed, 254 insertions(+)
> create mode 100644 Documentation/hwmon/fsp-3y.rst
> create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
>
> diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
> new file mode 100644
> index 000000000000..68a547021846
> --- /dev/null
> +++ b/Documentation/hwmon/fsp-3y.rst
> @@ -0,0 +1,26 @@
> +Kernel driver fsp3y
> +======================
> +Supported devices:
> + * 3Y POWER YH-5151E
> + * 3Y POWER YM-2151E
> +
> +Author: Václav Kubernát <[email protected]>
> +
> +Description
> +-----------
> +This driver implements limited support for two 3Y POWER devices.
> +
> +Sysfs entries
> +-------------
> +in1_input input voltage
> +in2_input 12V output voltage
> +in3_input 5V output voltage
> +curr1_input input current
> +curr2_input 12V output current
> +curr3_input 5V output current
> +fan1_input fan rpm
> +temp1_input temperature 1
> +temp2_input temperature 2
> +temp3_input temperature 3
> +power1_input input power
> +power2_input output power
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 03606d4298a4..9d12d446396c 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
> This driver can also be built as a module. If so, the module will
> be called bel-pfe.
>
> +config SENSORS_FSP_3Y
> + tristate "FSP/3Y-Power power supplies"
> + help
> + If you say yes here you get hardware monitoring support for
> + FSP/3Y-Power hot-swap power supplies.
> + Supported models: YH-5151E, YM-2151E
> +
> + This driver can also be built as a module. If so, the module will
> + be called fsp-3y.
> +
> config SENSORS_IBM_CFFPS
> tristate "IBM Common Form Factor Power Supply"
> depends on LEDS_CLASS
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..bfe218ad898f 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> new file mode 100644
> index 000000000000..2c165e034fa8
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/fsp-3y.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for FSP 3Y-Power PSUs
> + *
> + * Copyright (c) 2021 Václav Kubernát, CESNET
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +#define YM2151_PAGE_12V_LOG 0x00
> +#define YM2151_PAGE_12V_REAL 0x00
> +#define YM2151_PAGE_5VSB_LOG 0x01
> +#define YM2151_PAGE_5VSB_REAL 0x20
> +#define YH5151E_PAGE_12V_LOG 0x00
> +#define YH5151E_PAGE_12V_REAL 0x00
> +#define YH5151E_PAGE_5V_LOG 0x01
> +#define YH5151E_PAGE_5V_REAL 0x10
> +#define YH5151E_PAGE_3V3_LOG 0x02
> +#define YH5151E_PAGE_3V3_REAL 0x11
> +
> +enum chips {
> + ym2151e,
> + yh5151e
> +};
> +
> +struct fsp3y_data {
> + struct pmbus_driver_info info;
> + enum chips chip;
> + int page;
> +};
> +
> +#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
> +
> +static int page_log_to_page_real(int page_log, enum chips chip)
> +{
> + switch (chip) {
> + case ym2151e:
> + switch (page_log) {
> + case YM2151_PAGE_12V_LOG:
> + return YM2151_PAGE_12V_REAL;
> + case YM2151_PAGE_5VSB_LOG:
> + return YM2151_PAGE_5VSB_REAL;
> + }
> + return -1;
> + case yh5151e:
> + switch (page_log) {
> + case YH5151E_PAGE_12V_LOG:
> + return YH5151E_PAGE_12V_REAL;
> + case YH5151E_PAGE_5V_LOG:
> + return YH5151E_PAGE_5V_LOG;
> + case YH5151E_PAGE_3V3_LOG:
> + return YH5151E_PAGE_3V3_REAL;
> + }
> + return -1;
> + }
> +
> + return -1;
> +}
> +
> +static int set_page(struct i2c_client *client, int page_log)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + struct fsp3y_data *data = to_fsp3y_data(info);
> + int rv;
> + int page_real = page_log_to_page_real(page_log, data->chip);
> +
> + if (page_log < 0)
> + return 0;
> +
I am quite sure this should be
if (page_real < 0)
return 0;
I would suggest to return an error code, though; if page_log_to_page_real()
indeed returns an error, the selected page is wrong, and the reported data
would be pretty much random. That should not really happen, but since you check
for the error code here you might as well do it right. Best would be to have
page_log_to_page_real() return, say, -EINVAL on error and implement the check
as follows.
if (page_real < 0)
return page_real;
> + if (data->page != page_real) {
> + rv = pmbus_set_page(client, page_real, 0xff);
> + if (rv < 0)
> + return rv;
> +
> + data->page = page_real;
> +
> + /* Testing showed that the device has a timing issue. After
> + * setting a page, it takes a while, before the device actually
> + * gives the correct values from the correct page. 20 ms was
> + * tested to be enough to not give wrong values (15 ms wasn't
> + * enough)
> + */
> + usleep_range(20000, 30000);
> + }
> +
> + return 0;
> +}
> +
> +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> + int rv;
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> + int rv;
> +
> + switch (reg) {
> + case PMBUS_READ_FAN_SPEED_1:
> + case PMBUS_READ_IIN:
> + case PMBUS_READ_IOUT:
> + case PMBUS_READ_PIN:
> + case PMBUS_READ_POUT:
> + case PMBUS_READ_TEMPERATURE_1:
> + case PMBUS_READ_TEMPERATURE_2:
> + case PMBUS_READ_TEMPERATURE_3:
> + case PMBUS_READ_VIN:
> + case PMBUS_READ_VOUT:
> + case PMBUS_STATUS_WORD:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_word_data(client, reg);
> +}
> +
> +struct pmbus_driver_info fsp3y_info[] = {
> + [ym2151e] = {
> + .pages = 2,
> + .func[YM2151_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> + PMBUS_HAVE_FAN12,
> + .func[YM2151_PAGE_5VSB_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> + PMBUS_HAVE_IIN,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + },
> + [yh5151e] = {
> + .pages = 3,
> + .func[YH5151E_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> + .func[YH5151E_PAGE_5V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .func[YH5151E_PAGE_3V3_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + }
> +};
> +
> +static int fsp3y_probe(struct i2c_client *client)
> +{
> + struct fsp3y_data *data;
> + int rv;
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> +
> + data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> + if (rv < 0)
> + return rv;
> + data->page = rv;
> +
> + rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> + if (rv < 0)
> + return rv;
> + if (rv != 8)
> + return -ENODEV;
> +
> + if (!strncmp(buf, "YM-2151E", strlen("YM-2151E")))
> + data->chip = ym2151e;
> + else if (!strncmp(buf, "YH-5151E", strlen("YH-5151E")))
> + data->chip = yh5151e;
> + else
> + return -ENODEV;
> +
> + data->info = fsp3y_info[data->chip];
> +
> + return pmbus_do_probe(client, &data->info);
> +}
> +
> +static const struct i2c_device_id pmbus_id[] = {
> + {"fsp3y", 0},
Normally you would have something like
{"ym2151e", ym2151e},
{"yh5151e", yh5151e},
to match specific supplies. The probe function should then warn
if there is a mismatch. See other pmbus drivers for examples.
Guenter
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver fsp3y_driver = {
> + .driver = {
> + .name = "fsp3y",
> + },
> + .probe_new = fsp3y_probe,
> + .id_table = pmbus_id
> +};
> +
> +module_i2c_driver(fsp3y_driver);
> +
> +MODULE_AUTHOR("Václav Kubernát");
> +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> +MODULE_LICENSE("GPL");
>
This patch adds support for these devices:
- YH-5151E - the PDU
- YM-2151E - the PSU
The device datasheet says that the devices support PMBus 1.2, but in my
testing, a lot of the commands aren't supported and if they are, they
sometimes behave strangely or inconsistently. For example, writes to the
PAGE command requires using PEC, otherwise the write won't work and the
page won't switch, even though, the standard says that PEC is opiotnal.
On the other hand, writes the SMBALERT don't require PEC. Because of
this, the driver is mostly reverse engineered with the help of a tool
called pmbus_peek written by David Brownell (and later adopted by my
colleague Jan Kundrát).
The device also has some sort of a timing issue when switching pages,
which is explained further in the code.
Because of this, the driver support is limited. It exposes only the
values, that have been tested to work correctly.
Signed-off-by: Václav Kubernát <[email protected]>
---
Documentation/hwmon/fsp-3y.rst | 26 ++++
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 236 +++++++++++++++++++++++++++++++++
4 files changed, 273 insertions(+)
create mode 100644 Documentation/hwmon/fsp-3y.rst
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
new file mode 100644
index 000000000000..68a547021846
--- /dev/null
+++ b/Documentation/hwmon/fsp-3y.rst
@@ -0,0 +1,26 @@
+Kernel driver fsp3y
+======================
+Supported devices:
+ * 3Y POWER YH-5151E
+ * 3Y POWER YM-2151E
+
+Author: Václav Kubernát <[email protected]>
+
+Description
+-----------
+This driver implements limited support for two 3Y POWER devices.
+
+Sysfs entries
+-------------
+in1_input input voltage
+in2_input 12V output voltage
+in3_input 5V output voltage
+curr1_input input current
+curr2_input 12V output current
+curr3_input 5V output current
+fan1_input fan rpm
+temp1_input temperature 1
+temp2_input temperature 2
+temp3_input temperature 3
+power1_input input power
+power2_input output power
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..9d12d446396c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+ Supported models: YH-5151E, YM-2151E
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..f03c4e27ec8c
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V_LOG 0x00
+#define YM2151_PAGE_12V_REAL 0x00
+#define YM2151_PAGE_5VSB_LOG 0x01
+#define YM2151_PAGE_5VSB_REAL 0x20
+#define YH5151E_PAGE_12V_LOG 0x00
+#define YH5151E_PAGE_12V_REAL 0x00
+#define YH5151E_PAGE_5V_LOG 0x01
+#define YH5151E_PAGE_5V_REAL 0x10
+#define YH5151E_PAGE_3V3_LOG 0x02
+#define YH5151E_PAGE_3V3_REAL 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+struct fsp3y_data {
+ struct pmbus_driver_info info;
+ enum chips chip;
+ int page;
+};
+
+#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
+
+static int page_log_to_page_real(int page_log, enum chips chip)
+{
+ switch (chip) {
+ case ym2151e:
+ switch (page_log) {
+ case YM2151_PAGE_12V_LOG:
+ return YM2151_PAGE_12V_REAL;
+ case YM2151_PAGE_5VSB_LOG:
+ return YM2151_PAGE_5VSB_REAL;
+ }
+ return -EINVAL;
+ case yh5151e:
+ switch (page_log) {
+ case YH5151E_PAGE_12V_LOG:
+ return YH5151E_PAGE_12V_REAL;
+ case YH5151E_PAGE_5V_LOG:
+ return YH5151E_PAGE_5V_LOG;
+ case YH5151E_PAGE_3V3_LOG:
+ return YH5151E_PAGE_3V3_REAL;
+ }
+ return -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
+static int set_page(struct i2c_client *client, int page_log)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct fsp3y_data *data = to_fsp3y_data(info);
+ int rv;
+ int page_real;
+
+ if (page_log < 0)
+ return 0;
+
+ page_real = page_log_to_page_real(page_log, data->chip);
+ if (page_real < 0)
+ return page_real;
+
+ if (data->page != page_real) {
+ rv = pmbus_set_page(client, page_real, 0xff);
+ if (rv < 0)
+ return rv;
+
+ data->page = page_real;
+
+ /* Testing showed that the device has a timing issue. After
+ * setting a page, it takes a while, before the device actually
+ * gives the correct values from the correct page. 20 ms was
+ * tested to be enough to not give wrong values (15 ms wasn't
+ * enough)
+ */
+ usleep_range(20000, 30000);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ switch (reg) {
+ case PMBUS_READ_FAN_SPEED_1:
+ case PMBUS_READ_IIN:
+ case PMBUS_READ_IOUT:
+ case PMBUS_READ_PIN:
+ case PMBUS_READ_POUT:
+ case PMBUS_READ_TEMPERATURE_1:
+ case PMBUS_READ_TEMPERATURE_2:
+ case PMBUS_READ_TEMPERATURE_3:
+ case PMBUS_READ_VIN:
+ case PMBUS_READ_VOUT:
+ case PMBUS_STATUS_WORD:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 2,
+ .func[YM2151_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5VSB_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 3,
+ .func[YH5151E_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_detect(struct i2c_client *client)
+{
+ int rv;
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+
+ rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+ if (rv < 0)
+ return rv;
+
+ if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
+ return ym2151e;
+ else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
+ return yh5151e;
+
+ dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
+ return -ENODEV;
+}
+
+static const struct i2c_device_id fsp3y_id[] = {
+ {"ym2151e", ym2151e},
+ {"yh5151e", yh5151e}
+};
+
+static int fsp3y_probe(struct i2c_client *client)
+{
+ struct fsp3y_data *data;
+ const struct i2c_device_id *id;
+ int rv;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->chip = fsp3y_detect(client);
+
+ id = i2c_match_id(fsp3y_id, client);
+ if (data->chip != id->driver_data)
+ dev_warn(&client->dev,
+ "Device mismatch: Configured %s (%d), detected %d\n",
+ id->name,
+ (int)id->driver_data,
+ data->chip);
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ if (rv < 0)
+ return rv;
+ data->page = rv;
+
+ data->info = fsp3y_info[data->chip];
+
+ return pmbus_do_probe(client, &data->info);
+}
+
+MODULE_DEVICE_TABLE(i2c, pmbus_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe_new = fsp3y_probe,
+ .id_table = fsp3y_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
On 4/8/21 6:27 PM, Václav Kubernát wrote:
> This patch adds support for these devices:
> - YH-5151E - the PDU
> - YM-2151E - the PSU
>
> The device datasheet says that the devices support PMBus 1.2, but in my
> testing, a lot of the commands aren't supported and if they are, they
> sometimes behave strangely or inconsistently. For example, writes to the
> PAGE command requires using PEC, otherwise the write won't work and the
> page won't switch, even though, the standard says that PEC is opiotnal.
> On the other hand, writes the SMBALERT don't require PEC. Because of
> this, the driver is mostly reverse engineered with the help of a tool
> called pmbus_peek written by David Brownell (and later adopted by my
> colleague Jan Kundrát).
>
> The device also has some sort of a timing issue when switching pages,
> which is explained further in the code.
>
> Because of this, the driver support is limited. It exposes only the
> values, that have been tested to work correctly.
>
> Signed-off-by: Václav Kubernát <[email protected]>
> ---
> Documentation/hwmon/fsp-3y.rst | 26 ++++
> drivers/hwmon/pmbus/Kconfig | 10 ++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/fsp-3y.c | 236 +++++++++++++++++++++++++++++++++
> 4 files changed, 273 insertions(+)
> create mode 100644 Documentation/hwmon/fsp-3y.rst
> create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
>
> diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
> new file mode 100644
> index 000000000000..68a547021846
> --- /dev/null
> +++ b/Documentation/hwmon/fsp-3y.rst
> @@ -0,0 +1,26 @@
> +Kernel driver fsp3y
> +======================
> +Supported devices:
> + * 3Y POWER YH-5151E
> + * 3Y POWER YM-2151E
> +
> +Author: Václav Kubernát <[email protected]>
> +
> +Description
> +-----------
> +This driver implements limited support for two 3Y POWER devices.
> +
> +Sysfs entries
> +-------------
> +in1_input input voltage
> +in2_input 12V output voltage
> +in3_input 5V output voltage
> +curr1_input input current
> +curr2_input 12V output current
> +curr3_input 5V output current
> +fan1_input fan rpm
> +temp1_input temperature 1
> +temp2_input temperature 2
> +temp3_input temperature 3
> +power1_input input power
> +power2_input output power
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 03606d4298a4..9d12d446396c 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
> This driver can also be built as a module. If so, the module will
> be called bel-pfe.
>
> +config SENSORS_FSP_3Y
> + tristate "FSP/3Y-Power power supplies"
> + help
> + If you say yes here you get hardware monitoring support for
> + FSP/3Y-Power hot-swap power supplies.
> + Supported models: YH-5151E, YM-2151E
> +
> + This driver can also be built as a module. If so, the module will
> + be called fsp-3y.
> +
> config SENSORS_IBM_CFFPS
> tristate "IBM Common Form Factor Power Supply"
> depends on LEDS_CLASS
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..bfe218ad898f 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> new file mode 100644
> index 000000000000..f03c4e27ec8c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/fsp-3y.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for FSP 3Y-Power PSUs
> + *
> + * Copyright (c) 2021 Václav Kubernát, CESNET
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +#define YM2151_PAGE_12V_LOG 0x00
> +#define YM2151_PAGE_12V_REAL 0x00
> +#define YM2151_PAGE_5VSB_LOG 0x01
> +#define YM2151_PAGE_5VSB_REAL 0x20
> +#define YH5151E_PAGE_12V_LOG 0x00
> +#define YH5151E_PAGE_12V_REAL 0x00
> +#define YH5151E_PAGE_5V_LOG 0x01
> +#define YH5151E_PAGE_5V_REAL 0x10
> +#define YH5151E_PAGE_3V3_LOG 0x02
> +#define YH5151E_PAGE_3V3_REAL 0x11
> +
> +enum chips {
> + ym2151e,
> + yh5151e
> +};
> +
> +struct fsp3y_data {
> + struct pmbus_driver_info info;
> + enum chips chip;
> + int page;
> +};
> +
> +#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
> +
> +static int page_log_to_page_real(int page_log, enum chips chip)
> +{
> + switch (chip) {
> + case ym2151e:
> + switch (page_log) {
> + case YM2151_PAGE_12V_LOG:
> + return YM2151_PAGE_12V_REAL;
> + case YM2151_PAGE_5VSB_LOG:
> + return YM2151_PAGE_5VSB_REAL;
> + }
> + return -EINVAL;
> + case yh5151e:
> + switch (page_log) {
> + case YH5151E_PAGE_12V_LOG:
> + return YH5151E_PAGE_12V_REAL;
> + case YH5151E_PAGE_5V_LOG:
> + return YH5151E_PAGE_5V_LOG;
> + case YH5151E_PAGE_3V3_LOG:
> + return YH5151E_PAGE_3V3_REAL;
> + }
> + return -EINVAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int set_page(struct i2c_client *client, int page_log)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + struct fsp3y_data *data = to_fsp3y_data(info);
> + int rv;
> + int page_real;
> +
> + if (page_log < 0)
> + return 0;
> +
> + page_real = page_log_to_page_real(page_log, data->chip);
> + if (page_real < 0)
> + return page_real;
> +
> + if (data->page != page_real) {
> + rv = pmbus_set_page(client, page_real, 0xff);
Uh, that won't work. pmbus_set_page() indexes its internal data with the
passed page number, or in other words needs to use the logical page number.
You'll have to call i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real)
directly here.
> + if (rv < 0)
> + return rv;
> +
> + data->page = page_real;
> +
> + /* Testing showed that the device has a timing issue. After
> + * setting a page, it takes a while, before the device actually
> + * gives the correct values from the correct page. 20 ms was
> + * tested to be enough to not give wrong values (15 ms wasn't
> + * enough)
> + */
> + usleep_range(20000, 30000);
> + }
> +
> + return 0;
> +}
> +
> +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> + int rv;
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> + int rv;
> +
> + switch (reg) {
> + case PMBUS_READ_FAN_SPEED_1:
> + case PMBUS_READ_IIN:
> + case PMBUS_READ_IOUT:
> + case PMBUS_READ_PIN:
> + case PMBUS_READ_POUT:
> + case PMBUS_READ_TEMPERATURE_1:
> + case PMBUS_READ_TEMPERATURE_2:
> + case PMBUS_READ_TEMPERATURE_3:
> + case PMBUS_READ_VIN:
> + case PMBUS_READ_VOUT:
> + case PMBUS_STATUS_WORD:
> + break;
> + default:
Add a comment here, explaining why other registers are ignored (because
it is unknown what exactly they report).
> + return -ENXIO;
> + }
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_word_data(client, reg);
> +}
> +
> +struct pmbus_driver_info fsp3y_info[] = {
> + [ym2151e] = {
> + .pages = 2,
> + .func[YM2151_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> + PMBUS_HAVE_FAN12,
> + .func[YM2151_PAGE_5VSB_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> + PMBUS_HAVE_IIN,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + },
> + [yh5151e] = {
> + .pages = 3,
> + .func[YH5151E_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> + .func[YH5151E_PAGE_5V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .func[YH5151E_PAGE_3V3_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + }
> +};
> +
> +static int fsp3y_detect(struct i2c_client *client)
> +{
> + int rv;
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> +
> + rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> + if (rv < 0)
> + return rv;
> +
> + if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
> + return ym2151e;
> + else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
> + return yh5151e;
> +
> + dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
> + return -ENODEV;
> +}
> +
> +static const struct i2c_device_id fsp3y_id[] = {
> + {"ym2151e", ym2151e},
> + {"yh5151e", yh5151e}
> +};
> +
> +static int fsp3y_probe(struct i2c_client *client)
> +{
> + struct fsp3y_data *data;
> + const struct i2c_device_id *id;
> + int rv;
> +
> + data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->chip = fsp3y_detect(client);
fsp3y_detect() can return an error which needs to be checked.
Easiest way would be to declare 'data->chip' as int and use
data->chip = fsp3y_detect(client);
if (data->chip < 0)
return data->chip;
> +
> + id = i2c_match_id(fsp3y_id, client);
> + if (data->chip != id->driver_data)
> + dev_warn(&client->dev,
> + "Device mismatch: Configured %s (%d), detected %d\n",
> + id->name,
> + (int)id->driver_data,
> + data->chip);
Please drop excessive continuation lines.
> +
> + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> + if (rv < 0)
> + return rv;
> + data->page = rv;
> +
> + data->info = fsp3y_info[data->chip];
> +
> + return pmbus_do_probe(client, &data->info);
> +}
> +
> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> +
> +/* This is the driver that will be inserted */
> +static struct i2c_driver fsp3y_driver = {
> + .driver = {
> + .name = "fsp3y",
> + },
> + .probe_new = fsp3y_probe,
> + .id_table = fsp3y_id
> +};
> +
> +module_i2c_driver(fsp3y_driver);
> +
> +MODULE_AUTHOR("Václav Kubernát");
> +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> +MODULE_LICENSE("GPL");
>
This patch adds support for these devices:
- YH-5151E - the PDU
- YM-2151E - the PSU
The device datasheet says that the devices support PMBus 1.2, but in my
testing, a lot of the commands aren't supported and if they are, they
sometimes behave strangely or inconsistently. For example, writes to the
PAGE command requires using PEC, otherwise the write won't work and the
page won't switch, even though, the standard says that PEC is opiotnal.
On the other hand, writes the SMBALERT don't require PEC. Because of
this, the driver is mostly reverse engineered with the help of a tool
called pmbus_peek written by David Brownell (and later adopted by my
colleague Jan Kundrát).
The device also has some sort of a timing issue when switching pages,
which is explained further in the code.
Because of this, the driver support is limited. It exposes only the
values, that have been tested to work correctly.
Signed-off-by: Václav Kubernát <[email protected]>
---
Documentation/hwmon/fsp-3y.rst | 26 ++++
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 239 +++++++++++++++++++++++++++++++++
4 files changed, 276 insertions(+)
create mode 100644 Documentation/hwmon/fsp-3y.rst
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
new file mode 100644
index 000000000000..68a547021846
--- /dev/null
+++ b/Documentation/hwmon/fsp-3y.rst
@@ -0,0 +1,26 @@
+Kernel driver fsp3y
+======================
+Supported devices:
+ * 3Y POWER YH-5151E
+ * 3Y POWER YM-2151E
+
+Author: Václav Kubernát <[email protected]>
+
+Description
+-----------
+This driver implements limited support for two 3Y POWER devices.
+
+Sysfs entries
+-------------
+in1_input input voltage
+in2_input 12V output voltage
+in3_input 5V output voltage
+curr1_input input current
+curr2_input 12V output current
+curr3_input 5V output current
+fan1_input fan rpm
+temp1_input temperature 1
+temp2_input temperature 2
+temp3_input temperature 3
+power1_input input power
+power2_input output power
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..9d12d446396c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+ Supported models: YH-5151E, YM-2151E
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..2185ab119fd2
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V_LOG 0x00
+#define YM2151_PAGE_12V_REAL 0x00
+#define YM2151_PAGE_5VSB_LOG 0x01
+#define YM2151_PAGE_5VSB_REAL 0x20
+#define YH5151E_PAGE_12V_LOG 0x00
+#define YH5151E_PAGE_12V_REAL 0x00
+#define YH5151E_PAGE_5V_LOG 0x01
+#define YH5151E_PAGE_5V_REAL 0x10
+#define YH5151E_PAGE_3V3_LOG 0x02
+#define YH5151E_PAGE_3V3_REAL 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+struct fsp3y_data {
+ struct pmbus_driver_info info;
+ int chip;
+ int page;
+};
+
+#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
+
+static int page_log_to_page_real(int page_log, enum chips chip)
+{
+ switch (chip) {
+ case ym2151e:
+ switch (page_log) {
+ case YM2151_PAGE_12V_LOG:
+ return YM2151_PAGE_12V_REAL;
+ case YM2151_PAGE_5VSB_LOG:
+ return YM2151_PAGE_5VSB_REAL;
+ }
+ return -EINVAL;
+ case yh5151e:
+ switch (page_log) {
+ case YH5151E_PAGE_12V_LOG:
+ return YH5151E_PAGE_12V_REAL;
+ case YH5151E_PAGE_5V_LOG:
+ return YH5151E_PAGE_5V_LOG;
+ case YH5151E_PAGE_3V3_LOG:
+ return YH5151E_PAGE_3V3_REAL;
+ }
+ return -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
+static int set_page(struct i2c_client *client, int page_log)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct fsp3y_data *data = to_fsp3y_data(info);
+ int rv;
+ int page_real;
+
+ if (page_log < 0)
+ return 0;
+
+ page_real = page_log_to_page_real(page_log, data->chip);
+ if (page_real < 0)
+ return page_real;
+
+ if (data->page != page_real) {
+ rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
+ if (rv < 0)
+ return rv;
+
+ data->page = page_real;
+
+ /* Testing showed that the device has a timing issue. After
+ * setting a page, it takes a while, before the device actually
+ * gives the correct values from the correct page. 20 ms was
+ * tested to be enough to not give wrong values (15 ms wasn't
+ * enough)
+ */
+ usleep_range(20000, 30000);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ /* This masks commands which weren't tested to work correctly. Some of the masked commands
+ * return either 0xFFFF. These would probably get tagged as invalid by pmbus_core. Other
+ * ones do return values, which might be useful (that is, they are not 0xFFFF), but their
+ * encoding is unknown, and so they are unsupported.
+ */
+ switch (reg) {
+ case PMBUS_READ_FAN_SPEED_1:
+ case PMBUS_READ_IIN:
+ case PMBUS_READ_IOUT:
+ case PMBUS_READ_PIN:
+ case PMBUS_READ_POUT:
+ case PMBUS_READ_TEMPERATURE_1:
+ case PMBUS_READ_TEMPERATURE_2:
+ case PMBUS_READ_TEMPERATURE_3:
+ case PMBUS_READ_VIN:
+ case PMBUS_READ_VOUT:
+ case PMBUS_STATUS_WORD:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 2,
+ .func[YM2151_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5VSB_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 3,
+ .func[YH5151E_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_detect(struct i2c_client *client)
+{
+ int rv;
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+
+ rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+ if (rv < 0)
+ return rv;
+
+ if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
+ return ym2151e;
+ else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
+ return yh5151e;
+
+ dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
+ return -ENODEV;
+}
+
+static const struct i2c_device_id fsp3y_id[] = {
+ {"ym2151e", ym2151e},
+ {"yh5151e", yh5151e}
+};
+
+static int fsp3y_probe(struct i2c_client *client)
+{
+ struct fsp3y_data *data;
+ const struct i2c_device_id *id;
+ int rv;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->chip = fsp3y_detect(client);
+ if (data->chip < 0)
+ return data->chip;
+
+ id = i2c_match_id(fsp3y_id, client);
+ if (data->chip != id->driver_data)
+ dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ if (rv < 0)
+ return rv;
+ data->page = rv;
+
+ data->info = fsp3y_info[data->chip];
+
+ return pmbus_do_probe(client, &data->info);
+}
+
+MODULE_DEVICE_TABLE(i2c, pmbus_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe_new = fsp3y_probe,
+ .id_table = fsp3y_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
út 13. 4. 2021 v 7:36 odesílatel Guenter Roeck <[email protected]> napsal:
>
> On 4/12/21 9:31 PM, Václav Kubernát wrote:
> [ ... ]
> > Okay, I have made some additional testing. Some of the registers
> > return 0xFFFF, some don't. These are the ones that pmbus_core queries
> > when the driver is loading (with values I got from
> > i2c_smbus_read_word_data):
> > PMBUS_POUT_MAX 0xFFFF
> > PMBUS_FAN_COMMAND_1 0x0
> > PMBUS_VOUT_OV_FAULT_LIMIT 0xFFFF
> > PMBUS_VOUT_OV_WARN_LIMIT 0xFFFF
> > PMBUS_VOUT_UV_WARN_LIMIT 0xFFFF
> > PMBUS_VOUT_UV_FAULT_LIMIT 0xFFFF
> > PMBUS_IOUT_OC_FAULT_LIMIT 0xFFFF
> > PMBUS_IOUT_OC_WARN_LIMIT 0x10
> > PMBUS_IOUT_UC_FAULT_LIMIT 0xFFFF
> > PMBUS_OT_FAULT_LIMIT 0xFFFF
> > PMBUS_OT_WARN_LIMIT 0xFFFFFFB6
> > PMBUS_UT_WARN_LIMIT 0xFFFF
> > PMBUS_UT_FAULT_LIMIT 0xFFFF
> > PMBUS_VIN_OV_FAULT_LIMIT 0xFFFF
> > PMBUS_VIN_OV_WARN_LIMIT 0xFFFF
> > PMBUS_VIN_UV_WARN_LIMIT 0xFFFF
> > PMBUS_VIN_UV_FAULT_LIMIT 0xFFFF
> > PMBUS_IIN_OC_FAULT_LIMIT 0xFFFF
> > PMBUS_IIN_OC_WARN_LIMIT 0x0
> > PMBUS_POUT_OP_FAULT_LIMIT 0xFFFF
> > PMBUS_POUT_OP_WARN_LIMIT 0xFFFFFFB6
> > PMBUS_PIN_OP_WARN_LIMIT 0x0
> > PMBUS_READ_FAN_SPEED_2 0x0
> > PMBUS_MFR_VIN_MIN 0x5A
> > PMBUS_MFR_VIN_MAX 0x108
> > PMBUS_MFR_IIN_MAX 0x3
> > PMBUS_MFR_PIN_MAX 0xC8
> > PMBUS_MFR_VOUT_MIN 0x1748
> > PMBUS_MFR_VOUT_MAX 0x18B0
> > PMBUS_MFR_IOUT_MAX 0xF
> > PMBUS_MFR_POUT_MAX 0x96
> > PMBUS_MFR_MAX_TEMP_1 0xFFFF
> > PMBUS_MFR_MAX_TEMP_2 0xFFFF
> >
>
> The question is really what the status register reports for those.
> I bet that PMBUS_STATUS_CML will set the "invalid command" bit
> for many if not all registers returning 0xffff. Those will
> be filtered out by the PMBus core automatically. That leaves the
> ones returning some data. Of course it would be desirable
> to have those supported, but I can understand if you don't know
> the encoding. The reason for masking those needs to be explained.
>
I thought I sent the v4 patch before, but it seems that I only sent it
to myself. Also, I realized I've sent some of the emails only to you.
That wasn't my intention. Sorry. I'm bad at emailing.
Anyway, I have posted the v4 patch right now and it includes a comment
about the masking.
Václav
> Thanks,
> Guenter
On 4/13/21 3:42 AM, Václav Kubernát wrote:
> This patch adds support for these devices:
> - YH-5151E - the PDU
> - YM-2151E - the PSU
>
> The device datasheet says that the devices support PMBus 1.2, but in my
> testing, a lot of the commands aren't supported and if they are, they
> sometimes behave strangely or inconsistently. For example, writes to the
> PAGE command requires using PEC, otherwise the write won't work and the
> page won't switch, even though, the standard says that PEC is opiotnal.
optional
> On the other hand, writes the SMBALERT don't require PEC. Because of
s/writes the/writes to/ ?
> this, the driver is mostly reverse engineered with the help of a tool
> called pmbus_peek written by David Brownell (and later adopted by my
> colleague Jan Kundrát).
>
> The device also has some sort of a timing issue when switching pages,
> which is explained further in the code.
>
> Because of this, the driver support is limited. It exposes only the
> values, that have been tested to work correctly.
>
You might want to add those details into the driver code, below the
copyright line. It would be more valuable there than in the commit log.
> Signed-off-by: Václav Kubernát <[email protected]>
> ---
> Documentation/hwmon/fsp-3y.rst | 26 ++++
Needs to be added to index.rst.
> drivers/hwmon/pmbus/Kconfig | 10 ++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/fsp-3y.c | 239 +++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+)
> create mode 100644 Documentation/hwmon/fsp-3y.rst
> create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
>
> diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
> new file mode 100644
> index 000000000000..68a547021846
> --- /dev/null
> +++ b/Documentation/hwmon/fsp-3y.rst
> @@ -0,0 +1,26 @@
> +Kernel driver fsp3y
> +======================
> +Supported devices:
> + * 3Y POWER YH-5151E
> + * 3Y POWER YM-2151E
> +
> +Author: Václav Kubernát <[email protected]>
> +
> +Description
> +-----------
> +This driver implements limited support for two 3Y POWER devices.
> +
> +Sysfs entries
> +-------------
> +in1_input input voltage
> +in2_input 12V output voltage
> +in3_input 5V output voltage
> +curr1_input input current
> +curr2_input 12V output current
> +curr3_input 5V output current
> +fan1_input fan rpm
> +temp1_input temperature 1
> +temp2_input temperature 2
> +temp3_input temperature 3
> +power1_input input power
> +power2_input output power
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 03606d4298a4..9d12d446396c 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
> This driver can also be built as a module. If so, the module will
> be called bel-pfe.
>
> +config SENSORS_FSP_3Y
> + tristate "FSP/3Y-Power power supplies"
> + help
> + If you say yes here you get hardware monitoring support for
> + FSP/3Y-Power hot-swap power supplies.
> + Supported models: YH-5151E, YM-2151E
> +
> + This driver can also be built as a module. If so, the module will
> + be called fsp-3y.
> +
> config SENSORS_IBM_CFFPS
> tristate "IBM Common Form Factor Power Supply"
> depends on LEDS_CLASS
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 6a4ba0fdc1db..bfe218ad898f 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> new file mode 100644
> index 000000000000..2185ab119fd2
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/fsp-3y.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Hardware monitoring driver for FSP 3Y-Power PSUs
> + *
> + * Copyright (c) 2021 Václav Kubernát, CESNET
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include "pmbus.h"
> +
> +#define YM2151_PAGE_12V_LOG 0x00
> +#define YM2151_PAGE_12V_REAL 0x00
> +#define YM2151_PAGE_5VSB_LOG 0x01
> +#define YM2151_PAGE_5VSB_REAL 0x20
> +#define YH5151E_PAGE_12V_LOG 0x00
> +#define YH5151E_PAGE_12V_REAL 0x00
> +#define YH5151E_PAGE_5V_LOG 0x01
> +#define YH5151E_PAGE_5V_REAL 0x10
> +#define YH5151E_PAGE_3V3_LOG 0x02
> +#define YH5151E_PAGE_3V3_REAL 0x11
> +
> +enum chips {
> + ym2151e,
> + yh5151e
> +};
> +
> +struct fsp3y_data {
> + struct pmbus_driver_info info;
> + int chip;
> + int page;
> +};
> +
> +#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
> +
> +static int page_log_to_page_real(int page_log, enum chips chip)
> +{
> + switch (chip) {
> + case ym2151e:
> + switch (page_log) {
> + case YM2151_PAGE_12V_LOG:
> + return YM2151_PAGE_12V_REAL;
> + case YM2151_PAGE_5VSB_LOG:
> + return YM2151_PAGE_5VSB_REAL;
> + }
> + return -EINVAL;
> + case yh5151e:
> + switch (page_log) {
> + case YH5151E_PAGE_12V_LOG:
> + return YH5151E_PAGE_12V_REAL;
> + case YH5151E_PAGE_5V_LOG:
> + return YH5151E_PAGE_5V_LOG;
> + case YH5151E_PAGE_3V3_LOG:
> + return YH5151E_PAGE_3V3_REAL;
> + }
> + return -EINVAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int set_page(struct i2c_client *client, int page_log)
> +{
> + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> + struct fsp3y_data *data = to_fsp3y_data(info);
> + int rv;
> + int page_real;
> +
> + if (page_log < 0)
> + return 0;
> +
> + page_real = page_log_to_page_real(page_log, data->chip);
> + if (page_real < 0)
> + return page_real;
> +
> + if (data->page != page_real) {
> + rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
> + if (rv < 0)
> + return rv;
> +
> + data->page = page_real;
> +
> + /* Testing showed that the device has a timing issue. After
Somehow network subsystem multi-line alignments slipped in.
Not in hwmon, please. I cringe at those; it makes my brain focus on the
comment (because it is asynchronous) instead of the code.
> + * setting a page, it takes a while, before the device actually
> + * gives the correct values from the correct page. 20 ms was
> + * tested to be enough to not give wrong values (15 ms wasn't
> + * enough)
> + */
> + usleep_range(20000, 30000);
> + }
> +
> + return 0;
> +}
> +
> +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> + int rv;
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> +{
> + int rv;
> +
> + /* This masks commands which weren't tested to work correctly. Some of the masked commands
> + * return either 0xFFFF. These would probably get tagged as invalid by pmbus_core. Other
s/either// ?
> + * ones do return values, which might be useful (that is, they are not 0xFFFF), but their
s/values,/values/
> + * encoding is unknown, and so they are unsupported.
> + */
> + switch (reg) {
> + case PMBUS_READ_FAN_SPEED_1:
> + case PMBUS_READ_IIN:
> + case PMBUS_READ_IOUT:
> + case PMBUS_READ_PIN:
> + case PMBUS_READ_POUT:
> + case PMBUS_READ_TEMPERATURE_1:
> + case PMBUS_READ_TEMPERATURE_2:
> + case PMBUS_READ_TEMPERATURE_3:
> + case PMBUS_READ_VIN:
> + case PMBUS_READ_VOUT:
> + case PMBUS_STATUS_WORD:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + rv = set_page(client, page);
> + if (rv < 0)
> + return rv;
> +
> + return i2c_smbus_read_word_data(client, reg);
> +}
> +
> +struct pmbus_driver_info fsp3y_info[] = {
> + [ym2151e] = {
> + .pages = 2,
> + .func[YM2151_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> + PMBUS_HAVE_FAN12,
> + .func[YM2151_PAGE_5VSB_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> + PMBUS_HAVE_IIN,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + },
> + [yh5151e] = {
> + .pages = 3,
> + .func[YH5151E_PAGE_12V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> + .func[YH5151E_PAGE_5V_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .func[YH5151E_PAGE_3V3_LOG] =
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_POUT,
> + .read_word_data = fsp3y_read_word_data,
> + .read_byte_data = fsp3y_read_byte_data,
> + }
> +};
> +
> +static int fsp3y_detect(struct i2c_client *client)
> +{
> + int rv;
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> +
> + rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> + if (rv < 0)
> + return rv;
> +
> + if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
> + return ym2151e;
> + else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
> + return yh5151e;
better
if (rv == 8) {
/* rest of check */
}
> +
> + dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
Sorry I didn't notice before. This assumes that the buffer is zero-terminated,
which may not be the case. For this to work, add another byte to the buffer
(u8 buf[I2C_SMBUS_BLOCK_MAX + 1];), and add
buf[rv] = '\0';
You could actually do that before checking the returned strings and then just
use strcmp() for the model comparisons, without bothering about the return
length.
> + return -ENODEV;
> +}
> +
> +static const struct i2c_device_id fsp3y_id[] = {
> + {"ym2151e", ym2151e},
> + {"yh5151e", yh5151e}
> +};
> +
> +static int fsp3y_probe(struct i2c_client *client)
> +{
> + struct fsp3y_data *data;
> + const struct i2c_device_id *id;
> + int rv;
> +
> + data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->chip = fsp3y_detect(client);
> + if (data->chip < 0)
> + return data->chip;
> +
> + id = i2c_match_id(fsp3y_id, client);
> + if (data->chip != id->driver_data)
> + dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
> +
> + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> + if (rv < 0)
> + return rv;
> + data->page = rv;
> +
> + data->info = fsp3y_info[data->chip];
> +
> + return pmbus_do_probe(client, &data->info);
> +}
> +
> +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> +
> +/* This is the driver that will be inserted */
Nit: Useless comment
> +static struct i2c_driver fsp3y_driver = {
> + .driver = {
> + .name = "fsp3y",
> + },
> + .probe_new = fsp3y_probe,
> + .id_table = fsp3y_id
> +};
> +
> +module_i2c_driver(fsp3y_driver);
> +
> +MODULE_AUTHOR("Václav Kubernát");
> +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> +MODULE_LICENSE("GPL");
>
This patch adds support for these devices:
- YH-5151E - the PDU
- YM-2151E - the PSU
The device datasheet says that the devices support PMBus 1.2, but in my
testing, a lot of the commands aren't supported and if they are, they
sometimes behave strangely or inconsistently. For example, writes to the
PAGE command requires using PEC, otherwise the write won't work and the
page won't switch, even though, the standard says that PEC is optional.
On the other hand, writes to SMBALERT don't require PEC. Because of
this, the driver is mostly reverse engineered with the help of a tool
called pmbus_peek written by David Brownell (and later adopted by my
colleague Jan Kundrát).
The device also has some sort of a timing issue when switching pages,
which is explained further in the code.
Because of this, the driver support is limited. It exposes only the
values, that have been tested to work correctly.
Signed-off-by: Václav Kubernát <[email protected]>
---
Documentation/hwmon/fsp-3y.rst | 26 ++++
Documentation/hwmon/index.rst | 1 +
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 251 +++++++++++++++++++++++++++++++++
5 files changed, 289 insertions(+)
create mode 100644 Documentation/hwmon/fsp-3y.rst
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
new file mode 100644
index 000000000000..fc87d4686032
--- /dev/null
+++ b/Documentation/hwmon/fsp-3y.rst
@@ -0,0 +1,26 @@
+Kernel driver fsp3y
+======================
+Supported devices:
+ * 3Y POWER YH-5151E
+ * 3Y POWER YM-2151E
+
+Author: Václav Kubernát <[email protected]>
+
+Description
+-----------
+This driver implements limited support for two 3Y POWER devices.
+
+Sysfs entries
+-------------
+ * in1_input input voltage
+ * in2_input 12V output voltage
+ * in3_input 5V output voltage
+ * curr1_input input current
+ * curr2_input 12V output current
+ * curr3_input 5V output current
+ * fan1_input fan rpm
+ * temp1_input temperature 1
+ * temp2_input temperature 2
+ * temp3_input temperature 3
+ * power1_input input power
+ * power2_input output power
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fcb870ce6286..55c9f014c248 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -63,6 +63,7 @@ Hardware Monitoring Kernel Drivers
f71805f
f71882fg
fam15h_power
+ fsp-3y
ftsteutates
g760a
g762
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..9d12d446396c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+ Supported models: YH-5151E, YM-2151E
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..af58f9950f3d
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ *
+ * This driver is mostly reverse engineered with the help of a tool called pmbus_peek written by
+ * David Brownell (and later adopted by Jan Kundrát). The device has some sort of a timing issue
+ * when switching pages, details are explained in the code. The driver support is limited. It
+ * exposes only the values, that have been tested to work correctly. Unsupported values either
+ * aren't supported by the devices or their encondings are unknown.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V_LOG 0x00
+#define YM2151_PAGE_12V_REAL 0x00
+#define YM2151_PAGE_5VSB_LOG 0x01
+#define YM2151_PAGE_5VSB_REAL 0x20
+#define YH5151E_PAGE_12V_LOG 0x00
+#define YH5151E_PAGE_12V_REAL 0x00
+#define YH5151E_PAGE_5V_LOG 0x01
+#define YH5151E_PAGE_5V_REAL 0x10
+#define YH5151E_PAGE_3V3_LOG 0x02
+#define YH5151E_PAGE_3V3_REAL 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+struct fsp3y_data {
+ struct pmbus_driver_info info;
+ int chip;
+ int page;
+};
+
+#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
+
+static int page_log_to_page_real(int page_log, enum chips chip)
+{
+ switch (chip) {
+ case ym2151e:
+ switch (page_log) {
+ case YM2151_PAGE_12V_LOG:
+ return YM2151_PAGE_12V_REAL;
+ case YM2151_PAGE_5VSB_LOG:
+ return YM2151_PAGE_5VSB_REAL;
+ }
+ return -EINVAL;
+ case yh5151e:
+ switch (page_log) {
+ case YH5151E_PAGE_12V_LOG:
+ return YH5151E_PAGE_12V_REAL;
+ case YH5151E_PAGE_5V_LOG:
+ return YH5151E_PAGE_5V_LOG;
+ case YH5151E_PAGE_3V3_LOG:
+ return YH5151E_PAGE_3V3_REAL;
+ }
+ return -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
+static int set_page(struct i2c_client *client, int page_log)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct fsp3y_data *data = to_fsp3y_data(info);
+ int rv;
+ int page_real;
+
+ if (page_log < 0)
+ return 0;
+
+ page_real = page_log_to_page_real(page_log, data->chip);
+ if (page_real < 0)
+ return page_real;
+
+ if (data->page != page_real) {
+ rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
+ if (rv < 0)
+ return rv;
+
+ data->page = page_real;
+
+ /*
+ * Testing showed that the device has a timing issue. After
+ * setting a page, it takes a while, before the device actually
+ * gives the correct values from the correct page. 20 ms was
+ * tested to be enough to not give wrong values (15 ms wasn't
+ * enough).
+ */
+ usleep_range(20000, 30000);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ /*
+ * This masks commands which weren't tested to work correctly. Some of
+ * the masked commands return 0xFFFF. These would probably get tagged as
+ * invalid by pmbus_core. Other ones do return values which might be
+ * useful (that is, they are not 0xFFFF), but their encoding is unknown,
+ * and so they are unsupported.
+ */
+ switch (reg) {
+ case PMBUS_READ_FAN_SPEED_1:
+ case PMBUS_READ_IIN:
+ case PMBUS_READ_IOUT:
+ case PMBUS_READ_PIN:
+ case PMBUS_READ_POUT:
+ case PMBUS_READ_TEMPERATURE_1:
+ case PMBUS_READ_TEMPERATURE_2:
+ case PMBUS_READ_TEMPERATURE_3:
+ case PMBUS_READ_VIN:
+ case PMBUS_READ_VOUT:
+ case PMBUS_STATUS_WORD:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 2,
+ .func[YM2151_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5VSB_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 3,
+ .func[YH5151E_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_detect(struct i2c_client *client)
+{
+ int rv;
+ u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+
+ rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+ if (rv < 0)
+ return rv;
+
+ buf[rv] = '\0';
+
+ if (rv == 8) {
+ if (!strcmp(buf, "YM-2151E"))
+ return ym2151e;
+ else if (!strcmp(buf, "YH-5151E"))
+ return yh5151e;
+ }
+
+ dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
+ return -ENODEV;
+}
+
+static const struct i2c_device_id fsp3y_id[] = {
+ {"ym2151e", ym2151e},
+ {"yh5151e", yh5151e}
+};
+
+static int fsp3y_probe(struct i2c_client *client)
+{
+ struct fsp3y_data *data;
+ const struct i2c_device_id *id;
+ int rv;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->chip = fsp3y_detect(client);
+ if (data->chip < 0)
+ return data->chip;
+
+ id = i2c_match_id(fsp3y_id, client);
+ if (data->chip != id->driver_data)
+ dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ if (rv < 0)
+ return rv;
+ data->page = rv;
+
+ data->info = fsp3y_info[data->chip];
+
+ return pmbus_do_probe(client, &data->info);
+}
+
+MODULE_DEVICE_TABLE(i2c, fsp3y_id);
+
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe_new = fsp3y_probe,
+ .id_table = fsp3y_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
út 13. 4. 2021 v 16:59 odesílatel Guenter Roeck <[email protected]> napsal:
>
> On 4/13/21 3:42 AM, Václav Kubernát wrote:
> > This patch adds support for these devices:
> > - YH-5151E - the PDU
> > - YM-2151E - the PSU
> >
> > The device datasheet says that the devices support PMBus 1.2, but in my
> > testing, a lot of the commands aren't supported and if they are, they
> > sometimes behave strangely or inconsistently. For example, writes to the
> > PAGE command requires using PEC, otherwise the write won't work and the
> > page won't switch, even though, the standard says that PEC is opiotnal.
>
> optional
Done.
>
> > On the other hand, writes the SMBALERT don't require PEC. Because of
>
> s/writes the/writes to/ ?
Done.
>
> > this, the driver is mostly reverse engineered with the help of a tool
> > called pmbus_peek written by David Brownell (and later adopted by my
> > colleague Jan Kundrát).
> >
> > The device also has some sort of a timing issue when switching pages,
> > which is explained further in the code.
> >
> > Because of this, the driver support is limited. It exposes only the
> > values, that have been tested to work correctly.
> >
>
> You might want to add those details into the driver code, below the
> copyright line. It would be more valuable there than in the commit log.
>
Done.
> > Signed-off-by: Václav Kubernát <[email protected]>
> > ---
> > Documentation/hwmon/fsp-3y.rst | 26 ++++
>
> Needs to be added to index.rst.
Done.
>
> > drivers/hwmon/pmbus/Kconfig | 10 ++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/fsp-3y.c | 239 +++++++++++++++++++++++++++++++++
> > 4 files changed, 276 insertions(+)
> > create mode 100644 Documentation/hwmon/fsp-3y.rst
> > create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
> >
> > diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
> > new file mode 100644
> > index 000000000000..68a547021846
> > --- /dev/null
> > +++ b/Documentation/hwmon/fsp-3y.rst
> > @@ -0,0 +1,26 @@
> > +Kernel driver fsp3y
> > +======================
> > +Supported devices:
> > + * 3Y POWER YH-5151E
> > + * 3Y POWER YM-2151E
> > +
> > +Author: Václav Kubernát <[email protected]>
> > +
> > +Description
> > +-----------
> > +This driver implements limited support for two 3Y POWER devices.
> > +
> > +Sysfs entries
> > +-------------
> > +in1_input input voltage
> > +in2_input 12V output voltage
> > +in3_input 5V output voltage
> > +curr1_input input current
> > +curr2_input 12V output current
> > +curr3_input 5V output current
> > +fan1_input fan rpm
> > +temp1_input temperature 1
> > +temp2_input temperature 2
> > +temp3_input temperature 3
> > +power1_input input power
> > +power2_input output power
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 03606d4298a4..9d12d446396c 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
> > This driver can also be built as a module. If so, the module will
> > be called bel-pfe.
> >
> > +config SENSORS_FSP_3Y
> > + tristate "FSP/3Y-Power power supplies"
> > + help
> > + If you say yes here you get hardware monitoring support for
> > + FSP/3Y-Power hot-swap power supplies.
> > + Supported models: YH-5151E, YM-2151E
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called fsp-3y.
> > +
> > config SENSORS_IBM_CFFPS
> > tristate "IBM Common Form Factor Power Supply"
> > depends on LEDS_CLASS
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index 6a4ba0fdc1db..bfe218ad898f 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> > obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
> > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> > obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
> > +obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
> > obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> > obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> > diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
> > new file mode 100644
> > index 000000000000..2185ab119fd2
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/fsp-3y.c
> > @@ -0,0 +1,239 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Hardware monitoring driver for FSP 3Y-Power PSUs
> > + *
> > + * Copyright (c) 2021 Václav Kubernát, CESNET
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include "pmbus.h"
> > +
> > +#define YM2151_PAGE_12V_LOG 0x00
> > +#define YM2151_PAGE_12V_REAL 0x00
> > +#define YM2151_PAGE_5VSB_LOG 0x01
> > +#define YM2151_PAGE_5VSB_REAL 0x20
> > +#define YH5151E_PAGE_12V_LOG 0x00
> > +#define YH5151E_PAGE_12V_REAL 0x00
> > +#define YH5151E_PAGE_5V_LOG 0x01
> > +#define YH5151E_PAGE_5V_REAL 0x10
> > +#define YH5151E_PAGE_3V3_LOG 0x02
> > +#define YH5151E_PAGE_3V3_REAL 0x11
> > +
> > +enum chips {
> > + ym2151e,
> > + yh5151e
> > +};
> > +
> > +struct fsp3y_data {
> > + struct pmbus_driver_info info;
> > + int chip;
> > + int page;
> > +};
> > +
> > +#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
> > +
> > +static int page_log_to_page_real(int page_log, enum chips chip)
> > +{
> > + switch (chip) {
> > + case ym2151e:
> > + switch (page_log) {
> > + case YM2151_PAGE_12V_LOG:
> > + return YM2151_PAGE_12V_REAL;
> > + case YM2151_PAGE_5VSB_LOG:
> > + return YM2151_PAGE_5VSB_REAL;
> > + }
> > + return -EINVAL;
> > + case yh5151e:
> > + switch (page_log) {
> > + case YH5151E_PAGE_12V_LOG:
> > + return YH5151E_PAGE_12V_REAL;
> > + case YH5151E_PAGE_5V_LOG:
> > + return YH5151E_PAGE_5V_LOG;
> > + case YH5151E_PAGE_3V3_LOG:
> > + return YH5151E_PAGE_3V3_REAL;
> > + }
> > + return -EINVAL;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int set_page(struct i2c_client *client, int page_log)
> > +{
> > + const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> > + struct fsp3y_data *data = to_fsp3y_data(info);
> > + int rv;
> > + int page_real;
> > +
> > + if (page_log < 0)
> > + return 0;
> > +
> > + page_real = page_log_to_page_real(page_log, data->chip);
> > + if (page_real < 0)
> > + return page_real;
> > +
> > + if (data->page != page_real) {
> > + rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
> > + if (rv < 0)
> > + return rv;
> > +
> > + data->page = page_real;
> > +
> > + /* Testing showed that the device has a timing issue. After
>
> Somehow network subsystem multi-line alignments slipped in.
> Not in hwmon, please. I cringe at those; it makes my brain focus on the
> comment (because it is asynchronous) instead of the code.
>
Done.
> > + * setting a page, it takes a while, before the device actually
> > + * gives the correct values from the correct page. 20 ms was
> > + * tested to be enough to not give wrong values (15 ms wasn't
> > + * enough)
> > + */
> > + usleep_range(20000, 30000);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
> > +{
> > + int rv;
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
> > +{
> > + int rv;
> > +
> > + /* This masks commands which weren't tested to work correctly. Some of the masked commands
> > + * return either 0xFFFF. These would probably get tagged as invalid by pmbus_core. Other
>
> s/either// ?
Done.
>
> > + * ones do return values, which might be useful (that is, they are not 0xFFFF), but their
>
> s/values,/values/
Done.
>
> > + * encoding is unknown, and so they are unsupported.
> > + */
> > + switch (reg) {
> > + case PMBUS_READ_FAN_SPEED_1:
> > + case PMBUS_READ_IIN:
> > + case PMBUS_READ_IOUT:
> > + case PMBUS_READ_PIN:
> > + case PMBUS_READ_POUT:
> > + case PMBUS_READ_TEMPERATURE_1:
> > + case PMBUS_READ_TEMPERATURE_2:
> > + case PMBUS_READ_TEMPERATURE_3:
> > + case PMBUS_READ_VIN:
> > + case PMBUS_READ_VOUT:
> > + case PMBUS_STATUS_WORD:
> > + break;
> > + default:
> > + return -ENXIO;
> > + }
> > +
> > + rv = set_page(client, page);
> > + if (rv < 0)
> > + return rv;
> > +
> > + return i2c_smbus_read_word_data(client, reg);
> > +}
> > +
> > +struct pmbus_driver_info fsp3y_info[] = {
> > + [ym2151e] = {
> > + .pages = 2,
> > + .func[YM2151_PAGE_12V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> > + PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> > + PMBUS_HAVE_FAN12,
> > + .func[YM2151_PAGE_5VSB_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
> > + PMBUS_HAVE_IIN,
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + },
> > + [yh5151e] = {
> > + .pages = 3,
> > + .func[YH5151E_PAGE_12V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
> > + .func[YH5151E_PAGE_5V_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .func[YH5151E_PAGE_3V3_LOG] =
> > + PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_POUT,
> > + .read_word_data = fsp3y_read_word_data,
> > + .read_byte_data = fsp3y_read_byte_data,
> > + }
> > +};
> > +
> > +static int fsp3y_detect(struct i2c_client *client)
> > +{
> > + int rv;
> > + u8 buf[I2C_SMBUS_BLOCK_MAX];
> > +
> > + rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
> > + if (rv < 0)
> > + return rv;
> > +
> > + if (rv == 8 && !strncmp(buf, "YM-2151E", strlen("YM-2151E")))
> > + return ym2151e;
> > + else if (rv == 8 && !strncmp(buf, "YH-5151E", strlen("YH-5151E")))
> > + return yh5151e;
>
> better
> if (rv == 8) {
> /* rest of check */
> }
>
Done.
> > +
> > + dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
>
> Sorry I didn't notice before. This assumes that the buffer is zero-terminated,
> which may not be the case. For this to work, add another byte to the buffer
> (u8 buf[I2C_SMBUS_BLOCK_MAX + 1];), and add
> buf[rv] = '\0';
>
> You could actually do that before checking the returned strings and then just
> use strcmp() for the model comparisons, without bothering about the return
> length.
>
Done.
> > + return -ENODEV;
> > +}
> > +
> > +static const struct i2c_device_id fsp3y_id[] = {
> > + {"ym2151e", ym2151e},
> > + {"yh5151e", yh5151e}
> > +};
> > +
> > +static int fsp3y_probe(struct i2c_client *client)
> > +{
> > + struct fsp3y_data *data;
> > + const struct i2c_device_id *id;
> > + int rv;
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->chip = fsp3y_detect(client);
> > + if (data->chip < 0)
> > + return data->chip;
> > +
> > + id = i2c_match_id(fsp3y_id, client);
> > + if (data->chip != id->driver_data)
> > + dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
> > +
> > + rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> > + if (rv < 0)
> > + return rv;
> > + data->page = rv;
> > +
> > + data->info = fsp3y_info[data->chip];
> > +
> > + return pmbus_do_probe(client, &data->info);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(i2c, pmbus_id);
> > +
> > +/* This is the driver that will be inserted */
>
> Nit: Useless comment
>
Done.
> > +static struct i2c_driver fsp3y_driver = {
> > + .driver = {
> > + .name = "fsp3y",
> > + },
> > + .probe_new = fsp3y_probe,
> > + .id_table = fsp3y_id
> > +};
> > +
> > +module_i2c_driver(fsp3y_driver);
> > +
> > +MODULE_AUTHOR("Václav Kubernát");
> > +MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
> > +MODULE_LICENSE("GPL");
> >
>
This patch adds support for these devices:
- YH-5151E - the PDU
- YM-2151E - the PSU
The device datasheet says that the devices support PMBus 1.2, but in my
testing, a lot of the commands aren't supported and if they are, they
sometimes behave strangely or inconsistently. For example, writes to the
PAGE command requires using PEC, otherwise the write won't work and the
page won't switch, even though, the standard says that PEC is optional.
On the other hand, writes to SMBALERT don't require PEC. Because of
this, the driver is mostly reverse engineered with the help of a tool
called pmbus_peek written by David Brownell (and later adopted by my
colleague Jan Kundrát).
The device also has some sort of a timing issue when switching pages,
which is explained further in the code.
Because of this, the driver support is limited. It exposes only the
values, that have been tested to work correctly.
Signed-off-by: Václav Kubernát <[email protected]>
---
Documentation/hwmon/fsp-3y.rst | 28 ++++
Documentation/hwmon/index.rst | 1 +
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/fsp-3y.c | 253 +++++++++++++++++++++++++++++++++
5 files changed, 293 insertions(+)
create mode 100644 Documentation/hwmon/fsp-3y.rst
create mode 100644 drivers/hwmon/pmbus/fsp-3y.c
diff --git a/Documentation/hwmon/fsp-3y.rst b/Documentation/hwmon/fsp-3y.rst
new file mode 100644
index 000000000000..5693d83a2035
--- /dev/null
+++ b/Documentation/hwmon/fsp-3y.rst
@@ -0,0 +1,28 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver fsp3y
+======================
+Supported devices:
+ * 3Y POWER YH-5151E
+ * 3Y POWER YM-2151E
+
+Author: Václav Kubernát <[email protected]>
+
+Description
+-----------
+This driver implements limited support for two 3Y POWER devices.
+
+Sysfs entries
+-------------
+ * in1_input input voltage
+ * in2_input 12V output voltage
+ * in3_input 5V output voltage
+ * curr1_input input current
+ * curr2_input 12V output current
+ * curr3_input 5V output current
+ * fan1_input fan rpm
+ * temp1_input temperature 1
+ * temp2_input temperature 2
+ * temp3_input temperature 3
+ * power1_input input power
+ * power2_input output power
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fcb870ce6286..55c9f014c248 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -63,6 +63,7 @@ Hardware Monitoring Kernel Drivers
f71805f
f71882fg
fam15h_power
+ fsp-3y
ftsteutates
g760a
g762
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 03606d4298a4..9d12d446396c 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,16 @@ config SENSORS_BEL_PFE
This driver can also be built as a module. If so, the module will
be called bel-pfe.
+config SENSORS_FSP_3Y
+ tristate "FSP/3Y-Power power supplies"
+ help
+ If you say yes here you get hardware monitoring support for
+ FSP/3Y-Power hot-swap power supplies.
+ Supported models: YH-5151E, YM-2151E
+
+ This driver can also be built as a module. If so, the module will
+ be called fsp-3y.
+
config SENSORS_IBM_CFFPS
tristate "IBM Common Form Factor Power Supply"
depends on LEDS_CLASS
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 6a4ba0fdc1db..bfe218ad898f 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o
+obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
diff --git a/drivers/hwmon/pmbus/fsp-3y.c b/drivers/hwmon/pmbus/fsp-3y.c
new file mode 100644
index 000000000000..564649e87e34
--- /dev/null
+++ b/drivers/hwmon/pmbus/fsp-3y.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Hardware monitoring driver for FSP 3Y-Power PSUs
+ *
+ * Copyright (c) 2021 Václav Kubernát, CESNET
+ *
+ * This driver is mostly reverse engineered with the help of a tool called pmbus_peek written by
+ * David Brownell (and later adopted by Jan Kundrát). The device has some sort of a timing issue
+ * when switching pages, details are explained in the code. The driver support is limited. It
+ * exposes only the values, that have been tested to work correctly. Unsupported values either
+ * aren't supported by the devices or their encondings are unknown.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+#define YM2151_PAGE_12V_LOG 0x00
+#define YM2151_PAGE_12V_REAL 0x00
+#define YM2151_PAGE_5VSB_LOG 0x01
+#define YM2151_PAGE_5VSB_REAL 0x20
+#define YH5151E_PAGE_12V_LOG 0x00
+#define YH5151E_PAGE_12V_REAL 0x00
+#define YH5151E_PAGE_5V_LOG 0x01
+#define YH5151E_PAGE_5V_REAL 0x10
+#define YH5151E_PAGE_3V3_LOG 0x02
+#define YH5151E_PAGE_3V3_REAL 0x11
+
+enum chips {
+ ym2151e,
+ yh5151e
+};
+
+struct fsp3y_data {
+ struct pmbus_driver_info info;
+ int chip;
+ int page;
+};
+
+#define to_fsp3y_data(x) container_of(x, struct fsp3y_data, info)
+
+static int page_log_to_page_real(int page_log, enum chips chip)
+{
+ switch (chip) {
+ case ym2151e:
+ switch (page_log) {
+ case YM2151_PAGE_12V_LOG:
+ return YM2151_PAGE_12V_REAL;
+ case YM2151_PAGE_5VSB_LOG:
+ return YM2151_PAGE_5VSB_REAL;
+ }
+ return -EINVAL;
+ case yh5151e:
+ switch (page_log) {
+ case YH5151E_PAGE_12V_LOG:
+ return YH5151E_PAGE_12V_REAL;
+ case YH5151E_PAGE_5V_LOG:
+ return YH5151E_PAGE_5V_LOG;
+ case YH5151E_PAGE_3V3_LOG:
+ return YH5151E_PAGE_3V3_REAL;
+ }
+ return -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
+static int set_page(struct i2c_client *client, int page_log)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct fsp3y_data *data = to_fsp3y_data(info);
+ int rv;
+ int page_real;
+
+ if (page_log < 0)
+ return 0;
+
+ page_real = page_log_to_page_real(page_log, data->chip);
+ if (page_real < 0)
+ return page_real;
+
+ if (data->page != page_real) {
+ rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page_real);
+ if (rv < 0)
+ return rv;
+
+ data->page = page_real;
+
+ /*
+ * Testing showed that the device has a timing issue. After
+ * setting a page, it takes a while, before the device actually
+ * gives the correct values from the correct page. 20 ms was
+ * tested to be enough to not give wrong values (15 ms wasn't
+ * enough).
+ */
+ usleep_range(20000, 30000);
+ }
+
+ return 0;
+}
+
+static int fsp3y_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+ int rv;
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_byte_data(client, reg);
+}
+
+static int fsp3y_read_word_data(struct i2c_client *client, int page, int phase, int reg)
+{
+ int rv;
+
+ /*
+ * This masks commands which weren't tested to work correctly. Some of
+ * the masked commands return 0xFFFF. These would probably get tagged as
+ * invalid by pmbus_core. Other ones do return values which might be
+ * useful (that is, they are not 0xFFFF), but their encoding is unknown,
+ * and so they are unsupported.
+ */
+ switch (reg) {
+ case PMBUS_READ_FAN_SPEED_1:
+ case PMBUS_READ_IIN:
+ case PMBUS_READ_IOUT:
+ case PMBUS_READ_PIN:
+ case PMBUS_READ_POUT:
+ case PMBUS_READ_TEMPERATURE_1:
+ case PMBUS_READ_TEMPERATURE_2:
+ case PMBUS_READ_TEMPERATURE_3:
+ case PMBUS_READ_VIN:
+ case PMBUS_READ_VOUT:
+ case PMBUS_STATUS_WORD:
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ rv = set_page(client, page);
+ if (rv < 0)
+ return rv;
+
+ return i2c_smbus_read_word_data(client, reg);
+}
+
+struct pmbus_driver_info fsp3y_info[] = {
+ [ym2151e] = {
+ .pages = 2,
+ .func[YM2151_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
+ PMBUS_HAVE_FAN12,
+ .func[YM2151_PAGE_5VSB_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT,
+ PMBUS_HAVE_IIN,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ },
+ [yh5151e] = {
+ .pages = 3,
+ .func[YH5151E_PAGE_12V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3,
+ .func[YH5151E_PAGE_5V_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .func[YH5151E_PAGE_3V3_LOG] =
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_POUT,
+ .read_word_data = fsp3y_read_word_data,
+ .read_byte_data = fsp3y_read_byte_data,
+ }
+};
+
+static int fsp3y_detect(struct i2c_client *client)
+{
+ int rv;
+ u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
+
+ rv = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+ if (rv < 0)
+ return rv;
+
+ buf[rv] = '\0';
+
+ if (rv == 8) {
+ if (!strcmp(buf, "YM-2151E"))
+ return ym2151e;
+ else if (!strcmp(buf, "YH-5151E"))
+ return yh5151e;
+ }
+
+ dev_err(&client->dev, "Unsupported model %.*s\n", rv, buf);
+ return -ENODEV;
+}
+
+static const struct i2c_device_id fsp3y_id[] = {
+ {"ym2151e", ym2151e},
+ {"yh5151e", yh5151e},
+ {0}
+};
+
+static int fsp3y_probe(struct i2c_client *client)
+{
+ struct fsp3y_data *data;
+ const struct i2c_device_id *id;
+ int rv;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct fsp3y_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->chip = fsp3y_detect(client);
+ if (data->chip < 0)
+ return data->chip;
+
+ id = i2c_match_id(fsp3y_id, client);
+ if (data->chip != id->driver_data)
+ dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n",
+ id->name, (int)id->driver_data, data->chip);
+
+ rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+ if (rv < 0)
+ return rv;
+ data->page = rv;
+
+ data->info = fsp3y_info[data->chip];
+
+ return pmbus_do_probe(client, &data->info);
+}
+
+MODULE_DEVICE_TABLE(i2c, fsp3y_id);
+
+static struct i2c_driver fsp3y_driver = {
+ .driver = {
+ .name = "fsp3y",
+ },
+ .probe_new = fsp3y_probe,
+ .id_table = fsp3y_id
+};
+
+module_i2c_driver(fsp3y_driver);
+
+MODULE_AUTHOR("Václav Kubernát");
+MODULE_DESCRIPTION("PMBus driver for FSP/3Y-Power power supplies");
+MODULE_LICENSE("GPL");
--
2.31.1
st 14. 4. 2021 v 5:29 odesílatel Guenter Roeck <[email protected]> napsal:
>
> On Wed, Apr 14, 2021 at 02:13:06AM +0200, Václav Kubernát wrote:
> > This patch adds support for these devices:
> > - YH-5151E - the PDU
> > - YM-2151E - the PSU
> >
> > The device datasheet says that the devices support PMBus 1.2, but in my
> > testing, a lot of the commands aren't supported and if they are, they
> > sometimes behave strangely or inconsistently. For example, writes to the
> > PAGE command requires using PEC, otherwise the write won't work and the
> > page won't switch, even though, the standard says that PEC is optional.
> > On the other hand, writes to SMBALERT don't require PEC. Because of
> > this, the driver is mostly reverse engineered with the help of a tool
> > called pmbus_peek written by David Brownell (and later adopted by my
> > colleague Jan Kundrát).
> >
> > The device also has some sort of a timing issue when switching pages,
> > which is explained further in the code.
> >
> > Because of this, the driver support is limited. It exposes only the
> > values, that have been tested to work correctly.
> >
> > Signed-off-by: Václav Kubernát <[email protected]>
>
> checkpatch says:
>
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #108: FILE: Documentation/hwmon/fsp-3y.rst:1:
> +Kernel driver fsp3y
>
> WARNING: line length of 137 exceeds 100 columns
> #409: FILE: drivers/hwmon/pmbus/fsp-3y.c:225:
> + dev_warn(&client->dev, "Device mismatch: Configured %s (%d), detected %d\n", id->name, (int)id->driver_data, data->chip);
>
> Please fix and resubmit.
>
Done.
Václav
> Thanks,
> Guenter