drm/dsi: DSI for devices with different control bus

We can have devices where the data bus is MIPI DSI, but the control bus is something else (i2c, spi etc). A typical example is i2c controlled encoder bridge chips.

Such devices too require passing DSI specific parameters (number of data lanes, DSI mode flags, color format etc) to their DSI host. For a device that isn't 'mipi_dsi_device', there is no way of passing such parameters.

Provide the option of creating a dummy DSI device. The main purpose of this would be to attach to a DSI host by calling mipi_dsi_attach, and pass DSI params.

Create mipi_dsi_new_dummy for creating a dummy dsi device. The driver calling this needs to be aware of the mipi_dsi_host it wants to attach to, and also the DSI virtual channel the DSI device intends to use.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 78 ++++++++++++++++++++++++++++++++++++++++--
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 78 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index 2d5ca8ee..9bfe215 100644

drivers/gpu/drm/drm_mipi_dsi.c +5 -0

+
+	return 0;
 }
 
 static const struct dev_pm_ops mipi_dsi_device_pm_ops = {

@@ -171,6 +178,67 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 	return dsi;
 }
 
+static int dummy_probe(struct mipi_dsi_device *dsi)
+{
+	return 0;

drivers/gpu/drm/drm_mipi_dsi.c +55 -1

+
+static struct mipi_dsi_driver dummy_dsi_driver = {
+	.probe = dummy_probe,
+	.remove = dummy_remove,
+	.shutdown = dummy_shutdown,
+	.driver.name = "mipi_dsi_dummy",
+};
+
+static int mipi_dsi_device_add_dummy(struct mipi_dsi_device *dsi)
+{
+	struct mipi_dsi_host *host = dsi->host;
+
+	dev_set_name(&dsi->dev, "%s.dummy_dev.%d", dev_name(host->dev),
+			dsi->channel);
+
+	return device_add(&dsi->dev);
+}
+
+struct mipi_dsi_device *mipi_dsi_new_dummy(struct mipi_dsi_host *host, u32 reg)
+{
+	struct mipi_dsi_device *dsi;
+	struct device *dev = host->dev;
+	int ret;
+
+	if (reg > 3) {
+		dev_err(dev, "invalid reg property %u\n", reg);
+		return ERR_PTR(-EINVAL);
+	}
+
+	dsi = mipi_dsi_device_alloc(host);
+	if (IS_ERR(dsi)) {
+		dev_err(dev, "failed to allocate dummy DSI device %ld\n",
+			PTR_ERR(dsi));
+		return dsi;
+	}
+
+	dsi->channel = reg;
+
+	ret = mipi_dsi_device_add_dummy(dsi);
+	if (ret) {
+		dev_err(dev, "failed to add dummy DSI device %d\n", ret);
+		kfree(dsi);
+		return ERR_PTR(ret);
+	}
+
+	return dsi;
+}
+
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;

@@ -924,7 +992,13 @@ EXPORT_SYMBOL(mipi_dsi_driver_unregister);
 
 static int __init mipi_dsi_bus_init(void)
 {
-	return bus_register(&mipi_dsi_bus_type);
+	int ret;
+
+	ret = bus_register(&mipi_dsi_bus_type);
+	if (ret < 0)
+		return ret;
+
+	return mipi_dsi_driver_register(&dummy_dsi_driver);
 }
 postcore_initcall(mipi_dsi_bus_init);
 

include/drm/drm_mipi_dsi.h +0 -0

index f1d8d0d..d06ba99 100644

Quoted 27 lines · Archit Taneja

We can have devices where the data bus is MIPI DSI, but the control bus is something else (i2c, spi etc). A typical example is i2c controlled encoder bridge chips.

Such devices too require passing DSI specific parameters (number of data lanes, DSI mode flags, color format etc) to their DSI host. For a device that isn't 'mipi_dsi_device', there is no way of passing such parameters.

Provide the option of creating a dummy DSI device. The main purpose of this would be to attach to a DSI host by calling mipi_dsi_attach, and pass DSI params.

Create mipi_dsi_new_dummy for creating a dummy dsi device. The driver calling this needs to be aware of the mipi_dsi_host it wants to attach to, and also the DSI virtual channel the DSI device intends to use.

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/gpu/drm/drm_mipi_dsi.c | 78 ++++++++++++++++++++++++++++++++++++++++--
  include/drm/drm_mipi_dsi.h     |  2 ++
  2 files changed, 78 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index 2d5ca8ee..9bfe215 100644

drivers/gpu/drm/drm_mipi_dsi.c line 55

Quoted 10 lines

drivers/gpu/drm/drm_mipi_dsi.c +6 -1

@@ -47,7 +47,14 @@
 
  static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
  {
-	return of_driver_match_device(dev, drv);
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	if (!strcmp(drv->name, "mipi_dsi_dummy") &&
+			strstr(dev_name(dev), "dummy_dev"))
+		return 1;

Is this kind of fuzzy matching used in other dummy devs? It looks little bit scary. You
can at least replace

strstr(dev_name(dev), "dummy_dev"))

with

strstr(dev_name(dev), ".dummy_dev."))

Anyway, currently it should not break anything, am I right?

Quoted 10 lines

drivers/gpu/drm/drm_mipi_dsi.c +2 -0

+
+	return 0;
  }
 
  static const struct dev_pm_ops mipi_dsi_device_pm_ops = {

@@ -171,6 +178,67 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
  	return dsi;
  }
 

drivers/gpu/drm/drm_mipi_dsi.c of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node) · line 193

Quoted 13 lines

drivers/gpu/drm/drm_mipi_dsi.c +13 -0

+static int dummy_probe(struct mipi_dsi_device *dsi)
+{
+	return 0;
+}
+
+static int dummy_remove(struct mipi_dsi_device *dsi)
+{
+	return 0;
+}
+
+static void dummy_shutdown(struct mipi_dsi_device *dsi)
+{
+}

I suppose these callbacks are optional, so you can omit them.

mipi_dsi_devices are inherently aware of their host because they share a parent-child hierarchy in the device tree.

Non-dsi drivers that create a dummy dsi device don't have this data. In order to get this information, they require to a phandle to the dsi host in the device tree.

Maintain a list of all the hosts DSI that are currently registered.

This list will be used to find the mipi_dsi_host corresponding to the device_node passed in of_find_mipi_dsi_host_by_node.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 32 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +30 -0

index 9bfe215..81ddb73 100644
@@ -239,6 +239,28 @@ struct mipi_dsi_device *mipi_dsi_new_dummy(struct mipi_dsi_host *host, u32 reg)
 	return dsi;
 }
 
+static DEFINE_MUTEX(host_lock);
+static LIST_HEAD(host_list);
+
+struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node)
+{
+	struct mipi_dsi_host *host;
+
+	mutex_lock(&host_lock);
+
+	list_for_each_entry(host, &host_list, list) {
+		if (host->dev->of_node == node) {
+			mutex_unlock(&host_lock);
+			return host;
+		}
+	}
+
+	mutex_unlock(&host_lock);
+
+	return NULL;
+}
+EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
+
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;

@@ -250,6 +272,10 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 		of_mipi_dsi_device_add(host, node);
 	}
 
