* Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Konrad Dybcio @ 2024-04-10 18:10 UTC (permalink / raw)
To: quic_fenglinw, kernel, Andy Gross, Bjorn Andersson,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree
In-Reply-To: <20240401-pm8xxx-vibrator-new-design-v8-3-6f2b8b03b4c7@quicinc.com>
On 4/1/24 10:38, Fenglin Wu via B4 Relay wrote:
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> Add support for a new SPMI vibrator module which is very similar
> to the vibrator module inside PM8916 but has a finer drive voltage
> step and different output voltage range, its drive level control
> is expanded across 2 registers. The vibrator module can be found
> in following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
[...]
>
> -#define VIB_MAX_LEVEL_mV (3100)
> -#define VIB_MIN_LEVEL_mV (1200)
> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
> +#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100))
You shouldn't need the additional inside parentheses
Also, is this really a good discriminator for the voltage ranges? Do *all*
PMIC vibrators with a drv2_addr operate within this range? If not, consider
a struct field here
> +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200))
> +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib))
If the ranges are supposed to be inclusive, this is off-by-one. But looking
at the driver, it seems like MIN_LEVEL may be "off"? I'm not sure though.
Either way, this would be a separate fix.
[...]
> +static struct pm8xxx_regs pmi632_regs = {
> + .enable_offset = 0x46,
> + .enable_mask = BIT(7),
> + .drv_offset = 0x40,
> + .drv_mask = 0xFF,
GENMASK(7, 0)
> + .drv_shift = 0,
> + .drv2_offset = 0x41,
> + .drv2_mask = 0x0F,
GENMASK(3, 0)
[...]
>
> + if (regs->drv2_mask) {
> + if (on)
> + val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
> + else
> + val = 0;
> + rc = regmap_write(vib->regmap, vib->drv2_addr, val);
Are you purposefuly zeroing out the other bits?
If yes, consider regmap_write_bits here
If not, consider regmap_update_bits here
> + if (rc < 0)
> + return rc;
Ignore regmap_r/w errors, these mean a complete failure of the API and
we don't generally assume MMIO accesses can fail
Unless SPMI is known to have issues here
> + }
> +
> if (regs->enable_mask)
> rc = regmap_update_bits(vib->regmap, vib->enable_addr,
> regs->enable_mask, on ? ~0 : 0);
> @@ -114,19 +141,22 @@ static void pm8xxx_work_handler(struct work_struct *work)
> return;
>
> /*
> - * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
> + * pmic vibrator supports voltage ranges from MIN_LEVEL to MAX_LEVEL, so
> * scale the level to fit into these ranges.
> */
> if (vib->speed) {
> vib->active = true;
> - vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> - VIB_MIN_LEVEL_mV;
> - vib->level /= 100;
> + vib->level = ((VIB_MAX_LEVELS(vib) * vib->speed) / MAX_FF_SPEED) +
mult_frac()
> + VIB_MIN_LEVEL_mV(vib);
vib->level = VIB_MIN_LEVEL_mV;
vib->level += the other thing
for readability?
> } else {
> vib->active = false;
> - vib->level = VIB_MIN_LEVEL_mV / 100;
> + vib->level = VIB_MIN_LEVEL_mV(vib);
> +
> }
>
> + if (!vib->drv2_addr)
> + vib->level /= 100;
Maybe this could be moved to pm8xxx_vib_set() instead
> +
> pm8xxx_vib_set(vib, vib->active);
> }
>
> @@ -202,7 +232,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>
> vib->enable_addr = reg_base + regs->enable_offset;
> vib->drv_addr = reg_base + regs->drv_offset;
> -
> + vib->drv2_addr = reg_base + regs->drv2_offset;
It would be nice to preserve a newline between assignments and rw
functions here
Thanks for working on this!
Konrad
^ permalink raw reply
* [PATCH 0/6] media: Fix warnings for smatch and sparse
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (6):
media: usb: siano: Fix allocation of urbs
media: cxd2880: Replaze kmalloc with kzalloc
media: platform: sti: hva: clk_unprepare unconditionally
media: v4l2-ctrls-core.c: Do not use iterator outside loop
media: adv7180: Only request valids IRQs
media: touchscreen: sur40: convert le16 to cpu before use
drivers/input/touchscreen/sur40.c | 2 +-
drivers/media/i2c/adv7180.c | 2 +-
drivers/media/platform/st/sti/hva/hva-hw.c | 3 +--
drivers/media/spi/cxd2880-spi.c | 2 +-
drivers/media/usb/siano/smsusb.c | 28 +++++++++++++++++--------
drivers/media/v4l2-core/v4l2-ctrls-api.c | 33 +++++++++++++++++-------------
6 files changed, 43 insertions(+), 27 deletions(-)
---
base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8
change-id: 20240410-smatch-8f235d50753d
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
^ permalink raw reply
* [PATCH 1/6] media: usb: siano: Fix allocation of urbs
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
USB urbs must be allocated with usb_alloc_urb. Quoting the manual
Only use this function (usb_init_urb) if you _really_ understand what you
are doing.
Fix the following smatch error:
drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 723510520d092..d85308e0785db 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -40,7 +40,7 @@ struct smsusb_urb_t {
struct smscore_buffer_t *cb;
struct smsusb_device_t *dev;
- struct urb urb;
+ struct urb *urb;
/* For the bottom half */
struct work_struct wq;
@@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
}
usb_fill_bulk_urb(
- &surb->urb,
+ surb->urb,
dev->udev,
usb_rcvbulkpipe(dev->udev, dev->in_ep),
surb->cb->p,
@@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
smsusb_onresponse,
surb
);
- surb->urb.transfer_flags |= URB_FREE_BUFFER;
+ surb->urb->transfer_flags |= URB_FREE_BUFFER;
- return usb_submit_urb(&surb->urb, GFP_ATOMIC);
+ return usb_submit_urb(surb->urb, GFP_ATOMIC);
}
static void smsusb_stop_streaming(struct smsusb_device_t *dev)
@@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
int i;
for (i = 0; i < MAX_URBS; i++) {
- usb_kill_urb(&dev->surbs[i].urb);
+ usb_kill_urb(dev->surbs[i].urb);
if (dev->surbs[i].wq.func)
cancel_work_sync(&dev->surbs[i].wq);
@@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf)
struct smsusb_device_t *dev = usb_get_intfdata(intf);
if (dev) {
+ int i;
+
dev->state = SMSUSB_DISCONNECTED;
smsusb_stop_streaming(dev);
@@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf)
if (dev->coredev)
smscore_unregister_device(dev->coredev);
+ for (i = 0; i < MAX_URBS; i++)
+ usb_free_urb(dev->surbs[i].urb);
+
pr_debug("device 0x%p destroyed\n", dev);
kfree(dev);
}
@@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
void *mdev;
int i, rc;
int align = 0;
+ int n_urb = 0;
/* create device object */
dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
dev->coredev->is_usb_device = true;
/* initialize urbs */
- for (i = 0; i < MAX_URBS; i++) {
- dev->surbs[i].dev = dev;
- usb_init_urb(&dev->surbs[i].urb);
+ for (n_urb = 0; n_urb < MAX_URBS; n_urb++) {
+ dev->surbs[n_urb].dev = dev;
+ dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!dev->surbs[n_urb].urb)
+ goto free_urbs;
}
pr_debug("smsusb_start_streaming(...).\n");
@@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
return rc;
+free_urbs:
+ for (i = 0; i < n_urb; i++)
+ usb_free_urb(dev->surbs[n_urb].urb);
+
err_unregister_device:
smsusb_term_device(intf);
#ifdef CONFIG_MEDIA_CONTROLLER_DVB
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [PATCH 2/6] media: cxd2880: Replaze kmalloc with kzalloc
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
Fix smatch error:
drivers/media/spi/cxd2880-spi.c:391 cxd2880_start_feed() warn: Please consider using kzalloc instead of kmalloc
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/spi/cxd2880-spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c
index 6be4e5528879f..65fa7f857fcaf 100644
--- a/drivers/media/spi/cxd2880-spi.c
+++ b/drivers/media/spi/cxd2880-spi.c
@@ -388,7 +388,7 @@ static int cxd2880_start_feed(struct dvb_demux_feed *feed)
if (dvb_spi->feed_count == 0) {
dvb_spi->ts_buf =
- kmalloc(MAX_TRANS_PKT * 188,
+ kzalloc(MAX_TRANS_PKT * 188,
GFP_KERNEL | GFP_DMA);
if (!dvb_spi->ts_buf) {
pr_err("ts buffer allocate failed\n");
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [PATCH 3/6] media: platform: sti: hva: clk_unprepare unconditionally
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
hva->clk cannot be NULL at this point. Simplify the code and make smatch
happy:
drivers/media/platform/st/sti/hva/hva-hw.c:412 hva_hw_probe() warn: 'hva->clk' from clk_prepare() not released on lines: 412
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/platform/st/sti/hva/hva-hw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/platform/st/sti/hva/hva-hw.c b/drivers/media/platform/st/sti/hva/hva-hw.c
index fe4ea2e7f37e3..fcb18fb52fdd7 100644
--- a/drivers/media/platform/st/sti/hva/hva-hw.c
+++ b/drivers/media/platform/st/sti/hva/hva-hw.c
@@ -406,8 +406,7 @@ int hva_hw_probe(struct platform_device *pdev, struct hva_dev *hva)
err_disable:
pm_runtime_disable(dev);
err_clk:
- if (hva->clk)
- clk_unprepare(hva->clk);
+ clk_unprepare(hva->clk);
return ret;
}
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [PATCH 4/6] media: v4l2-ctrls-core.c: Do not use iterator outside loop
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
Simplify a bit the code introducing a new variable for iterating through
the control list.
It also makes smatch happy:
drivers/media/v4l2-core/v4l2-ctrls-api.c:1091 v4l2_query_ext_ctrl() warn: iterator used outside loop: 'ref'
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/v4l2-core/v4l2-ctrls-api.c | 33 ++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index d9a422017bd9d..42b7a45bfa79c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -1052,35 +1052,40 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
if (id >= node2id(hdl->ctrl_refs.prev)) {
ref = NULL; /* Yes, so there is no next control */
} else if (ref) {
+ struct v4l2_ctrl_ref *pos = ref;
+
/*
* We found a control with the given ID, so just get
* the next valid one in the list.
*/
- list_for_each_entry_continue(ref, &hdl->ctrl_refs, node) {
- is_compound = ref->ctrl->is_array ||
- ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
- if (id < ref->ctrl->id &&
- (is_compound & mask) == match)
+ ref = NULL;
+ list_for_each_entry_continue(pos, &hdl->ctrl_refs, node) {
+ is_compound = pos->ctrl->is_array ||
+ pos->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+ if (id < pos->ctrl->id &&
+ (is_compound & mask) == match) {
+ ref = pos;
break;
+ }
}
- if (&ref->node == &hdl->ctrl_refs)
- ref = NULL;
} else {
+ struct v4l2_ctrl_ref *pos;
+
/*
* No control with the given ID exists, so start
* searching for the next largest ID. We know there
* is one, otherwise the first 'if' above would have
* been true.
*/
- list_for_each_entry(ref, &hdl->ctrl_refs, node) {
- is_compound = ref->ctrl->is_array ||
- ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
- if (id < ref->ctrl->id &&
- (is_compound & mask) == match)
+ list_for_each_entry(pos, &hdl->ctrl_refs, node) {
+ is_compound = pos->ctrl->is_array ||
+ pos->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+ if (id < pos->ctrl->id &&
+ (is_compound & mask) == match) {
+ ref = pos;
break;
+ }
}
- if (&ref->node == &hdl->ctrl_refs)
- ref = NULL;
}
}
mutex_unlock(hdl->lock);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [PATCH 5/6] media: adv7180: Only request valids IRQs
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
i2c_device_probe(), seems to assume that irq = 0 means that there is no
irq to request.
The driver also believes that on the clean path. So lets be consistent
here.
Also make smatch happy.
Fix:
drivers/media/i2c/adv7180.c:1526 adv7180_probe() warn: 'client->irq' from request_threaded_irq() not released on lines: 1526
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/i2c/adv7180.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4829cbe324198..819ff9f7c90fe 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1486,7 +1486,7 @@ static int adv7180_probe(struct i2c_client *client)
if (ret)
goto err_media_entity_cleanup;
- if (state->irq) {
+ if (state->irq > 0) {
ret = request_threaded_irq(client->irq, NULL, adv7180_irq,
IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
KBUILD_MODNAME, state);
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* [PATCH 6/6] media: touchscreen: sur40: convert le16 to cpu before use
From: Ricardo Ribalda @ 2024-04-10 21:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Yasunari Takiguchi, Jean-Christophe Trotin,
Lars-Peter Clausen, Dmitry Torokhov
Cc: Hans Verkuil, linux-media, linux-kernel, linux-input,
Ricardo Ribalda
In-Reply-To: <20240410-smatch-v1-0-785d009a852b@chromium.org>
Smatch found this issue:
drivers/input/touchscreen/sur40.c:424:55: warning: incorrect type in argument 2 (different base types)
drivers/input/touchscreen/sur40.c:424:55: expected int key
drivers/input/touchscreen/sur40.c:424:55: got restricted __le16 [usertype] blob_id
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/input/touchscreen/sur40.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index ae3aab4283370..5f2cf8881e724 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -421,7 +421,7 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input)
if (blob->type != SUR40_TOUCH)
return;
- slotnum = input_mt_get_slot_by_key(input, blob->blob_id);
+ slotnum = input_mt_get_slot_by_key(input, le16_to_cpu(blob->blob_id));
if (slotnum < 0 || slotnum >= MAX_CONTACTS)
return;
--
2.44.0.478.gd926399ef9-goog
^ permalink raw reply related
* Re: [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
From: Dmitry Torokhov @ 2024-04-11 0:02 UTC (permalink / raw)
To: Mark Pearson
Cc: Peter Hutterer, Hans de Goede, Ilpo Järvinen,
Henrique de Moraes Holschuh, ibm-acpi-devel,
platform-driver-x86@vger.kernel.org, linux-input, linux-kernel,
Nitin Joshi1, Vishnu Sankar
In-Reply-To: <92ee5cb2-565e-413c-b968-81393a9211c4@app.fastmail.com>
On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote:
> Hi Dmitry
>
> On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote:
> > On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote:
> >> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote:
> >> > On 09/04/2024 09:31, Dmitry Torokhov wrote:
> >> > > Hi Mark,
> >> > >
> >> > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote:
> >> > > > Add support for new input events on Lenovo laptops that need exporting to
> >> > > > user space.
> >> > > >
> >> > > > Lenovo trackpoints are adding the ability to generate a doubletap event.
> >> > > > Add a new keycode to allow this to be used by userspace.
> >> > >
> >> > > What is the intended meaning of this keycode? How does it differ from
> >> > > the driver sending BTN_LEFT press/release twice?
> >> > > >
> >> > > > Lenovo support is using FN+N with Windows to collect needed details for
> >> > > > support cases. Add a keycode so that we'll be able to provide similar
> >> > > > support on Linux.
> >> > >
> >> > > Is there a userspace consumer for this?
> >> >
> >> > Funnily enough XKB has had a keysym for this for decades but it's not
> >> > hooked up anywhere due to the way it's pointer keys accessibility
> >> > feature was implemented. Theory is that most of userspace just needs
> >> > to patch the various pieces together for the new evdev code + keysym,
> >> > it's not really any different to handling a volume key (except this
> >> > one needs to be assignable).
> >>
> >> What is the keysym? If we can make them relatable to each other that
> >> would be good. Or maybe we could find a matching usage from HID usage
> >> tables...
> >
> > I was looking through the existing codes and I see:
> >
> > #define KEY_INFO 0x166 /* AL OEM Features/Tips/Tutorial */
> >
> > We also have KEY_VENDOR used in a few drivers/plafrom/x86, including
> > thinkkpad_acpi.c and I wonder if it would be suitable for this vendor
> > specific debug info collection application (which I honestly doubt will
> > materialize).
> >
>
> That's a somewhat disappointing note on your doubts, is that based on
> anything? Just wondering what we've done to deserve that criticism.
Sorry, this was not meant as a criticism really, but you mentioned
yourself that there isn't anything in the works yet, you just have some
plans.
For such a project to succeed Lenovo needs to invest into selling
devices with Linux as a primary operating system, and it has to be
consumer segment (or small business, because for corporate they
typically roll their own support channels). The case of retrofitting
Linux onto a that device originally came with Windows OS rarely gets
much if any response from the normal support channels.
Is this something that is actually happening?
>
> That aside, I guess KEY_INFO or KEY_VENDOR could be a good fit (I
> personally don't think KEY_CONFIG matches well), but I would be
> worried about clashing with existing functionality.
>
> Peter - do you have any opinion from the user space side of things, or
> are these likely unused? KEY_VENDOR seems the safer bet to me (but I
> don't love it).
>
> Dmitry - What are the downsides or concerns of introducing a new code?
> I'd like to evaluate that against the potential to cause conflicts by
> re-using existing codes. If you feel strongly about it then I'll defer
> to your judgement, but I'd like to understand better the context.
The keycode space is finite and extending bitmaps leads to more memory
consumption and weird breakages (like uevent generation exceeding 4K
memory page resulting in failures). I am trying to balance need for new
keycodes with how likely they are to be used.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
From: Mark Pearson @ 2024-04-11 2:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Peter Hutterer, Hans de Goede, Ilpo Järvinen,
Henrique de Moraes Holschuh, ibm-acpi-devel,
platform-driver-x86@vger.kernel.org, linux-input, linux-kernel,
Nitin Joshi1, Vishnu Sankar
In-Reply-To: <ZhcogDESvZmUPEEf@google.com>
Hi Dmitry,
On Wed, Apr 10, 2024, at 8:02 PM, Dmitry Torokhov wrote:
> On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote:
>> Hi Dmitry
>>
>> On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote:
>> > On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote:
>> >> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote:
>> >> > On 09/04/2024 09:31, Dmitry Torokhov wrote:
>> >> > > Hi Mark,
>> >> > >
>> >> > > On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote:
>> >> > > > Add support for new input events on Lenovo laptops that need exporting to
>> >> > > > user space.
>> >> > > >
>> >> > > > Lenovo trackpoints are adding the ability to generate a doubletap event.
>> >> > > > Add a new keycode to allow this to be used by userspace.
>> >> > >
>> >> > > What is the intended meaning of this keycode? How does it differ from
>> >> > > the driver sending BTN_LEFT press/release twice?
>> >> > > >
>> >> > > > Lenovo support is using FN+N with Windows to collect needed details for
>> >> > > > support cases. Add a keycode so that we'll be able to provide similar
>> >> > > > support on Linux.
>> >> > >
>> >> > > Is there a userspace consumer for this?
>> >> >
>> >> > Funnily enough XKB has had a keysym for this for decades but it's not
>> >> > hooked up anywhere due to the way it's pointer keys accessibility
>> >> > feature was implemented. Theory is that most of userspace just needs
>> >> > to patch the various pieces together for the new evdev code + keysym,
>> >> > it's not really any different to handling a volume key (except this
>> >> > one needs to be assignable).
>> >>
>> >> What is the keysym? If we can make them relatable to each other that
>> >> would be good. Or maybe we could find a matching usage from HID usage
>> >> tables...
>> >
>> > I was looking through the existing codes and I see:
>> >
>> > #define KEY_INFO 0x166 /* AL OEM Features/Tips/Tutorial */
>> >
>> > We also have KEY_VENDOR used in a few drivers/plafrom/x86, including
>> > thinkkpad_acpi.c and I wonder if it would be suitable for this vendor
>> > specific debug info collection application (which I honestly doubt will
>> > materialize).
>> >
>>
>> That's a somewhat disappointing note on your doubts, is that based on
>> anything? Just wondering what we've done to deserve that criticism.
>
> Sorry, this was not meant as a criticism really, but you mentioned
> yourself that there isn't anything in the works yet, you just have some
> plans.
All good - I was just worried we'd promised something previously and not delivered. We're often slow at delivering - but I try not to fail completely. I try especially hard not to annoy hard working kernel maintainers :)
We do have something developed internally, but it's pretty basic and we'll need to have discussion with 'userspace' fol as to if we release that as a standalone application, or if we tie into gnome (which we don't have a lot of experience with)...or something else.
Kernel world is much easier :)
>
> For such a project to succeed Lenovo needs to invest into selling
> devices with Linux as a primary operating system, and it has to be
> consumer segment (or small business, because for corporate they
> typically roll their own support channels). The case of retrofitting
> Linux onto a that device originally came with Windows OS rarely gets
> much if any response from the normal support channels.
>
> Is this something that is actually happening?
This is way off topic for the patch set, but as you asked :)
I'm afraid I'm going to disagree. What you're suggesting is probably the more common and easier route vendors take, but it has some issues and I think some longer term limitations.
For me one of the things I like the most about our program using the same exact HW as Windows is it means:
- I have a business lever to get Linux support from HW vendors. This makes a difference when you're dealing with 'minor' components - panels, fingerprint readers, touchpads etc - the smaller devices (though it helps for the CPU vendors too). We have requirements that state the upstream Linux support is needed or the chip vendor will not be considered for the platform...and that's a big deal.
- It gets FW teams thinking about Linux support. Yeah, there are still a ton of issues there and it's far from perfect, but it means they have to think about Linux support and at least helps us shine a bit of light on what is going on in that horrible closed box.
- It means I get Linux running at a good level on a wider range of HW then would be otherwise possible. It means that when there is new technology I get to go and ask "how about Linux" and have that discussion (including schedules). It might come later than I'd like - but at least we get to put Linux on the roadmap rather than being excluded for being so niche. Shrug.
I think our Linux program still has a long way to go before it's even close to good - but it's in better shape than it (at least I think so, I don't love the way the industry is going with more being stuffed in FW...but that's above my paygrade)
>
>>
>> That aside, I guess KEY_INFO or KEY_VENDOR could be a good fit (I
>> personally don't think KEY_CONFIG matches well), but I would be
>> worried about clashing with existing functionality.
>>
>> Peter - do you have any opinion from the user space side of things, or
>> are these likely unused? KEY_VENDOR seems the safer bet to me (but I
>> don't love it).
>>
>> Dmitry - What are the downsides or concerns of introducing a new code?
>> I'd like to evaluate that against the potential to cause conflicts by
>> re-using existing codes. If you feel strongly about it then I'll defer
>> to your judgement, but I'd like to understand better the context.
>
> The keycode space is finite and extending bitmaps leads to more memory
> consumption and weird breakages (like uevent generation exceeding 4K
> memory page resulting in failures). I am trying to balance need for new
> keycodes with how likely they are to be used.
>
Ack.
So I've been refactoring my patches to implement the feedback from Hans for the latter patches and just need to figure out what works here.
For the SYS_DEBUG_INFO, if you'd rather we use KEY_VENDOR (I think that's my preference - as its intended for a Lenovo support role it seems the best fit), and Peter has no objections, I will make that change.
I have a stronger preference to keep the KEY_DOUBLECLICK - that one seems less controversial as a genuine new input event.
Is that OK?
Mark
^ permalink raw reply
* Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2024-04-11 6:41 UTC (permalink / raw)
To: Konrad Dybcio, kernel, Andy Gross, Bjorn Andersson,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree
In-Reply-To: <3f8c970c-6a0d-4fc3-a2d3-e0797e7055cf@linaro.org>
Hi Konrad,
On 4/11/2024 2:10 AM, Konrad Dybcio wrote:
>
>
>> -#define VIB_MAX_LEVEL_mV (3100)
>> -#define VIB_MIN_LEVEL_mV (1200)
>> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
>> +#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100))
>
> You shouldn't need the additional inside parentheses
>
> Also, is this really a good discriminator for the voltage ranges? Do *all*
> PMIC vibrators with a drv2_addr operate within this range? If not, consider
> a struct field here
>
Currently, yes, all PMIC vibrators with a drv2_addr (PMI632, PM7250B,
PM7325B, PM7550BA) operate within the same range because they are the
same type.
>
>> +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200))
>> +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) -
>> VIB_MIN_LEVEL_mV(vib))
>
> If the ranges are supposed to be inclusive, this is off-by-one. But looking
> at the driver, it seems like MIN_LEVEL may be "off"? I'm not sure though.
>
> Either way, this would be a separate fix.
> [...]
The voltage range [1504, 3544] for the new SPMI vibrator is inclusive. I
checked the voltage range [1200 3100] for PM8916 SPMI vibrator is also
inclusive. I couldn't find any document to confirm if the SSBI vibrator
but I assume it is the same as PM8916. I will make change before the
series to address it.
Thanks for reviewing the changes!
Fenglin
^ permalink raw reply
* Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2024-04-11 6:58 UTC (permalink / raw)
To: Konrad Dybcio, kernel, Andy Gross, Bjorn Andersson,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree
In-Reply-To: <3f8c970c-6a0d-4fc3-a2d3-e0797e7055cf@linaro.org>
Hi Konrad,
On 4/11/2024 2:10 AM, Konrad Dybcio wrote:
>
>
>> + if (regs->drv2_mask) {
>> + if (on)
>> + val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
>> + else
>> + val = 0;
>> + rc = regmap_write(vib->regmap, vib->drv2_addr, val);
>
> Are you purposefuly zeroing out the other bits?
>
> If yes, consider regmap_write_bits here
> If not, consider regmap_update_bits here
>
>> + if (rc < 0)
>> + return rc;
>
> Ignore regmap_r/w errors, these mean a complete failure of the API and
> we don't generally assume MMIO accesses can fail
>
> Unless SPMI is known to have issues here
>
Sorry, forgot to reply on this comment. Yes, SPMI transaction would fail
(even with very low odds) on some boards if the layout of SPMI lines is
not good enough. I'd like to keep the consistence since the whole driver
also checks the regmap_r/w errors.
^ permalink raw reply
* [PATCH] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
From: Benjamin Tissoires @ 2024-04-11 7:05 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Reported by linux-next:
After merging the hid tree, today's linux-next build (x86_64 allmodconfig)
failed like this:
x86_64-linux-gnu-ld: vmlinux.o: in function `hid_bpf_input_report':
(.text+0x1c75181): undefined reference to `hid_input_report'
Caused by commit 9be50ac30a83 ("HID: bpf: allow to inject HID event
from BPF")
I just forgot to put the indirection in place.
Link: https://lore.kernel.org/linux-kernel/20240411105131.7830f966@canb.auug.org.au/
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Seems like an overlook in my patch.
---
drivers/hid/bpf/hid_bpf_dispatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index 79ece3d1b9e2..10289f44d0cc 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -534,7 +534,7 @@ hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *buf
hdev = (struct hid_device *)ctx->hid; /* discard const */
- return hid_input_report(hdev, type, buf, size, 0);
+ return hid_bpf_ops->hid_input_report(hdev, type, buf, size, 0);
}
__bpf_kfunc_end_defs();
---
base-commit: 685dadafbde29dc3d6b7a13be284d684b06d4d4f
change-id: 20240411-fix-hid-bpf-fb67411ac650
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply related
* Re: [PATCH 02/18] HID: bpf: add first in-tree HID-BPF fix for the XPPen Artist 24
From: Benjamin Tissoires @ 2024-04-11 7:09 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kernel, linux-kselftest
In-Reply-To: <20240410-bpf_sources-v1-2-a8bf16033ef8@kernel.org>
On Apr 10 2024, Benjamin Tissoires wrote:
> This commit adds a fix for XPPen Artist 24 where the second button on
> the pen is used as an eraser.
>
> It's a "feature" from Microsoft, but it turns out that it's actually
> painful for artists. So we ship here a HID-BPF program that turns this
> second button into an actual button.
>
> Note that the HID-BPF program is not directly loaded by the kernel itself
> but by udev-hid-bpf[0]. But having the sources here allows us to also
> integrate tests into tools/testing/selftests/hid to ensure the HID-BPF
> program are actually tested.
>
> [0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> drivers/hid/bpf/progs/Makefile | 91 +++++++++++
> drivers/hid/bpf/progs/README | 102 +++++++++++++
> drivers/hid/bpf/progs/XPPen__Artist24.bpf.c | 229 ++++++++++++++++++++++++++++
> drivers/hid/bpf/progs/hid_bpf.h | 15 ++
> drivers/hid/bpf/progs/hid_bpf_helpers.h | 170 +++++++++++++++++++++
> 5 files changed, 607 insertions(+)
>
> diff --git a/drivers/hid/bpf/progs/Makefile b/drivers/hid/bpf/progs/Makefile
> new file mode 100644
> index 000000000000..63ed7e02adf1
> --- /dev/null
> +++ b/drivers/hid/bpf/progs/Makefile
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: GPL-2.0
> +OUTPUT := .output
> +abs_out := $(abspath $(OUTPUT))
> +
> +CLANG ?= clang
> +LLC ?= llc
> +LLVM_STRIP ?= llvm-strip
> +
> +TOOLS_PATH := $(abspath ../../../../tools)
> +BPFTOOL_SRC := $(TOOLS_PATH)/bpf/bpftool
> +BPFTOOL_OUTPUT := $(abs_out)/bpftool
> +DEFAULT_BPFTOOL := $(BPFTOOL_OUTPUT)/bootstrap/bpftool
> +BPFTOOL ?= $(DEFAULT_BPFTOOL)
> +
> +LIBBPF_SRC := $(TOOLS_PATH)/lib/bpf
> +LIBBPF_OUTPUT := $(abs_out)/libbpf
> +LIBBPF_DESTDIR := $(LIBBPF_OUTPUT)
> +LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include
> +BPFOBJ := $(LIBBPF_OUTPUT)/libbpf.a
> +
> +INCLUDES := -I$(OUTPUT) -I$(LIBBPF_INCLUDE) -I$(TOOLS_PATH)/include/uapi
> +CFLAGS := -g -Wall
> +
> +VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux) \
> + $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux) \
> + ../../../../vmlinux \
> + /sys/kernel/btf/vmlinux \
> + /boot/vmlinux-$(shell uname -r)
> +VMLINUX_BTF ?= $(abspath $(firstword $(wildcard $(VMLINUX_BTF_PATHS))))
> +ifeq ($(VMLINUX_BTF),)
> +$(error Cannot find a vmlinux for VMLINUX_BTF at any of "$(VMLINUX_BTF_PATHS)")
> +endif
> +
> +ifeq ($(V),1)
> +Q =
> +msg =
> +else
> +Q = @
> +msg = @printf ' %-8s %s%s\n' "$(1)" "$(notdir $(2))" "$(if $(3), $(3))";
> +MAKEFLAGS += --no-print-directory
> +submake_extras := feature_display=0
> +endif
> +
> +.DELETE_ON_ERROR:
> +
> +.PHONY: all clean
> +
> +SOURCES = $(wildcard *.bpf.c)
> +TARGETS = $(SOURCES:.bpf.c=.bpf.o)
> +
> +all: $(TARGETS)
> +
> +clean:
> + $(call msg,CLEAN)
> + $(Q)rm -rf $(OUTPUT) $(TARGETS)
> +
> +%.bpf.o: %.bpf.c vmlinux.h $(BPFOBJ) | $(OUTPUT)
> + $(call msg,BPF,$@)
> + $(Q)$(CLANG) -g -O2 --target=bpf $(INCLUDES) \
> + -c $(filter %.c,$^) -o $@ && \
> + $(LLVM_STRIP) -g $@
> +
> +vmlinux.h: $(VMLINUX_BTF) $(BPFTOOL) | $(INCLUDE_DIR)
> +ifeq ($(VMLINUX_H),)
> + $(call msg,GEN,,$@)
> + $(Q)$(BPFTOOL) btf dump file $(VMLINUX_BTF) format c > $@
> +else
> + $(call msg,CP,,$@)
> + $(Q)cp "$(VMLINUX_H)" $@
> +endif
> +
> +$(OUTPUT) $(LIBBPF_OUTPUT) $(BPFTOOL_OUTPUT):
> + $(call msg,MKDIR,$@)
> + $(Q)mkdir -p $@
> +
> +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUTPUT)
> + $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) \
> + OUTPUT=$(abspath $(dir $@))/ prefix= \
> + DESTDIR=$(LIBBPF_DESTDIR) $(abspath $@) install_headers
> +
> +ifeq ($(CROSS_COMPILE),)
> +$(DEFAULT_BPFTOOL): $(BPFOBJ) | $(BPFTOOL_OUTPUT)
> + $(Q)$(MAKE) $(submake_extras) -C $(BPFTOOL_SRC) \
> + OUTPUT=$(BPFTOOL_OUTPUT)/ \
> + LIBBPF_BOOTSTRAP_OUTPUT=$(LIBBPF_OUTPUT)/ \
> + LIBBPF_BOOTSTRAP_DESTDIR=$(LIBBPF_DESTDIR)/ bootstrap
> +else
> +$(DEFAULT_BPFTOOL): | $(BPFTOOL_OUTPUT)
> + $(Q)$(MAKE) $(submake_extras) -C $(BPFTOOL_SRC) \
> + OUTPUT=$(BPFTOOL_OUTPUT)/ bootstrap
> +endif
> diff --git a/drivers/hid/bpf/progs/README b/drivers/hid/bpf/progs/README
> new file mode 100644
> index 000000000000..20b0928f385b
> --- /dev/null
> +++ b/drivers/hid/bpf/progs/README
> @@ -0,0 +1,102 @@
> +# HID-BPF programs
> +
> +This directory contains various fixes for devices. They add new features or
> +fix some behaviors without being entirely mandatory. It is better to load them
> +when you have such a device, but they should not be a requirement for a device
> +to be working during the boot stage.
> +
> +The .bpf.c files provided here are not automatically compiled in the kernel.
> +They should be loaded in the kernel by `udev-hid-bpf`:
> +
> +https://gitlab.freedesktop.org/libevdev/udev-hid-bpf
> +
> +The main reasons for these fixes to be here is to have a central place to
> +"upstream" them, but also this way we can test them thanks to the HID
> +selftests.
> +
> +Once a .bpf.c file is accepted here, it is duplicated in `udev-hid-bpf`
> +in the `src/bpf/stable` directory, and distributions are encouraged to
> +only ship those bpf objects. So adding a file here should eventually
> +land in distributions when they update `udev-hid-bpf`
> +
> +## Compilation
> +
> +Just run `make`
> +
> +## Installation
> +
> +### Automated way
> +
> +Just run `sudo udev-hid-bpf install ./my-awesome-fix.bpf.o`
> +
> +### Manual way
> +
> +- copy the `.bpf.o` you want in `/etc/udev-hid-bpf/`
> +- create a new udev rule to automatically load it
> +
> +The following should do the trick (assuming udev-hid-bpf is available in
> +/usr/bin):
> +
> +```
> +$> cp xppen-ArtistPro16Gen2.bpf.o /etc/udev-hid-bpf/
> +$> udev-hid-bpf inspect xppen-ArtistPro16Gen2.bpf.o
> +[
> + {
> + "name": "xppen-ArtistPro16Gen2.bpf.o",
> + "devices": [
> + {
> + "bus": "0x0003",
> + "group": "0x0001",
> + "vid": "0x28BD",
> + "pid": "0x095A"
> + },
> + {
> + "bus": "0x0003",
> + "group": "0x0001",
> + "vid": "0x28BD",
> + "pid": "0x095B"
> + }
> + ],
> +...
> +$> cat <EOF > /etc/udev/rules.d/99-load-hid-bpf-xppen-ArtistPro16Gen2.rules
> +ACTION!="add|remove", GOTO="hid_bpf_end"
> +SUBSYSTEM!="hid", GOTO="hid_bpf_end"
> +
> +# xppen-ArtistPro16Gen2.bpf.o
> +ACTION=="add",ENV{MODALIAS}=="hid:b0003g0001v000028BDp0000095A", RUN{program}+="/usr/local/bin/udev-hid-bpf add $sys$devpath /etc/udev-hid-bpf/xppen-ArtistPro16Gen2.bpf.o"
> +ACTION=="remove",ENV{MODALIAS}=="hid:b0003g0001v000028BDp0000095A", RUN{program}+="/usr/local/bin/udev-hid-bpf remove $sys$devpath "
> +# xppen-ArtistPro16Gen2.bpf.o
> +ACTION=="add",ENV{MODALIAS}=="hid:b0003g0001v000028BDp0000095B", RUN{program}+="/usr/local/bin/udev-hid-bpf add $sys$devpath /etc/udev-hid-bpf/xppen-ArtistPro16Gen2.bpf.o"
> +ACTION=="remove",ENV{MODALIAS}=="hid:b0003g0001v000028BDp0000095B", RUN{program}+="/usr/local/bin/udev-hid-bpf remove $sys$devpath "
> +
> +LABEL="hid_bpf_end"
> +EOF
> +$> udevadm control --reload
> +```
> +
> +Then unplug and replug the device.
> +
> +## Checks
> +
> +### udev rule
> +
> +You can check that the udev rule is correctly working by issuing
> +
> +```
> +$> udevadm test /sys/bus/hid/devices/0003:28BD:095B*
> +...
> +run: '/usr/local/bin/udev-hid-bpf add /sys/devices/virtual/misc/uhid/0003:28BD:095B.0E57 /etc/udev-hid-bpf/xppen-ArtistPro16Gen2.bpf.o'
> +```
> +
> +### program loaded
> +
> +You can check that the program has been properly loaded with `bpftool`
> +
> +```
> +$> bpftool prog
> +...
> +247: tracing name xppen_16_fix_eraser tag 18d389353ed2ef07 gpl
> + loaded_at 2024-03-28T16:02:28+0100 uid 0
> + xlated 120B jited 77B memlock 4096B
> + btf_id 487
> +```
> diff --git a/drivers/hid/bpf/progs/XPPen__Artist24.bpf.c b/drivers/hid/bpf/progs/XPPen__Artist24.bpf.c
> new file mode 100644
> index 000000000000..e1be6a12bb75
> --- /dev/null
> +++ b/drivers/hid/bpf/progs/XPPen__Artist24.bpf.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2023 Benjamin Tissoires
> + */
> +
> +#include "vmlinux.h"
> +#include "hid_bpf.h"
> +#include "hid_bpf_helpers.h"
> +#include <bpf/bpf_tracing.h>
> +
> +#define VID_UGEE 0x28BD /* VID is shared with SinoWealth and Glorious and prob others */
> +#define PID_ARTIST_24 0x093A
> +#define PID_ARTIST_24_PRO 0x092D
> +
> +HID_BPF_CONFIG(
> + HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, VID_UGEE, PID_ARTIST_24),
> + HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, VID_UGEE, PID_ARTIST_24_PRO)
> +);
> +
> +/*
> + * We need to amend the report descriptor for the following:
> + * - the device reports Eraser instead of using Secondary Barrel Switch
> + * - the pen doesn't have a rubber tail, so basically we are removing any
> + * eraser/invert bits
> + */
> +static const __u8 fixed_rdesc[] = {
> + 0x05, 0x0d, // Usage Page (Digitizers) 0
> + 0x09, 0x02, // Usage (Pen) 2
> + 0xa1, 0x01, // Collection (Application) 4
> + 0x85, 0x07, // Report ID (7) 6
> + 0x09, 0x20, // Usage (Stylus) 8
> + 0xa1, 0x00, // Collection (Physical) 10
> + 0x09, 0x42, // Usage (Tip Switch) 12
> + 0x09, 0x44, // Usage (Barrel Switch) 14
> + 0x09, 0x5a, // Usage (Secondary Barrel Switch) 16 /* changed from 0x45 (Eraser) to 0x5a (Secondary Barrel Switch) */
> + 0x15, 0x00, // Logical Minimum (0) 18
> + 0x25, 0x01, // Logical Maximum (1) 20
> + 0x75, 0x01, // Report Size (1) 22
> + 0x95, 0x03, // Report Count (3) 24
> + 0x81, 0x02, // Input (Data,Var,Abs) 26
> + 0x95, 0x02, // Report Count (2) 28
> + 0x81, 0x03, // Input (Cnst,Var,Abs) 30
> + 0x09, 0x32, // Usage (In Range) 32
> + 0x95, 0x01, // Report Count (1) 34
> + 0x81, 0x02, // Input (Data,Var,Abs) 36
> + 0x95, 0x02, // Report Count (2) 38
> + 0x81, 0x03, // Input (Cnst,Var,Abs) 40
> + 0x75, 0x10, // Report Size (16) 42
> + 0x95, 0x01, // Report Count (1) 44
> + 0x35, 0x00, // Physical Minimum (0) 46
> + 0xa4, // Push 48
> + 0x05, 0x01, // Usage Page (Generic Desktop) 49
> + 0x09, 0x30, // Usage (X) 51
> + 0x65, 0x13, // Unit (EnglishLinear: in) 53
> + 0x55, 0x0d, // Unit Exponent (-3) 55
> + 0x46, 0xf0, 0x50, // Physical Maximum (20720) 57
> + 0x26, 0xff, 0x7f, // Logical Maximum (32767) 60
> + 0x81, 0x02, // Input (Data,Var,Abs) 63
> + 0x09, 0x31, // Usage (Y) 65
> + 0x46, 0x91, 0x2d, // Physical Maximum (11665) 67
> + 0x26, 0xff, 0x7f, // Logical Maximum (32767) 70
> + 0x81, 0x02, // Input (Data,Var,Abs) 73
> + 0xb4, // Pop 75
> + 0x09, 0x30, // Usage (Tip Pressure) 76
> + 0x45, 0x00, // Physical Maximum (0) 78
> + 0x26, 0xff, 0x1f, // Logical Maximum (8191) 80
> + 0x81, 0x42, // Input (Data,Var,Abs,Null) 83
> + 0x09, 0x3d, // Usage (X Tilt) 85
> + 0x15, 0x81, // Logical Minimum (-127) 87
> + 0x25, 0x7f, // Logical Maximum (127) 89
> + 0x75, 0x08, // Report Size (8) 91
> + 0x95, 0x01, // Report Count (1) 93
> + 0x81, 0x02, // Input (Data,Var,Abs) 95
> + 0x09, 0x3e, // Usage (Y Tilt) 97
> + 0x15, 0x81, // Logical Minimum (-127) 99
> + 0x25, 0x7f, // Logical Maximum (127) 101
> + 0x81, 0x02, // Input (Data,Var,Abs) 103
> + 0xc0, // End Collection 105
> + 0xc0, // End Collection 106
> +};
> +
> +#define BIT(n) (1UL << n)
> +
> +#define TIP_SWITCH BIT(0)
> +#define BARREL_SWITCH BIT(1)
> +#define ERASER BIT(2)
> +/* padding BIT(3) */
> +/* padding BIT(4) */
> +#define IN_RANGE BIT(5)
> +/* padding BIT(6) */
> +/* padding BIT(7) */
> +
> +#define U16(index) (data[index] | (data[index + 1] << 8))
> +
> +SEC("fmod_ret/hid_bpf_rdesc_fixup")
> +int BPF_PROG(hid_fix_rdesc_xppen_artist24, struct hid_bpf_ctx *hctx)
> +{
> + __u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 4096 /* size */);
> +
> + if (!data)
> + return 0; /* EPERM check */
> +
> + __builtin_memcpy(data, fixed_rdesc, sizeof(fixed_rdesc));
> +
> + return sizeof(fixed_rdesc);
> +}
> +
> +static __u8 prev_state = 0;
> +
> +/*
> + * There are a few cases where the device is sending wrong event
> + * sequences, all related to the second button (the pen doesn't
> + * have an eraser switch on the tail end):
> + *
> + * whenever the second button gets pressed or released, an
> + * out-of-proximity event is generated and then the firmware
> + * compensate for the missing state (and the firmware uses
> + * eraser for that button):
> + *
> + * - if the pen is in range, an extra out-of-range is sent
> + * when the second button is pressed/released:
> + * // Pen is in range
> + * E: InRange
> + *
> + * // Second button is pressed
> + * E:
> + * E: Eraser InRange
> + *
> + * // Second button is released
> + * E:
> + * E: InRange
> + *
> + * This case is ignored by this filter, it's "valid"
> + * and userspace knows how to deal with it, there are just
> + * a few out-of-prox events generated, but the user doesn´t
> + * see them.
> + *
> + * - if the pen is in contact, 2 extra events are added when
> + * the second button is pressed/released: an out of range
> + * and an in range:
> + *
> + * // Pen is in contact
> + * E: TipSwitch InRange
> + *
> + * // Second button is pressed
> + * E: <- false release, needs to be filtered out
> + * E: Eraser InRange <- false release, needs to be filtered out
> + * E: TipSwitch Eraser InRange
> + *
> + * // Second button is released
> + * E: <- false release, needs to be filtered out
> + * E: InRange <- false release, needs to be filtered out
> + * E: TipSwitch InRange
> + *
> + */
> +SEC("fmod_ret/hid_bpf_device_event")
> +int BPF_PROG(xppen_24_fix_eraser, struct hid_bpf_ctx *hctx)
> +{
> + __u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 10 /* size */);
> + __u8 current_state, changed_state;
> + bool prev_tip;
> + __u16 tilt;
> +
> + if (!data)
> + return 0; /* EPERM check */
> +
> + current_state = data[1];
> +
> + /* if the state is identical to previously, early return */
> + if (current_state == prev_state)
> + return 0;
> +
> + prev_tip = !!(prev_state & TIP_SWITCH);
> +
> + /*
> + * Illegal transition: pen is in range with the tip pressed, and
> + * it goes into out of proximity.
> + *
> + * Ideally we should hold the event, start a timer and deliver it
> + * only if the timer ends, but we are not capable of that now.
> + *
> + * And it doesn't matter because when we are in such cases, this
> + * means we are detecting a false release.
> + */
> + if ((current_state & IN_RANGE) == 0) {
> + if (prev_tip)
> + return HID_IGNORE_EVENT;
> + return 0;
> + }
> +
> + /*
> + * XOR to only set the bits that have changed between
> + * previous and current state
> + */
> + changed_state = prev_state ^ current_state;
> +
> + /* Store the new state for future processing */
> + prev_state = current_state;
> +
> + /*
> + * We get both a tipswitch and eraser change in the same HID report:
> + * this is not an authorized transition and is unlikely to happen
> + * in real life.
> + * This is likely to be added by the firmware to emulate the
> + * eraser mode so we can skip the event.
> + */
> + if ((changed_state & (TIP_SWITCH | ERASER)) == (TIP_SWITCH | ERASER)) /* we get both a tipswitch and eraser change at the same time */
> + return HID_IGNORE_EVENT;
> +
> + return 0;
> +}
> +
> +SEC("syscall")
> +int probe(struct hid_bpf_probe_args *ctx)
> +{
> + /*
> + * The device exports 3 interfaces.
> + */
> + ctx->retval = ctx->rdesc_size != 107;
> + if (ctx->retval)
> + ctx->retval = -EINVAL;
> +
> + /* ensure the kernel isn't fixed already */
> + if (ctx->rdesc[17] != 0x45) /* Eraser */
> + ctx->retval = -EINVAL;
> +
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/drivers/hid/bpf/progs/hid_bpf.h b/drivers/hid/bpf/progs/hid_bpf.h
> new file mode 100644
> index 000000000000..7ee371cac2e1
> --- /dev/null
> +++ b/drivers/hid/bpf/progs/hid_bpf.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2022 Benjamin Tissoires
> + */
> +
> +#ifndef ____HID_BPF__H
> +#define ____HID_BPF__H
> +
> +struct hid_bpf_probe_args {
> + unsigned int hid;
> + unsigned int rdesc_size;
> + unsigned char rdesc[4096];
> + int retval;
> +};
> +
> +#endif /* ____HID_BPF__H */
> diff --git a/drivers/hid/bpf/progs/hid_bpf_helpers.h b/drivers/hid/bpf/progs/hid_bpf_helpers.h
> new file mode 100644
> index 000000000000..1d53b10aaa2e
> --- /dev/null
> +++ b/drivers/hid/bpf/progs/hid_bpf_helpers.h
> @@ -0,0 +1,170 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2022 Benjamin Tissoires
> + */
> +
> +#ifndef __HID_BPF_HELPERS_H
> +#define __HID_BPF_HELPERS_H
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <linux/errno.h>
> +
> +extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> + unsigned int offset,
> + const size_t __sz) __ksym;
> +extern struct hid_bpf_ctx *hid_bpf_allocate_context(unsigned int hid_id) __ksym;
> +extern void hid_bpf_release_context(struct hid_bpf_ctx *ctx) __ksym;
> +extern int hid_bpf_hw_request(struct hid_bpf_ctx *ctx,
> + __u8 *data,
> + size_t buf__sz,
> + enum hid_report_type type,
> + enum hid_class_request reqtype) __ksym;
> +
> +#define HID_MAX_DESCRIPTOR_SIZE 4096
> +#define HID_IGNORE_EVENT -1
> +
> +/* extracted from <linux/input.h> */
> +#define BUS_ANY 0x00
> +#define BUS_PCI 0x01
> +#define BUS_ISAPNP 0x02
> +#define BUS_USB 0x03
> +#define BUS_HIL 0x04
> +#define BUS_BLUETOOTH 0x05
> +#define BUS_VIRTUAL 0x06
> +#define BUS_ISA 0x10
> +#define BUS_I8042 0x11
> +#define BUS_XTKBD 0x12
> +#define BUS_RS232 0x13
> +#define BUS_GAMEPORT 0x14
> +#define BUS_PARPORT 0x15
> +#define BUS_AMIGA 0x16
> +#define BUS_ADB 0x17
> +#define BUS_I2C 0x18
> +#define BUS_HOST 0x19
> +#define BUS_GSC 0x1A
> +#define BUS_ATARI 0x1B
> +#define BUS_SPI 0x1C
> +#define BUS_RMI 0x1D
> +#define BUS_CEC 0x1E
> +#define BUS_INTEL_ISHTP 0x1F
> +#define BUS_AMD_SFH 0x20
> +
> +/* extracted from <linux/hid.h> */
> +#define HID_GROUP_ANY 0x0000
> +#define HID_GROUP_GENERIC 0x0001
> +#define HID_GROUP_MULTITOUCH 0x0002
> +#define HID_GROUP_SENSOR_HUB 0x0003
> +#define HID_GROUP_MULTITOUCH_WIN_8 0x0004
> +#define HID_GROUP_RMI 0x0100
> +#define HID_GROUP_WACOM 0x0101
> +#define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> +#define HID_GROUP_STEAM 0x0103
> +#define HID_GROUP_LOGITECH_27MHZ_DEVICE 0x0104
> +#define HID_GROUP_VIVALDI 0x0105
> +
> +/* include/linux/mod_devicetable.h defines as (~0), but that gives us negative size arrays */
> +#define HID_VID_ANY 0x0000
> +#define HID_PID_ANY 0x0000
> +
> +/* duplicated from incluse/linux/array_size.h
> + */
FWIW, Peter mentioned on the matching MR on udev-hid-bpf that this
comment was likely superflous:
https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/66#note_2365932
Cheers,
Benjamin
> +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
> +/* Helper macro to convert (foo, __LINE__) into foo134 so we can use __LINE__ for
> + * field/variable names
> + */
> +#define COMBINE1(X, Y) X ## Y
> +#define COMBINE(X, Y) COMBINE1(X, Y)
> +
> +/* Macro magic:
> + * __uint(foo, 123) creates a int (*foo)[1234]
> + *
> + * We use that macro to declare an anonymous struct with several
> + * fields, each is the declaration of an pointer to an array of size
> + * bus/group/vid/pid. (Because it's a pointer to such an array, actual storage
> + * would be sizeof(pointer) rather than sizeof(array). Not that we ever
> + * instantiate it anyway).
> + *
> + * This is only used for BTF introspection, we can later check "what size
> + * is the bus array" in the introspection data and thus extract the bus ID
> + * again.
> + *
> + * And we use the __LINE__ to give each of our structs a unique name so the
> + * BPF program writer doesn't have to.
> + *
> + * $ bpftool btf dump file target/bpf/HP_Elite_Presenter.bpf.o
> + * shows the inspection data, start by searching for .hid_bpf_config
> + * and working backwards from that (each entry references the type_id of the
> + * content).
> + */
> +
> +#define HID_DEVICE(b, g, ven, prod) \
> + struct { \
> + __uint(name, 0); \
> + __uint(bus, (b)); \
> + __uint(group, (g)); \
> + __uint(vid, (ven)); \
> + __uint(pid, (prod)); \
> + } COMBINE(_entry, __LINE__)
> +
> +/* Macro magic below is to make HID_BPF_CONFIG() look like a function call that
> + * we can pass multiple HID_DEVICE() invocations in.
> + *
> + * For up to 16 arguments, HID_BPF_CONFIG(one, two) resolves to
> + *
> + * union {
> + * HID_DEVICE(...);
> + * HID_DEVICE(...);
> + * } _device_ids SEC(".hid_bpf_config")
> + *
> + */
> +
> +/* Returns the number of macro arguments, this expands
> + * NARGS(a, b, c) to NTH_ARG(a, b, c, 15, 14, 13, .... 4, 3, 2, 1).
> + * NTH_ARG always returns the 16th argument which in our case is 3.
> + *
> + * If we want more than 16 values _COUNTDOWN and _NTH_ARG both need to be
> + * updated.
> + */
> +#define _NARGS(...) _NARGS1(__VA_ARGS__, _COUNTDOWN)
> +#define _NARGS1(...) _NTH_ARG(__VA_ARGS__)
> +
> +/* Add to this if we need more than 16 args */
> +#define _COUNTDOWN \
> + 15, 14, 13, 12, 11, 10, 9, 8, \
> + 7, 6, 5, 4, 3, 2, 1, 0
> +
> +/* Return the 16 argument passed in. See _NARGS above for usage. Note this is
> + * 1-indexed.
> + */
> +#define _NTH_ARG( \
> + _1, _2, _3, _4, _5, _6, _7, _8, \
> + _9, _10, _11, _12, _13, _14, _15,\
> + N, ...) N
> +
> +/* Turns EXPAND(_ARG, a, b, c) into _ARG3(a, b, c) */
> +#define _EXPAND(func, ...) COMBINE(func, _NARGS(__VA_ARGS__)) (__VA_ARGS__)
> +
> +/* And now define all the ARG macros for each number of args we want to accept */
> +#define _ARG1(_1) _1;
> +#define _ARG2(_1, _2) _1; _2;
> +#define _ARG3(_1, _2, _3) _1; _2; _3;
> +#define _ARG4(_1, _2, _3, _4) _1; _2; _3; _4;
> +#define _ARG5(_1, _2, _3, _4, _5) _1; _2; _3; _4; _5;
> +#define _ARG6(_1, _2, _3, _4, _5, _6) _1; _2; _3; _4; _5; _6;
> +#define _ARG7(_1, _2, _3, _4, _5, _6, _7) _1; _2; _3; _4; _5; _6; _7;
> +#define _ARG8(_1, _2, _3, _4, _5, _6, _7, _8) _1; _2; _3; _4; _5; _6; _7; _8;
> +#define _ARG9(_1, _2, _3, _4, _5, _6, _7, _8, _9) _1; _2; _3; _4; _5; _6; _7; _8; _9;
> +#define _ARG10(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a;
> +#define _ARG11(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a; _b;
> +#define _ARG12(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a; _b; _c;
> +#define _ARG13(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, _d) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a; _b; _c; _d;
> +#define _ARG14(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, _d, _e) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a; _b; _c; _d; _e;
> +#define _ARG15(_1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, _d, _e, _f) _1; _2; _3; _4; _5; _6; _7; _8; _9; _a; _b; _c; _d; _e; _f;
> +
> +
> +#define HID_BPF_CONFIG(...) union { \
> + _EXPAND(_ARG, __VA_ARGS__) \
> +} _device_ids SEC(".hid_bpf_config")
> +
> +#endif /* __HID_BPF_HELPERS_H */
>
> --
> 2.44.0
>
^ permalink raw reply
* [PATCH v9 1/4] input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
From: Fenglin Wu via B4 Relay @ 2024-04-11 8:30 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
The output voltage is inclusive hence the max level calculation is
off-by-one-step. Correct it.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..89f0f1c810d8 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -14,7 +14,8 @@
#define VIB_MAX_LEVEL_mV (3100)
#define VIB_MIN_LEVEL_mV (1200)
-#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
+#define VIB_PER_STEP_mV (100)
+#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
#define MAX_FF_SPEED 0xff
@@ -118,10 +119,10 @@ static void pm8xxx_work_handler(struct work_struct *work)
vib->active = true;
vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
VIB_MIN_LEVEL_mV;
- vib->level /= 100;
+ vib->level /= VIB_PER_STEP_mV;
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / 100;
+ vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
}
pm8xxx_vib_set(vib, vib->active);
--
2.25.1
^ permalink raw reply related
* [PATCH v9 0/4] Add support for vibrator in multiple PMICs
From: Fenglin Wu via B4 Relay @ 2024-04-11 8:30 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu,
Krzysztof Kozlowski
Add SW support for the vibrator module inside PMI632, PM7250B, PM7325B, PM7550BA.
It is very similar to the vibrator module inside PM8916 which is supported in
pm8xxx-vib driver but just the drive amplitude is controlled with 2 registers,
and the register base offset in each PMIC is different.
Changes in v9:
1. Add a preceding change to correct VIB_MAX_LEVELS calculation
2. Address review comments from Konrad
Link to v8: https://lore.kernel.org/r/20240401-pm8xxx-vibrator-new-design-v8-0-6f2b8b03b4c7@quicinc.com
Changes in v8:
1. Remove hw_type, and still keep the register info in match data
2. Update to use register offset in pm8xxx_regs, and the base address
defined in DT for SPMI vibrator will be added in register access
3. Update voltage output range for SPMI vibrator which has 2 bytes drive
registers
Changes in v7:
1. Fix a typo: SSBL_VIB_DRV_REG --> SSBI_VIB_DRV_REG
2. Move the hw_type switch case in pm8xxx_vib_set() to the refactoring
change.
Changes in v6:
1. Add "qcom,pmi632-vib" as a standalone compatible string.
Changes in v5:
1. Drop "qcom,spmi-vib-gen2" generic compatible string as requested
and use device specific compatible strings only.
Changes in v4:
1. Update to use the combination of the HW type and register offset
as the constant match data, the register base address defined in
'reg' property will be added when accessing SPMI registers using
regmap APIs.
2. Remove 'qcom,spmi-vib-gen1' generic compatible string.
Changes in v3:
1. Refactor the driver to support different type of the vibrators with
better flexibility by introducing the HW type with corresponding
register fields definitions.
2. Add 'qcom,spmi-vib-gen1' and 'qcom,spmi-vib-gen2' compatible
strings, and add PMI632, PM7250B, PM7325B, PM7550BA as compatbile as
spmi-vib-gen2.
Changes in v2:
Remove the "pm7550ba-vib" compatible string as it's compatible with pm7325b.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
Fenglin Wu (4):
input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
input: pm8xxx-vibrator: refactor to support new SPMI vibrator
dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
input: pm8xxx-vibrator: add new SPMI vibrator support
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 +++-
drivers/input/misc/pm8xxx-vibrator.c | 92 ++++++++++++++++------
2 files changed, 82 insertions(+), 26 deletions(-)
---
base-commit: 650cda2ce25f08e8fae391b3ba6be27e7296c6a5
change-id: 20240328-pm8xxx-vibrator-new-design-e5811ad59e8a
Best regards,
--
Fenglin Wu <quic_fenglinw@quicinc.com>
^ permalink raw reply
* [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu via B4 Relay @ 2024-04-11 8:31 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add support for a new SPMI vibrator module which is very similar
to the vibrator module inside PM8916 but has a finer drive voltage
step and different output voltage range, its drive level control
is expanded across 2 registers. The vibrator module can be found
in following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 51 +++++++++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 2959edca8eb9..35bb6f450fd2 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,10 +12,10 @@
#include <linux/regmap.h>
#include <linux/slab.h>
-#define VIB_MAX_LEVEL_mV (3100)
-#define VIB_MIN_LEVEL_mV (1200)
-#define VIB_PER_STEP_mV (100)
-#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
+#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? 3544 : 3100)
+#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? 1504 : 1200)
+#define VIB_PER_STEP_mV(vib) (vib->drv2_addr ? 8 : 100)
+#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib) + VIB_PER_STEP_mV(vib))
#define MAX_FF_SPEED 0xff
@@ -26,6 +26,9 @@ struct pm8xxx_regs {
unsigned int drv_offset;
unsigned int drv_mask;
unsigned int drv_shift;
+ unsigned int drv2_offset;
+ unsigned int drv2_mask;
+ unsigned int drv2_shift;
unsigned int drv_en_manual_mask;
};
@@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
.drv_en_manual_mask = 0,
};
+static struct pm8xxx_regs pmi632_regs = {
+ .enable_offset = 0x46,
+ .enable_mask = BIT(7),
+ .drv_offset = 0x40,
+ .drv_mask = GENMASK(7, 0),
+ .drv_shift = 0,
+ .drv2_offset = 0x41,
+ .drv2_mask = GENMASK(3, 0),
+ .drv2_shift = 8,
+ .drv_en_manual_mask = 0,
+};
+
/**
* struct pm8xxx_vib - structure to hold vibrator data
* @vib_input_dev: input device supporting force feedback
@@ -53,6 +68,7 @@ static struct pm8xxx_regs pm8916_regs = {
* @regs: registers' info
* @enable_addr: vibrator enable register
* @drv_addr: vibrator drive strength register
+ * @drv2_addr: vibrator drive strength upper byte register
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -65,6 +81,7 @@ struct pm8xxx_vib {
const struct pm8xxx_regs *regs;
unsigned int enable_addr;
unsigned int drv_addr;
+ unsigned int drv2_addr;
int speed;
int level;
bool active;
@@ -82,6 +99,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
unsigned int val = vib->reg_vib_drv;
const struct pm8xxx_regs *regs = vib->regs;
+ /* vibrator without drv2_addr needs be programmed in step increments */
+ if (!vib->drv2_addr)
+ vib->level /= VIB_PER_STEP_mV(vib);
+
if (on)
val |= (vib->level << regs->drv_shift) & regs->drv_mask;
else
@@ -93,6 +114,17 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
vib->reg_vib_drv = val;
+ if (regs->drv2_mask) {
+ if (on)
+ val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
+ else
+ val = 0;
+
+ rc = regmap_write_bits(vib->regmap, vib->drv2_addr, regs->drv2_mask, val);
+ if (rc < 0)
+ return rc;
+ }
+
if (regs->enable_mask)
rc = regmap_update_bits(vib->regmap, vib->enable_addr,
regs->enable_mask, on ? regs->enable_mask : 0);
@@ -115,17 +147,16 @@ static void pm8xxx_work_handler(struct work_struct *work)
return;
/*
- * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
+ * pmic vibrator supports voltage ranges from MIN_LEVEL to MAX_LEVEL, so
* scale the level to fit into these ranges.
*/
if (vib->speed) {
vib->active = true;
- vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
- VIB_MIN_LEVEL_mV;
- vib->level /= VIB_PER_STEP_mV;
+ vib->level = VIB_MIN_LEVEL_mV(vib);
+ vib->level += mult_frac(VIB_MAX_LEVELS(vib), vib->speed, MAX_FF_SPEED);
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
+ vib->level = VIB_MIN_LEVEL_mV(vib);
}
pm8xxx_vib_set(vib, vib->active);
@@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
vib->enable_addr = reg_base + regs->enable_offset;
vib->drv_addr = reg_base + regs->drv_offset;
+ vib->drv2_addr = reg_base + regs->drv2_offset;
/* operate in manual mode */
error = regmap_read(vib->regmap, vib->drv_addr, &val);
@@ -257,6 +289,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+ { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Fenglin Wu via B4 Relay @ 2024-04-11 8:30 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Currently, vibrator control register addresses are hard coded,
including the base address and offsets, it's not flexible to
support new SPMI vibrator module which is usually included in
different PMICs with different base address. Refactor it by using
the base address defined in devicetree.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 42 ++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 89f0f1c810d8..2959edca8eb9 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -20,26 +20,26 @@
#define MAX_FF_SPEED 0xff
struct pm8xxx_regs {
- unsigned int enable_addr;
+ unsigned int enable_offset;
unsigned int enable_mask;
- unsigned int drv_addr;
+ unsigned int drv_offset;
unsigned int drv_mask;
unsigned int drv_shift;
unsigned int drv_en_manual_mask;
};
static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
+ .drv_offset = 0x4A,
.drv_mask = 0xf8,
.drv_shift = 3,
.drv_en_manual_mask = 0xfc,
};
static struct pm8xxx_regs pm8916_regs = {
- .enable_addr = 0xc046,
+ .enable_offset = 0x46,
.enable_mask = BIT(7),
- .drv_addr = 0xc041,
+ .drv_offset = 0x41,
.drv_mask = 0x1F,
.drv_shift = 0,
.drv_en_manual_mask = 0,
@@ -51,6 +51,8 @@ static struct pm8xxx_regs pm8916_regs = {
* @work: work structure to set the vibration parameters
* @regmap: regmap for register read/write
* @regs: registers' info
+ * @enable_addr: vibrator enable register
+ * @drv_addr: vibrator drive strength register
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -61,6 +63,8 @@ struct pm8xxx_vib {
struct work_struct work;
struct regmap *regmap;
const struct pm8xxx_regs *regs;
+ unsigned int enable_addr;
+ unsigned int drv_addr;
int speed;
int level;
bool active;
@@ -83,15 +87,15 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
else
val &= ~regs->drv_mask;
- rc = regmap_write(vib->regmap, regs->drv_addr, val);
+ rc = regmap_write(vib->regmap, vib->drv_addr, val);
if (rc < 0)
return rc;
vib->reg_vib_drv = val;
if (regs->enable_mask)
- rc = regmap_update_bits(vib->regmap, regs->enable_addr,
- regs->enable_mask, on ? ~0 : 0);
+ rc = regmap_update_bits(vib->regmap, vib->enable_addr,
+ regs->enable_mask, on ? regs->enable_mask : 0);
return rc;
}
@@ -103,11 +107,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
static void pm8xxx_work_handler(struct work_struct *work)
{
struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
- const struct pm8xxx_regs *regs = vib->regs;
- int rc;
unsigned int val;
+ int rc;
- rc = regmap_read(vib->regmap, regs->drv_addr, &val);
+ rc = regmap_read(vib->regmap, vib->drv_addr, &val);
if (rc < 0)
return;
@@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
int error;
- unsigned int val;
+ unsigned int val, reg_base = 0;
const struct pm8xxx_regs *regs;
vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
@@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
regs = of_device_get_match_data(&pdev->dev);
+ if (regs->enable_offset != 0) {
+ error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base);
+ if (error < 0) {
+ dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
+ return error;
+ }
+ }
+
+ vib->enable_addr = reg_base + regs->enable_offset;
+ vib->drv_addr = reg_base + regs->drv_offset;
+
/* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
+ error = regmap_read(vib->regmap, vib->drv_addr, &val);
if (error < 0)
return error;
val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_write(vib->regmap, vib->drv_addr, val);
if (error < 0)
return error;
--
2.25.1
^ permalink raw reply related
* [PATCH v9 3/4] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Fenglin Wu via B4 Relay @ 2024-04-11 8:30 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu,
Krzysztof Kozlowski
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add compatible strings to support vibrator module inside PMI632,
PMI7250B, PM7325B, PM7550BA.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
index c8832cd0d7da..2025d6a5423e 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
@@ -11,10 +11,18 @@ maintainers:
properties:
compatible:
- enum:
- - qcom,pm8058-vib
- - qcom,pm8916-vib
- - qcom,pm8921-vib
+ oneOf:
+ - enum:
+ - qcom,pm8058-vib
+ - qcom,pm8916-vib
+ - qcom,pm8921-vib
+ - qcom,pmi632-vib
+ - items:
+ - enum:
+ - qcom,pm7250b-vib
+ - qcom,pm7325b-vib
+ - qcom,pm7550ba-vib
+ - const: qcom,pmi632-vib
reg:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
From: Jiri Kosina @ 2024-04-11 9:08 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20240411-fix-hid-bpf-v1-1-4ae913031a8c@kernel.org>
On Thu, 11 Apr 2024, Benjamin Tissoires wrote:
> Reported by linux-next:
> After merging the hid tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
>
> x86_64-linux-gnu-ld: vmlinux.o: in function `hid_bpf_input_report':
> (.text+0x1c75181): undefined reference to `hid_input_report'
>
> Caused by commit 9be50ac30a83 ("HID: bpf: allow to inject HID event
> from BPF")
>
> I just forgot to put the indirection in place.
>
> Link: https://lore.kernel.org/linux-kernel/20240411105131.7830f966@canb.auug.org.au/
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
Please also include Fixes: tag.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
From: Benjamin Tissoires @ 2024-04-11 9:51 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2404111107370.5680@cbobk.fhfr.pm>
On Apr 11 2024, Jiri Kosina wrote:
> On Thu, 11 Apr 2024, Benjamin Tissoires wrote:
>
> > Reported by linux-next:
> > After merging the hid tree, today's linux-next build (x86_64 allmodconfig)
> > failed like this:
> >
> > x86_64-linux-gnu-ld: vmlinux.o: in function `hid_bpf_input_report':
> > (.text+0x1c75181): undefined reference to `hid_input_report'
> >
> > Caused by commit 9be50ac30a83 ("HID: bpf: allow to inject HID event
> > from BPF")
> >
> > I just forgot to put the indirection in place.
> >
> > Link: https://lore.kernel.org/linux-kernel/20240411105131.7830f966@canb.auug.org.au/
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> Please also include Fixes: tag.
Oops, correct. I'll add the following (in case b4 is smart enough to
pick it up by itself):
Fixes: 9be50ac30a83 ("HID: bpf: allow to inject HID event from BPF")
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
From: Benjamin Tissoires @ 2024-04-11 9:57 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel
In-Reply-To: <jdwmoghmwice2arahxdksjs224pssdtcfipxxrxhm3ujv6jfzc@n2s5nyiddjnt>
On Apr 11 2024, Benjamin Tissoires wrote:
> On Apr 11 2024, Jiri Kosina wrote:
> > On Thu, 11 Apr 2024, Benjamin Tissoires wrote:
> >
> > > Reported by linux-next:
> > > After merging the hid tree, today's linux-next build (x86_64 allmodconfig)
> > > failed like this:
> > >
> > > x86_64-linux-gnu-ld: vmlinux.o: in function `hid_bpf_input_report':
> > > (.text+0x1c75181): undefined reference to `hid_input_report'
> > >
> > > Caused by commit 9be50ac30a83 ("HID: bpf: allow to inject HID event
> > > from BPF")
> > >
> > > I just forgot to put the indirection in place.
> > >
> > > Link: https://lore.kernel.org/linux-kernel/20240411105131.7830f966@canb.auug.org.au/
> > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> >
> > Please also include Fixes: tag.
>
> Oops, correct. I'll add the following (in case b4 is smart enough to
> pick it up by itself):
FTR, b4 is actually smart enough to pick this one up :)
>
> Fixes: 9be50ac30a83 ("HID: bpf: allow to inject HID event from BPF")
>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v9 1/4] input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
From: Dmitry Baryshkov @ 2024-04-11 10:56 UTC (permalink / raw)
To: quic_fenglinw
Cc: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-input, linux-kernel, devicetree
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-1-7bf56cb92b28@quicinc.com>
On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> The output voltage is inclusive hence the max level calculation is
> off-by-one-step. Correct it.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Fixes tag?
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 04cb87efd799..89f0f1c810d8 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -14,7 +14,8 @@
>
> #define VIB_MAX_LEVEL_mV (3100)
> #define VIB_MIN_LEVEL_mV (1200)
> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
> +#define VIB_PER_STEP_mV (100)
> +#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
>
> #define MAX_FF_SPEED 0xff
>
> @@ -118,10 +119,10 @@ static void pm8xxx_work_handler(struct work_struct *work)
> vib->active = true;
> vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> VIB_MIN_LEVEL_mV;
> - vib->level /= 100;
> + vib->level /= VIB_PER_STEP_mV;
> } else {
> vib->active = false;
> - vib->level = VIB_MIN_LEVEL_mV / 100;
> + vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
> }
>
> pm8xxx_vib_set(vib, vib->active);
>
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Dmitry Baryshkov @ 2024-04-11 10:58 UTC (permalink / raw)
To: quic_fenglinw
Cc: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-input, linux-kernel, devicetree
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-2-7bf56cb92b28@quicinc.com>
On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> Currently, vibrator control register addresses are hard coded,
> including the base address and offsets, it's not flexible to
> support new SPMI vibrator module which is usually included in
> different PMICs with different base address. Refactor it by using
> the base address defined in devicetree.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 42 ++++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 89f0f1c810d8..2959edca8eb9 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -20,26 +20,26 @@
> #define MAX_FF_SPEED 0xff
>
> struct pm8xxx_regs {
> - unsigned int enable_addr;
> + unsigned int enable_offset;
> unsigned int enable_mask;
>
> - unsigned int drv_addr;
> + unsigned int drv_offset;
> unsigned int drv_mask;
> unsigned int drv_shift;
> unsigned int drv_en_manual_mask;
> };
>
> static const struct pm8xxx_regs pm8058_regs = {
> - .drv_addr = 0x4A,
> + .drv_offset = 0x4A,
If the DT already has reg = <0x4a> and you add drv_offset = 0x4a,
which register will be used by the driver?
Also, while we are at it, please downcase all the hex numbers that you
are touching.
> .drv_mask = 0xf8,
> .drv_shift = 3,
> .drv_en_manual_mask = 0xfc,
> };
>
> static struct pm8xxx_regs pm8916_regs = {
> - .enable_addr = 0xc046,
> + .enable_offset = 0x46,
> .enable_mask = BIT(7),
> - .drv_addr = 0xc041,
> + .drv_offset = 0x41,
> .drv_mask = 0x1F,
> .drv_shift = 0,
> .drv_en_manual_mask = 0,
> @@ -51,6 +51,8 @@ static struct pm8xxx_regs pm8916_regs = {
> * @work: work structure to set the vibration parameters
> * @regmap: regmap for register read/write
> * @regs: registers' info
> + * @enable_addr: vibrator enable register
> + * @drv_addr: vibrator drive strength register
> * @speed: speed of vibration set from userland
> * @active: state of vibrator
> * @level: level of vibration to set in the chip
> @@ -61,6 +63,8 @@ struct pm8xxx_vib {
> struct work_struct work;
> struct regmap *regmap;
> const struct pm8xxx_regs *regs;
> + unsigned int enable_addr;
> + unsigned int drv_addr;
> int speed;
> int level;
> bool active;
> @@ -83,15 +87,15 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> else
> val &= ~regs->drv_mask;
>
> - rc = regmap_write(vib->regmap, regs->drv_addr, val);
> + rc = regmap_write(vib->regmap, vib->drv_addr, val);
> if (rc < 0)
> return rc;
>
> vib->reg_vib_drv = val;
>
> if (regs->enable_mask)
> - rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> - regs->enable_mask, on ? ~0 : 0);
> + rc = regmap_update_bits(vib->regmap, vib->enable_addr,
> + regs->enable_mask, on ? regs->enable_mask : 0);
>
> return rc;
> }
> @@ -103,11 +107,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> static void pm8xxx_work_handler(struct work_struct *work)
> {
> struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
> - const struct pm8xxx_regs *regs = vib->regs;
> - int rc;
> unsigned int val;
> + int rc;
>
> - rc = regmap_read(vib->regmap, regs->drv_addr, &val);
> + rc = regmap_read(vib->regmap, vib->drv_addr, &val);
> if (rc < 0)
> return;
>
> @@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> struct pm8xxx_vib *vib;
> struct input_dev *input_dev;
> int error;
> - unsigned int val;
> + unsigned int val, reg_base = 0;
> const struct pm8xxx_regs *regs;
>
> vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>
> regs = of_device_get_match_data(&pdev->dev);
>
> + if (regs->enable_offset != 0) {
> + error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base);
> + if (error < 0) {
> + dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
> + return error;
> + }
> + }
> +
> + vib->enable_addr = reg_base + regs->enable_offset;
> + vib->drv_addr = reg_base + regs->drv_offset;
> +
> /* operate in manual mode */
> - error = regmap_read(vib->regmap, regs->drv_addr, &val);
> + error = regmap_read(vib->regmap, vib->drv_addr, &val);
> if (error < 0)
> return error;
>
> val &= regs->drv_en_manual_mask;
> - error = regmap_write(vib->regmap, regs->drv_addr, val);
> + error = regmap_write(vib->regmap, vib->drv_addr, val);
> if (error < 0)
> return error;
>
>
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Baryshkov @ 2024-04-11 11:02 UTC (permalink / raw)
To: quic_fenglinw
Cc: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-input, linux-kernel, devicetree
In-Reply-To: <20240411-pm8xxx-vibrator-new-design-v9-4-7bf56cb92b28@quicinc.com>
On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> Add support for a new SPMI vibrator module which is very similar
> to the vibrator module inside PM8916 but has a finer drive voltage
> step and different output voltage range, its drive level control
> is expanded across 2 registers. The vibrator module can be found
> in following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 51 +++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 2959edca8eb9..35bb6f450fd2 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -12,10 +12,10 @@
> #include <linux/regmap.h>
> #include <linux/slab.h>
>
> -#define VIB_MAX_LEVEL_mV (3100)
> -#define VIB_MIN_LEVEL_mV (1200)
> -#define VIB_PER_STEP_mV (100)
> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
> +#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? 3544 : 3100)
> +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? 1504 : 1200)
> +#define VIB_PER_STEP_mV(vib) (vib->drv2_addr ? 8 : 100)
> +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib) + VIB_PER_STEP_mV(vib))
>
> #define MAX_FF_SPEED 0xff
>
> @@ -26,6 +26,9 @@ struct pm8xxx_regs {
> unsigned int drv_offset;
> unsigned int drv_mask;
> unsigned int drv_shift;
> + unsigned int drv2_offset;
> + unsigned int drv2_mask;
> + unsigned int drv2_shift;
> unsigned int drv_en_manual_mask;
> };
>
> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
> .drv_en_manual_mask = 0,
> };
>
> +static struct pm8xxx_regs pmi632_regs = {
> + .enable_offset = 0x46,
> + .enable_mask = BIT(7),
> + .drv_offset = 0x40,
> + .drv_mask = GENMASK(7, 0),
> + .drv_shift = 0,
> + .drv2_offset = 0x41,
> + .drv2_mask = GENMASK(3, 0),
> + .drv2_shift = 8,
> + .drv_en_manual_mask = 0,
> +};
> +
> /**
> * struct pm8xxx_vib - structure to hold vibrator data
> * @vib_input_dev: input device supporting force feedback
> @@ -53,6 +68,7 @@ static struct pm8xxx_regs pm8916_regs = {
> * @regs: registers' info
> * @enable_addr: vibrator enable register
> * @drv_addr: vibrator drive strength register
> + * @drv2_addr: vibrator drive strength upper byte register
> * @speed: speed of vibration set from userland
> * @active: state of vibrator
> * @level: level of vibration to set in the chip
> @@ -65,6 +81,7 @@ struct pm8xxx_vib {
> const struct pm8xxx_regs *regs;
> unsigned int enable_addr;
> unsigned int drv_addr;
> + unsigned int drv2_addr;
> int speed;
> int level;
> bool active;
> @@ -82,6 +99,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> unsigned int val = vib->reg_vib_drv;
> const struct pm8xxx_regs *regs = vib->regs;
>
> + /* vibrator without drv2_addr needs be programmed in step increments */
How are these two items related? Are you using vib->drv2_addr as a
marker for 'particular generation'? In such a case please use a flag
instead.
The rest looks good to me.
> + if (!vib->drv2_addr)
> + vib->level /= VIB_PER_STEP_mV(vib);
> +
> if (on)
> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
> else
> @@ -93,6 +114,17 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>
> vib->reg_vib_drv = val;
>
> + if (regs->drv2_mask) {
> + if (on)
> + val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
> + else
> + val = 0;
> +
> + rc = regmap_write_bits(vib->regmap, vib->drv2_addr, regs->drv2_mask, val);
> + if (rc < 0)
> + return rc;
> + }
> +
> if (regs->enable_mask)
> rc = regmap_update_bits(vib->regmap, vib->enable_addr,
> regs->enable_mask, on ? regs->enable_mask : 0);
> @@ -115,17 +147,16 @@ static void pm8xxx_work_handler(struct work_struct *work)
> return;
>
> /*
> - * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
> + * pmic vibrator supports voltage ranges from MIN_LEVEL to MAX_LEVEL, so
> * scale the level to fit into these ranges.
> */
> if (vib->speed) {
> vib->active = true;
> - vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> - VIB_MIN_LEVEL_mV;
> - vib->level /= VIB_PER_STEP_mV;
> + vib->level = VIB_MIN_LEVEL_mV(vib);
> + vib->level += mult_frac(VIB_MAX_LEVELS(vib), vib->speed, MAX_FF_SPEED);
> } else {
> vib->active = false;
> - vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
> + vib->level = VIB_MIN_LEVEL_mV(vib);
> }
>
> pm8xxx_vib_set(vib, vib->active);
> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>
> vib->enable_addr = reg_base + regs->enable_offset;
> vib->drv_addr = reg_base + regs->drv_offset;
> + vib->drv2_addr = reg_base + regs->drv2_offset;
>
> /* operate in manual mode */
> error = regmap_read(vib->regmap, vib->drv_addr, &val);
> @@ -257,6 +289,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
> { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
> + { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
>
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ 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