From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Brian Norris <computersforpeace@gmail.com>
Cc: Johan Hovold <johan@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Jaiganesh Narayanan <njaigane@codeaurora.org>,
Doug Anderson <dianders@chromium.org>,
<linux-arm-msm@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support
Date: Fri, 26 Apr 2024 16:43:54 -0700 [thread overview]
Message-ID: <Ziw8OrNS55AtyDkI@hu-bjorande-lv.qualcomm.com> (raw)
In-Reply-To: <CAN8TOE_Vd9c2eYgomhu_ukofTeO9eK8Yhrtt-8BQckmJnGfj6w@mail.gmail.com>
On Fri, Apr 26, 2024 at 03:08:06PM -0700, Brian Norris wrote:
> 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.
False alarm on the breakage part, I got lost in the software layers.
> 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?
>
There is a problem that if a non-ipq4019 device where to be pinconf'ed
for open-drain, the outcome would be unexpected and I have a concern
that someone one day would implement set_config().
So, I'd like to fix this, but my argumentation is at least wrong.
> > > 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
>
Perhaps we could convert that to yaml?
> This is used in OpenWrt device trees.
>
Thanks, I couldn't find a user, so this was helpful input for deciding
the path forward.
> > > 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.
>
Thank you for taking a look, Brian. This was valuable input. I will
rework this to have a valid motivation - at least.
> 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.
>
As my observation was incorrect, I don't think that is urgent.
Regards,
Bjorn
next prev parent reply other threads:[~2024-04-26 23:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 3:45 [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support Bjorn Andersson
2024-04-25 12:02 ` Johan Hovold
2024-04-25 13:47 ` Bjorn Andersson
2024-04-26 22:08 ` Brian Norris
2024-04-26 23:43 ` Bjorn Andersson [this message]
2024-04-27 6:18 ` Brian Norris
2024-05-03 7:28 ` Linus Walleij
2024-05-03 7:35 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ziw8OrNS55AtyDkI@hu-bjorande-lv.qualcomm.com \
--to=quic_bjorande@quicinc.com \
--cc=andersson@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dianders@chromium.org \
--cc=johan@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=njaigane@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.