Hi Guenter!
> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Sunday, May 2, 2021 6:47 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; robh+dt@xxxxxxxxxx
> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH hwmon-next v3 2/3] hwmon: (pmbus) Add support for
> MPS Multi-phase mp2888 controller
>
> On 4/26/21 3:17 PM, Vadim Pasternak wrote:
> > Add support for mp2888 device from Monolithic Power Systems, Inc.
> > (MPS) vendor. This is a digital, multi-phase, pulse-width modulation
> > controller.
> >
> > This device supports:
> > - One power rail.
> > - Programmable Multi-Phase up to 10 Phases.
> > - PWM-VID Interface
> > - One pages 0 for telemetry.
> > - Programmable pins for PMBus Address.
> > - Built-In EEPROM to Store Custom Configurations.
> > - Can configured VOUT readout in direct or VID format and allows
> > setting of different formats on rails 1 and 2. For VID the following
> > protocols are available: VR13 mode with 5-mV DAC; VR13 mode with
> > 10-mV DAC, IMVP9 mode with 5-mV DAC.
> >
> > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxx>
> > ---
> > v2->v3
> > Comments pointed out by Guenter:
> > - Fix precision for PMBUS_READ_VIN (it requires adding scale for
> > PMBUS_VIN_OV_FAULT_LIMIT and PMBUS_VIN_UV_WARN_LIMIT.
> > - Fix precision for PMBUS_READ_TEMPERATURE_1 (it requires adding
> > scale for PMBUS_OT_WARN_LIMIT).
> > - Use DIV_ROUND_CLOSEST_ULL for scaling PMBUS_READ_POUT,
> > PMBUS_READ_PIN readouts.
> > Notes and fixes from Vadim:
> > - READ_IOUT in linear11 does not give write calculation (tested with
> > external load), while in direct format readouts are correct.
> > - Disable non-configured phases in mp2888_identify_multiphase().
> >
> > v1->v2:
> > Comments pointed out by Guenter:
> > - Use standard access for getting PMBUS_OT_WARN_LIMIT,
> > PMBUS_VIN_OV_FAULT_LIMIT, PMBUS_VIN_UV_WARN_LIMIT.
> > - Use linear11 conversion for PMBUS_READ_VIN, PMBUS_READ_POUT,
> > PMBUS_READ_PIN, PMBUS_READ_TEMPERATURE_1 and adjust
> coefficients.
> > - Add reading phases current from the dedicated registers.
> > - Add comment for not implemented or implemented not according to the
> > spec registers, for which "ENXIO" code is returned.
> > - Set PMBUS_HAVE_IOUT" statically.
> > Notes from Vadim:
> > - READ_IOUT uses direct format, so I did not adjust it like the below
> > registers.
> > ---
> > Documentation/hwmon/mp2888.rst | 111 ++++++++++++
> > drivers/hwmon/pmbus/Kconfig | 9 +
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/mp2888.c | 387
> +++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 508 insertions(+)
> > create mode 100644 Documentation/hwmon/mp2888.rst create mode
> 100644
> > drivers/hwmon/pmbus/mp2888.c
> >
> > diff --git a/Documentation/hwmon/mp2888.rst
> > b/Documentation/hwmon/mp2888.rst new file mode 100644 index
> > 000000000000..7839a010642a
> > --- /dev/null
> > +++ b/Documentation/hwmon/mp2888.rst
> > @@ -0,0 +1,111 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver mp2888
> > +====================
> > +
> > +Supported chips:
> > +
> > + * MPS MP12254
> > +
> > + Prefix: 'mp2888'
> > +
> > +Author:
> > +
> > + Vadim Pasternak <vadimp@xxxxxxxxxx>
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for Monolithic Power Systems, Inc.
> > +(MPS) vendor dual-loop, digital, multi-phase controller MP2888.
> > +
> > +This device: supports:
> > +
> > +- One power rail.
> > +- Programmable Multi-Phase up to 10 Phases.
> > +- PWM-VID Interface
> > +- One pages 0 for telemetry.
> > +- Programmable pins for PMBus Address.
> > +- Built-In EEPROM to Store Custom Configurations.
> > +
> > +Device complaint with:
> > +
> > +- PMBus rev 1.3 interface.
> > +
> > +Device supports direct format for reading output current, output
> > +voltage, input and output power and temperature.
> > +Device supports linear format for reading input voltage and input power.
> > +
> > +The driver provides the next attributes for the current:
> > +
> > +- for current out input and maximum alarm;
> > +- for phase current: input and label.
> > +
> > +The driver exports the following attributes via the 'sysfs' files, where:
> > +
> > +- 'n' is number of configured phases (from 1 to 10);
> > +- index 1 for "iout";
> > +- indexes 2 ... 1 + n for phases.
> > +
> > +**curr1_alarm**
> > +
> > +**curr[1-{1+n}]_input**
> > +
> > +**curr[1-{1+n}]_label**
> > +
> > +The driver provides the next attributes for the voltage:
> > +
> > +- for voltage in: input, low and high critical thresholds, low and
> > +high
> > + critical alarms;
> > +- for voltage out: input and high alarm;
> > +
> > +The driver exports the following attributes via the 'sysfs' files,
> > +where
> > +
> > +**in1_crit**
> > +
> > +**in1_crit_alarm**
> > +
> > +**in1_input**
> > +
> > +**in1_label**
> > +
> > +**in1_min**
> > +
> > +**in1_min_alarm**
> > +
> > +**in2_alarm**
> > +
> > +**in2_input**
> > +
> > +**in2_label**
> > +
> > +The driver provides the next attributes for the power:
> > +
> > +- for power in alarm and input.
> > +- for power out: cap, cap alarm an input.
> > +
> > +The driver exports the following attributes via the 'sysfs' files,
> > +where
> > +- indexes 1 for "pin";
> > +- indexes 2 for "pout";
> > +
> > +**power1_alarm**
> > +
> > +**power1_input**
> > +
> > +**power1_label**
> > +
> > +**power2_cap**
> > +
> > +**power2_cap_alarm**
> > +
> > +**power2_input**
> > +
> > +**power2_label**
> > +
> > +The driver provides the next attributes for the temperature:
> > +
> > +**temp1_input**
> > +
> > +**temp1_max**
> > +
> > +**temp1_max_alarm**
> > diff --git a/drivers/hwmon/pmbus/Kconfig
> b/drivers/hwmon/pmbus/Kconfig
> > index 32d2fc850621..a57571928b31 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -211,6 +211,15 @@ config SENSORS_MAX8688
> > This driver can also be built as a module. If so, the module will
> > be called max8688.
> >
> > +config SENSORS_MP2888
> > + tristate "MPS MP2888"
> > + help
> > + If you say yes here you get hardware monitoring support for MPS
> > + MP2888 Digital, Multi-Phase, Pulse-Width Modulation Controller.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called mp2888.
> > +
> > config SENSORS_MP2975
> > tristate "MPS MP2975"
> > help
> > diff --git a/drivers/hwmon/pmbus/Makefile
> > b/drivers/hwmon/pmbus/Makefile index 6a4ba0fdc1db..a6d7352621ca
> 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_SENSORS_MAX20751) +=
> max20751.o
> > obj-$(CONFIG_SENSORS_MAX31785) += max31785.o
> > obj-$(CONFIG_SENSORS_MAX34440) += max34440.o
> > obj-$(CONFIG_SENSORS_MAX8688) += max8688.o
> > +obj-$(CONFIG_SENSORS_MP2888) += mp2888.o
> > obj-$(CONFIG_SENSORS_MP2975) += mp2975.o
> > obj-$(CONFIG_SENSORS_PM6764TR) += pm6764tr.o
> > obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o
> > diff --git a/drivers/hwmon/pmbus/mp2888.c
> > b/drivers/hwmon/pmbus/mp2888.c new file mode 100644 index
> > 000000000000..4b90f0e8cbd7
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/mp2888.c
> > @@ -0,0 +1,387 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Hardware monitoring driver for MPS Multi-phase Digital VR
> > +Controllers
> > + *
> > + * Copyright (C) 2020 Nvidia Technologies Ltd.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include "pmbus.h"
> > +
> > +/* Vendor specific registers. */
> > +#define MP2888_MFR_SYS_CONFIG 0x44
> > +#define MP2888_MFR_READ_CS1_2 0x73
> > +#define MP2888_MFR_READ_CS3_4 0x74
> > +#define MP2888_MFR_READ_CS5_6 0x75
> > +#define MP2888_MFR_READ_CS7_8 0x76
> > +#define MP2888_MFR_READ_CS9_10 0x77
> > +#define MP2888_MFR_VR_CONFIG1 0xe1
> > +
> > +#define MP2888_TOTAL_CURRENT_RESOLUTION BIT(3)
> > +#define MP2888_PHASE_CURRENT_RESOLUTION BIT(4)
> > +#define MP2888_DRMOS_KCS GENMASK(2, 0)
> > +#define MP2888_VIN_LIMIT_UNIT 8
> > +#define MP2888_TEMP_UNIT 10
> > +#define MP2888_VIN_MANTISSA 32
> > +#define MP2888_MAX_PHASE 10
> > +
> > +struct mp2888_data {
> > + struct pmbus_driver_info info;
> > + int total_curr_resolution;
> > + int phase_curr_resolution;
> > + int curr_sense_gain;
> > +};
> > +
> > +#define to_mp2888_data(x) container_of(x, struct mp2888_data, info)
> > +
> > +static int mp2888_read_byte_data(struct i2c_client *client, int page,
> > +int reg) {
> > + switch (reg) {
> > + case PMBUS_VOUT_MODE:
> > + /* Enforce VOUT direct format. */
> > + return PB_VOUT_MODE_DIRECT;
> > + default:
> > + return -ENODATA;
> > + }
> > +}
> > +
> > +/* Convert linear sensor values to milli- or micro-units depending on
> > +sensor type. */ static s64 mp2888_reg2data_linear11(int data, enum
> > +pmbus_sensor_classes class) {
> > + s16 exponent;
> > + s32 mantissa;
> > + s64 val;
> > +
> > + exponent = ((s16)data) >> 11;
> > + mantissa = ((s16)((data & 0x7ff) << 5)) >> 5;
> > + val = mantissa;
> > +
> > + /* Scale result to micro-units for power sensors. */
> > + if (class == PSC_POWER)
> > + val = val * 1000LL;
> > +
> > + if (exponent >= 0)
> > + val <<= exponent;
> > + else
> > + val >>= -exponent;
> > +
> > + return val;
> > +}
> > +
> > +static int
> > +mp2888_current_sense_gain_and_resolution_get(struct i2c_client
> > +*client, struct mp2888_data *data) {
> > + int ret;
> > +
> > + /*
> > + * Obtain DrMOS current sense gain of power stage from the register
> > + * , bits 0-2. The value is selected as below:
> > + * 00b - 5µA/A, 01b - 8.5µA/A, 10b - 9.7µA/A, 11b - 10µA/A. Other
> > + * values are reserved.
> > + */
> > + ret = i2c_smbus_read_word_data(client,
> MP2888_MFR_SYS_CONFIG);
> > + if (ret < 0)
> > + return ret;
> > +
> > + switch (ret & MP2888_DRMOS_KCS) {
> > + case 0:
> > + data->curr_sense_gain = 85;
> > + break;
> > + case 1:
> > + data->curr_sense_gain = 97;
> > + break;
> > + case 2:
> > + data->curr_sense_gain = 100;
> > + break;
> > + case 3:
> > + data->curr_sense_gain = 50;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Obtain resolution selector for total and phase current report and
> protection.
> > + * 0: original resolution; 1: half resolution (in such case phase current
> value should
> > + * be doubled.
> > + */
> > + data->total_curr_resolution = (ret &
> MP2888_TOTAL_CURRENT_RESOLUTION) >> 3;
> > + data->phase_curr_resolution = (ret &
> > +MP2888_PHASE_CURRENT_RESOLUTION) >> 4;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +mp2888_read_phase(struct i2c_client *client, struct mp2888_data
> > +*data, int page, int phase, u8 reg) {
> > + int ret;
> > +
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!((phase + 1) % 2))
> > + ret >>= 8;
> > + ret &= 0xff;
> > +
> > + /*
> > + * Output value is calculated as: (READ_CSx / 80 – 1.23) / (Kcs * Rcs)
> > + * where:
> > + * - Kcs is the DrMOS current sense gain of power stage, which is
> obtained from the
> > + * register MP2888_MFR_VR_CONFIG1, bits 13-12 with the following
> selection of DrMOS
> > + * (data->curr_sense_gain):
> > + * 00b - 5µA/A, 01b - 8.5µA/A, 10b - 9.7µA/A, 11b - 10µA/A.
> > + * - Rcs is the internal phase current sense resistor. This parameter
> depends on hardware
> > + * assembly. By default it is set to 1kΩ. In case of different assembly,
> user should
> > + * scale this parameter by dividing it by Rcs.
> > + * If phase current resolution bit is set to 1, READ_CSx value should
> be doubled.
> > + * Note, that current phase sensing, providing by the device is not
> accurate. This is
> > + * because sampling of current occurrence of bit weight has a big
> deviation, especially for
> > + * light load.
> > + */
> > + if (data->phase_curr_resolution)
> > + ret *= 2;
> > + return DIV_ROUND_CLOSEST(ret * 100 - 9800, data-
> >curr_sense_gain); }
> > +
> > +static int
> > +mp2888_read_phases(struct i2c_client *client, struct mp2888_data
> > +*data, int page, int phase) {
> > + int ret;
> > +
> > + switch (phase) {
> > + case 0 ... 1:
> > + ret = mp2888_read_phase(client, data, page, phase,
> MP2888_MFR_READ_CS1_2);
> > + break;
> > + case 2 ... 3:
> > + ret = mp2888_read_phase(client, data, page, phase,
> MP2888_MFR_READ_CS3_4);
> > + break;
> > + case 4 ... 5:
> > + ret = mp2888_read_phase(client, data, page, phase,
> MP2888_MFR_READ_CS5_6);
> > + break;
> > + case 6 ... 7:
> > + ret = mp2888_read_phase(client, data, page, phase,
> MP2888_MFR_READ_CS7_8);
> > + break;
> > + case 8 ... 9:
> > + ret = mp2888_read_phase(client, data, page, phase,
> MP2888_MFR_READ_CS9_10);
> > + break;
> > + default:
> > + return -ENODATA;
> > + }
> > + return ret;
> > +}
> > +
> > +static int mp2888_read_word_data(struct i2c_client *client, int page,
> > +int phase, int reg) {
> > + const struct pmbus_driver_info *info =
> pmbus_get_driver_info(client);
> > + struct mp2888_data *data = to_mp2888_data(info);
> > + s64 val;
> > + int ret;
> > +
> > + switch (reg) {
> > + case PMBUS_READ_VIN:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + /*
> > + * READ_VIN register has unused bits 15:10 with fixed value
> 111011b. Clear these
> > + * bits and scale with coefficient 0.03125 by using mantissa 32.
> > + */
> > + ret = (ret & GENMASK(9, 0));
> > + break;
> > + case PMBUS_VIN_OV_FAULT_LIMIT:
> > + case PMBUS_VIN_UV_WARN_LIMIT:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + val = mp2888_reg2data_linear11(ret, PSC_VOLTAGE_IN);
> > + ret = val * MP2888_VIN_MANTISSA;
> > + break;
>
> Unless I am missing something, this is linear11 with, for some reason, bit 10
> erroneously set for PMBUS_READ_VIN. That means that setting the format
> for PSC_VOLTAGE_IN and clearing bit 10 for PMBUS_READ_VIN should work
> without local conversion. Am I missing something ?
On board I have vin 12V.
>From register I am getting 0xed80.
If I am masking bit 10, 0xe980, it gives me 48.12 V, masking bits 11,10 gives 24.00.
For getting 12, I should mask bits 11, 10 and shift left bits 8 - 0.
But I it doesn't look right things to do.
>
> > + case PMBUS_OT_WARN_LIMIT:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > + ret *= MP2888_TEMP_UNIT;
>
> Explanation would be useful: chip reports limits in degrees C, but the actual
> temperature in 10th of degrees C. This is needed to match the two.
>
ACK.
> > + break;
> > + case PMBUS_READ_IOUT:
> > + if (phase != 0xff)
> > + return mp2888_read_phases(client, data, page,
> phase);
> > +
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > + /*
> > + * READ_IOUT register has unused bits 15:12 with fixed value
> 1110b. Clear these
> > + * bits and scale with total current resolution. Data is provided
> in direct format.
> > + */
> > + ret &= GENMASK(11, 0);
> > + ret = data->total_curr_resolution ?
> DIV_ROUND_CLOSEST(ret, 2) :
> > + DIV_ROUND_CLOSEST(ret, 4);
>
> I don't think this is a good idea: It unnecessarily reduces the resolution (which
> is bad enough already). The format for output current is already set to direct
> with a mantissa of 1. It might make sense to adjust the mantissa to always
> report the correct current. Specifically, if current resolution is set to 1, the
> value here should be multiplied, not divided.
>
Yes, I'll do it through mantissa.
>
> > + break;
> > + case PMBUS_READ_POUT:
> > + case PMBUS_READ_PIN:
> > + ret = pmbus_read_word_data(client, page, phase, reg);
> > + if (ret < 0)
> > + return ret;
> > + val = mp2888_reg2data_linear11(ret, PSC_POWER);
>
> This call seems pretty pointless: The exponent bits are always 0, and we
> know that the resolution is either 0.5W or 1W.
>
> Conversion to micro-watt should be handled by the PMBus core.
>
> > + ret = data->total_curr_resolution ? val :
> > +DIV_ROUND_CLOSEST_ULL(val, 2);
>
> Same as above: Any adjustment should be a multiplication, not a divide
> operation, to avoid resolution loss.
ACK.
>
> > + break;
> > + /*
> > + * The below registers are not implemented by device or
> implemented not according to the
> > + * spec. Skip all of them to avoid exposing non-relevant inputs to
> sysfs.
> > + */
> > + case PMBUS_OT_FAULT_LIMIT:
> > + case PMBUS_UT_WARN_LIMIT:
> > + case PMBUS_UT_FAULT_LIMIT:
> > + case PMBUS_VIN_UV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_WARN_LIMIT:
> > + case PMBUS_VOUT_OV_WARN_LIMIT:
> > + case PMBUS_VOUT_UV_FAULT_LIMIT:
> > + case PMBUS_VOUT_OV_FAULT_LIMIT:
> > + case PMBUS_VIN_OV_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_LV_FAULT_LIMIT:
> > + case PMBUS_IOUT_OC_WARN_LIMIT:
>
> Datasheet says this is a 10-bit value with a scale of 1A or 2A depending on the
> current resolution.
OK
>
> > + case PMBUS_IOUT_OC_FAULT_LIMIT:
> > + case PMBUS_IOUT_UC_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_WARN_LIMIT:
>
> Datasheet: 10-bit value with 1W/LSB or 2W/LSB depending on current
> resolution.
OK
Along with it I should disable PMBUS_POUT_MAX. In datasheet it is for different purpose.
I missed it.
>
>
> Unless there is reqason to believe that the datasheet is wrong, those should
> be supported. If the datasheet is wrong it should be explained.
>
> > + case PMBUS_PIN_OP_WARN_LIMIT:
> > + case PMBUS_MFR_VIN_MIN:
> > + case PMBUS_MFR_VIN_MAX:
> > + case PMBUS_MFR_VOUT_MAX:
> > + case PMBUS_MFR_IIN_MAX:
> > + case PMBUS_MFR_IOUT_MAX:
> > + case PMBUS_MFR_PIN_MAX:
> > + case PMBUS_MFR_POUT_MAX:
> > + case PMBUS_MFR_MAX_TEMP_1:
> > + return -ENXIO;
> > + default:
> > + return -ENODATA;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int
> > +mp2888_identify_multiphase(struct i2c_client *client, struct mp2888_data
> *data,
> > + struct pmbus_driver_info *info) {
> > + int i, ret;
> > +
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify multiphase number - could be from 1 to 10. */
> > + ret = i2c_smbus_read_word_data(client,
> MP2888_MFR_VR_CONFIG1);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + info->phases[0] = ret & GENMASK(3, 0);
> > +
> > + /*
> > + * The device provides a total of 10 PWM pins, and can be configured
> to different phase
> > + * count applications for rail.
> > + */
> > + if (info->phases[0] > MP2888_MAX_PHASE)
> > + return -EINVAL;
> > +
> > + /* Disable non-configured phases. */
> > + for (i = info->phases[0]; i < MP2888_MAX_PHASE; i++)
> > + info->pfunc[i] = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static struct pmbus_driver_info mp2888_info = {
> > + .pages = 1,
> > + .format[PSC_VOLTAGE_IN] = direct,
> > + .format[PSC_VOLTAGE_OUT] = direct,
> > + .format[PSC_TEMPERATURE] = direct,
> > + .format[PSC_CURRENT_IN] = linear,
> > + .format[PSC_CURRENT_OUT] = direct,
> > + .format[PSC_POWER] = direct,
> > + .m[PSC_VOLTAGE_IN] = MP2888_VIN_MANTISSA,
> > + .m[PSC_TEMPERATURE] = 1,
> > + .R[PSC_TEMPERATURE] = 1,
> > + .m[PSC_VOLTAGE_OUT] = 1,
> > + .R[PSC_VOLTAGE_OUT] = 3,
> > + .m[PSC_CURRENT_OUT] = 1,
> > + .m[PSC_POWER] = 1,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT |
> PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_TEMP |
> PMBUS_HAVE_STATUS_TEMP |
> > + PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
> PMBUS_HAVE_STATUS_INPUT |
> > + PMBUS_PHASE_VIRTUAL,
> > + .pfunc[0] = PMBUS_HAVE_IOUT,
> > + .pfunc[1] = PMBUS_HAVE_IOUT,
> > + .pfunc[2] = PMBUS_HAVE_IOUT,
> > + .pfunc[3] = PMBUS_HAVE_IOUT,
> > + .pfunc[4] = PMBUS_HAVE_IOUT,
> > + .pfunc[5] = PMBUS_HAVE_IOUT,
> > + .pfunc[6] = PMBUS_HAVE_IOUT,
> > + .pfunc[7] = PMBUS_HAVE_IOUT,
> > + .pfunc[8] = PMBUS_HAVE_IOUT,
> > + .pfunc[9] = PMBUS_HAVE_IOUT,
> > + .read_byte_data = mp2888_read_byte_data,
> > + .read_word_data = mp2888_read_word_data,
>
> I don't see a write function here. Did you try setting limits ?
> Given the adjustments needed for reading limits, I don't think this will work.
No, I didn't try.
>
> Thanks,
> Guenter
>
> > +};
> > +
> > +static int mp2888_probe(struct i2c_client *client) {
> > + struct pmbus_driver_info *info;
> > + struct mp2888_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(struct mp2888_data),
> GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + memcpy(&data->info, &mp2888_info, sizeof(*info));
> > + info = &data->info;
> > +
> > + /* Identify multiphase configuration. */
> > + ret = mp2888_identify_multiphase(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Obtain current sense gain of power stage and current resolution.
> */
> > + ret = mp2888_current_sense_gain_and_resolution_get(client, data);
> > + if (ret)
> > + return ret;
> > +
> > + return pmbus_do_probe(client, info); }
> > +
> > +static const struct i2c_device_id mp2888_id[] = {
> > + {"mp2888", 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mp2888_id);
> > +
> > +static const struct of_device_id __maybe_unused mp2888_of_match[] = {
> > + {.compatible = "mps,mp2888"},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, mp2888_of_match);
> > +
> > +static struct i2c_driver mp2888_driver = {
> > + .driver = {
> > + .name = "mp2888",
> > + .of_match_table = of_match_ptr(mp2888_of_match),
> > + },
> > + .probe_new = mp2888_probe,
> > + .id_table = mp2888_id,
> > +};
> > +
> > +module_i2c_driver(mp2888_driver);
> > +
> > +MODULE_AUTHOR("Vadim Pasternak <vadimp@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("PMBus driver for MPS MP2888 device");
> > +MODULE_LICENSE("GPL");
> >
