This series includes a number of improvements to the ee1004 driver.
Heiner Kallweit (13):
eeprom: ee1004: Use kobj_to_i2c_client to simplify the code
eeprom: ee1004: Remove not needed check in ee1004_read
eeprom: ee1004: Remove not needed check in ee1004_eeprom_read
eeprom: ee1004: Remove usage of i2c_adapter_id in adapter comparison
eeprom: ee1004: Improve check for SMBUS features
eeprom: ee1004: Improve creating dummy devices
eeprom: ee1004: Switch to i2c probe_new callback
eeprom: ee1004: Cache current page at initialization of first device
only
eeprom: ee1004: Factor out setting page to ee1004_set_current_page
eeprom: ee1004: Improve error handling in ee1004_read
eeprom: ee1004: Move call to ee1004_set_current_page to
ee1004_eeprom_read
eeprom: ee1004: Add constant EE1004_NUM_PAGES
eeprom: ee1004: Add helper ee1004_cleanup
drivers/misc/eeprom/ee1004.c | 191 +++++++++++++++--------------------
1 file changed, 80 insertions(+), 111 deletions(-)
--
2.31.1
Switch to helper kobj_to_i2c_client() to simplify the code.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 0950d4d9d..0613a5300 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -93,8 +93,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr,
char *buf, loff_t off, size_t count)
{
- struct device *dev = kobj_to_dev(kobj);
- struct i2c_client *client = to_i2c_client(dev);
+ struct i2c_client *client = kobj_to_i2c_client(kobj);
size_t requested = count;
int page;
--
2.31.1
sysfs_kf_bin_read() checks this for us already. In addition
the function works correctly also w/o this check.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 0613a5300..6aff333ff 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -97,9 +97,6 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
size_t requested = count;
int page;
- if (unlikely(!count))
- return count;
-
page = off >> EE1004_PAGE_SHIFT;
if (unlikely(page > 1))
return 0;
--
2.31.1
i2c_smbus_read_i2c_block_data_or_emulated() checks its length argument,
so we don't have to do it. In addition remove the unlikely hint from
the checks, we do i2c reads and therefore are in a slow path.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 6aff333ff..2824dba76 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -76,10 +76,8 @@ static ssize_t ee1004_eeprom_read(struct i2c_client *client, char *buf,
{
int status;
- if (count > I2C_SMBUS_BLOCK_MAX)
- count = I2C_SMBUS_BLOCK_MAX;
/* Can't cross page boundaries */
- if (unlikely(offset + count > EE1004_PAGE_SIZE))
+ if (offset + count > EE1004_PAGE_SIZE)
count = EE1004_PAGE_SIZE - offset;
status = i2c_smbus_read_i2c_block_data_or_emulated(client, offset,
--
2.31.1
We can compare the adapter pointers directly instead of using
i2c_adapter_id().
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 2824dba76..b991ab250 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -199,8 +199,7 @@ static int ee1004_probe(struct i2c_client *client,
goto err_clients;
}
}
- } else if (i2c_adapter_id(client->adapter) !=
- i2c_adapter_id(ee1004_set_page[0]->adapter)) {
+ } else if (client->adapter != ee1004_set_page[0]->adapter) {
dev_err(&client->dev,
"Driver only supports devices on a single I2C bus\n");
err = -EOPNOTSUPP;
--
2.31.1
i2c_new_dummy_device() calls i2c_new_client_device() that complains
if it fails to create the device. Therefore we don't have to emit an
error message in case of failure. In addition ensure that
ee1004_set_page is only set if creating the device succeeded.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 0d497e0e4..4b2c60a18 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -179,15 +179,14 @@ static int ee1004_probe(struct i2c_client *client,
mutex_lock(&ee1004_bus_lock);
if (++ee1004_dev_count == 1) {
for (cnr = 0; cnr < 2; cnr++) {
- ee1004_set_page[cnr] = i2c_new_dummy_device(client->adapter,
- EE1004_ADDR_SET_PAGE + cnr);
- if (IS_ERR(ee1004_set_page[cnr])) {
- dev_err(&client->dev,
- "address 0x%02x unavailable\n",
- EE1004_ADDR_SET_PAGE + cnr);
- err = PTR_ERR(ee1004_set_page[cnr]);
+ struct i2c_client *cl;
+
+ cl = i2c_new_dummy_device(client->adapter, EE1004_ADDR_SET_PAGE + cnr);
+ if (IS_ERR(cl)) {
+ err = PTR_ERR(cl);
goto err_clients;
}
+ ee1004_set_page[cnr] = cl;
}
} else if (client->adapter != ee1004_set_page[0]->adapter) {
dev_err(&client->dev,
--
2.31.1
Switch to the new i2c_driver probe callback version.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 4b2c60a18..460cc22ea 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -163,8 +163,7 @@ static struct bin_attribute *ee1004_attrs[] = {
BIN_ATTRIBUTE_GROUPS(ee1004);
-static int ee1004_probe(struct i2c_client *client,
- const struct i2c_device_id *id)
+static int ee1004_probe(struct i2c_client *client)
{
int err, cnr = 0;
@@ -246,7 +245,7 @@ static struct i2c_driver ee1004_driver = {
.name = "ee1004",
.dev_groups = ee1004_groups,
},
- .probe = ee1004_probe,
+ .probe_new = ee1004_probe,
.remove = ee1004_remove,
.id_table = ee1004_ids,
};
--
2.31.1
Add a constant for the number of pages.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 65fe11d8f..5173d040c 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -32,16 +32,17 @@
*/
#define EE1004_ADDR_SET_PAGE 0x36
-#define EE1004_EEPROM_SIZE 512
+#define EE1004_NUM_PAGES 2
#define EE1004_PAGE_SIZE 256
#define EE1004_PAGE_SHIFT 8
+#define EE1004_EEPROM_SIZE (EE1004_PAGE_SIZE * EE1004_NUM_PAGES)
/*
* Mutex protects ee1004_set_page and ee1004_dev_count, and must be held
* from page selection to end of read.
*/
static DEFINE_MUTEX(ee1004_bus_lock);
-static struct i2c_client *ee1004_set_page[2];
+static struct i2c_client *ee1004_set_page[EE1004_NUM_PAGES];
static unsigned int ee1004_dev_count;
static int ee1004_current_page;
@@ -172,7 +173,7 @@ static int ee1004_probe(struct i2c_client *client)
/* Use 2 dummy devices for page select command */
mutex_lock(&ee1004_bus_lock);
if (++ee1004_dev_count == 1) {
- for (cnr = 0; cnr < 2; cnr++) {
+ for (cnr = 0; cnr < EE1004_NUM_PAGES; cnr++) {
struct i2c_client *cl;
cl = i2c_new_dummy_device(client->adapter, EE1004_ADDR_SET_PAGE + cnr);
@@ -222,7 +223,7 @@ static int ee1004_remove(struct i2c_client *client)
/* Remove page select clients if this is the last device */
mutex_lock(&ee1004_bus_lock);
if (--ee1004_dev_count == 0) {
- for (i = 0; i < 2; i++) {
+ for (i = 0; i < EE1004_NUM_PAGES; i++) {
i2c_unregister_device(ee1004_set_page[i]);
ee1004_set_page[i] = NULL;
}
--
2.31.1
The value of ee1004_current_page applies to all SPD eeproms connected
to the adapter. Therefore it's sufficient if we set ee1004_current_page
when the first device is added.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 460cc22ea..d7c693b26 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -187,20 +187,19 @@ static int ee1004_probe(struct i2c_client *client)
}
ee1004_set_page[cnr] = cl;
}
+
+ /* Remember current page to avoid unneeded page select */
+ err = ee1004_get_current_page();
+ if (err < 0)
+ goto err_clients;
+ dev_dbg(&client->dev, "Currently selected page: %d\n", err);
+ ee1004_current_page = err;
} else if (client->adapter != ee1004_set_page[0]->adapter) {
dev_err(&client->dev,
"Driver only supports devices on a single I2C bus\n");
err = -EOPNOTSUPP;
goto err_clients;
}
-
- /* Remember current page to avoid unneeded page select */
- err = ee1004_get_current_page();
- if (err < 0)
- goto err_clients;
- ee1004_current_page = err;
- dev_dbg(&client->dev, "Currently selected page: %d\n",
- ee1004_current_page);
mutex_unlock(&ee1004_bus_lock);
dev_info(&client->dev,
--
2.31.1
Factor out setting the page, this makes the code better readable.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 52 +++++++++++++++++++++---------------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index d7c693b26..33855e459 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -71,6 +71,32 @@ static int ee1004_get_current_page(void)
return 0;
}
+static int ee1004_set_current_page(struct device *dev, int page)
+{
+ int ret;
+
+ if (page == ee1004_current_page)
+ return 0;
+
+ /* Data is ignored */
+ ret = i2c_smbus_write_byte(ee1004_set_page[page], 0x00);
+ /*
+ * Don't give up just yet. Some memory modules will select the page
+ * but not ack the command. Check which page is selected now.
+ */
+ if (ret == -ENXIO && ee1004_get_current_page() == page)
+ ret = 0;
+ if (ret < 0) {
+ dev_err(dev, "Failed to select page %d (%d)\n", page, ret);
+ return ret;
+ }
+
+ dev_dbg(dev, "Selected page %d\n", page);
+ ee1004_current_page = page;
+
+ return 0;
+}
+
static ssize_t ee1004_eeprom_read(struct i2c_client *client, char *buf,
unsigned int offset, size_t count)
{
@@ -110,28 +136,10 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
int status;
/* Select page */
- if (page != ee1004_current_page) {
- /* Data is ignored */
- status = i2c_smbus_write_byte(ee1004_set_page[page],
- 0x00);
- if (status == -ENXIO) {
- /*
- * Don't give up just yet. Some memory
- * modules will select the page but not
- * ack the command. Check which page is
- * selected now.
- */
- if (ee1004_get_current_page() == page)
- status = 0;
- }
- if (status < 0) {
- dev_err(dev, "Failed to select page %d (%d)\n",
- page, status);
- mutex_unlock(&ee1004_bus_lock);
- return status;
- }
- dev_dbg(dev, "Selected page %d\n", page);
- ee1004_current_page = page;
+ status = ee1004_set_current_page(dev, page);
+ if (status) {
+ mutex_unlock(&ee1004_bus_lock);
+ return status;
}
status = ee1004_eeprom_read(client, buf, off, count);
--
2.31.1
Moving the call to ee1004_set_current_page() to ee1004_eeprom_read()
allows to simplify the code.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index d18348ee4..65fe11d8f 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -100,7 +100,14 @@ static int ee1004_set_current_page(struct device *dev, int page)
static ssize_t ee1004_eeprom_read(struct i2c_client *client, char *buf,
unsigned int offset, size_t count)
{
- int status;
+ int status, page;
+
+ page = offset >> EE1004_PAGE_SHIFT;
+ offset &= (1 << EE1004_PAGE_SHIFT) - 1;
+
+ status = ee1004_set_current_page(&client->dev, page);
+ if (status)
+ return status;
/* Can't cross page boundaries */
if (offset + count > EE1004_PAGE_SIZE)
@@ -119,12 +126,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
{
struct i2c_client *client = kobj_to_i2c_client(kobj);
size_t requested = count;
- int page, ret = 0;
-
- page = off >> EE1004_PAGE_SHIFT;
- if (unlikely(page > 1))
- return 0;
- off &= (1 << EE1004_PAGE_SHIFT) - 1;
+ int ret = 0;
/*
* Read data from chip, protecting against concurrent access to
@@ -133,11 +135,6 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
mutex_lock(&ee1004_bus_lock);
while (count) {
- /* Select page */
- ret = ee1004_set_current_page(dev, page);
- if (ret)
- goto out;
-
ret = ee1004_eeprom_read(client, buf, off, count);
if (ret < 0)
goto out;
@@ -145,11 +142,6 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
buf += ret;
off += ret;
count -= ret;
-
- if (off == EE1004_PAGE_SIZE) {
- page++;
- off = 0;
- }
}
out:
mutex_unlock(&ee1004_bus_lock);
--
2.31.1
Simplify the error handling and make it better readable. No functional
change intended.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 33855e459..d18348ee4 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -119,7 +119,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
{
struct i2c_client *client = kobj_to_i2c_client(kobj);
size_t requested = count;
- int page;
+ int page, ret = 0;
page = off >> EE1004_PAGE_SHIFT;
if (unlikely(page > 1))
@@ -133,33 +133,28 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
mutex_lock(&ee1004_bus_lock);
while (count) {
- int status;
-
/* Select page */
- status = ee1004_set_current_page(dev, page);
- if (status) {
- mutex_unlock(&ee1004_bus_lock);
- return status;
- }
+ ret = ee1004_set_current_page(dev, page);
+ if (ret)
+ goto out;
- status = ee1004_eeprom_read(client, buf, off, count);
- if (status < 0) {
- mutex_unlock(&ee1004_bus_lock);
- return status;
- }
- buf += status;
- off += status;
- count -= status;
+ ret = ee1004_eeprom_read(client, buf, off, count);
+ if (ret < 0)
+ goto out;
+
+ buf += ret;
+ off += ret;
+ count -= ret;
if (off == EE1004_PAGE_SIZE) {
page++;
off = 0;
}
}
-
+out:
mutex_unlock(&ee1004_bus_lock);
- return requested;
+ return ret < 0 ? ret : requested;
}
static BIN_ATTR_RO(eeprom, EE1004_EEPROM_SIZE);
--
2.31.1
We have to read 512 bytes only, therefore read performance isn't really
a concern. Don't bother the user if i2c block read isn't supported.
For i2c_smbus_read_i2c_block_data_or_emulated() to work it's sufficient
if I2C_FUNC_SMBUS_READ_I2C_BLOCK or I2C_FUNC_SMBUS_READ_BYTE_DATA is
supported. Therefore remove the check for I2C_FUNC_SMBUS_READ_WORD_DATA.
In addition check for I2C_FUNC_SMBUS_WRITE_BYTE (included in
I2C_FUNC_SMBUS_BYTE) which is needed for setting the page.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index b991ab250..0d497e0e4 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -167,23 +167,13 @@ static int ee1004_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
int err, cnr = 0;
- const char *slow = NULL;
/* Make sure we can operate on this adapter */
if (!i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_READ_BYTE |
- I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
- if (i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_READ_BYTE |
- I2C_FUNC_SMBUS_READ_WORD_DATA))
- slow = "word";
- else if (i2c_check_functionality(client->adapter,
- I2C_FUNC_SMBUS_READ_BYTE |
- I2C_FUNC_SMBUS_READ_BYTE_DATA))
- slow = "byte";
- else
- return -EPFNOSUPPORT;
- }
+ I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_READ_I2C_BLOCK) &&
+ !i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_READ_BYTE_DATA))
+ return -EPFNOSUPPORT;
/* Use 2 dummy devices for page select command */
mutex_lock(&ee1004_bus_lock);
@@ -218,10 +208,6 @@ static int ee1004_probe(struct i2c_client *client,
dev_info(&client->dev,
"%u byte EE1004-compliant SPD EEPROM, read-only\n",
EE1004_EEPROM_SIZE);
- if (slow)
- dev_notice(&client->dev,
- "Falling back to %s reads, performance will suffer\n",
- slow);
return 0;
--
2.31.1
Factor out the cleanup code to a new helper ee1004_cleanup().
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/misc/eeprom/ee1004.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/misc/eeprom/ee1004.c b/drivers/misc/eeprom/ee1004.c
index 5173d040c..00f61a83d 100644
--- a/drivers/misc/eeprom/ee1004.c
+++ b/drivers/misc/eeprom/ee1004.c
@@ -159,6 +159,15 @@ static struct bin_attribute *ee1004_attrs[] = {
BIN_ATTRIBUTE_GROUPS(ee1004);
+static void ee1004_cleanup(int idx)
+{
+ if (--ee1004_dev_count == 0)
+ while (--idx >= 0) {
+ i2c_unregister_device(ee1004_set_page[idx]);
+ ee1004_set_page[idx] = NULL;
+ }
+}
+
static int ee1004_probe(struct i2c_client *client)
{
int err, cnr = 0;
@@ -205,12 +214,7 @@ static int ee1004_probe(struct i2c_client *client)
return 0;
err_clients:
- if (--ee1004_dev_count == 0) {
- for (cnr--; cnr >= 0; cnr--) {
- i2c_unregister_device(ee1004_set_page[cnr]);
- ee1004_set_page[cnr] = NULL;
- }
- }
+ ee1004_cleanup(cnr);
mutex_unlock(&ee1004_bus_lock);
return err;
@@ -218,16 +222,9 @@ static int ee1004_probe(struct i2c_client *client)
static int ee1004_remove(struct i2c_client *client)
{
- int i;
-
/* Remove page select clients if this is the last device */
mutex_lock(&ee1004_bus_lock);
- if (--ee1004_dev_count == 0) {
- for (i = 0; i < EE1004_NUM_PAGES; i++) {
- i2c_unregister_device(ee1004_set_page[i]);
- ee1004_set_page[i] = NULL;
- }
- }
+ ee1004_cleanup(EE1004_NUM_PAGES);
mutex_unlock(&ee1004_bus_lock);
return 0;
--
2.31.1