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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
> + *
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@xxxxxxxxxxx>");
> +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.
--
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
