Re: [RFC v2 3/5] video: display: Add MIPI DBI bus support

2012-11-30 Thread Tomi Valkeinen
Hi,

On 2012-11-22 23:45, Laurent Pinchart wrote:
 From: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com
 
 Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
 ---
  drivers/video/display/Kconfig|4 +
  drivers/video/display/Makefile   |1 +
  drivers/video/display/mipi-dbi-bus.c |  228 
 ++
  include/video/display.h  |5 +
  include/video/mipi-dbi-bus.h |  125 +++
  5 files changed, 363 insertions(+), 0 deletions(-)
  create mode 100644 drivers/video/display/mipi-dbi-bus.c
  create mode 100644 include/video/mipi-dbi-bus.h

I've been doing some omapdss testing with CDF and DSI, and I have some
thoughts about the bus stuff. I already told these to Laurent, but I'll
write them to the mailing list also for discussion.

So with the current CDF model we have separate control and video buses.
The control bus is represented as proper Linux bus, and video bus is
represented via custom display_entity. This sounds good on paper, and I
also agreed to this approach when we were planning CDF.

However, now I doubt that approach.

First, I want to list some examples of devices with different bus
configurations:

1) Panel without any control, only video bus
2) Panel with separate control and video buses, e.g. i2c for control,
DPI for video
3) Panel with the same control and video buses, like DSI or DBI.

The first one is simple, it's just a platform device. No questions there.

The second one can be a bit tricky. Say, if we have a panel controlled
via i2c, and DSI/DBI used for video. The problem here is that with the
current model, DSI/DBI would be represented as a real bus, for control.
But in this case there's only the video path.

So if all the DSI/DBI bus configuration is handled on the DSI/DBI
control bus side, how can it be handled with only the video bus? And if
we add the same bus configuration to the video bus side as we have on
control bus side, then we have duplicated the API, and it's also
somewhat confusing. I don't have any good suggestion for this.

Third one is kinda clear, but I feel slightly uneasy about it. In theory
we can have separate control and video buses, which use the same HW
transport. However, I feel that we'll have some trouble with the
implementation, as we'll then have two more or less independent users
for the HW transport. I can't really point out why this would not be
possible to implement, but I have a gut feeling that it will be
difficult, at least for DSI.

So I think my question is: what does it give us to have separate control
and video buses, and what does the Linux bus give us with the control bus?

I don't see us ever having a case where a device would use one of the
display buses only for control. So either the display bus is only used
for video, or it's used for both control and video. And the display bus
is always 1-to-1, so we're talking about really simple bus here.

I believe things would be much simpler if we just have one entity for
the display buses, which support both video and (when available)
control. What would be the downsides of this approach versus the current
CDF proposal?

 Tomi




signature.asc
Description: OpenPGP digital signature


[RFC v2 3/5] video: display: Add MIPI DBI bus support

2012-11-22 Thread Laurent Pinchart
From: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com

Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
 drivers/video/display/Kconfig|4 +
 drivers/video/display/Makefile   |1 +
 drivers/video/display/mipi-dbi-bus.c |  228 ++
 include/video/display.h  |5 +
 include/video/mipi-dbi-bus.h |  125 +++
 5 files changed, 363 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/display/mipi-dbi-bus.c
 create mode 100644 include/video/mipi-dbi-bus.h

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 0f9b990..b04c8be 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig
@@ -5,6 +5,10 @@ menuconfig DISPLAY_CORE
 
 if DISPLAY_CORE
 
+config DISPLAY_MIPI_DBI
+   tristate
+   default n
+
 config DISPLAY_PANEL_DPI
tristate DPI (Parallel) Display Panels
---help---
diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile
index 47978d4..00ef1c2 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_DISPLAY_CORE) += display-core.o
+obj-$(CONFIG_DISPLAY_MIPI_DBI) += mipi-dbi-bus.o
 obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o
diff --git a/drivers/video/display/mipi-dbi-bus.c 
b/drivers/video/display/mipi-dbi-bus.c
new file mode 100644
index 000..bd39a97
--- /dev/null
+++ b/drivers/video/display/mipi-dbi-bus.c
@@ -0,0 +1,228 @@
+/*
+ * MIPI DBI Bus
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * Contacts: Laurent Pinchart laurent.pinch...@ideasonboard.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/device.h
+#include linux/export.h
+#include linux/kernel.h
+#include linux/list.h
+#include linux/module.h
+#include linux/mutex.h
+#include linux/pm.h
+#include linux/pm_runtime.h
+
+#include video/mipi-dbi-bus.h
+
+/* 
-
+ * Bus operations
+ */
+
+int mipi_dbi_set_data_width(struct mipi_dbi_device *dev, unsigned int width)
+{
+   if (width != 8  width != 16)
+   return -EINVAL;
+
+   dev-data_width = width;
+   return 0;
+}
+EXPORT_SYMBOL_GPL(mipi_dbi_set_data_width);
+
+int mipi_dbi_write_command(struct mipi_dbi_device *dev, u16 cmd)
+{
+   return dev-bus-ops-write_command(dev-bus, dev, cmd);
+}
+EXPORT_SYMBOL_GPL(mipi_dbi_write_command);
+
+int mipi_dbi_write_data(struct mipi_dbi_device *dev, const u8 *data,
+   size_t len)
+{
+   return dev-bus-ops-write_data(dev-bus, dev, data, len);
+}
+EXPORT_SYMBOL_GPL(mipi_dbi_write_data);
+
+int mipi_dbi_read_data(struct mipi_dbi_device *dev, u8 *data, size_t len)
+{
+   return dev-bus-ops-read_data(dev-bus, dev, data, len);
+}
+EXPORT_SYMBOL_GPL(mipi_dbi_read_data);
+
+/* 
-
+ * Bus type
+ */
+
+static const struct mipi_dbi_device_id *
+mipi_dbi_match_id(const struct mipi_dbi_device_id *id,
+ struct mipi_dbi_device *dev)
+{
+   while (id-name[0]) {
+   if (strcmp(dev-name, id-name) == 0) {
+   dev-id_entry = id;
+   return id;
+   }
+   id++;
+   }
+   return NULL;
+}
+
+static int mipi_dbi_match(struct device *_dev, struct device_driver *_drv)
+{
+   struct mipi_dbi_device *dev = to_mipi_dbi_device(_dev);
+   struct mipi_dbi_driver *drv = to_mipi_dbi_driver(_drv);
+
+   if (drv-id_table)
+   return mipi_dbi_match_id(drv-id_table, dev) != NULL;
+
+   return (strcmp(dev-name, _drv-name) == 0);
+}
+
+static ssize_t modalias_show(struct device *_dev, struct device_attribute *a,
+char *buf)
+{
+   struct mipi_dbi_device *dev = to_mipi_dbi_device(_dev);
+   int len = snprintf(buf, PAGE_SIZE, MIPI_DBI_MODULE_PREFIX %s\n,
+  dev-name);
+
+   return (len = PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
+}
+
+static struct device_attribute mipi_dbi_dev_attrs[] = {
+   __ATTR_RO(modalias),
+   __ATTR_NULL,
+};
+
+static int mipi_dbi_uevent(struct device *_dev, struct kobj_uevent_env *env)
+{
+   struct mipi_dbi_device *dev = to_mipi_dbi_device(_dev);
+
+   add_uevent_var(env, MODALIAS=%s%s, MIPI_DBI_MODULE_PREFIX,
+  dev-name);
+   return 0;
+}
+
+static const struct dev_pm_ops mipi_dbi_dev_pm_ops = {
+   .runtime_suspend = pm_generic_runtime_suspend,
+   .runtime_resume = pm_generic_runtime_resume,
+   .runtime_idle = pm_generic_runtime_idle,
+   .suspend = pm_generic_suspend,
+   .resume = pm_generic_resume,
+   .freeze = pm_generic_freeze,
+   .thaw = pm_generic_thaw,
+