| Message ID | 20210430132735.127342-1-robert.marko@sartura.hr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU | expand |
On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > This adds support for the Delta DPS-920AB PSU. > > Only missing feature is fan control which the PSU supports. > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > --- > Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > Documentation/hwmon/index.rst | 1 + > drivers/hwmon/pmbus/Kconfig | 9 ++++ > drivers/hwmon/pmbus/Makefile | 1 + > drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > 5 files changed, 154 insertions(+) > create mode 100644 Documentation/hwmon/dps920ab.rst > create mode 100644 drivers/hwmon/pmbus/dps920ab.c > > diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > new file mode 100644 > index 000000000000..df0aef530c7e > --- /dev/null > +++ b/Documentation/hwmon/dps920ab.rst > @@ -0,0 +1,80 @@ > +.. SPDX-License-Identifier: GPL-2.0-or-later > + > +Kernel driver dps920ab > +======================== > + > +Supported chips: > + > + * Delta DPS920AB > + > + Prefix: 'dps920ab' > + > + Addresses scanned: - > + > +Authors: > + Robert Marko <robert.marko@sartura.hr> > + > + > +Description > +----------- > + > +This driver implements support for Delta DPS920AB 920W 54V DC single output > +power supply with PMBus support. > + > +The driver is a client driver to the core PMBus driver. > +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > + > + > +Usage Notes > +----------- > + > +This driver does not auto-detect devices. You will have to instantiate the > +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > +details. > + > + > +Sysfs entries > +------------- > + > +======================= ====================================================== > +curr1_label "iin" > +curr1_input Measured input current > +curr1_crit Critical maximum current > +curr1_crit_alarm Current critical high alarm > + > +curr2_label "iout1" > +curr2_input Measured output current > +curr2_crit Critical maximum current > +curr2_crit_alarm Current critical high alarm > + > +in1_label "vin" > +in1_input Measured input voltage > +in1_lcrit Critical minimum input voltage > +in1_lcrit_alarm Input voltage critical low alarm > +in1_crit Critical maximum input voltage > +in1_crit_alarm Input voltage critical high alarm > + > +in2_label "vout1" > +in2_input Measured output voltage > +in2_lcrit Critical minimum output voltage > +in2_lcrit_alarm Output voltage critical low alarm > +in2_crit Critical maximum output voltage > +in2_crit_alarm Output voltage critical high alarm > + > +power1_label "pin" > +power1_input Measured input power > +power1_alarm Input power high alarm > + > +power2_label "pout1" > +power2_input Measured output power > + > +temp[1-2]_input Measured temperature > +temp[1-2]_crit Critical high temperature > +temp[1-2]_crit_alarm Chip temperature critical high alarm > +temp[1-2]_max Maximum temperature > +temp[1-2]_max_alarm Chip temperature high alarm > + > +fan1_alarm Fan 1 warning. > +fan1_fault Fan 1 fault. > +fan1_input Fan 1 speed in RPM. > +======================= ====================================================== > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index 8d5a2df1ecb6..b24436f22052 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > dell-smm-hwmon > dme1737 > drivetemp > + dps920ab > ds1621 > ds620 > emc1403 > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index 32d2fc850621..865ade0aa205 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > This driver can also be built as a module. If so, the module will > be called ibm-cffps. > > +config SENSORS_DPS920AB > + tristate "Delta DPS920AB Power Supply" > + help > + If you say yes here you get hardware monitoring support for Delta > + DPS920AB Power Supplies. > + > + This driver can also be built as a module. If so, the module will > + be called dps920ab. > + > config SENSORS_INSPUR_IPSPS > tristate "INSPUR Power System Power Supply" > help > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > new file mode 100644 > index 000000000000..d579ed9f879c > --- /dev/null > +++ b/drivers/hwmon/pmbus/dps920ab.c > @@ -0,0 +1,63 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Driver for Delta DPS920AB PSU > + * > + * Copyright (C) 2021 Delta Networks, Inc. > + * Copyright (C) 2021 Sartura Ltd. > + */ > + > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include "pmbus.h" > + > +static struct pmbus_driver_info dps920ab_info = { > + .pages = 1, > + > + .format[PSC_VOLTAGE_IN] = linear, > + .format[PSC_VOLTAGE_OUT] = linear, > + .format[PSC_CURRENT_IN] = linear, > + .format[PSC_CURRENT_OUT] = linear, > + .format[PSC_POWER] = linear, > + .format[PSC_FAN] = linear, > + .format[PSC_TEMPERATURE] = linear, > + > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > +}; > + > +static int dps920ab_probe(struct i2c_client *client) > +{ > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_BYTE_DATA | > + I2C_FUNC_SMBUS_WORD_DATA)) > + return -ENODEV; This check is done in pmbus_do_probe(), and repeating it here does not add any value. That makes me wonder: Is this driver needed in the first place, or could it be added to drivers/hwmon/pmbus/pmbus.c ? Thanks, Guenter > + > + return pmbus_do_probe(client, &dps920ab_info); > +} > + > +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { > + { .compatible = "delta,dps920ab", }, > + {} > +}; > + > +MODULE_DEVICE_TABLE(of, dps920ab_of_match); > + > +static struct i2c_driver dps920ab_driver = { > + .driver = { > + .name = "dps920ab", > + .of_match_table = of_match_ptr(dps920ab_of_match), > + }, > + .probe_new = dps920ab_probe, > +}; > + > +module_i2c_driver(dps920ab_driver); > + > +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); > +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); > +MODULE_LICENSE("GPL"); > -- > 2.31.1 >
On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > > This adds support for the Delta DPS-920AB PSU. > > > > Only missing feature is fan control which the PSU supports. > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > --- > > Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > > Documentation/hwmon/index.rst | 1 + > > drivers/hwmon/pmbus/Kconfig | 9 ++++ > > drivers/hwmon/pmbus/Makefile | 1 + > > drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > > 5 files changed, 154 insertions(+) > > create mode 100644 Documentation/hwmon/dps920ab.rst > > create mode 100644 drivers/hwmon/pmbus/dps920ab.c > > > > diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > > new file mode 100644 > > index 000000000000..df0aef530c7e > > --- /dev/null > > +++ b/Documentation/hwmon/dps920ab.rst > > @@ -0,0 +1,80 @@ > > +.. SPDX-License-Identifier: GPL-2.0-or-later > > + > > +Kernel driver dps920ab > > +======================== > > + > > +Supported chips: > > + > > + * Delta DPS920AB > > + > > + Prefix: 'dps920ab' > > + > > + Addresses scanned: - > > + > > +Authors: > > + Robert Marko <robert.marko@sartura.hr> > > + > > + > > +Description > > +----------- > > + > > +This driver implements support for Delta DPS920AB 920W 54V DC single output > > +power supply with PMBus support. > > + > > +The driver is a client driver to the core PMBus driver. > > +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > > + > > + > > +Usage Notes > > +----------- > > + > > +This driver does not auto-detect devices. You will have to instantiate the > > +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > > +details. > > + > > + > > +Sysfs entries > > +------------- > > + > > +======================= ====================================================== > > +curr1_label "iin" > > +curr1_input Measured input current > > +curr1_crit Critical maximum current > > +curr1_crit_alarm Current critical high alarm > > + > > +curr2_label "iout1" > > +curr2_input Measured output current > > +curr2_crit Critical maximum current > > +curr2_crit_alarm Current critical high alarm > > + > > +in1_label "vin" > > +in1_input Measured input voltage > > +in1_lcrit Critical minimum input voltage > > +in1_lcrit_alarm Input voltage critical low alarm > > +in1_crit Critical maximum input voltage > > +in1_crit_alarm Input voltage critical high alarm > > + > > +in2_label "vout1" > > +in2_input Measured output voltage > > +in2_lcrit Critical minimum output voltage > > +in2_lcrit_alarm Output voltage critical low alarm > > +in2_crit Critical maximum output voltage > > +in2_crit_alarm Output voltage critical high alarm > > + > > +power1_label "pin" > > +power1_input Measured input power > > +power1_alarm Input power high alarm > > + > > +power2_label "pout1" > > +power2_input Measured output power > > + > > +temp[1-2]_input Measured temperature > > +temp[1-2]_crit Critical high temperature > > +temp[1-2]_crit_alarm Chip temperature critical high alarm > > +temp[1-2]_max Maximum temperature > > +temp[1-2]_max_alarm Chip temperature high alarm > > + > > +fan1_alarm Fan 1 warning. > > +fan1_fault Fan 1 fault. > > +fan1_input Fan 1 speed in RPM. > > +======================= ====================================================== > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > > index 8d5a2df1ecb6..b24436f22052 100644 > > --- a/Documentation/hwmon/index.rst > > +++ b/Documentation/hwmon/index.rst > > @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > > dell-smm-hwmon > > dme1737 > > drivetemp > > + dps920ab > > ds1621 > > ds620 > > emc1403 > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > > index 32d2fc850621..865ade0aa205 100644 > > --- a/drivers/hwmon/pmbus/Kconfig > > +++ b/drivers/hwmon/pmbus/Kconfig > > @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > > This driver can also be built as a module. If so, the module will > > be called ibm-cffps. > > > > +config SENSORS_DPS920AB > > + tristate "Delta DPS920AB Power Supply" > > + help > > + If you say yes here you get hardware monitoring support for Delta > > + DPS920AB Power Supplies. > > + > > + This driver can also be built as a module. If so, the module will > > + be called dps920ab. > > + > > config SENSORS_INSPUR_IPSPS > > tristate "INSPUR Power System Power Supply" > > help > > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > > index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > > +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > > obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > > obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > > diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > > new file mode 100644 > > index 000000000000..d579ed9f879c > > --- /dev/null > > +++ b/drivers/hwmon/pmbus/dps920ab.c > > @@ -0,0 +1,63 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Driver for Delta DPS920AB PSU > > + * > > + * Copyright (C) 2021 Delta Networks, Inc. > > + * Copyright (C) 2021 Sartura Ltd. > > + */ > > + > > +#include <linux/i2c.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include "pmbus.h" > > + > > +static struct pmbus_driver_info dps920ab_info = { > > + .pages = 1, > > + > > + .format[PSC_VOLTAGE_IN] = linear, > > + .format[PSC_VOLTAGE_OUT] = linear, > > + .format[PSC_CURRENT_IN] = linear, > > + .format[PSC_CURRENT_OUT] = linear, > > + .format[PSC_POWER] = linear, > > + .format[PSC_FAN] = linear, > > + .format[PSC_TEMPERATURE] = linear, > > + > > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > > + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > > + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > > + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > > + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > > + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > > + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > > +}; > > + > > +static int dps920ab_probe(struct i2c_client *client) > > +{ > > + if (!i2c_check_functionality(client->adapter, > > + I2C_FUNC_SMBUS_BYTE_DATA | > > + I2C_FUNC_SMBUS_WORD_DATA)) > > + return -ENODEV; > > This check is done in pmbus_do_probe(), and repeating it here does not add > any value. Ok, makes sense. > > That makes me wonder: Is this driver needed in the first place, or could > it be added to drivers/hwmon/pmbus/pmbus.c ? It could be added as a generic driver, but that creates properties in the sysfs that this thing does not support like 3 voltage readings, 3 fans, and so on. Is it okay to keep it as a separate driver then? Regards, Robert > > Thanks, > Guenter > > > + > > + return pmbus_do_probe(client, &dps920ab_info); > > +} > > + > > +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { > > + { .compatible = "delta,dps920ab", }, > > + {} > > +}; > > + > > +MODULE_DEVICE_TABLE(of, dps920ab_of_match); > > + > > +static struct i2c_driver dps920ab_driver = { > > + .driver = { > > + .name = "dps920ab", > > + .of_match_table = of_match_ptr(dps920ab_of_match), > > + }, > > + .probe_new = dps920ab_probe, > > +}; > > + > > +module_i2c_driver(dps920ab_driver); > > + > > +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); > > +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.31.1 > >
On 5/19/21 5:38 AM, Robert Marko wrote: > On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>> This adds support for the Delta DPS-920AB PSU. >>> >>> Only missing feature is fan control which the PSU supports. >>> >>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>> --- >>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>> Documentation/hwmon/index.rst | 1 + >>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>> drivers/hwmon/pmbus/Makefile | 1 + >>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>> 5 files changed, 154 insertions(+) >>> create mode 100644 Documentation/hwmon/dps920ab.rst >>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>> >>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>> new file mode 100644 >>> index 000000000000..df0aef530c7e >>> --- /dev/null >>> +++ b/Documentation/hwmon/dps920ab.rst >>> @@ -0,0 +1,80 @@ >>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>> + >>> +Kernel driver dps920ab >>> +======================== >>> + >>> +Supported chips: >>> + >>> + * Delta DPS920AB >>> + >>> + Prefix: 'dps920ab' >>> + >>> + Addresses scanned: - >>> + >>> +Authors: >>> + Robert Marko <robert.marko@sartura.hr> >>> + >>> + >>> +Description >>> +----------- >>> + >>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>> +power supply with PMBus support. >>> + >>> +The driver is a client driver to the core PMBus driver. >>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>> + >>> + >>> +Usage Notes >>> +----------- >>> + >>> +This driver does not auto-detect devices. You will have to instantiate the >>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>> +details. >>> + >>> + >>> +Sysfs entries >>> +------------- >>> + >>> +======================= ====================================================== >>> +curr1_label "iin" >>> +curr1_input Measured input current >>> +curr1_crit Critical maximum current >>> +curr1_crit_alarm Current critical high alarm >>> + >>> +curr2_label "iout1" >>> +curr2_input Measured output current >>> +curr2_crit Critical maximum current >>> +curr2_crit_alarm Current critical high alarm >>> + >>> +in1_label "vin" >>> +in1_input Measured input voltage >>> +in1_lcrit Critical minimum input voltage >>> +in1_lcrit_alarm Input voltage critical low alarm >>> +in1_crit Critical maximum input voltage >>> +in1_crit_alarm Input voltage critical high alarm >>> + >>> +in2_label "vout1" >>> +in2_input Measured output voltage >>> +in2_lcrit Critical minimum output voltage >>> +in2_lcrit_alarm Output voltage critical low alarm >>> +in2_crit Critical maximum output voltage >>> +in2_crit_alarm Output voltage critical high alarm >>> + >>> +power1_label "pin" >>> +power1_input Measured input power >>> +power1_alarm Input power high alarm >>> + >>> +power2_label "pout1" >>> +power2_input Measured output power >>> + >>> +temp[1-2]_input Measured temperature >>> +temp[1-2]_crit Critical high temperature >>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>> +temp[1-2]_max Maximum temperature >>> +temp[1-2]_max_alarm Chip temperature high alarm >>> + >>> +fan1_alarm Fan 1 warning. >>> +fan1_fault Fan 1 fault. >>> +fan1_input Fan 1 speed in RPM. >>> +======================= ====================================================== >>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>> index 8d5a2df1ecb6..b24436f22052 100644 >>> --- a/Documentation/hwmon/index.rst >>> +++ b/Documentation/hwmon/index.rst >>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>> dell-smm-hwmon >>> dme1737 >>> drivetemp >>> + dps920ab >>> ds1621 >>> ds620 >>> emc1403 >>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>> index 32d2fc850621..865ade0aa205 100644 >>> --- a/drivers/hwmon/pmbus/Kconfig >>> +++ b/drivers/hwmon/pmbus/Kconfig >>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>> This driver can also be built as a module. If so, the module will >>> be called ibm-cffps. >>> >>> +config SENSORS_DPS920AB >>> + tristate "Delta DPS920AB Power Supply" >>> + help >>> + If you say yes here you get hardware monitoring support for Delta >>> + DPS920AB Power Supplies. >>> + >>> + This driver can also be built as a module. If so, the module will >>> + be called dps920ab. >>> + >>> config SENSORS_INSPUR_IPSPS >>> tristate "INSPUR Power System Power Supply" >>> help >>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>> new file mode 100644 >>> index 000000000000..d579ed9f879c >>> --- /dev/null >>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>> @@ -0,0 +1,63 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later >>> +/* >>> + * Driver for Delta DPS920AB PSU >>> + * >>> + * Copyright (C) 2021 Delta Networks, Inc. >>> + * Copyright (C) 2021 Sartura Ltd. >>> + */ >>> + >>> +#include <linux/i2c.h> >>> +#include <linux/module.h> >>> +#include <linux/of_device.h> >>> +#include "pmbus.h" >>> + >>> +static struct pmbus_driver_info dps920ab_info = { >>> + .pages = 1, >>> + >>> + .format[PSC_VOLTAGE_IN] = linear, >>> + .format[PSC_VOLTAGE_OUT] = linear, >>> + .format[PSC_CURRENT_IN] = linear, >>> + .format[PSC_CURRENT_OUT] = linear, >>> + .format[PSC_POWER] = linear, >>> + .format[PSC_FAN] = linear, >>> + .format[PSC_TEMPERATURE] = linear, >>> + >>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>> +}; >>> + >>> +static int dps920ab_probe(struct i2c_client *client) >>> +{ >>> + if (!i2c_check_functionality(client->adapter, >>> + I2C_FUNC_SMBUS_BYTE_DATA | >>> + I2C_FUNC_SMBUS_WORD_DATA)) >>> + return -ENODEV; >> >> This check is done in pmbus_do_probe(), and repeating it here does not add >> any value. > > Ok, makes sense. >> >> That makes me wonder: Is this driver needed in the first place, or could >> it be added to drivers/hwmon/pmbus/pmbus.c ? > > It could be added as a generic driver, but that creates properties in > the sysfs that this thing > does not support like 3 voltage readings, 3 fans, and so on. > Can you be more specific ? What additional unsupported attributes are added, and why ? Are you saying the PSU reports registers as existing which do not really exist ? If so, which registers are those ? In this context, I have a hard time finding a reference for this power supply. Do you have a datasheet or some other documents you can share ? Thanks, Guenter > Is it okay to keep it as a separate driver then? > > Regards, > Robert >> >> Thanks, >> Guenter >> >>> + >>> + return pmbus_do_probe(client, &dps920ab_info); >>> +} >>> + >>> +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { >>> + { .compatible = "delta,dps920ab", }, >>> + {} >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(of, dps920ab_of_match); >>> + >>> +static struct i2c_driver dps920ab_driver = { >>> + .driver = { >>> + .name = "dps920ab", >>> + .of_match_table = of_match_ptr(dps920ab_of_match), >>> + }, >>> + .probe_new = dps920ab_probe, >>> +}; >>> + >>> +module_i2c_driver(dps920ab_driver); >>> + >>> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); >>> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); >>> +MODULE_LICENSE("GPL"); >>> -- >>> 2.31.1 >>> > > >
On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/19/21 5:38 AM, Robert Marko wrote: > > On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>> This adds support for the Delta DPS-920AB PSU. > >>> > >>> Only missing feature is fan control which the PSU supports. > >>> > >>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>> --- > >>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>> Documentation/hwmon/index.rst | 1 + > >>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>> drivers/hwmon/pmbus/Makefile | 1 + > >>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>> 5 files changed, 154 insertions(+) > >>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>> > >>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>> new file mode 100644 > >>> index 000000000000..df0aef530c7e > >>> --- /dev/null > >>> +++ b/Documentation/hwmon/dps920ab.rst > >>> @@ -0,0 +1,80 @@ > >>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>> + > >>> +Kernel driver dps920ab > >>> +======================== > >>> + > >>> +Supported chips: > >>> + > >>> + * Delta DPS920AB > >>> + > >>> + Prefix: 'dps920ab' > >>> + > >>> + Addresses scanned: - > >>> + > >>> +Authors: > >>> + Robert Marko <robert.marko@sartura.hr> > >>> + > >>> + > >>> +Description > >>> +----------- > >>> + > >>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>> +power supply with PMBus support. > >>> + > >>> +The driver is a client driver to the core PMBus driver. > >>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>> + > >>> + > >>> +Usage Notes > >>> +----------- > >>> + > >>> +This driver does not auto-detect devices. You will have to instantiate the > >>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>> +details. > >>> + > >>> + > >>> +Sysfs entries > >>> +------------- > >>> + > >>> +======================= ====================================================== > >>> +curr1_label "iin" > >>> +curr1_input Measured input current > >>> +curr1_crit Critical maximum current > >>> +curr1_crit_alarm Current critical high alarm > >>> + > >>> +curr2_label "iout1" > >>> +curr2_input Measured output current > >>> +curr2_crit Critical maximum current > >>> +curr2_crit_alarm Current critical high alarm > >>> + > >>> +in1_label "vin" > >>> +in1_input Measured input voltage > >>> +in1_lcrit Critical minimum input voltage > >>> +in1_lcrit_alarm Input voltage critical low alarm > >>> +in1_crit Critical maximum input voltage > >>> +in1_crit_alarm Input voltage critical high alarm > >>> + > >>> +in2_label "vout1" > >>> +in2_input Measured output voltage > >>> +in2_lcrit Critical minimum output voltage > >>> +in2_lcrit_alarm Output voltage critical low alarm > >>> +in2_crit Critical maximum output voltage > >>> +in2_crit_alarm Output voltage critical high alarm > >>> + > >>> +power1_label "pin" > >>> +power1_input Measured input power > >>> +power1_alarm Input power high alarm > >>> + > >>> +power2_label "pout1" > >>> +power2_input Measured output power > >>> + > >>> +temp[1-2]_input Measured temperature > >>> +temp[1-2]_crit Critical high temperature > >>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>> +temp[1-2]_max Maximum temperature > >>> +temp[1-2]_max_alarm Chip temperature high alarm > >>> + > >>> +fan1_alarm Fan 1 warning. > >>> +fan1_fault Fan 1 fault. > >>> +fan1_input Fan 1 speed in RPM. > >>> +======================= ====================================================== > >>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>> index 8d5a2df1ecb6..b24436f22052 100644 > >>> --- a/Documentation/hwmon/index.rst > >>> +++ b/Documentation/hwmon/index.rst > >>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>> dell-smm-hwmon > >>> dme1737 > >>> drivetemp > >>> + dps920ab > >>> ds1621 > >>> ds620 > >>> emc1403 > >>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>> index 32d2fc850621..865ade0aa205 100644 > >>> --- a/drivers/hwmon/pmbus/Kconfig > >>> +++ b/drivers/hwmon/pmbus/Kconfig > >>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>> This driver can also be built as a module. If so, the module will > >>> be called ibm-cffps. > >>> > >>> +config SENSORS_DPS920AB > >>> + tristate "Delta DPS920AB Power Supply" > >>> + help > >>> + If you say yes here you get hardware monitoring support for Delta > >>> + DPS920AB Power Supplies. > >>> + > >>> + This driver can also be built as a module. If so, the module will > >>> + be called dps920ab. > >>> + > >>> config SENSORS_INSPUR_IPSPS > >>> tristate "INSPUR Power System Power Supply" > >>> help > >>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>> new file mode 100644 > >>> index 000000000000..d579ed9f879c > >>> --- /dev/null > >>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>> @@ -0,0 +1,63 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>> +/* > >>> + * Driver for Delta DPS920AB PSU > >>> + * > >>> + * Copyright (C) 2021 Delta Networks, Inc. > >>> + * Copyright (C) 2021 Sartura Ltd. > >>> + */ > >>> + > >>> +#include <linux/i2c.h> > >>> +#include <linux/module.h> > >>> +#include <linux/of_device.h> > >>> +#include "pmbus.h" > >>> + > >>> +static struct pmbus_driver_info dps920ab_info = { > >>> + .pages = 1, > >>> + > >>> + .format[PSC_VOLTAGE_IN] = linear, > >>> + .format[PSC_VOLTAGE_OUT] = linear, > >>> + .format[PSC_CURRENT_IN] = linear, > >>> + .format[PSC_CURRENT_OUT] = linear, > >>> + .format[PSC_POWER] = linear, > >>> + .format[PSC_FAN] = linear, > >>> + .format[PSC_TEMPERATURE] = linear, > >>> + > >>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>> +}; > >>> + > >>> +static int dps920ab_probe(struct i2c_client *client) > >>> +{ > >>> + if (!i2c_check_functionality(client->adapter, > >>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>> + return -ENODEV; > >> > >> This check is done in pmbus_do_probe(), and repeating it here does not add > >> any value. > > > > Ok, makes sense. > >> > >> That makes me wonder: Is this driver needed in the first place, or could > >> it be added to drivers/hwmon/pmbus/pmbus.c ? > > > > It could be added as a generic driver, but that creates properties in > > the sysfs that this thing > > does not support like 3 voltage readings, 3 fans, and so on. > > > > Can you be more specific ? What additional unsupported attributes > are added, and why ? Are you saying the PSU reports registers as > existing which do not really exist ? If so, which registers are those ? Sure, when core does probing and autodiscovery, then the following sysfs attributes are created: > > curr1_crit fan3_fault in3_min_alarm temp1_max > curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > curr1_input fan3_target in3_rated_min temp1_min > curr1_label in1_crit name temp1_min_alarm > curr1_max in1_crit_alarm of_node temp1_rated_max > curr1_max_alarm in1_input power temp2_crit > curr1_rated_max in1_label power1_alarm temp2_crit_alarm > curr2_crit in1_lcrit power1_input temp2_input > curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > curr2_input in1_max power1_max temp2_lcrit_alarm > curr2_label in1_max_alarm power1_rated_max temp2_max > curr2_lcrit in1_min power2_cap temp2_max_alarm > curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > curr2_max in1_rated_max power2_crit temp2_min_alarm > curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > curr2_rated_max in2_input power2_input temp3_crit > device in2_label power2_label temp3_crit_alarm > fan1_alarm in3_crit power2_max temp3_input > fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > fan1_input in3_input power2_rated_max temp3_lcrit_alarm > fan1_target in3_label subsystem temp3_max > fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > fan2_input in3_max temp1_input temp3_min_alarm > fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > fan3_alarm in3_min temp1_lcrit_alarm uevent The following return -1, or -500 so they are not supported. * fan2 * in2 Weirdly, with the external driver both fan2 and in2 are enabled and work fine, but when auto probing they are fan3 and in3. temp3 actually seems to return a valid temperature despite it not being used in the vendor driver that features were picked from. > > In this context, I have a hard time finding a reference for > this power supply. Do you have a datasheet or some other documents > you can share ? Unfortunately, I don't have a datasheet as that would have made this way easier. It was all based on the vendor "driver" from DENT: https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c I will try asking Delta for the datasheet. Regards, Robert > > Thanks, > Guenter > > > Is it okay to keep it as a separate driver then? > > > > Regards, > > Robert > >> > >> Thanks, > >> Guenter > >> > >>> + > >>> + return pmbus_do_probe(client, &dps920ab_info); > >>> +} > >>> + > >>> +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { > >>> + { .compatible = "delta,dps920ab", }, > >>> + {} > >>> +}; > >>> + > >>> +MODULE_DEVICE_TABLE(of, dps920ab_of_match); > >>> + > >>> +static struct i2c_driver dps920ab_driver = { > >>> + .driver = { > >>> + .name = "dps920ab", > >>> + .of_match_table = of_match_ptr(dps920ab_of_match), > >>> + }, > >>> + .probe_new = dps920ab_probe, > >>> +}; > >>> + > >>> +module_i2c_driver(dps920ab_driver); > >>> + > >>> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); > >>> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); > >>> +MODULE_LICENSE("GPL"); > >>> -- > >>> 2.31.1 > >>> > > > > > > >
On 5/21/21 1:36 AM, Robert Marko wrote: > On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/19/21 5:38 AM, Robert Marko wrote: >>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>>>> This adds support for the Delta DPS-920AB PSU. >>>>> >>>>> Only missing feature is fan control which the PSU supports. >>>>> >>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>>>> --- >>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>>>> Documentation/hwmon/index.rst | 1 + >>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>>>> 5 files changed, 154 insertions(+) >>>>> create mode 100644 Documentation/hwmon/dps920ab.rst >>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>>>> >>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>>>> new file mode 100644 >>>>> index 000000000000..df0aef530c7e >>>>> --- /dev/null >>>>> +++ b/Documentation/hwmon/dps920ab.rst >>>>> @@ -0,0 +1,80 @@ >>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>>>> + >>>>> +Kernel driver dps920ab >>>>> +======================== >>>>> + >>>>> +Supported chips: >>>>> + >>>>> + * Delta DPS920AB >>>>> + >>>>> + Prefix: 'dps920ab' >>>>> + >>>>> + Addresses scanned: - >>>>> + >>>>> +Authors: >>>>> + Robert Marko <robert.marko@sartura.hr> >>>>> + >>>>> + >>>>> +Description >>>>> +----------- >>>>> + >>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>>>> +power supply with PMBus support. >>>>> + >>>>> +The driver is a client driver to the core PMBus driver. >>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>>>> + >>>>> + >>>>> +Usage Notes >>>>> +----------- >>>>> + >>>>> +This driver does not auto-detect devices. You will have to instantiate the >>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>>>> +details. >>>>> + >>>>> + >>>>> +Sysfs entries >>>>> +------------- >>>>> + >>>>> +======================= ====================================================== >>>>> +curr1_label "iin" >>>>> +curr1_input Measured input current >>>>> +curr1_crit Critical maximum current >>>>> +curr1_crit_alarm Current critical high alarm >>>>> + >>>>> +curr2_label "iout1" >>>>> +curr2_input Measured output current >>>>> +curr2_crit Critical maximum current >>>>> +curr2_crit_alarm Current critical high alarm >>>>> + >>>>> +in1_label "vin" >>>>> +in1_input Measured input voltage >>>>> +in1_lcrit Critical minimum input voltage >>>>> +in1_lcrit_alarm Input voltage critical low alarm >>>>> +in1_crit Critical maximum input voltage >>>>> +in1_crit_alarm Input voltage critical high alarm >>>>> + >>>>> +in2_label "vout1" >>>>> +in2_input Measured output voltage >>>>> +in2_lcrit Critical minimum output voltage >>>>> +in2_lcrit_alarm Output voltage critical low alarm >>>>> +in2_crit Critical maximum output voltage >>>>> +in2_crit_alarm Output voltage critical high alarm >>>>> + >>>>> +power1_label "pin" >>>>> +power1_input Measured input power >>>>> +power1_alarm Input power high alarm >>>>> + >>>>> +power2_label "pout1" >>>>> +power2_input Measured output power >>>>> + >>>>> +temp[1-2]_input Measured temperature >>>>> +temp[1-2]_crit Critical high temperature >>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>>>> +temp[1-2]_max Maximum temperature >>>>> +temp[1-2]_max_alarm Chip temperature high alarm >>>>> + >>>>> +fan1_alarm Fan 1 warning. >>>>> +fan1_fault Fan 1 fault. >>>>> +fan1_input Fan 1 speed in RPM. >>>>> +======================= ====================================================== >>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>>>> index 8d5a2df1ecb6..b24436f22052 100644 >>>>> --- a/Documentation/hwmon/index.rst >>>>> +++ b/Documentation/hwmon/index.rst >>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>>>> dell-smm-hwmon >>>>> dme1737 >>>>> drivetemp >>>>> + dps920ab >>>>> ds1621 >>>>> ds620 >>>>> emc1403 >>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>> index 32d2fc850621..865ade0aa205 100644 >>>>> --- a/drivers/hwmon/pmbus/Kconfig >>>>> +++ b/drivers/hwmon/pmbus/Kconfig >>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>>>> This driver can also be built as a module. If so, the module will >>>>> be called ibm-cffps. >>>>> >>>>> +config SENSORS_DPS920AB >>>>> + tristate "Delta DPS920AB Power Supply" >>>>> + help >>>>> + If you say yes here you get hardware monitoring support for Delta >>>>> + DPS920AB Power Supplies. >>>>> + >>>>> + This driver can also be built as a module. If so, the module will >>>>> + be called dps920ab. >>>>> + >>>>> config SENSORS_INSPUR_IPSPS >>>>> tristate "INSPUR Power System Power Supply" >>>>> help >>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>>>> new file mode 100644 >>>>> index 000000000000..d579ed9f879c >>>>> --- /dev/null >>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>>>> @@ -0,0 +1,63 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>> +/* >>>>> + * Driver for Delta DPS920AB PSU >>>>> + * >>>>> + * Copyright (C) 2021 Delta Networks, Inc. >>>>> + * Copyright (C) 2021 Sartura Ltd. >>>>> + */ >>>>> + >>>>> +#include <linux/i2c.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/of_device.h> >>>>> +#include "pmbus.h" >>>>> + >>>>> +static struct pmbus_driver_info dps920ab_info = { >>>>> + .pages = 1, >>>>> + >>>>> + .format[PSC_VOLTAGE_IN] = linear, >>>>> + .format[PSC_VOLTAGE_OUT] = linear, >>>>> + .format[PSC_CURRENT_IN] = linear, >>>>> + .format[PSC_CURRENT_OUT] = linear, >>>>> + .format[PSC_POWER] = linear, >>>>> + .format[PSC_FAN] = linear, >>>>> + .format[PSC_TEMPERATURE] = linear, >>>>> + >>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>>>> +}; >>>>> + >>>>> +static int dps920ab_probe(struct i2c_client *client) >>>>> +{ >>>>> + if (!i2c_check_functionality(client->adapter, >>>>> + I2C_FUNC_SMBUS_BYTE_DATA | >>>>> + I2C_FUNC_SMBUS_WORD_DATA)) >>>>> + return -ENODEV; >>>> >>>> This check is done in pmbus_do_probe(), and repeating it here does not add >>>> any value. >>> >>> Ok, makes sense. >>>> >>>> That makes me wonder: Is this driver needed in the first place, or could >>>> it be added to drivers/hwmon/pmbus/pmbus.c ? >>> >>> It could be added as a generic driver, but that creates properties in >>> the sysfs that this thing >>> does not support like 3 voltage readings, 3 fans, and so on. >>> >> >> Can you be more specific ? What additional unsupported attributes >> are added, and why ? Are you saying the PSU reports registers as >> existing which do not really exist ? If so, which registers are those ? > > Sure, when core does probing and autodiscovery, then the following > sysfs attributes are created: >> >> curr1_crit fan3_fault in3_min_alarm temp1_max >> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm >> curr1_input fan3_target in3_rated_min temp1_min >> curr1_label in1_crit name temp1_min_alarm >> curr1_max in1_crit_alarm of_node temp1_rated_max >> curr1_max_alarm in1_input power temp2_crit >> curr1_rated_max in1_label power1_alarm temp2_crit_alarm >> curr2_crit in1_lcrit power1_input temp2_input >> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit >> curr2_input in1_max power1_max temp2_lcrit_alarm >> curr2_label in1_max_alarm power1_rated_max temp2_max >> curr2_lcrit in1_min power2_cap temp2_max_alarm >> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min >> curr2_max in1_rated_max power2_crit temp2_min_alarm >> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max >> curr2_rated_max in2_input power2_input temp3_crit >> device in2_label power2_label temp3_crit_alarm >> fan1_alarm in3_crit power2_max temp3_input >> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit >> fan1_input in3_input power2_rated_max temp3_lcrit_alarm >> fan1_target in3_label subsystem temp3_max >> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm >> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min >> fan2_input in3_max temp1_input temp3_min_alarm >> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max >> fan3_alarm in3_min temp1_lcrit_alarm uevent > > The following return -1, or -500 so they are not supported. > * fan2 > * in2 > > Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > but when auto probing they are fan3 and in3. > > temp3 actually seems to return a valid temperature despite it not being used in > the vendor driver that features were picked from. > Can you run "grep . *" in the hwmon directory so I can see actual values ? Thanks, Guenter >> >> In this context, I have a hard time finding a reference for >> this power supply. Do you have a datasheet or some other documents >> you can share ? > > Unfortunately, I don't have a datasheet as that would have made this way easier. > It was all based on the vendor "driver" from DENT: > https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c > > I will try asking Delta for the datasheet. > Regards, > Robert > >> >> Thanks, >> Guenter >> >>> Is it okay to keep it as a separate driver then? >>> >>> Regards, >>> Robert >>>> >>>> Thanks, >>>> Guenter >>>> >>>>> + >>>>> + return pmbus_do_probe(client, &dps920ab_info); >>>>> +} >>>>> + >>>>> +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { >>>>> + { .compatible = "delta,dps920ab", }, >>>>> + {} >>>>> +}; >>>>> + >>>>> +MODULE_DEVICE_TABLE(of, dps920ab_of_match); >>>>> + >>>>> +static struct i2c_driver dps920ab_driver = { >>>>> + .driver = { >>>>> + .name = "dps920ab", >>>>> + .of_match_table = of_match_ptr(dps920ab_of_match), >>>>> + }, >>>>> + .probe_new = dps920ab_probe, >>>>> +}; >>>>> + >>>>> +module_i2c_driver(dps920ab_driver); >>>>> + >>>>> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); >>>>> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); >>>>> +MODULE_LICENSE("GPL"); >>>>> -- >>>>> 2.31.1 >>>>> >>> >>> >>> >> > >
On 5/21/21 1:36 AM, Robert Marko wrote: [ ... ] >> In this context, I have a hard time finding a reference for >> this power supply. Do you have a datasheet or some other documents >> you can share ? > > Unfortunately, I don't have a datasheet as that would have made this way easier. > It was all based on the vendor "driver" from DENT: > https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c > Ah, so this is not a driver for a power supply from DeltaPSU, but a power supply from Delta Networks Technology Corporation, as used in that company's TN48M-POE switch. That is a world of difference, even though the parent company seems to be the same. I am not sure if, based on this information, the driver should claim to be for "Delta DPS-920AB PSU" in the first place. Can you run a block read on MFR_MODEL and MFG_SERIAL ? That might give us an idea about the actual manufacturer and model of this power supply. Also, isn't that the same power supply for which you were trying to add another hwmon driver to display some of its status information, obtained from some CPLD ? Thanks, Guenter
On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/21/21 1:36 AM, Robert Marko wrote: > > On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 5/19/21 5:38 AM, Robert Marko wrote: > >>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>>>> This adds support for the Delta DPS-920AB PSU. > >>>>> > >>>>> Only missing feature is fan control which the PSU supports. > >>>>> > >>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>>>> --- > >>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>>>> Documentation/hwmon/index.rst | 1 + > >>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>>>> drivers/hwmon/pmbus/Makefile | 1 + > >>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>>>> 5 files changed, 154 insertions(+) > >>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>>>> > >>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>>>> new file mode 100644 > >>>>> index 000000000000..df0aef530c7e > >>>>> --- /dev/null > >>>>> +++ b/Documentation/hwmon/dps920ab.rst > >>>>> @@ -0,0 +1,80 @@ > >>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>>>> + > >>>>> +Kernel driver dps920ab > >>>>> +======================== > >>>>> + > >>>>> +Supported chips: > >>>>> + > >>>>> + * Delta DPS920AB > >>>>> + > >>>>> + Prefix: 'dps920ab' > >>>>> + > >>>>> + Addresses scanned: - > >>>>> + > >>>>> +Authors: > >>>>> + Robert Marko <robert.marko@sartura.hr> > >>>>> + > >>>>> + > >>>>> +Description > >>>>> +----------- > >>>>> + > >>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>>>> +power supply with PMBus support. > >>>>> + > >>>>> +The driver is a client driver to the core PMBus driver. > >>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>>>> + > >>>>> + > >>>>> +Usage Notes > >>>>> +----------- > >>>>> + > >>>>> +This driver does not auto-detect devices. You will have to instantiate the > >>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>>>> +details. > >>>>> + > >>>>> + > >>>>> +Sysfs entries > >>>>> +------------- > >>>>> + > >>>>> +======================= ====================================================== > >>>>> +curr1_label "iin" > >>>>> +curr1_input Measured input current > >>>>> +curr1_crit Critical maximum current > >>>>> +curr1_crit_alarm Current critical high alarm > >>>>> + > >>>>> +curr2_label "iout1" > >>>>> +curr2_input Measured output current > >>>>> +curr2_crit Critical maximum current > >>>>> +curr2_crit_alarm Current critical high alarm > >>>>> + > >>>>> +in1_label "vin" > >>>>> +in1_input Measured input voltage > >>>>> +in1_lcrit Critical minimum input voltage > >>>>> +in1_lcrit_alarm Input voltage critical low alarm > >>>>> +in1_crit Critical maximum input voltage > >>>>> +in1_crit_alarm Input voltage critical high alarm > >>>>> + > >>>>> +in2_label "vout1" > >>>>> +in2_input Measured output voltage > >>>>> +in2_lcrit Critical minimum output voltage > >>>>> +in2_lcrit_alarm Output voltage critical low alarm > >>>>> +in2_crit Critical maximum output voltage > >>>>> +in2_crit_alarm Output voltage critical high alarm > >>>>> + > >>>>> +power1_label "pin" > >>>>> +power1_input Measured input power > >>>>> +power1_alarm Input power high alarm > >>>>> + > >>>>> +power2_label "pout1" > >>>>> +power2_input Measured output power > >>>>> + > >>>>> +temp[1-2]_input Measured temperature > >>>>> +temp[1-2]_crit Critical high temperature > >>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>>>> +temp[1-2]_max Maximum temperature > >>>>> +temp[1-2]_max_alarm Chip temperature high alarm > >>>>> + > >>>>> +fan1_alarm Fan 1 warning. > >>>>> +fan1_fault Fan 1 fault. > >>>>> +fan1_input Fan 1 speed in RPM. > >>>>> +======================= ====================================================== > >>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>>>> index 8d5a2df1ecb6..b24436f22052 100644 > >>>>> --- a/Documentation/hwmon/index.rst > >>>>> +++ b/Documentation/hwmon/index.rst > >>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>>>> dell-smm-hwmon > >>>>> dme1737 > >>>>> drivetemp > >>>>> + dps920ab > >>>>> ds1621 > >>>>> ds620 > >>>>> emc1403 > >>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>>>> index 32d2fc850621..865ade0aa205 100644 > >>>>> --- a/drivers/hwmon/pmbus/Kconfig > >>>>> +++ b/drivers/hwmon/pmbus/Kconfig > >>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>>>> This driver can also be built as a module. If so, the module will > >>>>> be called ibm-cffps. > >>>>> > >>>>> +config SENSORS_DPS920AB > >>>>> + tristate "Delta DPS920AB Power Supply" > >>>>> + help > >>>>> + If you say yes here you get hardware monitoring support for Delta > >>>>> + DPS920AB Power Supplies. > >>>>> + > >>>>> + This driver can also be built as a module. If so, the module will > >>>>> + be called dps920ab. > >>>>> + > >>>>> config SENSORS_INSPUR_IPSPS > >>>>> tristate "INSPUR Power System Power Supply" > >>>>> help > >>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>>>> new file mode 100644 > >>>>> index 000000000000..d579ed9f879c > >>>>> --- /dev/null > >>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>>>> @@ -0,0 +1,63 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>> +/* > >>>>> + * Driver for Delta DPS920AB PSU > >>>>> + * > >>>>> + * Copyright (C) 2021 Delta Networks, Inc. > >>>>> + * Copyright (C) 2021 Sartura Ltd. > >>>>> + */ > >>>>> + > >>>>> +#include <linux/i2c.h> > >>>>> +#include <linux/module.h> > >>>>> +#include <linux/of_device.h> > >>>>> +#include "pmbus.h" > >>>>> + > >>>>> +static struct pmbus_driver_info dps920ab_info = { > >>>>> + .pages = 1, > >>>>> + > >>>>> + .format[PSC_VOLTAGE_IN] = linear, > >>>>> + .format[PSC_VOLTAGE_OUT] = linear, > >>>>> + .format[PSC_CURRENT_IN] = linear, > >>>>> + .format[PSC_CURRENT_OUT] = linear, > >>>>> + .format[PSC_POWER] = linear, > >>>>> + .format[PSC_FAN] = linear, > >>>>> + .format[PSC_TEMPERATURE] = linear, > >>>>> + > >>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>>>> +}; > >>>>> + > >>>>> +static int dps920ab_probe(struct i2c_client *client) > >>>>> +{ > >>>>> + if (!i2c_check_functionality(client->adapter, > >>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>>>> + return -ENODEV; > >>>> > >>>> This check is done in pmbus_do_probe(), and repeating it here does not add > >>>> any value. > >>> > >>> Ok, makes sense. > >>>> > >>>> That makes me wonder: Is this driver needed in the first place, or could > >>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > >>> > >>> It could be added as a generic driver, but that creates properties in > >>> the sysfs that this thing > >>> does not support like 3 voltage readings, 3 fans, and so on. > >>> > >> > >> Can you be more specific ? What additional unsupported attributes > >> are added, and why ? Are you saying the PSU reports registers as > >> existing which do not really exist ? If so, which registers are those ? > > > > Sure, when core does probing and autodiscovery, then the following > > sysfs attributes are created: > >> > >> curr1_crit fan3_fault in3_min_alarm temp1_max > >> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > >> curr1_input fan3_target in3_rated_min temp1_min > >> curr1_label in1_crit name temp1_min_alarm > >> curr1_max in1_crit_alarm of_node temp1_rated_max > >> curr1_max_alarm in1_input power temp2_crit > >> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > >> curr2_crit in1_lcrit power1_input temp2_input > >> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > >> curr2_input in1_max power1_max temp2_lcrit_alarm > >> curr2_label in1_max_alarm power1_rated_max temp2_max > >> curr2_lcrit in1_min power2_cap temp2_max_alarm > >> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > >> curr2_max in1_rated_max power2_crit temp2_min_alarm > >> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > >> curr2_rated_max in2_input power2_input temp3_crit > >> device in2_label power2_label temp3_crit_alarm > >> fan1_alarm in3_crit power2_max temp3_input > >> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > >> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > >> fan1_target in3_label subsystem temp3_max > >> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > >> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > >> fan2_input in3_max temp1_input temp3_min_alarm > >> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > >> fan3_alarm in3_min temp1_lcrit_alarm uevent > > > > The following return -1, or -500 so they are not supported. > > * fan2 > > * in2 > > > > Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > > but when auto probing they are fan3 and in3. > > > > temp3 actually seems to return a valid temperature despite it not being used in > > the vendor driver that features were picked from. > > > > Can you run "grep . *" in the hwmon directory so I can see actual values ? > Sure: > > curr1_crit:-500 > curr1_crit_alarm:0 > curr1_input:195 > curr1_label:iin > curr1_max:-500 > curr1_max_alarm:0 > curr1_rated_max:-500 > curr2_crit:-500 > curr2_crit_alarm:0 > curr2_input:320 > curr2_label:iout1 > curr2_lcrit:-500 > curr2_lcrit_alarm:0 > curr2_max:18625 > curr2_max_alarm:0 > curr2_rated_max:16875 > grep: device: Is a directory > fan1_alarm:0 > fan1_fault:0 > fan1_input:10000 > fan1_target:0 > fan2_alarm:1 > fan2_fault:1 > fan2_input:-1 > fan2_target:-1 > fan3_alarm:1 > fan3_fault:1 > fan3_input:-1 > fan3_target:-1 > in1_crit:-500 > in1_crit_alarm:0 > in1_input:245250 > in1_label:vin > in1_lcrit:-500 > in1_lcrit_alarm:0 > in1_max:-500 > in1_max_alarm:0 > in1_min:-500 > in1_min_alarm:0 > in1_rated_max:-500 > in1_rated_min:-500 > in2_input:-500 > in2_label:vcap > in3_crit:255996 > in3_crit_alarm:0 > in3_input:54511 > in3_label:vout1 > in3_lcrit:255996 > in3_lcrit_alarm:0 > in3_max:255996 > in3_max_alarm:0 > in3_min:255996 > in3_min_alarm:0 > in3_rated_max:56136 > in3_rated_min:52863 > name:dps920ab > grep: of_node: Is a directory > grep: power: Is a directory > power1_alarm:0 > power1_input:33250000 > power1_label:pin > power1_max:-500000 > power1_rated_max:-500000 > power2_cap:-500000 > power2_cap_alarm:0 > power2_crit:-500000 > power2_crit_alarm:0 > power2_input:17750000 > power2_label:pout1 > power2_max:-500000 > power2_max_alarm:0 > power2_rated_max:920000000 > grep: subsystem: Is a directory > temp1_crit:-500 > temp1_crit_alarm:0 > temp1_input:23000 > temp1_lcrit:-500 > temp1_lcrit_alarm:0 > temp1_max:-500 > temp1_max_alarm:0 > temp1_min:-500 > temp1_min_alarm:0 > temp1_rated_max:-500 > temp2_crit:-500 > temp2_crit_alarm:0 > temp2_input:26000 > temp2_lcrit:-500 > temp2_lcrit_alarm:0 > temp2_max:-500 > temp2_max_alarm:0 > temp2_min:-500 > temp2_min_alarm:0 > temp2_rated_max:-500 > temp3_crit:-500 > temp3_crit_alarm:0 > temp3_input:30000 > temp3_lcrit:-500 > temp3_lcrit_alarm:0 > temp3_max:-500 > temp3_max_alarm:0 > temp3_min:-500 > temp3_min_alarm:0 > temp3_rated_max:-500 > uevent:OF_NAME=psu > uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > uevent:OF_COMPATIBLE_0=delta,dps920ab > uevent:OF_COMPATIBLE_N=1 > Thanks, > Guenter > > >> > >> In this context, I have a hard time finding a reference for > >> this power supply. Do you have a datasheet or some other documents > >> you can share ? > > > > Unfortunately, I don't have a datasheet as that would have made this way easier. > > It was all based on the vendor "driver" from DENT: > > https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c > > > > I will try asking Delta for the datasheet. > > Regards, > > Robert > > > >> > >> Thanks, > >> Guenter > >> > >>> Is it okay to keep it as a separate driver then? > >>> > >>> Regards, > >>> Robert > >>>> > >>>> Thanks, > >>>> Guenter > >>>> > >>>>> + > >>>>> + return pmbus_do_probe(client, &dps920ab_info); > >>>>> +} > >>>>> + > >>>>> +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { > >>>>> + { .compatible = "delta,dps920ab", }, > >>>>> + {} > >>>>> +}; > >>>>> + > >>>>> +MODULE_DEVICE_TABLE(of, dps920ab_of_match); > >>>>> + > >>>>> +static struct i2c_driver dps920ab_driver = { > >>>>> + .driver = { > >>>>> + .name = "dps920ab", > >>>>> + .of_match_table = of_match_ptr(dps920ab_of_match), > >>>>> + }, > >>>>> + .probe_new = dps920ab_probe, > >>>>> +}; > >>>>> + > >>>>> +module_i2c_driver(dps920ab_driver); > >>>>> + > >>>>> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); > >>>>> +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); > >>>>> +MODULE_LICENSE("GPL"); > >>>>> -- > >>>>> 2.31.1 > >>>>> > >>> > >>> > >>> > >> > > > > >
On Fri, May 21, 2021 at 1:41 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/21/21 1:36 AM, Robert Marko wrote: > [ ... ] > >> In this context, I have a hard time finding a reference for > >> this power supply. Do you have a datasheet or some other documents > >> you can share ? > > > > Unfortunately, I don't have a datasheet as that would have made this way easier. > > It was all based on the vendor "driver" from DENT: > > https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c > > > > Ah, so this is not a driver for a power supply from DeltaPSU, > but a power supply from Delta Networks Technology Corporation, > as used in that company's TN48M-POE switch. That is a world > of difference, even though the parent company seems to be the > same. I am not sure if, based on this information, the driver > should claim to be for "Delta DPS-920AB PSU" in the first place. It's actually a PSU from Delta Electronics INC, Delta Networks are just using it inside of the Delta Networks TN48M-DN-P switch. I checked the label on the PSU-s. > > Can you run a block read on MFR_MODEL and MFG_SERIAL ? > That might give us an idea about the actual manufacturer > and model of this power supply. MFG_SERIAL is just a bunch of 0xf-s, but MFR_MODEL has something. However, the Armada 7040 I2C adapter cannot do block reads although it returns 11 bytes but it's just zeros. > > Also, isn't that the same power supply for which you were > trying to add another hwmon driver to display some of its > status information, obtained from some CPLD ? This and one more as the non-PoE version of the TN48M-DN switch has a single 150W PSU that does not support PMBus, but the CPLD always provides presence information and Power Good status. > > Thanks, > Guenter
On 5/21/21 5:33 AM, Robert Marko wrote: > On Fri, May 21, 2021 at 1:41 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/21/21 1:36 AM, Robert Marko wrote: >> [ ... ] >>>> In this context, I have a hard time finding a reference for >>>> this power supply. Do you have a datasheet or some other documents >>>> you can share ? >>> >>> Unfortunately, I don't have a datasheet as that would have made this way easier. >>> It was all based on the vendor "driver" from DENT: >>> https://github.com/dentproject/dentOS/blob/main/packages/platforms/delta/arm64/tn48m/tn48m-poe/modules/builds/src/arm64-delta-tn48m-poe-psu.c >>> >> >> Ah, so this is not a driver for a power supply from DeltaPSU, >> but a power supply from Delta Networks Technology Corporation, >> as used in that company's TN48M-POE switch. That is a world >> of difference, even though the parent company seems to be the >> same. I am not sure if, based on this information, the driver >> should claim to be for "Delta DPS-920AB PSU" in the first place. > > It's actually a PSU from Delta Electronics INC, Delta Networks are just using > it inside of the Delta Networks TN48M-DN-P switch. > I checked the label on the PSU-s. Ok, thanks. Guess we'll have to live with that. >> >> Can you run a block read on MFR_MODEL and MFG_SERIAL ? >> That might give us an idea about the actual manufacturer >> and model of this power supply. > > MFG_SERIAL is just a bunch of 0xf-s, but MFR_MODEL has something. > However, the Armada 7040 I2C adapter cannot do block reads although > it returns 11 bytes but it's just zeros. Hmm, it seems more likely that the field is empty. Block read isn't really different from other read operations. Thanks, Guenter >> >> Also, isn't that the same power supply for which you were >> trying to add another hwmon driver to display some of its >> status information, obtained from some CPLD ? > > This and one more as the non-PoE version of the TN48M-DN switch > has a single 150W PSU that does not support PMBus, but the CPLD > always provides presence information and Power Good status. >> >> Thanks, >> Guenter > > >
On 5/21/21 4:56 AM, Robert Marko wrote: > On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/21/21 1:36 AM, Robert Marko wrote: >>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On 5/19/21 5:38 AM, Robert Marko wrote: >>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>> >>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>>>>>> This adds support for the Delta DPS-920AB PSU. >>>>>>> >>>>>>> Only missing feature is fan control which the PSU supports. >>>>>>> >>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>>>>>> --- >>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>>>>>> Documentation/hwmon/index.rst | 1 + >>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>>>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>>>>>> 5 files changed, 154 insertions(+) >>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst >>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>>>>>> >>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>>>>>> new file mode 100644 >>>>>>> index 000000000000..df0aef530c7e >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/hwmon/dps920ab.rst >>>>>>> @@ -0,0 +1,80 @@ >>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>>>>>> + >>>>>>> +Kernel driver dps920ab >>>>>>> +======================== >>>>>>> + >>>>>>> +Supported chips: >>>>>>> + >>>>>>> + * Delta DPS920AB >>>>>>> + >>>>>>> + Prefix: 'dps920ab' >>>>>>> + >>>>>>> + Addresses scanned: - >>>>>>> + >>>>>>> +Authors: >>>>>>> + Robert Marko <robert.marko@sartura.hr> >>>>>>> + >>>>>>> + >>>>>>> +Description >>>>>>> +----------- >>>>>>> + >>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>>>>>> +power supply with PMBus support. >>>>>>> + >>>>>>> +The driver is a client driver to the core PMBus driver. >>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>>>>>> + >>>>>>> + >>>>>>> +Usage Notes >>>>>>> +----------- >>>>>>> + >>>>>>> +This driver does not auto-detect devices. You will have to instantiate the >>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>>>>>> +details. >>>>>>> + >>>>>>> + >>>>>>> +Sysfs entries >>>>>>> +------------- >>>>>>> + >>>>>>> +======================= ====================================================== >>>>>>> +curr1_label "iin" >>>>>>> +curr1_input Measured input current >>>>>>> +curr1_crit Critical maximum current >>>>>>> +curr1_crit_alarm Current critical high alarm >>>>>>> + >>>>>>> +curr2_label "iout1" >>>>>>> +curr2_input Measured output current >>>>>>> +curr2_crit Critical maximum current >>>>>>> +curr2_crit_alarm Current critical high alarm >>>>>>> + >>>>>>> +in1_label "vin" >>>>>>> +in1_input Measured input voltage >>>>>>> +in1_lcrit Critical minimum input voltage >>>>>>> +in1_lcrit_alarm Input voltage critical low alarm >>>>>>> +in1_crit Critical maximum input voltage >>>>>>> +in1_crit_alarm Input voltage critical high alarm >>>>>>> + >>>>>>> +in2_label "vout1" >>>>>>> +in2_input Measured output voltage >>>>>>> +in2_lcrit Critical minimum output voltage >>>>>>> +in2_lcrit_alarm Output voltage critical low alarm >>>>>>> +in2_crit Critical maximum output voltage >>>>>>> +in2_crit_alarm Output voltage critical high alarm >>>>>>> + >>>>>>> +power1_label "pin" >>>>>>> +power1_input Measured input power >>>>>>> +power1_alarm Input power high alarm >>>>>>> + >>>>>>> +power2_label "pout1" >>>>>>> +power2_input Measured output power >>>>>>> + >>>>>>> +temp[1-2]_input Measured temperature >>>>>>> +temp[1-2]_crit Critical high temperature >>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>>>>>> +temp[1-2]_max Maximum temperature >>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm >>>>>>> + >>>>>>> +fan1_alarm Fan 1 warning. >>>>>>> +fan1_fault Fan 1 fault. >>>>>>> +fan1_input Fan 1 speed in RPM. >>>>>>> +======================= ====================================================== >>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 >>>>>>> --- a/Documentation/hwmon/index.rst >>>>>>> +++ b/Documentation/hwmon/index.rst >>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>>>>>> dell-smm-hwmon >>>>>>> dme1737 >>>>>>> drivetemp >>>>>>> + dps920ab >>>>>>> ds1621 >>>>>>> ds620 >>>>>>> emc1403 >>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>>>> index 32d2fc850621..865ade0aa205 100644 >>>>>>> --- a/drivers/hwmon/pmbus/Kconfig >>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig >>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>>>>>> This driver can also be built as a module. If so, the module will >>>>>>> be called ibm-cffps. >>>>>>> >>>>>>> +config SENSORS_DPS920AB >>>>>>> + tristate "Delta DPS920AB Power Supply" >>>>>>> + help >>>>>>> + If you say yes here you get hardware monitoring support for Delta >>>>>>> + DPS920AB Power Supplies. >>>>>>> + >>>>>>> + This driver can also be built as a module. If so, the module will >>>>>>> + be called dps920ab. >>>>>>> + >>>>>>> config SENSORS_INSPUR_IPSPS >>>>>>> tristate "INSPUR Power System Power Supply" >>>>>>> help >>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>>>>>> new file mode 100644 >>>>>>> index 000000000000..d579ed9f879c >>>>>>> --- /dev/null >>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>>>>>> @@ -0,0 +1,63 @@ >>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>>>> +/* >>>>>>> + * Driver for Delta DPS920AB PSU >>>>>>> + * >>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. >>>>>>> + * Copyright (C) 2021 Sartura Ltd. >>>>>>> + */ >>>>>>> + >>>>>>> +#include <linux/i2c.h> >>>>>>> +#include <linux/module.h> >>>>>>> +#include <linux/of_device.h> >>>>>>> +#include "pmbus.h" >>>>>>> + >>>>>>> +static struct pmbus_driver_info dps920ab_info = { >>>>>>> + .pages = 1, >>>>>>> + >>>>>>> + .format[PSC_VOLTAGE_IN] = linear, >>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, >>>>>>> + .format[PSC_CURRENT_IN] = linear, >>>>>>> + .format[PSC_CURRENT_OUT] = linear, >>>>>>> + .format[PSC_POWER] = linear, >>>>>>> + .format[PSC_FAN] = linear, >>>>>>> + .format[PSC_TEMPERATURE] = linear, >>>>>>> + >>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>>>>>> +}; >>>>>>> + >>>>>>> +static int dps920ab_probe(struct i2c_client *client) >>>>>>> +{ >>>>>>> + if (!i2c_check_functionality(client->adapter, >>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | >>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) >>>>>>> + return -ENODEV; >>>>>> >>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add >>>>>> any value. >>>>> >>>>> Ok, makes sense. >>>>>> >>>>>> That makes me wonder: Is this driver needed in the first place, or could >>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? >>>>> >>>>> It could be added as a generic driver, but that creates properties in >>>>> the sysfs that this thing >>>>> does not support like 3 voltage readings, 3 fans, and so on. >>>>> >>>> >>>> Can you be more specific ? What additional unsupported attributes >>>> are added, and why ? Are you saying the PSU reports registers as >>>> existing which do not really exist ? If so, which registers are those ? >>> >>> Sure, when core does probing and autodiscovery, then the following >>> sysfs attributes are created: >>>> >>>> curr1_crit fan3_fault in3_min_alarm temp1_max >>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm >>>> curr1_input fan3_target in3_rated_min temp1_min >>>> curr1_label in1_crit name temp1_min_alarm >>>> curr1_max in1_crit_alarm of_node temp1_rated_max >>>> curr1_max_alarm in1_input power temp2_crit >>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm >>>> curr2_crit in1_lcrit power1_input temp2_input >>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit >>>> curr2_input in1_max power1_max temp2_lcrit_alarm >>>> curr2_label in1_max_alarm power1_rated_max temp2_max >>>> curr2_lcrit in1_min power2_cap temp2_max_alarm >>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min >>>> curr2_max in1_rated_max power2_crit temp2_min_alarm >>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max >>>> curr2_rated_max in2_input power2_input temp3_crit >>>> device in2_label power2_label temp3_crit_alarm >>>> fan1_alarm in3_crit power2_max temp3_input >>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit >>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm >>>> fan1_target in3_label subsystem temp3_max >>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm >>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min >>>> fan2_input in3_max temp1_input temp3_min_alarm >>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max >>>> fan3_alarm in3_min temp1_lcrit_alarm uevent >>> >>> The following return -1, or -500 so they are not supported. >>> * fan2 >>> * in2 >>> >>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, >>> but when auto probing they are fan3 and in3. >>> >>> temp3 actually seems to return a valid temperature despite it not being used in >>> the vendor driver that features were picked from. >>> >> >> Can you run "grep . *" in the hwmon directory so I can see actual values ? >> > Sure: >> >> curr1_crit:-500 >> curr1_crit_alarm:0 >> curr1_input:195 >> curr1_label:iin >> curr1_max:-500 >> curr1_max_alarm:0 >> curr1_rated_max:-500 >> curr2_crit:-500 >> curr2_crit_alarm:0 >> curr2_input:320 >> curr2_label:iout1 >> curr2_lcrit:-500 >> curr2_lcrit_alarm:0 >> curr2_max:18625 >> curr2_max_alarm:0 >> curr2_rated_max:16875 >> grep: device: Is a directory >> fan1_alarm:0 >> fan1_fault:0 >> fan1_input:10000 >> fan1_target:0 >> fan2_alarm:1 >> fan2_fault:1 >> fan2_input:-1 >> fan2_target:-1 >> fan3_alarm:1 >> fan3_fault:1 >> fan3_input:-1 >> fan3_target:-1 >> in1_crit:-500 >> in1_crit_alarm:0 >> in1_input:245250 >> in1_label:vin >> in1_lcrit:-500 >> in1_lcrit_alarm:0 >> in1_max:-500 >> in1_max_alarm:0 >> in1_min:-500 >> in1_min_alarm:0 >> in1_rated_max:-500 >> in1_rated_min:-500 >> in2_input:-500 >> in2_label:vcap >> in3_crit:255996 >> in3_crit_alarm:0 >> in3_input:54511 >> in3_label:vout1 >> in3_lcrit:255996 >> in3_lcrit_alarm:0 >> in3_max:255996 >> in3_max_alarm:0 >> in3_min:255996 >> in3_min_alarm:0 >> in3_rated_max:56136 >> in3_rated_min:52863 >> name:dps920ab >> grep: of_node: Is a directory >> grep: power: Is a directory >> power1_alarm:0 >> power1_input:33250000 >> power1_label:pin >> power1_max:-500000 >> power1_rated_max:-500000 >> power2_cap:-500000 >> power2_cap_alarm:0 >> power2_crit:-500000 >> power2_crit_alarm:0 >> power2_input:17750000 >> power2_label:pout1 >> power2_max:-500000 >> power2_max_alarm:0 >> power2_rated_max:920000000 >> grep: subsystem: Is a directory >> temp1_crit:-500 >> temp1_crit_alarm:0 >> temp1_input:23000 >> temp1_lcrit:-500 >> temp1_lcrit_alarm:0 >> temp1_max:-500 >> temp1_max_alarm:0 >> temp1_min:-500 >> temp1_min_alarm:0 >> temp1_rated_max:-500 >> temp2_crit:-500 >> temp2_crit_alarm:0 >> temp2_input:26000 >> temp2_lcrit:-500 >> temp2_lcrit_alarm:0 >> temp2_max:-500 >> temp2_max_alarm:0 >> temp2_min:-500 >> temp2_min_alarm:0 >> temp2_rated_max:-500 >> temp3_crit:-500 >> temp3_crit_alarm:0 >> temp3_input:30000 >> temp3_lcrit:-500 >> temp3_lcrit_alarm:0 >> temp3_max:-500 >> temp3_max_alarm:0 >> temp3_min:-500 >> temp3_min_alarm:0 >> temp3_rated_max:-500 >> uevent:OF_NAME=psu >> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a >> uevent:OF_COMPATIBLE_0=delta,dps920ab >> uevent:OF_COMPATIBLE_N=1 > Ok, good enough. It looks like the PSU reports values for pretty much everything, including registers which don't exist. With that in mind, please check the attributes generated by your driver - I suspect that some of the limit attributes are not really supported (maybe none of them is supported). Thanks, Guenter
On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/21/21 4:56 AM, Robert Marko wrote: > > On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 5/21/21 1:36 AM, Robert Marko wrote: > >>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>> On 5/19/21 5:38 AM, Robert Marko wrote: > >>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>> > >>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>>>>>> This adds support for the Delta DPS-920AB PSU. > >>>>>>> > >>>>>>> Only missing feature is fan control which the PSU supports. > >>>>>>> > >>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>>>>>> --- > >>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>>>>>> Documentation/hwmon/index.rst | 1 + > >>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>>>>>> drivers/hwmon/pmbus/Makefile | 1 + > >>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>>>>>> 5 files changed, 154 insertions(+) > >>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>>>>>> > >>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>>>>>> new file mode 100644 > >>>>>>> index 000000000000..df0aef530c7e > >>>>>>> --- /dev/null > >>>>>>> +++ b/Documentation/hwmon/dps920ab.rst > >>>>>>> @@ -0,0 +1,80 @@ > >>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>> + > >>>>>>> +Kernel driver dps920ab > >>>>>>> +======================== > >>>>>>> + > >>>>>>> +Supported chips: > >>>>>>> + > >>>>>>> + * Delta DPS920AB > >>>>>>> + > >>>>>>> + Prefix: 'dps920ab' > >>>>>>> + > >>>>>>> + Addresses scanned: - > >>>>>>> + > >>>>>>> +Authors: > >>>>>>> + Robert Marko <robert.marko@sartura.hr> > >>>>>>> + > >>>>>>> + > >>>>>>> +Description > >>>>>>> +----------- > >>>>>>> + > >>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>>>>>> +power supply with PMBus support. > >>>>>>> + > >>>>>>> +The driver is a client driver to the core PMBus driver. > >>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>>>>>> + > >>>>>>> + > >>>>>>> +Usage Notes > >>>>>>> +----------- > >>>>>>> + > >>>>>>> +This driver does not auto-detect devices. You will have to instantiate the > >>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>>>>>> +details. > >>>>>>> + > >>>>>>> + > >>>>>>> +Sysfs entries > >>>>>>> +------------- > >>>>>>> + > >>>>>>> +======================= ====================================================== > >>>>>>> +curr1_label "iin" > >>>>>>> +curr1_input Measured input current > >>>>>>> +curr1_crit Critical maximum current > >>>>>>> +curr1_crit_alarm Current critical high alarm > >>>>>>> + > >>>>>>> +curr2_label "iout1" > >>>>>>> +curr2_input Measured output current > >>>>>>> +curr2_crit Critical maximum current > >>>>>>> +curr2_crit_alarm Current critical high alarm > >>>>>>> + > >>>>>>> +in1_label "vin" > >>>>>>> +in1_input Measured input voltage > >>>>>>> +in1_lcrit Critical minimum input voltage > >>>>>>> +in1_lcrit_alarm Input voltage critical low alarm > >>>>>>> +in1_crit Critical maximum input voltage > >>>>>>> +in1_crit_alarm Input voltage critical high alarm > >>>>>>> + > >>>>>>> +in2_label "vout1" > >>>>>>> +in2_input Measured output voltage > >>>>>>> +in2_lcrit Critical minimum output voltage > >>>>>>> +in2_lcrit_alarm Output voltage critical low alarm > >>>>>>> +in2_crit Critical maximum output voltage > >>>>>>> +in2_crit_alarm Output voltage critical high alarm > >>>>>>> + > >>>>>>> +power1_label "pin" > >>>>>>> +power1_input Measured input power > >>>>>>> +power1_alarm Input power high alarm > >>>>>>> + > >>>>>>> +power2_label "pout1" > >>>>>>> +power2_input Measured output power > >>>>>>> + > >>>>>>> +temp[1-2]_input Measured temperature > >>>>>>> +temp[1-2]_crit Critical high temperature > >>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>>>>>> +temp[1-2]_max Maximum temperature > >>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm > >>>>>>> + > >>>>>>> +fan1_alarm Fan 1 warning. > >>>>>>> +fan1_fault Fan 1 fault. > >>>>>>> +fan1_input Fan 1 speed in RPM. > >>>>>>> +======================= ====================================================== > >>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 > >>>>>>> --- a/Documentation/hwmon/index.rst > >>>>>>> +++ b/Documentation/hwmon/index.rst > >>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>>>>>> dell-smm-hwmon > >>>>>>> dme1737 > >>>>>>> drivetemp > >>>>>>> + dps920ab > >>>>>>> ds1621 > >>>>>>> ds620 > >>>>>>> emc1403 > >>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>>>>>> index 32d2fc850621..865ade0aa205 100644 > >>>>>>> --- a/drivers/hwmon/pmbus/Kconfig > >>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig > >>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>>>>>> This driver can also be built as a module. If so, the module will > >>>>>>> be called ibm-cffps. > >>>>>>> > >>>>>>> +config SENSORS_DPS920AB > >>>>>>> + tristate "Delta DPS920AB Power Supply" > >>>>>>> + help > >>>>>>> + If you say yes here you get hardware monitoring support for Delta > >>>>>>> + DPS920AB Power Supplies. > >>>>>>> + > >>>>>>> + This driver can also be built as a module. If so, the module will > >>>>>>> + be called dps920ab. > >>>>>>> + > >>>>>>> config SENSORS_INSPUR_IPSPS > >>>>>>> tristate "INSPUR Power System Power Supply" > >>>>>>> help > >>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>> new file mode 100644 > >>>>>>> index 000000000000..d579ed9f879c > >>>>>>> --- /dev/null > >>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>> @@ -0,0 +1,63 @@ > >>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>> +/* > >>>>>>> + * Driver for Delta DPS920AB PSU > >>>>>>> + * > >>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. > >>>>>>> + * Copyright (C) 2021 Sartura Ltd. > >>>>>>> + */ > >>>>>>> + > >>>>>>> +#include <linux/i2c.h> > >>>>>>> +#include <linux/module.h> > >>>>>>> +#include <linux/of_device.h> > >>>>>>> +#include "pmbus.h" > >>>>>>> + > >>>>>>> +static struct pmbus_driver_info dps920ab_info = { > >>>>>>> + .pages = 1, > >>>>>>> + > >>>>>>> + .format[PSC_VOLTAGE_IN] = linear, > >>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, > >>>>>>> + .format[PSC_CURRENT_IN] = linear, > >>>>>>> + .format[PSC_CURRENT_OUT] = linear, > >>>>>>> + .format[PSC_POWER] = linear, > >>>>>>> + .format[PSC_FAN] = linear, > >>>>>>> + .format[PSC_TEMPERATURE] = linear, > >>>>>>> + > >>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>>>>>> +}; > >>>>>>> + > >>>>>>> +static int dps920ab_probe(struct i2c_client *client) > >>>>>>> +{ > >>>>>>> + if (!i2c_check_functionality(client->adapter, > >>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>>>>>> + return -ENODEV; > >>>>>> > >>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add > >>>>>> any value. > >>>>> > >>>>> Ok, makes sense. > >>>>>> > >>>>>> That makes me wonder: Is this driver needed in the first place, or could > >>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > >>>>> > >>>>> It could be added as a generic driver, but that creates properties in > >>>>> the sysfs that this thing > >>>>> does not support like 3 voltage readings, 3 fans, and so on. > >>>>> > >>>> > >>>> Can you be more specific ? What additional unsupported attributes > >>>> are added, and why ? Are you saying the PSU reports registers as > >>>> existing which do not really exist ? If so, which registers are those ? > >>> > >>> Sure, when core does probing and autodiscovery, then the following > >>> sysfs attributes are created: > >>>> > >>>> curr1_crit fan3_fault in3_min_alarm temp1_max > >>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > >>>> curr1_input fan3_target in3_rated_min temp1_min > >>>> curr1_label in1_crit name temp1_min_alarm > >>>> curr1_max in1_crit_alarm of_node temp1_rated_max > >>>> curr1_max_alarm in1_input power temp2_crit > >>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > >>>> curr2_crit in1_lcrit power1_input temp2_input > >>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > >>>> curr2_input in1_max power1_max temp2_lcrit_alarm > >>>> curr2_label in1_max_alarm power1_rated_max temp2_max > >>>> curr2_lcrit in1_min power2_cap temp2_max_alarm > >>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > >>>> curr2_max in1_rated_max power2_crit temp2_min_alarm > >>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > >>>> curr2_rated_max in2_input power2_input temp3_crit > >>>> device in2_label power2_label temp3_crit_alarm > >>>> fan1_alarm in3_crit power2_max temp3_input > >>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > >>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > >>>> fan1_target in3_label subsystem temp3_max > >>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > >>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > >>>> fan2_input in3_max temp1_input temp3_min_alarm > >>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > >>>> fan3_alarm in3_min temp1_lcrit_alarm uevent > >>> > >>> The following return -1, or -500 so they are not supported. > >>> * fan2 > >>> * in2 > >>> > >>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > >>> but when auto probing they are fan3 and in3. > >>> > >>> temp3 actually seems to return a valid temperature despite it not being used in > >>> the vendor driver that features were picked from. > >>> > >> > >> Can you run "grep . *" in the hwmon directory so I can see actual values ? > >> > > Sure: > >> > >> curr1_crit:-500 > >> curr1_crit_alarm:0 > >> curr1_input:195 > >> curr1_label:iin > >> curr1_max:-500 > >> curr1_max_alarm:0 > >> curr1_rated_max:-500 > >> curr2_crit:-500 > >> curr2_crit_alarm:0 > >> curr2_input:320 > >> curr2_label:iout1 > >> curr2_lcrit:-500 > >> curr2_lcrit_alarm:0 > >> curr2_max:18625 > >> curr2_max_alarm:0 > >> curr2_rated_max:16875 > >> grep: device: Is a directory > >> fan1_alarm:0 > >> fan1_fault:0 > >> fan1_input:10000 > >> fan1_target:0 > >> fan2_alarm:1 > >> fan2_fault:1 > >> fan2_input:-1 > >> fan2_target:-1 > >> fan3_alarm:1 > >> fan3_fault:1 > >> fan3_input:-1 > >> fan3_target:-1 > >> in1_crit:-500 > >> in1_crit_alarm:0 > >> in1_input:245250 > >> in1_label:vin > >> in1_lcrit:-500 > >> in1_lcrit_alarm:0 > >> in1_max:-500 > >> in1_max_alarm:0 > >> in1_min:-500 > >> in1_min_alarm:0 > >> in1_rated_max:-500 > >> in1_rated_min:-500 > >> in2_input:-500 > >> in2_label:vcap > >> in3_crit:255996 > >> in3_crit_alarm:0 > >> in3_input:54511 > >> in3_label:vout1 > >> in3_lcrit:255996 > >> in3_lcrit_alarm:0 > >> in3_max:255996 > >> in3_max_alarm:0 > >> in3_min:255996 > >> in3_min_alarm:0 > >> in3_rated_max:56136 > >> in3_rated_min:52863 > >> name:dps920ab > >> grep: of_node: Is a directory > >> grep: power: Is a directory > >> power1_alarm:0 > >> power1_input:33250000 > >> power1_label:pin > >> power1_max:-500000 > >> power1_rated_max:-500000 > >> power2_cap:-500000 > >> power2_cap_alarm:0 > >> power2_crit:-500000 > >> power2_crit_alarm:0 > >> power2_input:17750000 > >> power2_label:pout1 > >> power2_max:-500000 > >> power2_max_alarm:0 > >> power2_rated_max:920000000 > >> grep: subsystem: Is a directory > >> temp1_crit:-500 > >> temp1_crit_alarm:0 > >> temp1_input:23000 > >> temp1_lcrit:-500 > >> temp1_lcrit_alarm:0 > >> temp1_max:-500 > >> temp1_max_alarm:0 > >> temp1_min:-500 > >> temp1_min_alarm:0 > >> temp1_rated_max:-500 > >> temp2_crit:-500 > >> temp2_crit_alarm:0 > >> temp2_input:26000 > >> temp2_lcrit:-500 > >> temp2_lcrit_alarm:0 > >> temp2_max:-500 > >> temp2_max_alarm:0 > >> temp2_min:-500 > >> temp2_min_alarm:0 > >> temp2_rated_max:-500 > >> temp3_crit:-500 > >> temp3_crit_alarm:0 > >> temp3_input:30000 > >> temp3_lcrit:-500 > >> temp3_lcrit_alarm:0 > >> temp3_max:-500 > >> temp3_max_alarm:0 > >> temp3_min:-500 > >> temp3_min_alarm:0 > >> temp3_rated_max:-500 > >> uevent:OF_NAME=psu > >> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > >> uevent:OF_COMPATIBLE_0=delta,dps920ab > >> uevent:OF_COMPATIBLE_N=1 > > > Ok, good enough. It looks like the PSU reports values for pretty much everything, > including registers which don't exist. With that in mind, please check > the attributes generated by your driver - I suspect that some of the limit > attributes are not really supported (maybe none of them is supported). Yeah, I also think that none of those limits are actually supported. Does the core expose a way to not register those? Regards, Robert > > Thanks, > Guenter
On 5/24/21 5:21 AM, Robert Marko wrote: > On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/21/21 4:56 AM, Robert Marko wrote: >>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On 5/21/21 1:36 AM, Robert Marko wrote: >>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>> >>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: >>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>> >>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>>>>>>>> This adds support for the Delta DPS-920AB PSU. >>>>>>>>> >>>>>>>>> Only missing feature is fan control which the PSU supports. >>>>>>>>> >>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>>>>>>>> --- >>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>>>>>>>> Documentation/hwmon/index.rst | 1 + >>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>>>>>>>> 5 files changed, 154 insertions(+) >>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst >>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>>>>>>>> >>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..df0aef530c7e >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst >>>>>>>>> @@ -0,0 +1,80 @@ >>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>> + >>>>>>>>> +Kernel driver dps920ab >>>>>>>>> +======================== >>>>>>>>> + >>>>>>>>> +Supported chips: >>>>>>>>> + >>>>>>>>> + * Delta DPS920AB >>>>>>>>> + >>>>>>>>> + Prefix: 'dps920ab' >>>>>>>>> + >>>>>>>>> + Addresses scanned: - >>>>>>>>> + >>>>>>>>> +Authors: >>>>>>>>> + Robert Marko <robert.marko@sartura.hr> >>>>>>>>> + >>>>>>>>> + >>>>>>>>> +Description >>>>>>>>> +----------- >>>>>>>>> + >>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>>>>>>>> +power supply with PMBus support. >>>>>>>>> + >>>>>>>>> +The driver is a client driver to the core PMBus driver. >>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>>>>>>>> + >>>>>>>>> + >>>>>>>>> +Usage Notes >>>>>>>>> +----------- >>>>>>>>> + >>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the >>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>>>>>>>> +details. >>>>>>>>> + >>>>>>>>> + >>>>>>>>> +Sysfs entries >>>>>>>>> +------------- >>>>>>>>> + >>>>>>>>> +======================= ====================================================== >>>>>>>>> +curr1_label "iin" >>>>>>>>> +curr1_input Measured input current >>>>>>>>> +curr1_crit Critical maximum current >>>>>>>>> +curr1_crit_alarm Current critical high alarm >>>>>>>>> + >>>>>>>>> +curr2_label "iout1" >>>>>>>>> +curr2_input Measured output current >>>>>>>>> +curr2_crit Critical maximum current >>>>>>>>> +curr2_crit_alarm Current critical high alarm >>>>>>>>> + >>>>>>>>> +in1_label "vin" >>>>>>>>> +in1_input Measured input voltage >>>>>>>>> +in1_lcrit Critical minimum input voltage >>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm >>>>>>>>> +in1_crit Critical maximum input voltage >>>>>>>>> +in1_crit_alarm Input voltage critical high alarm >>>>>>>>> + >>>>>>>>> +in2_label "vout1" >>>>>>>>> +in2_input Measured output voltage >>>>>>>>> +in2_lcrit Critical minimum output voltage >>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm >>>>>>>>> +in2_crit Critical maximum output voltage >>>>>>>>> +in2_crit_alarm Output voltage critical high alarm >>>>>>>>> + >>>>>>>>> +power1_label "pin" >>>>>>>>> +power1_input Measured input power >>>>>>>>> +power1_alarm Input power high alarm >>>>>>>>> + >>>>>>>>> +power2_label "pout1" >>>>>>>>> +power2_input Measured output power >>>>>>>>> + >>>>>>>>> +temp[1-2]_input Measured temperature >>>>>>>>> +temp[1-2]_crit Critical high temperature >>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>>>>>>>> +temp[1-2]_max Maximum temperature >>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm >>>>>>>>> + >>>>>>>>> +fan1_alarm Fan 1 warning. >>>>>>>>> +fan1_fault Fan 1 fault. >>>>>>>>> +fan1_input Fan 1 speed in RPM. >>>>>>>>> +======================= ====================================================== >>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 >>>>>>>>> --- a/Documentation/hwmon/index.rst >>>>>>>>> +++ b/Documentation/hwmon/index.rst >>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>>>>>>>> dell-smm-hwmon >>>>>>>>> dme1737 >>>>>>>>> drivetemp >>>>>>>>> + dps920ab >>>>>>>>> ds1621 >>>>>>>>> ds620 >>>>>>>>> emc1403 >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>>>>>> index 32d2fc850621..865ade0aa205 100644 >>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig >>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig >>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>>>>>>>> This driver can also be built as a module. If so, the module will >>>>>>>>> be called ibm-cffps. >>>>>>>>> >>>>>>>>> +config SENSORS_DPS920AB >>>>>>>>> + tristate "Delta DPS920AB Power Supply" >>>>>>>>> + help >>>>>>>>> + If you say yes here you get hardware monitoring support for Delta >>>>>>>>> + DPS920AB Power Supplies. >>>>>>>>> + >>>>>>>>> + This driver can also be built as a module. If so, the module will >>>>>>>>> + be called dps920ab. >>>>>>>>> + >>>>>>>>> config SENSORS_INSPUR_IPSPS >>>>>>>>> tristate "INSPUR Power System Power Supply" >>>>>>>>> help >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..d579ed9f879c >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>> @@ -0,0 +1,63 @@ >>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>> +/* >>>>>>>>> + * Driver for Delta DPS920AB PSU >>>>>>>>> + * >>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. >>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#include <linux/i2c.h> >>>>>>>>> +#include <linux/module.h> >>>>>>>>> +#include <linux/of_device.h> >>>>>>>>> +#include "pmbus.h" >>>>>>>>> + >>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { >>>>>>>>> + .pages = 1, >>>>>>>>> + >>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, >>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, >>>>>>>>> + .format[PSC_CURRENT_IN] = linear, >>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, >>>>>>>>> + .format[PSC_POWER] = linear, >>>>>>>>> + .format[PSC_FAN] = linear, >>>>>>>>> + .format[PSC_TEMPERATURE] = linear, >>>>>>>>> + >>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) >>>>>>>>> +{ >>>>>>>>> + if (!i2c_check_functionality(client->adapter, >>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | >>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) >>>>>>>>> + return -ENODEV; >>>>>>>> >>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add >>>>>>>> any value. >>>>>>> >>>>>>> Ok, makes sense. >>>>>>>> >>>>>>>> That makes me wonder: Is this driver needed in the first place, or could >>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? >>>>>>> >>>>>>> It could be added as a generic driver, but that creates properties in >>>>>>> the sysfs that this thing >>>>>>> does not support like 3 voltage readings, 3 fans, and so on. >>>>>>> >>>>>> >>>>>> Can you be more specific ? What additional unsupported attributes >>>>>> are added, and why ? Are you saying the PSU reports registers as >>>>>> existing which do not really exist ? If so, which registers are those ? >>>>> >>>>> Sure, when core does probing and autodiscovery, then the following >>>>> sysfs attributes are created: >>>>>> >>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max >>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm >>>>>> curr1_input fan3_target in3_rated_min temp1_min >>>>>> curr1_label in1_crit name temp1_min_alarm >>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max >>>>>> curr1_max_alarm in1_input power temp2_crit >>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm >>>>>> curr2_crit in1_lcrit power1_input temp2_input >>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit >>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm >>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max >>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm >>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min >>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm >>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max >>>>>> curr2_rated_max in2_input power2_input temp3_crit >>>>>> device in2_label power2_label temp3_crit_alarm >>>>>> fan1_alarm in3_crit power2_max temp3_input >>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit >>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm >>>>>> fan1_target in3_label subsystem temp3_max >>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm >>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min >>>>>> fan2_input in3_max temp1_input temp3_min_alarm >>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max >>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent >>>>> >>>>> The following return -1, or -500 so they are not supported. >>>>> * fan2 >>>>> * in2 >>>>> >>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, >>>>> but when auto probing they are fan3 and in3. >>>>> >>>>> temp3 actually seems to return a valid temperature despite it not being used in >>>>> the vendor driver that features were picked from. >>>>> >>>> >>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? >>>> >>> Sure: >>>> >>>> curr1_crit:-500 >>>> curr1_crit_alarm:0 >>>> curr1_input:195 >>>> curr1_label:iin >>>> curr1_max:-500 >>>> curr1_max_alarm:0 >>>> curr1_rated_max:-500 >>>> curr2_crit:-500 >>>> curr2_crit_alarm:0 >>>> curr2_input:320 >>>> curr2_label:iout1 >>>> curr2_lcrit:-500 >>>> curr2_lcrit_alarm:0 >>>> curr2_max:18625 >>>> curr2_max_alarm:0 >>>> curr2_rated_max:16875 >>>> grep: device: Is a directory >>>> fan1_alarm:0 >>>> fan1_fault:0 >>>> fan1_input:10000 >>>> fan1_target:0 >>>> fan2_alarm:1 >>>> fan2_fault:1 >>>> fan2_input:-1 >>>> fan2_target:-1 >>>> fan3_alarm:1 >>>> fan3_fault:1 >>>> fan3_input:-1 >>>> fan3_target:-1 >>>> in1_crit:-500 >>>> in1_crit_alarm:0 >>>> in1_input:245250 >>>> in1_label:vin >>>> in1_lcrit:-500 >>>> in1_lcrit_alarm:0 >>>> in1_max:-500 >>>> in1_max_alarm:0 >>>> in1_min:-500 >>>> in1_min_alarm:0 >>>> in1_rated_max:-500 >>>> in1_rated_min:-500 >>>> in2_input:-500 >>>> in2_label:vcap >>>> in3_crit:255996 >>>> in3_crit_alarm:0 >>>> in3_input:54511 >>>> in3_label:vout1 >>>> in3_lcrit:255996 >>>> in3_lcrit_alarm:0 >>>> in3_max:255996 >>>> in3_max_alarm:0 >>>> in3_min:255996 >>>> in3_min_alarm:0 >>>> in3_rated_max:56136 >>>> in3_rated_min:52863 >>>> name:dps920ab >>>> grep: of_node: Is a directory >>>> grep: power: Is a directory >>>> power1_alarm:0 >>>> power1_input:33250000 >>>> power1_label:pin >>>> power1_max:-500000 >>>> power1_rated_max:-500000 >>>> power2_cap:-500000 >>>> power2_cap_alarm:0 >>>> power2_crit:-500000 >>>> power2_crit_alarm:0 >>>> power2_input:17750000 >>>> power2_label:pout1 >>>> power2_max:-500000 >>>> power2_max_alarm:0 >>>> power2_rated_max:920000000 >>>> grep: subsystem: Is a directory >>>> temp1_crit:-500 >>>> temp1_crit_alarm:0 >>>> temp1_input:23000 >>>> temp1_lcrit:-500 >>>> temp1_lcrit_alarm:0 >>>> temp1_max:-500 >>>> temp1_max_alarm:0 >>>> temp1_min:-500 >>>> temp1_min_alarm:0 >>>> temp1_rated_max:-500 >>>> temp2_crit:-500 >>>> temp2_crit_alarm:0 >>>> temp2_input:26000 >>>> temp2_lcrit:-500 >>>> temp2_lcrit_alarm:0 >>>> temp2_max:-500 >>>> temp2_max_alarm:0 >>>> temp2_min:-500 >>>> temp2_min_alarm:0 >>>> temp2_rated_max:-500 >>>> temp3_crit:-500 >>>> temp3_crit_alarm:0 >>>> temp3_input:30000 >>>> temp3_lcrit:-500 >>>> temp3_lcrit_alarm:0 >>>> temp3_max:-500 >>>> temp3_max_alarm:0 >>>> temp3_min:-500 >>>> temp3_min_alarm:0 >>>> temp3_rated_max:-500 >>>> uevent:OF_NAME=psu >>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a >>>> uevent:OF_COMPATIBLE_0=delta,dps920ab >>>> uevent:OF_COMPATIBLE_N=1 >>> >> Ok, good enough. It looks like the PSU reports values for pretty much everything, >> including registers which don't exist. With that in mind, please check >> the attributes generated by your driver - I suspect that some of the limit >> attributes are not really supported (maybe none of them is supported). > > Yeah, I also think that none of those limits are actually supported. > Does the core expose a way to not register those? > Have the read_word function return -ENXIO for unsupported registers (and add a comment to the code explaining why you do that). See drivers/hwmon/pmbus/fsp-3y.c for an example. Guenter
On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/24/21 5:21 AM, Robert Marko wrote: > > On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 5/21/21 4:56 AM, Robert Marko wrote: > >>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>> On 5/21/21 1:36 AM, Robert Marko wrote: > >>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>> > >>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: > >>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>> > >>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>>>>>>>> This adds support for the Delta DPS-920AB PSU. > >>>>>>>>> > >>>>>>>>> Only missing feature is fan control which the PSU supports. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>>>>>>>> --- > >>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>>>>>>>> Documentation/hwmon/index.rst | 1 + > >>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + > >>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>>>>>>>> 5 files changed, 154 insertions(+) > >>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>> > >>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>>>>>>>> new file mode 100644 > >>>>>>>>> index 000000000000..df0aef530c7e > >>>>>>>>> --- /dev/null > >>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst > >>>>>>>>> @@ -0,0 +1,80 @@ > >>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>> + > >>>>>>>>> +Kernel driver dps920ab > >>>>>>>>> +======================== > >>>>>>>>> + > >>>>>>>>> +Supported chips: > >>>>>>>>> + > >>>>>>>>> + * Delta DPS920AB > >>>>>>>>> + > >>>>>>>>> + Prefix: 'dps920ab' > >>>>>>>>> + > >>>>>>>>> + Addresses scanned: - > >>>>>>>>> + > >>>>>>>>> +Authors: > >>>>>>>>> + Robert Marko <robert.marko@sartura.hr> > >>>>>>>>> + > >>>>>>>>> + > >>>>>>>>> +Description > >>>>>>>>> +----------- > >>>>>>>>> + > >>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>>>>>>>> +power supply with PMBus support. > >>>>>>>>> + > >>>>>>>>> +The driver is a client driver to the core PMBus driver. > >>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>>>>>>>> + > >>>>>>>>> + > >>>>>>>>> +Usage Notes > >>>>>>>>> +----------- > >>>>>>>>> + > >>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the > >>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>>>>>>>> +details. > >>>>>>>>> + > >>>>>>>>> + > >>>>>>>>> +Sysfs entries > >>>>>>>>> +------------- > >>>>>>>>> + > >>>>>>>>> +======================= ====================================================== > >>>>>>>>> +curr1_label "iin" > >>>>>>>>> +curr1_input Measured input current > >>>>>>>>> +curr1_crit Critical maximum current > >>>>>>>>> +curr1_crit_alarm Current critical high alarm > >>>>>>>>> + > >>>>>>>>> +curr2_label "iout1" > >>>>>>>>> +curr2_input Measured output current > >>>>>>>>> +curr2_crit Critical maximum current > >>>>>>>>> +curr2_crit_alarm Current critical high alarm > >>>>>>>>> + > >>>>>>>>> +in1_label "vin" > >>>>>>>>> +in1_input Measured input voltage > >>>>>>>>> +in1_lcrit Critical minimum input voltage > >>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm > >>>>>>>>> +in1_crit Critical maximum input voltage > >>>>>>>>> +in1_crit_alarm Input voltage critical high alarm > >>>>>>>>> + > >>>>>>>>> +in2_label "vout1" > >>>>>>>>> +in2_input Measured output voltage > >>>>>>>>> +in2_lcrit Critical minimum output voltage > >>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm > >>>>>>>>> +in2_crit Critical maximum output voltage > >>>>>>>>> +in2_crit_alarm Output voltage critical high alarm > >>>>>>>>> + > >>>>>>>>> +power1_label "pin" > >>>>>>>>> +power1_input Measured input power > >>>>>>>>> +power1_alarm Input power high alarm > >>>>>>>>> + > >>>>>>>>> +power2_label "pout1" > >>>>>>>>> +power2_input Measured output power > >>>>>>>>> + > >>>>>>>>> +temp[1-2]_input Measured temperature > >>>>>>>>> +temp[1-2]_crit Critical high temperature > >>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>>>>>>>> +temp[1-2]_max Maximum temperature > >>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm > >>>>>>>>> + > >>>>>>>>> +fan1_alarm Fan 1 warning. > >>>>>>>>> +fan1_fault Fan 1 fault. > >>>>>>>>> +fan1_input Fan 1 speed in RPM. > >>>>>>>>> +======================= ====================================================== > >>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 > >>>>>>>>> --- a/Documentation/hwmon/index.rst > >>>>>>>>> +++ b/Documentation/hwmon/index.rst > >>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>>>>>>>> dell-smm-hwmon > >>>>>>>>> dme1737 > >>>>>>>>> drivetemp > >>>>>>>>> + dps920ab > >>>>>>>>> ds1621 > >>>>>>>>> ds620 > >>>>>>>>> emc1403 > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>> index 32d2fc850621..865ade0aa205 100644 > >>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig > >>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>>>>>>>> This driver can also be built as a module. If so, the module will > >>>>>>>>> be called ibm-cffps. > >>>>>>>>> > >>>>>>>>> +config SENSORS_DPS920AB > >>>>>>>>> + tristate "Delta DPS920AB Power Supply" > >>>>>>>>> + help > >>>>>>>>> + If you say yes here you get hardware monitoring support for Delta > >>>>>>>>> + DPS920AB Power Supplies. > >>>>>>>>> + > >>>>>>>>> + This driver can also be built as a module. If so, the module will > >>>>>>>>> + be called dps920ab. > >>>>>>>>> + > >>>>>>>>> config SENSORS_INSPUR_IPSPS > >>>>>>>>> tristate "INSPUR Power System Power Supply" > >>>>>>>>> help > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>> new file mode 100644 > >>>>>>>>> index 000000000000..d579ed9f879c > >>>>>>>>> --- /dev/null > >>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>> @@ -0,0 +1,63 @@ > >>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>> +/* > >>>>>>>>> + * Driver for Delta DPS920AB PSU > >>>>>>>>> + * > >>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. > >>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. > >>>>>>>>> + */ > >>>>>>>>> + > >>>>>>>>> +#include <linux/i2c.h> > >>>>>>>>> +#include <linux/module.h> > >>>>>>>>> +#include <linux/of_device.h> > >>>>>>>>> +#include "pmbus.h" > >>>>>>>>> + > >>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { > >>>>>>>>> + .pages = 1, > >>>>>>>>> + > >>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, > >>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, > >>>>>>>>> + .format[PSC_CURRENT_IN] = linear, > >>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, > >>>>>>>>> + .format[PSC_POWER] = linear, > >>>>>>>>> + .format[PSC_FAN] = linear, > >>>>>>>>> + .format[PSC_TEMPERATURE] = linear, > >>>>>>>>> + > >>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>>>>>>>> +}; > >>>>>>>>> + > >>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) > >>>>>>>>> +{ > >>>>>>>>> + if (!i2c_check_functionality(client->adapter, > >>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>>>>>>>> + return -ENODEV; > >>>>>>>> > >>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add > >>>>>>>> any value. > >>>>>>> > >>>>>>> Ok, makes sense. > >>>>>>>> > >>>>>>>> That makes me wonder: Is this driver needed in the first place, or could > >>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > >>>>>>> > >>>>>>> It could be added as a generic driver, but that creates properties in > >>>>>>> the sysfs that this thing > >>>>>>> does not support like 3 voltage readings, 3 fans, and so on. > >>>>>>> > >>>>>> > >>>>>> Can you be more specific ? What additional unsupported attributes > >>>>>> are added, and why ? Are you saying the PSU reports registers as > >>>>>> existing which do not really exist ? If so, which registers are those ? > >>>>> > >>>>> Sure, when core does probing and autodiscovery, then the following > >>>>> sysfs attributes are created: > >>>>>> > >>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max > >>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > >>>>>> curr1_input fan3_target in3_rated_min temp1_min > >>>>>> curr1_label in1_crit name temp1_min_alarm > >>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max > >>>>>> curr1_max_alarm in1_input power temp2_crit > >>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > >>>>>> curr2_crit in1_lcrit power1_input temp2_input > >>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > >>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm > >>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max > >>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm > >>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > >>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm > >>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > >>>>>> curr2_rated_max in2_input power2_input temp3_crit > >>>>>> device in2_label power2_label temp3_crit_alarm > >>>>>> fan1_alarm in3_crit power2_max temp3_input > >>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > >>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > >>>>>> fan1_target in3_label subsystem temp3_max > >>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > >>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > >>>>>> fan2_input in3_max temp1_input temp3_min_alarm > >>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > >>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent > >>>>> > >>>>> The following return -1, or -500 so they are not supported. > >>>>> * fan2 > >>>>> * in2 > >>>>> > >>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > >>>>> but when auto probing they are fan3 and in3. > >>>>> > >>>>> temp3 actually seems to return a valid temperature despite it not being used in > >>>>> the vendor driver that features were picked from. > >>>>> > >>>> > >>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? > >>>> > >>> Sure: > >>>> > >>>> curr1_crit:-500 > >>>> curr1_crit_alarm:0 > >>>> curr1_input:195 > >>>> curr1_label:iin > >>>> curr1_max:-500 > >>>> curr1_max_alarm:0 > >>>> curr1_rated_max:-500 > >>>> curr2_crit:-500 > >>>> curr2_crit_alarm:0 > >>>> curr2_input:320 > >>>> curr2_label:iout1 > >>>> curr2_lcrit:-500 > >>>> curr2_lcrit_alarm:0 > >>>> curr2_max:18625 > >>>> curr2_max_alarm:0 > >>>> curr2_rated_max:16875 > >>>> grep: device: Is a directory > >>>> fan1_alarm:0 > >>>> fan1_fault:0 > >>>> fan1_input:10000 > >>>> fan1_target:0 > >>>> fan2_alarm:1 > >>>> fan2_fault:1 > >>>> fan2_input:-1 > >>>> fan2_target:-1 > >>>> fan3_alarm:1 > >>>> fan3_fault:1 > >>>> fan3_input:-1 > >>>> fan3_target:-1 > >>>> in1_crit:-500 > >>>> in1_crit_alarm:0 > >>>> in1_input:245250 > >>>> in1_label:vin > >>>> in1_lcrit:-500 > >>>> in1_lcrit_alarm:0 > >>>> in1_max:-500 > >>>> in1_max_alarm:0 > >>>> in1_min:-500 > >>>> in1_min_alarm:0 > >>>> in1_rated_max:-500 > >>>> in1_rated_min:-500 > >>>> in2_input:-500 > >>>> in2_label:vcap > >>>> in3_crit:255996 > >>>> in3_crit_alarm:0 > >>>> in3_input:54511 > >>>> in3_label:vout1 > >>>> in3_lcrit:255996 > >>>> in3_lcrit_alarm:0 > >>>> in3_max:255996 > >>>> in3_max_alarm:0 > >>>> in3_min:255996 > >>>> in3_min_alarm:0 > >>>> in3_rated_max:56136 > >>>> in3_rated_min:52863 > >>>> name:dps920ab > >>>> grep: of_node: Is a directory > >>>> grep: power: Is a directory > >>>> power1_alarm:0 > >>>> power1_input:33250000 > >>>> power1_label:pin > >>>> power1_max:-500000 > >>>> power1_rated_max:-500000 > >>>> power2_cap:-500000 > >>>> power2_cap_alarm:0 > >>>> power2_crit:-500000 > >>>> power2_crit_alarm:0 > >>>> power2_input:17750000 > >>>> power2_label:pout1 > >>>> power2_max:-500000 > >>>> power2_max_alarm:0 > >>>> power2_rated_max:920000000 > >>>> grep: subsystem: Is a directory > >>>> temp1_crit:-500 > >>>> temp1_crit_alarm:0 > >>>> temp1_input:23000 > >>>> temp1_lcrit:-500 > >>>> temp1_lcrit_alarm:0 > >>>> temp1_max:-500 > >>>> temp1_max_alarm:0 > >>>> temp1_min:-500 > >>>> temp1_min_alarm:0 > >>>> temp1_rated_max:-500 > >>>> temp2_crit:-500 > >>>> temp2_crit_alarm:0 > >>>> temp2_input:26000 > >>>> temp2_lcrit:-500 > >>>> temp2_lcrit_alarm:0 > >>>> temp2_max:-500 > >>>> temp2_max_alarm:0 > >>>> temp2_min:-500 > >>>> temp2_min_alarm:0 > >>>> temp2_rated_max:-500 > >>>> temp3_crit:-500 > >>>> temp3_crit_alarm:0 > >>>> temp3_input:30000 > >>>> temp3_lcrit:-500 > >>>> temp3_lcrit_alarm:0 > >>>> temp3_max:-500 > >>>> temp3_max_alarm:0 > >>>> temp3_min:-500 > >>>> temp3_min_alarm:0 > >>>> temp3_rated_max:-500 > >>>> uevent:OF_NAME=psu > >>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > >>>> uevent:OF_COMPATIBLE_0=delta,dps920ab > >>>> uevent:OF_COMPATIBLE_N=1 > >>> > >> Ok, good enough. It looks like the PSU reports values for pretty much everything, > >> including registers which don't exist. With that in mind, please check > >> the attributes generated by your driver - I suspect that some of the limit > >> attributes are not really supported (maybe none of them is supported). > > > > Yeah, I also think that none of those limits are actually supported. > > Does the core expose a way to not register those? > > > > Have the read_word function return -ENXIO for unsupported registers > (and add a comment to the code explaining why you do that). > See drivers/hwmon/pmbus/fsp-3y.c for an example. Thanks, will implement that. In the meantime I finally got the PMBus command list that the PSU supports. It implements PMBus 1.2, but it also claims that MFR_ID, MFR_MODEL and MFR_REVISION are supported. Block read returns what looks like a correct length, but the buffer is empty. > > [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: > [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: > [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: > [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 > [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: > [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 > [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: > [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: > [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 > [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: > [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 > [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: However, manually reading using i2cget in word mode returns something that looks ok. i2cget -y 0 0x5a 0x9a w 0x440b This would be D is ASCII, it looks like any kind of block read fails actually. I am now a bit confused. I tried applying the block support for mv64xx as well: https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ But no luck. Regards, Robert > > Guenter
On Thu, May 27, 2021 at 12:22 PM Robert Marko <robert.marko@sartura.hr> wrote: > > On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 5/24/21 5:21 AM, Robert Marko wrote: > > > On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > > >> > > >> On 5/21/21 4:56 AM, Robert Marko wrote: > > >>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > > >>>> > > >>>> On 5/21/21 1:36 AM, Robert Marko wrote: > > >>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>>> > > >>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: > > >>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>>>>> > > >>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > > >>>>>>>>> This adds support for the Delta DPS-920AB PSU. > > >>>>>>>>> > > >>>>>>>>> Only missing feature is fan control which the PSU supports. > > >>>>>>>>> > > >>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > >>>>>>>>> --- > > >>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > > >>>>>>>>> Documentation/hwmon/index.rst | 1 + > > >>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > > >>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + > > >>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > > >>>>>>>>> 5 files changed, 154 insertions(+) > > >>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > > >>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > > >>>>>>>>> > > >>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > > >>>>>>>>> new file mode 100644 > > >>>>>>>>> index 000000000000..df0aef530c7e > > >>>>>>>>> --- /dev/null > > >>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst > > >>>>>>>>> @@ -0,0 +1,80 @@ > > >>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > > >>>>>>>>> + > > >>>>>>>>> +Kernel driver dps920ab > > >>>>>>>>> +======================== > > >>>>>>>>> + > > >>>>>>>>> +Supported chips: > > >>>>>>>>> + > > >>>>>>>>> + * Delta DPS920AB > > >>>>>>>>> + > > >>>>>>>>> + Prefix: 'dps920ab' > > >>>>>>>>> + > > >>>>>>>>> + Addresses scanned: - > > >>>>>>>>> + > > >>>>>>>>> +Authors: > > >>>>>>>>> + Robert Marko <robert.marko@sartura.hr> > > >>>>>>>>> + > > >>>>>>>>> + > > >>>>>>>>> +Description > > >>>>>>>>> +----------- > > >>>>>>>>> + > > >>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > > >>>>>>>>> +power supply with PMBus support. > > >>>>>>>>> + > > >>>>>>>>> +The driver is a client driver to the core PMBus driver. > > >>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > > >>>>>>>>> + > > >>>>>>>>> + > > >>>>>>>>> +Usage Notes > > >>>>>>>>> +----------- > > >>>>>>>>> + > > >>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the > > >>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > > >>>>>>>>> +details. > > >>>>>>>>> + > > >>>>>>>>> + > > >>>>>>>>> +Sysfs entries > > >>>>>>>>> +------------- > > >>>>>>>>> + > > >>>>>>>>> +======================= ====================================================== > > >>>>>>>>> +curr1_label "iin" > > >>>>>>>>> +curr1_input Measured input current > > >>>>>>>>> +curr1_crit Critical maximum current > > >>>>>>>>> +curr1_crit_alarm Current critical high alarm > > >>>>>>>>> + > > >>>>>>>>> +curr2_label "iout1" > > >>>>>>>>> +curr2_input Measured output current > > >>>>>>>>> +curr2_crit Critical maximum current > > >>>>>>>>> +curr2_crit_alarm Current critical high alarm > > >>>>>>>>> + > > >>>>>>>>> +in1_label "vin" > > >>>>>>>>> +in1_input Measured input voltage > > >>>>>>>>> +in1_lcrit Critical minimum input voltage > > >>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm > > >>>>>>>>> +in1_crit Critical maximum input voltage > > >>>>>>>>> +in1_crit_alarm Input voltage critical high alarm > > >>>>>>>>> + > > >>>>>>>>> +in2_label "vout1" > > >>>>>>>>> +in2_input Measured output voltage > > >>>>>>>>> +in2_lcrit Critical minimum output voltage > > >>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm > > >>>>>>>>> +in2_crit Critical maximum output voltage > > >>>>>>>>> +in2_crit_alarm Output voltage critical high alarm > > >>>>>>>>> + > > >>>>>>>>> +power1_label "pin" > > >>>>>>>>> +power1_input Measured input power > > >>>>>>>>> +power1_alarm Input power high alarm > > >>>>>>>>> + > > >>>>>>>>> +power2_label "pout1" > > >>>>>>>>> +power2_input Measured output power > > >>>>>>>>> + > > >>>>>>>>> +temp[1-2]_input Measured temperature > > >>>>>>>>> +temp[1-2]_crit Critical high temperature > > >>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > > >>>>>>>>> +temp[1-2]_max Maximum temperature > > >>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm > > >>>>>>>>> + > > >>>>>>>>> +fan1_alarm Fan 1 warning. > > >>>>>>>>> +fan1_fault Fan 1 fault. > > >>>>>>>>> +fan1_input Fan 1 speed in RPM. > > >>>>>>>>> +======================= ====================================================== > > >>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > > >>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 > > >>>>>>>>> --- a/Documentation/hwmon/index.rst > > >>>>>>>>> +++ b/Documentation/hwmon/index.rst > > >>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > > >>>>>>>>> dell-smm-hwmon > > >>>>>>>>> dme1737 > > >>>>>>>>> drivetemp > > >>>>>>>>> + dps920ab > > >>>>>>>>> ds1621 > > >>>>>>>>> ds620 > > >>>>>>>>> emc1403 > > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > > >>>>>>>>> index 32d2fc850621..865ade0aa205 100644 > > >>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig > > >>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig > > >>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > > >>>>>>>>> This driver can also be built as a module. If so, the module will > > >>>>>>>>> be called ibm-cffps. > > >>>>>>>>> > > >>>>>>>>> +config SENSORS_DPS920AB > > >>>>>>>>> + tristate "Delta DPS920AB Power Supply" > > >>>>>>>>> + help > > >>>>>>>>> + If you say yes here you get hardware monitoring support for Delta > > >>>>>>>>> + DPS920AB Power Supplies. > > >>>>>>>>> + > > >>>>>>>>> + This driver can also be built as a module. If so, the module will > > >>>>>>>>> + be called dps920ab. > > >>>>>>>>> + > > >>>>>>>>> config SENSORS_INSPUR_IPSPS > > >>>>>>>>> tristate "INSPUR Power System Power Supply" > > >>>>>>>>> help > > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > > >>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > > >>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > > >>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > > >>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > > >>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > > >>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > > >>>>>>>>> new file mode 100644 > > >>>>>>>>> index 000000000000..d579ed9f879c > > >>>>>>>>> --- /dev/null > > >>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > > >>>>>>>>> @@ -0,0 +1,63 @@ > > >>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > > >>>>>>>>> +/* > > >>>>>>>>> + * Driver for Delta DPS920AB PSU > > >>>>>>>>> + * > > >>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. > > >>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. > > >>>>>>>>> + */ > > >>>>>>>>> + > > >>>>>>>>> +#include <linux/i2c.h> > > >>>>>>>>> +#include <linux/module.h> > > >>>>>>>>> +#include <linux/of_device.h> > > >>>>>>>>> +#include "pmbus.h" > > >>>>>>>>> + > > >>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { > > >>>>>>>>> + .pages = 1, > > >>>>>>>>> + > > >>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, > > >>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, > > >>>>>>>>> + .format[PSC_CURRENT_IN] = linear, > > >>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, > > >>>>>>>>> + .format[PSC_POWER] = linear, > > >>>>>>>>> + .format[PSC_FAN] = linear, > > >>>>>>>>> + .format[PSC_TEMPERATURE] = linear, > > >>>>>>>>> + > > >>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > > >>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > > >>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > > >>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > > >>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > > >>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > > >>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > > >>>>>>>>> +}; > > >>>>>>>>> + > > >>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) > > >>>>>>>>> +{ > > >>>>>>>>> + if (!i2c_check_functionality(client->adapter, > > >>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > > >>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > > >>>>>>>>> + return -ENODEV; > > >>>>>>>> > > >>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add > > >>>>>>>> any value. > > >>>>>>> > > >>>>>>> Ok, makes sense. > > >>>>>>>> > > >>>>>>>> That makes me wonder: Is this driver needed in the first place, or could > > >>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > > >>>>>>> > > >>>>>>> It could be added as a generic driver, but that creates properties in > > >>>>>>> the sysfs that this thing > > >>>>>>> does not support like 3 voltage readings, 3 fans, and so on. > > >>>>>>> > > >>>>>> > > >>>>>> Can you be more specific ? What additional unsupported attributes > > >>>>>> are added, and why ? Are you saying the PSU reports registers as > > >>>>>> existing which do not really exist ? If so, which registers are those ? > > >>>>> > > >>>>> Sure, when core does probing and autodiscovery, then the following > > >>>>> sysfs attributes are created: > > >>>>>> > > >>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max > > >>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > > >>>>>> curr1_input fan3_target in3_rated_min temp1_min > > >>>>>> curr1_label in1_crit name temp1_min_alarm > > >>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max > > >>>>>> curr1_max_alarm in1_input power temp2_crit > > >>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > > >>>>>> curr2_crit in1_lcrit power1_input temp2_input > > >>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > > >>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm > > >>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max > > >>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm > > >>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > > >>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm > > >>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > > >>>>>> curr2_rated_max in2_input power2_input temp3_crit > > >>>>>> device in2_label power2_label temp3_crit_alarm > > >>>>>> fan1_alarm in3_crit power2_max temp3_input > > >>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > > >>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > > >>>>>> fan1_target in3_label subsystem temp3_max > > >>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > > >>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > > >>>>>> fan2_input in3_max temp1_input temp3_min_alarm > > >>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > > >>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent > > >>>>> > > >>>>> The following return -1, or -500 so they are not supported. > > >>>>> * fan2 > > >>>>> * in2 > > >>>>> > > >>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > > >>>>> but when auto probing they are fan3 and in3. > > >>>>> > > >>>>> temp3 actually seems to return a valid temperature despite it not being used in > > >>>>> the vendor driver that features were picked from. > > >>>>> > > >>>> > > >>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? > > >>>> > > >>> Sure: > > >>>> > > >>>> curr1_crit:-500 > > >>>> curr1_crit_alarm:0 > > >>>> curr1_input:195 > > >>>> curr1_label:iin > > >>>> curr1_max:-500 > > >>>> curr1_max_alarm:0 > > >>>> curr1_rated_max:-500 > > >>>> curr2_crit:-500 > > >>>> curr2_crit_alarm:0 > > >>>> curr2_input:320 > > >>>> curr2_label:iout1 > > >>>> curr2_lcrit:-500 > > >>>> curr2_lcrit_alarm:0 > > >>>> curr2_max:18625 > > >>>> curr2_max_alarm:0 > > >>>> curr2_rated_max:16875 > > >>>> grep: device: Is a directory > > >>>> fan1_alarm:0 > > >>>> fan1_fault:0 > > >>>> fan1_input:10000 > > >>>> fan1_target:0 > > >>>> fan2_alarm:1 > > >>>> fan2_fault:1 > > >>>> fan2_input:-1 > > >>>> fan2_target:-1 > > >>>> fan3_alarm:1 > > >>>> fan3_fault:1 > > >>>> fan3_input:-1 > > >>>> fan3_target:-1 > > >>>> in1_crit:-500 > > >>>> in1_crit_alarm:0 > > >>>> in1_input:245250 > > >>>> in1_label:vin > > >>>> in1_lcrit:-500 > > >>>> in1_lcrit_alarm:0 > > >>>> in1_max:-500 > > >>>> in1_max_alarm:0 > > >>>> in1_min:-500 > > >>>> in1_min_alarm:0 > > >>>> in1_rated_max:-500 > > >>>> in1_rated_min:-500 > > >>>> in2_input:-500 > > >>>> in2_label:vcap > > >>>> in3_crit:255996 > > >>>> in3_crit_alarm:0 > > >>>> in3_input:54511 > > >>>> in3_label:vout1 > > >>>> in3_lcrit:255996 > > >>>> in3_lcrit_alarm:0 > > >>>> in3_max:255996 > > >>>> in3_max_alarm:0 > > >>>> in3_min:255996 > > >>>> in3_min_alarm:0 > > >>>> in3_rated_max:56136 > > >>>> in3_rated_min:52863 > > >>>> name:dps920ab > > >>>> grep: of_node: Is a directory > > >>>> grep: power: Is a directory > > >>>> power1_alarm:0 > > >>>> power1_input:33250000 > > >>>> power1_label:pin > > >>>> power1_max:-500000 > > >>>> power1_rated_max:-500000 > > >>>> power2_cap:-500000 > > >>>> power2_cap_alarm:0 > > >>>> power2_crit:-500000 > > >>>> power2_crit_alarm:0 > > >>>> power2_input:17750000 > > >>>> power2_label:pout1 > > >>>> power2_max:-500000 > > >>>> power2_max_alarm:0 > > >>>> power2_rated_max:920000000 > > >>>> grep: subsystem: Is a directory > > >>>> temp1_crit:-500 > > >>>> temp1_crit_alarm:0 > > >>>> temp1_input:23000 > > >>>> temp1_lcrit:-500 > > >>>> temp1_lcrit_alarm:0 > > >>>> temp1_max:-500 > > >>>> temp1_max_alarm:0 > > >>>> temp1_min:-500 > > >>>> temp1_min_alarm:0 > > >>>> temp1_rated_max:-500 > > >>>> temp2_crit:-500 > > >>>> temp2_crit_alarm:0 > > >>>> temp2_input:26000 > > >>>> temp2_lcrit:-500 > > >>>> temp2_lcrit_alarm:0 > > >>>> temp2_max:-500 > > >>>> temp2_max_alarm:0 > > >>>> temp2_min:-500 > > >>>> temp2_min_alarm:0 > > >>>> temp2_rated_max:-500 > > >>>> temp3_crit:-500 > > >>>> temp3_crit_alarm:0 > > >>>> temp3_input:30000 > > >>>> temp3_lcrit:-500 > > >>>> temp3_lcrit_alarm:0 > > >>>> temp3_max:-500 > > >>>> temp3_max_alarm:0 > > >>>> temp3_min:-500 > > >>>> temp3_min_alarm:0 > > >>>> temp3_rated_max:-500 > > >>>> uevent:OF_NAME=psu > > >>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > > >>>> uevent:OF_COMPATIBLE_0=delta,dps920ab > > >>>> uevent:OF_COMPATIBLE_N=1 > > >>> > > >> Ok, good enough. It looks like the PSU reports values for pretty much everything, > > >> including registers which don't exist. With that in mind, please check > > >> the attributes generated by your driver - I suspect that some of the limit > > >> attributes are not really supported (maybe none of them is supported). > > > > > > Yeah, I also think that none of those limits are actually supported. > > > Does the core expose a way to not register those? > > > > > > > Have the read_word function return -ENXIO for unsupported registers > > (and add a comment to the code explaining why you do that). > > See drivers/hwmon/pmbus/fsp-3y.c for an example. > > Thanks, will implement that. > In the meantime I finally got the PMBus command list that > the PSU supports. > It implements PMBus 1.2, but it also claims that MFR_ID, > MFR_MODEL and MFR_REVISION are supported. > > Block read returns what looks like a correct length, but the buffer > is empty. > > > > [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > > [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: > > [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > > [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: > > [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > > [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: > > [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 > > [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: > > [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 > > [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: > > [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > > [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: > > [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 > > [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: > > [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 > > [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: > > > However, manually reading using i2cget in word mode returns > something that looks ok. > i2cget -y 0 0x5a 0x9a w > 0x440b > > This would be D is ASCII, it looks like any kind of block read fails actually. Ok, so after using i2c_smbus_read_i2c_block_data() I can actually read the model kind of OK. There actually are 10 bytes of valid data, no idea why i2c_smbus_read_block_data() does not work. > > [ 721.194706] dps920ab 0-005a: MFR MODEL: > DPS-920AB > [ 721.245502] dps920ab 0-005b: MFR MODEL: > DPS-920AB It behaves weirdly when printing by creating a newline before and stuff like that. Regards, Robert > > I am now a bit confused. > I tried applying the block support for mv64xx as well: > https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > > But no luck. > > Regards, > Robert > > > > Guenter > > > > -- > Robert Marko > Staff Embedded Linux Engineer > Sartura Ltd. > Lendavska ulica 16a > 10000 Zagreb, Croatia > Email: robert.marko@sartura.hr > Web: www.sartura.hr
On 5/27/21 3:59 AM, Robert Marko wrote: > On Thu, May 27, 2021 at 12:22 PM Robert Marko <robert.marko@sartura.hr> wrote: >> >> On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 5/24/21 5:21 AM, Robert Marko wrote: >>>> On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>> >>>>> On 5/21/21 4:56 AM, Robert Marko wrote: >>>>>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>> >>>>>>> On 5/21/21 1:36 AM, Robert Marko wrote: >>>>>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>>> >>>>>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: >>>>>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>>>>> >>>>>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>>>>>>>>>>> This adds support for the Delta DPS-920AB PSU. >>>>>>>>>>>> >>>>>>>>>>>> Only missing feature is fan control which the PSU supports. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>>>>>>>>>>> --- >>>>>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>>>>>>>>>>> Documentation/hwmon/index.rst | 1 + >>>>>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>>>>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>>>>>>>>>>> 5 files changed, 154 insertions(+) >>>>>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst >>>>>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>>>>>>>>>>> new file mode 100644 >>>>>>>>>>>> index 000000000000..df0aef530c7e >>>>>>>>>>>> --- /dev/null >>>>>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst >>>>>>>>>>>> @@ -0,0 +1,80 @@ >>>>>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>>>>> + >>>>>>>>>>>> +Kernel driver dps920ab >>>>>>>>>>>> +======================== >>>>>>>>>>>> + >>>>>>>>>>>> +Supported chips: >>>>>>>>>>>> + >>>>>>>>>>>> + * Delta DPS920AB >>>>>>>>>>>> + >>>>>>>>>>>> + Prefix: 'dps920ab' >>>>>>>>>>>> + >>>>>>>>>>>> + Addresses scanned: - >>>>>>>>>>>> + >>>>>>>>>>>> +Authors: >>>>>>>>>>>> + Robert Marko <robert.marko@sartura.hr> >>>>>>>>>>>> + >>>>>>>>>>>> + >>>>>>>>>>>> +Description >>>>>>>>>>>> +----------- >>>>>>>>>>>> + >>>>>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>>>>>>>>>>> +power supply with PMBus support. >>>>>>>>>>>> + >>>>>>>>>>>> +The driver is a client driver to the core PMBus driver. >>>>>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>>>>>>>>>>> + >>>>>>>>>>>> + >>>>>>>>>>>> +Usage Notes >>>>>>>>>>>> +----------- >>>>>>>>>>>> + >>>>>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the >>>>>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>>>>>>>>>>> +details. >>>>>>>>>>>> + >>>>>>>>>>>> + >>>>>>>>>>>> +Sysfs entries >>>>>>>>>>>> +------------- >>>>>>>>>>>> + >>>>>>>>>>>> +======================= ====================================================== >>>>>>>>>>>> +curr1_label "iin" >>>>>>>>>>>> +curr1_input Measured input current >>>>>>>>>>>> +curr1_crit Critical maximum current >>>>>>>>>>>> +curr1_crit_alarm Current critical high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +curr2_label "iout1" >>>>>>>>>>>> +curr2_input Measured output current >>>>>>>>>>>> +curr2_crit Critical maximum current >>>>>>>>>>>> +curr2_crit_alarm Current critical high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +in1_label "vin" >>>>>>>>>>>> +in1_input Measured input voltage >>>>>>>>>>>> +in1_lcrit Critical minimum input voltage >>>>>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm >>>>>>>>>>>> +in1_crit Critical maximum input voltage >>>>>>>>>>>> +in1_crit_alarm Input voltage critical high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +in2_label "vout1" >>>>>>>>>>>> +in2_input Measured output voltage >>>>>>>>>>>> +in2_lcrit Critical minimum output voltage >>>>>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm >>>>>>>>>>>> +in2_crit Critical maximum output voltage >>>>>>>>>>>> +in2_crit_alarm Output voltage critical high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +power1_label "pin" >>>>>>>>>>>> +power1_input Measured input power >>>>>>>>>>>> +power1_alarm Input power high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +power2_label "pout1" >>>>>>>>>>>> +power2_input Measured output power >>>>>>>>>>>> + >>>>>>>>>>>> +temp[1-2]_input Measured temperature >>>>>>>>>>>> +temp[1-2]_crit Critical high temperature >>>>>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>>>>>>>>>>> +temp[1-2]_max Maximum temperature >>>>>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm >>>>>>>>>>>> + >>>>>>>>>>>> +fan1_alarm Fan 1 warning. >>>>>>>>>>>> +fan1_fault Fan 1 fault. >>>>>>>>>>>> +fan1_input Fan 1 speed in RPM. >>>>>>>>>>>> +======================= ====================================================== >>>>>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>>>>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 >>>>>>>>>>>> --- a/Documentation/hwmon/index.rst >>>>>>>>>>>> +++ b/Documentation/hwmon/index.rst >>>>>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>>>>>>>>>>> dell-smm-hwmon >>>>>>>>>>>> dme1737 >>>>>>>>>>>> drivetemp >>>>>>>>>>>> + dps920ab >>>>>>>>>>>> ds1621 >>>>>>>>>>>> ds620 >>>>>>>>>>>> emc1403 >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>> index 32d2fc850621..865ade0aa205 100644 >>>>>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>>>>>>>>>>> This driver can also be built as a module. If so, the module will >>>>>>>>>>>> be called ibm-cffps. >>>>>>>>>>>> >>>>>>>>>>>> +config SENSORS_DPS920AB >>>>>>>>>>>> + tristate "Delta DPS920AB Power Supply" >>>>>>>>>>>> + help >>>>>>>>>>>> + If you say yes here you get hardware monitoring support for Delta >>>>>>>>>>>> + DPS920AB Power Supplies. >>>>>>>>>>>> + >>>>>>>>>>>> + This driver can also be built as a module. If so, the module will >>>>>>>>>>>> + be called dps920ab. >>>>>>>>>>>> + >>>>>>>>>>>> config SENSORS_INSPUR_IPSPS >>>>>>>>>>>> tristate "INSPUR Power System Power Supply" >>>>>>>>>>>> help >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>>>>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>> new file mode 100644 >>>>>>>>>>>> index 000000000000..d579ed9f879c >>>>>>>>>>>> --- /dev/null >>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>> @@ -0,0 +1,63 @@ >>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>>>>> +/* >>>>>>>>>>>> + * Driver for Delta DPS920AB PSU >>>>>>>>>>>> + * >>>>>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. >>>>>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. >>>>>>>>>>>> + */ >>>>>>>>>>>> + >>>>>>>>>>>> +#include <linux/i2c.h> >>>>>>>>>>>> +#include <linux/module.h> >>>>>>>>>>>> +#include <linux/of_device.h> >>>>>>>>>>>> +#include "pmbus.h" >>>>>>>>>>>> + >>>>>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { >>>>>>>>>>>> + .pages = 1, >>>>>>>>>>>> + >>>>>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, >>>>>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, >>>>>>>>>>>> + .format[PSC_CURRENT_IN] = linear, >>>>>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, >>>>>>>>>>>> + .format[PSC_POWER] = linear, >>>>>>>>>>>> + .format[PSC_FAN] = linear, >>>>>>>>>>>> + .format[PSC_TEMPERATURE] = linear, >>>>>>>>>>>> + >>>>>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>>>>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>>>>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>>>>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>>>>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>>>>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>>>>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>>>>>>>>>>> +}; >>>>>>>>>>>> + >>>>>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) >>>>>>>>>>>> +{ >>>>>>>>>>>> + if (!i2c_check_functionality(client->adapter, >>>>>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | >>>>>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) >>>>>>>>>>>> + return -ENODEV; >>>>>>>>>>> >>>>>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add >>>>>>>>>>> any value. >>>>>>>>>> >>>>>>>>>> Ok, makes sense. >>>>>>>>>>> >>>>>>>>>>> That makes me wonder: Is this driver needed in the first place, or could >>>>>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? >>>>>>>>>> >>>>>>>>>> It could be added as a generic driver, but that creates properties in >>>>>>>>>> the sysfs that this thing >>>>>>>>>> does not support like 3 voltage readings, 3 fans, and so on. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Can you be more specific ? What additional unsupported attributes >>>>>>>>> are added, and why ? Are you saying the PSU reports registers as >>>>>>>>> existing which do not really exist ? If so, which registers are those ? >>>>>>>> >>>>>>>> Sure, when core does probing and autodiscovery, then the following >>>>>>>> sysfs attributes are created: >>>>>>>>> >>>>>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max >>>>>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm >>>>>>>>> curr1_input fan3_target in3_rated_min temp1_min >>>>>>>>> curr1_label in1_crit name temp1_min_alarm >>>>>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max >>>>>>>>> curr1_max_alarm in1_input power temp2_crit >>>>>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm >>>>>>>>> curr2_crit in1_lcrit power1_input temp2_input >>>>>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit >>>>>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm >>>>>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max >>>>>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm >>>>>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min >>>>>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm >>>>>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max >>>>>>>>> curr2_rated_max in2_input power2_input temp3_crit >>>>>>>>> device in2_label power2_label temp3_crit_alarm >>>>>>>>> fan1_alarm in3_crit power2_max temp3_input >>>>>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit >>>>>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm >>>>>>>>> fan1_target in3_label subsystem temp3_max >>>>>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm >>>>>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min >>>>>>>>> fan2_input in3_max temp1_input temp3_min_alarm >>>>>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max >>>>>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent >>>>>>>> >>>>>>>> The following return -1, or -500 so they are not supported. >>>>>>>> * fan2 >>>>>>>> * in2 >>>>>>>> >>>>>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, >>>>>>>> but when auto probing they are fan3 and in3. >>>>>>>> >>>>>>>> temp3 actually seems to return a valid temperature despite it not being used in >>>>>>>> the vendor driver that features were picked from. >>>>>>>> >>>>>>> >>>>>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? >>>>>>> >>>>>> Sure: >>>>>>> >>>>>>> curr1_crit:-500 >>>>>>> curr1_crit_alarm:0 >>>>>>> curr1_input:195 >>>>>>> curr1_label:iin >>>>>>> curr1_max:-500 >>>>>>> curr1_max_alarm:0 >>>>>>> curr1_rated_max:-500 >>>>>>> curr2_crit:-500 >>>>>>> curr2_crit_alarm:0 >>>>>>> curr2_input:320 >>>>>>> curr2_label:iout1 >>>>>>> curr2_lcrit:-500 >>>>>>> curr2_lcrit_alarm:0 >>>>>>> curr2_max:18625 >>>>>>> curr2_max_alarm:0 >>>>>>> curr2_rated_max:16875 >>>>>>> grep: device: Is a directory >>>>>>> fan1_alarm:0 >>>>>>> fan1_fault:0 >>>>>>> fan1_input:10000 >>>>>>> fan1_target:0 >>>>>>> fan2_alarm:1 >>>>>>> fan2_fault:1 >>>>>>> fan2_input:-1 >>>>>>> fan2_target:-1 >>>>>>> fan3_alarm:1 >>>>>>> fan3_fault:1 >>>>>>> fan3_input:-1 >>>>>>> fan3_target:-1 >>>>>>> in1_crit:-500 >>>>>>> in1_crit_alarm:0 >>>>>>> in1_input:245250 >>>>>>> in1_label:vin >>>>>>> in1_lcrit:-500 >>>>>>> in1_lcrit_alarm:0 >>>>>>> in1_max:-500 >>>>>>> in1_max_alarm:0 >>>>>>> in1_min:-500 >>>>>>> in1_min_alarm:0 >>>>>>> in1_rated_max:-500 >>>>>>> in1_rated_min:-500 >>>>>>> in2_input:-500 >>>>>>> in2_label:vcap >>>>>>> in3_crit:255996 >>>>>>> in3_crit_alarm:0 >>>>>>> in3_input:54511 >>>>>>> in3_label:vout1 >>>>>>> in3_lcrit:255996 >>>>>>> in3_lcrit_alarm:0 >>>>>>> in3_max:255996 >>>>>>> in3_max_alarm:0 >>>>>>> in3_min:255996 >>>>>>> in3_min_alarm:0 >>>>>>> in3_rated_max:56136 >>>>>>> in3_rated_min:52863 >>>>>>> name:dps920ab >>>>>>> grep: of_node: Is a directory >>>>>>> grep: power: Is a directory >>>>>>> power1_alarm:0 >>>>>>> power1_input:33250000 >>>>>>> power1_label:pin >>>>>>> power1_max:-500000 >>>>>>> power1_rated_max:-500000 >>>>>>> power2_cap:-500000 >>>>>>> power2_cap_alarm:0 >>>>>>> power2_crit:-500000 >>>>>>> power2_crit_alarm:0 >>>>>>> power2_input:17750000 >>>>>>> power2_label:pout1 >>>>>>> power2_max:-500000 >>>>>>> power2_max_alarm:0 >>>>>>> power2_rated_max:920000000 >>>>>>> grep: subsystem: Is a directory >>>>>>> temp1_crit:-500 >>>>>>> temp1_crit_alarm:0 >>>>>>> temp1_input:23000 >>>>>>> temp1_lcrit:-500 >>>>>>> temp1_lcrit_alarm:0 >>>>>>> temp1_max:-500 >>>>>>> temp1_max_alarm:0 >>>>>>> temp1_min:-500 >>>>>>> temp1_min_alarm:0 >>>>>>> temp1_rated_max:-500 >>>>>>> temp2_crit:-500 >>>>>>> temp2_crit_alarm:0 >>>>>>> temp2_input:26000 >>>>>>> temp2_lcrit:-500 >>>>>>> temp2_lcrit_alarm:0 >>>>>>> temp2_max:-500 >>>>>>> temp2_max_alarm:0 >>>>>>> temp2_min:-500 >>>>>>> temp2_min_alarm:0 >>>>>>> temp2_rated_max:-500 >>>>>>> temp3_crit:-500 >>>>>>> temp3_crit_alarm:0 >>>>>>> temp3_input:30000 >>>>>>> temp3_lcrit:-500 >>>>>>> temp3_lcrit_alarm:0 >>>>>>> temp3_max:-500 >>>>>>> temp3_max_alarm:0 >>>>>>> temp3_min:-500 >>>>>>> temp3_min_alarm:0 >>>>>>> temp3_rated_max:-500 >>>>>>> uevent:OF_NAME=psu >>>>>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a >>>>>>> uevent:OF_COMPATIBLE_0=delta,dps920ab >>>>>>> uevent:OF_COMPATIBLE_N=1 >>>>>> >>>>> Ok, good enough. It looks like the PSU reports values for pretty much everything, >>>>> including registers which don't exist. With that in mind, please check >>>>> the attributes generated by your driver - I suspect that some of the limit >>>>> attributes are not really supported (maybe none of them is supported). >>>> >>>> Yeah, I also think that none of those limits are actually supported. >>>> Does the core expose a way to not register those? >>>> >>> >>> Have the read_word function return -ENXIO for unsupported registers >>> (and add a comment to the code explaining why you do that). >>> See drivers/hwmon/pmbus/fsp-3y.c for an example. >> >> Thanks, will implement that. >> In the meantime I finally got the PMBus command list that >> the PSU supports. >> It implements PMBus 1.2, but it also claims that MFR_ID, >> MFR_MODEL and MFR_REVISION are supported. >> >> Block read returns what looks like a correct length, but the buffer >> is empty. >>> >>> [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 >>> [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: >>> [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 >>> [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: >>> [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 >>> [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: >>> [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 >>> [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: >>> [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 >>> [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: >>> [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 >>> [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: >>> [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 >>> [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: >>> [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 >>> [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: >> >> >> However, manually reading using i2cget in word mode returns >> something that looks ok. >> i2cget -y 0 0x5a 0x9a w >> 0x440b >> >> This would be D is ASCII, it looks like any kind of block read fails actually. > > Ok, so after using i2c_smbus_read_i2c_block_data() I can actually read > the model kind of OK. > There actually are 10 bytes of valid data, no idea why > i2c_smbus_read_block_data() does not work. >> >> [ 721.194706] dps920ab 0-005a: MFR MODEL: >> DPS-920AB >> [ 721.245502] dps920ab 0-005b: MFR MODEL: >> DPS-920AB > > It behaves weirdly when printing by creating a newline before and > stuff like that. > I suspect it prints the first byte, which should be the length. I also suspect that the i2c controller driver has a bug and doesn't support SMBus block data correctly. Did you try i2cdump with i and s options ? >> I tried applying the block support for mv64xx as well: >> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ That patch would be needed, but it looks buggy to me. This chunk: + drv_data->effective_length = data+1; + drv_data->bytes_left = data+1; + drv_data->msg->len = data+1; should be: + drv_data->effective_length = data+1; + drv_data->bytes_left = data; + drv_data->msg->len = data+1; It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, and bail out if it isn't. Do you see a difference with the patch applied ? Guenter
On Thu, May 27, 2021 at 4:03 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/27/21 3:59 AM, Robert Marko wrote: > > On Thu, May 27, 2021 at 12:22 PM Robert Marko <robert.marko@sartura.hr> wrote: > >> > >> On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>> > >>> On 5/24/21 5:21 AM, Robert Marko wrote: > >>>> On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>> > >>>>> On 5/21/21 4:56 AM, Robert Marko wrote: > >>>>>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>> > >>>>>>> On 5/21/21 1:36 AM, Robert Marko wrote: > >>>>>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>>> > >>>>>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: > >>>>>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>>>>> > >>>>>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>>>>>>>>>>> This adds support for the Delta DPS-920AB PSU. > >>>>>>>>>>>> > >>>>>>>>>>>> Only missing feature is fan control which the PSU supports. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>>>>>>>>>>> --- > >>>>>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>>>>>>>>>>> Documentation/hwmon/index.rst | 1 + > >>>>>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>>>>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + > >>>>>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>>>>>>>>>>> 5 files changed, 154 insertions(+) > >>>>>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>> new file mode 100644 > >>>>>>>>>>>> index 000000000000..df0aef530c7e > >>>>>>>>>>>> --- /dev/null > >>>>>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>> @@ -0,0 +1,80 @@ > >>>>>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>>>>> + > >>>>>>>>>>>> +Kernel driver dps920ab > >>>>>>>>>>>> +======================== > >>>>>>>>>>>> + > >>>>>>>>>>>> +Supported chips: > >>>>>>>>>>>> + > >>>>>>>>>>>> + * Delta DPS920AB > >>>>>>>>>>>> + > >>>>>>>>>>>> + Prefix: 'dps920ab' > >>>>>>>>>>>> + > >>>>>>>>>>>> + Addresses scanned: - > >>>>>>>>>>>> + > >>>>>>>>>>>> +Authors: > >>>>>>>>>>>> + Robert Marko <robert.marko@sartura.hr> > >>>>>>>>>>>> + > >>>>>>>>>>>> + > >>>>>>>>>>>> +Description > >>>>>>>>>>>> +----------- > >>>>>>>>>>>> + > >>>>>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>>>>>>>>>>> +power supply with PMBus support. > >>>>>>>>>>>> + > >>>>>>>>>>>> +The driver is a client driver to the core PMBus driver. > >>>>>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>>>>>>>>>>> + > >>>>>>>>>>>> + > >>>>>>>>>>>> +Usage Notes > >>>>>>>>>>>> +----------- > >>>>>>>>>>>> + > >>>>>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the > >>>>>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>>>>>>>>>>> +details. > >>>>>>>>>>>> + > >>>>>>>>>>>> + > >>>>>>>>>>>> +Sysfs entries > >>>>>>>>>>>> +------------- > >>>>>>>>>>>> + > >>>>>>>>>>>> +======================= ====================================================== > >>>>>>>>>>>> +curr1_label "iin" > >>>>>>>>>>>> +curr1_input Measured input current > >>>>>>>>>>>> +curr1_crit Critical maximum current > >>>>>>>>>>>> +curr1_crit_alarm Current critical high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +curr2_label "iout1" > >>>>>>>>>>>> +curr2_input Measured output current > >>>>>>>>>>>> +curr2_crit Critical maximum current > >>>>>>>>>>>> +curr2_crit_alarm Current critical high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +in1_label "vin" > >>>>>>>>>>>> +in1_input Measured input voltage > >>>>>>>>>>>> +in1_lcrit Critical minimum input voltage > >>>>>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm > >>>>>>>>>>>> +in1_crit Critical maximum input voltage > >>>>>>>>>>>> +in1_crit_alarm Input voltage critical high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +in2_label "vout1" > >>>>>>>>>>>> +in2_input Measured output voltage > >>>>>>>>>>>> +in2_lcrit Critical minimum output voltage > >>>>>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm > >>>>>>>>>>>> +in2_crit Critical maximum output voltage > >>>>>>>>>>>> +in2_crit_alarm Output voltage critical high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +power1_label "pin" > >>>>>>>>>>>> +power1_input Measured input power > >>>>>>>>>>>> +power1_alarm Input power high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +power2_label "pout1" > >>>>>>>>>>>> +power2_input Measured output power > >>>>>>>>>>>> + > >>>>>>>>>>>> +temp[1-2]_input Measured temperature > >>>>>>>>>>>> +temp[1-2]_crit Critical high temperature > >>>>>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>>>>>>>>>>> +temp[1-2]_max Maximum temperature > >>>>>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm > >>>>>>>>>>>> + > >>>>>>>>>>>> +fan1_alarm Fan 1 warning. > >>>>>>>>>>>> +fan1_fault Fan 1 fault. > >>>>>>>>>>>> +fan1_input Fan 1 speed in RPM. > >>>>>>>>>>>> +======================= ====================================================== > >>>>>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>>>>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 > >>>>>>>>>>>> --- a/Documentation/hwmon/index.rst > >>>>>>>>>>>> +++ b/Documentation/hwmon/index.rst > >>>>>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>>>>>>>>>>> dell-smm-hwmon > >>>>>>>>>>>> dme1737 > >>>>>>>>>>>> drivetemp > >>>>>>>>>>>> + dps920ab > >>>>>>>>>>>> ds1621 > >>>>>>>>>>>> ds620 > >>>>>>>>>>>> emc1403 > >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>> index 32d2fc850621..865ade0aa205 100644 > >>>>>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>>>>>>>>>>> This driver can also be built as a module. If so, the module will > >>>>>>>>>>>> be called ibm-cffps. > >>>>>>>>>>>> > >>>>>>>>>>>> +config SENSORS_DPS920AB > >>>>>>>>>>>> + tristate "Delta DPS920AB Power Supply" > >>>>>>>>>>>> + help > >>>>>>>>>>>> + If you say yes here you get hardware monitoring support for Delta > >>>>>>>>>>>> + DPS920AB Power Supplies. > >>>>>>>>>>>> + > >>>>>>>>>>>> + This driver can also be built as a module. If so, the module will > >>>>>>>>>>>> + be called dps920ab. > >>>>>>>>>>>> + > >>>>>>>>>>>> config SENSORS_INSPUR_IPSPS > >>>>>>>>>>>> tristate "INSPUR Power System Power Supply" > >>>>>>>>>>>> help > >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>>>>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>>>>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>>>>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>> new file mode 100644 > >>>>>>>>>>>> index 000000000000..d579ed9f879c > >>>>>>>>>>>> --- /dev/null > >>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>> @@ -0,0 +1,63 @@ > >>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>>>>> +/* > >>>>>>>>>>>> + * Driver for Delta DPS920AB PSU > >>>>>>>>>>>> + * > >>>>>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. > >>>>>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. > >>>>>>>>>>>> + */ > >>>>>>>>>>>> + > >>>>>>>>>>>> +#include <linux/i2c.h> > >>>>>>>>>>>> +#include <linux/module.h> > >>>>>>>>>>>> +#include <linux/of_device.h> > >>>>>>>>>>>> +#include "pmbus.h" > >>>>>>>>>>>> + > >>>>>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { > >>>>>>>>>>>> + .pages = 1, > >>>>>>>>>>>> + > >>>>>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, > >>>>>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, > >>>>>>>>>>>> + .format[PSC_CURRENT_IN] = linear, > >>>>>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, > >>>>>>>>>>>> + .format[PSC_POWER] = linear, > >>>>>>>>>>>> + .format[PSC_FAN] = linear, > >>>>>>>>>>>> + .format[PSC_TEMPERATURE] = linear, > >>>>>>>>>>>> + > >>>>>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>>>>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>>>>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>>>>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>>>>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>>>>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>>>>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>>>>>>>>>>> +}; > >>>>>>>>>>>> + > >>>>>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) > >>>>>>>>>>>> +{ > >>>>>>>>>>>> + if (!i2c_check_functionality(client->adapter, > >>>>>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>>>>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>>>>>>>>>>> + return -ENODEV; > >>>>>>>>>>> > >>>>>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add > >>>>>>>>>>> any value. > >>>>>>>>>> > >>>>>>>>>> Ok, makes sense. > >>>>>>>>>>> > >>>>>>>>>>> That makes me wonder: Is this driver needed in the first place, or could > >>>>>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > >>>>>>>>>> > >>>>>>>>>> It could be added as a generic driver, but that creates properties in > >>>>>>>>>> the sysfs that this thing > >>>>>>>>>> does not support like 3 voltage readings, 3 fans, and so on. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> Can you be more specific ? What additional unsupported attributes > >>>>>>>>> are added, and why ? Are you saying the PSU reports registers as > >>>>>>>>> existing which do not really exist ? If so, which registers are those ? > >>>>>>>> > >>>>>>>> Sure, when core does probing and autodiscovery, then the following > >>>>>>>> sysfs attributes are created: > >>>>>>>>> > >>>>>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max > >>>>>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > >>>>>>>>> curr1_input fan3_target in3_rated_min temp1_min > >>>>>>>>> curr1_label in1_crit name temp1_min_alarm > >>>>>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max > >>>>>>>>> curr1_max_alarm in1_input power temp2_crit > >>>>>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > >>>>>>>>> curr2_crit in1_lcrit power1_input temp2_input > >>>>>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > >>>>>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm > >>>>>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max > >>>>>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm > >>>>>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > >>>>>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm > >>>>>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > >>>>>>>>> curr2_rated_max in2_input power2_input temp3_crit > >>>>>>>>> device in2_label power2_label temp3_crit_alarm > >>>>>>>>> fan1_alarm in3_crit power2_max temp3_input > >>>>>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > >>>>>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > >>>>>>>>> fan1_target in3_label subsystem temp3_max > >>>>>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > >>>>>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > >>>>>>>>> fan2_input in3_max temp1_input temp3_min_alarm > >>>>>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > >>>>>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent > >>>>>>>> > >>>>>>>> The following return -1, or -500 so they are not supported. > >>>>>>>> * fan2 > >>>>>>>> * in2 > >>>>>>>> > >>>>>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > >>>>>>>> but when auto probing they are fan3 and in3. > >>>>>>>> > >>>>>>>> temp3 actually seems to return a valid temperature despite it not being used in > >>>>>>>> the vendor driver that features were picked from. > >>>>>>>> > >>>>>>> > >>>>>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? > >>>>>>> > >>>>>> Sure: > >>>>>>> > >>>>>>> curr1_crit:-500 > >>>>>>> curr1_crit_alarm:0 > >>>>>>> curr1_input:195 > >>>>>>> curr1_label:iin > >>>>>>> curr1_max:-500 > >>>>>>> curr1_max_alarm:0 > >>>>>>> curr1_rated_max:-500 > >>>>>>> curr2_crit:-500 > >>>>>>> curr2_crit_alarm:0 > >>>>>>> curr2_input:320 > >>>>>>> curr2_label:iout1 > >>>>>>> curr2_lcrit:-500 > >>>>>>> curr2_lcrit_alarm:0 > >>>>>>> curr2_max:18625 > >>>>>>> curr2_max_alarm:0 > >>>>>>> curr2_rated_max:16875 > >>>>>>> grep: device: Is a directory > >>>>>>> fan1_alarm:0 > >>>>>>> fan1_fault:0 > >>>>>>> fan1_input:10000 > >>>>>>> fan1_target:0 > >>>>>>> fan2_alarm:1 > >>>>>>> fan2_fault:1 > >>>>>>> fan2_input:-1 > >>>>>>> fan2_target:-1 > >>>>>>> fan3_alarm:1 > >>>>>>> fan3_fault:1 > >>>>>>> fan3_input:-1 > >>>>>>> fan3_target:-1 > >>>>>>> in1_crit:-500 > >>>>>>> in1_crit_alarm:0 > >>>>>>> in1_input:245250 > >>>>>>> in1_label:vin > >>>>>>> in1_lcrit:-500 > >>>>>>> in1_lcrit_alarm:0 > >>>>>>> in1_max:-500 > >>>>>>> in1_max_alarm:0 > >>>>>>> in1_min:-500 > >>>>>>> in1_min_alarm:0 > >>>>>>> in1_rated_max:-500 > >>>>>>> in1_rated_min:-500 > >>>>>>> in2_input:-500 > >>>>>>> in2_label:vcap > >>>>>>> in3_crit:255996 > >>>>>>> in3_crit_alarm:0 > >>>>>>> in3_input:54511 > >>>>>>> in3_label:vout1 > >>>>>>> in3_lcrit:255996 > >>>>>>> in3_lcrit_alarm:0 > >>>>>>> in3_max:255996 > >>>>>>> in3_max_alarm:0 > >>>>>>> in3_min:255996 > >>>>>>> in3_min_alarm:0 > >>>>>>> in3_rated_max:56136 > >>>>>>> in3_rated_min:52863 > >>>>>>> name:dps920ab > >>>>>>> grep: of_node: Is a directory > >>>>>>> grep: power: Is a directory > >>>>>>> power1_alarm:0 > >>>>>>> power1_input:33250000 > >>>>>>> power1_label:pin > >>>>>>> power1_max:-500000 > >>>>>>> power1_rated_max:-500000 > >>>>>>> power2_cap:-500000 > >>>>>>> power2_cap_alarm:0 > >>>>>>> power2_crit:-500000 > >>>>>>> power2_crit_alarm:0 > >>>>>>> power2_input:17750000 > >>>>>>> power2_label:pout1 > >>>>>>> power2_max:-500000 > >>>>>>> power2_max_alarm:0 > >>>>>>> power2_rated_max:920000000 > >>>>>>> grep: subsystem: Is a directory > >>>>>>> temp1_crit:-500 > >>>>>>> temp1_crit_alarm:0 > >>>>>>> temp1_input:23000 > >>>>>>> temp1_lcrit:-500 > >>>>>>> temp1_lcrit_alarm:0 > >>>>>>> temp1_max:-500 > >>>>>>> temp1_max_alarm:0 > >>>>>>> temp1_min:-500 > >>>>>>> temp1_min_alarm:0 > >>>>>>> temp1_rated_max:-500 > >>>>>>> temp2_crit:-500 > >>>>>>> temp2_crit_alarm:0 > >>>>>>> temp2_input:26000 > >>>>>>> temp2_lcrit:-500 > >>>>>>> temp2_lcrit_alarm:0 > >>>>>>> temp2_max:-500 > >>>>>>> temp2_max_alarm:0 > >>>>>>> temp2_min:-500 > >>>>>>> temp2_min_alarm:0 > >>>>>>> temp2_rated_max:-500 > >>>>>>> temp3_crit:-500 > >>>>>>> temp3_crit_alarm:0 > >>>>>>> temp3_input:30000 > >>>>>>> temp3_lcrit:-500 > >>>>>>> temp3_lcrit_alarm:0 > >>>>>>> temp3_max:-500 > >>>>>>> temp3_max_alarm:0 > >>>>>>> temp3_min:-500 > >>>>>>> temp3_min_alarm:0 > >>>>>>> temp3_rated_max:-500 > >>>>>>> uevent:OF_NAME=psu > >>>>>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > >>>>>>> uevent:OF_COMPATIBLE_0=delta,dps920ab > >>>>>>> uevent:OF_COMPATIBLE_N=1 > >>>>>> > >>>>> Ok, good enough. It looks like the PSU reports values for pretty much everything, > >>>>> including registers which don't exist. With that in mind, please check > >>>>> the attributes generated by your driver - I suspect that some of the limit > >>>>> attributes are not really supported (maybe none of them is supported). > >>>> > >>>> Yeah, I also think that none of those limits are actually supported. > >>>> Does the core expose a way to not register those? > >>>> > >>> > >>> Have the read_word function return -ENXIO for unsupported registers > >>> (and add a comment to the code explaining why you do that). > >>> See drivers/hwmon/pmbus/fsp-3y.c for an example. > >> > >> Thanks, will implement that. > >> In the meantime I finally got the PMBus command list that > >> the PSU supports. > >> It implements PMBus 1.2, but it also claims that MFR_ID, > >> MFR_MODEL and MFR_REVISION are supported. > >> > >> Block read returns what looks like a correct length, but the buffer > >> is empty. > >>> > >>> [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > >>> [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: > >>> [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > >>> [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: > >>> [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > >>> [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: > >>> [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 > >>> [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: > >>> [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 > >>> [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: > >>> [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > >>> [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: > >>> [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 > >>> [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: > >>> [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 > >>> [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: > >> > >> > >> However, manually reading using i2cget in word mode returns > >> something that looks ok. > >> i2cget -y 0 0x5a 0x9a w > >> 0x440b > >> > >> This would be D is ASCII, it looks like any kind of block read fails actually. > > > > Ok, so after using i2c_smbus_read_i2c_block_data() I can actually read > > the model kind of OK. > > There actually are 10 bytes of valid data, no idea why > > i2c_smbus_read_block_data() does not work. > >> > >> [ 721.194706] dps920ab 0-005a: MFR MODEL: > >> DPS-920AB > >> [ 721.245502] dps920ab 0-005b: MFR MODEL: > >> DPS-920AB > > > > It behaves weirdly when printing by creating a newline before and > > stuff like that. > > > > I suspect it prints the first byte, which should be the length. > I also suspect that the i2c controller driver has a bug and doesn't > support SMBus block data correctly. Did you try i2cdump with i and s > options ? That could be the case, I just wanted to see whether the controller or PSU are at fault. I tried using s with i2cdump and it will error out on the controller not supporting block reads. The controller most likely has a bug in the driver. > > >> I tried applying the block support for mv64xx as well: > >> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > > That patch would be needed, but it looks buggy to me. This chunk: > > + drv_data->effective_length = data+1; > + drv_data->bytes_left = data+1; > + drv_data->msg->len = data+1; > > should be: > > + drv_data->effective_length = data+1; > + drv_data->bytes_left = data; > + drv_data->msg->len = data+1; > > It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, > and bail out if it isn't. Yeah, I did not check the contents, I just saw 2 reviews and tested it since it can't hurt. > > Do you see a difference with the patch applied ? No, unfortunately not, i2cdump in s mode returns all zeroes. I can see that it gets the correct length but returns all zeroes. I did some development on the driver, added whitelisting for word-based registers. Found that this thing actually has the third temperature sensor after all and a lot of warnings and stuff are supported. I need to review the byte-based registers and whitelist those if needed. Tried adding PWM/DC control for the fan as it supports it, but it was a bit confusing so I gave up on that for now. Regards, Robert > > Guenter
On 5/27/21 7:17 AM, Robert Marko wrote: > On Thu, May 27, 2021 at 4:03 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 5/27/21 3:59 AM, Robert Marko wrote: >>> On Thu, May 27, 2021 at 12:22 PM Robert Marko <robert.marko@sartura.hr> wrote: >>>> >>>> On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>> >>>>> On 5/24/21 5:21 AM, Robert Marko wrote: >>>>>> On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>> >>>>>>> On 5/21/21 4:56 AM, Robert Marko wrote: >>>>>>>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>>> >>>>>>>>> On 5/21/21 1:36 AM, Robert Marko wrote: >>>>>>>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>>>>> >>>>>>>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: >>>>>>>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: >>>>>>>>>>>>>> This adds support for the Delta DPS-920AB PSU. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Only missing feature is fan control which the PSU supports. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ >>>>>>>>>>>>>> Documentation/hwmon/index.rst | 1 + >>>>>>>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ >>>>>>>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + >>>>>>>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ >>>>>>>>>>>>>> 5 files changed, 154 insertions(+) >>>>>>>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst >>>>>>>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst >>>>>>>>>>>>>> new file mode 100644 >>>>>>>>>>>>>> index 000000000000..df0aef530c7e >>>>>>>>>>>>>> --- /dev/null >>>>>>>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst >>>>>>>>>>>>>> @@ -0,0 +1,80 @@ >>>>>>>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Kernel driver dps920ab >>>>>>>>>>>>>> +======================== >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Supported chips: >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + * Delta DPS920AB >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + Prefix: 'dps920ab' >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + Addresses scanned: - >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Authors: >>>>>>>>>>>>>> + Robert Marko <robert.marko@sartura.hr> >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Description >>>>>>>>>>>>>> +----------- >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output >>>>>>>>>>>>>> +power supply with PMBus support. >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +The driver is a client driver to the core PMBus driver. >>>>>>>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Usage Notes >>>>>>>>>>>>>> +----------- >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the >>>>>>>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for >>>>>>>>>>>>>> +details. >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +Sysfs entries >>>>>>>>>>>>>> +------------- >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +======================= ====================================================== >>>>>>>>>>>>>> +curr1_label "iin" >>>>>>>>>>>>>> +curr1_input Measured input current >>>>>>>>>>>>>> +curr1_crit Critical maximum current >>>>>>>>>>>>>> +curr1_crit_alarm Current critical high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +curr2_label "iout1" >>>>>>>>>>>>>> +curr2_input Measured output current >>>>>>>>>>>>>> +curr2_crit Critical maximum current >>>>>>>>>>>>>> +curr2_crit_alarm Current critical high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +in1_label "vin" >>>>>>>>>>>>>> +in1_input Measured input voltage >>>>>>>>>>>>>> +in1_lcrit Critical minimum input voltage >>>>>>>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm >>>>>>>>>>>>>> +in1_crit Critical maximum input voltage >>>>>>>>>>>>>> +in1_crit_alarm Input voltage critical high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +in2_label "vout1" >>>>>>>>>>>>>> +in2_input Measured output voltage >>>>>>>>>>>>>> +in2_lcrit Critical minimum output voltage >>>>>>>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm >>>>>>>>>>>>>> +in2_crit Critical maximum output voltage >>>>>>>>>>>>>> +in2_crit_alarm Output voltage critical high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +power1_label "pin" >>>>>>>>>>>>>> +power1_input Measured input power >>>>>>>>>>>>>> +power1_alarm Input power high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +power2_label "pout1" >>>>>>>>>>>>>> +power2_input Measured output power >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +temp[1-2]_input Measured temperature >>>>>>>>>>>>>> +temp[1-2]_crit Critical high temperature >>>>>>>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm >>>>>>>>>>>>>> +temp[1-2]_max Maximum temperature >>>>>>>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +fan1_alarm Fan 1 warning. >>>>>>>>>>>>>> +fan1_fault Fan 1 fault. >>>>>>>>>>>>>> +fan1_input Fan 1 speed in RPM. >>>>>>>>>>>>>> +======================= ====================================================== >>>>>>>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst >>>>>>>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 >>>>>>>>>>>>>> --- a/Documentation/hwmon/index.rst >>>>>>>>>>>>>> +++ b/Documentation/hwmon/index.rst >>>>>>>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers >>>>>>>>>>>>>> dell-smm-hwmon >>>>>>>>>>>>>> dme1737 >>>>>>>>>>>>>> drivetemp >>>>>>>>>>>>>> + dps920ab >>>>>>>>>>>>>> ds1621 >>>>>>>>>>>>>> ds620 >>>>>>>>>>>>>> emc1403 >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>>>> index 32d2fc850621..865ade0aa205 100644 >>>>>>>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig >>>>>>>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS >>>>>>>>>>>>>> This driver can also be built as a module. If so, the module will >>>>>>>>>>>>>> be called ibm-cffps. >>>>>>>>>>>>>> >>>>>>>>>>>>>> +config SENSORS_DPS920AB >>>>>>>>>>>>>> + tristate "Delta DPS920AB Power Supply" >>>>>>>>>>>>>> + help >>>>>>>>>>>>>> + If you say yes here you get hardware monitoring support for Delta >>>>>>>>>>>>>> + DPS920AB Power Supplies. >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + This driver can also be built as a module. If so, the module will >>>>>>>>>>>>>> + be called dps920ab. >>>>>>>>>>>>>> + >>>>>>>>>>>>>> config SENSORS_INSPUR_IPSPS >>>>>>>>>>>>>> tristate "INSPUR Power System Power Supply" >>>>>>>>>>>>>> help >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile >>>>>>>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o >>>>>>>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>>>> new file mode 100644 >>>>>>>>>>>>>> index 000000000000..d579ed9f879c >>>>>>>>>>>>>> --- /dev/null >>>>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c >>>>>>>>>>>>>> @@ -0,0 +1,63 @@ >>>>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>>>>>>>>>>> +/* >>>>>>>>>>>>>> + * Driver for Delta DPS920AB PSU >>>>>>>>>>>>>> + * >>>>>>>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. >>>>>>>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. >>>>>>>>>>>>>> + */ >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +#include <linux/i2c.h> >>>>>>>>>>>>>> +#include <linux/module.h> >>>>>>>>>>>>>> +#include <linux/of_device.h> >>>>>>>>>>>>>> +#include "pmbus.h" >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { >>>>>>>>>>>>>> + .pages = 1, >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, >>>>>>>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, >>>>>>>>>>>>>> + .format[PSC_CURRENT_IN] = linear, >>>>>>>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, >>>>>>>>>>>>>> + .format[PSC_POWER] = linear, >>>>>>>>>>>>>> + .format[PSC_FAN] = linear, >>>>>>>>>>>>>> + .format[PSC_TEMPERATURE] = linear, >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | >>>>>>>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | >>>>>>>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | >>>>>>>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | >>>>>>>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | >>>>>>>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | >>>>>>>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, >>>>>>>>>>>>>> +}; >>>>>>>>>>>>>> + >>>>>>>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) >>>>>>>>>>>>>> +{ >>>>>>>>>>>>>> + if (!i2c_check_functionality(client->adapter, >>>>>>>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | >>>>>>>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) >>>>>>>>>>>>>> + return -ENODEV; >>>>>>>>>>>>> >>>>>>>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add >>>>>>>>>>>>> any value. >>>>>>>>>>>> >>>>>>>>>>>> Ok, makes sense. >>>>>>>>>>>>> >>>>>>>>>>>>> That makes me wonder: Is this driver needed in the first place, or could >>>>>>>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? >>>>>>>>>>>> >>>>>>>>>>>> It could be added as a generic driver, but that creates properties in >>>>>>>>>>>> the sysfs that this thing >>>>>>>>>>>> does not support like 3 voltage readings, 3 fans, and so on. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Can you be more specific ? What additional unsupported attributes >>>>>>>>>>> are added, and why ? Are you saying the PSU reports registers as >>>>>>>>>>> existing which do not really exist ? If so, which registers are those ? >>>>>>>>>> >>>>>>>>>> Sure, when core does probing and autodiscovery, then the following >>>>>>>>>> sysfs attributes are created: >>>>>>>>>>> >>>>>>>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max >>>>>>>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm >>>>>>>>>>> curr1_input fan3_target in3_rated_min temp1_min >>>>>>>>>>> curr1_label in1_crit name temp1_min_alarm >>>>>>>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max >>>>>>>>>>> curr1_max_alarm in1_input power temp2_crit >>>>>>>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm >>>>>>>>>>> curr2_crit in1_lcrit power1_input temp2_input >>>>>>>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit >>>>>>>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm >>>>>>>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max >>>>>>>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm >>>>>>>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min >>>>>>>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm >>>>>>>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max >>>>>>>>>>> curr2_rated_max in2_input power2_input temp3_crit >>>>>>>>>>> device in2_label power2_label temp3_crit_alarm >>>>>>>>>>> fan1_alarm in3_crit power2_max temp3_input >>>>>>>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit >>>>>>>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm >>>>>>>>>>> fan1_target in3_label subsystem temp3_max >>>>>>>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm >>>>>>>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min >>>>>>>>>>> fan2_input in3_max temp1_input temp3_min_alarm >>>>>>>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max >>>>>>>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent >>>>>>>>>> >>>>>>>>>> The following return -1, or -500 so they are not supported. >>>>>>>>>> * fan2 >>>>>>>>>> * in2 >>>>>>>>>> >>>>>>>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, >>>>>>>>>> but when auto probing they are fan3 and in3. >>>>>>>>>> >>>>>>>>>> temp3 actually seems to return a valid temperature despite it not being used in >>>>>>>>>> the vendor driver that features were picked from. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? >>>>>>>>> >>>>>>>> Sure: >>>>>>>>> >>>>>>>>> curr1_crit:-500 >>>>>>>>> curr1_crit_alarm:0 >>>>>>>>> curr1_input:195 >>>>>>>>> curr1_label:iin >>>>>>>>> curr1_max:-500 >>>>>>>>> curr1_max_alarm:0 >>>>>>>>> curr1_rated_max:-500 >>>>>>>>> curr2_crit:-500 >>>>>>>>> curr2_crit_alarm:0 >>>>>>>>> curr2_input:320 >>>>>>>>> curr2_label:iout1 >>>>>>>>> curr2_lcrit:-500 >>>>>>>>> curr2_lcrit_alarm:0 >>>>>>>>> curr2_max:18625 >>>>>>>>> curr2_max_alarm:0 >>>>>>>>> curr2_rated_max:16875 >>>>>>>>> grep: device: Is a directory >>>>>>>>> fan1_alarm:0 >>>>>>>>> fan1_fault:0 >>>>>>>>> fan1_input:10000 >>>>>>>>> fan1_target:0 >>>>>>>>> fan2_alarm:1 >>>>>>>>> fan2_fault:1 >>>>>>>>> fan2_input:-1 >>>>>>>>> fan2_target:-1 >>>>>>>>> fan3_alarm:1 >>>>>>>>> fan3_fault:1 >>>>>>>>> fan3_input:-1 >>>>>>>>> fan3_target:-1 >>>>>>>>> in1_crit:-500 >>>>>>>>> in1_crit_alarm:0 >>>>>>>>> in1_input:245250 >>>>>>>>> in1_label:vin >>>>>>>>> in1_lcrit:-500 >>>>>>>>> in1_lcrit_alarm:0 >>>>>>>>> in1_max:-500 >>>>>>>>> in1_max_alarm:0 >>>>>>>>> in1_min:-500 >>>>>>>>> in1_min_alarm:0 >>>>>>>>> in1_rated_max:-500 >>>>>>>>> in1_rated_min:-500 >>>>>>>>> in2_input:-500 >>>>>>>>> in2_label:vcap >>>>>>>>> in3_crit:255996 >>>>>>>>> in3_crit_alarm:0 >>>>>>>>> in3_input:54511 >>>>>>>>> in3_label:vout1 >>>>>>>>> in3_lcrit:255996 >>>>>>>>> in3_lcrit_alarm:0 >>>>>>>>> in3_max:255996 >>>>>>>>> in3_max_alarm:0 >>>>>>>>> in3_min:255996 >>>>>>>>> in3_min_alarm:0 >>>>>>>>> in3_rated_max:56136 >>>>>>>>> in3_rated_min:52863 >>>>>>>>> name:dps920ab >>>>>>>>> grep: of_node: Is a directory >>>>>>>>> grep: power: Is a directory >>>>>>>>> power1_alarm:0 >>>>>>>>> power1_input:33250000 >>>>>>>>> power1_label:pin >>>>>>>>> power1_max:-500000 >>>>>>>>> power1_rated_max:-500000 >>>>>>>>> power2_cap:-500000 >>>>>>>>> power2_cap_alarm:0 >>>>>>>>> power2_crit:-500000 >>>>>>>>> power2_crit_alarm:0 >>>>>>>>> power2_input:17750000 >>>>>>>>> power2_label:pout1 >>>>>>>>> power2_max:-500000 >>>>>>>>> power2_max_alarm:0 >>>>>>>>> power2_rated_max:920000000 >>>>>>>>> grep: subsystem: Is a directory >>>>>>>>> temp1_crit:-500 >>>>>>>>> temp1_crit_alarm:0 >>>>>>>>> temp1_input:23000 >>>>>>>>> temp1_lcrit:-500 >>>>>>>>> temp1_lcrit_alarm:0 >>>>>>>>> temp1_max:-500 >>>>>>>>> temp1_max_alarm:0 >>>>>>>>> temp1_min:-500 >>>>>>>>> temp1_min_alarm:0 >>>>>>>>> temp1_rated_max:-500 >>>>>>>>> temp2_crit:-500 >>>>>>>>> temp2_crit_alarm:0 >>>>>>>>> temp2_input:26000 >>>>>>>>> temp2_lcrit:-500 >>>>>>>>> temp2_lcrit_alarm:0 >>>>>>>>> temp2_max:-500 >>>>>>>>> temp2_max_alarm:0 >>>>>>>>> temp2_min:-500 >>>>>>>>> temp2_min_alarm:0 >>>>>>>>> temp2_rated_max:-500 >>>>>>>>> temp3_crit:-500 >>>>>>>>> temp3_crit_alarm:0 >>>>>>>>> temp3_input:30000 >>>>>>>>> temp3_lcrit:-500 >>>>>>>>> temp3_lcrit_alarm:0 >>>>>>>>> temp3_max:-500 >>>>>>>>> temp3_max_alarm:0 >>>>>>>>> temp3_min:-500 >>>>>>>>> temp3_min_alarm:0 >>>>>>>>> temp3_rated_max:-500 >>>>>>>>> uevent:OF_NAME=psu >>>>>>>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a >>>>>>>>> uevent:OF_COMPATIBLE_0=delta,dps920ab >>>>>>>>> uevent:OF_COMPATIBLE_N=1 >>>>>>>> >>>>>>> Ok, good enough. It looks like the PSU reports values for pretty much everything, >>>>>>> including registers which don't exist. With that in mind, please check >>>>>>> the attributes generated by your driver - I suspect that some of the limit >>>>>>> attributes are not really supported (maybe none of them is supported). >>>>>> >>>>>> Yeah, I also think that none of those limits are actually supported. >>>>>> Does the core expose a way to not register those? >>>>>> >>>>> >>>>> Have the read_word function return -ENXIO for unsupported registers >>>>> (and add a comment to the code explaining why you do that). >>>>> See drivers/hwmon/pmbus/fsp-3y.c for an example. >>>> >>>> Thanks, will implement that. >>>> In the meantime I finally got the PMBus command list that >>>> the PSU supports. >>>> It implements PMBus 1.2, but it also claims that MFR_ID, >>>> MFR_MODEL and MFR_REVISION are supported. >>>> >>>> Block read returns what looks like a correct length, but the buffer >>>> is empty. >>>>> >>>>> [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 >>>>> [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: >>>>> [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 >>>>> [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: >>>>> [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 >>>>> [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: >>>>> [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 >>>>> [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: >>>>> [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 >>>>> [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: >>>>> [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 >>>>> [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: >>>>> [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 >>>>> [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: >>>>> [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 >>>>> [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: >>>> >>>> >>>> However, manually reading using i2cget in word mode returns >>>> something that looks ok. >>>> i2cget -y 0 0x5a 0x9a w >>>> 0x440b >>>> >>>> This would be D is ASCII, it looks like any kind of block read fails actually. >>> >>> Ok, so after using i2c_smbus_read_i2c_block_data() I can actually read >>> the model kind of OK. >>> There actually are 10 bytes of valid data, no idea why >>> i2c_smbus_read_block_data() does not work. >>>> >>>> [ 721.194706] dps920ab 0-005a: MFR MODEL: >>>> DPS-920AB >>>> [ 721.245502] dps920ab 0-005b: MFR MODEL: >>>> DPS-920AB >>> >>> It behaves weirdly when printing by creating a newline before and >>> stuff like that. >>> >> >> I suspect it prints the first byte, which should be the length. >> I also suspect that the i2c controller driver has a bug and doesn't >> support SMBus block data correctly. Did you try i2cdump with i and s >> options ? > > That could be the case, I just wanted to see whether the controller or > PSU are at fault. > I tried using s with i2cdump and it will error out on the controller > not supporting block reads. > The controller most likely has a bug in the driver. >> >>>> I tried applying the block support for mv64xx as well: >>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ >> >> That patch would be needed, but it looks buggy to me. This chunk: >> >> + drv_data->effective_length = data+1; >> + drv_data->bytes_left = data+1; >> + drv_data->msg->len = data+1; >> >> should be: >> >> + drv_data->effective_length = data+1; >> + drv_data->bytes_left = data; >> + drv_data->msg->len = data+1; >> >> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, >> and bail out if it isn't. > > Yeah, I did not check the contents, I just saw 2 reviews and tested it > since it can't hurt. That patch doesn't work at all. Make the above change, and also change the type of effective_length from u32 to int, and try again. Guenter >> >> Do you see a difference with the patch applied ? > > No, unfortunately not, i2cdump in s mode returns all zeroes. > I can see that it gets the correct length but returns all zeroes. > > I did some development on the driver, added whitelisting for > word-based registers. > Found that this thing actually has the third temperature sensor after > all and a lot of warnings > and stuff are supported. > > I need to review the byte-based registers and whitelist those if needed. > > Tried adding PWM/DC control for the fan as it supports it, but it was > a bit confusing so I > gave up on that for now. > > Regards, > Robert >> >> Guenter > > >
On Thu, May 27, 2021 at 4:50 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/27/21 7:17 AM, Robert Marko wrote: > > On Thu, May 27, 2021 at 4:03 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On 5/27/21 3:59 AM, Robert Marko wrote: > >>> On Thu, May 27, 2021 at 12:22 PM Robert Marko <robert.marko@sartura.hr> wrote: > >>>> > >>>> On Mon, May 24, 2021 at 3:47 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>> > >>>>> On 5/24/21 5:21 AM, Robert Marko wrote: > >>>>>> On Fri, May 21, 2021 at 4:46 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>> > >>>>>>> On 5/21/21 4:56 AM, Robert Marko wrote: > >>>>>>>> On Fri, May 21, 2021 at 12:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>>> > >>>>>>>>> On 5/21/21 1:36 AM, Robert Marko wrote: > >>>>>>>>>> On Wed, May 19, 2021 at 3:19 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>>>>> > >>>>>>>>>>> On 5/19/21 5:38 AM, Robert Marko wrote: > >>>>>>>>>>>> On Fri, Apr 30, 2021 at 3:48 PM Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Fri, Apr 30, 2021 at 03:27:33PM +0200, Robert Marko wrote: > >>>>>>>>>>>>>> This adds support for the Delta DPS-920AB PSU. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Only missing feature is fan control which the PSU supports. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ > >>>>>>>>>>>>>> Documentation/hwmon/index.rst | 1 + > >>>>>>>>>>>>>> drivers/hwmon/pmbus/Kconfig | 9 ++++ > >>>>>>>>>>>>>> drivers/hwmon/pmbus/Makefile | 1 + > >>>>>>>>>>>>>> drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ > >>>>>>>>>>>>>> 5 files changed, 154 insertions(+) > >>>>>>>>>>>>>> create mode 100644 Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>>>> create mode 100644 drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>>>> new file mode 100644 > >>>>>>>>>>>>>> index 000000000000..df0aef530c7e > >>>>>>>>>>>>>> --- /dev/null > >>>>>>>>>>>>>> +++ b/Documentation/hwmon/dps920ab.rst > >>>>>>>>>>>>>> @@ -0,0 +1,80 @@ > >>>>>>>>>>>>>> +.. SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Kernel driver dps920ab > >>>>>>>>>>>>>> +======================== > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Supported chips: > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + * Delta DPS920AB > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + Prefix: 'dps920ab' > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + Addresses scanned: - > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Authors: > >>>>>>>>>>>>>> + Robert Marko <robert.marko@sartura.hr> > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Description > >>>>>>>>>>>>>> +----------- > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +This driver implements support for Delta DPS920AB 920W 54V DC single output > >>>>>>>>>>>>>> +power supply with PMBus support. > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +The driver is a client driver to the core PMBus driver. > >>>>>>>>>>>>>> +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Usage Notes > >>>>>>>>>>>>>> +----------- > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +This driver does not auto-detect devices. You will have to instantiate the > >>>>>>>>>>>>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > >>>>>>>>>>>>>> +details. > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +Sysfs entries > >>>>>>>>>>>>>> +------------- > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +======================= ====================================================== > >>>>>>>>>>>>>> +curr1_label "iin" > >>>>>>>>>>>>>> +curr1_input Measured input current > >>>>>>>>>>>>>> +curr1_crit Critical maximum current > >>>>>>>>>>>>>> +curr1_crit_alarm Current critical high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +curr2_label "iout1" > >>>>>>>>>>>>>> +curr2_input Measured output current > >>>>>>>>>>>>>> +curr2_crit Critical maximum current > >>>>>>>>>>>>>> +curr2_crit_alarm Current critical high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +in1_label "vin" > >>>>>>>>>>>>>> +in1_input Measured input voltage > >>>>>>>>>>>>>> +in1_lcrit Critical minimum input voltage > >>>>>>>>>>>>>> +in1_lcrit_alarm Input voltage critical low alarm > >>>>>>>>>>>>>> +in1_crit Critical maximum input voltage > >>>>>>>>>>>>>> +in1_crit_alarm Input voltage critical high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +in2_label "vout1" > >>>>>>>>>>>>>> +in2_input Measured output voltage > >>>>>>>>>>>>>> +in2_lcrit Critical minimum output voltage > >>>>>>>>>>>>>> +in2_lcrit_alarm Output voltage critical low alarm > >>>>>>>>>>>>>> +in2_crit Critical maximum output voltage > >>>>>>>>>>>>>> +in2_crit_alarm Output voltage critical high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +power1_label "pin" > >>>>>>>>>>>>>> +power1_input Measured input power > >>>>>>>>>>>>>> +power1_alarm Input power high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +power2_label "pout1" > >>>>>>>>>>>>>> +power2_input Measured output power > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +temp[1-2]_input Measured temperature > >>>>>>>>>>>>>> +temp[1-2]_crit Critical high temperature > >>>>>>>>>>>>>> +temp[1-2]_crit_alarm Chip temperature critical high alarm > >>>>>>>>>>>>>> +temp[1-2]_max Maximum temperature > >>>>>>>>>>>>>> +temp[1-2]_max_alarm Chip temperature high alarm > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +fan1_alarm Fan 1 warning. > >>>>>>>>>>>>>> +fan1_fault Fan 1 fault. > >>>>>>>>>>>>>> +fan1_input Fan 1 speed in RPM. > >>>>>>>>>>>>>> +======================= ====================================================== > >>>>>>>>>>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > >>>>>>>>>>>>>> index 8d5a2df1ecb6..b24436f22052 100644 > >>>>>>>>>>>>>> --- a/Documentation/hwmon/index.rst > >>>>>>>>>>>>>> +++ b/Documentation/hwmon/index.rst > >>>>>>>>>>>>>> @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers > >>>>>>>>>>>>>> dell-smm-hwmon > >>>>>>>>>>>>>> dme1737 > >>>>>>>>>>>>>> drivetemp > >>>>>>>>>>>>>> + dps920ab > >>>>>>>>>>>>>> ds1621 > >>>>>>>>>>>>>> ds620 > >>>>>>>>>>>>>> emc1403 > >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>>>> index 32d2fc850621..865ade0aa205 100644 > >>>>>>>>>>>>>> --- a/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/Kconfig > >>>>>>>>>>>>>> @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS > >>>>>>>>>>>>>> This driver can also be built as a module. If so, the module will > >>>>>>>>>>>>>> be called ibm-cffps. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> +config SENSORS_DPS920AB > >>>>>>>>>>>>>> + tristate "Delta DPS920AB Power Supply" > >>>>>>>>>>>>>> + help > >>>>>>>>>>>>>> + If you say yes here you get hardware monitoring support for Delta > >>>>>>>>>>>>>> + DPS920AB Power Supplies. > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + This driver can also be built as a module. If so, the module will > >>>>>>>>>>>>>> + be called dps920ab. > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> config SENSORS_INSPUR_IPSPS > >>>>>>>>>>>>>> tristate "INSPUR Power System Power Supply" > >>>>>>>>>>>>>> help > >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile > >>>>>>>>>>>>>> index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o > >>>>>>>>>>>>>> +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o > >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o > >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o > >>>>>>>>>>>>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o > >>>>>>>>>>>>>> diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>>>> new file mode 100644 > >>>>>>>>>>>>>> index 000000000000..d579ed9f879c > >>>>>>>>>>>>>> --- /dev/null > >>>>>>>>>>>>>> +++ b/drivers/hwmon/pmbus/dps920ab.c > >>>>>>>>>>>>>> @@ -0,0 +1,63 @@ > >>>>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>>>>>>>>>>> +/* > >>>>>>>>>>>>>> + * Driver for Delta DPS920AB PSU > >>>>>>>>>>>>>> + * > >>>>>>>>>>>>>> + * Copyright (C) 2021 Delta Networks, Inc. > >>>>>>>>>>>>>> + * Copyright (C) 2021 Sartura Ltd. > >>>>>>>>>>>>>> + */ > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +#include <linux/i2c.h> > >>>>>>>>>>>>>> +#include <linux/module.h> > >>>>>>>>>>>>>> +#include <linux/of_device.h> > >>>>>>>>>>>>>> +#include "pmbus.h" > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +static struct pmbus_driver_info dps920ab_info = { > >>>>>>>>>>>>>> + .pages = 1, > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + .format[PSC_VOLTAGE_IN] = linear, > >>>>>>>>>>>>>> + .format[PSC_VOLTAGE_OUT] = linear, > >>>>>>>>>>>>>> + .format[PSC_CURRENT_IN] = linear, > >>>>>>>>>>>>>> + .format[PSC_CURRENT_OUT] = linear, > >>>>>>>>>>>>>> + .format[PSC_POWER] = linear, > >>>>>>>>>>>>>> + .format[PSC_FAN] = linear, > >>>>>>>>>>>>>> + .format[PSC_TEMPERATURE] = linear, > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | > >>>>>>>>>>>>>> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | > >>>>>>>>>>>>>> + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | > >>>>>>>>>>>>>> + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | > >>>>>>>>>>>>>> + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | > >>>>>>>>>>>>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | > >>>>>>>>>>>>>> + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, > >>>>>>>>>>>>>> +}; > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> +static int dps920ab_probe(struct i2c_client *client) > >>>>>>>>>>>>>> +{ > >>>>>>>>>>>>>> + if (!i2c_check_functionality(client->adapter, > >>>>>>>>>>>>>> + I2C_FUNC_SMBUS_BYTE_DATA | > >>>>>>>>>>>>>> + I2C_FUNC_SMBUS_WORD_DATA)) > >>>>>>>>>>>>>> + return -ENODEV; > >>>>>>>>>>>>> > >>>>>>>>>>>>> This check is done in pmbus_do_probe(), and repeating it here does not add > >>>>>>>>>>>>> any value. > >>>>>>>>>>>> > >>>>>>>>>>>> Ok, makes sense. > >>>>>>>>>>>>> > >>>>>>>>>>>>> That makes me wonder: Is this driver needed in the first place, or could > >>>>>>>>>>>>> it be added to drivers/hwmon/pmbus/pmbus.c ? > >>>>>>>>>>>> > >>>>>>>>>>>> It could be added as a generic driver, but that creates properties in > >>>>>>>>>>>> the sysfs that this thing > >>>>>>>>>>>> does not support like 3 voltage readings, 3 fans, and so on. > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Can you be more specific ? What additional unsupported attributes > >>>>>>>>>>> are added, and why ? Are you saying the PSU reports registers as > >>>>>>>>>>> existing which do not really exist ? If so, which registers are those ? > >>>>>>>>>> > >>>>>>>>>> Sure, when core does probing and autodiscovery, then the following > >>>>>>>>>> sysfs attributes are created: > >>>>>>>>>>> > >>>>>>>>>>> curr1_crit fan3_fault in3_min_alarm temp1_max > >>>>>>>>>>> curr1_crit_alarm fan3_input in3_rated_max temp1_max_alarm > >>>>>>>>>>> curr1_input fan3_target in3_rated_min temp1_min > >>>>>>>>>>> curr1_label in1_crit name temp1_min_alarm > >>>>>>>>>>> curr1_max in1_crit_alarm of_node temp1_rated_max > >>>>>>>>>>> curr1_max_alarm in1_input power temp2_crit > >>>>>>>>>>> curr1_rated_max in1_label power1_alarm temp2_crit_alarm > >>>>>>>>>>> curr2_crit in1_lcrit power1_input temp2_input > >>>>>>>>>>> curr2_crit_alarm in1_lcrit_alarm power1_label temp2_lcrit > >>>>>>>>>>> curr2_input in1_max power1_max temp2_lcrit_alarm > >>>>>>>>>>> curr2_label in1_max_alarm power1_rated_max temp2_max > >>>>>>>>>>> curr2_lcrit in1_min power2_cap temp2_max_alarm > >>>>>>>>>>> curr2_lcrit_alarm in1_min_alarm power2_cap_alarm temp2_min > >>>>>>>>>>> curr2_max in1_rated_max power2_crit temp2_min_alarm > >>>>>>>>>>> curr2_max_alarm in1_rated_min power2_crit_alarm temp2_rated_max > >>>>>>>>>>> curr2_rated_max in2_input power2_input temp3_crit > >>>>>>>>>>> device in2_label power2_label temp3_crit_alarm > >>>>>>>>>>> fan1_alarm in3_crit power2_max temp3_input > >>>>>>>>>>> fan1_fault in3_crit_alarm power2_max_alarm temp3_lcrit > >>>>>>>>>>> fan1_input in3_input power2_rated_max temp3_lcrit_alarm > >>>>>>>>>>> fan1_target in3_label subsystem temp3_max > >>>>>>>>>>> fan2_alarm in3_lcrit temp1_crit temp3_max_alarm > >>>>>>>>>>> fan2_fault in3_lcrit_alarm temp1_crit_alarm temp3_min > >>>>>>>>>>> fan2_input in3_max temp1_input temp3_min_alarm > >>>>>>>>>>> fan2_target in3_max_alarm temp1_lcrit temp3_rated_max > >>>>>>>>>>> fan3_alarm in3_min temp1_lcrit_alarm uevent > >>>>>>>>>> > >>>>>>>>>> The following return -1, or -500 so they are not supported. > >>>>>>>>>> * fan2 > >>>>>>>>>> * in2 > >>>>>>>>>> > >>>>>>>>>> Weirdly, with the external driver both fan2 and in2 are enabled and work fine, > >>>>>>>>>> but when auto probing they are fan3 and in3. > >>>>>>>>>> > >>>>>>>>>> temp3 actually seems to return a valid temperature despite it not being used in > >>>>>>>>>> the vendor driver that features were picked from. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> Can you run "grep . *" in the hwmon directory so I can see actual values ? > >>>>>>>>> > >>>>>>>> Sure: > >>>>>>>>> > >>>>>>>>> curr1_crit:-500 > >>>>>>>>> curr1_crit_alarm:0 > >>>>>>>>> curr1_input:195 > >>>>>>>>> curr1_label:iin > >>>>>>>>> curr1_max:-500 > >>>>>>>>> curr1_max_alarm:0 > >>>>>>>>> curr1_rated_max:-500 > >>>>>>>>> curr2_crit:-500 > >>>>>>>>> curr2_crit_alarm:0 > >>>>>>>>> curr2_input:320 > >>>>>>>>> curr2_label:iout1 > >>>>>>>>> curr2_lcrit:-500 > >>>>>>>>> curr2_lcrit_alarm:0 > >>>>>>>>> curr2_max:18625 > >>>>>>>>> curr2_max_alarm:0 > >>>>>>>>> curr2_rated_max:16875 > >>>>>>>>> grep: device: Is a directory > >>>>>>>>> fan1_alarm:0 > >>>>>>>>> fan1_fault:0 > >>>>>>>>> fan1_input:10000 > >>>>>>>>> fan1_target:0 > >>>>>>>>> fan2_alarm:1 > >>>>>>>>> fan2_fault:1 > >>>>>>>>> fan2_input:-1 > >>>>>>>>> fan2_target:-1 > >>>>>>>>> fan3_alarm:1 > >>>>>>>>> fan3_fault:1 > >>>>>>>>> fan3_input:-1 > >>>>>>>>> fan3_target:-1 > >>>>>>>>> in1_crit:-500 > >>>>>>>>> in1_crit_alarm:0 > >>>>>>>>> in1_input:245250 > >>>>>>>>> in1_label:vin > >>>>>>>>> in1_lcrit:-500 > >>>>>>>>> in1_lcrit_alarm:0 > >>>>>>>>> in1_max:-500 > >>>>>>>>> in1_max_alarm:0 > >>>>>>>>> in1_min:-500 > >>>>>>>>> in1_min_alarm:0 > >>>>>>>>> in1_rated_max:-500 > >>>>>>>>> in1_rated_min:-500 > >>>>>>>>> in2_input:-500 > >>>>>>>>> in2_label:vcap > >>>>>>>>> in3_crit:255996 > >>>>>>>>> in3_crit_alarm:0 > >>>>>>>>> in3_input:54511 > >>>>>>>>> in3_label:vout1 > >>>>>>>>> in3_lcrit:255996 > >>>>>>>>> in3_lcrit_alarm:0 > >>>>>>>>> in3_max:255996 > >>>>>>>>> in3_max_alarm:0 > >>>>>>>>> in3_min:255996 > >>>>>>>>> in3_min_alarm:0 > >>>>>>>>> in3_rated_max:56136 > >>>>>>>>> in3_rated_min:52863 > >>>>>>>>> name:dps920ab > >>>>>>>>> grep: of_node: Is a directory > >>>>>>>>> grep: power: Is a directory > >>>>>>>>> power1_alarm:0 > >>>>>>>>> power1_input:33250000 > >>>>>>>>> power1_label:pin > >>>>>>>>> power1_max:-500000 > >>>>>>>>> power1_rated_max:-500000 > >>>>>>>>> power2_cap:-500000 > >>>>>>>>> power2_cap_alarm:0 > >>>>>>>>> power2_crit:-500000 > >>>>>>>>> power2_crit_alarm:0 > >>>>>>>>> power2_input:17750000 > >>>>>>>>> power2_label:pout1 > >>>>>>>>> power2_max:-500000 > >>>>>>>>> power2_max_alarm:0 > >>>>>>>>> power2_rated_max:920000000 > >>>>>>>>> grep: subsystem: Is a directory > >>>>>>>>> temp1_crit:-500 > >>>>>>>>> temp1_crit_alarm:0 > >>>>>>>>> temp1_input:23000 > >>>>>>>>> temp1_lcrit:-500 > >>>>>>>>> temp1_lcrit_alarm:0 > >>>>>>>>> temp1_max:-500 > >>>>>>>>> temp1_max_alarm:0 > >>>>>>>>> temp1_min:-500 > >>>>>>>>> temp1_min_alarm:0 > >>>>>>>>> temp1_rated_max:-500 > >>>>>>>>> temp2_crit:-500 > >>>>>>>>> temp2_crit_alarm:0 > >>>>>>>>> temp2_input:26000 > >>>>>>>>> temp2_lcrit:-500 > >>>>>>>>> temp2_lcrit_alarm:0 > >>>>>>>>> temp2_max:-500 > >>>>>>>>> temp2_max_alarm:0 > >>>>>>>>> temp2_min:-500 > >>>>>>>>> temp2_min_alarm:0 > >>>>>>>>> temp2_rated_max:-500 > >>>>>>>>> temp3_crit:-500 > >>>>>>>>> temp3_crit_alarm:0 > >>>>>>>>> temp3_input:30000 > >>>>>>>>> temp3_lcrit:-500 > >>>>>>>>> temp3_lcrit_alarm:0 > >>>>>>>>> temp3_max:-500 > >>>>>>>>> temp3_max_alarm:0 > >>>>>>>>> temp3_min:-500 > >>>>>>>>> temp3_min_alarm:0 > >>>>>>>>> temp3_rated_max:-500 > >>>>>>>>> uevent:OF_NAME=psu > >>>>>>>>> uevent:OF_FULLNAME=/ap806/config-space@f0000000/i2c@511000/psu@5a > >>>>>>>>> uevent:OF_COMPATIBLE_0=delta,dps920ab > >>>>>>>>> uevent:OF_COMPATIBLE_N=1 > >>>>>>>> > >>>>>>> Ok, good enough. It looks like the PSU reports values for pretty much everything, > >>>>>>> including registers which don't exist. With that in mind, please check > >>>>>>> the attributes generated by your driver - I suspect that some of the limit > >>>>>>> attributes are not really supported (maybe none of them is supported). > >>>>>> > >>>>>> Yeah, I also think that none of those limits are actually supported. > >>>>>> Does the core expose a way to not register those? > >>>>>> > >>>>> > >>>>> Have the read_word function return -ENXIO for unsupported registers > >>>>> (and add a comment to the code explaining why you do that). > >>>>> See drivers/hwmon/pmbus/fsp-3y.c for an example. > >>>> > >>>> Thanks, will implement that. > >>>> In the meantime I finally got the PMBus command list that > >>>> the PSU supports. > >>>> It implements PMBus 1.2, but it also claims that MFR_ID, > >>>> MFR_MODEL and MFR_REVISION are supported. > >>>> > >>>> Block read returns what looks like a correct length, but the buffer > >>>> is empty. > >>>>> > >>>>> [ 195.246464] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > >>>>> [ 195.251495] dps920ab 0-005a: PMBUS_MFR_ID: > >>>>> [ 195.300789] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > >>>>> [ 195.305762] dps920ab 0-005b: PMBUS_MFR_ID: > >>>>> [ 272.795222] dps920ab 0-005a: PMBUS_MFR_ID length: 5 > >>>>> [ 272.800190] dps920ab 0-005a: PMBUS_MFR_ID: > >>>>> [ 272.804862] dps920ab 0-005a: PMBUS_MFR_MODEL length: 11 > >>>>> [ 272.810172] dps920ab 0-005a: PMBUS_MFR_MODEL: > >>>>> [ 272.815098] dps920ab 0-005a: PMBUS_MFR_REVISION length: 2 > >>>>> [ 272.820600] dps920ab 0-005a: PMBUS_MFR_REVISION: > >>>>> [ 272.874060] dps920ab 0-005b: PMBUS_MFR_ID length: 5 > >>>>> [ 272.879102] dps920ab 0-005b: PMBUS_MFR_ID: > >>>>> [ 272.883833] dps920ab 0-005b: PMBUS_MFR_MODEL length: 11 > >>>>> [ 272.889151] dps920ab 0-005b: PMBUS_MFR_MODEL: > >>>>> [ 272.894097] dps920ab 0-005b: PMBUS_MFR_REVISION length: 2 > >>>>> [ 272.899548] dps920ab 0-005b: PMBUS_MFR_REVISION: > >>>> > >>>> > >>>> However, manually reading using i2cget in word mode returns > >>>> something that looks ok. > >>>> i2cget -y 0 0x5a 0x9a w > >>>> 0x440b > >>>> > >>>> This would be D is ASCII, it looks like any kind of block read fails actually. > >>> > >>> Ok, so after using i2c_smbus_read_i2c_block_data() I can actually read > >>> the model kind of OK. > >>> There actually are 10 bytes of valid data, no idea why > >>> i2c_smbus_read_block_data() does not work. > >>>> > >>>> [ 721.194706] dps920ab 0-005a: MFR MODEL: > >>>> DPS-920AB > >>>> [ 721.245502] dps920ab 0-005b: MFR MODEL: > >>>> DPS-920AB > >>> > >>> It behaves weirdly when printing by creating a newline before and > >>> stuff like that. > >>> > >> > >> I suspect it prints the first byte, which should be the length. > >> I also suspect that the i2c controller driver has a bug and doesn't > >> support SMBus block data correctly. Did you try i2cdump with i and s > >> options ? > > > > That could be the case, I just wanted to see whether the controller or > > PSU are at fault. > > I tried using s with i2cdump and it will error out on the controller > > not supporting block reads. > > The controller most likely has a bug in the driver. > >> > >>>> I tried applying the block support for mv64xx as well: > >>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > >> > >> That patch would be needed, but it looks buggy to me. This chunk: > >> > >> + drv_data->effective_length = data+1; > >> + drv_data->bytes_left = data+1; > >> + drv_data->msg->len = data+1; > >> > >> should be: > >> > >> + drv_data->effective_length = data+1; > >> + drv_data->bytes_left = data; > >> + drv_data->msg->len = data+1; > >> > >> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, > >> and bail out if it isn't. > > > > Yeah, I did not check the contents, I just saw 2 reviews and tested it > > since it can't hurt. > > That patch doesn't work at all. Make the above change, and also change > the type of effective_length from u32 to int, and try again. I was just looking and it, and doing the changes you recommended make no difference at all. I will see who can I contact to check this out further. Robert > > Guenter > > >> > >> Do you see a difference with the patch applied ? > > > > No, unfortunately not, i2cdump in s mode returns all zeroes. > > I can see that it gets the correct length but returns all zeroes. > > > > I did some development on the driver, added whitelisting for > > word-based registers. > > Found that this thing actually has the third temperature sensor after > > all and a lot of warnings > > and stuff are supported. > > > > I need to review the byte-based registers and whitelist those if needed. > > > > Tried adding PWM/DC control for the fan as it supports it, but it was > > a bit confusing so I > > gave up on that for now. > > > > Regards, > > Robert > >> > >> Guenter > > > > > > >
On 5/27/21 7:58 AM, Robert Marko wrote: [ ... ] >>>>>> I tried applying the block support for mv64xx as well: >>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ >>>> >>>> That patch would be needed, but it looks buggy to me. This chunk: >>>> >>>> + drv_data->effective_length = data+1; >>>> + drv_data->bytes_left = data+1; >>>> + drv_data->msg->len = data+1; >>>> >>>> should be: >>>> >>>> + drv_data->effective_length = data+1; >>>> + drv_data->bytes_left = data; >>>> + drv_data->msg->len = data+1; >>>> >>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, >>>> and bail out if it isn't. >>> >>> Yeah, I did not check the contents, I just saw 2 reviews and tested it >>> since it can't hurt. >> >> That patch doesn't work at all. Make the above change, and also change >> the type of effective_length from u32 to int, and try again. > > I was just looking and it, and doing the changes you recommended make > no difference at all. > Is the i2c controller compatible with marvell,mv78230-i2c ? The block transfers would not work in that case. Let me know and I'll send you a patch that might fix it. Guenter
On Thu, May 27, 2021 at 6:37 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/27/21 7:58 AM, Robert Marko wrote: > [ ... ] > > >>>>>> I tried applying the block support for mv64xx as well: > >>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > >>>> > >>>> That patch would be needed, but it looks buggy to me. This chunk: > >>>> > >>>> + drv_data->effective_length = data+1; > >>>> + drv_data->bytes_left = data+1; > >>>> + drv_data->msg->len = data+1; > >>>> > >>>> should be: > >>>> > >>>> + drv_data->effective_length = data+1; > >>>> + drv_data->bytes_left = data; > >>>> + drv_data->msg->len = data+1; > >>>> > >>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, > >>>> and bail out if it isn't. > >>> > >>> Yeah, I did not check the contents, I just saw 2 reviews and tested it > >>> since it can't hurt. > >> > >> That patch doesn't work at all. Make the above change, and also change > >> the type of effective_length from u32 to int, and try again. > > > > I was just looking and it, and doing the changes you recommended make > > no difference at all. > > > > Is the i2c controller compatible with marvell,mv78230-i2c ? > The block transfers would not work in that case. Let me know > and I'll send you a patch that might fix it. Yes, the CPU is Armada 7040 and I can see that compatible is marvell,mv78230-i2c indeed. It would be great if you have the patch, Regards Robert > > Guenter
On Thu, May 27, 2021 at 09:14:03PM +0200, Robert Marko wrote: > On Thu, May 27, 2021 at 6:37 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 5/27/21 7:58 AM, Robert Marko wrote: > > [ ... ] > > > > >>>>>> I tried applying the block support for mv64xx as well: > > >>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > > >>>> > > >>>> That patch would be needed, but it looks buggy to me. This chunk: > > >>>> > > >>>> + drv_data->effective_length = data+1; > > >>>> + drv_data->bytes_left = data+1; > > >>>> + drv_data->msg->len = data+1; > > >>>> > > >>>> should be: > > >>>> > > >>>> + drv_data->effective_length = data+1; > > >>>> + drv_data->bytes_left = data; > > >>>> + drv_data->msg->len = data+1; > > >>>> > > >>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, > > >>>> and bail out if it isn't. > > >>> > > >>> Yeah, I did not check the contents, I just saw 2 reviews and tested it > > >>> since it can't hurt. > > >> > > >> That patch doesn't work at all. Make the above change, and also change > > >> the type of effective_length from u32 to int, and try again. > > > > > > I was just looking and it, and doing the changes you recommended make > > > no difference at all. > > > > > > > Is the i2c controller compatible with marvell,mv78230-i2c ? > > The block transfers would not work in that case. Let me know > > and I'll send you a patch that might fix it. > > Yes, the CPU is Armada 7040 and I can see that compatible is > marvell,mv78230-i2c indeed. > > It would be great if you have the patch, See below. Guenter --- From 41f6113560e3082293cef6e8f2013e70ee8f54db Mon Sep 17 00:00:00 2001 From: Federico Fuga <fuga@studiofuga.com> Date: Sat, 18 Jan 2020 12:58:20 +0100 Subject: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function essential to allow blocks with variable length to be read from an i2c slave. This is needed to implement the SMBus Read Block Data function. This patch implements the function by changing the bytes_left and msg len on the fly if the flag is specified. It has been successfully tested on Allwinner A33 with a special i2c chip that returns variable length blocks on reading. Signed-off-by: Federico Fuga <fuga@studiofuga.com> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com> Link: https://lore.kernel.org/r/20200118115820.9080-1-fuga@studiofuga.com [groeck: Various fixes] Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/i2c/busses/i2c-mv64xxx.c | 75 ++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 14 deletions(-) diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 5c8e94b6cdb5..9dda90359966 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -341,7 +341,17 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) { drv_data->msg = drv_data->msgs; drv_data->byte_posn = 0; - drv_data->bytes_left = drv_data->msg->len; + + /* + * If we should retrieve the length from the buffer, make sure + * to read enough bytes to avoid sending the STOP bit after + * the read if the first byte + */ + if (drv_data->msg->flags & I2C_M_RECV_LEN) + drv_data->bytes_left = 3; + else + drv_data->bytes_left = drv_data->msg->len; + drv_data->aborting = 0; drv_data->rc = 0; @@ -350,6 +360,47 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) drv_data->reg_base + drv_data->reg_offsets.control); } +static void +mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data) +{ + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; + writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, + drv_data->reg_base + drv_data->reg_offsets.control); + drv_data->block = 0; + if (drv_data->errata_delay) + udelay(5); + + wake_up(&drv_data->waitq); +} + +static void +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data) +{ + u8 data; + + data = readl(drv_data->reg_base + drv_data->reg_offsets.data); + drv_data->msg->buf[drv_data->byte_posn++] = data; + + if (drv_data->msg->flags & I2C_M_RECV_LEN) { + if (!data || data > I2C_SMBUS_BLOCK_MAX) { + /* + * FIXME + * Abort and report error. Needs to be + * verified/tested on real hardware. + */ + drv_data->rc = -EPROTO; + mv64xxx_i2c_do_send_stop(drv_data); + return; + } + drv_data->msg->flags &= ~I2C_M_RECV_LEN; + drv_data->bytes_left = data; + drv_data->msg->len = data + 1; + } + + writel(drv_data->cntl_bits, + drv_data->reg_base + drv_data->reg_offsets.control); +} + static void mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) { @@ -400,23 +451,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) break; case MV64XXX_I2C_ACTION_RCV_DATA: - drv_data->msg->buf[drv_data->byte_posn++] = - readl(drv_data->reg_base + drv_data->reg_offsets.data); - writel(drv_data->cntl_bits, - drv_data->reg_base + drv_data->reg_offsets.control); + mv64xxx_i2c_do_read_data(drv_data); break; case MV64XXX_I2C_ACTION_RCV_DATA_STOP: drv_data->msg->buf[drv_data->byte_posn++] = readl(drv_data->reg_base + drv_data->reg_offsets.data); - drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; - writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, - drv_data->reg_base + drv_data->reg_offsets.control); - drv_data->block = 0; - if (drv_data->errata_delay) - udelay(5); - - wake_up(&drv_data->waitq); + mv64xxx_i2c_do_send_stop(drv_data); break; case MV64XXX_I2C_ACTION_INVALID: @@ -680,6 +721,10 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) if (!drv_data->offload_enabled) return false; + /* Offload not supported for block data transfers */ + if (msgs[0].flags & I2C_M_RECV_LEN) + return false; + /* * We can offload a transaction consisting of a single * message, as long as the message has a length between 1 and @@ -697,6 +742,7 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) mv64xxx_i2c_valid_offload_sz(msgs) && mv64xxx_i2c_valid_offload_sz(msgs + 1) && !(msgs[0].flags & I2C_M_RD) && + !(msgs[1].flags & I2C_M_RECV_LEN) && msgs[1].flags & I2C_M_RD) return true; @@ -713,7 +759,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) static u32 mv64xxx_i2c_functionality(struct i2c_adapter *adap) { - return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL; + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | + I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL; } static int
On Thu, May 27, 2021 at 9:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, May 27, 2021 at 09:14:03PM +0200, Robert Marko wrote: > > On Thu, May 27, 2021 at 6:37 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On 5/27/21 7:58 AM, Robert Marko wrote: > > > [ ... ] > > > > > > >>>>>> I tried applying the block support for mv64xx as well: > > > >>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ > > > >>>> > > > >>>> That patch would be needed, but it looks buggy to me. This chunk: > > > >>>> > > > >>>> + drv_data->effective_length = data+1; > > > >>>> + drv_data->bytes_left = data+1; > > > >>>> + drv_data->msg->len = data+1; > > > >>>> > > > >>>> should be: > > > >>>> > > > >>>> + drv_data->effective_length = data+1; > > > >>>> + drv_data->bytes_left = data; > > > >>>> + drv_data->msg->len = data+1; > > > >>>> > > > >>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, > > > >>>> and bail out if it isn't. > > > >>> > > > >>> Yeah, I did not check the contents, I just saw 2 reviews and tested it > > > >>> since it can't hurt. > > > >> > > > >> That patch doesn't work at all. Make the above change, and also change > > > >> the type of effective_length from u32 to int, and try again. > > > > > > > > I was just looking and it, and doing the changes you recommended make > > > > no difference at all. > > > > > > > > > > Is the i2c controller compatible with marvell,mv78230-i2c ? > > > The block transfers would not work in that case. Let me know > > > and I'll send you a patch that might fix it. > > > > Yes, the CPU is Armada 7040 and I can see that compatible is > > marvell,mv78230-i2c indeed. > > > > It would be great if you have the patch, > > See below. I can confirm that this does indeed enable block reads and i2c_smbus_read_block_data() works now. PMBUS_MFR_ID and PMBUS_MFR_MODEL are indeed populated. Regards, Robert > > Guenter > > --- > From 41f6113560e3082293cef6e8f2013e70ee8f54db Mon Sep 17 00:00:00 2001 > From: Federico Fuga <fuga@studiofuga.com> > Date: Sat, 18 Jan 2020 12:58:20 +0100 > Subject: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and > I2C_FUNC_SMBUS_READ_BLOCK_DATA > > The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function > essential to allow blocks with variable length to be read from an i2c > slave. > This is needed to implement the SMBus Read Block Data function. > > This patch implements the function by changing the bytes_left and > msg len on the fly if the flag is specified. > > It has been successfully tested on Allwinner A33 with a special > i2c chip that returns variable length blocks on reading. > > Signed-off-by: Federico Fuga <fuga@studiofuga.com> > Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com> > Link: https://lore.kernel.org/r/20200118115820.9080-1-fuga@studiofuga.com > [groeck: Various fixes] > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/i2c/busses/i2c-mv64xxx.c | 75 ++++++++++++++++++++++++++------ > 1 file changed, 61 insertions(+), 14 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index 5c8e94b6cdb5..9dda90359966 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -341,7 +341,17 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) > { > drv_data->msg = drv_data->msgs; > drv_data->byte_posn = 0; > - drv_data->bytes_left = drv_data->msg->len; > + > + /* > + * If we should retrieve the length from the buffer, make sure > + * to read enough bytes to avoid sending the STOP bit after > + * the read if the first byte > + */ > + if (drv_data->msg->flags & I2C_M_RECV_LEN) > + drv_data->bytes_left = 3; > + else > + drv_data->bytes_left = drv_data->msg->len; > + > drv_data->aborting = 0; > drv_data->rc = 0; > > @@ -350,6 +360,47 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data) > drv_data->reg_base + drv_data->reg_offsets.control); > } > > +static void > +mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data) > +{ > + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > + writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > + drv_data->reg_base + drv_data->reg_offsets.control); > + drv_data->block = 0; > + if (drv_data->errata_delay) > + udelay(5); > + > + wake_up(&drv_data->waitq); > +} > + > +static void > +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data) > +{ > + u8 data; > + > + data = readl(drv_data->reg_base + drv_data->reg_offsets.data); > + drv_data->msg->buf[drv_data->byte_posn++] = data; > + > + if (drv_data->msg->flags & I2C_M_RECV_LEN) { > + if (!data || data > I2C_SMBUS_BLOCK_MAX) { > + /* > + * FIXME > + * Abort and report error. Needs to be > + * verified/tested on real hardware. > + */ > + drv_data->rc = -EPROTO; > + mv64xxx_i2c_do_send_stop(drv_data); > + return; > + } > + drv_data->msg->flags &= ~I2C_M_RECV_LEN; > + drv_data->bytes_left = data; > + drv_data->msg->len = data + 1; > + } > + > + writel(drv_data->cntl_bits, > + drv_data->reg_base + drv_data->reg_offsets.control); > +} > + > static void > mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > { > @@ -400,23 +451,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data) > break; > > case MV64XXX_I2C_ACTION_RCV_DATA: > - drv_data->msg->buf[drv_data->byte_posn++] = > - readl(drv_data->reg_base + drv_data->reg_offsets.data); > - writel(drv_data->cntl_bits, > - drv_data->reg_base + drv_data->reg_offsets.control); > + mv64xxx_i2c_do_read_data(drv_data); > break; > > case MV64XXX_I2C_ACTION_RCV_DATA_STOP: > drv_data->msg->buf[drv_data->byte_posn++] = > readl(drv_data->reg_base + drv_data->reg_offsets.data); > - drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN; > - writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP, > - drv_data->reg_base + drv_data->reg_offsets.control); > - drv_data->block = 0; > - if (drv_data->errata_delay) > - udelay(5); > - > - wake_up(&drv_data->waitq); > + mv64xxx_i2c_do_send_stop(drv_data); > break; > > case MV64XXX_I2C_ACTION_INVALID: > @@ -680,6 +721,10 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) > if (!drv_data->offload_enabled) > return false; > > + /* Offload not supported for block data transfers */ > + if (msgs[0].flags & I2C_M_RECV_LEN) > + return false; > + > /* > * We can offload a transaction consisting of a single > * message, as long as the message has a length between 1 and > @@ -697,6 +742,7 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) > mv64xxx_i2c_valid_offload_sz(msgs) && > mv64xxx_i2c_valid_offload_sz(msgs + 1) && > !(msgs[0].flags & I2C_M_RD) && > + !(msgs[1].flags & I2C_M_RECV_LEN) && > msgs[1].flags & I2C_M_RD) > return true; > > @@ -713,7 +759,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data) > static u32 > mv64xxx_i2c_functionality(struct i2c_adapter *adap) > { > - return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL; > + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | > + I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL; > } > > static int > -- > 2.25.1 >
On 5/27/21 12:43 PM, Robert Marko wrote: > On Thu, May 27, 2021 at 9:27 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Thu, May 27, 2021 at 09:14:03PM +0200, Robert Marko wrote: >>> On Thu, May 27, 2021 at 6:37 PM Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On 5/27/21 7:58 AM, Robert Marko wrote: >>>> [ ... ] >>>> >>>>>>>>>> I tried applying the block support for mv64xx as well: >>>>>>>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/ >>>>>>>> >>>>>>>> That patch would be needed, but it looks buggy to me. This chunk: >>>>>>>> >>>>>>>> + drv_data->effective_length = data+1; >>>>>>>> + drv_data->bytes_left = data+1; >>>>>>>> + drv_data->msg->len = data+1; >>>>>>>> >>>>>>>> should be: >>>>>>>> >>>>>>>> + drv_data->effective_length = data+1; >>>>>>>> + drv_data->bytes_left = data; >>>>>>>> + drv_data->msg->len = data+1; >>>>>>>> >>>>>>>> It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX, >>>>>>>> and bail out if it isn't. >>>>>>> >>>>>>> Yeah, I did not check the contents, I just saw 2 reviews and tested it >>>>>>> since it can't hurt. >>>>>> >>>>>> That patch doesn't work at all. Make the above change, and also change >>>>>> the type of effective_length from u32 to int, and try again. >>>>> >>>>> I was just looking and it, and doing the changes you recommended make >>>>> no difference at all. >>>>> >>>> >>>> Is the i2c controller compatible with marvell,mv78230-i2c ? >>>> The block transfers would not work in that case. Let me know >>>> and I'll send you a patch that might fix it. >>> >>> Yes, the CPU is Armada 7040 and I can see that compatible is >>> marvell,mv78230-i2c indeed. >>> >>> It would be great if you have the patch, >> >> See below. > > I can confirm that this does indeed enable block reads and > i2c_smbus_read_block_data() > works now. > > PMBUS_MFR_ID and PMBUS_MFR_MODEL are indeed populated. > Excellent. Guenter
On Thu, May 27, 2021 at 09:43:33PM +0200, Robert Marko wrote: [ ... ] > > I can confirm that this does indeed enable block reads and > i2c_smbus_read_block_data() > works now. > Follow-up: > > +static void > > +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data) > > +{ > > + u8 data; > > + > > + data = readl(drv_data->reg_base + drv_data->reg_offsets.data); > > + drv_data->msg->buf[drv_data->byte_posn++] = data; > > + > > + if (drv_data->msg->flags & I2C_M_RECV_LEN) { > > + if (!data || data > I2C_SMBUS_BLOCK_MAX) { > > + /* > > + * FIXME > > + * Abort and report error. Needs to be > > + * verified/tested on real hardware. > > + */ > > + drv_data->rc = -EPROTO; > > + mv64xxx_i2c_do_send_stop(drv_data); > > + return; Can you possibly test if this works ? Trying a block read on any register which returns 0 or > 32 as first byte should do. Thanks, Guenter
diff --git a/Documentation/hwmon/dps920ab.rst b/Documentation/hwmon/dps920ab.rst new file mode 100644 index 000000000000..df0aef530c7e --- /dev/null +++ b/Documentation/hwmon/dps920ab.rst @@ -0,0 +1,80 @@ +.. SPDX-License-Identifier: GPL-2.0-or-later + +Kernel driver dps920ab +======================== + +Supported chips: + + * Delta DPS920AB + + Prefix: 'dps920ab' + + Addresses scanned: - + +Authors: + Robert Marko <robert.marko@sartura.hr> + + +Description +----------- + +This driver implements support for Delta DPS920AB 920W 54V DC single output +power supply with PMBus support. + +The driver is a client driver to the core PMBus driver. +Please see Documentation/hwmon/pmbus.rst for details on PMBus client drivers. + + +Usage Notes +----------- + +This driver does not auto-detect devices. You will have to instantiate the +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for +details. + + +Sysfs entries +------------- + +======================= ====================================================== +curr1_label "iin" +curr1_input Measured input current +curr1_crit Critical maximum current +curr1_crit_alarm Current critical high alarm + +curr2_label "iout1" +curr2_input Measured output current +curr2_crit Critical maximum current +curr2_crit_alarm Current critical high alarm + +in1_label "vin" +in1_input Measured input voltage +in1_lcrit Critical minimum input voltage +in1_lcrit_alarm Input voltage critical low alarm +in1_crit Critical maximum input voltage +in1_crit_alarm Input voltage critical high alarm + +in2_label "vout1" +in2_input Measured output voltage +in2_lcrit Critical minimum output voltage +in2_lcrit_alarm Output voltage critical low alarm +in2_crit Critical maximum output voltage +in2_crit_alarm Output voltage critical high alarm + +power1_label "pin" +power1_input Measured input power +power1_alarm Input power high alarm + +power2_label "pout1" +power2_input Measured output power + +temp[1-2]_input Measured temperature +temp[1-2]_crit Critical high temperature +temp[1-2]_crit_alarm Chip temperature critical high alarm +temp[1-2]_max Maximum temperature +temp[1-2]_max_alarm Chip temperature high alarm + +fan1_alarm Fan 1 warning. +fan1_fault Fan 1 fault. +fan1_input Fan 1 speed in RPM. +======================= ====================================================== diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 8d5a2df1ecb6..b24436f22052 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -54,6 +54,7 @@ Hardware Monitoring Kernel Drivers dell-smm-hwmon dme1737 drivetemp + dps920ab ds1621 ds620 emc1403 diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig index 32d2fc850621..865ade0aa205 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -66,6 +66,15 @@ config SENSORS_IBM_CFFPS This driver can also be built as a module. If so, the module will be called ibm-cffps. +config SENSORS_DPS920AB + tristate "Delta DPS920AB Power Supply" + help + If you say yes here you get hardware monitoring support for Delta + DPS920AB Power Supplies. + + This driver can also be built as a module. If so, the module will + be called dps920ab. + config SENSORS_INSPUR_IPSPS tristate "INSPUR Power System Power Supply" help diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile index 6a4ba0fdc1db..f59ba0123d68 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_IBM_CFFPS) += ibm-cffps.o +obj-$(CONFIG_SENSORS_DPS920AB) += dps920ab.o obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o obj-$(CONFIG_SENSORS_IR35221) += ir35221.o obj-$(CONFIG_SENSORS_IR38064) += ir38064.o diff --git a/drivers/hwmon/pmbus/dps920ab.c b/drivers/hwmon/pmbus/dps920ab.c new file mode 100644 index 000000000000..d579ed9f879c --- /dev/null +++ b/drivers/hwmon/pmbus/dps920ab.c @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Driver for Delta DPS920AB PSU + * + * Copyright (C) 2021 Delta Networks, Inc. + * Copyright (C) 2021 Sartura Ltd. + */ + +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include "pmbus.h" + +static struct pmbus_driver_info dps920ab_info = { + .pages = 1, + + .format[PSC_VOLTAGE_IN] = linear, + .format[PSC_VOLTAGE_OUT] = linear, + .format[PSC_CURRENT_IN] = linear, + .format[PSC_CURRENT_OUT] = linear, + .format[PSC_POWER] = linear, + .format[PSC_FAN] = linear, + .format[PSC_TEMPERATURE] = linear, + + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | + PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | + PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 | + PMBUS_HAVE_PIN | PMBUS_HAVE_POUT | + PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | + PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP, +}; + +static int dps920ab_probe(struct i2c_client *client) +{ + if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_BYTE_DATA | + I2C_FUNC_SMBUS_WORD_DATA)) + return -ENODEV; + + return pmbus_do_probe(client, &dps920ab_info); +} + +static const struct of_device_id __maybe_unused dps920ab_of_match[] = { + { .compatible = "delta,dps920ab", }, + {} +}; + +MODULE_DEVICE_TABLE(of, dps920ab_of_match); + +static struct i2c_driver dps920ab_driver = { + .driver = { + .name = "dps920ab", + .of_match_table = of_match_ptr(dps920ab_of_match), + }, + .probe_new = dps920ab_probe, +}; + +module_i2c_driver(dps920ab_driver); + +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>"); +MODULE_DESCRIPTION("PMBus driver for Delta DPS920AB PSU"); +MODULE_LICENSE("GPL");
This adds support for the Delta DPS-920AB PSU. Only missing feature is fan control which the PSU supports. Signed-off-by: Robert Marko <robert.marko@sartura.hr> --- Documentation/hwmon/dps920ab.rst | 80 ++++++++++++++++++++++++++++++++ Documentation/hwmon/index.rst | 1 + drivers/hwmon/pmbus/Kconfig | 9 ++++ drivers/hwmon/pmbus/Makefile | 1 + drivers/hwmon/pmbus/dps920ab.c | 63 +++++++++++++++++++++++++ 5 files changed, 154 insertions(+) create mode 100644 Documentation/hwmon/dps920ab.rst create mode 100644 drivers/hwmon/pmbus/dps920ab.c