[PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Wolfram Sang]

I recently remembered this old series because I needed to add a dummy to the
DA9063 driver, so I had a look at it. I am somewhat sorry for the long delay,
yet the overload of patches is something I am not really responsible for :/

However, the series v6 from December 2017 was mostly OK. I decided to fix the
things I noticed right away, only minor stuff:

* instead of '__' prefix for the new functions, I added an 'errptr' suffix.
  '__' indicates internal use, mostly unlocked flavors. However, these functions
  could be exported somewhen and then it makes sense to have a more descriptive
  name. This also removes the inconsistency that the devm_* variant returned an
  errptr while the non-devm variant returned NULL. Now, the name makes it clear.

* some minor issues/typos in the kernel doc sections

This series does not include converting 'i2c_new_secondary_device' which I plan
to convert (including all users) to use errptr only. And maybe the same for
'i2c_new_probed_device', I am not sure about it yet.

I added a new user of this series, since at24 has changed a bit since back then.
Patch 3 is only for testing. First, this series needs to be applied.

Tested on a Renesas Lager board (R-Car Gen2).

If the feedback is positive, then I plan to create an immutable branch for the
at24- and MFD-trees after next rc1, so stuff can be based on top of it.

Looking forward to comments,

   Wolfram


Heiner Kallweit (2):
  i2c: core: improve return value handling of i2c_new_device and
    i2c_new_dummy
  i2c: core: add device-managed version of i2c_new_dummy

Wolfram Sang (1):
  mfd: da9063: occupy second I2C address, too

 Documentation/driver-model/devres.txt |   3 +
 drivers/i2c/i2c-core-base.c           | 114 ++++++++++++++++++++++++++++++----
 drivers/mfd/da9063-i2c.c              |   2 +
 include/linux/i2c.h                   |   3 +
 4 files changed, 109 insertions(+), 13 deletions(-)

-- 
2.11.0

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

[Wolfram Sang]

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>
---
 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
+ * 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);
-- 
2.11.0

Re: [PATCH v7 1/3] i2c: core: improve return value handling of i2c_new_device and i2c_new_dummy

[Simon Horman]

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>

Re: [PATCH v7 1/3] i2c: core: improve return value handling of i2c_new_device and i2c_new_dummy

[Kieran Bingham]

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

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

[Wolfram Sang]

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()
+
 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

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

[Peter Rosin]

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

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

[Kieran Bingham]

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
> 

-- 
Regards
--
Kieran

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Rosin]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

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

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

[Peter Rosin]

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

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

[Simon Horman]

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
> 

[PATCH v7 3/3] mfd: da9063: occupy second I2C address, too

[Wolfram Sang]

Even though we don't use it yet, we should mark the second I2C address
this device is listening to as used.

Not yet for upstream until all dependencies are merged!

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mfd/da9063-i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 50a24b1921d0..92ae2681d9e4 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -227,6 +227,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	devm_i2c_new_dummy_errptr(&i2c->dev, i2c->adapter, i2c->addr + 1);
+
 	return da9063_device_init(da9063, i2c->irq);
 }
 
-- 
2.11.0

Re: [PATCH v7 3/3] mfd: da9063: occupy second I2C address, too

[Simon Horman]

On Wed, Mar 13, 2019 at 05:55:26PM +0100, Wolfram Sang wrote:
> Even though we don't use it yet, we should mark the second I2C address
> this device is listening to as used.
> 
> Not yet for upstream until all dependencies are merged!
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> ---
>  drivers/mfd/da9063-i2c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 50a24b1921d0..92ae2681d9e4 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -227,6 +227,8 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
>  
> +	devm_i2c_new_dummy_errptr(&i2c->dev, i2c->adapter, i2c->addr + 1);
> +
>  	return da9063_device_init(da9063, i2c->irq);
>  }
>  
> -- 
> 2.11.0
> 

Re: [PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Bartosz Golaszewski]

śr., 13 mar 2019 o 17:56 Wolfram Sang
<wsa+renesas@sang-engineering.com> napisał(a):
>
> I recently remembered this old series because I needed to add a dummy to the
> DA9063 driver, so I had a look at it. I am somewhat sorry for the long delay,
> yet the overload of patches is something I am not really responsible for :/
>
> However, the series v6 from December 2017 was mostly OK. I decided to fix the
> things I noticed right away, only minor stuff:
>
> * instead of '__' prefix for the new functions, I added an 'errptr' suffix.
>   '__' indicates internal use, mostly unlocked flavors. However, these functions
>   could be exported somewhen and then it makes sense to have a more descriptive
>   name. This also removes the inconsistency that the devm_* variant returned an
>   errptr while the non-devm variant returned NULL. Now, the name makes it clear.
>
> * some minor issues/typos in the kernel doc sections
>
> This series does not include converting 'i2c_new_secondary_device' which I plan
> to convert (including all users) to use errptr only. And maybe the same for
> 'i2c_new_probed_device', I am not sure about it yet.
>
> I added a new user of this series, since at24 has changed a bit since back then.
> Patch 3 is only for testing. First, this series needs to be applied.
>
> Tested on a Renesas Lager board (R-Car Gen2).
>
> If the feedback is positive, then I plan to create an immutable branch for the
> at24- and MFD-trees after next rc1, so stuff can be based on top of it.
>
> Looking forward to comments,
>
>    Wolfram
>
>
> Heiner Kallweit (2):
>   i2c: core: improve return value handling of i2c_new_device and
>     i2c_new_dummy
>   i2c: core: add device-managed version of i2c_new_dummy
>
> Wolfram Sang (1):
>   mfd: da9063: occupy second I2C address, too
>
>  Documentation/driver-model/devres.txt |   3 +
>  drivers/i2c/i2c-core-base.c           | 114 ++++++++++++++++++++++++++++++----
>  drivers/mfd/da9063-i2c.c              |   2 +
>  include/linux/i2c.h                   |   3 +
>  4 files changed, 109 insertions(+), 13 deletions(-)
>
> --
> 2.11.0
>

