Messages in this thread | | | From | Brian Norris <> | Date | Fri, 26 Apr 2024 15:08:06 -0700 | Subject | Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support |
| |
Hi Johan, Bjorn,
On Thu, Apr 25, 2024 at 5:02 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Apr 24, 2024 at 08:45:31PM -0700, Bjorn Andersson wrote: > > When a GPIO is configured as OPEN_DRAIN gpiolib will in > > gpiod_direction_output() attempt to configure the open-drain property of > > the hardware and if this fails fall back to software emulation of this > > state. > > > > The TLMM block in most Qualcomm platform does not implement such > > functionality, so this call would be expected to fail. But due to lack > > of checks for this condition, the zero-initialized od_bit will cause > > this request to silently corrupt the lowest bit in the config register > > (which typically is part of the bias configuration) and happily continue > > on.
Apologies if I broke something here. Both the pinctrl subsystem and the wide world of diverse QCOM chips can be complicated beasts. I definitely could have missed things along the way. (And on first glance, it seems like you may have found one. I definitely did not consider the gpiod_direction_output() "emulation" behavior here when submitting this.)
But I can't tell based on subsequent conversation: are you observing a real problem, or is this a theoretical one that only exists if the gpiochip driver adds set_config() support?
> > Fix this by checking if the od_bit value is unspecified and if so fail > > the request to avoid the unexpected state, and to make sure the software > > fallback actually kicks in. > > Fortunately, this is currently not a problem as the gpiochip driver does > not implement the set_config() callback, which means that the attempt to > change the pin configuration currently always fails with -ENOTSUP (see > gpio_do_set_config()). > > Specifically, this means that the software fallback kicks in, which I > had already verified. > > Now, perhaps there is some other path which can allow you to end up > here, but it's at least not via gpiod_direction_output(). > > The msm pinctrl binding does not allow 'drive-open-drain' so that path > should also be ok unless you have a non-conformant devicetree.
The ipq4019 binding does: https://git.kernel.org/linus/99d19f5a48ee6fbc647935de458505e9308078e3
This is used in OpenWrt device trees.
> > It is assumed for now that no implementation will come into existence > > with BIT(0) being the open-drain bit, simply for convenience sake. > > > > Fixes: 13355ca35cd1 ("pinctrl: qcom: ipq4019: add open drain support") > > I guess hardware open-drain mode has never been properly tested on > ipq4019.
It was quite some time ago that I wrote and tested this, and per the above, I easily could have missed things. (Plus, the open drain configuration may not have much practical effect on the systems in question, so certain errors may not even be observable.)
But I do recall seeing the code in question activate. And inspection shows that the pinconf_apply_setting() -> ... msm_config_group_set() path is non-dead code here, for appropriate device trees.
I can try to fire up my development devices again and see what's up if that helps, but I won't have time to do that in the next few days.
Brian
| |