From: Guenter Roeck <linux@roeck-us.net>
To: Andrew Davis <afd@ti.com>
Cc: Rob Herring <robh@kernel.org>, 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
Subject: Re: [PATCH 00/31] Remove use of i2c_match_id in HWMON
Date: Thu, 18 Apr 2024 06:42:39 -0700 [thread overview]
Message-ID: <340e8274-dd6b-49b0-906b-32da60745b22@roeck-us.net> (raw)
In-Reply-To: <fce93a8b-7225-4775-b265-d283a863f969@ti.com>
On Tue, Apr 16, 2024 at 12:08:50PM -0500, Andrew Davis wrote:
> On 4/16/24 9:16 AM, Guenter Roeck wrote:
> > On Mon, Apr 08, 2024 at 04:49:43AM -0700, Guenter Roeck wrote:
> > > On Wed, Apr 03, 2024 at 05:06:43PM -0500, Andrew Davis wrote:
> > > > 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.
> > > >
> > >
> > > That doesn't really answer my question. It does not explain why it was
> > > necessary to change the chip ID base for other drivers from 0 to 1,
> > > but not for the drivers in this series. I fail to see the difference,
> > > and I have to assume that others looking into the code will have the
> > > same problem.
> > >
> >
> > Just to follow up: I am not going to apply this series until I understand
> > why the chip ID range had to be changed from 0.. to 1.. for other hardware
> > monitoring drivers (lm25066, nct6775) but not for the drivers changed
> > in this series. I have been telling people that chip IDs need to start
> > with 1 if i2c_get_match_data() is used. I'll need understand when and
> > why this is needed to be able to provide guidance to other developers.
> >
>
> I was hoping one of those patch authors that made those 0->1 changes
> would speak up (+Rob), I can't know what their thinking was, only
> offer my best guess as I did above.
>
I can see three possibilities.
- Chip IDs must start with 1 if i2c_get_match_data() is used, as I was told
previously. If so, this series is wrong.
- It is ok for chip IDs to start with 0. If so, what I have been told
previously is wrong, and the patches changing chip IDs to start with 1
can and should be partially reverted to avoid confusion.
- Chip IDs must sometimes, but not always, start with 1. If so, the
conditions will have to be documented to help driver developers decide
the valid starting value and to be able to determine if all the patches
in this series follow the rules.
Someone will have to step up and explain to me which one it is.
Thanks,
Guenter
next prev parent reply other threads:[~2024-04-18 13:42 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 20:36 [PATCH 00/31] Remove use of i2c_match_id in HWMON Andrew Davis
2024-04-03 20:36 ` [PATCH 01/31] hwmon: (ad7418) Remove use of i2c_match_id() Andrew Davis
2024-04-03 20:36 ` [PATCH 02/31] hwmon: (adm1021) " Andrew Davis
2024-04-03 20:36 ` [PATCH 03/31] hwmon: (adm1031) " Andrew Davis
2024-04-03 20:36 ` [PATCH 04/31] hwmon: (ads7828) " Andrew Davis
2024-04-03 20:36 ` [PATCH 05/31] hwmon: (adt7475) " Andrew Davis
2024-04-03 20:36 ` [PATCH 06/31] hwmon: (aht10) " Andrew Davis
2024-04-03 20:36 ` [PATCH 07/31] hwmon: (dme1737) " Andrew Davis
2024-04-03 20:36 ` [PATCH 08/31] hwmon: (ds1621) " Andrew Davis
2024-04-03 20:36 ` [PATCH 09/31] hwmon: (f75375s) " Andrew Davis
2024-04-03 20:36 ` [PATCH 10/31] hwmon: (fschmd) " Andrew Davis
2024-04-03 20:36 ` [PATCH 11/31] hwmon: (ina2xx) " Andrew Davis
2024-04-03 20:36 ` [PATCH 12/31] hwmon: (lm63) " Andrew Davis
2024-04-03 20:36 ` [PATCH 13/31] hwmon: (lm75) " Andrew Davis
2024-04-03 20:36 ` [PATCH 14/31] hwmon: (lm78) " Andrew Davis
2024-04-03 20:36 ` [PATCH 15/31] hwmon: (lm83) " Andrew Davis
2024-04-03 20:36 ` [PATCH 16/31] hwmon: (lm85) " Andrew Davis
2024-04-03 20:36 ` [PATCH 17/31] hwmon: (lm90) " Andrew Davis
2024-04-03 20:36 ` [PATCH 18/31] hwmon: (lm95234) " Andrew Davis
2024-04-03 20:36 ` [PATCH 19/31] hwmon: (max16065) " Andrew Davis
2024-04-03 20:36 ` [PATCH 20/31] hwmon: (max1668) " Andrew Davis
2024-04-03 20:36 ` [PATCH 21/31] hwmon: (max6697) " Andrew Davis
2024-04-03 20:36 ` [PATCH 22/31] hwmon: (mcp3021) " Andrew Davis
2024-04-03 20:36 ` [PATCH 23/31] hwmon: (powr1220) " Andrew Davis
2024-04-03 20:36 ` [PATCH 24/31] hwmon: (sht3x) " Andrew Davis
2024-04-03 20:36 ` [PATCH 25/31] hwmon: (shtc1) " Andrew Davis
2024-04-03 20:36 ` [PATCH 26/31] hwmon: (thmc50) " Andrew Davis
2024-04-03 20:36 ` [PATCH 27/31] hwmon: (tmp401) " Andrew Davis
2024-04-03 20:36 ` [PATCH 28/31] hwmon: (tmp421) " Andrew Davis
2024-04-03 20:36 ` [PATCH 29/31] hwmon: (tmp464) " Andrew Davis
2024-04-03 20:36 ` [PATCH 30/31] hwmon: (w83781d) " Andrew Davis
2024-04-03 20:36 ` [PATCH 31/31] hwmon: (w83795): " Andrew Davis
2024-04-03 21:30 ` [PATCH 00/31] Remove use of i2c_match_id in HWMON Guenter Roeck
2024-04-03 22:06 ` Andrew Davis
2024-04-08 11:49 ` Guenter Roeck
2024-04-16 14:16 ` Guenter Roeck
2024-04-16 17:08 ` Andrew Davis
2024-04-18 13:42 ` Guenter Roeck [this message]
2024-06-06 17:17 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=340e8274-dd6b-49b0-906b-32da60745b22@roeck-us.net \
--to=linux@roeck-us.net \
--cc=afd@ti.com \
--cc=jdelvare@suse.com \
--cc=juergh@proton.me \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=riku.voipio@iki.fi \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).