Messages in this thread | | | Date | Wed, 3 Apr 2024 17:06:43 -0500 | Subject | Re: [PATCH 00/31] Remove use of i2c_match_id in HWMON | From | Andrew Davis <> |
| |
On 4/3/24 4:30 PM, Guenter Roeck wrote: > On Wed, Apr 03, 2024 at 03:36:02PM -0500, Andrew Davis wrote: >> Hello all, >> >> Goal here is to remove the i2c_match_id() function from all drivers. >> Using i2c_get_match_data() can simplify code and has some other >> benefits described in the patches. >> > > The return value from i2c_match_id() is typically an integer (chip ID) > starting with 0. Previously it has been claimed that this would be > unacceptable for i2c_get_match_data(), and chip IDs were changed to start > with 1. Commit ac0c26bae662 ("hwmon: (lm25066) Use i2c_get_match_data()") > is an example. Either this series is wrong, or the previous claim that > chip IDs (i.e., the content of .driver_data or .data) must not be 0 was > wrong. Which one is it ? I find it very confusing that the chip type for > some drivers now starts with 1 and for others with 0. Given that, I am not > inclined to accept this series unless it is explained in detail why the > chip type enum in, for example, drivers/hwmon/pmbus/lm25066.c has to start > with one but is ok to start with 0 for all drivers affected by this > series. Quite frankly, even if there is some kind of explanation, I am not > sure if I am going to accept it because future driver developers won't > know if they have to start chip types with 0 or 1. >
i2c_get_match_data() has no issue with returning 0 when the driver_data for the match is also 0 (as it will be when the chip type is 0 here).
The confusion might be that returning 0 is also considered a failure code. This is a problem in general with returning errors in-band with data, and that is nothing new as i2c_match_id() does the same thing.
Actually, i2c_match_id() is worse as most of these drivers take the result from that and immediately dereference it. Meaning if i2c_match_id() ever did failed to find a match, they would crash before this series. Luckily i2c_match_id() can't fail to find a match as far as I can tell, and so for the same reason neither can i2c_get_match_data(), which means if 0 is returned it is always because the chip ID was actually 0.
At some point we should switch all the *_get_match_data() functions to return an error code and put the match if found as a argument pointer. Forcing everyone to changing the chip type to avoid 0 as done in ac0c26bae662 is the wrong way to fix an issue like that.
Andrew
| |