[PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[Inki Dae]

this patch addes MIPI-DSI based sample panel driver.
to write MIPI-DSI based lcd panel driver, you can refer to
this sample driver.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/video/Kconfig           |    7 ++
 drivers/video/Makefile          |    1 +
 drivers/video/s5p_mipi_sample.c |  220 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 drivers/video/s5p_mipi_sample.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index aa305c5..325ce86 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2003,6 +2003,13 @@ config S5P_MIPI_DSI
 	---help---
 	  This enables support for MIPI-DSI device.
 
+config S5P_MIPI_SAMPLE
+	tristate "Samsung SoC MIPI-DSI based sample driver."
+	depends on S5P_MIPI_DSI && BACKLIGHT_LCD_SUPPORT
+	default n
+	---help---
+	  This enables support for MIPI-DSI based sample driver.
+
 config FB_NUC900
         bool "NUC900 LCD framebuffer support"
         depends on FB && ARCH_W90X900
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index f4baed6..c8ac591 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_FB_S3C)		  += s3c-fb.o
 obj-$(CONFIG_FB_S3C2410)	  += s3c2410fb.o
 obj-$(CONFIG_S5P_MIPI_DSI)	  += s5p_mipi_dsi.o s5p_mipi_dsi_common.o \
     					s5p_mipi_dsi_lowlevel.o
+obj-$(CONFIG_S5P_MIPI_SAMPLE)		+= s5p_mipi_sample.o
 obj-$(CONFIG_FB_FSL_DIU)	  += fsl-diu-fb.o
 obj-$(CONFIG_FB_COBALT)           += cobalt_lcdfb.o
 obj-$(CONFIG_FB_PNX4008_DUM)	  += pnx4008/
diff --git a/drivers/video/s5p_mipi_sample.c b/drivers/video/s5p_mipi_sample.c
new file mode 100644
index 0000000..8a8abfe
--- /dev/null
+++ b/drivers/video/s5p_mipi_sample.c
@@ -0,0 +1,220 @@
+/* linux/drivers/video/sample.c
+ *
+ * MIPI-DSI based sample AMOLED lcd panel driver.
+ *
+ * Inki Dae, <inki.dae@samsung.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/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/ctype.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/lcd.h>
+#include <linux/backlight.h>
+
+#include <video/mipi_display.h>
+
+#include <plat/mipi-dsi.h>
+#include <plat/mipi-ddi.h>
+
+#define MIN_BRIGHTNESS		(0)
+#define MAX_BRIGHTNESS		(10)
+
+#define lcd_to_master(a)	(a->mipi_dev->master)
+#define lcd_to_master_ops(a)	((lcd_to_master(a))->master_ops)
+#define device_to_ddi_pd(a)	(a->master->dsim_info->mipi_ddi_pd)
+
+struct sample {
+	struct device			*dev;
+
+	struct lcd_device		*ld;
+	struct backlight_device		*bd;
+
+	struct mipi_lcd_device		*mipi_dev;
+	struct mipi_ddi_platform_data	*ddi_pd;
+};
+
+static void sample_long_command(struct sample *lcd)
+{
+	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
+	unsigned char data_to_send[5] = {
+		0x00, 0x00, 0x00, 0x00, 0x00
+	};
+
+	ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
+		(unsigned int) data_to_send, sizeof(data_to_send));
+}
+
+static void sample_short_command(struct sample *lcd)
+{
+	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
+
+	ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_SHORT_WRITE_PARAM,
+				0x00, 0x00);
+}
+
+static int sample_panel_init(struct sample *lcd)
+{
+	sample_long_command(lcd);
+	sample_short_command(lcd);
+
+	return 0;
+}
+
+static int sample_gamma_ctrl(struct sample *lcd, int gamma)
+{
+	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
+
+	/* change transfer mode to LP mode */
+	if (ops->change_dsim_transfer_mode)
+		ops->change_dsim_transfer_mode(lcd_to_master(lcd), 0);
+
+	/* update gamma table. */
+
+	/* change transfer mode to HS mode */
+	if (ops->change_dsim_transfer_mode)
+		ops->change_dsim_transfer_mode(lcd_to_master(lcd), 1);
+
+	return 0;
+}
+
+static int sample_get_brightness(struct backlight_device *bd)
+{
+	return bd->props.brightness;
+}
+
+static int sample_set_brightness(struct backlight_device *bd)
+{
+	int ret = 0, brightness = bd->props.brightness;
+	struct sample *lcd = bl_get_data(bd);
+
+	if (brightness < MIN_BRIGHTNESS ||
+		brightness > bd->props.max_brightness) {
+		dev_err(lcd->dev, "lcd brightness should be %d to %d.\n",
+			MIN_BRIGHTNESS, MAX_BRIGHTNESS);
+		return -EINVAL;
+	}
+
+	ret = sample_gamma_ctrl(lcd, bd->props.brightness);
+	if (ret) {
+		dev_err(&bd->dev, "lcd brightness setting failed.\n");
+		return -EIO;
+	}
+
+	return ret;
+}
+
+static const struct backlight_ops sample_backlight_ops = {
+	.get_brightness = sample_get_brightness,
+	.update_status = sample_set_brightness,
+};
+
+static int sample_probe(struct mipi_lcd_device *mipi_dev)
+{
+	struct sample *lcd = NULL;
+	struct backlight_device *bd = NULL;
+
+	lcd = kzalloc(sizeof(struct sample), GFP_KERNEL);
+	if (!lcd) {
+		dev_err(&mipi_dev->dev, "failed to allocate sample structure.\n");
+		return -ENOMEM;
+	}
+
+	lcd->mipi_dev = mipi_dev;
+	lcd->ddi_pd +		(struct mipi_ddi_platform_data *)device_to_ddi_pd(mipi_dev);
+	lcd->dev = &mipi_dev->dev;
+
+	dev_set_drvdata(&mipi_dev->dev, lcd);
+
+	bd = backlight_device_register("sample-bl", lcd->dev, lcd,
+		&sample_backlight_ops, NULL);
+
+	lcd->bd = bd;
+
+	bd->props.max_brightness = MAX_BRIGHTNESS;
+	bd->props.brightness = MAX_BRIGHTNESS;
+
+	/* lcd power on */
+	if (lcd->ddi_pd->lcd_power_on)
+		lcd->ddi_pd->lcd_power_on(NULL, 1);
+
+	mdelay(lcd->ddi_pd->reset_delay);
+
+	/* lcd reset */
+	if (lcd->ddi_pd->lcd_reset)
+		lcd->ddi_pd->lcd_reset(NULL);
+
+	sample_panel_init(lcd);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int sample_suspend(struct mipi_lcd_device *mipi_dev)
+{
+	struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
+
+	/* some work. */
+
+	mdelay(lcd->ddi_pd->power_off_delay);
+
+	/* lcd power off */
+	if (lcd->ddi_pd->lcd_power_on)
+		lcd->ddi_pd->lcd_power_on(NULL, 0);
+
+	return 0;
+}
+
+static int sample_resume(struct mipi_lcd_device *mipi_dev)
+{
+	struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
+
+	mdelay(lcd->ddi_pd->power_on_delay);
+
+	/* lcd power on */
+	if (lcd->ddi_pd->lcd_power_on)
+		lcd->ddi_pd->lcd_power_on(NULL, 1);
+
+	/* some work. */
+
+	return 0;
+}
+#else
+#define sample_suspend		NULL
+#define sample_resume		NULL
+#endif
+
+static struct mipi_lcd_driver sample_mipi_driver = {
+	.name = "sample",
+
+	.probe = sample_probe,
+	.suspend = sample_suspend,
+	.resume = sample_resume,
+};
+
+static int sample_init(void)
+{
+	s5p_mipi_register_lcd_driver(&sample_mipi_driver);
+
+	return 0;
+}
+
+static void sample_exit(void)
+{
+	return;
+}
+
+module_init(sample_init);
+module_exit(sample_exit);
+
+MODULE_AUTHOR("Inki Dae <inki.dae@samsung.com>");
+MODULE_DESCRIPTION("MIPI-DSI based sample AMOLED LCD Panel Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

RE: [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[Kukjin Kim]

Inki Dae wrote:
> 
> this patch addes MIPI-DSI based sample panel driver.
> to write MIPI-DSI based lcd panel driver, you can refer to
> this sample driver.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/video/Kconfig           |    7 ++
>  drivers/video/Makefile          |    1 +
>  drivers/video/s5p_mipi_sample.c |  220
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/s5p_mipi_sample.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index aa305c5..325ce86 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -2003,6 +2003,13 @@ config S5P_MIPI_DSI
>  	---help---
>  	  This enables support for MIPI-DSI device.
> 
> +config S5P_MIPI_SAMPLE
> +	tristate "Samsung SoC MIPI-DSI based sample driver."
> +	depends on S5P_MIPI_DSI && BACKLIGHT_LCD_SUPPORT

Do we really need SAMPLE driver?...
And is this MIPI SAMPLE not MIPI DSIM sample?

> +	default n
> +	---help---
> +	  This enables support for MIPI-DSI based sample driver.
> +
>  config FB_NUC900
>          bool "NUC900 LCD framebuffer support"
>          depends on FB && ARCH_W90X900
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index f4baed6..c8ac591 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -117,6 +117,7 @@ obj-$(CONFIG_FB_S3C)		  += s3c-fb.o
>  obj-$(CONFIG_FB_S3C2410)	  += s3c2410fb.o
>  obj-$(CONFIG_S5P_MIPI_DSI)	  += s5p_mipi_dsi.o s5p_mipi_dsi_common.o \
>      					s5p_mipi_dsi_lowlevel.o
> +obj-$(CONFIG_S5P_MIPI_SAMPLE)		+= s5p_mipi_sample.o
>  obj-$(CONFIG_FB_FSL_DIU)	  += fsl-diu-fb.o
>  obj-$(CONFIG_FB_COBALT)           += cobalt_lcdfb.o
>  obj-$(CONFIG_FB_PNX4008_DUM)	  += pnx4008/

(snip)

Happy New Year!
Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

Re: [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[daeinki]

Kukjin Kim 쓴 글:
> Inki Dae wrote:
>> this patch addes MIPI-DSI based sample panel driver.
>> to write MIPI-DSI based lcd panel driver, you can refer to
>> this sample driver.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> ---
>>  drivers/video/Kconfig           |    7 ++
>>  drivers/video/Makefile          |    1 +
>>  drivers/video/s5p_mipi_sample.c |  220
>> +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 228 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/video/s5p_mipi_sample.c
>>
>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>> index aa305c5..325ce86 100644
>> --- a/drivers/video/Kconfig
>> +++ b/drivers/video/Kconfig
>> @@ -2003,6 +2003,13 @@ config S5P_MIPI_DSI
>>  	---help---
>>  	  This enables support for MIPI-DSI device.
>>
>> +config S5P_MIPI_SAMPLE
>> +	tristate "Samsung SoC MIPI-DSI based sample driver."
>> +	depends on S5P_MIPI_DSI && BACKLIGHT_LCD_SUPPORT
> 
> Do we really need SAMPLE driver?...
> And is this MIPI SAMPLE not MIPI DSIM sample?
> 
I added this one for that someone who uses my mipi-dsi driver could 
understand my driver easily. it is just for reference driver.
this sample driver has some problem?

>> +	default n
>> +	---help---
>> +	  This enables support for MIPI-DSI based sample driver.
>> +
>>  config FB_NUC900
>>          bool "NUC900 LCD framebuffer support"
>>          depends on FB && ARCH_W90X900
>> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
>> index f4baed6..c8ac591 100644
>> --- a/drivers/video/Makefile
>> +++ b/drivers/video/Makefile
>> @@ -117,6 +117,7 @@ obj-$(CONFIG_FB_S3C)		  += s3c-fb.o
>>  obj-$(CONFIG_FB_S3C2410)	  += s3c2410fb.o
>>  obj-$(CONFIG_S5P_MIPI_DSI)	  += s5p_mipi_dsi.o s5p_mipi_dsi_common.o \
>>      					s5p_mipi_dsi_lowlevel.o
>> +obj-$(CONFIG_S5P_MIPI_SAMPLE)		+= s5p_mipi_sample.o
>>  obj-$(CONFIG_FB_FSL_DIU)	  += fsl-diu-fb.o
>>  obj-$(CONFIG_FB_COBALT)           += cobalt_lcdfb.o
>>  obj-$(CONFIG_FB_PNX4008_DUM)	  += pnx4008/
> 
> (snip)
> 
> Happy New Year!
> Thanks.
> 
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
> 
> 

Re: [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[Paul Mundt]

On Tue, Dec 28, 2010 at 08:26:35PM +0900, Inki Dae wrote:
> this patch addes MIPI-DSI based sample panel driver.
> to write MIPI-DSI based lcd panel driver, you can refer to
> this sample driver.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>

Sample drivers are ok, but unless it has some sort of practical run-time
use you are probably better off just including this along with your
subsystem/platform documentation in Documentation somewhere. You can
search for .c files there to see how others are doing it.

Having said that ..

> diff --git a/drivers/video/s5p_mipi_sample.c b/drivers/video/s5p_mipi_sample.c
> new file mode 100644
> index 0000000..8a8abfe
> --- /dev/null
> +++ b/drivers/video/s5p_mipi_sample.c
> @@ -0,0 +1,220 @@
> +/* linux/drivers/video/sample.c
> + *
This is precisely why file comments are useless, since they invariably
fail to match up.

> + * MIPI-DSI based sample AMOLED lcd panel driver.
> + *
> + * Inki Dae, <inki.dae@samsung.com>
> + *
No Copyright notice?

> +static void sample_long_command(struct sample *lcd)
> +{
> +	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> +	unsigned char data_to_send[5] = {
> +		0x00, 0x00, 0x00, 0x00, 0x00
> +	};
> +
> +	ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
> +		(unsigned int) data_to_send, sizeof(data_to_send));
> +}
> +
> +static void sample_short_command(struct sample *lcd)
> +{
> +	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> +
> +	ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_SHORT_WRITE_PARAM,
> +				0x00, 0x00);
> +}
> +
ops->cmd_write() can fail for any number of reasons, so you will want to
change these to make sure that you are actually handling the error cases.

> +static int sample_panel_init(struct sample *lcd)
> +{
> +	sample_long_command(lcd);
> +	sample_short_command(lcd);
> +
> +	return 0;

At which point you can fail the initialization instead of just blowing up
later.

> +static int sample_gamma_ctrl(struct sample *lcd, int gamma)
> +{
> +	struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
> +
> +	/* change transfer mode to LP mode */
> +	if (ops->change_dsim_transfer_mode)
> +		ops->change_dsim_transfer_mode(lcd_to_master(lcd), 0);
> +
ops->change_dsim_transfer_mode() can also fail. You could do this more
cleanly as:

enum { DSIM_XFER_LP, DSIM_XFER_HS };

static inline int dsim_set_xfer_mode(struct mipi_dsim_device *dsim, int mode)
{
	struct mipi_dsim_master_ops *ops = dsim->master_ops;

	if (!ops->change_dsim_transfer_mode)
		return -ENOSYS;	/* not implemented */

	return ops->change_dsim_transfer_mode(dsim, mode);
}

Then simply do your sample_gamma_ctrl() as:

	ret = dsim_set_xfer_mode(dsim, DSIM_XFER_LP);
	if (ret != 0)
		return ret;

> +	/* update gamma table. */
> +

	return dsim_set_xfer_mode(dsim, DSIM_XFER_HS);
> +}
> +

Or something similar. Your sample code should really be as
self-documenting and error-proof as possible. You don't really want to be
in a situation where subtle bugs leak through that then everyone who uses
this code as a reference will carry over!

> +static int sample_set_brightness(struct backlight_device *bd)
> +{
> +	int ret = 0, brightness = bd->props.brightness;
> +	struct sample *lcd = bl_get_data(bd);
> +
> +	if (brightness < MIN_BRIGHTNESS ||
> +		brightness > bd->props.max_brightness) {
> +		dev_err(lcd->dev, "lcd brightness should be %d to %d.\n",
> +			MIN_BRIGHTNESS, MAX_BRIGHTNESS);
> +		return -EINVAL;
> +	}
> +
> +	ret = sample_gamma_ctrl(lcd, bd->props.brightness);
> +	if (ret) {
> +		dev_err(&bd->dev, "lcd brightness setting failed.\n");
> +		return -EIO;
> +	}
> +
With your current approach this error condition will never be reached,
for example.

> +static int sample_probe(struct mipi_lcd_device *mipi_dev)
> +{
> +	struct sample *lcd = NULL;
> +	struct backlight_device *bd = NULL;
> +
> +	lcd = kzalloc(sizeof(struct sample), GFP_KERNEL);
> +	if (!lcd) {
> +		dev_err(&mipi_dev->dev, "failed to allocate sample structure.\n");
> +		return -ENOMEM;
> +	}
> +
> +	lcd->mipi_dev = mipi_dev;
> +	lcd->ddi_pd > +		(struct mipi_ddi_platform_data *)device_to_ddi_pd(mipi_dev);

Does this really need to be casted?

> +	lcd->dev = &mipi_dev->dev;
> +
> +	dev_set_drvdata(&mipi_dev->dev, lcd);
> +
> +	bd = backlight_device_register("sample-bl", lcd->dev, lcd,
> +		&sample_backlight_ops, NULL);
> +
> +	lcd->bd = bd;
> +
And here you have no error checking for backlight registration, so you
will get a NULL pointer deref:

> +	bd->props.max_brightness = MAX_BRIGHTNESS;
> +	bd->props.brightness = MAX_BRIGHTNESS;
> +
here. backlight_device_register() returns an ERR_PTR(), so you will want
to do an IS_ERR() check, which you can then map back with PTR_ERR() for a
more precise idea of why it failed.

> +	/* lcd power on */
> +	if (lcd->ddi_pd->lcd_power_on)
> +		lcd->ddi_pd->lcd_power_on(NULL, 1);
> +
You may wish to use enums for this too. It's not strictly necessary, but
it does help to clarify which is the on and which is the off position.

> +	mdelay(lcd->ddi_pd->reset_delay);
> +
> +	/* lcd reset */
> +	if (lcd->ddi_pd->lcd_reset)
> +		lcd->ddi_pd->lcd_reset(NULL);
> +
Reset can fail?

> +	sample_panel_init(lcd);
> +
> +	return 0;
> +}
sample_panel_init() can fail as well, and in both cases you will need to
clean up all of the above work.

> +
> +#ifdef CONFIG_PM
> +static int sample_suspend(struct mipi_lcd_device *mipi_dev)
> +{
> +	struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
> +
> +	/* some work. */
> +
> +	mdelay(lcd->ddi_pd->power_off_delay);
> +
Adding delays in the suspend/resume path sounds like a pretty bad idea,
is there a technical reason for it? If so, please document it, so people
get some idea of where their suspend/resume latencies are coming from,
and why.

> +static struct mipi_lcd_driver sample_mipi_driver = {
> +	.name = "sample",
> +
> +	.probe = sample_probe,

No remove?

> +	.suspend = sample_suspend,
> +	.resume = sample_resume,
> +};
> +
> +static int sample_init(void)
> +{
> +	s5p_mipi_register_lcd_driver(&sample_mipi_driver);
> +
> +	return 0;
> +}
> +
This should be:

	return s5p_mipi_register_lcd_driver(&sample_mipi_driver);

And sample_init should be __init annotated.

> +static void sample_exit(void)
> +{
> +	return;
> +}
> +
This should be balanced with a

	s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);

> +module_init(sample_init);
> +module_exit(sample_exit);
> +
> +MODULE_AUTHOR("Inki Dae <inki.dae@samsung.com>");
> +MODULE_DESCRIPTION("MIPI-DSI based sample AMOLED LCD Panel Driver");
> +MODULE_LICENSE("GPL");

Since you have a fairly complex subsystem it's probably a good idea to
work out how your MODULE_ALIAS() is going to work, so that you can handle
autoprobe for these things via udev. The fact you have no exit path
definitely suggests you haven't tested this in a modular configuration,
so there is probably going to be quite a bit of work to do here.

Re: [PATCH 5/5] S5PC110: add MIPI-DSI based sample lcd panel driver.

[InKi Dae]

thank you for your reviews.

all the problems you  pointed out would be corrected for actual lcd
panel driver.
(this lcd panel driver has being worked locally and it will be posted
in the future)

after this patch, I posted next patches.
refer to below please.
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/101521

in this patch, I removed sample driver and also have code compacting
and refactoring for MIPI-DSI driver. so could you please give me your
review about this one?

2011/1/6 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Dec 28, 2010 at 08:26:35PM +0900, Inki Dae wrote:
>> this patch addes MIPI-DSI based sample panel driver.
>> to write MIPI-DSI based lcd panel driver, you can refer to
>> this sample driver.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>
> Sample drivers are ok, but unless it has some sort of practical run-time
> use you are probably better off just including this along with your
> subsystem/platform documentation in Documentation somewhere. You can
> search for .c files there to see how others are doing it.
>
> Having said that ..
>
>> diff --git a/drivers/video/s5p_mipi_sample.c b/drivers/video/s5p_mipi_sample.c
>> new file mode 100644
>> index 0000000..8a8abfe
>> --- /dev/null
>> +++ b/drivers/video/s5p_mipi_sample.c
>> @@ -0,0 +1,220 @@
>> +/* linux/drivers/video/sample.c
>> + *
> This is precisely why file comments are useless, since they invariably
> fail to match up.
>
>> + * MIPI-DSI based sample AMOLED lcd panel driver.
>> + *
>> + * Inki Dae, <inki.dae@samsung.com>
>> + *
> No Copyright notice?
>
>> +static void sample_long_command(struct sample *lcd)
>> +{
>> +     struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +     unsigned char data_to_send[5] = {
>> +             0x00, 0x00, 0x00, 0x00, 0x00
>> +     };
>> +
>> +     ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_LONG_WRITE,
>> +             (unsigned int) data_to_send, sizeof(data_to_send));
>> +}
>> +
>> +static void sample_short_command(struct sample *lcd)
>> +{
>> +     struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> +     ops->cmd_write(lcd_to_master(lcd), MIPI_DSI_DCS_SHORT_WRITE_PARAM,
>> +                             0x00, 0x00);
>> +}
>> +
> ops->cmd_write() can fail for any number of reasons, so you will want to
> change these to make sure that you are actually handling the error cases.
>
>> +static int sample_panel_init(struct sample *lcd)
>> +{
>> +     sample_long_command(lcd);
>> +     sample_short_command(lcd);
>> +
>> +     return 0;
>
> At which point you can fail the initialization instead of just blowing up
> later.
>
>> +static int sample_gamma_ctrl(struct sample *lcd, int gamma)
>> +{
>> +     struct dsim_master_ops *ops = lcd_to_master_ops(lcd);
>> +
>> +     /* change transfer mode to LP mode */
>> +     if (ops->change_dsim_transfer_mode)
>> +             ops->change_dsim_transfer_mode(lcd_to_master(lcd), 0);
>> +
> ops->change_dsim_transfer_mode() can also fail. You could do this more
> cleanly as:
>
> enum { DSIM_XFER_LP, DSIM_XFER_HS };
>
> static inline int dsim_set_xfer_mode(struct mipi_dsim_device *dsim, int mode)
> {
>        struct mipi_dsim_master_ops *ops = dsim->master_ops;
>
>        if (!ops->change_dsim_transfer_mode)
>                return -ENOSYS; /* not implemented */
>
>        return ops->change_dsim_transfer_mode(dsim, mode);
> }
>
> Then simply do your sample_gamma_ctrl() as:
>
>        ret = dsim_set_xfer_mode(dsim, DSIM_XFER_LP);
>        if (ret != 0)
>                return ret;
>
>> +     /* update gamma table. */
>> +
>
>        return dsim_set_xfer_mode(dsim, DSIM_XFER_HS);
>> +}
>> +
>
> Or something similar. Your sample code should really be as
> self-documenting and error-proof as possible. You don't really want to be
> in a situation where subtle bugs leak through that then everyone who uses
> this code as a reference will carry over!
>
>> +static int sample_set_brightness(struct backlight_device *bd)
>> +{
>> +     int ret = 0, brightness = bd->props.brightness;
>> +     struct sample *lcd = bl_get_data(bd);
>> +
>> +     if (brightness < MIN_BRIGHTNESS ||
>> +             brightness > bd->props.max_brightness) {
>> +             dev_err(lcd->dev, "lcd brightness should be %d to %d.\n",
>> +                     MIN_BRIGHTNESS, MAX_BRIGHTNESS);
>> +             return -EINVAL;
>> +     }
>> +
>> +     ret = sample_gamma_ctrl(lcd, bd->props.brightness);
>> +     if (ret) {
>> +             dev_err(&bd->dev, "lcd brightness setting failed.\n");
>> +             return -EIO;
>> +     }
>> +
> With your current approach this error condition will never be reached,
> for example.
>
>> +static int sample_probe(struct mipi_lcd_device *mipi_dev)
>> +{
>> +     struct sample *lcd = NULL;
>> +     struct backlight_device *bd = NULL;
>> +
>> +     lcd = kzalloc(sizeof(struct sample), GFP_KERNEL);
>> +     if (!lcd) {
>> +             dev_err(&mipi_dev->dev, "failed to allocate sample structure.\n");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     lcd->mipi_dev = mipi_dev;
>> +     lcd->ddi_pd >> +             (struct mipi_ddi_platform_data *)device_to_ddi_pd(mipi_dev);
>
> Does this really need to be casted?
>
>> +     lcd->dev = &mipi_dev->dev;
>> +
>> +     dev_set_drvdata(&mipi_dev->dev, lcd);
>> +
>> +     bd = backlight_device_register("sample-bl", lcd->dev, lcd,
>> +             &sample_backlight_ops, NULL);
>> +
>> +     lcd->bd = bd;
>> +
> And here you have no error checking for backlight registration, so you
> will get a NULL pointer deref:
>
>> +     bd->props.max_brightness = MAX_BRIGHTNESS;
>> +     bd->props.brightness = MAX_BRIGHTNESS;
>> +
> here. backlight_device_register() returns an ERR_PTR(), so you will want
> to do an IS_ERR() check, which you can then map back with PTR_ERR() for a
> more precise idea of why it failed.
>
>> +     /* lcd power on */
>> +     if (lcd->ddi_pd->lcd_power_on)
>> +             lcd->ddi_pd->lcd_power_on(NULL, 1);
>> +
> You may wish to use enums for this too. It's not strictly necessary, but
> it does help to clarify which is the on and which is the off position.
>
>> +     mdelay(lcd->ddi_pd->reset_delay);
>> +
>> +     /* lcd reset */
>> +     if (lcd->ddi_pd->lcd_reset)
>> +             lcd->ddi_pd->lcd_reset(NULL);
>> +
> Reset can fail?
>
>> +     sample_panel_init(lcd);
>> +
>> +     return 0;
>> +}
> sample_panel_init() can fail as well, and in both cases you will need to
> clean up all of the above work.
>
>> +
>> +#ifdef CONFIG_PM
>> +static int sample_suspend(struct mipi_lcd_device *mipi_dev)
>> +{
>> +     struct sample *lcd = dev_get_drvdata(&mipi_dev->dev);
>> +
>> +     /* some work. */
>> +
>> +     mdelay(lcd->ddi_pd->power_off_delay);
>> +
> Adding delays in the suspend/resume path sounds like a pretty bad idea,
> is there a technical reason for it? If so, please document it, so people
> get some idea of where their suspend/resume latencies are coming from,
> and why.
>
>> +static struct mipi_lcd_driver sample_mipi_driver = {
>> +     .name = "sample",
>> +
>> +     .probe = sample_probe,
>
> No remove?
>
>> +     .suspend = sample_suspend,
>> +     .resume = sample_resume,
>> +};
>> +
>> +static int sample_init(void)
>> +{
>> +     s5p_mipi_register_lcd_driver(&sample_mipi_driver);
>> +
>> +     return 0;
>> +}
>> +
> This should be:
>
>        return s5p_mipi_register_lcd_driver(&sample_mipi_driver);
>
> And sample_init should be __init annotated.
>
>> +static void sample_exit(void)
>> +{
>> +     return;
>> +}
>> +
> This should be balanced with a
>
>        s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);
>
>> +module_init(sample_init);
>> +module_exit(sample_exit);
>> +
>> +MODULE_AUTHOR("Inki Dae <inki.dae@samsung.com>");
>> +MODULE_DESCRIPTION("MIPI-DSI based sample AMOLED LCD Panel Driver");
>> +MODULE_LICENSE("GPL");
>
> Since you have a fairly complex subsystem it's probably a good idea to
> work out how your MODULE_ALIAS() is going to work, so that you can handle
> autoprobe for these things via udev. The fact you have no exit path
> definitely suggests you haven't tested this in a modular configuration,
> so there is probably going to be quite a bit of work to do here.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>