[PATCH 0/4] eeprom: at24: last bits of the big refactoring

[PATCH 0/4] eeprom: at24: last bits of the big refactoring

[Bartosz Golaszewski]

This is a follow-up to the big series merged for 4.17. The patches
contain some bits and pieces that were missing in the last submission
or depend on some new features merged this merge window.

Bartosz Golaszewski (4):
  eeprom: at24: don't assign the i2c ID table to at24_driver
  eeprom: at24: use devm_nvmem_register()
  eeprom: at24: provide and use a helper for releasing dummy i2c clients
  eeprom: at24: provide a separate routine for creating dummy i2c
    clients

 drivers/misc/eeprom/at24.c | 70 ++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 26 deletions(-)

-- 
2.17.0

[PATCH 1/4] eeprom: at24: don't assign the i2c ID table to at24_driver

[Bartosz Golaszewski]

We switched to using probe_new(), so this is no longer used
by i2c core.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/misc/eeprom/at24.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0c125f207aea..2310f26ac4f3 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -737,7 +737,6 @@ static struct i2c_driver at24_driver = {
 	},
 	.probe_new = at24_probe,
 	.remove = at24_remove,
-	.id_table = at24_ids,
 };
 
 static int __init at24_init(void)
-- 
2.17.0

Re: [PATCH 1/4] eeprom: at24: don't assign the i2c ID table to at24_driver

[Peter Rosin]

On 2018-04-10 15:12, Bartosz Golaszewski wrote:
> We switched to using probe_new(), so this is no longer used
> by i2c core.

It seems to be used in i2c_device_match() ???

This could easily be me not understanding something...

Cheers,
Peter

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/misc/eeprom/at24.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 0c125f207aea..2310f26ac4f3 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -737,7 +737,6 @@ static struct i2c_driver at24_driver = {
>  	},
>  	.probe_new = at24_probe,
>  	.remove = at24_remove,
> -	.id_table = at24_ids,
>  };
>  
>  static int __init at24_init(void)
> 

Re: [PATCH 1/4] eeprom: at24: don't assign the i2c ID table to at24_driver

[Bartosz Golaszewski]

2018-04-11 11:56 GMT+02:00 Peter Rosin <peda@axentia.se>:
> On 2018-04-10 15:12, Bartosz Golaszewski wrote:
>> We switched to using probe_new(), so this is no longer used
>> by i2c core.
>
> It seems to be used in i2c_device_match() ???
>
> This could easily be me not understanding something...
>

Yes, but i2c core no longer uses the id_table field in struct
i2c_driver. We call i2c_device_match() ourselves instead of letting
i2c core do it and pass the driver data to probe().

Hope that helps,
Bartosz

Re: [PATCH 1/4] eeprom: at24: don't assign the i2c ID table to at24_driver

[Peter Rosin]

On 2018-04-11 12:09, Bartosz Golaszewski wrote:
> 2018-04-11 11:56 GMT+02:00 Peter Rosin <peda@axentia.se>:
>> On 2018-04-10 15:12, Bartosz Golaszewski wrote:
>>> We switched to using probe_new(), so this is no longer used
>>> by i2c core.
>>
>> It seems to be used in i2c_device_match() ???
>>
>> This could easily be me not understanding something...
>>
> 
> Yes, but i2c core no longer uses the id_table field in struct
> i2c_driver. We call i2c_device_match() ourselves instead of letting
> i2c core do it and pass the driver data to probe().
> 
> Hope that helps,
> Bartosz
> 

But, i2c_device_match is a static function in the core. I think you
are confusing it with i2c_match_id, which is in fact called from
the static i2c_device_match (with is a bus operation).

So, no, it didn't really help...

Cheers,
Peter

Re: [PATCH 1/4] eeprom: at24: don't assign the i2c ID table to at24_driver

[Bartosz Golaszewski]

2018-04-11 12:12 GMT+02:00 Peter Rosin <peda@axentia.se>:
> On 2018-04-11 12:09, Bartosz Golaszewski wrote:
>> 2018-04-11 11:56 GMT+02:00 Peter Rosin <peda@axentia.se>:
>>> On 2018-04-10 15:12, Bartosz Golaszewski wrote:
>>>> We switched to using probe_new(), so this is no longer used
>>>> by i2c core.
>>>
>>> It seems to be used in i2c_device_match() ???
>>>
>>> This could easily be me not understanding something...
>>>
>>
>> Yes, but i2c core no longer uses the id_table field in struct
>> i2c_driver. We call i2c_device_match() ourselves instead of letting
>> i2c core do it and pass the driver data to probe().
>>
>> Hope that helps,
>> Bartosz
>>
>
> But, i2c_device_match is a static function in the core. I think you
> are confusing it with i2c_match_id, which is in fact called from
> the static i2c_device_match (with is a bus operation).
>

Right. So probe_new() in i2c no longer has the id parameter and the
users are supposed to call i2c_match_id() themselves but
i2c_device_match() still expects the id_table to be present in struct
i2c_driver. I should have tested it with some board that uses platform
data.

Consider this patch dropped.

Thanks for spotting that,
Bartosz

[PATCH 2/4] eeprom: at24: use devm_nvmem_register()

[Bartosz Golaszewski]

We now have a managed variant of nvmem_register(). Use it
in at24_probe().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/misc/eeprom/at24.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2310f26ac4f3..5e56a99435c6 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -685,7 +685,7 @@ static int at24_probe(struct i2c_client *client)
 	nvmem_config.word_size = 1;
 	nvmem_config.size = pdata.byte_len;
 
-	at24->nvmem = nvmem_register(&nvmem_config);
+	at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
 	if (IS_ERR(at24->nvmem)) {
 		err = PTR_ERR(at24->nvmem);
 		goto err_clients;
@@ -718,8 +718,6 @@ static int at24_remove(struct i2c_client *client)
 
 	at24 = i2c_get_clientdata(client);
 
-	nvmem_unregister(at24->nvmem);
-
 	for (i = 1; i < at24->num_addresses; i++)
 		i2c_unregister_device(at24->client[i].client);
 
-- 
2.17.0

[PATCH 3/4] eeprom: at24: provide and use a helper for releasing dummy i2c clients

[Bartosz Golaszewski]

This allows us to drop two opencoded for loops. We also don't need to
check if the i2c client is NULL before calling i2c_unregister_device().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/misc/eeprom/at24.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 5e56a99435c6..c57eaea181d8 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -532,6 +532,14 @@ static int at24_get_pdata(struct device *dev, struct at24_platform_data *pdata)
 	return 0;
 }
 
