Linux Framebuffer Layer development

* Re: [PATCH v6 1/2] video: support MIPI-DSI controller driver
From: Kyungmin Park @ 2012-01-19  0:15 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20120118130550.0bdb227e.akpm@linux-foundation.org>

On 1/19/12, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 18 Jan 2012 11:03:56 +0900
> Donghwa Lee <dh09.lee@samsung.com> wrote:
>
>> 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?

Hi,
As I know today is the last day for merge, So if you don't mind and
it's okay. we hope to merge it first and fix it at rc period as you
commented.
And another patch from Andrew, it's also helpful if it's merge also

Thank you,
Kyungmin Park
>
> A few minor comments:
>
>>
>> ...
>>
>> +#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!
>
>>
>> ...
>>
>> +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.
>
>
>> +	if (!lcd_dev->name) {
>> +		pr_err("dsim_lcd_device name is NULL.\n");
>> +		return -EFAULT;
>> +	}
>
> Ditto.
>
>> +	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.
>
>> +	}
>> +
>> +	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.
>
>> +{
>> +	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.
>
>> +			/**
>> +			 * 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!
>
>> +	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.
>
>> +{
>> +	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!
>
>> +	}
>> +
>> +	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*
>
>> +	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.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

^ permalink raw reply