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
