| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <004601cddcd7$c571eeb0$5055cc10$%han@samsung.com>
Date: Tue, 18 Dec 2012 13:25:55 +0900
From: Jingoo Han <jg1.han@...sung.com>
To: "'devendra.aaru'" <devendra.aaru@...il.com>
Cc: 'Andrew Morton' <akpm@...ux-foundation.org>,
'LKML' <linux-kernel@...r.kernel.org>,
'Richard Purdie' <rpurdie@...ys.net>,
'Jingoo Han' <jg1.han@...sung.com>
Subject: Re: PATCH] backlight: add lms501kf03 LCD driver
On Tuesday, December 18, 2012 1:51 AM, devendra.aaru wrote
> 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.
OK. I will fix it.
>
> > +}
> > +
> > +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.
OK, I will add more explanation.
However, link to datasheet is not 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.
OK. I will fix it.
>
> > + } 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..
OK. I will fix it.
>
> > + } 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?
else case is as follows:
+ else
+ return = 0;
So, else case is not necessary.
>
> > + 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?
OK, I will use -EINVAL.
>
> > + }
> > +
> > + 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.
OK. I will fix it.
>
> > + 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..
OK. I will fix it.
> > +#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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists