* Re: [PATCH] Input: pwm-beeper - Support volume setting via sysfs
From: Uwe Kleine-König @ 2023-06-14 6:45 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-input, Dmitry Torokhov, Frieder Schrempf, Manuel Traut,
Thierry Reding, linux-pwm
In-Reply-To: <20230512185551.183049-1-marex@denx.de>
[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]
On Fri, May 12, 2023 at 08:55:51PM +0200, Marek Vasut wrote:
> The PWM beeper volume can be controlled by adjusting the PWM duty cycle,
> expose volume setting via sysfs, so users can make the beeper quieter.
> This patch adds sysfs attribute 'volume' in range 0..50000, i.e. from 0
> to 50% in 1/1000th of percent steps, this resolution should be sufficient.
>
> The reason for 50000 cap on volume or PWM duty cycle is because duty cycle
> above 50% again reduces the loudness, the PWM wave form is inverted wave
> form of the one for duty cycle below 50% and the beeper gets quieter the
> closer the setting is to 100% . Hence, 50% cap where the wave form yields
> the loudest result.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> An alternative option would be to extend the userspace input ABI, e.g. by
> using SND_TONE top 16bits to encode the duty cycle in 0..50000 range, and
> bottom 16bit to encode the existing frequency in Hz . Since frequency in
> Hz is likely to be below some 25 kHz for audible bell, this fits in 16bits
> just fine. Thoughts ?
> ---
> NOTE: This uses approach similar to [1], except it is much simpler.
> [1] https://patchwork.kernel.org/project/linux-input/cover/20230201152128.614439-1-manuel.traut@mt.com/
> ---
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> Cc: Manuel Traut <manuel.traut@mt.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: linux-pwm@vger.kernel.org
> ---
> drivers/input/misc/pwm-beeper.c | 58 ++++++++++++++++++++++++++++++++-
> 1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 3cf1812384e6a..f63d0ebbaf573 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -21,6 +21,7 @@ struct pwm_beeper {
> struct regulator *amplifier;
> struct work_struct work;
> unsigned long period;
> + unsigned long duty_cycle;
> unsigned int bell_frequency;
> bool suspended;
> bool amplifier_on;
> @@ -37,7 +38,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
>
> state.enabled = true;
> state.period = period;
> - pwm_set_relative_duty_cycle(&state, 50, 100);
> + pwm_set_relative_duty_cycle(&state, beeper->duty_cycle, 100000);
>
> error = pwm_apply_state(beeper->pwm, &state);
> if (error)
> @@ -119,6 +120,53 @@ static void pwm_beeper_close(struct input_dev *input)
> pwm_beeper_stop(beeper);
> }
>
> +static ssize_t volume_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pwm_beeper *beeper = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%ld\n", beeper->duty_cycle);
> +}
> +
> +static ssize_t volume_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pwm_beeper *beeper = dev_get_drvdata(dev);
> + unsigned long val;
> +
> + if (kstrtoul(buf, 0, &val) < 0)
> + return -EINVAL;
> +
> + /*
> + * Volume is really PWM duty cycle in pcm (per cent mille, 1/1000th
> + * of percent). This value therefore ranges from 0 to 50000 . Duty
> + * cycle of 50% = 50000pcm is the maximum volume .
> + */
> + val = clamp(val, 0UL, 50000UL);
I wonder if you want to refuse values here that are not in the specified
range, that is, something like:
if (val != clamp(val, 0UL, 50000UL))
return -EINVAL;
I think this is more in line who other sysfs properties work?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] HID: lenovo: Replacing snprintf with scnprintf
From: wuyonggang001 @ 2023-06-14 6:19 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-kernel, linux-input
In-Reply-To: <af4c99853e8abcbba13e406e6e432148@208suo.com>
Fix the following coccicheck warning:
drivers/hid/hid-lenovo.c:786:8-16: WARNING: use scnprintf or sprintf
Signed-off-by: Yonggang Wu <wuyonggang001@208suo.com>
---
drivers/hid/hid-lenovo.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..147fcf69d235 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -537,7 +537,7 @@ static ssize_t attr_fn_lock_show(struct device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n", data->fn_lock);
+ return scnprintf(buf, PAGE_SIZE, "%u\n", data->fn_lock);
}
static ssize_t attr_fn_lock_store(struct device *dev,
@@ -581,7 +581,7 @@ static ssize_t attr_sensitivity_show_cptkbd(struct
device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
cptkbd_data->sensitivity);
}
@@ -753,7 +753,7 @@ static ssize_t
attr_press_to_select_show_tpkbd(struct device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->press_to_select);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->press_to_select);
}
static ssize_t attr_press_to_select_store_tpkbd(struct device *dev,
@@ -783,7 +783,7 @@ static ssize_t attr_dragging_show_tpkbd(struct
device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
+ return scnprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
}
static ssize_t attr_dragging_store_tpkbd(struct device *dev,
@@ -813,7 +813,7 @@ static ssize_t
attr_release_to_select_show_tpkbd(struct device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->release_to_select);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->release_to_select);
}
static ssize_t attr_release_to_select_store_tpkbd(struct device *dev,
@@ -843,7 +843,7 @@ static ssize_t attr_select_right_show_tpkbd(struct
device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->select_right);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->select_right);
}
static ssize_t attr_select_right_store_tpkbd(struct device *dev,
@@ -873,7 +873,7 @@ static ssize_t attr_sensitivity_show_tpkbd(struct
device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->sensitivity);
}
@@ -902,7 +902,7 @@ static ssize_t attr_press_speed_show_tpkbd(struct
device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
- return snprintf(buf, PAGE_SIZE, "%u\n",
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
data_pointer->press_speed);
}
^ permalink raw reply related
* Re: [PATCH] dt-bindings: input: mediatek,pmic-keys: Fix typo in "linux,keycodes" property name
From: Conor Dooley @ 2023-06-13 21:04 UTC (permalink / raw)
To: Rob Herring
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Chen Zhong,
linux-input, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
In-Reply-To: <20230613201040.2823802-1-robh@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 311 bytes --]
On Tue, Jun 13, 2023 at 02:10:40PM -0600, Rob Herring wrote:
> "linux-keycodes" is the wrong property name and is unused. It should be
> "linux,keycodes" instead.
Nothing in-tree at least, looks like they use the correct property
already.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH V2] Input: synaptics-rmi4- Add a new feature for Forepad.
From: Andi Shyti @ 2023-06-13 20:48 UTC (permalink / raw)
To: Marge Yang
Cc: dmitry.torokhov, linux-input, linux-kernel, david.chiu,
derek.cheng, sam.tsai, vincent.huang
In-Reply-To: <20230612022607.315149-1-marge.yang@tw.synaptics.com>
Hi Marge,
[...]
> +static irqreturn_t rmi_f21_attention(int irq, void *ctx)
> +{
> + struct rmi_function *fn = ctx;
> + struct f21_data *f21 = dev_get_drvdata(&fn->dev);
> + int error;
> +
> + error = rmi_read_block(fn->rmi_dev,
> + fn->fd.data_base_addr+RMI_F21_FORCE_CLICK_OFFSET,
> + f21->data_regs, 1);
> + if (error) {
> + dev_err(&fn->dev,
> + "%s: Failed to read f21 data registers: %d\n",
> + __func__, error);
> + return IRQ_RETVAL(error);
> + }
> +
> + if (!!(f21->data_regs[0] & RMI_f21_FORCE_CLICK))
no need for double negation here.
Andi
> + input_report_key(f21->input, f21->key_code, true);
> + else
> + input_report_key(f21->input, f21->key_code, false);
> + return IRQ_HANDLED;
> +}
^ permalink raw reply
* Re: [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
From: Andi Shyti @ 2023-06-13 20:40 UTC (permalink / raw)
To: Johannes Roith; +Cc: jikos, linux-kernel, linux-input
In-Reply-To: <20230611144831.41238-1-johannes@gnu-linux.rocks>
Hi Joannes,
[...]
> +/* this executes the READ_ALL cmd */
> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
> +{
> + struct mcp_read_all *read_all;
> + int len, t;
> +
> + reinit_completion(&mcp->wait_in_report);
> + mutex_lock(&mcp->lock);
> +
> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
> + if (!read_all)
> + return -ENOMEM;
where are you unlocking?
> + read_all->cmd = READ_ALL;
> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> + sizeof(struct mcp_read_all));
> +
> + if (len != sizeof(struct mcp_read_all))
> + return -EINVAL;
what about read_all? what about the lock?
> +
> + kfree(read_all);
> + mutex_unlock(&mcp->lock);
> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
> + if (!t)
> + return -ETIMEDOUT;
> +
> + /* return status, negative value if wrong response was received */
> + return mcp->status;
> +}
[...]
Andi
^ permalink raw reply
* [PATCH] dt-bindings: input: mediatek,pmic-keys: Fix typo in "linux,keycodes" property name
From: Rob Herring @ 2023-06-13 20:10 UTC (permalink / raw)
To: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Chen Zhong
Cc: linux-input, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
"linux-keycodes" is the wrong property name and is unused. It should be
"linux,keycodes" instead.
Signed-off-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml b/Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml
index 037c3ae9f1c3..e34c9e78d38d 100644
--- a/Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml
+++ b/Documentation/devicetree/bindings/input/mediatek,pmic-keys.yaml
@@ -55,7 +55,7 @@ patternProperties:
interrupt-names: true
- linux-keycodes:
+ linux,keycodes:
maxItems: 1
wakeup-source: true
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Doug Anderson @ 2023-06-13 15:56 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <boqzlmbrp5rvepmckkqht4h5auspjlbt5leam4xivy7a4bqxnj@iuxxhooxcphk>
Hi,
On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > > What I'm trying to say is: could we just make it work by passing a bunch
> > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > driver directly?
> >
> > I think I'm still confused about what you're proposing. Sorry! :( Let
> > me try rephrasing why I'm confused and perhaps we can get on the same
> > page. :-)
> >
> > First, I guess I'm confused about how you have one of these devices
> > "register" the other device.
> >
> > I can understand how one device might "register" its sub-devices in
> > the MFD case. To make it concrete, we can look at a PMIC like
> > max77686.c. The parent MFD device gets probed and then it's in charge
> > of creating all of its sub-devices. These sub-devices are intimately
> > tied to one another. They have shared data structures and can
> > coordinate power sequencing and whatnot. All good.
>
> We don't necessarily need to use MFD, but yeah, we could just register a
> device for the i2c-hid driver to probe from (using
> i2c_new_client_device?)
I think this can work for devices where the panel and touchscreen are
truly integrated where the panel driver knows enough about the related
touchscreen to fully describe and instantiate it. It doesn't work
quite as well for cases where the power and reset lines are shared
just because of what a given board designer did. To handle that, each
panel driver would need to get enough DT properties added to it so
that it could fully describe any arbitrary touchscreen, right?
Let's think about the generic panel-edp driver. This driver runs the
panel on many sc7180-trogdor laptops, including coachz, lazor, and
pompom. All three of those boards have a shared power rail for the
touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
you can see the touchscreen currently looks like this:
ap_ts: touchscreen@5d {
compatible = "goodix,gt7375p";
reg = <0x5d>;
pinctrl-names = "default";
pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
vdd-supply = <&pp3300_ts>;
};
In "sc7180-trogdor-lazor.dtsi" we have:
ap_ts: touchscreen@10 {
compatible = "hid-over-i2c";
reg = <0x10>;
pinctrl-names = "default";
pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
vdd-supply = <&pp3300_ts>;
};
In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
So I think to do what you propose, we need to add this information to
the panel-edp DT node so that it could dynamically construct the i2c
device for the touchscreen:
a) Which touchscreen is actually connected (generic hid-over-i2c,
goodix, ...). I guess this would be a "compatible" string?
b) Which i2c bus that device is hooked up to.
c) Which i2c address that device is hooked up to.
d) What the touchscreen interrupt GPIO is.
e) Possibly what the "hid-descr-addr" for the touchscreen is.
f) Any extra timing information needed to be passed to the touchscreen
driver, like "post-power-on-delay-ms"
The "pinctrl" stuff would be easy to subsume into the panel's DT node,
at least. ...and, in this case, we could skip the "vdd-supply" since
the panel and eDP are sharing power rails (which is what got us into
this situation). ...but, the above is still a lot. At this point, it
would make sense to have a sub-node under the panel to describe it,
which we could do but it starts to feel weird. We'd essentially be
describing an i2c device but not under the i2c controller it belongs
to.
I guess I'd also say that the above design also need additional code
if/when someone had a touchscreen that used a different communication
method, like SPI.
So I guess the tl;dr of all the above is that I think it could all work if:
1. We described the touchscreen in a sub-node of the panel.
2. We added a property to the panel saying what the true parent of the
touchscreen was (an I2C controller, a SPI controller, ...) and what
type of controller it was ("SPI" vs "I2C").
3. We added some generic helpers that panels could call that would
understand how to instantiate the touchscreen under the appropriate
controller.
4. From there, we added a new private / generic API between panels and
touchscreens letting them know that the panel was turning on/off.
That seems much more complex to me, though. It also seems like an
awkward way to describe it in DT.
> > In any case, is there any chance that we're in violent agreement
>
> Is it even violent? Sorry if it came across that way, it's really isn't
> on my end.
Sorry, maybe a poor choice of words on my end. I've heard that term
thrown about when two people spend a lot of time discussing something
/ trying to persuade the other person only to find out in the end that
they were both on the same side of the issue. ;-)
> > and that if you dig into my design more you might like it? Other than
> > the fact that the panel doesn't "register" the touchscreen device, it
> > kinda sounds as if what my patches are already doing is roughly what
> > you're describing. The touchscreen and panel driver are really just
> > coordinating with each other through a shared data structure (struct
> > drm_panel_follower) that has a few callback functions. Just like with
> > "hdmi-codec", the devices probe separately but find each other through
> > a phandle. The coordination between the two happens through a few
> > simple helper functions.
>
> I guess we very much agree on the end-goal, and I'd really like to get
> this addressed somehow. There's a couple of things I'm not really
> sold on with your proposal though:
>
> - It creates a ad-hoc KMS API for some problem that looks fairly
> generic. It's also redundant with the notifier mechanism without
> using it (probably for the best though).
>
> - MIPI-DSI panel probe sequence is already fairly complex and fragile
> (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> I'd rather avoid creating a new dependency in that graph.
>
> - And yeah, to some extent it's inconsistent with how we dealt with
> secondary devices in KMS so far.
Hmmmm. To a large extent, my current implementation actually has no
impact on the DRM probe sequence. The panel itself never looks for the
touchscreen code and everything DRM-related can register without a
care in the world. From reading your bullet points, I guess that's
both a strength and a weakness of my current proposal. It's really
outside the world of bridge chains and DRM components which makes it a
special snowflake that people need to understand on its own. ...but,
at the same time, the fact that it is outside all the rest of that
stuff means it doesn't add complexity to an already complex system.
I guess I'd point to the panel backlight as a preexisting design
that's not totally unlike what I'm doing. The backlight is not part of
the DRM bridge chain and doesn't fit in like other components. This
actually makes sense since the backlight doesn't take in or put out
video data and it's simply something associated with the panel. The
backlight also has a loose connection to the panel driver and a given
panel could be associated with any number of different backlight
drivers depending on the board design. I guess one difference between
the backlight and what I'm doing with "panel follower" is that we
typically don't let the panel probe until after the backlight has
probed. In the case of my "panel follower" proposal it's the opposite.
As per above, from a DRM probe point of view this actually makes my
proposal less intrusive. I guess also a difference between backlight
and "panel follower" is that I allow an arbitrary number of followers
but there's only one backlight.
One additional note: if I actually make the panel probe function start
registering the touchscreen, that actually _does_ add more complexity
to the already complex DRM probe ordering. It's yet another thing that
could fail and/or defer...
Also, I'm curious: would my proposal be more or less palatable if I
made it less generic? Instead of "panel follower", I could hardcode it
to "touchscreen" and then remove all the list management. From a DRM
point of view this would make it even more like the preexisting
"backlight" except for the ordering difference.
-Doug
^ permalink raw reply
* [PATCH RESEND] HID: input: Support devices sending Eraser without Invert
From: Illia Ostapyshyn @ 2023-06-13 15:26 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Illia Ostapyshyn, Nils Fuhler
Some digitizers (notably XP-Pen Artist 24) do not report the Invert
usage when erasing. This causes the device to be permanently stuck with
the BTN_TOOL_RUBBER tool after sending Eraser, as Invert is the only
usage that can release the tool. In this state, Touch and Inrange are
no longer reported to userspace, rendering the pen unusable.
Prior to commit 87562fcd1342 ("HID: input: remove the need for
HID_QUIRK_INVERT"), BTN_TOOL_RUBBER was never set and Eraser events were
simply translated into BTN_TOUCH without causing an inconsistent state.
Introduce HID_QUIRK_NOINVERT for such digitizers and detect them during
hidinput_configure_usage(). This quirk causes the tool to be released
as soon as Eraser is reported as not set. Set BTN_TOOL_RUBBER in
input->keybit when mapping Eraser.
Fixes: 87562fcd1342 ("HID: input: remove the need for HID_QUIRK_INVERT")
Co-developed-by: Nils Fuhler <nils@nilsfuhler.de>
Signed-off-by: Nils Fuhler <nils@nilsfuhler.de>
Signed-off-by: Illia Ostapyshyn <ostapyshyn@sra.uni-hannover.de>
---
We were wondering about the reason to keep quirk bits reserved for
backward compatibility. Is it because of dynamic quirks at module load
time? In that case, it could be reasonable to revive bit 0 for this
purpose as it was overwritten in hidinput_hid_event() anyway.
---
drivers/hid/hid-input.c | 18 ++++++++++++++++--
include/linux/hid.h | 1 +
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a1d2690a1a0d..c443f78287df 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -988,6 +988,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
return;
case 0x3c: /* Invert */
+ device->quirks &= ~HID_QUIRK_NOINVERT;
map_key_clear(BTN_TOOL_RUBBER);
break;
@@ -1013,9 +1014,13 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x45: /* ERASER */
/*
* This event is reported when eraser tip touches the surface.
- * Actual eraser (BTN_TOOL_RUBBER) is set by Invert usage when
- * tool gets in proximity.
+ * Actual eraser (BTN_TOOL_RUBBER) is set and released either
+ * by Invert if tool reports proximity or by Eraser directly.
*/
+ if (!test_bit(BTN_TOOL_RUBBER, input->keybit)) {
+ device->quirks |= HID_QUIRK_NOINVERT;
+ set_bit(BTN_TOOL_RUBBER, input->keybit);
+ }
map_key_clear(BTN_TOUCH);
break;
@@ -1579,6 +1584,15 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
else if (report->tool != BTN_TOOL_RUBBER)
/* value is off, tool is not rubber, ignore */
return;
+ else if (*quirks & HID_QUIRK_NOINVERT &&
+ !test_bit(BTN_TOUCH, input->key)) {
+ /*
+ * There is no invert to release the tool, let hid_input
+ * send BTN_TOUCH with scancode and release the tool after.
+ */
+ hid_report_release_tool(report, input, BTN_TOOL_RUBBER);
+ return;
+ }
/* let hid-input set BTN_TOUCH */
break;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 4e4c4fe36911..7cbc10073a1f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -360,6 +360,7 @@ struct hid_item {
#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP BIT(18)
#define HID_QUIRK_HAVE_SPECIAL_DRIVER BIT(19)
#define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE BIT(20)
+#define HID_QUIRK_NOINVERT BIT(21)
#define HID_QUIRK_FULLSPEED_INTERVAL BIT(28)
#define HID_QUIRK_NO_INIT_REPORTS BIT(29)
#define HID_QUIRK_NO_IGNORE BIT(30)
--
2.30.2
^ permalink raw reply related
* Re: Tree dumb questions from an occasional
From: Benjamin Tissoires @ 2023-06-13 15:28 UTC (permalink / raw)
To: Marco Morandini; +Cc: Jiri Kosina, linux-input
In-Reply-To: <cce96d0b-1642-bf52-b9e6-64e40e8ae275@polimi.it>
On Mon, Jun 12, 2023 at 9:33 PM Marco Morandini
<marco.morandini@polimi.it> wrote:
>
> > Yeah, right. This is the kind of situation where it's usually easy
> > enough to detect with hid-tools[0]. We can record the device on your
> > machine, then we can replay it locally on ours, and make several
> > attempts.
>
> I was not aware of hid-tools, I will mention it in my doc attempt!
>
> > Sure. Please write (if you want) your first draft, we can review it
> > and we can iterate from there. Do not forget to add the linux doc
> > mailing list in CC in case some people from there want to also add
> > things.
>
> Ok, will try.
\o/
>
> >> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
> >> while loading the usb_hid module, but not while loading the usb_generic
> >> one (that turned out to be the module that manages my HP pointer),
> >> even if the statically defined quirks were moved into their own file.
> >> Would it make sense to add the possibility to
> >> add quirks while loading hid_generic? Is this the right place for
> >> such code? If yes, I can try in my spare time to do this,
> >> even if I'm not sure I'll be able to get it right.
> >
> > I'm not 100% sure of what you mean, but currently dynamic quirks can
> > be added to the *usbhid* module (not usb_hid or usb_generic), which is
> > the transport layer driver for HID.
> > This module is responsible for creating a HID device, which can be
> > handled by HID drivers, hid_generic being one of them.
>
> You are right, I should have written usbhid.
> I was convinced that hid_generic
> did not get the quirks that were set at loading time by usbhid, but only those
> defined in quirks.c .
>
> What I really meant was that the quirk I ended up adding is
>
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
>
> If I got it right usbhid can add only quirks with
>
> BUS_USB (and not BUS_BLUETOOTH, like the above)
Yes, that is correct
>
> because of the code in usbhid/hid-core.c
>
> (
>
> retval = hid_quirks_init(quirks_param, BUS_USB, MAX_USBHID_BOOT_QUIRKS);
>
> is this the right line in hid_init ?
>
> )
>
> and the fact that one cannot specify the
> bus that should be used (whatever this "bus" means in the kernel, I
> still need to get it):
The bus is the transport layer that exposes the HID device.
If you plug in your mouse over USB, then the bus is BUS_USB. If the
mouse is wirelessly connected through Bluetooth, then it's
BUS_BLUETOOTH, and if your touchpad is integrated in the laptop and is
using I2C to communicate, the bus is BUS_I2C :)
Each bus has its own transport driver (usbhid, i2c_hid, hidp
(Bluetooth classic), uhid (BLE)) and they all translate the data from
the original bus into HID.
>
> MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
> " quirks=vendorID:productID:quirks"
> " where vendorID, productID, and quirks are all in"
> " 0x-prefixed hex");
>
> Long story short: if I
>
> - either boot with
>
> usbhid.quirks=0x3f0:0x464a:0x40
>
> - or, alternatively try to
>
> sun:/home/marco/READMEs/HPElitePresenterMouse # rmmod usbhid
> sun:/home/marco/READMEs/HPElitePresenterMouse # modprobe -v usbhid "quirks=0x03f0:0x464a:0x40"
> insmod /lib/modules/6.3.6-1-default/kernel/drivers/hid/usbhid/usbhid.ko.zst quirks=0x03f0:0x464a:0x40
>
> my device does not work correctly, but with the added
>
> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
>
> it does work.
That is correct, because usbhid is only responsible for USB devices
(which is the vast majority of devices), and hidp (nor uhid) doesn't
have such dynamic quirks.
>
> Hoping that I got it right and HID_QUIRK_MULTI_INPUT corresponds to
> 0x40, otherwise all what I've written make no sense, and I should go back
> and re-do my homework....
>
> At any rate: if there is a way to specify the correct quirk for a device like the one
> I stumbled upon, while waiting for the correct upstream fix to percolate down
> to the distributions, then of course my questions 2) and 3) (add the options to
> specify quirks while loading hid-generic (question 2) and by resorting to ebpf (question 3))
> do not make sense.
OK. The missing point was that you were using a Bluetooth device, and
not a USB one. And that makes a big difference, because yes, you can
not dynamically quirk devices for anything but USB.
I still stand by the fact that hid_generic is not the correct place.
These kinds of quirks are global to the device, and putting them in
hid_generic would make it the wrong place IMO.
Actually, the one place where it would make sense to have such dynamic
quirks is in the hid-core (hid.ko) module itself. It would make sense
to have a BUS:VID:PID:QUIRKS parameter.
But having such a parameter is not without constraints, because it's
not really "dynamic", and we can only set a limited number of quirks.
In your particular case, we might as well use an HID-BPF program that
tweaks the report descriptor which would force the kernel to "use" the
multi-input quirk.
Would you mind attaching the output of hid-recorder while you do some
HID events and where you show the bug?
Also, FWIW, the number of MULTI_INPUT quirk required in the kernel is
probably a sign that we are not using the best default implementation,
and I've already been pinged to change that. I couldn't find the time
to get back to this, but your device might also help me in having a
broader range of use cases so that we can ditch that quirk once and
for all.
Cheers.
Benjamin
>
> > As the name says, hid_generic is supposed to be generic, and I do not
> > want to see special cases handled there, because it would not be
> > generic anymore.
>
> Understood, thank you.
>
>
> > Furthermore, if you submit your patch to the LKML, the quirk will
> > likely end up in driver/hid/hid-quirks.c which is exactly the static
> > equivalent of the dynamic one from usbhid.
>
> Not exactly, because of the bus issue (again, assuming I got it right).
>
> > No need to apologize. You are actually proposing ideas and your help
> > to make things better for end-users, which is extremely valuable in
> > itself :)
>
> Thank you, you are very kind. I only hope I've not written too much
> nonsense here above.
>
> Marco
>
^ permalink raw reply
* Re: [PATCH 2/2] Input: cap11xx - add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-06-13 13:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: krzysztof.kozlowski+dt, devicetree, linux-input, linux-kernel,
robh+dt, u.kleine-koenig
In-Reply-To: <ZF2A/STY66sqoOLX@google.com>
Hi Dmitry,
On 5/12/23 01:57, Dmitry Torokhov wrote:
> Hi Jiri,
>
> On Sat, Apr 15, 2023 at 01:38:15AM +0200, Jiri Valek - 2N wrote:
>> @@ -474,7 +645,7 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
>> if (error)
>> return error;
>>
>> - irq = irq_of_parse_and_map(node, 0);
>> + irq = irq_of_parse_and_map(dev->of_node, 0);
>
> Do you know if this is actually needed or we can rely on I2C core to
> figure out the interrupt for us?
Agree and verified that's really not needed. I2C core do the job.
I will remove it.
>
> Also, could I ask you to move the driver from of_property_*() to
> device_property_*() API?
And yes. I can move it to device_property API.
I prepare the changes as separate commits after I solve issues
with my "add advanced sensitivity settings" patch.
>
> Thanks.
>
BR,
Jiri Valek
^ permalink raw reply
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Maxime Ripard @ 2023-06-13 12:06 UTC (permalink / raw)
To: Doug Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=XnANRM=+2D9+DzcXx9Gw6iKKQsgkAiq8=izNEN-91f_Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6926 bytes --]
On Mon, Jun 12, 2023 at 02:13:46PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > I guess we can have
> > > > something much simpler with a bunch of helpers that would register a
> > > > i2c-hid device and would be called by the panel driver itself.
> > > >
> > > > And then, since everything is self-contained managing the power state
> > > > becomes easier as well.
> > >
> > > Can you give me more details about how you think this would work?
> > >
> > > When you say that the panel would register an i2c-hid device itself,
> > > do you mean that we'd do something like give a phandle to the i2c bus
> > > to the panel and then the panel would manually instantiate the i2c-hid
> > > device on it? ...and I guess it would need to be a "subclass" of
> > > i2c-hid that knew about the connection to the panel code? This
> > > subclass and the panel code would communicate with each other about
> > > power sequencing needs through some private API (like MFD devices
> > > usually do?). Assuming I'm understanding correctly, I think that could
> > > work.
> >
> > I guess what I had in mind is to do something similar to what we're
> > doing with hdmi-codec already for example.
>
> By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right?
No, sorry it was a bit ambiguous. I meant how we instantiate the
hdmi-codec driver here for example:
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/exynos/exynos_hdmi.c#L1665
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L2539
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/tegra/hdmi.c#L1525
> > We have several logical components already, in separate drivers, that
> > still need some cooperation.
> >
> > If the panel and touchscreen are on the same i2c bus, I think we could
> > even just get a reference to the panel i2c adapter, get a reference, and
> > pass that to i2c-hid (with a nice layer of helpers).
>
> Just for reference: the panel and touchscreen aren't on the same i2c
> bus. In the cases that I've looked at the panel is either controlled
> entirely by eDP or MIPI signals and isn't on any i2c bus at all. The
> touchscreen is on the i2c bus in the cases I've looked at, though I
> suppose I could imagine one that used a different bus.
Ok, so we would indeed need a phandle to the i2c controller
> > What I'm trying to say is: could we just make it work by passing a bunch
> > of platform_data, 2-3 callbacks and a device registration from the panel
> > driver directly?
>
> I think I'm still confused about what you're proposing. Sorry! :( Let
> me try rephrasing why I'm confused and perhaps we can get on the same
> page. :-)
>
> First, I guess I'm confused about how you have one of these devices
> "register" the other device.
>
> I can understand how one device might "register" its sub-devices in
> the MFD case. To make it concrete, we can look at a PMIC like
> max77686.c. The parent MFD device gets probed and then it's in charge
> of creating all of its sub-devices. These sub-devices are intimately
> tied to one another. They have shared data structures and can
> coordinate power sequencing and whatnot. All good.
We don't necessarily need to use MFD, but yeah, we could just register a
device for the i2c-hid driver to probe from (using
i2c_new_client_device?)
> ...but here, we really have something different in two fundamental ways:
>
> a) In this case, the two components (panel and touchscreen) both use
> separate primary communication methods. In DT the primary
> communication method determines where the device is described in the
> hierarchy. For eDP, this means that the DT node for the panel should
> be under the eDP controller. For an i2c touchscreen, this means that
> the DT node for the touchscreen should be under the i2c controller.
> Describing things like this causes the eDP controller to "register"
> the panel and the i2c controller to "register" the touchscreen. If we
> wanted the panel driver to "register" the touchscreen then it would
> get really awkward. Do we leave the touchscreen DT node under the i2c
> controller but somehow tell the i2c subsytem not to register it? Do we
> try to dynamically construct the touchscreen i2c node? Do we make a
> fake i2c controller under our panel DT node and somehow tell the i2c
> core to look at it?
I would expect not to have any DT node for the touchscreen, but we would
register a new i2c device on the bus that it's connected to.
In essence, it's also fairly similar to what we're doing with
i2c_new_ancillary_device() on some bridges. Except the primary device
isn't necessarily controlled through the I2C bus (but could be, I'm
pretty sure we have that situation for RGB or LVDS panels too).
The plus side would also be that we don't really need a DT to make it
work either. We just need the panel driver to probe somehow and a
pointer to the i2c_adapter.
> b) Things are different because the two devices here are not nearly as
> intimately tied to one another. At least in the case of "homestar",
> the only reason that the devices were tied to one another was because
> the board designers chose to share power rails, but otherwise the
> drivers were both generic.
Yeah, and that's fine I guess?
> In any case, is there any chance that we're in violent agreement
Is it even violent? Sorry if it came across that way, it's really isn't
on my end.
> and that if you dig into my design more you might like it? Other than
> the fact that the panel doesn't "register" the touchscreen device, it
> kinda sounds as if what my patches are already doing is roughly what
> you're describing. The touchscreen and panel driver are really just
> coordinating with each other through a shared data structure (struct
> drm_panel_follower) that has a few callback functions. Just like with
> "hdmi-codec", the devices probe separately but find each other through
> a phandle. The coordination between the two happens through a few
> simple helper functions.
I guess we very much agree on the end-goal, and I'd really like to get
this addressed somehow. There's a couple of things I'm not really
sold on with your proposal though:
- It creates a ad-hoc KMS API for some problem that looks fairly
generic. It's also redundant with the notifier mechanism without
using it (probably for the best though).
- MIPI-DSI panel probe sequence is already fairly complex and fragile
(See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
I'd rather avoid creating a new dependency in that graph.
- And yeah, to some extent it's inconsistent with how we dealt with
secondary devices in KMS so far.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH] HID: logitech-dj: Fix error handling in logi_dj_recv_switch_to_dj_mode()
From: Nikita Zhandarovich @ 2023-06-13 10:16 UTC (permalink / raw)
To: Filipe Laíns
Cc: Nikita Zhandarovich, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, lvc-project
Presently, if a call to logi_dj_recv_send_report() fails, we do
not learn about the error until after sending short
HID_OUTPUT_REPORT with hid_hw_raw_request().
To handle this somewhat unlikely issue, return on error in
logi_dj_recv_send_report() (minding ugly sleep workaround) and
take into account the result of hid_hw_raw_request().
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 6a9ddc897883 ("HID: logitech-dj: enable notifications on connect/disconnect")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
drivers/hid/hid-logitech-dj.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 62180414efcc..e6a8b6d8eab7 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1285,6 +1285,9 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
* 50 msec should gives enough time to the receiver to be ready.
*/
msleep(50);
+
+ if (retval)
+ return retval;
}
/*
@@ -1306,7 +1309,7 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
buf[5] = 0x09;
buf[6] = 0x00;
- hid_hw_raw_request(hdev, REPORT_ID_HIDPP_SHORT, buf,
+ retval = hid_hw_raw_request(hdev, REPORT_ID_HIDPP_SHORT, buf,
HIDPP_REPORT_SHORT_LENGTH, HID_OUTPUT_REPORT,
HID_REQ_SET_REPORT);
--
2.25.1
^ permalink raw reply related
* Re: [RESEND] Input: support pre-stored effects
From: Jeff LaBundy @ 2023-06-13 1:25 UTC (permalink / raw)
To: James Ogletree; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20230612194357.1022137-1-james.ogletree@cirrus.com>
Hi James,
On Mon, Jun 12, 2023 at 07:43:57PM +0000, James Ogletree wrote:
> At present, the best way to define effects that
> pre-exist in device memory is by utilizing
> the custom_data field, which it was not intended
> for, and requires arbitrary interpretation by
> the driver to make meaningful.
>
> Provide option for defining pre-stored effects in
> device memory.
>
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
> include/uapi/linux/input.h | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 2557eb7b0561..689e5fa10647 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -428,17 +428,27 @@ struct ff_rumble_effect {
> __u16 weak_magnitude;
> };
>
> +/**
> + * struct ff_prestored_effect - defines parameters of a pre-stored force-feedback effect
> + * @index: index of effect
> + * @bank: memory bank of effect
> + */
> +struct ff_prestored_effect {
> + __u16 index;
> + __u16 bank;
> +};
This seems like a good start; I do wonder if it's useful to review recent
customer vibrator HAL implementations and decide whether you want to pack
any additional members here such as magnitude, etc. as is done for periodic
effects?
Back in L25 days, some customers would assign click or tap effects to one
or more entries in the wavetable and then use a separate digital volume
control (at that time exposed through sysfs) to create a few discrete
amplitude levels. Perhaps it would be handy to bundle this information as
part of the same call?
It's just a suggestion; I'll defer to your much more recent expertise.
> +
> /**
> * struct ff_effect - defines force feedback effect
> * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
> - * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
> + * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
> * @id: an unique id assigned to an effect
> * @direction: direction of the effect
> * @trigger: trigger conditions (struct ff_trigger)
> * @replay: scheduling of the effect (struct ff_replay)
> * @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
> - * ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
> - * defining effect parameters
> + * ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
> + * further defining effect parameters
> *
> * This structure is sent through ioctl from the application to the driver.
> * To create a new effect application should set its @id to -1; the kernel
> @@ -464,6 +474,7 @@ struct ff_effect {
> struct ff_periodic_effect periodic;
> struct ff_condition_effect condition[2]; /* One for each axis */
> struct ff_rumble_effect rumble;
> + struct ff_prestored_effect prestored;
> } u;
> };
>
> @@ -479,20 +490,21 @@ struct ff_effect {
> #define FF_DAMPER 0x55
> #define FF_INERTIA 0x56
> #define FF_RAMP 0x57
> +#define FF_PRESTORED 0x58
>
> #define FF_EFFECT_MIN FF_RUMBLE
> -#define FF_EFFECT_MAX FF_RAMP
> +#define FF_EFFECT_MAX FF_PRESTORED
>
> /*
> * Force feedback periodic effect types
> */
>
> -#define FF_SQUARE 0x58
> -#define FF_TRIANGLE 0x59
> -#define FF_SINE 0x5a
> -#define FF_SAW_UP 0x5b
> -#define FF_SAW_DOWN 0x5c
> -#define FF_CUSTOM 0x5d
> +#define FF_SQUARE 0x59
> +#define FF_TRIANGLE 0x5a
> +#define FF_SINE 0x5b
> +#define FF_SAW_UP 0x5c
> +#define FF_SAW_DOWN 0x5d
> +#define FF_CUSTOM 0x5e
>
> #define FF_WAVEFORM_MIN FF_SQUARE
> #define FF_WAVEFORM_MAX FF_CUSTOM
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: Add bindings for Azoteq IQS7210A/7211A/E
From: Jeff LaBundy @ 2023-06-13 0:58 UTC (permalink / raw)
To: Rob Herring
Cc: dmitry.torokhov, linux-input, devicetree, krzysztof.kozlowski+dt,
conor+dt
In-Reply-To: <20230612152925.GA65549-robh@kernel.org>
Hi Rob,
On Mon, Jun 12, 2023 at 09:29:25AM -0600, Rob Herring wrote:
> On Sun, Jun 11, 2023 at 08:06:24PM -0500, Jeff LaBundy wrote:
> > Hi Rob,
> >
> > On Fri, Jun 09, 2023 at 08:25:38AM -0600, Rob Herring wrote:
> > > On Mon, May 29, 2023 at 07:33:47PM -0500, Jeff LaBundy wrote:
> > > > Add bindings for the Azoteq IQS7210A/7211A/E family of trackpad/
> > > > touchscreen controllers.
> > > >
> > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > > > ---
> > > > Changes in v2:
> > > > - Renamed 'azoteq,default-comms' to 'azoteq,forced-comms-default' and redefined
> > > > 0, 1 and 2 as unspecified, 0 and 1, respectively
> > > > - Defined ATI upon its first occurrence
> > > > - Redefined 'azoteq,gesture-angle' in units of degrees
> > > > - Declared 'azoteq,rx-enable' to depend upon 'azoteq,tx-enable' within the
> > > > 'trackpad' node
> > > >
> > > > Hi Rob,
> > > >
> > > > I attempted to reference existing properties from a common binding [1] as per
> > > > your feedback in [2], however 'make DT_CHECKER_FLAGS=-m dt_binding_check' fails
> > > > with the message 'Vendor specific properties must have a type and description
> > > > unless they have a defined, common suffix.'
> > >
> > > Is that because you have differing constraints in each case?
> >
> > In the failing example [2], I have started with a simple boolean that carries
> > nothing but a type and description. From the new azoteq,common.yaml:
> >
> > properties:
> > [...]
> >
> > azoteq,use-prox:
> > type: boolean
> > description: foo
> >
> > And from the first consumer:
> >
> > patternProperties:
> > "^hall-switch-(north|south)$":
> > type: object
> > allOf:
> > - $ref: input.yaml#
> > - $ref: azoteq,common.yaml#
> > description: bar
> >
> > properties:
> > linux,code: true
> > azoteq,use-prox: true
> >
> > However, the tooling presents the following:
> >
> > CHKDT Documentation/devicetree/bindings/processed-schema.json
> > /home/jlabundy/work/linux/Documentation/devicetree/bindings/input/iqs62x-keys.yaml: patternProperties:^hall-switch-(north|south)$:properties:azoteq,use-prox: True is not of type 'object'
> > hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> > from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> >
> > [...]
> >
> > I am committed to addressing your feedback; to help me do so, can you help me
> > identify what I've done wrong, and/or point me to an example that successfully
> > passes dt_binding_check?
>
> You're not doing anything wrong. There's 2 options here. The first is we
> could just relax the check to allow boolean schema for vendor
> properties. The main issue with that is we then have to look for that
> improperly used and it doesn't help if you do have additional
> constraints to add on top of the common definition. The former can
> mostly be addressed by checking there is a type associated with the
> property. I'm going to look into adding that.
Thank you for your feedback. I started with a boolean property at first to
simply test the idea before moving too far forward. In reality however, the
common binding has many uint32's and uint32-arrays as well, often with
different constraints among consumers. For this option to be effective, it
would need to be extended to all types IMO.
>
> Alternatively, you could drop listing the properties and
> use 'unevaluatedProperties'. That's not quite equal to what you have.
> Presumably, you have 'additionalProperties' in this case, so listing
> them serves the purpose of defining what subset of properties each node
> uses from the referenced schema. We frequently don't worry if there are
> common properties not used by a specific schema. This also wouldn't work
> if you have additional constraints to add.
Because of varying constraints among each consumer, I do not believe this
option is viable either.
I also think adopting 'unevaluatedProperties' here would be confusing from
a customer perspective in this case. The new common binding has dozens of
properties for which some are shared between devices A and B but not C, or
devices B and C but not A.
Without each device's binding explicitly opting in for supported properties,
it's difficult for customers to know what is supported for a given device.
These particular devices are highly configurable yet void of nonvolatile
memory, so there is simply no way around having so many properties. Most are
touched in some way throughout various downstream applications.
Therefore I'd like to propose option (3), which is to move forward with patch
[1/2] as-is and decouple the merging of this driver from future enhancements
to the tooling. While patch [1/2] is admittedly a big binding with some repeat
descriptions, none of the duplicate properties introduce a conflicting type.
If in the future option (1) can support all property types and handle varying
constraints among consumers, I would be happy to be one of the first guinea
pigs. Does this path seem like a reasonable compromise?
>
> Rob
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Doug Anderson @ 2023-06-12 21:13 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <z7wi4z4lxpkhvooqhihlkpubyvueb37gvrpmwk6v7xwj2lm6jn@b7rwyr5ic5x5>
Hi,
On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > > I guess we can have
> > > something much simpler with a bunch of helpers that would register a
> > > i2c-hid device and would be called by the panel driver itself.
> > >
> > > And then, since everything is self-contained managing the power state
> > > becomes easier as well.
> >
> > Can you give me more details about how you think this would work?
> >
> > When you say that the panel would register an i2c-hid device itself,
> > do you mean that we'd do something like give a phandle to the i2c bus
> > to the panel and then the panel would manually instantiate the i2c-hid
> > device on it? ...and I guess it would need to be a "subclass" of
> > i2c-hid that knew about the connection to the panel code? This
> > subclass and the panel code would communicate with each other about
> > power sequencing needs through some private API (like MFD devices
> > usually do?). Assuming I'm understanding correctly, I think that could
> > work.
>
> I guess what I had in mind is to do something similar to what we're
> doing with hdmi-codec already for example.
By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right?
> We have several logical components already, in separate drivers, that
> still need some cooperation.
>
> If the panel and touchscreen are on the same i2c bus, I think we could
> even just get a reference to the panel i2c adapter, get a reference, and
> pass that to i2c-hid (with a nice layer of helpers).
Just for reference: the panel and touchscreen aren't on the same i2c
bus. In the cases that I've looked at the panel is either controlled
entirely by eDP or MIPI signals and isn't on any i2c bus at all. The
touchscreen is on the i2c bus in the cases I've looked at, though I
suppose I could imagine one that used a different bus.
> What I'm trying to say is: could we just make it work by passing a bunch
> of platform_data, 2-3 callbacks and a device registration from the panel
> driver directly?
I think I'm still confused about what you're proposing. Sorry! :( Let
me try rephrasing why I'm confused and perhaps we can get on the same
page. :-)
First, I guess I'm confused about how you have one of these devices
"register" the other device.
I can understand how one device might "register" its sub-devices in
the MFD case. To make it concrete, we can look at a PMIC like
max77686.c. The parent MFD device gets probed and then it's in charge
of creating all of its sub-devices. These sub-devices are intimately
tied to one another. They have shared data structures and can
coordinate power sequencing and whatnot. All good.
...but here, we really have something different in two fundamental ways:
a) In this case, the two components (panel and touchscreen) both use
separate primary communication methods. In DT the primary
communication method determines where the device is described in the
hierarchy. For eDP, this means that the DT node for the panel should
be under the eDP controller. For an i2c touchscreen, this means that
the DT node for the touchscreen should be under the i2c controller.
Describing things like this causes the eDP controller to "register"
the panel and the i2c controller to "register" the touchscreen. If we
wanted the panel driver to "register" the touchscreen then it would
get really awkward. Do we leave the touchscreen DT node under the i2c
controller but somehow tell the i2c subsytem not to register it? Do we
try to dynamically construct the touchscreen i2c node? Do we make a
fake i2c controller under our panel DT node and somehow tell the i2c
core to look at it?
b) Things are different because the two devices here are not nearly as
intimately tied to one another. At least in the case of "homestar",
the only reason that the devices were tied to one another was because
the board designers chose to share power rails, but otherwise the
drivers were both generic.
In any case, is there any chance that we're in violent agreement and
that if you dig into my design more you might like it? Other than the
fact that the panel doesn't "register" the touchscreen device, it
kinda sounds as if what my patches are already doing is roughly what
you're describing. The touchscreen and panel driver are really just
coordinating with each other through a shared data structure (struct
drm_panel_follower) that has a few callback functions. Just like with
"hdmi-codec", the devices probe separately but find each other through
a phandle. The coordination between the two happens through a few
simple helper functions.
> > Is it cleaner than my current approach, though?
>
> "cleaner" is subjective, really, but it's a more "mainstream" approach
> that one can follow more easily through function calls.
>
> > I guess, alternatively, we could put the "panel" directly under the
> > i2c bus in this case. That would probably work for Cong Yang's current
> > needs, but we'd end up in trouble if we ever had a similar situation
> > with an eDP panel since eDP panels need to be under the DP-AUX bus.
>
> I don't know DP-AUX very well, what is the issue that you're mentioning?
Hopefully I've explained what I meant above (see point "a)").
^ permalink raw reply
* [RESEND] Input: support pre-stored effects
From: James Ogletree @ 2023-06-12 19:43 UTC (permalink / raw)
Cc: James Ogletree, Dmitry Torokhov, linux-input, linux-kernel
At present, the best way to define effects that
pre-exist in device memory is by utilizing
the custom_data field, which it was not intended
for, and requires arbitrary interpretation by
the driver to make meaningful.
Provide option for defining pre-stored effects in
device memory.
Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
---
include/uapi/linux/input.h | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 2557eb7b0561..689e5fa10647 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -428,17 +428,27 @@ struct ff_rumble_effect {
__u16 weak_magnitude;
};
+/**
+ * struct ff_prestored_effect - defines parameters of a pre-stored force-feedback effect
+ * @index: index of effect
+ * @bank: memory bank of effect
+ */
+struct ff_prestored_effect {
+ __u16 index;
+ __u16 bank;
+};
+
/**
* struct ff_effect - defines force feedback effect
* @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
- * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
+ * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
* @id: an unique id assigned to an effect
* @direction: direction of the effect
* @trigger: trigger conditions (struct ff_trigger)
* @replay: scheduling of the effect (struct ff_replay)
* @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
- * ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
- * defining effect parameters
+ * ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
+ * further defining effect parameters
*
* This structure is sent through ioctl from the application to the driver.
* To create a new effect application should set its @id to -1; the kernel
@@ -464,6 +474,7 @@ struct ff_effect {
struct ff_periodic_effect periodic;
struct ff_condition_effect condition[2]; /* One for each axis */
struct ff_rumble_effect rumble;
+ struct ff_prestored_effect prestored;
} u;
};
@@ -479,20 +490,21 @@ struct ff_effect {
#define FF_DAMPER 0x55
#define FF_INERTIA 0x56
#define FF_RAMP 0x57
+#define FF_PRESTORED 0x58
#define FF_EFFECT_MIN FF_RUMBLE
-#define FF_EFFECT_MAX FF_RAMP
+#define FF_EFFECT_MAX FF_PRESTORED
/*
* Force feedback periodic effect types
*/
-#define FF_SQUARE 0x58
-#define FF_TRIANGLE 0x59
-#define FF_SINE 0x5a
-#define FF_SAW_UP 0x5b
-#define FF_SAW_DOWN 0x5c
-#define FF_CUSTOM 0x5d
+#define FF_SQUARE 0x59
+#define FF_TRIANGLE 0x5a
+#define FF_SINE 0x5b
+#define FF_SAW_UP 0x5c
+#define FF_SAW_DOWN 0x5d
+#define FF_CUSTOM 0x5e
#define FF_WAVEFORM_MIN FF_SQUARE
#define FF_WAVEFORM_MAX FF_CUSTOM
--
2.25.1
^ permalink raw reply related
* Re: Tree dumb questions from an occasional
From: Marco Morandini @ 2023-06-12 19:33 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAO-hwJKNqosABrr-VGSUVPs8CKRpQNTL0xgNLeT4yg453ADWZA@mail.gmail.com>
> Yeah, right. This is the kind of situation where it's usually easy
> enough to detect with hid-tools[0]. We can record the device on your
> machine, then we can replay it locally on ours, and make several
> attempts.
I was not aware of hid-tools, I will mention it in my doc attempt!
> Sure. Please write (if you want) your first draft, we can review it
> and we can iterate from there. Do not forget to add the linux doc
> mailing list in CC in case some people from there want to also add
> things.
Ok, will try.
>> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
>> while loading the usb_hid module, but not while loading the usb_generic
>> one (that turned out to be the module that manages my HP pointer),
>> even if the statically defined quirks were moved into their own file.
>> Would it make sense to add the possibility to
>> add quirks while loading hid_generic? Is this the right place for
>> such code? If yes, I can try in my spare time to do this,
>> even if I'm not sure I'll be able to get it right.
>
> I'm not 100% sure of what you mean, but currently dynamic quirks can
> be added to the *usbhid* module (not usb_hid or usb_generic), which is
> the transport layer driver for HID.
> This module is responsible for creating a HID device, which can be
> handled by HID drivers, hid_generic being one of them.
You are right, I should have written usbhid.
I was convinced that hid_generic
did not get the quirks that were set at loading time by usbhid, but only those
defined in quirks.c .
What I really meant was that the quirk I ended up adding is
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
If I got it right usbhid can add only quirks with
BUS_USB (and not BUS_BLUETOOTH, like the above)
because of the code in usbhid/hid-core.c
(
retval = hid_quirks_init(quirks_param, BUS_USB, MAX_USBHID_BOOT_QUIRKS);
is this the right line in hid_init ?
)
and the fact that one cannot specify the
bus that should be used (whatever this "bus" means in the kernel, I
still need to get it):
MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
" quirks=vendorID:productID:quirks"
" where vendorID, productID, and quirks are all in"
" 0x-prefixed hex");
Long story short: if I
- either boot with
usbhid.quirks=0x3f0:0x464a:0x40
- or, alternatively try to
sun:/home/marco/READMEs/HPElitePresenterMouse # rmmod usbhid
sun:/home/marco/READMEs/HPElitePresenterMouse # modprobe -v usbhid "quirks=0x03f0:0x464a:0x40"
insmod /lib/modules/6.3.6-1-default/kernel/drivers/hid/usbhid/usbhid.ko.zst quirks=0x03f0:0x464a:0x40
my device does not work correctly, but with the added
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_HP, 0x464a), HID_QUIRK_MULTI_INPUT }
it does work.
Hoping that I got it right and HID_QUIRK_MULTI_INPUT corresponds to
0x40, otherwise all what I've written make no sense, and I should go back
and re-do my homework....
At any rate: if there is a way to specify the correct quirk for a device like the one
I stumbled upon, while waiting for the correct upstream fix to percolate down
to the distributions, then of course my questions 2) and 3) (add the options to
specify quirks while loading hid-generic (question 2) and by resorting to ebpf (question 3))
do not make sense.
> As the name says, hid_generic is supposed to be generic, and I do not
> want to see special cases handled there, because it would not be
> generic anymore.
Understood, thank you.
> Furthermore, if you submit your patch to the LKML, the quirk will
> likely end up in driver/hid/hid-quirks.c which is exactly the static
> equivalent of the dynamic one from usbhid.
Not exactly, because of the bus issue (again, assuming I got it right).
> No need to apologize. You are actually proposing ideas and your help
> to make things better for end-users, which is extremely valuable in
> itself :)
Thank you, you are very kind. I only hope I've not written too much
nonsense here above.
Marco
^ permalink raw reply
* Re: [PATCH RFC 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-12 17:07 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <a87160e5-b895-3dae-bba0-94fc67c92679@linaro.org>
Hi Neil,
[...]
> > > +static const struct input_id goodix_berlin_spi_input_id = {
> > > + .bustype = BUS_SPI,
> > > + .vendor = 0x0416,
> > > + .product = 0x1001,
> >
> > After having seen these in the I2C counterpart; consider defining them
> > in goodix_berlin.h.
>
> To be honest, I blindly copied it from goodix.c because the vendor
> driver puts random values here.
>
> input_dev->id.product = 0xDEAD;
> input_dev->id.vendor = 0xBEEF;
>
> So what should I set ?
If there is no explicit guidance from the vendor, I would simply leave
these unassigned; in theory one would imagine that this controller would
have a different product ID than other models.
[...]
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH RFC 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-12 16:59 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <3ad327dd-cefd-b195-2d64-6d6afc2d4283@linaro.org>
Hi Neil,
[...]
> > > + * - PEN Events
> >
> > What is "PEN"; is it "stylus" as written elsewhere?
>
> Yes, they use both terms in the vendor driver.
I recommend using a common term throughout; as written, I first thought
"PEN" was an acronym.
[...]
> > > + misc->fw_attr_len = le16_to_cpu(misc->fw_attr_len);
> > > + misc->fw_log_len = le16_to_cpu(misc->fw_log_len);
> > > + misc->stylus_struct_len = le16_to_cpu(misc->stylus_struct_len);
> > > + misc->mutual_struct_len = le16_to_cpu(misc->mutual_struct_len);
> > > + misc->self_struct_len = le16_to_cpu(misc->self_struct_len);
> > > + misc->noise_struct_len = le16_to_cpu(misc->noise_struct_len);
> > > + misc->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
> > > + misc->touch_data_head_len = le16_to_cpu(misc->touch_data_head_len);
> > > + misc->point_struct_len = le16_to_cpu(misc->point_struct_len);
> > > + misc->mutual_rawdata_addr = le32_to_cpu(misc->mutual_rawdata_addr);
> > > + misc->mutual_diffdata_addr = le32_to_cpu(misc->mutual_diffdata_addr);
> > > + misc->mutual_refdata_addr = le32_to_cpu(misc->mutual_refdata_addr);
> > > + misc->self_rawdata_addr = le32_to_cpu(misc->self_rawdata_addr);
> > > + misc->self_diffdata_addr = le32_to_cpu(misc->self_diffdata_addr);
> > > + misc->self_refdata_addr = le32_to_cpu(misc->self_refdata_addr);
> > > + misc->iq_rawdata_addr = le32_to_cpu(misc->iq_rawdata_addr);
> > > + misc->iq_refdata_addr = le32_to_cpu(misc->iq_refdata_addr);
> > > + misc->im_rawdata_addr = le32_to_cpu(misc->im_rawdata_addr);
> > > + misc->im_readata_len = le16_to_cpu(misc->im_readata_len);
> > > + misc->noise_rawdata_addr = le32_to_cpu(misc->noise_rawdata_addr);
> > > + misc->noise_rawdata_len = le16_to_cpu(misc->noise_rawdata_len);
> > > + misc->stylus_rawdata_addr = le32_to_cpu(misc->stylus_rawdata_addr);
> > > + misc->stylus_rawdata_len = le16_to_cpu(misc->stylus_rawdata_len);
> > > + misc->noise_data_addr = le32_to_cpu(misc->noise_data_addr);
> > > + misc->esd_addr = le32_to_cpu(misc->esd_addr);
> >
> > What is all of this stuff for? The beginning of the driver explicitly states
> > that ESD recovery is not supported; please consider stripping all of this if
> > its only purpose is to support noise in the logs or sysfs attributes that we
> > cannot reasonably react to.
>
> Those are all the firmware parameters used by the driver, for now only a little
> are used, we already read the entire struct so stripping isn't an option otherwise
> the values will be shifted.
>
> We only print those over debugfs se it won't fill any logs
I feel this is a lot of bloat to simply enable pretty printing of register
values. Some of these struct members aren't even used anywhere; others seem
to be related to functions this driver does not support. Do you realistically
expect one to read those values?
For registers that customers would in fact need to read for debugging, can
you not simply rely on regmap's natural debugfs facility? It seems you are
largely duplicating that here.
I understand that some parameters are used elsewhere and it may be efficient
to read the entire memory region as a packed struct, but there is no need to
unalign and store what is effectively padding in many cases.
[...]
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re:Re: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
From: johannes @ 2023-06-12 16:35 UTC (permalink / raw)
To: christophe.jaillet; +Cc: linux-input, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 10755 bytes --]
>Le 11/06/2023 à 18:48, Johannes Roith a écrit :
>> Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200
>> by using the HID interface.
>>
>> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
>> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
>> default).
>>
>> The driver was tested while also using the UART of the chip. Setting
>> and reading the GPIOs has no effect on the UART communication. However,
>> a reset is triggered after the CONFIGURE command. If the GPIO Direction
>> is constantly changed, this will affect the communication at low baud
>> rates. This is a hardware problem of the MCP2200 and is not caused by
>> the driver.
>>
>> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
>
>Hi,
>
>a few nits below, should it help the review.
>
>[...]
>
>> --- /dev/null
>> +++ b/drivers/hid/hid-mcp2200.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MCP2200 - Microchip USB to GPIO bridge
>> + *
>> + * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
>> + *
>> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
>> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
>> + */
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/bitfield.h>
>
>Is this include needed?
>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/hid.h>
>> +#include <linux/hidraw.h>
>> +#include <linux/gpio/driver.h>
>
>Many maintainers prefer alphabetic order for includes.
>
>
>> +#include "hid-ids.h"
>> +
>> +/* Commands codes in a raw output report */
>> +enum {
>> + SET_CLEAR_OUTPUTS = 0x08,
>> + CONFIGURE = 0x10,
>> + READ_EE = 0x20,
>> + WRITE_EE = 0x40,
>> + READ_ALL = 0x80
>> +};
>
>Does some
> #define xxx BIT(n)
>would make more sense than this enum?
I have changed the enum to defines. But I kept the hexadecimal format
as the commands comes from the HID appnote and I think it is more readable
that way.
>
>> +
>> +/* MCP GPIO direction encoding */
>> +enum MCP_IO_DIR {
>> + MCP2200_DIR_OUT = 0x00,
>> + MCP2200_DIR_IN = 0x01,
>> +};
>> +
>> +/* Altternative pin assignments */
>> +enum {
>> + TXLED = 2,
>> + RXLED = 3,
>> + USBCFG = 6,
>> + SSPND = 7,
>> +
>
>Uneeded new line.
>
>> +};
>> +
>> +#define MCP_NGPIO 8
>> +
>
>[...]
>
>> +/* this executes the READ_ALL cmd */
>> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
>> +{
>> + struct mcp_read_all *read_all;
>> + int len, t;
>> +
>> + reinit_completion(&mcp->wait_in_report);
>> + mutex_lock(&mcp->lock);
>> +
>> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
>> + if (!read_all)
>> + return -ENOMEM;
>
>Allocation could be deone before the lock.
>
>> +
>> + read_all->cmd = READ_ALL;
>> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
>> + sizeof(struct mcp_read_all));
>> +
>> + if (len != sizeof(struct mcp_read_all))
>
>kfree(read_all); ?
>(or move the call just below before the test)
>
>> + return -EINVAL;
>> +
>> + kfree(read_all);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree() or even before the "if
>(len..." a few lines above.
>
>> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
>> + if (!t)
>> + return -ETIMEDOUT;
>> +
>> + /* return status, negative value if wrong response was received */
>> + return mcp->status;
>> +}
>> +
>> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + u8 value;
>> + int status;
>> + struct mcp_set_clear_outputs *cmd;
>> +
>> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
>> + if (!cmd)
>> + return;
>> +
>> + mutex_lock(&mcp->lock);
>> +
>> + value = mcp->gpio_val & ~*mask;
>> + value |= (*mask & *bits);
>> +
>> + cmd->cmd = SET_CLEAR_OUTPUTS;
>> + cmd->set_bmap = value;
>> + cmd->clear_bmap = ~(value);
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs))
>> + mcp->gpio_val = value;
>> +
>> + kfree(cmd);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +}
>> +
>> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
>> +{
>> + unsigned long mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + unsigned long bmap_value = (value<<gpio_nr);
>
>Uneeded () and missing spaces aoud <<
>
>> +
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> +}
>> +
>> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + u32 val;
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + int status;
>> +
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + val = mcp->gpio_inval;
>> + *bits = (val & *mask);
>> + return 0;
>> +}
>> +
>> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + unsigned long mask = 0, bits = 0;
>
>No need to init long (and maybe bits)
>
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + mcp_get_multiple(gc, &mask, &bits);
>> + return (bits > 0) ? 1 : 0;
>> +}
>> +
>> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> +
>> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
>> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>> +}
>> +
>> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
>> + enum MCP_IO_DIR io_direction)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + struct mcp_configure *conf;
>> + int status;
>> + /* after the configure cmd we will need to set the outputs again */
>> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
>> + unsigned long bits = mcp->gpio_val;
>> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
>> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
>> + u8 config_alt_pins = mcp->config_alt_pins;
>> +
>> + /* Read in the reset baudrate first, we need it later */
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
>> + if (!conf)
>> + return -ENOMEM;
>> + mutex_lock(&mcp->lock);
>> +
>> + /* configure will reset the chip! */
>> + conf->cmd = CONFIGURE;
>> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
>> + | (io_direction << gpio_nr);
>> + /* Don't overwrite the reset parameters */
>> + conf->baud_h = mcp->baud_h;
>> + conf->baud_l = mcp->baud_l;
>> + conf->config_alt_options = mcp->config_alt_options;
>> + conf->io_default_val_bmap = mcp->gpio_reset_val;
>> + /* Adjust alt. func if necessary */
>> + if (alt_pin_conf[gpio_nr])
>> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
>> + conf->config_alt_pins = config_alt_pins;
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs)) {
>> + mcp->gpio_dir &= ~(1 << gpio_nr);
>> + mcp->config_alt_pins = config_alt_pins;
>> + } else {
>> + return -EIO;
>> + }
>> +
>> + kfree(conf);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +
>> + /* Configure CMD will clear all IOs -> rewrite them */
>> + mcp_set_multiple(gc, &mask, &bits);
>> + return 0;
>> +}
>> +
>> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
>> +}
>> +
>> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
>> + int value)
>> +{
>> + int ret;
>> + unsigned long mask, bmap_value;
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + bmap_value = (value << gpio_nr);
>
>Uneeded ()
>
>> +
>> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
>> + if (ret == 0)
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> + return ret;
>> +}
>> +
>> +
>
>No need for 2 new lines.
>
>[...]
>
>> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + int ret;
>> + struct mcp2200 *mcp;
>> +
>> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
>> + if (!mcp)
>> + return -ENOMEM;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't parse reports\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * This driver uses the .raw_event callback and therefore does not need any
>> + * HID_CONNECT_xxx flags.
>> + */
>> + ret = hid_hw_start(hdev, 0);
>> + if (ret) {
>> + hid_err(hdev, "can't start hardware\n");
>> + return ret;
>> + }
>> +
>> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
>> + hdev->version & 0xff, hdev->name, hdev->phys);
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't open device\n");
>> + hid_hw_stop(hdev);
>> + return ret;
>> + }
>> +
>> + mutex_init(&mcp->lock);
>> + init_completion(&mcp->wait_in_report);
>> + hid_set_drvdata(hdev, mcp);
>> + mcp->hdev = hdev;
>> +
>> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
>> + if (ret)
>> + return ret;
>> +
>> + mcp->gc = template_chip;
>> + mcp->gc.parent = &hdev->dev;
>> +
>> + ret = gpiochip_add_data(&mcp->gc, mcp);
>
>devm_gpiochip_add_data() and no .remove function?
The gpiochip will be removed in mcp2200_remove
>
>> + if (ret < 0) {
>> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
>
>hid_err() to be consistent?
>
>> + hid_hw_stop(hdev);
>
>hid_hw_stop() would be called twice. Once here and once because of the
>devm_add_action_or_reset() above.
I think it is only called once. It will be called when gpiochip_add_data or
hid_hw_open returns an error or when the mcp2200_hid_unregister gets called.
>
>
>Just my 2c,
>
>CJ
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
>[...]
>
Hi,
thanks for your feedback. I have added most of it in my Kernel module. For the
view things, I haven't added, I add a comment under your comment.
I am sorry for the error in hid-ids.h. It seems I forgot to run a git add hid-ids.h
to rename the second USB_DEVICE_ID_MCP2221 into USB_DEVICE_ID_MCP2200.
What do you think is the best way to deliver my driver? Should I create a new
single patch containg everything I have changed or should I go with the two
already created patches, reorder them, fixing the error in hid-ids.h and adding
a new patch including the comments from your review? This is my first
contribution, that's why I am asking.
Best regards,
Johannes
^ permalink raw reply
* Re:Re: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
From: johannes @ 2023-06-12 16:27 UTC (permalink / raw)
To: christophe.jaillet; +Cc: linux-input, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 10388 bytes --]
>Le 11/06/2023 à 18:48, Johannes Roith a écrit :
>> Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200
>> by using the HID interface.
>>
>> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
>> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
>> default).
>>
>> The driver was tested while also using the UART of the chip. Setting
>> and reading the GPIOs has no effect on the UART communication. However,
>> a reset is triggered after the CONFIGURE command. If the GPIO Direction
>> is constantly changed, this will affect the communication at low baud
>> rates. This is a hardware problem of the MCP2200 and is not caused by
>> the driver.
>>
>> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
>
>Hi,
>
>a few nits below, should it help the review.
>
>[...]
>
>> --- /dev/null
>> +++ b/drivers/hid/hid-mcp2200.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MCP2200 - Microchip USB to GPIO bridge
>> + *
>> + * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
>> + *
>> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
>> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
>> + */
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/bitfield.h>
>
>Is this include needed?
>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/hid.h>
>> +#include <linux/hidraw.h>
>> +#include <linux/gpio/driver.h>
>
>Many maintainers prefer alphabetic order for includes.
>
>
>> +#include "hid-ids.h"
>> +
>> +/* Commands codes in a raw output report */
>> +enum {
>> + SET_CLEAR_OUTPUTS = 0x08,
>> + CONFIGURE = 0x10,
>> + READ_EE = 0x20,
>> + WRITE_EE = 0x40,
>> + READ_ALL = 0x80
>> +};
>
>Does some
> #define xxx BIT(n)
>would make more sense than this enum?
>
>> +
>> +/* MCP GPIO direction encoding */
>> +enum MCP_IO_DIR {
>> + MCP2200_DIR_OUT = 0x00,
>> + MCP2200_DIR_IN = 0x01,
>> +};
>> +
>> +/* Altternative pin assignments */
>> +enum {
>> + TXLED = 2,
>> + RXLED = 3,
>> + USBCFG = 6,
>> + SSPND = 7,
>> +
>
>Uneeded new line.
>
>> +};
>> +
>> +#define MCP_NGPIO 8
>> +
>
>[...]
>
>> +/* this executes the READ_ALL cmd */
>> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
>> +{
>> + struct mcp_read_all *read_all;
>> + int len, t;
>> +
>> + reinit_completion(&mcp->wait_in_report);
>> + mutex_lock(&mcp->lock);
>> +
>> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
>> + if (!read_all)
>> + return -ENOMEM;
>
>Allocation could be deone before the lock.
>
>> +
>> + read_all->cmd = READ_ALL;
>> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
>> + sizeof(struct mcp_read_all));
>> +
>> + if (len != sizeof(struct mcp_read_all))
>
>kfree(read_all); ?
>(or move the call just below before the test)
>
>> + return -EINVAL;
>> +
>> + kfree(read_all);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree() or even before the "if
>(len..." a few lines above.
>
>> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
>> + if (!t)
>> + return -ETIMEDOUT;
>> +
>> + /* return status, negative value if wrong response was received */
>> + return mcp->status;
>> +}
>> +
>> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + u8 value;
>> + int status;
>> + struct mcp_set_clear_outputs *cmd;
>> +
>> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
>> + if (!cmd)
>> + return;
>> +
>> + mutex_lock(&mcp->lock);
>> +
>> + value = mcp->gpio_val & ~*mask;
>> + value |= (*mask & *bits);
>> +
>> + cmd->cmd = SET_CLEAR_OUTPUTS;
>> + cmd->set_bmap = value;
>> + cmd->clear_bmap = ~(value);
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs))
>> + mcp->gpio_val = value;
>> +
>> + kfree(cmd);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +}
>> +
>> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
>> +{
>> + unsigned long mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + unsigned long bmap_value = (value<<gpio_nr);
>
>Uneeded () and missing spaces aoud <<
>
>> +
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> +}
>> +
>> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + u32 val;
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + int status;
>> +
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + val = mcp->gpio_inval;
>> + *bits = (val & *mask);
>> + return 0;
>> +}
>> +
>> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + unsigned long mask = 0, bits = 0;
>
>No need to init long (and maybe bits)
>
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + mcp_get_multiple(gc, &mask, &bits);
>> + return (bits > 0) ? 1 : 0;
>> +}
>> +
>> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> +
>> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
>> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>> +}
>> +
>> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
>> + enum MCP_IO_DIR io_direction)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + struct mcp_configure *conf;
>> + int status;
>> + /* after the configure cmd we will need to set the outputs again */
>> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
>> + unsigned long bits = mcp->gpio_val;
>> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
>> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
>> + u8 config_alt_pins = mcp->config_alt_pins;
>> +
>> + /* Read in the reset baudrate first, we need it later */
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
>> + if (!conf)
>> + return -ENOMEM;
>> + mutex_lock(&mcp->lock);
>> +
>> + /* configure will reset the chip! */
>> + conf->cmd = CONFIGURE;
>> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
>> + | (io_direction << gpio_nr);
>> + /* Don't overwrite the reset parameters */
>> + conf->baud_h = mcp->baud_h;
>> + conf->baud_l = mcp->baud_l;
>> + conf->config_alt_options = mcp->config_alt_options;
>> + conf->io_default_val_bmap = mcp->gpio_reset_val;
>> + /* Adjust alt. func if necessary */
>> + if (alt_pin_conf[gpio_nr])
>> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
>> + conf->config_alt_pins = config_alt_pins;
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs)) {
>> + mcp->gpio_dir &= ~(1 << gpio_nr);
>> + mcp->config_alt_pins = config_alt_pins;
>> + } else {
>> + return -EIO;
>> + }
>> +
>> + kfree(conf);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +
>> + /* Configure CMD will clear all IOs -> rewrite them */
>> + mcp_set_multiple(gc, &mask, &bits);
>> + return 0;
>> +}
>> +
>> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
>> +}
>> +
>> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
>> + int value)
>> +{
>> + int ret;
>> + unsigned long mask, bmap_value;
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + bmap_value = (value << gpio_nr);
>
>Uneeded ()
>
>> +
>> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
>> + if (ret == 0)
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> + return ret;
>> +}
>> +
>> +
>
>No need for 2 new lines.
>
>[...]
>
>> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + int ret;
>> + struct mcp2200 *mcp;
>> +
>> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
>> + if (!mcp)
>> + return -ENOMEM;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't parse reports\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * This driver uses the .raw_event callback and therefore does not need any
>> + * HID_CONNECT_xxx flags.
>> + */
>> + ret = hid_hw_start(hdev, 0);
>> + if (ret) {
>> + hid_err(hdev, "can't start hardware\n");
>> + return ret;
>> + }
>> +
>> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
>> + hdev->version & 0xff, hdev->name, hdev->phys);
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't open device\n");
>> + hid_hw_stop(hdev);
>> + return ret;
>> + }
>> +
>> + mutex_init(&mcp->lock);
>> + init_completion(&mcp->wait_in_report);
>> + hid_set_drvdata(hdev, mcp);
>> + mcp->hdev = hdev;
>> +
>> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
>> + if (ret)
>> + return ret;
>> +
>> + mcp->gc = template_chip;
>> + mcp->gc.parent = &hdev->dev;
>> +
>> + ret = gpiochip_add_data(&mcp->gc, mcp);
>
>devm_gpiochip_add_data() and no .remove function?
>
>> + if (ret < 0) {
>> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
>
>hid_err() to be consistent?
>
>> + hid_hw_stop(hdev);
>
>hid_hw_stop() would be called twice. Once here and once because of the
>devm_add_action_or_reset() above.
>
>
>Just my 2c,
>
>CJ
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
>[...]
>
Hi,
thanks for your feedback. I have added most of it in my Kernel module. For the
view things, I haven't added, I add a comment under your comment.
I am sorry for the error in hid-ids.h. It seems I forgot to run a git add hid-ids.h
to rename the second USB_DEVICE_ID_MCP2221 into USB_DEVICE_ID_MCP2200.
What do you think is the best way to deliver my driver? Should I create a new
patch containg everything I have changed or should I go with the two already
created patches, reorder them, fixing the error in hid-ids.h and adding a new
patch including the comments from your review? This is my first contribution,
that's why I am asking.
Best regards,
Johannes
^ permalink raw reply
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Maxime Ripard @ 2023-06-12 16:03 UTC (permalink / raw)
To: Doug Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=Wr7Xatw1LsofiZ5Xx7WBvAuMMdq4D5Po1yJUC1VdtZdg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]
Hi Doug,
On Thu, Jun 08, 2023 at 07:38:58AM -0700, Doug Anderson wrote:
> On Thu, Jun 8, 2023 at 12:17 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi Douglas,
> >
> > On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote:
> > >
> > > The big motivation for this patch series is mostly described in the patch
> > > ("drm/panel: Add a way for other devices to follow panel state"), but to
> > > quickly summarize here: for touchscreens that are connected to a panel we
> > > need the ability to power sequence the two device together. This is not a
> > > new need, but so far we've managed to get by through a combination of
> > > inefficiency, added costs, or perhaps just a little bit of brokenness.
> > > It's time to do better. This patch series allows us to do better.
> > >
> > > Assuming that people think this patch series looks OK, we'll have to
> > > figure out the right way to land it. The panel patches and i2c-hid
> > > patches will go through very different trees and so either we'll need
> > > an Ack from one side or the other or someone to create a tag for the
> > > other tree to pull in. This will _probably_ require the true drm-misc
> > > maintainers to get involved, not a lowly committer. ;-)
> > >
> > > Version 2 of this patch series doesn't change too much. At a high level:
> > > * I added all the forgotten "static" to functions.
> > > * I've hopefully made the bindings better.
> > > * I've integrated into fw_devlink.
> > > * I cleaned up a few descriptions / comments.
> > >
> > > This still needs someone to say that the idea looks OK or to suggest
> > > an alternative that solves the problems. ;-)
> >
> > Thanks for working on this.
> >
> > I haven't seen in any of your commit messages how the panels were
> > actually "packaged" together?
> >
> > Do a panel model typically come together with the i2c-hid support, or is
> > it added at manufacture time?
> >
> > If it's the latter, it's indeed a fairly loose connection and we need
> > your work.
> >
> > If it's the former though and we don't expect a given panel reference to
> > always (or never) come with a touchscreen attached,
>
> Thanks for your reply. Let me see what I can do to bring clarity.
>
> In at least some of the cases, I believe that the panel and the
> touchscreen _are_ logically distinct components, even if they've been
> glued together at some stage in manufacturing. Even on one of the
> "poster child" boards that I talk about in patch #3, the early
> versions of "homestar", I believe this to be the case. However, even
> if the panel and touchscreen are separate components then they still
> could be connected to the main board in a way that they share power
> and/or reset signals. In my experience, in every case where they do
> the EEs expect that the panel is power sequenced first and then the
> touchscreen is power sequenced second. The EEs look at the power
> sequencing requirements of the panel and touchscreen, see that there
> is a valid power sequence protocol where they can share rails, and
> design the board that way. Even if the touchscreen and panel are
> logically separate, the moment the board designers hook them up to the
> same power rails and/or reset signals they become tied. This is well
> supported by my patch series.
>
> The case that really motivated my patch series, though, is the case
> that Cong Yang recently has been working on. I think most of the
> discussion is in his original patch series [1]. Cong Yang's patch
> series is largely focused on supporting the "ILI9882T" chip and some
> panels that it's used with. I found a datasheet for that, and the
> title from the first page is illustrative: "In-cell IC Integrates TFT
> LCD Driver and Capacitive Touch Controller into a Two Chip Cascade".
> This is an integrated solution that's designed to handle both the LCD
> and the touchscreen.
>
> [1] https://lore.kernel.org/lkml/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com/
Ok, I think we're on the same page at the hardware level then :)
> > I guess we can have
> > something much simpler with a bunch of helpers that would register a
> > i2c-hid device and would be called by the panel driver itself.
> >
> > And then, since everything is self-contained managing the power state
> > becomes easier as well.
>
> Can you give me more details about how you think this would work?
>
> When you say that the panel would register an i2c-hid device itself,
> do you mean that we'd do something like give a phandle to the i2c bus
> to the panel and then the panel would manually instantiate the i2c-hid
> device on it? ...and I guess it would need to be a "subclass" of
> i2c-hid that knew about the connection to the panel code? This
> subclass and the panel code would communicate with each other about
> power sequencing needs through some private API (like MFD devices
> usually do?). Assuming I'm understanding correctly, I think that could
> work.
I guess what I had in mind is to do something similar to what we're
doing with hdmi-codec already for example.
We have several logical components already, in separate drivers, that
still need some cooperation.
If the panel and touchscreen are on the same i2c bus, I think we could
even just get a reference to the panel i2c adapter, get a reference, and
pass that to i2c-hid (with a nice layer of helpers).
What I'm trying to say is: could we just make it work by passing a bunch
of platform_data, 2-3 callbacks and a device registration from the panel
driver directly?
> Is it cleaner than my current approach, though?
"cleaner" is subjective, really, but it's a more "mainstream" approach
that one can follow more easily through function calls.
> I guess, alternatively, we could put the "panel" directly under the
> i2c bus in this case. That would probably work for Cong Yang's current
> needs, but we'd end up in trouble if we ever had a similar situation
> with an eDP panel since eDP panels need to be under the DP-AUX bus.
I don't know DP-AUX very well, what is the issue that you're mentioning?
> I guess overall, though, while I think this approach could solve Cong
> Yang's needs, I still feel like it's worth solving the case where
> board designers have made panel and touchscreens "coupled" by having
> them rely on the same power rails and/or reset signals.
Sure, I definitely want that too :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: Add bindings for Azoteq IQS7210A/7211A/E
From: Rob Herring @ 2023-06-12 15:29 UTC (permalink / raw)
To: Jeff LaBundy
Cc: dmitry.torokhov, linux-input, devicetree, krzysztof.kozlowski+dt,
conor+dt
In-Reply-To: <ZIZvkGqr4a0kOGnR@nixie71>
On Sun, Jun 11, 2023 at 08:06:24PM -0500, Jeff LaBundy wrote:
> Hi Rob,
>
> On Fri, Jun 09, 2023 at 08:25:38AM -0600, Rob Herring wrote:
> > On Mon, May 29, 2023 at 07:33:47PM -0500, Jeff LaBundy wrote:
> > > Add bindings for the Azoteq IQS7210A/7211A/E family of trackpad/
> > > touchscreen controllers.
> > >
> > > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > > ---
> > > Changes in v2:
> > > - Renamed 'azoteq,default-comms' to 'azoteq,forced-comms-default' and redefined
> > > 0, 1 and 2 as unspecified, 0 and 1, respectively
> > > - Defined ATI upon its first occurrence
> > > - Redefined 'azoteq,gesture-angle' in units of degrees
> > > - Declared 'azoteq,rx-enable' to depend upon 'azoteq,tx-enable' within the
> > > 'trackpad' node
> > >
> > > Hi Rob,
> > >
> > > I attempted to reference existing properties from a common binding [1] as per
> > > your feedback in [2], however 'make DT_CHECKER_FLAGS=-m dt_binding_check' fails
> > > with the message 'Vendor specific properties must have a type and description
> > > unless they have a defined, common suffix.'
> >
> > Is that because you have differing constraints in each case?
>
> In the failing example [2], I have started with a simple boolean that carries
> nothing but a type and description. From the new azoteq,common.yaml:
>
> properties:
> [...]
>
> azoteq,use-prox:
> type: boolean
> description: foo
>
> And from the first consumer:
>
> patternProperties:
> "^hall-switch-(north|south)$":
> type: object
> allOf:
> - $ref: input.yaml#
> - $ref: azoteq,common.yaml#
> description: bar
>
> properties:
> linux,code: true
> azoteq,use-prox: true
>
> However, the tooling presents the following:
>
> CHKDT Documentation/devicetree/bindings/processed-schema.json
> /home/jlabundy/work/linux/Documentation/devicetree/bindings/input/iqs62x-keys.yaml: patternProperties:^hall-switch-(north|south)$:properties:azoteq,use-prox: True is not of type 'object'
> hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>
> [...]
>
> I am committed to addressing your feedback; to help me do so, can you help me
> identify what I've done wrong, and/or point me to an example that successfully
> passes dt_binding_check?
You're not doing anything wrong. There's 2 options here. The first is we
could just relax the check to allow boolean schema for vendor
properties. The main issue with that is we then have to look for that
improperly used and it doesn't help if you do have additional
constraints to add on top of the common definition. The former can
mostly be addressed by checking there is a type associated with the
property. I'm going to look into adding that.
Alternatively, you could drop listing the properties and
use 'unevaluatedProperties'. That's not quite equal to what you have.
Presumably, you have 'additionalProperties' in this case, so listing
them serves the purpose of defining what subset of properties each node
uses from the referenced schema. We frequently don't worry if there are
common properties not used by a specific schema. This also wouldn't work
if you have additional constraints to add.
Rob
^ permalink raw reply
* Re: [PATCH] HID: sensor-hub: Allow multi-function sensor devices
From: srinivas pandruvada @ 2023-06-12 13:34 UTC (permalink / raw)
To: Jiri Kosina
Cc: Daniel Thompson, Jonathan Cameron, Benjamin Tissoires,
Bjorn Andersson, linux-input, linux-iio, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2306120938360.5716@cbobk.fhfr.pm>
On Mon, 2023-06-12 at 09:39 +0200, Jiri Kosina wrote:
> On Sun, 11 Jun 2023, srinivas pandruvada wrote:
>
> > > > The Lenovo Yoga C630 has a combined keyboard and accelerometer
> > > > that
> > > > interfaces via i2c-hid. Currently this laptop either has a
> > > > working
> > > > keyboard (if CONFIG_HID_SENSOR_HUB is disabled) or a working
> > > > accelerometer. only works on kernels. Put another way, most
> > > > distro
> > > > kernels enable CONFIG_HID_SENSOR_HUB and therefore cannot work
> > > > on
> > > > this device since the keyboard doesn't work!
> > > >
> > > > Fix this by providing a richer connect mask during the probe.
> > > > With
> > > > this change both keyboard and screen orientation sensors work
> > > > correctly.
> > > >
> > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > >
> > > Srinivas, are you aware of any multi-function device that this
> > > patch
> > > might break?
> > I tried two systems, one with ISH and another with external hub.
> > Didn't
> > break anything. I don't have 5+ years old systems, to confirm if it
> > will
> > break something on older systems with external hub.
>
> Thanks a lot. Can I take the above as an equivalent of
>
> Acked-by: Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>
Sure.
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
>
Thanks,
Srinivas
> ? :)
>
^ permalink raw reply
* Re: Tree dumb questions from an occasional
From: Benjamin Tissoires @ 2023-06-12 13:07 UTC (permalink / raw)
To: Marco Morandini; +Cc: Jiri Kosina, linux-input
In-Reply-To: <5673fd15-b3f2-7911-6eea-39e5126f4874@polimi.it>
Hi Marco,
On Mon, Jun 12, 2023 at 12:29 PM Marco Morandini
<marco.morandini@polimi.it> wrote:
>
> First of all, please bear with me for writing this.
>
> Should this appear email to be a criticism toward any of you,
> be assured that this is not the intention. I'm
> really grateful to all you guys, who keep improving the kernel
> and allows us to use a free operating system.
>
> My questions here below are basically along the lines of
> "would it make sense to write this and that?". This is not
> because I'd like someone else to do the work for me, but
> because I really don't know whether it makes sense or not.
> Should be wrong to make such type of questions without
> accompanying proofs of concept, then please ignore me, and
> I'll go back into my cave.
Asking questions is always fine. And given that you already did a lot
of homework, not answering would be rude ;)
>
> Some background: I've recently bought a laser digital pointer
> from HP. It connects through Bluetooth.
> Since it did not work, I made a point to have it working,
> even if I'm not a kernel developer, and do not plan to become a
> kernel developer.
> At the end, this turned out to be a two-line patch, adding
> the HID_QUIRK_MULTI_INPUT for such device. Something that
> would likely require less than 5 minutes
> of a not-so proficient kernel developer.
Yeah, right. This is the kind of situation where it's usually easy
enough to detect with hid-tools[0]. We can record the device on your
machine, then we can replay it locally on ours, and make several
attempts.
>
> However, the process for me was much more cumbersome:
> I had to navigate a lot o wrong or misleading documentation
> in different forums, try to make sense of the kernel documentation,
> understand what a HID descriptor is,
> understand how to parse it, try to make sense of some
> unknown kernel code (mostly unsuccessfully),
> try with ebpf, try this and that... you get the idea.
Heh, you tried hid-bpf :) thanks!
>
> Now, I'm writing because I _think_ I've learned something
> in the process, and perhaps it could be useful to share it.
>
> Thus the questions:
>
> 1) do you think it would make sense to have some basic documentation
> describing what a hid descriptor is, where to download the documents
> defining it (https://www.usb.org/ is linked from the docs,
> but this is not enough to get started, at least for someone like me),
> how to actually read it from the hardware, how to parse it... ?
Yes, very much yes. At least having pointers to various projects that
can read HID descriptors and parse them would be already better.
> Very basic things, that, if I'm not wrong, are not currently
> covered by the kernel documentation, and that could allow
> someone else like me to get started more quickly?
> If yes, I can try to write a skeleton for that, but I'm not sure
> there will not be errors and/or omissions, thus it would likely need
> to be fixed by someone more knowledgeable than me.
Sure. Please write (if you want) your first draft, we can review it
and we can iterate from there. Do not forget to add the linux doc
mailing list in CC in case some people from there want to also add
things.
>
> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
> while loading the usb_hid module, but not while loading the usb_generic
> one (that turned out to be the module that manages my HP pointer),
> even if the statically defined quirks were moved into their own file.
> Would it make sense to add the possibility to
> add quirks while loading hid_generic? Is this the right place for
> such code? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.
I'm not 100% sure of what you mean, but currently dynamic quirks can
be added to the *usbhid* module (not usb_hid or usb_generic), which is
the transport layer driver for HID.
This module is responsible for creating a HID device, which can be
handled by HID drivers, hid_generic being one of them.
As the name says, hid_generic is supposed to be generic, and I do not
want to see special cases handled there, because it would not be
generic anymore.
However, other drivers, (hid_multitouch for instance) can have a
.driver_data dynamically set too, which allows for quirking a device.
But the quirk is local to the driver.
Given that HID_QUIRK_MULTI_INPUT is a global HID quirk, it makes sense
IMO to add it at the usbhid level because you are just using the
hid-generic implementation.
Furthermore, if you submit your patch to the LKML, the quirk will
likely end up in driver/hid/hid-quirks.c which is exactly the static
equivalent of the dynamic one from usbhid.
So I don't think having such a new quirk mechanism makes sense.
Furthermore, that quirk mechanism allows for quick and dirty testing
of the impact on the kernel, but a proper submission as a kernel patch
ensures that everybody gets the same fix, so I'd rather not have a
forum that explains in details what to do for a given HID product when
we can just quirk it in the tree and forget.
>
> 3) always if I get it right, currently it is not possible to inject quirks
> using ebpf, but only to modify the HID descriptor.
> Is this correct?
You can also change the events and filter them out if you want, but
yes that's pretty much the extent of HID_BPF nowadays.
> If yes, do you think it would be feasible and reasonable
> to add such a possibility? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.
Long story short: I would rather not have such bpf call.
The fundamental of HID-BPF is that you take a HID thingy, and you
output another HID thingy. Messing up with the kernel drivers is out
of the scope.
But furthermore, what would be the benefit compared to the already
available dynamic quirks the kernel allows to have? You can simply add
a configuration file to your system to locally have the dynamic quirk
set, and then it gets re-applied on every reboot.
As much as I'd like bpf to be the universal answer, this doesn't seem
to be a good replacement to what we currently have.
HID-BPF seems to be a good answer to replace (some) drivers, but I
don't think it should replace the generic kernel processing. So that's
why I don't think this is a good idea.
>
> Apologize again for this long email:
> I understand it's full of good intentions but without any
> significant contribution :(
No need to apologize. You are actually proposing ideas and your help
to make things better for end-users, which is extremely valuable in
itself :)
Cheers,
Benjamin
[0] https://gitlab.freedesktop.org/libevdev/hid-tools
^ 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