Re: PATCH] backlight: add lms501kf03 LCD driver

["devendra.aaru" <devendra.aaru@gmail.com>]

Hello,

> +static int lms501kf03_spi_write(struct lms501kf03 *lcd, unsigned char address,
> +                               unsigned char command)
> +{
> +       int ret;
> +
> +       ret = lms501kf03_spi_write_byte(lcd, address, command);
> +
> +       return ret;

there is redundancy here,
you can do just removing the ret and do return.

> +}
> +
> +static int lms501kf03_panel_send_sequence(struct lms501kf03 *lcd,
> +                                       const unsigned short *wbuf)
> +{
> +       int ret = 0, i = 0;
> +
> +       while (wbuf[i] != ENDDEF) {
> +               if (i == 0)
> +                       ret = lms501kf03_spi_write(lcd, COMMAND_ONLY, wbuf[i]);
> +               else
> +                       ret = lms501kf03_spi_write(lcd, DATA_ONLY, wbuf[i]);
> +               if (ret)
> +                       break;
> +               i += 1;
> +       }
> +
> +       return ret;
> +}
> +
> +static int lms501kf03_ldi_init(struct lms501kf03 *lcd)
> +{
> +       int ret, i;
> +       static const unsigned short *init_seq[] = {
> +               seq_password,
> +               seq_power,
> +               seq_display,
> +               seq_rgb_if,
> +               seq_display_inv,
> +               seq_vcom,
> +               seq_gate,
> +               seq_panel,
> +               seq_col_mod,
> +               seq_w_gamma,
> +               seq_rgb_gamma,
> +               seq_sleep_out,
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(init_seq); i++) {
> +               ret = lms501kf03_panel_send_sequence(lcd, init_seq[i]);
> +               if (ret)
> +                       break;
> +       }
> +       /* according to the datasheet, 120ms delay time is required. */
why the 120ms delay required would be good to specify as comment. or
you can put the link to datasheet if available.

> +       msleep(120);
> +
> +       return ret;
> +}
> +
> +static int lms501kf03_ldi_enable(struct lms501kf03 *lcd)
> +{
> +       return lms501kf03_panel_send_sequence(lcd, seq_display_on);
> +}
> +
> +static int lms501kf03_ldi_disable(struct lms501kf03 *lcd)
> +{
> +       return lms501kf03_panel_send_sequence(lcd, seq_display_off);
> +}
> +
> +static int lms501kf03_power_is_on(int power)
> +{
> +       return (power) <= FB_BLANK_NORMAL;
> +}
> +
> +static int lms501kf03_power_on(struct lms501kf03 *lcd)
> +{
> +       int ret = 0;
> +       struct lcd_platform_data *pd;
> +
> +       pd = lcd->lcd_pd;
> +
> +       if (!pd->power_on) {
> +               dev_err(lcd->dev, "power_on is NULL.\n");
> +               return -EFAULT;
we may need to do -EINVAL instead of EFAULT as EFAULT tends to be for
the invalid memory addresses.

> +       } else {
> +               pd->power_on(lcd->ld, 1);
> +               msleep(pd->power_on_delay);
> +       }
> +
> +       if (!pd->reset) {
> +               dev_err(lcd->dev, "reset is NULL.\n");
> +               return -EFAULT;

may be here too..

> +       } else {
> +               pd->reset(lcd->ld);
> +               msleep(pd->reset_delay);
> +       }
> +
> +       ret = lms501kf03_ldi_init(lcd);
> +       if (ret) {
> +               dev_err(lcd->dev, "failed to initialize ldi.\n");
> +               return ret;
> +       }
> +
> +       ret = lms501kf03_ldi_enable(lcd);
> +       if (ret) {
> +               dev_err(lcd->dev, "failed to enable ldi.\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int lms501kf03_power_off(struct lms501kf03 *lcd)
> +{
> +       int ret = 0;
> +       struct lcd_platform_data *pd;
> +
> +       pd = lcd->lcd_pd;
> +
> +       ret = lms501kf03_ldi_disable(lcd);
> +       if (ret) {
> +               dev_err(lcd->dev, "lcd setting failed.\n");
> +               return -EIO;
> +       }
> +
> +       msleep(pd->power_off_delay);
> +
> +       pd->power_on(lcd->ld, 0);
> +

seems that you are calling the core lcd framework api, i am curious to
know why, :p , and obviously i dunno why its calling that way.

> +       return 0;
> +}
> +
> +static int lms501kf03_power(struct lms501kf03 *lcd, int power)
> +{
> +       int ret = 0;
> +
> +       if (lms501kf03_power_is_on(power) &&
> +               !lms501kf03_power_is_on(lcd->power))
> +               ret = lms501kf03_power_on(lcd);
> +       else if (!lms501kf03_power_is_on(power) &&
> +               lms501kf03_power_is_on(lcd->power))
> +               ret = lms501kf03_power_off(lcd);
> +

seems that ret is used unitialised in this function bug is covered by
ret = 0 assignment, but why that else case is missed, or i am just
misreading?

> +       if (!ret)
> +               lcd->power = power;
> +
> +       return ret;
> +}
> +

[snip]
> +
> +static int lms501kf03_probe(struct spi_device *spi)
> +{
> +       struct lms501kf03 *lcd = NULL;
> +       struct lcd_device *ld = NULL;
> +       int ret = 0;
> +
> +       lcd = devm_kzalloc(&spi->dev, sizeof(struct lms501kf03), GFP_KERNEL);
> +       if (!lcd)
> +               return -ENOMEM;
> +

glad you used devm api.

> +       /* lms501kf03 lcd panel uses 3-wire 9-bit SPI Mode. */
> +       spi->bits_per_word = 9;
> +
> +       ret = spi_setup(spi);
> +       if (ret < 0) {
> +               dev_err(&spi->dev, "spi setup failed.\n");
> +               return ret;
> +       }
> +
> +       lcd->spi = spi;
> +       lcd->dev = &spi->dev;
> +
> +       lcd->lcd_pd = spi->dev.platform_data;
> +       if (!lcd->lcd_pd) {
> +               dev_err(&spi->dev, "platform data is NULL\n");
> +               return -EFAULT;
May be here too -EINVAL or some correct errno should be returned not
the EFAULT no?

> +       }
> +
> +       ld = lcd_device_register("lms501kf03", &spi->dev, lcd,
> +                               &lms501kf03_lcd_ops);
> +       if (IS_ERR(ld))
> +               return PTR_ERR(ld);
> +
> +       lcd->ld = ld;
> +

[snip]

> +#if defined(CONFIG_PM)
> +static unsigned int before_power;
> +
> +static int lms501kf03_suspend(struct spi_device *spi, pm_message_t mesg)
> +{
> +       int ret = 0;
> +       struct lms501kf03 *lcd = dev_get_drvdata(&spi->dev);
> +
> +       dev_dbg(&spi->dev, "lcd->power = %d\n", lcd->power);
> +
> +       before_power = lcd->power;
> +
> +       /*
> +        * when lcd panel is suspend, lcd panel becomes off
> +        * regardless of status.
> +        */
> +       ret = lms501kf03_power(lcd, FB_BLANK_POWERDOWN);
> +

returning ret is redundant just like that return that is mentioned
above. you can return as is.

> +       return ret;
> +}
> +
> +static int lms501kf03_resume(struct spi_device *spi)
> +{
> +       int ret = 0;
> +       struct lms501kf03 *lcd = dev_get_drvdata(&spi->dev);
> +
> +       lcd->power = FB_BLANK_POWERDOWN;
> +
> +       ret = lms501kf03_power(lcd, FB_BLANK_UNBLANK);
> +
> +       return ret;
> +}

here too..
> +#else
> +#define lms501kf03_suspend     NULL
> +#define lms501kf03_resume      NULL
> +#endif
> +
> +void lms501kf03_shutdown(struct spi_device *spi)
> +{
> +       struct lms501kf03 *lcd = dev_get_drvdata(&spi->dev);
> +
> +       lms501kf03_power(lcd, FB_BLANK_POWERDOWN);
> +}
> +
> +static struct spi_driver lms501kf03_driver = {
> +       .driver = {
> +               .name   = "lms501kf03",
> +               .owner  = THIS_MODULE,
> +       },
> +       .probe          = lms501kf03_probe,
> +       .remove         = lms501kf03_remove,
> +       .shutdown       = lms501kf03_shutdown,
> +       .suspend        = lms501kf03_suspend,
> +       .resume         = lms501kf03_resume,
> +};
> +
> +module_spi_driver(lms501kf03_driver);
> +

nice, using module_spi_driver reduce the duplication of the
spi_register unregister code.