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(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
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(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 2d5ca8ee..9bfe215 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -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;
+
+ 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;
+}
+
+static int dummy_remove(struct mipi_dsi_device *dsi)
+{
+ return 0;
+}
+
+static void dummy_shutdown(struct mipi_dsi_device *dsi)
+{
+}
+
+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);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index f1d8d0d..d06ba99 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -174,6 +174,8 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
size_t num_params, void *data, size_t size);
+struct mipi_dsi_device *mipi_dsi_new_dummy(struct mipi_dsi_host *host, u32 reg);
+
/**
* 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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
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(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 9bfe215..81ddb73 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -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);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index d06ba99..1684a0e 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -100,10 +100,12 @@ struct mipi_dsi_host_ops {
struct mipi_dsi_host {
struct device *dev;
const struct mipi_dsi_host_ops *ops;
+ struct list_head list;
};
int mipi_dsi_host_register(struct mipi_dsi_host *host);
void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
+struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
/* DSI mode flags */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
Hi,
On 06/30/2015 10:54 AM, Archit Taneja wrote:
> 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!
Any comments on this?
Archit
>
> 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(-)
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 06/30/2015 07:24 AM, Archit Taneja wrote:
> 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(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 2d5ca8ee..9bfe215 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -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?
> +
> + 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;
> +}
> +
> +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.
> +
> +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);
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index f1d8d0d..d06ba99 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -174,6 +174,8 @@ ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
> ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
> size_t num_params, void *data, size_t size);
>
> +struct mipi_dsi_device *mipi_dsi_new_dummy(struct mipi_dsi_host *host, u32 reg);
> +
> /**
> * 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
On 06/30/2015 07:24 AM, Archit Taneja wrote:
> 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.
The lock protects only the list, there is no guarantee that mipi_dsi_host
returned by of_find_mipi_dsi_host_by_node is still valid, or will be valid long
enough.
But this issue affects many kernel frameworks so I am not sure if it should
block this
particular patch.
Reviewed-by: Andrzej Hajda <[email protected]>
Regards
Andrzej
>
> 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(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 9bfe215..81ddb73 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -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);
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index d06ba99..1684a0e 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -100,10 +100,12 @@ struct mipi_dsi_host_ops {
> struct mipi_dsi_host {
> struct device *dev;
> const struct mipi_dsi_host_ops *ops;
> + struct list_head list;
> };
>
> int mipi_dsi_host_register(struct mipi_dsi_host *host);
> void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
> +struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node);
>
> /* DSI mode flags */
>
Hi,
On 08/19/2015 01:40 PM, Andrzej Hajda wrote:
> On 06/30/2015 07:24 AM, Archit Taneja wrote:
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 2d5ca8ee..9bfe215 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -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?
I took i2c's dummy dev creation as reference. The i2c_driver struct has
an id_table param, that allows the match function (i2c_device_match) to
not have a special case to check for a dummy device.
We could a 'id_table' entry in mipi_dsi_driver, and a 'name' entry in
mipi_dsi_device. But that would be a bit of an overkill just to support
dummy devices.
I could make the check more thorough by adding a func which does
something similar to 'i2c_verify_client', but I think we would
still need the above string.
I will change "dummy_dev" to ".dummy_dev.". I grepped the kernel for
devices named "dummy_dev", but didn't find anything as such, so it
shouldn't really break anything.
>
>> +
>> + 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;
>> +}
>> +
>> +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.
Right. I will remove these.
Thanks for the review.
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Wed, Aug 19, 2015 at 10:37:54AM +0530, Archit Taneja wrote:
> Hi,
>
> On 06/30/2015 10:54 AM, Archit Taneja wrote:
> >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!
>
> 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.
Thierry
Hi Thierry, Archit,
Am Mittwoch, den 19.08.2015, 15:13 +0200 schrieb Thierry Reding:
> On Wed, Aug 19, 2015 at 10:37:54AM +0530, Archit Taneja wrote:
> > Hi,
> >
> > On 06/30/2015 10:54 AM, Archit Taneja wrote:
> > >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!
> >
> > 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.
>
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.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> Hi Thierry, Archit,
>
> Am Mittwoch, den 19.08.2015, 15:13 +0200 schrieb Thierry Reding:
> > On Wed, Aug 19, 2015 at 10:37:54AM +0530, Archit Taneja wrote:
> > > Hi,
> > >
> > > On 06/30/2015 10:54 AM, Archit Taneja wrote:
> > > >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!
> > >
> > > 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.
> >
> 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.
> 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.
> 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.
Thierry
Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> > Hi Thierry, Archit,
> >
[...]
> > > 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.
> >
> > 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.
>
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?
> > 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.
> > 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.
>
Just disregard this comment then. Lets concentrate on the points above.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
> > On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> > > Hi Thierry, Archit,
> > >
> [...]
> > > > 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.
> > >
> > > 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.
> >
> 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.
> > > 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.
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.
Thierry
On Wed, 19 Aug 2015, Thierry Reding <[email protected]> wrote:
> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
>> > 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.
As a drive-by comment, the Toshiba TC358764XBG/65XBG chip can be
configured via both DSI and I2C. The I2C interface requires DSI clock on
the DSI interface to operate. You may not use the interfaces
simultaneously, but provided you protect against races in register
access, both interfaces can be operational at the same time.
Sorry I couldn't find a public spec of the chip to share.
HTH,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
Hi Thierry, Lucas,
On 08/19/2015 08:32 PM, Thierry Reding wrote:
> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
>>> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
>>>> Hi Thierry, Archit,
>>>>
>> [...]
>>>>> 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.
>>>>
>>>> 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.
>>>
>> 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.
>
>>>> 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.
>
> 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
> 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.
>
> 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.
Thanks,
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:
> Hi Thierry, Lucas,
>
>
> On 08/19/2015 08:32 PM, Thierry Reding wrote:
> >On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
> >>Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
> >>>On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
> >>>>Hi Thierry, Archit,
> >>>>
> >>[...]
> >>>>>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.
> >>>>
> >>>>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.
> >>>
> >>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.
> >
> >>>>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.
> >
> >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
>
> >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?
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.
> 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?
> >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?
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.
> 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.
> 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.
Thierry
On 08/20/2015 05:18 PM, Thierry Reding wrote:
> On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:
>> Hi Thierry, Lucas,
>>
>>
>> On 08/19/2015 08:32 PM, Thierry Reding wrote:
>>> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
>>>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
>>>>> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
>>>>>> Hi Thierry, Archit,
>>>>>>
>>>> [...]
>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>>>
>>>> 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.
>>>
>>>>>> 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.
>>>
>>> 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
>>
>>> 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?
>
> 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.
>
>> 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.
>
>>> 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?
>
> 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.
>
>> 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.
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.
>
>> 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.
Thanks,
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project