> -----Original Message-----
> From: Valkeinen, Tomi
> Sent: Thursday, May 12, 2011 8:40 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 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 <mythr...@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()?
The matching API for i2c_add_device is i2c_del_device.
And i2c_unregister_device is a matching API for i2c_new_device.
i2c_del_device (a call present in picodlp_exit) would call i2c_remove and then
i2c_remove has a call to i2c_unregister_device which would unregister the
i2c_device.
So panel_remove should also have a call to i2c_unregister_device.
This would solve the problem.
>
> Tomi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