+	mutex_lock(&host_lock);
+	list_add_tail(&host->list, &host_list);
+	mutex_unlock(&host_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL(mipi_dsi_host_register);

@@ -266,6 +292,10 @@ static int mipi_dsi_remove_device_fn(struct device *dev, void *priv)
 void mipi_dsi_host_unregister(struct mipi_dsi_host *host)
 {
 	device_for_each_child(host->dev, NULL, mipi_dsi_remove_device_fn);
+
+	mutex_lock(&host_lock);
+	list_del_init(&host->list);
+	mutex_unlock(&host_lock);
 }
 EXPORT_SYMBOL(mipi_dsi_host_unregister);
 

include/drm/drm_mipi_dsi.h +0 -0

index d06ba99..1684a0e 100644
@@ -100,10 +100,12 @@ struct mipi_dsi_host_ops {
 struct mipi_dsi_host {
 	struct device *dev;

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Archit Taneja (2):
drm/dsi: Create dummy DSI devices
drm/dsi: Get DSI host by DT device node

  drivers/gpu/drm/drm_mipi_dsi.c | 108 ++++++++++++++++++++++++++++++++++++++++-
  include/drm/drm_mipi_dsi.h     |   4 ++
  2 files changed, 110 insertions(+), 2 deletions(-)

Hi,

Quoted 13 lines · Archit Taneja

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

Quoted 4 lines

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Any comments on this?

Quoted 16 lines

Hi,

Quoted 13 lines · Archit Taneja

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

Quoted 6 lines

Quoted 4 lines

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Any comments on this?

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

Hi Thierry, Archit,

Am Mittwoch, den 19.08.2015, 15:13 +0200 schrieb Thierry Reding:

Quoted 91 lines · Archit Taneja

Quoted 16 lines

Hi,

Quoted 13 lines · Archit Taneja

We are currently restricted when it comes to supporting DSI on devices that have a non-DSI control bus. For example, DSI encoder chips are available in the market that are configured via i2c. Configuring their registers via DSI bus is either optional or not available at all.

These devices still need to pass DSI parameters (data lanes, mode flags etc) to the DSI host they are connected to. We don't have a way to do that at the moment.

The method presented in these patches is to provide an API to create a 'dummy' mipi_dsi_device. This device is populated with the desired DSI params, which are passed on to the host via mipi_dsi_attach().

Quoted 6 lines

Quoted 4 lines

This method will require the device driver to get a phandle to the DSI host since there is no parent-child relation between the two.

Is there a better way to do this? Please let me know!

Any comments on this?

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Multiple device drivers in both the media and DRM universe have shown that they are a working way to represent those data bus connections between devices.
I know this might make things a bit more complicated for Tegra DRM, where you have a nice parent<->child relationship between the components even on the control path so far, but we should really move into the direction of more drivers using the standardized bindings for this stuff, instead of doing another round of NIH.

Quoted 2 lines

Hi Thierry, Archit,

Quoted 72 lines

Quoted 67 lines

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

Quoted 8 lines

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

Quoted 8 lines

Multiple device drivers in both the media and DRM universe have shown that they are a working way to represent those data bus connections between devices.
I know this might make things a bit more complicated for Tegra DRM, where you have a nice parent<->child relationship between the components even on the control path so far, but we should really move into the direction of more drivers using the standardized bindings for this stuff, instead of doing another round of NIH.

Why are you bringing up Tegra DRM? I don't see how it's relevant in any way to this discussion.

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 81 lines

Quoted 80 lines

Quoted 67 lines

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

Quoted 4 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

Quoted 10 lines

Quoted 4 lines

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

You can then use of-graph to model the data path between the DSI encoder and device.

Quoted 5 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 4 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Quoted 3 lines

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 86 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 81 lines

Quoted 80 lines

Quoted 67 lines

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

Quoted 7 lines

Quoted 4 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 14 lines

Quoted 10 lines

Quoted 4 lines

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

Quoted 2 lines

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Hi Thierry, Lucas,

Quoted 113 lines · Thierry Reding

Quoted 86 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 81 lines

Quoted 80 lines

Quoted 67 lines

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

Quoted 7 lines

Quoted 4 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 14 lines

Quoted 10 lines

Quoted 4 lines

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

Quoted 7 lines

Quoted 2 lines

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
	adv7533@.. {

		port {
			adv_in: endpoint {
				remote-endpoint = <&dsi_out>;
			};
		};
	};
};

dsi_host@.. {
	...
	...

	port {
		dsi_out: endpoint {
			remote-endpoint = <&adv_in>;
		}
	};
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Could you explain what you meant by this? I.e. describing the kind of DSI device?

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

Quoted 7 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

From an adv75xx driver perspective, it should also support the ADV7511 chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Quoted 166 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 113 lines · Thierry Reding

Quoted 86 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 81 lines

Quoted 80 lines

Quoted 67 lines

Perhaps a better way would be to invert this relationship. According to your proposal we'd have to have DT like this:

	i2c@... {
		...

		dsi-device@... {
			...
			dsi-bus = <&dsi>;
			...
		};

		...
	};

	dsi@... {
		...
	};

Inversing the relationship would become something like this:

	i2c@... {
		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			i2c-bus = <&i2c>;
			...
		};

		...
	};

Both of those aren't fundamentally different, and they both have the disavantage of lacking ways to transport configuration data that the other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at the driver level:

	i2c@... {
		...

		i2cdsi: dsi-device@... {
			...
		};

		...
	};

	dsi@... {
		...

		peripheral@... {
			...
			control = <&i2cdsi>;
			...
		};

		...
	};

Quoted 5 lines

This way we'll get both an I2C device and a DSI device that we can fully describe using the standard device tree bindings. At driver time we can get the I2C device from the phandle in the control property of the DSI device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Devicetree is structured along the control buses, even if the devices are connected to multiple buses, in the DT they are always children of the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly a child of the i2c master controller and only of that one.

Quoted 7 lines

Quoted 4 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my opinion that's reason enough to represent it as two logical devices.

Does it really have 2 control interfaces that are used at the same time? Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 14 lines

Quoted 10 lines

Quoted 4 lines

If you need to model connections between devices that are not reflected through the control bus hierarchy you should really consider using the standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add two logical devices to DT (one for each bus interface), you don't have anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But if there is only one control bus to the device I think it should be one device sitting beneath the control bus.

Quoted 7 lines

Quoted 2 lines

You can then use of-graph to model the data path between the DSI encoder and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
	adv7533@.. {

		port {
			adv_in: endpoint {
				remote-endpoint = <&dsi_out>;
			};
		};
	};
};

dsi_host@.. {
	...
	...

	port {
		dsi_out: endpoint {
			remote-endpoint = <&adv_in>;
		}
	};
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

    i2c@... {
        ...

        dsi-device@... {
            ...
            dsi-bus = <&dsi>;
            ...
        };

        ...
    };

    dsi@... {
        ...
    };

Inversing the relationship would become something like this:

    i2c@... {
        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            i2c-bus = <&i2c>;
            ...
        };

        ...
    };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

    i2c@... {
        ...

        i2cdsi: dsi-device@... {
            ...
        };

        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            control = <&i2cdsi>;
            ...
        };

        ...
    };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
    adv7533@.. {

        port {
            adv_in: endpoint {
                remote-endpoint = <&dsi_out>;
            };
        };
    };
};

dsi_host@.. {
    ...
    ...

    port {
        dsi_out: endpoint {
            remote-endpoint = <&adv_in>;
        }
    };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

Thierry,

Quoted 284 lines · Archit Taneja

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

    i2c@... {
        ...

        dsi-device@... {
            ...
            dsi-bus = <&dsi>;
            ...
        };

        ...
    };

    dsi@... {
        ...
    };

Inversing the relationship would become something like this:

    i2c@... {
        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            i2c-bus = <&i2c>;
            ...
        };

        ...
    };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

    i2c@... {
        ...

        i2cdsi: dsi-device@... {
            ...
        };

        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            control = <&i2cdsi>;
            ...
        };

        ...
    };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
    adv7533@.. {

        port {
            adv_in: endpoint {
                remote-endpoint = <&dsi_out>;
            };
        };
    };
};

dsi_host@.. {
    ...
    ...

    port {
        dsi_out: endpoint {
            remote-endpoint = <&adv_in>;
        }
    };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 3 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

Thanks,
Archit

Quoted 314 lines · Archit Taneja

Thierry,

Quoted 284 lines · Archit Taneja

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

     i2c@... {
         ...

         dsi-device@... {
             ...
             dsi-bus = <&dsi>;
             ...
         };

         ...
     };

     dsi@... {
         ...
     };

Inversing the relationship would become something like this:

     i2c@... {
         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             i2c-bus = <&i2c>;
             ...
         };

         ...
     };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

     i2c@... {
         ...

         i2cdsi: dsi-device@... {
             ...
         };

         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             control = <&i2cdsi>;
             ...
         };

         ...
     };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
     adv7533@.. {

         port {
             adv_in: endpoint {
                 remote-endpoint = <&dsi_out>;
             };
         };
     };
};

dsi_host@.. {
     ...
     ...

     port {
         dsi_out: endpoint {
             remote-endpoint = <&adv_in>;
         }
     };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 3 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

Quoted 7 lines

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here.
The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 338 lines · Andrzej Hajda

Quoted 314 lines · Archit Taneja

Thierry,

Quoted 284 lines · Archit Taneja

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

    i2c@... {
        ...

        dsi-device@... {
            ...
            dsi-bus = <&dsi>;
            ...
        };

        ...
    };

    dsi@... {
        ...
    };

Inversing the relationship would become something like this:

    i2c@... {
        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            i2c-bus = <&i2c>;
            ...
        };

        ...
    };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

    i2c@... {
        ...

        i2cdsi: dsi-device@... {
            ...
        };

        ...
    };

    dsi@... {
        ...

        peripheral@... {
            ...
            control = <&i2cdsi>;
            ...
        };

        ...
    };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
    adv7533@.. {

        port {
            adv_in: endpoint {
                remote-endpoint = <&dsi_out>;
            };
        };
    };
};

dsi_host@.. {
    ...
    ...

    port {
        dsi_out: endpoint {
            remote-endpoint = <&adv_in>;
        }
    };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 3 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

Quoted 7 lines

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here.
The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Quoted 2 lines

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Quoted 341 lines · Archit Taneja

Quoted 338 lines · Andrzej Hajda

Quoted 314 lines · Archit Taneja

Thierry,

Quoted 284 lines · Archit Taneja

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

     i2c@... {
         ...

         dsi-device@... {
             ...
             dsi-bus = <&dsi>;
             ...
         };

         ...
     };

     dsi@... {
         ...
     };

Inversing the relationship would become something like this:

     i2c@... {
         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             i2c-bus = <&i2c>;
             ...
         };

         ...
     };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

     i2c@... {
         ...

         i2cdsi: dsi-device@... {
             ...
         };

         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             control = <&i2cdsi>;
             ...
         };

         ...
     };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
     adv7533@.. {

         port {
             adv_in: endpoint {
                 remote-endpoint = <&dsi_out>;
             };
         };
     };
};

dsi_host@.. {
     ...
     ...

     port {
         dsi_out: endpoint {
             remote-endpoint = <&adv_in>;
         }
     };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 3 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

Quoted 7 lines

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here.
The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Quoted 5 lines

Quoted 2 lines

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

Hi Rob, Mark,

We've been trying to figure out the right way to represent a class of display encoder devices in DT.

These devices have registers that are generally configured via i2c. Once the device is configured, it takes in video data from the mipi dsi bus.

Until now, all the devices we've supported devices that can be are configured by the dsi bus itself, and hence, we've been able to represent them in DT as children under the dsi host.

For the above class of devices (using both i2c and dsi), we aren't able to conclude upon what's the better approach among the two:

1. Represent the device via 2 different nodes in DT. One would be a child under an i2c adapter, the other a child of a dsi host. We would have two device drivers, one i2c client, and the other a mipi dsi device.

2. Represent the device as an i2c client in DT. Provide an api to create "dummy" dsi devices. The i2c client driver would use this api to register a dsi device, and link itself with the dsi host.

What do you think would be the way to go here? I guess you might have faced something similar in other subsystems.

I've given a relatively brief description of the problem. There were some more nitty gritties discussed in this thread before.

Thierry, Andrzej, Lucas,

Please feel free to add your comments if I have missed out on something.

Thanks,
Archit

Quoted 419 lines · Thierry Reding

Quoted 407 lines · Archit Taneja

Quoted 404 lines · Andrzej Hajda

Quoted 369 lines · Archit Taneja

Thierry,

Quoted 337 lines · Archit Taneja

Quoted 208 lines · Thierry Reding

Quoted 207 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 154 lines · Thierry Reding

Quoted 110 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 98 lines

Quoted 89 lines

Quoted 85 lines

Perhaps a better way would be to invert this relationship.
According to
your proposal we'd have to have DT like this:

i2c@... {
...

dsi-device@... {
...
dsi-bus = <&dsi>;
...
};

...
};

dsi@... {
...
};

Inversing the relationship would become something like this:

i2c@... {
...
};

dsi@... {
...

peripheral@... {
...
i2c-bus = <&i2c>;
...
};

...
};

Both of those aren't fundamentally different, and they both
have
the
disavantage of lacking ways to transport configuration data
that
the
other bus needs to instantiate the dummy device (such as
the reg
property for example, denoting the I2C slave address or the
DSI
VC).

So how about we create two devices in the device tree and fuse
them at
the driver level:

i2c@... {
...

i2cdsi: dsi-device@... {
...
};

...
};

dsi@... {
...

peripheral@... {
...
control = <&i2cdsi>;
...
};

...
};

This way we'll get both an I2C device and a DSI device that we
can fully
describe using the standard device tree bindings. At driver
time
we can
get the I2C device from the phandle in the control property of
the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing
yet-another-way to
handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 7 lines

I think that's a flawed interpretation of what's going on here. The
device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 6 lines

Does it really have 2 control interfaces that are used at the same
time?
Or is the DSI connection a passive data bus if the register control
happens through i2c?

The interfaces may not be used at the same time, and the DSI interface
may even be crippled, but the device is still addressable from the DSI
host controller, if for nothing else than for routing the video stream.

Quoted 25 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 10 lines

The problem is that the original proposal would instantiate a dummy
device, so it wouldn't be represented in DT at all. So unless you
do add
two logical devices to DT (one for each bus interface), you don't
have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution.
But
if there is only one control bus to the device I think it should be
one
device sitting beneath the control bus.

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

Quoted 6 lines

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to
add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which
has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
     adv7533@.. {

         port {
             adv_in: endpoint {
                 remote-endpoint = <&dsi_out>;
             };
         };
     };
};

dsi_host@.. {
     ...
     ...

     port {
         dsi_out: endpoint {
             remote-endpoint = <&adv_in>;
         }
     };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 8 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags
via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client
node, but that does make it seem like a quirk. The 'reg' way under the
DSI host is definitely better to populate the virtual channel.

Quoted 11 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C
driver, that's what I find backwards. There is already a standard way
for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or
dsi, but not both. Hence, I came up with this approach.

Quoted 20 lines

Quoted 13 lines

Quoted 12 lines

On the other hand if you properly instantiate the DSI device you can
easily write a driver for it, and the driver will set up the correct
properties as implied by the compatible string. Once you have that you
can easily hook it up to the I2C control interface in whatever way you
like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 6 lines

With the fused approach you suggested, we would have 2 drivers:
one i2c
and the other dsi. The i2c client driver would be more or less minimal,
preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Quoted 5 lines

Correct. That's kind of similar to the way an HDMI encoder driver would
use an I2C adapter to query EDID. The i2c_client device would be a means
for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore,
an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 9 lines

Quoted 8 lines

We can do without dummy dsi devices with this method. But, representing
a device with 2 DT nodes seems a bit off. We'd also need to compatible
strings for the same device, one for the i2c part, and the other for
the dsi part.

Quoted 11 lines

I agree that this somewhat stretches the capabilities of device tree.
Another alternative I guess would be to not have a compatible string for
the I2C device at all (that's technically not valid, I guess) because we
really don't need an I2C driver for the device. What we really need is a
DSI driver with a means to talk over some I2C bus with some other part
of its device.

I think what the driver should 'really' be is a bit subjective, and can
vary based on what the buses are used for in the device. For the Toshiba
chip that Jani mentioned, it tends more towards a DSI driver. Whereas,
for an ADV75xx chip, it's closer to an I2C driver since only I2C can be
used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for
routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e,
virtual channel number) always gives it some 'control' aspect.

Quoted 10 lines

Quoted 9 lines

From an adv75xx driver perspective, it should also support the
ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a
DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the
other, with both chips being supported by the same driver.

Quoted 2 lines

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 4 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2
compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

Quoted 7 lines

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but
in this particular case it is purely data bus. So of-graph bindings seem to be
better choice. As already Lucas Stach said DT hierarchy should describe control
buses and of-graph bindings should describe data bus. Argument that hw has two
interfaces does not seem to be valid here - it has only one control interface.
The other one is only for data, representing every data interface using DT
hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess
lightweight framework providing DSI hosts detached from Linux device model could
work better here.
The only problem here is that it should coexist somehow with dsi bus used to
control devices. Anyway implementing it shouldn't be hard, question is if it
would be eventually accepted :) I guess we can live for now with dummy devs.

Quoted 5 lines

Quoted 2 lines

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

Quoted 4 lines · Archit Taneja

Hi Rob, Mark,

We've been trying to figure out the right way to represent a class of display encoder devices in DT.

I've been meaning to reply on this.

Quoted 16 lines

These devices have registers that are generally configured via i2c. Once the device is configured, it takes in video data from the mipi dsi bus.

Until now, all the devices we've supported devices that can be are configured by the dsi bus itself, and hence, we've been able to represent them in DT as children under the dsi host.

For the above class of devices (using both i2c and dsi), we aren't able to conclude upon what's the better approach among the two:

1. Represent the device via 2 different nodes in DT. One would be a child under an i2c adapter, the other a child of a dsi host. We would have two device drivers, one i2c client, and the other a mipi dsi device.

Quoted 7 lines

2. Represent the device as an i2c client in DT. Provide an api to create "dummy" dsi devices. The i2c client driver would use this api to register a dsi device, and link itself with the dsi host.

What do you think would be the way to go here? I guess you might have faced something similar in other subsystems.

The closest thing I can think of are WiFi/BT combo chips that use SDIO+UART. In that case, 2 nodes makes sense as these chips are essentially 2 independent functions in a single chip and both interfaces are control interfaces (as well as data). This case is a bit different with both interfaces being tied to the same function.

So in this case, I think option 2 is the right way for the reasons already outlined in this thread. I think there needs to be more consistency in how the of-graph connections are defined with the core doing more of the graph traversing. So having an i2c device plus of-graph seems more consistent with other non-DSI cases.

The main open issue seemed to be setting the VC. At least for the ADV7533, it can be whatever you want it to be. The host and device just need to agree. I see no need for that to be in DT at least in this case. But then I'm not sure what are typical constraints for VC assignment. I'd guess that constraints are on the device side and hosts can support whatever the device wants. If so, then it can purely be up to the driver to set.

Implementation-wise, I don't think that I'd call this a dummy device. There are multiple ways for devices to get created in the driver model. DT is just one way. You are just creating the device in a different way outside of DT which is fine.

Quoted 341 lines · Archit Taneja

Quoted 338 lines · Andrzej Hajda

Quoted 314 lines · Archit Taneja

Thierry,

Quoted 284 lines · Archit Taneja

Quoted 187 lines · Thierry Reding

Quoted 186 lines · Archit Taneja

Hi Thierry, Lucas,

Quoted 132 lines · Thierry Reding

Quoted 102 lines · Lucas Stach

Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:

Quoted 3 lines · Lucas Stach

Quoted 2 lines

Hi Thierry, Archit,

[...]

Quoted 92 lines

Quoted 83 lines

Quoted 72 lines

Perhaps a better way would be to invert this relationship. According to
your proposal we'd have to have DT like this:

     i2c@... {
         ...

         dsi-device@... {
             ...
             dsi-bus = <&dsi>;
             ...
         };

         ...
     };

     dsi@... {
         ...
     };

Inversing the relationship would become something like this:

     i2c@... {
         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             i2c-bus = <&i2c>;
             ...
         };

         ...
     };

Both of those aren't fundamentally different, and they both have the
disavantage of lacking ways to transport configuration data that the
other bus needs to instantiate the dummy device (such as the reg property for example, denoting the I2C slave address or the DSI VC).

So how about we create two devices in the device tree and fuse them at
the driver level:

     i2c@... {
         ...

         i2cdsi: dsi-device@... {
             ...
         };

         ...
     };

     dsi@... {
         ...

         peripheral@... {
             ...
             control = <&i2cdsi>;
             ...
         };

         ...
     };

Quoted 8 lines

This way we'll get both an I2C device and a DSI device that we can fully
describe using the standard device tree bindings. At driver time we can
get the I2C device from the phandle in the control property of the DSI
device and use it to execute I2C transactions.

I don't really like to see that you are inventing yet-another-way to handle devices connected to multiple buses.

Quoted 8 lines

Devicetree is structured along the control buses, even if the devices
are connected to multiple buses, in the DT they are always children of
the bus that is used to control their registers from the CPUs perspective. So a DSI encoder that is controlled through i2c is clearly
a child of the i2c master controller and only of that one.

Quoted 5 lines

I think that's a flawed interpretation of what's going on here. The device in fact has two interfaces: one is I2C, the other is DSI. In my
opinion that's reason enough to represent it as two logical devices.

Quoted 4 lines

Does it really have 2 control interfaces that are used at the same time?
Or is the DSI connection a passive data bus if the register control happens through i2c?

The interfaces may not be used at the same time, and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream.

Quoted 20 lines

Quoted 7 lines

Quoted 6 lines

If you need to model connections between devices that are not reflected
through the control bus hierarchy you should really consider using the
standardized of-graph bindings. (Documentation/devicetree/bindings/graph.txt)

Quoted 7 lines

The problem is that the original proposal would instantiate a dummy device, so it wouldn't be represented in DT at all. So unless you do add
two logical devices to DT (one for each bus interface), you don't have
anything to glue together with an OF graph.

I see that the having dummy device is the least desirable solution. But
if there is only one control bus to the device I think it should be one
device sitting beneath the control bus.

Quoted 8 lines

Quoted 3 lines

You can then use of-graph to model the data path between the DSI encoder
and device.

But you will be needing a device below the DSI host controller to represent that endpoint of the connection. The DSI host controller itself is in no way connected to the I2C adapter. You would have to add some sort of quirk to the DSI controller binding to allow it to

Thanks for the review.

I implemented this to support ADV7533 DSI to HDMI encoder chip, which has a DSI video bus and an i2c control bus.

There weren't any quirks as such in the device tree when I tried to implement this. The DT seems to manage fine without a node corresponding to a mipi_dsi_device:

i2c_adap@.. {
     adv7533@.. {

         port {
             adv_in: endpoint {
                 remote-endpoint = <&dsi_out>;
             };
         };
     };
};

dsi_host@.. {
     ...
     ...

     port {
         dsi_out: endpoint {
             remote-endpoint = <&adv_in>;
         }
     };
};

It's the i2c driver's job to parse the graph and retrieve the phandle to the dsi host. Using this, it can proceed with registering itself to this host using the new dsi funcs. This patch does the same for the adv7533 i2c driver:

http://www.spinics.net/lists/dri-devel/msg86840.html

Quoted 2 lines

hook up with a control endpoint. And then you'll need more quirks to describe what kind of DSI device this is.

Quoted 7 lines

Quoted 2 lines

Could you explain what you meant by this? I.e. describing the kind of DSI device?

Describing the number of lanes, specifying the virtual channel, mode flags, etc. You could probably set the number of lanes and mode flags via the I2C driver, but especially the virtual channel cannot be set because it isn't known to the I2C DT branch of the device.

I agree with the VC part. It could be a DT entry within the i2c client node, but that does make it seem like a quirk. The 'reg' way under the DSI host is definitely better to populate the virtual channel.

Quoted 9 lines

Quoted 5 lines

The dsi device created isn't really a dummy device as such. It's dummy in the sense that there isn't a real dsi driver associated with it. The dsi device is still used to attach to a mipi dsi host, the way normal dsi devices do.

I understand, but I don't see why it has to be instantiated by the I2C driver, that's what I find backwards. There is already a standard way for instantiating DSI devices, why not use it?

I assumed we could either represent the device using an i2c driver, or dsi, but not both. Hence, I came up with this approach.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

On the other hand if you properly instantiate the DSI device you can easily write a driver for it, and the driver will set up the correct properties as implied by the compatible string. Once you have that you can easily hook it up to the I2C control interface in whatever way you like, be that an OF graph or just a simple unidirectional link by phandle.

Quoted 8 lines

Quoted 4 lines

With the fused approach you suggested, we would have 2 drivers: one i2c and the other dsi. The i2c client driver would be more or less minimal, preparing an i2c_client device for the dsi driver to use. Is my understanding correct?

Correct. That's kind of similar to the way an HDMI encoder driver would use an I2C adapter to query EDID. The i2c_client device would be a means for the DSI driver to access the control interface.

Okay.

Although, I'm not sure about the HDMI encoder example. An HDMI encoder would read off edid directly from the adapter(with an address specified), it wouldn't need to create an i2c client device. Therefore, an HDMI encoder wouldn't need to have a separate node for i2c in DT.

Quoted 6 lines

Quoted 5 lines

We can do without dummy dsi devices with this method. But, representing a device with 2 DT nodes seems a bit off. We'd also need to compatible strings for the same device, one for the i2c part, and the other for the dsi part.

Quoted 6 lines

I agree that this somewhat stretches the capabilities of device tree. Another alternative I guess would be to not have a compatible string for the I2C device at all (that's technically not valid, I guess) because we really don't need an I2C driver for the device. What we really need is a DSI driver with a means to talk over some I2C bus with some other part of its device.

I think what the driver should 'really' be is a bit subjective, and can vary based on what the buses are used for in the device. For the Toshiba chip that Jani mentioned, it tends more towards a DSI driver. Whereas, for an ADV75xx chip, it's closer to an I2C driver since only I2C can be used to configure the chip registers.

Although, I agree with the point you made about the DSI bus here:

"and the DSI interface may even be crippled, but the device is still addressable from the DSI host controller, if for nothing else than for routing the video stream."

The fact that the data on the DSI bus contains routing information (i.e, virtual channel number) always gives it some 'control' aspect.

Quoted 9 lines

Quoted 6 lines

From an adv75xx driver perspective, it should also support the ADV7511
chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a DSI DT node. It would be a bit inconsistent to have the bindings require both DSI and I2C nodes for one chip, and only I2C node for the other, with both chips being supported by the same driver.

Why would that be inconsistent? That sounds like the most accurate representation of the hardware to me.

Inconsistent wasn't the right term. I should have used 'uncommon' :) It's common for two chips of the same family to have a different set optional properties in DT, but it's not common for two chips of the same family to be represented by a different number of devices in DT.

Quoted 3 lines

I don't have an issue with the fused approach you suggested, as long as people are okay with the DT parts. Especially the part of needing 2 compatible strings in the DT.

I implemented the ADV7533 driver with the approach you suggested above (2 drivers for 2 different components of the chip). I posted it out just a while back (with you in loop).

The DT node with this apporach would look like this:

https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162

The main irritant with the '2 driver' approach is that we need to share the per-device driver data with them. For ADV7533, I've made the i2c driver allocate driver data (struct adv7511).

The dsi driver gets the driver data in the following way:

- The i2c driver sets the driver data as its client data using i2c_set_clientdata()
- Parse the i2c-control phandle to get the corresponding i2c client.
- Extract the adv7511 struct by getting i2c_get_clientdata()

This way of getting the same driver data is a bit strange, but it works. For this, we do need to ensure that the dsi driver defers as long as the i2c driver isn't probed.

Quoted 7 lines

I've now implemented both approaches for the driver. The first using a dummy dsi device, and this one using 2 drivers (with both being represented in DT). The advantage of the latter is that we don't need to create any dummy device stuff, the disadvantage is that DT is a bit uncommon.

Can we now come to a conclusion on what approach is better?

DSI by design is data bus which can be used additionally as a control bus, but in this particular case it is purely data bus. So of-graph bindings seem to be better choice. As already Lucas Stach said DT hierarchy should describe control buses and of-graph bindings should describe data bus. Argument that hw has two interfaces does not seem to be valid here - it has only one control interface. The other one is only for data, representing every data interface using DT hierarchy would lead to inflation of pseudo devices.

On the other side I do not see dummy device approach ideal solution, I guess lightweight framework providing DSI hosts detached from Linux device model could work better here.
The only problem here is that it should coexist somehow with dsi bus used to control devices. Anyway implementing it shouldn't be hard, question is if it would be eventually accepted :) I guess we can live for now with dummy devs.

Quoted 5 lines

Quoted 2 lines

Summarizing I would prefer version with dummy devices, as it seems more compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev patches after waiting for some more comments.

Let's wait for someone from the device tree maintainers to comment instead of going around in circles.

Thierry

Simplify the mipi dsi device creation process. device_initialize and device_add don't need to be called separately when creating mipi_dsi_device's. Use device_register instead to simplify things.

Create a helper function mipi_dsi_device_new which takes in struct mipi_dsi_device_info and mipi_dsi_host. It clubs the functions mipi_dsi_device_alloc and mipi_dsi_device_add into one.

mipi_dsi_device_info acts as a template to populate the dsi device information. This is populated by of_mipi_dsi_device_add and passed to mipi_dsi_device_new.

Later on, we'll provide mipi_dsi_device_new as a standalone way to create a dsi device not available via DT.

The new device creation process tries to closely follow what's been done in i2c_new_device in i2c-core.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 61 +++++++++++++++++-------------------------
 include/drm/drm_mipi_dsi.h     | 15 +++++++++++
 2 files changed, 40 insertions(+), 36 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +10 -1

index 2d5ca8ee..245ecfe 100644
@@ -102,9 +102,18 @@ static const struct device_type mipi_dsi_device_type = {
 	.release = mipi_dsi_dev_release,
 };
 
-static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
+struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
+					    struct mipi_dsi_device_info *info)
 {
 	struct mipi_dsi_device *dsi;
+	struct device *dev = host->dev;
+	int r;
 
+
+	if (info->reg > 3) {
+		dev_err(dev, "device node has invalid reg property: %u\n",
+			info->reg);
+		return ERR_PTR(-EINVAL);
+	}
 
 	dsi = kzalloc(sizeof(*dsi), GFP_KERNEL);

drivers/gpu/drm/drm_mipi_dsi.c +15 -35

 	if (!dsi)

@@ -114,26 +123,27 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
 	dsi->dev.bus = &mipi_dsi_bus_type;
 	dsi->dev.parent = host->dev;
 	dsi->dev.type = &mipi_dsi_device_type;
+	dsi->dev.of_node = info->node;
+	dsi->channel = info->reg;
 
-	device_initialize(&dsi->dev);
-
-	return dsi;
-}
-
-static int mipi_dsi_device_add(struct mipi_dsi_device *dsi)
-{
-	struct mipi_dsi_host *host = dsi->host;
+	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
 
-	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev),  dsi->channel);
+	r = device_register(&dsi->dev);
+	if (r) {
+		dev_err(dev, "failed to register device: %d\n", r);
+		kfree(dsi);
+		return ERR_PTR(r);
+	}
 
-	return device_add(&dsi->dev);
+	return dsi;
 }
+EXPORT_SYMBOL(mipi_dsi_device_new);
 
 static struct mipi_dsi_device *
 of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 {
-	struct mipi_dsi_device *dsi;
 	struct device *dev = host->dev;
+	struct mipi_dsi_device_info info = { };
 	int ret;
 	u32 reg;
 

@@ -144,31 +154,10 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (reg > 3) {
-		dev_err(dev, "device node %s has invalid reg property: %u\n",
-			node->full_name, reg);
-		return ERR_PTR(-EINVAL);
-	}
-
-	dsi = mipi_dsi_device_alloc(host);
-	if (IS_ERR(dsi)) {
-		dev_err(dev, "failed to allocate DSI device %s: %ld\n",
-			node->full_name, PTR_ERR(dsi));
-		return dsi;
-	}
-
-	dsi->dev.of_node = of_node_get(node);
-	dsi->channel = reg;
+	info.reg = reg;
+	info.node = of_node_get(node);
 
-	ret = mipi_dsi_device_add(dsi);
-	if (ret) {
-		dev_err(dev, "failed to add DSI device %s: %d\n",
-			node->full_name, ret);
-		kfree(dsi);
-		return ERR_PTR(ret);
-	}
-
-	return dsi;
+	return mipi_dsi_device_new(host, &info);
 }
 
 int mipi_dsi_host_register(struct mipi_dsi_host *host)

include/drm/drm_mipi_dsi.h +13 -0

