Re: [PATCH v5 2/3] memory: omap-gpmc: add support for wait pin polarity

[Roger Quadros <rogerq@kernel.org>]

Hi,

On 20/09/2022 15:17, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote:
>> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote:
>>>> I commented exactly below the line which I question. I don't question
>>>> other lines. So let me be a bit more specific:
>>>>
>>>> Why do you need
>>>> "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
>>>> ? Can you write a scenario where this is useful?
>>>>
>>> Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
>>>
>>> If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
>>
>> Exactly this will happen. As expected. This value cannot appear in DTS,
>> therefore I would expect error message.
>>
>> Now you allow such value in DTS which is not the same as your bindings.
>>
> And now I completely got it...
> With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT...
> 
> Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional
> property anymore.
> If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed.
> Is this an acceptable solution for you?
> 

No this is not acceptable. As current implementations don't define polarity and rely on reset defaults.

You can check return value of "of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait_pin_polarity))"

" * Return: 0 on success, -EINVAL if the property does not exist,
 * -ENODATA if property does not have a value, and -EOVERFLOW if the
 * property data isn't large enough."

If property is present you don't need to check for WAITPINPOLARITY_DEFAULT as that is not valid value for this property.
If property is not present you force WAITPINPOLARITY_DEFAULT and don't print error message.


> 
>>
>>> But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from
>>> DT),
>>> and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
>>> So in gpmc_cs_program_settings() if:
>>>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
>>>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
>>>     p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register
>>>
>>> We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
>>> we might break platforms which rely on these reset values. 
>>
>> Best regards,
>> Krzysztof
> Cheers,
> benedikt
> 

cheers,
-roger