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
