| Message ID | 20141103143023.GA8260@kahuna |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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)