Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452753 > unrolled thread

[PATCH 0/5] usb: ulpi: remove "dev" field from struct ulpi_ops

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] usb: ulpi: remove "dev" field from struct ulpi_ops Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200
    [PATCH 1/5] usb: ulpi: move setting of ulpi->dev parent up in ulpi_register() Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200
    [PATCH 2/5] usb: ulpi: change operations api to pass parent device directly Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200
    [PATCH 4/5] usb: ulpi: make ops struct constant Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200
    [PATCH 3/5] usb: ulpi: remove "dev" field from struct ulpi_ops Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200
    [PATCH 5/5] usb: dwc3: ulpi: make dwc3_ulpi_ops constant Tal Shorer <tal.shorer@gmail.com> - 2016-07-31 01:30 +0200

#1452753 — [PATCH 0/5] usb: ulpi: remove "dev" field from struct ulpi_ops

struct ulpi_ops is defined as follows:

struct ulpi_ops {
        struct device *dev;
        int (*read)(struct ulpi_ops *ops, u8 addr);
        int (*write)(struct ulpi_ops *ops, u8 addr, u8 val);
};

Upon calling ulpi_register_interface(), the struct device argument is
put inside the struct ulpi_ops argument's dev field. Later, when
calling the actual read()/write() operations, the struct ulpi_ops is
passed to them and they use the stored device to access whatever
private data they need.

This means that if one wishes to reuse the same oprations for multiple
interfaces (e.g if we have multiple instances of the same controller),
any but the last interface registered will not operate properly (and
the one that does work will be at the mercy of the others to not mess
it up).

I understand that barely any driver uses this bus right now, but I
suppose it's there to be used at some point. We might as well fix the
design here before we hit this bug.

This series fixes this by passing the given struct device directly to
the operation functions via ulpi->dev.parent in ulpi_read() and
ulpi_write(). It also changes the operations struct to be constant
since now nobody has a reason to modify it.

Tal Shorer (5):
  usb: ulpi: move setting of ulpi->dev parent up in ulpi_register()
  usb: ulpi: change operations api to pass parent device directly
  usb: ulpi: remove "dev" field from struct ulpi_ops
  usb: ulpi: make ops struct constant
  usb: dwc3: ulpi: make dwc3_ulpi_ops constant

 drivers/usb/common/ulpi.c      | 11 ++++++-----
 drivers/usb/dwc3/ulpi.c        | 10 +++++-----
 include/linux/ulpi/driver.h    |  2 +-
 include/linux/ulpi/interface.h |  8 ++++----
 4 files changed, 16 insertions(+), 15 deletions(-)

--
2.7.4

[toc] | [next] | [standalone]

#1452754 — [PATCH 1/5] usb: ulpi: move setting of ulpi->dev parent up in ulpi_register()

