| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f9c8fd492e9db120e33e42712608d54bb6ad15cc.camel@sylv.io>
Date: Mon, 14 Feb 2022 10:26:27 +0100
From: sylv <sylv@...v.io>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: Jean Delvare <jdelvare@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 3/4] pmbus: Add support for pli1209bc
On Fri, 2022-02-11 at 11:40 -0800, Guenter Roeck wrote:
> On 2/11/22 09:17, Marcello Sylvester Bauer wrote:
> > PLI1209BC is a Digital Supervisor from Vicor Corporation.
> >
> > Signed-off-by: Marcello Sylvester Bauer <sylv@...v.io>
> > ---
> > Documentation/hwmon/pli1209bc.rst | 73 +++++++++++++++++++++
> > drivers/hwmon/pmbus/Kconfig | 9 +++
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/pli1209bc.c | 104
> > ++++++++++++++++++++++++++++++
> > 4 files changed, 187 insertions(+)
> > create mode 100644 Documentation/hwmon/pli1209bc.rst
> > create mode 100644 drivers/hwmon/pmbus/pli1209bc.c
> >
> > diff --git a/Documentation/hwmon/pli1209bc.rst
> > b/Documentation/hwmon/pli1209bc.rst
> > new file mode 100644
> > index 000000000000..a3f686d03cf2
> > --- /dev/null
> > +++ b/Documentation/hwmon/pli1209bc.rst
> > @@ -0,0 +1,73 @@
> > +Kernel driver pli1209bc
> > +=======================
> > +
> > +Supported chips:
> > +
> > + * Digital Supervisor PLI1209BC
> > +
> > + Prefix: 'pli1209bc'
> > +
> > + Addresses scanned: 0x50 - 0x5F
> > +
> > + Datasheet:
> > https://www.vicorpower.com/documents/datasheets/ds-PLI1209BCxyzz-VICOR.pdf
> > +
> > +Authors:
> > + - Marcello Sylvester Bauer <sylv@...v.io>
> > +
> > +Description
> > +-----------
> > +
> > +The Vicor PLI1209BC is an isolated digital power system supervisor
> > thatprovides
> > +a communication interface between a host processor and one Bus
> > Converter Module
> > +(BCM). The PLI communicates with a system controller via a PMBus
> > compatible
> > +interface over an isolated UART interface. Through the PLI, the
> > host processor
> > +can configure, set protection limits, and monitor the BCM.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +=======================
> > ========================================================
> > +in1_label "vin2"
> > +in1_input Input voltage.
> > +in1_rated_min Minimum rated input voltage.
> > +in1_rated_max Maximum rated input voltage.
> > +in1_max Maximum input voltage.
> > +in1_max_alarm Input voltage high alarm.
> > +in1_crit Critical input voltage.
> > +in1_crit_alarm Input voltage critical alarm.
> > +
> > +in2_label "vout2"
> > +in2_input Output voltage.
> > +in2_rated_min Minimum rated output voltage.
> > +in2_rated_max Maximum rated output voltage.
> > +in2_alarm Output voltage alarm
> > +
> > +curr1_label "iin2"
> > +curr1_input Input current.
> > +curr1_max Maximum input current.
> > +curr1_max_alarm Maximum input current high alarm.
> > +curr1_crit Critical input current.
> > +curr1_crit_alarm Input current critical alarm.
> > +
> > +curr2_label "iout2"
> > +curr2_input Output current.
> > +curr2_crit Critical output current.
> > +curr2_crit_alarm Output current critical alarm.
> > +curr2_max Maximum output current.
> > +curr2_max_alarm Output current high alarm.
> > +
> > +power1_label "pin2"
> > +power1_input Input power.
> > +power1_alarm Input power alarm.
> > +
> > +power2_label "pout2"
> > +power2_input Output power.
> > +power2_rated_max Maximum rated output power.
> > +
> > +temp1_input Die temperature.
> > +temp1_alarm Die temperature alarm.
> > +temp1_max Maximum die temperature.
> > +temp1_max_alarm Die temperature high alarm.
> > +temp1_crit Critical die temperature.
> > +temp1_crit_alarm Die temperature critical alarm.
> > +=======================
> > ========================================================
> > diff --git a/drivers/hwmon/pmbus/Kconfig
> > b/drivers/hwmon/pmbus/Kconfig
> > index c96f7b7338bd..831db423bea0 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -310,6 +310,15 @@ config SENSORS_PIM4328
> > This driver can also be built as a module. If so, the
> > module will
> > be called pim4328.
> >
> > +config SENSORS_PLI1209BC
> > + tristate "Vicor PLI1209BC"
> > + help
> > + If you say yes here you get hardware monitoring support
> > for Vicor
> > + PLI1209BC Digital Supervisor.
> > +
> > + This driver can also be built as a module. If so, the
> > module will
> > + be called pli1209bc.
> > +
> > config SENSORS_PM6764TR
> > tristate "ST PM6764TR"
> > help
> > diff --git a/drivers/hwmon/pmbus/Makefile
> > b/drivers/hwmon/pmbus/Makefile
> > index e5935f70c9e0..7ce74e3b8552 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_SENSORS_MP2888) += mp2888.o
> > obj-$(CONFIG_SENSORS_MP2975) += mp2975.o
> > obj-$(CONFIG_SENSORS_MP5023) += mp5023.o
> > obj-$(CONFIG_SENSORS_PM6764TR) += pm6764tr.o
> > +obj-$(CONFIG_SENSORS_PLI1209BC) += pli1209bc.o
> > obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o
> > obj-$(CONFIG_SENSORS_Q54SJ108A2) += q54sj108a2.o
> > obj-$(CONFIG_SENSORS_STPDDC60) += stpddc60.o
> > diff --git a/drivers/hwmon/pmbus/pli1209bc.c
> > b/drivers/hwmon/pmbus/pli1209bc.c
> > new file mode 100644
> > index 000000000000..da345eb105fd
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/pli1209bc.c
> > @@ -0,0 +1,104 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for Vicor PLI1209BC Digital
> > Supervisor
> > + *
> > + * Copyright (c) 2022 9elements GmbH
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pmbus.h>
> > +#include "pmbus.h"
> > +
> > +/*
> > + * The capability command is only supported at page 0. Probing the
> > device while
> > + * the page register is set to 1 will falsely enable PEC support.
> > Disable
> > + * capability probing accordingly, since the PLI1209BC does not
> > have any
> > + * additional capabilities.
> > + */
> > +static struct pmbus_platform_data pli1209bc_plat_data = {
> > + .flags = PMBUS_NO_CAPABILITY,
> > +};
> > +
> > +static int pli1209bc_read_word_data(struct i2c_client *client, int
> > page,
> > + int phase, int reg)
> > +{
> > + int data;
> > +
> > + switch (reg) {
> > + /* PMBUS_READ_PIN uses a direct format with R=1 */
> > + case PMBUS_READ_PIN:
> > + data = pmbus_read_word_data(client, page, phase,
> > reg);
> > + if (data < 0)
> > + return data;
> > + return div_s64(data + 5LL, 10L);
>
> This results in a loss of precision. I think it would be better to
> avoid that
> and instead model the output power as direct format with R=1, and
> multiply
> register values reported for PMBUS_READ_POUT with 10.
>
Makes sense, ack.
> > + default:
> > + return -ENODATA;
> > + }
> > +}
> > +
> > +static struct pmbus_driver_info pli1209bc_info = {
> > + .pages = 2,
> > + .format[PSC_VOLTAGE_IN] = direct,
> > + .format[PSC_VOLTAGE_OUT] = direct,
> > + .format[PSC_CURRENT_IN] = direct,
> > + .format[PSC_CURRENT_OUT] = direct,
> > + .format[PSC_POWER] = linear,
> > + .format[PSC_TEMPERATURE] = linear,
>
> Those look like direct values to me. Linear would mean there is a
> mantissa and an exponent, but I don't see that in the datasheet.
> Please clarify; we don't want to have power values of 1024 and
> above show up as negative numbers.
My bad. I misunderstood the meaning of linear and thought it means
direct m=1,R=0,b=0. Thanks.
>
> > + .m[PSC_VOLTAGE_IN] = 1,
> > + .b[PSC_VOLTAGE_IN] = 0,
> > + .R[PSC_VOLTAGE_IN] = 1,
> > + .m[PSC_VOLTAGE_OUT] = 1,
> > + .b[PSC_VOLTAGE_OUT] = 0,
> > + .R[PSC_VOLTAGE_OUT] = 1,
> > + .m[PSC_CURRENT_IN] = 1,
> > + .b[PSC_CURRENT_IN] = 0,
> > + .R[PSC_CURRENT_IN] = 3,
> > + .m[PSC_CURRENT_OUT] = 1,
> > + .b[PSC_CURRENT_OUT] = 0,
> > + .R[PSC_CURRENT_OUT] = 2,
> > + .func[0] = 0, /* supervisor summing page without voltage
> > readings */
>
> As far as I can see from the manual, the actually reported values for
> other
> attributes match those of page 1, making the values on page 0
> redundant.
> I think this should be mentioned in a comment to avoid confusion.
>
Sure, sgtm.
>
> > + .func[1] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT
> > + | PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT
> > + | PMBUS_HAVE_PIN | PMBUS_HAVE_POUT
> > + | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> > + | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT,
> > + .read_word_data = pli1209bc_read_word_data,
> > +};
> > +
> > +static int pli1209bc_probe(struct i2c_client *client)
> > +{
> > + client->dev.platform_data = &pli1209bc_plat_data;
> > + return pmbus_do_probe(client, &pli1209bc_info);
> > +}
> > +
> > +static const struct i2c_device_id pli1209bc_id[] = {
> > + {"pli1209bc", 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, pli1209bc_id);
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id pli1209bc_of_match[] = {
> > + { .compatible = "vicor,pli1209bc" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, pli1209bc_of_match);
> > +#endif
> > +
> > +/* This is the driver that will be inserted */
>
> Pretty useless comment.
ack.
Thanks for your quick review Guenter.
Marcello
>
> > +static struct i2c_driver pli1209bc_driver = {
> > + .driver = {
> > + .name = "pli1209bc",
> > + .of_match_table =
> > of_match_ptr(pli1209bc_of_match),
> > + },
> > + .probe_new = pli1209bc_probe,
> > + .id_table = pli1209bc_id,
> > +};
> > +
> > +module_i2c_driver(pli1209bc_driver);
> > +
> > +MODULE_AUTHOR("Marcello Sylvester Bauer <sylv@...v.io>");
> > +MODULE_DESCRIPTION("PMBus driver for Vicor PLI1209BC");
> > +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists