Re: [PATCH] video: add ili922x lcd driver

[Anatolij Gustschin <agust@denx.de>]

On Mon, 04 Feb 2013 14:45:51 +0900
Jingoo Han <jg1.han@samsung.com> wrote:
...
> > diff --git a/drivers/video/backlight/ili922x.c b/drivers/video/backlight/ili922x.c
> > new file mode 100644
> > index 0000000..18c33df
> > --- /dev/null
> > +++ b/drivers/video/backlight/ili922x.c
> > @@ -0,0 +1,586 @@
> > +/*
> > + * (C) Copyright 2008
> > + * Stefano Babic, DENX Software Engineering, sbabic@denx.de.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> 
> Please remove this comment. It is hard to keep track of the address of
> Free Software Foundation.
> Also, above mentioned address is not the same with the current address.

okay, will do.

> > + *
> > + * This driver implements a lcd device for the ILITEK 922x display
> > + * controller. The interface to the display is SPI and the display's
> > + * memory is cyclically updated
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/string.h>
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
> > +#include <linux/fb.h>
> > +#include <linux/init.h>
> > +#include <linux/lcd.h>
> > +#include <linux/of.h>
> > +#include <linux/spi/spi.h>
> 
> Would you order inclusions of <linux/xxx.h> according to alphabetical
> ordering, for readability?

I'll do it in the next patch version.


...
> > +	xfer.bits_per_word = 8;
> > +	xfer.len = NUM_DUMMY_BYTES;
> > +	spi_message_add_tail(&xfer, &m);
> > +	ret = spi_sync(spi, &m);
> > +
> > +	udelay(10);
> 
> How about replacing udelay() with usleep_range()?

as I see now the send_dummy() is a nop, I'll remove it entirely.


...
> > +static u16 read_status(struct spi_device *spi)
> > +{
> > +	struct spi_message m;
> 
> Would you replace 'm' with 'msg' or 'message' for the readability?
> It's too short, even though 'm' is used in include/linux/spi/spi.h.
> 
> 	struct spi_message msg;

okay.

...
> > +static int read_reg(struct spi_device *spi, u8 reg, u16 *rx)
> > +{
> > +	struct spi_message m;
> > +	struct spi_transfer xfer_regindex, xfer_regvalue;
> > +	unsigned char tbuf[CMD_BUFSIZE];
> > +	unsigned char rbuf[CMD_BUFSIZE];
> > +	int ret = 0, len = 0, i, send_bytes;
> > +
> > +	send_dummy(spi);
> > +
> > +	memset(&xfer_regindex, 0, sizeof(struct spi_transfer));
> > +	memset(&xfer_regvalue, 0, sizeof(struct spi_transfer));
> > +	spi_message_init(&m);
> > +	xfer_regindex.tx_buf = tbuf;
> > +	xfer_regindex.rx_buf = rbuf;
> > +	xfer_regindex.cs_change = 1;
> > +	CHECK_FREQ_REG(spi, &xfer_regindex);
> > +
> > +	tbuf[0] = set_tx_byte(START_BYTE(ili922x_id, START_RS_INDEX,
> > +					 START_RW_WRITE));
> > +	tbuf[1] = set_tx_byte(0);
> > +	tbuf[2] = set_tx_byte(reg);
> > +	xfer_regindex.bits_per_word = 8;
> > +	len = xfer_regindex.len = 3;
> > +	spi_message_add_tail(&xfer_regindex, &m);
> > +
> > +	send_bytes = len;
> > +
> > +	tbuf[len++] = set_tx_byte(START_BYTE(ili922x_id, START_RS_REG,
> > +					     START_RW_READ));
> > +	for (i = len; i < CMD_BUFSIZE; i++)
> > +		tbuf[i] = set_tx_byte(0);	/* dummy */
> > +
> > +	xfer_regvalue.cs_change = 1;
> > +	xfer_regvalue.len = 4;
> 
> I don't understand why length 4 is necessary.
> In my opinion, length 3 seems to be enough.
> - tbuf[4] is used for sending 'START_BYTE(ili922x_id, START_RS_REG, START_RW_READ)'.
> - rbuf[5] and rbuf[6] are used for receiving value as below.
>    *rx = (rbuf[1 + send_bytes] << 8) + rbuf[2 + send_bytes];
> 
> However, tbuf[7] or rbuf[7] seems to be unnecessary.
> If I'm wrong, please let me know kindly.

will fix, thanks.

...
> > +static void ili922x_reg_dump(struct spi_device *spi)
> > +{
> > +	u8 reg;
> > +	u16 rx;
> > +
> > +	pr_info("ILI922x configuration registers:\n");
> 
> Please replace pr_info() with dev_info() as below.
> 
> 	dev_err(&spi->dev, "ILI922x configuration registers:\n");

okay, I'll use dev_dbg().

> 
> > +	for (reg = REG_START_OSCILLATION;
> > +	     reg <= REG_OTP_PROGRAMMING_ID_KEY; reg++) {
> > +		read_reg(spi, reg, &rx);
> > +		pr_info("reg @ 0x%02X: 0x%04X\n", reg, rx);
> 
> Same as above.

will change.


...
> > +static int ili922x_poweron(struct spi_device *spi)
> > +{
> > +	int ret = 0;
> 
> Initialization is not necessary.
> Just declare it as below:
> 	int ret;

okay.

> > +
> > +	/* Power on */
> > +	ret = write_reg(spi, REG_POWER_CONTROL_1, 0x0000);
> > +	mdelay(10);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0000);
> > +	mdelay(40);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_4, 0x0000);
> > +	mdelay(40);
> > +	ret += write_reg(spi, 0x56, 0x080F);
> 
> Would you replace this hard-coded address with the bit definition?

It is an undocumented register. I do not know what to use for the
bit definition. Do you know where this register is documented?
Otherwise I'll add a comment that it is not documented.

...
> > +	ret += write_reg(spi, REG_POWER_CONTROL_1, 0x4240);
> > +	mdelay(10);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0014);
> > +	mdelay(40);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_4, 0x1319);
> > +	mdelay(40);
> 
> How about replacing mdelay() with msleep()?

will fix.

...
> > +static int ili922x_poweroff(struct spi_device *spi)
> > +{
> > +	int ret = 0;
> 
> Initialization is not necessary.
> Just declare it as below:
> 	int ret;
> 
> 
> > +
> > +	/* Power off */
> > +	ret = write_reg(spi, REG_POWER_CONTROL_1, 0x0000);
> > +	mdelay(10);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_2, 0x0000);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_3, 0x0000);
> > +	mdelay(40);
> > +	ret += write_reg(spi, REG_POWER_CONTROL_4, 0x0000);
> > +	mdelay(40);
> 
> Same as above.

...
> > +static void ili922x_display_init(struct spi_device *spi)
> > +{
> > +	write_reg(spi, REG_START_OSCILLATION, 1);
> > +	mdelay(10);
> 
> Same as above.

okay, will change.

Thanks,

Anatolij