Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>]

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@xxxxxxxxxx> 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