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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/06/2025 08:31, 吳梓豪 wrote:
>>
>>> +     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

No, this makes it a singleton. I don't see the code in current driver,
so I don't get whether you meant current code or future. If current:
where is it modified?

mp2869a_read_vout() has terrible style btw, really not looking like
Linux coding style. Be sure you carefully follow the style.

>>
>>> +     .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

Where do you modify the contents?


Best regards,
Krzysztof