Re: [PATCH 02/11] hwmon: (pmbus/ibm-cffps) Remove use of i2c_match_id()

[Andrew Davis <afd@ti.com>]

On 3/6/26 10:19 AM, Guenter Roeck wrote:
> On Thu, Mar 05, 2026 at 01:56:33PM -0600, Andrew Davis wrote:
>> The function i2c_match_id() is used to fetch the matching ID from
>> the i2c_device_id table. This is often used to then retrieve the
>> matching driver_data. This can be done in one step with the helper
>> i2c_get_match_data().
>>
>> This helper has a couple other benefits:
>>   * It doesn't need the i2c_device_id passed in so we do not need
>>     to have that forward declared, allowing us to remove those or
>>     move the i2c_device_id table down to its more natural spot
>>     with the other module info.
>>   * It also checks for device match data, which allows for OF and
>>     ACPI based probing. That means we do not have to manually check
>>     those first and can remove those checks.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/hwmon/pmbus/ibm-cffps.c | 12 +-----------
>>   1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
>> index d05ef7a968a96..884b17de3b623 100644
>> --- a/drivers/hwmon/pmbus/ibm-cffps.c
>> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
>> @@ -482,19 +482,9 @@ MODULE_DEVICE_TABLE(i2c, ibm_cffps_id);
>>   static int ibm_cffps_probe(struct i2c_client *client)
>>   {
>>   	int i, rc;
>> -	enum versions vs = cffps_unknown;
>> +	enum versions vs = (uintptr_t)i2c_get_match_data(client);
> 
> AI feedback:
> 
>    Does this inadvertently change the fallback behavior for unmatched devices?
> 
>    In the old code, `vs` was initialized to `cffps_unknown` (which is 2). If a
>    device was instantiated manually via sysfs without a matching I2C name, both
>    `md` and `id` would be NULL, and `vs` remained `cffps_unknown`. This
>    correctly triggered the dynamic version detection logic using
>    `CFFPS_CCIN_CMD`.
> 
>    With the new code, if `i2c_get_match_data()` doesn't find a match, it returns
>    `NULL`. Since `NULL` casts to `0` (`cffps1`), the driver will now default to
>    `cffps1` and skip the dynamic version detection entirely.
> 
>    Additionally, because `cffps1` is `0`, `i2c_get_match_data()` cannot distinguish
>    between an explicit `cffps1` match (where `.data` is `0`) and no match at
>    all, since both result in returning `NULL`.
> 
>    To safely support fallback, `cffps1` could be assigned a non-zero value in
>    `enum versions` (e.g., `enum versions { cffps1 = 1, ... }`).
> 

This does seem valid. I will make `cffps_unknown` the 0th enum so existing
behavior is preserved.

Andrew

> Guenter
> 
>>   	struct dentry *debugfs;
>>   	struct ibm_cffps *psu;
>> -	const void *md = of_device_get_match_data(&client->dev);
>> -	const struct i2c_device_id *id;
>> -
>> -	if (md) {
>> -		vs = (uintptr_t)md;
>> -	} else {
>> -		id = i2c_match_id(ibm_cffps_id, client);
>> -		if (id)
>> -			vs = (enum versions)id->driver_data;
>> -	}
>>   
>>   	if (vs == cffps_unknown) {
>>   		u16 ccin_revision = 0;
>> -- 
>> 2.39.2
>>