Received-SPF: pass (google.com: domain of linux-kernel+bounces-130665-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <77b2f8ce-0b71-4a7a-81bc-a64a1af3566d@ti.com>
Date: Wed, 3 Apr 2024 17:06:43 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 00/31] Remove use of i2c_match_id in HWMON
To: Guenter Roeck <linux@roeck-us.net>
CC: Jean Delvare <jdelvare@suse.com>, Juerg Haefliger <juergh@proton.me>,
        Riku
 Voipio <riku.voipio@iki.fi>, <linux-hwmon@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20240403203633.914389-1-afd@ti.com>
 <0e43aa83-2e02-49e2-96b8-24cac0362a7b@roeck-us.net>
Content-Language: en-US
From: Andrew Davis <afd@ti.com>
In-Reply-To: <0e43aa83-2e02-49e2-96b8-24cac0362a7b@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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