Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/09/2022 11:21, Roger Quadros wrote:
> 
> 
> On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
>> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>>> Hi Benedikt,
>>>
>>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>
>>>> Add a new dt-binding for the wait-pin-polarity property
>>>>
>>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
>>>>>
>>>> ---
>>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7
>>>> +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-
>>>> controllers/ti,gpmc-child.yaml
>>>> b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> index 6e3995bb1630..7c721206f10b 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> @@ -230,6 +230,13 @@ properties:
>>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>>> waitpins".
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>  
>>>> +  gpmc,wait-pin-polarity:
>>>> +    description: |
>>>> +      Wait-pin polarity used by the clien. It relates to the pin
>>>> defined
>>>
>>> did you mean "client?"
>>> Can you please specify what value is for Active Low vs Active High?
>>
>> Yes, that makes sense. And yes I meant "client". My typo.....
>>>
>>>> +      with "gpmc,wait-pin".
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>> Why can't type be boolean?
>>
>> Of course we can use the boolean there. In that case I should give the
>> property a more meaningful name e.g. wait-pin-active-high or wait-pin-
>> active-low. 
>> Since the default behavour of this pin is Active High,
>> a bool property "gpmc,wait-pin-active-low" would make more sense for
>> backwards compatibility. 
>> If the property is missing, than the polarity stays on Active High like
>> before.
>>
> 
> OK, in that case you don't have to clarify the polarity in description.

I don't understand (and it is not explained in commit msg), why do you
need such property instead of using standard GPIO flags.

The driver should use standard GPIO descriptor and standard bindings. If
it cannot, this has to be explained.

Best regards,
Krzysztof