On 20/04/2023 15:00, Mark Brown wrote: > On Thu, Apr 20, 2023 at 02:30:17PM +0200, Krzysztof Kozlowski wrote: >> On 20/04/2023 13:58, Mark Brown wrote: >>> On Thu, Apr 20, 2023 at 12:16:12PM +0200, Krzysztof Kozlowski wrote: > >>>> - gpio_direction_output(wcd938x->reset_gpio, 0); >>>> - /* 20us sleep required after pulling the reset gpio to LOW */ >>>> + gpiod_set_value_cansleep(wcd938x->reset_gpio, 1); >>>> + /* 20us sleep required after asserting the reset gpio */ > >>> This is inverting the sense of the GPIO in the API from active low to >>> active high which will mean we're introducing a new reliance on having >>> the signal described as active low in DT. That's an ABI concern. > >> It's bringing it to the correct level. Old code was not respecting the >> DTS thus if such DTS came with inverted design, the driver would not work. > > Sure, but OTOH if the user didn't bother specifying as active low it > would work. I suspect it's more likely that someone missed a flag that > had no practical impact in DT than that someone would add an inverter to > their design. > >> We were already fixing the upstream DTS users and I thought all of them >> are fixed since long time (half a year) or even correct from the >> beginning. Now I found one more case with incorrect level, which I will fix. > > That's just upstream, what about any downstream users? Life of downstream. We all know the drill: merge your DTS or suffer. The WCD938x codecs are moderately new, so I do not expect many downstream users. They are in theory possible, because driver was merged in v5.14-rc1 and for the newest products Qualcomm uses v5.15. Although now it is v5.15, but the time driver was merged, maybe it was v5.10. I could rework this patch to provide backwards compatible solution like I did for WSA: https://lore.kernel.org/all/20230102114152.297305-4-krzysztof.kozlowski@xxxxxxxxxx/ There are downsides of it, but as you pointed out - it's actually very rare to have the signal inverted in hardware. > >>> I remain deeply unconvinced that remapping active low outputs like this >>> in the GPIO API is helping. > >> The code is mapping them to correct state. The previous state was >> incorrect and did not allow to handle active high (which can happen). >> This is the effort to make code correct - driver and DTS. > > We could handle inversions through an explicit property if that were > needed, that would be a less problematic transition and clearer in the > consumer code. I am not sure if it is worth. The DTS is supposed to describe hardware, so even if reset pin flag was not effective, it is a mistake to describe it as ACTIVE_HIGH. Do we care about keeping broken code happy? If yes, then property is the way to go. If partially, then I can add backwards-compatible approach like I mentioned above. Best regards, Krzysztof
