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 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