index f1d8d0d..90f4f3c 100644
@@ -140,6 +140,19 @@ enum mipi_dsi_pixel_format {
 };
 
 /**
+ * struct mipi_dsi_device_info - template for creating a mipi_dsi_device
+ * @reg: DSI virtual channel assigned to peripheral
+ * @node: pointer to OF device node
+ *
+ * This is populated and passed to mipi_dsi_device_new to create a new
+ * DSI device
+ */
+struct mipi_dsi_device_info {
+	u32 reg;
+	struct device_node *node;
+};
+
+/**
  * struct mipi_dsi_device - DSI peripheral device
  * @host: DSI host for this peripheral
  * @dev: driver model device node for this peripheral

Add a device name field in mipi_dsi_device. This name is different from the actual dev name (which is of the format "hostname.reg"). When the device is created via DT, this name is set to the modalias string. In the non-DT case, the driver creating the DSI device provides the name by populating a filed in mipi_dsi_device_info.

Matching for DT case would be as it was before. For the non-DT case, we compare the device and driver names. Matching by comparing driver and device names isn't the best thing to do. I'd appreciate some suggestions here.

Other buses (like i2c/spi) perform a non-DT match by comparing the device name and entries in the driver's id_table. The id_table structs for different buses are defined in "include/linux/mod_devicetable.h", I didn't want to touch that for now.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 29 ++++++++++++++++++++++++++++-
 include/drm/drm_mipi_dsi.h     |  8 ++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

drivers/gpu/drm/drm_mipi_dsi.c +28 -1

index 245ecfe..46ee515 100644
@@ -45,9 +45,30 @@
  * subset of the MIPI DCS command set.
  */
 
