RE: [PATCH v4 4/4] OMAP: DSS: Add picodlp panel driver

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

On Thu, 2011-05-12 at 20:25 +0530, Janorkar, Mayuresh wrote:
> 
> > -----Original Message-----
> > From: Valkeinen, Tomi
> > Sent: Thursday, May 12, 2011 7:58 PM
> > To: Janorkar, Mayuresh
> > Cc: linux-omap@vger.kernel.org; K, Mythri P
> > Subject: Re: [PATCH v4 4/4] OMAP: DSS: Add picodlp panel driver
> > 
> > On Tue, 2011-05-10 at 18:55 +0530, Mayuresh Janorkar wrote:
> > > picodlp panel driver is required for driving DLP dpp2600.
> > > It consists of a dss driver and an i2c_client.
> > >
> > > i2c_client is required for sending commands to dpp (DLP Pico Projector).
> > >
> > > Based on original design from Mythri P K <mythripk@ti.com>
> > >
> > > The power-up sequence consists of:
> > > Setting PWRGOOD to a logic low state. Once power is stable and thus the
> > ++++++++++++++++++++++++++
> > >  3 files changed, 622 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/video/omap2/displays/panel-picodlp.c
> 
> <snip>
> 
> > > +
> > > +#include <plat/display.h>
> > > +#include <plat/panel-picodlp.h>
> > > +
> > > +#include "panel-picodlp.h"
> > > +
> > > +#define DRIVER_NAME	"picodlp_i2c_driver"
> > > +
> > > +#define MAX_TRIAL_VALUE			100
> > 
> > I'll repeat my comment from previous review round:
> > 
> > The name of this define is not descriptive and you use this define in
> > two places which have nothing to do with each other. I think it's better
> > just to use the value where it's needed.
> 
> I think it looks more readable if we have a MACRO.
> But as they are used at only couple of places I would remove these MACROs.

Well, the problem with this macro is that it's very unclear. What does
max trial value mean? It doesn't define anything sensible, just a number
which doesn't mean anything without the code context.

If it was MAX_TRIAL_TIME_MS, telling the maximum time in milliseconds
that the code would wait, then it would be sensible.

Another problem is that you used the same macro in two different places,
which have nothing to do with each other. The other place requires a
wait of 500ms, if I recall right, and is related to the power up. The
other one is related to waiting for some DMA transfer inside pico to
finish, and this time is in microseconds or possibly few milliseconds if
I understood right.

It's not good to use the same define in such different places,
especially as it only defines a max loop number, so it depends on the
msleeps in the code.

> > 
> > > +struct picodlp_data {
> > > +	struct mutex lock;
> 
> 
> > > +static int picodlp_i2c_remove(struct i2c_client *client)
> > > +{
> > > +	struct picodlp_i2c_data *picodlp_i2c_data =
> > > +					i2c_get_clientdata(client);
> > > +
> > > +	kfree(picodlp_i2c_data);
> > > +	i2c_unregister_device(client);
> > 
> > You add the i2c device in the dss probe function, but unregister it in
> > i2c remove function. That's probably not right. These things should
> > usually be symmetric, and the unregister should be at the dss remove
> > function.
> > 
> Isnt it good to have a remove function removing i2c_client?

Well, when is picodlp_i2c_remove() called? Isn't it called when the i2c
client is being removed, i.e. when somebody has called
i2c_unregister_device()?

 Tomi