diff mbox series

[v8,1/3] i2c: core: improve return value handling of i2c_new_device and i2c_new_dummy

Message ID 20190516211310.22277-2-wsa+renesas@sang-engineering.com (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: improve i2c_new_{device|dummy} | expand

Commit Message

Wolfram Sang May 16, 2019, 9:13 p.m. UTC 
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 and fix minor kdoc issues]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 74 ++++++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 13 deletions(-)

Comments

Peter Rosin May 17, 2019, 7:16 a.m. UTC | #1 
On 2019-05-16 23:13, 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 and fix minor kdoc issues]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

The only nit I can find is that you could perhaps sweep the patches
with sed 's/i2c /I2C /'. But that is indeed a nit, so if you're in
a hurry...

Reviewed-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter
Kieran Bingham May 17, 2019, 8:02 a.m. UTC | #2 
Hi Wolfram,

On 16/05/2019 22:13, 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 and fix minor kdoc issues]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


I like the new naming a lot more this time :-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/i2c/i2c-core-base.c | 74 ++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9732a81bb7dd..9c38dde73366 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -714,7 +714,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources,
>  }
>  
>  /**
> - * i2c_new_device - instantiate an i2c device
> + * i2c_new_client_device - instantiate an i2c device
>   * @adap: the adapter managing the device
>   * @info: describes one I2C device; bus_num is ignored
>   * Context: can sleep
> @@ -727,17 +727,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_client_device(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;
>  
> @@ -803,7 +803,31 @@ 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);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_client_device);
> +
> +/**
> + * 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 deprecated function has the same functionality as
> + * @i2c_new_client_device, it just returns NULL instead of an ERR_PTR in case of
> + * an error for compatibility with current I2C API. It will be removed once all
> + * users are converted.
> + *
> + * 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_client_device(adap, info);
> +	return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_device);
>  
> @@ -854,7 +878,7 @@ static struct i2c_driver dummy_driver = {
>  };
>  
>  /**
> - * i2c_new_dummy - return a new i2c device bound to a dummy driver
> + * i2c_new_dummy_device - 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
> @@ -869,15 +893,39 @@ 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_device(struct i2c_adapter *adapter, u16 address)
>  {
>  	struct i2c_board_info info = {
>  		I2C_BOARD_INFO("dummy", address),

I wonder if in the future we could find an easy way to propagate a
device name into this 'dummy' info field for dummy devices.

Then it would be much easier to identify all of the rogue dummys that
appear in my system (ok, not rogue - I've put them there - but I've put
in sooo many :D)

Anyway - just musings - not a topic for this patch.

>  	};
>  
> -	return i2c_new_device(adapter, &info);
> +	return i2c_new_client_device(adapter, &info);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_dummy_device);
> +
> +/**
> + * 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 deprecated function has the same functionality as @i2c_new_dummy_device,
> + * it just returns NULL instead of an ERR_PTR in case of an error for
> + * compatibility with current I2C API. It will be removed once all users are
> + * converted.
> + *
> + * 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_device(adapter, address);
> +	return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>  
> @@ -1000,9 +1048,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_client_device(adap, &info);
> +	if (IS_ERR(client))
> +		return PTR_ERR(client);
>  
>  	/* Keep track of the added device */
>  	mutex_lock(&adap->userspace_clients_lock);
>
Bartosz Golaszewski May 17, 2019, 8:56 a.m. UTC | #3 
czw., 16 maj 2019 o 23:13 Wolfram Sang
<wsa+renesas@sang-engineering.com> napisał(a):
>
> 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 and fix minor kdoc issues]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 74 ++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9732a81bb7dd..9c38dde73366 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -714,7 +714,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources,
>  }
>
>  /**
> - * i2c_new_device - instantiate an i2c device
> + * i2c_new_client_device - instantiate an i2c device
>   * @adap: the adapter managing the device
>   * @info: describes one I2C device; bus_num is ignored
>   * Context: can sleep
> @@ -727,17 +727,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_client_device(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;
>
> @@ -803,7 +803,31 @@ 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);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_client_device);
> +
> +/**
> + * 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 deprecated function has the same functionality as
> + * @i2c_new_client_device, it just returns NULL instead of an ERR_PTR in case of
> + * an error for compatibility with current I2C API. It will be removed once all
> + * users are converted.
> + *
> + * 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_client_device(adap, info);
> +       return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_device);
>
> @@ -854,7 +878,7 @@ static struct i2c_driver dummy_driver = {
>  };
>
>  /**
> - * i2c_new_dummy - return a new i2c device bound to a dummy driver
> + * i2c_new_dummy_device - 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
> @@ -869,15 +893,39 @@ 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_device(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_client_device(adapter, &info);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_dummy_device);
> +
> +/**
> + * 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 deprecated function has the same functionality as @i2c_new_dummy_device,
> + * it just returns NULL instead of an ERR_PTR in case of an error for
> + * compatibility with current I2C API. It will be removed once all users are
> + * converted.
> + *
> + * 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_device(adapter, address);
> +       return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>
> @@ -1000,9 +1048,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_client_device(adap, &info);
> +       if (IS_ERR(client))
> +               return PTR_ERR(client);
>
>         /* Keep track of the added device */
>         mutex_lock(&adap->userspace_clients_lock);
> --
> 2.19.1
>

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Wolfram Sang May 17, 2019, 5:35 p.m. UTC | #4 
> The only nit I can find is that you could perhaps sweep the patches
> with sed 's/i2c /I2C /'. But that is indeed a nit, so if you're in
> a hurry...

Valid point. I'd like to fix it as a seperate patch, though. One reason
is that it is needed in far more places in this file, too. The main
reason is that I don't want to introduce some stupid last minute typo
after all these reviews.

> Reviewed-by: Peter Rosin <peda@axentia.se>

Thanks!
Wolfram Sang May 17, 2019, 5:35 p.m. UTC | #5 
On Thu, May 16, 2019 at 11:13:08PM +0200, 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 and fix minor kdoc issues]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks everyone!
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9732a81bb7dd..9c38dde73366 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -714,7 +714,7 @@  static int i2c_dev_irq_from_resources(const struct resource *resources,
 }
 
 /**
- * i2c_new_device - instantiate an i2c device
+ * i2c_new_client_device - instantiate an i2c device
  * @adap: the adapter managing the device
  * @info: describes one I2C device; bus_num is ignored
  * Context: can sleep
@@ -727,17 +727,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_client_device(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;
 
@@ -803,7 +803,31 @@  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);
+}
+EXPORT_SYMBOL_GPL(i2c_new_client_device);
+
+/**
+ * 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 deprecated function has the same functionality as
+ * @i2c_new_client_device, it just returns NULL instead of an ERR_PTR in case of
+ * an error for compatibility with current I2C API. It will be removed once all
+ * users are converted.
+ *
+ * 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_client_device(adap, info);
+	return IS_ERR(ret) ? NULL : ret;
 }
 EXPORT_SYMBOL_GPL(i2c_new_device);
 
@@ -854,7 +878,7 @@  static struct i2c_driver dummy_driver = {
 };
 
 /**
- * i2c_new_dummy - return a new i2c device bound to a dummy driver
+ * i2c_new_dummy_device - 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
@@ -869,15 +893,39 @@  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_device(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_client_device(adapter, &info);
+}
+EXPORT_SYMBOL_GPL(i2c_new_dummy_device);
+
+/**
+ * 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 deprecated function has the same functionality as @i2c_new_dummy_device,
+ * it just returns NULL instead of an ERR_PTR in case of an error for
+ * compatibility with current I2C API. It will be removed once all users are
+ * converted.
+ *
+ * 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_device(adapter, address);
+	return IS_ERR(ret) ? NULL : ret;
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
@@ -1000,9 +1048,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_client_device(adap, &info);
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	/* Keep track of the added device */
 	mutex_lock(&adap->userspace_clients_lock);