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 = <®_eldo1>; /* TODO: Use only one of these */ + cpvdd-supply = <®_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 = <®_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
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
[-- 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
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
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
[-- 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
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
[-- 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
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
[-- 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
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
[-- 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
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
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
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
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
[-- 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
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 = <®_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
[-- 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 = <®_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
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 = <®_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