Re: [PATCH RFC] arm64: dts: allwinner: a64: teres-i: Enable audio

[Harald Geyer <harald@ccbib.org>]

Hi Maxime!

Maxime Ripard writes:
> On Wed, Feb 13, 2019 at 12:43:46PM +0100, Harald Geyer wrote:
> > Maxime Ripard writes:
> > > On Tue, Feb 12, 2019 at 08:37:36PM +0100, Harald Geyer wrote:
> > > > > There's a few issues with that approach as well:
> > > > > 
> > > > >   - We're actively trying to remove the pinctrl nodes for the GPIOs
> > > > 
> > > > For what reason? Maybe it doesn't apply to this usecase?
> > > 
> > > This is kind of separate. At the moment, on all our SoCs but the H6,
> > > requesting a pin to a separate state using pinctrl doesn't mark the
> > > GPIO muxed on that pin as reserved, so through the GPIO userspace
> > > interface (or calling gpio_request from within the kernel, but that
> > > seems less of a risk) anyone is free to just request a GPIO on a pin
> > > already requested, behind the consumer drivers' back. Which is pretty
> > > bad.
> > 
> > Really, I'm surprised. This is not the behaviour I remember from A20
> > and A64. Indeed, testing this on teres with the debug detect pin claimed
> > by audio, I get:
> > 
> > root@teres:/sys/kernel/debug/pinctrl/1f02c00.pinctrl# cat pinmux-pins
> > Pinmux settings per pin
> > Format: pin (name): mux_owner|gpio_owner (strict) hog?
> > pin 352 (PL0): device 1f03400.rsb function s_rsb group PL0
> > pin 353 (PL1): device 1f03400.rsb function s_rsb group PL1
> > pin 354 (PL2): GPIO 1f02c00.pinctrl:354
> > pin 355 (PL3): UNCLAIMED
> > pin 356 (PL4): UNCLAIMED
> > pin 357 (PL5): UNCLAIMED
> > pin 358 (PL6): UNCLAIMED
> > pin 359 (PL7): GPIO 1f02c00.pinctrl:359
> > pin 360 (PL8): GPIO 1f02c00.pinctrl:360
> > pin 361 (PL9): device sound function gpio_out group PL9
> > pin 362 (PL10): UNCLAIMED
> > pin 363 (PL11): UNCLAIMED
> > pin 364 (PL12): GPIO 1f02c00.pinctrl:364
> > [...]
> > 
> > root@teres:/sys/class/gpio# echo 361 >export
> > bash: echo: Schreibfehler: Das Argument ist ungültig.
> > 
> > So I can't access this from sysfs, even though the error code is a
> > bit odd: I'd expect EBUSY instead of EINVAL. I can export any of the
> > UNCLAIMED pins/gpios.
> > 
> > Trying with libgpiod as well, I see that the state of the pin is reported
> > incorretly, but I still can't access it:
> > 
> > gpiochip0 - 32 lines:
> >         line   0:      unnamed       unused   input  active-high
> >         line   1:      unnamed       unused   input  active-high
> >         line   2:      unnamed      "reset"  output   active-low [used]
> >         line   3:      unnamed       unused   input  active-high
> >         line   4:      unnamed       unused   input  active-high
> >         line   5:      unnamed       unused   input  active-high
> >         line   6:      unnamed       unused   input  active-high
> >         line   7:      unnamed  "usb1-vbus"  output  active-high [used]
> >         line   8:      unnamed "Lid Switch"   input   active-low [used]
> >         line   9:      unnamed       unused   input  active-high
> >         line  10:      unnamed      "sysfs"   input  active-high [used]
> >         line  11:      unnamed       unused   input  active-high
> >         line  12:      unnamed     "enable"  output  active-high [used]
> >         line  13:      unnamed       unused   input  active-high
> > 
> > root@teres:~# gpioget 1f02c00.pinctrl 9
> > gpioget: error reading GPIO values: Invalid argument
> > 
> > On a pin exported to sysfs I get EBUSY as expected:
> > root@teres:~# gpioget 1f02c00.pinctrl 10
> > gpioget: error reading GPIO values: Device or resource busy
> > 
> > And reading an unclaimed pin works as expected too:
> > root@teres:~# gpioget 1f02c00.pinctrl 11
> > 0
> > 
> > Either I misunderstood what you have written or it isn't true.
> 
> This happens when you have a pin requested in pinctrl, but for a
> function that isn't a GPIO, and you try to request the GPIO on that
> pin.

It's unclear what you mean: The sound pin is requested for function
"gpio_out".

> In you example, such a case can happen if you do sed
> s/364/361/. Since this is the PMIC, you should probably test this on
> some other device though :)

No, I can't verify that either:
root@teres:/sys/class/gpio# echo 364 >export
bash: echo: Schreibfehler: Das Gerät oder die Ressource ist belegt.
root@teres:/sys/class/gpio# gpioget 1f02c00.pinctrl 12
gpioget: error reading GPIO values: Device or resource busy

Both attempts give me EBUSY.

