Re: [PATCH v7 2/3] i2c: core: add device-managed version of i2c_new_dummy

From: Peter Rosin <peda@axentia.se>
To: Wolfram Sang <wsa@the-dreams.de>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Kieran Bingham <kieran@ksquared.org.uk>
Subject: Re: [PATCH v7 2/3] i2c: core: add device-managed version of i2c_new_dummy
Date: Sun, 17 Mar 2019 21:32:17 +0000	[thread overview]
Message-ID: <93ded6d5-1688-896a-f42e-0bbeda446c1d@axentia.se> (raw)
In-Reply-To: <20190316164308.GA1747@kunai>

On 2019-03-16 17:43, Wolfram Sang wrote:
> 
>> I personally really dislike the proposed name. It is akin to the abomination
>> where some sort of abbreviation of the types of variables are included also
>> in the variable names. It's useless clutter, at least to me.
> 
> In general, I agree with you...
> 
>> I can see that you do not want to change the i2c_new_{device,dummy} names
>> since they are kind of widespread and I can also see that you want to
>> match the devm_ name with the non-devm_ name. So, I see *why* this naming
>> is as it is, but I just don't like it.
> 
> ... yet, there is another reason which is consistency. i2c_new_{device,dummy}
> return NULL if something went wrong, and if the devm_* variants return
> an ERRPTR, this is super confusing and error prone IMO. And the
> difference between 'i2c_new_dummy' and 'i2c_register_dummy' doesn't make
> that more clear, I think.
> 
> I would love to convert all of those functions to return an errptr at
> some time. However, they are so widespread that this is difficult. I
> actually had a look to convert the users with coccinelle, but handling
> the error cases is so diverse that I don't think this is a way forward.

Ok, it's your call obviously, and doing a rename away from the _ptrerr
suffix when the old name is no longer in use is easier than changing
the return value convention. You just have to wait a couple of releases
so that stragglers that are slow to upstream don't get caught by the
changed semantics when it eventually happens. However, my point is that
these conversions tend to drag out, and in the meantime we are stuck
with whatever names we chose.

Perhaps add a rule to checkpatch to avoid new instances of the now
inferior i2c_new_{device,dummy}?

Anyway, what happens for the callers that ignore the return value of
these functions? Is that always a leak or are there cases when it is ok?
__must_check?? (not applicable for devm_... of course)

Cheers,
Peter