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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71a6398f-1cda-4d83-992d-328d906ad8af@kernel.org>
Date: Wed, 25 Jun 2025 10:08:41 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: 吳梓豪 <tzuhao.wtmh@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>,
 Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Naresh Solanki <naresh.solanki@...ements.com>,
 Rodrigo Gobbi <rodrigo.gobbi.7@...il.com>,
 Michal Simek <michal.simek@....com>, Fabio Estevam <festevam@...il.com>,
 Henry Wu <Henry_Wu@...ntatw.com>, Grant Peltier <grantpeltier93@...il.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Cedric Encarnacion <cedricjustine.encarnacion@...log.com>,
 Kim Seer Paller <kimseer.paller@...log.com>,
 Leo Yang <leo.yang.sy0@...il.com>, Ninad Palsule <ninad@...ux.ibm.com>,
 Alex Vdovydchenko <xzeol@...oo.com>,
 John Erasmus Mari Geronimo <johnerasmusmari.geronimo@...log.com>,
 Nuno Sa <nuno.sa@...log.com>, Jerome Brunet <jbrunet@...libre.com>,
 Noah Wang <noahwang.wang@...look.com>,
 Mariel Tinaco <Mariel.Tinaco@...log.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] hwmon: (pmbus) Add support for MPS multi-phase
 mp2869a/mp29612a controllers

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