Wasn't there a patch using this new managed variant in at24 as well?

Bart

Re: [PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]


> Wasn't there a patch using this new managed variant in at24 as well?

My Lager board has no EEPROM, so I skipped that. I was more interested
in the core parts for now.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Bartosz Golaszewski]

śr., 13 mar 2019 o 22:09 Wolfram Sang <wsa@the-dreams.de> napisał(a):
>
>
> > Wasn't there a patch using this new managed variant in at24 as well?
>
> My Lager board has no EEPROM, so I skipped that. I was more interested
> in the core parts for now.
>

No problem. Once you'll have these in your tree, could you provide me
with an immutable branch and I'll try to dig it out for at24 for v5.2?

Bart

> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAlyJcYAACgkQFA3kzBSg
> KbbMeRAAlwHEaXAk4DZcUCvVIUiuz0j9wcNNZXpwpZJsF0+1DAOJ6LpGYRlA4HSM
> RpHMTuuiM1rtgoYrW1cp90KalfqCUBYC0+lAT/yE8CWXRCOYMHUfyhDiArDHj0Sy
> xSsIsj2dDP6FqzyGSKf//Hqfs4HXAm8kooApELrX7cQaMmarIBqf1avbtF5egR2M
> e3jIf21KG6TUhqNw4rpUmj6NOb50XomVKBOgZzTZwPQVdI5YQMxhM0vKX7ck7GYF
> RyNXP3gAdg3GqCHFy4Ot46begqMUqLwbU7Bl/D9gmJWw1Jawl5V/GA3x7p9/81g2
> iii9XLIGzZemIRnETN/XdUwuWRZiIg1WKFZFO7xCsg9VXs1Pa3EuNWLLVU2ytrrZ
> YNKzQFazbucsqVnbmFPjsmkggDynRz7g8mvjIzgBATcthbAiyMyDu660ivzUrv/2
> LG4jl3chGqkvYww6YaA6plJCtXas1rMLLwuqV7gaBQy6ZcoZu2WuJ2tPnz2xiKBo
> m7HSJeK0KfEK7IYvnKDgpMnhsHgKw+RtkngErH4l9DXbH3jJHNFQA4UOVKAnY5fF
> v1BlYCcPMBqegwjDlVHeed7IRx2L2uFHh8QXOHxP2f1R1/BOvTA6L2vcaLTu261U
> Mcftv6MvhsDGdZ4aP4UPJDbNaIIcKzt54KCpIKV62mnxMq+B9ok=
> =2v3C
> -----END PGP SIGNATURE-----

Re: [PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]


> No problem. Once you'll have these in your tree, could you provide me
> with an immutable branch and I'll try to dig it out for at24 for v5.2?

Quoting my cover letter ;)

===

If the feedback is positive, then I plan to create an immutable branch for the
at24- and MFD-trees after next rc1, so stuff can be based on top of it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 0/3] i2c: improve i2c_new_{device|dummy}

[Bartosz Golaszewski]

śr., 13 mar 2019 o 22:19 Wolfram Sang <wsa@the-dreams.de> napisał(a):
>
>
> > No problem. Once you'll have these in your tree, could you provide me
> > with an immutable branch and I'll try to dig it out for at24 for v5.2?
>
> Quoting my cover letter ;)
>
> ===
>
> If the feedback is positive, then I plan to create an immutable branch for the
> at24- and MFD-trees after next rc1, so stuff can be based on top of it.
>

Point taken. Thanks!

Bart

> -----BEGIN PGP SIGNATURE-----
>
> iQIzBAABCgAdFiEEOZGx6rniZ1Gk92RdFA3kzBSgKbYFAlyJc+oACgkQFA3kzBSg
> KbaZNw//QKa58jcJaRbJYvmnw3QOtzrIrRK4TI9l6dVM6nb9gtcDtS1oJM6qvE9s
> PthN6nNOBj9+TOepD34gTgnfal+QhafC23SjziETrfAE/N57seKttJZW+P2JOKhW
> LLIkYIU8tmC68PLOxNTLbk8yG63n8z2527ICzF1Jp06v4RDP2FMntMRvaD5d5M48
> mJMrNCR9nzX52bk5lXu1zw4Tdh4vj3IwRFa2uUUA+dS7EYt82cRT6pq8BiGk/Svg
> NaySKqXjiNDHG8EQLUuQF7e30LWo+D+JCqxG0E+ir1rINlw9dJkzQSA49UmMbwn0
> jwwCFoM15ugVDAohdI64Pj5FiNGMl5zt2Fw3Ude0G+n5SN1BKRG2v947OvCJtYra
> Yd3PBT7CfubqfZHK4cF9SqGTsavHBYACIzOCjHCAnZDsrNEANCFuDcFUla94WQsa
> USMBw/BnEI6rpPlCgUA5n0NrcGResj3g9GJXxD7x0JKSjKyBXBeApZUx4tEWa2m8
> tc+JAHJ136BFDI7zaRgi2LDqFBvYwZpvJt0ttc9uA1B3u3UecG2QHBjp6XdTc7qM
> aDhXSYTxBXP6fJeFI3cf4aIwN9HqsWNwJDnG3I+/9ODiwvtG7IQFjxXJ+dSGSrlw
> 0sAWsPg3mavtEVjSYBLuUDZAchHGTULpjf/38iTx2D6BmmtX3S4=
> =Rd2y
> -----END PGP SIGNATURE-----