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

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

[Harald Geyer]

Hi Torsten!

Torsten Duwe writes:
> > hpvcc-supply vs. cpvdd-supply:
> > On the A64 manual the pin is called CPVDD and the binding documents
> > requires a cpvdd-supply property. However in the actual driver and
> > devicetrees so far hpvcc-supply is used. This is a very new binding,
> > so we have the luxury to decide either way, I think. Any input from
> > the devicetree maintainers would be appreciated.
> 
> I don't really have a strong opinion here, besides settling this ASAP,
> to minimise confusion.

Yeah, given that this issue was introduced only in 5.0 and 5.0 is
almost out of the door, I'd have believed people to be more eager to
get this fixed soon.

Well, if nobody speaks up, I'll just include a patch to change the
binding to match the source in the next version of my submission.

> > debug console multiplexing:
> > Olimex have a userspace script that controls gpio PL9 during boot,
> > to select between HP and serial console. I guess this is not acceptable
> > for mainline.
> 
> > The best solution I can see is to switch the HP jack from serial console
> > to audio once the audio drivers load. With this people can still capture
> > the bootlogs but everybody gets audio once the system is up and
> > switching back to console output is as simple as unloading the audio
> > drivers.
> 
> IMO it is really not the audio driver's business to mess with this switch.
> You could argue as well that the serial driver should get a flag to claim
> the HP jack, which would be similarily unjustified.

Not the audio driver's business, but perheps the audio card's DT node.
Which is why I came up with the pinctrl idea.

> My solution is to confine this choice inside U-Boot:
> in sun50i-a64-teres-i-u-boot.dtsi I put
> 
> / {
>         leds {
>                 compatible = "gpio-leds";
>                 /* Not really a LED, but the least intrusive workaround */
>                 audioconsole {
>                         label = "teres-i:audio:console";
>                         gpio = <&r_pio 0 9 GPIO_ACTIVE_LOW>; /* PL9 */
>                         default-state = "on";
>                 };
>         };
> [...]
> 
> and place a "gpio set PL9;" somewhere in the bootcmd_* logic. Otherwise there's
> a *lot* of chirping on the right ear during boot ;-)
> The switch is for early boot debugging, so it's best to have U-Boot enable it
> when required, and keep it off by default.

I agree that some quirk in u-boot will be required for those who want
to boot with headphones plugged in.

But I still think we need a proper description of the HW in the linux DT:
* When linux doesn't know about the pin at all, there are no guarantees
  that it won't accidentally touch the pin during some operations like
  suspend/resume, etc.
* There is the usecase where somebody puts the system console on ttyS1
  and uses ttyS0 to connect to some other board. (Actually I believe this
  is a quite likely usecase given Olimex' usual target market.) I'd like
  to support this.

Using something like your leds hack in the linux DT would be fine for
me, if the maintainers are willing to accept it.

As it is, I'm currently working on supporting "output-high" property
in pinctrl-sunxi.
 
> > Testing:
> > I don't have a headset with combo connector, so I could only test the
> > headphones output, but not the headset mic. If somebody happens to
> > have a TERES-I and a suitable headset, testing this would be nice.
> 
> The pinout required is the "CTIA" one, *not* OMTP (standards are great...) see
> https://source.android.com/devices/accessories/headset/plug-headset-spec#mechanical
> 
> I do have a compatible headset, but was neither able to record from the HS mic
> nor from the internal mic, so I have to try harder, I guess ;-) I could hear
> both mics when choosing the mixer as output source though. Mute and boost
> worked for both mics, in alsamixer.

Probably you need to enable more controls in alsamixer.
"AIF1 Data Digital ADC" comes to mind as a plausible candidate.

> Kernel is 4.20.6, so ca0412a05756cd0b
> is missing; which shouldn't make a difference in this respect, right?

I suppose not, but I didn't actually test audio on 4.20.

Thanks,
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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3148 bytes --]

On Mon, Feb 11, 2019 at 02:36:53PM +0100, Harald Geyer wrote:
> > > debug console multiplexing:
> > > Olimex have a userspace script that controls gpio PL9 during boot,
> > > to select between HP and serial console. I guess this is not acceptable
> > > for mainline.
> > 
> > > The best solution I can see is to switch the HP jack from serial console
> > > to audio once the audio drivers load. With this people can still capture
> > > the bootlogs but everybody gets audio once the system is up and
> > > switching back to console output is as simple as unloading the audio
> > > drivers.
> > 
> > IMO it is really not the audio driver's business to mess with this switch.
> > You could argue as well that the serial driver should get a flag to claim
> > the HP jack, which would be similarily unjustified.
> 
> Not the audio driver's business, but perheps the audio card's DT node.
> Which is why I came up with the pinctrl idea.