> > > There's support for such a check in pinctrl, and we did enable it for
> > > the H6. However, one of its side effect is that you can't have a
> > > pinctrl node for a GPIO anymore (at least without significantly
> > > reworking the GPIO API in the kernel).
> > 
> > Can you point me to some background reading?
> 
> Background reading for what?

Anything that helps me comprehend the situation really. Maybe a pointer
to the documentation of the check in pinctrl and it's side effect. Maybe
a pointer to the discussion of what reworking of the GPIO API would be
necessary. Maybe a pointer to the discussion where it was decided to
move away from pinctrl nodes for gpios for sunxi (or whatever was
decided).

Also since the problem, as you explain it, seems general, I wonder how
other platforms deal with it.

> > > We did enable it for the H6, since it didn't have any backward
> > > compatibility to take care of, but it's disabled at the moment for all
> > > the other SoCs to be able to flip that switch at some point. And
> > > that's why we're moving away from it as well.
> > 
> > Well ... that's good to know, because I have a couple of custom DTs
> > with pinctrl nodes for a GPIO. I think it should be documented as
> > deprecated in the binding then.
> 
> It's not documented anywhere that we need it.

I don't quite understand what you mean here.

As I see it, the patch I proposed (minus the output-high property, which
I think is orthogonal to this discussion) conforms to the binding as
it is documented right now. 

But you tell me, that it is using a feature, that is actually kind of
deprecated. 

I believe it should be possible to tell if a DT is valid, just be looking
at the binding documentation. Without looking on the linux source code
or other side channel information.

As you write below, people are putting DTs into ROM. Which means that
IMO the DTs should actually be OS independent, so that people can use
different OSes on their equipment. This in turn requires the binding
documentation to be comprehensive.

So please update the DT bindings of allwinner,pinctrl* to mark all
features as deprecated that won't pass your review.

> > Also I wonder how I can select drive strength or bias on a gpio line when
> > I can't use pinctrl with them anymore.
> 
> That's one of the items we need to take care of as well, yes, but that
> can be handled through a GPIO flag in the descriptor.
> 
> There's a series currently taking care of the bias:
> https://www.spinics.net/lists/linux-gpio/msg36444.html

Thanks for the pointer, this was interesting.

However the cover letter clearly states that this feature is only for
simple GPIO controllers without any pinmux capabilities.

Also the newly introduced binding states that this is only for simple
bias without explicit resistor value. Which I guess is true for sunxi
ATM, but doesn't seem future proof.

And even if you add a similar ABI for drive strength, you still couldn't
support gpio consumers, that want to set different pin configurations
like "default", "idle", "suspend".

This has become quite tangential to the issue at hand, but I don't see
how this "get rid of pinctrl for gpios" agenda can actually work in a
general and portable way.

> > > > But we need a way to control the mux from userspace and aside from
> > > > unbinding the ideas proposed thus far are:
> > > >
> > > > a) control the gpio directly
> > > > b) control the gpio via leds-gpio
> > > > 
> > > > (a) was dismissed because we can't set a default from DT
> > > > (b) was dismissed because some rogue app might try to blink it
> > > > 
> > > > The clean solution might be to write mux-gpio, which is actually
> > > > identical to leds-gpio but lives in /sys/class/mux_switches/ and
> > > > uses different filenames. But that's going down the "invent a new
> > > > subsystem road", which I believe is overkill for what is a debugging
> > > > facility for a single board.
> > > 
> > > I still believe we should aim at supporting this through pinctrl, and
> > > adding an userspace API is definitely easier than a full subsystem.
> > 
> > Getting everybody to agree on a new API (especially a userspace ABI)
> > is a major headache (and rightly so, we want to get something right on
> > the first attempt that is going to stay around forever). I don't think
> > some quirky debugging feature is worth the effort.
> > 
> > And frankly I don't care much about audio on the teres. I started
> > working on this because I feel kind of responsible for keeping the
> > teres DT up-to-date with what the kernel can support. But if the
> > kernel can't support it ATM: so be it.
> > 
> > As a compromise I think we could add all the nodes to the DT but mark
> > their status as "disabled". That would help everybody wanting to enable
> > audio but still be technically correct.
> 
> I understand if you don't want to go after that goal yourself, but
> that doesn't sound practical either. Especially since the A64, more
> and more people are putting the DT in a ROM, so we can't just change
> it as we wish.

I don't follow you here at all:
1) We don't talk about generic A64. We only talk about TERES I, for
   which we know that the DT can always be updated.
2) Even if TERES I had a ROM, how is a node with status = "disabled"
   worse then no node at all. The OSes treat it exactly the same and
   people will never have audio either way.
3) Actually the nodes are there already anyway (because the stubs
   get provided by the A64 dtsi), we would only populate them with
   more accurate information regarding audio routing and such.

I suspect some distributions will pick up the patch I posted anyway, if
it doesn't get merged mainline. (This is how I forgot missing backlight
support - it just worked for too many people ...) If we ship
sun50i-a64-teres-i.dts with the proper nodes (but disabled), their
patches will be much easier to maintain as the official DT evolves.

Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via CLAM to xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
or via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel