Re: [Linux-fbdev-devel] [RFC 2.6.27 2/2] mach-pxa: add AM300 platform driver

[Jaya K. <jay...@gm...>]

On Tue, Dec 23, 2008 at 4:24 AM, Lothar Waßmann <LW...@ka...> wrote:
> Hi,
>
> Eric Miao writes:
>> On Sun, Dec 21, 2008 at 6:59 PM, Jaya Kumar <jay...@gm...> wrote:
>> > This patch adds support for the AM300 platform driver.
>> >
>>
>> Patch looks good, I feel I'm nit-picking. Anyway , see my comments below:
>>
> [...]
>> > +static int am300_wait_for_rdy(struct broadsheetfb_par *par)
>> > +{
>> > +       unsigned long flags;
>> > +       DEFINE_WAIT(wait);
>> > +
>> > +       spin_lock_irqsave(par->lock, flags);
>> > +       while (!gpio_get_value(RDY_GPIO_PIN)) {
>> > +               prepare_to_wait(&par->waitq, &wait, TASK_INTERRUPTIBLE);
>                                                             ^^^^^^^^^^^^^
> This should be TASK_UNINTERRUPTIBLE, or the case of an interrupted
> wait need to be handled in some way!

Agreed. Will add handling for restarting.

>
>> wait_event() might be the perfect choice for the above case, unless your
>>
> ... which would also fix the above bug.
>

Yes, I had used wait_event in the previous version of am300epd that I
posted. In doing this, I encountered the problem that the ready
interrupt can occur between the point in time when wait_event performs
the gpio_get_value and when it puts the caller in the waitqueue. This
is why I switched to using the locking and granular wait.

Thanks,
jaya