Messages in this thread | | | Date | Thu, 17 Feb 2022 10:11:55 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH v2 2/3] hwmon (xdpe12284): Add support for xdpe11280 |
| |
On 2/17/22 08:39, sylv wrote: > On Thu, 2022-02-17 at 07:20 -0800, Guenter Roeck wrote: >> On 2/17/22 06:41, Marcello Sylvester Bauer wrote: >>> Add support for another family of Infineon Multi-phase controllers. >>> The xdpe11280 uses linear instead of vid data format for VOUT. >>> Add the required logic to detect VOUT_MODE during identification, >>> and set the format accordingly. >>> >> This talks about the chip, but ... > > Oh, sure. I should be more clear that it is a chip of a different > family. > >> >>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >>> --- >>> Documentation/hwmon/xdpe12284.rst | 10 +++++++--- >>> drivers/hwmon/pmbus/xdpe12284.c | 17 ++++++++++++++++- >>> 2 files changed, 23 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/hwmon/xdpe12284.rst >>> b/Documentation/hwmon/xdpe12284.rst >>> index 67d1f87808e5..e9a5a19b0c3d 100644 >>> --- a/Documentation/hwmon/xdpe12284.rst >>> +++ b/Documentation/hwmon/xdpe12284.rst >>> @@ -5,6 +5,10 @@ Kernel driver xdpe122 >>> >>> Supported chips: >>> >>> + * Infineon XDPE11280 >>> + >>> + Prefix: 'xdpe11280' >>> + >>> * Infineon XDPE12254 >>> >>> Prefix: 'xdpe12254' >>> @@ -20,9 +24,9 @@ Authors: >>> Description >>> ----------- >>> >>> -This driver implements support for Infineon Multi-phase XDPE122 >>> family >>> -dual loop voltage regulators. >>> -The family includes XDPE12284 and XDPE12254 devices. >>> +This driver implements support for Infineon Multi-phase XDPE112 >>> and XDPE122 >>> +family dual loop voltage regulators. >>> +These families include XDPE11280, XDPE12284 and XDPE12254 devices. >>> The devices from this family complaint with: >> >> Hmm, s/complaint/compliant/. > > ack > >> >> Anyway, is it still correct that the XDPE11280 is compliant with the >> Intel specifications ? The datasheet isn't public, so I can not check >> myself >> what the chip actually supports. > > The hardware we are testing on is an Intel Cascade Lake, which uses > this chip. The datasheet tells us it is compliant: > > ``` > The XDPE11280B implements a fully compliant Intel® VR 13, VR12, VR > 12.5, VR13.HC, and IMVP8 Serial VID (SVID) > interface. > ``` >
This seems to be one of those chips where the vendor doesn't even admit that it exists unless one signs an NDA. Sigh :-(.
>> >>> >>> - Intel VR13 and VR13HC rev 1.3, IMVP8 rev 1.2 and IMPVP9 rev 1.3 >>> DC-DC >>> diff --git a/drivers/hwmon/pmbus/xdpe12284.c >>> b/drivers/hwmon/pmbus/xdpe12284.c >>> index b07da06a40c9..ec947c697670 100644 >>> --- a/drivers/hwmon/pmbus/xdpe12284.c >>> +++ b/drivers/hwmon/pmbus/xdpe12284.c >>> @@ -27,6 +27,9 @@ static int xdpe122_read_word_data(struct >>> i2c_client *client, int page, >>> s32 mantissa; >>> int ret; >>> >>> + if (info->format[PSC_VOLTAGE_OUT] == linear) >>> + return -ENODATA; >>> + >>> switch (reg) { >>> case PMBUS_VOUT_OV_FAULT_LIMIT: >>> case PMBUS_VOUT_UV_FAULT_LIMIT: >>> @@ -75,9 +78,19 @@ static int xdpe122_read_word_data(struct >>> i2c_client *client, int page, >>> static int xdpe122_identify(struct i2c_client *client, >>> struct pmbus_driver_info *info) >>> { >>> - u8 vout_params; >>> + u8 vout_mode, vout_params; >>> int i, ret; >>> >>> + ret = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE); >>> + if (ret < 0) >>> + return ret; >>> + >>> + vout_mode = ret >> 5; >>> + if (vout_mode == 0) { >>> + info->format[PSC_VOLTAGE_OUT] = linear; >>> + return 0; >>> + } >> >> .... the code above is not chip dependent but this code checks the >> mode register on page 0. At the very least this is confusing. >> >> What the driver should do instead is to check the chip type during >> probe and make the necessary changes there, ie set the output format >> for xdpe11280 to linear and drop the pointers to the .identify >> and .read_word_data. > > Sgtm. This sounds like a better approach indeed. >
That makes me wonder if the chip needs to be added to this driver in the first place, or if it could be added to pmbus.c instead. Any idea ?
Thanks, Guenter
> Thanks, > Marcello > >> >> Guenter >> >>> + >>> for (i = 0; i < XDPE122_PAGE_NUM; i++) { >>> /* Read the register with VOUT scaling value.*/ >>> ret = pmbus_read_byte_data(client, i, >>> PMBUS_VOUT_MODE); >>> @@ -140,6 +153,7 @@ static int xdpe122_probe(struct i2c_client >>> *client) >>> } >>> >>> static const struct i2c_device_id xdpe122_id[] = { >>> + {"xdpe11280", 0}, >>> {"xdpe12254", 0}, >>> {"xdpe12284", 0}, >>> {} >>> @@ -148,6 +162,7 @@ static const struct i2c_device_id xdpe122_id[] >>> = { >>> MODULE_DEVICE_TABLE(i2c, xdpe122_id); >>> >>> static const struct of_device_id __maybe_unused >>> xdpe122_of_match[] = { >>> + {.compatible = "infineon,xdpe11280"}, >>> {.compatible = "infineon,xdpe12254"}, >>> {.compatible = "infineon,xdpe12284"}, >>> {} >> >
| |