From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6D1F6EB64DD for ; Tue, 1 Aug 2023 06:11:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231331AbjHAGLO (ORCPT ); Tue, 1 Aug 2023 02:11:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32978 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230301AbjHAGLM (ORCPT ); Tue, 1 Aug 2023 02:11:12 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 244671BF3; Mon, 31 Jul 2023 23:11:07 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 9542621E50; Tue, 1 Aug 2023 06:11:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1690870265; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=fdJTYqhdHyFSYpY5y6RQJQGLe9d83A/vO6eFw4zlLVU=; b=MqCIiy0xuDG9ACWVMMBUfhQg34RA+KNZqZ8sQUMHxm1q00dMofH4GDv2p2s6ijlY+vUVs/ 74FZcP72tE6nExTxhmgyLXDfQ+tukLGcVqeA0we1TR1oesNntpEBtYkl+4crdWoAiAJYfJ PRPl8526KdeqQld5NHySaxFXfv3VYtA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1690870265; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=fdJTYqhdHyFSYpY5y6RQJQGLe9d83A/vO6eFw4zlLVU=; b=h+T9V/Ueu9iXSQjcjILxyPcNF2I1sFuPGcKidtmxfcHikAtOuVypmDQt01AwJbps8uuxP4 h8SzBv5EmJFWWNCA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 10B47139BD; Tue, 1 Aug 2023 06:11:05 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id YvAbAfmhyGTQWwAAMHmgww (envelope-from ); Tue, 01 Aug 2023 06:11:05 +0000 Date: Tue, 01 Aug 2023 08:11:04 +0200 Message-ID: <87a5vbi96v.wl-tiwai@suse.de> From: Takashi Iwai To: Jeff LaBundy Cc: Marek Vasut , Dmitry Torokhov , linux-input@vger.kernel.org, Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , Frieder Schrempf , Manuel Traut , Thierry Reding , linux-pwm@vger.kernel.org, alsa-devel@alsa-project.org, Jaroslav Kysela , Takashi Iwai Subject: Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs In-Reply-To: References: <06379f26-ab24-85f9-783f-0c49d4291b23@denx.de> <873514d2ju.wl-tiwai@suse.de> <63adce9a-df65-b462-9055-0ece5216d680@denx.de> <87tttkjmyu.wl-tiwai@suse.de> <0cffe366-75af-d8a8-8920-6fb94c321a89@denx.de> <87h6pkjh7q.wl-tiwai@suse.de> <618add56-3675-4efe-5b20-665c10040e03@denx.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-pwm@vger.kernel.org On Tue, 01 Aug 2023 04:56:09 +0200, Jeff LaBundy wrote: > > Hi all, > > On Mon, Jul 31, 2023 at 07:49:50PM +0200, Marek Vasut wrote: > > On 7/31/23 18:24, Dmitry Torokhov wrote: > > > On Mon, Jul 31, 2023 at 04:36:01PM +0200, Marek Vasut wrote: > > > > On 7/31/23 16:20, Takashi Iwai wrote: > > > > > > > > [...] > > > > > > > > > > > > Uh, I don't need a full sound device to emit beeps, that's not even > > > > > > > > possible with this hardware. > > > > > > > > > > > > > > Heh, I also don't recommend that route, either :) > > > > > > > (Though, it must be possible to create a sound device with that beep > > > > > > > control in theory) > > > > > > > > > > > > I mean, I can imagine one could possibly use PCM DMA to cook samples > > > > > > to feed some of the PWM devices so they could possibly be used to > > > > > > generate low quality audio, as a weird limited DAC, but ... that's not > > > > > > really generic, and not what I want. > > > > > > > > > > Oh I see how the misunderstanding came; I didn't mean the PCM > > > > > implementation like pcsp driver. The pcsp driver is a real hack and > > > > > it's there just for fun, not for any real practical use. > > > > > > > > Ah :) > > > > > > > > > What I meant was rather that you can create a sound device containing > > > > > a mixer volume control that serves exactly like the sysfs or whatever > > > > > other interface, without any PCM stream or other interface. > > > > > > > > Ahhh, hum, I still feel like this might be a bit overkill here. > > > > > > > > > > > > I only need to control loudness of the > > > > > > > > beeper that is controlled by PWM output. That's why I am trying to > > > > > > > > extend the pwm-beeper driver, which seems the best fit for such a > > > > > > > > device, it is only missing this one feature (loudness control). > > > > > > > > > > > > > > So the question is what's expected from user-space POV. If a more > > > > > > > generic control of beep volume is required, e.g. for desktop-like > > > > > > > usages, an implementation of sound driver wouldn't be too bad. > > > > > > > OTOH, for other specific use-cases, it doesn't matter much in which > > > > > > > interface it's implemented, and sysfs could be an easy choice. > > > > > > > > > > > > The whole discussion above has been exactly about this. Basically the > > > > > > thing is, we can either have: > > > > > > - SND_TONE (via some /dev/input/eventX) + sysfs volume control > > > > > > -> This is simple, but sounds racy between input and sysfs accesses > > > > > > > > > > Hmm, how can it be racy if you do proper locking? > > > > > > > > I can imagine two applications can each grab one of the controls and that > > > > makes the interface a bit not nice. That would require extra synchronization > > > > in userspace and so on. > > > > > > > > > > - SND_TONE + SND_TONE_SET_VOLUME > > > > > > -> User needs to do two ioctls, hum > > > > > > - some new SND_TONE_WITH_VOLUME > > > > > > -> Probably the best option, user sets both tone frequency and volume > > > > > > in one go, and it also only extends the IOCTL interface, so older > > > > > > userspace won't have issues > > > > > > > > > > Those are "extensions" I have mentioned, and I'm not a big fan for > > > > > that, honestly speaking. > > > > > > > > > > The fact that the beep *output* stuff is provided by the *input* > > > > > device is already confusing > > > > > > > > I agree, this confused me as well. > > > > > > This comes from the times when keyboards themselves were capable of > > > emitting bells (SUN, DEC, etc). In hindsight it was not the best way of > > > structuring things, same with the keyboard LEDs (that are now plugged > > > into the LED subsystem, but still allow be driven through input). > > > > > > And in the same vein I wonder if we should bite the bullet and pay with > > > a bit of complexity but move sound-related things to sound subsystem. > > > > I am not sure that's the right approach here, since the device cannot do PCM > > playback, just bleeps. > > > > > > > (it was so just because of historical > > > > > reason), and yet you start implementing more full-featured mixer > > > > > control. I'd rather keep fingers away. > > > > > > > > > > Again, if user-space requires the compatible behavior like the > > > > > existing desktop usages > > > > > > > > It does not. These pwm-beeper devices keep showing up in various embedded > > > > devices these days. > > > > > > > > > , it can be implemented in a similar way like > > > > > the existing ones; i.e. provide a mixer control with a proper sound > > > > > device. The sound device doesn't need to provide a PCM interface but > > > > > just with a mixer interface. > > > > > > > > > > Or, if the purpose of your target device is a special usage, you don't > > > > > need to consider too much about the existing interface, and try to > > > > > keep the change as minimal as possible without too intrusive API > > > > > changes. > > > > > > > > My use case is almost perfectly matched by the current input pwm-beeper > > > > driver, the only missing bit is the ability to control the loudness at > > > > runtime. I think adding the SND_TONE_WITH_VOLUME parameter would cover it, > > > > with least intrusive API changes. > > > > > > > > The SND_TONE already supports configuring tone frequency in Hz as its > > > > parameter. Since anything above 64 kHz is certainly not hearable by humans, > > > > I would say the SND_TONE_WITH_VOLUME could use 16 LSbits for frequency (so > > > > up to 65535 Hz , 0 is OFF), and 16 MSbits for volume . > > > > > > > > I'm hesitant to overcomplicate something which can currently be controlled > > > > via single ioctl by pulling in sound subsystem into the picture. > > > > > > Can you tell a bit more about your use case? What needs to control the > > > volume of beeps? Is this the only source of sounds on the system? > > > > Custom user space application. The entire userspace is custom built in this > > case. > > > > In this case, it is a single-use device (think e.g. the kind of thermometer > > you stick in your ear when you're ill, to find out how warm you are). > > > > The beeper there is used to do just that, bleep (with different frequencies > > to indicate different stuff), and that works. What I need in addition to > > that is control the volume of the bleeps from the application, so it isn't > > too noisy. And that needs to be user-controllable at runtime, so not > > something that goes in DT. > > > > Right now there is just the bleeper , yes. > > It sounds like we essentially need an option within pcsp to drive PWM > instead of PCM, but input already has pwm-beeper; it seems harmless to > gently extend the latter for this use-case as opposed to reworking the > former. Nah, please forget pcsp driver. As mentioned earlier, it's a driver that is present just for fun. I believe what we need is a simple sound card instance providing a mixer control for the beep volume, something like a patch like below (totally untested!) Takashi --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -595,6 +595,13 @@ config INPUT_PWM_BEEPER To compile this driver as a module, choose M here: the module will be called pwm-beeper. +config INPUT_PWM_BEEPER_MIXER + bool "Mixer volume support for PWM beeper" + depends on INPUT_PWM_BEEPER + depends on SND=y || INPUT_PWM_BEEPER=SND + help + Say Y here to enable sound mixer for PWM beeper volume. + config INPUT_PWM_VIBRA tristate "PWM vibrator support" depends on PWM --- a/drivers/input/misc/pwm-beeper.c +++ b/drivers/input/misc/pwm-beeper.c @@ -14,6 +14,8 @@ #include #include #include +#include +#include struct pwm_beeper { struct input_dev *input; @@ -21,6 +23,7 @@ struct pwm_beeper { struct regulator *amplifier; struct work_struct work; unsigned long period; + unsigned long duty_cycle; unsigned int bell_frequency; bool suspended; bool amplifier_on; @@ -37,7 +40,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period) state.enabled = true; state.period = period; - pwm_set_relative_duty_cycle(&state, 50, 100); + pwm_set_relative_duty_cycle(&state, beeper->duty_cycle, 100000); error = pwm_apply_state(beeper->pwm, &state); if (error) @@ -112,6 +115,66 @@ static void pwm_beeper_stop(struct pwm_beeper *beeper) pwm_beeper_off(beeper); } +#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER +static int pwm_beeper_mixer_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; + uinfo->count = 1; + uinfo->value.integer.min = 0; + uinfo->value.integer.max = 50000; + return 0; +} + +static int pwm_beeper_mixer_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol); + + ucontrol->value.integer.value[0] = beeper->duty_cycle; + return 0; +} + +static int pwm_beeper_mixer_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct pwm_beeper *beeper = snd_kcontrol_chip(kcontrol); + unsigned long val = ucontrol->value.integer.value[0]; + + val = min(val, 50000UL); + if (beeper->duty_cycle == val) + return 0; + beeper->duty_cycle = val; + if (!beeper->suspended) + schedule_work(&beeper->work); + return 1; +} + +static const struct snd_kcontrol_new pwm_beeper_mixer_ctl = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Beep Playback Switch", + .info = pwm_beeper_mixer_info, + .get = pwm_beeper_mixer_get, + .put = pwm_beeper_mixer_put, +}; + +static int pwm_beeper_mixer_attach(struct device *dev, struct pwm_beeper *beeper) +{ + struct snd_card *card; + int err; + + err = snd_devm_card_new(dev, 0, NULL, THIS_MODULE, 0, &card); + if (err) + return err; + + err = snd_ctl_add(card, snd_ctl_new1(&pwm_beeper_mixer_ctl, beeper)); + if (err) + return err; + + return snd_card_register(card); +} +#endif /* CONFIG_INPUT_PWM_BEEPER_MIXER */ + static void pwm_beeper_close(struct input_dev *input) { struct pwm_beeper *beeper = input_get_drvdata(input); @@ -189,6 +252,7 @@ static int pwm_beeper_probe(struct platform_device *pdev) beeper->input->event = pwm_beeper_event; beeper->input->close = pwm_beeper_close; + beeper->duty_cycle = 50000; input_set_drvdata(beeper->input, beeper); @@ -200,6 +264,11 @@ static int pwm_beeper_probe(struct platform_device *pdev) platform_set_drvdata(pdev, beeper); +#ifdef CONFIG_INPUT_PWM_BEEPER_MIXER + error = pwm_beeper_mixer_attach(dev, beeper); + if (error) + return error; +#endif return 0; }