Hi Wolfram,
On 14/03/2019 10:25, Peter Rosin wrote:
> Hi!
>
> A comment from the peanut gallery...
>
> On 2019-03-13 17:55, Wolfram Sang wrote:
>> From: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>
>> i2c_new_dummy is typically called from the probe function of the
>> driver for the primary i2c client. It requires calls to
>> i2c_unregister_device in the error path of the probe function and
>> in the remove function.
>> This can be simplified by introducing a device-managed version.
>>
>> Note the changed error case return value type:
>> i2c_new_dummy returns NULL whilst devm_new_i2c_dummy returns an ERR_PTR.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> [wsa: rename new functions to 'errptr' style and fix minor kdoc issues]
>> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> Documentation/driver-model/devres.txt | 3 +++
>> drivers/i2c/i2c-core-base.c | 44 +++++++++++++++++++++++++++++++++++
>> include/linux/i2c.h | 3 +++
>> 3 files changed, 50 insertions(+)
>>
>> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
>> index b277cafce71e..e36dc8041857 100644
>> --- a/Documentation/driver-model/devres.txt
>> +++ b/Documentation/driver-model/devres.txt
>> @@ -266,6 +266,9 @@ GPIO
>> devm_gpio_request_one()
>> devm_gpio_free()
>>
>> +I2C
>> + devm_i2c_new_dummy_errptr()
>> +
>
> TL;DR Can we call the new functions
>
> i2c_register_device
> i2c_register_dummy
> devm_i2c_register_dummy
>
> or
>
> i2c_add_device
> i2c_add_dummy
> devm_i2c_add_dummy
>
> or something? Please?
>
> 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.
I hate to be jumping in with just a 'me-too' - but I also had a similar
disliking to the _errptr suffix on the function name here.
A definite +1 for the addition of the devres handling though. That's got
a lot of benefit for the additional client addresses on multi-address
devices.
--
Regards
Kieran
>
> 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.
>
> I had a look at a couple of the i2c_new_dummy call sites, and some of them
> can be easily updated to simply pass on the PTRERR instead of hardcoding
> INVAL (or something) on NULL, some of them ignore the return value (and are
> thus not able to call i2c_unregister_device correctly) and some are
> different in other ways. In short, it seems that there are plenty of good
> opportunities to update lots of call sites to the new interfaces.
>
> However, after most of the maintained uses have been updated we are bound
> to have a tail of unmaintained code that will use the old interface. At
> some point, someone might convert the tail of call sites using the old
> NULL-returning functions. At that point we are stuck with a bunch of ugly
> hysterical foo_errptr names. And again it will be a daunting series to
> change them all at once.
>
> Cheers,
> Peter
>
--
Regards
--
Kieran
