| Message ID | 20190313165526.28588-2-wsa+renesas@sang-engineering.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: improve i2c_new_{device|dummy} | expand |
On Wed, Mar 13, 2019 at 05:55:24PM +0100, Wolfram Sang wrote: > From: Heiner Kallweit <hkallweit1@gmail.com> > > Currently i2c_new_device and i2c_new_dummy return just NULL in error > case although they have more error details internally. Therefore move > the functionality into new functions returning detailed errors and > add wrappers for compatibility with the current API. > > This allows to use these functions with detailed error codes within > the i2c core or for API extensions. > > 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>
Hi Wolfram, On 13/03/2019 16:55, Wolfram Sang wrote: > From: Heiner Kallweit <hkallweit1@gmail.com> > > Currently i2c_new_device and i2c_new_dummy return just NULL in error > case although they have more error details internally. Therefore move > the functionality into new functions returning detailed errors and > add wrappers for compatibility with the current API. > > This allows to use these functions with detailed error codes within > the i2c core or for API extensions. > > 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> As mentioned in 2/3, the _errptr suffix here feels a bit 'hungarian-notation', but I can see how we would want a good clear distinction between the two types, otherwise we might see people adding code that checks for ERR_PTR() using the non-errptr functions. Perhaps that's not really an issue if it's caught by review, or coccinelle though. Aside from the function-name-bike-shedding which can be handled as you wished/discussed in 2/3, and one small grammatical fix in a comment below: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/i2c/i2c-core-base.c | 70 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 57 insertions(+), 13 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index cb6c5cb0df0b..bbca2e8bb019 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -710,7 +710,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources, > } > > /** > - * i2c_new_device - instantiate an i2c device > + * i2c_new_device_errptr - instantiate an i2c device > * @adap: the adapter managing the device > * @info: describes one I2C device; bus_num is ignored > * Context: can sleep > @@ -723,17 +723,17 @@ static int i2c_dev_irq_from_resources(const struct resource *resources, > * before any i2c_adapter could exist. > * > * This returns the new i2c client, which may be saved for later use with > - * i2c_unregister_device(); or NULL to indicate an error. > + * i2c_unregister_device(); or an ERR_PTR to describe the error. > */ > -struct i2c_client * > -i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > +static struct i2c_client * > +i2c_new_device_errptr(struct i2c_adapter *adap, struct i2c_board_info const *info) > { > struct i2c_client *client; > int status; > > client = kzalloc(sizeof *client, GFP_KERNEL); > if (!client) > - return NULL; > + return ERR_PTR(-ENOMEM); > > client->adapter = adap; > > @@ -799,7 +799,29 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > client->name, client->addr, status); > out_err_silent: > kfree(client); > - return NULL; > + return ERR_PTR(status); > +} > + > +/** > + * i2c_new_device - instantiate an i2c device > + * @adap: the adapter managing the device > + * @info: describes one I2C device; bus_num is ignored > + * Context: can sleep > + * > + * This function has the same functionality like i2c_new_device_errptr, it just s/like/as/ > + * returns NULL instead of an ERR_PTR in case of an error for compatibility > + * with current I2C API. > + * > + * This returns the new i2c client, which may be saved for later use with > + * i2c_unregister_device(); or NULL to indicate an error. > + */ > +struct i2c_client * > +i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) > +{ > + struct i2c_client *ret; > + > + ret = i2c_new_device_errptr(adap, info); > + return IS_ERR(ret) ? NULL : ret; > } > EXPORT_SYMBOL_GPL(i2c_new_device); > > @@ -850,7 +872,7 @@ static struct i2c_driver dummy_driver = { > }; > > /** > - * i2c_new_dummy - return a new i2c device bound to a dummy driver > + * i2c_new_dummy_errptr - return a new i2c device bound to a dummy driver > * @adapter: the adapter managing the device > * @address: seven bit address to be used > * Context: can sleep > @@ -865,15 +887,37 @@ static struct i2c_driver dummy_driver = { > * different driver. > * > * This returns the new i2c client, which should be saved for later use with > - * i2c_unregister_device(); or NULL to indicate an error. > + * i2c_unregister_device(); or an ERR_PTR to describe the error. > */ > -struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) > +static struct i2c_client * > +i2c_new_dummy_errptr(struct i2c_adapter *adapter, u16 address) > { > struct i2c_board_info info = { > I2C_BOARD_INFO("dummy", address), > }; > > - return i2c_new_device(adapter, &info); > + return i2c_new_device_errptr(adapter, &info); > +} > + > +/** > + * i2c_new_dummy - return a new i2c device bound to a dummy driver > + * @adapter: the adapter managing the device > + * @address: seven bit address to be used > + * Context: can sleep > + * > + * This function has the same functionality like i2c_new_dummy_errptr, it just > + * returns NULL instead of an ERR_PTR in case of an error for compatibility > + * with current I2C API. > + * > + * This returns the new i2c client, which should be saved for later use with > + * i2c_unregister_device(); or NULL to indicate an error. > + */ > +struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) > +{ > + struct i2c_client *ret; > + > + ret = i2c_new_dummy_errptr(adapter, address); > + return IS_ERR(ret) ? NULL : ret; > } > EXPORT_SYMBOL_GPL(i2c_new_dummy); > > @@ -996,9 +1040,9 @@ i2c_sysfs_new_device(struct device *dev, struct device_attribute *attr, > info.flags |= I2C_CLIENT_SLAVE; > } > > - client = i2c_new_device(adap, &info); > - if (!client) > - return -EINVAL; > + client = i2c_new_device_errptr(adap, &info); > + if (IS_ERR(client)) > + return PTR_ERR(client); > > /* Keep track of the added device */ > mutex_lock(&adap->userspace_clients_lock); >
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index cb6c5cb0df0b..bbca2e8bb019 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -710,7 +710,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources, } /** - * i2c_new_device - instantiate an i2c device + * i2c_new_device_errptr - instantiate an i2c device * @adap: the adapter managing the device * @info: describes one I2C device; bus_num is ignored * Context: can sleep @@ -723,17 +723,17 @@ static int i2c_dev_irq_from_resources(const struct resource *resources, * before any i2c_adapter could exist. * * This returns the new i2c client, which may be saved for later use with - * i2c_unregister_device(); or NULL to indicate an error. + * i2c_unregister_device(); or an ERR_PTR to describe the error. */ -struct i2c_client * -i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) +static struct i2c_client * +i2c_new_device_errptr(struct i2c_adapter *adap, struct i2c_board_info const *info) { struct i2c_client *client; int status; client = kzalloc(sizeof *client, GFP_KERNEL); if (!client) - return NULL; + return ERR_PTR(-ENOMEM); client->adapter = adap; @@ -799,7 +799,29 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) client->name, client->addr, status); out_err_silent: kfree(client); - return NULL; + return ERR_PTR(status); +} + +/** + * i2c_new_device - instantiate an i2c device + * @adap: the adapter managing the device + * @info: describes one I2C device; bus_num is ignored + * Context: can sleep + * + * This function has the same functionality like i2c_new_device_errptr, it just + * returns NULL instead of an ERR_PTR in case of an error for compatibility + * with current I2C API. + * + * This returns the new i2c client, which may be saved for later use with + * i2c_unregister_device(); or NULL to indicate an error. + */ +struct i2c_client * +i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info) +{ + struct i2c_client *ret; + + ret = i2c_new_device_errptr(adap, info); + return IS_ERR(ret) ? NULL : ret; } EXPORT_SYMBOL_GPL(i2c_new_device); @@ -850,7 +872,7 @@ static struct i2c_driver dummy_driver = { }; /** - * i2c_new_dummy - return a new i2c device bound to a dummy driver + * i2c_new_dummy_errptr - return a new i2c device bound to a dummy driver * @adapter: the adapter managing the device * @address: seven bit address to be used * Context: can sleep @@ -865,15 +887,37 @@ static struct i2c_driver dummy_driver = { * different driver. * * This returns the new i2c client, which should be saved for later use with - * i2c_unregister_device(); or NULL to indicate an error. + * i2c_unregister_device(); or an ERR_PTR to describe the error. */ -struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) +static struct i2c_client * +i2c_new_dummy_errptr(struct i2c_adapter *adapter, u16 address) { struct i2c_board_info info = { I2C_BOARD_INFO("dummy", address), }; - return i2c_new_device(adapter, &info); + return i2c_new_device_errptr(adapter, &info); +} + +/** + * i2c_new_dummy - return a new i2c device bound to a dummy driver + * @adapter: the adapter managing the device + * @address: seven bit address to be used + * Context: can sleep + * + * This function has the same functionality like i2c_new_dummy_errptr, it just + * returns NULL instead of an ERR_PTR in case of an error for compatibility + * with current I2C API. + * + * This returns the new i2c client, which should be saved for later use with + * i2c_unregister_device(); or NULL to indicate an error. + */ +struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address) +{ + struct i2c_client *ret; + + ret = i2c_new_dummy_errptr(adapter, address); + return IS_ERR(ret) ? NULL : ret; } EXPORT_SYMBOL_GPL(i2c_new_dummy); @@ -996,9 +1040,9 @@ i2c_sysfs_new_device(struct device *dev, struct device_attribute *attr, info.flags |= I2C_CLIENT_SLAVE; } - client = i2c_new_device(adap, &info); - if (!client) - return -EINVAL; + client = i2c_new_device_errptr(adap, &info); + if (IS_ERR(client)) + return PTR_ERR(client); /* Keep track of the added device */ mutex_lock(&adap->userspace_clients_lock);