eeprom: at24: last bits of the big refactoring

PATCH Superseded Bartosz Golaszewski 2 participants 10 messages Apr 10, 2018 – Apr 11, 2018

Subsystems (2)

Char and misc drivers I2C subsystem host drivers

lore

Share & tools

lore

Bartosz Golaszewski

author

2018-04-10 13:12 UTC #0 lore raw

cover eeprom: at24: last bits of the big refactoring

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

Signature · 1 line

2.17.0

Bartosz Golaszewski

author

2018-04-10 13:12 UTC #1 lore raw

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

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

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/misc/eeprom/at24.c | 1 -
 1 file changed, 1 deletion(-)

drivers/misc/eeprom/at24.c +0 -1

index 0c125f207aea..2310f26ac4f3 100644
@@ -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)

Signature · 1 line

2.17.0

Peter Rosin

2018-04-11 09:56 UTC #2 lore raw

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

Quoted 2 lines · Bartosz Golaszewski

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

Quoted 18 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/misc/eeprom/at24.c | 1 -
 1 file changed, 1 deletion(-)

drivers/misc/eeprom/at24.c +0 -1

index 0c125f207aea..2310f26ac4f3 100644
@@ -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)

Bartosz Golaszewski

author

2018-04-11 10:09 UTC #3 lore raw

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

2018-04-11 11:56 GMT+02:00 Peter Rosin <[email protected]>:

Quoted 8 lines · Bartosz Golaszewski

Quoted 2 lines
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

Peter Rosin

2018-04-11 10:12 UTC #4 lore raw

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

Quoted 10 lines · Bartosz Golaszewski

2018-04-11 11:56 GMT+02:00 Peter Rosin <[email protected]>:
Quoted 8 lines · Bartosz Golaszewski
Quoted 2 lines
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...

Quoted 7 lines

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

Bartosz Golaszewski

author

2018-04-11 11:14 UTC #5 lore raw

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

2018-04-11 12:12 GMT+02:00 Peter Rosin <[email protected]>:

Quoted 15 lines · Bartosz Golaszewski

Quoted 14 lines
2018-04-11 11:56 GMT+02:00 Peter Rosin <[email protected]>:
Quoted 8 lines · Bartosz Golaszewski
Quoted 2 lines
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().

Quoted 8 lines

Quoted 3 lines
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

Bartosz Golaszewski

author

2018-04-10 13:12 UTC #6 lore raw

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

In reply to Bartosz Golaszewski

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

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/misc/eeprom/at24.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/misc/eeprom/at24.c +1 -3

index 2310f26ac4f3..5e56a99435c6 100644
@@ -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);

Signature · 1 line

2.17.0

Bartosz Golaszewski

author

2018-04-10 13:12 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/misc/eeprom/at24.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/misc/eeprom/at24.c +10 -8

index 5e56a99435c6..c57eaea181d8 100644
@@ -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);

Signature · 1 line

2.17.0

Bartosz Golaszewski

author

2018-04-10 13:12 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/misc/eeprom/at24.c | 47 ++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

drivers/misc/eeprom/at24.c +33 ~~-14~~

index c57eaea181d8..0cf79ca4f1bf 100644
@@ -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;
 		}
 	}

Signature · 1 line

2.17.0

Peter Rosin

2018-04-11 11:44 UTC #9 lore raw

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

Quoted 27 lines · 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 <[email protected]>

---
 drivers/misc/eeprom/at24.c | 47 ++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

drivers/misc/eeprom/at24.c +11 -0

index c57eaea181d8..0cf79ca4f1bf 100644
@@ -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;

drivers/misc/eeprom/at24.c static void at24_remove_dummy_clients(struct at24_data *at24) · line 563

Quoted 10 lines

drivers/misc/eeprom/at24.c +10 -0

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

Quoted 37 lines

drivers/misc/eeprom/at24.c +12 ~~-14~~

+		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;
 		}
 	}