* [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down @ 2014-10-31 10:52 Roger Quadros 2014-10-31 13:50 ` Nishanth Menon 2014-11-03 10:09 ` [PATCH v2] " Roger Quadros 0 siblings, 2 replies; 11+ messages in thread From: Roger Quadros @ 2014-10-31 10:52 UTC (permalink / raw) To: tony; +Cc: nm, balbi, linux-omap, devicetree, Roger Quadros For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the PULL_DIS bit which disables the PULLs. While at that get rid for the PULL_ENA defination. It is misleading as there is no PULL enable bit in the register. And a zero bit defination makes no sense. Fixes: 23d9cec07c58 (:pinctrl: dra: dt-bindings: Fix pull enable/disable") Signed-off-by: Roger Quadros <rogerq@ti.com> --- include/dt-bindings/pinctrl/dra.h | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h index 3d33794..71098e4 100644 --- a/include/dt-bindings/pinctrl/dra.h +++ b/include/dt-bindings/pinctrl/dra.h @@ -30,7 +30,6 @@ #define MUX_MODE14 0xe #define MUX_MODE15 0xf -#define PULL_ENA (0 << 16) #define PULL_DIS (1 << 16) #define PULL_UP (1 << 17) #define INPUT_EN (1 << 18) @@ -40,12 +39,12 @@ /* Active pin states */ #define PIN_OUTPUT (0 | PULL_DIS) -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) +#define PIN_OUTPUT_PULLUP (PULL_UP) +#define PIN_OUTPUT_PULLDOWN (0) #define PIN_INPUT (INPUT_EN | PULL_DIS) #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) -#define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) -#define PIN_INPUT_PULLDOWN (PULL_ENA | INPUT_EN) +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) +#define PIN_INPUT_PULLDOWN (INPUT_EN) #endif -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-10-31 10:52 [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down Roger Quadros @ 2014-10-31 13:50 ` Nishanth Menon 2014-11-03 9:28 ` Roger Quadros 2014-11-03 10:09 ` [PATCH v2] " Roger Quadros 1 sibling, 1 reply; 11+ messages in thread From: Nishanth Menon @ 2014-10-31 13:50 UTC (permalink / raw) To: Roger Quadros; +Cc: tony, balbi, linux-omap, devicetree On 12:52-20141031, Roger Quadros wrote: > For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the > PULL_DIS bit which disables the PULLs. > > While at that get rid for the PULL_ENA defination. It is misleading > as there is no PULL enable bit in the register. And a zero bit defination > makes no sense. It was done to make it readable. Pull is enabled when that bit is 0 and disabled when that bit is 1. it is counter intutive enough for a macro to be defined. I suggest retaining that and not mixing with the current patch. > > Fixes: 23d9cec07c58 (:pinctrl: dra: dt-bindings: Fix pull enable/disable") s/:pinctrl/"pinctrl/ ? > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > include/dt-bindings/pinctrl/dra.h | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h > index 3d33794..71098e4 100644 > --- a/include/dt-bindings/pinctrl/dra.h > +++ b/include/dt-bindings/pinctrl/dra.h > @@ -30,7 +30,6 @@ > #define MUX_MODE14 0xe > #define MUX_MODE15 0xf > > -#define PULL_ENA (0 << 16) > #define PULL_DIS (1 << 16) > #define PULL_UP (1 << 17) > #define INPUT_EN (1 << 18) > @@ -40,12 +39,12 @@ > > /* Active pin states */ > #define PIN_OUTPUT (0 | PULL_DIS) > -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) > -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) > +#define PIN_OUTPUT_PULLUP (PULL_UP) > +#define PIN_OUTPUT_PULLDOWN (0) > #define PIN_INPUT (INPUT_EN | PULL_DIS) > #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) > -#define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) > -#define PIN_INPUT_PULLDOWN (PULL_ENA | INPUT_EN) > +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) > +#define PIN_INPUT_PULLDOWN (INPUT_EN) > > #endif > > -- > 1.8.3.2 > -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-10-31 13:50 ` Nishanth Menon @ 2014-11-03 9:28 ` Roger Quadros 0 siblings, 0 replies; 11+ messages in thread From: Roger Quadros @ 2014-11-03 9:28 UTC (permalink / raw) To: Nishanth Menon; +Cc: tony, balbi, linux-omap, devicetree On 10/31/2014 03:50 PM, Nishanth Menon wrote: > On 12:52-20141031, Roger Quadros wrote: >> For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the >> PULL_DIS bit which disables the PULLs. >> >> While at that get rid for the PULL_ENA defination. It is misleading >> as there is no PULL enable bit in the register. And a zero bit defination >> makes no sense. > It was done to make it readable. Pull is enabled when that bit is 0 and > disabled when that bit is 1. it is counter intutive enough for a macro But you can't enable the PULL by ORing it. you have to AND it's inverted version with the rest. It is confusing because it doesn't follow the logic of the other bit masks. > to be defined. I suggest retaining that and not mixing with the current > patch. Agreed. I'll revise this patch to not touch that macro. cheers, -roger > > >> >> Fixes: 23d9cec07c58 (:pinctrl: dra: dt-bindings: Fix pull enable/disable") > s/:pinctrl/"pinctrl/ ? > >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> include/dt-bindings/pinctrl/dra.h | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >> index 3d33794..71098e4 100644 >> --- a/include/dt-bindings/pinctrl/dra.h >> +++ b/include/dt-bindings/pinctrl/dra.h >> @@ -30,7 +30,6 @@ >> #define MUX_MODE14 0xe >> #define MUX_MODE15 0xf >> >> -#define PULL_ENA (0 << 16) >> #define PULL_DIS (1 << 16) >> #define PULL_UP (1 << 17) >> #define INPUT_EN (1 << 18) >> @@ -40,12 +39,12 @@ >> >> /* Active pin states */ >> #define PIN_OUTPUT (0 | PULL_DIS) >> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >> +#define PIN_OUTPUT_PULLUP (PULL_UP) >> +#define PIN_OUTPUT_PULLDOWN (0) >> #define PIN_INPUT (INPUT_EN | PULL_DIS) >> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >> -#define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >> -#define PIN_INPUT_PULLDOWN (PULL_ENA | INPUT_EN) >> +#define PIN_INPUT_PULLUP (INPUT_EN | PULL_UP) >> +#define PIN_INPUT_PULLDOWN (INPUT_EN) >> >> #endif >> >> -- >> 1.8.3.2 >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-10-31 10:52 [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down Roger Quadros 2014-10-31 13:50 ` Nishanth Menon @ 2014-11-03 10:09 ` Roger Quadros 2014-11-03 14:30 ` Nishanth Menon 2014-11-05 17:10 ` Nishanth Menon 1 sibling, 2 replies; 11+ messages in thread From: Roger Quadros @ 2014-11-03 10:09 UTC (permalink / raw) To: tony; +Cc: nm, balbi, linux-omap, devicetree, Roger Quadros For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the PULL_DIS bit which disables the PULLs. PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't use it in the PIN_OUTPUT_PULLUP/DOWN macros. Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") Signed-off-by: Roger Quadros <rogerq@ti.com> --- include/dt-bindings/pinctrl/dra.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h index 3d33794..7448edf 100644 --- a/include/dt-bindings/pinctrl/dra.h +++ b/include/dt-bindings/pinctrl/dra.h @@ -40,8 +40,8 @@ /* Active pin states */ #define PIN_OUTPUT (0 | PULL_DIS) -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) +#define PIN_OUTPUT_PULLUP (PULL_UP) +#define PIN_OUTPUT_PULLDOWN (0) #define PIN_INPUT (INPUT_EN | PULL_DIS) #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 10:09 ` [PATCH v2] " Roger Quadros @ 2014-11-03 14:30 ` Nishanth Menon 2014-11-03 14:44 ` Roger Quadros 2014-11-05 17:10 ` Nishanth Menon 1 sibling, 1 reply; 11+ messages in thread From: Nishanth Menon @ 2014-11-03 14:30 UTC (permalink / raw) To: Roger Quadros; +Cc: tony, balbi, linux-omap, devicetree On 12:09-20141103, Roger Quadros wrote: > For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the > PULL_DIS bit which disables the PULLs. > > PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't > use it in the PIN_OUTPUT_PULLUP/DOWN macros. > > Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > include/dt-bindings/pinctrl/dra.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h > index 3d33794..7448edf 100644 > --- a/include/dt-bindings/pinctrl/dra.h > +++ b/include/dt-bindings/pinctrl/dra.h > @@ -40,8 +40,8 @@ > > /* Active pin states */ > #define PIN_OUTPUT (0 | PULL_DIS) > -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) > -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) > +#define PIN_OUTPUT_PULLUP (PULL_UP) > +#define PIN_OUTPUT_PULLDOWN (0) > #define PIN_INPUT (INPUT_EN | PULL_DIS) > #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) > #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) You are right, we do have an issue with using PIN_OUTPUT along with remaining setting. For a moment, I wondered why input was not impacted - then I realized that INPUT_EN was being used instead of PIN_INPUT - following that convention. With the intent being explicitly using macros that clearly indicate what each setting combination is is, how about the following? diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h index 3d33794..d4037e7 100644 --- a/include/dt-bindings/pinctrl/dra.h +++ b/include/dt-bindings/pinctrl/dra.h @@ -34,14 +34,15 @@ #define PULL_DIS (1 << 16) #define PULL_UP (1 << 17) #define INPUT_EN (1 << 18) +#define OUTPUT_EN (0 << 18) #define SLEWCONTROL (1 << 19) #define WAKEUP_EN (1 << 24) #define WAKEUP_EVENT (1 << 25) /* Active pin states */ -#define PIN_OUTPUT (0 | PULL_DIS) -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) +#define PIN_OUTPUT (OUTPUT_EN | PULL_DIS) +#define PIN_OUTPUT_PULLUP (OUTPUT_EN | PULL_ENA | PULL_UP) +#define PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) #define PIN_INPUT (INPUT_EN | PULL_DIS) #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) -- Regards, Nishanth Menon ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 14:30 ` Nishanth Menon @ 2014-11-03 14:44 ` Roger Quadros 2014-11-03 14:59 ` Nishanth Menon 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2014-11-03 14:44 UTC (permalink / raw) To: Nishanth Menon; +Cc: tony, balbi, linux-omap, devicetree On 11/03/2014 04:30 PM, Nishanth Menon wrote: > On 12:09-20141103, Roger Quadros wrote: >> For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the >> PULL_DIS bit which disables the PULLs. >> >> PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't >> use it in the PIN_OUTPUT_PULLUP/DOWN macros. >> >> Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> include/dt-bindings/pinctrl/dra.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >> index 3d33794..7448edf 100644 >> --- a/include/dt-bindings/pinctrl/dra.h >> +++ b/include/dt-bindings/pinctrl/dra.h >> @@ -40,8 +40,8 @@ >> >> /* Active pin states */ >> #define PIN_OUTPUT (0 | PULL_DIS) >> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >> +#define PIN_OUTPUT_PULLUP (PULL_UP) >> +#define PIN_OUTPUT_PULLDOWN (0) >> #define PIN_INPUT (INPUT_EN | PULL_DIS) >> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) > > You are right, we do have an issue with using PIN_OUTPUT along with > remaining setting. > > For a moment, I wondered why input was not impacted - then I realized > that INPUT_EN was being used instead of PIN_INPUT - following that > convention. With the intent being explicitly using macros that > clearly indicate what each setting combination is is, how about the > following? > > > diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h > index 3d33794..d4037e7 100644 > --- a/include/dt-bindings/pinctrl/dra.h > +++ b/include/dt-bindings/pinctrl/dra.h > @@ -34,14 +34,15 @@ > #define PULL_DIS (1 << 16) > #define PULL_UP (1 << 17) > #define INPUT_EN (1 << 18) > +#define OUTPUT_EN (0 << 18) > #define SLEWCONTROL (1 << 19) > #define WAKEUP_EN (1 << 24) > #define WAKEUP_EVENT (1 << 25) > > /* Active pin states */ > -#define PIN_OUTPUT (0 | PULL_DIS) > -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) > -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) > +#define PIN_OUTPUT (OUTPUT_EN | PULL_DIS) > +#define PIN_OUTPUT_PULLUP (OUTPUT_EN | PULL_ENA | PULL_UP) > +#define PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) To me it adds more confusion and this change is a NOP as we're ORing 0 here with OUTPUT_EN. > #define PIN_INPUT (INPUT_EN | PULL_DIS) > #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) > #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) > cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 14:44 ` Roger Quadros @ 2014-11-03 14:59 ` Nishanth Menon 2014-11-03 15:07 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Nishanth Menon @ 2014-11-03 14:59 UTC (permalink / raw) To: Roger Quadros; +Cc: tony, balbi, linux-omap, devicetree On 11/03/2014 08:44 AM, Roger Quadros wrote: > On 11/03/2014 04:30 PM, Nishanth Menon wrote: >> On 12:09-20141103, Roger Quadros wrote: >>> For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the >>> PULL_DIS bit which disables the PULLs. >>> >>> PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't >>> use it in the PIN_OUTPUT_PULLUP/DOWN macros. >>> >>> Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") >>> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> include/dt-bindings/pinctrl/dra.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >>> index 3d33794..7448edf 100644 >>> --- a/include/dt-bindings/pinctrl/dra.h >>> +++ b/include/dt-bindings/pinctrl/dra.h >>> @@ -40,8 +40,8 @@ >>> >>> /* Active pin states */ >>> #define PIN_OUTPUT (0 | PULL_DIS) >>> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >>> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >>> +#define PIN_OUTPUT_PULLUP (PULL_UP) >>> +#define PIN_OUTPUT_PULLDOWN (0) >>> #define PIN_INPUT (INPUT_EN | PULL_DIS) >>> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >>> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >> >> You are right, we do have an issue with using PIN_OUTPUT along with >> remaining setting. >> >> For a moment, I wondered why input was not impacted - then I realized >> that INPUT_EN was being used instead of PIN_INPUT - following that >> convention. With the intent being explicitly using macros that >> clearly indicate what each setting combination is is, how about the >> following? >> >> >> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >> index 3d33794..d4037e7 100644 >> --- a/include/dt-bindings/pinctrl/dra.h >> +++ b/include/dt-bindings/pinctrl/dra.h >> @@ -34,14 +34,15 @@ >> #define PULL_DIS (1 << 16) >> #define PULL_UP (1 << 17) >> #define INPUT_EN (1 << 18) >> +#define OUTPUT_EN (0 << 18) >> #define SLEWCONTROL (1 << 19) >> #define WAKEUP_EN (1 << 24) >> #define WAKEUP_EVENT (1 << 25) >> >> /* Active pin states */ >> -#define PIN_OUTPUT (0 | PULL_DIS) >> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >> +#define PIN_OUTPUT (OUTPUT_EN | PULL_DIS) >> +#define PIN_OUTPUT_PULLUP (OUTPUT_EN | PULL_ENA | PULL_UP) >> +#define PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) > > To me it adds more confusion and this change is a NOP as we're ORing 0 here > with OUTPUT_EN. look at this this way: PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) should probably trigger in mind (what about PULLDOWN?) PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA | PULL_DOWN) then verify values of each OUTPUT_EN -> 0 in bit 18, ok, etc. if we ensure that PIN_XX macros use just the basic primitives, it is easier to prevent the mistake like the one I made. the other option of not defining macros whose values are 0 implies that the reviewer has to recheck against trm to ensure all the right "1" bits are set. just my view here. > >> #define PIN_INPUT (INPUT_EN | PULL_DIS) >> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >> > > cheers, > -roger > -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 14:59 ` Nishanth Menon @ 2014-11-03 15:07 ` Roger Quadros 2014-11-03 17:56 ` Nishanth Menon 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2014-11-03 15:07 UTC (permalink / raw) To: Nishanth Menon; +Cc: tony, balbi, linux-omap, devicetree On 11/03/2014 04:59 PM, Nishanth Menon wrote: > On 11/03/2014 08:44 AM, Roger Quadros wrote: >> On 11/03/2014 04:30 PM, Nishanth Menon wrote: >>> On 12:09-20141103, Roger Quadros wrote: >>>> For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the >>>> PULL_DIS bit which disables the PULLs. >>>> >>>> PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't >>>> use it in the PIN_OUTPUT_PULLUP/DOWN macros. >>>> >>>> Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> --- >>>> include/dt-bindings/pinctrl/dra.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >>>> index 3d33794..7448edf 100644 >>>> --- a/include/dt-bindings/pinctrl/dra.h >>>> +++ b/include/dt-bindings/pinctrl/dra.h >>>> @@ -40,8 +40,8 @@ >>>> >>>> /* Active pin states */ >>>> #define PIN_OUTPUT (0 | PULL_DIS) >>>> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >>>> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >>>> +#define PIN_OUTPUT_PULLUP (PULL_UP) >>>> +#define PIN_OUTPUT_PULLDOWN (0) >>>> #define PIN_INPUT (INPUT_EN | PULL_DIS) >>>> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >>>> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >>> >>> You are right, we do have an issue with using PIN_OUTPUT along with >>> remaining setting. >>> >>> For a moment, I wondered why input was not impacted - then I realized >>> that INPUT_EN was being used instead of PIN_INPUT - following that >>> convention. With the intent being explicitly using macros that >>> clearly indicate what each setting combination is is, how about the >>> following? >>> >>> >>> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >>> index 3d33794..d4037e7 100644 >>> --- a/include/dt-bindings/pinctrl/dra.h >>> +++ b/include/dt-bindings/pinctrl/dra.h >>> @@ -34,14 +34,15 @@ >>> #define PULL_DIS (1 << 16) >>> #define PULL_UP (1 << 17) >>> #define INPUT_EN (1 << 18) >>> +#define OUTPUT_EN (0 << 18) >>> #define SLEWCONTROL (1 << 19) >>> #define WAKEUP_EN (1 << 24) >>> #define WAKEUP_EVENT (1 << 25) >>> >>> /* Active pin states */ >>> -#define PIN_OUTPUT (0 | PULL_DIS) >>> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >>> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >>> +#define PIN_OUTPUT (OUTPUT_EN | PULL_DIS) >>> +#define PIN_OUTPUT_PULLUP (OUTPUT_EN | PULL_ENA | PULL_UP) >>> +#define PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) >> >> To me it adds more confusion and this change is a NOP as we're ORing 0 here >> with OUTPUT_EN. > > look at this this way: > > PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) > > should probably trigger in mind (what about PULLDOWN?) > > PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA | PULL_DOWN) > > then verify values of each OUTPUT_EN -> 0 in bit 18, ok, etc. > > > if we ensure that PIN_XX macros use just the basic primitives, it is > easier to prevent the mistake like the one I made. the other option of > not defining macros whose values are 0 implies that the reviewer has > to recheck against trm to ensure all the right "1" bits are set. > > > just my view here. Aren't the macros defining the bit positions not the actual enable or disable actions? If we go by what you said then you will have to add WAKEUP_DIS, SLEWCONTROL_DIS, WAKEUP_EVENT_DIS and so on. Which again makes no sense as you will have to define them to zero. cheers, -roger > >> >>> #define PIN_INPUT (INPUT_EN | PULL_DIS) >>> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >>> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >>> >> >> cheers, >> -roger >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 15:07 ` Roger Quadros @ 2014-11-03 17:56 ` Nishanth Menon 0 siblings, 0 replies; 11+ messages in thread From: Nishanth Menon @ 2014-11-03 17:56 UTC (permalink / raw) To: Roger Quadros; +Cc: Tony Lindgren, Balbi, Felipe, linux-omap, dt list On Mon, Nov 3, 2014 at 9:07 AM, Roger Quadros <rogerq@ti.com> wrote: > On 11/03/2014 04:59 PM, Nishanth Menon wrote: >> On 11/03/2014 08:44 AM, Roger Quadros wrote: >>> On 11/03/2014 04:30 PM, Nishanth Menon wrote: >>>> On 12:09-20141103, Roger Quadros wrote: >>>>> For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the >>>>> PULL_DIS bit which disables the PULLs. >>>>> >>>>> PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't >>>>> use it in the PIN_OUTPUT_PULLUP/DOWN macros. >>>>> >>>>> Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") >>>>> >>>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>>> --- >>>>> include/dt-bindings/pinctrl/dra.h | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >>>>> index 3d33794..7448edf 100644 >>>>> --- a/include/dt-bindings/pinctrl/dra.h >>>>> +++ b/include/dt-bindings/pinctrl/dra.h >>>>> @@ -40,8 +40,8 @@ >>>>> >>>>> /* Active pin states */ >>>>> #define PIN_OUTPUT (0 | PULL_DIS) >>>>> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >>>>> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >>>>> +#define PIN_OUTPUT_PULLUP (PULL_UP) >>>>> +#define PIN_OUTPUT_PULLDOWN (0) >>>>> #define PIN_INPUT (INPUT_EN | PULL_DIS) >>>>> #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) >>>>> #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) >>>> >>>> You are right, we do have an issue with using PIN_OUTPUT along with >>>> remaining setting. >>>> >>>> For a moment, I wondered why input was not impacted - then I realized >>>> that INPUT_EN was being used instead of PIN_INPUT - following that >>>> convention. With the intent being explicitly using macros that >>>> clearly indicate what each setting combination is is, how about the >>>> following? >>>> >>>> >>>> diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h >>>> index 3d33794..d4037e7 100644 >>>> --- a/include/dt-bindings/pinctrl/dra.h >>>> +++ b/include/dt-bindings/pinctrl/dra.h >>>> @@ -34,14 +34,15 @@ >>>> #define PULL_DIS (1 << 16) >>>> #define PULL_UP (1 << 17) >>>> #define INPUT_EN (1 << 18) >>>> +#define OUTPUT_EN (0 << 18) >>>> #define SLEWCONTROL (1 << 19) >>>> #define WAKEUP_EN (1 << 24) >>>> #define WAKEUP_EVENT (1 << 25) >>>> >>>> /* Active pin states */ >>>> -#define PIN_OUTPUT (0 | PULL_DIS) >>>> -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) >>>> -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) >>>> +#define PIN_OUTPUT (OUTPUT_EN | PULL_DIS) >>>> +#define PIN_OUTPUT_PULLUP (OUTPUT_EN | PULL_ENA | PULL_UP) >>>> +#define PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) >>> >>> To me it adds more confusion and this change is a NOP as we're ORing 0 here >>> with OUTPUT_EN. >> >> look at this this way: >> >> PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA) >> >> should probably trigger in mind (what about PULLDOWN?) >> >> PIN_OUTPUT_PULLDOWN (OUTPUT_EN | PULL_ENA | PULL_DOWN) >> >> then verify values of each OUTPUT_EN -> 0 in bit 18, ok, etc. >> >> >> if we ensure that PIN_XX macros use just the basic primitives, it is >> easier to prevent the mistake like the one I made. the other option of >> not defining macros whose values are 0 implies that the reviewer has >> to recheck against trm to ensure all the right "1" bits are set. >> >> >> just my view here. > > Aren't the macros defining the bit positions not the actual enable or > disable actions? > > If we go by what you said then you will have to add > WAKEUP_DIS, SLEWCONTROL_DIS, WAKEUP_EVENT_DIS and so on. > > Which again makes no sense as you will have to define them to zero. WAKEUP should not be defined anyways - pinctrl controls them - should probably remove them from header. but your point is probably a valid view w.r.t TI SoC bit definitions - we just define the "1" states in omap.h as well. -- --- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-03 10:09 ` [PATCH v2] " Roger Quadros 2014-11-03 14:30 ` Nishanth Menon @ 2014-11-05 17:10 ` Nishanth Menon 2014-11-10 22:29 ` Tony Lindgren 1 sibling, 1 reply; 11+ messages in thread From: Nishanth Menon @ 2014-11-05 17:10 UTC (permalink / raw) To: Roger Quadros, tony; +Cc: balbi, linux-omap, devicetree On 11/03/2014 04:09 AM, Roger Quadros wrote: > For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the > PULL_DIS bit which disables the PULLs. > > PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't > use it in the PIN_OUTPUT_PULLUP/DOWN macros. > > Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > include/dt-bindings/pinctrl/dra.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h > index 3d33794..7448edf 100644 > --- a/include/dt-bindings/pinctrl/dra.h > +++ b/include/dt-bindings/pinctrl/dra.h > @@ -40,8 +40,8 @@ > > /* Active pin states */ > #define PIN_OUTPUT (0 | PULL_DIS) > -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) > -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) > +#define PIN_OUTPUT_PULLUP (PULL_UP) > +#define PIN_OUTPUT_PULLDOWN (0) > #define PIN_INPUT (INPUT_EN | PULL_DIS) > #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) > #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) > There might be additional updates needed to DRA7 pinctrl sequencing requirements based on latest documentation: http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf#page=4004&zoom=auto,0,274.1 But, that has nothing to do with the bug that is currently being fixed in this patch,. So, for the record: Acked-by: Nishanth Menon <nm@ti.com> -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down 2014-11-05 17:10 ` Nishanth Menon @ 2014-11-10 22:29 ` Tony Lindgren 0 siblings, 0 replies; 11+ messages in thread From: Tony Lindgren @ 2014-11-10 22:29 UTC (permalink / raw) To: Nishanth Menon; +Cc: Roger Quadros, balbi, linux-omap, devicetree * Nishanth Menon <nm@ti.com> [141105 09:12]: > On 11/03/2014 04:09 AM, Roger Quadros wrote: > > For PIN_OUTPUT_PULLUP and PIN_OUTPUT_PULLDOWN we must not set the > > PULL_DIS bit which disables the PULLs. > > > > PULL_ENA is a 0 and using it in an OR operation is a NOP, so don't > > use it in the PIN_OUTPUT_PULLUP/DOWN macros. > > > > Fixes: 23d9cec07c58 ("pinctrl: dra: dt-bindings: Fix pull enable/disable") > > > > Signed-off-by: Roger Quadros <rogerq@ti.com> > > --- > > include/dt-bindings/pinctrl/dra.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/dt-bindings/pinctrl/dra.h b/include/dt-bindings/pinctrl/dra.h > > index 3d33794..7448edf 100644 > > --- a/include/dt-bindings/pinctrl/dra.h > > +++ b/include/dt-bindings/pinctrl/dra.h > > @@ -40,8 +40,8 @@ > > > > /* Active pin states */ > > #define PIN_OUTPUT (0 | PULL_DIS) > > -#define PIN_OUTPUT_PULLUP (PIN_OUTPUT | PULL_ENA | PULL_UP) > > -#define PIN_OUTPUT_PULLDOWN (PIN_OUTPUT | PULL_ENA) > > +#define PIN_OUTPUT_PULLUP (PULL_UP) > > +#define PIN_OUTPUT_PULLDOWN (0) > > #define PIN_INPUT (INPUT_EN | PULL_DIS) > > #define PIN_INPUT_SLEW (INPUT_EN | SLEWCONTROL) > > #define PIN_INPUT_PULLUP (PULL_ENA | INPUT_EN | PULL_UP) > > > > > There might be additional updates needed to DRA7 pinctrl sequencing > requirements based on latest documentation: > > http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf#page=4004&zoom=auto,0,274.1 > > But, that has nothing to do with the bug that is currently being fixed > in this patch,. > > So, for the record: > Acked-by: Nishanth Menon <nm@ti.com> OK applying into omap-for-v3.18/fixes thanks. Tony ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-10 22:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-31 10:52 [PATCH] pinctrl: dra: dt-bindings: Fix output pull up/down Roger Quadros 2014-10-31 13:50 ` Nishanth Menon 2014-11-03 9:28 ` Roger Quadros 2014-11-03 10:09 ` [PATCH v2] " Roger Quadros 2014-11-03 14:30 ` Nishanth Menon 2014-11-03 14:44 ` Roger Quadros 2014-11-03 14:59 ` Nishanth Menon 2014-11-03 15:07 ` Roger Quadros 2014-11-03 17:56 ` Nishanth Menon 2014-11-05 17:10 ` Nishanth Menon 2014-11-10 22:29 ` Tony Lindgren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).