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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux