On Thu, 19 Jan 2012, Andrew Morton wrote:
On Wed, 18 Jan 2012 11:03:56 +0900
Donghwa Lee [off-list ref] wrote:
quoted
Samsung S5PC210 and EXYNOS SoC platform has MIPI-DSI controller and MIPI-DSI
based LCD Panel could be used with it. This patch supports MIPI-DSI driver
based Samsung SoC chip.
LCD panel driver based MIPI-DSI should be registered to MIPI-DSI driver at
machine code and LCD panel driver specific function registered to mipi_dsim_ddi
structure at lcd panel init function called system init.
In the MIPI-DSI driver, find lcd panel driver by using registered
lcd panel name, and then initialize lcd panel driver.
The code looks nice to me. I assume that Florian will be processing
the patch?
A few minor comments:
Thank you for your comments. I will send corrected next version patch set.
quoted
...
+#define master_to_driver(a) (a->dsim_lcd_drv)
+#define master_to_device(a) (a->dsim_lcd_dev)
+#define dev_to_dsim(a) platform_get_drvdata(to_platform_device(a))
These aren't very type-safe: can be called on any struct whcih has a
field called "dsim_lcd_drv". Also the "a" should be parenthesized to
prevent obscure compile problems.
Really, it's best to do away with all such problems by implementing
these helpers in C rather than in cpp!
Yes, you're right. I will fix it next version patch set.
quoted
...
+int s5p_mipi_dsi_register_lcd_device(struct mipi_dsim_lcd_device *lcd_dev)
+{
+ struct mipi_dsim_ddi *dsim_ddi;
+
+ if (!lcd_dev) {
+ pr_err("mipi_dsim_lcd_device is NULL.\n");
+ return -EFAULT;
+ }
Can this happen? If not then BUG() is more appropriate. Or just
remove the code altogether and let the kernel oops.
Maybe I think it can't happen. I will remove it.
quoted
+ if (!lcd_dev->name) {
+ pr_err("dsim_lcd_device name is NULL.\n");
+ return -EFAULT;
+ }
Ditto.
quoted
+ dsim_ddi = kzalloc(sizeof(struct mipi_dsim_ddi), GFP_KERNEL);
+ if (!dsim_ddi) {
+ pr_err("failed to allocate dsim_ddi object.\n");
+ return -EFAULT;
Should be ENOMEM.
Yes, I agree with you.
quoted
+ }
+
+ dsim_ddi->dsim_lcd_dev = lcd_dev;
+
+ mutex_lock(&mipi_dsim_lock);
+ list_add_tail(&dsim_ddi->list, &dsim_ddi_list);
+ mutex_unlock(&mipi_dsim_lock);
+
+ return 0;
+}
+
+struct mipi_dsim_ddi
+ *s5p_mipi_dsi_find_lcd_device(struct mipi_dsim_lcd_driver *lcd_drv)
Strange code layout.
Yes,
quoted
+{
+ struct mipi_dsim_ddi *dsim_ddi, *next;
+ struct mipi_dsim_lcd_device *lcd_dev;
+
+ mutex_lock(&mipi_dsim_lock);
+
+ list_for_each_entry_safe(dsim_ddi, next, &dsim_ddi_list, list) {
+ if (!dsim_ddi)
+ goto out;
+
+ lcd_dev = dsim_ddi->dsim_lcd_dev;
+ if (!lcd_dev)
+ continue;
+
+ if ((strcmp(lcd_drv->name, lcd_dev->name)) == 0) {
hm, using strcmp() to compare devices in this way looks fishy. But I
don't know what is going on here.
This mipi dsi driver may have one or more lcd driver. So, this function can find
appropriate lcd driver by using strcmp().
quoted
+ /**
+ * bus_id would be used to identify
+ * connected bus.
+ */
+ dsim_ddi->bus_id = lcd_dev->bus_id;
+ mutex_unlock(&mipi_dsim_lock);
+
+ return dsim_ddi;
+ }
+
+ list_del(&dsim_ddi->list);
+ kfree(dsim_ddi);
+ }
+
+out:
+ mutex_unlock(&mipi_dsim_lock);
+
+ return NULL;
+}
+
+int s5p_mipi_dsi_register_lcd_driver(struct mipi_dsim_lcd_driver *lcd_drv)
+{
+ struct mipi_dsim_ddi *dsim_ddi;
+
+ if (!lcd_drv) {
+ pr_err("mipi_dsim_lcd_driver is NULL.\n");
+ return -EFAULT;
+ }
+
+ if (!lcd_drv->name) {
+ pr_err("dsim_lcd_driver name is NULL.\n");
+ return -EFAULT;
+ }
+
+ dsim_ddi = s5p_mipi_dsi_find_lcd_device(lcd_drv);
+ if (!dsim_ddi) {
+ pr_err("mipi_dsim_ddi object not found.\n");
+ return -EFAULT;
+ }
Boy, someone really likes EFAULT!
Ok, I will fix it.
quoted
+ dsim_ddi->dsim_lcd_drv = lcd_drv;
+
+ pr_info("registered panel driver(%s) to mipi-dsi driver.\n",
+ lcd_drv->name);
+
+ return 0;
+
+}
+
+struct mipi_dsim_ddi
+ *s5p_mipi_dsi_bind_lcd_ddi(struct mipi_dsim_device *dsim,
+ const char *name)
More code layout oddness.
quoted
+{
+ struct mipi_dsim_ddi *dsim_ddi, *next;
+ struct mipi_dsim_lcd_driver *lcd_drv;
+ struct mipi_dsim_lcd_device *lcd_dev;
+ int ret;
+
+ mutex_lock(&dsim->lock);
+
+ list_for_each_entry_safe(dsim_ddi, next, &dsim_ddi_list, list) {
+ lcd_drv = dsim_ddi->dsim_lcd_drv;
+ lcd_dev = dsim_ddi->dsim_lcd_dev;
+ if (!lcd_drv || !lcd_dev ||
+ (dsim->id != dsim_ddi->bus_id))
+ continue;
+
+ dev_dbg(dsim->dev, "lcd_drv->id = %d, lcd_dev->id = %d\n",
+ lcd_drv->id, lcd_dev->id);
+ dev_dbg(dsim->dev, "lcd_dev->bus_id = %d, dsim->id = %d\n",
+ lcd_dev->bus_id, dsim->id);
+
+ if ((strcmp(lcd_drv->name, name) == 0)) {
+ lcd_dev->master = dsim;
+
+ lcd_dev->dev.parent = dsim->dev;
+ dev_set_name(&lcd_dev->dev, "%s", lcd_drv->name);
+
+ ret = device_register(&lcd_dev->dev);
+ if (ret < 0) {
+ dev_err(dsim->dev,
+ "can't register %s, status %d\n",
+ dev_name(&lcd_dev->dev), ret);
+ mutex_unlock(&dsim->lock);
+
+ return NULL;
+ }
+
+ dsim->dsim_lcd_dev = lcd_dev;
+ dsim->dsim_lcd_drv = lcd_drv;
+
+ mutex_unlock(&dsim->lock);
+
+ return dsim_ddi;
+ }
+ }
+
+ mutex_unlock(&dsim->lock);
+
+ return NULL;
+}
...
+static int s5p_mipi_dsi_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ struct mipi_dsim_device *dsim;
+ struct mipi_dsim_config *dsim_config;
+ struct mipi_dsim_platform_data *dsim_pd;
+ struct mipi_dsim_ddi *dsim_ddi;
+ int ret = -EINVAL;
+
+ dsim = kzalloc(sizeof(struct mipi_dsim_device), GFP_KERNEL);
+ if (!dsim) {
+ dev_err(&pdev->dev, "failed to allocate dsim object.\n");
+ return -EFAULT;
ENOMEM!
quoted
+ }
+
+ dsim->pd = to_dsim_plat(pdev);
+ dsim->dev = &pdev->dev;
+ dsim->id = pdev->id;
+
+ /* get mipi_dsim_platform_data. */
+ dsim_pd = (struct mipi_dsim_platform_data *)dsim->pd;
+ if (dsim_pd == NULL) {
+ dev_err(&pdev->dev, "failed to get platform data for dsim.\n");
+ goto err_clock_get;
+ }
...
+irqreturn_t s5p_mipi_dsi_interrupt_handler(int irq, void *dev_id)
+{
+ unsigned int intsrc = 0;
+ unsigned int intmsk = 0;
+ struct mipi_dsim_device *dsim = NULL;
+
+ dsim = (struct mipi_dsim_device *)dev_id;
unneeded and undesirable cast of void*
Yes, I will fix it.
quoted
+ if (!dsim) {
+ dev_dbg(dsim->dev, KERN_ERR "%s:error: wrong parameter\n",
+ __func__);
+ return IRQ_HANDLED;
+ }
+
+ intsrc = s5p_mipi_dsi_read_interrupt(dsim);
+ intmsk = s5p_mipi_dsi_read_interrupt_mask(dsim);
+
+ intmsk = ~(intmsk) & intsrc;
+
+ switch (intmsk) {
+ case INTMSK_RX_DONE:
+ complete(&dsim_rd_comp);
+ dev_dbg(dsim->dev, "MIPI INTMSK_RX_DONE\n");
+ break;
+ case INTMSK_FIFO_EMPTY:
+ complete(&dsim_wr_comp);
+ dev_dbg(dsim->dev, "MIPI INTMSK_FIFO_EMPTY\n");
+ break;
+ default:
+ break;
+ }
+
+ s5p_mipi_dsi_clear_interrupt(dsim, intmsk);
+
+ return IRQ_HANDLED;
+}
...
Generally: the use of the Efoo errno codes is chaotic. Please check it
all over.