I know that the nexus7 is quite well supported, and iirc they were
using a similar trick on their UART. Have you looked at how they were
doing that?

> > My solution is to confine this choice inside U-Boot:
> > in sun50i-a64-teres-i-u-boot.dtsi I put
> > 
> > / {
> >         leds {
> >                 compatible = "gpio-leds";
> >                 /* Not really a LED, but the least intrusive workaround */
> >                 audioconsole {
> >                         label = "teres-i:audio:console";
> >                         gpio = <&r_pio 0 9 GPIO_ACTIVE_LOW>; /* PL9 */
> >                         default-state = "on";
> >                 };
> >         };
> > [...]
> > 
> > and place a "gpio set PL9;" somewhere in the bootcmd_* logic. Otherwise there's
> > a *lot* of chirping on the right ear during boot ;-)
> > The switch is for early boot debugging, so it's best to have U-Boot enable it
> > when required, and keep it off by default.
> 
> I agree that some quirk in u-boot will be required for those who want
> to boot with headphones plugged in.
> 
> But I still think we need a proper description of the HW in the linux DT:
> * When linux doesn't know about the pin at all, there are no guarantees
>   that it won't accidentally touch the pin during some operations like
>   suspend/resume, etc.
> * There is the usecase where somebody puts the system console on ttyS1
>   and uses ttyS0 to connect to some other board. (Actually I believe this
>   is a quite likely usecase given Olimex' usual target market.) I'd like
>   to support this.

Agreed.

> Using something like your leds hack in the linux DT would be fine for
> me, if the maintainers are willing to accept it.

Not really. The side-effects of that is that any user-space stack is
free to come by and treat it like an LED as well which would be quite
horrible as far as audio or UART reliability is concerned :)

We want to model this properly. I guess using a pinctrl driver
controlled through GPIO (similar to what regulator-gpio is) would be a
good first step.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

Maxime Ripard writes:
> On Mon, Feb 11, 2019 at 02:36:53PM +0100, Harald Geyer wrote:
> > > > debug console multiplexing:
> > > > Olimex have a userspace script that controls gpio PL9 during boot,
> > > > to select between HP and serial console. I guess this is not acceptable
> > > > for mainline.
> > > >
> > > > The best solution I can see is to switch the HP jack from serial console
> > > > to audio once the audio drivers load. With this people can still capture
> > > > the bootlogs but everybody gets audio once the system is up and
> > > > switching back to console output is as simple as unloading the audio
> > > > drivers.
> > >
> > > IMO it is really not the audio driver's business to mess with this switch.
> > > You could argue as well that the serial driver should get a flag to claim
> > > the HP jack, which would be similarily unjustified.
> >
> > Not the audio driver's business, but perheps the audio card's DT node.
> > Which is why I came up with the pinctrl idea.
> > 
> > I know that the nexus7 is quite well supported, and iirc they were using
> > a similar trick on their UART. Have you looked at how they were doing
> > that?

I didn't know that. Thanks for the info, it was quite interessting reading.
Alas it seems it doesn't help us:
1) I couldn't find proof that audio support is actually in mainline.
2) Their HW works differently: Their debug cable uses external voltage
on the mic pin to drive the switch. (They also have a GPIO, but it's
input to detect the debug adapter in software, not output as in the
case of the teres.)
 
> We want to model this properly. I guess using a pinctrl driver
> controlled through GPIO (similar to what regulator-gpio is) would be a
> good first step.

I considered this too, but didn't like it:

1) Seems like a bit of overkill.

