* [PATCH v2 1/4] HID: hid-picolcd_core: Convert sprintf/snprintf to sysfs_emit
From: Li Zhijian @ 2024-03-14 8:47 UTC (permalink / raw)
To: linux-kernel
Cc: Li Zhijian, Bruno Prémont, Jiri Kosina, Benjamin Tissoires,
linux-input
Per filesystems/sysfs.rst, show() should only use sysfs_emit()
or sysfs_emit_at() when formatting the value to be returned to user space.
coccinelle complains that there are still a couple of functions that use
snprintf(). Convert them to sysfs_emit().
sprintf() will be converted as weel if they have.
Generally, this patch is generated by
make coccicheck M=<path/to/file> MODE=patch \
COCCI=scripts/coccinelle/api/device_attr_show.cocci
No functional change intended
CC: "Bruno Prémont" <bonbons@linux-vserver.org>
CC: Jiri Kosina <jikos@kernel.org>
CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
CC: linux-input@vger.kernel.org
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
This is a part of the work "Fix coccicheck device_attr_show warnings"[1]
Split them per subsystem so that the maintainer can review it easily
[1] https://lore.kernel.org/lkml/20240116041129.3937800-1-lizhijian@fujitsu.com/
---
drivers/hid/hid-picolcd_core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index bbda231a7ce3..fa46fb6eab3f 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -256,9 +256,9 @@ static ssize_t picolcd_operation_mode_show(struct device *dev,
struct picolcd_data *data = dev_get_drvdata(dev);
if (data->status & PICOLCD_BOOTLOADER)
- return snprintf(buf, PAGE_SIZE, "[bootloader] lcd\n");
+ return sysfs_emit(buf, "[bootloader] lcd\n");
else
- return snprintf(buf, PAGE_SIZE, "bootloader [lcd]\n");
+ return sysfs_emit(buf, "bootloader [lcd]\n");
}
static ssize_t picolcd_operation_mode_store(struct device *dev,
@@ -301,7 +301,7 @@ static ssize_t picolcd_operation_mode_delay_show(struct device *dev,
{
struct picolcd_data *data = dev_get_drvdata(dev);
- return snprintf(buf, PAGE_SIZE, "%hu\n", data->opmode_delay);
+ return sysfs_emit(buf, "%hu\n", data->opmode_delay);
}
static ssize_t picolcd_operation_mode_delay_store(struct device *dev,
--
2.29.2
^ permalink raw reply related
* [PATCH v2 3/4] HID: roccat: Convert sprintf/snprintf to sysfs_emit
From: Li Zhijian @ 2024-03-14 8:47 UTC (permalink / raw)
To: linux-kernel
Cc: Li Zhijian, Stefan Achatz, Jiri Kosina, Benjamin Tissoires,
linux-input
In-Reply-To: <20240314084753.1322110-1-lizhijian@fujitsu.com>
Per filesystems/sysfs.rst, show() should only use sysfs_emit()
or sysfs_emit_at() when formatting the value to be returned to user space.
coccinelle complains that there are still a couple of functions that use
snprintf(). Convert them to sysfs_emit().
sprintf() will be converted as weel if they have.
Generally, this patch is generated by
make coccicheck M=<path/to/file> MODE=patch \
COCCI=scripts/coccinelle/api/device_attr_show.cocci
No functional change intended
CC: Stefan Achatz <erazor_de@users.sourceforge.net>
CC: Jiri Kosina <jikos@kernel.org>
CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
CC: linux-input@vger.kernel.org
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
This is a part of the work "Fix coccicheck device_attr_show warnings"[1]
Split them per subsystem so that the maintainer can review it easily
[1] https://lore.kernel.org/lkml/20240116041129.3937800-1-lizhijian@fujitsu.com/
---
drivers/hid/hid-roccat-isku.c | 2 +-
drivers/hid/hid-roccat-kone.c | 12 ++++++------
drivers/hid/hid-roccat-koneplus.c | 4 ++--
drivers/hid/hid-roccat-kovaplus.c | 10 +++++-----
drivers/hid/hid-roccat-pyra.c | 6 +++---
5 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/hid/hid-roccat-isku.c b/drivers/hid/hid-roccat-isku.c
index 458060403397..0cd6208fb371 100644
--- a/drivers/hid/hid-roccat-isku.c
+++ b/drivers/hid/hid-roccat-isku.c
@@ -61,7 +61,7 @@ static ssize_t isku_sysfs_show_actual_profile(struct device *dev,
{
struct isku_device *isku =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", isku->actual_profile);
+ return sysfs_emit(buf, "%d\n", isku->actual_profile);
}
static ssize_t isku_sysfs_set_actual_profile(struct device *dev,
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 00a1abc7e839..3f8f459edcf3 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -400,7 +400,7 @@ static ssize_t kone_sysfs_show_actual_profile(struct device *dev,
{
struct kone_device *kone =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kone->actual_profile);
+ return sysfs_emit(buf, "%d\n", kone->actual_profile);
}
static DEVICE_ATTR(actual_profile, 0440, kone_sysfs_show_actual_profile, NULL);
@@ -409,7 +409,7 @@ static ssize_t kone_sysfs_show_actual_dpi(struct device *dev,
{
struct kone_device *kone =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kone->actual_dpi);
+ return sysfs_emit(buf, "%d\n", kone->actual_dpi);
}
static DEVICE_ATTR(actual_dpi, 0440, kone_sysfs_show_actual_dpi, NULL);
@@ -432,7 +432,7 @@ static ssize_t kone_sysfs_show_weight(struct device *dev,
if (retval)
return retval;
- return snprintf(buf, PAGE_SIZE, "%d\n", weight);
+ return sysfs_emit(buf, "%d\n", weight);
}
static DEVICE_ATTR(weight, 0440, kone_sysfs_show_weight, NULL);
@@ -441,7 +441,7 @@ static ssize_t kone_sysfs_show_firmware_version(struct device *dev,
{
struct kone_device *kone =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kone->firmware_version);
+ return sysfs_emit(buf, "%d\n", kone->firmware_version);
}
static DEVICE_ATTR(firmware_version, 0440, kone_sysfs_show_firmware_version,
NULL);
@@ -451,7 +451,7 @@ static ssize_t kone_sysfs_show_tcu(struct device *dev,
{
struct kone_device *kone =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kone->settings.tcu);
+ return sysfs_emit(buf, "%d\n", kone->settings.tcu);
}
static int kone_tcu_command(struct usb_device *usb_dev, int number)
@@ -553,7 +553,7 @@ static ssize_t kone_sysfs_show_startup_profile(struct device *dev,
{
struct kone_device *kone =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kone->settings.startup_profile);
+ return sysfs_emit(buf, "%d\n", kone->settings.startup_profile);
}
static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c
index 22b895436a7c..8ccb3b14a1a9 100644
--- a/drivers/hid/hid-roccat-koneplus.c
+++ b/drivers/hid/hid-roccat-koneplus.c
@@ -242,7 +242,7 @@ static ssize_t koneplus_sysfs_show_actual_profile(struct device *dev,
{
struct koneplus_device *koneplus =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", koneplus->actual_profile);
+ return sysfs_emit(buf, "%d\n", koneplus->actual_profile);
}
static ssize_t koneplus_sysfs_set_actual_profile(struct device *dev,
@@ -309,7 +309,7 @@ static ssize_t koneplus_sysfs_show_firmware_version(struct device *dev,
&info, KONEPLUS_SIZE_INFO);
mutex_unlock(&koneplus->koneplus_lock);
- return snprintf(buf, PAGE_SIZE, "%d\n", info.firmware_version);
+ return sysfs_emit(buf, "%d\n", info.firmware_version);
}
static DEVICE_ATTR(firmware_version, 0440,
koneplus_sysfs_show_firmware_version, NULL);
diff --git a/drivers/hid/hid-roccat-kovaplus.c b/drivers/hid/hid-roccat-kovaplus.c
index 86af538c10d6..748d4d7cb2fc 100644
--- a/drivers/hid/hid-roccat-kovaplus.c
+++ b/drivers/hid/hid-roccat-kovaplus.c
@@ -272,7 +272,7 @@ static ssize_t kovaplus_sysfs_show_actual_profile(struct device *dev,
{
struct kovaplus_device *kovaplus =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kovaplus->actual_profile);
+ return sysfs_emit(buf, "%d\n", kovaplus->actual_profile);
}
static ssize_t kovaplus_sysfs_set_actual_profile(struct device *dev,
@@ -325,7 +325,7 @@ static ssize_t kovaplus_sysfs_show_actual_cpi(struct device *dev,
{
struct kovaplus_device *kovaplus =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kovaplus->actual_cpi);
+ return sysfs_emit(buf, "%d\n", kovaplus->actual_cpi);
}
static DEVICE_ATTR(actual_cpi, 0440, kovaplus_sysfs_show_actual_cpi, NULL);
@@ -334,7 +334,7 @@ static ssize_t kovaplus_sysfs_show_actual_sensitivity_x(struct device *dev,
{
struct kovaplus_device *kovaplus =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kovaplus->actual_x_sensitivity);
+ return sysfs_emit(buf, "%d\n", kovaplus->actual_x_sensitivity);
}
static DEVICE_ATTR(actual_sensitivity_x, 0440,
kovaplus_sysfs_show_actual_sensitivity_x, NULL);
@@ -344,7 +344,7 @@ static ssize_t kovaplus_sysfs_show_actual_sensitivity_y(struct device *dev,
{
struct kovaplus_device *kovaplus =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", kovaplus->actual_y_sensitivity);
+ return sysfs_emit(buf, "%d\n", kovaplus->actual_y_sensitivity);
}
static DEVICE_ATTR(actual_sensitivity_y, 0440,
kovaplus_sysfs_show_actual_sensitivity_y, NULL);
@@ -365,7 +365,7 @@ static ssize_t kovaplus_sysfs_show_firmware_version(struct device *dev,
&info, KOVAPLUS_SIZE_INFO);
mutex_unlock(&kovaplus->kovaplus_lock);
- return snprintf(buf, PAGE_SIZE, "%d\n", info.firmware_version);
+ return sysfs_emit(buf, "%d\n", info.firmware_version);
}
static DEVICE_ATTR(firmware_version, 0440,
kovaplus_sysfs_show_firmware_version, NULL);
diff --git a/drivers/hid/hid-roccat-pyra.c b/drivers/hid/hid-roccat-pyra.c
index 5663b9cd9c69..eeb3d38cd805 100644
--- a/drivers/hid/hid-roccat-pyra.c
+++ b/drivers/hid/hid-roccat-pyra.c
@@ -283,7 +283,7 @@ static ssize_t pyra_sysfs_show_actual_cpi(struct device *dev,
{
struct pyra_device *pyra =
hid_get_drvdata(dev_get_drvdata(dev->parent->parent));
- return snprintf(buf, PAGE_SIZE, "%d\n", pyra->actual_cpi);
+ return sysfs_emit(buf, "%d\n", pyra->actual_cpi);
}
static DEVICE_ATTR(actual_cpi, 0440, pyra_sysfs_show_actual_cpi, NULL);
@@ -300,7 +300,7 @@ static ssize_t pyra_sysfs_show_actual_profile(struct device *dev,
&settings, PYRA_SIZE_SETTINGS);
mutex_unlock(&pyra->pyra_lock);
- return snprintf(buf, PAGE_SIZE, "%d\n", settings.startup_profile);
+ return sysfs_emit(buf, "%d\n", settings.startup_profile);
}
static DEVICE_ATTR(actual_profile, 0440, pyra_sysfs_show_actual_profile, NULL);
static DEVICE_ATTR(startup_profile, 0440, pyra_sysfs_show_actual_profile, NULL);
@@ -321,7 +321,7 @@ static ssize_t pyra_sysfs_show_firmware_version(struct device *dev,
&info, PYRA_SIZE_INFO);
mutex_unlock(&pyra->pyra_lock);
- return snprintf(buf, PAGE_SIZE, "%d\n", info.firmware_version);
+ return sysfs_emit(buf, "%d\n", info.firmware_version);
}
static DEVICE_ATTR(firmware_version, 0440, pyra_sysfs_show_firmware_version,
NULL);
--
2.29.2
^ permalink raw reply related
* Re: [PATCH v2 1/4] HID: hid-picolcd_core: Convert sprintf/snprintf to sysfs_emit
From: Zhijian Li (Fujitsu) @ 2024-03-14 10:39 UTC (permalink / raw)
To: Bruno Prémont
Cc: Jiri Kosina, Benjamin Tissoires, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20240314084753.1322110-1-lizhijian@fujitsu.com>
Sorry Bruno, i forgot to address your comments for V1, I will re-send a new version later.
Please ignore this one.
Thanks
Zhijian
On 14/03/2024 16:47, Li Zhijian wrote:
> Per filesystems/sysfs.rst, show() should only use sysfs_emit()
> or sysfs_emit_at() when formatting the value to be returned to user space.
>
> coccinelle complains that there are still a couple of functions that use
> snprintf(). Convert them to sysfs_emit().
>
> sprintf() will be converted as weel if they have.
>
> Generally, this patch is generated by
> make coccicheck M=<path/to/file> MODE=patch \
> COCCI=scripts/coccinelle/api/device_attr_show.cocci
>
> No functional change intended
>
> CC: "Bruno Prémont" <bonbons@linux-vserver.org>
> CC: Jiri Kosina <jikos@kernel.org>
> CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> CC: linux-input@vger.kernel.org
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> This is a part of the work "Fix coccicheck device_attr_show warnings"[1]
> Split them per subsystem so that the maintainer can review it easily
> [1] https://lore.kernel.org/lkml/20240116041129.3937800-1-lizhijian@fujitsu.com/
> ---
> drivers/hid/hid-picolcd_core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index bbda231a7ce3..fa46fb6eab3f 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -256,9 +256,9 @@ static ssize_t picolcd_operation_mode_show(struct device *dev,
> struct picolcd_data *data = dev_get_drvdata(dev);
>
> if (data->status & PICOLCD_BOOTLOADER)
> - return snprintf(buf, PAGE_SIZE, "[bootloader] lcd\n");
> + return sysfs_emit(buf, "[bootloader] lcd\n");
> else
> - return snprintf(buf, PAGE_SIZE, "bootloader [lcd]\n");
> + return sysfs_emit(buf, "bootloader [lcd]\n");
> }
>
> static ssize_t picolcd_operation_mode_store(struct device *dev,
> @@ -301,7 +301,7 @@ static ssize_t picolcd_operation_mode_delay_show(struct device *dev,
> {
> struct picolcd_data *data = dev_get_drvdata(dev);
>
> - return snprintf(buf, PAGE_SIZE, "%hu\n", data->opmode_delay);
> + return sysfs_emit(buf, "%hu\n", data->opmode_delay);
> }
>
> static ssize_t picolcd_operation_mode_delay_store(struct device *dev,
^ permalink raw reply
* Re: [PATCH v9 1/5] firmware: cs_dsp: Add write sequencer interface
From: Jeff LaBundy @ 2024-03-14 14:40 UTC (permalink / raw)
To: Charles Keepax
Cc: James Ogletree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, lee, broonie, patches, linux-sound, linux-input,
devicetree
In-Reply-To: <Ze7ZeY+FTPuk0zyE@ediswmail9.ad.cirrus.com>
Hi Charles,
On Mon, Mar 11, 2024 at 10:14:17AM +0000, Charles Keepax wrote:
> On Sun, Mar 10, 2024 at 02:12:16PM -0500, Jeff LaBundy wrote:
> > On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> > > + switch (op_code) {
> > > + case CS_DSP_WSEQ_FULL:
> > > + cs_dsp_chunk_write(&ch, 32, op_new->address);
> > > + cs_dsp_chunk_write(&ch, 32, op_new->data);
> > > + break;
> > > + case CS_DSP_WSEQ_L16:
> > > + case CS_DSP_WSEQ_H16:
> > > + cs_dsp_chunk_write(&ch, 24, op_new->address);
> > > + cs_dsp_chunk_write(&ch, 16, op_new->data);
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + cs_dsp_err(dsp, "Op code not supported: %X\n", op_code);
> > > + goto op_new_free;
> >
> > There is no need to drop down and call devm_kfree() here; it's sufficient
> > to simply return -EINVAL. The devres core will free any instances of
> > cs_dsp_wseq_op when the driver is unbound.
> >
> > I imagine you're calling devm_kfree() to protect against the case where
> > the driver successfully probes, but the contents of the firmware are found
> > to be invalid later. In that case, yes, this newly allocated memory will
> > persist throughout the length of the driver's life.
> >
> > That's an error condition though; if it happens, the customer will surely
> > remove the module, correct the .wmfw or .bin file, then insert the module
> > again. All we need to do here is make sure that memory does not leak after
> > the module is removed, and device-managed allocation already handles this.
> >
>
> I disagree here. This is the write function, there is no guarantee
> this is even called at probe time. This is generic code going
> into the DSP library and will presumably get re-used for different
> purposes and on different parts in the future. Freeing on the error
> path is much safer here.
Agreed that this won't be called during probe; all I mean to say is
that I don't see any issue in hanging on to a bit of memory while the
device is in an error state, before the module is unloaded and devres
unrolls all of the device-managed resources.
It's also perfectly fine to leave this as-is; the existing method is
functionally correct, and I understand the perspective of having the
generic library clean up immediately. If that's the intent however,
the same error handling needs to be applied in cs_dsp_populate_wseq();
currently only cs_dsp_wseq_write() calls devm_kfree() on error. Since
L50 uses asynchronous FW loading, neither are called until after the
device probes.
>
> > > +int cs_dsp_wseq_multi_write(struct cs_dsp *dsp, struct cs_dsp_wseq *wseq,
> > > + const struct reg_sequence *reg_seq, int num_regs,
> > > + u8 op_code, bool update)
> > > +{
> > > + int ret, i;
> > > +
> > > + for (i = 0; i < num_regs; i++) {
> > > + ret = cs_dsp_wseq_write(dsp, wseq, reg_seq[i].reg,
> > > + reg_seq[i].def, update, op_code);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> >
> > This is absolutely a nit-pick, but in cs_dsp_wseq_init(), you use the pattern
> > of breaking on error and always returning ret; here you return on error. Both
> > are functionally equivalent but it would be nice to be consistent in terms of
> > style.
> >
> > If you do opt to match cs_dsp_wseq_multi_write() to cs_dsp_wseq_init(), I do
> > think you'll need to initialize ret to zero here as well to avoid a compiler
> > warning for the num_regs = 0 case.
>
> I would be inclined to make cs_dsp_wseq_init function like this
> one personally, rather than the other way around. Generally best
> to return if there is no clean up to do.
Makes sense to me.
>
> Thanks,
> Charles
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v9 2/5] dt-bindings: input: cirrus,cs40l50: Add initial DT binding
From: Jeff LaBundy @ 2024-03-14 14:42 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: James Ogletree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, lee, broonie, patches, linux-sound, linux-input,
devicetree
In-Reply-To: <2914e313-1fb1-41f5-9db9-ce44dcd4903c@linaro.org>
Hi Krzysztof,
On Mon, Mar 11, 2024 at 07:31:36AM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2024 20:29, Jeff LaBundy wrote:
> >
> >> + - vio-supply
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> + - |
> >> + #include <dt-bindings/gpio/gpio.h>
> >> + #include <dt-bindings/interrupt-controller/irq.h>
> >> +
> >> + i2c {
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >
> > Nit: most device trees tend to use 8-column indentation as with kernel code.
>
> If you meant DTS, then kernel coding style applies, which does not use
> spaces. But this is a binding, so please use indentation as defined by
> writing-schema: 2 or 4 spaces.
Good to know; I'll follow this definition moving forward.
>
>
> Best regards,
> Krzysztof
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v9 3/5] mfd: cs40l50: Add support for CS40L50 core driver
From: Jeff LaBundy @ 2024-03-14 15:15 UTC (permalink / raw)
To: Charles Keepax
Cc: James Ogletree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, lee, broonie, patches, linux-sound, linux-input,
devicetree
In-Reply-To: <Ze7dXdVdZKN5Kmv/@ediswmail9.ad.cirrus.com>
Hi Charles,
On Mon, Mar 11, 2024 at 10:30:53AM +0000, Charles Keepax wrote:
> On Sun, Mar 10, 2024 at 06:40:04PM -0500, Jeff LaBundy wrote:
> > On Fri, Mar 08, 2024 at 10:24:19PM +0000, James Ogletree wrote:
> > > +static void cs40l50_start_dsp(const struct firmware *bin, void *context)
> > > +{
> > > + struct cs40l50 *cs40l50 = context;
> > > + int err;
> > > +
> > > + /* Wavetable is optional; start DSP regardless */
> > > + cs40l50->bin = bin;
> > > +
> > > + mutex_lock(&cs40l50->lock);
> >
> > It seems the mutex is used only to prevent interrupt handling while the DSP
> > is being configured; can't we just call disable_irq() and enable_irq() here?
> >
>
> The trouble with diabling the IRQ is it masks the IRQ line. That
> means if the IRQ is shared with other devices you will be
> silencing their IRQs for the duration as well, which is slightly
> rude. Especially given the driver requests the IRQ with the
> SHARED flag I would be inclinded to stick with the mutex unless
> there are other reasons not to.
Ah, yes; I see we're using IRQF_SHARED here. Shared interrupts seem
like they made more sense for L+R audio amplifiers, and less for a
haptic driver; but if customers are indeed sharing this interrupt,
then the mutex is OK.
>
> > > +static int cs40l50_irq_init(struct cs40l50 *cs40l50)
> > > +{
> > > + struct device *dev = cs40l50->dev;
> > > + int err, i, virq;
> > > +
> > > + err = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> > > + IRQF_ONESHOT | IRQF_SHARED, 0,
> > > + &cs40l50_irq_chip, &cs40l50->irq_data);
> > > + if (err) {
> > > + dev_err(dev, "Failed adding IRQ chip\n");
> >
> > I don't see any need for individual prints in this function, since the call
> > to cs40l50_irq_init() is already followed by a call to dev_err_probe().
>
> I would probably suggest keeping these individual prints here and
> removing the one in cs40l50_probe, it is nicer to know exactly
> what failed when something goes wrong. That said at least the
> devm_request_threaded_irq can probe defer so that print should be
> a dev_err_probe since this function is only called on the probe
> path.
Makes sense to me.
>
> > > + dev_info(cs40l50->dev, "Cirrus Logic CS40L50 rev. %02X\n", cs40l50->revid);
> >
> > This should be dev_dbg().
> >
>
> Not sure what the concenus is on this, but personally I greatly
> prefer these device ID prints being infos. Nice to always have
> some indication of the device and its version.
Right, but every other silicon vendor feels the same way too, and many
customers' dmesg is typically quite messy for the first couple minutes.
This is only one print, but my stance is that this type of information
belongs in debugfs; in fact for this case, we can get this information
from regmap if we know what we're looking for.
>
> Thanks,
> Charles
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 2/4] HID: hid-sensor-custom: Convert sprintf/snprintf to sysfs_emit
From: Jonathan Cameron @ 2024-03-14 15:33 UTC (permalink / raw)
To: Li Zhijian
Cc: linux-kernel, Jiri Kosina, Srinivas Pandruvada,
Benjamin Tissoires, linux-input, linux-iio
In-Reply-To: <20240314084753.1322110-2-lizhijian@fujitsu.com>
On Thu, 14 Mar 2024 16:47:51 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:
> Per filesystems/sysfs.rst, show() should only use sysfs_emit()
> or sysfs_emit_at() when formatting the value to be returned to user space.
>
> coccinelle complains that there are still a couple of functions that use
> snprintf(). Convert them to sysfs_emit().
>
> sprintf() will be converted as weel if they have.
>
> Generally, this patch is generated by
> make coccicheck M=<path/to/file> MODE=patch \
> COCCI=scripts/coccinelle/api/device_attr_show.cocci
>
> No functional change intended
>
> CC: Jiri Kosina <jikos@kernel.org>
> CC: Jonathan Cameron <jic23@kernel.org>
> CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> CC: linux-input@vger.kernel.org
> CC: linux-iio@vger.kernel.org
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Hi Li Zhijian,
One trivial comment inline.
Otherwise straight forward so
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> This is a part of the work "Fix coccicheck device_attr_show warnings"[1]
> Split them per subsystem so that the maintainer can review it easily
> [1] https://lore.kernel.org/lkml/20240116041129.3937800-1-lizhijian@fujitsu.com/
> ---
> drivers/hid/hid-sensor-custom.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index d85398721659..ee1a118834f0 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -155,7 +155,7 @@ static ssize_t enable_sensor_show(struct device *dev,
> {
> struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
>
> - return sprintf(buf, "%d\n", sensor_inst->enable);
> + return sysfs_emit(buf, "%d\n", sensor_inst->enable);
> }
>
> static int set_power_report_state(struct hid_sensor_custom *sensor_inst,
> @@ -372,14 +372,14 @@ static ssize_t show_value(struct device *dev, struct device_attribute *attr,
> sizeof(struct hid_custom_usage_desc),
> usage_id_cmp);
> if (usage_desc)
> - return snprintf(buf, PAGE_SIZE, "%s\n",
> - usage_desc->desc);
> + return sysfs_emit(buf, "%s\n",
> + usage_desc->desc);
rewrap the line as will be under 80 chars.
> else
> - return sprintf(buf, "not-specified\n");
> + return sysfs_emit(buf, "not-specified\n");
> } else
> return -EINVAL;
>
> - return sprintf(buf, "%d\n", value);
> + return sysfs_emit(buf, "%d\n", value);
> }
>
> static ssize_t store_value(struct device *dev, struct device_attribute *attr,
^ permalink raw reply
* Re: [PATCH v9 4/5] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: Jeff LaBundy @ 2024-03-14 15:54 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
broonie, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240308222421.188858-5-jogletre@opensource.cirrus.com>
Hi James,
On Fri, Mar 08, 2024 at 10:24:20PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The input driver provides the interface for control of
> haptic effects through the device.
>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> v7: v8: v9:
> Please advise if playback stop is still misused with respect to not
> specifying an effect ID. The device can only play one effect at a
> time, but setting max effects for the EVIOCGEFFECTS ioctl to 1 would
> restrict the number of uploads to 1 as well. So I'm unsure of the correct
> approach here.
>
> MAINTAINERS | 1 +
> drivers/input/misc/Kconfig | 10 +
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/cs40l50-vibra.c | 578 +++++++++++++++++++++++++++++
> 4 files changed, 590 insertions(+)
> create mode 100644 drivers/input/misc/cs40l50-vibra.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 69a9e0a3b968..24cfb4f017bb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4940,6 +4940,7 @@ 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: drivers/mfd/cs40l*
> F: include/linux/mfd/cs40l*
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 6ba984d7f0b1..ee45dbb0636e 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -140,6 +140,16 @@ config INPUT_BMA150
> To compile this driver as a module, choose M here: the
> module will be called bma150.
>
> +config INPUT_CS40L50_VIBRA
> + tristate "CS40L50 Haptic Driver support"
> + depends on MFD_CS40L50_CORE
> + help
> + Say Y here to enable support for Cirrus Logic's CS40L50
> + haptic driver.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called cs40l50-vibra.
> +
> 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..88279de6d3d5 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,7 @@ 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_VIBRA) += cs40l50-vibra.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-vibra.c b/drivers/input/misc/cs40l50-vibra.c
> new file mode 100644
> index 000000000000..c7e68862bbfe
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-vibra.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2024 Cirrus Logic, Inc.
> + *
> + * Author: James Ogletree <james.ogletree@cirrus.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/input.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +/* Wavetables */
> +#define CS40L50_RAM_INDEX_START 0x1000000
> +#define CS40L50_RAM_INDEX_END 0x100007F
> +#define CS40L50_RTH_INDEX_START 0x1400000
> +#define CS40L50_RTH_INDEX_END 0x1400001
> +#define CS40L50_ROM_INDEX_START 0x1800000
> +#define CS40L50_ROM_INDEX_END 0x180001A
> +#define CS40L50_TYPE_PCM 8
> +#define CS40L50_TYPE_PWLE 12
> +#define CS40L50_PCM_ID 0x0
> +#define CS40L50_OWT_CUSTOM_DATA_SIZE 2
> +
> +/* DSP */
> +#define CS40L50_GPIO_BASE 0x2804140
> +#define CS40L50_OWT_BASE 0x2805C34
> +#define CS40L50_OWT_SIZE 0x2805C38
> +#define CS40L50_OWT_NEXT 0x2805C3C
> +
> +/* GPIO */
> +#define CS40L50_GPIO_NUM_MASK GENMASK(14, 12)
> +#define CS40L50_GPIO_EDGE_MASK BIT(15)
> +#define CS40L50_GPIO_MAPPING_NONE 0
> +#define CS40L50_GPIO_DISABLE 0x1FF
> +
> +enum vibra_bank_type {
> + WVFRM_BANK_RAM,
> + WVFRM_BANK_ROM,
> + WVFRM_BANK_OWT,
> + WVFRM_BANK_NUM,
> +};
Please namespace everything, e.g. CS40L50_... and cs40l50_vibra_...
> +
> +/* Describes an area in DSP memory populated by effects */
> +struct vibra_bank {
> + enum vibra_bank_type bank;
> + u32 base_index;
> + u32 max_index;
> +};
> +
> +struct vibra_effect {
> + enum vibra_bank_type bank;
> + struct list_head list;
> + u32 gpio_reg;
> + u32 index;
> + int id;
> +};
> +
> +/* Describes haptic interface of loaded DSP firmware */
> +struct vibra_dsp {
> + struct vibra_bank *banks;
> + u32 gpio_base_reg;
> + u32 owt_offset_reg;
> + u32 owt_size_reg;
> + u32 owt_base_reg;
> + int (*write)(struct device *dev, struct regmap *regmap, u32 val);
> + u32 push_owt_cmd;
> + u32 delete_owt_cmd;
> + u32 stop_cmd;
> +};
> +
> +/* Describes configuration and state of haptic operations */
> +struct vibra_info {
> + struct device *dev;
> + struct regmap *regmap;
> + struct input_dev *input;
> + struct mutex lock;
> + struct workqueue_struct *vibe_wq;
> + struct list_head effect_head;
> + struct vibra_dsp dsp;
> +};
> +
> +struct vibra_work {
> + struct vibra_info *info;
> + struct ff_effect *effect;
> + struct work_struct work;
> + s16 *custom_data;
> + int custom_len;
> + int count;
> + int error;
> +};
> +
> +static struct vibra_bank cs40l50_banks[] = {
> + {
> + .bank = WVFRM_BANK_RAM,
> + .base_index = CS40L50_RAM_INDEX_START,
> + .max_index = CS40L50_RAM_INDEX_END,
> + },
> + {
> + .bank = WVFRM_BANK_ROM,
> + .base_index = CS40L50_ROM_INDEX_START,
> + .max_index = CS40L50_ROM_INDEX_END,
> + },
> + {
> + .bank = WVFRM_BANK_OWT,
> + .base_index = CS40L50_RTH_INDEX_START,
> + .max_index = CS40L50_RTH_INDEX_END,
> + },
> +};
> +
> +static struct vibra_dsp cs40l50_dsp = {
> + .banks = cs40l50_banks,
> + .gpio_base_reg = CS40L50_GPIO_BASE,
> + .owt_base_reg = CS40L50_OWT_BASE,
> + .owt_offset_reg = CS40L50_OWT_NEXT,
> + .owt_size_reg = CS40L50_OWT_SIZE,
> +
> + .push_owt_cmd = CS40L50_OWT_PUSH,
> + .delete_owt_cmd = CS40L50_OWT_DELETE,
> + .stop_cmd = CS40L50_STOP_PLAYBACK,
> +
> + .write = cs40l50_dsp_write,
> +};
Nit: I don't see any need for any newlines here.
> +
> +static struct vibra_effect *vibra_find_effect(int id, struct list_head *effect_head)
I recommend namespacing the function names too.
> +{
> + struct vibra_effect *effect;
> +
> + list_for_each_entry(effect, effect_head, list)
> + if (effect->id == id)
> + return effect;
> +
> + return NULL;
> +}
> +
> +static int vibra_effect_bank_set(struct vibra_work *work_data,
> + struct vibra_effect *effect)
> +{
> + s16 bank = work_data->custom_data[0] & 0xffffu;
This mask seems to get used in a couple different places; I would recommend
#defining it using GENMASK().
> +
> + if (bank >= WVFRM_BANK_NUM) {
> + dev_err(work_data->info->dev, "Invalid waveform bank: %d\n", bank);
> + return -EINVAL;
> + }
> +
> + if (work_data->custom_len > CS40L50_OWT_CUSTOM_DATA_SIZE)
> + effect->bank = WVFRM_BANK_OWT;
> + else
> + effect->bank = bank;
> +
> + return 0;
> +}
> +
> +static int vibra_effect_gpio_mapping_set(struct vibra_work *work_data,
> + struct vibra_effect *effect)
> +{
> + u16 button = work_data->effect->trigger.button;
> + struct vibra_info *info = work_data->info;
> + u32 gpio_num, gpio_edge;
> +
> + if (button) {
> + gpio_num = FIELD_GET(CS40L50_GPIO_NUM_MASK, button);
> + gpio_edge = FIELD_GET(CS40L50_GPIO_EDGE_MASK, button);
> + effect->gpio_reg = info->dsp.gpio_base_reg + (gpio_num * 8) - gpio_edge;
> +
> + return regmap_write(info->regmap, effect->gpio_reg, button);
> + }
> +
> + effect->gpio_reg = CS40L50_GPIO_MAPPING_NONE;
> +
> + return 0;
> +}
> +
> +static int vibra_effect_index_set(struct vibra_work *work_data,
> + struct vibra_effect *effect)
> +{
> + struct vibra_info *info = work_data->info;
> + struct vibra_effect *owt_effect;
> + u32 base_index, max_index;
> +
> + base_index = info->dsp.banks[effect->bank].base_index;
> + max_index = info->dsp.banks[effect->bank].max_index;
> +
> + effect->index = base_index;
> +
> + switch (effect->bank) {
> + case WVFRM_BANK_OWT:
> + list_for_each_entry(owt_effect, &info->effect_head, list)
> + if (owt_effect->bank == WVFRM_BANK_OWT)
> + effect->index++;
> + break;
> + case WVFRM_BANK_ROM:
> + case WVFRM_BANK_RAM:
> + effect->index += work_data->custom_data[1] & 0xffffu;
> + break;
> + default:
> + dev_err(info->dev, "Bank not supported: %d\n", effect->bank);
> + return -EINVAL;
> + }
> +
> + if (effect->index > max_index || effect->index < base_index) {
> + dev_err(info->dev, "Index out of bounds: %u\n", effect->index);
> + return -ENOSPC;
> + }
> +
> + return 0;
> +}
> +
> +/* Describes a header for an OWT effect */
> +struct owt_header {
> + u32 type;
> + u32 data_words;
> + u32 offset;
> +} __packed;
> +
> +static int vibra_upload_owt(struct vibra_work *work_data)
> +{
> + u32 len = 2 * work_data->custom_len, wt_offset, wt_size;
> + struct vibra_info *info = work_data->info;
> + struct owt_header header;
> + u8 *out_data;
> + int error;
> +
> + error = regmap_read(info->regmap, info->dsp.owt_size_reg, &wt_size);
> + if (error)
> + return error;
> +
> + if ((wt_size * sizeof(u32)) < sizeof(header) + len) {
> + dev_err(info->dev, "No space in OWT bank for effect\n");
> + return -ENOSPC;
> + }
> +
> + out_data = kzalloc(sizeof(header) + len, GFP_KERNEL);
> + if (!out_data)
> + return -ENOMEM;
> +
> + header.type = work_data->custom_data[0] == CS40L50_PCM_ID ? CS40L50_TYPE_PCM :
> + CS40L50_TYPE_PWLE;
> + header.offset = sizeof(header) / sizeof(u32);
> + header.data_words = len / sizeof(u32);
> +
> + memcpy(out_data, &header, sizeof(header));
> + memcpy(out_data + sizeof(header), work_data->custom_data, len);
> +
> + error = regmap_read(info->regmap, info->dsp.owt_offset_reg, &wt_offset);
> + if (error)
> + goto err_free;
> +
> + error = regmap_bulk_write(info->regmap, info->dsp.owt_base_reg +
> + (wt_offset * sizeof(u32)), out_data,
> + sizeof(header) + len);
> + if (error)
> + goto err_free;
> +
> + error = info->dsp.write(info->dev, info->regmap, info->dsp.push_owt_cmd);
> +err_free:
> + kfree(out_data);
> +
> + return error;
> +}
> +
> +static void vibra_add_worker(struct work_struct *work)
> +{
> + struct vibra_work *work_data = container_of(work, struct vibra_work, work);
> + struct vibra_info *info = work_data->info;
> + struct vibra_effect *effect;
> + bool is_new = false;
> + int error;
> +
> + error = pm_runtime_resume_and_get(info->dev);
> + if (error < 0)
> + goto err;
> +
> + mutex_lock(&info->lock);
> +
> + /* Update effect if already present, otherwise create new effect */
> + effect = vibra_find_effect(work_data->effect->id, &info->effect_head);
> + if (!effect) {
> + effect = kzalloc(sizeof(*effect), GFP_KERNEL);
> + if (!effect) {
> + error = -ENOMEM;
> + goto err_mutex;
> + }
> +
> + effect->id = work_data->effect->id;
> + is_new = true;
> + }
> +
> + error = vibra_effect_bank_set(work_data, effect);
> + if (error)
> + goto err_free;
> +
> + error = vibra_effect_index_set(work_data, effect);
> + if (error)
> + goto err_free;
> +
> + error = vibra_effect_gpio_mapping_set(work_data, effect);
> + if (error)
> + goto err_free;
> +
> + if (effect->bank == WVFRM_BANK_OWT)
> + error = vibra_upload_owt(work_data);
> +err_free:
> + if (is_new) {
> + if (error)
> + kfree(effect);
> + else
> + list_add(&effect->list, &info->effect_head);
> + }
> +err_mutex:
> + mutex_unlock(&info->lock);
> +
> + pm_runtime_mark_last_busy(info->dev);
> + pm_runtime_put_autosuspend(info->dev);
> +err:
> + work_data->error = error;
> +}
> +
> +static int vibra_add(struct input_dev *dev, struct ff_effect *effect,
> + struct ff_effect *old)
> +{
> + struct ff_periodic_effect *periodic = &effect->u.periodic;
> + struct vibra_info *info = input_get_drvdata(dev);
> + u32 len = effect->u.periodic.custom_len;
> + struct vibra_work work_data;
> +
> + if (effect->type != FF_PERIODIC || periodic->waveform != FF_CUSTOM) {
> + dev_err(info->dev, "Type (%#X) or waveform (%#X) unsupported\n",
> + effect->type, periodic->waveform);
> + return -EINVAL;
> + }
> +
> + work_data.custom_data = kcalloc(len, sizeof(s16), GFP_KERNEL);
> + if (!work_data.custom_data)
> + return -ENOMEM;
> +
> + if (copy_from_user(work_data.custom_data, effect->u.periodic.custom_data,
> + sizeof(s16) * len)) {
> + work_data.error = -EFAULT;
> + goto err_free;
> + }
> +
> + work_data.custom_len = len;
> + work_data.info = info;
> + work_data.effect = effect;
> + INIT_WORK(&work_data.work, vibra_add_worker);
> +
> + /* Push to the workqueue to serialize with playbacks */
> + queue_work(info->vibe_wq, &work_data.work);
> + flush_work(&work_data.work);
> +err_free:
> + kfree(work_data.custom_data);
> +
> + return work_data.error;
> +}
> +
> +static void vibra_start_worker(struct work_struct *work)
> +{
> + struct vibra_work *work_data = container_of(work, struct vibra_work, work);
> + struct vibra_info *info = work_data->info;
> + struct vibra_effect *start_effect;
> +
> + if (pm_runtime_resume_and_get(info->dev) < 0)
> + goto err_free;
> +
> + mutex_lock(&info->lock);
> +
> + start_effect = vibra_find_effect(work_data->effect->id, &info->effect_head);
> + if (start_effect) {
> + while (--work_data->count >= 0) {
> + info->dsp.write(info->dev, info->regmap, start_effect->index);
> + usleep_range(work_data->effect->replay.length,
> + work_data->effect->replay.length + 100);
> + }
> + }
> +
> + mutex_unlock(&info->lock);
> +
> + if (!start_effect)
> + dev_err(info->dev, "Effect to play not found\n");
> +
> + pm_runtime_mark_last_busy(info->dev);
> + pm_runtime_put_autosuspend(info->dev);
> +err_free:
> + kfree(work_data);
This business of allocating memory in one thread and freeing it in another
got pretty hard to follow, which means it will be hard to maintain. I know
there are some restrictions around writing into the HALO core, but I'd like
to see something simpler; the da7280 driver is nowhere near as complex.
How many s16 words do you realistically need for custom_data? Is it possible
to simply include an array in the cs40l50_vibra_info struct, and bleat if
user space tries to upload greater than the maximum size? This seems to be
the approach of the much simpler da7280 driver as well.
This array would go on the heap just the same, but you do not have to manage
it so carefully. Opinions may vary, but mine is that memory allocation and
freeing should be done in the same scope where possible.
> +}
> +
> +static void vibra_stop_worker(struct work_struct *work)
> +{
> + struct vibra_work *work_data = container_of(work, struct vibra_work, work);
> + struct vibra_info *info = work_data->info;
> +
> + if (pm_runtime_resume_and_get(info->dev) < 0)
> + return;
> +
> + mutex_lock(&info->lock);
> +
> + info->dsp.write(info->dev, info->regmap, info->dsp.stop_cmd);
> +
> + mutex_unlock(&info->lock);
> +
> + pm_runtime_mark_last_busy(info->dev);
> + pm_runtime_put_autosuspend(info->dev);
> +
> + kfree(work_data);
> +}
> +
> +static int vibra_playback(struct input_dev *dev, int effect_id, int val)
> +{
> + struct vibra_info *info = input_get_drvdata(dev);
> + struct vibra_work *work_data;
> +
> + work_data = kzalloc(sizeof(*work_data), GFP_ATOMIC);
> + if (!work_data)
> + return -ENOMEM;
> +
> + work_data->info = info;
> +
> + if (val > 0) {
> + work_data->effect = &dev->ff->effects[effect_id];
> + work_data->count = val;
> + INIT_WORK(&work_data->work, vibra_start_worker);
> + } else {
> + /* Stop the amplifier as device drives only one effect */
> + INIT_WORK(&work_data->work, vibra_stop_worker);
> + }
> +
> + queue_work(info->vibe_wq, &work_data->work);
> +
> + return 0;
> +}
> +
> +static void vibra_erase_worker(struct work_struct *work)
> +{
> + struct vibra_work *work_data = container_of(work, struct vibra_work, work);
> + struct vibra_effect *owt_effect, *erase_effect;
> + struct vibra_info *info = work_data->info;
> + int error;
> +
> + error = pm_runtime_resume_and_get(info->dev);
> + if (error < 0)
if (error)
> + goto err;
Please consider a more descriptive label.
> +
> + mutex_lock(&info->lock);
> +
> + erase_effect = vibra_find_effect(work_data->effect->id, &info->effect_head);
> + if (!erase_effect) {
> + dev_err(info->dev, "Effect to erase not found\n");
> + error = -EINVAL;
> + goto err_mutex;
> + }
> +
> + if (erase_effect->gpio_reg != CS40L50_GPIO_MAPPING_NONE) {
> + error = regmap_write(info->regmap, erase_effect->gpio_reg,
> + CS40L50_GPIO_DISABLE);
> + if (error)
> + goto err_mutex;
> + }
> +
> + if (erase_effect->bank == WVFRM_BANK_OWT) {
> + error = info->dsp.write(info->dev, info->regmap,
> + info->dsp.delete_owt_cmd |
> + erase_effect->index);
> + if (error)
> + goto err_mutex;
> +
> + list_for_each_entry(owt_effect, &info->effect_head, list)
> + if (owt_effect->bank == WVFRM_BANK_OWT &&
> + owt_effect->index > erase_effect->index)
> + owt_effect->index--;
> + }
> +
> + list_del(&erase_effect->list);
> + kfree(erase_effect);
> +err_mutex:
> + mutex_unlock(&info->lock);
> +
> + pm_runtime_mark_last_busy(info->dev);
> + pm_runtime_put_autosuspend(info->dev);
> +err:
> + work_data->error = error;
> +}
> +
> +static int vibra_erase(struct input_dev *dev, int effect_id)
> +{
> + struct vibra_info *info = input_get_drvdata(dev);
> + struct vibra_work work_data;
> +
> + work_data.info = info;
> + work_data.effect = &dev->ff->effects[effect_id];
> +
> + INIT_WORK(&work_data.work, vibra_erase_worker);
> +
> + /* Push to workqueue to serialize with playbacks */
> + queue_work(info->vibe_wq, &work_data.work);
> + flush_work(&work_data.work);
> +
> + return work_data.error;
> +}
> +
> +static void vibra_remove_wq(void *data)
> +{
> + flush_workqueue(data);
> + destroy_workqueue(data);
> +}
> +
> +static int cs40l50_vibra_probe(struct platform_device *pdev)
> +{
> + struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> + struct vibra_info *info;
> + int error;
> +
> + info = devm_kzalloc(pdev->dev.parent, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + mutex_init(&info->lock);
> +
> + info->dev = cs40l50->dev;
> + info->regmap = cs40l50->regmap;
> + info->dsp = cs40l50_dsp;
> +
> + info->input = devm_input_allocate_device(info->dev);
> + if (!info->input)
> + return -ENOMEM;
> + info->input->id.product = cs40l50->devid & 0xFFFF;
> + info->input->id.version = cs40l50->revid;
> + info->input->name = "cs40l50_vibra";
> +
> + input_set_drvdata(info->input, info);
> + input_set_capability(info->input, EV_FF, FF_PERIODIC);
> + input_set_capability(info->input, EV_FF, FF_CUSTOM);
> +
> + error = input_ff_create(info->input, FF_MAX_EFFECTS);
> + if (error) {
> + dev_err(info->dev, "Failed to create input device\n");
> + return error;
> + }
> +
> + info->input->ff->upload = vibra_add;
> + info->input->ff->playback = vibra_playback;
> + info->input->ff->erase = vibra_erase;
> +
> + INIT_LIST_HEAD(&info->effect_head);
> +
> + info->vibe_wq = alloc_ordered_workqueue("vibe_wq", WQ_HIGHPRI);
> + if (!info->vibe_wq)
> + return -ENOMEM;
> +
> + error = devm_add_action_or_reset(info->dev, vibra_remove_wq, info->vibe_wq);
> + if (error)
> + return error;
> +
> + return input_register_device(info->input);
> +}
> +
> +static const struct platform_device_id vibra_id_match[] = {
> + { "cs40l50-vibra", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(platform, vibra_id_match);
> +
> +static struct platform_driver cs40l50_vibra_driver = {
> + .probe = cs40l50_vibra_probe,
> + .id_table = vibra_id_match,
> + .driver = {
> + .name = "cs40l50-vibra",
> + },
> +};
> +module_platform_driver(cs40l50_vibra_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v9 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50
From: Jeff LaBundy @ 2024-03-14 16:05 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
broonie, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240308222421.188858-6-jogletre@opensource.cirrus.com>
Hi James,
This one looks pretty good; just a few nits since a new series is
coming anyway.
On Fri, Mar 08, 2024 at 10:24:21PM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The ASoC driver enables I2S streaming to the device.
>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> MAINTAINERS | 1 +
> sound/soc/codecs/Kconfig | 11 ++
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/cs40l50-codec.c | 307 +++++++++++++++++++++++++++++++
> 4 files changed, 321 insertions(+)
> create mode 100644 sound/soc/codecs/cs40l50-codec.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 24cfb4f017bb..fca2454a7a38 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4943,6 +4943,7 @@ F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> F: drivers/input/misc/cs40l*
> F: drivers/mfd/cs40l*
> F: include/linux/mfd/cs40l*
> +F: sound/soc/codecs/cs40l*
>
> CIRRUS LOGIC DSP FIRMWARE DRIVER
> M: Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index f1e1dbc509f6..1a81bedfdbe3 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -73,6 +73,7 @@ config SND_SOC_ALL_CODECS
> imply SND_SOC_CS35L56_I2C
> imply SND_SOC_CS35L56_SPI
> imply SND_SOC_CS35L56_SDW
> + imply SND_SOC_CS40L50
> imply SND_SOC_CS42L42
> imply SND_SOC_CS42L42_SDW
> imply SND_SOC_CS42L43
> @@ -800,6 +801,16 @@ config SND_SOC_CS35L56_SDW
> help
> Enable support for Cirrus Logic CS35L56 boosted amplifier with SoundWire control
>
> +config SND_SOC_CS40L50
> + tristate "Cirrus Logic CS40L50 CODEC"
> + depends on MFD_CS40L50_CORE
> + help
> + This option enables support for I2S streaming to Cirrus Logic CS40L50.
> +
> + CS40L50 is a haptic driver with waveform memory, an integrated
> + DSP, and closed-loop algorithms. If built as a module, it will be
> + called snd-soc-cs40l50.
> +
> config SND_SOC_CS42L42_CORE
> tristate
>
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index a87e56938ce5..7e31f000774a 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -74,6 +74,7 @@ snd-soc-cs35l56-shared-objs := cs35l56-shared.o
> snd-soc-cs35l56-i2c-objs := cs35l56-i2c.o
> snd-soc-cs35l56-spi-objs := cs35l56-spi.o
> snd-soc-cs35l56-sdw-objs := cs35l56-sdw.o
> +snd-soc-cs40l50-objs := cs40l50-codec.o
> snd-soc-cs42l42-objs := cs42l42.o
> snd-soc-cs42l42-i2c-objs := cs42l42-i2c.o
> snd-soc-cs42l42-sdw-objs := cs42l42-sdw.o
> @@ -460,6 +461,7 @@ obj-$(CONFIG_SND_SOC_CS35L56_SHARED) += snd-soc-cs35l56-shared.o
> obj-$(CONFIG_SND_SOC_CS35L56_I2C) += snd-soc-cs35l56-i2c.o
> obj-$(CONFIG_SND_SOC_CS35L56_SPI) += snd-soc-cs35l56-spi.o
> obj-$(CONFIG_SND_SOC_CS35L56_SDW) += snd-soc-cs35l56-sdw.o
> +obj-$(CONFIG_SND_SOC_CS40L50) += snd-soc-cs40l50.o
> obj-$(CONFIG_SND_SOC_CS42L42_CORE) += snd-soc-cs42l42.o
> obj-$(CONFIG_SND_SOC_CS42L42) += snd-soc-cs42l42-i2c.o
> obj-$(CONFIG_SND_SOC_CS42L42_SDW) += snd-soc-cs42l42-sdw.o
> diff --git a/sound/soc/codecs/cs40l50-codec.c b/sound/soc/codecs/cs40l50-codec.c
> new file mode 100644
> index 000000000000..23299b8dacfb
> --- /dev/null
> +++ b/sound/soc/codecs/cs40l50-codec.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// CS40L50 Advanced Haptic Driver with waveform memory,
> +// integrated DSP, and closed-loop algorithms
> +//
> +// Copyright 2024 Cirrus Logic, Inc.
> +//
> +// Author: James Ogletree <james.ogletree@cirrus.com>
Typically we see a // C++ style comment for the SPDX identifier, and then
/* block comments */ for the rest, unless the maintainer has explicitly
asked for this style for this specific subsystem. The introductory block
in the rest of the series is much more common.
> +
> +#include <linux/bitfield.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +
> +#define CS40L50_REFCLK_INPUT 0x2C04
> +#define CS40L50_ASP_CONTROL2 0x4808
> +#define CS40L50_ASP_DATA_CONTROL5 0x4840
> +
> +/* PLL Config */
> +#define CS40L50_PLL_REFCLK_BCLK 0x0
> +#define CS40L50_PLL_REFCLK_MCLK 0x5
> +#define CS40L50_PLL_REFCLK_LOOP_MASK BIT(11)
> +#define CS40L50_PLL_REFCLK_OPEN_LOOP 1
> +#define CS40L50_PLL_REFCLK_CLOSED_LOOP 0
> +#define CS40L50_PLL_REFCLK_LOOP_SHIFT 11
> +#define CS40L50_PLL_REFCLK_FREQ_MASK GENMASK(10, 5)
> +#define CS40L50_PLL_REFCLK_FREQ_SHIFT 5
> +#define CS40L50_PLL_REFCLK_SEL_MASK GENMASK(2, 0)
> +#define CS40L50_BCLK_RATIO_DEFAULT 32
> +
> +/* ASP Config */
> +#define CS40L50_ASP_RX_WIDTH_SHIFT 24
> +#define CS40L50_ASP_RX_WIDTH_MASK GENMASK(31, 24)
> +#define CS40L50_ASP_RX_WL_MASK GENMASK(5, 0)
> +#define CS40L50_ASP_FSYNC_INV_MASK BIT(2)
> +#define CS40L50_ASP_BCLK_INV_MASK BIT(6)
> +#define CS40L50_ASP_FMT_MASK GENMASK(10, 8)
> +#define CS40L50_ASP_FMT_I2S 0x2
> +
> +struct cs40l50_pll_config {
> + unsigned int freq;
> + unsigned int cfg;
> +};
> +
> +struct cs40l50_codec {
> + struct device *dev;
> + struct regmap *regmap;
> + unsigned int daifmt;
> + unsigned int bclk_ratio;
> + unsigned int rate;
> +};
> +
> +static const struct cs40l50_pll_config cs40l50_pll_cfg[] = {
> + {32768, 0x00},
> + {1536000, 0x1B},
> + {3072000, 0x21},
> + {6144000, 0x28},
> + {9600000, 0x30},
> + {12288000, 0x33},
Nit: please space these as is done elsewhere throughout the series, i.e.:
{ 12288000, 0x33 },
> +};
> +
> +static int cs40l50_get_clk_config(unsigned int freq, unsigned int *cfg)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(cs40l50_pll_cfg); i++) {
> + if (cs40l50_pll_cfg[i].freq == freq) {
> + *cfg = cs40l50_pll_cfg[i].cfg;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int cs40l50_swap_ext_clk(struct cs40l50_codec *codec, unsigned int clk_src)
> +{
> + unsigned int cfg;
> + int ret;
> +
> + switch (clk_src) {
> + case CS40L50_PLL_REFCLK_BCLK:
> + ret = cs40l50_get_clk_config(codec->bclk_ratio * codec->rate, &cfg);
> + if (ret)
> + return ret;
> + break;
> + case CS40L50_PLL_REFCLK_MCLK:
> + cfg = 0x00;
0x00 seems special; please consider #defining it.
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> + CS40L50_PLL_REFCLK_LOOP_MASK,
> + CS40L50_PLL_REFCLK_OPEN_LOOP <<
> + CS40L50_PLL_REFCLK_LOOP_SHIFT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> + CS40L50_PLL_REFCLK_FREQ_MASK |
> + CS40L50_PLL_REFCLK_SEL_MASK,
> + (cfg << CS40L50_PLL_REFCLK_FREQ_SHIFT) | clk_src);
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> + CS40L50_PLL_REFCLK_LOOP_MASK,
> + CS40L50_PLL_REFCLK_CLOSED_LOOP <<
> + CS40L50_PLL_REFCLK_LOOP_SHIFT);
> +}
> +
> +static int cs40l50_clk_en(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol,
> + int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(comp);
> + int ret;
> +
> + switch (event) {
> + case SND_SOC_DAPM_POST_PMU:
> + ret = cs40l50_dsp_write(codec->dev, codec->regmap, CS40L50_STOP_PLAYBACK);
> + if (ret)
> + return ret;
> +
> + ret = cs40l50_dsp_write(codec->dev, codec->regmap, CS40L50_START_I2S);
> + if (ret)
> + return ret;
> +
> + ret = cs40l50_swap_ext_clk(codec, CS40L50_PLL_REFCLK_BCLK);
> + if (ret)
> + return ret;
> + break;
> + case SND_SOC_DAPM_PRE_PMD:
> + ret = cs40l50_swap_ext_clk(codec, CS40L50_PLL_REFCLK_MCLK);
> + if (ret)
> + return ret;
> + break;
Just for my own understanding, does the HALO core need to be told that I2S
streaming is about to stop? Or does the L50 gracefully park the class D
outputs when the PLL source is suddenly switched?
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_dapm_widget cs40l50_dapm_widgets[] = {
> + SND_SOC_DAPM_SUPPLY_S("ASP PLL", 0, SND_SOC_NOPM, 0, 0, cs40l50_clk_en,
> + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
> + SND_SOC_DAPM_AIF_IN("ASPRX1", NULL, 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_IN("ASPRX2", NULL, 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_OUTPUT("OUT"),
> +};
> +
> +static const struct snd_soc_dapm_route cs40l50_dapm_routes[] = {
> + { "ASP Playback", NULL, "ASP PLL" },
> + { "ASPRX1", NULL, "ASP Playback" },
> + { "ASPRX2", NULL, "ASP Playback" },
> +
> + { "OUT", NULL, "ASPRX1" },
> + { "OUT", NULL, "ASPRX2" },
> +};
> +
> +static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
> +
> + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBC_CFC)
> + return -EINVAL;
> +
> + switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> + case SND_SOC_DAIFMT_NB_NF:
> + codec->daifmt = 0;
> + break;
> + case SND_SOC_DAIFMT_NB_IF:
> + codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK;
> + break;
> + case SND_SOC_DAIFMT_IB_NF:
> + codec->daifmt = CS40L50_ASP_BCLK_INV_MASK;
> + break;
> + case SND_SOC_DAIFMT_IB_IF:
> + codec->daifmt = CS40L50_ASP_FSYNC_INV_MASK | CS40L50_ASP_BCLK_INV_MASK;
> + break;
> + default:
> + dev_err(codec->dev, "Invalid clock invert\n");
> + return -EINVAL;
> + }
> +
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> + case SND_SOC_DAIFMT_I2S:
> + codec->daifmt |= FIELD_PREP(CS40L50_ASP_FMT_MASK, CS40L50_ASP_FMT_I2S);
> + break;
> + default:
> + dev_err(codec->dev, "Unsupported DAI format\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int cs40l50_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(dai->component);
> + unsigned int asp_rx_wl = params_width(params);
> + int ret;
> +
> + codec->rate = params_rate(params);
> +
> + ret = regmap_update_bits(codec->regmap, CS40L50_ASP_DATA_CONTROL5,
> + CS40L50_ASP_RX_WL_MASK, asp_rx_wl);
> + if (ret)
> + return ret;
> +
> + codec->daifmt |= (asp_rx_wl << CS40L50_ASP_RX_WIDTH_SHIFT);
> +
> + return regmap_update_bits(codec->regmap, CS40L50_ASP_CONTROL2,
> + CS40L50_ASP_FSYNC_INV_MASK |
> + CS40L50_ASP_BCLK_INV_MASK |
> + CS40L50_ASP_FMT_MASK |
> + CS40L50_ASP_RX_WIDTH_MASK, codec->daifmt);
> +}
> +
> +static int cs40l50_set_dai_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
> +{
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(dai->component);
> +
> + codec->bclk_ratio = ratio;
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_dai_ops cs40l50_dai_ops = {
> + .set_fmt = cs40l50_set_dai_fmt,
> + .set_bclk_ratio = cs40l50_set_dai_bclk_ratio,
> + .hw_params = cs40l50_hw_params,
> +};
> +
> +static struct snd_soc_dai_driver cs40l50_dai[] = {
> + {
> + .name = "cs40l50-pcm",
> + .id = 0,
> + .playback = {
> + .stream_name = "ASP Playback",
> + .channels_min = 1,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_48000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
> + },
> + .ops = &cs40l50_dai_ops,
> + },
> +};
> +
> +static int cs40l50_codec_probe(struct snd_soc_component *component)
> +{
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(component);
> +
> + codec->bclk_ratio = CS40L50_BCLK_RATIO_DEFAULT;
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_component_driver soc_codec_dev_cs40l50 = {
> + .probe = cs40l50_codec_probe,
> + .dapm_widgets = cs40l50_dapm_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(cs40l50_dapm_widgets),
> + .dapm_routes = cs40l50_dapm_routes,
> + .num_dapm_routes = ARRAY_SIZE(cs40l50_dapm_routes),
> +};
> +
> +static int cs40l50_codec_driver_probe(struct platform_device *pdev)
> +{
> + struct cs40l50 *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> + struct cs40l50_codec *codec;
> +
> + codec = devm_kzalloc(&pdev->dev, sizeof(*codec), GFP_KERNEL);
> + if (!codec)
> + return -ENOMEM;
> +
> + codec->regmap = cs40l50->regmap;
> + codec->dev = &pdev->dev;
> +
> + return devm_snd_soc_register_component(&pdev->dev, &soc_codec_dev_cs40l50,
> + cs40l50_dai, ARRAY_SIZE(cs40l50_dai));
> +}
> +
> +static const struct platform_device_id cs40l50_id[] = {
> + { "cs40l50-codec", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(platform, cs40l50_id);
> +
> +static struct platform_driver cs40l50_codec_driver = {
> + .probe = cs40l50_codec_driver_probe,
> + .id_table = cs40l50_id,
> + .driver = {
> + .name = "cs40l50-codec",
> + },
> +};
> +module_platform_driver(cs40l50_codec_driver);
> +
> +MODULE_DESCRIPTION("ASoC CS40L50 driver");
> +MODULE_AUTHOR("James Ogletree <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v9 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50
From: Mark Brown @ 2024-03-14 16:22 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, lee, patches, linux-sound, linux-input, devicetree
In-Reply-To: <ZfMgLc16lOTK62wa@nixie71>
[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]
On Thu, Mar 14, 2024 at 11:05:01AM -0500, Jeff LaBundy wrote:
> On Fri, Mar 08, 2024 at 10:24:21PM +0000, James Ogletree wrote:
> > Introduce support for Cirrus Logic Device CS40L50: a
> > haptic driver with waveform memory, integrated DSP,
> > and closed-loop algorithms.
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// CS40L50 Advanced Haptic Driver with waveform memory,
> > +// integrated DSP, and closed-loop algorithms
> > +//
> > +// Copyright 2024 Cirrus Logic, Inc.
> > +//
> > +// Author: James Ogletree <james.ogletree@cirrus.com>
> Typically we see a // C++ style comment for the SPDX identifier, and then
> /* block comments */ for the rest, unless the maintainer has explicitly
> asked for this style for this specific subsystem. The introductory block
> in the rest of the series is much more common.
This is something I specifically request.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v9 3/5] mfd: cs40l50: Add support for CS40L50 core driver
From: James Ogletree @ 2024-03-14 21:03 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Mark Brown,
open list:CIRRUS LOGIC HAPTIC DRIVERS,
open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)...,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <Ze5E1KxRltUTX4R6@nixie71>
Hi Jeff,
Thank you for the kind remarks and thorough review.
All of your points, here and across the series, you can consider
acknowledged and I will work to adopt them in the next version.
There were a few loose ends and questions throughout the series which I
intend to address over the coming days. The first one is below.
>> +
>> +static const struct reg_sequence cs40l50_irq_mask_override[] = {
>> + { CS40L50_IRQ1_MASK_2, CS40L50_IRQ_MASK_2_OVERRIDE },
>> + { CS40L50_IRQ1_MASK_20, CS40L50_IRQ_MASK_20_OVERRIDE },
>> +};
>> +
>> +static int cs40l50_configure_dsp(struct cs_dsp *dsp)
>> +{
>> + struct cs40l50 *cs40l50 = container_of(dsp, struct cs40l50, dsp);
>> + u32 nwaves;
>> + int err;
>> +
>> + /* Log number of effects if wavetable was loaded */
>> + if (cs40l50->bin) {
>
> Is there any other clue you can use to discern whether a wavetable was
> loaded? The memory at cs40l50->bin is gone now, and although you're not
> dereferencing it anymore, another contributor might be fooled into doing
> so down the road.
>
> Maybe a boolean would be more maintainable; I don't feel strongly about
> it though.
I will simply remove the conditional. If no wavetable was loaded, the
register will have a value of 0, which is no less worthwhile to report to the
user.
Best,
James
^ permalink raw reply
* Re: [PATCH v2 2/4] HID: hid-sensor-custom: Convert sprintf/snprintf to sysfs_emit
From: Zhijian Li (Fujitsu) @ 2024-03-15 1:14 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, Jiri Kosina, Srinivas Pandruvada,
Benjamin Tissoires, linux-input@vger.kernel.org,
linux-iio@vger.kernel.org
In-Reply-To: <20240314153326.2126d3df@jic23-huawei>
On 14/03/2024 23:33, Jonathan Cameron wrote:
> On Thu, 14 Mar 2024 16:47:51 +0800
> Li Zhijian <lizhijian@fujitsu.com> wrote:
>
>> Per filesystems/sysfs.rst, show() should only use sysfs_emit()
>> or sysfs_emit_at() when formatting the value to be returned to user space.
>>
>> coccinelle complains that there are still a couple of functions that use
>> snprintf(). Convert them to sysfs_emit().
>>
>> sprintf() will be converted as weel if they have.
>>
>> Generally, this patch is generated by
>> make coccicheck M=<path/to/file> MODE=patch \
>> COCCI=scripts/coccinelle/api/device_attr_show.cocci
>>
>> No functional change intended
>>
>> CC: Jiri Kosina <jikos@kernel.org>
>> CC: Jonathan Cameron <jic23@kernel.org>
>> CC: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> CC: linux-input@vger.kernel.org
>> CC: linux-iio@vger.kernel.org
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Hi Li Zhijian,
>
> One trivial comment inline.
> Otherwise straight forward so
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>> This is a part of the work "Fix coccicheck device_attr_show warnings"[1]
>> Split them per subsystem so that the maintainer can review it easily
>> [1] https://lore.kernel.org/lkml/20240116041129.3937800-1-lizhijian@fujitsu.com/
>> ---
>> drivers/hid/hid-sensor-custom.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
>> index d85398721659..ee1a118834f0 100644
>> --- a/drivers/hid/hid-sensor-custom.c
>> +++ b/drivers/hid/hid-sensor-custom.c
>> @@ -155,7 +155,7 @@ static ssize_t enable_sensor_show(struct device *dev,
>> {
>> struct hid_sensor_custom *sensor_inst = dev_get_drvdata(dev);
>>
>> - return sprintf(buf, "%d\n", sensor_inst->enable);
>> + return sysfs_emit(buf, "%d\n", sensor_inst->enable);
>> }
>>
>> static int set_power_report_state(struct hid_sensor_custom *sensor_inst,
>> @@ -372,14 +372,14 @@ static ssize_t show_value(struct device *dev, struct device_attribute *attr,
>> sizeof(struct hid_custom_usage_desc),
>> usage_id_cmp);
>> if (usage_desc)
>> - return snprintf(buf, PAGE_SIZE, "%s\n",
>> - usage_desc->desc);
>> + return sysfs_emit(buf, "%s\n",
>> + usage_desc->desc);
> rewrap the line as will be under 80 chars.
Good catch, i will update it.
Thanks
Zhijian
>
>> else
>> - return sprintf(buf, "not-specified\n");
>> + return sysfs_emit(buf, "not-specified\n");
>> } else
>> return -EINVAL;
>>
>> - return sprintf(buf, "%d\n", value);
>> + return sysfs_emit(buf, "%d\n", value);
>> }
>>
>> static ssize_t store_value(struct device *dev, struct device_attribute *attr,
>
^ permalink raw reply
* Re: [PATCH v9 1/5] firmware: cs_dsp: Add write sequencer interface
From: Charles Keepax @ 2024-03-15 9:29 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, lee, broonie, patches, linux-sound, linux-input,
devicetree
In-Reply-To: <ZfMMYm1k7qP7Uqm8@nixie71>
On Thu, Mar 14, 2024 at 09:40:34AM -0500, Jeff LaBundy wrote:
> On Mon, Mar 11, 2024 at 10:14:17AM +0000, Charles Keepax wrote:
> > On Sun, Mar 10, 2024 at 02:12:16PM -0500, Jeff LaBundy wrote:
> > > On Fri, Mar 08, 2024 at 10:24:17PM +0000, James Ogletree wrote:
> > I disagree here. This is the write function, there is no guarantee
> > this is even called at probe time. This is generic code going
> > into the DSP library and will presumably get re-used for different
> > purposes and on different parts in the future. Freeing on the error
> > path is much safer here.
>
> Agreed that this won't be called during probe; all I mean to say is
> that I don't see any issue in hanging on to a bit of memory while the
> device is in an error state, before the module is unloaded and devres
> unrolls all of the device-managed resources.
I think that's the problem the assumption that all users will
completely bork the device if this operation fails. Likely but
far from certain.
> It's also perfectly fine to leave this as-is; the existing method is
> functionally correct, and I understand the perspective of having the
> generic library clean up immediately. If that's the intent however,
> the same error handling needs to be applied in cs_dsp_populate_wseq();
> currently only cs_dsp_wseq_write() calls devm_kfree() on error. Since
> L50 uses asynchronous FW loading, neither are called until after the
> device probes.
>
Hmm... yes I guess in my head I wasn't thinking so much about
populate being called at runtime, but I am inclined to agree. We
should probably update populate to also free on the error path.
Thanks,
Charles
^ permalink raw reply
* Advise needed on stale patches
From: Francesco Dolcini @ 2024-03-15 9:33 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: Stefan Eichenberger, Emanuele Ghidoli
Hello Dmitry and Linux Input people,
In the last months I sent (or was involved in) a couple of small patches
[1][2] (bug fixes or small addition to an existing driver) and they have
been around in the mailing list for quite some time (months, 2023-09-20
the oldest one).
I am not sure on how to progress here, therefore looking for some advise.
Maybe they did just fall through the cracks, in this case just take this
as a kind head up (or maybe those email did not pass some aggressive
spam filtering?).
Or maybe the changes are not deemed correct or something wrong was done,
and in this case it would be great to know what we could improve.
Or, maybe you just had other priorities (life and work are challenging
at times, I know), in this case, if possible, I would love to know if I
can provide some small help in any way.
[1] https://lore.kernel.org/all/20240209105012.22470-1-eichest@gmail.com/
[2] https://lore.kernel.org/all/20231222183114.30775-1-francesco@dolcini.it/
Thanks,
Francesco
p.s. of course, I know that the Linux merge window is currently open and
these twos are busy weeks, I'm not expecting any action till the merge
window closes.
^ permalink raw reply
* [PATCH v4 0/7] Extend HID-BPF kfuncs (was: allow HID-BPF to do device IOs)
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
New version of the sleepable bpf_timer code, without BPF changes, as
they can now go through the HID tree independantly:
https://lore.kernel.org/all/20240315-hid-bpf-sleepable-v4-0-5658f2540564@kernel.org/
For reference, the use cases I have in mind:
---
Basically, I need to be able to defer a HID-BPF program for the
following reasons (from the aforementioned patch):
1. defer an event:
Sometimes we receive an out of proximity event, but the device can not
be trusted enough, and we need to ensure that we won't receive another
one in the following n milliseconds. So we need to wait those n
milliseconds, and eventually re-inject that event in the stack.
2. inject new events in reaction to one given event:
We might want to transform one given event into several. This is the
case for macro keys where a single key press is supposed to send
a sequence of key presses. But this could also be used to patch a
faulty behavior, if a device forgets to send a release event.
3. communicate with the device in reaction to one event:
We might want to communicate back to the device after a given event.
For example a device might send us an event saying that it came back
from sleeping state and needs to be re-initialized.
Currently we can achieve that by keeping a userspace program around,
raise a bpf event, and let that userspace program inject the events and
commands.
However, we are just keeping that program alive as a daemon for just
scheduling commands. There is no logic in it, so it doesn't really justify
an actual userspace wakeup. So a kernel workqueue seems simpler to handle.
bpf_timers are currently running in a soft IRQ context, this patch
series implements a sleppable context for them.
Cheers,
Benjamin
To: Jiri Kosina <jikos@kernel.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Jonathan Corbet <corbet@lwn.net>
To: Shuah Khan <shuah@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: <linux-input@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: <bpf@vger.kernel.org>
Cc: <linux-doc@vger.kernel.org>
Cc: <linux-kselftest@vger.kernel.org>
---
Changes in v4:
- dropped the BPF changes, they can go independently in bpf-core
- dropped the HID-BPF integration tests with the sleppable timers,
I'll re-add them once both series (this and sleepable timers) are
merged
- Link to v3: https://lore.kernel.org/r/20240221-hid-bpf-sleepable-v3-0-1fb378ca6301@kernel.org
Changes in v3:
- fixed the crash from v2
- changed the API to have only BPF_F_TIMER_SLEEPABLE for
bpf_timer_start()
- split the new kfuncs/verifier patch into several sub-patches, for
easier reviews
- Link to v2: https://lore.kernel.org/r/20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org
Changes in v2:
- make use of bpf_timer (and dropped the custom HID handling)
- implemented bpf_timer_set_sleepable_cb as a kfunc
- still not implemented global subprogs
- no sleepable bpf_timer selftests yet
- Link to v1: https://lore.kernel.org/r/20240209-hid-bpf-sleepable-v1-0-4cc895b5adbd@kernel.org
---
Benjamin Tissoires (7):
HID: bpf/dispatch: regroup kfuncs definitions
HID: bpf: export hid_hw_output_report as a BPF kfunc
selftests/hid: add KASAN to the VM tests
selftests/hid: Add test for hid_bpf_hw_output_report
HID: bpf: allow to inject HID event from BPF
selftests/hid: add tests for hid_bpf_input_report
HID: bpf: allow to use bpf_timer_set_sleepable_cb() in tracing callbacks.
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 226 ++++++++++++++-------
drivers/hid/hid-core.c | 2 +
include/linux/hid_bpf.h | 3 +
tools/testing/selftests/hid/config.common | 1 +
tools/testing/selftests/hid/hid_bpf.c | 112 +++++++++-
tools/testing/selftests/hid/progs/hid.c | 46 +++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 6 +
8 files changed, 324 insertions(+), 74 deletions(-)
---
base-commit: 3e78a6c0d3e02e4cf881dc84c5127e9990f939d6
change-id: 20240314-b4-hid-bpf-new-funcs-ecf05d0ef870
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* [PATCH v4 1/7] HID: bpf/dispatch: regroup kfuncs definitions
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
No code change, just move down the hid_bpf_get_data() kfunc definition
so we have only one block of __bpf_kfunc_start/end_defs()
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
no changes in v2
---
drivers/hid/bpf/hid_bpf_dispatch.c | 80 ++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 42 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index e630caf644e8..52abb27426f4 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -143,48 +143,6 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
}
EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
-/* Disables missing prototype warnings */
-__bpf_kfunc_start_defs();
-
-/**
- * hid_bpf_get_data - Get the kernel memory pointer associated with the context @ctx
- *
- * @ctx: The HID-BPF context
- * @offset: The offset within the memory
- * @rdwr_buf_size: the const size of the buffer
- *
- * @returns %NULL on error, an %__u8 memory pointer on success
- */
-__bpf_kfunc __u8 *
-hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr_buf_size)
-{
- struct hid_bpf_ctx_kern *ctx_kern;
-
- if (!ctx)
- return NULL;
-
- ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
-
- if (rdwr_buf_size + offset > ctx->allocated_size)
- return NULL;
-
- return ctx_kern->data + offset;
-}
-__bpf_kfunc_end_defs();
-
-/*
- * The following set contains all functions we agree BPF programs
- * can use.
- */
-BTF_KFUNCS_START(hid_bpf_kfunc_ids)
-BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
-BTF_KFUNCS_END(hid_bpf_kfunc_ids)
-
-static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
- .owner = THIS_MODULE,
- .set = &hid_bpf_kfunc_ids,
-};
-
static int device_match_id(struct device *dev, const void *id)
{
struct hid_device *hdev = to_hid_device(dev);
@@ -281,6 +239,31 @@ static int do_hid_bpf_attach_prog(struct hid_device *hdev, int prog_fd, struct b
/* Disables missing prototype warnings */
__bpf_kfunc_start_defs();
+/**
+ * hid_bpf_get_data - Get the kernel memory pointer associated with the context @ctx
+ *
+ * @ctx: The HID-BPF context
+ * @offset: The offset within the memory
+ * @rdwr_buf_size: the const size of the buffer
+ *
+ * @returns %NULL on error, an %__u8 memory pointer on success
+ */
+__bpf_kfunc __u8 *
+hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr_buf_size)
+{
+ struct hid_bpf_ctx_kern *ctx_kern;
+
+ if (!ctx)
+ return NULL;
+
+ ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
+
+ if (rdwr_buf_size + offset > ctx->allocated_size)
+ return NULL;
+
+ return ctx_kern->data + offset;
+}
+
/**
* hid_bpf_attach_prog - Attach the given @prog_fd to the given HID device
*
@@ -474,6 +457,19 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
}
__bpf_kfunc_end_defs();
+/*
+ * The following set contains all functions we agree BPF programs
+ * can use.
+ */
+BTF_KFUNCS_START(hid_bpf_kfunc_ids)
+BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
+BTF_KFUNCS_END(hid_bpf_kfunc_ids)
+
+static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
+ .owner = THIS_MODULE,
+ .set = &hid_bpf_kfunc_ids,
+};
+
/* our HID-BPF entrypoints */
BTF_SET8_START(hid_bpf_fmodret_ids)
BTF_ID_FLAGS(func, hid_bpf_device_event)
--
2.44.0
^ permalink raw reply related
* [PATCH v4 2/7] HID: bpf: export hid_hw_output_report as a BPF kfunc
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
We currently only export hid_hw_raw_request() as a BPF kfunc.
However, some devices require an explicit write on the Output Report
instead of the use of the control channel.
So also export hid_hw_output_report to BPF
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
no changes in v2
---
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 112 +++++++++++++++++++++++++++----------
drivers/hid/hid-core.c | 1 +
include/linux/hid_bpf.h | 1 +
4 files changed, 86 insertions(+), 30 deletions(-)
diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
index 4fad83a6ebc3..a575004d9025 100644
--- a/Documentation/hid/hid-bpf.rst
+++ b/Documentation/hid/hid-bpf.rst
@@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
-----------------------------------------------------------
.. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
- :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_allocate_context hid_bpf_release_context
+ :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
General overview of a HID-BPF program
=====================================
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index 52abb27426f4..a5b88b491b80 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -376,6 +376,46 @@ hid_bpf_release_context(struct hid_bpf_ctx *ctx)
put_device(&hid->dev);
}
+static int
+__hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
+ enum hid_report_type rtype)
+{
+ struct hid_report_enum *report_enum;
+ struct hid_report *report;
+ struct hid_device *hdev;
+ u32 report_len;
+
+ /* check arguments */
+ if (!ctx || !hid_bpf_ops || !buf)
+ return -EINVAL;
+
+ switch (rtype) {
+ case HID_INPUT_REPORT:
+ case HID_OUTPUT_REPORT:
+ case HID_FEATURE_REPORT:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (*buf__sz < 1)
+ return -EINVAL;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ report_enum = hdev->report_enum + rtype;
+ report = hid_bpf_ops->hid_get_report(report_enum, buf);
+ if (!report)
+ return -EINVAL;
+
+ report_len = hid_report_len(report);
+
+ if (*buf__sz > report_len)
+ *buf__sz = report_len;
+
+ return 0;
+}
+
/**
* hid_bpf_hw_request - Communicate with a HID device
*
@@ -392,24 +432,14 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
enum hid_report_type rtype, enum hid_class_request reqtype)
{
struct hid_device *hdev;
- struct hid_report *report;
- struct hid_report_enum *report_enum;
+ size_t size = buf__sz;
u8 *dma_data;
- u32 report_len;
int ret;
/* check arguments */
- if (!ctx || !hid_bpf_ops || !buf)
- return -EINVAL;
-
- switch (rtype) {
- case HID_INPUT_REPORT:
- case HID_OUTPUT_REPORT:
- case HID_FEATURE_REPORT:
- break;
- default:
- return -EINVAL;
- }
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
+ if (ret)
+ return ret;
switch (reqtype) {
case HID_REQ_GET_REPORT:
@@ -423,29 +453,16 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
return -EINVAL;
}
- if (buf__sz < 1)
- return -EINVAL;
-
hdev = (struct hid_device *)ctx->hid; /* discard const */
- report_enum = hdev->report_enum + rtype;
- report = hid_bpf_ops->hid_get_report(report_enum, buf);
- if (!report)
- return -EINVAL;
-
- report_len = hid_report_len(report);
-
- if (buf__sz > report_len)
- buf__sz = report_len;
-
- dma_data = kmemdup(buf, buf__sz, GFP_KERNEL);
+ dma_data = kmemdup(buf, size, GFP_KERNEL);
if (!dma_data)
return -ENOMEM;
ret = hid_bpf_ops->hid_hw_raw_request(hdev,
dma_data[0],
dma_data,
- buf__sz,
+ size,
rtype,
reqtype);
@@ -455,6 +472,42 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
kfree(dma_data);
return ret;
}
+
+/**
+ * hid_bpf_hw_output_report - Send an output report to a HID device
+ *
+ * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
+ * @buf: a %PTR_TO_MEM buffer
+ * @buf__sz: the size of the data to transfer
+ *
+ * @returns the number of bytes transferred on success, a negative error code otherwise.
+ */
+__bpf_kfunc int
+hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
+{
+ struct hid_device *hdev;
+ size_t size = buf__sz;
+ u8 *dma_data;
+ int ret;
+
+ /* check arguments */
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, HID_OUTPUT_REPORT);
+ if (ret)
+ return ret;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ dma_data = kmemdup(buf, size, GFP_KERNEL);
+ if (!dma_data)
+ return -ENOMEM;
+
+ ret = hid_bpf_ops->hid_hw_output_report(hdev,
+ dma_data,
+ size);
+
+ kfree(dma_data);
+ return ret;
+}
__bpf_kfunc_end_defs();
/*
@@ -488,6 +541,7 @@ BTF_ID_FLAGS(func, hid_bpf_attach_prog)
BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
+BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index de7a477d6665..1243595890ba 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2974,6 +2974,7 @@ EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
static struct hid_bpf_ops hid_ops = {
.hid_get_report = hid_get_report,
.hid_hw_raw_request = hid_hw_raw_request,
+ .hid_hw_output_report = hid_hw_output_report,
.owner = THIS_MODULE,
.bus_type = &hid_bus_type,
};
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 7118ac28d468..5c7ff93dc73e 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -103,6 +103,7 @@ struct hid_bpf_ops {
unsigned char reportnum, __u8 *buf,
size_t len, enum hid_report_type rtype,
enum hid_class_request reqtype);
+ int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
struct module *owner;
const struct bus_type *bus_type;
};
--
2.44.0
^ permalink raw reply related
* [PATCH v4 3/7] selftests/hid: add KASAN to the VM tests
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
It's always a good idea to have KASAN in tests.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
new in v3
---
tools/testing/selftests/hid/config.common | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/hid/config.common b/tools/testing/selftests/hid/config.common
index 0f456dbab62f..45b5570441ce 100644
--- a/tools/testing/selftests/hid/config.common
+++ b/tools/testing/selftests/hid/config.common
@@ -238,3 +238,4 @@ CONFIG_VLAN_8021Q=y
CONFIG_XFRM_SUB_POLICY=y
CONFIG_XFRM_USER=y
CONFIG_ZEROPLUS_FF=y
+CONFIG_KASAN=y
--
2.44.0
^ permalink raw reply related
* [PATCH v4 4/7] selftests/hid: Add test for hid_bpf_hw_output_report
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
This time we need to ensure uhid receives it, thus the new mutex and
condition.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
no changes in v2
---
tools/testing/selftests/hid/hid_bpf.c | 63 ++++++++++++++++++++++
tools/testing/selftests/hid/progs/hid.c | 24 +++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 2 +
3 files changed, 89 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 2cf96f818f25..8332014838b0 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -16,6 +16,11 @@
#define SHOW_UHID_DEBUG 0
+#define min(a, b) \
+ ({ __typeof__(a) _a = (a); \
+ __typeof__(b) _b = (b); \
+ _a < _b ? _a : _b; })
+
static unsigned char rdesc[] = {
0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
0x09, 0x21, /* Usage (Vendor Usage 0x21) */
@@ -111,6 +116,10 @@ struct hid_hw_request_syscall_args {
static pthread_mutex_t uhid_started_mtx = PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t uhid_started = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t uhid_output_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t uhid_output_cond = PTHREAD_COND_INITIALIZER;
+static unsigned char output_report[10];
+
/* no need to protect uhid_stopped, only one thread accesses it */
static bool uhid_stopped;
@@ -205,6 +214,13 @@ static int uhid_event(struct __test_metadata *_metadata, int fd)
break;
case UHID_OUTPUT:
UHID_LOG("UHID_OUTPUT from uhid-dev");
+
+ pthread_mutex_lock(&uhid_output_mtx);
+ memcpy(output_report,
+ ev.u.output.data,
+ min(ev.u.output.size, sizeof(output_report)));
+ pthread_cond_signal(&uhid_output_cond);
+ pthread_mutex_unlock(&uhid_output_mtx);
break;
case UHID_GET_REPORT:
UHID_LOG("UHID_GET_REPORT from uhid-dev");
@@ -733,6 +749,53 @@ TEST_F(hid_bpf, test_hid_change_report)
ASSERT_EQ(buf[2], 0) TH_LOG("leftovers_from_previous_test");
}
+/*
+ * Call hid_bpf_hw_output_report against the given uhid device,
+ * check that the program is called and does the expected.
+ */
+TEST_F(hid_bpf, test_hid_user_output_report_call)
+{
+ struct hid_hw_request_syscall_args args = {
+ .retval = -1,
+ .size = 10,
+ };
+ DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
+ .ctx_in = &args,
+ .ctx_size_in = sizeof(args),
+ );
+ int err, cond_err, prog_fd;
+ struct timespec time_to_wait;
+
+ LOAD_BPF;
+
+ args.hid = self->hid_id;
+ args.data[0] = 1; /* report ID */
+ args.data[1] = 2; /* report ID */
+ args.data[2] = 42; /* report ID */
+
+ prog_fd = bpf_program__fd(self->skel->progs.hid_user_output_report);
+
+ pthread_mutex_lock(&uhid_output_mtx);
+
+ memset(output_report, 0, sizeof(output_report));
+ clock_gettime(CLOCK_REALTIME, &time_to_wait);
+ time_to_wait.tv_sec += 2;
+
+ err = bpf_prog_test_run_opts(prog_fd, &tattrs);
+ cond_err = pthread_cond_timedwait(&uhid_output_cond, &uhid_output_mtx, &time_to_wait);
+
+ ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
+ ASSERT_OK(cond_err) TH_LOG("error while calling waiting for the condition");
+
+ ASSERT_EQ(args.retval, 3);
+
+ ASSERT_EQ(output_report[0], 1);
+ ASSERT_EQ(output_report[1], 2);
+ ASSERT_EQ(output_report[2], 42);
+
+ pthread_mutex_unlock(&uhid_output_mtx);
+}
+
/*
* Attach hid_user_raw_request to the given uhid device,
* call the bpf program from userspace
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 1e558826b809..2c2b679a83b1 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -101,6 +101,30 @@ int hid_user_raw_request(struct hid_hw_request_syscall_args *args)
return 0;
}
+SEC("syscall")
+int hid_user_output_report(struct hid_hw_request_syscall_args *args)
+{
+ struct hid_bpf_ctx *ctx;
+ const size_t size = args->size;
+ int i, ret = 0;
+
+ if (size > sizeof(args->data))
+ return -7; /* -E2BIG */
+
+ ctx = hid_bpf_allocate_context(args->hid);
+ if (!ctx)
+ return -1; /* EPERM check */
+
+ ret = hid_bpf_hw_output_report(ctx,
+ args->data,
+ size);
+ args->retval = ret;
+
+ hid_bpf_release_context(ctx);
+
+ return 0;
+}
+
static const __u8 rdesc[] = {
0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
0x09, 0x32, /* USAGE (Z) */
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 65e657ac1198..50c6a0d5765e 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -94,5 +94,7 @@ extern int hid_bpf_hw_request(struct hid_bpf_ctx *ctx,
size_t buf__sz,
enum hid_report_type type,
enum hid_class_request reqtype) __ksym;
+extern int hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx,
+ __u8 *buf, size_t buf__sz) __ksym;
#endif /* __HID_BPF_HELPERS_H */
--
2.44.0
^ permalink raw reply related
* [PATCH v4 5/7] HID: bpf: allow to inject HID event from BPF
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
It can be interesting to inject events from BPF as if the event were
to come from the device.
For example, some multitouch devices do not all the time send a proximity
out event, and we might want to send it for the physical device.
Compared to uhid, we can now inject events on any physical device, not
just uhid virtual ones.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
no changes in v2
---
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++++++++
drivers/hid/hid-core.c | 1 +
include/linux/hid_bpf.h | 2 ++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
index a575004d9025..0765b3298ecf 100644
--- a/Documentation/hid/hid-bpf.rst
+++ b/Documentation/hid/hid-bpf.rst
@@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
-----------------------------------------------------------
.. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
- :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
+ :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_input_report hid_bpf_allocate_context hid_bpf_release_context
General overview of a HID-BPF program
=====================================
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index a5b88b491b80..e1a650f4a626 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -508,6 +508,34 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
kfree(dma_data);
return ret;
}
+
+/**
+ * hid_bpf_input_report - Inject a HID report in the kernel from a HID device
+ *
+ * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
+ * @type: the type of the report (%HID_INPUT_REPORT, %HID_FEATURE_REPORT, %HID_OUTPUT_REPORT)
+ * @buf: a %PTR_TO_MEM buffer
+ * @buf__sz: the size of the data to transfer
+ *
+ * @returns %0 on success, a negative error code otherwise.
+ */
+__bpf_kfunc int
+hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *buf,
+ const size_t buf__sz)
+{
+ struct hid_device *hdev;
+ size_t size = buf__sz;
+ int ret;
+
+ /* check arguments */
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, type);
+ if (ret)
+ return ret;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ return hid_input_report(hdev, type, buf, size, 0);
+}
__bpf_kfunc_end_defs();
/*
@@ -542,6 +570,7 @@ BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
+BTF_ID_FLAGS(func, hid_bpf_input_report)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1243595890ba..b1fa0378e8f4 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2975,6 +2975,7 @@ static struct hid_bpf_ops hid_ops = {
.hid_get_report = hid_get_report,
.hid_hw_raw_request = hid_hw_raw_request,
.hid_hw_output_report = hid_hw_output_report,
+ .hid_input_report = hid_input_report,
.owner = THIS_MODULE,
.bus_type = &hid_bus_type,
};
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 5c7ff93dc73e..17b08f500098 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -104,6 +104,8 @@ struct hid_bpf_ops {
size_t len, enum hid_report_type rtype,
enum hid_class_request reqtype);
int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
+ int (*hid_input_report)(struct hid_device *hid, enum hid_report_type type,
+ u8 *data, u32 size, int interrupt);
struct module *owner;
const struct bus_type *bus_type;
};
--
2.44.0
^ permalink raw reply related
* [PATCH v4 6/7] selftests/hid: add tests for hid_bpf_input_report
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
Usual way of testing, we call the function and ensures we receive
the event
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
no changes in v2
---
tools/testing/selftests/hid/hid_bpf.c | 49 +++++++++++++++++++++-
tools/testing/selftests/hid/progs/hid.c | 22 ++++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 4 ++
3 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 8332014838b0..f825623e3edc 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -749,6 +749,52 @@ TEST_F(hid_bpf, test_hid_change_report)
ASSERT_EQ(buf[2], 0) TH_LOG("leftovers_from_previous_test");
}
+/*
+ * Call hid_bpf_input_report against the given uhid device,
+ * check that the program is called and does the expected.
+ */
+TEST_F(hid_bpf, test_hid_user_input_report_call)
+{
+ struct hid_hw_request_syscall_args args = {
+ .retval = -1,
+ .size = 10,
+ };
+ DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
+ .ctx_in = &args,
+ .ctx_size_in = sizeof(args),
+ );
+ __u8 buf[10] = {0};
+ int err, prog_fd;
+
+ LOAD_BPF;
+
+ args.hid = self->hid_id;
+ args.data[0] = 1; /* report ID */
+ args.data[1] = 2; /* report ID */
+ args.data[2] = 42; /* report ID */
+
+ prog_fd = bpf_program__fd(self->skel->progs.hid_user_input_report);
+
+ /* check that there is no data to read from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+
+ err = bpf_prog_test_run_opts(prog_fd, &tattrs);
+
+ ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
+
+ ASSERT_EQ(args.retval, 0);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 1);
+ ASSERT_EQ(buf[1], 2);
+ ASSERT_EQ(buf[2], 42);
+}
+
/*
* Call hid_bpf_hw_output_report against the given uhid device,
* check that the program is called and does the expected.
@@ -797,8 +843,7 @@ TEST_F(hid_bpf, test_hid_user_output_report_call)
}
/*
- * Attach hid_user_raw_request to the given uhid device,
- * call the bpf program from userspace
+ * Call hid_hw_raw_request against the given uhid device,
* check that the program is called and does the expected.
*/
TEST_F(hid_bpf, test_hid_user_raw_request_call)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 2c2b679a83b1..f67d35def142 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -125,6 +125,28 @@ int hid_user_output_report(struct hid_hw_request_syscall_args *args)
return 0;
}
+SEC("syscall")
+int hid_user_input_report(struct hid_hw_request_syscall_args *args)
+{
+ struct hid_bpf_ctx *ctx;
+ const size_t size = args->size;
+ int i, ret = 0;
+
+ if (size > sizeof(args->data))
+ return -7; /* -E2BIG */
+
+ ctx = hid_bpf_allocate_context(args->hid);
+ if (!ctx)
+ return -1; /* EPERM check */
+
+ ret = hid_bpf_input_report(ctx, HID_INPUT_REPORT, args->data, size);
+ args->retval = ret;
+
+ hid_bpf_release_context(ctx);
+
+ return 0;
+}
+
static const __u8 rdesc[] = {
0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
0x09, 0x32, /* USAGE (Z) */
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 50c6a0d5765e..9cd56821d0f1 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -96,5 +96,9 @@ extern int hid_bpf_hw_request(struct hid_bpf_ctx *ctx,
enum hid_class_request reqtype) __ksym;
extern int hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx,
__u8 *buf, size_t buf__sz) __ksym;
+extern int hid_bpf_input_report(struct hid_bpf_ctx *ctx,
+ enum hid_report_type type,
+ __u8 *data,
+ size_t buf__sz) __ksym;
#endif /* __HID_BPF_HELPERS_H */
--
2.44.0
^ permalink raw reply related
* [PATCH v4 7/7] HID: bpf: allow to use bpf_timer_set_sleepable_cb() in tracing callbacks.
From: Benjamin Tissoires @ 2024-03-15 14:44 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: Benjamin Tissoires, linux-input, linux-kernel, bpf, linux-doc,
linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-0-079c282469d3@kernel.org>
Export the sleepable kfuncs we have on HID-BPF in tracing bpf programs,
but with the condition of being used in a sleepable context.
This allows to use the bpf_timer when used in a sleepable context
through bpf_timer_set_sleepable_cb() and initiate work from a device event.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v4
no changes in v3
new in v2
---
drivers/hid/bpf/hid_bpf_dispatch.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index e1a650f4a626..e27c2ffb4f60 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -544,6 +544,11 @@ __bpf_kfunc_end_defs();
*/
BTF_KFUNCS_START(hid_bpf_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
+BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_request, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_output_report, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_input_report, KF_SLEEPABLE)
BTF_KFUNCS_END(hid_bpf_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v4 5/7] HID: bpf: allow to inject HID event from BPF
From: Randy Dunlap @ 2024-03-15 15:55 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Benjamin Tissoires,
Jonathan Corbet, Shuah Khan
Cc: linux-input, linux-kernel, bpf, linux-doc, linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-5-079c282469d3@kernel.org>
On 3/15/24 07:44, Benjamin Tissoires wrote:
> It can be interesting to inject events from BPF as if the event were
> to come from the device.
> For example, some multitouch devices do not all the time send a proximity
> out event, and we might want to send it for the physical device.
>
> Compared to uhid, we can now inject events on any physical device, not
> just uhid virtual ones.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> no changes in v4
>
> no changes in v3
>
> no changes in v2
> ---
> Documentation/hid/hid-bpf.rst | 2 +-
> drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++++++++
> drivers/hid/hid-core.c | 1 +
> include/linux/hid_bpf.h | 2 ++
> 4 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
> index a575004d9025..0765b3298ecf 100644
> --- a/Documentation/hid/hid-bpf.rst
> +++ b/Documentation/hid/hid-bpf.rst
> @@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
> -----------------------------------------------------------
>
> .. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
> - :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
> + :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_input_report hid_bpf_allocate_context hid_bpf_release_context
>
> General overview of a HID-BPF program
> =====================================
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index a5b88b491b80..e1a650f4a626 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -508,6 +508,34 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
> kfree(dma_data);
> return ret;
> }
> +
> +/**
> + * hid_bpf_input_report - Inject a HID report in the kernel from a HID device
> + *
> + * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
> + * @type: the type of the report (%HID_INPUT_REPORT, %HID_FEATURE_REPORT, %HID_OUTPUT_REPORT)
> + * @buf: a %PTR_TO_MEM buffer
> + * @buf__sz: the size of the data to transfer
> + *
> + * @returns %0 on success, a negative error code otherwise.
Minimum change:
@returns:
Preferred change:
Returns:
"@returns" is not documented.
> + */
> +__bpf_kfunc int
> +hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *buf,
> + const size_t buf__sz)
> +{
--
#Randy
^ permalink raw reply
* Re: [PATCH v4 2/7] HID: bpf: export hid_hw_output_report as a BPF kfunc
From: Randy Dunlap @ 2024-03-15 15:55 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Benjamin Tissoires,
Jonathan Corbet, Shuah Khan
Cc: linux-input, linux-kernel, bpf, linux-doc, linux-kselftest
In-Reply-To: <20240315-b4-hid-bpf-new-funcs-v4-2-079c282469d3@kernel.org>
On 3/15/24 07:44, Benjamin Tissoires wrote:
> +/**
> + * hid_bpf_hw_output_report - Send an output report to a HID device
> + *
> + * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
> + * @buf: a %PTR_TO_MEM buffer
> + * @buf__sz: the size of the data to transfer
> + *
> + * @returns the number of bytes transferred on success, a negative error code otherwise.
> + */
Minimum change:
@returns:
Preferred change:
Returns:
"@returns" is not documented.
Thanks.
--
#Randy
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: samsung,s3c6410-keypad: convert to DT Schema
From: Rob Herring @ 2024-03-15 16:10 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Dmitry Torokhov, linux-kernel, Krzysztof Kozlowski, Conor Dooley,
linux-input, devicetree
In-Reply-To: <20240312183001.714626-1-krzysztof.kozlowski@linaro.org>
On Tue, 12 Mar 2024 19:30:01 +0100, Krzysztof Kozlowski wrote:
> Convert Samsung SoC Keypad bindings to DT schema with changes:
> 1. Rename "linux,keypad-no-autorepeat" property to
> "linux,input-no-autorepeat", because the latter was implemented in
> the Linux driver.
> 2. Add clocks and clock-names, already used by DTS and the Linux driver.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> .../input/samsung,s3c6410-keypad.yaml | 121 ++++++++++++++++++
> .../bindings/input/samsung-keypad.txt | 77 -----------
> .../bindings/power/wakeup-source.txt | 2 +-
> 3 files changed, 122 insertions(+), 78 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> delete mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ 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