Re: [PATCH v6 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

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

On 05/10/2022 12:05, Niedermayr, BENEDIKT wrote:
> On Fri, 2022-09-30 at 22:01 +0200, Krzysztof Kozlowski wrote:
>> On 30/09/2022 21:42, Rob Herring wrote:
>>> On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote:
>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>
>>>> The GPMC controller has the ability to configure the polarity for the
>>>> wait pin. The current properties do not allow this configuration.
>>>> This binding directly configures the WAITPIN<X>POLARITY bit
>>>> in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity
>>>> dt-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..477189973334 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:
>>>
>>> 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'.
>>>
>>>> +    description: |
>>>> +      Set the desired polarity for the selected wait pin.
>>>> +      1 for active low, 0 for active high.
>>>
>>> Well that looks backwards. I assume from the commit msg above, it's the 
>>> register value, but that's not what the description says. Please go with 
>>> the logical state here and do the inversion in the driver.
>>
>> This was actually my suggestion to keep the same value as
>> ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines.
>>
> Ok, but how to proceed know? IMHO it makes more sense to use the value which actually lands in the register since most 
> people will use the value found in the Datasheet. 
> 
> We already had a discussion with Roger about the GPIO bindings vs. wait-pin binding. The point was that we do not use GPIO bindings
> in this case, or? 

Go with Rob's suggestion, so back to previous version.

Best regards,
Krzysztof