eeprom: ee1004: Enable devices on multiple busses
v3 cover eeprom: ee1004: Enable devices on multiple busses
The driver previously prevented probing devices on more than one bus due to locking constraints with the special page addresses. This constraint can be removed by allocating a reference-counted bus structure containing the lock, rather than using global variables. In addition, add devicetree bindings for the EE1004 driver for the AT30TSE device and add the devices to the Bonnell BMC system.
Changes since v2:
- Remove of_device.h include
- Use the full model number of the device for the binding
- Fix commit name for the bindings documentation and the OF matching support
Changes since v1:
- Add the devicetree changes
Eddie James (4):
eeprom: ee1004: Enable devices on multiple busses
dt-bindings: trivial-devices: Add Atmel AT30TSE004A serial eeprom
eeprom: ee1004: Add OF matching support
ARM: dts: aspeed: bonnell: Add DIMM SPD
.../devicetree/bindings/trivial-devices.yaml | 2 + arch/arm/boot/dts/aspeed-bmc-ibm-bonnell.dts | 20 ++ drivers/misc/eeprom/ee1004.c | 181 +++++++++++------- 3 files changed, 134 insertions(+), 69 deletions(-)
Signature · 1 line
2.31.1
PATCH v3 1/4 eeprom: ee1004: Enable devices on multiple busses
The driver previously prevented probing devices on more than one bus due to locking constraints with the special page addresses. This constraint can be removed by allocating a reference-counted bus structure containing the lock, rather than using global variables.
---
Changes since v2:
- Remove of_device.h include
drivers/misc/eeprom/ee1004.c | 174 +++++++++++++++++++++-------------- 1 file changed, 105 insertions(+), 69 deletions(-)
drivers/misc/eeprom/ee1004.c +105 -69
@@ -9,9 +9,11 @@* Copyright (C) 2008 Wolfram Sang, Pengutronix */ +#include <linux/err.h> #include <linux/i2c.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/list.h> #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/mutex.h>
@@ -31,20 +33,24 @@* over performance. */ -#define EE1004_ADDR_SET_PAGE 0x36 +#define EE1004_ADDR_SET_PAGE0 0x36 +#define EE1004_ADDR_SET_PAGE1 0x37#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[EE1004_NUM_PAGES ]; -static unsigned int ee1004_dev_count; -static int ee1004_current_page; +struct ee1004_bus { +struct kref kref; +struct list_head list; +struct mutex lock; +struct i2c_adapter *adapter; +struct i2c_client *set_page_clients[EE1004_NUM_PAGES ]; +int page; +}; + +static LIST_HEAD (ee1004_busses); +static DEFINE_MUTEX (ee1004_busses_lock);static const struct i2c_device_id ee1004_ids[] = { {"ee1004" ,0 },
@@ -54,11 +60,11 @@ MODULE_DEVICE_TABLE(i2c, ee1004_ids);/*-------------------------------------------------------------------------*/ -static int ee1004_get_current_page (void ) +static int ee1004_get_current_page (struct ee1004_bus *bus) {int err; - err =i2c_smbus_read_byte (ee1004_set_page[0 ]); + err =i2c_smbus_read_byte (bus->set_page_clients[0 ]);if (err == -ENXIO ) {/* Nack means page 1 is selected */ return 1 ;
@@ -72,33 +78,30 @@ static int ee1004_get_current_page(void)return 0 ; } -static int ee1004_set_current_page (struct device *dev,int page) +static int ee1004_set_current_page (struct ee1004_bus *bus,int page) {int ret; -if (page == ee1004_current_page) +if (page == bus->page)return 0 ;/* Data is ignored */ - ret =i2c_smbus_write_byte (ee1004_set_page[page],0x00 ); + ret =i2c_smbus_write_byte (bus->set_page_clients[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) +if (ret == -ENXIO &&ee1004_get_current_page (bus) == page) ret =0 ; -if (ret <0 ) { -dev_err (dev,"Failed to select page %d (%d)\n" , page, ret); +if (ret <0 )return ret; - } - -dev_dbg (dev,"Selected page %d\n" , page); - ee1004_current_page = page; + bus->page = page;return 0 ; } -static ssize_t ee1004_eeprom_read (struct i2c_client *client,char *buf, +static ssize_t ee1004_eeprom_read (struct i2c_client *client,struct ee1004_bus *bus,char *buf,unsigned int offset,size_t count) {int status, page;
@@ -106,9 +109,11 @@ static ssize_t ee1004_eeprom_read(struct i2c_client *client, char *buf, page = offset >>EE1004_PAGE_SHIFT ; offset &= (1 <<EE1004_PAGE_SHIFT ) -1 ; - status =ee1004_set_current_page (&client->dev, page); -if (status) + status =ee1004_set_current_page (bus, page); +if (status) { +dev_err (&client->dev,"Failed to select page %d (%d)\n" , page, status);return status; + }/* Can't cross page boundaries */ if (offset + count >EE1004_PAGE_SIZE )
@@ -125,6 +130,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj, char *buf, loff_t off, size_t count) {struct i2c_client *client =kobj_to_i2c_client (kobj); +struct ee1004_bus *bus =i2c_get_clientdata (client);size_t requested = count;int ret =0 ;
@@ -132,10 +138,10 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,* Read data from chip, protecting against concurrent access to * other EE1004 SPD EEPROMs on the same adapter. */ -mutex_lock (&ee1004_bus_lock); +mutex_lock (&bus->lock);while (count) { - ret =ee1004_eeprom_read (client, buf, off, count); + ret =ee1004_eeprom_read (client, bus, buf, off, count);if (ret <0 ) goto out;
@@ -144,7 +150,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj, count -= ret; } out: -mutex_unlock (&ee1004_bus_lock); +mutex_unlock (&bus->lock);return ret <0 ? ret : requested; }
@@ -158,18 +164,56 @@ static struct bin_attribute *ee1004_attrs[] = {BIN_ATTRIBUTE_GROUPS (ee1004); -static void ee1004_cleanup (int idx) +static void ee1004_bus_unregister (struct ee1004_bus *bus) { -if (--ee1004_dev_count ==0 ) -while (--idx >=0 ) { -i2c_unregister_device (ee1004_set_page[idx]); - ee1004_set_page[idx] =NULL ; - } +i2c_unregister_device (bus->set_page_clients[1 ]); +i2c_unregister_device (bus->set_page_clients[0 ]); +} + +static void ee1004_bus_release (struct kref *kref) +{ + struct ee1004_bus *bus = container_of(kref, struct ee1004_bus, kref); + +ee1004_bus_unregister (bus); + +mutex_lock (&ee1004_busses_lock); +list_del (&bus->list); +mutex_unlock (&ee1004_busses_lock); + +kfree (bus); +} + +static int ee1004_bus_initialize (struct ee1004_bus *bus,struct i2c_adapter *adapter) +{ + bus->set_page_clients[0 ] =i2c_new_dummy_device (adapter,EE1004_ADDR_SET_PAGE0 ); +if (IS_ERR (bus->set_page_clients[0 ])) +return PTR_ERR (bus->set_page_clients[0 ]); + + bus->set_page_clients[1 ] =i2c_new_dummy_device (adapter,EE1004_ADDR_SET_PAGE1 ); +if (IS_ERR (bus->set_page_clients[1 ])) { +i2c_unregister_device (bus->set_page_clients[0 ]); +return PTR_ERR (bus->set_page_clients[1 ]); + } + + bus->page =ee1004_get_current_page (bus); +if (bus->page <0 ) { +ee1004_bus_unregister (bus); +return bus->page; + } + +kref_init (&bus->kref); +list_add (&bus->list, &ee1004_busses); +mutex_init (&bus->lock); + bus->adapter = adapter; + +return 0 ; } static int ee1004_probe(struct i2c_client *client) { - int err, cnr = 0; + struct ee1004_bus *bus; + bool found = false; + int rc = 0; /* Make sure we can operate on this adapter */ if (!i2c_check_functionality(client->adapter,
@@ -178,53 +222,45 @@ static int ee1004_probe(struct i2c_client *client) 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); -if (++ee1004_dev_count ==1 ) { -for (cnr =0 ; cnr <EE1004_NUM_PAGES ; 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; +mutex_lock (&ee1004_busses_lock); +list_for_each_entry (bus, &ee1004_busses, list) { +if (bus->adapter == client->adapter) { +kref_get (&bus->kref); + found = true; +break ; + } + } + +if (!found) { + bus =kzalloc (sizeof (*bus),GFP_KERNEL ); +if (!bus) { + rc = -ENOMEM ; + goto unlock; } -/* 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; + rc =ee1004_bus_initialize (bus, client->adapter); +if (rc) { +kfree (bus); + goto unlock; + } } -mutex_unlock (&ee1004_bus_lock); + +i2c_set_clientdata (client, bus);dev_info (&client->dev,"%u byte EE1004-compliant SPD EEPROM, read-only\n" ,EE1004_EEPROM_SIZE ); -return 0 ; - - err_clients: -ee1004_cleanup (cnr); -mutex_unlock (&ee1004_bus_lock); - -return err; +unlock: +mutex_unlock (&ee1004_busses_lock); +return rc; }static void ee1004_remove (struct i2c_client *client) { -/* Remove page select clients if this is the last device */ -mutex_lock (&ee1004_bus_lock); -ee1004_cleanup (EE1004_NUM_PAGES ); -mutex_unlock (&ee1004_bus_lock); +struct ee1004_bus *bus =i2c_get_clientdata (client); + +kref_put (&bus->kref, ee1004_bus_release); }/*-------------------------------------------------------------------------*/
Signature · 1 line
2.31.1
PATCH v3 2/4 dt-bindings: trivial-devices: Add Atmel AT30TSE004A serial eeprom
In reply to Eddie JamesThe AT30TSE004A EEPROM is compatible with the JEDEC EE1004 standard. Document it as a trivial I2C device.
---
Changes since v2:
- Use full model name
- Fix commit name
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/trivial-devices.yaml +2 -0
@@ -47,6 +47,8 @@ properties: - ams,iaq-core # i2c serial eeprom (24cxx) - at,24c08 + # i2c serial eeprom (EE1004 standard) + - atmel,at30tse004a # i2c trusted platform module (TPM) - atmel,at97sc3204t # ATSHA204 - i2c h/w symmetric crypto module
Signature · 1 line
2.31.1
PATCH v3 3/4 eeprom: ee1004: Add OF matching support
In reply to Eddie JamesAdd an OF match table for devicetree instantiation of the AT30TSE004A EEPROM.
---
Changes since v2:
- Use full model number
- Fix commit name
drivers/misc/eeprom/ee1004.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/misc/eeprom/ee1004.c +7 -0
@@ -58,6 +58,12 @@ static const struct i2c_device_id ee1004_ids[] = { };MODULE_DEVICE_TABLE (i2c, ee1004_ids); +static const struct of_device_id ee1004_of_match[] = { + { .compatible ="atmel,at30tse004a" }, + {} +}; +MODULE_DEVICE_TABLE (of, ee1004_of_match); +/*-------------------------------------------------------------------------*/ static int ee1004_get_current_page (struct ee1004_bus *bus)
@@ -269,6 +275,7 @@ static struct i2c_driver ee1004_driver = { .driver = { .name = "ee1004", .dev_groups = ee1004_groups, + .of_match_table = ee1004_of_match, }, .probe_new = ee1004_probe, .remove = ee1004_remove,
Signature · 1 line
2.31.1
PATCH v3 4/4 ARM: dts: aspeed: bonnell: Add DIMM SPD
In reply to Eddie JamesAdd the DIMM SPD to the processor I2C busses.
--- arch/arm/boot/dts/aspeed-bmc-ibm-bonnell.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
arch/arm/boot/dts/aspeed-bmc-ibm-bonnell.dts +20 -0
@@ -232,18 +232,38 @@ cfam0_i2c1: i2c-bus at 1 { cfam0_i2c10: i2c-bus at a { reg = <10>; /* OP3A */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c11: i2c-bus at b { reg = <11>; /* OP3B */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c12: i2c-bus at c { reg = <12>; /* OP4A */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c13: i2c-bus at d { reg = <13>; /* OP4B */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c14: i2c-bus at e {
Signature · 1 line
2.31.1
Re: [PATCH v3 2/4] dt-bindings: trivial-devices: Add Atmel AT30TSE004A serial eeprom
In reply to Eddie JamesQuoted 5 lines · Eddie James
The AT30TSE004A EEPROM is compatible with the JEDEC EE1004 standard. Document it as a trivial I2C device.
Signed-off-by Eddie James <[email protected]>---
Best regards,
Krzysztof
Re: [PATCH v3 4/4] ARM: dts: aspeed: bonnell: Add DIMM SPD
In reply to Eddie JamesQuoted 4 lines · Eddie James
Add the DIMM SPD to the processor I2C busses.
Signed-off-by Eddie James <[email protected]>
I will take this through the aspeed tree.
Quoted 50 lines
--- arch/arm/boot/dts/aspeed-bmc-ibm-bonnell.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)arch/arm/boot/dts/aspeed-bmc-ibm-bonnell.dts +20
-1@@ -232,18 +232,38 @@ cfam0_i2c1: i2c-bus at 1 { cfam0_i2c10: i2c-bus at a { reg = <10>; /* OP3A */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c11: i2c-bus at b { reg = <11>; /* OP3B */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c12: i2c-bus at c { reg = <12>; /* OP4A */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c13: i2c-bus at d { reg = <13>; /* OP4B */ + + eeprom at 50 { + compatible = "atmel,at30tse004a"; + reg = <0x50>; + }; }; cfam0_i2c14: i2c-bus at e { --2.31.1
Greg KH
maintainerQuoted 50 lines · Eddie James
The driver previously prevented probing devices on more than one bus due to locking constraints with the special page addresses. This constraint can be removed by allocating a reference-counted bus structure containing the lock, rather than using global variables.
Signed-off-by Eddie James <[email protected]>---
Changes since v2:
- Removeof_device.hincludedrivers/misc/eeprom/ee1004.c | 174 +++++++++++++++++++++-------------- 1 file changed, 105 insertions(+), 69 deletions(-)drivers/misc/eeprom/ee1004.c +5
-9@@ -9,9 +9,11 @@* Copyright (C) 2008 Wolfram Sang, Pengutronix */ +#include <linux/err.h> #include <linux/i2c.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/list.h> #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/mutex.h> @@ -31,20 +33,24 @@* over performance. */ -#define EE1004_ADDR_SET_PAGE 0x36 +#define EE1004_ADDR_SET_PAGE0 0x36 +#define EE1004_ADDR_SET_PAGE1 0x37#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[EE1004_NUM_PAGES ]; -static unsigned int ee1004_dev_count; -static int ee1004_current_page; +struct ee1004_bus {
drivers/misc/eeprom/ee1004.c line 53
Quoted 10 lines
drivers/misc/eeprom/ee1004.c +10
-0+struct kref kref; +struct list_head list; +struct mutex lock; +struct i2c_adapter *adapter; +struct i2c_client *set_page_clients[EE1004_NUM_PAGES ]; +int page; +}; + +static LIST_HEAD (ee1004_busses); +static DEFINE_MUTEX (ee1004_busses_lock);
This really looks like you are just emulating a tiny portion of the driver core (busses, lists of busses, reference counting, etc.)
Why not just use an aux device instead and get all of that logic "for free" in a way that will be properly shown to userspace? Right now it has no idea what is happening here with individual portions of the device and the like.
Please look into that instead of this hand-rolled device model.
thanks,
greg k-h