| Message ID | CAFqH_517U6vUz7adwaF4h78wvvwPf3v0CYMw93_aFY7_=an1Xg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2013-12-10 12:56, Enric Balletbo Serra wrote: > Hi all, > > 2013/12/9 Javier Martinez Canillas <javier@dowhile0.org>: >> Hi Tomi, >> >> On Mon, Dec 9, 2013 at 4:30 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>> On 2013-12-09 17:09, Javier Martinez Canillas wrote: >>>> Hi Tomi, >>>> >>>> On Mon, Dec 9, 2013 at 1:56 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: >>>>> On 2013-12-06 10:57, Javier Martinez Canillas wrote: >>>>> >>>>>>> + tfp410: encoder@0 { >>>>>>> + compatible = "ti,tfp410"; >>>>>>> + gpios = <&gpio1 0 0>; /* 0, power-down */ >>>>>>> + >>>>>> >>>>>> Please use the constants from include/dt-bindings/ instead of magic >>>>>> numbers, i.e: >>>>>> >>>>>> gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; /* 0, power-down */ >>>>> >>>>> Thanks, fixed now (for all .dts files) >>>>> >>>>> However... The TFP410 gpio is "power-down". I think we should actually >>>>> mark it as GPIO_ACTIVE_LOW, as setting it to 0 powers down the device. >>>>> >>>> >>>> yes, I looked at the TFP410 datasheet [0] and the Power Down pin is >>>> indeed an active-low, I just replaced to GPIO_ACTIVE_HIGH since you >>>> were using a constant 0 and include/dt-bindings/gpio/gpio.h defines >>>> GPIO_ACTIVE_HIGH as 0. >>>> >>>> I just asked to Enric why we use GPIO_ACTIVE_HIGH for the PD pin on >>>> the IGEPv2 DTS instad and is because the IGEP board uses a hardware >>>> signal inverter but that is a special case. I don't know about the >>>> Panda board since I haven't looked at its datasheet. >>> >>> Oh. Does it work on igep? The TFP410 driver always handles the PD GPIO >>> as it were active-low. The flag is ignored. >>> >> >> How weird, it does work on the IGEPv2 but you are right I just looked >> at at drivers/video/omap2/displays-new/encoder-tfp410.c and I see >> that it indeed just does: >> >> r = devm_gpio_request_one(&pdev->dev, ddata->pd_gpio, >> GPIOF_OUT_INIT_LOW, "tfp410 PD"); >> >> So I don't know how it is working... I'm on the road and won't have >> access to my IGEPv2 to dig further on this. Maybe Enric can shed more >> light on this. >> > > On IGEPv2 the GPIO that controls the power-down pin is connected > through a dual/buffer driver [1]. This driver is only a buffer, not > inverts the signal (I had told you wrong, sorry Javier ), so the pin > continues being active low. > > As both of you pointed the driver ignores the flag to handle the PD > GPIO, so doesn't matter if in the device tree we put GPIO_ACTIVE_HIGH > or GPIO_ACTIVE_LOW, so simply it works. About the patch to support > display for IGEP, to be coherent, the gpio should be defined as > GPIO_ACTIVE_LOW not GPIO_ACTIVE_HIGH. I have tested, and of course, > works. > > > diff --git a/arch/arm/boot/dts/omap3-igep0020.dts > b/arch/arm/boot/dts/omap3-igep0020.dts > index 2569d60..d185e06 100644 > --- a/arch/arm/boot/dts/omap3-igep0020.dts > +++ b/arch/arm/boot/dts/omap3-igep0020.dts > @@ -233,7 +233,7 @@ > > tfp410: encoder@0 { > compatible = "ti,tfp410"; > - gpios = <&gpio6 10 GPIO_ACTIVE_HIGH>; /* 170, power-down */ > + gpios = <&gpio6 10 GPIO_ACTIVE_LOW>; /* 170, power-down */ > > ports { > #address-cells = <1>; > > > [1] http://www.ti.com/product/sn74lvc2g07 Ok, looks good. I have changed the TFP gpios to active-low in my series for all .dts files, which includes the igep0020.dts. Tomi
diff --git a/arch/arm/boot/dts/omap3-igep0020.dts b/arch/arm/boot/dts/omap3-igep0020.dts index 2569d60..d185e06 100644 --- a/arch/arm/boot/dts/omap3-igep0020.dts +++ b/arch/arm/boot/dts/omap3-igep0020.dts @@ -233,7 +233,7 @@ tfp410: encoder@0 { compatible = "ti,tfp410"; - gpios = <&gpio6 10 GPIO_ACTIVE_HIGH>; /* 170, power-down */ + gpios = <&gpio6 10 GPIO_ACTIVE_LOW>; /* 170, power-down */ ports { #address-cells = <1>;