> -----Original Message----- > From: Valkeinen, Tomi > Sent: Tuesday, April 19, 2011 5:23 PM > To: Janorkar, Mayuresh > Cc: linux-omap@xxxxxxxxxxxxxxx; K, Mythri P > Subject: Re: [PATCH 5/7] OMAP: DSS: Adding initialization routine to > picodlp panel > > On Mon, 2011-04-18 at 11:45 +0530, Mayuresh Janorkar wrote: > > From: Mythri P K <mythripk@xxxxxx> > > > > picodlp_i2c_client needs to send commands over i2c as a part of > initialiazation. > > system controller dlp pico processor dpp2600 is used. > > It configures the splash screen of picodlp using a sequence of commands. > > A programmer's guide is available at: > > http://focus.ti.com/lit/ug/dlpu002a/dlpu002a.pdf > > > > API is defined for sending command over i2c as an i2c_write operation. > > > > Signed-off-by: Mythri P K <mythripk@xxxxxx> > > Signed-off-by: Mayuresh Janorkar <mayur@xxxxxx> > > --- > > drivers/video/omap2/displays/panel-picodlp.c | 317 > ++++++++++++++++++++++++++ > > 1 files changed, 317 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/video/omap2/displays/panel-picodlp.c > b/drivers/video/omap2/displays/panel-picodlp.c > > index e361674..785e406 100644 > > --- a/drivers/video/omap2/displays/panel-picodlp.c > > +++ b/drivers/video/omap2/displays/panel-picodlp.c > > @@ -32,7 +32,15 @@ > > #include <plat/display.h> > > #include <plat/panel-picodlp.h> > > > > +#include "panel-picodlp.h" > > + > > #define DRIVER_NAME "picodlp_i2c_driver" > > + > > +/* This defines the minit of data which is allowed into single write > block */ > > +#define MAX_I2C_WRITE_BLOCK_SIZE 32 > > +#define PICO_MAJOR 1 /* 2 bits */ > > +#define PICO_MINOR 1 /* 2 bits */ > > + > > struct picodlp_data { > > struct mutex lock; > > struct i2c_client *picodlp_i2c_client; > > @@ -50,6 +58,11 @@ struct i2c_device_id picodlp_i2c_id[] = { > > { "picodlp_i2c_driver", 0 }, > > }; > > > > +struct picodlp_i2c_command { > > + u8 reg; > > + u32 value; > > +}; > > + > > static struct omap_video_timings pico_ls_timings = { > > .x_res = 864, > > .y_res = 480, > > @@ -70,6 +83,305 @@ static inline struct picodlp_panel_data > > return (struct picodlp_panel_data *) dssdev->data; > > } > > > > +static int picodlp_i2c_write_block(struct i2c_client *client, > > + u8 *data, int len) > > +{ > > + struct i2c_msg msg; > > + int i, r; > > + > > + struct picodlp_i2c_data *picodlp_i2c_data = > i2c_get_clientdata(client); > > + > > + if (len < 1 || len > MAX_I2C_WRITE_BLOCK_SIZE) { > > + dev_err(&client->dev, > > + "too long syn_write_block len %d\n", len); > > + return -EIO; > > + } > > + > > + mutex_lock(&picodlp_i2c_data->xfer_lock); > > + > > + msg.addr = client->addr; > > + msg.flags = 0; > > + msg.len = len; > > + msg.buf = data; > > + > > + r = i2c_transfer(client->adapter, &msg, 1); > > + mutex_unlock(&picodlp_i2c_data->xfer_lock); > > + > > + if (r == 1) { > > + for (i = 0; i < len; i++) > > + dev_dbg(&client->dev, > > + "addr %x bw 0x%02x[%d]: 0x%02x\n", > > + client->addr, data[0] + i, i, data[i]); > > + return 0; > > + } > > + > > + dev_err(&client->dev, " picodlp_i2c_write error\n"); > > + return r; > > This is, at least to my eyes, a bit uncommon way to do this. Usually the > error path is handled inside an if(), and the ok path is at the end of > the function. Would change this as per the most used formats. > > > +} > > + int r; > > + static const struct picodlp_i2c_command init_cmd_set1[] = { > > + {SOFT_RESET, 1}, {DMD_PARK_TRIGGER, 1}, > > + {MISC_REG, (PICO_MAJOR << 2 | PICO_MINOR)}, {SEQ_CONTROL, 0}, > > + {SEQ_VECTOR, 0x100}, {DMD_BLOCK_COUNT, 7}, > > + {DMD_VCC_CONTROL, 0x109}, {DMD_PARK_PULSE_COUNT, 0xA}, > > + {DMD_PARK_PULSE_WIDTH, 0xB}, {DMD_PARK_DELAY, 0x2ED}, > > + {DMD_SHADOW_ENABLE, 0}, {FLASH_OPCODE, 0xB}, > > + {FLASH_DUMMY_BYTES, 1}, {FLASH_ADDR_BYTES, 3}, > > + }; > > It would make these command lists clearer if there was only one command > per line. Fine > > > + > > + if (r) > > + return r; > > + > > + /* display logo splash image */ > > + r = picodlp_dpp2600_config_splash(client); > > + if (r) > > + return r; > > So this will show some hardcoded splash screen? How much of the code > could be removed if the splash screen feature is removed? I don't see > much point in such a feature, and it looks to me that at least two of > the functions above are just for the splash screen. It's just extra code > for a feature nobody wants. Yes, this would show a hardcoded splash screen. > > > + r = picodlp_i2c_write_array(client, init_cmd_set3, > > + ARRAY_SIZE(init_cmd_set3)); > > + if (r) > > + return r; > > + > > + r = picodlp_dpp2600_config_rgb(client); > > + if (r) > > + return r; > > + > > + return 0; > > +} > > + > > static int picodlp_i2c_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -134,6 +446,11 @@ static int picodlp_panel_power_on(struct > omap_dss_device *dssdev) > > picodlp_i2c_data = > > i2c_get_clientdata(picod->picodlp_i2c_client); > > > > + msleep(700); /* sleep till panel is settled */ > > And another huge sleep. This, unlike the other sleeps, make some sense, > as there's an i2c transaction done below. > > Somehow I get the feeling that you've just put big sleeps here and there > until the driver started working... Can you point me to the > documentation that describes the delays required? Except msleep(5) in /* transfer control to flash controller */ r = picodlp_i2c_write(client, PBC_CONTROL, 1); msleep(5); r = picodlp_i2c_write(client, PBC_CONTROL, 0); Other delays are not documented. But it is practically observed that we need to wait otherwise the i2c_packet does not succeed. As I mentioned earlier, I can follow one approach, in i2c_write see if packet transfer happens. If not, sleep for some time and retry. This approach would remove these delays from this place. > > + r = picodlp_i2c_init(picod->picodlp_i2c_client); > > + if (r) > > + goto err; > > + > > return r; > > err: > > if (dssdev->platform_disable) > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
