* Re: [PATCH] HID: Reorder fields in 'struct hid_input'
From: Jiri Kosina @ 2023-08-14 9:21 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Benjamin Tissoires, linux-kernel, kernel-janitors, linux-input
In-Reply-To: <9accb4ebd1247e2f2acc77dd053f67e60bf8d283.1687079502.git.christophe.jaillet@wanadoo.fr>
On Sun, 18 Jun 2023, Christophe JAILLET wrote:
> Group some variables based on their sizes to reduce hole and avoid padding.
> On x86_64, this shrinks the size of 'struct hid_input'
> from 72 to 64 bytes.
>
> It saves a few bytes of memory and is more cache-line friendly.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH RESEND] HID: input: Support devices sending Eraser without Invert
From: Jiri Kosina @ 2023-08-14 9:20 UTC (permalink / raw)
To: Illia Ostapyshyn
Cc: Benjamin Tissoires, linux-input, linux-kernel, Nils Fuhler
In-Reply-To: <20230613152559.108594-1-ostapyshyn@sra.uni-hannover.de>
On Tue, 13 Jun 2023, Illia Ostapyshyn wrote:
> Some digitizers (notably XP-Pen Artist 24) do not report the Invert
> usage when erasing. This causes the device to be permanently stuck with
> the BTN_TOOL_RUBBER tool after sending Eraser, as Invert is the only
> usage that can release the tool. In this state, Touch and Inrange are
> no longer reported to userspace, rendering the pen unusable.
>
> Prior to commit 87562fcd1342 ("HID: input: remove the need for
> HID_QUIRK_INVERT"), BTN_TOOL_RUBBER was never set and Eraser events were
> simply translated into BTN_TOUCH without causing an inconsistent state.
>
> Introduce HID_QUIRK_NOINVERT for such digitizers and detect them during
> hidinput_configure_usage(). This quirk causes the tool to be released
> as soon as Eraser is reported as not set. Set BTN_TOOL_RUBBER in
> input->keybit when mapping Eraser.
>
> Fixes: 87562fcd1342 ("HID: input: remove the need for HID_QUIRK_INVERT")
> Co-developed-by: Nils Fuhler <nils@nilsfuhler.de>
> Signed-off-by: Nils Fuhler <nils@nilsfuhler.de>
> Signed-off-by: Illia Ostapyshyn <ostapyshyn@sra.uni-hannover.de>
Applied, thanks.
> ---
> We were wondering about the reason to keep quirk bits reserved for
> backward compatibility. Is it because of dynamic quirks at module load
> time?
Yes, that is indeed the case.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix error handling in logi_dj_recv_switch_to_dj_mode()
From: Jiri Kosina @ 2023-08-14 9:18 UTC (permalink / raw)
To: Nikita Zhandarovich
Cc: Filipe Laíns, Benjamin Tissoires, linux-input, linux-kernel,
lvc-project
In-Reply-To: <20230613101635.77820-1-n.zhandarovich@fintech.ru>
On Tue, 13 Jun 2023, Nikita Zhandarovich wrote:
> Presently, if a call to logi_dj_recv_send_report() fails, we do
> not learn about the error until after sending short
> HID_OUTPUT_REPORT with hid_hw_raw_request().
> To handle this somewhat unlikely issue, return on error in
> logi_dj_recv_send_report() (minding ugly sleep workaround) and
> take into account the result of hid_hw_raw_request().
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: 6a9ddc897883 ("HID: logitech-dj: enable notifications on connect/disconnect")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> drivers/hid/hid-logitech-dj.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 62180414efcc..e6a8b6d8eab7 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1285,6 +1285,9 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
> * 50 msec should gives enough time to the receiver to be ready.
> */
> msleep(50);
> +
> + if (retval)
> + return retval;
> }
>
> /*
> @@ -1306,7 +1309,7 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
> buf[5] = 0x09;
> buf[6] = 0x00;
>
> - hid_hw_raw_request(hdev, REPORT_ID_HIDPP_SHORT, buf,
> + retval = hid_hw_raw_request(hdev, REPORT_ID_HIDPP_SHORT, buf,
> HIDPP_REPORT_SHORT_LENGTH, HID_OUTPUT_REPORT,
> HID_REQ_SET_REPORT);
Looks good to me. Benjamin, could you please Ack/apply it?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] hid-wiimote-debug.c: Drop error checking for debugfs_create_file
From: Jiri Kosina @ 2023-08-14 9:15 UTC (permalink / raw)
To: Osama Muhammad
Cc: david.rheinsberg, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20230530154252.7895-1-osmtendev@gmail.com>
On Tue, 30 May 2023, Osama Muhammad wrote:
> This patch removes the error checking for debugfs_create_file
> in hid-wiimote-debug.c.c. This is because the debugfs_create_file()
> does not return NULL but an ERR_PTR after an error.
> The DebugFS kernel API is developed in a way that the
> caller can safely ignore the errors that occur during
> the creation of DebugFS nodes.The debugfs Api handles
> it gracefully. The check is unnecessary.
>
> Link to the comment above debugfs_create_file:
> https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L451
>
> Signed-off-by: Osama Muhammad <osmtendev@gmail.com>
>
> ---
> changes since v1
> -In v1 the IS_ERR was used for error checking which is dropped now.
Now applied, sorry for the delay.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: sensor-hub: Allow multi-function sensor devices
From: Jiri Kosina @ 2023-08-14 9:13 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jonathan Cameron, Srinivas Pandruvada, Benjamin Tissoires,
Bjorn Andersson, linux-input, linux-iio, linux-kernel
In-Reply-To: <20230528092427.42332-1-daniel.thompson@linaro.org>
On Sun, 28 May 2023, Daniel Thompson wrote:
> The Lenovo Yoga C630 has a combined keyboard and accelerometer that
> interfaces via i2c-hid. Currently this laptop either has a working
> keyboard (if CONFIG_HID_SENSOR_HUB is disabled) or a working accelerometer.
> only works on kernels. Put another way, most distro kernels enable
> CONFIG_HID_SENSOR_HUB and therefore cannot work on this device since the
> keyboard doesn't work!
>
> Fix this by providing a richer connect mask during the probe. With this
> change both keyboard and screen orientation sensors work correctly.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Applied. Sorry for the delay,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [syzbot] Monthly input report (Aug 2023)
From: syzbot @ 2023-08-14 7:14 UTC (permalink / raw)
To: linux-input, linux-kernel, syzkaller-bugs
Hello input maintainers/developers,
This is a 31-day syzbot report for the input subsystem.
All related reports/information can be found at:
https://syzkaller.appspot.com/upstream/s/input
During the period, 1 new issues were detected and 0 were fixed.
In total, 12 issues are still open and 48 have been fixed so far.
Some of the still happening issues:
Ref Crashes Repro Title
<1> 2957 Yes WARNING in input_mt_init_slots
https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5
<2> 102 Yes WARNING in cm109_urb_irq_callback/usb_submit_urb
https://syzkaller.appspot.com/bug?extid=2d6d691af5ab4b7e66df
<3> 25 Yes INFO: task hung in uhid_char_release
https://syzkaller.appspot.com/bug?extid=8fe2d362af0e1cba8735
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
To disable reminders for individual bugs, reply with the following command:
#syz set <Ref> no-reminders
To change bug's subsystems, reply with:
#syz set <Ref> subsystems: new-subsystem
You may send multiple commands in a single email message.
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Tony Lindgren @ 2023-08-14 5:25 UTC (permalink / raw)
To: Rob Herring
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20230811151048.GA3452914-robh@kernel.org>
* Rob Herring <robh@kernel.org> [230811 15:10]:
> On Fri, Aug 11, 2023 at 02:04:31PM +0300, Tony Lindgren wrote:
> > Allow configuring optional dedicated wakeirq that some SoCs have.
> > Let's use the interrupt naming "irq" and "wakeup" that we already have
> > in use for some drivers and subsystems like i2c.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> > .../devicetree/bindings/input/gpio-keys.yaml | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/gpio-keys.yaml b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > --- a/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > +++ b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> > @@ -31,7 +31,17 @@ patternProperties:
> > maxItems: 1
> >
> > interrupts:
> > - maxItems: 1
> > + description:
> > + Optional interrupts if different from the gpio interrupt
> > + maxItems: 2
> > +
> > + interrupt-names:
> > + description:
> > + Optional interrupt names, can be used to specify a separate
> > + dedicated wake-up interrupt
> > + items:
> > + -const: irq
> > + -const: wakeup
>
> Also need a space after '-'.
Oops sorry about that, obviously I did not run make dtbs_check on this
binding. I guess I just grepped so we don't have interrupt-names in use
right now.
> >
> > label:
> > description: Descriptive name of the key.
> > @@ -130,6 +140,9 @@ examples:
> > label = "GPIO Key UP";
> > linux,code = <103>;
> > gpios = <&gpio1 0 1>;
> > + interrupts-extended = <&intc_wakeup 0 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "wakeup";
>
> That's not what your schema allows. You need:
>
> minItems: 1
> items:
> - enum: [ irq, wakeup ]
> - const: wakeup
>
> (repeating 'wakeup' is disallowed globally for ".*-names".)
OK
> > + wakeup-source;
>
> Of course with this, a single interrupt is the wake-up source and
> doesn't need a name. So you could define that 'interrupt-names' is only
> used when there are 2 interrupts. In that case, the schema is right and
> the example is wrong.
OK. So here's what gpio-keys currently allows:
1. gpios property with no interrupt in the dts, the driver tries to
find the interrupt based on the gpio
2. gpios property with one interrupts property and no interrupt-names
And here's what we could allow in the binding with the wakeirq support
added:
1. gpios property with no interrupt in the dts, the driver tries to
find the interrupt based on the gpio
2. gpios property with one interrupts property and no interrupt-names
3. gpios property with one interrupts property and interrupt-names = "irq"
4. gpios property with one wakeirq and interrupt-names = "wakeirq", the
driver tries to find the io interrupt based on the gpio
5. gpios property with two interrupts and interrupt-names =
"irq", "wakeirq"
So yeah we could only allow interrupt-names if there are two interrupts
like the attempted binding has. This would leave out #3 and #4 options
above. No need to limit these options from driver point of view though.
Any preferences on what the binding should have?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH] hid: Add "Hailuck" to the list of non-apple apple keyboards
From: Rahul Rameshbabu @ 2023-08-14 5:11 UTC (permalink / raw)
To: Nils Tonnaett; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20230811202932.30413-1-ntonnatt@ccrma.stanford.edu>
On Fri, 11 Aug, 2023 13:29:31 -0700 "Nils Tonnaett" <ntonnatt@ccrma.Stanford.EDU> wrote:
> Powzan keyboards KB750 and KB770 identify as
> "Hailuck Co.,Ltd USB Keyboard". Adding "Hailuck" to the list
> of non-apple apple keyboards fixes function keys for these models.
>
> Signed-off-by: Nils Tonnaett <ntonnatt@ccrma.stanford.edu>
> ---
You will likely want to resubmit this patch using "HID: apple:" in
place of "hid:" in the commit message subject.
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/drivers/hid/hid-apple.c
> drivers/hid/hid-apple.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index d7b932925730..6664f90cecaf 100644
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -343,7 +343,8 @@ static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
> { "SONiX USB DEVICE" },
> { "Keychron" },
> { "AONE" },
> - { "GANSS" }
> + { "GANSS" },
> + { "Hailuck" }
This isn't a must, but maybe it makes sense to comma terminate the last
member in this array specifically if you will be resubmitting this
change as a v2. This way, future keyboards added will only touch one
line in the patch rather than two.
> };
>
> static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Marek Vasut @ 2023-08-14 2:26 UTC (permalink / raw)
To: John Watts, Dmitry Torokhov
Cc: Jeff LaBundy, Takashi Iwai, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ZNYsGr6yBeVTtNMK@titan>
On 8/11/23 14:39, John Watts wrote:
> On Tue, Aug 01, 2023 at 12:28:29AM -0700, Dmitry Torokhov wrote:
>> If we want to extend the API we will need to define exactly how it will
>> all work. I.e. what happens if userspace mixes the old SND_TONE and
>> SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
>> previously set volume? The default one? How to set the default one? How
>> to figure out what the current volume is if we decide to make volume
>> "sticky"?
>>
>> As far as userspace I expect it is more common to have one program (or
>> component of a program) to set volume and then something else requests
>> sound, so having one-shot API is of dubious value to me.
>>
>> I hope we can go with Takashi's proposal downthread, but if not I wonder
>> if the sysfs approach is not the simplest one. Do we expect more beepers
>> that can control volume besides pwm-beeper?
>>
>> Thanks.
>>
>> --
>> Dmitry
>
> (Just to duck in as someone that has written a little program to play beeps and
> tones using the EV_TONE API)
>
> It might be worth distinguishing between the goals of having some beeps with
> different volumes compared to all beeps with different volumes.
>
> Sound card mixers generally control some sort of global volume while I would
> imagine the tone API would control per-tone volume. I don't know too much about
> safety guarantees but writing an input then sysfs or mixer then input again
> seems like it could get jumbled up.
>
> In that speicfic case I think it would make more sense to send volume and tone
> from whatever beep API is being used, with the volume being a multiplier of the
> loudest volume. This is similar to how audio works with PCM output. Existing
> beeps would have the volume set to 100%.
I agree binding tone frequency and volume together would be better.
The API would be nicer and easier to use in my opinion too.
^ permalink raw reply
* JOB OFFER
From: Philil Burphly @ 2023-08-12 20:01 UTC (permalink / raw)
To: linux-input
Good day,
Are you looking for a well paid full Time/Part-time job???
AA COURIER SERVICES LTD is looking to hire a couple of workers
urgently!
JOBS Available :-
Truck Drivers
Parcel Handler
Data Entry (Work from home)
Payroll Clerk (Work from home)
Administrative Assistant
Customer Service Representative (Work from home)
=== Excellent Working enviroment, Great Pay, Guaranteed Pay,
Bonuses, Flexible schedule, Temporary and Permanent Positions
Available ===
GREAT FOR RETIREES AND YOUNG ADULTS(Students too)!
**Start as soon as possible**
Payments: Daily/Weekly
Pay: Monthly Minimum Wage is $2000. e.g., $500 per week
Durration: Monday-Friday
REQUIREMENT
* Must be able to work in the United States
* Must be at least 18 years old
* Must provide your ID/Drivers license for verification
* Basics in English
* You must be competitive, organized, self-motivated
* Some of the jobs don't need experience (Training will be
provided)
For more information, you can visit our website or send us a
direct reply to this message.
Thank you and best wishes,
Philil Burphly
Head of HR Department.
-----------------------------------------------------------------
-------------------
AA COURIER SERVICES LTD
Robinhood Street Office Center
2525 Robinhood Street Suite 1100,
Houston, TX 77005
Tel: +1 201 878 4108
Email: info@aacourierservice.com
^ permalink raw reply
* [PATCH V2 RESEND] Input: xpad - Add HyperX Clutch Gladiate Support
From: HP Dev @ 2023-08-11 22:54 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, HP Dev, Chris Toledanes, Rahul Rameshbabu, Carl Ng,
Max Nguyen
Add HyperX controller support to xpad_device and xpad_table.
Suggested-by: Chris Toledanes <chris.toledanes@hp.com>
Reviewed-by: Rahul Rameshbabu <rrameshbaby@nvidia.com>
Reviewed-by: Carl Ng <carl.ng@hp.com>
Signed-off-by: Max Nguyen <maxwell.nguyen@hp.com>
---
V1 -> V2: Remove Xbox 360 vendor-specific class support.
drivers/input/joystick/xpad.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index cdb193317c3b..1e377d040c43 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -130,6 +130,7 @@ static const struct xpad_device {
{ 0x0079, 0x18d4, "GPD Win 2 X-Box Controller", 0, XTYPE_XBOX360 },
{ 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
{ 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
+ { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
{ 0x044f, 0x0f00, "Thrustmaster Wheel", 0, XTYPE_XBOX },
{ 0x044f, 0x0f03, "Thrustmaster Wheel", 0, XTYPE_XBOX },
{ 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", 0, XTYPE_XBOX },
@@ -457,6 +458,7 @@ static const struct usb_device_id xpad_table[] = {
{ USB_INTERFACE_INFO('X', 'B', 0) }, /* Xbox USB-IF not-approved class */
XPAD_XBOX360_VENDOR(0x0079), /* GPD Win 2 controller */
XPAD_XBOX360_VENDOR(0x03eb), /* Wooting Keyboards (Legacy) */
+ XPAD_XBOXONE_VENDOR(0x03f0), /* HP HyperX Xbox One controllers */
XPAD_XBOX360_VENDOR(0x044f), /* Thrustmaster Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x045e), /* Microsoft Xbox 360 controllers */
XPAD_XBOXONE_VENDOR(0x045e), /* Microsoft Xbox One controllers */
--
2.39.3
^ permalink raw reply related
* [PATCH] hid: Add "Hailuck" to the list of non-apple apple keyboards
From: Nils Tonnaett @ 2023-08-11 20:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel; +Cc: Nils Tonnaett
Powzan keyboards KB750 and KB770 identify as
"Hailuck Co.,Ltd USB Keyboard". Adding "Hailuck" to the list
of non-apple apple keyboards fixes function keys for these models.
Signed-off-by: Nils Tonnaett <ntonnatt@ccrma.stanford.edu>
---
drivers/hid/hid-apple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index d7b932925730..6664f90cecaf 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -343,7 +343,8 @@ static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
{ "SONiX USB DEVICE" },
{ "Keychron" },
{ "AONE" },
- { "GANSS" }
+ { "GANSS" },
+ { "Hailuck" }
};
static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
--
2.41.0
^ permalink raw reply related
* Goodix GT911 - no /dev/input
From: Niels Petter @ 2023-08-11 19:29 UTC (permalink / raw)
To: linux-input
Hi folks,
I have problems driving a spi screen with i2c touch gt911.
I was able to get it working on a raspberry pi. I am familiar with
device trees, kernel config,compiling kernels, c/c++ etc.
I am working now on a NXP LX2160A headless platform. Using NXP Kernel
git tag LSDK-21.08 (also tested lf-6.1.1-1.0.0).
Kernel is configured with "make defconfig lsdk.config" and menuconfig
evdev and goodix touch screen support (and st7789v). Device tree is
configured.
OS is bootstrapped debian stable, maybe something is missing here ...
dmesg shows that touch is found and registered under /platform/device
something ...
The console is shown on the display. I know that did not mean anything.
But there is no /dev/input directory.
I double checked everything again. Only thing I am not 100% sure about
at the moment is if the IRQ pin configured is the right one.
Could this lead to a not under /dev/input registered device ?
Maybe an udev rule is missing (missing debian packet) ? Or do I need to
create /dev/input with mknod manually ?
Everything else on I2C / SPI is working as expected.
Any ideas ?
Thanks
Niels
^ permalink raw reply
* Re: [PATCH v4 2/3] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Rob Herring @ 2023-08-11 17:41 UTC (permalink / raw)
To: Fenglin Wu
Cc: robh+dt, devicetree, quic_kamalw, linux-kernel, quic_collinsd,
linux-arm-msm, jestar, dmitry.baryshkov, agross, quic_subbaram,
Dmitry Torokhov, krzysztof.kozlowski+dt, Konrad Dybcio, andersson,
linux-input
In-Reply-To: <20230731053712.2220898-3-quic_fenglinw@quicinc.com>
On Mon, 31 Jul 2023 13:37:07 +0800, Fenglin Wu wrote:
> Add compatible string 'qcom,spmi-vib-gen2' to support vibrator module
> inside PMI632, PMI7250B, PM7325B, PM7550BA.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> .../bindings/input/qcom,pm8xxx-vib.yaml | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: input: convert syna,rmi4 to DT schema
From: Rob Herring @ 2023-08-11 17:17 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Jason A. Donenfeld, linux-input, Dmitry Torokhov,
Matthias Schiffer, devicetree, Vincent Huang, Conor Dooley,
linux-kernel, Krzysztof Kozlowski, Rob Herring
In-Reply-To: <20230728165054.88678-1-krzysztof.kozlowski@linaro.org>
On Fri, 28 Jul 2023 18:50:54 +0200, Krzysztof Kozlowski wrote:
> Convert the bindings for Synaptics RMI4 bus and devices to DT schema.
> Changes during conversion:
> 1. Add reset-gpios already used in DTS and mentioned by RMI4
> specification.
> 2. Do not require address/size cells, because without functions
> (children) they aren't really needed.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Changes in v2:
> 1. Correct [0-9a-z] to [0-9a-f] (Rob).
>
> Jason, Matthias, Vincent,
> I put your names as maintainers, because moderately recently you were
> changing the driver. Let me know if this is okay or you prefer not to
> maintain the hardware.
> ---
> .../bindings/input/rmi4/rmi_2d_sensor.txt | 56 ----
> .../bindings/input/rmi4/rmi_f01.txt | 39 ---
> .../bindings/input/rmi4/rmi_i2c.txt | 61 ----
> .../bindings/input/rmi4/rmi_spi.txt | 56 ----
> .../devicetree/bindings/input/syna,rmi4.yaml | 271 ++++++++++++++++++
> 5 files changed, 271 insertions(+), 212 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> delete mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
> create mode 100644 Documentation/devicetree/bindings/input/syna,rmi4.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Rob Herring @ 2023-08-11 15:10 UTC (permalink / raw)
To: Tony Lindgren
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20230811110432.3968-1-tony@atomide.com>
On Fri, Aug 11, 2023 at 02:04:31PM +0300, Tony Lindgren wrote:
> Allow configuring optional dedicated wakeirq that some SoCs have.
> Let's use the interrupt naming "irq" and "wakeup" that we already have
> in use for some drivers and subsystems like i2c.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> .../devicetree/bindings/input/gpio-keys.yaml | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/input/gpio-keys.yaml b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> --- a/Documentation/devicetree/bindings/input/gpio-keys.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-keys.yaml
> @@ -31,7 +31,17 @@ patternProperties:
> maxItems: 1
>
> interrupts:
> - maxItems: 1
> + description:
> + Optional interrupts if different from the gpio interrupt
> + maxItems: 2
> +
> + interrupt-names:
> + description:
> + Optional interrupt names, can be used to specify a separate
> + dedicated wake-up interrupt
> + items:
> + -const: irq
> + -const: wakeup
Also need a space after '-'.
>
> label:
> description: Descriptive name of the key.
> @@ -130,6 +140,9 @@ examples:
> label = "GPIO Key UP";
> linux,code = <103>;
> gpios = <&gpio1 0 1>;
> + interrupts-extended = <&intc_wakeup 0 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "wakeup";
That's not what your schema allows. You need:
minItems: 1
items:
- enum: [ irq, wakeup ]
- const: wakeup
(repeating 'wakeup' is disallowed globally for ".*-names".)
> + wakeup-source;
Of course with this, a single interrupt is the wake-up source and
doesn't need a name. So you could define that 'interrupt-names' is only
used when there are 2 interrupts. In that case, the schema is right and
the example is wrong.
Rob
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Rob Herring @ 2023-08-11 14:00 UTC (permalink / raw)
To: Tony Lindgren
Cc: devicetree, Conor Dooley, linux-arm-kernel, Rob Herring,
linux-input, Krzysztof Kozlowski, linux-kernel, Dmitry Torokhov
In-Reply-To: <20230811110432.3968-1-tony@atomide.com>
On Fri, 11 Aug 2023 14:04:31 +0300, Tony Lindgren wrote:
> Allow configuring optional dedicated wakeirq that some SoCs have.
> Let's use the interrupt naming "irq" and "wakeup" that we already have
> in use for some drivers and subsystems like i2c.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> .../devicetree/bindings/input/gpio-keys.yaml | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: [error] syntax error: found character '\t' that cannot start any token (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/input/gpio-keys.example.dts'
Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: found character '\t' that cannot start any token
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/input/gpio-keys.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/input/gpio-keys.yaml:40:1: found character '\t' that cannot start any token
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/gpio-keys.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230811110432.3968-1-tony@atomide.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: John Watts @ 2023-08-11 12:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jeff LaBundy, Marek Vasut, Takashi Iwai, linux-input,
Uwe Kleine-König, Frieder Schrempf, Manuel Traut,
Thierry Reding, linux-pwm, alsa-devel, Jaroslav Kysela,
Takashi Iwai
In-Reply-To: <ZMi0HT/yaTo9uTyi@google.com>
On Tue, Aug 01, 2023 at 12:28:29AM -0700, Dmitry Torokhov wrote:
> If we want to extend the API we will need to define exactly how it will
> all work. I.e. what happens if userspace mixes the old SND_TONE and
> SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
> previously set volume? The default one? How to set the default one? How
> to figure out what the current volume is if we decide to make volume
> "sticky"?
>
> As far as userspace I expect it is more common to have one program (or
> component of a program) to set volume and then something else requests
> sound, so having one-shot API is of dubious value to me.
>
> I hope we can go with Takashi's proposal downthread, but if not I wonder
> if the sysfs approach is not the simplest one. Do we expect more beepers
> that can control volume besides pwm-beeper?
>
> Thanks.
>
> --
> Dmitry
(Just to duck in as someone that has written a little program to play beeps and
tones using the EV_TONE API)
It might be worth distinguishing between the goals of having some beeps with
different volumes compared to all beeps with different volumes.
Sound card mixers generally control some sort of global volume while I would
imagine the tone API would control per-tone volume. I don't know too much about
safety guarantees but writing an input then sysfs or mixer then input again
seems like it could get jumbled up.
In that speicfic case I think it would make more sense to send volume and tone
from whatever beep API is being used, with the volume being a multiplier of the
loudest volume. This is similar to how audio works with PCM output. Existing
beeps would have the volume set to 100%.
John.
^ permalink raw reply
* [PATCH 2/2] Input: gpio-keys - Add system suspend support for dedicated wakeirqs
From: Tony Lindgren @ 2023-08-11 11:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel, Dhruva Gole
In-Reply-To: <20230811110432.3968-1-tony@atomide.com>
Some SoCs have a separate dedicated wake-up interrupt controller that can
be used to wake up the system from deeper idle states. We already support
configuring a separate interrupt for a gpio-keys button to be used with a
gpio line. However, we are lacking support system suspend for cases where
a separate interrupt needs to be used in deeper sleep modes.
Because of it's nature, gpio-keys does not know about the runtime PM state
of the button gpios, and may have several gpio buttons configured for each
gpio-keys device instance. Implementing runtime PM support for gpio-keys
does not help, and we cannot use drivers/base/power/wakeirq.c support. We
need to implement custom wakeirq support for gpio-keys.
For handling a dedicated wakeirq for system suspend, we enable and disable
it with gpio_keys_enable_wakeup() and gpio_keys_disable_wakeup() that we
already use based on device_may_wakeup().
Some systems may have a dedicated wakeirq that can also be used as the
main interrupt, this is already working for gpio-keys. Let's add some
wakeirq related comments while at it as the usage with a gpio line and
separate interrupt line may not be obvious.
Tested-by: Dhruva Gole <d-gole@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/input/keyboard/gpio_keys.c | 69 ++++++++++++++++++++++++++++--
include/linux/gpio_keys.h | 2 +
2 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -45,7 +45,9 @@ struct gpio_button_data {
unsigned int software_debounce; /* in msecs, for GPIO-driven buttons */
unsigned int irq;
+ unsigned int wakeirq;
unsigned int wakeup_trigger_type;
+
spinlock_t lock;
bool disabled;
bool key_pressed;
@@ -511,6 +513,7 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
struct gpio_button_data *bdata = &ddata->data[idx];
irq_handler_t isr;
unsigned long irqflags;
+ const char *wakedesc;
int irq;
int error;
@@ -575,6 +578,14 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
!gpiod_cansleep(bdata->gpiod);
}
+ /*
+ * If an interrupt was specified, use it instead of the gpio
+ * interrupt and use the gpio for reading the state. A separate
+ * interrupt may be used as the main button interrupt for
+ * runtime PM to detect events also in deeper idle states. If a
+ * dedicated wakeirq is used for system suspend only, see below
+ * for bdata->wakeirq setup.
+ */
if (button->irq) {
bdata->irq = button->irq;
} else {
@@ -672,6 +683,36 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
return error;
}
+ if (!button->wakeirq)
+ return 0;
+
+ /* Use :wakeup suffix like drivers/base/power/wakeirq.c does */
+ wakedesc = devm_kasprintf(dev, GFP_KERNEL, "%s:wakeup", desc);
+ if (!wakedesc)
+ return -ENOMEM;
+
+ bdata->wakeirq = button->wakeirq;
+ irqflags |= IRQF_NO_SUSPEND;
+
+ /*
+ * Wakeirq shares the handler with the main interrupt, it's only
+ * active during system suspend. See gpio_keys_button_enable_wakeup()
+ * and gpio_keys_button_disable_wakeup().
+ */
+ error = devm_request_any_context_irq(dev, bdata->wakeirq, isr,
+ irqflags, wakedesc, bdata);
+ if (error < 0) {
+ dev_err(dev, "Unable to claim wakeirq %d; error %d\n",
+ bdata->irq, error);
+ return error;
+ }
+
+ /*
+ * Disable wakeirq until suspend. IRQF_NO_AUTOEN won't work if
+ * IRQF_SHARED was set based on !button->can_disable.
+ */
+ disable_irq_nosync(bdata->wakeirq);
+
return 0;
}
@@ -728,7 +769,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
struct gpio_keys_platform_data *pdata;
struct gpio_keys_button *button;
struct fwnode_handle *child;
- int nbuttons;
+ int nbuttons, irq;
nbuttons = device_get_child_node_count(dev);
if (nbuttons == 0)
@@ -750,9 +791,19 @@ gpio_keys_get_devtree_pdata(struct device *dev)
device_property_read_string(dev, "label", &pdata->name);
device_for_each_child_node(dev, child) {
- if (is_of_node(child))
- button->irq =
- irq_of_parse_and_map(to_of_node(child), 0);
+ if (is_of_node(child)) {
+ irq = of_irq_get_byname(to_of_node(child), "irq");
+ if (irq > 0)
+ button->irq = irq;
+
+ irq = of_irq_get_byname(to_of_node(child), "wakeup");
+ if (irq > 0)
+ button->wakeirq = irq;
+
+ if (!button->irq && !button->wakeirq)
+ button->irq =
+ irq_of_parse_and_map(to_of_node(child), 0);
+ }
if (fwnode_property_read_u32(child, "linux,code",
&button->code)) {
@@ -921,6 +972,11 @@ gpio_keys_button_enable_wakeup(struct gpio_button_data *bdata)
}
}
+ if (bdata->wakeirq) {
+ enable_irq(bdata->wakeirq);
+ disable_irq_nosync(bdata->irq);
+ }
+
return 0;
}
@@ -929,6 +985,11 @@ gpio_keys_button_disable_wakeup(struct gpio_button_data *bdata)
{
int error;
+ if (bdata->wakeirq) {
+ enable_irq(bdata->irq);
+ disable_irq_nosync(bdata->wakeirq);
+ }
+
/*
* The trigger type is always both edges for gpio-based keys and we do
* not support changing wakeup trigger for interrupt-based keys.
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -21,6 +21,7 @@ struct device;
* disable button via sysfs
* @value: axis value for %EV_ABS
* @irq: Irq number in case of interrupt keys
+ * @wakeirq: Optional dedicated wake-up interrupt
*/
struct gpio_keys_button {
unsigned int code;
@@ -34,6 +35,7 @@ struct gpio_keys_button {
bool can_disable;
int value;
unsigned int irq;
+ unsigned int wakeirq;
};
/**
--
2.41.0
^ permalink raw reply
* [PATCH 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Tony Lindgren @ 2023-08-11 11:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel
Allow configuring optional dedicated wakeirq that some SoCs have.
Let's use the interrupt naming "irq" and "wakeup" that we already have
in use for some drivers and subsystems like i2c.
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
.../devicetree/bindings/input/gpio-keys.yaml | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/gpio-keys.yaml b/Documentation/devicetree/bindings/input/gpio-keys.yaml
--- a/Documentation/devicetree/bindings/input/gpio-keys.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-keys.yaml
@@ -31,7 +31,17 @@ patternProperties:
maxItems: 1
interrupts:
- maxItems: 1
+ description:
+ Optional interrupts if different from the gpio interrupt
+ maxItems: 2
+
+ interrupt-names:
+ description:
+ Optional interrupt names, can be used to specify a separate
+ dedicated wake-up interrupt
+ items:
+ -const: irq
+ -const: wakeup
label:
description: Descriptive name of the key.
@@ -130,6 +140,9 @@ examples:
label = "GPIO Key UP";
linux,code = <103>;
gpios = <&gpio1 0 1>;
+ interrupts-extended = <&intc_wakeup 0 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "wakeup";
+ wakeup-source;
};
key-down {
--
2.41.0
^ permalink raw reply
* RE: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Traut Manuel LCPF-CH @ 2023-08-11 10:47 UTC (permalink / raw)
To: Takashi Iwai, Jeff LaBundy
Cc: Marek Vasut, Dmitry Torokhov, linux-input@vger.kernel.org,
Uwe Kleine-König, Frieder Schrempf, Thierry Reding,
linux-pwm@vger.kernel.org, alsa-devel@alsa-project.org,
Jaroslav Kysela, Takashi Iwai
In-Reply-To: <87h6p6rp6k.wl-tiwai@suse.de>
Hi
> On Fri, 11 Aug 2023 06:19:50 +0200,
> Jeff LaBundy wrote:
> >
> > Hi Marek, Dmitry and Takashi,
> >
> > On Tue, Aug 01, 2023 at 01:51:50PM +0200, Marek Vasut wrote:
> > > On 8/1/23 09:28, Dmitry Torokhov wrote:
> > > > On Mon, Jul 31, 2023 at 09:56:09PM -0500, 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.
> > > > >
> > > > > I agree that we should not invest too heavily in a legacy ABI,
> > > > > however something like SND_BELL_VOL seems like a low-cost
> > > > > addition that doesn't work against extending pcsp in the future.
> > > > > In fact, input already has precedent for this exact same thing
> > > > > by way of FF rumble effects, which are often PWM-based themselves.
> > > > >
> > > > > If SND_BELL_VOL or similar is not acceptable, then the original
> > > > > sysfs approach seems like the next-best compromise. My only
> > > > > issue with it was that I felt the range was not abstracted enough.
> > > >
> > > > If we want to extend the API we will need to define exactly how it
> > > > will all work. I.e. what happens if userspace mixes the old
> > > > SND_TONE and SND_BELL with the new SND_BELL_VOL or whatever.
> Does
> > > > it play with previously set volume? The default one?
> > >
> > > Default one, to preserve current behavior, yes.
> >
> > This was my idea as well, but I appreciate that the devil is in the
> > details and each driver may have to duplicate some overhead.
> >
> > >
> > > > How to set the default one?
> > >
> > > We do not, we can call pwm_get_duty_cycle() to get the current duty
> > > cycle of the PWM to figure out the default.
> > >
> > > > How
> > > > to figure out what the current volume is if we decide to make
> > > > volume "sticky"?
> > >
> > > The patch stores the current volume configured via sysfs into
> > > beeper->duty_cycle .
> > >
> > > > As far as userspace I expect it is more common to have one program
> > > > (or component of a program) to set volume and then something else
> > > > requests sound, so having one-shot API is of dubious value to me.
> > >
> > > Currently the use case I have for this is a single user facing
> > > application which configures both.
> > >
> > > > I hope we can go with Takashi's proposal downthread, but if not I
> > > > wonder if the sysfs approach is not the simplest one. Do we expect
> > > > more beepers that can control volume besides pwm-beeper?
> > >
> > > It seems to me pulling in dependency on the entire sound subsystem
> > > only to set beeper volume is overkill. I currently don't even have
> > > sound subsystem compiled in.
> >
> > I like Takashi's patch; it seems like a more scalable solution.
> > However, I can appreciate the reluctance to bring in the entire sound
> > subsytem for what is probably a tiny piezoelectric buzzer.
> >
> > It seems like the sysfs solution is the best compromise in the
> > meantime. If more and more users need to shoe-horn these kind of
> > features in the future, we can make more informed decisions as to how to
> extend the API (if at all).
>
> That's my impression, too. The original sysfs usage would be the right fit at
> this moment.
I am fine with both using the Sound API and sysfs. I would additionally like to
specify the pwm values in device-tree like done in pwm-backlight. It really depends
on the hardware which values actually make a difference in volume.
Regards
Manuel
^ permalink raw reply
* Re: [PATCH v6 1/2] dt-bindings: input: i2c-hid: Introduce Ilitek ili9882t
From: cong yang @ 2023-08-11 8:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
dianders, jikos, benjamin.tissoires, hsinyi, linux-input,
devicetree, linux-kernel
In-Reply-To: <8b47da4b-ec68-40f1-c3eb-939dcfa7550e@linaro.org>
Hi,Krzysztof:
The changelog is on the cover letter. Thank you.
On Thu, Aug 3, 2023 at 7:12 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 02/08/2023 09:19, Cong Yang wrote:
> > The ili9882t touch screen chip same as Elan eKTH6915 controller
> > has a reset gpio. The difference is that ili9882t needs to use
> > vccio-supply instead of vcc33-supply. Doug's series[1] allows panels
> > and touchscreens to power on/off together, let's add a phandle for this.
> >
> > [1]: https://lore.kernel.org/r/20230607215224.2067679-1-dianders@chromium.org
> >
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> > .../bindings/input/ilitek,ili9882t.yaml | 67 +++++++++++++++++++
> > 1 file changed, 67 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili9882t.yaml
>
> It's v6 but this still misses the changelog.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Takashi Iwai @ 2023-08-11 7:52 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Marek Vasut, Dmitry Torokhov, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <ZNW25qlzh8YbZtu8@nixie71>
On Fri, 11 Aug 2023 06:19:50 +0200,
Jeff LaBundy wrote:
>
> Hi Marek, Dmitry and Takashi,
>
> On Tue, Aug 01, 2023 at 01:51:50PM +0200, Marek Vasut wrote:
> > On 8/1/23 09:28, Dmitry Torokhov wrote:
> > > On Mon, Jul 31, 2023 at 09:56:09PM -0500, 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.
> > > >
> > > > I agree that we should not invest too heavily in a legacy ABI, however
> > > > something like SND_BELL_VOL seems like a low-cost addition that doesn't
> > > > work against extending pcsp in the future. In fact, input already has
> > > > precedent for this exact same thing by way of FF rumble effects, which
> > > > are often PWM-based themselves.
> > > >
> > > > If SND_BELL_VOL or similar is not acceptable, then the original sysfs
> > > > approach seems like the next-best compromise. My only issue with it was
> > > > that I felt the range was not abstracted enough.
> > >
> > > If we want to extend the API we will need to define exactly how it will
> > > all work. I.e. what happens if userspace mixes the old SND_TONE and
> > > SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
> > > previously set volume? The default one?
> >
> > Default one, to preserve current behavior, yes.
>
> This was my idea as well, but I appreciate that the devil is in the details
> and each driver may have to duplicate some overhead.
>
> >
> > > How to set the default one?
> >
> > We do not, we can call pwm_get_duty_cycle() to get the current duty cycle of
> > the PWM to figure out the default.
> >
> > > How
> > > to figure out what the current volume is if we decide to make volume
> > > "sticky"?
> >
> > The patch stores the current volume configured via sysfs into
> > beeper->duty_cycle .
> >
> > > As far as userspace I expect it is more common to have one program (or
> > > component of a program) to set volume and then something else requests
> > > sound, so having one-shot API is of dubious value to me.
> >
> > Currently the use case I have for this is a single user facing application
> > which configures both.
> >
> > > I hope we can go with Takashi's proposal downthread, but if not I wonder
> > > if the sysfs approach is not the simplest one. Do we expect more beepers
> > > that can control volume besides pwm-beeper?
> >
> > It seems to me pulling in dependency on the entire sound subsystem only to
> > set beeper volume is overkill. I currently don't even have sound subsystem
> > compiled in.
>
> I like Takashi's patch; it seems like a more scalable solution. However, I
> can appreciate the reluctance to bring in the entire sound subsytem for what
> is probably a tiny piezoelectric buzzer.
>
> It seems like the sysfs solution is the best compromise in the meantime. If
> more and more users need to shoe-horn these kind of features in the future,
> we can make more informed decisions as to how to extend the API (if at all).
That's my impression, too. The original sysfs usage would be the
right fit at this moment.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Jeff LaBundy @ 2023-08-11 4:19 UTC (permalink / raw)
To: Marek Vasut
Cc: Dmitry Torokhov, Takashi Iwai, linux-input, Uwe Kleine-König,
Frieder Schrempf, Manuel Traut, Thierry Reding, linux-pwm,
alsa-devel, Jaroslav Kysela, Takashi Iwai
In-Reply-To: <d0f284d5-6922-ea3b-3447-c089b0d24587@denx.de>
Hi Marek, Dmitry and Takashi,
On Tue, Aug 01, 2023 at 01:51:50PM +0200, Marek Vasut wrote:
> On 8/1/23 09:28, Dmitry Torokhov wrote:
> > On Mon, Jul 31, 2023 at 09:56:09PM -0500, 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.
> > >
> > > I agree that we should not invest too heavily in a legacy ABI, however
> > > something like SND_BELL_VOL seems like a low-cost addition that doesn't
> > > work against extending pcsp in the future. In fact, input already has
> > > precedent for this exact same thing by way of FF rumble effects, which
> > > are often PWM-based themselves.
> > >
> > > If SND_BELL_VOL or similar is not acceptable, then the original sysfs
> > > approach seems like the next-best compromise. My only issue with it was
> > > that I felt the range was not abstracted enough.
> >
> > If we want to extend the API we will need to define exactly how it will
> > all work. I.e. what happens if userspace mixes the old SND_TONE and
> > SND_BELL with the new SND_BELL_VOL or whatever. Does it play with
> > previously set volume? The default one?
>
> Default one, to preserve current behavior, yes.
This was my idea as well, but I appreciate that the devil is in the details
and each driver may have to duplicate some overhead.
>
> > How to set the default one?
>
> We do not, we can call pwm_get_duty_cycle() to get the current duty cycle of
> the PWM to figure out the default.
>
> > How
> > to figure out what the current volume is if we decide to make volume
> > "sticky"?
>
> The patch stores the current volume configured via sysfs into
> beeper->duty_cycle .
>
> > As far as userspace I expect it is more common to have one program (or
> > component of a program) to set volume and then something else requests
> > sound, so having one-shot API is of dubious value to me.
>
> Currently the use case I have for this is a single user facing application
> which configures both.
>
> > I hope we can go with Takashi's proposal downthread, but if not I wonder
> > if the sysfs approach is not the simplest one. Do we expect more beepers
> > that can control volume besides pwm-beeper?
>
> It seems to me pulling in dependency on the entire sound subsystem only to
> set beeper volume is overkill. I currently don't even have sound subsystem
> compiled in.
I like Takashi's patch; it seems like a more scalable solution. However, I
can appreciate the reluctance to bring in the entire sound subsytem for what
is probably a tiny piezoelectric buzzer.
It seems like the sysfs solution is the best compromise in the meantime. If
more and more users need to shoe-horn these kind of features in the future,
we can make more informed decisions as to how to extend the API (if at all).
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50
From: Jeff LaBundy @ 2023-08-11 4:07 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, Fred Treven, Ben Bright, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Lee Jones, Joel Stanley,
Arnd Bergmann, Jacky Bai, Jean Delvare, Eddie James,
Markus Schneider-Pargmann, ChiYuan Huang, Randy Dunlap,
Wolfram Sang, patches, linux-input, devicetree, linux-kernel
In-Reply-To: <20230809191032.820271-3-james.ogletree@cirrus.com>
Hi James,
Great work as always; just some minor comments throughout.
On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.
From my extremely naive point of view, some of the code that follows
bares resemblance to the recently reviewed L26. My assumption is that
these devices are different enough in nature to warrant completely
different drivers; is that accurate?
One reason for asking is because the L26 driver included a cornucopia
of power-management overhead, yet we see none of that here. Assuming
both L50 and L26 are built around the same Halo DSP, why is there such
a fundamental difference between the two in terms of power management?
To that end, how does this driver handle hibernation? Is hibernation
not supported, or do we simply defer to the DSP? In the case of the
latter, why is L50 given this freedom but not L26?
This is not a suggestion to change anything; it's just to help set the
context and provide more background information. However, I'm curious
whether you've considered consolidating both L26 and L50 into a single
driver, especially given recent discussion around promoting L26 into an
MFD with both input and codec children. Would L50 ever support audio-to
-haptics over I2S?
>
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
> MAINTAINERS | 2 +
> drivers/input/misc/Kconfig | 33 +
> drivers/input/misc/Makefile | 3 +
> drivers/input/misc/cs40l50-i2c.c | 67 ++
> drivers/input/misc/cs40l50-spi.c | 67 ++
> drivers/input/misc/cs40l50.c | 1008 ++++++++++++++++++++++++++++++
> include/linux/input/cs40l50.h | 321 ++++++++++
> 7 files changed, 1501 insertions(+)
> create mode 100644 drivers/input/misc/cs40l50-i2c.c
> create mode 100644 drivers/input/misc/cs40l50-spi.c
> create mode 100644 drivers/input/misc/cs40l50.c
> create mode 100644 include/linux/input/cs40l50.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2869dd2dbbcf..e82d03293383 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2088,6 +2088,8 @@ M: Ben Bright <ben.bright@cirrus.com>
> L: patches@opensource.cirrus.com
> S: Supported
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +F: drivers/input/misc/cs40l*
> +F: include/linux/input/cs40l*
>
> ARM/CLKDEV SUPPORT
> M: Russell King <linux@armlinux.org.uk>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index c47eecc6f83b..c1c1572ec39c 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -140,6 +140,39 @@ config INPUT_BMA150
> To compile this driver as a module, choose M here: the
> module will be called bma150.
>
> +config INPUT_CS40L50
> + tristate "Cirrus Logic CS40L50 Haptic Driver support"
> + select CS_DSP
> + select REGMAP_IRQ
> + help
> + Say Y here to enable support for CS40L50 boosted haptic
> + driver.
> +
> + To compile the driver as a module choose M here: the module
> + will be called cs40l50.
> +
> +config INPUT_CS40L50_I2C
> + tristate "Support I2C bus connection"
> + depends on INPUT_CS40L50 && I2C
> + select REGMAP_I2C
> + help
> + Say Y here if you have Cirrus Logic's CS40L50 connected
> + to an I2C bus.
> +
> + To compile the driver as a module choose M here: the
> + module will be called cs40l50_i2c.
> +
> +config INPUT_CS40L50_SPI
> + tristate "Support SPI bus connection"
> + depends on INPUT_CS40L50 && SPI
> + select REGMAP_SPI
> + help
> + Say Y here if you have Cirrus Logic's CS40L50 connected
> + to a SPI bus.
> +
> + To compile the driver as a module choose M here: the
> + module will be called cs40l50_spi.
> +
> config INPUT_E3X0_BUTTON
> tristate "NI Ettus Research USRP E3xx Button support."
> default n
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 04296a4abe8e..77f34a33c364 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,9 @@ obj-$(CONFIG_INPUT_CMA3000) += cma3000_d0x.o
> obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
> obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o
> obj-$(CONFIG_INPUT_CPCAP_PWRBUTTON) += cpcap-pwrbutton.o
> +obj-$(CONFIG_INPUT_CS40L50) += cs40l50.o
> +obj-$(CONFIG_INPUT_CS40L50_I2C) += cs40l50-i2c.o
> +obj-$(CONFIG_INPUT_CS40L50_SPI) += cs40l50-spi.o
> obj-$(CONFIG_INPUT_DA7280_HAPTICS) += da7280.o
> obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o
> obj-$(CONFIG_INPUT_DA9055_ONKEY) += da9055_onkey.o
> diff --git a/drivers/input/misc/cs40l50-i2c.c b/drivers/input/misc/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..6ebe1b51990b
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-i2c.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
Nit: the capitalization and general sentence structure here is hard to
follow; maybe something like "... with DSP-based waveform memory and
closed-loop algorithms" would make more sense?
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/i2c.h>
> +#include <linux/input/cs40l50.h>
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> + {"cs40l50", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct cs40l50_private *cs40l50;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
Nit: line breaks tend to be more readable when aligned against the
parenthesis as follows:
if (...)
return dev_err_probe(...,
...);
At least, it's much more common.
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = client->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> + struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + },
> + .id_table = cs40l50_id_i2c,
> + .probe_new = cs40l50_i2c_probe,
Mainline has moved back to .probe now; .probe_new is becoming depracated.
> + .remove = cs40l50_i2c_remove,
> +};
> +
> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/input/misc/cs40l50-spi.c b/drivers/input/misc/cs40l50-spi.c
> new file mode 100644
> index 000000000000..5fcf3ba137f0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-spi.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/spi/spi.h>
> +#include <linux/input/cs40l50.h>
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> + {"cs40l50", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50;
> + struct device *dev = &spi->dev;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = spi->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static struct spi_driver cs40l50_spi_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + },
> + .id_table = cs40l50_id_spi,
> + .probe = cs40l50_spi_probe,
> + .remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/input/misc/cs40l50.c b/drivers/input/misc/cs40l50.c
> new file mode 100644
> index 000000000000..9bb2881683c0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50.c
> @@ -0,0 +1,1008 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
Why to #include i2c.h here? This driver should be agnostic to the transport.
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/bitfield.h>
> +#include <linux/input/cs40l50.h>
Please sort these alphabetically. Are you sure you're using all of them?
> +
> +const struct regmap_config cs40l50_regmap = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .val_format_endian = REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> + { .supply = "vp" },
> + { .supply = "vio" },
> + { .supply = "va" },
> +};
> +
> +static int cs40l50_dsp_write(struct cs40l50_private *cs40l50, u32 write_reg, u32 write_val)
> +{
> + int error = 0, i;
No need to initialize 'error' here.
> +
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = regmap_write(cs40l50->regmap, write_reg, write_val);
> + if (!error)
> + break;
This seems more efficient:
if (!error)
return 0;
...then you can avoid the check for i == CS40L50_DSP_TIMEOUT_COUNT below.
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> + dev_err(cs40l50->dev, "Error writing %#X to %#X: %d", write_val, write_reg, error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int cs40l50_dsp_read(struct cs40l50_private *cs40l50, u32 read_reg, u32 *read_val)
> +{
> + int error = 0, i;
...and here.
> +
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = regmap_read(cs40l50->regmap, read_reg, read_val);
> + if (!error)
> + break;
...and here.
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> + dev_err(cs40l50->dev, "Error reading %#X: %d", read_reg, error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int cs40l50_pseq_write(struct cs40l50_private *cs40l50, u32 addr, u32 data)
> +{
> + u8 operation = 0;
> + struct cs40l50_pseq_op *op_new, *op_end;
> + int error;
Nit: it is kind of personal preference, but "reverse Christmas tree" tends
to be more common, i.e. move the struct definitions (longest) to the top.
> +
> + op_new = devm_kzalloc(cs40l50->dev, sizeof(*op_new), GFP_KERNEL);
> + if (!op_new)
> + return -ENOMEM;
> +
> + op_new->words[0] = (addr >> CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT);
> + op_new->words[1] = ((addr & CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK) <<
> + CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT) |
> + (data >> CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT);
Nit: consider aligning the line breaks past the equal sign for readability.
> + op_new->words[2] = data & CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK;
> +
> + list_for_each_entry(op_end, &cs40l50->pseq_op_head, list) {
> + operation = op_end->operation;
> + if (operation == CS40L50_PSEQ_OP_END)
> + break;
> + }
> +
> + if (operation != CS40L50_PSEQ_OP_END) {
> + error = -ENOENT;
> + goto op_new_free;
> + }
> +
> + if ((CS40L50_PSEQ_MAX_WORDS - op_end->offset) < ARRAY_SIZE(op_new->words)) {
> + error = -ENOSPC;
> + goto op_new_free;
> + }
> +
> + op_new->offset = op_end->offset;
> + op_end->offset += (ARRAY_SIZE(op_new->words) * sizeof(u32));
> +
> + error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ + op_new->offset,
> + op_new->words, ARRAY_SIZE(op_new->words));
Nit: alignment
> + if (error)
> + goto op_new_free;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_POWER_ON_SEQ + op_end->offset,
> + op_end->words[0]);
...and here.
> + if (error)
> + goto op_new_free;
> +
> + list_add(&op_new->list, &cs40l50->pseq_op_head);
> +
> + return 0;
> +
> +op_new_free:
> + devm_kfree(cs40l50->dev, op_new);
> +
> + return error;
> +}
> +
> +static struct cs40l50_effect *cs40l50_find_effect(struct cs40l50_private *cs40l50, int id)
> +{
> + struct cs40l50_effect *effect;
> +
> + if (!list_empty(&cs40l50->effect_head)) {
> + list_for_each_entry(effect, &cs40l50->effect_head, list) {
> + if (effect->id == id)
> + return effect;
> + }
> + }
Are you sure the check against list_empty is necessary? Won't list_for_each_entry()
simply be a NOP if the list is empty? (genuine question)
> +
> + return NULL;
> +}
> +
> +static int cs40l50_owt_upload(struct cs40l50_private *cs40l50, s16 *in_data, u32 in_data_nibbles)
> +{
> + size_t in_data_bytes = 2 * in_data_nibbles;
> + struct cs40l50_owt_header header;
> + u32 wt_offset, wt_size_words;
> + int error;
> + u8 *data;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_NEXT, &wt_offset);
> + if (error)
> + return error;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_OWT_SIZE, &wt_size_words);
> + if (error)
> + return error;
> +
> + if ((wt_size_words * sizeof(u32)) < CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes)
> + return -ENOSPC;
> +
> + data = kcalloc(CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes, sizeof(u8), GFP_KERNEL);
> + if (IS_ERR_OR_NULL(data))
> + return -ENOMEM;
> +
> + header.wvfrm_type = in_data[0] == CS40L50_PCM_ID ? CS40L50_WT_TYPE_PCM :
> + CS40L50_WT_TYPE_PWLE;
Nit: this is a bit more readable:
header.wvfrm_type = in_data[0] == CS40L50_PCM_ID ? CS40L50_WT_TYPE_PCM
: CS40L50_WT_TYPE_PWLE;
> + header.offset = CS40L50_WT_HEADER_OFFSET;
> + header.data_size = in_data_bytes / sizeof(u32);
> +
> + memcpy(data, &header, sizeof(header));
> + memcpy(data + (CS40L50_WT_HEADER_OFFSET * sizeof(u32)), in_data, in_data_bytes);
> +
> + error = regmap_bulk_write(cs40l50->regmap, CS40L50_DSP1_OWT_BASE +
> + (wt_offset * sizeof(u32)), data,
> + CS40L50_WT_HEADER_PWLE_SIZE + in_data_bytes);
> + if (error)
> + goto err_free;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_OWT_PUSH);
> +err_free:
> + kfree(data);
We can avoid a goto and label for this simple case as follows:
error = ...
if (!error)
error = ...
kfree(...);
return error;
> +
> + return error;
> +}
> +
> +static int cs40l50_add_effect(struct input_dev *dev, struct ff_effect *effect,
> + struct ff_effect *old)
Nit: alignment
> +{
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> + bool is_new = false;
> + struct cs40l50_effect *new_effect;
> + u32 nwaves, base_index, max_index;
> + s16 *raw_custom_data;
> + int error;
> +
> + if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
> + dev_err(cs40l50->dev, "Type (%#X) or waveform (%#X) unsupported\n",
> + effect->type, effect->u.periodic.waveform);
> + return -EINVAL;
> + }
> +
> + raw_custom_data = kcalloc(effect->u.periodic.custom_len, sizeof(s16), GFP_KERNEL);
> + if (IS_ERR_OR_NULL(raw_custom_data))
> + return -ENOMEM;
> +
> + error = copy_from_user(raw_custom_data, effect->u.periodic.custom_data,
> + sizeof(s16) * effect->u.periodic.custom_len);
> + if (error)
> + goto out_free;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + new_effect = cs40l50_find_effect(cs40l50, effect->id);
> + if (!new_effect) {
> + is_new = true;
> + new_effect = kzalloc(sizeof(*new_effect), GFP_KERNEL);
> + if (IS_ERR_OR_NULL(new_effect)) {
> + error = -ENOMEM;
> + goto err_mutex;
> + }
> + }
> +
> + if (effect->u.periodic.custom_len > CS40L50_CUSTOM_DATA_SIZE) {
> + error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_OWT_WAVES,
> + &new_effect->trigger_index);
> + if (error)
> + goto err_free;
> +
> + new_effect->wvfrm_bank = CS40L50_WVFRM_BANK_OWT;
> + base_index = CS40L50_OWT_INDEX_START;
> + max_index = CS40L50_OWT_INDEX_END;
> + } else {
> + new_effect->trigger_index = (raw_custom_data[1] & 0xffffu);
> + new_effect->wvfrm_bank = (raw_custom_data[0] & 0xffffu);
> +
> + if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_ROM) {
> + base_index = CS40L50_ROM_BANK_INDEX_START;
> + max_index = CS40L50_ROM_BANK_INDEX_END;
> + } else if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_RAM) {
> + error = cs40l50_dsp_read(cs40l50, CS40L50_NUM_OF_WAVES, &nwaves);
> + if (error)
> + goto err_free;
> +
> + base_index = CS40L50_RAM_BANK_INDEX_START;
> + max_index = base_index + nwaves - 1;
> + } else {
> + dev_err(cs40l50->dev, "Invalid waveform bank\n");
> + error = -EINVAL;
> + goto err_free;
> + }
> + }
> +
> + new_effect->trigger_index += base_index;
> +
> + if (new_effect->trigger_index > max_index) {
> + dev_err(cs40l50->dev, "Index %#X out of bounds\n", new_effect->trigger_index);
> + error = -EINVAL;
> + goto err_free;
> + }
> +
> + if (effect->trigger.button) {
> + new_effect->mapping = CS40L50_DSP1_GPIO_BASE + sizeof(u32) *
> + FIELD_GET(CS40L50_BTN_NUM_MASK, effect->trigger.button) * 2 -
> + FIELD_GET(CS40L50_BTN_EDGE_MASK, effect->trigger.button);
> +
> + error = cs40l50_dsp_write(cs40l50, new_effect->mapping, effect->trigger.button);
> + if (error)
> + goto err_free;
> + }
> +
> + if (new_effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> + error = cs40l50_owt_upload(cs40l50, raw_custom_data,
> + effect->u.periodic.custom_len);
> + if (error)
> + goto err_free;
> + }
> +
> +err_free:
> + if (is_new)
> + error ? kfree(new_effect) : list_add(&new_effect->list, &cs40l50->effect_head);
Just wanted to echo Charles' point that this made my head spin a bit. Good
to know it's not just me ;)
> +err_mutex:
> + mutex_unlock(&cs40l50->lock);
> +out_free:
> + kfree(raw_custom_data);
> +
> + return error ? -EFAULT : 0;
> +}
> +
> +
> +static void cs40l50_vibe_start_worker(struct work_struct *work)
> +{
> + struct cs40l50_private *cs40l50 =
> + container_of(work, struct cs40l50_private, vibe_start_work);
> + struct cs40l50_effect *effect;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + effect = cs40l50_find_effect(cs40l50, cs40l50->trigger_effect->id);
> + if (effect)
> + cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, effect->trigger_index);
> +
> + mutex_unlock(&cs40l50->lock);
> +}
> +
> +static void cs40l50_vibe_stop_worker(struct work_struct *work)
> +{
> + struct cs40l50_private *cs40l50 =
> + container_of(work, struct cs40l50_private, vibe_stop_work);
> +
> + mutex_lock(&cs40l50->lock);
> +
> + cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1, CS40L50_MBOX_CMD_STOP_PLAYBACK);
> +
> + mutex_unlock(&cs40l50->lock);
> +}
> +
> +static int cs40l50_playback_effect(struct input_dev *dev, int effect_id, int val)
> +{
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> + cs40l50->trigger_effect = &dev->ff->effects[effect_id];
> +
> + if (val > 0)
> + queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_start_work);
> + else
> + queue_work(cs40l50->vibe_workqueue, &cs40l50->vibe_stop_work);
> +
> + return 0;
> +}
> +
> +static int cs40l50_erase_effect(struct input_dev *dev, int effect_id)
> +{
> + int error = 0;
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> + struct cs40l50_effect *effect, *effect_owt;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + effect = cs40l50_find_effect(cs40l50, dev->ff->effects[effect_id].id);
> + if (!effect) {
> + error = -EINVAL;
> + goto err_mutex;
> + }
> +
> + if (effect->mapping != CS40L50_GPIO_MAPPING_INVALID) {
> + error = cs40l50_dsp_write(cs40l50, effect->mapping, CS40L50_GPI_DISABLE);
> + if (error)
> + goto err_mutex;
> + }
> +
> + if (effect->wvfrm_bank == CS40L50_WVFRM_BANK_OWT) {
> + error = cs40l50_dsp_write(cs40l50, CS40L50_DSP_VIRTUAL1_MBOX_1,
> + CS40L50_MBOX_CMD_OWT_DELETE | effect->trigger_index);
Nit: alignment here and elsewhere.
> + if (error)
> + goto err_mutex;
> +
> + list_for_each_entry(effect_owt, &cs40l50->effect_head, list) {
> + if (effect_owt->wvfrm_bank == CS40L50_WVFRM_BANK_OWT &&
> + effect_owt->trigger_index > effect->trigger_index)
> + effect_owt->trigger_index--;
> + }
> + }
> +
> + list_del(&effect->list);
> + kfree(effect);
> +err_mutex:
> + mutex_unlock(&cs40l50->lock);
> +
> + return error;
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> + u32 wt_ptr, rd_ptr;
> + int error = 0;
No need to initialize 'error' here.
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_WT, &wt_ptr);
> + if (error)
> + return error;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, &rd_ptr);
> + if (error)
> + return error;
> +
> + if (wt_ptr == rd_ptr) {
> + *val = 0;
> + return 0;
> + }
> +
> + error = cs40l50_dsp_read(cs40l50, rd_ptr, val);
> + if (error)
> + return error;
> +
> + rd_ptr += sizeof(u32);
> + if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> + rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> + return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> + int error;
> + u32 f_zero;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_F0_ESTIMATION, &f_zero);
> + if (error)
> + return error;
> +
> + return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_F0_STORED, f_zero);
> +}
> +
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> + int integer, fractional, error;
> + u32 redc;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_RE_EST_STATUS, &redc);
> + if (error)
> + return error;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_ESTIMATION, redc);
> + if (error)
> + return error;
> +
> + /* Convert from Q8.15 to (Q7.17 * 29/240) */
> + integer = (redc >> 15) & 0xFF;
> + fractional = (redc & 0x7FFF) * 4;
> +
> + return cs40l50_dsp_write(cs40l50, CS40L50_DSP1_REDC_STORED,
> + (((integer << 17) | fractional) * 29 / 240) & GENMASK(23, 0));
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> + int error;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, CS40L50_ERROR_RELEASE_MASK);
> + if (error)
> + return error;
> +
> + return cs40l50_dsp_write(cs40l50, CS40L50_ERROR_RELEASE, 0);
> +}
> +
> +static irqreturn_t cs40l50_handle_mbox_buffer(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> + int error = 0;
> + u32 val;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
> + switch (val) {
> + case 0:
> + goto out_irq;
> + case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
> + break;
> + case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> + break;
> + case CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> + break;
> + case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> + break;
> + case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> + break;
> + case CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> + break;
> + case CS40L50_DSP_MBOX_F0_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> + break;
> + case CS40L50_DSP_MBOX_F0_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> + error = cs40l50_handle_f0_est_done(cs40l50);
> + if (error)
> + goto out_irq;
> + break;
> + case CS40L50_DSP_MBOX_REDC_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> + break;
> + case CS40L50_DSP_MBOX_REDC_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> + error = cs40l50_handle_redc_est_done(cs40l50);
> + if (error)
> + goto out_irq;
> + break;
> + case CS40L50_DSP_MBOX_LE_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> + break;
> + case CS40L50_DSP_MBOX_LE_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> + break;
> + case CS40L50_DSP_MBOX_AWAKE:
> + dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> + break;
> + case CS40L50_DSP_MBOX_INIT:
> + dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> + break;
> + case CS40L50_DSP_MBOX_ACK:
> + dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED:
> + dev_err(cs40l50->dev, "Unmapped event\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_EVENT_MODIFY:
> + dev_err(cs40l50->dev, "Attempt to modify event index failed\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_NULLPTR:
> + dev_err(cs40l50->dev, "Null pointer\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_BRAKING:
> + dev_err(cs40l50->dev, "Braking not in progress\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_INVAL_SRC:
> + dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_ENABLE_RANGE:
> + dev_err(cs40l50->dev, "GPIO enable out of range\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED:
> + dev_err(cs40l50->dev, "GPIO not mapped\n");
> + break;
> + case CS40L50_DSP_MBOX_ERR_ISR_RANGE:
> + dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> + break;
> + case CS40L50_DSP_MBOX_PERMANENT_SHORT:
> + dev_crit(cs40l50->dev, "Permanent short detected\n");
> + break;
> + case CS40L50_DSP_MBOX_RUNTIME_SHORT:
> + dev_err(cs40l50->dev, "Runtime short detected\n");
> + error = cs40l50_error_release(cs40l50);
> + if (error)
> + goto out_irq;
> + break;
> + default:
> + dev_err(cs40l50->dev, "Mailbox value %#X not recognized\n", val);
> + error = -EINVAL;
> + goto out_irq;
> + }
> + }
> +
> + error = -EIO;
> +
> +out_irq:
> + mutex_unlock(&cs40l50->lock);
> +
> + return IRQ_RETVAL(!error);
> +}
> +
> +
> +static irqreturn_t cs40l50_vddb_uv(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "VDD_B undervoltage error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_current_limit(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "Boost current limit engagement\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_short(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "Boost inductor short error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_boost_uv(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "Boost undervoltage error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_overtemperature(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "Overtemperature error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_amp_short(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "Amp short error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static irqreturn_t cs40l50_global_error(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "MSM chip error\n");
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> + CS40L50_IRQ(CS40L50_AMP_SHORT_IRQ, "Amp short", cs40l50_amp_short),
> + CS40L50_IRQ(CS40L50_VIRT2_MBOX_IRQ, "Firmware interrupt", cs40l50_handle_mbox_buffer),
> + CS40L50_IRQ(CS40L50_TEMP_ERR_IRQ, "Overtemperature", cs40l50_overtemperature),
> + CS40L50_IRQ(CS40L50_BST_UVP_IRQ, "Boost undervoltage", cs40l50_boost_uv),
> + CS40L50_IRQ(CS40L50_BST_SHORT_IRQ, "Boost short", cs40l50_boost_short),
> + CS40L50_IRQ(CS40L50_BST_ILIMIT_IRQ, "Boost current limit", cs40l50_boost_current_limit),
> + CS40L50_IRQ(CS40L50_UVLO_VDDBATT_IRQ, "VDD_B undervoltage", cs40l50_vddb_uv),
> + CS40L50_IRQ(CS40L50_GLOBAL_ERROR_IRQ, "Global error", cs40l50_global_error),
> +};
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> + CS40L50_REG_IRQ(IRQ1_INT_1, AMP_SHORT),
> + CS40L50_REG_IRQ(IRQ1_INT_2, VIRT2_MBOX),
> + CS40L50_REG_IRQ(IRQ1_INT_8, TEMP_ERR),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_UVP),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_SHORT),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_ILIMIT),
> + CS40L50_REG_IRQ(IRQ1_INT_10, UVLO_VDDBATT),
> + CS40L50_REG_IRQ(IRQ1_INT_18, GLOBAL_ERROR),
> +};
> +
> +static const struct regmap_irq_chip cs40l50_regmap_irq_chip = {
> + .name = "cs40l50 IRQ1 Controller",
> + .status_base = CS40L50_IRQ1_INT_1,
> + .mask_base = CS40L50_IRQ1_MASK_1,
> + .ack_base = CS40L50_IRQ1_INT_1,
> + .num_regs = 19,
> + .irqs = cs40l50_reg_irqs,
> + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
> + .runtime_pm = true,
> +};
> +
> +static const struct reg_sequence cs40l50_ext_bst_seq[] = {
> + { CS40L50_BLOCK_ENABLES, CS40L50_MON_ENABLE },
> + { CS40L50_MON_VALUE_CTRL, CS40L50_VBST_VDD_AMP },
> + { CS40L50_TEST_KEY, CS40L50_KEY_SET_1 },
> + { CS40L50_TEST_KEY, CS40L50_KEY_SET_2 },
> + { CS40L50_TEST_VIS_SPARE, CS40L50_TEMP_SENSOR_3 },
> + { CS40L50_MON_ERROR_OVERIDE, CS40L50_TEMP_ERR_THLD },
> + { CS40L50_STATUS, CS40L50_TEMP_WARN_THLD },
> + { CS40L50_TEST_CONFIG, CS40L50_TEMP_FILT_DEBOUNCE_OFF },
> + { CS40L50_TEMP_WARN_CONFIG, CS40L50_TEMP_WARN_ATK_VOL },
> +};
> +
> +static int cs40l50_config_bst(struct cs40l50_private *cs40l50)
> +{
> + int error, i;
> +
> + if (device_property_present(cs40l50->dev, "cirrus,external-boost")) {
> + error = regmap_multi_reg_write(cs40l50->regmap, cs40l50_ext_bst_seq,
> + ARRAY_SIZE(cs40l50_ext_bst_seq));
> + if (error)
> + return error;
> +
> + for (i = 0; i < ARRAY_SIZE(cs40l50_ext_bst_seq); i++) {
> + error = cs40l50_pseq_write(cs40l50, cs40l50_ext_bst_seq[i].reg,
> + cs40l50_ext_bst_seq[i].def);
> + if (error)
> + return error;
> + }
> +
> + return 0;
> + }
> +
> + /* Internal boost config */
This driver is conspicuously void of comments and documentation; why is
this line mysterious enough to warrant one?
> + return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
> +}
> +
> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
> +{
> + const struct firmware *bin = NULL, *wmfw = NULL;
> + int error = 0;
> +
> + if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
> + dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
> +
> + if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
> + dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
> +
> + if (wmfw) {
It is a much more common design pattern to bail if request_firmware() returns
an error, than to proceed and check against the FW pointer remaining NULL.
Is this done because cs_dsp_power_up() must be called, with or without a wmfw
or coefficient file being available?
> + error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
> + CS40L50_DSP1_CLOCK_DISABLE);
> + if (error)
> + goto err_fw;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_RAM_INIT,
> + CS40L50_DSP1_RAM_INIT_FLAG);
> + if (error)
> + goto err_fw;
> +
> + error = cs40l50_dsp_write(cs40l50, CS40L50_PWRMGT_CTL,
> + CS40L50_DSP1_MEM_RDY_HW);
> + if (error)
> + goto err_fw;
> + }
> +
> + mutex_lock(&cs40l50->lock);
> +
> + cs_dsp_power_up(&cs40l50->dsp, wmfw, "cs40l50.wmfw", bin, "cs40l50.bin", "cs40l50");
> +
> + mutex_unlock(&cs40l50->lock);
> +
> + if (wmfw)
> + error = cs40l50_dsp_write(cs40l50, CS40L50_CCM_CORE_CONTROL,
> + CS40L50_DSP1_CLOCK_ENABLE);
> +
> +err_fw:
> + release_firmware(wmfw);
> + release_firmware(bin);
> +
> + return error;
> +}
> +
> +static int cs40l50_pseq_init(struct cs40l50_private *cs40l50)
> +{
> + struct cs40l50_pseq_op *pseq_op;
> + int error, i, num_words;
> + u8 operation;
> + u32 *words;
> +
> + INIT_LIST_HEAD(&cs40l50->pseq_op_head);
> +
> + words = kcalloc(CS40L50_PSEQ_MAX_WORDS, sizeof(u32), GFP_KERNEL);
> + if (IS_ERR_OR_NULL(words))
> + return -ENOMEM;
Just curious what is the need to call IS_ERR_OR_NULL() here and throughout
after allocation rather than just !foo ?
> +
> + error = regmap_bulk_read(cs40l50->regmap, CS40L50_DSP1_POWER_ON_SEQ, words,
> + CS40L50_PSEQ_MAX_WORDS);
> + if (error)
> + goto err_free;
> +
> + for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i++)
> + words[i] = be32_to_cpu(words[i]);
> +
> + for (i = 0; i < CS40L50_PSEQ_MAX_WORDS; i += num_words) {
> + operation = (words[i] & CS40L50_PSEQ_OP_MASK) >> CS40L50_PSEQ_OP_SHIFT;
> +
> + switch (operation) {
> + case CS40L50_PSEQ_OP_END:
> + num_words = CS40L50_PSEQ_OP_END_WORDS;
> + break;
> + case CS40L50_PSEQ_OP_WRITE_ADDR8:
> + case CS40L50_PSEQ_OP_WRITE_H16:
> + case CS40L50_PSEQ_OP_WRITE_L16:
> + num_words = CS40L50_PSEQ_OP_WRITE_X16_WORDS;
> + break;
> + case CS40L50_PSEQ_OP_WRITE_FULL:
> + num_words = CS40L50_PSEQ_OP_WRITE_FULL_WORDS;
> + break;
> + default:
> + dev_err(cs40l50->dev, "Invalid OP code 0x%02X\n", operation);
> + error = -EINVAL;
> + goto err_free;
> + }
> +
> + pseq_op = devm_kzalloc(cs40l50->dev, sizeof(*pseq_op), GFP_KERNEL);
> + if (!pseq_op) {
> + error = -ENOMEM;
> + goto err_free;
> + }
> +
> + memcpy(pseq_op->words, &words[i], num_words * sizeof(u32));
> + pseq_op->size = num_words;
> + pseq_op->offset = i * sizeof(u32);
> + pseq_op->operation = operation;
> + list_add(&pseq_op->list, &cs40l50->pseq_op_head);
> +
> + if (operation == CS40L50_PSEQ_OP_END)
> + break;
> + }
> +
> + if (operation != CS40L50_PSEQ_OP_END) {
> + dev_err(cs40l50->dev, "PSEQ_END_OF_SCRIPT not found\n");
> + error = -ENOENT;
> + }
> +
> +err_free:
> + kfree(words);
> +
> + return error;
> +}
> +
> +static int cs40l50_input_init(struct cs40l50_private *cs40l50)
> +{
> + int error;
> + u32 val;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DEVID, &val);
> + if (error)
> + return error;
> +
> + if (val != CS40L50_DEVID_A) {
> + dev_err(cs40l50->dev, "Invalid device ID: %#010X\n", val);
> + return -EINVAL;
> + }
> +
> + cs40l50->input->id.product = val & 0xFFFF;
> +
> + error = cs40l50_dsp_read(cs40l50, CS40L50_REVID, &val);
> + if (error)
> + return error;
> +
> + if (val != CS40L50_REVID_B0) {
> + dev_err(cs40l50->dev, "Invalid device revision: %#04X\n", val);
> + return -EINVAL;
> + }
> +
> + cs40l50->input->id.version = val;
> +
> + cs40l50->input = devm_input_allocate_device(cs40l50->dev);
> + if (IS_ERR_OR_NULL(cs40l50->input))
> + return cs40l50->input ? PTR_ERR(cs40l50->input) : -ENOMEM;
> +
> + cs40l50->input->name = "cs40l50_input";
> + input_set_drvdata(cs40l50->input, cs40l50);
> + input_set_capability(cs40l50->input, EV_FF, FF_PERIODIC);
> + input_set_capability(cs40l50->input, EV_FF, FF_CUSTOM);
> +
> + error = input_ff_create(cs40l50->input, FF_MAX_EFFECTS);
> + if (error)
> + return error;
> +
> + clear_bit(FF_RUMBLE, cs40l50->input->ffbit);
If there is a HW limitation that prevents this capability, it would be
handy to explain as much with a comment.
> +
> + cs40l50->input->ff->upload = cs40l50_add_effect;
> + cs40l50->input->ff->playback = cs40l50_playback_effect;
> + cs40l50->input->ff->erase = cs40l50_erase_effect;
> +
> + INIT_LIST_HEAD(&cs40l50->effect_head);
> +
> + return input_register_device(cs40l50->input);
> +}
> +
> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
> +
> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_DSP1_PMEM_0 },
> + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_DSP1_XMEM_PACKED_0 },
> + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_DSP1_YMEM_PACKED_0 },
> + { .type = WMFW_ADSP2_XM, .base = CS40L50_DSP1_XMEM_UNPACKED24_0 },
> + { .type = WMFW_ADSP2_YM, .base = CS40L50_DSP1_YMEM_UNPACKED24_0 },
> +};
This is more readable as:
[...],
{
.type = ...,
.base = ...,
},
[...],
> +
> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
> +{
> + cs40l50->dsp.num = 1;
> + cs40l50->dsp.type = WMFW_HALO;
> + cs40l50->dsp.dev = cs40l50->dev;
> + cs40l50->dsp.regmap = cs40l50->regmap;
> + cs40l50->dsp.base = CS40L50_DSP1_CORE_BASE;
> + cs40l50->dsp.base_sysinfo = CS40L50_DSP1_SYS_INFO_ID;
> + cs40l50->dsp.mem = cs40l50_dsp_regions;
> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> + cs40l50->dsp.lock_regions = 0xFFFFFFFF;
> + cs40l50->dsp.no_core_startstop = true;
> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> +
> + return cs_dsp_halo_init(&cs40l50->dsp);
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> + int error, i, irq;
> + u32 val;
> +
> + mutex_init(&cs40l50->lock);
> +
> + error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error)
> + return dev_err_probe(cs40l50->dev, error, "Failed to request supplies\n");
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> + if (error)
> + return dev_err_probe(cs40l50->dev, error, "Failed to enable supplies\n");
> +
> + cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(cs40l50->reset_gpio)) {
> + error = PTR_ERR(cs40l50->reset_gpio);
> + goto err;
> + }
Krzysztof covered this already, but I just want to re-iterate that you are
kind of swallowing your own head by calling cs40l50_remove() in your error
path. This is a confusing and uncommon design pattern.
You cannot have possibly booted the DSP or allocated work here, so why not
just return?
> +
> + usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> + usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> + if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> + break;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> + dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> + goto err;
> + }
> +
> + cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> + if (!cs40l50->vibe_workqueue) {
> + error = -ENOMEM;
> + goto err;
> + }
> +
> + INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> + INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> + error = cs40l50_cs_dsp_init(cs40l50);
> + if (error)
> + goto err;
It seems your error path is complex because you intend to unwind the DSP
in case of an error, in which case any teardown should be explicitly
called from the bottom of probe() rather than calling remove() on your
own.
Opinions may differ here, but mine is that you need not go out of your
way to power down the DSP if glaring errors are encountered during probe.
If you cannot read HALO_STATE, do you really expect to be able to do any
additional work anyway?
Understanding that haptics bears the risk of a motor stuck on, perhaps it
is sufficient to simply throw the device into reset if things go afoul?
> +
> + error = cs40l50_input_init(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_patch_firmware(cs40l50);
> + if (error)
> + goto err;
I just want to re-iterate Charles' comment; this _will_not_ work across
all customer platforms because you cannot guarantee the filesystem is
ready here if the driver is built-in.
In the best case you will get -ENOENT right away; in the worst case you
will get a deadlock. If this device does not have nonvolatile memory and
you must load FW from probe, use request_firmware_nowait() instead.
> +
> + error = cs40l50_pseq_init(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_config_bst(cs40l50);
> + if (error)
> + goto err;
> +
> + error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
> + &cs40l50->irq_data);
We should not be mandating IRQ polarity in the driver. Sometimes the customer
may use a GPIO expander with inverted polarity; we need to let dts govern the
polarity.
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
> + goto err;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> + irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> + if (irq < 0) {
> + dev_err(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
> + error = irq;
> + goto err;
> + }
> +
> + error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
> + cs40l50_irqs[i].name, cs40l50);
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
> + cs40l50_irqs[i].name, error);
> + goto err;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + cs40l50_remove(cs40l50);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> + disable_irq(cs40l50->irq);
> +
> + if (cs40l50->dsp.booted)
> + cs_dsp_power_down(&cs40l50->dsp);
> + if (&cs40l50->dsp)
> + cs_dsp_remove(&cs40l50->dsp);
> +
> + if (cs40l50->vibe_workqueue) {
> + flush_workqueue(cs40l50->vibe_workqueue);
> + destroy_workqueue(cs40l50->vibe_workqueue);
> + }
> +
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/cs40l50.h b/include/linux/input/cs40l50.h
> new file mode 100644
> index 000000000000..f2c8a101fa00
> --- /dev/null
> +++ b/include/linux/input/cs40l50.h
> @@ -0,0 +1,321 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/bitops.h>
bitops.h is quite outdated; did you mean bits.h?
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/firmware/cirrus/wmfw.h>
> +
> +/* Boost */
> +#define CS40L50_TEST_KEY 0x40
> +#define CS40L50_BLOCK_ENABLES 0x2018
> +#define CS40L50_ERROR_RELEASE 0x2034
> +#define CS40L50_PWRMGT_CTL 0x2900
> +#define CS40L50_BST_LPMODE_SEL 0x3810
> +#define CS40L50_STATUS 0x4200
> +#define CS40L50_MON_ERROR_OVERIDE 0x4220
> +#define CS40L50_TEST_CONFIG 0x4240
> +#define CS40L50_MON_VALUE_CTRL 0x4404
> +#define CS40L50_TEST_VIS_SPARE 0x5C00
> +#define CS40L50_TEMP_WARN_CONFIG 0x6024
> +#define CS40L50_KEY_SET_1 0x55
> +#define CS40L50_KEY_SET_2 0xAA
> +#define CS40L50_TEMP_SENSOR_3 0x400
> +#define CS40L50_DCM_LOW_POWER 0x1
> +#define CS40L50_MON_ENABLE 0x3201
> +#define CS40L50_VBST_VDD_AMP 0x1000000
> +#define CS40L50_TEMP_ERR_THLD 0x8000007D
> +#define CS40L50_TEMP_WARN_THLD 0x8
> +#define CS40L50_TEMP_FILT_DEBOUNCE_OFF 0x510002B5
> +#define CS40L50_TEMP_WARN_ATK_VOL 0x522303
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1 0xE010
> +#define CS40L50_IRQ1_INT_2 0xE014
> +#define CS40L50_IRQ1_INT_3 0xE018
> +#define CS40L50_IRQ1_INT_4 0xE01C
> +#define CS40L50_IRQ1_INT_8 0xE02C
> +#define CS40L50_IRQ1_INT_9 0xE030
> +#define CS40L50_IRQ1_INT_10 0xE034
> +#define CS40L50_IRQ1_INT_18 0xE054
> +#define CS40L50_IRQ1_INT_19 0xE058
> +#define CS40L50_IRQ1_MASK_1 0xE090
> +#define CS40L50_IRQ1_MASK_2 0xE094
> +#define CS40L50_IRQ1_MASK_3 0xE098
> +#define CS40L50_IRQ1_MASK_8 0xE0AC
> +#define CS40L50_IRQ1_MASK_9 0xE0B0
> +#define CS40L50_IRQ1_MASK_10 0xE0B4
> +#define CS40L50_IRQ1_MASK_18 0xE0D4
> +#define CS40L50_IRQ1_MASK_19 0xE0D8
> +#define CS40L50_IRQ1_STS_1 0xE210
> +#define CS40L50_IRQ1_STS_2 0xE214
> +#define CS40L50_IRQ1_STS_3 0xE218
> +#define CS40L50_IRQ1_STS_8 0xE22C
> +#define CS40L50_IRQ1_STS_9 0xE230
> +#define CS40L50_IRQ1_STS_10 0xE234
> +#define CS40L50_IRQ1_STS_18 0xE254
> +#define CS40L50_IRQ1_STS_19 0xE258
> +#define CS40L50_AMP_SHORT_MASK BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK BIT(21)
> +#define CS40L50_TEMP_ERR_MASK BIT(31)
> +#define CS40L50_BST_UVP_MASK BIT(6)
> +#define CS40L50_BST_SHORT_MASK BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK BIT(15)
> +#define CS40L50_ERROR_RELEASE_MASK BIT(11)
> +#define CS40L50_BOOT_DONE_MASK BIT(1)
> +#define CS40L50_IRQ(_irq, _name, _hand) \
> + { \
> + .irq = _irq, \
> + .name = _name, \
> + .handler = _hand, \
> + }
> +#define CS40L50_REG_IRQ(_reg, _irq) \
> + [CS40L50_ ## _irq ## _IRQ] = { \
> + .reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1, \
> + .mask = CS40L50_ ## _irq ## _MASK \
> + }
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START 0x1000000
> +#define CS40L50_RAM_BANK_INDEX_END 0x100007F
> +#define CS40L50_OWT_INDEX_START 0x1400000
> +#define CS40L50_OWT_INDEX_END 0x140007F
> +#define CS40L50_ROM_BANK_INDEX_START 0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END 0x180001A
> +#define CS40L50_OTP_BUZZ 0x1800080
> +#define CS40L50_RAM_BUZZ_INDEX_START 0x1800081
> +#define CS40L50_RAM_BUZZ_INDEX_END 0x1800085
> +#define CS40L50_MBOX_CMD_HIBERNATE 0x2000001
> +#define CS40L50_MBOX_CMD_WAKEUP 0x2000002
> +#define CS40L50_MBOX_CMD_PREVENT_HIBERNATE 0x2000003
> +#define CS40L50_MBOX_CMD_ALLOW_HIBERNATION 0x2000004
> +#define CS40L50_MBOX_CMD_SHUTDOWN 0x2000005
> +#define CS40L50_MBOX_CMD_DURATION_REPORT 0x3000001
> +#define CS40L50_MBOX_CMD_START_I2S 0x3000002
> +#define CS40L50_MBOX_CMD_STOP_I2S 0x3000003
> +#define CS40L50_MBOX_CMD_HAPTICS_LOGGER_RESET 0x3000004
> +#define CS40L50_MBOX_CMD_OWT_PUSH 0x3000008
> +#define CS40L50_MBOX_CMD_OWT_RESET 0x3000009
> +#define CS40L50_MBOX_CMD_DVL_REINIT 0x300000A
> +#define CS40L50_MBOX_CMD_STOP_PLAYBACK 0x5000000
> +#define CS40L50_MBOX_CMD_F0_EST 0x7000001
> +#define CS40L50_MBOX_CMD_REDC_EST 0x7000002
> +#define CS40L50_MBOX_CMD_LE_EST 0x7000004
> +#define CS40L50_MBOX_CMD_OWT_DELETE 0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE 0x11004
> +#define CS40L50_MBOX_QUEUE_END 0x1101C
> +#define CS40L50_DSP_VIRTUAL1_MBOX_1 0x11020
> +#define CS40L50_DSP1_MBOX_QUEUE_WT 0x28042C8
> +#define CS40L50_DSP1_MBOX_QUEUE_RD 0x28042CC
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_MBOX 0x1000000
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_GPIO 0x1000001
> +#define CS40L50_DSP_MBOX_HAPTIC_COMPLETE_I2S 0x1000002
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_MBOX 0x1000010
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_GPIO 0x1000011
> +#define CS40L50_DSP_MBOX_HAPTIC_TRIGGER_I2S 0x1000012
> +#define CS40L50_DSP_MBOX_INIT 0x2000000
> +#define CS40L50_DSP_MBOX_AWAKE 0x2000002
> +#define CS40L50_DSP_MBOX_F0_EST_START 0x7000011
> +#define CS40L50_DSP_MBOX_F0_EST_DONE 0x7000021
> +#define CS40L50_DSP_MBOX_REDC_EST_START 0x7000012
> +#define CS40L50_DSP_MBOX_REDC_EST_DONE 0x7000022
> +#define CS40L50_DSP_MBOX_LE_EST_START 0x7000014
> +#define CS40L50_DSP_MBOX_LE_EST_DONE 0x7000024
> +#define CS40L50_DSP_MBOX_ACK 0xA000000
> +#define CS40L50_DSP_MBOX_PANIC 0xC000000
> +#define CS40L50_DSP_MBOX_WATERMARK 0xD000000
> +#define CS40L50_DSP_MBOX_ERR_EVENT_UNMAPPED 0xC0004B3
> +#define CS40L50_DSP_MBOX_ERR_EVENT_MODIFY 0xC0004B4
> +#define CS40L50_DSP_MBOX_ERR_NULLPTR 0xC0006A5
> +#define CS40L50_DSP_MBOX_ERR_BRAKING 0xC0006A8
> +#define CS40L50_DSP_MBOX_ERR_INVAL_SRC 0xC0006AC
> +#define CS40L50_DSP_MBOX_ERR_ENABLE_RANGE 0xC000836
> +#define CS40L50_DSP_MBOX_ERR_GPIO_UNMAPPED 0xC000837
> +#define CS40L50_DSP_MBOX_ERR_ISR_RANGE 0xC000838
> +#define CS40L50_DSP_MBOX_PERMANENT_SHORT 0xC000C1C
> +#define CS40L50_DSP_MBOX_RUNTIME_SHORT 0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0 0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0 0x2400000
> +#define CS40L50_DSP1_SYS_INFO_ID 0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0 0x2800000
> +#define CS40L50_NUM_OF_WAVES 0x280CB4C
> +#define CS40L50_DSP1_CORE_BASE 0x2B80000
> +#define CS40L50_DSP1_SCRATCH1 0x2B805C0
> +#define CS40L50_DSP1_SCRATCH2 0x2B805C8
> +#define CS40L50_DSP1_SCRATCH3 0x2B805D0
> +#define CS40L50_DSP1_SCRATCH4 0x2B805D8
I didn't look too closely, but some of these #defines are unused.
> +#define CS40L50_CCM_CORE_CONTROL 0x2BC1000
> +#define CS40L50_RAM_INIT 0x28021DC
> +#define CS40L50_DSP1_YMEM_PACKED_0 0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0 0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0 0x3400000
> +#define CS40L50_DSP1_PMEM_0 0x3800000
> +#define CS40L50_DSP1_PMEM_5114 0x3804FE8
> +#define CS40L50_DSP1_MEM_RDY_HW 0x2
> +#define CS40L50_DSP1_RAM_INIT_FLAG 0x1
> +#define CS40L50_DSP1_CLOCK_DISABLE 0x80
> +#define CS40L50_DSP1_CLOCK_ENABLE 0x281
> +#define CS40L50_DSP_POLL_US 1000
> +#define CS40L50_DSP_TIMEOUT_COUNT 100
> +#define CS40L50_MBOX_POLL_US 5000
> +
> +/* Calibration */
> +#define CS40L50_DSP1_REDC_ESTIMATION 0x2802F7C
> +#define CS40L50_DSP1_F0_ESTIMATION 0x2802F84
> +#define CS40L50_DSP1_DYNAMIC_F0_ENABLE 0x2802F8C
> +#define CS40L50_DSP1_DYNAMIC_F0_THRESHOLD 0x2802F90
> +#define CS40L50_DSP1_F0_STORED 0x2805C00
> +#define CS40L50_DSP1_REDC_STORED 0x2805C04
> +#define CS40L50_DSP1_RE_EST_STATUS 0x3401B40
> +#define CS40L50_REDC_EST_DELAY_US 30000
> +#define CS40L50_F0_EST_DELAY_US 10000
> +
> +/* OWT */
> +#define CS40L50_DSP1_OWT_SIZE 0x2805C38
> +#define CS40L50_DSP1_OWT_NEXT 0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES 0x2805C40
> +#define CS40L50_DSP1_OWT_BASE 0x2805C34
> +#define CS40L50_MAX_PWLE_SECTIONS 126
> +#define CS40L50_CUSTOM_DATA_SIZE 2
> +#define CS40L50_WT_HEADER_PWLE_SIZE 12
> +#define CS40L50_WT_HEADER_DEFAULT_FLAGS 0x0000
> +#define CS40L50_WT_HEADER_OFFSET 3
> +#define CS40L50_WT_TYPE_PCM 8
> +#define CS40L50_WT_TYPE_PWLE 12
> +#define CS40L50_PCM_ID 0x0000
> +
> +/* GPIO */
> +#define CS40L50_DSP1_GPIO_BASE 0x2804140
> +#define CS40L50_GPIO_MAPPING_INVALID 0
> +#define CS40L50_GPI_DISABLE 0x1FF
> +#define CS40L50_BTN_NUM_MASK GENMASK(14, 12)
> +#define CS40L50_BTN_EDGE_MASK BIT(15)
> +
> +/* State */
> +#define CS40L50_DSP1_HALO_STATE 0x28021E0
> +#define CS40L50_HALO_STATE_BOOT_DONE 2
> +#define CS40L50_MIN_RESET_PULSE_WIDTH_US 1100
> +#define CS40L50_CP_READY_DELAY_US 2200
> +
> +/* Device */
> +#define CS40L50_DEVID 0x0
> +#define CS40L50_REVID 0x4
> +#define CS40L50_DEVID_A 0x40A50
> +#define CS40L50_REVID_B0 0xB0
> +
> +/* Write sequencer */
> +#define CS40L50_DSP1_POWER_ON_SEQ 0x280C320
> +#define CS40L50_PSEQ_MAX_WORDS 200
> +#define CS40L50_PSEQ_OP_MASK GENMASK(23, 16)
> +#define CS40L50_PSEQ_OP_SHIFT 16
> +#define CS40L50_PSEQ_OP_WRITE_ADDR8 0x02
> +#define CS40L50_PSEQ_OP_WRITE_L16 0x04
> +#define CS40L50_PSEQ_OP_WRITE_H16 0x05
> +#define CS40L50_PSEQ_OP_WRITE_FULL 0x00
> +#define CS40L50_PSEQ_OP_WRITE_FULL_WORDS 3
> +#define CS40L50_PSEQ_OP_WRITE_X16_WORDS 2
> +#define CS40L50_PSEQ_OP_END 0xFF
> +#define CS40L50_PSEQ_OP_END_WORDS 1
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_SHIFT 8
> +#define CS40L50_PSEQ_WRITE_FULL_UPPER_ADDR_SHIFT 16
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_ADDR_MASK GENMASK(15, 0)
> +#define CS40L50_PSEQ_WRITE_FULL_UPPER_DATA_SHIFT 24
> +#define CS40L50_PSEQ_WRITE_FULL_LOWER_DATA_MASK GENMASK(23, 0)
> +
> +enum cs40l50_wvfrm_bank {
> + CS40L50_WVFRM_BANK_RAM,
> + CS40L50_WVFRM_BANK_ROM,
> + CS40L50_WVFRM_BANK_OWT,
> + CS40L50_WVFRM_BANK_BUZ,
> + CS40L50_WVFRM_BANK_NUM,
> +};
> +
> +enum cs40l50_irq_list {
> + CS40L50_GLOBAL_ERROR_IRQ,
> + CS40L50_UVLO_VDDBATT_IRQ,
> + CS40L50_BST_ILIMIT_IRQ,
> + CS40L50_BST_SHORT_IRQ,
> + CS40L50_BST_UVP_IRQ,
> + CS40L50_TEMP_ERR_IRQ,
> + CS40L50_VIRT2_MBOX_IRQ,
> + CS40L50_AMP_SHORT_IRQ,
> + CS40L50_NUM_IRQ
> +};
> +
> +union cs40l50_buzzgen {
> + u32 words[3];
> + struct {
> + u32 freq;
> + u32 level;
> + u32 duration;
> + } params;
> +};
> +
> +struct cs40l50_owt_header {
> + u32 wvfrm_type;
> + u32 offset;
> + u32 data_size;
> +};
> +
> +struct cs40l50_irq {
> + int irq;
> + const char *name;
> + irqreturn_t (*handler)(int irq, void *data);
> +};
> +
> +struct cs40l50_effect {
> + int id;
> + u32 trigger_index;
> + enum cs40l50_wvfrm_bank wvfrm_bank;
> + u32 mapping;
> + struct list_head list;
> +};
> +
> +struct cs40l50_pseq_op {
> + u16 offset;
> + u8 operation;
> + u32 words[3];
> + u32 size;
> + struct list_head list;
> +};
> +
> +struct cs40l50_private {
> + struct device *dev;
> + struct regmap *regmap;
> + struct cs_dsp dsp;
> + struct mutex lock;
> + struct gpio_desc *reset_gpio;
> + struct input_dev *input;
> + struct list_head effect_head;
> + struct ff_effect *trigger_effect;
> + struct workqueue_struct *vibe_workqueue;
> + struct work_struct vibe_start_work;
> + struct work_struct vibe_stop_work;
> + struct regmap_irq_chip_data *irq_data;
> + struct list_head pseq_op_head;
> + int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +
> +#endif /* __CS40L50_H__ */
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox