S5PC110: add MIPI-DSI based sample lcd panel driver.
PATCH 5/5 S5PC110: add MIPI-DSI based sample lcd panel driver.
this patch addes MIPI-DSI based sample panel driver. to write MIPI-DSI based lcd panel driver, you can refer to this sample driver.
--- 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
drivers/video/Kconfig +7 -0
@@ -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
drivers/video/Makefile +1 -0
@@ -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/
drivers/video/s5p_mipi_sample.c new file +220 -0
220 lines not on this page · raw message
Signature · 1 line
1.7.0.4
PATCH 5/5 S5PC110: add MIPI-DSI based sample lcd panel driver.
Quoted 19 lines · 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 <[email protected]>--- 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.cdrivers/video/Kconfig +0
-0
drivers/video/Kconfig config S5P_MIPI_DSI · line 2008
Quoted 6 lines
drivers/video/Kconfig +3
-0@@ -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?
Quoted 12 lines
drivers/video/Kconfig +4
-0+ default n + ---help--- + This enables support for MIPI-DSI based sample driver. + config FB_NUC900 bool "NUC900 LCD framebuffer support" depends on FB && ARCH_W90X900drivers/video/Makefile +0
-0
drivers/video/Makefile obj-$(CONFIG_FB_S3C) += s3c-fb.o · line 123
Quoted 7 lines
drivers/video/Makefile +1
-0@@ -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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
PATCH 5/5 S5PC110: add MIPI-DSI based sample lcd panel driver.
Kukjin Kim ? ?:
Quoted 22 lines
Quoted 21 lines · 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 <[email protected]>--- 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.cdrivers/video/Kconfig +0
-0@@ -2003,6 +2003,13 @@ config S5P_MIPI_DSI ---help--- This enables support for MIPI-DSI device.
Quoted 7 lines
drivers/video/Kconfig config S5P_MIPI_DSI · line 2008
Quoted 3 lines
drivers/video/Kconfig +3
-0+config S5P_MIPI_SAMPLE + tristate "Samsung SoC MIPI-DSI based sample driver." + depends on S5P_MIPI_DSI && BACKLIGHT_LCD_SUPPORTDo 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?
Quoted 32 lines
Quoted 12 lines
drivers/video/Kconfig +4
-0+ default n + ---help--- + This enables support for MIPI-DSI based sample driver. + config FB_NUC900 bool "NUC900 LCD framebuffer support" depends on FB && ARCH_W90X900drivers/video/Makefile +0
-0drivers/video/Makefile obj-$(CONFIG_FB_S3C) += s3c-fb.o · line 123
Quoted 7 lines
drivers/video/Makefile +1
-0@@ -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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
PATCH 5/5 S5PC110: add MIPI-DSI based sample lcd panel driver.
In reply to Inki DaeQuoted 5 lines · 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 <[email protected]>
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 ..
drivers/video/s5p_mipi_sample.c line 2
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +2
-0@@ -0,0 +1,220 @@ +/* linux/drivers/video/sample.c +*
This is precisely why file comments are useless, since they invariably fail to match up.
drivers/video/s5p_mipi_sample.c line 6
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+* MIPI-DSI based sample AMOLED lcd panel driver. +* +* Inki Dae, <[email protected]> +*
No Copyright notice?
Quoted 9 lines
drivers/video/s5p_mipi_sample.c new file +9
-0+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));
drivers/video/s5p_mipi_sample.c line 62
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+} + +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.
drivers/video/s5p_mipi_sample.c line 68
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+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.
drivers/video/s5p_mipi_sample.c line 78
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +8
-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 ); +
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;
drivers/video/s5p_mipi_sample.c line 41
Quoted 2 lines
drivers/video/s5p_mipi_sample.c new file +2
-0+/* update gamma table. */ +
return dsim_set_xfer_mode(dsim, DSIM_XFER_HS);
drivers/video/s5p_mipi_sample.c line 43
Quoted 2 lines
drivers/video/s5p_mipi_sample.c new file +2
-0+} +
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!
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +8
-0+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",
drivers/video/s5p_mipi_sample.c line 110
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+ 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.
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+static int sample_probe (struct mipi_lcd_device *mipi_dev) +{ +struct sample *lcd =NULL ; +struct backlight_device *bd =NULL ;
drivers/video/s5p_mipi_sample.c line 132
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+ + 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?
drivers/video/s5p_mipi_sample.c line 141
Quoted 9 lines
drivers/video/s5p_mipi_sample.c new file +9
-0+ 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:
drivers/video/s5p_mipi_sample.c line 144
Quoted 3 lines
drivers/video/s5p_mipi_sample.c new file +3
-0+ 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.
drivers/video/s5p_mipi_sample.c line 148
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+/* 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.
drivers/video/s5p_mipi_sample.c line 154
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+mdelay (lcd->ddi_pd->reset_delay); + +/* lcd reset */ +if (lcd->ddi_pd->lcd_reset) + lcd->ddi_pd->lcd_reset (NULL ); +
Reset can fail?
drivers/video/s5p_mipi_sample.c line 158
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+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.
drivers/video/s5p_mipi_sample.c line 168
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-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); +
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.
drivers/video/s5p_mipi_sample.c line 198
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+static struct mipi_lcd_driver sample_mipi_driver = { + .name ="sample" , + + .probe = sample_probe,
No remove?
drivers/video/s5p_mipi_sample.c line 209
Quoted 11 lines
drivers/video/s5p_mipi_sample.c new file +11
-0+ .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.
drivers/video/s5p_mipi_sample.c line 214
Quoted 5 lines
drivers/video/s5p_mipi_sample.c new file +5
-0+static void sample_exit (void ) +{ +return ; +} +
This should be balanced with a
s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);
drivers/video/s5p_mipi_sample.c line 220
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+module_init (sample_init); +module_exit (sample_exit); + +MODULE_AUTHOR ("Inki Dae <[email protected]>" ); +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.
PATCH 5/5 S5PC110: add MIPI-DSI based sample lcd panel driver.
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 <[email protected]>:
Quoted 249 lines · Inki Dae
Quoted 5 lines
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 <[email protected]>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 ..
drivers/video/s5p_mipi_sample.c line 2
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +2
-0@@ -0,0 +1,220 @@ +/* linux/drivers/video/sample.c +* This is precisely why file comments are useless, since they invariably fail to match up.
drivers/video/s5p_mipi_sample.c line 6
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+* MIPI-DSI based sample AMOLED lcd panel driver. +* +* Inki Dae, <[email protected]> +* No Copyright notice?
Quoted 9 lines
drivers/video/s5p_mipi_sample.c new file +9
-0+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));drivers/video/s5p_mipi_sample.c line 62
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+} + +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.drivers/video/s5p_mipi_sample.c
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+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.
drivers/video/s5p_mipi_sample.c line 78
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +8
-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); +
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){
? ? ? ?structmipi_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;drivers/video/s5p_mipi_sample.c line 41
Quoted 2 lines
drivers/video/s5p_mipi_sample.c new file +2
-0+ ? ? /* update gamma table. */ +? ? ? ?return
dsim_set_xfer_mode(dsim, DSIM_XFER_HS);drivers/video/s5p_mipi_sample.c line 43
Quoted 2 lines
drivers/video/s5p_mipi_sample.c new file +2
-0+} +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!
Quoted 8 lines
drivers/video/s5p_mipi_sample.c new file +8
-0+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",drivers/video/s5p_mipi_sample.c line 61
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+ ? ? ? ? ? ? ? ? ? ? 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.
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+static int sample_probe (struct mipi_lcd_device *mipi_dev) +{ + ? ? struct sample *lcd = NULL; + ? ? struct backlight_device *bd = NULL;drivers/video/s5p_mipi_sample.c line 75
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-0+ + ? ? 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?
drivers/video/s5p_mipi_sample.c line 84
Quoted 9 lines
drivers/video/s5p_mipi_sample.c new file +9
-0+ ? ? 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:
drivers/video/s5p_mipi_sample.c line 87
Quoted 3 lines
drivers/video/s5p_mipi_sample.c new file +3
-0+ ? ? bd->props.max_brightness = MAX_BRIGHTNESS; + ? ? bd->props.brightness = MAX_BRIGHTNESS; +here.
backlight_device_register()returns anERR_PTR(), so you will want to do anIS_ERR()check, which you can then map back withPTR_ERR()for a more precise idea of why it failed.drivers/video/s5p_mipi_sample.c line 91
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+ ? ? /* 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.
drivers/video/s5p_mipi_sample.c line 97
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+ ? ? mdelay(lcd->ddi_pd->reset_delay); + + ? ? /* lcd reset */ + ? ? if (lcd->ddi_pd->lcd_reset) + ? ? ? ? ? ? lcd->ddi_pd->lcd_reset(NULL); +Reset can fail?
drivers/video/s5p_mipi_sample.c line 53
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+ ? ? 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.drivers/video/s5p_mipi_sample.c line 168
Quoted 10 lines
drivers/video/s5p_mipi_sample.c new file +10
-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); +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.
drivers/video/s5p_mipi_sample.c
Quoted 4 lines
drivers/video/s5p_mipi_sample.c new file +4
-0+static struct mipi_lcd_driver sample_mipi_driver = { + ? ? .name = "sample", + + ? ? .probe = sample_probe,No remove?
drivers/video/s5p_mipi_sample.c line 209
Quoted 11 lines
drivers/video/s5p_mipi_sample.c new file +11
-0+ ? ? .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_initshould be__initannotated.drivers/video/s5p_mipi_sample.c line 214
Quoted 5 lines
drivers/video/s5p_mipi_sample.c new file +5
-0+static void sample_exit (void ) +{ + ? ? return; +} +This should be balanced with a
? ? ? ?s5p_mipi_unregister_lcd_driver(&sample_mipi_driver);
drivers/video/s5p_mipi_sample.c line 220
Quoted 6 lines
drivers/video/s5p_mipi_sample.c new file +6
-0+module_init (sample_init); +module_exit (sample_exit); + +MODULE_AUTHOR ("Inki Dae <[email protected]>" ); +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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel