| Message ID | 20211207071521.543-2-zev@bewilderbeest.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (pmbus) Add Delta AHE-50DC fan control module driver | expand |
On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: > This device is an integrated module of the Delta AHE-50DC Open19 power > shelf. For lack of proper documentation, this driver has been developed > referencing an existing (GPL) driver that was included in a code release > from LinkedIn [1]. It provides four fan speeds, four temperatures, and > one voltage reading, as well as a handful of warning and fault > indicators. > > [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c > Hmm, that reference isn't really accurate anymore. I think it would be better to just say that the device was found to be PMBus compliant. > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> > --- > MAINTAINERS | 6 ++ > drivers/hwmon/pmbus/Kconfig | 12 ++++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ > 4 files changed, 103 insertions(+) > create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0ac052200ecb..8bb7ba52d2f5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5425,6 +5425,12 @@ W: https://linuxtv.org > T: git git://linuxtv.org/media_tree.git > F: drivers/media/platform/sti/delta > > +DELTA AHE-50DC FAN CONTROL MODULE DRIVER > +M: Zev Weiss <zev@bewilderbeest.net> > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c > + > DELTA DPS920AB PSU DRIVER > M: Robert Marko <robert.marko@sartura.hr> > L: linux-hwmon@vger.kernel.org > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index ffb609cee3a4..937e1c2c11e7 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 > This driver can also be built as a module. If so, the module will > be called bpa-rs600. > > +config SENSORS_DELTA_AHE50DC_FAN > + tristate "Delta AHE-50DC fan control module" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get hardware monitoring support for > + the integrated fan control module of the Delta AHE-50DC > + Open19 power shelf. > + > + This driver can also be built as a module. If so, the module > + will be called delta-ahe50dc-fan. > + > config SENSORS_FSP_3Y > tristate "FSP/3Y-Power power supplies" > help > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index 0ed4d596a948..a56b2897288d 100644 > --- a/drivers/hwmon/pmbus/Makefile > +++ b/drivers/hwmon/pmbus/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o > obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o > obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o > +obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o > obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o > obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o > obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c > new file mode 100644 > index 000000000000..07b1e7c5f5f5 > --- /dev/null > +++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c > @@ -0,0 +1,84 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Delta AHE-50DC power shelf fan control module driver > + * > + * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/i2c.h> > +#include <linux/pmbus.h> Alphabetic include file order please. > + > +#include "pmbus.h" > + > +#define AHE50DC_PMBUS_READ_TEMP4 0xd0 > + > +static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) > +{ > + /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ > + if (page == 1) { > + if (reg == PMBUS_READ_TEMPERATURE_1) > + return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); > + return -EOPNOTSUPP; > + } > + return -ENODATA; > +} > + > +static struct pmbus_driver_info ahe50dc_fan_info = { > + .pages = 2, > + .format[PSC_FAN] = direct, > + .format[PSC_TEMPERATURE] = direct, > + .format[PSC_VOLTAGE_IN] = direct, > + .m[PSC_FAN] = 1, > + .b[PSC_FAN] = 0, > + .R[PSC_FAN] = 0, > + .m[PSC_TEMPERATURE] = 1, > + .b[PSC_TEMPERATURE] = 0, > + .R[PSC_TEMPERATURE] = 1, > + .m[PSC_VOLTAGE_IN] = 1, > + .b[PSC_VOLTAGE_IN] = 0, > + .R[PSC_VOLTAGE_IN] = 3, How did you determine the exponents ? The referenced driver doesn't seem to make a difference between voltage and temperature exponents (nor fan speed, which is a bit odd). > + .func[0] = PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | > + PMBUS_HAVE_VIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_FAN34 | > + PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_STATUS_FAN34 | PMBUS_PAGE_VIRTUAL, > + .func[1] = PMBUS_HAVE_TEMP | PMBUS_PAGE_VIRTUAL, > + .read_word_data = ahe50dc_fan_read_word_data, > +}; > + > +static struct pmbus_platform_data ahe50dc_fan_data = { > + .flags = PMBUS_NO_CAPABILITY, > +}; Is that necessary ? It should only be used if trying to read CAPABILITY reports bad data. Thanks, Guenter > + > +static int ahe50dc_fan_probe(struct i2c_client *client) > +{ > + client->dev.platform_data = &ahe50dc_fan_data; > + return pmbus_do_probe(client, &ahe50dc_fan_info); > +} > + > +static const struct i2c_device_id ahe50dc_fan_id[] = { > + { "ahe50dc_fan" }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, ahe50dc_fan_id); > + > +static const struct of_device_id __maybe_unused ahe50dc_fan_of_match[] = { > + { .compatible = "delta,ahe50dc-fan" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ahe50dc_fan_of_match); > + > +static struct i2c_driver ahe50dc_fan_driver = { > + .driver = { > + .name = "ahe50dc_fan", > + .of_match_table = of_match_ptr(ahe50dc_fan_of_match), > + }, > + .probe_new = ahe50dc_fan_probe, > + .id_table = ahe50dc_fan_id, > +}; > +module_i2c_driver(ahe50dc_fan_driver); > + > +MODULE_AUTHOR("Zev Weiss <zev@bewilderbeest.net>"); > +MODULE_DESCRIPTION("Driver for Delta AHE-50DC power shelf fan control module"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(PMBUS); > -- > 2.34.1 >
On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >> This device is an integrated module of the Delta AHE-50DC Open19 power >> shelf. For lack of proper documentation, this driver has been developed >> referencing an existing (GPL) driver that was included in a code release >> from LinkedIn [1]. It provides four fan speeds, four temperatures, and >> one voltage reading, as well as a handful of warning and fault >> indicators. >> >> [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >> > >Hmm, that reference isn't really accurate anymore. I think it would be >better to just say that the device was found to be PMBus compliant. Sure, will do. > >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >> --- >> MAINTAINERS | 6 ++ >> drivers/hwmon/pmbus/Kconfig | 12 ++++ >> drivers/hwmon/pmbus/Makefile | 1 + >> drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ >> 4 files changed, 103 insertions(+) >> create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 0ac052200ecb..8bb7ba52d2f5 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -5425,6 +5425,12 @@ W: https://linuxtv.org >> T: git git://linuxtv.org/media_tree.git >> F: drivers/media/platform/sti/delta >> >> +DELTA AHE-50DC FAN CONTROL MODULE DRIVER >> +M: Zev Weiss <zev@bewilderbeest.net> >> +L: linux-hwmon@vger.kernel.org >> +S: Maintained >> +F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c >> + >> DELTA DPS920AB PSU DRIVER >> M: Robert Marko <robert.marko@sartura.hr> >> L: linux-hwmon@vger.kernel.org >> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >> index ffb609cee3a4..937e1c2c11e7 100644 >> --- a/drivers/hwmon/pmbus/Kconfig >> +++ b/drivers/hwmon/pmbus/Kconfig >> @@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 >> This driver can also be built as a module. If so, the module will >> be called bpa-rs600. >> >> +config SENSORS_DELTA_AHE50DC_FAN >> + tristate "Delta AHE-50DC fan control module" >> + depends on I2C >> + select REGMAP_I2C And I realize I neglected to drop the depends & select lines here when moving this bit into the pmbus directory... >> + help >> + If you say yes here you get hardware monitoring support for >> + the integrated fan control module of the Delta AHE-50DC >> + Open19 power shelf. >> + >> + This driver can also be built as a module. If so, the module >> + will be called delta-ahe50dc-fan. >> + >> config SENSORS_FSP_3Y >> tristate "FSP/3Y-Power power supplies" >> help >> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >> index 0ed4d596a948..a56b2897288d 100644 >> --- a/drivers/hwmon/pmbus/Makefile >> +++ b/drivers/hwmon/pmbus/Makefile >> @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o >> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o >> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o >> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o >> +obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o >> obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o >> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o >> obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >> diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >> new file mode 100644 >> index 000000000000..07b1e7c5f5f5 >> --- /dev/null >> +++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >> @@ -0,0 +1,84 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Delta AHE-50DC power shelf fan control module driver >> + * >> + * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/i2c.h> >> +#include <linux/pmbus.h> > >Alphabetic include file order please. Ack, will fix. > >> + >> +#include "pmbus.h" >> + >> +#define AHE50DC_PMBUS_READ_TEMP4 0xd0 >> + >> +static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) >> +{ >> + /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ >> + if (page == 1) { >> + if (reg == PMBUS_READ_TEMPERATURE_1) >> + return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); >> + return -EOPNOTSUPP; >> + } >> + return -ENODATA; >> +} >> + >> +static struct pmbus_driver_info ahe50dc_fan_info = { >> + .pages = 2, >> + .format[PSC_FAN] = direct, >> + .format[PSC_TEMPERATURE] = direct, >> + .format[PSC_VOLTAGE_IN] = direct, >> + .m[PSC_FAN] = 1, >> + .b[PSC_FAN] = 0, >> + .R[PSC_FAN] = 0, >> + .m[PSC_TEMPERATURE] = 1, >> + .b[PSC_TEMPERATURE] = 0, >> + .R[PSC_TEMPERATURE] = 1, >> + .m[PSC_VOLTAGE_IN] = 1, >> + .b[PSC_VOLTAGE_IN] = 0, >> + .R[PSC_VOLTAGE_IN] = 3, > >How did you determine the exponents ? The referenced driver >doesn't seem to make a difference between voltage and temperature >exponents (nor fan speed, which is a bit odd). Lacking documentation, the honest answer here is that I just sort of eyeballed it. However, after doing so, I dug through the code dump a bit more and found some userspace unit-conversion bits that appear to confirm what I arrived at: https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/platform-lib/files/powershelf/powershelf_fan.c (Lines 107, and 153/161/169/177.) > >> + .func[0] = PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | >> + PMBUS_HAVE_VIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_FAN34 | >> + PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_STATUS_FAN34 | PMBUS_PAGE_VIRTUAL, >> + .func[1] = PMBUS_HAVE_TEMP | PMBUS_PAGE_VIRTUAL, >> + .read_word_data = ahe50dc_fan_read_word_data, >> +}; >> + >> +static struct pmbus_platform_data ahe50dc_fan_data = { >> + .flags = PMBUS_NO_CAPABILITY, >> +}; > >Is that necessary ? It should only be used if trying to read CAPABILITY >reports bad data. Unfortunately it does seem to be needed -- CAPABILITY returns 0xff, but despite having bit 7 set the device mangles all subsequent transactions by returning bad PEC bytes if we enable I2C_CLIENT_PEC. Actually, none of the fields in that CAPABILITY value seem to accurately reflect the device's observed behavior; I'm pretty sure 0xff is just what it returns when it doesn't support something (I think the bad PECs it sends are all 0xff too). I'll add an explanatory comment in v4. Thanks for the review! Zev
On 12/7/21 11:22 AM, Zev Weiss wrote: > On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >> On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>> This device is an integrated module of the Delta AHE-50DC Open19 power >>> shelf. For lack of proper documentation, this driver has been developed >>> referencing an existing (GPL) driver that was included in a code release >>> from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>> one voltage reading, as well as a handful of warning and fault >>> indicators. >>> >>> [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>> >> >> Hmm, that reference isn't really accurate anymore. I think it would be >> better to just say that the device was found to be PMBus compliant. > > Sure, will do. > >> >>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>> --- >>> MAINTAINERS | 6 ++ >>> drivers/hwmon/pmbus/Kconfig | 12 ++++ >>> drivers/hwmon/pmbus/Makefile | 1 + >>> drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ >>> 4 files changed, 103 insertions(+) >>> create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 0ac052200ecb..8bb7ba52d2f5 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -5425,6 +5425,12 @@ W: https://linuxtv.org >>> T: git git://linuxtv.org/media_tree.git >>> F: drivers/media/platform/sti/delta >>> >>> +DELTA AHE-50DC FAN CONTROL MODULE DRIVER >>> +M: Zev Weiss <zev@bewilderbeest.net> >>> +L: linux-hwmon@vger.kernel.org >>> +S: Maintained >>> +F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>> + >>> DELTA DPS920AB PSU DRIVER >>> M: Robert Marko <robert.marko@sartura.hr> >>> L: linux-hwmon@vger.kernel.org >>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>> index ffb609cee3a4..937e1c2c11e7 100644 >>> --- a/drivers/hwmon/pmbus/Kconfig >>> +++ b/drivers/hwmon/pmbus/Kconfig >>> @@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 >>> This driver can also be built as a module. If so, the module will >>> be called bpa-rs600. >>> >>> +config SENSORS_DELTA_AHE50DC_FAN >>> + tristate "Delta AHE-50DC fan control module" >>> + depends on I2C >>> + select REGMAP_I2C > > And I realize I neglected to drop the depends & select lines here when moving this bit into the pmbus directory... > >>> + help >>> + If you say yes here you get hardware monitoring support for >>> + the integrated fan control module of the Delta AHE-50DC >>> + Open19 power shelf. >>> + >>> + This driver can also be built as a module. If so, the module >>> + will be called delta-ahe50dc-fan. >>> + >>> config SENSORS_FSP_3Y >>> tristate "FSP/3Y-Power power supplies" >>> help >>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>> index 0ed4d596a948..a56b2897288d 100644 >>> --- a/drivers/hwmon/pmbus/Makefile >>> +++ b/drivers/hwmon/pmbus/Makefile >>> @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o >>> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o >>> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o >>> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o >>> +obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o >>> obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o >>> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o >>> obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>> diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>> new file mode 100644 >>> index 000000000000..07b1e7c5f5f5 >>> --- /dev/null >>> +++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>> @@ -0,0 +1,84 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Delta AHE-50DC power shelf fan control module driver >>> + * >>> + * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> >>> + */ >>> + >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/i2c.h> >>> +#include <linux/pmbus.h> >> >> Alphabetic include file order please. > > Ack, will fix. > >> >>> + >>> +#include "pmbus.h" >>> + >>> +#define AHE50DC_PMBUS_READ_TEMP4 0xd0 >>> + >>> +static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) >>> +{ >>> + /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ >>> + if (page == 1) { >>> + if (reg == PMBUS_READ_TEMPERATURE_1) >>> + return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); >>> + return -EOPNOTSUPP; >>> + } >>> + return -ENODATA; >>> +} >>> + >>> +static struct pmbus_driver_info ahe50dc_fan_info = { >>> + .pages = 2, >>> + .format[PSC_FAN] = direct, >>> + .format[PSC_TEMPERATURE] = direct, >>> + .format[PSC_VOLTAGE_IN] = direct, >>> + .m[PSC_FAN] = 1, >>> + .b[PSC_FAN] = 0, >>> + .R[PSC_FAN] = 0, >>> + .m[PSC_TEMPERATURE] = 1, >>> + .b[PSC_TEMPERATURE] = 0, >>> + .R[PSC_TEMPERATURE] = 1, >>> + .m[PSC_VOLTAGE_IN] = 1, >>> + .b[PSC_VOLTAGE_IN] = 0, >>> + .R[PSC_VOLTAGE_IN] = 3, >> >> How did you determine the exponents ? The referenced driver >> doesn't seem to make a difference between voltage and temperature >> exponents (nor fan speed, which is a bit odd). > > Lacking documentation, the honest answer here is that I just sort of eyeballed it. However, after doing so, I dug through the code dump a bit more and found some userspace unit-conversion bits that appear to confirm what I arrived at: > > https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/platform-lib/files/powershelf/powershelf_fan.c > > (Lines 107, and 153/161/169/177.) > Brr. Well, why use a standard API/ABI if one can invent a non-standard one. Can you check this with real hardware, by any chance ? >> >>> + .func[0] = PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | >>> + PMBUS_HAVE_VIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_FAN34 | >>> + PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_STATUS_FAN34 | PMBUS_PAGE_VIRTUAL, >>> + .func[1] = PMBUS_HAVE_TEMP | PMBUS_PAGE_VIRTUAL, >>> + .read_word_data = ahe50dc_fan_read_word_data, >>> +}; >>> + >>> +static struct pmbus_platform_data ahe50dc_fan_data = { >>> + .flags = PMBUS_NO_CAPABILITY, >>> +}; >> >> Is that necessary ? It should only be used if trying to read CAPABILITY >> reports bad data. > > Unfortunately it does seem to be needed -- CAPABILITY returns 0xff, but despite having bit 7 set the device mangles all subsequent transactions by returning bad PEC bytes if we enable I2C_CLIENT_PEC. Actually, none of the fields in that CAPABILITY value seem to accurately reflect the device's observed behavior; I'm pretty sure 0xff is just what it returns when it doesn't support something (I think the bad PECs it sends are all 0xff too). > > I'll add an explanatory comment in v4. > > Please do. Thanks, Guenter
On 12/7/21 11:22 AM, Zev Weiss wrote: > On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >> On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>> This device is an integrated module of the Delta AHE-50DC Open19 power >>> shelf. For lack of proper documentation, this driver has been developed >>> referencing an existing (GPL) driver that was included in a code release >>> from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>> one voltage reading, as well as a handful of warning and fault >>> indicators. >>> >>> [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>> >> >> Hmm, that reference isn't really accurate anymore. I think it would be >> better to just say that the device was found to be PMBus compliant. > > Sure, will do. > Makes me wonder: How do you know that the referenced driver is for Delta AHE-50DC ? Guenter
On Tue, Dec 07, 2021 at 11:44:01AM PST, Guenter Roeck wrote: >On 12/7/21 11:22 AM, Zev Weiss wrote: >>On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >>>On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>>>This device is an integrated module of the Delta AHE-50DC Open19 power >>>>shelf. For lack of proper documentation, this driver has been developed >>>>referencing an existing (GPL) driver that was included in a code release >>>>from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>>>one voltage reading, as well as a handful of warning and fault >>>>indicators. >>>> >>>>[1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>>> >>> >>>Hmm, that reference isn't really accurate anymore. I think it would be >>>better to just say that the device was found to be PMBus compliant. >> >>Sure, will do. >> > > Makes me wonder: How do you know that the referenced driver is for > Delta AHE-50DC ? We'd been waiting for the source code for the software it ships with for a while, and were finally provided with that repo; everything I've observed from the factory software is consistent with the code in that driver. A sampling: # no modinfo command available, but... root@bmc-oob:~# strings -n8 /lib/modules/4.1.51-deltapower/extra/fancontrol.ko | head fancontrol license=GPL description=FANCTRL Driver author=Ping Mao <pmao@linkedin.com> alias=i2c:fancontrol depends=i2c_dev_sysfs vermagic=4.1.51-deltapower mod_unload ARMv5 p2v8 fancontrol status_word bit 2: Temparature Warning root@bmc-oob:~# cd /sys/bus/i2c/drivers/fancontrol/8-0030 root@bmc-oob:/sys/bus/i2c/drivers/fancontrol/8-0030# grep . fan* temp* vin fan1_2_status:0 fan1_speed:7860 fan2_speed:7860 fan3_4_status:0 fan3_speed:7680 fan4_speed:7560 temperature1:292 temperature2:278 temperature3:286 temperature4:302 vin:12251 >>> >>>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>>>--- >>>> MAINTAINERS | 6 ++ >>>> drivers/hwmon/pmbus/Kconfig | 12 ++++ >>>> drivers/hwmon/pmbus/Makefile | 1 + >>>> drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ >>>> 4 files changed, 103 insertions(+) >>>> create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>> >>>>diff --git a/MAINTAINERS b/MAINTAINERS >>>>index 0ac052200ecb..8bb7ba52d2f5 100644 >>>>--- a/MAINTAINERS >>>>+++ b/MAINTAINERS >>>>@@ -5425,6 +5425,12 @@ W: https://linuxtv.org >>>> T: git git://linuxtv.org/media_tree.git >>>> F: drivers/media/platform/sti/delta >>>> >>>>+DELTA AHE-50DC FAN CONTROL MODULE DRIVER >>>>+M: Zev Weiss <zev@bewilderbeest.net> >>>>+L: linux-hwmon@vger.kernel.org >>>>+S: Maintained >>>>+F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>+ >>>> DELTA DPS920AB PSU DRIVER >>>> M: Robert Marko <robert.marko@sartura.hr> >>>> L: linux-hwmon@vger.kernel.org >>>>diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>index ffb609cee3a4..937e1c2c11e7 100644 >>>>--- a/drivers/hwmon/pmbus/Kconfig >>>>+++ b/drivers/hwmon/pmbus/Kconfig >>>>@@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 >>>> This driver can also be built as a module. If so, the module will >>>> be called bpa-rs600. >>>> >>>>+config SENSORS_DELTA_AHE50DC_FAN >>>>+ tristate "Delta AHE-50DC fan control module" >>>>+ depends on I2C >>>>+ select REGMAP_I2C >> >>And I realize I neglected to drop the depends & select lines here when moving this bit into the pmbus directory... >> >>>>+ help >>>>+ If you say yes here you get hardware monitoring support for >>>>+ the integrated fan control module of the Delta AHE-50DC >>>>+ Open19 power shelf. >>>>+ >>>>+ This driver can also be built as a module. If so, the module >>>>+ will be called delta-ahe50dc-fan. >>>>+ >>>> config SENSORS_FSP_3Y >>>> tristate "FSP/3Y-Power power supplies" >>>> help >>>>diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>index 0ed4d596a948..a56b2897288d 100644 >>>>--- a/drivers/hwmon/pmbus/Makefile >>>>+++ b/drivers/hwmon/pmbus/Makefile >>>>@@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o >>>> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o >>>> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o >>>> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o >>>>+obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o >>>> obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o >>>> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o >>>> obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>new file mode 100644 >>>>index 000000000000..07b1e7c5f5f5 >>>>--- /dev/null >>>>+++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>@@ -0,0 +1,84 @@ >>>>+// SPDX-License-Identifier: GPL-2.0 >>>>+/* >>>>+ * Delta AHE-50DC power shelf fan control module driver >>>>+ * >>>>+ * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> >>>>+ */ >>>>+ >>>>+#include <linux/kernel.h> >>>>+#include <linux/module.h> >>>>+#include <linux/i2c.h> >>>>+#include <linux/pmbus.h> >>> >>>Alphabetic include file order please. >> >>Ack, will fix. >> >>> >>>>+ >>>>+#include "pmbus.h" >>>>+ >>>>+#define AHE50DC_PMBUS_READ_TEMP4 0xd0 >>>>+ >>>>+static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) >>>>+{ >>>>+ /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ >>>>+ if (page == 1) { >>>>+ if (reg == PMBUS_READ_TEMPERATURE_1) >>>>+ return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); >>>>+ return -EOPNOTSUPP; >>>>+ } >>>>+ return -ENODATA; >>>>+} >>>>+ >>>>+static struct pmbus_driver_info ahe50dc_fan_info = { >>>>+ .pages = 2, >>>>+ .format[PSC_FAN] = direct, >>>>+ .format[PSC_TEMPERATURE] = direct, >>>>+ .format[PSC_VOLTAGE_IN] = direct, >>>>+ .m[PSC_FAN] = 1, >>>>+ .b[PSC_FAN] = 0, >>>>+ .R[PSC_FAN] = 0, >>>>+ .m[PSC_TEMPERATURE] = 1, >>>>+ .b[PSC_TEMPERATURE] = 0, >>>>+ .R[PSC_TEMPERATURE] = 1, >>>>+ .m[PSC_VOLTAGE_IN] = 1, >>>>+ .b[PSC_VOLTAGE_IN] = 0, >>>>+ .R[PSC_VOLTAGE_IN] = 3, >>> >>>How did you determine the exponents ? The referenced driver >>>doesn't seem to make a difference between voltage and temperature >>>exponents (nor fan speed, which is a bit odd). >> >>Lacking documentation, the honest answer here is that I just sort of eyeballed it. However, after doing so, I dug through the code dump a bit more and found some userspace unit-conversion bits that appear to confirm what I arrived at: >> >>https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/platform-lib/files/powershelf/powershelf_fan.c >> >>(Lines 107, and 153/161/169/177.) >> > >Brr. Well, why use a standard API/ABI if one can invent a non-standard one. > It's...not the only bit of gratuitous wheel-reinvention we've come across in this particular system. >Can you check this with real hardware, by any chance ? > If you mean running that code on it, yes -- here's the userspace utility that invokes that library routine: root@bmc-oob:~# fan-util.sh fan1 speed: 7860 RPM fan2 speed: 7860 RPM fan3 speed: 7620 RPM fan4 speed: 7560 RPM temperature1: 29.20 C temperature2: 27.80 C temperature3: 28.50 C temperature4: 30.20 C vin_undervolt_fault: no overtemperature_warning: no fan_fault: no fan_warning: no fan_status: ok That fan-util.sh script being this: https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/liutils/files/fan-util.sh which invokes this: https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/liutils/files/fan-ctrl/fan-util.c (which calls into the routine in powershelf_fan.c linked above). Zev
On Tue, Dec 07, 2021 at 01:53:44PM PST, Zev Weiss wrote: >On Tue, Dec 07, 2021 at 11:44:01AM PST, Guenter Roeck wrote: >>On 12/7/21 11:22 AM, Zev Weiss wrote: >>>On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >>>>On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>>>>This device is an integrated module of the Delta AHE-50DC Open19 power >>>>>shelf. For lack of proper documentation, this driver has been developed >>>>>referencing an existing (GPL) driver that was included in a code release >>>>>from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>>>>one voltage reading, as well as a handful of warning and fault >>>>>indicators. >>>>> >>>>>[1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>>>> >>>> >>>>Hmm, that reference isn't really accurate anymore. I think it would be >>>>better to just say that the device was found to be PMBus compliant. >>> >>>Sure, will do. >>> >> >>Makes me wonder: How do you know that the referenced driver is for >>Delta AHE-50DC ? > >We'd been waiting for the source code for the software it ships with >for a while, and were finally provided with that repo; everything I've >observed from the factory software is consistent with the code in that >driver. A sampling: > > # no modinfo command available, but... > root@bmc-oob:~# strings -n8 /lib/modules/4.1.51-deltapower/extra/fancontrol.ko | head > fancontrol > license=GPL > description=FANCTRL Driver > author=Ping Mao <pmao@linkedin.com> > alias=i2c:fancontrol > depends=i2c_dev_sysfs > vermagic=4.1.51-deltapower mod_unload ARMv5 p2v8 > fancontrol > status_word > bit 2: Temparature Warning > root@bmc-oob:~# cd /sys/bus/i2c/drivers/fancontrol/8-0030 > root@bmc-oob:/sys/bus/i2c/drivers/fancontrol/8-0030# grep . fan* temp* vin > fan1_2_status:0 > fan1_speed:7860 > fan2_speed:7860 > fan3_4_status:0 > fan3_speed:7680 > fan4_speed:7560 > temperature1:292 > temperature2:278 > temperature3:286 > temperature4:302 > vin:12251 > > >>>> >>>>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>>>>--- >>>>> MAINTAINERS | 6 ++ >>>>> drivers/hwmon/pmbus/Kconfig | 12 ++++ >>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>> drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ >>>>> 4 files changed, 103 insertions(+) >>>>> create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>> >>>>>diff --git a/MAINTAINERS b/MAINTAINERS >>>>>index 0ac052200ecb..8bb7ba52d2f5 100644 >>>>>--- a/MAINTAINERS >>>>>+++ b/MAINTAINERS >>>>>@@ -5425,6 +5425,12 @@ W: https://linuxtv.org >>>>> T: git git://linuxtv.org/media_tree.git >>>>> F: drivers/media/platform/sti/delta >>>>> >>>>>+DELTA AHE-50DC FAN CONTROL MODULE DRIVER >>>>>+M: Zev Weiss <zev@bewilderbeest.net> >>>>>+L: linux-hwmon@vger.kernel.org >>>>>+S: Maintained >>>>>+F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>>+ >>>>> DELTA DPS920AB PSU DRIVER >>>>> M: Robert Marko <robert.marko@sartura.hr> >>>>> L: linux-hwmon@vger.kernel.org >>>>>diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>>index ffb609cee3a4..937e1c2c11e7 100644 >>>>>--- a/drivers/hwmon/pmbus/Kconfig >>>>>+++ b/drivers/hwmon/pmbus/Kconfig >>>>>@@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 >>>>> This driver can also be built as a module. If so, the module will >>>>> be called bpa-rs600. >>>>> >>>>>+config SENSORS_DELTA_AHE50DC_FAN >>>>>+ tristate "Delta AHE-50DC fan control module" >>>>>+ depends on I2C >>>>>+ select REGMAP_I2C >>> >>>And I realize I neglected to drop the depends & select lines here when moving this bit into the pmbus directory... >>> >>>>>+ help >>>>>+ If you say yes here you get hardware monitoring support for >>>>>+ the integrated fan control module of the Delta AHE-50DC >>>>>+ Open19 power shelf. >>>>>+ >>>>>+ This driver can also be built as a module. If so, the module >>>>>+ will be called delta-ahe50dc-fan. >>>>>+ >>>>> config SENSORS_FSP_3Y >>>>> tristate "FSP/3Y-Power power supplies" >>>>> help >>>>>diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>>index 0ed4d596a948..a56b2897288d 100644 >>>>>--- a/drivers/hwmon/pmbus/Makefile >>>>>+++ b/drivers/hwmon/pmbus/Makefile >>>>>@@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o >>>>> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o >>>>> obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o >>>>> obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o >>>>>+obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o >>>>> obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o >>>>> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o >>>>> obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>>diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>>new file mode 100644 >>>>>index 000000000000..07b1e7c5f5f5 >>>>>--- /dev/null >>>>>+++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c >>>>>@@ -0,0 +1,84 @@ >>>>>+// SPDX-License-Identifier: GPL-2.0 >>>>>+/* >>>>>+ * Delta AHE-50DC power shelf fan control module driver >>>>>+ * >>>>>+ * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> >>>>>+ */ >>>>>+ >>>>>+#include <linux/kernel.h> >>>>>+#include <linux/module.h> >>>>>+#include <linux/i2c.h> >>>>>+#include <linux/pmbus.h> >>>> >>>>Alphabetic include file order please. >>> >>>Ack, will fix. >>> >>>> >>>>>+ >>>>>+#include "pmbus.h" >>>>>+ >>>>>+#define AHE50DC_PMBUS_READ_TEMP4 0xd0 >>>>>+ >>>>>+static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) >>>>>+{ >>>>>+ /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ >>>>>+ if (page == 1) { >>>>>+ if (reg == PMBUS_READ_TEMPERATURE_1) >>>>>+ return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); >>>>>+ return -EOPNOTSUPP; >>>>>+ } >>>>>+ return -ENODATA; >>>>>+} >>>>>+ >>>>>+static struct pmbus_driver_info ahe50dc_fan_info = { >>>>>+ .pages = 2, >>>>>+ .format[PSC_FAN] = direct, >>>>>+ .format[PSC_TEMPERATURE] = direct, >>>>>+ .format[PSC_VOLTAGE_IN] = direct, >>>>>+ .m[PSC_FAN] = 1, >>>>>+ .b[PSC_FAN] = 0, >>>>>+ .R[PSC_FAN] = 0, >>>>>+ .m[PSC_TEMPERATURE] = 1, >>>>>+ .b[PSC_TEMPERATURE] = 0, >>>>>+ .R[PSC_TEMPERATURE] = 1, >>>>>+ .m[PSC_VOLTAGE_IN] = 1, >>>>>+ .b[PSC_VOLTAGE_IN] = 0, >>>>>+ .R[PSC_VOLTAGE_IN] = 3, >>>> >>>>How did you determine the exponents ? The referenced driver >>>>doesn't seem to make a difference between voltage and temperature >>>>exponents (nor fan speed, which is a bit odd). >>> >>>Lacking documentation, the honest answer here is that I just sort of eyeballed it. However, after doing so, I dug through the code dump a bit more and found some userspace unit-conversion bits that appear to confirm what I arrived at: >>> >>>https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/platform-lib/files/powershelf/powershelf_fan.c >>> >>>(Lines 107, and 153/161/169/177.) >>> >> >>Brr. Well, why use a standard API/ABI if one can invent a non-standard one. >> > >It's...not the only bit of gratuitous wheel-reinvention we've come >across in this particular system. > >>Can you check this with real hardware, by any chance ? >> > >If you mean running that code on it, yes -- here's the userspace >utility that invokes that library routine: > > root@bmc-oob:~# fan-util.sh > fan1 speed: 7860 RPM > fan2 speed: 7860 RPM > fan3 speed: 7620 RPM > fan4 speed: 7560 RPM > temperature1: 29.20 C > temperature2: 27.80 C > temperature3: 28.50 C > temperature4: 30.20 C > vin_undervolt_fault: no > overtemperature_warning: no > fan_fault: no > fan_warning: no > fan_status: ok > >That fan-util.sh script being this: > >https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/liutils/files/fan-util.sh > >which invokes this: > >https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-deltapower/liutils/files/fan-ctrl/fan-util.c > >(which calls into the routine in powershelf_fan.c linked above). > ...though I think I accidentally glossed over some additional steps in beteween there; looks like there's actually another daemon involved that does the read from the sysfs files and the unit normalization and then drops the resulting data into a file that fan-util.c then reads: https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-core/health-mon/files/healthmon.c Anyway, the upshot is that I'm fairly confident that the code there is in fact what's running on the device as shipped from the factory, and I think the coefficients I determined for the pmbus driver should handle it appropriately. (It produces readings that align with what the factory software gives and are generally sane-looking.) Zev
On 12/7/21 1:53 PM, Zev Weiss wrote: > On Tue, Dec 07, 2021 at 11:44:01AM PST, Guenter Roeck wrote: >> On 12/7/21 11:22 AM, Zev Weiss wrote: >>> On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >>>> On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>>>> This device is an integrated module of the Delta AHE-50DC Open19 power >>>>> shelf. For lack of proper documentation, this driver has been developed >>>>> referencing an existing (GPL) driver that was included in a code release >>>>> from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>>>> one voltage reading, as well as a handful of warning and fault >>>>> indicators. >>>>> >>>>> [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>>>> >>>> >>>> Hmm, that reference isn't really accurate anymore. I think it would be >>>> better to just say that the device was found to be PMBus compliant. >>> >>> Sure, will do. >>> >> >> Makes me wonder: How do you know that the referenced driver is for Delta AHE-50DC ? > > We'd been waiting for the source code for the software it ships with for a while, and were finally provided with that repo; everything I've observed from the factory software is consistent with the code in that driver. A sampling: > I assume you mean "Delta AHE-50DC" when you refer to "it". [ ... ] >> Can you check this with real hardware, by any chance ? >> > > If you mean running that code on it, yes -- here's the userspace utility that invokes that library routine: > > root@bmc-oob:~# fan-util.sh > fan1 speed: 7860 RPM > fan2 speed: 7860 RPM > fan3 speed: 7620 RPM > fan4 speed: 7560 RPM > temperature1: 29.20 C > temperature2: 27.80 C > temperature3: 28.50 C > temperature4: 30.20 C > vin_undervolt_fault: no > overtemperature_warning: no > fan_fault: no > fan_warning: no > fan_status: ok > That doesn't really tell me anything in the context of the driver you submitted. Would it be possible to install your driver and provide the output from the "sensors" command ? It should match the information from the proprietary driver/tool. Thanks, Guenter
On Tue, Dec 07, 2021 at 03:15:18PM PST, Guenter Roeck wrote: >On 12/7/21 1:53 PM, Zev Weiss wrote: >>On Tue, Dec 07, 2021 at 11:44:01AM PST, Guenter Roeck wrote: >>>On 12/7/21 11:22 AM, Zev Weiss wrote: >>>>On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >>>>>On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>>>>>This device is an integrated module of the Delta AHE-50DC Open19 power >>>>>>shelf. For lack of proper documentation, this driver has been developed >>>>>>referencing an existing (GPL) driver that was included in a code release >>>>>>from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>>>>>one voltage reading, as well as a handful of warning and fault >>>>>>indicators. >>>>>> >>>>>>[1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>>>>> >>>>> >>>>>Hmm, that reference isn't really accurate anymore. I think it would be >>>>>better to just say that the device was found to be PMBus compliant. >>>> >>>>Sure, will do. >>>> >>> >>>Makes me wonder: How do you know that the referenced driver is for Delta AHE-50DC ? >> >>We'd been waiting for the source code for the software it ships with for a while, and were finally provided with that repo; everything I've observed from the factory software is consistent with the code in that driver. A sampling: >> > >I assume you mean "Delta AHE-50DC" when you refer to "it". > Yes. >[ ... ] >>>Can you check this with real hardware, by any chance ? >>> >> >>If you mean running that code on it, yes -- here's the userspace utility that invokes that library routine: >> >> root@bmc-oob:~# fan-util.sh >> fan1 speed: 7860 RPM >> fan2 speed: 7860 RPM >> fan3 speed: 7620 RPM >> fan4 speed: 7560 RPM >> temperature1: 29.20 C >> temperature2: 27.80 C >> temperature3: 28.50 C >> temperature4: 30.20 C >> vin_undervolt_fault: no >> overtemperature_warning: no >> fan_fault: no >> fan_warning: no >> fan_status: ok >> > >That doesn't really tell me anything in the context of the driver you submitted. >Would it be possible to install your driver and provide the output from the >"sensors" command ? It should match the information from the proprietary >driver/tool. > Thanks, in doing so I realized I'd neglected to prevent reads from unsupported registers in the read_word_data function, which was leading to the driver producing sysfs files for meaningless sensor limits that the device doesn't actually support. With that fix (which I'll include in v4): root@ahe-50dc:~# /tmp/sensors 'ahe50dc_fan-*' ahe50dc_fan-i2c-28-30 Adapter: i2c-8-mux (chan_id 0) vin: 12.29 V fan1: 7680 RPM fan2: 7860 RPM fan3: 7680 RPM fan4: 7380 RPM temp1: +27.8 C temp2: +23.4 C temp3: +25.3 C temp4: +24.5 C Zev
On 12/7/21 4:41 PM, Zev Weiss wrote: > On Tue, Dec 07, 2021 at 03:15:18PM PST, Guenter Roeck wrote: >> On 12/7/21 1:53 PM, Zev Weiss wrote: >>> On Tue, Dec 07, 2021 at 11:44:01AM PST, Guenter Roeck wrote: >>>> On 12/7/21 11:22 AM, Zev Weiss wrote: >>>>> On Tue, Dec 07, 2021 at 09:50:15AM PST, Guenter Roeck wrote: >>>>>> On Mon, Dec 06, 2021 at 11:15:20PM -0800, Zev Weiss wrote: >>>>>>> This device is an integrated module of the Delta AHE-50DC Open19 power >>>>>>> shelf. For lack of proper documentation, this driver has been developed >>>>>>> referencing an existing (GPL) driver that was included in a code release >>>>>>> from LinkedIn [1]. It provides four fan speeds, four temperatures, and >>>>>>> one voltage reading, as well as a handful of warning and fault >>>>>>> indicators. >>>>>>> >>>>>>> [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c >>>>>>> >>>>>> >>>>>> Hmm, that reference isn't really accurate anymore. I think it would be >>>>>> better to just say that the device was found to be PMBus compliant. >>>>> >>>>> Sure, will do. >>>>> >>>> >>>> Makes me wonder: How do you know that the referenced driver is for Delta AHE-50DC ? >>> >>> We'd been waiting for the source code for the software it ships with for a while, and were finally provided with that repo; everything I've observed from the factory software is consistent with the code in that driver. A sampling: >>> >> >> I assume you mean "Delta AHE-50DC" when you refer to "it". >> > > Yes. > >> [ ... ] >>>> Can you check this with real hardware, by any chance ? >>>> >>> >>> If you mean running that code on it, yes -- here's the userspace utility that invokes that library routine: >>> >>> root@bmc-oob:~# fan-util.sh >>> fan1 speed: 7860 RPM >>> fan2 speed: 7860 RPM >>> fan3 speed: 7620 RPM >>> fan4 speed: 7560 RPM >>> temperature1: 29.20 C >>> temperature2: 27.80 C >>> temperature3: 28.50 C >>> temperature4: 30.20 C >>> vin_undervolt_fault: no >>> overtemperature_warning: no >>> fan_fault: no >>> fan_warning: no >>> fan_status: ok >>> >> >> That doesn't really tell me anything in the context of the driver you submitted. >> Would it be possible to install your driver and provide the output from the >> "sensors" command ? It should match the information from the proprietary >> driver/tool. >> > > Thanks, in doing so I realized I'd neglected to prevent reads from unsupported registers in the read_word_data function, which was leading to the driver producing sysfs files for meaningless sensor limits that the device doesn't actually support. With that fix (which I'll include in v4): > What a surprise. See drivers/hwmon/pmbus/dps920ab.c; that seems to be a common theme for devices from Delta. I would suggest to add a similar comment. Guenter
diff --git a/MAINTAINERS b/MAINTAINERS index 0ac052200ecb..8bb7ba52d2f5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5425,6 +5425,12 @@ W: https://linuxtv.org T: git git://linuxtv.org/media_tree.git F: drivers/media/platform/sti/delta +DELTA AHE-50DC FAN CONTROL MODULE DRIVER +M: Zev Weiss <zev@bewilderbeest.net> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: drivers/hwmon/pmbus/delta-ahe50dc-fan.c + DELTA DPS920AB PSU DRIVER M: Robert Marko <robert.marko@sartura.hr> L: linux-hwmon@vger.kernel.org diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index ffb609cee3a4..937e1c2c11e7 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -66,6 +66,18 @@ config SENSORS_BPA_RS600 This driver can also be built as a module. If so, the module will be called bpa-rs600. +config SENSORS_DELTA_AHE50DC_FAN + tristate "Delta AHE-50DC fan control module" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get hardware monitoring support for + the integrated fan control module of the Delta AHE-50DC + Open19 power shelf. + + This driver can also be built as a module. If so, the module + will be called delta-ahe50dc-fan. + config SENSORS_FSP_3Y tristate "FSP/3Y-Power power supplies" help diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 0ed4d596a948..a56b2897288d 100644 --- a/drivers/hwmon/pmbus/Makefile +++ b/drivers/hwmon/pmbus/Makefile @@ -9,6 +9,7 @@ obj-$(CONFIG_SENSORS_ADM1266) += adm1266.o obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o obj-$(CONFIG_SENSORS_BEL_PFE) += bel-pfe.o obj-$(CONFIG_SENSORS_BPA_RS600) += bpa-rs600.o +obj-$(CONFIG_SENSORS_DELTA_AHE50DC_FAN) += delta-ahe50dc-fan.o obj-$(CONFIG_SENSORS_FSP_3Y) += fsp-3y.o obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o diff --git a/drivers/hwmon/pmbus/delta-ahe50dc-fan.c b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c new file mode 100644 index 000000000000..07b1e7c5f5f5 --- /dev/null +++ b/drivers/hwmon/pmbus/delta-ahe50dc-fan.c @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Delta AHE-50DC power shelf fan control module driver + * + * Copyright 2021 Zev Weiss <zev@bewilderbeest.net> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/pmbus.h> + +#include "pmbus.h" + +#define AHE50DC_PMBUS_READ_TEMP4 0xd0 + +static int ahe50dc_fan_read_word_data(struct i2c_client *client, int page, int phase, int reg) +{ + /* temp1 in (virtual) page 1 is remapped to mfr-specific temp4 */ + if (page == 1) { + if (reg == PMBUS_READ_TEMPERATURE_1) + return i2c_smbus_read_word_data(client, AHE50DC_PMBUS_READ_TEMP4); + return -EOPNOTSUPP; + } + return -ENODATA; +} + +static struct pmbus_driver_info ahe50dc_fan_info = { + .pages = 2, + .format[PSC_FAN] = direct, + .format[PSC_TEMPERATURE] = direct, + .format[PSC_VOLTAGE_IN] = direct, + .m[PSC_FAN] = 1, + .b[PSC_FAN] = 0, + .R[PSC_FAN] = 0, + .m[PSC_TEMPERATURE] = 1, + .b[PSC_TEMPERATURE] = 0, + .R[PSC_TEMPERATURE] = 1, + .m[PSC_VOLTAGE_IN] = 1, + .b[PSC_VOLTAGE_IN] = 0, + .R[PSC_VOLTAGE_IN] = 3, + .func[0] = PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | + PMBUS_HAVE_VIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_FAN34 | + PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_STATUS_FAN34 | PMBUS_PAGE_VIRTUAL, + .func[1] = PMBUS_HAVE_TEMP | PMBUS_PAGE_VIRTUAL, + .read_word_data = ahe50dc_fan_read_word_data, +}; + +static struct pmbus_platform_data ahe50dc_fan_data = { + .flags = PMBUS_NO_CAPABILITY, +}; + +static int ahe50dc_fan_probe(struct i2c_client *client) +{ + client->dev.platform_data = &ahe50dc_fan_data; + return pmbus_do_probe(client, &ahe50dc_fan_info); +} + +static const struct i2c_device_id ahe50dc_fan_id[] = { + { "ahe50dc_fan" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, ahe50dc_fan_id); + +static const struct of_device_id __maybe_unused ahe50dc_fan_of_match[] = { + { .compatible = "delta,ahe50dc-fan" }, + { } +}; +MODULE_DEVICE_TABLE(of, ahe50dc_fan_of_match); + +static struct i2c_driver ahe50dc_fan_driver = { + .driver = { + .name = "ahe50dc_fan", + .of_match_table = of_match_ptr(ahe50dc_fan_of_match), + }, + .probe_new = ahe50dc_fan_probe, + .id_table = ahe50dc_fan_id, +}; +module_i2c_driver(ahe50dc_fan_driver); + +MODULE_AUTHOR("Zev Weiss <zev@bewilderbeest.net>"); +MODULE_DESCRIPTION("Driver for Delta AHE-50DC power shelf fan control module"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(PMBUS);
This device is an integrated module of the Delta AHE-50DC Open19 power shelf. For lack of proper documentation, this driver has been developed referencing an existing (GPL) driver that was included in a code release from LinkedIn [1]. It provides four fan speeds, four temperatures, and one voltage reading, as well as a handful of warning and fault indicators. [1] https://github.com/linkedin/o19-bmc-firmware/blob/master/meta-openbmc/meta-linkedin/meta-deltapower/recipes-kernel/fancontrol-mod/files/fancontrol.c Signed-off-by: Zev Weiss <zev@bewilderbeest.net> --- MAINTAINERS | 6 ++ drivers/hwmon/pmbus/Kconfig | 12 ++++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/delta-ahe50dc-fan.c | 84 +++++++++++++++++++++++++ 4 files changed, 103 insertions(+) create mode 100644 drivers/hwmon/pmbus/delta-ahe50dc-fan.c