[PATCH v4 0/3] i2c: introduce devm_i2c_new_dummy and use it in at24 driver

[PATCH v4 0/3] i2c: introduce devm_i2c_new_dummy and use it in at24 driver

[Heiner Kallweit]

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.

Make at24 driver the first user of the new function.

Changes in v2:
- add change to i2c core to make a version of i2c_new_device
  available which returns an ERR_PTR instead of NULL in error case
- few minor improvements

Changes in v3:
- rename _i2c_new_device to __i2c_new_device

Changes in v4:
- add missing kernel doc comments
- add Reviewed-by

Heiner Kallweit (3):
  i2c: core: improve return value handling of i2c_new_device and i2c_new_dummy
  i2c: core: add device-managed version of i2c_new_dummy
  eeprom: at24: switch to device-managed version of i2c_new_dummy

 Documentation/driver-model/devres.txt |   3 +
 drivers/i2c/i2c-core-base.c           | 108 ++++++++++++++++++++++++++++++----
 drivers/misc/eeprom/at24.c            |  31 ++++------
 include/linux/i2c.h                   |   3 +
 4 files changed, 112 insertions(+), 33 deletions(-)

-- 
2.15.1

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

[Heiner Kallweit]

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 compatibilty 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>
Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
v3:
- prefix i2c_new_device and i2c_new_dummy with two underscores
  instead one
v4:
- add missing kernel doc
- add reviewed-by
---
 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 bb34a5d41..3b962456c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -656,7 +656,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources,
 }
 
 /**
- * i2c_new_device - instantiate an i2c 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
@@ -669,17 +669,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 indicate an error.
  */
-struct i2c_client *
-i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
+static struct i2c_client *
+__i2c_new_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;
 
@@ -746,7 +746,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 __i2_new_device, 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(adap, info);
+	return IS_ERR(ret) ? NULL : ret;
 }
 EXPORT_SYMBOL_GPL(i2c_new_device);
 
@@ -793,7 +815,7 @@ static struct i2c_driver dummy_driver = {
 };
 
 /**
- * i2c_new_dummy - return a new i2c device bound to a dummy driver
+ * __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
@@ -808,15 +830,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 indicate an error.
  */
-struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
+static struct i2c_client *
+__i2c_new_dummy(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(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 __i2_new_dummy, 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 an ERR_PTR to indicate an error.
+ */
+struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
+{
+	struct i2c_client *ret;
+
+	ret = __i2c_new_dummy(adapter, address);
+	return IS_ERR(ret) ? NULL : ret;
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
@@ -939,9 +983,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(adap, &info);
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	/* Keep track of the added device */
 	mutex_lock(&adap->userspace_clients_lock);
-- 
2.15.1

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

[Bartosz Golaszewski]

2017-12-15 18:43 GMT+01:00 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 compatibilty 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>
> Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>

Hi Heiner,

I only Reviewed-by this patch, not the whole series. I always
explicitly state it if I do. Patch 3/3 looks good but since it's at24,
it's probably going to go through my tree anyway (possibly with the
rest of this series), so no need for the tag.

> ---
> v3:
> - prefix i2c_new_device and i2c_new_dummy with two underscores
>   instead one
> v4:
> - add missing kernel doc
> - add reviewed-by
> ---
>  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 bb34a5d41..3b962456c 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -656,7 +656,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources,
>  }
>
>  /**
> - * i2c_new_device - instantiate an i2c 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
> @@ -669,17 +669,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 indicate an error.
>   */
> -struct i2c_client *
> -i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> +static struct i2c_client *
> +__i2c_new_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;
>
> @@ -746,7 +746,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 __i2_new_device, 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(adap, info);
> +       return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_device);
>
> @@ -793,7 +815,7 @@ static struct i2c_driver dummy_driver = {
>  };
>
>  /**
> - * i2c_new_dummy - return a new i2c device bound to a dummy driver
> + * __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
> @@ -808,15 +830,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 indicate an error.
>   */
> -struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
> +static struct i2c_client *
> +__i2c_new_dummy(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(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 __i2_new_dummy, 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 an ERR_PTR to indicate an error.

You're contradicting the previous paragraph here.

Thanks,
Bartosz

> + */
> +struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
> +{
> +       struct i2c_client *ret;
> +
> +       ret = __i2c_new_dummy(adapter, address);
> +       return IS_ERR(ret) ? NULL : ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>
> @@ -939,9 +983,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(adap, &info);
> +       if (IS_ERR(client))
> +               return PTR_ERR(client);
>
>         /* Keep track of the added device */
>         mutex_lock(&adap->userspace_clients_lock);
> --
> 2.15.1
>
>

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

[Heiner Kallweit]

Am 15.12.2017 um 23:02 schrieb Bartosz Golaszewski:
> 2017-12-15 18:43 GMT+01:00 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 compatibilty 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>
>> Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
> 
> Hi Heiner,
> 
> I only Reviewed-by this patch, not the whole series. I always
> explicitly state it if I do. Patch 3/3 looks good but since it's at24,
> it's probably going to go through my tree anyway (possibly with the
> rest of this series), so no need for the tag.
> 
Sorry, I misunderstood you here.

>> ---
>> v3:
>> - prefix i2c_new_device and i2c_new_dummy with two underscores
>>   instead one
>> v4:
>> - add missing kernel doc
>> - add reviewed-by
>> ---
>>  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 bb34a5d41..3b962456c 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -656,7 +656,7 @@ static int i2c_dev_irq_from_resources(const struct resource *resources,
>>  }
>>
>>  /**
>> - * i2c_new_device - instantiate an i2c 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
>> @@ -669,17 +669,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 indicate an error.
>>   */
>> -struct i2c_client *
>> -i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>> +static struct i2c_client *
>> +__i2c_new_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;
>>
>> @@ -746,7 +746,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 __i2_new_device, 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(adap, info);
>> +       return IS_ERR(ret) ? NULL : ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_new_device);
>>
>> @@ -793,7 +815,7 @@ static struct i2c_driver dummy_driver = {
>>  };
>>
>>  /**
>> - * i2c_new_dummy - return a new i2c device bound to a dummy driver
>> + * __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
>> @@ -808,15 +830,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 indicate an error.
>>   */
>> -struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
>> +static struct i2c_client *
>> +__i2c_new_dummy(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(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 __i2_new_dummy, 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 an ERR_PTR to indicate an error.
> 
> You're contradicting the previous paragraph here.
> 
Right, copy & paste error ..

> Thanks,
> Bartosz
> 
>> + */
>> +struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
>> +{
>> +       struct i2c_client *ret;
>> +
>> +       ret = __i2c_new_dummy(adapter, address);
>> +       return IS_ERR(ret) ? NULL : ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>>
>> @@ -939,9 +983,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(adap, &info);
>> +       if (IS_ERR(client))
>> +               return PTR_ERR(client);
>>
>>         /* Keep track of the added device */
>>         mutex_lock(&adap->userspace_clients_lock);
>> --
>> 2.15.1
>>
>>
> 

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

[Heiner Kallweit]

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>
Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
v2:
- use new function _i2c_new_dummy with detailed error codes
v3:
- no changes
v4:
- reflect renaming to __i2c_new_dummy
- add reviewed-by
---
 Documentation/driver-model/devres.txt |  3 +++
 drivers/i2c/i2c-core-base.c           | 38 +++++++++++++++++++++++++++++++++++
 include/linux/i2c.h                   |  3 +++
 3 files changed, 44 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index c180045eb..6e2bccf85 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -259,6 +259,9 @@ GPIO
   devm_gpio_request_one()
   devm_gpio_free()
 
+I2C
+  devm_i2c_new_dummy
+
 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 3b962456c..7a6669f07 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -864,6 +864,44 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
+static void devm_i2c_release_dummy(struct device *dev, void *res)
+{
+	i2c_unregister_device(*(struct i2c_client **)res);
+}
+
+/**
+ * devm_i2c_new_dummy - 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.
+ * Note the changed return value type: It returns the new i2c client
+ * or an ERR_PTR in case of an error.
+ */
+struct i2c_client *devm_i2c_new_dummy(struct device *dev,
+				      struct i2c_adapter *adapter,
+				      u16 address)
+{
+	struct i2c_client *ret, **dr;
+
+	dr = devres_alloc(devm_i2c_release_dummy, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return ERR_PTR(-ENOMEM);
+
+	ret = __i2c_new_dummy(adapter, address);
+	if (IS_ERR(ret)) {
+		devres_free(dr);
+	} else {
+		*dr = ret;
+		devres_add(dev, dr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_i2c_new_dummy);
+
 /**
  * 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 5d7f3c185..aca6ebbb8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -441,6 +441,9 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
 extern struct i2c_client *
 i2c_new_dummy(struct i2c_adapter *adap, u16 address);
 
+extern struct i2c_client *
+devm_i2c_new_dummy(struct device *dev, struct i2c_adapter *adap, u16 address);
+
 extern struct i2c_client *
 i2c_new_secondary_device(struct i2c_client *client,
 				const char *name,
-- 
2.15.1

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

[Bartosz Golaszewski]

2017-12-15 18:44 GMT+01:00 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>
> Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
> v2:
> - use new function _i2c_new_dummy with detailed error codes
> v3:
> - no changes
> v4:
> - reflect renaming to __i2c_new_dummy
> - add reviewed-by
> ---
>  Documentation/driver-model/devres.txt |  3 +++
>  drivers/i2c/i2c-core-base.c           | 38 +++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h                   |  3 +++
>  3 files changed, 44 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index c180045eb..6e2bccf85 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -259,6 +259,9 @@ GPIO
>    devm_gpio_request_one()
>    devm_gpio_free()
>
> +I2C
> +  devm_i2c_new_dummy
> +
>  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 3b962456c..7a6669f07 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -864,6 +864,44 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>
> +static void devm_i2c_release_dummy(struct device *dev, void *res)
> +{
> +       i2c_unregister_device(*(struct i2c_client **)res);
> +}
> +
> +/**
> + * devm_i2c_new_dummy - 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.
> + * Note the changed return value type: It returns the new i2c client
> + * or an ERR_PTR in case of an error.
> + */
> +struct i2c_client *devm_i2c_new_dummy(struct device *dev,
> +                                     struct i2c_adapter *adapter,
> +                                     u16 address)
> +{
> +       struct i2c_client *ret, **dr;
> +
> +       dr = devres_alloc(devm_i2c_release_dummy, sizeof(*dr), GFP_KERNEL);
> +       if (!dr)
> +               return ERR_PTR(-ENOMEM);
> +

While I understand that this pattern has been used before I still
think it's much less readable than using a separate devm struct
containing the i2c_client. Please take a look at kernel/irq/irq_sim.c:
while I may be biased since I added that code, I think it's cleaner to
avoid the casts and double pointers. Remember - we prefer readability
over saving three lines of code (and probably none in its compiled
form).

Thanks,
Bartosz

> +       ret = __i2c_new_dummy(adapter, address);
> +       if (IS_ERR(ret)) {
> +               devres_free(dr);
> +       } else {
> +               *dr = ret;
> +               devres_add(dev, dr);
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_i2c_new_dummy);
> +
>  /**
>   * 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 5d7f3c185..aca6ebbb8 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -441,6 +441,9 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
>  extern struct i2c_client *
>  i2c_new_dummy(struct i2c_adapter *adap, u16 address);
>
> +extern struct i2c_client *
> +devm_i2c_new_dummy(struct device *dev, struct i2c_adapter *adap, u16 address);
> +
>  extern struct i2c_client *
>  i2c_new_secondary_device(struct i2c_client *client,
>                                 const char *name,
> --
> 2.15.1
>
>

[PATCH v4 3/3] eeprom: at24: switch to device-managed version of i2c_new_dummy

[Heiner Kallweit]

Make use of recently introduced device-managed version of
i2c_new_dummy to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
v2:
- small improvements regarding code readability
v3:
- no changes
v4:
- no changes
---
 drivers/misc/eeprom/at24.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 625b00166..5f3a27a69 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -621,20 +621,19 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	/* use dummy devices for multiple-address chips */
 	for (i = 1; i < num_addresses; i++) {
-		at24->client[i].client = i2c_new_dummy(client->adapter,
-						       client->addr + i);
-		if (!at24->client[i].client) {
+		struct at24_client *cl;
+
+		cl = &at24->client[i];
+		cl->client = devm_i2c_new_dummy(&client->dev, client->adapter,
+						client->addr + i);
+		if (IS_ERR(cl->client)) {
 			dev_err(&client->dev, "address 0x%02x unavailable\n",
-					client->addr + i);
-			err = -EADDRINUSE;
-			goto err_clients;
-		}
-		at24->client[i].regmap = devm_regmap_init_i2c(
-					at24->client[i].client, config);
-		if (IS_ERR(at24->client[i].regmap)) {
-			err = PTR_ERR(at24->client[i].regmap);
-			goto err_clients;
+				client->addr + i);
+			return PTR_ERR(cl->client);
 		}
+		cl->regmap = devm_regmap_init_i2c(cl->client, config);
+		if (IS_ERR(cl->regmap))
+			return PTR_ERR(cl->regmap);
 	}
 
 	i2c_set_clientdata(client, at24);
@@ -686,10 +685,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	return 0;
 
 err_clients:
-	for (i = 1; i < num_addresses; i++)
-		if (at24->client[i].client)
-			i2c_unregister_device(at24->client[i].client);
-
 	pm_runtime_disable(&client->dev);
 
 	return err;
@@ -698,15 +693,11 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 static int at24_remove(struct i2c_client *client)
 {
 	struct at24_data *at24;
-	int i;
 
 	at24 = i2c_get_clientdata(client);
 
 	nvmem_unregister(at24->nvmem);
 
-	for (i = 1; i < at24->num_addresses; i++)
-		i2c_unregister_device(at24->client[i].client);
-
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
-- 
2.15.1