On Thu, 19 Jan 2012, Andrew Morton wrote:
> On Wed, 18 Jan 2012 11:03:56 +0900
> Donghwa Lee <dh09.lee@xxxxxxxxxxx> 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?
>
> A few minor comments:
>
Thank you for your comments. I will send corrected next version patch set.
>>
>> ...
>>
>> +#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.
>>
>> ...
>>
>> +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.
>
>> + 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.
>
Yes, I agree with you.
>> + }
>> +
>> + 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,
>> +{
>> + 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().
>> + /**
>> + * 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.
>> + 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*
>
Yes, I will fix it.
>> + 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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