Once ulpi operations use the parent device directly, this will be
needed during the operations used in ulpi_register() itself, so set
the parent field before calling any ulpi operations.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/common/ulpi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 01c0c04..d1f419c 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -156,6 +156,8 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
 {
 	int ret;
 
+	ulpi->dev.parent = dev; /* needed early for ops */
+
 	/* Test the interface */
 	ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
 	if (ret < 0)
@@ -174,7 +176,6 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
 	ulpi->id.product = ulpi_read(ulpi, ULPI_PRODUCT_ID_LOW);
 	ulpi->id.product |= ulpi_read(ulpi, ULPI_PRODUCT_ID_HIGH) << 8;
 
-	ulpi->dev.parent = dev;
 	ulpi->dev.bus = &ulpi_bus;
 	ulpi->dev.type = &ulpi_dev_type;
 	dev_set_name(&ulpi->dev, "%s.ulpi", dev_name(dev));
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452755 — [PATCH 2/5] usb: ulpi: change operations api to pass parent device directly

struct ulpi_ops is defined as follows:

struct ulpi_ops {
	struct device *dev;
	int (*read)(struct ulpi_ops *ops, u8 addr);
	int (*write)(struct ulpi_ops *ops, u8 addr, u8 val);
};

Upon calling ulpi_register_interface(), the struct device argument is
put inside the struct ulpi_ops argument's dev field. Later, when
calling the actual read()/write() operations, the struct ulpi_ops is
passed to them and they use the stored device to access whatever
private data they need.

This means that if one wishes to reuse the same oprations for multiple
interfaces (e.g if we have multiple instances of the same controller),
any but the last interface registered will not operate properly (and
the one that does work will be at the mercy of the others to not mess
it up).

Fix this by passing the given struct device directly to the operation
functions (ulpi->dev.parent) in ulpi_read() and ulpi_write() instead
of via the ops argument

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/common/ulpi.c      | 4 ++--
 drivers/usb/dwc3/ulpi.c        | 8 ++++----
 include/linux/ulpi/interface.h | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index d1f419c..fdaed7c 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -21,13 +21,13 @@
 
 int ulpi_read(struct ulpi *ulpi, u8 addr)
 {
-	return ulpi->ops->read(ulpi->ops, addr);
+	return ulpi->ops->read(ulpi->dev.parent, addr);
 }
 EXPORT_SYMBOL_GPL(ulpi_read);
 
 int ulpi_write(struct ulpi *ulpi, u8 addr, u8 val)
 {
-	return ulpi->ops->write(ulpi->ops, addr, val);
+	return ulpi->ops->write(ulpi->dev.parent, addr, val);
 }
 EXPORT_SYMBOL_GPL(ulpi_write);
 
diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index ec004c6..51ac939 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -35,9 +35,9 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
 	return -ETIMEDOUT;
 }
 
-static int dwc3_ulpi_read(struct ulpi_ops *ops, u8 addr)
+static int dwc3_ulpi_read(struct device *dev, u8 addr)
 {
-	struct dwc3 *dwc = dev_get_drvdata(ops->dev);
+	struct dwc3 *dwc = dev_get_drvdata(dev);
 	u32 reg;
 	int ret;
 
@@ -53,9 +53,9 @@ static int dwc3_ulpi_read(struct ulpi_ops *ops, u8 addr)
 	return DWC3_GUSB2PHYACC_DATA(reg);
 }
 
