* Re: [PATCH v10 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-10-26 5:05 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, linux-input, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bastien Nocera, Hans de Goede, Henrik Rydberg,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-2-88eec2e51c0b@linaro.org>
Hi Neil,
On Mon, Oct 23, 2023 at 05:03:46PM +0200, Neil Armstrong wrote:
[...]
> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
> +{
> + __le16 length_raw;
> + u8 *afe_data;
> + u16 length;
> + int error;
> +
> + afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
> + if (!afe_data)
> + return -ENOMEM;
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + &length_raw, sizeof(length_raw));
> + if (error) {
> + dev_info(cd->dev, "failed get ic info length, %d\n", error);
This should be dev_err().
> + goto free_afe_data;
> + }
> +
> + length = le16_to_cpu(length_raw);
> + if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
> + dev_info(cd->dev, "invalid ic info length %d\n", length);
And here.
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
> + afe_data, length);
> + if (error) {
> + dev_info(cd->dev, "failed get ic info data, %d\n", error);
> + return error;
> + goto free_afe_data;
> + }
This return statement is left over from v9; the print should also be dev_err().
> +
> + /* check whether the data is valid (ex. bus default values) */
> + if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
> + dev_err(cd->dev, "fw info data invalid\n");
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + if (!goodix_berlin_checksum_valid(afe_data, length)) {
> + dev_info(cd->dev, "fw info checksum error\n");
And here.
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + error = goodix_berlin_convert_ic_info(cd, afe_data, length);
> + if (error)
> + goto free_afe_data;
> +
> + /* check some key info */
> + if (!cd->touch_data_addr) {
> + dev_err(cd->dev, "touch_data_addr is null\n");
> + error = -EINVAL;
> + goto free_afe_data;
> + }
> +
> + return 0;
> +
> +free_afe_data:
> + kfree(afe_data);
> +
> + return error;
> +}
[...]
> +static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
> +{
> + gpiod_set_value(cd->reset_gpio, 1);
> + usleep_range(2000, 2100);
> + gpiod_set_value(cd->reset_gpio, 0);
I see that now, this function is only called if the reset GPIO is defined,
but there used to be another msleep() here that has since been dropped. Is
that intentional?
> +
> + return 0;
> +}
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] input: gpio-keys - optimize wakeup sequence.
From: Abhishek Kumar Singh @ 2023-10-26 5:53 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: robh@kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, SRI-N IT Security,
Abhishek Kumar Singh
In-Reply-To: <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p4>
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
Dear Mr. Dmitry,
Greetings!
The patch removes unused many APIs call chain for every suspend/resume of the device
if no key press event triggered.
There is a call back function gpio_keys_resume() called for
every suspend/resume of the device. and whenever this function called, it is
reading the status of the key. And gpio_keys_resume() API further calls the
below chain of API irrespective of key press event
APIs call chain:
static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
gpiod_get_value_cansleep(bdata->gpiod);
input_event(input, type, *bdata->code, state);
input_sync(input);
The patch avoid the above APIs call chain if there is no key press event triggered.
It will save the device computational resources, power resources and optimize the suspend/resume time
Signed-off-by: Abhishek Kumar Singh <abhi1.singh@samsung.com>
---
linux-6.4.11/drivers/input/keyboard/gpio_keys.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
index 13a8ba5..cd1609e 100644
--- a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
+++ b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
@@ -687,8 +687,12 @@ static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
for (i = 0; i < ddata->pdata->nbuttons; i++) {
struct gpio_button_data *bdata = &ddata->data[i];
- if (bdata->gpiod)
- gpio_keys_gpio_report_event(bdata);
+ if (bdata->gpiod) {
+ struct gpio_keys_button *button = bdata->button;
+ if(button->value) {
+ gpio_keys_gpio_report_event(bdata);
+ }
+ }
}
input_sync(input);
}
---
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
[-- Attachment #2: input_keys_optimized.zip --]
[-- Type: application/octet-stream, Size: 998 bytes --]
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: Stop IO before calling hid_connect()
From: Benjamin Tissoires @ 2023-10-26 10:04 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, Hans de Goede
Cc: linux-input
In-Reply-To: <20231025190151.302376-1-hdegoede@redhat.com>
On Wed, 25 Oct 2023 21:01:51 +0200, Hans de Goede wrote:
> hid_connect() will call hid_pidff_init() which does
> hid_device_io_start() leading to an "io already started" warning.
>
> To fix this call hid_device_io_stop() before calling hid_connect(),
> stopping IO means that connect events may be lost while hid_connect()
> runs, re-enable IO and move the hidpp_connect_event() work queuing
> after the hid_connect().
>
> [...]
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.7/logitech), thanks!
[1/1] HID: logitech-hidpp: Stop IO before calling hid_connect()
https://git.kernel.org/hid/hid/c/3e6b0bb22a80
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
From: Jeff LaBundy @ 2023-10-26 14:46 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20230510-feature-ts_virtobj_patch-v5-2-ff6b5c4db693@wolfvision.net>
Hi Javier,
Thank you for continuing to drive this high-quality work.
On Tue, Oct 17, 2023 at 01:00:08PM +0200, Javier Carrasco wrote:
> The overlay-touchscreen object defines an area within the touchscreen
> where touch events are reported and their coordinates get converted to
> the overlay origin. This object avoids getting events from areas that
> are physically hidden by overlay frames.
>
> For touchscreens where overlay buttons on the touchscreen surface are
> provided, the overlay-buttons object contains a node for every button
> and the key event that should be reported when pressed.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
> .../bindings/input/touchscreen/touchscreen.yaml | 143 +++++++++++++++++++++
> 1 file changed, 143 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> index 431c13335c40..5c58eb79ee9a 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
> @@ -87,6 +87,129 @@ properties:
> touchscreen-y-plate-ohms:
> description: Resistance of the Y-plate in Ohms
>
> + overlay-touchscreen:
> + description: Clipped touchscreen area
> +
> + This object can be used to describe a frame that restricts the area
> + within touch events are reported, ignoring the events that occur outside
> + this area. This is of special interest if the touchscreen is shipped
> + with a physical overlay on top of it with a frame that hides some part
> + of the original touchscreen area.
> +
> + The x-origin and y-origin properties of this object define the offset of
> + a new origin from where the touchscreen events are referenced.
> + This offset is applied to the events accordingly. The x-size and y-size
> + properties define the size of the overlay-touchscreen (effective area).
> +
> + The following example shows the new touchscreen area and the new origin
> + (0',0') for the touch events generated by the device.
> +
> + Touchscreen (full area)
> + ┌────────────────────────────────────────┐
> + │ ┌───────────────────────────────┐ │
> + │ │ │ │
> + │ ├ y-size │ │
> + │ │ │ │
> + │ │ overlay-touchscreen │ │
> + │ │ │ │
> + │ │ │ │
> + │ │ x-size │ │
> + │ ┌└──────────────┴────────────────┘ │
> + │(0',0') │
> + ┌└────────────────────────────────────────┘
> + (0,0)
> +
> + where (0',0') = (0+x-origin,0+y-origin)
> +
> + type: object
> + $ref: '#/$defs/overlay-node'
> + unevaluatedProperties: false
> +
> + required:
> + - x-origin
> + - y-origin
> + - x-size
> + - y-size
> +
> + overlay-buttons:
> + description: list of nodes defining the buttons on the touchscreen
> +
> + This object can be used to describe buttons on the touchscreen area,
> + reporting the touch events on their surface as key events instead of
> + the original touch events.
> +
> + This is of special interest if the touchscreen is shipped with a
> + physical overlay on top of it where a number of buttons with some
> + predefined functionality are printed. In that case a specific behavior
> + is expected from those buttons instead of raw touch events.
> +
> + The overlay-buttons properties define a per-button area as well as an
> + origin relative to the real touchscreen origin. Touch events within the
> + button area are reported as the key event defined in the linux,code
> + property. Given that the key events do not provide coordinates, the
> + button origin is only used to place the button area on the touchscreen
> + surface. Any event outside the overlay-buttons object is reported as a
> + touch event with no coordinate transformation.
> +
> + The following example shows a touchscreen with a single button on it
> +
> + Touchscreen (full area)
> + ┌───────────────────────────────────┐
> + │ │
> + │ │
> + │ ┌─────────┐ │
> + │ │button 0 │ │
> + │ │KEY_POWER│ │
> + │ └─────────┘ │
> + │ │
> + │ │
> + ┌└───────────────────────────────────┘
> + (0,0)
> +
> + The overlay-buttons object can be combined with the overlay-touchscreen
> + object as shown in the following example. In that case only the events
> + within the overlay-touchscreen object are reported as touch events.
> +
> + Touchscreen (full area)
> + ┌─────────┬──────────────────────────────┐
> + │ │ │
> + │ │ ┌───────────────────────┐ │
> + │ button 0│ │ │ │
> + │KEY_POWER│ │ │ │
> + │ │ │ │ │
> + ├─────────┤ │ overlay-touchscreen │ │
> + │ │ │ │ │
> + │ │ │ │ │
> + │ button 1│ │ │ │
> + │ KEY_INFO│ ┌└───────────────────────┘ │
> + │ │(0',0') │
> + ┌└─────────┴──────────────────────────────┘
> + (0,0)
> +
> + type: object
I am still confused why the buttons need to live under an 'overlay-buttons'
parent node, which seems like an imaginary boundary. In my view, the touch
surface comprises the following types of rectangular areas:
1. A touchscreen, wherein granular coordinates and pressure are reported.
2. A momentary button, wherein pressure is quantized into a binary value
(press or release), and coordinates are ignored.
Any contact that falls outside of (1) and (2) is presumed to be part of a
border or matting, and is hence ignored.
Areas (1) and (2) exist in the same "plane", so why can they not reside
under the same parent node? The following seems much more representative
of the actual hardware we intend to describe in the device tree:
touchscreen {
compatible = "...";
reg = <...>;
/* raw coordinates reported here */
touch-area-1 {
x-origin = <...>;
y-origin = <...>;
x-size = <...>;
y-size = <...>;
};
/* a button */
touch-area-2a {
x-origin = <...>;
y-origin = <...>;
x-size = <...>;
y-size = <...>;
linux,code = <KEY_POWER>;
};
/* another button */
touch-area-2b {
x-origin = <...>;
y-origin = <...>;
x-size = <...>;
y-size = <...>;
linux,code = <KEY_INFO>;
};
};
With this method, the driver merely stores a list head. The parsing code
then walks the client device node; for each touch* child encountered, it
allocates memory for a structure of five members, and adds it to the list.
The event handling code then simply iterates through the list and checks
if the coordinates reported by the hardware fall within each rectangle. If
so, and the keycode in the list element is equal to KEY_RESERVED (zero),
we assume the rectangle is of type (1); the coordinates are passed to
touchscreen_report_pos() and the pressure is reported as well.
If the keycode is not equal to KEY_RESERVED (e.g. KEY_POWER), we assume
the rectangle is of type (2); input_report_key() is called instead and
the coordinates are ignored altogether.
Instead, the existing implementation has two separate structures, one of
which inherits the other. Its corresponding properties are then parsed in
a separate function to account for the fact that the two structures exist
at different layers in the heirarchy.
My argument is that all nodes can be parsed in the same way, without having
to understand whether they represent case (1) or (2). The existing method
also has to count the nodes; this would not be necessary with a linked list.
Can you help me understand why the buttons must be "wrapped" in their own
node? As I mention in v2, this isn't a deal breaker necessarily, but I'd
like to understand the reasoning behind what I interpret as additional
complexity, and a degree of separation from a natural description of the
touch surface.
The only reason I can think to insert the 'overlay-buttons' parent is in
case we want to specify a property within this node that applies to all
buttons, but this does not exist in the current implementation, nor do I
see a compelling reason to do so in the future.
> +
> + patternProperties:
> + '^button-':
> + type: object
> + description:
> + Each button (key) is represented as a sub-node.
> + $ref: '#/$defs/overlay-node'
> + unevaluatedProperties: false
> +
> + properties:
> + label:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: descriptive name of the button
> +
> + linux,code: true
> +
> + required:
> + - linux,code
> + - x-origin
> + - y-origin
> + - x-size
> + - y-size
> +
> dependencies:
> touchscreen-size-x: [ touchscreen-size-y ]
> touchscreen-size-y: [ touchscreen-size-x ]
> @@ -94,3 +217,23 @@ dependencies:
> touchscreen-y-mm: [ touchscreen-x-mm ]
>
> additionalProperties: true
> +
> +$defs:
> + overlay-node:
> + type: object
> + properties:
> + x-origin:
> + description: horizontal origin of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + y-origin:
> + description: vertical origin of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + x-size:
> + description: horizontal resolution of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + y-size:
> + description: vertical resolution of the node area
> + $ref: /schemas/types.yaml#/definitions/uint32
>
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v5 1/4] Input: touch-overlay - Add touchscreen overlay object handling
From: Jeff LaBundy @ 2023-10-26 15:00 UTC (permalink / raw)
To: Javier Carrasco
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <20230510-feature-ts_virtobj_patch-v5-1-ff6b5c4db693@wolfvision.net>
Hi Javier,
Just a few minor comments in addition to my over-arching question in
patch [2/4].
On Tue, Oct 17, 2023 at 01:00:07PM +0200, Javier Carrasco wrote:
> Some touch devices provide mechanical overlays with different objects
> like buttons or clipped touchscreen surfaces.
>
> In order to support these objects, add a series of helper functions
> to the input subsystem to transform them into overlay objects via
> device tree nodes.
>
> These overlay objects consume the raw touch events and report the
> expected input events depending on the object properties.
Normally binding patches precede the code that parses the corresponding
properties, so that the documentation is in the tree by the time the
code is applied.
Typically this only matters for compatible strings, since checkpatch
may complain about undocumented compatible strings when applying a
driver whose binding is not yet merged. That's obviously not the case
here, but I would say let's swap patches 1/2 and 3/4 for consistency.
>
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
> MAINTAINERS | 7 +
> drivers/input/Makefile | 2 +-
> drivers/input/touch-overlay.c | 399 ++++++++++++++++++++++++++++++++++++
> include/linux/input/touch-overlay.h | 34 +++
> 4 files changed, 441 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..00c03824c3ac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21884,6 +21884,13 @@ W: https://github.com/srcres258/linux-doc
> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
> F: Documentation/translations/zh_TW/
>
> +TOUCH OVERLAY OBJECTS
> +M: Javier Carrasco <javier.carrasco@wolfvision.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: drivers/input/touch-overlay.c
> +F: include/linux/input/touch-overlay.h
> +
> TTY LAYER AND SERIAL DRIVERS
> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> M: Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index c78753274921..393e9f4d00dc 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -7,7 +7,7 @@
>
> obj-$(CONFIG_INPUT) += input-core.o
> input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> -input-core-y += touchscreen.o
> +input-core-y += touchscreen.o touch-overlay.o
>
> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> obj-$(CONFIG_INPUT_SPARSEKMAP) += sparse-keymap.o
> diff --git a/drivers/input/touch-overlay.c b/drivers/input/touch-overlay.c
> new file mode 100644
> index 000000000000..007dbd994474
> --- /dev/null
> +++ b/drivers/input/touch-overlay.c
> @@ -0,0 +1,399 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Helper functions for overlay objects on touchscreens
> + *
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touch-overlay.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +
> +enum touch_overlay_valid_objects {
> + TOUCH_OVERLAY_TS,
> + TOUCH_OVERLAY_BTN,
> +};
> +
> +static const char *const object_names[] = {
> + [TOUCH_OVERLAY_TS] = "overlay-touchscreen",
> + [TOUCH_OVERLAY_BTN] = "overlay-buttons",
> +};
> +
> +struct touch_overlay_segment {
> + u32 x_origin;
> + u32 y_origin;
> + u32 x_size;
> + u32 y_size;
> +};
> +
> +struct touch_overlay_button {
> + struct touch_overlay_segment segment;
> + u32 key;
> + bool pressed;
> + int slot;
> +};
> +
> +static int touch_overlay_get_segment_props(struct fwnode_handle *segment_node,
> + struct touch_overlay_segment *segment)
> +{
> + int error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-origin",
> + &segment->x_origin);
> + if (error < 0)
It is sufficient, and much more common in input, to write this as 'if (error)'
as the only nonzero return values are negative anyway.
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-origin",
> + &segment->y_origin);
> + if (error < 0)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "x-size",
> + &segment->x_size);
> + if (error < 0)
> + return error;
> +
> + error = fwnode_property_read_u32(segment_node, "y-size",
> + &segment->y_size);
> + if (error < 0)
> + return error;
> +
> + return 0;
> +}
> +
> +static int
> +touch_overlay_get_button_properties(struct device *dev,
> + struct fwnode_handle *overlay_node,
> + struct touch_overlay_button *btn)
> +{
> + struct fwnode_handle *btn_node;
> + int error;
> + int j = 0;
> +
> + fwnode_for_each_child_node(overlay_node, btn_node) {
> + error = touch_overlay_get_segment_props(btn_node,
> + &btn[j].segment);
> + if (error < 0)
> + goto button_put;
> +
> + error = fwnode_property_read_u32(btn_node, "linux,code",
> + &btn[j].key);
> + if (error < 0)
> + goto button_put;
> +
> + dev_dbg(dev, "Added button at (%u, %u), size %ux%u, code=%u\n",
> + btn[j].segment.x_origin, btn[j].segment.y_origin,
> + btn[j].segment.x_size, btn[j].segment.y_size, btn[j].key);
> + j++;
> + }
> +
> + return 0;
> +
> +button_put:
> + fwnode_handle_put(btn_node);
> + return error;
> +}
> +
> +static void touch_overlay_set_button_caps(struct touch_overlay_map *map,
> + struct input_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i < map->button_count; i++)
> + input_set_capability(dev, EV_KEY, map->buttons[i].key);
> +}
> +
> +static int touch_overlay_count_buttons(struct device *dev)
> +{
> + struct fwnode_handle *overlay;
> + struct fwnode_handle *button;
> + int count = 0;
> +
> + overlay = device_get_named_child_node(dev,
> + object_names[TOUCH_OVERLAY_BTN]);
> + if (!overlay)
> + return 0;
> +
> + fwnode_for_each_child_node(overlay, button)
> + count++;
> + fwnode_handle_put(overlay);
> +
> + return count;
> +}
> +
> +static int touch_overlay_map_touchscreen(struct device *dev,
> + struct touch_overlay_map *map)
> +{
> + struct fwnode_handle *ts_node;
> + int error = 0;
> +
> + ts_node = device_get_named_child_node(dev,
> + object_names[TOUCH_OVERLAY_TS]);
> + if (!ts_node)
> + return 0;
> +
> + map->touchscreen =
> + devm_kzalloc(dev, sizeof(*map->touchscreen), GFP_KERNEL);
This line break was a bit confusing to read; the choice of line break in
touch_overlay_map_buttons() is much more clear.
> + if (!map->touchscreen) {
> + error = -ENOMEM;
> + goto handle_put;
> + }
> + error = touch_overlay_get_segment_props(ts_node, map->touchscreen);
> + if (error < 0)
> + goto handle_put;
> +
> + map->overlay_touchscreen = true;
> + dev_dbg(dev, "Added overlay touchscreen at (%u, %u), size %u x %u\n",
> + map->touchscreen->x_origin, map->touchscreen->y_origin,
> + map->touchscreen->x_size, map->touchscreen->y_size);
> +
> +handle_put:
> + fwnode_handle_put(ts_node);
> +
> + return error;
> +}
> +
> +static int touch_overlay_map_buttons(struct device *dev,
> + struct touch_overlay_map *map)
> +{
> + struct fwnode_handle *button;
> + u32 count;
> + int error = 0;
> +
> + count = touch_overlay_count_buttons(dev);
> + if (!count)
> + return 0;
> +
> + map->buttons = devm_kcalloc(dev, count,
> + sizeof(*map->buttons), GFP_KERNEL);
> + if (!map->buttons) {
> + error = -ENOMEM;
> + goto map_buttons_ret;
> + }
> + button = device_get_named_child_node(dev,
> + object_names[TOUCH_OVERLAY_BTN]);
> + if (unlikely(!button)) {
> + error = -ENODEV;
> + goto map_buttons_ret;
> + }
> +
> + error = touch_overlay_get_button_properties(dev, button,
> + map->buttons);
> +
> + if (error < 0)
> + goto map_buttons_put;
> +
> + map->button_count = count;
> +
> +map_buttons_put:
> + fwnode_handle_put(button);
> +map_buttons_ret:
> + return error;
> +}
> +
> +static bool touch_overlay_defined_objects(struct device *dev)
> +{
> + struct fwnode_handle *obj_node;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(object_names); i++) {
> + obj_node = device_get_named_child_node(dev, object_names[i]);
> + if (obj_node) {
> + fwnode_handle_put(obj_node);
> + return true;
> + }
> + fwnode_handle_put(obj_node);
> + }
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_map_overlay - map overlay objects from the device tree and set
> + * key capabilities if buttons are defined.
> + * @keypad: pointer to the already allocated input_dev.
> + *
> + * Returns a pointer to the object mapping struct.
> + *
> + * If a keypad input device is provided and overlay buttons are defined,
> + * its button capabilities are set accordingly.
> + */
> +struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad)
> +{
> + struct device *dev = keypad->dev.parent;
> + struct touch_overlay_map *map = NULL;
> + int error;
> +
> + if (!touch_overlay_defined_objects(dev))
> + return NULL;
> +
> + map = devm_kzalloc(dev, sizeof(*map), GFP_KERNEL);
> + if (!map) {
> + error = -ENOMEM;
> + goto map_error;
> + }
> + error = touch_overlay_map_touchscreen(dev, map);
> + if (error < 0)
> + goto map_error;
> +
> + error = touch_overlay_map_buttons(dev, map);
> + if (error < 0)
> + goto map_error;
> +
> + touch_overlay_set_button_caps(map, keypad);
> +
> + return map;
> +
> +map_error:
> + return ERR_PTR(error);
> +}
> +EXPORT_SYMBOL(touch_overlay_map_overlay);
> +
> +/**
> + * touch_overlay_get_touchscreen_abs - get abs size from the overlay node
> + * @map: pointer to the struct that holds the object mapping
> + * @x: horizontal abs
> + * @y: vertical abs
> + *
> + */
> +void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map, u16 *x,
> + u16 *y)
> +{
> + *x = map->touchscreen->x_size - 1;
> + *y = map->touchscreen->y_size - 1;
> +}
> +EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
> +
> +static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
> + u32 x, u32 y)
> +{
> + if (!seg)
> + return false;
> +
> + if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
> + y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_mapped_touchscreen - check if an overlay touchscreen is mapped
> + * @map: pointer to the struct that holds the object mapping
> + *
> + * Returns true if an overlay touchscreen is mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map)
> +{
> + if (!map || !map->overlay_touchscreen)
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
> +
> +/**
> + * touch_overlay_mapped_buttons - check if overlay buttons are mapped
> + * @map: pointer to the struct that holds the object mapping
> + *
> + * Returns true if overlay buttons mapped or false otherwise.
> + */
> +bool touch_overlay_mapped_buttons(struct touch_overlay_map *map)
> +{
> + if (!map || !map->button_count)
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(touch_overlay_mapped_buttons);
> +
> +static bool touch_overlay_mt_on_touchscreen(struct touch_overlay_map *map,
> + u32 *x, u32 *y)
> +{
> + bool contact = x && y;
> +
> + if (!touch_overlay_mapped_touchscreen(map))
> + return true;
> +
> + /* Let the caller handle events with no coordinates (release) */
> + if (!contact)
> + return false;
> +
> + if (touch_overlay_segment_event(map->touchscreen, *x, *y)) {
> + *x -= map->touchscreen->x_origin;
> + *y -= map->touchscreen->y_origin;
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static bool touch_overlay_button_event(struct input_dev *input,
> + struct touch_overlay_button *button,
> + const u32 *x, const u32 *y, u32 slot)
> +{
> + bool contact = x && y;
> +
> + if (!contact && button->pressed && button->slot == slot) {
> + button->pressed = false;
> + input_report_key(input, button->key, false);
> + input_sync(input);
> + return true;
> + } else if (contact && touch_overlay_segment_event(&button->segment,
> + *x, *y)) {
> + button->pressed = true;
> + button->slot = slot;
> + input_report_key(input, button->key, true);
> + input_sync(input);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * touch_overlay_process_event - process input events according to the overlay
> + * mapping. This function acts as a filter to release the calling driver from
> + * the events that are either related to overlay buttons or out of the overlay
> + * touchscreen area if defined.
> + * @map: pointer to the struct that holds the object mapping
> + * @input: pointer to the input device associated to the event
> + * @x: pointer to the x coordinate (NULL if not available - no contact)
> + * @y: pointer to the y coordinate (NULL if not available - no contact)
> + * @slot: slot associated to the event
> + *
> + * Returns true if the event was processed (reported for valid key events
> + * and dropped for events outside the overlay touchscreen area) or false
> + * if the event must be processed by the caller. In that case this function
> + * shifts the (x,y) coordinates to the overlay touchscreen axis if required
> + */
> +bool touch_overlay_process_event(struct touch_overlay_map *map,
> + struct input_dev *input,
> + u32 *x, u32 *y, u32 slot)
> +{
> + int i;
> +
> + if (!map)
> + return false;
> +
> + /* buttons must be prioritized over overlay touchscreens to account for
> + * overlappings e.g. a button inside the touchscreen area
> + */
Please refer to the kernel coding style guide regarding multi-line comments.
> + if (touch_overlay_mapped_buttons(map)) {
> + for (i = 0; i < map->button_count; i++) {
> + if (touch_overlay_button_event(input, &map->buttons[i],
> + x, y, slot))
> + return true;
> + }
> + }
> + /* valid touch events on the overlay touchscreen are left for the client
> + * to be processed/reported according to its (possibly) unique features
> + */
> + return !touch_overlay_mt_on_touchscreen(map, x, y);
> +}
> +EXPORT_SYMBOL(touch_overlay_process_event);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
> diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
> new file mode 100644
> index 000000000000..3e0db813dc34
> --- /dev/null
> +++ b/include/linux/input/touch-overlay.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@wolfvision.net>
> + */
> +
> +#ifndef _TOUCH_OVERLAY
> +#define _TOUCH_OVERLAY
> +
> +#include <linux/types.h>
> +
> +struct input_dev;
> +struct device;
> +
> +struct touch_overlay_map {
> + struct touch_overlay_segment *touchscreen;
> + bool overlay_touchscreen;
> + struct touch_overlay_button *buttons;
> + u32 button_count;
> +};
> +
> +struct touch_overlay_map *touch_overlay_map_overlay(struct input_dev *keypad);
> +
> +void touch_overlay_get_touchscreen_abs(struct touch_overlay_map *map,
> + u16 *x, u16 *y);
> +
> +bool touch_overlay_mapped_touchscreen(struct touch_overlay_map *map);
> +
> +bool touch_overlay_mapped_buttons(struct touch_overlay_map *map);
> +
> +bool touch_overlay_process_event(struct touch_overlay_map *map,
> + struct input_dev *input,
> + u32 *x, u32 *y, u32 slot);
> +
> +#endif
>
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v6 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-27 5:18 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel, Conor Dooley,
Krzysztof Kozlowski
Adds bindings for the Adafruit Seesaw Gamepad.
The gamepad functions as an i2c device with the default address of 0x50
and has an IRQ pin that can be enabled in the driver to allow for a rising
edge trigger on each button press or joystick movement.
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v6:
- no updates
Changes for v5:
- Added link to the datasheet
Changes for v4:
- Fixed the URI for the id field
- Added `interrupts` property
Changes for v3:
- Updated id field to reflect updated file name from previous version
- Added `reg` property
Changes for v2:
- Renamed file to `adafruit,seesaw-gamepad.yaml`
- Removed quotes for `$id` and `$schema`
- Removed "Bindings for" from the description
- Changed node name to the generic name "joystick"
- Changed compatible to 'adafruit,seesaw-gamepad' instead of
'adafruit,seesaw_gamepad'
.../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
new file mode 100644
index 000000000000..3f0d1c5a3b9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+description: |
+ Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit percision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw-gamepad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description:
+ The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ reg = <0x50>;
+ };
+ };
--
2.42.0
^ permalink raw reply related
* [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-27 5:18 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <20231027051819.81333-1-anshulusr@gmail.com>
Adds a driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Driver tested on RPi Zero 2W
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v6:
- Added TODO
- Removed `clang-format` directives
- Namespaced device buttons
- Removed `char physical_path[32]` field from `struct seesaw_gamepad`
- Added `packed` attribute to `struct seesaw_data`
- Moved from having booleans per button to single `u32 button_state`
- Added `seesaw_button_description` array to directly associate device
buttons with respective keycodes
- Added wrapper functions `seesaw_register_` around `i2c_master_` API
- Ratelimited input error reporting with `dev_err_ratelimited`
- Removed `of_device_id`
Changes for v5:
- Added link to the datasheet
- Added debug log message when `seesaw_read_data` fails
Changes for v4:
- Changed `1UL << BUTTON_` to BIT(BUTTON_)
- Removed `hardware_id` field from `struct seesaw_gamepad`
- Removed redundant checks for the number of bytes written and received by
`i2c_master_send` and `i2c_master_recv`
- Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
- Changed `result & (1UL << BUTTON_)` to
`test_bit(BUTTON_, (long *)&result)`
- Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
- Fixed formatting issues
- Changed button reporting:
Since the gamepad had the action buttons in a non-standard layout:
(X)
(Y) (A)
(B)
Therefore moved to using generic directional action button event codes
instead of BTN_[ABXY].
Changes for v3:
- no updates
Changes for v2:
adafruit-seesaw.c:
- Renamed file from 'adafruit_seesaw.c'
- Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
- Changed count parameter for receiving joystick x on line 118:
`2` to `sizeof(write_buf)`
- Fixed invalid buffer size on line 123 and 126:
`data->y` to `sizeof(data->y)`
- Added comment for the `mdelay(10)` on line 169
- Changed inconsistent indentation on line 271
Kconfig:
- Fixed indentation for the help text
- Updated module name
Makefile:
- Updated module object file name
MAINTAINERS:
- Updated file name for the driver and bindings
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
4 files changed, 327 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..0595c832c248 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c
+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <anshulusr@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
+F: drivers/input/joystick/adafruit-seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <jikos@kernel.org>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..df9cd1830b29 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.
+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit-seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..9976f596a920 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
new file mode 100644
index 000000000000..1aa6fbe4fda4
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ *
+ * TODO:
+ * - Add interrupt support
+ */
+
+#include <asm-generic/unaligned.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0
+#define SEESAW_GPIO_BASE 1
+#define SEESAW_ADC_BASE 9
+
+#define SEESAW_GPIO_DIRCLR_BULK 3
+#define SEESAW_GPIO_BULK 4
+#define SEESAW_GPIO_BULK_SET 5
+#define SEESAW_GPIO_PULLENSET 11
+
+#define SEESAW_STATUS_HW_ID 1
+#define SEESAW_STATUS_SWRST 127
+
+#define SEESAW_ADC_OFFSET 7
+
+#define SEESAW_BUTTON_A 5
+#define SEESAW_BUTTON_B 1
+#define SEESAW_BUTTON_X 6
+#define SEESAW_BUTTON_Y 2
+#define SEESAW_BUTTON_START 16
+#define SEESAW_BUTTON_SELECT 0
+
+#define SEESAW_ANALOG_X 14
+#define SEESAW_ANALOG_Y 15
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+
+u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
+ BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
+ BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ __be16 x;
+ __be16 y;
+ u32 button_state;
+} __packed;
+
+struct seesaw_button_description {
+ unsigned int code;
+ unsigned int bit;
+};
+
+static const struct seesaw_button_description seesaw_buttons[] = {
+ {
+ .code = BTN_EAST,
+ .bit = SEESAW_BUTTON_A,
+ },
+ {
+ .code = BTN_SOUTH,
+ .bit = SEESAW_BUTTON_B,
+ },
+ {
+ .code = BTN_NORTH,
+ .bit = SEESAW_BUTTON_X,
+ },
+ {
+ .code = BTN_WEST,
+ .bit = SEESAW_BUTTON_Y,
+ },
+ {
+ .code = BTN_START,
+ .bit = SEESAW_BUTTON_START,
+ },
+ {
+ .code = BTN_SELECT,
+ .bit = SEESAW_BUTTON_SELECT,
+ },
+};
+
+static int seesaw_register_read(struct i2c_client *client, u8 register_high,
+ u8 register_low, char *buf, int count)
+{
+ int ret;
+ u8 register_buf[2] = { register_high, register_low };
+
+ ret = i2c_master_send(client, register_buf, sizeof(register_buf));
+ if (ret < 0)
+ return ret;
+ ret = i2c_master_recv(client, buf, count);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
+ u8 register_low, u8 value)
+{
+ int ret;
+ u8 write_buf[3] = { register_high, register_low, value };
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u32(struct i2c_client *client,
+ u8 register_high, u8 register_low,
+ u32 value)
+{
+ int ret;
+ u8 write_buf[6] = { register_high, register_low };
+
+ put_unaligned_be32(value, write_buf + 2);
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int ret;
+ u8 read_buf[4];
+
+ ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
+ read_buf, sizeof(read_buf));
+ if (ret)
+ return ret;
+
+ data->button_state = ~get_unaligned_be32(&read_buf);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
+ (char *)&data->x, sizeof(data->x));
+ if (ret)
+ return ret;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
+ (char *)&data->y, sizeof(data->y));
+ if (ret)
+ return ret;
+ data->y = be16_to_cpu(data->y);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ int err, i;
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err != 0) {
+ dev_err_ratelimited(&input->dev,
+ "failed to read joystick state: %d\n", err);
+ return;
+ }
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+
+ for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
+ input_report_key(input, seesaw_buttons[i].code,
+ data.button_state &
+ BIT(seesaw_buttons[i].bit));
+ }
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int err, i;
+ u8 hardware_id;
+ struct seesaw_gamepad *seesaw;
+
+ err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF);
+ if (err)
+ return err;
+
+ /* Wait for the registers to reset before proceeding */
+ mdelay(10);
+
+ seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
+ if (!seesaw)
+ return -ENOMEM;
+
+ err = seesaw_register_read(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_HW_ID, &hardware_id, 1);
+ if (err)
+ return err;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ hardware_id);
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_DIRCLR_BULK,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_PULLENSET,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+ err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_BULK_SET,
+ SEESAW_BUTTON_MASK);
+ if (err)
+ return err;
+
+ seesaw->i2c_client = client;
+ i2c_set_clientdata(client, seesaw);
+
+ seesaw->input_dev = devm_input_allocate_device(&client->dev);
+ if (!seesaw->input_dev)
+ return -ENOMEM;
+
+ seesaw->input_dev->id.bustype = BUS_I2C;
+ seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
+ seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
+ input_set_drvdata(seesaw->input_dev, seesaw);
+ input_set_abs_params(seesaw->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
+ input_set_capability(seesaw->input_dev, EV_KEY,
+ seesaw_buttons[i].code);
+ }
+
+ err = input_setup_polling(seesaw->input_dev, seesaw_poll);
+ if (err) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", err);
+ return err;
+ }
+
+ input_set_poll_interval(seesaw->input_dev,
+ SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
+
+ err = input_register_device(seesaw->input_dev);
+ if (err) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id seesaw_id_table[] = {
+ { SEESAW_DEVICE_NAME, 0 },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-27 6:14 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <20231027051819.81333-2-anshulusr@gmail.com>
Hi Anshul,
thanks for the reworks!
Some more comments inline.
On 2023-10-27 10:48:11+0530, Anshul Dalal wrote:
> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> Changes for v6:
> - Added TODO
> - Removed `clang-format` directives
> - Namespaced device buttons
> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
> - Added `packed` attribute to `struct seesaw_data`
> - Moved from having booleans per button to single `u32 button_state`
> - Added `seesaw_button_description` array to directly associate device
> buttons with respective keycodes
> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
> - Ratelimited input error reporting with `dev_err_ratelimited`
> - Removed `of_device_id`
>
> Changes for v5:
> - Added link to the datasheet
> - Added debug log message when `seesaw_read_data` fails
>
> Changes for v4:
> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> - Removed `hardware_id` field from `struct seesaw_gamepad`
> - Removed redundant checks for the number of bytes written and received by
> `i2c_master_send` and `i2c_master_recv`
> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> - Changed `result & (1UL << BUTTON_)` to
> `test_bit(BUTTON_, (long *)&result)`
> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> - Fixed formatting issues
> - Changed button reporting:
> Since the gamepad had the action buttons in a non-standard layout:
> (X)
> (Y) (A)
> (B)
> Therefore moved to using generic directional action button event codes
> instead of BTN_[ABXY].
>
> Changes for v3:
> - no updates
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
> 4 files changed, 327 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
[..]
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..1aa6fbe4fda4
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + *
> + * TODO:
> + * - Add interrupt support
> + */
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define SEESAW_BUTTON_A 5
> +#define SEESAW_BUTTON_B 1
> +#define SEESAW_BUTTON_X 6
> +#define SEESAW_BUTTON_Y 2
> +#define SEESAW_BUTTON_START 16
> +#define SEESAW_BUTTON_SELECT 0
> +
> +#define SEESAW_ANALOG_X 14
> +#define SEESAW_ANALOG_Y 15
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32
> +
> +u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
> + BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
> + BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
> +
> +struct seesaw_gamepad {
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + __be16 x;
> + __be16 y;
The fact that these are big endian is now an implementation detail
hidden within seesaw_read_data(), so the __be16 can go away.
> + u32 button_state;
> +} __packed;
While this was requested by Jeff I don't think it's correct.
The struct is never received in this form from the device.
I guess he also got confused, like me, by the fact that data is directly
read into the fields of the struct.
See my comment seesaw_read_data().
> +struct seesaw_button_description {
> + unsigned int code;
> + unsigned int bit;
> +};
> +
> +static const struct seesaw_button_description seesaw_buttons[] = {
> + {
> + .code = BTN_EAST,
> + .bit = SEESAW_BUTTON_A,
> + },
> + {
> + .code = BTN_SOUTH,
> + .bit = SEESAW_BUTTON_B,
> + },
> + {
> + .code = BTN_NORTH,
> + .bit = SEESAW_BUTTON_X,
> + },
> + {
> + .code = BTN_WEST,
> + .bit = SEESAW_BUTTON_Y,
> + },
> + {
> + .code = BTN_START,
> + .bit = SEESAW_BUTTON_START,
> + },
> + {
> + .code = BTN_SELECT,
> + .bit = SEESAW_BUTTON_SELECT,
> + },
> +};
This looks very much like a sparse keymap which can be implemented with
the helpers from <linux/input/sparse-keymap.h>.
Personally I prefer each keymap entry to be on a single line (without
designated initializers, maybe with some alignment) to save a bunch of
vertical space.
> +
> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
> + u8 register_low, char *buf, int count)
> +{
> + int ret;
> + u8 register_buf[2] = { register_high, register_low };
> +
> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
> + if (ret < 0)
> + return ret;
> + ret = i2c_master_recv(client, buf, count);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
> + u8 register_low, u8 value)
> +{
> + int ret;
> + u8 write_buf[3] = { register_high, register_low, value };
> +
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u32(struct i2c_client *client,
> + u8 register_high, u8 register_low,
> + u32 value)
> +{
> + int ret;
> + u8 write_buf[6] = { register_high, register_low };
> +
> + put_unaligned_be32(value, write_buf + 2);
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int ret;
> + u8 read_buf[4];
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;
> +
> + data->button_state = ~get_unaligned_be32(&read_buf);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&data->x, sizeof(data->x));
Nitpick: read into a dedicated local variable instead of 'data', as it's
easier to understand.
> + if (ret)
> + return ret;
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&data->y, sizeof(data->y));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(data->y);
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + int err, i;
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err != 0) {
Everywhere else this is just 'if (err)'.
> + dev_err_ratelimited(&input->dev,
> + "failed to read joystick state: %d\n", err);
> + return;
> + }
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> +
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_report_key(input, seesaw_buttons[i].code,
> + data.button_state &
> + BIT(seesaw_buttons[i].bit));
> + }
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err, i;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (err)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
> + if (err)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> +
> + seesaw->i2c_client = client;
> + i2c_set_clientdata(client, seesaw);
I'm not familiar with the i2c APIs but this clientdata seems to be
unused.
> +
> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
> + if (!seesaw->input_dev)
> + return -ENOMEM;
> +
> + seesaw->input_dev->id.bustype = BUS_I2C;
> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
> + input_set_drvdata(seesaw->input_dev, seesaw);
> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_set_capability(seesaw->input_dev, EV_KEY,
> + seesaw_buttons[i].code);
> + }
> +
> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(seesaw->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
Why the linebreak on this line and not on the ones below?
> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(seesaw->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
^ permalink raw reply
* [PATCH RFC 07/11] input: docs: properly format ToC headings
From: Vegard Nossum @ 2023-10-27 8:18 UTC (permalink / raw)
To: linux-doc, Jonathan Corbet, Mauro Carvalho Chehab
Cc: Jani Nikula, linux-kernel, Vegard Nossum, Dmitry Torokhov,
linux-input
In-Reply-To: <20231027081830.195056-1-vegard.nossum@oracle.com>
"class:: toc-title" was a workaround for older Sphinx versions that are
no longer supported.
The canonical way to add a heading to the ToC is to use :caption:.
Do that.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
Documentation/input/input_kapi.rst | 5 +----
Documentation/input/input_uapi.rst | 5 +----
Documentation/input/joydev/index.rst | 5 +----
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/Documentation/input/input_kapi.rst b/Documentation/input/input_kapi.rst
index 41f1b7e6b78e..9937522daa9b 100644
--- a/Documentation/input/input_kapi.rst
+++ b/Documentation/input/input_kapi.rst
@@ -4,11 +4,8 @@
Linux Input Subsystem kernel API
################################
-.. class:: toc-title
-
- Table of Contents
-
.. toctree::
+ :caption: Table of Contents
:maxdepth: 2
:numbered:
diff --git a/Documentation/input/input_uapi.rst b/Documentation/input/input_uapi.rst
index 4a0391609327..8275b4223a84 100644
--- a/Documentation/input/input_uapi.rst
+++ b/Documentation/input/input_uapi.rst
@@ -4,11 +4,8 @@
Linux Input Subsystem userspace API
###################################
-.. class:: toc-title
-
- Table of Contents
-
.. toctree::
+ :caption: Table of Contents
:maxdepth: 2
:numbered:
diff --git a/Documentation/input/joydev/index.rst b/Documentation/input/joydev/index.rst
index ebcff43056e2..d03d6f6cbfab 100644
--- a/Documentation/input/joydev/index.rst
+++ b/Documentation/input/joydev/index.rst
@@ -6,11 +6,8 @@ Linux Joystick support
:Copyright: |copy| 1996-2000 Vojtech Pavlik <vojtech@ucw.cz> - Sponsored by SuSE
-.. class:: toc-title
-
- Table of Contents
-
.. toctree::
+ :caption: Table of Contents
:maxdepth: 3
joystick
--
2.34.1
^ permalink raw reply related
* [PATCH v4 1/4] pwm: rename pwm_apply_state() to pwm_apply_cansleep()
From: Sean Young @ 2023-10-27 9:20 UTC (permalink / raw)
To: linux-media, linux-pwm, Ivaylo Dimitrov, Thierry Reding,
Uwe Kleine-König, Jonathan Corbet, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Daniel Vetter, Javier Martinez Canillas, Jean Delvare,
Guenter Roeck, Support Opensource, Dmitry Torokhov, Pavel Machek,
Lee Jones, Sean Young, Mauro Carvalho Chehab, Hans de Goede,
Ilpo Järvinen, Mark Gross, Liam Girdwood, Mark Brown,
Daniel Thompson, Jingoo Han, Helge Deller
Cc: Jani Nikula, linux-doc, linux-kernel, intel-gfx, dri-devel,
linux-hwmon, linux-input, linux-leds, platform-driver-x86,
linux-arm-kernel, linux-fbdev
In-Reply-To: <cover.1698398004.git.sean@mess.org>
In order to introduce a pwm api which can be used from atomic context,
we will need two functions for applying pwm changes:
int pwm_apply_cansleep(struct pwm *, struct pwm_state *);
int pwm_apply_atomic(struct pwm *, struct pwm_state *);
This commit just deals with renaming pwm_apply_state(), a following
commit will introduce the pwm_apply_atomic() function.
Acked-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Sean Young <sean@mess.org>
---
Documentation/driver-api/pwm.rst | 8 +++---
.../gpu/drm/i915/display/intel_backlight.c | 6 ++--
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
drivers/hwmon/pwm-fan.c | 8 +++---
drivers/input/misc/da7280.c | 4 +--
drivers/input/misc/pwm-beeper.c | 4 +--
drivers/input/misc/pwm-vibra.c | 8 +++---
drivers/leds/leds-pwm.c | 2 +-
drivers/leds/rgb/leds-pwm-multicolor.c | 4 +--
drivers/media/rc/pwm-ir-tx.c | 4 +--
drivers/platform/x86/lenovo-yogabook.c | 2 +-
drivers/pwm/core.c | 18 ++++++------
drivers/pwm/pwm-twl-led.c | 2 +-
drivers/pwm/pwm-vt8500.c | 2 +-
drivers/pwm/sysfs.c | 10 +++----
drivers/regulator/pwm-regulator.c | 4 +--
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 6 ++--
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 28 +++++++++----------
21 files changed, 64 insertions(+), 64 deletions(-)
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index 3fdc95f7a1d1..ff1b73431b04 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -41,7 +41,7 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist.
After being requested, a PWM has to be configured using::
- int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+ int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
This API controls both the PWM period/duty_cycle config and the
enable/disable state.
@@ -57,13 +57,13 @@ If supported by the driver, the signal can be optimized, for example to improve
EMI by phase shifting the individual channels of a chip.
The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
-around pwm_apply_state() and should not be used if the user wants to change
+around pwm_apply_cansleep() and should not be used if the user wants to change
several parameter at once. For example, if you see pwm_config() and
pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+should switch to pwm_apply_cansleep().
The PWM user API also allows one to query the PWM state that was passed to the
-last invocation of pwm_apply_state() using pwm_get_state(). Note this is
+last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
different to what the driver has actually implemented if the request cannot be
satisfied exactly with the hardware in use. There is currently no way for
consumers to get the actually implemented settings.
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index 2e8f17c04522..cf516190cde8 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
intel_backlight_set_pwm_level(old_conn_state, level);
panel->backlight.pwm_state.enabled = false;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
panel->backlight.pwm_state.enabled = true;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
}
static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 5a80b228d18c..5045966d4303 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -267,7 +267,7 @@ static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
pwm_init_state(ssd130x->pwm, &pwmstate);
pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
- pwm_apply_state(ssd130x->pwm, &pwmstate);
+ pwm_apply_cansleep(ssd130x->pwm, &pwmstate);
/* Enable the PWM */
pwm_enable(ssd130x->pwm);
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 6e4516c2ab89..f68deb1f236b 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -151,7 +151,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
}
state->enabled = true;
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_cansleep(ctx->pwm, state);
if (ret) {
dev_err(ctx->dev, "failed to enable PWM\n");
goto disable_regulator;
@@ -181,7 +181,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
state->enabled = false;
state->duty_cycle = 0;
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_cansleep(ctx->pwm, state);
if (ret) {
dev_err(ctx->dev, "failed to disable PWM\n");
return ret;
@@ -207,7 +207,7 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
period = state->period;
state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
- ret = pwm_apply_state(ctx->pwm, state);
+ ret = pwm_apply_cansleep(ctx->pwm, state);
if (ret)
return ret;
ret = pwm_fan_power_on(ctx);
@@ -278,7 +278,7 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
state,
&enable_regulator);
- pwm_apply_state(ctx->pwm, state);
+ pwm_apply_cansleep(ctx->pwm, state);
pwm_fan_switch_power(ctx, enable_regulator);
pwm_fan_update_state(ctx, 0);
}
diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index ce82548916bb..f10be2cdba80 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -352,7 +352,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
state.duty_cycle = period_mag_multi;
}
- error = pwm_apply_state(haptics->pwm_dev, &state);
+ error = pwm_apply_cansleep(haptics->pwm_dev, &state);
if (error)
dev_err(haptics->dev, "Failed to apply pwm state: %d\n", error);
@@ -1175,7 +1175,7 @@ static int da7280_probe(struct i2c_client *client)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(haptics->pwm_dev, &state);
state.enabled = false;
- error = pwm_apply_state(haptics->pwm_dev, &state);
+ error = pwm_apply_cansleep(haptics->pwm_dev, &state);
if (error) {
dev_err(dev, "Failed to apply PWM state: %d\n", error);
return error;
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 1e731d8397c6..1d6c4fb5f0ca 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -39,7 +39,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
state.period = period;
pwm_set_relative_duty_cycle(&state, 50, 100);
- error = pwm_apply_state(beeper->pwm, &state);
+ error = pwm_apply_cansleep(beeper->pwm, &state);
if (error)
return error;
@@ -138,7 +138,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(beeper->pwm, &state);
state.enabled = false;
- error = pwm_apply_state(beeper->pwm, &state);
+ error = pwm_apply_cansleep(beeper->pwm, &state);
if (error) {
dev_err(dev, "failed to apply initial PWM state: %d\n",
error);
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index acac79c488aa..6552ce712d8d 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -56,7 +56,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
state.enabled = true;
- err = pwm_apply_state(vibrator->pwm, &state);
+ err = pwm_apply_cansleep(vibrator->pwm, &state);
if (err) {
dev_err(pdev, "failed to apply pwm state: %d\n", err);
return err;
@@ -67,7 +67,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
state.duty_cycle = vibrator->direction_duty_cycle;
state.enabled = true;
- err = pwm_apply_state(vibrator->pwm_dir, &state);
+ err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
if (err) {
dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
pwm_disable(vibrator->pwm);
@@ -160,7 +160,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(vibrator->pwm, &state);
state.enabled = false;
- err = pwm_apply_state(vibrator->pwm, &state);
+ err = pwm_apply_cansleep(vibrator->pwm, &state);
if (err) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
@@ -174,7 +174,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
/* Sync up PWM state and ensure it is off. */
pwm_init_state(vibrator->pwm_dir, &state);
state.enabled = false;
- err = pwm_apply_state(vibrator->pwm_dir, &state);
+ err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
if (err) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 419b710984ab..e1fe1fd8f189 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -54,7 +54,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
led_dat->pwmstate.duty_cycle = duty;
led_dat->pwmstate.enabled = duty > 0;
- return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
+ return pwm_apply_cansleep(led_dat->pwm, &led_dat->pwmstate);
}
__attribute__((nonnull))
diff --git a/drivers/leds/rgb/leds-pwm-multicolor.c b/drivers/leds/rgb/leds-pwm-multicolor.c
index 46cd062b8b24..8114adcdad9b 100644
--- a/drivers/leds/rgb/leds-pwm-multicolor.c
+++ b/drivers/leds/rgb/leds-pwm-multicolor.c
@@ -51,8 +51,8 @@ static int led_pwm_mc_set(struct led_classdev *cdev,
priv->leds[i].state.duty_cycle = duty;
priv->leds[i].state.enabled = duty > 0;
- ret = pwm_apply_state(priv->leds[i].pwm,
- &priv->leds[i].state);
+ ret = pwm_apply_cansleep(priv->leds[i].pwm,
+ &priv->leds[i].state);
if (ret)
break;
}
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index c5f37c03af9c..ccb86890adce 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
for (i = 0; i < count; i++) {
state.enabled = !(i % 2);
- pwm_apply_state(pwm, &state);
+ pwm_apply_cansleep(pwm, &state);
edge = ktime_add_us(edge, txbuf[i]);
delta = ktime_us_delta(edge, ktime_get());
@@ -77,7 +77,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
}
state.enabled = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_cansleep(pwm, &state);
return count;
}
diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo-yogabook.c
index b8d0239192cb..cbc285f77c2b 100644
--- a/drivers/platform/x86/lenovo-yogabook.c
+++ b/drivers/platform/x86/lenovo-yogabook.c
@@ -435,7 +435,7 @@ static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level)
.enabled = level,
};
- pwm_apply_state(data->kbd_bl_pwm, &state);
+ pwm_apply_cansleep(data->kbd_bl_pwm, &state);
gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0);
return 0;
}
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc66e3405bf5..423114cee430 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -382,8 +382,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
}
EXPORT_SYMBOL_GPL(pwm_request_from_chip);
-static void pwm_apply_state_debug(struct pwm_device *pwm,
- const struct pwm_state *state)
+static void pwm_apply_debug(struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pwm_state *last = &pwm->last;
struct pwm_chip *chip = pwm->chip;
@@ -489,11 +489,11 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
}
/**
- * pwm_apply_state() - atomically apply a new state to a PWM device
+ * pwm_apply_cansleep() - atomically apply a new state to a PWM device
* @pwm: PWM device
* @state: new state to apply
*/
-int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+int pwm_apply_cansleep(struct pwm_device *pwm, const struct pwm_state *state)
{
struct pwm_chip *chip;
int err;
@@ -501,7 +501,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
/*
* Some lowlevel driver's implementations of .apply() make use of
* mutexes, also with some drivers only returning when the new
- * configuration is active calling pwm_apply_state() from atomic context
+ * configuration is active calling pwm_apply_cansleep() from atomic context
* is a bad idea. So make it explicit that calling this function might
* sleep.
*/
@@ -531,11 +531,11 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
* only do this after pwm->state was applied as some
* implementations of .get_state depend on this
*/
- pwm_apply_state_debug(pwm, state);
+ pwm_apply_debug(pwm, state);
return 0;
}
-EXPORT_SYMBOL_GPL(pwm_apply_state);
+EXPORT_SYMBOL_GPL(pwm_apply_cansleep);
/**
* pwm_capture() - capture and report a PWM signal
@@ -593,7 +593,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
state.period = pargs.period;
state.polarity = pargs.polarity;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_cansleep(pwm, &state);
}
/*
@@ -616,7 +616,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
state.duty_cycle = state.period - state.duty_cycle;
}
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_cansleep(pwm, &state);
}
EXPORT_SYMBOL_GPL(pwm_adjust_config);
diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 8fb84b441853..a1fc2fa0d03e 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -172,7 +172,7 @@ static int twl4030_pwmled_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* We cannot skip calling ->config even if state->period ==
* pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
* because we might have exited early in the last call to
- * pwm_apply_state because of !state->enabled and so the two values in
+ * pwm_apply_cansleep because of !state->enabled and so the two values in
* pwm->state might not be configured in hardware.
*/
ret = twl4030_pwmled_config(pwm->chip, pwm,
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 6d46db51daac..3a815dfbf31c 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -206,7 +206,7 @@ static int vt8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
* We cannot skip calling ->config even if state->period ==
* pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
* because we might have exited early in the last call to
- * pwm_apply_state because of !state->enabled and so the two values in
+ * pwm_apply_cansleep because of !state->enabled and so the two values in
* pwm->state might not be configured in hardware.
*/
err = vt8500_pwm_config(pwm->chip, pwm, state->duty_cycle, state->period);
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 8d1254761e4d..eca9cad3be76 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -62,7 +62,7 @@ static ssize_t period_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.period = val;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_cansleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -97,7 +97,7 @@ static ssize_t duty_cycle_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.duty_cycle = val;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_cansleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -144,7 +144,7 @@ static ssize_t enable_store(struct device *child,
goto unlock;
}
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_cansleep(pwm, &state);
unlock:
mutex_unlock(&export->lock);
@@ -194,7 +194,7 @@ static ssize_t polarity_store(struct device *child,
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
state.polarity = polarity;
- ret = pwm_apply_state(pwm, &state);
+ ret = pwm_apply_cansleep(pwm, &state);
mutex_unlock(&export->lock);
return ret ? : size;
@@ -401,7 +401,7 @@ static int pwm_class_apply_state(struct pwm_export *export,
struct pwm_device *pwm,
struct pwm_state *state)
{
- int ret = pwm_apply_state(pwm, state);
+ int ret = pwm_apply_cansleep(pwm, state);
/* release lock taken in pwm_class_get_state */
mutex_unlock(&export->lock);
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 2aff6db748e2..c19d37a479d4 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -90,7 +90,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
pwm_set_relative_duty_cycle(&pstate,
drvdata->duty_cycle_table[selector].dutycycle, 100);
- ret = pwm_apply_state(drvdata->pwm, &pstate);
+ ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
if (ret) {
dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
return ret;
@@ -216,7 +216,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
- ret = pwm_apply_state(drvdata->pwm, &pstate);
+ ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
if (ret) {
dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
return ret;
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 8fcb62be597b..5cb702989ef6 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -180,7 +180,7 @@ static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
- return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
+ return pwm_apply_cansleep(pchip->pwmd, &pchip->pwmd_state);
}
/* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index da1f124db69c..b7edbaaa169a 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -234,7 +234,7 @@ static int lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
state.duty_cycle = div_u64(br * state.period, max_br);
state.enabled = state.duty_cycle;
- return pwm_apply_state(lp->pwm, &state);
+ return pwm_apply_cansleep(lp->pwm, &state);
}
static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index a51fbab96368..f2568aaae476 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -103,7 +103,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
pwm_get_state(pb->pwm, &state);
state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
state.enabled = true;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_cansleep(pb->pwm, &state);
pwm_backlight_power_on(pb);
} else {
@@ -120,7 +120,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
* inactive output.
*/
state.enabled = !pb->power_supply && !pb->enable_gpio;
- pwm_apply_state(pb->pwm, &state);
+ pwm_apply_cansleep(pb->pwm, &state);
}
if (pb->notify_after)
@@ -528,7 +528,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (!state.period && (data->pwm_period_ns > 0))
state.period = data->pwm_period_ns;
- ret = pwm_apply_state(pb->pwm, &state);
+ ret = pwm_apply_cansleep(pb->pwm, &state);
if (ret) {
dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
ret);
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 5ae48e36fccb..e5cca01af55f 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -347,7 +347,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
pwm_init_state(par->pwm, &pwmstate);
pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
- pwm_apply_state(par->pwm, &pwmstate);
+ pwm_apply_cansleep(par->pwm, &pwmstate);
/* Enable the PWM */
pwm_enable(par->pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d2f9f690a9c1..af081cdbb77d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -95,8 +95,8 @@ struct pwm_device {
* @state: state to fill with the current PWM state
*
* The returned PWM state represents the state that was applied by a previous call to
- * pwm_apply_state(). Drivers may have to slightly tweak that state before programming it to
- * hardware. If pwm_apply_state() was never called, this returns either the current hardware
+ * pwm_apply_cansleep(). Drivers may have to slightly tweak that state before programming it to
+ * hardware. If pwm_apply_cansleep() was never called, this returns either the current hardware
* state (if supported) or the default settings.
*/
static inline void pwm_get_state(const struct pwm_device *pwm,
@@ -160,20 +160,20 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
}
/**
- * pwm_init_state() - prepare a new state to be applied with pwm_apply_state()
+ * pwm_init_state() - prepare a new state to be applied with pwm_apply_cansleep()
* @pwm: PWM device
* @state: state to fill with the prepared PWM state
*
* This functions prepares a state that can later be tweaked and applied
- * to the PWM device with pwm_apply_state(). This is a convenient function
+ * to the PWM device with pwm_apply_cansleep(). This is a convenient function
* that first retrieves the current PWM state and the replaces the period
* and polarity fields with the reference values defined in pwm->args.
* Once the function returns, you can adjust the ->enabled and ->duty_cycle
- * fields according to your needs before calling pwm_apply_state().
+ * fields according to your needs before calling pwm_apply_cansleep().
*
* ->duty_cycle is initially set to zero to avoid cases where the current
* ->duty_cycle value exceed the pwm_args->period one, which would trigger
- * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle
+ * an error if the user calls pwm_apply_cansleep() without adjusting ->duty_cycle
* first.
*/
static inline void pwm_init_state(const struct pwm_device *pwm,
@@ -229,7 +229,7 @@ pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
*
* pwm_init_state(pwm, &state);
* pwm_set_relative_duty_cycle(&state, 50, 100);
- * pwm_apply_state(pwm, &state);
+ * pwm_apply_cansleep(pwm, &state);
*
* This functions returns -EINVAL if @duty_cycle and/or @scale are
* inconsistent (@scale == 0 or @duty_cycle > @scale).
@@ -309,7 +309,7 @@ struct pwm_chip {
#if IS_ENABLED(CONFIG_PWM)
/* PWM user APIs */
-int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
+int pwm_apply_cansleep(struct pwm_device *pwm, const struct pwm_state *state);
int pwm_adjust_config(struct pwm_device *pwm);
/**
@@ -337,7 +337,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
state.duty_cycle = duty_ns;
state.period = period_ns;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_cansleep(pwm, &state);
}
/**
@@ -358,7 +358,7 @@ static inline int pwm_enable(struct pwm_device *pwm)
return 0;
state.enabled = true;
- return pwm_apply_state(pwm, &state);
+ return pwm_apply_cansleep(pwm, &state);
}
/**
@@ -377,7 +377,7 @@ static inline void pwm_disable(struct pwm_device *pwm)
return;
state.enabled = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_cansleep(pwm, &state);
}
/* PWM provider APIs */
@@ -408,8 +408,8 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
struct fwnode_handle *fwnode,
const char *con_id);
#else
-static inline int pwm_apply_state(struct pwm_device *pwm,
- const struct pwm_state *state)
+static inline int pwm_apply_cansleep(struct pwm_device *pwm,
+ const struct pwm_state *state)
{
might_sleep();
return -ENOTSUPP;
@@ -536,7 +536,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
state.period = pwm->args.period;
state.usage_power = false;
- pwm_apply_state(pwm, &state);
+ pwm_apply_cansleep(pwm, &state);
}
struct pwm_lookup {
--
2.42.0
^ permalink raw reply related
* [PATCH] Input: synaptics-rmi4 - fix use after free in rmi_unregister_function()
From: Dan Carpenter @ 2023-10-27 12:18 UTC (permalink / raw)
To: Nick Dyer
Cc: Dmitry Torokhov, Jiapeng Chong, Christopher Heiny, linux-input,
kernel-janitors
The put_device() calls rmi_release_function() which frees "fn" so the
dereference on the next line "fn->num_of_irqs" is a use after free.
Move the put_device() to the end to fix this.
Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/input/rmi4/rmi_bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index f2e093b0b998..1b45b1d3077d 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -277,11 +277,11 @@ void rmi_unregister_function(struct rmi_function *fn)
device_del(&fn->dev);
of_node_put(fn->dev.of_node);
- put_device(&fn->dev);
for (i = 0; i < fn->num_of_irqs; i++)
irq_dispose_mapping(fn->irq[i]);
+ put_device(&fn->dev);
}
/**
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap
From: Dmitry Torokhov @ 2023-10-27 19:59 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <20231025013939.353553-1-james.hilliard1@gmail.com>
Hi James,
On Tue, Oct 24, 2023 at 07:39:38PM -0600, James Hilliard wrote:
> The vendor cyttsp5 driver does not use regmap for i2c support, it
> would appear this is due to regmap not providing sufficient levels
> of control to handle various error conditions that may be present
> under some configuration/firmware variants.
>
> To improve reliability lets refactor the cyttsp5 i2c interface to
> function more like the vendor driver and implement some of the error
> handling retry/recovery techniques present there.
Sorry but you need to elaborate more on what is missing in regmap and
how vendor code is better. In my experience vendors rarely follow kernel
development and either are not aware of the latest kernel APIs, or they
simply have the driver written to what we had in 3.x kernels and have
not really updated it since then.
>
> As part of this rather than assuming the device is in bootloader mode
> we should first check that the device is in bootloader and only
> attempt to launch the app if it actually is in the bootloader.
I would prefer if this was split into a separate patch.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Dell Pro Wireless Keyboard and Mouse KM5221W require HID_QUIRK_ALWAYS_POLL patch
From: Robert Ayrapetyan @ 2023-10-27 20:45 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, benjamin.tissoires
In-Reply-To: <nycvar.YFH.7.76.2310271533070.3534@cbobk.fhfr.pm>
Thanks!
On Fri, Oct 27, 2023 at 6:33 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 17 Aug 2023, Robert Ayrapetyan wrote:
>
> > Dell Pro Wireless Keyboard and Mouse KM5221W constantly reconnect with a
> > following error messages:
>
> This totally fell in between cracks, sorry for that. I have now applied
> the patch below, thanks a lot for the report.
>
> From: Jiri Kosina <jkosina@suse.cz>
> Date: Fri, 27 Oct 2023 15:32:09 +0200
> Subject: [PATCH] HID: Add quirk for Dell Pro Wireless Keyboard and Mouse KM5221W
>
> This device needs ALWAYS_POLL quirk, otherwise it keeps reconnecting
> indefinitely.
>
> Reported-by: Robert Ayrapetyan <robert.ayrapetyan@gmail.com>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-quirks.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index e4d2dfd5d253..f7973ccd84a2 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -366,6 +366,7 @@
>
> #define USB_VENDOR_ID_DELL 0x413c
> #define USB_DEVICE_ID_DELL_PIXART_USB_OPTICAL_MOUSE 0x301a
> +#define USB_DEVICE_ID_DELL_PRO_WIRELESS_KM5221W 0x4503
>
> #define USB_VENDOR_ID_DELORME 0x1163
> #define USB_DEVICE_ID_DELORME_EARTHMATE 0x0100
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 3983b4f282f8..5a48fcaa32f0 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -66,6 +66,7 @@ static const struct hid_device_id hid_quirks[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_STRAFE), HID_QUIRK_NO_INIT_REPORTS | HID_QUIRK_ALWAYS_POLL },
> { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_CREATIVE_SB_OMNI_SURROUND_51), HID_QUIRK_NOGET },
> { HID_USB_DEVICE(USB_VENDOR_ID_DELL, USB_DEVICE_ID_DELL_PIXART_USB_OPTICAL_MOUSE), HID_QUIRK_ALWAYS_POLL },
> + { HID_USB_DEVICE(USB_VENDOR_ID_DELL, USB_DEVICE_ID_DELL_PRO_WIRELESS_KM5221W), HID_QUIRK_ALWAYS_POLL },
> { HID_USB_DEVICE(USB_VENDOR_ID_DMI, USB_DEVICE_ID_DMI_ENC), HID_QUIRK_NOGET },
> { HID_USB_DEVICE(USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_2NES2SNES), HID_QUIRK_MULTI_INPUT },
> { HID_USB_DEVICE(USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_4NES4SNES), HID_QUIRK_MULTI_INPUT },
>
>
> --
> Jiri Kosina
> SUSE Labs
>
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: touchscreen: Add Novatek NT519XX series bindings
From: Krzysztof Kozlowski @ 2023-10-28 8:50 UTC (permalink / raw)
To: Wei-Shih Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20231025082054.1190-2-Weishih_Lin@novatek.com.tw>
On 25/10/2023 10:20, Wei-Shih Lin wrote:
> This patch adds device tree bindings for Novatek NT519XX series
> touchscreen devices.
>
> Signed-off-by: Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
> ---
> .../input/touchscreen/novatek,nt519xx.yaml | 60 +++++++++++++++++++
> MAINTAINERS | 9 +++
> 2 files changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
> new file mode 100644
> index 000000000000..00912e265197
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/novatek,nt519xx.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/novatek,nt519xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Novatek nt519xx touchscreen controller bindings
Except that it was never tested...
> +
> +maintainers:
> + - Wei-Shih Lin <Weishih_Lin@novatek.com.tw>
> + - Leo LS Chang <Leo_LS_Chang@novatek.com.tw>
> +
> +allOf:
> + - $ref: touchscreen.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - novatek,NVT-ts
That's not a real compatible. Nope. Open existing code to find examples.
> +
> + reg:
> + maxItems: 1
> +
> + novatek,irq-gpio:
No drop. Use interrupts
> + maxItems: 1
> +
> + novatek,reset-gpio:
> + maxItems: 1
Really, please start from scratch from existing, recent bindings. This
must be generic reset-gpios.
> +
> + touchscreen-size-x: true
> + touchscreen-size-y: true
> +
> +required:
> + - compatible
> + - reg
> + - novatek,irq-gpio
> + - novatek,reset-gpio
> + - touchscreen-size-x
> + - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> + i2c {
> + novatek@62 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/2] Input: Add driver for Novatek NT519XX series touchscreen devices
From: Krzysztof Kozlowski @ 2023-10-28 8:52 UTC (permalink / raw)
To: Wei-Shih Lin, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20231025082054.1190-3-Weishih_Lin@novatek.com.tw>
On 25/10/2023 10:20, Wei-Shih Lin wrote:
> This patch adds support for Novatek NT519XX series touchscreen devices.
> Existing Novatek touchscreen driver code developed for Acer Iconia One 7
> B1-750 tablet with Novatek IC NT11205 is novatek-nvt-ts.c in the path
> drivers/input/touchscreen/. However, this patch supports touch features
> for automotive display with Novatek TDDI NT519XX.
>
> +
> +static int32_t nvt_ts_resume(struct device *dev)
> +{
> + if (bTouchIsAwake) {
> + NVT_LOG("Touch is already resume.\n");
> + return 0;
> + }
> +
> + mutex_lock(&ts->lock);
> +
> + NVT_LOG("start\n");
Sorry, you cannot have such silly debugs.
> +
> + nvt_bootloader_reset();
> + nvt_check_fw_reset_state(RESET_STATE_NORMAL_RUN);
> +
> + nvt_irq_enable(true);
> +
> + bTouchIsAwake = 1;
> +
> + mutex_unlock(&ts->lock);
> +
> + NVT_LOG("end\n");
....
> +#endif
> +
> +static struct i2c_driver nvt_i2c_driver = {
> + .probe = nvt_ts_probe,
> + .remove = nvt_ts_remove,
> + .shutdown = nvt_ts_shutdown,
> + .id_table = nvt_ts_id,
> + .driver = {
> + .name = NVT_I2C_NAME,
> + .owner = THIS_MODULE,
Drop
> +#ifdef CONFIG_OF
Nope, drop
> + .of_match_table = nvt_match_table,
> +#endif
> + },
> +};
> +
> +static int32_t __init nvt_driver_init(void)
> +{
> + int32_t ret = 0;
> +
> + NVT_LOG("start\n");
Drop entire init. Open existing code and use it as template.
> +
> + bTouchIsAwake = 0;
> +
> + ret = i2c_add_driver(&nvt_i2c_driver);
> + if (ret) {
> + NVT_ERR("Failed to add i2c driver!");
> + goto err_driver;
> + }
> +
> + NVT_LOG("end\n");
> +
> +err_driver:
> + return ret;
> +}
> +
> +static void __exit nvt_driver_exit(void)
> +{
> + i2c_del_driver(&nvt_i2c_driver);
> +}
> +
> +module_init(nvt_driver_init);
> +module_exit(nvt_driver_exit);
> +
> +MODULE_DESCRIPTION("Novatek Touchscreen Driver");
> +MODULE_LICENSE("GPL");
This driver has very poor quality. Pointing issues here would be even
too much work. Please start from scratch from existing, accepted and
reviewed driver and customize it for your needs. You will notice that it
does not have all this weird code you put here.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 3/7] Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
From: Pavel Machek @ 2023-10-28 10:40 UTC (permalink / raw)
To: Caleb Connolly
Cc: Dmitry Torokhov, Vincent Huang, methanal, linux-input, devicetree,
phone-devel, ~postmarketos/upstreaming
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-3-cc3c703f022d@linaro.org>
Hi!
> Some replacement displays include third-party touch ICs which are
> devoid of register descriptors. Create a fake data register descriptor
> for such ICs and provide hardcoded default values.
>
> It isn't possible to reliably determine if the touch IC is original or
> not, so these fallback values are offered as an alternative to the error
> path when register descriptors aren't available.
>
> Signed-off-by: methanal <baclofen@tuta.io>
I guess we should have full/real name here.
Best regards,
Pavel
--
^ permalink raw reply
* Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
From: dmitry.torokhov @ 2023-10-29 2:11 UTC (permalink / raw)
To: Abhishek Kumar Singh
Cc: robh@kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, SRI-N IT Security
In-Reply-To: <899376598.3134980.1698299600677@mail-kr5-0.mail-kr5.knoxportal-kr-prod-blue.svc.cluster.local>
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - fix use after free in rmi_unregister_function()
From: Dmitry Torokhov @ 2023-10-29 2:54 UTC (permalink / raw)
To: Dan Carpenter
Cc: Nick Dyer, Jiapeng Chong, Christopher Heiny, linux-input,
kernel-janitors
In-Reply-To: <706efd36-7561-42f3-adfa-dd1d0bd4f5a1@moroto.mountain>
On Fri, Oct 27, 2023 at 03:18:28PM +0300, Dan Carpenter wrote:
> The put_device() calls rmi_release_function() which frees "fn" so the
> dereference on the next line "fn->num_of_irqs" is a use after free.
> Move the put_device() to the end to fix this.
>
> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] Input: Add driver for Novatek NT519XX series touchscreen devices
From: Dmitry Torokhov @ 2023-10-29 3:26 UTC (permalink / raw)
To: Wei-Shih Lin
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, linux-input,
devicetree, linux-kernel
In-Reply-To: <20231025082054.1190-3-Weishih_Lin@novatek.com.tw>
Hi Wei-Shih,
On Wed, Oct 25, 2023 at 04:20:54PM +0800, Wei-Shih Lin wrote:
> This patch adds support for Novatek NT519XX series touchscreen devices.
> Existing Novatek touchscreen driver code developed for Acer Iconia One 7
> B1-750 tablet with Novatek IC NT11205 is novatek-nvt-ts.c in the path
> drivers/input/touchscreen/. However, this patch supports touch features
> for automotive display with Novatek TDDI NT519XX.
How different the protocol of this part from NT11205? Can the existing
driver be modified to support both variants?
You already got feedback from Krzysztof, on top of his, if we want to
continue with a separate driver:
- it should use standard device properties
- it should use gpiod API
- it looks like it will benefit of regmap's paging support
- helpers like nvt_irq_enable() should not be used - your code should
know whether itq is enabled or disabled at all times
- all caps are reserved for macros (CTP_I2C_WRITE and others)
- I am sure we have crc8 helpers in the kernel
- please use u8, u16, etc in the kernel code instead of uint8_t,
uint16_t
- your driver will likely benefit from devm APIs
- no compile-time conditionals like "#if TOUCH_MAX_FINGER_NUM > 1"
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap
From: Dmitry Torokhov @ 2023-10-29 3:34 UTC (permalink / raw)
To: James Hilliard; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <CADvTj4oH=3Q3EShC-FM9ob7EnvFe4t2LHyDEwr-e7=G8M=UzYg@mail.gmail.com>
On Sat, Oct 28, 2023 at 03:31:00AM -0600, James Hilliard wrote:
> On Fri, Oct 27, 2023 at 1:59 PM Dmitry Torokhov <dmitry.torokhov@gmail.com>
> wrote:
>
> > Hi James,
> >
> > On Tue, Oct 24, 2023 at 07:39:38PM -0600, James Hilliard wrote:
> > > The vendor cyttsp5 driver does not use regmap for i2c support, it
> > > would appear this is due to regmap not providing sufficient levels
> > > of control to handle various error conditions that may be present
> > > under some configuration/firmware variants.
> > >
> > > To improve reliability lets refactor the cyttsp5 i2c interface to
> > > function more like the vendor driver and implement some of the error
> > > handling retry/recovery techniques present there.
> >
> > Sorry but you need to elaborate more on what is missing in regmap and
> > how vendor code is better. In my experience vendors rarely follow kernel
> > development and either are not aware of the latest kernel APIs, or they
> > simply have the driver written to what we had in 3.x kernels and have
> > not really updated it since then.
> >
>
> I'm unaware of a way to do essentially raw reads when using regmap, for
> example I don't know of a way to implement the cyttsp5_deassert_read
> function using regmap, maybe there's a way I'm not aware of however?
What is wrong with current way of reading from the input register? It
should clear the interrupt line.
>
> In general the issue with regmap seems to be that regmap always does
> operations against specific registers and prevents doing raw i2c operations
> needed to handle some hardware/firmware issues for some variants.
What are those issues and why do they need raw access.
>
> Note that I'm not exactly doing things the same way the vendor driver does,
> I have simplified the error recovery/retry code paths in the startup
> function.
>
>
> >
> > >
> > > As part of this rather than assuming the device is in bootloader mode
> > > we should first check that the device is in bootloader and only
> > > attempt to launch the app if it actually is in the bootloader.
> >
> > I would prefer if this was split into a separate patch.
> >
>
> I think this change is somewhat intertwined with the probe retry/recovery
> logic
> changes and is a bit tricky to split out without breaking the startup
> sequence
> from my testing at least.
I understand that it might be tricky but each logical change should
stand on its own.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap
From: James Hilliard @ 2023-10-29 9:05 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linus Walleij, linux-kernel
In-Reply-To: <ZT3S43_eMdwHWu2u@google.com>
On Sat, Oct 28, 2023 at 9:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sat, Oct 28, 2023 at 03:31:00AM -0600, James Hilliard wrote:
> > On Fri, Oct 27, 2023 at 1:59 PM Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > wrote:
> >
> > > Hi James,
> > >
> > > On Tue, Oct 24, 2023 at 07:39:38PM -0600, James Hilliard wrote:
> > > > The vendor cyttsp5 driver does not use regmap for i2c support, it
> > > > would appear this is due to regmap not providing sufficient levels
> > > > of control to handle various error conditions that may be present
> > > > under some configuration/firmware variants.
> > > >
> > > > To improve reliability lets refactor the cyttsp5 i2c interface to
> > > > function more like the vendor driver and implement some of the error
> > > > handling retry/recovery techniques present there.
> > >
> > > Sorry but you need to elaborate more on what is missing in regmap and
> > > how vendor code is better. In my experience vendors rarely follow kernel
> > > development and either are not aware of the latest kernel APIs, or they
> > > simply have the driver written to what we had in 3.x kernels and have
> > > not really updated it since then.
> > >
> >
> > I'm unaware of a way to do essentially raw reads when using regmap, for
> > example I don't know of a way to implement the cyttsp5_deassert_read
> > function using regmap, maybe there's a way I'm not aware of however?
>
> What is wrong with current way of reading from the input register? It
> should clear the interrupt line.
It doesn't seem to work reliably, for example I would often see this error:
[ 2.234089] cyttsp5 2-0024: HID output cmd execution timed out
[ 2.239957] cyttsp5 2-0024: Error on launch app r=-110
[ 2.245150] cyttsp5 2-0024: Fail initial startup r=-110
[ 2.257502] cyttsp5: probe of 2-0024 failed with error -110
Note that cyttsp5_hid_output_bl_launch_app is called immediately after
cyttsp5_deassert_int with the existing driver so presumably it doesn't actually
clear the interrupt line correctly.
Some more details:
https://lore.kernel.org/all/CADvTj4pdSkg5RWGThmj8Z_gOL5g2Ovhvfc-XtYTU88_0ve4NPw@mail.gmail.com/
>
>
> >
> > In general the issue with regmap seems to be that regmap always does
> > operations against specific registers and prevents doing raw i2c operations
> > needed to handle some hardware/firmware issues for some variants.
>
> What are those issues and why do they need raw access.
Mostly startup issues with cyttsp5_deassert_read and retry logic from what I
can tell. It appears that in some cases(such as when the system is rebooted
without fully powering off) multiple i2c reads are often required to flush some
sort of firmware side message queue so that the firmware is in a state in which
it will respond to commands normally.
I don't understand the reason this driver was written using regmap in the first
place, from my understanding of the protocol using regmap is not appropriate
as you can't correctly model an i2c-hid like protocol(like cyttsp5 uses) using
regmap.
Note that upstream drivers like i2c-hid don't use regmap either.
>
> >
> > Note that I'm not exactly doing things the same way the vendor driver does,
> > I have simplified the error recovery/retry code paths in the startup
> > function.
> >
> >
> > >
> > > >
> > > > As part of this rather than assuming the device is in bootloader mode
> > > > we should first check that the device is in bootloader and only
> > > > attempt to launch the app if it actually is in the bootloader.
> > >
> > > I would prefer if this was split into a separate patch.
> > >
> >
> > I think this change is somewhat intertwined with the probe retry/recovery
> > logic
> > changes and is a bit tricky to split out without breaking the startup
> > sequence
> > from my testing at least.
>
> I understand that it might be tricky but each logical change should
> stand on its own.
Ok, I'll see what I can do there.
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH] HID: fix a crash in hid_debug_events_release
From: Rahul Rameshbabu @ 2023-10-29 16:44 UTC (permalink / raw)
To: be286; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <406a0a08.60e7.18b6116f6a4.Coremail.be286@163.com>
On Tue, 24 Oct, 2023 17:49:36 +0800 "be286" <be286@163.com> wrote:
> Hi Rahul,
>
>
> When hid_debug_events_release() was being called, in most case,
> hid_device_release() finish already, the memory of list->hdev
> freed by hid_device_release(), if list->hdev memory
> reallocate by others, and it's modified, zeroed, then
> list->hdev->debug_list_lock occasioned crash come out.
>
This makes sense to me. Thanks for the additional explanation. I would
add this detail to the commit message body of your v2 to make the patch
clearer. With this explanation, I think your patch makes a lot of sense
to me and I understand better why you are seeing the trace you shared.
> The runing order:
>
> [ 258.201069] CPU: 0 PID: 203 Comm: kworker/0:2 Not tainted 5.10.110 #255
> [ 258.201073] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
> [ 258.201086] Workqueue: usb_hub_wq hub_event
> [ 258.201092] Call trace:
> [ 258.201100] dump_backtrace+0x0/0x1f8
> [ 258.201105] show_stack+0x1c/0x2c
> [ 258.201112] dump_stack_lvl+0xd8/0x12c
> [ 258.201116] dump_stack+0x1c/0x60
> [ 258.201122] hid_device_release+0x94/0xb4
> [ 258.201127] device_release+0x38/0x94
> [ 258.201133] kobject_cleanup+0xd0/0x174
> [ 258.201137] kobject_put+0x68/0xa8
> [ 258.201143] put_device+0x1c/0x2c
> [ 258.201146] hid_destroy_device+0x60/0x74
> [ 258.201153] usbhid_disconnect+0x5c/0x90
> [ 258.201157] usb_unbind_interface+0xc4/0x268
> [ 258.201162] device_release_driver_internal+0x184/0x25c
> [ 258.201165] device_release_driver+0x1c/0x2c
> [ 258.201169] bus_remove_device+0xdc/0x138
> [ 258.201173] device_del+0x1d0/0x3d8
> [ 258.201177] usb_disable_device+0x108/0x228
> [ 258.201181] usb_disconnect+0xf4/0x304
> [ 258.201184] usb_disconnect+0xdc/0x304
> [ 258.201188] hub_port_connect+0x88/0xa24
> [ 258.201191] hub_port_connect_change+0x2d8/0x4cc
> [ 258.201195] port_event+0x550/0x614
> [ 258.201198] hub_event+0x12c/0x494
> [ 258.201203] process_one_work+0x1f4/0x490
> [ 258.201207] worker_thread+0x278/0x4dc
> [ 258.201211] kthread+0x130/0x338
> [ 258.201215] ret_from_fork+0x10/0x30
> [ 259.925641] CPU: 1 PID: 2354 Comm: hidt_bridge Not tainted 5.10.110 #255
> [ 259.925652] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
> [ 259.925656] Call trace:
> [ 259.925671] dump_backtrace+0x0/0x1f8
> [ 259.925676] show_stack+0x1c/0x2c
> [ 259.925685] dump_stack_lvl+0xd8/0x12c
> [ 259.925689] dump_stack+0x1c/0x60
> [ 259.925697] hid_debug_events_release+0x24/0x134
> [ 259.925704] full_proxy_release+0x50/0xbc
> [ 259.925709] __fput+0xdc/0x238
> [ 259.925714] ____fput+0x14/0x24
> [ 259.925720] task_work_run+0x90/0x148
> [ 259.925725] do_exit+0x1bc/0x8a4
> [ 259.925729] do_group_exit+0x8c/0xa4
> [ 259.925734] get_signal+0x468/0x744
> [ 259.925739] do_signal+0x84/0x280
> [ 259.925743] do_notify_resume+0xd8/0x228
> [ 259.925747] work_pending+0xc/0x3f0
>
> The crash:
>
> [ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
> [ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
> [ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
> [ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
> [ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
> [ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
> [ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
> [ 120.779120][ T4396] sp : ffffffc01e62bb60
> [ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
> [ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
> [ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
> [ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
> [ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
> [ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
> [ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
> [ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
> [ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
> [ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
> [ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
> [ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
> [ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
> [ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
> [ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
> [ 120.873122][ T4396] Call trace:
> [ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
> [ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
> [ 120.886617][ T4396] full_proxy_release+0x50/0xbc
> [ 120.891323][ T4396] __fput+0xdc/0x238
> [ 120.895075][ T4396] ____fput+0x14/0x24
> [ 120.898911][ T4396] task_work_run+0x90/0x148
> [ 120.903268][ T4396] do_exit+0x1bc/0x8a4
> [ 120.907193][ T4396] do_group_exit+0x8c/0xa4
> [ 120.911458][ T4396] get_signal+0x468/0x744
> [ 120.915643][ T4396] do_signal+0x84/0x280
> [ 120.919650][ T4396] do_notify_resume+0xd0/0x218
> [ 120.924262][ T4396] work_pending+0xc/0x3f0
>
If you would like, you can use git notes to add the trace as well to the
notes summary of the patches you send out. This isn't a must but in case
you want to provide additional context in the future without having this
be a part of the commit message body.
https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt---notesltrefgt
>
>
>
>
>
>
>
>
>
> At 2023-10-23 23:03:35, "Rahul Rameshbabu" <sergeantsagara@protonmail.com> wrote:
>>Hi Charles,
>>
>>On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@163.com> wrote:
>>> hid_debug_events_release() access released memory by
>>> hid_device_release(). This is fixed by the patch.
>>>
>>
>>A couple of things here. Can you add a Fixes: tag?
>>
>>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>>
>>Before any v2 however, it would be nice to understand where this issue
>>is coming from. I am wondering if it's really a core issue or rather an
>>issue with a higher level device specific driver making use of the hid
>>subsystem. I am having a hard time seeing how this issue occurs
>>currently. A stack trace in a follow-up email to this one pertaining to
>>the crash would be helpful. If you are resolving a syzbot report, a link
>>to that report would suffice.
>>
>>This patch doesn't make a lot of sense to me as-is because
>>hid_debug_events_release is about release resources related to hid debug
>>events (at least from my current understanding). It should not be
>>free-ing the underlying hid device/instance itself.
>>
>>> Signed-off-by: Charles Yi <be286@163.com>
>>> ---
[...]
Sorry for the delay in being able to review this. Excited to see a v2
with a Fixes: tag.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
From: Yihong Cao @ 2023-10-29 17:05 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Yihong Cao, open list:HID CORE LAYER, open list
Jamesdonkey A3R keyboard is identified as "Jamesdonkey A3R" in wired
mode, "A3R-U" in wireless mode and "A3R" in bluetooth mode. Adding them
to non-apple keyboards fixes function key.
Signed-off-by: Yihong Cao <caoyihong4@outlook.com>
---
drivers/hid/hid-apple.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 3ca45975c686..d9e9829b2200 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -345,6 +345,8 @@ static const struct apple_non_apple_keyboard non_apple_keyboards[] = {
{ "AONE" },
{ "GANSS" },
{ "Hailuck" },
+ { "Jamesdonkey" },
+ { "A3R" },
};
static bool apple_is_non_apple_keyboard(struct hid_device *hdev)
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 3/7] Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
From: Caleb Connolly @ 2023-10-29 19:56 UTC (permalink / raw)
To: Pavel Machek
Cc: Dmitry Torokhov, Vincent Huang, methanal, linux-input, devicetree,
phone-devel, ~postmarketos/upstreaming
In-Reply-To: <ZTzlChOS0OR95Ykp@localhost>
On 28/10/2023 11:40, Pavel Machek wrote:
> Hi!
>
>> Some replacement displays include third-party touch ICs which are
>> devoid of register descriptors. Create a fake data register descriptor
>> for such ICs and provide hardcoded default values.
>>
>> It isn't possible to reliably determine if the touch IC is original or
>> not, so these fallback values are offered as an alternative to the error
>> path when register descriptors aren't available.
>>
>> Signed-off-by: methanal <baclofen@tuta.io>
>
> I guess we should have full/real name here.
I must disagree [1] [2]. These patches have my SoB and are being sent by
me on behalf of the author, who has no interest in contributing upstream.
[1]:
https://lore.kernel.org/lkml/c1bf62a2-e381-c796-2219-17a578987a76@marcan.st/T/
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
>
> Best regards,
> Pavel
>
--
// Caleb (they/them)
^ permalink raw reply
* Re: [PATCH v6 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Dmitry Torokhov @ 2023-10-30 6:56 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <20231027051819.81333-2-anshulusr@gmail.com>
Hi Anshul,
On Fri, Oct 27, 2023 at 10:48:11AM +0530, Anshul Dalal wrote:
> Adds a driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> Changes for v6:
> - Added TODO
> - Removed `clang-format` directives
> - Namespaced device buttons
> - Removed `char physical_path[32]` field from `struct seesaw_gamepad`
> - Added `packed` attribute to `struct seesaw_data`
> - Moved from having booleans per button to single `u32 button_state`
> - Added `seesaw_button_description` array to directly associate device
> buttons with respective keycodes
> - Added wrapper functions `seesaw_register_` around `i2c_master_` API
> - Ratelimited input error reporting with `dev_err_ratelimited`
> - Removed `of_device_id`
>
> Changes for v5:
> - Added link to the datasheet
> - Added debug log message when `seesaw_read_data` fails
>
> Changes for v4:
> - Changed `1UL << BUTTON_` to BIT(BUTTON_)
> - Removed `hardware_id` field from `struct seesaw_gamepad`
> - Removed redundant checks for the number of bytes written and received by
> `i2c_master_send` and `i2c_master_recv`
> - Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
> - Changed `result & (1UL << BUTTON_)` to
> `test_bit(BUTTON_, (long *)&result)`
> - Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
> - Fixed formatting issues
> - Changed button reporting:
> Since the gamepad had the action buttons in a non-standard layout:
> (X)
> (Y) (A)
> (B)
> Therefore moved to using generic directional action button event codes
> instead of BTN_[ABXY].
>
> Changes for v3:
> - no updates
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 310 +++++++++++++++++++++++
> 4 files changed, 327 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc6bf79fdd8..0595c832c248 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <anshulusr@gmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +F: drivers/input/joystick/adafruit-seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@kernel.org>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..df9cd1830b29 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit-seesaw.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 3937535f0098..9976f596a920 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..1aa6fbe4fda4
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + *
> + * TODO:
> + * - Add interrupt support
> + */
> +
> +#include <asm-generic/unaligned.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define SEESAW_BUTTON_A 5
> +#define SEESAW_BUTTON_B 1
> +#define SEESAW_BUTTON_X 6
> +#define SEESAW_BUTTON_Y 2
> +#define SEESAW_BUTTON_START 16
> +#define SEESAW_BUTTON_SELECT 0
> +
> +#define SEESAW_ANALOG_X 14
> +#define SEESAW_ANALOG_Y 15
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32
> +
> +u32 SEESAW_BUTTON_MASK = BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) |
> + BIT(SEESAW_BUTTON_X) | BIT(SEESAW_BUTTON_Y) |
> + BIT(SEESAW_BUTTON_START) | BIT(SEESAW_BUTTON_SELECT);
> +
> +struct seesaw_gamepad {
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + __be16 x;
> + __be16 y;
> + u32 button_state;
> +} __packed;
> +
> +struct seesaw_button_description {
> + unsigned int code;
> + unsigned int bit;
> +};
> +
> +static const struct seesaw_button_description seesaw_buttons[] = {
> + {
> + .code = BTN_EAST,
> + .bit = SEESAW_BUTTON_A,
> + },
> + {
> + .code = BTN_SOUTH,
> + .bit = SEESAW_BUTTON_B,
> + },
> + {
> + .code = BTN_NORTH,
> + .bit = SEESAW_BUTTON_X,
> + },
> + {
> + .code = BTN_WEST,
> + .bit = SEESAW_BUTTON_Y,
> + },
> + {
> + .code = BTN_START,
> + .bit = SEESAW_BUTTON_START,
> + },
> + {
> + .code = BTN_SELECT,
> + .bit = SEESAW_BUTTON_SELECT,
> + },
> +};
> +
> +static int seesaw_register_read(struct i2c_client *client, u8 register_high,
> + u8 register_low, char *buf, int count)
I am curious why we have 2 u8s instead of u16 that we send as __be16?
> +{
> + int ret;
> + u8 register_buf[2] = { register_high, register_low };
> +
> + ret = i2c_master_send(client, register_buf, sizeof(register_buf));
> + if (ret < 0)
> + return ret;
> + ret = i2c_master_recv(client, buf, count);
I am curious can this be an i2c_transfer() with read/write messages
instead (so 1 transaction)?
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
> + u8 register_low, u8 value)
> +{
> + int ret;
> + u8 write_buf[3] = { register_high, register_low, value };
> +
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_register_write_u32(struct i2c_client *client,
> + u8 register_high, u8 register_low,
> + u32 value)
> +{
> + int ret;
> + u8 write_buf[6] = { register_high, register_low };
> +
> + put_unaligned_be32(value, write_buf + 2);
> + ret = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int ret;
> + u8 read_buf[4];
Why is this u8 buffer and not __be32?
> +
> + ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
> + read_buf, sizeof(read_buf));
> + if (ret)
> + return ret;
> +
> + data->button_state = ~get_unaligned_be32(&read_buf);
If you use __be32 you would not need to use get_unaligned_be32.
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
> + (char *)&data->x, sizeof(data->x));
> + if (ret)
> + return ret;
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> +
> + ret = seesaw_register_read(client, SEESAW_ADC_BASE,
> + SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
> + (char *)&data->y, sizeof(data->y));
> + if (ret)
> + return ret;
> + data->y = be16_to_cpu(data->y);
This is endianness violation and sparse should have warned you about
this. A variable can either carry BE data, LE data, or CPU-endianness
data, but not both. I'd recommend combining seesaw_read_data() with
seesaw_poll() into something like seesaw_report_events() and using
temporaries for x and y and button data, and do not store them in the
private structure at all.
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + int err, i;
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err != 0) {
> + dev_err_ratelimited(&input->dev,
> + "failed to read joystick state: %d\n", err);
> + return;
> + }
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> +
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_report_key(input, seesaw_buttons[i].code,
> + data.button_state &
> + BIT(seesaw_buttons[i].bit));
> + }
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err, i;
> + u8 hardware_id;
> + struct seesaw_gamepad *seesaw;
> +
> + err = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF);
> + if (err)
> + return err;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
Can this be usleep_range() instead? No need to block CPU.
> +
> + seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
> + if (!seesaw)
> + return -ENOMEM;
> +
> + err = seesaw_register_read(client, SEESAW_STATUS_BASE,
> + SEESAW_STATUS_HW_ID, &hardware_id, 1);
sizeof(hardware_id)
> + if (err)
> + return err;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + hardware_id);
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_DIRCLR_BULK,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_PULLENSET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> + err = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
> + SEESAW_GPIO_BULK_SET,
> + SEESAW_BUTTON_MASK);
> + if (err)
> + return err;
> +
> + seesaw->i2c_client = client;
> + i2c_set_clientdata(client, seesaw);
> +
> + seesaw->input_dev = devm_input_allocate_device(&client->dev);
> + if (!seesaw->input_dev)
> + return -ENOMEM;
> +
> + seesaw->input_dev->id.bustype = BUS_I2C;
> + seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
> + seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
> + input_set_drvdata(seesaw->input_dev, seesaw);
> + input_set_abs_params(seesaw->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + for (i = 0; i < ARRAY_SIZE(seesaw_buttons); i++) {
> + input_set_capability(seesaw->input_dev, EV_KEY,
> + seesaw_buttons[i].code);
> + }
> +
> + err = input_setup_polling(seesaw->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(seesaw->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(seesaw->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id seesaw_id_table[] = {
> + { SEESAW_DEVICE_NAME, 0 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox