dm: Add I2C support and convert sandbox, tegra

Factor out the common code to make it easier to adjust it.

Signed-off-by Simon Glass <[email protected]>

Factor out the common code to make it easier to adjust it.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add a suitable commit message

  common/cmd_i2c.c | 32 ++++++++++++++++++++++----------
  1 file changed, 22 insertions(+), 10 deletions(-)

Hello Simon,

Am 11.11.2014 18:46, schrieb Simon Glass:

Quoted 6 lines

Factor out the common code to make it easier to adjust it.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 5 lines

Changes in v2:
- Add a suitable commit message

  common/cmd_i2c.c | 32 ++++++++++++++++++++++----------
  1 file changed, 22 insertions(+), 10 deletions(-)

Acked-by Heiko Schocher <[email protected]>

When the device is created from a device tree node, it matches a compatible string. Allow access to that string and the associated data.

Signed-off-by Simon Glass <[email protected]>

When the device is created from a device tree node, it matches a compatible string. Allow access to that string and the associated data.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

  drivers/core/device.c |  5 +++++
  drivers/core/lists.c  | 17 ++++++++++++-----
  include/dm/device.h   | 11 +++++++++++
  3 files changed, 28 insertions(+), 5 deletions(-)

drivers/core/lists.c +5 -2

index 3a1ea85..9f33dde 100644
@@ -89,22 +89,26 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only)
  * tree error
  */
 static int driver_check_compatible(const void *blob, int offset,
-				   const struct udevice_id *of_match)
+				   const struct udevice_id *of_match,
+				   const struct udevice_id **of_idp)
 {
 	int ret;
 
+	*of_idp = NULL;
 	if (!of_match)
 		return -ENOENT;
 
 	while (of_match->compatible) {
 		ret = fdt_node_check_compatible(blob, offset,
 						of_match->compatible);
-		if (!ret)
+		if (!ret) {
+			*of_idp = of_match;

drivers/core/lists.c +3 -1

@@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 {
 	struct driver *driver = ll_entry_start(struct driver, driver);
 	const int n_ents = ll_entry_count(struct driver, driver);
+	const struct udevice_id *id;
 	struct driver *entry;
 	struct udevice *dev;
 	bool found = false;

@@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 	if (devp)
 		*devp = NULL;
 	for (entry = driver; entry != driver + n_ents; entry++) {
-		ret = driver_check_compatible(blob, offset, entry->of_match);
+		ret = driver_check_compatible(blob, offset, entry->of_match,
+					      &id);
 		name = fdt_get_name(blob, offset, NULL);
 		if (ret == -ENOENT) {
 			continue;

Hi Simon,

Quoted 6 lines · On Tue, 11 Nov 2014 10:46:18 -0700 Simon Glass <[email protected]> wrote:

drivers/core/device.c +0 -0

index 49faa29..0d84776 100644
@@ -548,3 +548,8 @@ int device_find_next_child(struct udevice **devp)
 

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 555

Quoted 7 lines

drivers/core/device.c +5 -0

      return 0;
 }
+
+ulong dev_get_of_data(struct udevice *dev)
+{
+     return dev->of_id->data;
+}

Since this function is short enough, perhaps you might want to define it as "static inline" in the header file include/dm/device.h

Quoted 27 lines

drivers/core/lists.c +5 -2

index 3a1ea85..9f33dde 100644
@@ -89,22 +89,26 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only)
  * tree error
  */
 static int driver_check_compatible(const void *blob, int offset,
-                                const struct udevice_id *of_match)
+                                const struct udevice_id *of_match,
+                                const struct udevice_id **of_idp)
 {
      int ret;
 
+     *of_idp = NULL;
      if (!of_match)
              return -ENOENT;
 
      while (of_match->compatible) {
              ret = fdt_node_check_compatible(blob, offset,
                                              of_match->compatible);
-             if (!ret)
+             if (!ret) {
+                     *of_idp = of_match;

drivers/core/lists.c int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) · line 113

Quoted 10 lines

drivers/core/lists.c +3 -2

                      return 0;
-             else if (ret == -FDT_ERR_NOTFOUND)
+             } else if (ret == -FDT_ERR_NOTFOUND) {
                      return -ENODEV;
-             else if (ret < 0)
+             } else if (ret < 0) {
                      return -EINVAL;
+             }
              of_match++;
      }

I think you are making things more complicated than is needed. I guess what you want to do in this patch is just to set "dev->of_id".

Why do you need to touch this function? (Please see below)

Quoted 23 lines

drivers/core/lists.c +3 -1

@@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 {
      struct driver *driver = ll_entry_start(struct driver, driver);
      const int n_ents = ll_entry_count(struct driver, driver);
+     const struct udevice_id *id;
      struct driver *entry;
      struct udevice *dev;
      bool found = false;

@@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
      if (devp)
              *devp = NULL;
      for (entry = driver; entry != driver + n_ents; entry++) {
-             ret = driver_check_compatible(blob, offset, entry->of_match);
+             ret = driver_check_compatible(blob, offset, entry->of_match,
+                                           &id);
              name = fdt_get_name(blob, offset, NULL);
              if (ret == -ENOENT) {
                      continue;

drivers/core/lists.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 156

Quoted 4 lines

drivers/core/lists.c +1 -0

@@ -147,6 +153,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
                      dm_warn("Error binding driver '%s'\n", entry->name);
                      return ret;
              } else {
+                     dev->of_id = id;

Instead of all the chages above, only one line change,

                        dev->of_id = entry->of_match

Does this work for you?

Hi Masahiro,

Quoted 13 lines · Masahiro Yamada

Hi Simon,

Quoted 6 lines · On Tue, 11 Nov 2014 10:46:18 -0700 Simon Glass <[email protected]> wrote:

drivers/core/device.c +0 -0

index 49faa29..0d84776 100644
@@ -548,3 +548,8 @@ int device_find_next_child(struct udevice **devp)
 

Quoted 13 lines

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 555

Quoted 7 lines

drivers/core/device.c +5 -0

      return 0;
 }
+
+ulong dev_get_of_data(struct udevice *dev)
+{
+     return dev->of_id->data;
+}

Since this function is short enough, perhaps you might want to define it as "static inline" in the header file include/dm/device.h

Thanks for looking at this series.

The background here is that I'm quite worried about all the pointers in driver model. It might be quite easy to use the wrong one and get confused.

So my plan is to add code to check that the pointers are what we think they are, like:

   DM_CHECK_PTR(dev, "udevice");

or similar. Then that code would compile to nothing unless it is enabled with a CONFIG_DM_CHECK_PTRS or whatever. That's the reason for accessors.

Quoted 32 lines

Quoted 20 lines

drivers/core/device.c +3 -1

@@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 {
      struct driver *driver = ll_entry_start(struct driver, driver);
      const int n_ents = ll_entry_count(struct driver, driver);
+     const struct udevice_id *id;
      struct driver *entry;
      struct udevice *dev;
      bool found = false;

@@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
      if (devp)
              *devp = NULL;
      for (entry = driver; entry != driver + n_ents; entry++) {
-             ret = driver_check_compatible(blob, offset, entry->of_match);
+             ret = driver_check_compatible(blob, offset, entry->of_match,
+                                           &id);
              name = fdt_get_name(blob, offset, NULL);
              if (ret == -ENOENT) {
                      continue;

drivers/core/device.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 156

Quoted 4 lines

drivers/core/device.c +1 -0

@@ -147,6 +153,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
                      dm_warn("Error binding driver '%s'\n", entry->name);
                      return ret;
              } else {
+                     dev->of_id = id;

Instead of all the chages above, only one line change,

                        dev->of_id = entry->of_match

Quoted 3 lines

Does this work for you?

entry->of_match is the first element in an array of records. I want to know exactly which one matches, so I can't just use the first one.

Hi Simon,

Quoted 30 lines · On Wed, 19 Nov 2014 09:35:54 +0000 Simon Glass <[email protected]> wrote:

Hi Masahiro,

Quoted 13 lines · Masahiro Yamada

Hi Simon,

Quoted 6 lines · On Tue, 11 Nov 2014 10:46:18 -0700 Simon Glass <[email protected]> wrote:

drivers/core/device.c +0 -0

index 49faa29..0d84776 100644
@@ -548,3 +548,8 @@ int device_find_next_child(struct udevice **devp)
 

Quoted 13 lines

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 555

Quoted 7 lines

drivers/core/device.c +5 -0

      return 0;
 }
+
+ulong dev_get_of_data(struct udevice *dev)
+{
+     return dev->of_id->data;
+}

Since this function is short enough, perhaps you might want to define it as "static inline" in the header file include/dm/device.h

Quoted 5 lines

Thanks for looking at this series.

The background here is that I'm quite worried about all the pointers in driver model. It might be quite easy to use the wrong one and get confused.

Me too.

Quoted 9 lines

So my plan is to add code to check that the pointers are what we think they are, like:

   DM_CHECK_PTR(dev, "udevice");

or similar. Then that code would compile to nothing unless it is enabled with a CONFIG_DM_CHECK_PTRS or whatever. That's the reason for accessors.

Sounds good!

Quoted 32 lines

Quoted 32 lines

Quoted 20 lines

drivers/core/device.c +3 -1

@@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 {
      struct driver *driver = ll_entry_start(struct driver, driver);
      const int n_ents = ll_entry_count(struct driver, driver);
+     const struct udevice_id *id;
      struct driver *entry;
      struct udevice *dev;
      bool found = false;

@@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
      if (devp)
              *devp = NULL;
      for (entry = driver; entry != driver + n_ents; entry++) {
-             ret = driver_check_compatible(blob, offset, entry->of_match);
+             ret = driver_check_compatible(blob, offset, entry->of_match,
+                                           &id);
              name = fdt_get_name(blob, offset, NULL);
              if (ret == -ENOENT) {
                      continue;

drivers/core/device.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 156

Quoted 4 lines

drivers/core/device.c +1 -0

@@ -147,6 +153,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
                      dm_warn("Error binding driver '%s'\n", entry->name);
                      return ret;
              } else {
+                     dev->of_id = id;

Instead of all the chages above, only one line change,

                        dev->of_id = entry->of_match

Quoted 7 lines

Quoted 3 lines

Does this work for you?

entry->of_match is the first element in an array of records. I want to know exactly which one matches, so I can't just use the first one.

Sorry, it was my misunderstanding.
Thanks for explaining this.

Could you update the comment block of driver_check_compatible?

/**
* driver_check_compatible() - Check if a driver is compatible with this node *

 * @param blob:         Device tree pointer
 * @param offset:       Offset of node in device tree
 * @param of_matchL     List of compatible strings to match

* @return 0 if there is a match, -ENOENT if no match, -ENODEV if the node
* does not have a compatible string, other error <0 if there is a device
* tree error
*/

- Add description of "@param of_idp"
- Fix "of_matchL" -> "of_match"

Hi Masahiro,

Quoted 100 lines · Masahiro Yamada

Hi Simon,

Quoted 30 lines · On Wed, 19 Nov 2014 09:35:54 +0000 Simon Glass <[email protected]> wrote:

Hi Masahiro,

Quoted 13 lines · Masahiro Yamada

Hi Simon,

Quoted 6 lines · On Tue, 11 Nov 2014 10:46:18 -0700 Simon Glass <[email protected]> wrote:

drivers/core/device.c +0 -0

index 49faa29..0d84776 100644
@@ -548,3 +548,8 @@ int device_find_next_child(struct udevice **devp)
 

Quoted 13 lines

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 555

Quoted 7 lines

drivers/core/device.c +5 -0

      return 0;
 }
+
+ulong dev_get_of_data(struct udevice *dev)
+{
+     return dev->of_id->data;
+}

Since this function is short enough, perhaps you might want to define it as "static inline" in the header file include/dm/device.h

Quoted 5 lines

Thanks for looking at this series.

The background here is that I'm quite worried about all the pointers in driver model. It might be quite easy to use the wrong one and get confused.

Me too.

Quoted 9 lines

So my plan is to add code to check that the pointers are what we think they are, like:

   DM_CHECK_PTR(dev, "udevice");

or similar. Then that code would compile to nothing unless it is enabled with a CONFIG_DM_CHECK_PTRS or whatever. That's the reason for accessors.

Sounds good!

Quoted 32 lines

Quoted 32 lines

Quoted 20 lines

drivers/core/device.c +3 -1

@@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
 {
      struct driver *driver = ll_entry_start(struct driver, driver);
      const int n_ents = ll_entry_count(struct driver, driver);
+     const struct udevice_id *id;
      struct driver *entry;
      struct udevice *dev;
      bool found = false;

@@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
      if (devp)
              *devp = NULL;
      for (entry = driver; entry != driver + n_ents; entry++) {
-             ret = driver_check_compatible(blob, offset, entry->of_match);
+             ret = driver_check_compatible(blob, offset, entry->of_match,
+                                           &id);
              name = fdt_get_name(blob, offset, NULL);
              if (ret == -ENOENT) {
                      continue;

drivers/core/device.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 156

Quoted 4 lines

drivers/core/device.c +1 -0

@@ -147,6 +153,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
                      dm_warn("Error binding driver '%s'\n", entry->name);
                      return ret;
              } else {
+                     dev->of_id = id;

Instead of all the chages above, only one line change,

                        dev->of_id = entry->of_match

Quoted 7 lines

Quoted 3 lines

Does this work for you?

entry->of_match is the first element in an array of records. I want to know exactly which one matches, so I can't just use the first one.

Quoted 6 lines

Sorry, it was my misunderstanding.
Thanks for explaining this.

Could you update the comment block of driver_check_compatible?

OK

Quoted 15 lines

/**
* driver_check_compatible() - Check if a driver is compatible with this node *

 * @param blob:         Device tree pointer
 * @param offset:       Offset of node in device tree
 * @param of_matchL     List of compatible strings to match

* @return 0 if there is a match, -ENOENT if no match, -ENODEV if the node
* does not have a compatible string, other error <0 if there is a device
* tree error
*/

Quoted 2 lines

- Add description of "@param of_idp"
- Fix "of_matchL" -> "of_match"

Will fix.

Add dev_get_parent() as a convenience to obtain the parent of a device.

Signed-off-by Simon Glass <[email protected]>

Add dev_get_parent() as a convenience to obtain the parent of a device.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

  drivers/core/device.c | 5 +++++
  include/dm/device.h   | 8 ++++++++
  2 files changed, 13 insertions(+)

Add dev_get_parent() as a convenience to obtain the parent of a device.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

 drivers/core/device.c | 5 +++++
 include/dm/device.h   | 8 ++++++++
 2 files changed, 13 insertions(+)

drivers/core/device.c +0 -0

index 0d84776..76b29fd 100644

Quoted 16 lines · On Tue, 11 Nov 2014 10:46:19 -0700 Simon Glass <[email protected]> wrote:

Add dev_get_parent() as a convenience to obtain the parent of a device.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

 drivers/core/device.c | 5 +++++
 include/dm/device.h   | 8 ++++++++
 2 files changed, 13 insertions(+)

drivers/core/device.c +0 -0

index 0d84776..76b29fd 100644

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 556

Quoted 8 lines

drivers/core/device.c +5 -0

@@ -549,6 +549,11 @@ int device_find_next_child(struct udevice **devp)
      return 0;
 }
 
+struct udevice *dev_get_parent(struct udevice *child)
+{
+     return child->parent;
+}
+

Why do you want this? "dev_get_parent(dev)" is longer than "dev->parent".

I am not sure if this helper function is useful, but if really necessary, static inline or macro ??

Perhaps, "struct udevice *dev" rather than "struct udevice *child" for consistency?

Hi Masahiro,

Quoted 31 lines · Masahiro Yamada

Quoted 16 lines · On Tue, 11 Nov 2014 10:46:19 -0700 Simon Glass <[email protected]> wrote:

Add dev_get_parent() as a convenience to obtain the parent of a device.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

 drivers/core/device.c | 5 +++++
 include/dm/device.h   | 8 ++++++++
 2 files changed, 13 insertions(+)

drivers/core/device.c +0 -0

index 0d84776..76b29fd 100644

drivers/core/device.c int device_find_next_child(struct udevice **devp) · line 556

Quoted 8 lines

drivers/core/device.c +5 -0

@@ -549,6 +549,11 @@ int device_find_next_child(struct udevice **devp)
      return 0;
 }
 
+struct udevice *dev_get_parent(struct udevice *child)
+{
+     return child->parent;
+}
+

Why do you want this? "dev_get_parent(dev)" is longer than "dev->parent".

Quoted 2 lines

I am not sure if this helper function is useful, but if really necessary, static inline or macro ??

See my comment on the other patch.

Quoted 4 lines

Perhaps, "struct udevice *dev" rather than "struct udevice *child" for consistency?

Maybe, but I feel this is clearer even if it is inconsistent. I try to use 'bus' instead of dev when there is a bus too, to help with understanding.

Regards,
Simon

Since we scan from left to right looking for the first digit, "i2c0" returns 2 instead of 0 for the alias number. Adjust the code to scan from right to left instead.

Signed-off-by Simon Glass <[email protected]>

Quoted 5 lines · Simon Glass

Since we scan from left to right looking for the first digit, "i2c0" returns 2 instead of 0 for the alias number. Adjust the code to scan from right to left instead.

Signed-off-by Simon Glass <[email protected]>

How about i2c10 ? I assume you see where I'm worried about here..

Hi Tom,

Quoted 7 lines · Tom Rini

Quoted 4 lines · Simon Glass

Since we scan from left to right looking for the first digit, "i2c0" returns 2 instead of 0 for the alias number. Adjust the code to scan from right to left instead.

Quoted 3 lines

Quoted 1 line

Signed-off-by Simon Glass <[email protected]>

How about i2c10 ? I assume you see where I'm worried about here..

It goes back to the first non-digit, so will produce 10 in this case as expected.

Quoted 12 lines · Simon Glass

Hi Tom,

Quoted 9 lines · Tom Rini

Quoted 5 lines · Simon Glass

Since we scan from left to right looking for the first digit, "i2c0" returns 2 instead of 0 for the alias number. Adjust the code to scan from right to left instead.

Signed-off-by Simon Glass <[email protected]>

Quoted 4 lines

Quoted 1 line

How about i2c10 ? I assume you see where I'm worried about here..

It goes back to the first non-digit, so will produce 10 in this case as expected.

Oh good.

Reviewed-by Tom Rini <[email protected]>

Since we scan from left to right looking for the first digit, "i2c0" returns 2 instead of 0 for the alias number. Adjust the code to scan from right to left instead.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patch to correct handling of aliases with embedded digits

  lib/fdtdec.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

In some cases we need to manually bind a device to a particular driver. Add a function to do this.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patch to add a function to bind a device by driver name

 drivers/core/lists.c | 21 +++++++++++++++++++++
 include/dm/lists.h   | 13 +++++++++++++
 2 files changed, 34 insertions(+)

drivers/core/lists.c +21 -0

index 9f33dde..32f2242 100644
@@ -77,6 +77,27 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only)
        return result;
 }
 
+int device_bind_driver(struct udevice *parent, const char *drv_name,
+                      const char *dev_name, struct udevice **devp)
+{
+       struct driver *drv;
+       int ret;
+
+       drv = lists_driver_lookup_name(drv_name);
+       if (!drv) {
+               printf("Cannot find driver '%s'\n", drv_name);
+               return -ENOENT;
+       }
+       ret = device_bind(parent, drv, dev_name, NULL, -1, devp);
+       if (ret) {
+               printf("Cannot create device named '%s' (err=%d)\n",
+                      dev_name, ret);
+               return ret;
+       }
+
+       return 0;
+}
+
 #ifdef CONFIG_OF_CONTROL
 /**
  * driver_check_compatible() - Check if a driver is compatible with this node

include/dm/lists.h +13 -1

index 704e33e..1b50af9 100644
@@ -60,4 +60,17 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);
 int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
                   struct udevice **devp);
 
+/**
+ * device_bind_driver() - bind a device to a driver
+ *
+ * This binds a new device to a driver.
+ *
+ * @parent:    Parent device
+ * @drv_name:  Name of driver to attach to this parent
+ * @dev_name:  Name of the new device thus created
+ * @devp:      Returns the newly bound device
+ */
+int device_bind_driver(struct udevice *parent, const char *drv_name,
+                      const char *dev_name, struct udevice **devp);
+
 #endif
--

2.1.0.rc2.206.gedb03e5

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

In some cases we need to manually bind a device to a particular driver. Add a function to do this.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patch to add a function to bind a device by driver name

  drivers/core/lists.c | 21 +++++++++++++++++++++
  include/dm/lists.h   | 13 +++++++++++++
  2 files changed, 34 insertions(+)

Hello Simon,

Am 11.11.2014 18:46, schrieb Simon Glass:

Quoted 10 lines

In some cases we need to manually bind a device to a particular driver. Add a function to do this.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patch to add a function to bind a device by driver name

Quoted 3 lines

  drivers/core/lists.c | 21 +++++++++++++++++++++
  include/dm/lists.h   | 13 +++++++++++++
  2 files changed, 34 insertions(+)

Acked-by Heiko Schocher <[email protected]>

This code was not updated when the chip select handling was adjusted. Fix it to call the correct function.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

 drivers/mtd/spi/sandbox.c |  2 +-
 drivers/spi/spi-uclass.c  | 11 +----------
 include/spi.h             | 10 ++++++++++
 3 files changed, 12 insertions(+), 11 deletions(-)

drivers/mtd/spi/sandbox.c +1 -1

index 1cf2f98..fecf6d4 100644
@@ -602,7 +602,7 @@ static int sandbox_sf_bind_bus_cs(struct sandbox_state *state, int busnum,
                       spec, ret);
                return ret;
        }
-       ret = device_find_child_by_seq(bus, cs, true, &slave);
+       ret = spi_find_chip_select(bus, cs, &slave);
        if (!ret) {
                printf("Chip select %d already exists for spec '%s'\n", cs,
                       spec);

drivers/spi/spi-uclass.c +1 -10

index 13c6b77..6bfc274 100644
@@ -115,16 +115,7 @@ int spi_chip_select(struct udevice *dev)
        return slave ? slave->cs : -ENOENT;
 }
 
-/**
- * spi_find_chip_select() - Find the slave attached to chip select
- *
- * @bus:       SPI bus to search
- * @cs:                Chip select to look for
- * @devp:      Returns the slave device if found
- * @return 0 if found, -ENODEV on error
- */
-static int spi_find_chip_select(struct udevice *bus, int cs,
-                               struct udevice **devp)
+int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp)
 {
        struct udevice *dev;
 

include/spi.h +10 -1

index aa0a48e..5975cda 100644
@@ -534,6 +534,16 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 int spi_chip_select(struct udevice *slave);
 
 /**
+ * spi_find_chip_select() - Find the slave attached to chip select
+ *
+ * @bus:       SPI bus to search
+ * @cs:                Chip select to look for
+ * @devp:      Returns the slave device if found
+ * @return 0 if found, -ENODEV on error
+ */
+int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp);
+
+/**
  * spi_bind_device() - bind a device to a bus's chip select
  *
  * This binds a new device to an given chip select (which must be unused).
--

2.1.0.rc2.206.gedb03e5

This code was not updated when the chip select handling was adjusted. Fix it to call the correct function.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

  drivers/mtd/spi/sandbox.c |  2 +-
  drivers/spi/spi-uclass.c  | 11 +----------
  include/spi.h             | 10 ++++++++++
  3 files changed, 12 insertions(+), 11 deletions(-)

Hello Simon,

Am 11.11.2014 18:46, schrieb Simon Glass:

Quoted 9 lines

This code was not updated when the chip select handling was adjusted. Fix it to call the correct function.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2: None

Quoted 4 lines

  drivers/mtd/spi/sandbox.c |  2 +-
  drivers/spi/spi-uclass.c  | 11 +----------
  include/spi.h             | 10 ++++++++++
  3 files changed, 12 insertions(+), 11 deletions(-)

Acked-by Heiko Schocher <[email protected]>

The SPI function does the same thing, so we may as well just use the new generic function. The 'cs' parameter was not actually used, so can be dropped.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patches to adjust SPI to use device_bind_driver()

 drivers/mtd/spi/sandbox.c |  2 +-
 drivers/spi/spi-uclass.c  | 23 +----------------------
 include/spi.h             | 14 --------------
 3 files changed, 2 insertions(+), 37 deletions(-)

drivers/mtd/spi/sandbox.c +1 -1

index fecf6d4..c6a5c4b 100644
@@ -609,7 +609,7 @@ static int sandbox_sf_bind_bus_cs(struct sandbox_state *state, int busnum,
                return -EEXIST;
        }
 
-       ret = spi_bind_device(bus, cs, "spi_flash_std", spec, &slave);
+       ret = device_bind_driver(bus, "spi_flash_std", spec, &slave);
        if (ret)
                return ret;
 

drivers/spi/spi-uclass.c +1 -22

index 6bfc274..64eb1f6 100644
@@ -188,27 +188,6 @@ int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info)
        return -ENODEV;
 }
 
-int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
-                   const char *dev_name, struct udevice **devp)
-{
-       struct driver *drv;
-       int ret;
-
-       drv = lists_driver_lookup_name(drv_name);
-       if (!drv) {
-               printf("Cannot find driver '%s'\n", drv_name);
-               return -ENOENT;
-       }
-       ret = device_bind(bus, drv, dev_name, NULL, -1, devp);
-       if (ret) {
-               printf("Cannot create device named '%s' (err=%d)\n",
-                      dev_name, ret);
-               return ret;
-       }
-
-       return 0;
-}
-
 int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
                        struct udevice **devp)
 {

@@ -255,7 +234,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
        if (ret == -ENODEV && drv_name) {
                debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
                      __func__, dev_name, busnum, cs, drv_name);
-               ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev);
+               ret = device_bind_driver(bus, drv_name, dev_name, &dev);
                if (ret)
                        return ret;
                created = true;

include/spi.h +0 -15

index 5975cda..5b78271 100644
@@ -544,20 +544,6 @@ int spi_chip_select(struct udevice *slave);
 int spi_find_chip_select(struct udevice *bus, int cs, struct udevice **devp);
 
 /**
- * spi_bind_device() - bind a device to a bus's chip select
- *
- * This binds a new device to an given chip select (which must be unused).
- *
- * @bus:       SPI bus to search
- * @cs:                Chip select to attach to
- * @drv_name:  Name of driver to attach to this chip select
- * @dev_name:  Name of the new device thus created
- * @devp:      Returns the newly bound device
- */
-int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
-                   const char *dev_name, struct udevice **devp);
-
-/**
  * spi_ofdata_to_platdata() - decode standard SPI platform data
  *
  * This decodes the speed and mode from a device tree node and puts it into
--

2.1.0.rc2.206.gedb03e5

The SPI function does the same thing, so we may as well just use the new generic function. The 'cs' parameter was not actually used, so can be dropped.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patches to adjust SPI to use device_bind_driver()

  drivers/mtd/spi/sandbox.c |  2 +-
  drivers/spi/spi-uclass.c  | 23 +----------------------
  include/spi.h             | 14 --------------
  3 files changed, 2 insertions(+), 37 deletions(-)

Hello Simon,

Am 11.11.2014 18:46, schrieb Simon Glass:

Quoted 11 lines

The SPI function does the same thing, so we may as well just use the new generic function. The 'cs' parameter was not actually used, so can be dropped.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Add new patches to adjust SPI to use device_bind_driver()

Quoted 4 lines

  drivers/mtd/spi/sandbox.c |  2 +-
  drivers/spi/spi-uclass.c  | 23 +----------------------
  include/spi.h             | 14 --------------
  3 files changed, 2 insertions(+), 37 deletions(-)

Acked-by Heiko Schocher <[email protected]>

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number

  drivers/i2c/Makefile       |   1 +
  drivers/i2c/i2c-uclass.c   | 275 +++++++++++++++++++++++++++++++++++++++++++
  include/config_fallbacks.h |   6 +
  include/dm/uclass-id.h     |   2 +
  include/i2c.h              | 288 +++++++++++++++++++++++++++++++++++++++++++++
  5 files changed, 572 insertions(+)

create mode 100644 drivers/i2c/i2c-uclass.c

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

---

Hi Simon,

Quoted 12 lines · On Tue, 11 Nov 2014 10:46:24 -0700 Simon Glass <[email protected]> wrote:

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

---

drivers/i2c/i2c-uclass.c line 251

Quoted 9 lines

drivers/i2c/i2c-uclass.c new file +9 -0

+static int i2c_post_probe(struct udevice *dev)
+{
+	struct dm_i2c_bus *i2c = dev->uclass_priv;
+
+	i2c->speed_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+				     "clock-frequency", 100000);
+
+	return i2c_set_bus_speed(dev, i2c->speed_hz);
+}

This code in drivers/i2c/i2c-uclass.c seems to highly depends on Device Tree.

I am not sure if I understood correctly, but
does this work on non Device Tree SoCs?

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number

drivers/i2c/i2c-uclass.c new file +16 -0

+
+int i2c_read(struct udevice *dev, uint addr, uint8_t *buffer, int len)
+{
+	struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+	struct udevice *bus = dev_get_parent(dev);
+	struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+	if (!ops->read)
+		return -ENOSYS;
+
+	return ops->read(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+			 len);
+}
+
+int i2c_write(struct udevice *dev, uint addr, const uint8_t *buffer, int len)
+{

drivers/i2c/i2c-uclass.c new file +16 -0

+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+	struct udevice *dev;
+
+	debug("%s: Searching bus '%s' for address %02x: ", __func__,
+	      bus->name, chip_addr);
+	for (device_find_first_child(bus, &dev); dev;
+			device_find_next_child(&dev)) {
+		struct dm_i2c_chip store;
+		struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+		int ret;
+
+		if (!chip) {
+			chip = &store;
+			i2c_chip_ofdata_to_platdata(gd->fdt_blob,

drivers/i2c/i2c-uclass.c new file +7 -0

+}
+
+int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+	struct dm_i2c_ops *ops = i2c_get_ops(bus);
+	int ret;
+

drivers/i2c/i2c-uclass.c new file +40 -0

+int i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
+{
+	struct dm_i2c_ops *ops = i2c_get_ops(bus);
+	struct dm_i2c_bus *i2c = bus->uclass_priv;
+	int ret;
+
+	if (ops->set_bus_speed) {
+		ret = ops->set_bus_speed(bus, speed);
+		if (ret)
+			return ret;
+	}
+	i2c->speed_hz = speed;
+
+	return 0;
+}
+
+/*
+ * i2c_get_bus_speed:
+ *
+ *  Returns speed of selected I2C bus in Hz
+ */
+int i2c_get_bus_speed(struct udevice *bus)
+{
+	struct dm_i2c_ops *ops = i2c_get_ops(bus);
+	struct dm_i2c_bus *i2c = bus->uclass_priv;
+
+	if (!ops->set_bus_speed)
+		return i2c->speed_hz;
+
+	return ops->get_bus_speed(bus);
+}
+
+int i2c_set_addr_len(struct udevice *dev, uint addr_len)
+{
+	struct udevice *bus = dev->parent;
+	struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+	struct dm_i2c_ops *ops = i2c_get_ops(bus);
+	int ret;
+
+	if (addr_len > 3)

include/i2c.h +26 -0

+
+/**
+ * struct dm_i2c_ops - driver operations for I2C uclass
+ *
+ * Drivers should support these operations unless otherwise noted. These
+ * operations are intended to be used by uclass code, not directly from
+ * other code.
+ */
+struct dm_i2c_ops {
+	/**
+	 * read() - read from a chip
+	 *
+	 * @bus:	Bus to read from
+	 * @chip_addr:	Chip address to read from
+	 * @alen:	Length of chip address in bytes
+	 * @offset:	Offset within chip to start reading
+	 * @buffer:	Place to put data
+	 * @len:	Number of bytes to read
+	 */
+	int (*read)(struct udevice *bus, uint chip_addr, uint alen,
+		    uint offset, uint8_t *buffer, int len);
+
+	/**
+	 * write() - write bytes to a chip
+	 *
+	 * @dev:	Device to write to

Quoted 24 lines

drivers/i2c/i2c-uclass.c new file +22 -0

+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+	struct udevice *dev;
+
+	debug("%s: Searching bus '%s' for address %02x: ", __func__,
+	      bus->name, chip_addr);
+	for (device_find_first_child(bus, &dev); dev;
+			device_find_next_child(&dev)) {
+		struct dm_i2c_chip store;
+		struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+		int ret;
+
+		if (!chip) {
+			chip = &store;
+			i2c_chip_ofdata_to_platdata(gd->fdt_blob,
+						    dev->of_offset, chip);
+		}
+		if (chip->chip_addr == chip_addr) {
+			ret = device_probe(dev);
+			debug("found, ret=%d\n", ret);
+			if (ret)

drivers/i2c/i2c-uclass.c line 109

Quoted 4 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+				return ret;
+			*devp = dev;
+			return 0;
+		}

If "chip" is "NULL", i2c_chip_ofdata_to_platdata() is called to create struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.

Hi Simon,

Quoted 19 lines · On Tue, 11 Nov 2014 10:46:24 -0700 Simon Glass <[email protected]> wrote:

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

- Remove the chip address from API calls
- Remove the address length from API calls
- Remove concept of 'current' I2C bus
- Drop all existing init functions

Signed-off-by Simon Glass <[email protected]>

---

Changes in v2:
- Fix cihp typo
- Implement generic I2C devices to allow 'i2c probe' on unknown devices
- Return the probed device from i2c_probe()
- Set the bus speed after the bus is probed
- Add some debugging for generic I2C device binding
- Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
- Add a helper function to find a chip on a particular bus number

[snip]

Quoted 16 lines

drivers/i2c/i2c-uclass.c new file +16 -0

+
+int i2c_read(struct udevice *dev, uint addr, uint8_t *buffer, int len)
+{
+     struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+     struct udevice *bus = dev_get_parent(dev);
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+     if (!ops->read)
+             return -ENOSYS;
+
+     return ops->read(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                      len);
+}
+
+int i2c_write(struct udevice *dev, uint addr, const uint8_t *buffer, int len)
+{

drivers/i2c/i2c-uclass.c line 43

Quoted 10 lines

drivers/i2c/i2c-uclass.c new file +10 -0

+     struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+     struct udevice *bus = dev_get_parent(dev);
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+
+     if (!ops->write)
+             return -ENOSYS;
+
+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);
+}

This seems inconsistent with "struct dm_i2c_ops".
In this function, the address length(chip->addr_len) is passed to the forth argument of ops->write().

You should compare it with "struct dm_i2c_ops" in include/i2c.h It says that the third argument is alen.

BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?

Quoted 19 lines

drivers/i2c/i2c-uclass.c new file +16 -0

+
+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+     struct udevice *dev;
+
+     debug("%s: Searching bus '%s' for address %02x: ", __func__,
+           bus->name, chip_addr);
+     for (device_find_first_child(bus, &dev); dev;
+                     device_find_next_child(&dev)) {
+             struct dm_i2c_chip store;
+             struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+             int ret;
+
+             if (!chip) {
+                     chip = &store;
+                     i2c_chip_ofdata_to_platdata(gd->fdt_blob,

drivers/i2c/i2c-uclass.c line 109

Quoted 10 lines

drivers/i2c/i2c-uclass.c new file +10 -0

+                                                 dev->of_offset, chip);
+             }
+             if (chip->chip_addr == chip_addr) {
+                     ret = device_probe(dev);
+                     debug("found, ret=%d\n", ret);
+                     if (ret)
+                             return ret;
+                     *devp = dev;
+                     return 0;
+             }

If "chip" is "NULL", i2c_chip_ofdata_to_platdata() is called to create struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.

If we use device_probe_child() instead of device_probe(), I think we can re-use it.

I mean,

                if (chip->chip_addr == chip_addr) {
                        ret = device_probe_child(dev, chip);

Perhaps, we can remove sandbox_i2c_child_pre_probe().

drivers/i2c/i2c-uclass.c line 113

Quoted 7 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

Quoted 19 lines

drivers/i2c/i2c-uclass.c new file +13 -0

+}
+
+int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp)
+{
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+     int ret;
+
+     *devp = NULL;
+     if (!ops->probe)
+             return -ENODEV;
+
+     /* First probe that chip */
+     ret = ops->probe(bus, chip_addr);

drivers/i2c/i2c-uclass.c line 149

Quoted 4 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name,
+           chip_addr, ret);
+     if (ret)
+             return ret;

Is the "ops->probe" responsible to probe child devices? (At least, sandbox_i2c probes its children)

drivers/i2c/i2c-uclass.c line 158

Quoted 11 lines

drivers/i2c/i2c-uclass.c new file +8 -0

+     /* The chip was found, see if we have a driver, and probe it */
+     ret = i2c_get_chip(bus, chip_addr, devp);
+     debug("%s:  i2c_get_chip: ret=%d\n", __func__, ret);
+     if (!ret || ret != -ENODEV)
+             return ret;
+
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If so, why do we need to call i2c_get_chip() and probe it again?

Quoted 44 lines

drivers/i2c/i2c-uclass.c new file +40 -0

+int i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
+{
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+     struct dm_i2c_bus *i2c = bus->uclass_priv;
+     int ret;
+
+     if (ops->set_bus_speed) {
+             ret = ops->set_bus_speed(bus, speed);
+             if (ret)
+                     return ret;
+     }
+     i2c->speed_hz = speed;
+
+     return 0;
+}
+
+/*
+ * i2c_get_bus_speed:
+ *
+ *  Returns speed of selected I2C bus in Hz
+ */
+int i2c_get_bus_speed(struct udevice *bus)
+{
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+     struct dm_i2c_bus *i2c = bus->uclass_priv;
+
+     if (!ops->set_bus_speed)
+             return i2c->speed_hz;
+
+     return ops->get_bus_speed(bus);
+}
+
+int i2c_set_addr_len(struct udevice *dev, uint addr_len)
+{
+     struct udevice *bus = dev->parent;
+     struct dm_i2c_chip *chip = dev_get_parentdata(dev);
+     struct dm_i2c_ops *ops = i2c_get_ops(bus);
+     int ret;
+
+     if (addr_len > 3)

drivers/i2c/i2c-uclass.c line 209

Quoted 10 lines

drivers/i2c/i2c-uclass.c new file +10 -0

+             return -EINVAL;
+     if (ops->set_addr_len) {
+             ret = ops->set_addr_len(dev, addr_len);
+             if (ret)
+                     return ret;
+     }
+     chip->addr_len = addr_len;
+
+     return 0;
+}

I am not 100% sure, but "addr_len" is a user-configurable parameter?

I think each device has its own fixed address size.

drivers/i2c/i2c-uclass.c line 275

Quoted 9 lines

drivers/i2c/i2c-uclass.c new file +9 -0

+UCLASS_DRIVER(i2c_generic) = {
+     .id             = UCLASS_I2C_GENERIC,
+     .name           = "i2c_generic",
+};
+
+U_BOOT_DRIVER(i2c_generic_drv) = {
+     .name           = "i2c_generic_drv",
+     .id             = UCLASS_I2C_GENERIC,
+};

Could you explain how "i2c_generic" is used.

Quoted 33 lines

include/i2c.h +31 -0

+
+/**
+ * struct dm_i2c_ops - driver operations for I2C uclass
+ *
+ * Drivers should support these operations unless otherwise noted. These
+ * operations are intended to be used by uclass code, not directly from
+ * other code.
+ */
+struct dm_i2c_ops {
+     /**
+      * read() - read from a chip
+      *
+      * @bus:        Bus to read from
+      * @chip_addr:  Chip address to read from
+      * @alen:       Length of chip address in bytes
+      * @offset:     Offset within chip to start reading
+      * @buffer:     Place to put data
+      * @len:        Number of bytes to read
+      */
+     int (*read)(struct udevice *bus, uint chip_addr, uint alen,
+                 uint offset, uint8_t *buffer, int len);
+
+     /**
+      * write() - write bytes to a chip
+      *
+      * @dev:        Device to write to
+      * @chip_addr:  Chip address to read from
+      * @alen:       Length of chip address in bytes
+      * @offset:     Offset within chip to start writing
+      * @buffer:     Buffer containing data to write
+      * @len:        Number of bytes to write

include/i2c.h line 190

Quoted 5 lines

include/i2c.h +5 -0

+      *
+      * @return 0 on success, -ve on failure
+      */
+     int (*write)(struct udevice *bus, uint chip_addr, uint alen,
+                  uint offset, const uint8_t *buffer, int len);

See. The third argument is address length.

Quoted 8 lines

BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?

I'm not sure what you mean by this sorry.

Quoted 2 lines

drivers/i2c/i2c-uclass.c line 42

Quoted 2 lines

drivers/i2c/i2c-uclass.c new file +2 -0

+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);

Quoted 14 lines

drivers/i2c/i2c-uclass.c line 113

Quoted 8 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

Quoted 8 lines

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.

As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.

Hi Simon,

Quoted 10 lines · On Wed, 19 Nov 2014 10:24:47 +0000 Simon Glass <[email protected]> wrote:

Quoted 8 lines

BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?

I'm not sure what you mean by this sorry.

Let's assume we want to write some data to a EEPROM chip connected to i2c bus.

We generally send

[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0
[byte 3] WData1
...

From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data.

I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written.

Quoted 2 lines

Quoted 2 lines

drivers/i2c/i2c-uclass.c line 42

Quoted 2 lines

drivers/i2c/i2c-uclass.c new file +2 -0

+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);

In this implementation, the offset_address is treated with "addr" and the *real* data is handled with "buffer".

It seems odd from the perspective of I2C protocol, I think.

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Quoted 25 lines

Quoted 16 lines

drivers/i2c/i2c-uclass.c line 113

Quoted 10 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

Quoted 8 lines

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

Quoted 8 lines

This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.

As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.

Sorry, I could not parse you here.

I2C is not a hot-plugged bus.
I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions?

BTW, sandbox_i2c_read() is 400KHz tolerate:

        /* For testing, don't allow reading above 400KHz */
        if (i2c->speed_hz > 400000 || alen != 1)
                return -EINVAL;

but sandbox_i2c_write() only allows 100KHz:

        /* For testing, don't allow writing above 100KHz */
        if (i2c->speed_hz > 100000 || alen != 1)
                return -EINVAL;

Because the clock-frequency is set to <400000> in the sandbox DTS, writing to I2C fails unless we change the I2C speed.

Is this intentional?

Personally, I like everything to work on the mail line.

+ A few more people to cc

Hi Masahiro,

Quoted 45 lines · Masahiro Yamada

Hi Simon,

Quoted 10 lines · On Wed, 19 Nov 2014 10:24:47 +0000 Simon Glass <[email protected]> wrote:

Quoted 8 lines

BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?

I'm not sure what you mean by this sorry.

Let's assume we want to write some data to a EEPROM chip connected to i2c bus.

We generally send

[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0
[byte 3] WData1

    ...

From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data.

I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written.

Quoted 2 lines

Quoted 2 lines

drivers/i2c/i2c-uclass.c line 42

Quoted 2 lines

drivers/i2c/i2c-uclass.c new file +2 -0

+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);

In this implementation, the offset_address is treated with "addr" and the *real* data is handled with "buffer".

It seems odd from the perspective of I2C protocol, I think.

Quoted 13 lines

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

Quoted 4 lines

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Quoted 2 lines

We could certainly change this. I'm not sure that I have a strong opinion either way.

I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements:
a write (for the address) then a read (to get the data).

If we want to change it, we should do it now. My question is, what is the point? Will we really want >2 elements in the message array, do we

want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I

feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely.

Perhaps Heiko can share an opinion here?

Quoted 36 lines

Quoted 26 lines

Quoted 17 lines

drivers/i2c/i2c-uclass.c line 113

Quoted 11 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

Quoted 8 lines

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

Quoted 8 lines

This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.

As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.

Quoted 6 lines

Sorry, I could not parse you here.

I2C is not a hot-plugged bus.
I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions?

If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device.

With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected.

Quoted 25 lines

BTW, sandbox_i2c_read() is 400KHz tolerate:

         /* For testing, don't allow reading above 400KHz */
         if (i2c->speed_hz > 400000 || alen != 1)
                 return -EINVAL;

but sandbox_i2c_write() only allows 100KHz:

         /* For testing, don't allow writing above 100KHz */
         if (i2c->speed_hz > 100000 || alen != 1)
                 return -EINVAL;

Quoted 6 lines

Because the clock-frequency is set to <400000> in the sandbox DTS, writing to I2C fails unless we change the I2C speed.

Is this intentional?

Personally, I like everything to work on the mail line.

This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops.

Regards,
Simon

Quoted 12 lines

Quoted 12 lines

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

Quoted 4 lines

Quoted 4 lines

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Yes, but this changes the current U-Boot API ...

Quoted 6 lines

Quoted 2 lines

We could certainly change this. I'm not sure that I have a strong opinion either way.

Quoted 8 lines

I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements:
a write (for the address) then a read (to get the data).

If we want to change it, we should do it now. My question is, what is the point? Will we really want >2 elements in the message array, do we

Do we need more than 2 elements? But of course, if we go into this direction we should support n elements ...

Quoted 2 lines

want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I

Thats the point ... do we need all this stuff in U-Boot?

Quoted 5 lines

feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely.

Perhaps Heiko can share an opinion here?

This implements that we must adapt each i2c driver "a little bit more" right? But I think, as we go with this approach more into the linux direction it sounds good to me (maybe we can directly use linux i2c drivers? ... that sounds good, and maybe should be a goal too?). I could not really say how many work this would be, but as we do this change step by step it is worth to go in this direction, as we can cleanup here and there (especially the eeprom driver) some "suboptimal" code ...

Thinking about it ... maybe we start from scratch with i2c drivers for DM and try to use linux i2c drivers?

Hi Heiko, Simon,

Quoted 14 lines · On Fri, 21 Nov 2014 08:24:18 +0100 Heiko Schocher <[email protected]> wrote:

Quoted 14 lines

Quoted 14 lines

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Yes, but this changes the current U-Boot API ...

I am hoping the translation code will be implemented in drivers/i2c/i2c-uclass.c, I think.

Quoted 41 lines

Quoted 10 lines

Quoted 6 lines

We could certainly change this. I'm not sure that I have a strong opinion either way.

Quoted 8 lines

I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements:
a write (for the address) then a read (to get the data).

If we want to change it, we should do it now. My question is, what is the point? Will we really want >2 elements in the message array, do we

Do we need more than 2 elements? But of course, if we go into this direction we should support n elements ...

Quoted 2 lines

want more control over how transactions are done (repeated start, etc.)? I'm not sure. Still it would be a fairly low-impact change I

Thats the point ... do we need all this stuff in U-Boot?

Quoted 5 lines

feel. We are changing the drivers anyway, so changing the uclass-to-driver API would be feasible. One advantage perhaps it that it would match Linux more closely.

Perhaps Heiko can share an opinion here?

This implements that we must adapt each i2c driver "a little bit more" right? But I think, as we go with this approach more into the linux direction it sounds good to me (maybe we can directly use linux i2c drivers? ... that sounds good, and maybe should be a goal too?). I could not really say how many work this would be, but as we do this change step by step it is worth to go in this direction, as we can cleanup here and there (especially the eeprom driver) some "suboptimal" code ...

Quoted 2 lines

Thinking about it ... maybe we start from scratch with i2c drivers for DM and try to use linux i2c drivers?

Anyway, DM is a giant change. I think it is the best (and perhaps the last) opportunity to implement things correctly.

Quoted 39 lines

Let's assume we want to write some data to a EEPROM chip connected to i2c bus.

We generally send

[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0
[byte 3] WData1
...

From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data.

I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written.

Quoted 2 lines

Quoted 2 lines

drivers/i2c/i2c-uclass.c line 42

Quoted 2 lines

drivers/i2c/i2c-uclass.c new file +2 -0

+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);

In this implementation, the offset_address is treated with "addr" and the *real* data is handled with "buffer".

It seems odd from the perspective of I2C protocol, I think.

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

Quoted 6 lines

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

We could certainly change this. I'm not sure that I have a strong opinion either way.

I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements:
a write (for the address) then a read (to get the data).

Quoted 35 lines

Quoted 25 lines

Quoted 16 lines

drivers/i2c/i2c-uclass.c line 113

Quoted 10 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

Quoted 8 lines

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

Quoted 8 lines

This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.

As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.

Quoted 6 lines

Sorry, I could not parse you here.

I2C is not a hot-plugged bus.
I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions?

If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device.

With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected.

Quoted 5 lines

Because the clock-frequency is set to <400000> in the sandbox DTS, writing to I2C fails unless we change the I2C speed.

Quoted 3 lines

Is this intentional?

Personally, I like everything to work on the mail line.

This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops.

Hi Simon, Heiko,

Quoted 46 lines · On Thu, 20 Nov 2014 14:04:52 +0000 Simon Glass <[email protected]> wrote:

Quoted 39 lines

Let's assume we want to write some data to a EEPROM chip connected to i2c bus.

We generally send

[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start writing [byte 2] WData0
[byte 3] WData1
...

From the perspective of I2C protocol, [byte 1], [byte 2], [byte 3], ... are all data.

I2C itself deos not (should not) know which byte is the offset_address in the chip and which is the *real* data to be written.

Quoted 2 lines

Quoted 2 lines

drivers/i2c/i2c-uclass.c line 42

Quoted 2 lines

drivers/i2c/i2c-uclass.c new file +2 -0

+     return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,
+                       len);

In this implementation, the offset_address is treated with "addr" and the *real* data is handled with "buffer".

It seems odd from the perspective of I2C protocol, I think.

Likewise, when we read data from a EEPROM chip connected to i2c bus,

We generally send/receive
[byte 0] SLAVE_ADDR (7bit) + W flag
[byte 1] Offset address in EEPROM where you want to start reading [byte 2] SLAVE_ADDR (7bit) + R flag
[byte 3] RData 0
[byte 4] Rdata 1

Quoted 6 lines

[byte 1], [byte 3], [byte 4] are data written/read via I2C bus.

In my view, each I2C driver should handle [byte 0] and [byte 1] in its ".write" operation and [byte 2], [byte3], [byte 4], .... in its ".read" operation.

Quoted 8 lines

We could certainly change this. I'm not sure that I have a strong opinion either way.

I haven't to date seen an I2C chip where we don't have an address as the first byte. If we change the API at the driver level, which I think is what we are discussing, then we would need to move to a message array format. The read transaction would become two elements:
a write (for the address) then a read (to get the data).

Yes, the code of the write (for the address) in the .read() handler would the same as the .write handler.
I thought it would not be a good idea to duplicate the write transaction code in every driver.

If we can accept this change, we only need to implement "write -> restart -> read" code in i2c_read() in i2c-uclass.c. Each driver would be much simpler.

That is the point of my suggestion.

Quoted 48 lines

Quoted 37 lines

Quoted 27 lines

Quoted 18 lines

drivers/i2c/i2c-uclass.c line 113

Quoted 12 lines

drivers/i2c/i2c-uclass.c new file +4 -0

+     }
+     debug("not found\n");
+     return i2c_bind_driver(bus, chip_addr, devp);
+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

Quoted 8 lines

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

Quoted 8 lines

This actually creates the device. A generic I2C device is something that has no specific driver, but you can use read()/write() on it.

As an example, if we have an EEPROM we might add a special driver for it with functions like 'erase' and 'lock'. In that case we would bind the EEPROM driver to this address on the I2C bus. But for many cases we don't have/need a special driver, and can just fall back to one that supports simple read() and write() only.

Quoted 6 lines

Sorry, I could not parse you here.

I2C is not a hot-plugged bus.
I could not understand why such a dummy device is created on run time. Is it related to 'erase' or 'lock' functions?

If we cannot write to the chip (i.e. it does not ACK when we send it its address) then we won't be able to talk to it, so there is no point in creating a device.

Quoted 3 lines

With driver model / device tree we could just blindly add the device and use it, but I chose to duplicate the current behaviour since this is expected.

Do you mean you implemented the same (similar) behavior as the legacy I2C framework?

Quoted 10 lines

Quoted 10 lines

Because the clock-frequency is set to <400000> in the sandbox DTS, writing to I2C fails unless we change the I2C speed.

Is this intentional?

Quoted 5 lines

Quoted 1 line

Personally, I like everything to work on the mail line.

This is test code, as it says in the comment. I'm considering splitting sandbox into two boards, one with the test code and one without. But let's see how this develops.

I see.

Quoted 1 line

Do you mean you implemented the same (similar) behavior as the legacy I2C framework?

Yes, we need the ability to scan the bus and find which devices are present.