2) The HW at hand is a rather different kind of multiplexer than
what pinctrl assumes. We don't want two mutually exclusive devices,
(Ie don't make the kernel unbind /dev/console for the sake of audio.)
but we want switch the jack between two devices, that might both be
active at the same time. This looks more like the channel multiplexers
used with many ADCs and such. I guess, I could start a new subsystem
around this. Seems like even more overkill.

Instead I just got the original patch working, by implementing
"output-high" DT property in sunxi-pinctrl. I'll send a patch for
review soon.

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

On Mon, Feb 11, 2019 at 08:32:35PM +0100, Harald Geyer wrote:
> > We want to model this properly. I guess using a pinctrl driver
> > controlled through GPIO (similar to what regulator-gpio is) would be a
> > good first step.
> 
> I considered this too, but didn't like it:
> 
> 1) Seems like a bit of overkill.
>
> 2) The HW at hand is a rather different kind of multiplexer than
> what pinctrl assumes. We don't want two mutually exclusive devices,
> (Ie don't make the kernel unbind /dev/console for the sake of audio.)
> but we want switch the jack between two devices, that might both be
> active at the same time. This looks more like the channel multiplexers
> used with many ADCs and such. I guess, I could start a new subsystem
> around this. Seems like even more overkill.

I'm not quite sure about how that's different from what pinctrl
assumes. pinctrl assumes to handle devices that have multiple signals
as input, and one as output. Isn't that exactly what you have?

And pinctrl can be used dynamically as well if you need to

> Instead I just got the original patch working, by implementing
> "output-high" DT property in sunxi-pinctrl. I'll send a patch for
> review soon.

What do you want to do with output-high exactly?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

Maxime Ripard writes:
> On Mon, Feb 11, 2019 at 08:32:35PM +0100, Harald Geyer wrote:
> > > We want to model this properly. I guess using a pinctrl driver
> > > controlled through GPIO (similar to what regulator-gpio is) would be a
> > > good first step.
> > 
> > I considered this too, but didn't like it:
> > 
> > 1) Seems like a bit of overkill.
> >
> > 2) The HW at hand is a rather different kind of multiplexer than
> > what pinctrl assumes. We don't want two mutually exclusive devices,
> > (Ie don't make the kernel unbind /dev/console for the sake of audio.)
> > but we want switch the jack between two devices, that might both be
> > active at the same time. This looks more like the channel multiplexers
> > used with many ADCs and such. I guess, I could start a new subsystem
> > around this. Seems like even more overkill.
> 
> I'm not quite sure about how that's different from what pinctrl
> assumes. pinctrl assumes to handle devices that have multiple signals
> as input, and one as output. Isn't that exactly what you have?

I think the pinctrl way would be to have the audio card device
request the HP jack and the uart node request the HP jack and only
once device could probe successfully. Ie it is about ressource
allocation, not true multiplexing where both devices can use the
ressource at the same time. Am I wrong?

Or course we don't actually want true multiplexing for audio quality
reasons, but I don't see how we could use pinctrl without doing nasty
things to /dev/console ...
 
> And pinctrl can be used dynamically as well if you need to

Can you explain or point me to the relevant explanation in the docs?
I don't seem to know about it.

> > Instead I just got the original patch working, by implementing
> > "output-high" DT property in sunxi-pinctrl. I'll send a patch for
> > review soon.
> 
> What do you want to do with output-high exactly?

Exactly what I do in the patch that started this thread.
(I'll resend when wens' cpvdd patch is available for me to rebase onto.)

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3352 bytes --]