-static int dwc3_ulpi_write(struct ulpi_ops *ops, u8 addr, u8 val)
+static int dwc3_ulpi_write(struct device *dev, u8 addr, u8 val)
 {
-	struct dwc3 *dwc = dev_get_drvdata(ops->dev);
+	struct dwc3 *dwc = dev_get_drvdata(dev);
 	u32 reg;
 
 	reg = DWC3_GUSB2PHYACC_NEWREGREQ | DWC3_ULPI_ADDR(addr);
diff --git a/include/linux/ulpi/interface.h b/include/linux/ulpi/interface.h
index 4de8ab4..ac3cd80 100644
--- a/include/linux/ulpi/interface.h
+++ b/include/linux/ulpi/interface.h
@@ -13,8 +13,8 @@ struct ulpi;
  */
 struct ulpi_ops {
 	struct device *dev;
-	int (*read)(struct ulpi_ops *ops, u8 addr);
-	int (*write)(struct ulpi_ops *ops, u8 addr, u8 val);
+	int (*read)(struct device *dev, u8 addr);
+	int (*write)(struct device *dev, u8 addr, u8 val);
 };
 
 struct ulpi *ulpi_register_interface(struct device *, struct ulpi_ops *);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452756 — [PATCH 4/5] usb: ulpi: make ops struct constant

None of the core ulpi functions perform any changes to the operations
struct, and logically as a struct that contains function pointers
there's no reason it shouldn't be constant.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/common/ulpi.c      | 3 ++-
 include/linux/ulpi/driver.h    | 2 +-
 include/linux/ulpi/interface.h | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 0439e96..d4ff6df 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -202,7 +202,8 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
  * Allocates and registers a ULPI device and an interface for it. Called from
  * the USB controller that provides the ULPI interface.
  */
-struct ulpi *ulpi_register_interface(struct device *dev, struct ulpi_ops *ops)
+struct ulpi *ulpi_register_interface(struct device *dev,
+				     const struct ulpi_ops *ops)
 {
 	struct ulpi *ulpi;
 	int ret;
diff --git a/include/linux/ulpi/driver.h b/include/linux/ulpi/driver.h
index 388f6e0..a44408f 100644
--- a/include/linux/ulpi/driver.h
+++ b/include/linux/ulpi/driver.h
@@ -15,7 +15,7 @@ struct ulpi_ops;
  */
 struct ulpi {
 	struct ulpi_device_id id;
-	struct ulpi_ops *ops;
+	const struct ulpi_ops *ops;
 	struct device dev;
 };
 
diff --git a/include/linux/ulpi/interface.h b/include/linux/ulpi/interface.h
index ee6e35a..43b0738 100644
--- a/include/linux/ulpi/interface.h
+++ b/include/linux/ulpi/interface.h
@@ -17,7 +17,7 @@ struct ulpi_ops {
 	int (*write)(struct device *dev, u8 addr, u8 val);
 };
 
-struct ulpi *ulpi_register_interface(struct device *, struct ulpi_ops *);
+struct ulpi *ulpi_register_interface(struct device *, const struct ulpi_ops *);
 void ulpi_unregister_interface(struct ulpi *);
 
 #endif /* __LINUX_ULPI_INTERFACE_H */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452757 — [PATCH 3/5] usb: ulpi: remove "dev" field from struct ulpi_ops

Operations now use ulpi->dev.parent directly instead of via the
ulpi_ops struct, making this field unused. Remove it.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/common/ulpi.c      | 1 -
 include/linux/ulpi/interface.h | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index fdaed7c..0439e96 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -212,7 +212,6 @@ struct ulpi *ulpi_register_interface(struct device *dev, struct ulpi_ops *ops)
 		return ERR_PTR(-ENOMEM);
 
 	ulpi->ops = ops;
-	ops->dev = dev;
 
 	ret = ulpi_register(dev, ulpi);
 	if (ret) {
diff --git a/include/linux/ulpi/interface.h b/include/linux/ulpi/interface.h
index ac3cd80..ee6e35a 100644
--- a/include/linux/ulpi/interface.h
+++ b/include/linux/ulpi/interface.h
@@ -4,6 +4,7 @@
 #include <linux/types.h>
 
 struct ulpi;
+struct device;
 
 /**
  * struct ulpi_ops - ULPI register access
@@ -12,7 +13,6 @@ struct ulpi;
  * @write: write operation for ULPI register access
  */
 struct ulpi_ops {
-	struct device *dev;
 	int (*read)(struct device *dev, u8 addr);
 	int (*write)(struct device *dev, u8 addr, u8 val);
 };
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452758 — [PATCH 5/5] usb: dwc3: ulpi: make dwc3_ulpi_ops constant

ulpi_register_interface() accepts a const struct ulpi_ops and dwc3
doesn't perform any changes to this struct at runtime, so there's no
reason it shouldn't be constant.

Signed-off-by: Tal Shorer <tal.shorer@gmail.com>
---
 drivers/usb/dwc3/ulpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
index 51ac939..bd86f84 100644
--- a/drivers/usb/dwc3/ulpi.c
+++ b/drivers/usb/dwc3/ulpi.c
@@ -65,7 +65,7 @@ static int dwc3_ulpi_write(struct device *dev, u8 addr, u8 val)
 	return dwc3_ulpi_busyloop(dwc);
 }
 
-static struct ulpi_ops dwc3_ulpi_ops = {
+static const struct ulpi_ops dwc3_ulpi_ops = {
 	.read = dwc3_ulpi_read,
 	.write = dwc3_ulpi_write,
 };
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web