+static void at24_remove_dummy_clients(struct at24_data *at24)
+{
+	int i;
+
+	for (i = 1; i < at24->num_addresses; i++)
+		i2c_unregister_device(at24->client[i].client);
+}
+
 static unsigned int at24_get_offset_adj(u8 flags, unsigned int byte_len)
 {
 	if (flags & AT24_FLAG_MAC) {
@@ -702,10 +710,7 @@ static int at24_probe(struct i2c_client *client)
 	return 0;
 
 err_clients:
-	for (i = 1; i < num_addresses; i++)
-		if (at24->client[i].client)
-			i2c_unregister_device(at24->client[i].client);
-
+	at24_remove_dummy_clients(at24);
 	pm_runtime_disable(dev);
 
 	return err;
@@ -714,13 +719,10 @@ static int at24_probe(struct i2c_client *client)
 static int at24_remove(struct i2c_client *client)
 {
 	struct at24_data *at24;
-	int i;
 
 	at24 = i2c_get_clientdata(client);
 
-	for (i = 1; i < at24->num_addresses; i++)
-		i2c_unregister_device(at24->client[i].client);
-
+	at24_remove_dummy_clients(at24);
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
-- 
2.17.0

[PATCH 4/4] eeprom: at24: provide a separate routine for creating dummy i2c clients

[Bartosz Golaszewski]

Move the code responsible for creating the dummy i2c clients used by
chips taking multiple slave addresses to a separate function.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/misc/eeprom/at24.c | 47 ++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c57eaea181d8..0cf79ca4f1bf 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -540,6 +540,35 @@ static void at24_remove_dummy_clients(struct at24_data *at24)
 		i2c_unregister_device(at24->client[i].client);
 }
 
+static int at24_make_dummy_client(struct at24_data *at24, unsigned int index,
+				  struct regmap_config *regmap_config)
+{
+	struct i2c_client *base_client, *dummy_client;
+	unsigned short int addr;
+	struct regmap *regmap;
+	struct device *dev;
+
+	base_client = at24->client[0].client;
+	dev = &base_client->dev;
+	addr = base_client->addr + index;
+
+	dummy_client = i2c_new_dummy(base_client->adapter,
+				     base_client->addr + index);
+	if (!dummy_client) {
+		dev_err(dev, "address 0x%02x unavailable\n", addr);
+		return -EADDRINUSE;
+	}
+
+	regmap = devm_regmap_init_i2c(dummy_client, regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	at24->client[index].client = dummy_client;
+	at24->client[index].regmap = regmap;
+
+	return 0;
+}
+
 static unsigned int at24_get_offset_adj(u8 flags, unsigned int byte_len)
 {
 	if (flags & AT24_FLAG_MAC) {
@@ -645,20 +674,10 @@ static int at24_probe(struct i2c_client *client)
 
 	/* 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) {
-			dev_err(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,
-						&regmap_config);
-		if (IS_ERR(at24->client[i].regmap)) {
-			err = PTR_ERR(at24->client[i].regmap);
-			goto err_clients;
+		err = at24_make_dummy_client(at24, i, &regmap_config);
+		if (err) {
+			at24_remove_dummy_clients(at24);
+			return err;
 		}
 	}
 
-- 
2.17.0

Re: [PATCH 4/4] eeprom: at24: provide a separate routine for creating dummy i2c clients

[Peter Rosin]

On 2018-04-10 15:12, Bartosz Golaszewski wrote:
> Move the code responsible for creating the dummy i2c clients used by
> chips taking multiple slave addresses to a separate function.
> 
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/misc/eeprom/at24.c | 47 ++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index c57eaea181d8..0cf79ca4f1bf 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -540,6 +540,35 @@ static void at24_remove_dummy_clients(struct at24_data *at24)
>  		i2c_unregister_device(at24->client[i].client);
>  }
>  
> +static int at24_make_dummy_client(struct at24_data *at24, unsigned int index,
> +				  struct regmap_config *regmap_config)
> +{
> +	struct i2c_client *base_client, *dummy_client;
> +	unsigned short int addr;
> +	struct regmap *regmap;
> +	struct device *dev;
> +
> +	base_client = at24->client[0].client;
> +	dev = &base_client->dev;
> +	addr = base_client->addr + index;
> +
> +	dummy_client = i2c_new_dummy(base_client->adapter,
> +				     base_client->addr + index);
> +	if (!dummy_client) {
> +		dev_err(dev, "address 0x%02x unavailable\n", addr);
> +		return -EADDRINUSE;
> +	}
> +
> +	regmap = devm_regmap_init_i2c(dummy_client, regmap_config);
> +	if (IS_ERR(regmap))

AFAICT, if regmap init fails and you return early, dummy_client is leaked.

> +		return PTR_ERR(regmap);
> +
> +	at24->client[index].client = dummy_client;
> +	at24->client[index].regmap = regmap;
> +
> +	return 0;
> +}
> +
>  static unsigned int at24_get_offset_adj(u8 flags, unsigned int byte_len)
>  {
>  	if (flags & AT24_FLAG_MAC) {
> @@ -645,20 +674,10 @@ static int at24_probe(struct i2c_client *client)
>  
>  	/* 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) {
> -			dev_err(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,
> -						&regmap_config);
> -		if (IS_ERR(at24->client[i].regmap)) {
> -			err = PTR_ERR(at24->client[i].regmap);
> -			goto err_clients;
> +		err = at24_make_dummy_client(at24, i, &regmap_config);
> +		if (err) {
> +			at24_remove_dummy_clients(at24);
> +			return err;
>  		}
>  	}
>  
>