Re: [PATCH v2] pinctrl: dra: dt-bindings: Fix output pull up/down

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

 



On Mon, Nov 3, 2014 at 9:07 AM, Roger Quadros <rogerq@xxxxxx> 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@xxxxxx>
>>>>> ---
>>>>>  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
--
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