* [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975
@ 2022-12-09 2:45 Wang Xiaohua
2022-12-10 17:54 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Wang Xiaohua @ 2022-12-09 2:45 UTC (permalink / raw)
To: linux-kernel, linux, linux-hwmon, jdelvare, openbmc, joel
Add mp2971/mp2973 support in mp2975
Tested with:
My unit only include mp2971 and mp2973 devices
MP2973:
cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*label
iin
iout1
iout2
vin
vout1
vout2
pin
pout1
pout2
cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*input
0
82500
14000
12187
1787
1793
0
148000000
25000000
54000
MP2971:
cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*label
iin
iout1
iout2
vin
vout1
vout2
pin
pout1
pout2
cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*input
18500
22000
500
12187
1025
1800
226000000
22000000
1000000
55000
Signed-off-by: Wang Xiaohua <wangxiaohua.1217@bytedance.com>
v2:
- Fix auto build test WARNING
v3:
- Fix incorrect return code
---
drivers/hwmon/pmbus/mp2975.c | 415 +++++++++++++++++++++++++++++++----
1 file changed, 374 insertions(+), 41 deletions(-)
diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
index 51986adfbf47..4d1b7ac1800e 100644
--- a/drivers/hwmon/pmbus/mp2975.c
+++ b/drivers/hwmon/pmbus/mp2975.c
@@ -52,10 +52,33 @@
#define MP2975_MAX_PHASE_RAIL2 4
#define MP2975_PAGE_NUM 2
+#define MP2971_RAIL2_FUNC \
+ (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT | \
+ PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT)
+
#define MP2975_RAIL2_FUNC (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
PMBUS_HAVE_POUT | PMBUS_PHASE_VIRTUAL)
+struct mp2971_device_info {
+ int max_phase_rail1;
+ int max_phase_rail2;
+ int imvp9_en_r1_mask;
+ int imvp9_en_r2_mask;
+};
+
+struct mp2971_data {
+ struct pmbus_driver_info info;
+ int vid_step[MP2975_PAGE_NUM];
+ int vout_format[MP2975_PAGE_NUM];
+ int vout_mode[MP2975_PAGE_NUM];
+ int vout_exponent[MP2975_PAGE_NUM];
+ int max_phase_rail1;
+ int max_phase_rail2;
+ int imvp9_en_r1_mask;
+ int imvp9_en_r2_mask;
+};
+
struct mp2975_data {
struct pmbus_driver_info info;
int vout_scale;
@@ -68,6 +91,9 @@ struct mp2975_data {
int curr_sense_gain[MP2975_PAGE_NUM];
};
+static const struct i2c_device_id mp2975_id[];
+
+#define to_mp2971_data(x) container_of(x, struct mp2971_data, info)
#define to_mp2975_data(x) container_of(x, struct mp2975_data, info)
static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
@@ -95,6 +121,40 @@ mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
return (ret > 0) ? ret & mask : ret;
}
+static int
+mp2971_linear2direct(struct mp2971_data *data, int page, int val)
+{
+ /* simple case */
+ if (val == 0)
+ return 0;
+
+ /* LINEAR16 does not support negative voltages */
+ if (val < 0)
+ return 0;
+
+ /*
+ * For a static exponents, we don't have a choice
+ * but to adjust the value to it.
+ */
+
+ if (data->vout_exponent[page] < 0)
+ val <<= -data->vout_exponent[page];
+ else
+ val >>= data->vout_exponent[page];
+ return clamp_val(val, 0, 0xffff);
+}
+
+static int
+mp2971_vid2direct(struct mp2971_data *data, int page, int val)
+{
+ int vrf = data->info.vrm_version[page];
+
+ if (vrf == imvp9)
+ return (val + 29) * data->vid_step[page];
+
+ return (val + 49) * data->vid_step[page];
+}
+
static int
mp2975_vid2direct(int vrf, int val)
{
@@ -214,6 +274,74 @@ mp2975_read_phases(struct i2c_client *client, struct mp2975_data *data,
return ret;
}
+static int
+mp2971_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 mp2971_data *data = to_mp2971_data(info);
+ int ret;
+
+ switch (reg) {
+ case PMBUS_OT_FAULT_LIMIT:
+ case PMBUS_VIN_OV_FAULT_LIMIT:
+ case PMBUS_VOUT_OV_FAULT_LIMIT:
+ case PMBUS_VOUT_UV_FAULT_LIMIT:
+ case PMBUS_READ_IOUT:
+ ret = mp2975_read_word_helper(client, page, phase,
+ reg, GENMASK(15, 0));
+ break;
+ case PMBUS_READ_VOUT:
+ ret = mp2975_read_word_helper(client, page, phase, reg,
+ GENMASK(11, 0));
+ if (ret < 0)
+ return ret;
+ /*
+ * READ_VOUT can be provided in VID or direct format. The
+ * format type is specified by bit 15 of the register
+ * MP2971_MFR_DC_LOOP_CTRL. The driver enforces VOUT direct
+ * format, since device allows to set the different formats for
+ * the different rails and also all VOUT limits registers are
+ * provided in a direct format. In case format is VID - convert
+ * to direct.
+ */
+ switch (data->vout_format[page]) {
+ case linear:
+ ret = mp2971_linear2direct(data, page, ret);
+ break;
+ case vid:
+ ret = mp2971_vid2direct(data, page, ret);
+ break;
+ case direct:
+ break;
+ default:
+ return -ENODATA;
+ }
+ break;
+ case PMBUS_UT_WARN_LIMIT:
+ case PMBUS_UT_FAULT_LIMIT:
+ case PMBUS_VIN_UV_WARN_LIMIT:
+ case PMBUS_VIN_UV_FAULT_LIMIT:
+ case PMBUS_VOUT_UV_WARN_LIMIT:
+ case PMBUS_VOUT_OV_WARN_LIMIT:
+ case PMBUS_VIN_OV_WARN_LIMIT:
+ case PMBUS_IIN_OC_FAULT_LIMIT:
+ case PMBUS_IOUT_OC_LV_FAULT_LIMIT:
+ case PMBUS_IIN_OC_WARN_LIMIT:
+ case PMBUS_IOUT_OC_WARN_LIMIT:
+ case PMBUS_IOUT_OC_FAULT_LIMIT:
+ case PMBUS_IOUT_UC_FAULT_LIMIT:
+ case PMBUS_POUT_OP_FAULT_LIMIT:
+ case PMBUS_POUT_OP_WARN_LIMIT:
+ case PMBUS_PIN_OP_WARN_LIMIT:
+ return -ENXIO;
+ default:
+ return -ENODATA;
+ }
+
+ return ret;
+}
+
static int mp2975_read_word_data(struct i2c_client *client, int page,
int phase, int reg)
{
@@ -365,6 +493,63 @@ mp2975_set_phase_rail2(struct pmbus_driver_info *info, int num_phases)
info->pfunc[MP2975_MAX_PHASE_RAIL1 - i] = PMBUS_HAVE_IOUT;
}
+static int mp2971_identify_multiphase(struct i2c_client *client,
+ struct mp2971_data *data,
+ struct pmbus_driver_info *info)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
+ if (ret < 0)
+ return ret;
+
+ /* Identify multiphase for rail 1 - could be from 1 to 12. */
+ ret = i2c_smbus_read_word_data(client, MP2975_MFR_VR_MULTI_CONFIG_R1);
+ if (ret <= 0)
+ return ret;
+
+ info->phases[0] = ret & GENMASK(3, 0);
+
+ /*
+ * The device provides a total of 8 PWM pins, and can be configured
+ * to different phase count applications for rail 1 and rail 2.
+ * Rail 1 can be set to 8 phases, while rail 2 can only be set to 4
+ * phases at most. When rail 1’s phase count is configured as 0, rail
+ * 1 operates with 1-phase DCM. When rail 2 phase count is configured
+ * as 0, rail 2 is disabled.
+ */
+ if (info->phases[0] > data->max_phase_rail1)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+mp2971_identify_vid(struct i2c_client *client, struct mp2971_data *data,
+ struct pmbus_driver_info *info, u32 reg, int page,
+ u32 imvp_bit, u32 vr_bit)
+{
+ int ret;
+
+ /* Identify VID mode and step selection. */
+ ret = i2c_smbus_read_word_data(client, reg);
+ if (ret < 0)
+ return ret;
+
+ if (ret & imvp_bit) {
+ info->vrm_version[page] = imvp9;
+ data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
+ } else if (ret & vr_bit) {
+ info->vrm_version[page] = vr12;
+ data->vid_step[page] = MP2975_PROT_DEV_OV_ON;
+ } else {
+ info->vrm_version[page] = vr13;
+ data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
+ }
+
+ return 0;
+}
+
static int
mp2975_identify_multiphase(struct i2c_client *client, struct mp2975_data *data,
struct pmbus_driver_info *info)
@@ -428,6 +613,68 @@ mp2975_identify_vid(struct i2c_client *client, struct mp2975_data *data,
return 0;
}
+static int
+mp2971_identify_rails_vid(struct i2c_client *client, struct mp2971_data *data,
+ struct pmbus_driver_info *info)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
+ if (ret < 0)
+ return ret;
+
+ /* Identify VID mode for rail 1. */
+ ret = mp2971_identify_vid(client, data, info,
+ MP2975_MFR_VR_MULTI_CONFIG_R1, 0,
+ data->imvp9_en_r1_mask,
+ MP2975_VID_STEP_SEL_R1);
+ if (ret < 0)
+ return ret;
+
+ /* Identify VID mode for rail 2, if connected. */
+ if (info->phases[1])
+ ret = mp2971_identify_vid(client, data, info,
+ MP2975_MFR_VR_MULTI_CONFIG_R2, 1,
+ data->imvp9_en_r2_mask,
+ MP2975_VID_STEP_SEL_R2);
+ return ret;
+}
+
+static int mp2971_identify_vout_format(struct i2c_client *client,
+ struct mp2971_data *data,
+ struct pmbus_driver_info *info)
+{
+ int i, ret, vout_mode;
+
+ for (i = 0; i < info->pages; i++) {
+ ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
+ if (ret < 0)
+ return ret;
+
+ vout_mode = ret;
+
+ switch (vout_mode >> 5) {
+ case 0:
+ data->vout_format[i] = linear;
+ data->vout_exponent[i] = ((s8)(vout_mode << 3)) >> 3;
+ break;
+ case 1:
+ data->vout_format[i] = vid;
+ break;
+ case 2:
+ data->vout_format[i] = direct;
+ break;
+ default:
+ return -ENODEV;
+ }
+ }
+ return 0;
+}
+
static int
mp2975_identify_rails_vid(struct i2c_client *client, struct mp2975_data *data,
struct pmbus_driver_info *info)
@@ -659,6 +906,24 @@ mp2975_vout_per_rail_config_get(struct i2c_client *client,
return 0;
}
+static struct pmbus_driver_info mp2971_info = {
+ .pages = 1,
+ .format[PSC_VOLTAGE_IN] = linear,
+ .format[PSC_VOLTAGE_OUT] = direct,
+ .format[PSC_TEMPERATURE] = linear,
+ .format[PSC_CURRENT_IN] = linear,
+ .format[PSC_CURRENT_OUT] = linear,
+ .format[PSC_POWER] = linear,
+ .m[PSC_VOLTAGE_OUT] = 1,
+ .R[PSC_VOLTAGE_OUT] = 3,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
+ PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
+ .read_byte_data = mp2975_read_byte_data,
+ .read_word_data = mp2971_read_word_data,
+};
+
static struct pmbus_driver_info mp2975_info = {
.pages = 1,
.format[PSC_VOLTAGE_IN] = linear,
@@ -683,63 +948,131 @@ static struct pmbus_driver_info mp2975_info = {
static int mp2975_probe(struct i2c_client *client)
{
struct pmbus_driver_info *info;
- struct mp2975_data *data;
int ret;
+ char *name;
- data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
- GFP_KERNEL);
- if (!data)
- return -ENOMEM;
+ name = (char *)i2c_match_id(mp2975_id, client)->name;
- memcpy(&data->info, &mp2975_info, sizeof(*info));
- info = &data->info;
+ if (!name)
+ return -EINVAL;
- /* Identify multiphase configuration for rail 2. */
- ret = mp2975_identify_multiphase_rail2(client);
- if (ret < 0)
- return ret;
+ if (!strcmp(name, "mp2971") || !strcmp(name, "mp2973")) {
+ struct mp2971_data *data;
+ struct mp2971_device_info *device_info;
- if (ret) {
- /* Two rails are connected. */
- data->info.pages = MP2975_PAGE_NUM;
- data->info.phases[1] = ret;
- data->info.func[1] = MP2975_RAIL2_FUNC;
- }
+ data = devm_kzalloc(&client->dev, sizeof(struct mp2971_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
- /* Identify multiphase configuration. */
- ret = mp2975_identify_multiphase(client, data, info);
- if (ret)
- return ret;
+ device_info =
+ (struct mp2971_device_info *)i2c_match_id(mp2975_id, client)
+ ->driver_data;
- /* Identify VID setting per rail. */
- ret = mp2975_identify_rails_vid(client, data, info);
- if (ret < 0)
- return ret;
+ memcpy(&data->info, &mp2971_info, sizeof(*info));
+ info = &data->info;
- /* Obtain current sense gain of power stage. */
- ret = mp2975_current_sense_gain_get(client, data);
- if (ret)
- return ret;
+ if (device_info) {
+ data->imvp9_en_r1_mask = device_info->imvp9_en_r1_mask;
+ data->imvp9_en_r2_mask = device_info->imvp9_en_r2_mask;
+ data->max_phase_rail1 = device_info->max_phase_rail1;
+ data->max_phase_rail2 = device_info->max_phase_rail2;
+ }
- /* Obtain voltage reference values. */
- ret = mp2975_vref_get(client, data, info);
- if (ret)
- return ret;
+ /* Identify multiphase configuration for rail 2. */
+ ret = mp2975_identify_multiphase_rail2(client);
+ if (ret < 0)
+ return ret;
- /* Obtain vout over-voltage scales. */
- ret = mp2975_vout_ov_scale_get(client, data, info);
- if (ret < 0)
- return ret;
+ if (ret) {
+ /* Two rails are connected. */
+ data->info.pages = MP2975_PAGE_NUM;
+ data->info.phases[1] = ret;
+ data->info.func[1] = MP2971_RAIL2_FUNC;
+ }
- /* Obtain offsets, maximum and format for vout. */
- ret = mp2975_vout_per_rail_config_get(client, data, info);
- if (ret)
- return ret;
+ /* Identify multiphase configuration. */
+ ret = mp2971_identify_multiphase(client, data, info);
+ if (ret)
+ return ret;
+
+ /* Identify VID setting per rail. */
+ ret = mp2971_identify_rails_vid(client, data, info);
+ if (ret < 0)
+ return ret;
+
+ /* Identify vout format. */
+ ret = mp2971_identify_vout_format(client, data, info);
+ if (ret < 0)
+ return ret;
+
+ } else {
+ struct mp2975_data *data;
+
+ data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
+ GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ memcpy(&data->info, &mp2975_info, sizeof(*info));
+ info = &data->info;
+
+ /* Identify multiphase configuration for rail 2. */
+ ret = mp2975_identify_multiphase_rail2(client);
+ if (ret < 0)
+ return ret;
+
+ if (ret) {
+ /* Two rails are connected. */
+ data->info.pages = MP2975_PAGE_NUM;
+ data->info.phases[1] = ret;
+ data->info.func[1] = MP2975_RAIL2_FUNC;
+ }
+
+ /* Identify multiphase configuration. */
+ ret = mp2975_identify_multiphase(client, data, info);
+ if (ret)
+ return ret;
+
+ /* Identify VID setting per rail. */
+ ret = mp2975_identify_rails_vid(client, data, info);
+ if (ret < 0)
+ return ret;
+
+ /* Obtain current sense gain of power stage. */
+ ret = mp2975_current_sense_gain_get(client, data);
+ if (ret)
+ return ret;
+
+ /* Obtain voltage reference values. */
+ ret = mp2975_vref_get(client, data, info);
+ if (ret)
+ return ret;
+
+ /* Obtain vout over-voltage scales. */
+ ret = mp2975_vout_ov_scale_get(client, data, info);
+ if (ret < 0)
+ return ret;
+
+ /* Obtain offsets, maximum and format for vout. */
+ ret = mp2975_vout_per_rail_config_get(client, data, info);
+ if (ret)
+ return ret;
+ }
return pmbus_do_probe(client, info);
}
+static const struct mp2971_device_info mp2971_device_info = {
+ .imvp9_en_r1_mask = BIT(14),
+ .imvp9_en_r2_mask = BIT(13),
+ .max_phase_rail1 = 8,
+ .max_phase_rail2 = 4,
+};
+
static const struct i2c_device_id mp2975_id[] = {
+ {"mp2971", (kernel_ulong_t)&mp2971_device_info },
+ {"mp2973", (kernel_ulong_t)&mp2971_device_info },
{"mp2975", 0},
{}
};
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975
2022-12-09 2:45 [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975 Wang Xiaohua
@ 2022-12-10 17:54 ` Guenter Roeck
2023-01-18 3:55 ` Joel Stanley
2023-01-18 5:42 ` [External] " 晓华王
0 siblings, 2 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-12-10 17:54 UTC (permalink / raw)
To: Wang Xiaohua, linux-kernel, linux-hwmon, jdelvare, openbmc, joel
On 12/8/22 18:45, Wang Xiaohua wrote:
> Add mp2971/mp2973 support in mp2975
>
> Tested with:
> My unit only include mp2971 and mp2973 devices
> MP2973:
> cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*label
> iin
> iout1
> iout2
> vin
> vout1
> vout2
> pin
> pout1
> pout2
> cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*input
> 0
> 82500
> 14000
> 12187
> 1787
> 1793
> 0
> 148000000
> 25000000
> 54000
> MP2971:
> cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*label
> iin
> iout1
> iout2
> vin
> vout1
> vout2
> pin
> pout1
> pout2
> cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*input
> 18500
> 22000
> 500
> 12187
> 1025
> 1800
> 226000000
> 22000000
> 1000000
> 55000
>
Test results are not very useful. Better use something like
"grep . /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*".
Either case, test results should be provided after "---" and not be part
of the commit description. Instead, the commit description should explain
what those chips actually are.
> Signed-off-by: Wang Xiaohua <wangxiaohua.1217@bytedance.com>
>
> v2:
> - Fix auto build test WARNING
>
> v3:
> - Fix incorrect return code
> ---
> drivers/hwmon/pmbus/mp2975.c | 415 +++++++++++++++++++++++++++++++----
> 1 file changed, 374 insertions(+), 41 deletions(-)
>
Update to Documentation/hwmon/mp2975.rst and
Documentation/devicetree/bindings/trivial-devices.yaml required.
However, there is a more severe problem: The changes are too complex
for me to review, and the chip datasheets are not published. I can not evaluate
if the changes are really needed, if there is a less complex solution,
or if they even make sense. Someone with access to a datasheet will have
to step up as maintainer for this driver.
Additional comments inline.
Guenter
> diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
> index 51986adfbf47..4d1b7ac1800e 100644
> --- a/drivers/hwmon/pmbus/mp2975.c
> +++ b/drivers/hwmon/pmbus/mp2975.c
> @@ -52,10 +52,33 @@
> #define MP2975_MAX_PHASE_RAIL2 4
> #define MP2975_PAGE_NUM 2
>
> +#define MP2971_RAIL2_FUNC \
> + (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT | \
> + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT)
> +
> #define MP2975_RAIL2_FUNC (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
> PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
> PMBUS_HAVE_POUT | PMBUS_PHASE_VIRTUAL)
>
> +struct mp2971_device_info {
> + int max_phase_rail1;
> + int max_phase_rail2;
> + int imvp9_en_r1_mask;
> + int imvp9_en_r2_mask;
> +};
> +
> +struct mp2971_data {
> + struct pmbus_driver_info info;
> + int vid_step[MP2975_PAGE_NUM];
> + int vout_format[MP2975_PAGE_NUM];
> + int vout_mode[MP2975_PAGE_NUM];
> + int vout_exponent[MP2975_PAGE_NUM];
> + int max_phase_rail1;
> + int max_phase_rail2;
> + int imvp9_en_r1_mask;
> + int imvp9_en_r2_mask;
> +};
> +
> struct mp2975_data {
> struct pmbus_driver_info info;
> int vout_scale;
> @@ -68,6 +91,9 @@ struct mp2975_data {
> int curr_sense_gain[MP2975_PAGE_NUM];
> };
>
> +static const struct i2c_device_id mp2975_id[];
> +
> +#define to_mp2971_data(x) container_of(x, struct mp2971_data, info)
> #define to_mp2975_data(x) container_of(x, struct mp2975_data, info)
>
> static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
> @@ -95,6 +121,40 @@ mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
> return (ret > 0) ? ret & mask : ret;
> }
>
> +static int
> +mp2971_linear2direct(struct mp2971_data *data, int page, int val)
> +{
> + /* simple case */
> + if (val == 0)
> + return 0;
> +
> + /* LINEAR16 does not support negative voltages */
> + if (val < 0)
> + return 0;
> +
> + /*
> + * For a static exponents, we don't have a choice
> + * but to adjust the value to it.
> + */
> +
> + if (data->vout_exponent[page] < 0)
> + val <<= -data->vout_exponent[page];
> + else
> + val >>= data->vout_exponent[page];
> + return clamp_val(val, 0, 0xffff);
> +}
> +
> +static int
> +mp2971_vid2direct(struct mp2971_data *data, int page, int val)
> +{
> + int vrf = data->info.vrm_version[page];
> +
> + if (vrf == imvp9)
> + return (val + 29) * data->vid_step[page];
> +
> + return (val + 49) * data->vid_step[page];
> +}
> +
This looks suspicious. VID -> voltage calculations should be well
defined and be implemented in mp2975_vid2direct(). It is not entirely
clear why a second conversion function should be needed, and why it would
use different calculations with different results than those for
mp2975.
Example, for vrf == imvp9, 10mV step size, and vid==1:
mp2971: (1 + 29) * 10 = 30 * 10 = 300
mp2975: 200 + (1 - 1) * 10 = 200 + 0 = 200
vid = 0xff = 255:
mp2971: (255 + 29) * 10 = 284 * 10 = 2840
mp2975: 200 + (255 - 1) * 10 = 200 + 254 * 10 = 2740
Also questionable is how there could ever be an IMVP9 setting with 5mV
step size since IMVP9 explicitly specifies a step size of 10mV.
Also, the maximum voltage for IMVP9 is specified as 2.74V.
> static int
> mp2975_vid2direct(int vrf, int val)
> {
> @@ -214,6 +274,74 @@ mp2975_read_phases(struct i2c_client *client, struct mp2975_data *data,
> return ret;
> }
>
> +static int
> +mp2971_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 mp2971_data *data = to_mp2971_data(info);
> + int ret;
> +
> + switch (reg) {
> + case PMBUS_OT_FAULT_LIMIT:
> + case PMBUS_VIN_OV_FAULT_LIMIT:
> + case PMBUS_VOUT_OV_FAULT_LIMIT:
> + case PMBUS_VOUT_UV_FAULT_LIMIT:
> + case PMBUS_READ_IOUT:
> + ret = mp2975_read_word_helper(client, page, phase,
> + reg, GENMASK(15, 0));
> + break;
> + case PMBUS_READ_VOUT:
> + ret = mp2975_read_word_helper(client, page, phase, reg,
> + GENMASK(11, 0));
> + if (ret < 0)
> + return ret;
> + /*
> + * READ_VOUT can be provided in VID or direct format. The
> + * format type is specified by bit 15 of the register
> + * MP2971_MFR_DC_LOOP_CTRL. The driver enforces VOUT direct
> + * format, since device allows to set the different formats for
> + * the different rails and also all VOUT limits registers are
> + * provided in a direct format. In case format is VID - convert
> + * to direct.
> + */
> + switch (data->vout_format[page]) {
> + case linear:
> + ret = mp2971_linear2direct(data, page, ret);
> + break;
> + case vid:
> + ret = mp2971_vid2direct(data, page, ret);
> + break;
> + case direct:
> + break;
> + default:
> + return -ENODATA;
> + }
> + break;
> + case PMBUS_UT_WARN_LIMIT:
> + case PMBUS_UT_FAULT_LIMIT:
> + case PMBUS_VIN_UV_WARN_LIMIT:
> + case PMBUS_VIN_UV_FAULT_LIMIT:
> + case PMBUS_VOUT_UV_WARN_LIMIT:
> + case PMBUS_VOUT_OV_WARN_LIMIT:
> + case PMBUS_VIN_OV_WARN_LIMIT:
> + case PMBUS_IIN_OC_FAULT_LIMIT:
> + case PMBUS_IOUT_OC_LV_FAULT_LIMIT:
> + case PMBUS_IIN_OC_WARN_LIMIT:
> + case PMBUS_IOUT_OC_WARN_LIMIT:
> + case PMBUS_IOUT_OC_FAULT_LIMIT:
> + case PMBUS_IOUT_UC_FAULT_LIMIT:
> + case PMBUS_POUT_OP_FAULT_LIMIT:
> + case PMBUS_POUT_OP_WARN_LIMIT:
> + case PMBUS_PIN_OP_WARN_LIMIT:
> + return -ENXIO;
> + default:
> + return -ENODATA;
> + }
> +
> + return ret;
> +}
> +
Much of that code seems duplicate from mp2975_read_word_data().
Without datasheets I can not determine if this really makes sense
and/or is needed, or if a single function can be used for all chips.
> static int mp2975_read_word_data(struct i2c_client *client, int page,
> int phase, int reg)
> {
> @@ -365,6 +493,63 @@ mp2975_set_phase_rail2(struct pmbus_driver_info *info, int num_phases)
> info->pfunc[MP2975_MAX_PHASE_RAIL1 - i] = PMBUS_HAVE_IOUT;
> }
>
> +static int mp2971_identify_multiphase(struct i2c_client *client,
> + struct mp2971_data *data,
> + struct pmbus_driver_info *info)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> + if (ret < 0)
> + return ret;
> +
> + /* Identify multiphase for rail 1 - could be from 1 to 12. */
> + ret = i2c_smbus_read_word_data(client, MP2975_MFR_VR_MULTI_CONFIG_R1);
> + if (ret <= 0)
> + return ret;
> +
> + info->phases[0] = ret & GENMASK(3, 0);
> +
> + /*
> + * The device provides a total of 8 PWM pins, and can be configured
> + * to different phase count applications for rail 1 and rail 2.
> + * Rail 1 can be set to 8 phases, while rail 2 can only be set to 4
> + * phases at most. When rail 1’s phase count is configured as 0, rail
> + * 1 operates with 1-phase DCM. When rail 2 phase count is configured
> + * as 0, rail 2 is disabled.
> + */
> + if (info->phases[0] > data->max_phase_rail1)
> + return -EINVAL;
> +
> + return 0;
> +}
Same here. The code is almost the same as mp2975_identify_multiphase().
Again, without datasheets I can not determine if this really makes sense
and/or is needed, or if a single function can be used for all chips.
This is a recurring problem. It appears that the patch maximizes the
changes against the current code instead of even trying to minimize them.
Without datasheet, it is impossible to compare the chips to check if an
implementation with fewer / less extensive changes would be warranted.
> +
> +static int
> +mp2971_identify_vid(struct i2c_client *client, struct mp2971_data *data,
> + struct pmbus_driver_info *info, u32 reg, int page,
> + u32 imvp_bit, u32 vr_bit)
> +{
> + int ret;
> +
> + /* Identify VID mode and step selection. */
> + ret = i2c_smbus_read_word_data(client, reg);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & imvp_bit) {
> + info->vrm_version[page] = imvp9;
> + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> + } else if (ret & vr_bit) {
> + info->vrm_version[page] = vr12;
> + data->vid_step[page] = MP2975_PROT_DEV_OV_ON;
> + } else {
> + info->vrm_version[page] = vr13;
> + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> + }
> +
> + return 0;
> +}
> +
> static int
> mp2975_identify_multiphase(struct i2c_client *client, struct mp2975_data *data,
> struct pmbus_driver_info *info)
> @@ -428,6 +613,68 @@ mp2975_identify_vid(struct i2c_client *client, struct mp2975_data *data,
> return 0;
> }
>
> +static int
> +mp2971_identify_rails_vid(struct i2c_client *client, struct mp2971_data *data,
> + struct pmbus_driver_info *info)
> +{
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> + if (ret < 0)
> + return ret;
> +
> + /* Identify VID mode for rail 1. */
> + ret = mp2971_identify_vid(client, data, info,
> + MP2975_MFR_VR_MULTI_CONFIG_R1, 0,
> + data->imvp9_en_r1_mask,
> + MP2975_VID_STEP_SEL_R1);
> + if (ret < 0)
> + return ret;
> +
> + /* Identify VID mode for rail 2, if connected. */
> + if (info->phases[1])
> + ret = mp2971_identify_vid(client, data, info,
> + MP2975_MFR_VR_MULTI_CONFIG_R2, 1,
> + data->imvp9_en_r2_mask,
> + MP2975_VID_STEP_SEL_R2);
> + return ret;
> +}
> +
> +static int mp2971_identify_vout_format(struct i2c_client *client,
> + struct mp2971_data *data,
> + struct pmbus_driver_info *info)
> +{
> + int i, ret, vout_mode;
> +
> + for (i = 0; i < info->pages; i++) {
> + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
> + if (ret < 0)
> + return ret;
> +
> + vout_mode = ret;
> +
> + switch (vout_mode >> 5) {
> + case 0:
> + data->vout_format[i] = linear;
> + data->vout_exponent[i] = ((s8)(vout_mode << 3)) >> 3;
> + break;
> + case 1:
> + data->vout_format[i] = vid;
> + break;
> + case 2:
> + data->vout_format[i] = direct;
> + break;
> + default:
> + return -ENODEV;
> + }
> + }
> + return 0;
> +}
> +
> static int
> mp2975_identify_rails_vid(struct i2c_client *client, struct mp2975_data *data,
> struct pmbus_driver_info *info)
> @@ -659,6 +906,24 @@ mp2975_vout_per_rail_config_get(struct i2c_client *client,
> return 0;
> }
>
> +static struct pmbus_driver_info mp2971_info = {
> + .pages = 1,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = direct,
> + .format[PSC_TEMPERATURE] = linear,
> + .format[PSC_CURRENT_IN] = linear,
> + .format[PSC_CURRENT_OUT] = linear,
> + .format[PSC_POWER] = linear,
> + .m[PSC_VOLTAGE_OUT] = 1,
> + .R[PSC_VOLTAGE_OUT] = 3,
> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> + PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
> + PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
> + .read_byte_data = mp2975_read_byte_data,
> + .read_word_data = mp2971_read_word_data,
> +};
> +
> static struct pmbus_driver_info mp2975_info = {
> .pages = 1,
> .format[PSC_VOLTAGE_IN] = linear,
> @@ -683,63 +948,131 @@ static struct pmbus_driver_info mp2975_info = {
> static int mp2975_probe(struct i2c_client *client)
> {
> struct pmbus_driver_info *info;
> - struct mp2975_data *data;
> int ret;
> + char *name;
>
> - data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> - GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> + name = (char *)i2c_match_id(mp2975_id, client)->name;
>
> - memcpy(&data->info, &mp2975_info, sizeof(*info));
> - info = &data->info;
> + if (!name)
> + return -EINVAL;
>
> - /* Identify multiphase configuration for rail 2. */
> - ret = mp2975_identify_multiphase_rail2(client);
> - if (ret < 0)
> - return ret;
> + if (!strcmp(name, "mp2971") || !strcmp(name, "mp2973")) {
> + struct mp2971_data *data;
> + struct mp2971_device_info *device_info;
>
> - if (ret) {
> - /* Two rails are connected. */
> - data->info.pages = MP2975_PAGE_NUM;
> - data->info.phases[1] = ret;
> - data->info.func[1] = MP2975_RAIL2_FUNC;
> - }
> + data = devm_kzalloc(&client->dev, sizeof(struct mp2971_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
>
> - /* Identify multiphase configuration. */
> - ret = mp2975_identify_multiphase(client, data, info);
> - if (ret)
> - return ret;
> + device_info =
> + (struct mp2971_device_info *)i2c_match_id(mp2975_id, client)
> + ->driver_data;
>
> - /* Identify VID setting per rail. */
> - ret = mp2975_identify_rails_vid(client, data, info);
> - if (ret < 0)
> - return ret;
> + memcpy(&data->info, &mp2971_info, sizeof(*info));
> + info = &data->info;
>
> - /* Obtain current sense gain of power stage. */
> - ret = mp2975_current_sense_gain_get(client, data);
> - if (ret)
> - return ret;
> + if (device_info) {
> + data->imvp9_en_r1_mask = device_info->imvp9_en_r1_mask;
> + data->imvp9_en_r2_mask = device_info->imvp9_en_r2_mask;
> + data->max_phase_rail1 = device_info->max_phase_rail1;
> + data->max_phase_rail2 = device_info->max_phase_rail2;
> + }
>
> - /* Obtain voltage reference values. */
> - ret = mp2975_vref_get(client, data, info);
> - if (ret)
> - return ret;
> + /* Identify multiphase configuration for rail 2. */
> + ret = mp2975_identify_multiphase_rail2(client);
> + if (ret < 0)
> + return ret;
>
> - /* Obtain vout over-voltage scales. */
> - ret = mp2975_vout_ov_scale_get(client, data, info);
> - if (ret < 0)
> - return ret;
> + if (ret) {
> + /* Two rails are connected. */
> + data->info.pages = MP2975_PAGE_NUM;
> + data->info.phases[1] = ret;
> + data->info.func[1] = MP2971_RAIL2_FUNC;
> + }
>
> - /* Obtain offsets, maximum and format for vout. */
> - ret = mp2975_vout_per_rail_config_get(client, data, info);
> - if (ret)
> - return ret;
> + /* Identify multiphase configuration. */
> + ret = mp2971_identify_multiphase(client, data, info);
> + if (ret)
> + return ret;
> +
> + /* Identify VID setting per rail. */
> + ret = mp2971_identify_rails_vid(client, data, info);
> + if (ret < 0)
> + return ret;
> +
> + /* Identify vout format. */
> + ret = mp2971_identify_vout_format(client, data, info);
> + if (ret < 0)
> + return ret;
> +
> + } else {
> + struct mp2975_data *data;
> +
> + data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + memcpy(&data->info, &mp2975_info, sizeof(*info));
> + info = &data->info;
> +
> + /* Identify multiphase configuration for rail 2. */
> + ret = mp2975_identify_multiphase_rail2(client);
> + if (ret < 0)
> + return ret;
> +
> + if (ret) {
> + /* Two rails are connected. */
> + data->info.pages = MP2975_PAGE_NUM;
> + data->info.phases[1] = ret;
> + data->info.func[1] = MP2975_RAIL2_FUNC;
> + }
> +
> + /* Identify multiphase configuration. */
> + ret = mp2975_identify_multiphase(client, data, info);
> + if (ret)
> + return ret;
> +
> + /* Identify VID setting per rail. */
> + ret = mp2975_identify_rails_vid(client, data, info);
> + if (ret < 0)
> + return ret;
> +
> + /* Obtain current sense gain of power stage. */
> + ret = mp2975_current_sense_gain_get(client, data);
> + if (ret)
> + return ret;
> +
> + /* Obtain voltage reference values. */
> + ret = mp2975_vref_get(client, data, info);
> + if (ret)
> + return ret;
> +
> + /* Obtain vout over-voltage scales. */
> + ret = mp2975_vout_ov_scale_get(client, data, info);
> + if (ret < 0)
> + return ret;
> +
> + /* Obtain offsets, maximum and format for vout. */
> + ret = mp2975_vout_per_rail_config_get(client, data, info);
> + if (ret)
> + return ret;
> + }
>
> return pmbus_do_probe(client, info);
> }
>
> +static const struct mp2971_device_info mp2971_device_info = {
> + .imvp9_en_r1_mask = BIT(14),
> + .imvp9_en_r2_mask = BIT(13),
> + .max_phase_rail1 = 8,
> + .max_phase_rail2 = 4,
> +};
> +
> static const struct i2c_device_id mp2975_id[] = {
> + {"mp2971", (kernel_ulong_t)&mp2971_device_info },
> + {"mp2973", (kernel_ulong_t)&mp2971_device_info },
> {"mp2975", 0},
> {}
> };
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975
2022-12-10 17:54 ` Guenter Roeck
@ 2023-01-18 3:55 ` Joel Stanley
2023-01-18 5:42 ` [External] " 晓华王
1 sibling, 0 replies; 4+ messages in thread
From: Joel Stanley @ 2023-01-18 3:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wang Xiaohua, linux-kernel, linux-hwmon, jdelvare, openbmc
Hello Xiaohua,
On Sat, 10 Dec 2022 at 17:54, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 12/8/22 18:45, Wang Xiaohua wrote:
> > Add mp2971/mp2973 support in mp2975
Guenter has some comments for you to address. Are you planning on
working on this further?
I would like to help you get support for this device in the tree, but
it will require some more work.
Cheers,
Joel
> >
> > Tested with:
> > My unit only include mp2971 and mp2973 devices
> > MP2973:
> > cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*label
> > iin
> > iout1
> > iout2
> > vin
> > vout1
> > vout2
> > pin
> > pout1
> > pout2
> > cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*input
> > 0
> > 82500
> > 14000
> > 12187
> > 1787
> > 1793
> > 0
> > 148000000
> > 25000000
> > 54000
> > MP2971:
> > cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*label
> > iin
> > iout1
> > iout2
> > vin
> > vout1
> > vout2
> > pin
> > pout1
> > pout2
> > cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*input
> > 18500
> > 22000
> > 500
> > 12187
> > 1025
> > 1800
> > 226000000
> > 22000000
> > 1000000
> > 55000
> >
>
> Test results are not very useful. Better use something like
> "grep . /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*".
>
> Either case, test results should be provided after "---" and not be part
> of the commit description. Instead, the commit description should explain
> what those chips actually are.
>
>
> > Signed-off-by: Wang Xiaohua <wangxiaohua.1217@bytedance.com>
> >
> > v2:
> > - Fix auto build test WARNING
> >
> > v3:
> > - Fix incorrect return code
> > ---
> > drivers/hwmon/pmbus/mp2975.c | 415 +++++++++++++++++++++++++++++++----
> > 1 file changed, 374 insertions(+), 41 deletions(-)
> >
>
> Update to Documentation/hwmon/mp2975.rst and
> Documentation/devicetree/bindings/trivial-devices.yaml required.
>
> However, there is a more severe problem: The changes are too complex
> for me to review, and the chip datasheets are not published. I can not evaluate
> if the changes are really needed, if there is a less complex solution,
> or if they even make sense. Someone with access to a datasheet will have
> to step up as maintainer for this driver.
>
> Additional comments inline.
>
> Guenter
>
> > diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
> > index 51986adfbf47..4d1b7ac1800e 100644
> > --- a/drivers/hwmon/pmbus/mp2975.c
> > +++ b/drivers/hwmon/pmbus/mp2975.c
> > @@ -52,10 +52,33 @@
> > #define MP2975_MAX_PHASE_RAIL2 4
> > #define MP2975_PAGE_NUM 2
> >
> > +#define MP2971_RAIL2_FUNC \
> > + (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT | \
> > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT)
> > +
> > #define MP2975_RAIL2_FUNC (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
> > PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
> > PMBUS_HAVE_POUT | PMBUS_PHASE_VIRTUAL)
> >
> > +struct mp2971_device_info {
> > + int max_phase_rail1;
> > + int max_phase_rail2;
> > + int imvp9_en_r1_mask;
> > + int imvp9_en_r2_mask;
> > +};
> > +
> > +struct mp2971_data {
> > + struct pmbus_driver_info info;
> > + int vid_step[MP2975_PAGE_NUM];
> > + int vout_format[MP2975_PAGE_NUM];
> > + int vout_mode[MP2975_PAGE_NUM];
> > + int vout_exponent[MP2975_PAGE_NUM];
> > + int max_phase_rail1;
> > + int max_phase_rail2;
> > + int imvp9_en_r1_mask;
> > + int imvp9_en_r2_mask;
> > +};
> > +
> > struct mp2975_data {
> > struct pmbus_driver_info info;
> > int vout_scale;
> > @@ -68,6 +91,9 @@ struct mp2975_data {
> > int curr_sense_gain[MP2975_PAGE_NUM];
> > };
> >
> > +static const struct i2c_device_id mp2975_id[];
> > +
> > +#define to_mp2971_data(x) container_of(x, struct mp2971_data, info)
> > #define to_mp2975_data(x) container_of(x, struct mp2975_data, info)
> >
> > static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
> > @@ -95,6 +121,40 @@ mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
> > return (ret > 0) ? ret & mask : ret;
> > }
> >
> > +static int
> > +mp2971_linear2direct(struct mp2971_data *data, int page, int val)
> > +{
> > + /* simple case */
> > + if (val == 0)
> > + return 0;
> > +
> > + /* LINEAR16 does not support negative voltages */
> > + if (val < 0)
> > + return 0;
> > +
> > + /*
> > + * For a static exponents, we don't have a choice
> > + * but to adjust the value to it.
> > + */
> > +
> > + if (data->vout_exponent[page] < 0)
> > + val <<= -data->vout_exponent[page];
> > + else
> > + val >>= data->vout_exponent[page];
> > + return clamp_val(val, 0, 0xffff);
> > +}
> > +
> > +static int
> > +mp2971_vid2direct(struct mp2971_data *data, int page, int val)
> > +{
> > + int vrf = data->info.vrm_version[page];
> > +
> > + if (vrf == imvp9)
> > + return (val + 29) * data->vid_step[page];
> > +
> > + return (val + 49) * data->vid_step[page];
> > +}
> > +
>
> This looks suspicious. VID -> voltage calculations should be well
> defined and be implemented in mp2975_vid2direct(). It is not entirely
> clear why a second conversion function should be needed, and why it would
> use different calculations with different results than those for
> mp2975.
>
> Example, for vrf == imvp9, 10mV step size, and vid==1:
>
> mp2971: (1 + 29) * 10 = 30 * 10 = 300
> mp2975: 200 + (1 - 1) * 10 = 200 + 0 = 200
>
> vid = 0xff = 255:
>
> mp2971: (255 + 29) * 10 = 284 * 10 = 2840
> mp2975: 200 + (255 - 1) * 10 = 200 + 254 * 10 = 2740
>
> Also questionable is how there could ever be an IMVP9 setting with 5mV
> step size since IMVP9 explicitly specifies a step size of 10mV.
> Also, the maximum voltage for IMVP9 is specified as 2.74V.
>
> > static int
> > mp2975_vid2direct(int vrf, int val)
> > {
> > @@ -214,6 +274,74 @@ mp2975_read_phases(struct i2c_client *client, struct mp2975_data *data,
> > return ret;
> > }
> >
> > +static int
> > +mp2971_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 mp2971_data *data = to_mp2971_data(info);
> > + int ret;
> > +
> > + switch (reg) {
> > + case PMBUS_OT_FAULT_LIMIT:
> > + case PMBUS_VIN_OV_FAULT_LIMIT:
> > + case PMBUS_VOUT_OV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_FAULT_LIMIT:
> > + case PMBUS_READ_IOUT:
> > + ret = mp2975_read_word_helper(client, page, phase,
> > + reg, GENMASK(15, 0));
> > + break;
> > + case PMBUS_READ_VOUT:
> > + ret = mp2975_read_word_helper(client, page, phase, reg,
> > + GENMASK(11, 0));
> > + if (ret < 0)
> > + return ret;
> > + /*
> > + * READ_VOUT can be provided in VID or direct format. The
> > + * format type is specified by bit 15 of the register
> > + * MP2971_MFR_DC_LOOP_CTRL. The driver enforces VOUT direct
> > + * format, since device allows to set the different formats for
> > + * the different rails and also all VOUT limits registers are
> > + * provided in a direct format. In case format is VID - convert
> > + * to direct.
> > + */
> > + switch (data->vout_format[page]) {
> > + case linear:
> > + ret = mp2971_linear2direct(data, page, ret);
> > + break;
> > + case vid:
> > + ret = mp2971_vid2direct(data, page, ret);
> > + break;
> > + case direct:
> > + break;
> > + default:
> > + return -ENODATA;
> > + }
> > + break;
> > + case PMBUS_UT_WARN_LIMIT:
> > + case PMBUS_UT_FAULT_LIMIT:
> > + case PMBUS_VIN_UV_WARN_LIMIT:
> > + case PMBUS_VIN_UV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_WARN_LIMIT:
> > + case PMBUS_VOUT_OV_WARN_LIMIT:
> > + case PMBUS_VIN_OV_WARN_LIMIT:
> > + case PMBUS_IIN_OC_FAULT_LIMIT:
> > + case PMBUS_IOUT_OC_LV_FAULT_LIMIT:
> > + case PMBUS_IIN_OC_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_FAULT_LIMIT:
> > + case PMBUS_IOUT_UC_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_WARN_LIMIT:
> > + case PMBUS_PIN_OP_WARN_LIMIT:
> > + return -ENXIO;
> > + default:
> > + return -ENODATA;
> > + }
> > +
> > + return ret;
> > +}
> > +
>
> Much of that code seems duplicate from mp2975_read_word_data().
> Without datasheets I can not determine if this really makes sense
> and/or is needed, or if a single function can be used for all chips.
>
> > static int mp2975_read_word_data(struct i2c_client *client, int page,
> > int phase, int reg)
> > {
> > @@ -365,6 +493,63 @@ mp2975_set_phase_rail2(struct pmbus_driver_info *info, int num_phases)
> > info->pfunc[MP2975_MAX_PHASE_RAIL1 - i] = PMBUS_HAVE_IOUT;
> > }
> >
> > +static int mp2971_identify_multiphase(struct i2c_client *client,
> > + struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int ret;
> > +
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify multiphase for rail 1 - could be from 1 to 12. */
> > + ret = i2c_smbus_read_word_data(client, MP2975_MFR_VR_MULTI_CONFIG_R1);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + info->phases[0] = ret & GENMASK(3, 0);
> > +
> > + /*
> > + * The device provides a total of 8 PWM pins, and can be configured
> > + * to different phase count applications for rail 1 and rail 2.
> > + * Rail 1 can be set to 8 phases, while rail 2 can only be set to 4
> > + * phases at most. When rail 1’s phase count is configured as 0, rail
> > + * 1 operates with 1-phase DCM. When rail 2 phase count is configured
> > + * as 0, rail 2 is disabled.
> > + */
> > + if (info->phases[0] > data->max_phase_rail1)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
>
> Same here. The code is almost the same as mp2975_identify_multiphase().
> Again, without datasheets I can not determine if this really makes sense
> and/or is needed, or if a single function can be used for all chips.
>
> This is a recurring problem. It appears that the patch maximizes the
> changes against the current code instead of even trying to minimize them.
> Without datasheet, it is impossible to compare the chips to check if an
> implementation with fewer / less extensive changes would be warranted.
>
> > +
> > +static int
> > +mp2971_identify_vid(struct i2c_client *client, struct mp2971_data *data,
> > + struct pmbus_driver_info *info, u32 reg, int page,
> > + u32 imvp_bit, u32 vr_bit)
> > +{
> > + int ret;
> > +
> > + /* Identify VID mode and step selection. */
> > + ret = i2c_smbus_read_word_data(client, reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret & imvp_bit) {
> > + info->vrm_version[page] = imvp9;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> > + } else if (ret & vr_bit) {
> > + info->vrm_version[page] = vr12;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_ON;
> > + } else {
> > + info->vrm_version[page] = vr13;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > mp2975_identify_multiphase(struct i2c_client *client, struct mp2975_data *data,
> > struct pmbus_driver_info *info)
> > @@ -428,6 +613,68 @@ mp2975_identify_vid(struct i2c_client *client, struct mp2975_data *data,
> > return 0;
> > }
> >
> > +static int
> > +mp2971_identify_rails_vid(struct i2c_client *client, struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int ret;
> > +
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify VID mode for rail 1. */
> > + ret = mp2971_identify_vid(client, data, info,
> > + MP2975_MFR_VR_MULTI_CONFIG_R1, 0,
> > + data->imvp9_en_r1_mask,
> > + MP2975_VID_STEP_SEL_R1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify VID mode for rail 2, if connected. */
> > + if (info->phases[1])
> > + ret = mp2971_identify_vid(client, data, info,
> > + MP2975_MFR_VR_MULTI_CONFIG_R2, 1,
> > + data->imvp9_en_r2_mask,
> > + MP2975_VID_STEP_SEL_R2);
> > + return ret;
> > +}
> > +
> > +static int mp2971_identify_vout_format(struct i2c_client *client,
> > + struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int i, ret, vout_mode;
> > +
> > + for (i = 0; i < info->pages; i++) {
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vout_mode = ret;
> > +
> > + switch (vout_mode >> 5) {
> > + case 0:
> > + data->vout_format[i] = linear;
> > + data->vout_exponent[i] = ((s8)(vout_mode << 3)) >> 3;
> > + break;
> > + case 1:
> > + data->vout_format[i] = vid;
> > + break;
> > + case 2:
> > + data->vout_format[i] = direct;
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static int
> > mp2975_identify_rails_vid(struct i2c_client *client, struct mp2975_data *data,
> > struct pmbus_driver_info *info)
> > @@ -659,6 +906,24 @@ mp2975_vout_per_rail_config_get(struct i2c_client *client,
> > return 0;
> > }
> >
> > +static struct pmbus_driver_info mp2971_info = {
> > + .pages = 1,
> > + .format[PSC_VOLTAGE_IN] = linear,
> > + .format[PSC_VOLTAGE_OUT] = direct,
> > + .format[PSC_TEMPERATURE] = linear,
> > + .format[PSC_CURRENT_IN] = linear,
> > + .format[PSC_CURRENT_OUT] = linear,
> > + .format[PSC_POWER] = linear,
> > + .m[PSC_VOLTAGE_OUT] = 1,
> > + .R[PSC_VOLTAGE_OUT] = 3,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> > + PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
> > + .read_byte_data = mp2975_read_byte_data,
> > + .read_word_data = mp2971_read_word_data,
> > +};
> > +
> > static struct pmbus_driver_info mp2975_info = {
> > .pages = 1,
> > .format[PSC_VOLTAGE_IN] = linear,
> > @@ -683,63 +948,131 @@ static struct pmbus_driver_info mp2975_info = {
> > static int mp2975_probe(struct i2c_client *client)
> > {
> > struct pmbus_driver_info *info;
> > - struct mp2975_data *data;
> > int ret;
> > + char *name;
> >
> > - data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> > - GFP_KERNEL);
> > - if (!data)
> > - return -ENOMEM;
> > + name = (char *)i2c_match_id(mp2975_id, client)->name;
> >
> > - memcpy(&data->info, &mp2975_info, sizeof(*info));
> > - info = &data->info;
> > + if (!name)
> > + return -EINVAL;
> >
> > - /* Identify multiphase configuration for rail 2. */
> > - ret = mp2975_identify_multiphase_rail2(client);
> > - if (ret < 0)
> > - return ret;
> > + if (!strcmp(name, "mp2971") || !strcmp(name, "mp2973")) {
> > + struct mp2971_data *data;
> > + struct mp2971_device_info *device_info;
> >
> > - if (ret) {
> > - /* Two rails are connected. */
> > - data->info.pages = MP2975_PAGE_NUM;
> > - data->info.phases[1] = ret;
> > - data->info.func[1] = MP2975_RAIL2_FUNC;
> > - }
> > + data = devm_kzalloc(&client->dev, sizeof(struct mp2971_data),
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> >
> > - /* Identify multiphase configuration. */
> > - ret = mp2975_identify_multiphase(client, data, info);
> > - if (ret)
> > - return ret;
> > + device_info =
> > + (struct mp2971_device_info *)i2c_match_id(mp2975_id, client)
> > + ->driver_data;
> >
> > - /* Identify VID setting per rail. */
> > - ret = mp2975_identify_rails_vid(client, data, info);
> > - if (ret < 0)
> > - return ret;
> > + memcpy(&data->info, &mp2971_info, sizeof(*info));
> > + info = &data->info;
> >
> > - /* Obtain current sense gain of power stage. */
> > - ret = mp2975_current_sense_gain_get(client, data);
> > - if (ret)
> > - return ret;
> > + if (device_info) {
> > + data->imvp9_en_r1_mask = device_info->imvp9_en_r1_mask;
> > + data->imvp9_en_r2_mask = device_info->imvp9_en_r2_mask;
> > + data->max_phase_rail1 = device_info->max_phase_rail1;
> > + data->max_phase_rail2 = device_info->max_phase_rail2;
> > + }
> >
> > - /* Obtain voltage reference values. */
> > - ret = mp2975_vref_get(client, data, info);
> > - if (ret)
> > - return ret;
> > + /* Identify multiphase configuration for rail 2. */
> > + ret = mp2975_identify_multiphase_rail2(client);
> > + if (ret < 0)
> > + return ret;
> >
> > - /* Obtain vout over-voltage scales. */
> > - ret = mp2975_vout_ov_scale_get(client, data, info);
> > - if (ret < 0)
> > - return ret;
> > + if (ret) {
> > + /* Two rails are connected. */
> > + data->info.pages = MP2975_PAGE_NUM;
> > + data->info.phases[1] = ret;
> > + data->info.func[1] = MP2971_RAIL2_FUNC;
> > + }
> >
> > - /* Obtain offsets, maximum and format for vout. */
> > - ret = mp2975_vout_per_rail_config_get(client, data, info);
> > - if (ret)
> > - return ret;
> > + /* Identify multiphase configuration. */
> > + ret = mp2971_identify_multiphase(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Identify VID setting per rail. */
> > + ret = mp2971_identify_rails_vid(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify vout format. */
> > + ret = mp2971_identify_vout_format(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + } else {
> > + struct mp2975_data *data;
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + memcpy(&data->info, &mp2975_info, sizeof(*info));
> > + info = &data->info;
> > +
> > + /* Identify multiphase configuration for rail 2. */
> > + ret = mp2975_identify_multiphase_rail2(client);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret) {
> > + /* Two rails are connected. */
> > + data->info.pages = MP2975_PAGE_NUM;
> > + data->info.phases[1] = ret;
> > + data->info.func[1] = MP2975_RAIL2_FUNC;
> > + }
> > +
> > + /* Identify multiphase configuration. */
> > + ret = mp2975_identify_multiphase(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Identify VID setting per rail. */
> > + ret = mp2975_identify_rails_vid(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Obtain current sense gain of power stage. */
> > + ret = mp2975_current_sense_gain_get(client, data);
> > + if (ret)
> > + return ret;
> > +
> > + /* Obtain voltage reference values. */
> > + ret = mp2975_vref_get(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Obtain vout over-voltage scales. */
> > + ret = mp2975_vout_ov_scale_get(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Obtain offsets, maximum and format for vout. */
> > + ret = mp2975_vout_per_rail_config_get(client, data, info);
> > + if (ret)
> > + return ret;
> > + }
> >
> > return pmbus_do_probe(client, info);
> > }
> >
> > +static const struct mp2971_device_info mp2971_device_info = {
> > + .imvp9_en_r1_mask = BIT(14),
> > + .imvp9_en_r2_mask = BIT(13),
> > + .max_phase_rail1 = 8,
> > + .max_phase_rail2 = 4,
> > +};
> > +
> > static const struct i2c_device_id mp2975_id[] = {
> > + {"mp2971", (kernel_ulong_t)&mp2971_device_info },
> > + {"mp2973", (kernel_ulong_t)&mp2971_device_info },
> > {"mp2975", 0},
> > {}
> > };
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [External] Re: [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975
2022-12-10 17:54 ` Guenter Roeck
2023-01-18 3:55 ` Joel Stanley
@ 2023-01-18 5:42 ` 晓华王
1 sibling, 0 replies; 4+ messages in thread
From: 晓华王 @ 2023-01-18 5:42 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, linux-hwmon, jdelvare, openbmc, joel
On Sun, Dec 11, 2022 at 1:54 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 12/8/22 18:45, Wang Xiaohua wrote:
> > Add mp2971/mp2973 support in mp2975
> >
> > Tested with:
> > My unit only include mp2971 and mp2973 devices
> > MP2973:
> > cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*label
> > iin
> > iout1
> > iout2
> > vin
> > vout1
> > vout2
> > pin
> > pout1
> > pout2
> > cat /sys/bus/i2c/devices/5-0076/hwmon/hwmon24/*input
> > 0
> > 82500
> > 14000
> > 12187
> > 1787
> > 1793
> > 0
> > 148000000
> > 25000000
> > 54000
> > MP2971:
> > cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*label
> > iin
> > iout1
> > iout2
> > vin
> > vout1
> > vout2
> > pin
> > pout1
> > pout2
> > cat /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*input
> > 18500
> > 22000
> > 500
> > 12187
> > 1025
> > 1800
> > 226000000
> > 22000000
> > 1000000
> > 55000
> >
>
> Test results are not very useful. Better use something like
> "grep . /sys/bus/i2c/devices/5-0062/hwmon/hwmon20/*".
>
> Either case, test results should be provided after "---" and not be part
> of the commit description. Instead, the commit description should explain
> what those chips actually are.
Thanks for your comments.
> > Signed-off-by: Wang Xiaohua <wangxiaohua.1217@bytedance.com>
> >
> > v2:
> > - Fix auto build test WARNING
> >
> > v3:
> > - Fix incorrect return code
> > ---
> > drivers/hwmon/pmbus/mp2975.c | 415 +++++++++++++++++++++++++++++++----
> > 1 file changed, 374 insertions(+), 41 deletions(-)
> >
>
> Update to Documentation/hwmon/mp2975.rst and
> Documentation/devicetree/bindings/trivial-devices.yaml required.
Thanks. I will update the Documentation/hwmon/mp2975.rst.
> However, there is a more severe problem: The changes are too complex
> for me to review, and the chip datasheets are not published. I can not evaluate
> if the changes are really needed, if there is a less complex solution,
> or if they even make sense. Someone with access to a datasheet will have
> to step up as maintainer for this driver.
>
> Additional comments inline.
>
> Guenter
Yes, the chip datasheets are still not published. I will try to use a
less complex solution.
>
> > diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
> > index 51986adfbf47..4d1b7ac1800e 100644
> > --- a/drivers/hwmon/pmbus/mp2975.c
> > +++ b/drivers/hwmon/pmbus/mp2975.c
> > @@ -52,10 +52,33 @@
> > #define MP2975_MAX_PHASE_RAIL2 4
> > #define MP2975_PAGE_NUM 2
> >
> > +#define MP2971_RAIL2_FUNC \
> > + (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT | \
> > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT)
> > +
> > #define MP2975_RAIL2_FUNC (PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | \
> > PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | \
> > PMBUS_HAVE_POUT | PMBUS_PHASE_VIRTUAL)
> >
> > +struct mp2971_device_info {
> > + int max_phase_rail1;
> > + int max_phase_rail2;
> > + int imvp9_en_r1_mask;
> > + int imvp9_en_r2_mask;
> > +};
> > +
> > +struct mp2971_data {
> > + struct pmbus_driver_info info;
> > + int vid_step[MP2975_PAGE_NUM];
> > + int vout_format[MP2975_PAGE_NUM];
> > + int vout_mode[MP2975_PAGE_NUM];
> > + int vout_exponent[MP2975_PAGE_NUM];
> > + int max_phase_rail1;
> > + int max_phase_rail2;
> > + int imvp9_en_r1_mask;
> > + int imvp9_en_r2_mask;
> > +};
> > +
> > struct mp2975_data {
> > struct pmbus_driver_info info;
> > int vout_scale;
> > @@ -68,6 +91,9 @@ struct mp2975_data {
> > int curr_sense_gain[MP2975_PAGE_NUM];
> > };
> >
> > +static const struct i2c_device_id mp2975_id[];
> > +
> > +#define to_mp2971_data(x) container_of(x, struct mp2971_data, info)
> > #define to_mp2975_data(x) container_of(x, struct mp2975_data, info)
> >
> > static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
> > @@ -95,6 +121,40 @@ mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
> > return (ret > 0) ? ret & mask : ret;
> > }
> >
> > +static int
> > +mp2971_linear2direct(struct mp2971_data *data, int page, int val)
> > +{
> > + /* simple case */
> > + if (val == 0)
> > + return 0;
> > +
> > + /* LINEAR16 does not support negative voltages */
> > + if (val < 0)
> > + return 0;
> > +
> > + /*
> > + * For a static exponents, we don't have a choice
> > + * but to adjust the value to it.
> > + */
> > +
> > + if (data->vout_exponent[page] < 0)
> > + val <<= -data->vout_exponent[page];
> > + else
> > + val >>= data->vout_exponent[page];
> > + return clamp_val(val, 0, 0xffff);
> > +}
> > +
> > +static int
> > +mp2971_vid2direct(struct mp2971_data *data, int page, int val)
> > +{
> > + int vrf = data->info.vrm_version[page];
> > +
> > + if (vrf == imvp9)
> > + return (val + 29) * data->vid_step[page];
> > +
> > + return (val + 49) * data->vid_step[page];
> > +}
> > +
>
> This looks suspicious. VID -> voltage calculations should be well
> defined and be implemented in mp2975_vid2direct(). It is not entirely
> clear why a second conversion function should be needed, and why it would
> use different calculations with different results than those for
> mp2975.
>
Thanks, I will define and implement it in mp2975_vid2direct() and add
more comments in the code.
> Example, for vrf == imvp9, 10mV step size, and vid==1:
>
> mp2971: (1 + 29) * 10 = 30 * 10 = 300
> mp2975: 200 + (1 - 1) * 10 = 200 + 0 = 200
>
> vid = 0xff = 255:
>
> mp2971: (255 + 29) * 10 = 284 * 10 = 2840
> mp2975: 200 + (255 - 1) * 10 = 200 + 254 * 10 = 2740
>
> Also questionable is how there could ever be an IMVP9 setting with 5mV
> step size since IMVP9 explicitly specifies a step size of 10mV.
> Also, the maximum voltage for IMVP9 is specified as 2.74V.
>
Yes, you are right, the step size is 10mV.
> > static int
> > mp2975_vid2direct(int vrf, int val)
> > {
> > @@ -214,6 +274,74 @@ mp2975_read_phases(struct i2c_client *client, struct mp2975_data *data,
> > return ret;
> > }
> >
> > +static int
> > +mp2971_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 mp2971_data *data = to_mp2971_data(info);
> > + int ret;
> > +
> > + switch (reg) {
> > + case PMBUS_OT_FAULT_LIMIT:
> > + case PMBUS_VIN_OV_FAULT_LIMIT:
> > + case PMBUS_VOUT_OV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_FAULT_LIMIT:
> > + case PMBUS_READ_IOUT:
> > + ret = mp2975_read_word_helper(client, page, phase,
> > + reg, GENMASK(15, 0));
> > + break;
> > + case PMBUS_READ_VOUT:
> > + ret = mp2975_read_word_helper(client, page, phase, reg,
> > + GENMASK(11, 0));
> > + if (ret < 0)
> > + return ret;
> > + /*
> > + * READ_VOUT can be provided in VID or direct format. The
> > + * format type is specified by bit 15 of the register
> > + * MP2971_MFR_DC_LOOP_CTRL. The driver enforces VOUT direct
> > + * format, since device allows to set the different formats for
> > + * the different rails and also all VOUT limits registers are
> > + * provided in a direct format. In case format is VID - convert
> > + * to direct.
> > + */
> > + switch (data->vout_format[page]) {
> > + case linear:
> > + ret = mp2971_linear2direct(data, page, ret);
> > + break;
> > + case vid:
> > + ret = mp2971_vid2direct(data, page, ret);
> > + break;
> > + case direct:
> > + break;
> > + default:
> > + return -ENODATA;
> > + }
> > + break;
> > + case PMBUS_UT_WARN_LIMIT:
> > + case PMBUS_UT_FAULT_LIMIT:
> > + case PMBUS_VIN_UV_WARN_LIMIT:
> > + case PMBUS_VIN_UV_FAULT_LIMIT:
> > + case PMBUS_VOUT_UV_WARN_LIMIT:
> > + case PMBUS_VOUT_OV_WARN_LIMIT:
> > + case PMBUS_VIN_OV_WARN_LIMIT:
> > + case PMBUS_IIN_OC_FAULT_LIMIT:
> > + case PMBUS_IOUT_OC_LV_FAULT_LIMIT:
> > + case PMBUS_IIN_OC_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_WARN_LIMIT:
> > + case PMBUS_IOUT_OC_FAULT_LIMIT:
> > + case PMBUS_IOUT_UC_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_FAULT_LIMIT:
> > + case PMBUS_POUT_OP_WARN_LIMIT:
> > + case PMBUS_PIN_OP_WARN_LIMIT:
> > + return -ENXIO;
> > + default:
> > + return -ENODATA;
> > + }
> > +
> > + return ret;
> > +}
> > +
>
> Much of that code seems duplicate from mp2975_read_word_data().
> Without datasheets I can not determine if this really makes sense
> and/or is needed, or if a single function can be used for all chips.
>
Thanks for your suggestion, I will try to optimize the
mp2975_vid2direct() function so it can be used for all chips.
> > static int mp2975_read_word_data(struct i2c_client *client, int page,
> > int phase, int reg)
> > {
> > @@ -365,6 +493,63 @@ mp2975_set_phase_rail2(struct pmbus_driver_info *info, int num_phases)
> > info->pfunc[MP2975_MAX_PHASE_RAIL1 - i] = PMBUS_HAVE_IOUT;
> > }
> >
> > +static int mp2971_identify_multiphase(struct i2c_client *client,
> > + struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int ret;
> > +
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify multiphase for rail 1 - could be from 1 to 12. */
> > + ret = i2c_smbus_read_word_data(client, MP2975_MFR_VR_MULTI_CONFIG_R1);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + info->phases[0] = ret & GENMASK(3, 0);
> > +
> > + /*
> > + * The device provides a total of 8 PWM pins, and can be configured
> > + * to different phase count applications for rail 1 and rail 2.
> > + * Rail 1 can be set to 8 phases, while rail 2 can only be set to 4
> > + * phases at most. When rail 1’s phase count is configured as 0, rail
> > + * 1 operates with 1-phase DCM. When rail 2 phase count is configured
> > + * as 0, rail 2 is disabled.
> > + */
> > + if (info->phases[0] > data->max_phase_rail1)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
>
> Same here. The code is almost the same as mp2975_identify_multiphase().
> Again, without datasheets I can not determine if this really makes sense
> and/or is needed, or if a single function can be used for all chips.
>
> This is a recurring problem. It appears that the patch maximizes the
> changes against the current code instead of even trying to minimize them.
> Without datasheet, it is impossible to compare the chips to check if an
> implementation with fewer / less extensive changes would be warranted.
>
Thanks for your comments. I will optimize the code.
> > +
> > +static int
> > +mp2971_identify_vid(struct i2c_client *client, struct mp2971_data *data,
> > + struct pmbus_driver_info *info, u32 reg, int page,
> > + u32 imvp_bit, u32 vr_bit)
> > +{
> > + int ret;
> > +
> > + /* Identify VID mode and step selection. */
> > + ret = i2c_smbus_read_word_data(client, reg);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret & imvp_bit) {
> > + info->vrm_version[page] = imvp9;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> > + } else if (ret & vr_bit) {
> > + info->vrm_version[page] = vr12;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_ON;
> > + } else {
> > + info->vrm_version[page] = vr13;
> > + data->vid_step[page] = MP2975_PROT_DEV_OV_OFF;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int
> > mp2975_identify_multiphase(struct i2c_client *client, struct mp2975_data *data,
> > struct pmbus_driver_info *info)
> > @@ -428,6 +613,68 @@ mp2975_identify_vid(struct i2c_client *client, struct mp2975_data *data,
> > return 0;
> > }
> >
> > +static int
> > +mp2971_identify_rails_vid(struct i2c_client *client, struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int ret;
> > +
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify VID mode for rail 1. */
> > + ret = mp2971_identify_vid(client, data, info,
> > + MP2975_MFR_VR_MULTI_CONFIG_R1, 0,
> > + data->imvp9_en_r1_mask,
> > + MP2975_VID_STEP_SEL_R1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify VID mode for rail 2, if connected. */
> > + if (info->phases[1])
> > + ret = mp2971_identify_vid(client, data, info,
> > + MP2975_MFR_VR_MULTI_CONFIG_R2, 1,
> > + data->imvp9_en_r2_mask,
> > + MP2975_VID_STEP_SEL_R2);
> > + return ret;
> > +}
> > +
> > +static int mp2971_identify_vout_format(struct i2c_client *client,
> > + struct mp2971_data *data,
> > + struct pmbus_driver_info *info)
> > +{
> > + int i, ret, vout_mode;
> > +
> > + for (i = 0; i < info->pages; i++) {
> > + ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_smbus_read_byte_data(client, PMBUS_VOUT_MODE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + vout_mode = ret;
> > +
> > + switch (vout_mode >> 5) {
> > + case 0:
> > + data->vout_format[i] = linear;
> > + data->vout_exponent[i] = ((s8)(vout_mode << 3)) >> 3;
> > + break;
> > + case 1:
> > + data->vout_format[i] = vid;
> > + break;
> > + case 2:
> > + data->vout_format[i] = direct;
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static int
> > mp2975_identify_rails_vid(struct i2c_client *client, struct mp2975_data *data,
> > struct pmbus_driver_info *info)
> > @@ -659,6 +906,24 @@ mp2975_vout_per_rail_config_get(struct i2c_client *client,
> > return 0;
> > }
> >
> > +static struct pmbus_driver_info mp2971_info = {
> > + .pages = 1,
> > + .format[PSC_VOLTAGE_IN] = linear,
> > + .format[PSC_VOLTAGE_OUT] = direct,
> > + .format[PSC_TEMPERATURE] = linear,
> > + .format[PSC_CURRENT_IN] = linear,
> > + .format[PSC_CURRENT_OUT] = linear,
> > + .format[PSC_POWER] = linear,
> > + .m[PSC_VOLTAGE_OUT] = 1,
> > + .R[PSC_VOLTAGE_OUT] = 3,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> > + PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
> > + PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
> > + PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT,
> > + .read_byte_data = mp2975_read_byte_data,
> > + .read_word_data = mp2971_read_word_data,
> > +};
> > +
> > static struct pmbus_driver_info mp2975_info = {
> > .pages = 1,
> > .format[PSC_VOLTAGE_IN] = linear,
> > @@ -683,63 +948,131 @@ static struct pmbus_driver_info mp2975_info = {
> > static int mp2975_probe(struct i2c_client *client)
> > {
> > struct pmbus_driver_info *info;
> > - struct mp2975_data *data;
> > int ret;
> > + char *name;
> >
> > - data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> > - GFP_KERNEL);
> > - if (!data)
> > - return -ENOMEM;
> > + name = (char *)i2c_match_id(mp2975_id, client)->name;
> >
> > - memcpy(&data->info, &mp2975_info, sizeof(*info));
> > - info = &data->info;
> > + if (!name)
> > + return -EINVAL;
> >
> > - /* Identify multiphase configuration for rail 2. */
> > - ret = mp2975_identify_multiphase_rail2(client);
> > - if (ret < 0)
> > - return ret;
> > + if (!strcmp(name, "mp2971") || !strcmp(name, "mp2973")) {
> > + struct mp2971_data *data;
> > + struct mp2971_device_info *device_info;
> >
> > - if (ret) {
> > - /* Two rails are connected. */
> > - data->info.pages = MP2975_PAGE_NUM;
> > - data->info.phases[1] = ret;
> > - data->info.func[1] = MP2975_RAIL2_FUNC;
> > - }
> > + data = devm_kzalloc(&client->dev, sizeof(struct mp2971_data),
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> >
> > - /* Identify multiphase configuration. */
> > - ret = mp2975_identify_multiphase(client, data, info);
> > - if (ret)
> > - return ret;
> > + device_info =
> > + (struct mp2971_device_info *)i2c_match_id(mp2975_id, client)
> > + ->driver_data;
> >
> > - /* Identify VID setting per rail. */
> > - ret = mp2975_identify_rails_vid(client, data, info);
> > - if (ret < 0)
> > - return ret;
> > + memcpy(&data->info, &mp2971_info, sizeof(*info));
> > + info = &data->info;
> >
> > - /* Obtain current sense gain of power stage. */
> > - ret = mp2975_current_sense_gain_get(client, data);
> > - if (ret)
> > - return ret;
> > + if (device_info) {
> > + data->imvp9_en_r1_mask = device_info->imvp9_en_r1_mask;
> > + data->imvp9_en_r2_mask = device_info->imvp9_en_r2_mask;
> > + data->max_phase_rail1 = device_info->max_phase_rail1;
> > + data->max_phase_rail2 = device_info->max_phase_rail2;
> > + }
> >
> > - /* Obtain voltage reference values. */
> > - ret = mp2975_vref_get(client, data, info);
> > - if (ret)
> > - return ret;
> > + /* Identify multiphase configuration for rail 2. */
> > + ret = mp2975_identify_multiphase_rail2(client);
> > + if (ret < 0)
> > + return ret;
> >
> > - /* Obtain vout over-voltage scales. */
> > - ret = mp2975_vout_ov_scale_get(client, data, info);
> > - if (ret < 0)
> > - return ret;
> > + if (ret) {
> > + /* Two rails are connected. */
> > + data->info.pages = MP2975_PAGE_NUM;
> > + data->info.phases[1] = ret;
> > + data->info.func[1] = MP2971_RAIL2_FUNC;
> > + }
> >
> > - /* Obtain offsets, maximum and format for vout. */
> > - ret = mp2975_vout_per_rail_config_get(client, data, info);
> > - if (ret)
> > - return ret;
> > + /* Identify multiphase configuration. */
> > + ret = mp2971_identify_multiphase(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Identify VID setting per rail. */
> > + ret = mp2971_identify_rails_vid(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Identify vout format. */
> > + ret = mp2971_identify_vout_format(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + } else {
> > + struct mp2975_data *data;
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(struct mp2975_data),
> > + GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + memcpy(&data->info, &mp2975_info, sizeof(*info));
> > + info = &data->info;
> > +
> > + /* Identify multiphase configuration for rail 2. */
> > + ret = mp2975_identify_multiphase_rail2(client);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret) {
> > + /* Two rails are connected. */
> > + data->info.pages = MP2975_PAGE_NUM;
> > + data->info.phases[1] = ret;
> > + data->info.func[1] = MP2975_RAIL2_FUNC;
> > + }
> > +
> > + /* Identify multiphase configuration. */
> > + ret = mp2975_identify_multiphase(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Identify VID setting per rail. */
> > + ret = mp2975_identify_rails_vid(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Obtain current sense gain of power stage. */
> > + ret = mp2975_current_sense_gain_get(client, data);
> > + if (ret)
> > + return ret;
> > +
> > + /* Obtain voltage reference values. */
> > + ret = mp2975_vref_get(client, data, info);
> > + if (ret)
> > + return ret;
> > +
> > + /* Obtain vout over-voltage scales. */
> > + ret = mp2975_vout_ov_scale_get(client, data, info);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Obtain offsets, maximum and format for vout. */
> > + ret = mp2975_vout_per_rail_config_get(client, data, info);
> > + if (ret)
> > + return ret;
> > + }
> >
> > return pmbus_do_probe(client, info);
> > }
> >
> > +static const struct mp2971_device_info mp2971_device_info = {
> > + .imvp9_en_r1_mask = BIT(14),
> > + .imvp9_en_r2_mask = BIT(13),
> > + .max_phase_rail1 = 8,
> > + .max_phase_rail2 = 4,
> > +};
> > +
> > static const struct i2c_device_id mp2975_id[] = {
> > + {"mp2971", (kernel_ulong_t)&mp2971_device_info },
> > + {"mp2973", (kernel_ulong_t)&mp2971_device_info },
> > {"mp2975", 0},
> > {}
> > };
>
BR,
Xiaohua
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-18 5:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 2:45 [PATCH linux dev-6.0 v3] pmbus: Add mp2971/mp2973 support in mp2975 Wang Xiaohua
2022-12-10 17:54 ` Guenter Roeck
2023-01-18 3:55 ` Joel Stanley
2023-01-18 5:42 ` [External] " 晓华王
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).