RE: [PATCH-V2] OMAP3EVM: Made backlight GPIO default state to off

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Varadarajan, Charulatha [mailto:charu@xxxxxx]
> Sent: Tuesday, January 25, 2011 10:24 AM
> To: Hiremath, Vaibhav
> Cc: Semwal, Sumit; linux-omap@xxxxxxxxxxxxxxx; tony@xxxxxxxxxxx;
> tomi.valkeinen@xxxxxxxxx
> Subject: Re: [PATCH-V2] OMAP3EVM: Made backlight GPIO default state to off
> 
> On Tue, Jan 25, 2011 at 10:18, Hiremath, Vaibhav <hvaibhav@xxxxxx> wrote:
> >
> >> -----Original Message-----
> >> From: Semwal, Sumit
> >> Sent: Tuesday, January 25, 2011 8:05 AM
> >> To: Hiremath, Vaibhav
> >> Cc: linux-omap@xxxxxxxxxxxxxxx; tony@xxxxxxxxxxx;
> tomi.valkeinen@xxxxxxxxx
> >> Subject: Re: [PATCH-V2] OMAP3EVM: Made backlight GPIO default state to
> off
> >>
> >> Vaibhav,
> >>
> >> On Tue, Jan 25, 2011 at 12:52 AM, Vaibhav Hiremath <hvaibhav@xxxxxx>
> >> wrote:
> >> > If you choose default output to DVI, the LCD backlight used to
> >> > stay on, since panel->disable function never gets called.
> >> >
> >> > So, during init put backlight GPIO to off state and the driver
> >> > code will decide which output to enable.
> >> >
> >> > Signed-off-by: Vaibhav Hiremath <hvaibhav@xxxxxx>
> >> > ---
> >> > Changes from V1 -
> >> >        - Added check for return value
> >> >
> >> >  arch/arm/mach-omap2/board-omap3evm.c |   15 +++++++++++++--
> >> >  1 files changed, 13 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-
> >> omap2/board-omap3evm.c
> >> > index 7119380..a888a7d 100644
> >> > --- a/arch/arm/mach-omap2/board-omap3evm.c
> >> > +++ b/arch/arm/mach-omap2/board-omap3evm.c
> >> > @@ -411,6 +411,8 @@ static struct platform_device leds_gpio = {
> >> >  static int omap3evm_twl_gpio_setup(struct device *dev,
> >> >                unsigned gpio, unsigned ngpio)
> >> >  {
> >> > +       int r;
> >> > +
> >> >        /* gpio + 0 is "mmc0_cd" (input/IRQ) */
> >> >        omap_mux_init_gpio(63, OMAP_PIN_INPUT);
> >> >        mmc[0].gpio_cd = gpio + 0;
> >> > @@ -426,8 +428,17 @@ static int omap3evm_twl_gpio_setup(struct device
> >> *dev,
> >> >         */
> >> >
> >> >        /* TWL4030_GPIO_MAX + 0 == ledA, LCD Backlight control */
> >> > -       gpio_request(gpio + TWL4030_GPIO_MAX, "EN_LCD_BKL");
> >> > -       gpio_direction_output(gpio + TWL4030_GPIO_MAX, 0);
> >> > +       r = gpio_request(gpio + TWL4030_GPIO_MAX, "EN_LCD_BKL");
> >> > +       if (r)
> >> > +               printk(KERN_ERR "failed to get lcd_bkl gpio\n");
> >> So even if the gpio_request fails, this code prints an error and
> >> continues?
> > [Hiremath, Vaibhav] yes, that's correct and intended.
> 
> As pointed out by Sumit, you should not continue modifying the
> direction of a gpio whose request failed (may be some other module
> is using this gpio)? Please mention why this is intentionally done so.
> 
[Hiremath, Vaibhav] First of all, if you look at implementation of gpio_xxx, it does handle all these scenarios gracefully. 

And second point is, the request is happening for backlight gpio (which is not something MUST required), so even if we fail to get handle here I think we should not return an error, just flag the warning and continue.

Let me understand, why do you want to return from middle of function, for backlight gpio request, which means we will not call "platform_device_register" for leds_gpio and many other things. And also if you look at the twl4030-gpio.c, we do same thing -


        } else if (pdata->setup) {
                int status;

                status = pdata->setup(&pdev->dev,
                                pdata->gpio_base, TWL4030_GPIO_MAX);
                if (status)
                        dev_dbg(&pdev->dev, "setup --> %d\n", status);
        }
	 return ret;

I hope this clarifies your doubts.

Thanks,
Vaibhav
> >
> > Thanks,
> > Vaibhav
> >> > +
> >> > +       if (get_omap3_evm_rev() >= OMAP3EVM_BOARD_GEN_2)
> >> > +               r = gpio_direction_output(gpio + TWL4030_GPIO_MAX,
> 1);
> >> > +       else
> >> > +               r = gpio_direction_output(gpio + TWL4030_GPIO_MAX,
> 0);
> >> > +
> >> > +       if (r)
> >> > +               printk(KERN_ERR "failed to set direction of lcd_bkl
> >> gpio\n");
> >> >
> >> >        /* gpio + 7 == DVI Enable */
> >> >        gpio_request(gpio + 7, "EN_DVI");
> >> > --
> >> > 1.6.2.4
> >> >
> >> > --
> >> > 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
> >> >
> > --
> > 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
> >
--
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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux