| Message ID | 20190313165526.28588-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i2c: improve i2c_new_{device|dummy} | expand |
Hi! A comment from the peanut gallery... On 2019-03-13 17:55, Wolfram Sang wrote: > From: Heiner Kallweit <hkallweit1@gmail.com> > > 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@gmail.com> > [wsa: rename new functions to 'errptr' style and fix minor kdoc issues] > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > 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
On Wed, Mar 13, 2019 at 05:55:25PM +0100, Wolfram Sang wrote: > From: Heiner Kallweit <hkallweit1@gmail.com> > > 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@gmail.com> > [wsa: rename new functions to 'errptr' style and fix minor kdoc issues] > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > 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() > + > IIO > devm_iio_device_alloc() > devm_iio_device_free() > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index bbca2e8bb019..7f20c6b8857f 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -921,6 +921,50 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) > } > EXPORT_SYMBOL_GPL(i2c_new_dummy); > > +struct i2c_dummy_devres { > + struct i2c_client *client; > +}; > + > +static void devm_i2c_release_dummy(struct device *dev, void *res) > +{ > + struct i2c_dummy_devres *this = res; > + > + i2c_unregister_device(this->client); > +} > + > +/** > + * devm_i2c_new_dummy_errptr - return a new i2c device bound to a dummy driver > + * @dev: device the managed resource is bound to > + * @adapter: the adapter managing the device > + * @address: seven bit address to be used > + * Context: can sleep > + * > + * This is the device-managed version of @i2c_new_dummy. It returns the new i2c > + * client or an ERR_PTR in case of an error. > + */ > +struct i2c_client *devm_i2c_new_dummy_errptr(struct device *dev, > + struct i2c_adapter *adapter, > + u16 address) > +{ > + struct i2c_dummy_devres *dr; > + struct i2c_client *client; > + > + dr = devres_alloc(devm_i2c_release_dummy, sizeof(*dr), GFP_KERNEL); > + if (!dr) > + return ERR_PTR(-ENOMEM); > + > + client = i2c_new_dummy_errptr(adapter, address); > + if (IS_ERR(client)) { > + devres_free(dr); > + } else { > + dr->client = client; > + devres_add(dev, dr); > + } > + > + return client; > +} > +EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_errptr); > + > /** > * i2c_new_secondary_device - Helper to get the instantiated secondary address > * and create the associated device > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 383510b4f083..93075c2ad9dc 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -470,6 +470,9 @@ extern struct i2c_client * > i2c_new_dummy(struct i2c_adapter *adap, u16 address); > > extern struct i2c_client * > +devm_i2c_new_dummy_errptr(struct device *dev, struct i2c_adapter *adap, u16 address); > + > +extern struct i2c_client * > i2c_new_secondary_device(struct i2c_client *client, > const char *name, > u16 default_addr); > -- > 2.11.0 >
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@gmail.com> >> >> 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@gmail.com> >> [wsa: rename new functions to 'errptr' style and fix minor kdoc issues] >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> 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 >
> > 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. As I said to Peter, I am not exactly happy with the naming. I just couldn't come up with something better. I am totally open for suggestions here. Let's give ourselves a few days. Maybe inspiration will hit one of us somehow somewhen.
On 2019-03-15 22:08, 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. >> >> >> 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. > > As I said to Peter, I am not exactly happy with the naming. I just > couldn't come up with something better. I am totally open for > suggestions here. Let's give ourselves a few days. Maybe inspiration > will hit one of us somehow somewhen. I can't seem to find what you said to me anywhere, it's not in my inbox and I don't see any (other) reply from you on patchwork for this patch either. Perhaps a resend is in order? Cheers, Peter
> 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. > 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 Those are likely prime candidates to convert to devm_*. > 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. Yes. But it is quite some work, even more with i2c_new_device. > 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. OK, I can see that. If the longterm goal is to return errnos, then it doesn't make sense to have it in the function name. This is valid criticism. Yet, I still keep the other criticism from the first paragraph where i2c_new_dummy and i2c_register_dummy is confusing. Unless someone comes up with a solution we all overlooked, it will probably be chosing the lesser evil.
> I can't seem to find what you said to me anywhere, it's not in my inbox and > I don't see any (other) reply from you on patchwork for this patch either. Oops, my fault, sorry. I couldn't send it when I was on the train, so it was still in my Drafts folder. Sent now.
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
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() + IIO devm_iio_device_alloc() devm_iio_device_free() diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index bbca2e8bb019..7f20c6b8857f 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -921,6 +921,50 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) } EXPORT_SYMBOL_GPL(i2c_new_dummy); +struct i2c_dummy_devres { + struct i2c_client *client; +}; + +static void devm_i2c_release_dummy(struct device *dev, void *res) +{ + struct i2c_dummy_devres *this = res; + + i2c_unregister_device(this->client); +} + +/** + * devm_i2c_new_dummy_errptr - return a new i2c device bound to a dummy driver + * @dev: device the managed resource is bound to + * @adapter: the adapter managing the device + * @address: seven bit address to be used + * Context: can sleep + * + * This is the device-managed version of @i2c_new_dummy. It returns the new i2c + * client or an ERR_PTR in case of an error. + */ +struct i2c_client *devm_i2c_new_dummy_errptr(struct device *dev, + struct i2c_adapter *adapter, + u16 address) +{ + struct i2c_dummy_devres *dr; + struct i2c_client *client; + + dr = devres_alloc(devm_i2c_release_dummy, sizeof(*dr), GFP_KERNEL); + if (!dr) + return ERR_PTR(-ENOMEM); + + client = i2c_new_dummy_errptr(adapter, address); + if (IS_ERR(client)) { + devres_free(dr); + } else { + dr->client = client; + devres_add(dev, dr); + } + + return client; +} +EXPORT_SYMBOL_GPL(devm_i2c_new_dummy_errptr); + /** * i2c_new_secondary_device - Helper to get the instantiated secondary address * and create the associated device diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 383510b4f083..93075c2ad9dc 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -470,6 +470,9 @@ extern struct i2c_client * i2c_new_dummy(struct i2c_adapter *adap, u16 address); extern struct i2c_client * +devm_i2c_new_dummy_errptr(struct device *dev, struct i2c_adapter *adap, u16 address); + +extern struct i2c_client * i2c_new_secondary_device(struct i2c_client *client, const char *name, u16 default_addr);