+static const struct device_type mipi_dsi_device_type;
+
 static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
 {
-	return of_driver_match_device(dev, drv);
+	struct mipi_dsi_device *dsi;
+
+	if (dev->type == &mipi_dsi_device_type)
+		dsi = to_mipi_dsi_device(dev);
+	else
+		return 0;
+
+	/* Attempt OF style match */
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	/*
+	 * Try to compare dsi device and driver names. If this matching approach
+	 * isn't strong, we'd probably want the dsi drivers to populate the
+	 * id_table field and use that instead
+	 */
+	if (!strcmp(dsi->name, drv->name))
+		return 1;
+
+	return 0;
 }
 
 static const struct dev_pm_ops mipi_dsi_device_pm_ops = {

@@ -125,6 +146,7 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
 	dsi->dev.type = &mipi_dsi_device_type;
 	dsi->dev.of_node = info->node;
 	dsi->channel = info->reg;
+	strlcpy(dsi->name, info->name, sizeof(dsi->name));
 
 	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
 

@@ -147,6 +169,11 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 	int ret;
 	u32 reg;
 
+	if (of_modalias_node(node, info.name, sizeof(info.name)) < 0) {
+		dev_err(dev, "modalias failure on %s\n", node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
+
 	ret = of_property_read_u32(node, "reg", &reg);
 	if (ret) {
 		dev_err(dev, "device node %s has no valid reg property: %d\n",

include/drm/drm_mipi_dsi.h +3 -0

index 90f4f3c..93dec7b 100644
@@ -139,8 +139,11 @@ enum mipi_dsi_pixel_format {
 	MIPI_DSI_FMT_RGB565,
 };
 
+#define DSI_DEV_NAME_SIZE		20
+
 /**
  * struct mipi_dsi_device_info - template for creating a mipi_dsi_device
+ * @name: name of the dsi peripheral
  * @reg: DSI virtual channel assigned to peripheral
  * @node: pointer to OF device node
  *

include/drm/drm_mipi_dsi.h +1 -0

 /**
  * struct mipi_dsi_device - DSI peripheral device
  * @host: DSI host for this peripheral
  * @dev: driver model device node for this peripheral
+ * @name: name of the dsi peripheral
  * @channel: virtual channel assigned to the peripheral
  * @format: pixel format for video mode
  * @lanes: number of active data lanes

Quoted 11 lines · Archit Taneja

Add a device name field in mipi_dsi_device. This name is different from the actual dev name (which is of the format "hostname.reg"). When the device is created via DT, this name is set to the modalias string. In the non-DT case, the driver creating the DSI device provides the name by populating a filed in mipi_dsi_device_info.

Matching for DT case would be as it was before. For the non-DT case, we compare the device and driver names. Matching by comparing driver and device names isn't the best thing to do. I'd appreciate some suggestions here.

Quoted 6 lines

Other buses (like i2c/spi) perform a non-DT match by comparing the device name and entries in the driver's id_table. The id_table structs for different buses are defined in "include/linux/mod_devicetable.h", I didn't want to touch that for now.

Signed-off-by Archit Taneja <[email protected]>

Beside small comments below.

Reviewed-by Andrzej Hajda <[email protected]>

Quoted 69 lines

---
  drivers/gpu/drm/drm_mipi_dsi.c | 29 ++++++++++++++++++++++++++++-
  include/drm/drm_mipi_dsi.h     |  8 ++++++++
  2 files changed, 36 insertions(+), 1 deletion(-)

drivers/gpu/drm/drm_mipi_dsi.c +28 -1

index 245ecfe..46ee515 100644
@@ -45,9 +45,30 @@
   * subset of the MIPI DCS command set.
   */
 
+static const struct device_type mipi_dsi_device_type;
+
  static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
  {
-	return of_driver_match_device(dev, drv);
+	struct mipi_dsi_device *dsi;
+
+	if (dev->type == &mipi_dsi_device_type)
+		dsi = to_mipi_dsi_device(dev);
+	else
+		return 0;
+
+	/* Attempt OF style match */
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	/*
+	 * Try to compare dsi device and driver names. If this matching approach
+	 * isn't strong, we'd probably want the dsi drivers to populate the
+	 * id_table field and use that instead
+	 */
+	if (!strcmp(dsi->name, drv->name))
+		return 1;
+
+	return 0;
  }
 
  static const struct dev_pm_ops mipi_dsi_device_pm_ops = {

@@ -125,6 +146,7 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
  	dsi->dev.type = &mipi_dsi_device_type;
  	dsi->dev.of_node = info->node;
  	dsi->channel = info->reg;
+	strlcpy(dsi->name, info->name, sizeof(dsi->name));
 
  	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
 

@@ -147,6 +169,11 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
  	int ret;
  	u32 reg;
 
+	if (of_modalias_node(node, info.name, sizeof(info.name)) < 0) {
+		dev_err(dev, "modalias failure on %s\n", node->full_name);
+		return ERR_PTR(-EINVAL);
+	}
+
  	ret = of_property_read_u32(node, "reg", &reg);
  	if (ret) {
  		dev_err(dev, "device node %s has no valid reg property: %d\n",

include/drm/drm_mipi_dsi.h +0 -0

index 90f4f3c..93dec7b 100644
@@ -139,8 +139,11 @@ enum mipi_dsi_pixel_format {
  	MIPI_DSI_FMT_RGB565,
  };
 

include/drm/drm_mipi_dsi.h enum mipi_dsi_pixel_format { · line 154

Quoted 13 lines

include/drm/drm_mipi_dsi.h +4 -0

+#define DSI_DEV_NAME_SIZE		20
+
  /**
   * struct mipi_dsi_device_info - template for creating a mipi_dsi_device
+ * @name: name of the dsi peripheral
   * @reg: DSI virtual channel assigned to peripheral
   * @node: pointer to OF device node
   *

@@ -148,14 +151,17 @@ enum mipi_dsi_pixel_format {
   * DSI device
   */
  struct mipi_dsi_device_info {
+	char name[DSI_DEV_NAME_SIZE];

I wonder if it shouldn't be called type as in case of i2c, but no strong feelings.

include/drm/drm_mipi_dsi.h enum mipi_dsi_pixel_format { · line 159

Quoted 5 lines

include/drm/drm_mipi_dsi.h +1 -0

  	u32 reg;
  	struct device_node *node;
  };
 
+

Empty line.

Quoted 12 lines

include/drm/drm_mipi_dsi.h +1 -0

  /**
   * struct mipi_dsi_device - DSI peripheral device
   * @host: DSI host for this peripheral
   * @dev: driver model device node for this peripheral
+ * @name: name of the dsi peripheral
   * @channel: virtual channel assigned to the peripheral
   * @format: pixel format for video mode
   * @lanes: number of active data lanes

@@ -165,6 +171,8 @@ struct mipi_dsi_device {
  	struct mipi_dsi_host *host;
  	struct device dev;
 

include/drm/drm_mipi_dsi.h struct mipi_dsi_device { · line 175

Quoted 2 lines

include/drm/drm_mipi_dsi.h +2 -0

+	char name[DSI_DEV_NAME_SIZE];
+

This empty line can be also removed.

Regards
Andrzej

include/drm/drm_mipi_dsi.h struct mipi_dsi_device { · line 178

Quoted 3 lines

include/drm/drm_mipi_dsi.h +0 -0

  	unsigned int channel;
  	unsigned int lanes;
  	enum mipi_dsi_pixel_format format;

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

We don't check whether a previously registered mipi_dsi_device under the same host shares the same virtual channel.

Before registering, check if any of the registered devices doesn't already have the same virtual channel.

This wasn't crucial when all the devices under a host were populated via DT. Now that we also support creating devices manually, we could end up in a situation where a driver tries to create a device with a virtual channel already taken by a device populated in DT.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index 46ee515..db6130a 100644

drivers/gpu/drm/drm_mipi_dsi.c +10 -0

+		return -EBUSY;
+
+	return 0;
+}
+
+static int mipi_dsi_check_chan_busy(struct mipi_dsi_host *host, u32 reg)
+{
+	return device_for_each_child(host->dev, &reg, __dsi_check_chan_busy);
+}
+
 struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
 					    struct mipi_dsi_device_info *info)
 {

drivers/gpu/drm/drm_mipi_dsi.c +7 -2

+	if (r)
+		goto err;
+
 	r = device_register(&dsi->dev);
 	if (r) {
 		dev_err(dev, "failed to register device: %d\n", r);
-		kfree(dsi);
-		return ERR_PTR(r);
+		goto err;
 	}
 
 	return dsi;
+err:
+	kfree(dsi);
+	return ERR_PTR(r);
 }
 EXPORT_SYMBOL(mipi_dsi_device_new);
 

Quoted 6 lines · Archit Taneja

We don't check whether a previously registered mipi_dsi_device under the same host shares the same virtual channel.

Before registering, check if any of the registered devices doesn't already have the same virtual channel.

Quoted 6 lines

This wasn't crucial when all the devices under a host were populated via DT. Now that we also support creating devices manually, we could end up in a situation where a driver tries to create a device with a virtual channel already taken by a device populated in DT.

Signed-off-by Archit Taneja <[email protected]>

Beside few non-blcking nitpicks.

Reviewed-by Andrzej Hajda <[email protected]>

Quoted 9 lines

---
  drivers/gpu/drm/drm_mipi_dsi.c | 26 ++++++++++++++++++++++++--
  1 file changed, 24 insertions(+), 2 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index 46ee515..db6130a 100644

drivers/gpu/drm/drm_mipi_dsi.c static const struct device_type mipi_dsi_device_type = { · line 131

Quoted 9 lines

drivers/gpu/drm/drm_mipi_dsi.c +6 -0

@@ -123,6 +123,22 @@ static const struct device_type mipi_dsi_device_type = {
  	.release = mipi_dsi_dev_release,
  };
 
+static int __dsi_check_chan_busy(struct device *dev, void *data)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
+	u32 reg = *(u32 *) data;
+
+	if (dsi && dsi->channel == reg)

Maybe simpler would be:

     u32 *reg = data;

     if (dsi && dsi->channel == *reg)

Quoted 17 lines

drivers/gpu/drm/drm_mipi_dsi.c +10 -0

+		return -EBUSY;
+
+	return 0;
+}
+
+static int mipi_dsi_check_chan_busy(struct mipi_dsi_host *host, u32 reg)
+{
+	return device_for_each_child(host->dev, &reg, __dsi_check_chan_busy);
+}
+
  struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
  					    struct mipi_dsi_device_info *info)
  {

@@ -150,14 +166,20 @@ struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
 

drivers/gpu/drm/drm_mipi_dsi.c struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, · line 169

Quoted 3 lines

drivers/gpu/drm/drm_mipi_dsi.c +1 -0

  	dev_set_name(&dsi->dev, "%s.%d", dev_name(host->dev), info->reg);
 
+	r = mipi_dsi_check_chan_busy(host, info->reg);

I know this r variable was introduced in the 1st patch, but if you will send next iteration
consider replacing it with ret, looks more readable and follows file convention :)

Regards
Andrzej

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c +3 -2

+	if (r)
+		goto err;
+
  	r = device_register(&dsi->dev);
  	if (r) {
  		dev_err(dev, "failed to register device: %d\n", r);
-		kfree(dsi);
-		return ERR_PTR(r);

drivers/gpu/drm/drm_mipi_dsi.c struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host, · line 185

Quoted 10 lines

drivers/gpu/drm/drm_mipi_dsi.c +4 -0

+		goto err;
  	}
 
  	return dsi;
+err:
+	kfree(dsi);
+	return ERR_PTR(r);
  }
  EXPORT_SYMBOL(mipi_dsi_device_new);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

A driver calling mipi_dsi_device_new might want to unregister the device once it's done. It might also require it in an error handling path in case something didn't go right.

When the dsi host driver calls mipi_dsi_host_unregister, the devices created by both DT and and without DT will be removed. This does leave the possibility of the host removing the dsi device without the peripheral driver being aware of it. I don't know a good way to solve this. Some suggestions here would be of help too.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 7 +++++++
 include/drm/drm_mipi_dsi.h     | 2 ++
 2 files changed, 9 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index db6130a..cbb7373 100644

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

 static struct mipi_dsi_device *
 of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 {

include/drm/drm_mipi_dsi.h +0 -0

index 93dec7b..68f49f4 100644

Quoted 4 lines · Archit Taneja

A driver calling mipi_dsi_device_new might want to unregister the device once it's done. It might also require it in an error handling path in case something didn't go right.

Quoted 5 lines

When the dsi host driver calls mipi_dsi_host_unregister, the devices created by both DT and and without DT will be removed. This does leave the possibility of the host removing the dsi device without the peripheral driver being aware of it. I don't know a good way to solve this. Some suggestions here would be of help too.

The 2nd paragraph is not relevant here. It is another issue. Some comments about it:

I am not sure, but I guess device should not be removed if it is refcounted properly, it will be just detached from the driver, bus and system (whatever it means:) ).
It does not mean it will be usable and probably some races can occur anyway. I guess i2c and other buses have the same problem, am I right?

Quoted 16 lines

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/gpu/drm/drm_mipi_dsi.c | 7 +++++++
  include/drm/drm_mipi_dsi.h     | 2 ++
  2 files changed, 9 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index db6130a..cbb7373 100644
@@ -183,6 +183,13 @@ err:
  }
  EXPORT_SYMBOL(mipi_dsi_device_new);
 

drivers/gpu/drm/drm_mipi_dsi.c err: · line 192

Quoted 7 lines

drivers/gpu/drm/drm_mipi_dsi.c +7 -0

+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
+{
+	if (dsi)
+		device_unregister(&dsi->dev);
+}
+EXPORT_SYMBOL(mipi_dsi_device_unregister);
+

I guess NULL check can be removed and the whole function can be inlined.

Regards
Andrzej

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

  static struct mipi_dsi_device *
  of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
  {

include/drm/drm_mipi_dsi.h +0 -0

index 93dec7b..68f49f4 100644

include/drm/drm_mipi_dsi.h ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, · line 204

Quoted 8 lines

include/drm/drm_mipi_dsi.h +2 -0

@@ -197,6 +197,8 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 
  struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
  					    struct mipi_dsi_device_info *info);
+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
+
  /**
   * enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
   * @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Andrzej Hajda

Quoted 4 lines · Archit Taneja

A driver calling mipi_dsi_device_new might want to unregister the device once it's done. It might also require it in an error handling path in case something didn't go right.

Quoted 7 lines

Quoted 5 lines

When the dsi host driver calls mipi_dsi_host_unregister, the devices created by both DT and and without DT will be removed. This does leave the possibility of the host removing the dsi device without the peripheral driver being aware of it. I don't know a good way to solve this. Some suggestions here would be of help too.

The 2nd paragraph is not relevant here. It is another issue. Some comments about it:

Yes, it's probably not the best to put it in the commit message of this patch.

Quoted 5 lines

I am not sure, but I guess device should not be removed if it is refcounted properly, it will be just detached from the driver, bus and system (whatever it means:) ).
It does not mean it will be usable and probably some races can occur anyway. I guess i2c and other buses have the same problem, am I right?

I was concerned about one particular sequence:

1) DSI host driver calls mipi_dsi_host_unregister: All dsi devices would be unregistered.

2) dsi device driver calls mipi_dsi_device_unregister: This will try to unregister our dsi device

The problem here is that the device will cease to exist after step (1) itself, because the refcount of our device will never be 2.

mipi_dsi_host_register() will only register devices represented in DT, not the one the drivers register manually.

In other words, the dsi pointer in our driver will point to nothing valid after mipi_dsi_host_unregister is called.

As you said, I guess this exists in other buses too, and it's the drivers job to not use them.

Quoted 15 lines

Quoted 15 lines

Signed-off-by Archit Taneja <[email protected]>

---
  drivers/gpu/drm/drm_mipi_dsi.c | 7 +++++++
  include/drm/drm_mipi_dsi.h     | 2 ++
  2 files changed, 9 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index db6130a..cbb7373 100644
@@ -183,6 +183,13 @@ err:
  }
  EXPORT_SYMBOL(mipi_dsi_device_new);
 

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c err: · line 192

Quoted 7 lines

drivers/gpu/drm/drm_mipi_dsi.c +7 -0

+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
+{
+	if (dsi)
+		device_unregister(&dsi->dev);
+}
+EXPORT_SYMBOL(mipi_dsi_device_unregister);
+

I guess NULL check can be removed and the whole function can be inlined.

Yeah, this check won't help anyway.

I think I'll mention that drivers should use this only in error handling paths, and not in the driver's remove() op.

I'll also change this to inlined.

Archit

Quoted 23 lines

Regards
Andrzej

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

  static struct mipi_dsi_device *
  of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
  {

include/drm/drm_mipi_dsi.h +0 -0

index 93dec7b..68f49f4 100644

include/drm/drm_mipi_dsi.h ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, · line 204

Quoted 8 lines

include/drm/drm_mipi_dsi.h +2 -0

@@ -197,6 +197,8 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 
  struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
  					    struct mipi_dsi_device_info *info);
+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
+
  /**
   * enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
   * @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 33 lines · Archit Taneja

Quoted 5 lines · Andrzej Hajda

Quoted 4 lines · Archit Taneja

A driver calling mipi_dsi_device_new might want to unregister the device once it's done. It might also require it in an error handling path in case something didn't go right.

Quoted 7 lines

Quoted 5 lines

When the dsi host driver calls mipi_dsi_host_unregister, the devices created by both DT and and without DT will be removed. This does leave the possibility of the host removing the dsi device without the peripheral driver being aware of it. I don't know a good way to solve this. Some suggestions here would be of help too.

The 2nd paragraph is not relevant here. It is another issue. Some comments about it:

Yes, it's probably not the best to put it in the commit message of this patch.

Quoted 5 lines

I am not sure, but I guess device should not be removed if it is refcounted properly, it will be just detached from the driver, bus and system (whatever it means:) ).
It does not mean it will be usable and probably some races can occur anyway. I guess i2c and other buses have the same problem, am I right?

I was concerned about one particular sequence:

1) DSI host driver calls mipi_dsi_host_unregister: All dsi devices would be unregistered.

2) dsi device driver calls mipi_dsi_device_unregister: This will try to unregister our dsi device

The problem here is that the device will cease to exist after step (1) itself, because the refcount of our device will never be 2.

Quoted 8 lines

mipi_dsi_host_register() will only register devices represented in DT, not the one the drivers register manually.

In other words, the dsi pointer in our driver will point to nothing valid after mipi_dsi_host_unregister is called.

As you said, I guess this exists in other buses too, and it's the drivers job to not use them.

I think the whole problem is due to fact we try to use devices
as interfaces to some bus hosts (DSI in our case), these devices are owned by bus host and we cannot control their lifetime from other code. The best solution IMO would be to create separate lightweight framework as I suggested in previous discussion[1]. It should be cleaner solution without any 'dummy' proxy devices.
But even in this case we would need some callbacks to notify that the provider is about to be removed.

2nd 'solution' is to leave it as is and pretend everything is OK, as in case of other frameworks :)

Maybe it would be possible 3rd solution - we could use probe and remove callbacks from dsi driver to notify clients about adding/removal of dsi device to/from bus. So during dummy dsi dev creation we would provide some callbacks which would be called
by dummy dsi driver probe/remove to notifiy client it can start/stop using dsi device.
This crazy construction probably can work but looks insane :)

[1]: http://www.spinics.net/lists/linux-arm-msm/msg16945.html

Regards
Andrzej

Quoted 23 lines

Quoted 15 lines

Quoted 15 lines

Signed-off-by Archit Taneja <[email protected]>

---
   drivers/gpu/drm/drm_mipi_dsi.c | 7 +++++++
   include/drm/drm_mipi_dsi.h     | 2 ++
   2 files changed, 9 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

index db6130a..cbb7373 100644
@@ -183,6 +183,13 @@ err:
   }
   EXPORT_SYMBOL(mipi_dsi_device_new);
 

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c err: · line 192

Quoted 7 lines

drivers/gpu/drm/drm_mipi_dsi.c +7 -0

+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
+{
+	if (dsi)
+		device_unregister(&dsi->dev);
+}
+EXPORT_SYMBOL(mipi_dsi_device_unregister);
+

I guess NULL check can be removed and the whole function can be inlined.

Quoted 32 lines

Yeah, this check won't help anyway.

I think I'll mention that drivers should use this only in error handling paths, and not in the driver's remove() op.

I'll also change this to inlined.

Archit

Quoted 23 lines

Regards
Andrzej

Quoted 8 lines

drivers/gpu/drm/drm_mipi_dsi.c +0 -0

   static struct mipi_dsi_device *
   of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
   {

include/drm/drm_mipi_dsi.h +0 -0

index 93dec7b..68f49f4 100644

include/drm/drm_mipi_dsi.h ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params, · line 204

Quoted 8 lines

include/drm/drm_mipi_dsi.h +2 -0

@@ -197,6 +197,8 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 
   struct mipi_dsi_device *mipi_dsi_device_new(struct mipi_dsi_host *host,
   					    struct mipi_dsi_device_info *info);
+void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi);
+
   /**
    * enum mipi_dsi_dcs_tear_mode - Tearing Effect Output Line mode
    * @MIPI_DSI_DCS_TEAR_MODE_VBLANK: the TE output line consists of V-Blanking

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

mipi_dsi_devices are inherently aware of their host because they share a parent-child hierarchy in the device tree.

Non-dsi drivers that create a dummy dsi device don't have this data. In order to get this information, they require to a phandle to the dsi host in the device tree.

Maintain a list of all the hosts DSI that are currently registered.

This list will be used to find the mipi_dsi_host corresponding to the device_node passed in of_find_mipi_dsi_host_by_node.

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  2 ++
 2 files changed, 32 insertions(+)

drivers/gpu/drm/drm_mipi_dsi.c +30 -0

index cbb7373..c51d73e 100644
@@ -216,6 +216,28 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node)
 	return mipi_dsi_device_new(host, &info);
 }
 
+static DEFINE_MUTEX(host_lock);
+static LIST_HEAD(host_list);
+
+struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node)
+{
+	struct mipi_dsi_host *host;
+
+	mutex_lock(&host_lock);
+
+	list_for_each_entry(host, &host_list, list) {
+		if (host->dev->of_node == node) {
+			mutex_unlock(&host_lock);
+			return host;
+		}
+	}
+
+	mutex_unlock(&host_lock);
+
+	return NULL;
+}
+EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
+
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;

@@ -227,6 +249,10 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 		of_mipi_dsi_device_add(host, node);
 	}
 
+	mutex_lock(&host_lock);
+	list_add_tail(&host->list, &host_list);
+	mutex_unlock(&host_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL(mipi_dsi_host_register);

@@ -243,6 +269,10 @@ static int mipi_dsi_remove_device_fn(struct device *dev, void *priv)
 void mipi_dsi_host_unregister(struct mipi_dsi_host *host)
 {
 	device_for_each_child(host->dev, NULL, mipi_dsi_remove_device_fn);
+
+	mutex_lock(&host_lock);
+	list_del_init(&host->list);
+	mutex_unlock(&host_lock);
 }
 EXPORT_SYMBOL(mipi_dsi_host_unregister);
 

include/drm/drm_mipi_dsi.h +0 -0

index 68f49f4..15d3068 100644
@@ -100,10 +100,12 @@ struct mipi_dsi_host_ops {
 struct mipi_dsi_host {
 	struct device *dev;