eeprom: at24: driver refactoring
Subsystems (2)
cover eeprom: at24: driver refactoring
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around at24_probe() which became quite complicated and hard to read.
The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if byte_len is a power of 2 (as we do support models for which it's not true).
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Bartosz Golaszewski (21):
eeprom: at24: disable regmap locking
eeprom: at24: remove nvmem_config from at24_data
eeprom: at24: arrange local variables
eeprom: at24: use SPDX identifier instead of GPL boiler-plate
eeprom: at24: remove code separators
eeprom: at24: drop redundant variable in at24_read()
eeprom: at24: drop redundant variable in at24_write()
eeprom: at24: make struct initialization uniform in at24_probe()
eeprom: at24: don't check if byte_len is a power of 2
eeprom: at24: rename at24_get_pdata()
eeprom: at24: rename chip to pdata in at24_probe()
eeprom: at24: use a helper variable for dev
eeprom: at24: readability tweak in at24_probe()
eeprom: at24: provide and use at24_base_client_dev()
eeprom: at24: switch to using probe_new() from the i2c framework
eeprom: at24: move platform data processing into a separate routine
eeprom: at24: remove at24_platform_data from at24_data
eeprom: at24: refactor at24_probe()
eeprom: at24: tweak newlines
eeprom: at24: fix a line break
eeprom: at24: simplify the i2c functionality checking
drivers/misc/eeprom/at24.c | 293 ++++++++++++++++++++++++--------------------- 1 file changed, 156 insertions(+), 137 deletions(-)
Signature · 1 line
2.16.1
PATCH 1/21 eeprom: at24: disable regmap locking
We use our own mutex for locking. Disable the regmap-specific locking.
--- drivers/misc/eeprom/at24.c | 1 + 1 file changed, 1 insertion(+)
drivers/misc/eeprom/at24.c +1 -0
@@ -577,6 +577,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) regmap_config.val_bits =8 ; regmap_config.reg_bits = (chip.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; + regmap_config.disable_locking = true; at24 =devm_kzalloc (&client->dev,sizeof (struct at24_data ) + num_addresses *sizeof (struct at24_client ),GFP_KERNEL );
Signature · 1 line
2.16.1
PATCH 2/21 eeprom: at24: remove nvmem_config from at24_data
In reply to Bartosz GolaszewskiThis structure only needs to exist during the call to nvmem_register().
--- drivers/misc/eeprom/at24.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/misc/eeprom/at24.c +16 -16
@@ -75,7 +75,6 @@ struct at24_data {unsigned int num_addresses;unsigned int offset_adj; -struct nvmem_config nvmem_config;struct nvmem_device *nvmem;struct gpio_desc *wp_gpio;
@@ -523,6 +522,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)int err;unsigned int i, num_addresses;struct regmap_config regmap_config = { }; +struct nvmem_config nvmem_config = { };u8 test_byte;if (client->dev.platform_data) {
@@ -650,21 +650,21 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) goto err_clients; } - at24->nvmem_config.name =dev_name (&client->dev); - at24->nvmem_config.dev = &client->dev; - at24->nvmem_config.read_only = !writable; - at24->nvmem_config.root_only = true; - at24->nvmem_config.owner =THIS_MODULE ; - at24->nvmem_config.compat = true; - at24->nvmem_config.base_dev = &client->dev; - at24->nvmem_config.reg_read = at24_read; - at24->nvmem_config.reg_write = at24_write; - at24->nvmem_config.priv = at24; - at24->nvmem_config.stride =1 ; - at24->nvmem_config.word_size =1 ; - at24->nvmem_config.size = chip.byte_len; - - at24->nvmem =nvmem_register (&at24->nvmem_config); + nvmem_config.name =dev_name (&client->dev); + nvmem_config.dev = &client->dev; + nvmem_config.read_only = !writable; + nvmem_config.root_only = true; + nvmem_config.owner =THIS_MODULE ; + nvmem_config.compat = true; + nvmem_config.base_dev = &client->dev; + nvmem_config.reg_read = at24_read; + nvmem_config.reg_write = at24_write; + nvmem_config.priv = at24; + nvmem_config.stride =1 ; + nvmem_config.word_size =1 ; + nvmem_config.size = chip.byte_len; + + at24->nvmem =nvmem_register (&nvmem_config);if (IS_ERR (at24->nvmem)) { err =PTR_ERR (at24->nvmem);
Signature · 1 line
2.16.1
PATCH 3/21 eeprom: at24: arrange local variables
In reply to Bartosz GolaszewskiArrange declarations of local variables by line length as visually it's easier to read.
--- drivers/misc/eeprom/at24.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
drivers/misc/eeprom/at24.c +16 -10
@@ -370,11 +370,14 @@ static ssize_t at24_regmap_write(struct at24_data *at24, const char *buf,static int at24_read (void *priv,unsigned int off,void *val,size_t count) { -struct at24_data *at24 = priv; -struct device *dev = &at24->client[0 ].client->dev; +struct at24_data *at24; +struct device *dev;char *buf = val;int ret; + at24 = priv; + dev = &at24->client[0 ].client->dev; +if (unlikely (!count))return count;
@@ -416,11 +419,14 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)static int at24_write (void *priv,unsigned int off,void *val,size_t count) { -struct at24_data *at24 = priv; -struct device *dev = &at24->client[0 ].client->dev; +struct at24_data *at24; +struct device *dev;char *buf = val;int ret; + at24 = priv; + dev = &at24->client[0 ].client->dev; +if (unlikely (!count))return -EINVAL ;
@@ -515,15 +521,15 @@ static unsigned int at24_get_offset_adj(u8 flags, unsigned int byte_len)static int at24_probe (struct i2c_client *client,const struct i2c_device_id *id) { -struct at24_platform_data chip = {0 }; -const struct at24_chip_data *cd =NULL ; -bool writable; -struct at24_data *at24; -int err; -unsigned int i, num_addresses;struct regmap_config regmap_config = { };struct nvmem_config nvmem_config = { }; +const struct at24_chip_data *cd =NULL ; +struct at24_platform_data chip = {0 }; +unsigned int i, num_addresses; +struct at24_data *at24; +bool writable;u8 test_byte; +int err;if (client->dev.platform_data) { chip = *(struct at24_platform_data *)client->dev.platform_data;
Signature · 1 line
2.16.1
Greg Kroah-Hartman
maintainerRe: [PATCH 03/21] eeprom: at24: arrange local variables
Quoted 2 lines · Bartosz Golaszewski
Arrange declarations of local variables by line length as visually it's easier to read.
That's the craziest thing I've seen in a while...
ok, if it makes it easier for you :)
greg k-h
Re: [PATCH 03/21] eeprom: at24: arrange local variables
2018-03-23 16:21 GMT+01:00 Greg Kroah-Hartman <[email protected]>:
Quoted 9 lines · Bartosz Golaszewski
Quoted 2 lines
Arrange declarations of local variables by line length as visually it's easier to read.
That's the craziest thing I've seen in a while...
ok, if it makes it easier for you :)
greg k-h
Hi Greg,
this isn't something unusual, see for example:
bef6b1b7a6ff ("nfp: reorder variables in nfp_net_tx()") 5318c53d5b4b ("crypto: s5p-sss - Use consistent indentation for variables and members")
Thanks,
Bart
Greg Kroah-Hartman
maintainerRe: [PATCH 03/21] eeprom: at24: arrange local variables
Quoted 11 lines · Bartosz Golaszewski
2018-03-23 16:21 GMT+01:00 Greg Kroah-Hartman <[email protected]>:
Quoted 9 lines · Bartosz Golaszewski
Quoted 2 lines
Arrange declarations of local variables by line length as visually it's easier to read.
That's the craziest thing I've seen in a while...
ok, if it makes it easier for you :)
greg k-h
Quoted 7 lines
Hi Greg,
this isn't something unusual, see for example:
bef6b1b7a6ff("nfp: reorder variables innfp_net_tx()")5318c53d5b4b("crypto: s5p-sss - Use consistent indentation for variables and members")
I still say it is crazy :)
PATCH 4/21 eeprom: at24: use SPDX identifier instead of GPL boiler-plate
In reply to Bartosz GolaszewskiReplace the GPL header with an SPDX identifier for GPL-2.0.
--- drivers/misc/eeprom/at24.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/misc/eeprom/at24.c +2 -5
@@ -1,14 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */ /* * at24.c - handle most I2C EEPROMs * * Copyright (C) 2005-2007 David Brownell * Copyright (C) 2008 Wolfram Sang, Pengutronix -* -* This program is free software; you can redistribute it and/or modify -* it under the terms of the GNU General Public License as published by -* the Free Software Foundation; either version 2 of the License, or -* (at your option) any later version. */ +#include <linux/kernel.h> #include <linux/init.h> #include <linux/module.h>
Signature · 1 line
2.16.1
Re: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
Quoted 1 line · Bartosz Golaszewski
Replace the GPL header with an SPDX identifier for GPL-2.0.
Great care should be exercised when dealing with copyrights of others.
drivers/misc/eeprom/at24.c line 1
Quoted 12 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/misc/eeprom/at24.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/misc/eeprom/at24.c +1
-0@@ -1,14 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */
This should be 2.0+
Also, use a // style comment for the SPDX line in C files.
Cheers,
Peter
Quoted 16 lines
drivers/misc/eeprom/at24.c +1
-5/* * at24.c - handle most I2C EEPROMs * * Copyright (C) 2005-2007 David Brownell * Copyright (C) 2008 Wolfram Sang, Pengutronix -* -* This program is free software; you can redistribute it and/or modify -* it under the terms of the GNU General Public License as published by -* the Free Software Foundation; either version 2 of the License, or -* (at your option) any later version. */ +#include <linux/kernel.h> #include <linux/init.h> #include <linux/module.h>
Re: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
2018-03-19 12:03 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 11 lines · Bartosz Golaszewski
Quoted 1 line
Replace the GPL header with an SPDX identifier for GPL-2.0.
Great care should be exercised when dealing with copyrights of others.
Quoted 6 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/misc/eeprom/at24.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
Quoted 8 lines
drivers/misc/eeprom/at24.c line 1
Quoted 6 lines
drivers/misc/eeprom/at24.c +1
-0@@ -1,14 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0 */ This should be 2.0+
Indeed, thanks.
Quoted 5 lines
Also, use a // style comment for the SPDX line in C files.
Cheers,
Peter
I'm seeing both /* */ and // style comments used for SPDX headers - is there any reason not to use /* */ here?
Bart
Re: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
Quoted 5 lines · Bartosz Golaszewski
2018-03-19 12:03 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 1 line
Also, use a // style comment for the SPDX line in C files.
I'm seeing both /* */ and // style comments used for SPDX headers - is there any reason not to use /* */ here?
Documentation/process/license-rules.rst states:
2. Style:
The SPDX license identifier is added in form of a comment. The comment style depends on the file type::
C source: // SPDX-License-Identifier: <SPDX License Expression> C header: /* SPDX-License-Identifier: <SPDX License Expression> */
ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
scripts: # SPDX-License-Identifier: <SPDX License Expression>
.rst: .. SPDX-License-Identifier: <SPDX License Expression>
.dts{i}: // SPDX-License-Identifier: <SPDX License Expression>
Read more in that file for reasons. If there are none, I personally think the reason is that "Linus said so". Or something like that?
Cheers,
Peter
Re: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
2018-03-19 13:51 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 21 lines · Bartosz Golaszewski
Quoted 5 lines
2018-03-19 12:03 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 1 line
Also, use a // style comment for the SPDX line in C files.
I'm seeing both /* */ and // style comments used for SPDX headers - is there any reason not to use /* */ here?
Documentation/process/license-rules.rststates:2. Style:
The SPDX license identifier is added in form of a comment. The comment style depends on the file type::
C source: // SPDX-License-Identifier: <SPDX License Expression> C header: /* SPDX-License-Identifier: <SPDX License Expression> */
ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
scripts: # SPDX-License-Identifier: <SPDX License Expression>
.rst: .. SPDX-License-Identifier: <SPDX License Expression>
.dts{i}: // SPDX-License-Identifier: <SPDX License Expression>
Quoted 5 lines
Read more in that file for reasons. If there are none, I personally think the reason is that "Linus said so". Or something like that?
Cheers,
Peter
Makes sense, thanks.
I'm thinking about dropping this file from this series and submitting it separately for Greg to Ack.
Unless he sees our exchange and acks it here. :)
Thanks,
Bart
Greg Kroah-Hartman
maintainerRe: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
Quoted 28 lines · Bartosz Golaszewski
2018-03-19 13:51 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 21 lines · Bartosz Golaszewski
Quoted 5 lines
2018-03-19 12:03 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 1 line
Also, use a // style comment for the SPDX line in C files.
I'm seeing both /* */ and // style comments used for SPDX headers - is there any reason not to use /* */ here?
Documentation/process/license-rules.rststates:2. Style:
The SPDX license identifier is added in form of a comment. The comment style depends on the file type::
C source: // SPDX-License-Identifier: <SPDX License Expression> C header: /* SPDX-License-Identifier: <SPDX License Expression> */
ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
scripts: # SPDX-License-Identifier: <SPDX License Expression>
.rst: .. SPDX-License-Identifier: <SPDX License Expression>
.dts{i}: // SPDX-License-Identifier: <SPDX License Expression>Quoted 5 lines
Read more in that file for reasons. If there are none, I personally think the reason is that "Linus said so". Or something like that?
Cheers,
Peter
Quoted 6 lines
Makes sense, thanks.
I'm thinking about dropping this file from this series and submitting it separately for Greg to Ack.
Unless he sees our exchange and acks it here. :)
I can't ack a patch that is incorrect :(
Please fix it up and resend...
thanks,
greg k-h
Re: [PATCH 04/21] eeprom: at24: use SPDX identifier instead of GPL boiler-plate
2018-03-19 16:38 GMT+01:00 Greg Kroah-Hartman <[email protected]>:
Quoted 31 lines · Bartosz Golaszewski
Quoted 30 lines
2018-03-19 13:51 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 21 lines · Bartosz Golaszewski
Quoted 5 lines
2018-03-19 12:03 GMT+01:00 Peter Rosin <[email protected]>:
Quoted 1 line
Also, use a // style comment for the SPDX line in C files.
I'm seeing both /* */ and // style comments used for SPDX headers - is there any reason not to use /* */ here?
Documentation/process/license-rules.rststates:2. Style:
The SPDX license identifier is added in form of a comment. The comment style depends on the file type::
C source: // SPDX-License-Identifier: <SPDX License Expression> C header: /* SPDX-License-Identifier: <SPDX License Expression> */
ASM: /* SPDX-License-Identifier: <SPDX License Expression> */
scripts: # SPDX-License-Identifier: <SPDX License Expression>
.rst: .. SPDX-License-Identifier: <SPDX License Expression>
.dts{i}: // SPDX-License-Identifier: <SPDX License Expression>Quoted 5 lines
Read more in that file for reasons. If there are none, I personally think the reason is that "Linus said so". Or something like that?
Cheers,
PeterMakes sense, thanks.
Quoted 9 lines
Quoted 4 lines
I'm thinking about dropping this file from this series and submitting it separately for Greg to Ack.
Unless he sees our exchange and acks it here. :)
I can't ack a patch that is incorrect :(
Please fix it up and resend...
Oh yes, sure, I was just waiting for more reviews before resending v2. It's 4.18 material anyway.
Thanks,
Bartosz
PATCH 5/21 eeprom: at24: remove code separators
In reply to Bartosz GolaszewskiThese are just two left-overs from times when this driver was bigger.
They are not really useful anymore. Remove them.
--- drivers/misc/eeprom/at24.c | 4 ---- 1 file changed, 4 deletions(-)
drivers/misc/eeprom/at24.c +0 -4
@@ -235,8 +235,6 @@ static const struct acpi_device_id at24_acpi_ids[] = { };MODULE_DEVICE_TABLE (acpi, at24_acpi_ids); -/*-------------------------------------------------------------------------*/ -/* * This routine supports chips which consume multiple I2C addresses. It * computes the addressing information to be used for a given r/w request.
@@ -712,8 +710,6 @@ static int at24_remove(struct i2c_client *client)return 0 ; } -/*-------------------------------------------------------------------------*/ -static struct i2c_driver at24_driver = { .driver = { .name ="at24" ,
Signature · 1 line
2.16.1
PATCH 6/21 eeprom: at24: drop redundant variable in at24_read()
In reply to Bartosz GolaszewskiWe can reuse ret instead of defining a loop-local status variable.
--- drivers/misc/eeprom/at24.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/misc/eeprom/at24.c +6 -8
@@ -392,17 +392,15 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)mutex_lock (&at24->lock);while (count) { -int status; - - status =at24_regmap_read (at24, buf, off, count); -if (status <0 ) { + ret =at24_regmap_read (at24, buf, off, count); +if (ret <0 ) {mutex_unlock (&at24->lock);pm_runtime_put (dev); -return status; +return ret; } - buf += status; - off += status; - count -= status; + buf += ret; + off += ret; + count -= ret; }mutex_unlock (&at24->lock);
Signature · 1 line
2.16.1
PATCH 7/21 eeprom: at24: drop redundant variable in at24_write()
In reply to Bartosz GolaszewskiWe can reuse ret instead of defining a loop-local status variable.
--- drivers/misc/eeprom/at24.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/misc/eeprom/at24.c +6 -8
@@ -440,18 +440,16 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)gpiod_set_value_cansleep (at24->wp_gpio,0 );while (count) { -int status; - - status =at24_regmap_write (at24, buf, off, count); -if (status <0 ) { + ret =at24_regmap_write (at24, buf, off, count); +if (ret <0 ) {gpiod_set_value_cansleep (at24->wp_gpio,1 );mutex_unlock (&at24->lock);pm_runtime_put (dev); -return status; +return ret; } - buf += status; - off += status; - count -= status; + buf += ret; + off += ret; + count -= ret; }gpiod_set_value_cansleep (at24->wp_gpio,1 );
Signature · 1 line
2.16.1
PATCH 8/21 eeprom: at24: make struct initialization uniform in at24_probe()
In reply to Bartosz GolaszewskiWhen zeroing structs, use "{ }" everywhere.
--- drivers/misc/eeprom/at24.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/misc/eeprom/at24.c +1 -1
@@ -515,7 +515,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)struct regmap_config regmap_config = { };struct nvmem_config nvmem_config = { };const struct at24_chip_data *cd =NULL ; -struct at24_platform_data chip = {0 }; +struct at24_platform_data chip = { };unsigned int i, num_addresses;struct at24_data *at24;bool writable;
Signature · 1 line
2.16.1
PATCH 9/21 eeprom: at24: don't check if byte_len is a power of 2
In reply to Bartosz GolaszewskiWe support certain models the size of which is not a power of 2. This is not a reason to emit a warning.
--- drivers/misc/eeprom/at24.c | 3 --- 1 file changed, 3 deletions(-)
drivers/misc/eeprom/at24.c +0 -3
@@ -550,9 +550,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)at24_get_pdata (&client->dev, &chip); } -if (!is_power_of_2 (chip.byte_len)) -dev_warn (&client->dev, -"byte_len looks suspicious (no power of 2)!\n" );if (!chip.page_size) {dev_err (&client->dev,"page_size must not be 0!\n" );return -EINVAL ;
Signature · 1 line
2.16.1
PATCH 10/21 eeprom: at24: rename at24_get_pdata()
In reply to Bartosz GolaszewskiAs preparation for at24_probe() refactoring: rename at24_get_pdata() to at24_properties_to_pdata(). We're doing it because we'll move the pdata parsing code into a separate function which will be called at24_get_pdata(). Current routine with that name actually parses the device properties so change its name to reflect its purpose.
--- drivers/misc/eeprom/at24.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +3 -2
@@ -460,7 +460,8 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)return 0 ; } -static void at24_get_pdata (struct device *dev,struct at24_platform_data *chip) +static void at24_properties_to_pdata (struct device *dev, +struct at24_platform_data *chip) {int err;u32 val;
@@ -547,7 +548,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) chip.byte_len = cd->byte_len; chip.flags = cd->flags; -at24_get_pdata (&client->dev, &chip); +at24_properties_to_pdata (&client->dev, &chip); }if (!chip.page_size) {
Signature · 1 line
2.16.1
PATCH 11/21 eeprom: at24: rename chip to pdata in at24_probe()
In reply to Bartosz GolaszewskiReflect the purpose of this variable: it contains platform data so name it such.
--- drivers/misc/eeprom/at24.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
drivers/misc/eeprom/at24.c +21 -21
@@ -516,7 +516,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)struct regmap_config regmap_config = { };struct nvmem_config nvmem_config = { };const struct at24_chip_data *cd =NULL ; -struct at24_platform_data chip = { }; +struct at24_platform_data pdata = { };unsigned int i, num_addresses;struct at24_data *at24;bool writable;
@@ -524,7 +524,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)int err;if (client->dev.platform_data) { - chip = *(struct at24_platform_data *)client->dev.platform_data; + pdata = *(struct at24_platform_data *)client->dev.platform_data; }else {/* * The I2C core allows OF nodes compatibles to match against the
@@ -546,32 +546,32 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)if (!cd)return -ENODEV ; - chip.byte_len = cd->byte_len; - chip.flags = cd->flags; -at24_properties_to_pdata (&client->dev, &chip); + pdata.byte_len = cd->byte_len; + pdata.flags = cd->flags; +at24_properties_to_pdata (&client->dev, &pdata); } -if (!chip.page_size) { +if (!pdata.page_size) {dev_err (&client->dev,"page_size must not be 0!\n" );return -EINVAL ; } -if (!is_power_of_2 (chip.page_size)) +if (!is_power_of_2 (pdata.page_size))dev_warn (&client->dev,"page_size looks suspicious (no power of 2)!\n" );if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && !i2c_check_functionality (client->adapter,I2C_FUNC_SMBUS_WRITE_I2C_BLOCK )) - chip.page_size =1 ; + pdata.page_size =1 ; -if (chip.flags &AT24_FLAG_TAKE8ADDR ) +if (pdata.flags &AT24_FLAG_TAKE8ADDR ) num_addresses =8 ;else - num_addresses =DIV_ROUND_UP (chip.byte_len, - (chip.flags &AT24_FLAG_ADDR16 ) ?65536 :256 ); + num_addresses =DIV_ROUND_UP (pdata.byte_len, + (pdata.flags &AT24_FLAG_ADDR16 ) ?65536 :256 ); regmap_config.val_bits =8 ; - regmap_config.reg_bits = (chip.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; + regmap_config.reg_bits = (pdata.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; regmap_config.disable_locking = true; at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
@@ -580,9 +580,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)return -ENOMEM ;mutex_init (&at24->lock); - at24->chip = chip; + at24->chip = pdata; at24->num_addresses = num_addresses; - at24->offset_adj =at24_get_offset_adj (chip.flags, chip.byte_len); + at24->offset_adj =at24_get_offset_adj (pdata.flags, pdata.byte_len); at24->wp_gpio =devm_gpiod_get_optional (&client->dev,"wp" ,GPIOD_OUT_HIGH );
@@ -594,16 +594,16 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)if (IS_ERR (at24->client[0 ].regmap))return PTR_ERR (at24->client[0 ].regmap); -if ((chip.flags &AT24_FLAG_SERIAL ) && (chip.flags &AT24_FLAG_MAC )) { +if ((pdata.flags &AT24_FLAG_SERIAL ) && (pdata.flags &AT24_FLAG_MAC )) {dev_err (&client->dev,"invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC." );return -EINVAL ; } - writable = !(chip.flags &AT24_FLAG_READONLY ); + writable = !(pdata.flags &AT24_FLAG_READONLY );if (writable) { at24->write_max = min_t(unsigned int, - chip.page_size, at24_io_limit); + pdata.page_size, at24_io_limit);if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && at24->write_max >I2C_SMBUS_BLOCK_MAX ) at24->write_max =I2C_SMBUS_BLOCK_MAX ;
@@ -657,7 +657,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) nvmem_config.priv = at24; nvmem_config.stride =1 ; nvmem_config.word_size =1 ; - nvmem_config.size = chip.byte_len; + nvmem_config.size = pdata.byte_len; at24->nvmem =nvmem_register (&nvmem_config);
@@ -667,12 +667,12 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) }dev_info (&client->dev,"%u byte %s EEPROM, %s, %u bytes/write\n" , - chip.byte_len, client->name, + pdata.byte_len, client->name, writable ?"writable" :"read-only" , at24->write_max);/* export data to kernel code */ -if (chip.setup) - chip.setup (at24->nvmem, chip.context); +if (pdata.setup) + pdata.setup (at24->nvmem, pdata.context);return 0 ;
Signature · 1 line
2.16.1
PATCH 12/21 eeprom: at24: use a helper variable for dev
In reply to Bartosz GolaszewskiWe use the &client->dev construct all over in at24_probe(). Use a helper variable which is more readable and allows to avoid a couple unnecessary line breaks.
--- drivers/misc/eeprom/at24.c | 48 ++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 25 deletions(-)
drivers/misc/eeprom/at24.c +23 -25
@@ -517,29 +517,29 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)struct nvmem_config nvmem_config = { };const struct at24_chip_data *cd =NULL ;struct at24_platform_data pdata = { }; +struct device *dev = &client->dev;unsigned int i, num_addresses;struct at24_data *at24;bool writable;u8 test_byte;int err; -if (client->dev.platform_data) { - pdata = *(struct at24_platform_data *)client->dev.platform_data; +if (dev->platform_data) { + pdata = *(struct at24_platform_data *)dev->platform_data; }else {/* * The I2C core allows OF nodes compatibles to match against the * I2C device ID table as a fallback, so check not only if an OF * node is present but also if it matches an OF device ID entry. */ -if (client->dev.of_node && -of_match_device (at24_of_match, &client->dev)) { - cd =of_device_get_match_data (&client->dev); +if (dev->of_node &&of_match_device (at24_of_match, dev)) { + cd =of_device_get_match_data (dev); }else if (id) { cd = (void *)id->driver_data; }else {const struct acpi_device_id *aid; - aid =acpi_match_device (at24_acpi_ids, &client->dev); + aid =acpi_match_device (at24_acpi_ids, dev);if (aid) cd = (void *)aid->driver_data; }
@@ -548,16 +548,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) pdata.byte_len = cd->byte_len; pdata.flags = cd->flags; -at24_properties_to_pdata (&client->dev, &pdata); +at24_properties_to_pdata (dev, &pdata); }if (!pdata.page_size) { -dev_err (&client->dev,"page_size must not be 0!\n" ); +dev_err (dev,"page_size must not be 0!\n" );return -EINVAL ; }if (!is_power_of_2 (pdata.page_size)) -dev_warn (&client->dev, -"page_size looks suspicious (no power of 2)!\n" ); +dev_warn (dev,"page_size looks suspicious (no power of 2)!\n" );if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && !i2c_check_functionality(client->adapter,
@@ -574,8 +573,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) regmap_config.reg_bits = (pdata.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; regmap_config.disable_locking = true; - at24 =devm_kzalloc (&client->dev,sizeof (struct at24_data ) + - num_addresses *sizeof (struct at24_client ),GFP_KERNEL ); + at24 =devm_kzalloc (dev,sizeof (struct at24_data ) + num_addresses * +sizeof (struct at24_client ),GFP_KERNEL );if (!at24)return -ENOMEM ;
@@ -584,8 +583,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) at24->num_addresses = num_addresses; at24->offset_adj =at24_get_offset_adj (pdata.flags, pdata.byte_len); - at24->wp_gpio =devm_gpiod_get_optional (&client->dev, -"wp" ,GPIOD_OUT_HIGH ); + at24->wp_gpio =devm_gpiod_get_optional (dev,"wp" ,GPIOD_OUT_HIGH );if (IS_ERR (at24->wp_gpio))return PTR_ERR (at24->wp_gpio);
@@ -595,7 +593,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)return PTR_ERR (at24->client[0 ].regmap);if ((pdata.flags &AT24_FLAG_SERIAL ) && (pdata.flags &AT24_FLAG_MAC )) { -dev_err (&client->dev, +dev_err (dev,"invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC." );return -EINVAL ; }
@@ -614,8 +612,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) at24->client[i].client =i2c_new_dummy (client->adapter, client->addr + i);if (!at24->client[i].client) { -dev_err (&client->dev,"address 0x%02x unavailable\n" , - client->addr + i); +dev_err (dev,"address 0x%02x unavailable\n" , + client->addr + i); err = -EADDRINUSE ; goto err_clients; }
@@ -631,27 +629,27 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)i2c_set_clientdata (client, at24);/* enable runtime pm */ -pm_runtime_set_active (&client->dev); -pm_runtime_enable (&client->dev); +pm_runtime_set_active (dev); +pm_runtime_enable (dev);/* * Perform a one-byte test read to verify that the * chip is functional. */ err =at24_read (at24,0 , &test_byte,1 ); -pm_runtime_idle (&client->dev); +pm_runtime_idle (dev);if (err) { err = -ENODEV ; goto err_clients; } - nvmem_config.name =dev_name (&client->dev); - nvmem_config.dev = &client->dev; + nvmem_config.name =dev_name (dev); + nvmem_config.dev = dev; nvmem_config.read_only = !writable; nvmem_config.root_only = true; nvmem_config.owner =THIS_MODULE ; nvmem_config.compat = true; - nvmem_config.base_dev = &client->dev; + nvmem_config.base_dev = dev; nvmem_config.reg_read = at24_read; nvmem_config.reg_write = at24_write; nvmem_config.priv = at24;
@@ -666,7 +664,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) goto err_clients; } -dev_info (&client->dev,"%u byte %s EEPROM, %s, %u bytes/write\n" , +dev_info (dev,"%u byte %s EEPROM, %s, %u bytes/write\n" , pdata.byte_len, client->name, writable ?"writable" :"read-only" , at24->write_max);
@@ -681,7 +679,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)if (at24->client[i].client)i2c_unregister_device (at24->client[i].client); -pm_runtime_disable (&client->dev); +pm_runtime_disable (dev);return err; }
Signature · 1 line
2.16.1
PATCH 13/21 eeprom: at24: readability tweak in at24_probe()
In reply to Bartosz GolaszewskiUse a helper variable for the size we want to allocate with devm_kzalloc() and save an ugly line break.
--- drivers/misc/eeprom/at24.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +3 -2
@@ -520,6 +520,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)struct device *dev = &client->dev;unsigned int i, num_addresses;struct at24_data *at24; +size_t at24_size;bool writable;u8 test_byte;int err;
@@ -573,8 +574,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) regmap_config.reg_bits = (pdata.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; regmap_config.disable_locking = true; - at24 =devm_kzalloc (dev,sizeof (struct at24_data ) + num_addresses * -sizeof (struct at24_client ),GFP_KERNEL ); + at24_size =sizeof (*at24) + num_addresses *sizeof (struct at24_client ); + at24 =devm_kzalloc (dev, at24_size,GFP_KERNEL );if (!at24)return -ENOMEM ;
Signature · 1 line
2.16.1
PATCH 14/21 eeprom: at24: provide and use at24_base_client_dev()
In reply to Bartosz GolaszewskiUse a helper function for accessing the device struct of the base i2c client. This routine is named in a way that reflects its purpose unlike the previously hand-coded dereferencing.
--- drivers/misc/eeprom/at24.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +7 -2
@@ -260,6 +260,11 @@ static struct at24_client *at24_translate_offset(struct at24_data *at24,return &at24->client[i]; } +static struct device *at24_base_client_dev (struct at24_data *at24) +{ +return &at24->client[0 ].client->dev; +} +static size_t at24_adjust_read_count (struct at24_data *at24,unsigned int offset,size_t count) {
@@ -371,7 +376,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)int ret; at24 = priv; - dev = &at24->client[0 ].client->dev; + dev =at24_base_client_dev (at24);if (unlikely (!count))return count;
@@ -418,7 +423,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)int ret; at24 = priv; - dev = &at24->client[0 ].client->dev; + dev =at24_base_client_dev (at24);if (unlikely (!count))return -EINVAL ;
Signature · 1 line
2.16.1
PATCH 15/21 eeprom: at24: switch to using probe_new() from the i2c framework
In reply to Bartosz GolaszewskiUse the new probe() style for i2c drivers.
--- drivers/misc/eeprom/at24.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +5 -2
@@ -516,13 +516,14 @@ static unsigned int at24_get_offset_adj(u8 flags, unsigned int byte_len) } } -static int at24_probe (struct i2c_client *client,const struct i2c_device_id *id) +static int at24_probe (struct i2c_client *client) {struct regmap_config regmap_config = { };struct nvmem_config nvmem_config = { };const struct at24_chip_data *cd =NULL ;struct at24_platform_data pdata = { };struct device *dev = &client->dev; +const struct i2c_device_id *id;unsigned int i, num_addresses;struct at24_data *at24;size_t at24_size;
@@ -530,6 +531,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)u8 test_byte;int err; + id =i2c_match_id (at24_ids, client); +if (dev->platform_data) { pdata = *(struct at24_platform_data *)dev->platform_data; }else {
@@ -714,7 +717,7 @@ static struct i2c_driver at24_driver = { .of_match_table = at24_of_match, .acpi_match_table = ACPI_PTR(at24_acpi_ids), }, - .probe = at24_probe, + .probe_new = at24_probe, .remove = at24_remove, .id_table = at24_ids, };
Signature · 1 line
2.16.1
PATCH 16/21 eeprom: at24: move platform data processing into a separate routine
In reply to Bartosz GolaszewskiThis driver can receive its device data from different sources depending on the system. Move the entire code processing platform data, device tree and acpi into a separate function.
--- drivers/misc/eeprom/at24.c | 70 ++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 30 deletions(-)
drivers/misc/eeprom/at24.c +40 -30
@@ -493,6 +493,43 @@ static void at24_properties_to_pdata(struct device *dev, } } +static int at24_get_pdata (struct device *dev,struct at24_platform_data *pdata) +{ +struct device_node *of_node = dev->of_node; +const struct at24_chip_data *cdata; +const struct i2c_device_id *id; +struct at24_platform_data *pd; + + pd =dev_get_platdata (dev); +if (pd) { +memcpy (pdata, pd,sizeof (*pdata)); +return 0 ; + } + + id =i2c_match_id (at24_ids,to_i2c_client (dev)); + +/* +* The I2C core allows OF nodes compatibles to match against the +* I2C device ID table as a fallback, so check not only if an OF +* node is present but also if it matches an OF device ID entry. +*/ +if (of_node &&of_match_device (at24_of_match, dev)) + cdata =of_device_get_match_data (dev); +else if (id) + cdata = (void *)&id->driver_data; +else + cdata =acpi_device_get_match_data (dev); + +if (!cdata) +return -ENODEV ; + + pdata->byte_len = cdata->byte_len; + pdata->flags = cdata->flags; +at24_properties_to_pdata (dev, pdata); + +return 0 ; +} +static unsigned int at24_get_offset_adj (u8 flags,unsigned int byte_len) {if (flags &AT24_FLAG_MAC ) {
@@ -520,10 +557,8 @@ static int at24_probe(struct i2c_client *client) {struct regmap_config regmap_config = { };struct nvmem_config nvmem_config = { }; -const struct at24_chip_data *cd =NULL ;struct at24_platform_data pdata = { };struct device *dev = &client->dev; -const struct i2c_device_id *id;unsigned int i, num_addresses;struct at24_data *at24;size_t at24_size;
@@ -531,34 +566,9 @@ static int at24_probe(struct i2c_client *client)u8 test_byte;int err; - id =i2c_match_id (at24_ids, client); - -if (dev->platform_data) { - pdata = *(struct at24_platform_data *)dev->platform_data; - }else { -/* -* The I2C core allows OF nodes compatibles to match against the -* I2C device ID table as a fallback, so check not only if an OF -* node is present but also if it matches an OF device ID entry. -*/ -if (dev->of_node &&of_match_device (at24_of_match, dev)) { - cd =of_device_get_match_data (dev); - }else if (id) { - cd = (void *)id->driver_data; - }else { -const struct acpi_device_id *aid; - - aid =acpi_match_device (at24_acpi_ids, dev); -if (aid) - cd = (void *)aid->driver_data; - } -if (!cd) -return -ENODEV ; - - pdata.byte_len = cd->byte_len; - pdata.flags = cd->flags; -at24_properties_to_pdata (dev, &pdata); - } + err =at24_get_pdata (dev, &pdata); +if (err) +return err;if (!pdata.page_size) {dev_err (dev,"page_size must not be 0!\n" );
Signature · 1 line
2.16.1
PATCH 17/21 eeprom: at24: remove at24_platform_data from at24_data
In reply to Bartosz GolaszewskiNot all fields from at24_platform_data are needed in at24_data. Let's keep just the ones we need and not carry the whole platform_data structure all the time.
--- drivers/misc/eeprom/at24.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
drivers/misc/eeprom/at24.c +13 -9
@@ -60,8 +60,6 @@ struct at24_client { };struct at24_data { -struct at24_platform_data chip; -/* * Lock protects against activities from other Linux tasks, * but not from changes by other I2C masters.
@@ -72,6 +70,10 @@ struct at24_data {unsigned int num_addresses;unsigned int offset_adj; +u32 byte_len; +u16 page_size; +u8 flags; +struct nvmem_device *nvmem;struct gpio_desc *wp_gpio;
@@ -249,7 +251,7 @@ static struct at24_client *at24_translate_offset(struct at24_data *at24, {unsigned int i; -if (at24->chip.flags &AT24_FLAG_ADDR16 ) { +if (at24->flags &AT24_FLAG_ADDR16 ) { i = *offset >>16 ; *offset &=0xffff ; }else {
@@ -276,8 +278,8 @@ static size_t at24_adjust_read_count(struct at24_data *at24,* the next slave address: truncate the count to the slave boundary, * so that the read never straddles slaves. */ -if (at24->chip.flags &AT24_FLAG_NO_RDROL ) { - bits = (at24->chip.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; +if (at24->flags &AT24_FLAG_NO_RDROL ) { + bits = (at24->flags &AT24_FLAG_ADDR16 ) ?16 :8 ; remainder =BIT (bits) - offset;if (count > remainder) count = remainder;
@@ -336,7 +338,7 @@ static size_t at24_adjust_write_count(struct at24_data *at24, count = at24->write_max;/* Never roll over backwards, to the start of this page */ - next_page =roundup (offset +1 , at24->chip.page_size); + next_page =roundup (offset +1 , at24->page_size);if (offset + count > next_page) count = next_page - offset;
@@ -381,7 +383,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)if (unlikely (!count))return count; -if (off + count > at24->chip.byte_len) +if (off + count > at24->byte_len)return -EINVAL ; ret =pm_runtime_get_sync (dev);
@@ -428,7 +430,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)if (unlikely (!count))return -EINVAL ; -if (off + count > at24->chip.byte_len) +if (off + count > at24->byte_len)return -EINVAL ; ret =pm_runtime_get_sync (dev);
@@ -598,7 +600,9 @@ static int at24_probe(struct i2c_client *client)return -ENOMEM ;mutex_init (&at24->lock); - at24->chip = pdata; + at24->byte_len = pdata.byte_len; + at24->page_size = pdata.page_size; + at24->flags = pdata.flags; at24->num_addresses = num_addresses; at24->offset_adj =at24_get_offset_adj (pdata.flags, pdata.byte_len);
Signature · 1 line
2.16.1
PATCH 18/21 eeprom: at24: refactor at24_probe()
In reply to Bartosz GolaszewskiThe code in at24_probe() is pretty mangled. It can be cleaned up a bit by doing things one by one.
Let's group the code by logic: parse and verify pdata, initialize the regmap, allocate and fill the fields of at24_data, allocate dummy i2c devices, initialize pm & register with nvmem.
--- drivers/misc/eeprom/at24.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)
drivers/misc/eeprom/at24.c +18 -16
@@ -563,6 +563,7 @@ static int at24_probe(struct i2c_client *client)struct device *dev = &client->dev;unsigned int i, num_addresses;struct at24_data *at24; +struct regmap *regmap;size_t at24_size;bool writable;u8 test_byte;
@@ -572,6 +573,11 @@ static int at24_probe(struct i2c_client *client)if (err)return err; +if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && + !i2c_check_functionality (client->adapter, +I2C_FUNC_SMBUS_WRITE_I2C_BLOCK )) + pdata.page_size =1 ; +if (!pdata.page_size) {dev_err (dev,"page_size must not be 0!\n" );return -EINVAL ;
@@ -579,21 +585,26 @@ static int at24_probe(struct i2c_client *client)if (!is_power_of_2 (pdata.page_size))dev_warn (dev,"page_size looks suspicious (no power of 2)!\n" ); -if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && - !i2c_check_functionality (client->adapter, -I2C_FUNC_SMBUS_WRITE_I2C_BLOCK )) - pdata.page_size =1 ; -if (pdata.flags &AT24_FLAG_TAKE8ADDR ) num_addresses =8 ;else num_addresses =DIV_ROUND_UP (pdata.byte_len, (pdata.flags &AT24_FLAG_ADDR16 ) ?65536 :256 ); +if ((pdata.flags &AT24_FLAG_SERIAL ) && (pdata.flags &AT24_FLAG_MAC )) { +dev_err (dev, +"invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC." ); +return -EINVAL ; + } + regmap_config.val_bits =8 ; regmap_config.reg_bits = (pdata.flags &AT24_FLAG_ADDR16 ) ?16 :8 ; regmap_config.disable_locking = true; + regmap =devm_regmap_init_i2c (client, ®map_config); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); + at24_size =sizeof (*at24) + num_addresses *sizeof (struct at24_client ); at24 =devm_kzalloc (dev, at24_size,GFP_KERNEL );if (!at24)
@@ -605,22 +616,13 @@ static int at24_probe(struct i2c_client *client) at24->flags = pdata.flags; at24->num_addresses = num_addresses; at24->offset_adj =at24_get_offset_adj (pdata.flags, pdata.byte_len); + at24->client[0 ].client = client; + at24->client[0 ].regmap = regmap; at24->wp_gpio =devm_gpiod_get_optional (dev,"wp" ,GPIOD_OUT_HIGH );if (IS_ERR (at24->wp_gpio))return PTR_ERR (at24->wp_gpio); - at24->client[0 ].client = client; - at24->client[0 ].regmap =devm_regmap_init_i2c (client, ®map_config); -if (IS_ERR (at24->client[0 ].regmap)) -return PTR_ERR (at24->client[0 ].regmap); - -if ((pdata.flags &AT24_FLAG_SERIAL ) && (pdata.flags &AT24_FLAG_MAC )) { -dev_err (dev, -"invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC." ); -return -EINVAL ; - } - writable = !(pdata.flags &AT24_FLAG_READONLY );if (writable) { at24->write_max = min_t(unsigned int,
Signature · 1 line
2.16.1
PATCH 19/21 eeprom: at24: tweak newlines
In reply to Bartosz GolaszewskiRemove the newline between the nvmem registration and its return value check. This is consistent with the rest of the driver code.
Add a missing newline between two pdata checks to stay consistent with all the others.
--- drivers/misc/eeprom/at24.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/misc/eeprom/at24.c +1 -1
@@ -582,6 +582,7 @@ static int at24_probe(struct i2c_client *client)dev_err (dev,"page_size must not be 0!\n" );return -EINVAL ; } +if (!is_power_of_2 (pdata.page_size))dev_warn (dev,"page_size looks suspicious (no power of 2)!\n" );
@@ -683,7 +684,6 @@ static int at24_probe(struct i2c_client *client) nvmem_config.size = pdata.byte_len; at24->nvmem =nvmem_register (&nvmem_config); -if (IS_ERR (at24->nvmem)) { err =PTR_ERR (at24->nvmem); goto err_clients;
Signature · 1 line
2.16.1
PATCH 20/21 eeprom: at24: fix a line break
In reply to Bartosz GolaszewskiAlign the broken line with the opening parenthesis to stay consistent with the rest of the driver code.
--- drivers/misc/eeprom/at24.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +2 -2
@@ -690,8 +690,8 @@ static int at24_probe(struct i2c_client *client) }dev_info (dev,"%u byte %s EEPROM, %s, %u bytes/write\n" , - pdata.byte_len, client->name, - writable ?"writable" :"read-only" , at24->write_max); + pdata.byte_len, client->name, + writable ?"writable" :"read-only" , at24->write_max);/* export data to kernel code */ if (pdata.setup)
Signature · 1 line
2.16.1
PATCH 21/21 eeprom: at24: simplify the i2c functionality checking
In reply to Bartosz GolaszewskiSave one call and make code prettier by checking the i2c functionality in the beginning of at24_probe(), saving the relevant values and reusing them later.
--- drivers/misc/eeprom/at24.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/misc/eeprom/at24.c +7 -5
@@ -561,6 +561,7 @@ static int at24_probe(struct i2c_client *client)struct nvmem_config nvmem_config = { };struct at24_platform_data pdata = { };struct device *dev = &client->dev; +bool i2c_fn_i2c, i2c_fn_block;unsigned int i, num_addresses;struct at24_data *at24;struct regmap *regmap;
@@ -569,13 +570,15 @@ static int at24_probe(struct i2c_client *client)u8 test_byte;int err; + i2c_fn_i2c =i2c_check_functionality (client->adapter,I2C_FUNC_I2C ); + i2c_fn_block =i2c_check_functionality (client->adapter, +I2C_FUNC_SMBUS_WRITE_I2C_BLOCK ); + err =at24_get_pdata (dev, &pdata);if (err)return err; -if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && - !i2c_check_functionality (client->adapter, -I2C_FUNC_SMBUS_WRITE_I2C_BLOCK )) +if (!i2c_fn_i2c && !i2c_fn_block) pdata.page_size =1 ;if (!pdata.page_size) {
@@ -628,8 +631,7 @@ static int at24_probe(struct i2c_client *client)if (writable) { at24->write_max = min_t(unsigned int, pdata.page_size, at24_io_limit); -if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C ) && - at24->write_max >I2C_SMBUS_BLOCK_MAX ) +if (!i2c_fn_i2c && at24->write_max >I2C_SMBUS_BLOCK_MAX ) at24->write_max =I2C_SMBUS_BLOCK_MAX ; }
Signature · 1 line
2.16.1
Quoted 11 lines · Bartosz Golaszewski
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around
at24_probe()which became quite complicated and hard to read.The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if
byte_lenis a power of 2 (as we do support models for which it's not true).
Quoted 4 lines
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Is there any available tree with that series applied?
I would test it on Intel Galileo Gen 2 which has ACPI enumerated AT24 EEPROM attached.
Quoted 30 lines
Bartosz Golaszewski (21):
eeprom: at24: disable regmap locking
eeprom: at24: removenvmem_configfromat24_data
eeprom: at24: arrange local variables
eeprom: at24: use SPDX identifier instead of GPL boiler-plate
eeprom: at24: remove code separators
eeprom: at24: drop redundant variable inat24_read()
eeprom: at24: drop redundant variable inat24_write()
eeprom: at24: make struct initialization uniform inat24_probe()
eeprom: at24: don't check ifbyte_lenis a power of 2
eeprom: at24: renameat24_get_pdata()
eeprom: at24: rename chip to pdata inat24_probe()
eeprom: at24: use a helper variable for dev
eeprom: at24: readability tweak inat24_probe()
eeprom: at24: provide and useat24_base_client_dev()
eeprom: at24: switch to usingprobe_new()from the i2c framework
eeprom: at24: move platform data processing into a separate routine
eeprom: at24: removeat24_platform_datafromat24_data
eeprom: at24: refactorat24_probe()
eeprom: at24: tweak newlines
eeprom: at24: fix a line break
eeprom: at24: simplify the i2c functionality checkingdrivers/misc/eeprom/at24.c | 293 ++++++++++++++++++++++++--------------------- 1 file changed, 156 insertions(+), 137 deletions(-)--
2.16.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
2018-03-19 15:43 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 12 lines · Bartosz Golaszewski
Quoted 11 lines
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around
at24_probe()which became quite complicated and hard to read.The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if
byte_lenis a power of 2 (as we do support models for which it's not true).
Quoted 9 lines
Quoted 4 lines
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Is there any available tree with that series applied?
I would test it on Intel Galileo Gen 2 which has ACPI enumerated AT24 EEPROM attached.
Yes, it's in my github tree:
https://github.com/brgl/linux topic/at24/refactoring
Thanks in advance for testing it!
Best regards,
Bartosz
Andy Shevchenko
Tested-byQuoted 23 lines · Bartosz Golaszewski
2018-03-19 15:43 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 12 lines · Bartosz Golaszewski
Quoted 11 lines
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around
at24_probe()which became quite complicated and hard to read.The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if
byte_lenis a power of 2 (as we do support models for which it's not true).Quoted 9 lines
Quoted 4 lines
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Is there any available tree with that series applied?
I would test it on Intel Galileo Gen 2 which has ACPI enumerated AT24 EEPROM attached.
Quoted 5 lines
Yes, it's in my github tree:
https://github.com/brgl/linux topic/at24/refactoring
Thanks in advance for testing it!
At least this didn't break AT24 on Intel Galileo Gen 2 board in ACPI mode.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Greg Kroah-Hartman
maintainerQuoted 26 lines · Andy Shevchenko
Quoted 25 lines · Bartosz Golaszewski
2018-03-19 15:43 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 12 lines · Bartosz Golaszewski
Quoted 11 lines
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around
at24_probe()which became quite complicated and hard to read.The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if
byte_lenis a power of 2 (as we do support models for which it's not true).Quoted 9 lines
Quoted 4 lines
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Is there any available tree with that series applied?
I would test it on Intel Galileo Gen 2 which has ACPI enumerated AT24 EEPROM attached.Yes, it's in my github tree:
Quoted 7 lines
Quoted 3 lines
https://github.com/brgl/linux topic/at24/refactoring
Thanks in advance for testing it!
At least this didn't break AT24 on Intel Galileo Gen 2 board in ACPI mode.
Tested-by Andy Shevchenko <[email protected]>
All applied except for patch 4.
thanks,
greg k-h
2018-03-23 16:26 GMT+01:00 Greg Kroah-Hartman <[email protected]>:
Quoted 33 lines · Andy Shevchenko
Quoted 32 lines · Bartosz Golaszewski
Quoted 23 lines
2018-03-19 15:43 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 12 lines · Bartosz Golaszewski
Quoted 11 lines
This series contains what I hope to be a non-controversial refactoring of the at24 eeprom driver.
Most changes revolve around
at24_probe()which became quite complicated and hard to read.The only functional changes are: disabling the internal locking mechanisms of regmap (since we already take care of that in the driver) and removing an if checking if
byte_lenis a power of 2 (as we do support models for which it's not true).Quoted 9 lines
Quoted 4 lines
All other patches affect readability and code structure.
Tested with a couple models and different both for device tree and platform data modes.
Is there any available tree with that series applied?
I would test it on Intel Galileo Gen 2 which has ACPI enumerated AT24 EEPROM attached.Quoted 5 lines
Yes, it's in my github tree:
https://github.com/brgl/linux topic/at24/refactoring
Thanks in advance for testing it!
At least this didn't break AT24 on Intel Galileo Gen 2 board in ACPI mode.
Quoted 7 lines
Quoted 1 line
Tested-by Andy Shevchenko <[email protected]>All applied except for patch 4.
thanks,
greg k-h
Hi Greg,
I maintain this driver now and my pull requests usually go through Wolfram's i2c tree (Cc'ed). I initially intended for this series to be applied for 4.18. For 4.17 luckily the only other changes for at24 are device tree bindings, so I guess we can keep it in misc-char, since there are no conflicts.
Let me resend the corrected patch 4.
Best regards,
Bartosz Golaszewski