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@lina...
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