On Tue, Feb 12, 2019 at 10:42:03AM +0100, Harald Geyer wrote:
> Maxime Ripard writes:
> > On Mon, Feb 11, 2019 at 08:32:35PM +0100, Harald Geyer wrote:
> > > > We want to model this properly. I guess using a pinctrl driver
> > > > controlled through GPIO (similar to what regulator-gpio is) would be a
> > > > good first step.
> > > 
> > > I considered this too, but didn't like it:
> > > 
> > > 1) Seems like a bit of overkill.
> > >
> > > 2) The HW at hand is a rather different kind of multiplexer than
> > > what pinctrl assumes. We don't want two mutually exclusive devices,
> > > (Ie don't make the kernel unbind /dev/console for the sake of audio.)
> > > but we want switch the jack between two devices, that might both be
> > > active at the same time. This looks more like the channel multiplexers
> > > used with many ADCs and such. I guess, I could start a new subsystem
> > > around this. Seems like even more overkill.
> > 
> > I'm not quite sure about how that's different from what pinctrl
> > assumes. pinctrl assumes to handle devices that have multiple signals
> > as input, and one as output. Isn't that exactly what you have?
> 
> I think the pinctrl way would be to have the audio card device
> request the HP jack and the uart node request the HP jack and only
> once device could probe successfully. Ie it is about ressource
> allocation, not true multiplexing where both devices can use the
> ressource at the same time. Am I wrong?

By default, it's what happens yes, but you can definitely have more
complex behaviour that would support changing dynamically back and
forth that muxing.

> Or course we don't actually want true multiplexing for audio quality
> reasons, but I don't see how we could use pinctrl without doing nasty
> things to /dev/console ...

You can't have multiplexing, because that device doesn't allow you to:
it allows only a single signal out at a given time. If you're using
audio, you will not be able to get your logs out of the UART.

And /dev/console is always there as far as I'm aware.

> > And pinctrl can be used dynamically as well if you need to
> 
> Can you explain or point me to the relevant explanation in the docs?
> I don't seem to know about it.

pinctrl_select_state is what you would want to call, you would have a
good example of that runtime change in
drivers/i2c/muxes/i2c-mux-pinctrl.c.

> > > Instead I just got the original patch working, by implementing
> > > "output-high" DT property in sunxi-pinctrl. I'll send a patch for
> > > review soon.
> > 
> > What do you want to do with output-high exactly?
> 
> Exactly what I do in the patch that started this thread.
> (I'll resend when wens' cpvdd patch is available for me to rebase onto.)

There's a few issues with that approach as well:

  - We're actively trying to remove the pinctrl nodes for the GPIOs

  - It's completely static: if one only wants to use the UART all the
    time, they would have to change the DTB, which might or might not
    be possible. Note that this could be trickier: why would you
    prevent the console from working for example if the audio support
    isn't built in? or as a module that might or might not be loaded?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

Hi Maxime!

Maxime Ripard writes:
> > > And pinctrl can be used dynamically as well if you need to
> >
> > Can you explain or point me to the relevant explanation in the docs? I
> > don't seem to know about it.
> 
> pinctrl_select_state is what you would want to call,
> you would have a good example of that runtime change in
> drivers/i2c/muxes/i2c-mux-pinctrl.c.

I believe we have misunderstood each other somewhere. The above example
seems to be about one device (mux driver) requesting a set of pins and then
switching between different pinconfig states thereby activating
different subsets of pins. I don't see a way to have two devices
request the same set of pins. I could write gpio-mux-pinctrl, but I
don't see how this helps much either.

> > > > Instead I just got the original patch working, by implementing
> > > > "output-high" DT property in sunxi-pinctrl. I'll send a patch for
> > > > review soon.
> > >
> > > What do you want to do with output-high exactly?
> >
> > Exactly what I do in the patch that started this thread. (I'll resend
> > when wens' cpvdd patch is available for me to rebase onto.)
> 
> 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?

>   - It's completely static: if one only wants to use the UART all the
>   time, they would have to change the DTB, which might or might not be
>   possible. Note that this could be trickier: why would you prevent the
>   console from working for example if the audio support isn't built in?
>   or as a module that might or might not be loaded?

No, it's not that static:
* Before the audio device probes, UART is active. (You want UART all the
  time: Just prevent the audio module from loading.)
* When you have used audio and suddenly want to have UART instead: Just
  unbind the audio device and set the gpio to low or input from userspace.
  (I intended unbinding to be sufficient, but it seems pinctrl doesn't
   set the pin back to input when it is freed and I don't see a straight
   forward way to force that. I guess it is okay however: Somebody
   switching between UART and audio regularly has to know about the
   details anyway.)

I also tested suspend/resume/poweroff and it mostly works. The only
glitch is, that during suspend there is a short period of UART noise
on the HP. (But no such problem during resume or poweroff.) I suspect
the regulator supplying the gpio bank is shut down quite early when
suspending. It doesn't matter for this discussion: There likely would
be the same behaviour with any other approach too.

I think the real downside of this approach is, that using the UART
makes the internal speakers/mic unuseable too. 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.

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5406 bytes --]



On Tue, Feb 12, 2019 at 08:37:36PM +0100, Harald Geyer wrote:
> Maxime Ripard writes:
> > > > And pinctrl can be used dynamically as well if you need to
> > >
> > > Can you explain or point me to the relevant explanation in the docs? I
> > > don't seem to know about it.
> > 
> > pinctrl_select_state is what you would want to call,
> > you would have a good example of that runtime change in
> > drivers/i2c/muxes/i2c-mux-pinctrl.c.
> 
> I believe we have misunderstood each other somewhere. The above example
> seems to be about one device (mux driver) requesting a set of pins and then
> switching between different pinconfig states thereby activating
> different subsets of pins.

Not quite, it's supposed to drive two set of pins (with different
muxings) connected to a single i2c controller. It's a 1-N relationship

> I don't see a way to have two devices request the same set of pins.
> I could write gpio-mux-pinctrl, but I don't see how this helps much
> either.

You can't at the same time. But precisely, pinctrl provides the
guarantee and the synchronisation needed for two devices to operate on
the same set of pins
> 
> > > > > Instead I just got the original patch working, by implementing
> > > > > "output-high" DT property in sunxi-pinctrl. I'll send a patch for
> > > > > review soon.
> > > >
> > > > What do you want to do with output-high exactly?
> > >
> > > Exactly what I do in the patch that started this thread. (I'll resend
> > > when wens' cpvdd patch is available for me to rebase onto.)
> > 
> > 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.

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).

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.

> >   - It's completely static: if one only wants to use the UART all the
> >   time, they would have to change the DTB, which might or might not be
> >   possible. Note that this could be trickier: why would you prevent the
> >   console from working for example if the audio support isn't built in?
> >   or as a module that might or might not be loaded?
> 
> No, it's not that static:
> * Before the audio device probes, UART is active. (You want UART all the
>   time: Just prevent the audio module from loading.)

If the bootloader configured it that way, if the audio support was
compiled as a module and not builtin, or if no-one fiddles with that
pin for whatever reason. 1 and (especially) 2 are pretty big ifs. 

> * When you have used audio and suddenly want to have UART instead: Just
>   unbind the audio device and set the gpio to low or input from userspace.
>   (I intended unbinding to be sufficient, but it seems pinctrl doesn't
>    set the pin back to input when it is freed and I don't see a straight
>    forward way to force that. I guess it is okay however: Somebody
>    switching between UART and audio regularly has to know about the
>    details anyway.)

And this one is going to be a nightmare for distros to discover :/

> I also tested suspend/resume/poweroff and it mostly works. The only
> glitch is, that during suspend there is a short period of UART noise
> on the HP. (But no such problem during resume or poweroff.) I suspect
> the regulator supplying the gpio bank is shut down quite early when
> suspending. It doesn't matter for this discussion: There likely would
> be the same behaviour with any other approach too.
> 
> I think the real downside of this approach is, that using the UART
> makes the internal speakers/mic unuseable too.

That's also a pretty big issue.

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

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

Hi Maxime!

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.

> 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?

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

Also I wonder how I can select drive strength or bias on a gpio line when
I can't use pinctrl with them anymore.

> > I think the real downside of this approach is, that using the UART
> > makes the internal speakers/mic unuseable too.
> 
> That's also a pretty big issue.

I certainly agree it's unfortunate.

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

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 7191 bytes --]

On Wed, Feb 13, 2019 at 12:43:46PM +0100, Harald Geyer wrote:
> Hi Maxime!
> 
> 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. 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 :)

> > 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?

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

> 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

> > > I think the real downside of this approach is, that using the UART
> > > makes the internal speakers/mic unuseable too.
> > 
> > That's also a pretty big issue.
> 
> I certainly agree it's unfortunate.
> 
> > > 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.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

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

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

[Torsten Duwe]

This is becoming big; can we please split this thread into 3 separate issues?
AFAICS there's the original request to have DT audio nodes for Teres-I,
then a discussion about gpio/pinctrl properties, and one about formal 
device tree verification.

Nonetheless I'll add to points 1 and 3 here :)

On Thu, Feb 14, 2019 at 01:12:44AM +0100, Harald Geyer wrote:
> 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:

[ GPIO discussion ]

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

From my view device trees are maintained in the linux kernel source just
because it's the most active, vivid community, with established procedures
on how to review and make changes. So yes, DTs should be OS agnostic, and
ideally comply with a binding spec (sic!). It's simply pragmatic to do this
on kernel.org.

And yes, some DT formal verification spinning off of this would be a really
good idea, to take ARM (et al.), Linux, and U-Boot further.

[...]

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

Hey, this is a debugging hack. If some rogue app tries to blink it, so be it.

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

As I wrote (held by the list admin), I consider the whole console mux
GPIO an U-Boot hack, and would put it into sun50i-a64-teres-i-u-boot.dtsi.
(As a LED, see above :-)

Would you care to submit a patch version without that GPIO handled?
I think it's very useful and has the pontential to be agreed upon.

	Torsten


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

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

[Harald Geyer]

Hi Torsten!

Torsten Duwe writes:

> > > > > > 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
> 
> Hey, this is a debugging hack. If some rogue app tries to blink it, so be it.
> 
> > 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.
> 
> As I wrote (held by the list admin), I consider the whole console mux
> GPIO an U-Boot hack, and would put it into sun50i-a64-teres-i-u-boot.dtsi.
> (As a LED, see above :-)

The thing is, one of the quite strict rules in kernel development is:
Never break userspace. That means: If we provide a way to userspace to
do something (ie switch between debug and audio), we are expected to
keep it around forever. So since there is a decent chance that someday
somebody might write a driver that handles this situation properly, we
don't want chain ourselves to some dirty hack.

(I'm not totally against writing such a driver myself, if there are
enough use cases. However, I think that TERES alone is not enough.)

> Would you care to submit a patch version without that GPIO handled?
> I think it's very useful and has the potential to be agreed upon.

That would enable audio from the internal speakers but select debug
output on the HP jack by default. I would be okay with that, despite
still thinking that audio on the head phones should be the default.

Maxime and Wens are the maintainers, so it's their call in the end.

HTE,
Harald

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

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

[Torsten Duwe]

On Sat, 16 Feb 2019 21:47:13 +0100
Harald Geyer <harald@ccbib.org> wrote:
> Torsten Duwe writes:

> > I consider the whole console
> > mux GPIO an U-Boot hack, and would put it into
> > sun50i-a64-teres-i-u-boot.dtsi. (As a LED, see above :-)
> 
> The thing is, one of the quite strict rules in kernel development is:
> Never break userspace. That means: If we provide a way to userspace to
> do something (ie switch between debug and audio), we are expected to
> keep it around forever.

No, that rule applies to mechanisms, not configuration. It's not a
problem if a device disappears completely, especially when it's
optional. For clarity's sake, imagine to drop a .dtbo on the fdt that
disables or removes it, after U-Boot has set it.

> > Would you care to submit a patch version without that GPIO handled?
> > I think it's very useful and has the potential to be agreed upon.
> 
> That would enable audio from the internal speakers but select debug
> output on the HP jack by default.

The kernel driver will use whatever the boot preset is; and a
production U-Boot should set that to audio.

> I would be okay with that, despite
> still thinking that audio on the head phones should be the default.

Yes, for production use. For kernel debugging, hack it in U-Boot.
The audio driver should be the last to care.

Imagine you are debugging a boot problem on the serial console and all
of a sudden it stops working just because the audio driver kicks in!

Should any driver ever be in control of that GPIO, it must not be
audio, IMO.

	Torsten


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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2559 bytes --]

On Sat, Feb 16, 2019 at 09:47:13PM +0100, Harald Geyer wrote:
> Hi Torsten!
> 
> Torsten Duwe writes:
> 
> > > > > > > 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
> > 
> > Hey, this is a debugging hack. If some rogue app tries to blink it, so be it.
> > 
> > > 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.
> > 
> > As I wrote (held by the list admin), I consider the whole console mux
> > GPIO an U-Boot hack, and would put it into sun50i-a64-teres-i-u-boot.dtsi.
> > (As a LED, see above :-)
> 
> The thing is, one of the quite strict rules in kernel development is:
> Never break userspace. That means: If we provide a way to userspace to
> do something (ie switch between debug and audio), we are expected to
> keep it around forever. So since there is a decent chance that someday
> somebody might write a driver that handles this situation properly, we
> don't want chain ourselves to some dirty hack.
> 
> (I'm not totally against writing such a driver myself, if there are
> enough use cases. However, I think that TERES alone is not enough.)
> 
> > Would you care to submit a patch version without that GPIO handled?
> > I think it's very useful and has the potential to be agreed upon.
> 
> That would enable audio from the internal speakers but select debug
> output on the HP jack by default. I would be okay with that, despite
> still thinking that audio on the head phones should be the default.
> 
> Maxime and Wens are the maintainers, so it's their call in the end.

At this point, I'm not really convinced by the solution in that patch,
but I don't have really good ideas either. I think it would be good to
discuss this with Mark and Linus Walleij, they will probably have way
better solutions than what I can come up with.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Torsten Duwe]

On Mon, Feb 18, 2019 at 11:24:42AM +0100, Maxime Ripard wrote:
> On Sat, Feb 16, 2019 at 09:47:13PM +0100, Harald Geyer wrote:
> > 
> > > Would you care to submit a patch version without that GPIO handled?
> > > I think it's very useful and has the potential to be agreed upon.
> > 
> > That would enable audio from the internal speakers but select debug
> > output on the HP jack by default. I would be okay with that, despite
> > still thinking that audio on the head phones should be the default.
> > 
> > Maxime and Wens are the maintainers, so it's their call in the end.
> 
> At this point, I'm not really convinced by the solution in that patch,
> but I don't have really good ideas either. I think it would be good to
> discuss this with Mark and Linus Walleij, they will probably have way
> better solutions than what I can come up with.

Once more my plead to *please* apply the unchallenged parts of this patch!

For reference:
https://patchwork.kernel.org/patch/10792589/


Just leave out the line

+	hpvcc-supply = <&reg_eldo1>; /* TODO: Use only one of these */
(as clarified by ChenYu)

and the

@@ -131,6 +151,14 @@ 
 	status = "okay";
 };
 
