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

[Andrew Davis]

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@xxxxxx>
> > ---
> >   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