Re: [PATCH 1/2] hwmon: (pmbus) Add support for MPS multi-phase mp2869a/mp29612a controllers

[吳梓豪]

Krzysztof Kozlowski <krzk@xxxxxxxxxx> 於 2025年6月24日 週二 下午3:48寫道：
>
> On 24/06/2025 09:41, tzuhao.wtmh@xxxxxxxxx wrote:
> > +static int
> > +MP2869A_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;
> > +     }
> > +}
> > +
> > +static int
> > +MP2869A_identify_vout_format(struct i2c_client *client,
>
> Use Linux coding style, so lowercase for variables, types and functions.
> Everywhere (except when coding style tells you different, so please read
> it).
>
> > +                         struct MP2869A_data *data)
> > +{
> > +     int i, ret;
> > +
> > +     for (i = 0; i < data->info.pages; i++) {
> > +             ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             ret = i2c_smbus_read_word_data(client, MP2869A_VOUT_MODE);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             switch (ret & MP2869A_VOUT_MODE_MASK) {
> > +             case MP2869A_VOUT_MODE_VID:
> > +                     data->vout_format[i] = vid;
> > +                     break;
> > +             default:
> > +             return -EINVAL;
> > +             }
> > +             }
>
> Messed indentation in multiple places.
>
> > +     return 0;
> > +}
> > +
> > +static struct pmbus_driver_info MP2869A_info = {
>
> This is const.
Since info will be modified by mp2869a_read_vout at runtime, I chose
not to make it constant
>
> > +     .pages = MP2869A_PAGE_NUM,
> > +     .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,
> > +     .b[PSC_VOLTAGE_OUT] = 0,
> > +     .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,
> > +     .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_IOUT |
> > +             PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT | PMBUS_HAVE_TEMP,
> > +     .read_byte_data = MP2869A_read_byte_data,
> > +     .read_word_data = MP2869A_read_word_data,
> > +};
> > +
> > +static int mp2869a_probe(struct i2c_client *client)
> > +{
> > +     struct pmbus_driver_info *info;
> > +     struct MP2869A_data *data;
> > +     int ret;
> > +
> > +     data = devm_kzalloc(&client->dev, sizeof(struct MP2869A_data),
>
> sizeof(*)
>
> > +             GFP_KERNEL);
>
> Misaligned. Run checkpatch --srtict
>
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->chip_id = (enum chips)(uintptr_t)i2c_get_match_data(client);
>
> These are just wrong or redundant casts. You need only one cast -
> kernel_ulong_t
>
> > +
> > +     memcpy(data->max_phases, mp2869a_max_phases[data->chip_id],
> > +             sizeof(data->max_phases));
>
> Why you cannot just store the pointer?
As chip_id and max_phase will be constant, it should be acceptable to
handle them via pointers in this case.
>
> > +
> > +     memcpy(&data->info, &MP2869A_info, sizeof(*info));
>
> Why you cannot just store the pointer?
Considering that the info can change at runtime, using memcpy is a
safer approach
>
> > +     info = &data->info;
> > +
> > +
>
> One blank line, not two
>
> > +     /* Identify vout format. */
> > +     ret = MP2869A_identify_vout_format(client, data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* set the device to page 0 */
> > +     i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);
> > +
> > +     return pmbus_do_probe(client, info);
> > +}
> > +
> > +static const struct of_device_id __maybe_unused mp2869a_of_match[] = {
> > +     { .compatible = "mps,mp2869a", .data = (void *)mp2869a },
> > +     { .compatible = "mps,mp29612a", .data = (void *)mp29612a},
> > +     {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, mp2869a_of_match);
> > +
> > +static struct i2c_driver mp2869a_driver = {
> > +     .driver = {
> > +             .name = "mp2869a",
> > +             .of_match_table = mp2869a_of_match,
> > +     },
> > +     .probe = mp2869a_probe,
> > +     .id_table = mp2869a_id,
> > +};
> > +
> > +module_i2c_driver(mp2869a_driver);
> > +
> > +
>
> One blank line, not two. This applies everywhere.
>
> > +MODULE_AUTHOR("Henry Wu <Henry_WU@xxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("PMBus driver for MPS MP2869A/MP29612A device");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS(PMBUS);
>
>
> Best regards,
> Krzysztof

Thank you for your advice. I still have a few questions and would
appreciate your help in resolving them.

Best regards,
Henry Wu