+&r_pio {
+	r_debug_select_pin: debug-select {
[...]

hunk, which the discussion was about. The patch is of good value
even without it.

IMHO it's a shame this didn't make it into 5.1

Acked-by: Torsten Duwe <duwe@suse.de>

	Torsten


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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1633 bytes --]

On Tue, Apr 30, 2019 at 03:32:32PM +0200, Torsten Duwe wrote:
> On Mon, Feb 18, 2019 at 11:24:42AM +0100, Maxime Ripard wrote:
> > On Sat, Feb 16, 2019 at 09:47:13PM +0100, Harald Geyer wrote:
> > >
> > > > Would you care to submit a patch version without that GPIO handled?
> > > > I think it's very useful and has the potential to be agreed upon.
> > >
> > > That would enable audio from the internal speakers but select debug
> > > output on the HP jack by default. I would be okay with that, despite
> > > still thinking that audio on the head phones should be the default.
> > >
> > > Maxime and Wens are the maintainers, so it's their call in the end.
> >
> > At this point, I'm not really convinced by the solution in that patch,
> > but I don't have really good ideas either. I think it would be good to
> > discuss this with Mark and Linus Walleij, they will probably have way
> > better solutions than what I can come up with.
>
> Once more my plead to *please* apply the unchallenged parts of this patch!
>
> For reference:
> https://patchwork.kernel.org/patch/10792589/
>
>
> Just leave out the line
>
> +	hpvcc-supply = <&reg_eldo1>; /* TODO: Use only one of these */
> (as clarified by ChenYu)
>
> and the
>
> @@ -131,6 +151,14 @@
>  	status = "okay";
>  };
>
> +&r_pio {
> +	r_debug_select_pin: debug-select {
> [...]
>
> hunk, which the discussion was about. The patch is of good value
> even without it.
>
> IMHO it's a shame this didn't make it into 5.1
>
> Acked-by: Torsten Duwe <duwe@suse.de>

Please resend that patch

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Harald Geyer]

Maxime Ripard writes:
> On Tue, Apr 30, 2019 at 03:32:32PM +0200, Torsten Duwe wrote:
> > On Mon, Feb 18, 2019 at 11:24:42AM +0100, Maxime Ripard wrote:
> > > On Sat, Feb 16, 2019 at 09:47:13PM +0100, Harald Geyer wrote:
> > > >
> > > > > Would you care to submit a patch version without that GPIO handled?
> > > > > I think it's very useful and has the potential to be agreed upon.
> > > >
> > > > That would enable audio from the internal speakers but select debug
> > > > output on the HP jack by default. I would be okay with that, despite
> > > > still thinking that audio on the head phones should be the default.
> > > >
> > > > Maxime and Wens are the maintainers, so it's their call in the end.
> > >
> > > At this point, I'm not really convinced by the solution in that patch,
> > > but I don't have really good ideas either. I think it would be good to
> > > discuss this with Mark and Linus Walleij, they will probably have way
> > > better solutions than what I can come up with.
> >
> > Once more my plead to *please* apply the unchallenged parts of this patch!
> >
> > For reference:
> > https://patchwork.kernel.org/patch/10792589/
> >
> >
> > Just leave out the line
> >
> > +	hpvcc-supply = <&reg_eldo1>; /* TODO: Use only one of these */
> > (as clarified by ChenYu)
> >
> > and the
> >
> > @@ -131,6 +151,14 @@
> >  	status = "okay";
> >  };
> >
> > +&r_pio {
> > +	r_debug_select_pin: debug-select {
> > [...]
> >
> > hunk, which the discussion was about. The patch is of good value
> > even without it.
> >
> > IMHO it's a shame this didn't make it into 5.1
> >
> > Acked-by: Torsten Duwe <duwe@suse.de>
> 
> Please resend that patch

Sorry, I don't have time to work on this ATM.

Torsten, if you care about this, feel free to take it over.

I should point out that since then I have learned that pinebook is
using the exactly same debug output multiplexing. Their sound DT nodes
got merged, so we don't really add to the mess, if wo do the same
for the Teres.

Thanks,
Harald

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
>>> application/pgp-signature attachment

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

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

[Chen-Yu Tsai]

On Fri, Feb 1, 2019 at 7:37 PM Harald Geyer <harald@ccbib.org> wrote:
>
> The TERES-I has internal speakers (left, right), internal microphone
> and a headset combo jack (headphones + mic).
>
> The headphone lines are multiplexed with the debug console.
> The headphone and mic detect lines of the A64 are connected properly,
> but AFAIK currently unsupported by the driver.
>
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> Hi all,
>
> a couple of issues make this patch RFC:
>
> hpvcc-supply vs. cpvdd-supply:
> On the A64 manual the pin is called CPVDD and the binding documents
> requires a cpvdd-supply property. However in the actual driver and
> devicetrees so far hpvcc-supply is used. This is a very new binding,
> so we have the luxury to decide either way, I think. Any input from
> the devicetree maintainers would be appreciated.

Sorry about the screw-up. I'll send out patches fixing this once I
test them. The preferred name is CPVDD, as that is what the datasheet
uses.

ChenYu

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

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

[Harald Geyer]

The TERES-I has internal speakers (left, right), internal microphone
and a headset combo jack (headphones + mic).

The headphone lines are multiplexed with the debug console.
The headphone and mic detect lines of the A64 are connected properly,
but AFAIK currently unsupported by the driver.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Hi all,

a couple of issues make this patch RFC:

hpvcc-supply vs. cpvdd-supply:
On the A64 manual the pin is called CPVDD and the binding documents
requires a cpvdd-supply property. However in the actual driver and
devicetrees so far hpvcc-supply is used. This is a very new binding,
so we have the luxury to decide either way, I think. Any input from
the devicetree maintainers would be appreciated.

debug console multiplexing:
Olimex have a userspace script that controls gpio PL9 during boot,
to select between HP and serial console. I guess this is not acceptable
for mainline.

The best solution I can see is to switch the HP jack from serial console
to audio once the audio drivers load. With this people can still capture
the bootlogs but everybody gets audio once the system is up and
switching back to console output is as simple as unloading the audio
drivers.

However the current implementation with a pinctrl group doesn't work:
The audio card device correctly claims the pin (ie I can't export the
gpio in sysfs anymore), but the pinctrl driver doesn't set the pin
to output.

Testing:
I don't have a headset with combo connector, so I could only test the
headphones output, but not the headset mic. If somebody happens to
have a TERES-I and a suitable headset, testing this would be nice.

TIA,
Harald

 .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
index f9eede0a8bd3..d57049fbdaca 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
@@ -70,6 +70,26 @@
 		compatible = "mmc-pwrseq-simple";
 		reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
 	};
+
+	speaker_amp: audio-amplifier {
+		compatible = "simple-audio-amplifier";
+		enable-gpios = <&r_pio 0 12 GPIO_ACTIVE_HIGH>; /* PL12 */
+		sound-name-prefix = "Speaker Amp";
+	};
+};
+
+&codec {
+	status = "okay";
+};
+
+&codec_analog {
+	hpvcc-supply = <&reg_eldo1>; /* TODO: Use only one of these */
+	cpvdd-supply = <&reg_eldo1>;
+	status = "okay";
+};
+
+&dai {
+	status = "okay";
 };
 
 &ehci1 {
@@ -131,6 +151,14 @@
 	status = "okay";
 };
 
+&r_pio {
+	r_debug_select_pin: debug-select {
+		pins = "PL9";
+		function = "gpio_out";
+		output-high;
+	};
+};
+
 &r_rsb {
 	status = "okay";
 
@@ -258,6 +286,31 @@
 	vcc-hdmi-supply = <&reg_dldo1>;
 };
 
+&sound {
+	pinctrl-names = "default";
+	pinctrl-0 = <&r_debug_select_pin>;
+	simple-audio-card,aux-devs = <&codec_analog>, <&speaker_amp>;
+	simple-audio-card,widgets = "Headphone", "Headphone Jack",
+				    "Microphone", "Headset Microphone",
+				    "Microphone", "Internal Microphone",
+				    "Speaker", "Internal Speaker";
+	simple-audio-card,routing =
+			"Left DAC", "AIF1 Slot 0 Left",
+			"Right DAC", "AIF1 Slot 0 Right",
+			"AIF1 Slot 0 Left ADC", "Left ADC",
+			"AIF1 Slot 0 Right ADC", "Right ADC",
+			"Headphone Jack", "HP",
+			"Speaker Amp INL", "LINEOUT",
+			"Speaker Amp INR", "LINEOUT",
+			"Internal Speaker", "Speaker Amp OUTL",
+			"Internal Speaker", "Speaker Amp OUTR",
+			"Internal Microphone", "MBIAS",
+			"MIC1", "Internal Microphone",
+			"Headset Microphone", "HBIAS",
+			"MIC2", "Headset Microphone";
+	status = "okay";
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_pb_pins>;
-- 
2.20.1


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