Re: [PATCH v6 1/2] video: support MIPI-DSI controller driver

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.