* RE: [PATCH v3 0/6] iio: hid-sensor: standardize scan_type initialization
From: Zhang, Lixu @ 2026-05-14 2:44 UTC (permalink / raw)
To: Jonathan Cameron, Natália Salvino André
Cc: andy@kernel.org, Lechner, David, jikos@kernel.org,
nuno.sa@analog.com, srinivas.pandruvada@linux.intel.com,
linux-iio@vger.kernel.org, linux-input@vger.kernel.org
In-Reply-To: <20260512154853.10aa2902@jic23-huawei>
>-----Original Message-----
>From: Zhang, Lixu
>Sent: Wednesday, May 13, 2026 11:27 AM
>To: Jonathan Cameron <jic23@kernel.org>; Natália Salvino André
><natalia.andre@ime.usp.br>
>Cc: andy@kernel.org; Lechner, David <dlechner@baylibre.com>;
>jikos@kernel.org; nuno.sa@analog.com; srinivas.pandruvada@linux.intel.com;
>linux-iio@vger.kernel.org; linux-input@vger.kernel.org
>Subject: RE: [PATCH v3 0/6] iio: hid-sensor: standardize scan_type initialization
>
>>-----Original Message-----
>>From: Jonathan Cameron <jic23@kernel.org>
>>Sent: Tuesday, May 12, 2026 10:49 PM
>>To: Natália Salvino André <natalia.andre@ime.usp.br>
>>Cc: andy@kernel.org; Lechner, David <dlechner@baylibre.com>;
>>jikos@kernel.org; nuno.sa@analog.com;
>>srinivas.pandruvada@linux.intel.com;
>>linux-iio@vger.kernel.org; linux-input@vger.kernel.org; Zhang, Lixu
>><lixu.zhang@intel.com>
>>Subject: Re: [PATCH v3 0/6] iio: hid-sensor: standardize scan_type
>>initialization
>>
>>On Mon, 11 May 2026 22:16:16 -0300
>>Natália Salvino André <natalia.andre@ime.usp.br> wrote:
>>
>>> This series refactors the HID sensor drivers to standardize the
>>> initialization of the iio_chan_spec scan_type structure.
>>> Direct initialization using compound literals was used.
>>>
>>> This change improves code readability and ensures that all fields of
>>> the scan_type structure are properly zero-initialized.
>>> Additionally, for the hid-sensor-accel-3d driver, the channel
>>> initialization loop was refactored to iterate directly over the scan
>>> indices, eliminating redundant logic.
>>
>>Series looks good to me but I'll leave it on the list a while to both
>>let sashiko catch up and anyone else who wants to can have time to
>>look. Also ideally get some testing.
>>
>>Lixu, if you don't mind, please can you give this a test on top of the
>>more radical refactor patches?
>
>Sure, I'll test this series on top of the refactor patches when I have some time.
Hi Jonathan,
I've done some basic testing on this series. Here are the results:
1. No issues seen for hid-sensor-gyro-3d, hid-sensor-als, hid-sensor-prox, hid-sensor-magn-3d.
2. Didn't test hid-sensor-press as I couldn't find a platform with the corresponding device.
3. There's an issue with hid-sensor-accel-3d. Sashiko AI review has already identified the problem and provided review comments.
Thanks,
Lixu
^ permalink raw reply
* RE: [PATCH v3 0/9] iio: introduce devm_ API for hid sensro setup and cleanup
From: Zhang, Lixu @ 2026-05-14 2:47 UTC (permalink / raw)
To: Sanjay Chitroda, jikos@kernel.org, jic23@kernel.org,
srinivas.pandruvada@linux.intel.com
Cc: Lechner, David, nuno.sa@analog.com, andy@kernel.org,
sakari.ailus@linux.intel.com, linux-input@vger.kernel.org,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20260509101040.791404-1-sanjayembedded@gmail.com>
Tested-by: Zhang Lixu <lixu.zhang@intel.com>
>-----Original Message-----
>From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>Sent: Saturday, May 9, 2026 6:11 PM
>To: jikos@kernel.org; jic23@kernel.org; srinivas.pandruvada@linux.intel.com
>Cc: Lechner, David <dlechner@baylibre.com>; nuno.sa@analog.com;
>andy@kernel.org; sanjayembeddedse@gmail.com;
>sakari.ailus@linux.intel.com; linux-input@vger.kernel.org; linux-
>iio@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: [PATCH v3 0/9] iio: introduce devm_ API for hid sensro setup and
>cleanup
>
>From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
>Key highlights:
>- Prepare change as pre-requisite for devm conversion for HID IIO
> drivers by removing redundant argument
>- Add devm API to setup trigger and clenaup resource using
> devm_add_action_or_reset()
>- few cleanup and prepratory changes before updating driver for devm_
>- few sample driver update using devm conversion to auto release resource
>
>changes in v3:
>- Added cleanup and prepratory changes before adding devm_ API
> conversion based on self review: 0002, 0004, 0006, 0007 and 0008
>- Address andy's review comment on commit message and coding style
>- v2 series -> https://lore.kernel.org/all/20260429175918.2541914-1-
>sanjayembedded@gmail.com/
>changes in v2:
>- Following input from Jonathan and Andy, squash initial patch v1
> series in single change as individual change should not break anything
>- Add devm API support and two driver using the same
>- v1 series -> https://lore.kernel.org/all/20260428071613.1134053-1-
>sanjayembedded@gmail.com/
>
>Testing:
> - Compiled with W=1
> - Build-tested on QEMU x86_64
>
>Based on further feedback and reviews, I would extend this series to convert all
>HID IIO driver to use devm_* API.
>
>Thanks,
>Sanjay Chitroda
>
>
>Sanjay Chitroda (9):
> iio: hid-sensors: drop redundant iio_dev argument
> iio: hid-sensors: cleanup codestyle warning
> iio: hid-sensors: introduce device managed API
> iio: gyro: hid-sensor-gyro-3d: cleanup codestyle warning
> iio: gyro: hid-sensor-gyro-3d: drop hid_sensor_remove_trigger() using
> devm API
> iio: humidity: hid-sensor-humidity: cleanup codestyle check
> iio: humidity: hid-sensor-humidity: use common device for devres
> iio: humidity: hid-sensor-humidity: use local struct device
> iio: humidity: hid-sensor-humidity: drop hid_sensor_remove_trigger()
> using devm API
>
> drivers/iio/accel/hid-sensor-accel-3d.c | 4 +-
> .../common/hid-sensors/hid-sensor-trigger.c | 24 +++++++-
> .../common/hid-sensors/hid-sensor-trigger.h | 5 +-
> drivers/iio/gyro/hid-sensor-gyro-3d.c | 16 ++---
> drivers/iio/humidity/hid-sensor-humidity.c | 61 +++++++++----------
> drivers/iio/light/hid-sensor-als.c | 4 +-
> drivers/iio/light/hid-sensor-prox.c | 4 +-
> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 4 +-
>drivers/iio/orientation/hid-sensor-incl-3d.c | 4 +- drivers/iio/orientation/hid-
>sensor-rotation.c | 4 +- .../position/hid-sensor-custom-intel-hinge.c | 4 +-
> drivers/iio/pressure/hid-sensor-press.c | 4 +-
> .../iio/temperature/hid-sensor-temperature.c | 4 +-
> 13 files changed, 78 insertions(+), 64 deletions(-)
>
>
>base-commit: 39b80c5c9830d12d2d6531059001301c4265322a
>--
>2.34.1
>
^ permalink raw reply
* Re: [PATCH 1/7] hid-asus: Fix up Zenbook Duo report descriptors
From: sashiko-bot @ 2026-05-14 6:50 UTC (permalink / raw)
To: Paolo Pisati; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513163248.16483-2-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The report descriptor fixup for the Zenbook Duo corrupts the descriptor because it incorrectly shifts only 12 bytes and allocates insufficient memory.
--
commit 6124d2bbfc4e5254d102730c30ac761f188e21b0
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Fix up Zenbook Duo report descriptors
This commit adds a quirk for the Zenbook Duo 2024+ keyboards to fix up an
unmapped Asus vendor usage page code. It reuses the existing report
descriptor fixup logic previously used for T100CHI and T90CHI devices.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 3f5e96900b67a..ce246efba74d3 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1384,17 +1385,20 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> hid_info(hdev, "Fixing up Asus T100 keyb report descriptor\n");
> rdesc[74] &= ~HID_MAIN_ITEM_CONSTANT;
> }
> - /* For the T100CHI/T90CHI keyboard dock */
> - if (drvdata->quirks & (QUIRK_T100CHI | QUIRK_T90CHI)) {
> + /* For the T100CHI/T90CHI keyboard dock and Zenbook Duo 2024+ keyboards */
> + if (drvdata->quirks & (QUIRK_T100CHI | QUIRK_T90CHI | QUIRK_ZENBOOK_DUO_KEYBOARD)) {
> int rsize_orig;
> int offs;
>
> if (drvdata->quirks & QUIRK_T100CHI) {
> rsize_orig = 403;
> offs = 388;
> - } else {
> + } else if (drvdata->quirks & QUIRK_T90CHI) {
> rsize_orig = 306;
> offs = 291;
> + } else if (drvdata->quirks & QUIRK_ZENBOOK_DUO_KEYBOARD) {
> + rsize_orig = 257;
> + offs = 176;
> }
Could this introduce a regression by corrupting the Zenbook Duo report
descriptor when failing to shift the remainder of the items correctly?
For the Zenbook Duo, the descriptor is 257 bytes long and the offset is 176,
leaving 79 bytes after the replaced item. However, the shared fixup logic
below this code hardcodes a memmove of exactly 12 bytes:
drivers/hid/hid-asus.c:asus_report_fixup() {
...
memcpy(new_rdesc, rdesc, rsize_orig);
*rsize = rsize_orig + 1;
rdesc = new_rdesc;
memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
rdesc[offs] = 0x19;
...
}
Does shifting only 12 bytes overwrite the valid original bytes at offsets 190
and 191, and leave the remaining 67 bytes unshifted?
Additionally, does the new_rdesc allocation properly account for the shift?
Replacing a 2-byte item with a 4-byte item requires shifting the remaining
items forward by 2 bytes. Allocating only rsize_orig + 1 could lead to a
1-byte out-of-bounds write if all 79 bytes are shifted properly.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] HID: Expose LattePanda IOTA UPS as a power_supply device
From: sashiko-bot @ 2026-05-14 6:50 UTC (permalink / raw)
To: Andrew Maney; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513155723.230707-1-andrewmaney05@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Kernel panic due to invalid type cast when transport is not USB (e.g. uhid).
- [High] Kernel panic from dereferencing an ERR_PTR in the raw event handler.
- [High] Exposing fabricated 0 values for unsupported TIME_TO_EMPTY properties causes immediate system shutdown.
- [Medium] Hardcoded sysfs name prevents multiple devices and causes probe failure.
- [Medium] Data race on `ups->charge_limit`.
- [Low] Missing `POWER_SUPPLY` dependency in Kconfig causes build failures.
- [Low] Driver synchronously blocks the probe path for 3 seconds.
--
commit 7d140ce589ec6c637bec5ce8b0c27bf8634322c6
Author: Andrew Maney <andrewmaney05@gmail.com>
Subject: HID: Expose LattePanda IOTA UPS as a power_supply device
This commit adds a driver for the LattePanda IOTA UPS. It exposes battery
status, capacity, and charging status via the power_supply subsystem by parsing
custom HID reports.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -105,6 +105,16 @@ config HID_HAPTIC
>
> menu "Special HID drivers"
>
> +config HID_LATTEPANDA_IOTA_UPS
> + tristate "LattePanda IOTA UPS"
> + depends on USB_HID && X86
If the kernel is configured with CONFIG_POWER_SUPPLY=m and
CONFIG_HID_LATTEPANDA_IOTA_UPS=y, will the built-in HID driver fail to link
against the modular power supply core? Should this include a dependency on
POWER_SUPPLY?
> diff --git a/drivers/hid/hid-lattepanda-iota-ups.c b/drivers/hid/hid-lattepanda-iota-ups.c
> --- /dev/null
> +++ b/drivers/hid/hid-lattepanda-iota-ups.c
[ ... ]
> +static enum power_supply_property iota_ups_properties[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_SCOPE,
> + POWER_SUPPLY_PROP_TECHNOLOGY,
> + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
> + POWER_SUPPLY_PROP_CAPACITY_LEVEL,
> + POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> + POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
If the hardware doesn't support reporting time remaining, is it safe to
include these properties and return 0?
Returning 0 seconds remaining while discharging can signal to userspace power
daemons like UPower or systemd-logind that the battery is empty, which might
trigger an immediate emergency suspend or shutdown. Could these unsupported
properties be omitted from the array entirely?
[ ... ]
> +static int iota_ups_set_property(struct power_supply *supply,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct iota_ups *ups = power_supply_get_drvdata(supply);
> +
> + if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
> + /*
> + * V1.0 supports 80% and 100% charge limits only, set via
> + * DIP switch on the board. This property allows userspace
> + * to inform the driver which limit is configured.
> + */
> + if (val->intval != 80 && val->intval != 100)
> + return -EINVAL;
> + ups->charge_limit = val->intval;
Does this introduce a data race on ups->charge_limit?
This field is modified here without holding ups->lock, but it is concurrently
read in iota_ups_raw_event() and iota_ups_get_property() where it is
protected by spin_lock_irqsave(). Should this assignment also be
protected by the spinlock?
[ ... ]
> +static int iota_ups_raw_event(struct hid_device *hdev,
> + struct hid_report *report,
> + u8 *data, int size)
> +{
> + struct iota_ups *ups = hid_get_drvdata(hdev);
[ ... ]
> + /*
> + * Notify the power_supply core outside the spinlock. This
> + * triggers UPower's PropertiesChanged signal with the new values.
> + */
> + if (changed && ups->psu)
> + power_supply_changed(ups->psu);
Could ups->psu contain an error pointer here?
In iota_ups_probe(), if devm_power_supply_register() fails, it returns an
error pointer like ERR_PTR(-ENOMEM), which gets directly assigned to ups->psu.
Because hid_hw_start() is called earlier, raw HID events can be processed
concurrently before the hardware is stopped in the error path. Since an error
pointer is not NULL, the check (changed && ups->psu) would pass, and
power_supply_changed() would be called with the error pointer, leading to a
crash.
Would it be safer to assign the return value of devm_power_supply_register()
to a local variable first, and only update ups->psu on success?
[ ... ]
> +static int iota_ups_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
[ ... ]
> + /* Retrieve serial number from the USB descriptor */
> + udev = to_usb_device(hdev->dev.parent->parent);
> + if (udev->serial)
> + strscpy(ups->serial, udev->serial, sizeof(ups->serial));
Can this cause a kernel panic if the HID device is instantiated via userspace
mock drivers like uhid?
When using uhid (which allows setting the bus type to BUS_USB), the parent
device is the uhid character device, not a usb_interface. Dereferencing the
resulting struct usb_device pointer would perform an out-of-bounds memory
read.
Should the driver verify the transport layer using hid_is_usb() before
dereferencing USB-specific structures, or simply use hdev->uniq instead?
[ ... ]
> + /*
> + * Wait for both status and capacity reports before registering.
> + * The device sends reports every ~1 second; 3 seconds is safe.
> + */
> + wait_for_completion_timeout(&ups->got_initial_data,
> + msecs_to_jiffies(3000));
Does blocking the probe path for 3 seconds cause issues for device
enumeration?
Calling wait_for_completion_timeout() synchronously stalls the kernel's
device enumeration thread for that bus, which can unnecessarily delay the
overall system boot process or stall the handling of other devices during
hotplug.
Could the registration be deferred, or could the device be registered
immediately with an unknown state instead of blocking?
> +
> + ups->psu_desc.name = "lattepanda-iota-ups";
Is it possible to have multiple of these UPS devices connected to a single
system?
If a second device is plugged in, it will attempt to register a sysfs
directory with the exact same hardcoded name, which fails with -EEXIST.
This probe failure would then trigger the error pointer panic mentioned
earlier. Should the driver dynamically construct a unique name using
devm_kasprintf() with dev_name(&hdev->dev)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513155723.230707-1-andrewmaney05@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH] input/evdev: move kill_fasync() outside buffer_lock to fix SOFTIRQ deadlock
From: sashiko-bot @ 2026-05-14 7:06 UTC (permalink / raw)
To: Rik van Riel; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513115000.77ec5975@fangorn>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving kill_fasync() outside of client->buffer_lock does not resolve the lock ordering violation because the function is still executing under dev->event_lock.
--
commit 803cd2c3ce0ada2c1fe4625fcbef594cb58290eb
Author: Rik van Riel <riel@surriel.com>
input/evdev: move kill_fasync() outside buffer_lock to fix SOFTIRQ deadlock
This commit attempts to fix a SOFTIRQ-safe to SOFTIRQ-unsafe lock inversion
by moving the kill_fasync() call outside of the client->buffer_lock.
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
[ ... ]
> @@ -280,9 +278,11 @@ static void evdev_pass_values(struct evdev_client *client,
>
> spin_unlock(&client->buffer_lock);
>
> - if (wakeup)
> + if (wakeup) {
> + kill_fasync(&client->fasync, SIGIO, POLL_IN);
> wake_up_interruptible_poll(&client->wait,
> EPOLLIN | EPOLLOUT | EPOLLRDNORM | EPOLLWRNORM);
> + }
> }
Does this completely resolve the lock ordering violation?
While kill_fasync() is now outside of client->buffer_lock, evdev_pass_values()
is called from evdev_events(), which is invoked by the input core in
input_pass_values().
During this dispatch, the input core strictly holds dev->event_lock:
drivers/input/input.c:input_pass_values() {
...
rcu_read_lock();
spin_lock_irqsave(&dev->event_lock, flags);
...
handler->events(handle, vals, count);
...
spin_unlock_irqrestore(&dev->event_lock, flags);
}
Since dev->event_lock is also an IRQ/SOFTIRQ-safe spinlock, does this
just shift the problem so we still have a dev->event_lock -> fa_lock
inversion?
Would it make sense to defer the kill_fasync() call entirely outside
the event lock context, perhaps by using irq_work?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513115000.77ec5975@fangorn?part=1
^ permalink raw reply
* Re: [PATCH] Input: wacom_w8001 - avoid double release of pen input device
From: Peter Hutterer @ 2026-05-14 7:10 UTC (permalink / raw)
To: Guangshuo Li
Cc: Dmitry Torokhov, Kees Cook, Benjamin Tissoires, linux-input,
linux-kernel
In-Reply-To: <20260430071311.451957-1-lgs201920130244@gmail.com>
On Thu, Apr 30, 2026 at 03:13:11PM +0800, Guangshuo Li wrote:
> When registering the touch input device fails after the pen input device
> has already been registered, w8001_connect() jumps to fail4 and
> unregisters w8001->pen_dev. It then falls through to fail1 where
> input_dev_pen is passed to input_free_device().
>
> Once input_register_device() has succeeded, the device must be released
> with input_unregister_device(), and input_free_device() must not be used
> on the same object afterwards. Since input_dev_pen still aliases
> w8001->pen_dev, this can result in a use-after-free or kref underflow.
>
> Clear the local and container aliases after unregistering the pen device
> so that the common cleanup path does not try to free it again.
>
> This issue was found by a static analysis tool I am developing.
>
> Fixes: e0361b70175f0 ("Input: wacom_w8001 - split the touch and pen devices into two devices")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
thanks
Cheers,
Peter
> ---
> drivers/input/touchscreen/wacom_w8001.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 45930d731873..a3b283c59cdd 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -665,8 +665,11 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
> return 0;
>
> fail4:
> - if (w8001->pen_dev)
> + if (w8001->pen_dev) {
> input_unregister_device(w8001->pen_dev);
> + input_dev_pen = NULL;
> + w8001->pen_dev = NULL;
> + }
> fail3:
> serio_close(serio);
> fail2:
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH 2/7] hid-asus: Add missing Zenbook Duo hotkeys
From: sashiko-bot @ 2026-05-14 7:49 UTC (permalink / raw)
To: Paolo Pisati; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513163248.16483-3-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The 'MyASUS' key (0x86) mapping is inconsistent with the existing Asus WMI driver.
--
commit ee67c289461b2da1755d4534b96e25db841e41d8
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Add missing Zenbook Duo hotkeys
This commit adds input mappings for several missing Zenbook Duo hotkeys,
including the MyASUS key, screen swap, and ROG Ally buttons, handling
them through the hid-asus driver.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index ce246efba74d3..cba638b19465f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1046,6 +1046,7 @@ static int asus_input_mapping(struct hid_device *hdev,
> case 0x6c: asus_map_key_clear(KEY_SLEEP); break;
> case 0x7c: asus_map_key_clear(KEY_MICMUTE); break;
> case 0x82: asus_map_key_clear(KEY_CAMERA); break;
> + case 0x86: asus_map_key_clear(KEY_CONTROLPANEL); break; /* MyASUS */
Is it intentional to map the MyASUS key to KEY_CONTROLPANEL here?
The existing asus-nb-wmi driver maps the identical 0x86 hardware event
for the MyASUS key to KEY_PROG1.
Mapping this to KEY_CONTROLPANEL in the HID driver might introduce an
inconsistent contract for user-space, where the exact same physical
button on Asus devices yields different keycodes depending on whether the
event is routed through the HID or WMI subsystem.
Could this cause issues for desktop environments attempting to bind
device-specific hotkeys consistently?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH] amd-sfh-hid: tablet mode switch and asus quirk
From: Helge Bahmann @ 2026-05-14 7:59 UTC (permalink / raw)
To: Jiri Kosina, linux-input
Cc: Nehal Bakulchandra Shah, Sandeep Singh, Basavaraj Natikar,
bentiss
In-Reply-To: <n0s5q66s-p747-612p-o743-pqr0q2r79448@xreary.bet>
On Tue, 12 May 2026, Jiri Kosina wrote:
> On Mon, 27 Apr 2026, Helge Bahmann wrote:
>
> > Add an input driver that interprets the "operation mode" sensor offered
> > by the amd sfh on some laptop models.
> >
> > Add a quirk to make the driver work again with the Asus VivoBook
> > VivoBook (turn off the "disable interrupts" flag).
> >
> > Expose the intr_disable flag as a module parameter in case it turns out
> > to be needed on further laptop models.
> >
> > Signed-off-by: Helge Bahmann <hcb@chaoticmind.net>
>
> Basavaraj, can you please review this one?
Some additional context, maybe helpful for review:
1. The numbers and behavior were extracted from the ACPI tables
(WMI driver of sorts) of the notebook; I don't have access to any
official AMD / ASUS docs or similar.
2. I have an alternate version of this change that is more indirect:
- create a HID driver providing an "abstract table mode" message
- have an input driver attaching to this newly defined HID driver
While that is keeping "more in line" with the current driver
architecture, I am not sure this indirection really helps. Particularly,
there is no "canonical" HID tablet mode switch message defined,
so it all remains completely bespoke. I am happy to change it if
you prefer, but would need your input.
3. Since this is based on Asus VivoBook and its ACPI tables,
there is a possibility that this "op sensor / tablet mode" behavior
is not as universal as I surmise. A point could be made to make this
entire behavior model-dependent (with a mod param to override
/ activate for other models). Happy to take input / advice.
Thanks for your work!
Helge
>
> Thanks,
>
>
^ permalink raw reply
* Re: [PATCH 4/7] hid-asus: Remove more bogus zero bytes from some report descriptors
From: sashiko-bot @ 2026-05-14 8:57 UTC (permalink / raw)
To: Paolo Pisati; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513163248.16483-5-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The trailing zero removal loop reads past the end of the report descriptor buffer and lacks a lower bounds check.
- [High] The `memcpy` operation copies more bytes than are available in the source `rdesc` buffer.
- [High] Trimming trailing bytes can shrink the new buffer size below what is required for subsequent fixed-offset memory operations, causing a heap buffer overflow.
--
commit 1a97afa374b1aa0ec76a035ced7d0e6414476f71
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Remove more bogus zero bytes from some report descriptors
This commit attempts to remove bogus trailing zero bytes from specific
HID report descriptors. It adjusts the new report descriptor size by
stripping zeroes from the end and then allocates a new buffer.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> if (*rsize == rsize_orig &&
> rdesc[offs] == 0x09 && rdesc[offs + 1] == 0x76) {
> __u8 *new_rdesc;
> + unsigned int new_rsize = rsize_orig;
>
> + /* Clear bogus trailing zero bytes. */
> + while (rdesc[new_rsize + 1] == 0)
> + --new_rsize;
Since the rdesc buffer contains exactly rsize_orig bytes, does evaluating
rdesc[new_rsize + 1] read two bytes past the end of the allocation?
Additionally, if the adjacent memory happens to contain zeroes, could
new_rsize decrement indefinitely and underflow since there is no lower
bound check?
> + /* Make room for the added bytes. */
> + new_rsize += 2;
> +
> + new_rdesc = devm_kzalloc(&hdev->dev, new_rsize,
> GFP_KERNEL);
> if (!new_rdesc)
> return rdesc;
[ ... ]
> - memcpy(new_rdesc, rdesc, rsize_orig);
> - *rsize = rsize_orig + 1;
> + memcpy(new_rdesc, rdesc, new_rsize);
If no trailing zeroes were stripped, new_rsize is now rsize_orig + 2. Does
this memcpy copy more bytes than are available in the original rdesc
buffer, leading to an out-of-bounds read?
> + *rsize = new_rsize;
> rdesc = new_rdesc;
>
> memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
If trailing zeroes were successfully removed, could new_rsize drop below the
offsets used in this memmove and subsequent assignments?
For example, if new_rsize shrinks below offs + 15, will the fixed-offset
memmove and subsequent assignments write past the end of the newly allocated
new_rdesc buffer?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=4
^ permalink raw reply
* [PATCH 0/6] tools/build: Allow versioning of all LLVM tools
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
I ran into the build issue mentioned in the first commit, because using
an option like LLVM=-15 is a documented way to build [1], but only
some of the LLVM tools defined in tools/ support this.
After fixing that, a lot of other tidyups fell out. I didn't go too far,
for example some selftests include tools/testing/selftests/lib.mk which
reimplements some of the LLVM_SUFFIX stuff, but other selftests include
tools/scripts/Makefile.include and I stopped before touching the former.
Some other non build scripts hard code toolchains and I didn't touch
those either, this change is only focused on things that already
include Makefile.include.
[1]: Documentation/kbuild/llvm.rst
Signed-off-by: James Clark <james.clark@linaro.org>
---
James Clark (6):
tools/build: Allow versioning of all LLVM tools defined in Makefile.include
tools/build: Indent if else blocks
selftests: Remove unused LLD variable
tools/build: Allow versioning LLVM readelf
tools/build: selftests: Allow versioning LLVM lld
tools/build: selftests: Remove some duplicate toolchain definitions
tools/bpf/resolve_btfids/Makefile | 3 --
tools/lib/api/Makefile | 4 --
tools/lib/bpf/Makefile | 8 ++--
tools/lib/subcmd/Makefile | 4 --
tools/lib/symbol/Makefile | 4 --
tools/perf/Makefile.perf | 7 ----
tools/scripts/Makefile.include | 77 ++++++++++++++++++++----------------
tools/testing/selftests/bpf/Makefile | 9 ++---
tools/testing/selftests/hid/Makefile | 7 ----
9 files changed, 50 insertions(+), 73 deletions(-)
---
base-commit: 09d355618f7ccc27ffc7fc668b2e232872962079
change-id: 20260511-james-perf-llvm-version-43ab1cb5bc7d
Best regards,
--
James Clark <james.clark@linaro.org>
^ permalink raw reply
* [PATCH 1/6] tools/build: Allow versioning of all LLVM tools defined in Makefile.include
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
The version of LLVM tools can be given on the build command with
LLVM=-15, but this isn't applied to all tools. For example $(CC) gets
versioned, but $(CLANG) doesn't. This causes a Perf build with LTO=1 to
fail with an error about mixed clang versions:
ld.lld: error: libperf/core.o: Unknown attribute kind (86)
(Producer: 'LLVM18.1.8' Reader: 'LLVM 15.0.7')
This file has two "ifneq ($(LLVM),)" blocks adjacent to each other, so
merge these blocks making it obvious that all tools should be versioned
consistently and there is nothing special about each block.
This also reveals that ?= and "allow-override" are used inconsistently
between the blocks. "allow-override" is technically only required for
builtin variables, but isn't only used on them, and doesn't do any harm
if used on a non-builtin. Make them all "allow-override" for
consistency. The only functional difference this will cause is if there
is a file level definition of one of the variables followed by an
"#include of Makefile.include" which will now overwrite. But this isn't
done and in a later commit some of the duplicate definitions will be
removed for good measure.
There are also some other LLVM tools that are not defined here and will
be moved in a later commit.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/scripts/Makefile.include | 37 ++++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index 41971a68972d..7022e78208a2 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -61,10 +61,18 @@ $(error Invalid value for LLVM, see Documentation/kbuild/llvm.rst)
endif
$(call allow-override,CC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
+$(call allow-override,CLANG,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
+$(call allow-override,HOSTCC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
$(call allow-override,AR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
+$(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
$(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
+$(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
$(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX))
$(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
+$(call allow-override,LLVM_STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
+$(call allow-override,LLC,$(LLVM_PREFIX)llc$(LLVM_SUFFIX))
+$(call allow-override,LLVM_CONFIG,$(LLVM_PREFIX)llvm-config$(LLVM_SUFFIX))
+$(call allow-override,LLVM_OBJCOPY,$(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX))
else
# Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
$(call allow-override,CC,$(CROSS_COMPILE)gcc)
@@ -72,26 +80,21 @@ $(call allow-override,AR,$(CROSS_COMPILE)ar)
$(call allow-override,LD,$(CROSS_COMPILE)ld)
$(call allow-override,CXX,$(CROSS_COMPILE)g++)
$(call allow-override,STRIP,$(CROSS_COMPILE)strip)
-endif
-
-CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
-ifneq ($(LLVM),)
-HOSTAR ?= $(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX)
-HOSTCC ?= $(LLVM_PREFIX)clang$(LLVM_SUFFIX)
-HOSTLD ?= $(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX)
-else
-HOSTAR ?= ar
-HOSTCC ?= gcc
-HOSTLD ?= ld
+# Host versions aren't prefixed
+$(call allow-override,HOSTAR,ar)
+$(call allow-override,HOSTCC,gcc)
+$(call allow-override,HOSTLD,ld)
+
+# Some tools still require Clang, LLC and/or LLVM utils
+$(call allow-override,CLANG,clang)
+$(call allow-override,LLC,llc)
+$(call allow-override,LLVM_CONFIG,llvm-config)
+$(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
+$(call allow-override,LLVM_STRIP,llvm-strip)
endif
-# Some tools require Clang, LLC and/or LLVM utils
-CLANG ?= clang
-LLC ?= llc
-LLVM_CONFIG ?= llvm-config
-LLVM_OBJCOPY ?= llvm-objcopy
-LLVM_STRIP ?= llvm-strip
+CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
# Some tools require bpftool
SYSTEM_BPFTOOL ?= bpftool
--
2.34.1
^ permalink raw reply related
* [PATCH 2/6] tools/build: Indent if else blocks
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
These blocks are quite big and unreadable without indentation. Indent
them.
No functional changes intended.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/scripts/Makefile.include | 76 +++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 38 deletions(-)
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index 7022e78208a2..e81e5b479c56 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -52,46 +52,46 @@ define allow-override
endef
ifneq ($(LLVM),)
-ifneq ($(filter %/,$(LLVM)),)
-LLVM_PREFIX := $(LLVM)
-else ifneq ($(filter -%,$(LLVM)),)
-LLVM_SUFFIX := $(LLVM)
-else ifneq ($(LLVM),1)
-$(error Invalid value for LLVM, see Documentation/kbuild/llvm.rst)
-endif
+ ifneq ($(filter %/,$(LLVM)),)
+ LLVM_PREFIX := $(LLVM)
+ else ifneq ($(filter -%,$(LLVM)),)
+ LLVM_SUFFIX := $(LLVM)
+ else ifneq ($(LLVM),1)
+ $(error Invalid value for LLVM, see Documentation/kbuild/llvm.rst)
+ endif
-$(call allow-override,CC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
-$(call allow-override,CLANG,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
-$(call allow-override,HOSTCC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
-$(call allow-override,AR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
-$(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
-$(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
-$(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
-$(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX))
-$(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
-$(call allow-override,LLVM_STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
-$(call allow-override,LLC,$(LLVM_PREFIX)llc$(LLVM_SUFFIX))
-$(call allow-override,LLVM_CONFIG,$(LLVM_PREFIX)llvm-config$(LLVM_SUFFIX))
-$(call allow-override,LLVM_OBJCOPY,$(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX))
+ $(call allow-override,CC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
+ $(call allow-override,CLANG,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
+ $(call allow-override,HOSTCC,$(LLVM_PREFIX)clang$(LLVM_SUFFIX))
+ $(call allow-override,AR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
+ $(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
+ $(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
+ $(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
+ $(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX))
+ $(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
+ $(call allow-override,LLVM_STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
+ $(call allow-override,LLC,$(LLVM_PREFIX)llc$(LLVM_SUFFIX))
+ $(call allow-override,LLVM_CONFIG,$(LLVM_PREFIX)llvm-config$(LLVM_SUFFIX))
+ $(call allow-override,LLVM_OBJCOPY,$(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX))
else
-# Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
-$(call allow-override,CC,$(CROSS_COMPILE)gcc)
-$(call allow-override,AR,$(CROSS_COMPILE)ar)
-$(call allow-override,LD,$(CROSS_COMPILE)ld)
-$(call allow-override,CXX,$(CROSS_COMPILE)g++)
-$(call allow-override,STRIP,$(CROSS_COMPILE)strip)
-
-# Host versions aren't prefixed
-$(call allow-override,HOSTAR,ar)
-$(call allow-override,HOSTCC,gcc)
-$(call allow-override,HOSTLD,ld)
-
-# Some tools still require Clang, LLC and/or LLVM utils
-$(call allow-override,CLANG,clang)
-$(call allow-override,LLC,llc)
-$(call allow-override,LLVM_CONFIG,llvm-config)
-$(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
-$(call allow-override,LLVM_STRIP,llvm-strip)
+ # Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
+ $(call allow-override,CC,$(CROSS_COMPILE)gcc)
+ $(call allow-override,AR,$(CROSS_COMPILE)ar)
+ $(call allow-override,LD,$(CROSS_COMPILE)ld)
+ $(call allow-override,CXX,$(CROSS_COMPILE)g++)
+ $(call allow-override,STRIP,$(CROSS_COMPILE)strip)
+
+ # Host versions aren't prefixed
+ $(call allow-override,HOSTAR,ar)
+ $(call allow-override,HOSTCC,gcc)
+ $(call allow-override,HOSTLD,ld)
+
+ # Some tools still require Clang, LLC and/or LLVM utils
+ $(call allow-override,CLANG,clang)
+ $(call allow-override,LLC,llc)
+ $(call allow-override,LLVM_CONFIG,llvm-config)
+ $(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
+ $(call allow-override,LLVM_STRIP,llvm-strip)
endif
CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
--
2.34.1
^ permalink raw reply related
* [PATCH 3/6] selftests: Remove unused LLD variable
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
This file was mostly copied from selftests/bpf/Makefile, but the LLD
variable is not used here. Also, this copied block didn't get the same
fixes as the original one did later.
Remove it to avoid confusion and so future fixes don't have to be in two
places.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/testing/selftests/hid/Makefile | 7 -------
1 file changed, 7 deletions(-)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 50ec9e0406ab..96071b4800e8 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -105,13 +105,6 @@ $(MAKE_DIRS):
$(call msg,MKDIR,,$@)
$(Q)mkdir -p $@
-# LLVM's ld.lld doesn't support all the architectures, so use it only on x86
-ifeq ($(SRCARCH),x86)
-LLD := lld
-else
-LLD := ld
-endif
-
DEFAULT_BPFTOOL := $(HOST_SCRATCH_DIR)/sbin/bpftool
TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
--
2.34.1
^ permalink raw reply related
* [PATCH 4/6] tools/build: Allow versioning LLVM readelf
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Documentation/kbuild/llvm.rst mentions that readelf is included in the
LLVM toolchain, but it's not currently included in this block.
Add it so that LLVM=... options also apply to readelf. Users in tools/
were Perf which was hardcoding it, and another was the BPF makefile.
Both already include Makefile.include so convert them to use the new
variable.
It also didn't have the cross compile prefix, so either readelf didn't
mind opening cross binaries, or it wasn't working for cross builds.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/lib/bpf/Makefile | 8 ++++----
tools/perf/Makefile.perf | 1 -
tools/scripts/Makefile.include | 2 ++
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 168140f8e646..180dca9c57c8 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -114,12 +114,12 @@ PC_FILE := $(addprefix $(OUTPUT),$(PC_FILE))
TAGS_PROG := $(if $(shell which etags 2>/dev/null),etags,ctags)
-GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN_SHARED) | \
+GLOBAL_SYM_COUNT = $(shell $(READELF) -s --wide $(BPF_IN_SHARED) | \
cut -d "@" -f1 | sed 's/_v[0-9]_[0-9]_[0-9].*//' | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}' | \
sort -u | wc -l)
-VERSIONED_SYM_COUNT = $(shell readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
+VERSIONED_SYM_COUNT = $(shell $(READELF) --dyn-syms --wide $(OUTPUT)libbpf.so | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}' | \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
@@ -182,12 +182,12 @@ check_abi: $(OUTPUT)libbpf.so $(VERSION_SCRIPT)
"versioned symbols in $^ ($(VERSIONED_SYM_COUNT))." \
"Please make sure all LIBBPF_API symbols are" \
"versioned in $(VERSION_SCRIPT)." >&2; \
- readelf -s --wide $(BPF_IN_SHARED) | \
+ $(READELF) -s --wide $(BPF_IN_SHARED) | \
cut -d "@" -f1 | sed 's/_v[0-9]_[0-9]_[0-9].*//' | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
sort -u > $(OUTPUT)libbpf_global_syms.tmp; \
- readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
+ $(READELF) --dyn-syms --wide $(OUTPUT)libbpf.so | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}'| \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | \
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 0aba14f22a06..63276bf55856 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -215,7 +215,6 @@ FLEX ?= flex
BISON ?= bison
STRIP = strip
AWK = awk
-READELF ?= readelf
# include Makefile.config by default and rule out
# non-config cases
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index e81e5b479c56..380ad84ac51e 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -73,6 +73,7 @@ ifneq ($(LLVM),)
$(call allow-override,LLC,$(LLVM_PREFIX)llc$(LLVM_SUFFIX))
$(call allow-override,LLVM_CONFIG,$(LLVM_PREFIX)llvm-config$(LLVM_SUFFIX))
$(call allow-override,LLVM_OBJCOPY,$(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX))
+ $(call allow-override,READELF,$(LLVM_PREFIX)llvm-readelf$(LLVM_SUFFIX))
else
# Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
$(call allow-override,CC,$(CROSS_COMPILE)gcc)
@@ -80,6 +81,7 @@ else
$(call allow-override,LD,$(CROSS_COMPILE)ld)
$(call allow-override,CXX,$(CROSS_COMPILE)g++)
$(call allow-override,STRIP,$(CROSS_COMPILE)strip)
+ $(call allow-override,READELF,$(CROSS_COMPILE)readelf)
# Host versions aren't prefixed
$(call allow-override,HOSTAR,ar)
--
2.34.1
^ permalink raw reply related
* [PATCH 5/6] tools/build: selftests: Allow versioning LLVM lld
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Building with LLVM=... could result in a different version of lld being
used than the main toolchain for liburandom_read.so because it's
hardcoded to "lld" in this makefile.
Make it consistent with the rest of the LLVM toolchain by adding an LLD
variable to Makefile.include. Keep the fallback for other architectures
in tools/testing/selftests/bpf/Makefile as it seems like it's something
specific to this make rule and shouldn't be global.
Clang accepts either a full path or "ld.lld-15" style inputs to
-fuse-ld= so this will work with LLD defined the same way as the other
LLVM tools. However, for full paths, we need to use ".../ld.lld" instead
of the generic driver "lld", but I don't think the original use of "lld"
was significant as this is always a linux build.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/scripts/Makefile.include | 2 ++
tools/testing/selftests/bpf/Makefile | 8 ++++----
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index 380ad84ac51e..5c2d505cba62 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -67,6 +67,7 @@ ifneq ($(LLVM),)
$(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
$(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
$(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
+ $(call allow-override,LLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
$(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX))
$(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
$(call allow-override,LLVM_STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
@@ -91,6 +92,7 @@ else
# Some tools still require Clang, LLC and/or LLVM utils
$(call allow-override,CLANG,clang)
$(call allow-override,LLC,llc)
+ $(call allow-override,LLD,ld.lld)
$(call allow-override,LLVM_CONFIG,llvm-config)
$(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
$(call allow-override,LLVM_STRIP,llvm-strip)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6ef6872adbc3..44ba829e5d4d 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -245,9 +245,9 @@ $(OUTPUT)/%:%.c
# LLVM's ld.lld doesn't support all the architectures, so use it only on x86
ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 riscv))
-LLD := lld
+USE_LD := $(LLD)
else
-LLD := $(shell command -v $(LD))
+USE_LD := $(shell command -v $(LD))
endif
# Filter out -static for liburandom_read.so and its dependent targets so that static builds
@@ -258,7 +258,7 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom
$(filter-out -static,$(CFLAGS) $(LDFLAGS)) \
$(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \
-Wno-unused-command-line-argument \
- -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
+ -fuse-ld=$(USE_LD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
-Wl,--version-script=liburandom_read.map \
-fPIC -shared -o $@
@@ -268,7 +268,7 @@ $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_r
$(filter-out -static,$(CFLAGS) $(LDFLAGS)) $(filter %.c,$^) \
-Wno-unused-command-line-argument \
-lurandom_read $(filter-out -static,$(LDLIBS)) -L$(OUTPUT) \
- -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
+ -fuse-ld=$(USE_LD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
-Wl,-rpath=. -o $@
$(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
--
2.34.1
^ permalink raw reply related
* [PATCH 6/6] tools/build: selftests: Remove some duplicate toolchain definitions
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Try to remove some, but not all duplicate toolchain definitions. In
these instances, their makefiles already include
tools/scripts/Makefile.include which defines these in a consistent way.
STRIP is the only one that was set with an '=', but I don't think it
was significant so that difference can be dropped.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/bpf/resolve_btfids/Makefile | 3 ---
tools/lib/api/Makefile | 4 ----
tools/lib/subcmd/Makefile | 4 ----
tools/lib/symbol/Makefile | 4 ----
tools/perf/Makefile.perf | 6 ------
tools/testing/selftests/bpf/Makefile | 1 -
6 files changed, 22 deletions(-)
diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
index 7672208f65e4..6fdb6302e0a2 100644
--- a/tools/bpf/resolve_btfids/Makefile
+++ b/tools/bpf/resolve_btfids/Makefile
@@ -20,9 +20,6 @@ HOST_OVERRIDES := AR="$(HOSTAR)" CC="$(HOSTCC)" LD="$(HOSTLD)" ARCH="$(HOSTARCH)
CROSS_COMPILE="" CLANG_CROSS_FLAGS="" EXTRA_CFLAGS="$(HOSTCFLAGS)"
RM ?= rm
-HOSTCC ?= gcc
-HOSTLD ?= ld
-HOSTAR ?= ar
HOSTPKG_CONFIG ?= pkg-config
CROSS_COMPILE =
diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
index 8665c799e0fa..a228fdb5adba 100644
--- a/tools/lib/api/Makefile
+++ b/tools/lib/api/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-AR ?= $(CROSS_COMPILE)ar
-LD ?= $(CROSS_COMPILE)ld
-
MAKEFLAGS += --no-print-directory
INSTALL = install
diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index 8703ab487b68..9f1ddcf0504d 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-LD ?= $(CROSS_COMPILE)ld
-AR ?= $(CROSS_COMPILE)ar
-
RM = rm -f
MAKEFLAGS += --no-print-directory
diff --git a/tools/lib/symbol/Makefile b/tools/lib/symbol/Makefile
index 426b845edfac..d692abe8add6 100644
--- a/tools/lib/symbol/Makefile
+++ b/tools/lib/symbol/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-AR ?= $(CROSS_COMPILE)ar
-LD ?= $(CROSS_COMPILE)ld
-
MAKEFLAGS += --no-print-directory
INSTALL = install
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 63276bf55856..948abfd2ee8d 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -174,11 +174,6 @@ endef
LD += $(EXTRA_LDFLAGS)
-HOSTCC ?= gcc
-HOSTLD ?= ld
-HOSTAR ?= ar
-CLANG ?= clang
-
# Some distros provide the command $(CROSS_COMPILE)pkg-config for
# searching packges installed with Multiarch. Use it for cross
# compilation if it is existed.
@@ -213,7 +208,6 @@ FIND = find
INSTALL = install
FLEX ?= flex
BISON ?= bison
-STRIP = strip
AWK = awk
# include Makefile.config by default and rule out
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 44ba829e5d4d..b3e356c34479 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -3,7 +3,6 @@ include ../../../build/Build.include
include ../../../scripts/Makefile.arch
include ../../../scripts/Makefile.include
-CXX ?= $(CROSS_COMPILE)g++
OBJCOPY ?= $(CROSS_COMPILE)objcopy
CURDIR := $(abspath .)
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-05-14 10:02 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260502124055.22475-3-clamor95@gmail.com>
On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> detection and common operations for EC's functions.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/mfd/Kconfig | 14 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> include/linux/mfd/asus-transformer-ec.h | 162 +++++
> 4 files changed, 939 insertions(+)
> create mode 100644 drivers/mfd/asus-transformer-ec.c
> create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7192c9d1d268..5aa4facfd2df 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> additional drivers must be enabled in order to use the
> functionality of the device.
>
> +config MFD_ASUS_TRANSFORMER_EC
> + tristate "ASUS Transformer's embedded controller"
> + depends on I2C && OF
> + help
> + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> +
> + This provides shared glue for functional part drivers:
> + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> + leds-asus-transformer-ec, asus-transformer-ec-battery
> + and asus-transformer-ec-charger.
A 760 line driver deserves more than just a list of leaf drivers.
> + This driver can also be built as a module. If so, the module
> + will be called asus-transformer-ec.
> +
> config MFD_AT91_USART
> tristate "AT91 USART Driver"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..fd80088d8a9a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 000000000000..75aa7ab99387
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c
> @@ -0,0 +1,762 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/device.h>
Should we sort the '#include' directives alphabetically? For instance,
'device.h' should typically appear before 'gpio/consumer.h'.
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#define ASUSEC_RSP_BUFFER_SIZE 8
> +
> +struct asus_ec_chip_data {
> + const char *name;
> + const struct mfd_cell *mfd_devices;
Use global `static const structs` instead.
Also, please do not pass MFD registration data through another
registration API like DT. It opens the gates to too much hackery. I'm
not saying that _this_ driver would do such a thing, but it's easier
just to keep the blanket rule in place.
> + unsigned int num_devices;
> + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> +};
> +
> +struct asus_ec_data {
> + struct asusec_info info;
You have 'data' and 'info' which a) using non-forthcoming nomenclature
and doesn't tell me anything and then you b) put 'info' in the device's
driver_data attribute which is very confusing. driver_data should be
for what we call ddata which I assume is expressed as 'data' here.
> + struct mutex ecreq_lock; /* prevent simultaneous access */
> + struct gpio_desc *ecreq;
If I hadn't seen the declaration, I'd have no idea this was a GPIO
descriptor. Please improve the nomenclature throughout.
> + struct i2c_client *self;
Again, please use standard naming conventions:
% git grep "struct i2c_client" | grep "\*self" | wc -l
0
% git grep "struct i2c_client" | grep "\*client" | wc -l
6304
% git grep "struct i2c_client" | grep "\*i2c" | wc -l
903
> + const struct asus_ec_chip_data *data;
'data', 'priv' and 'info' should be improved.
> + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
An array of chars called 'data'. This could be anything.
> + bool logging_disabled;
This debugging tool is probably never going to be used again.
Keep it local.
> +};
> +
> +struct dockram_ec_data {
> + struct mutex ctl_lock; /* prevent simultaneous access */
> + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> +};
> +
> +#define to_ec_data(ec) \
> + container_of(ec, struct asus_ec_data, info)
> +
> +/**
> + * asus_dockram_read - Read a register from the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to read.
> + * @buf: Byte array into which data will be read; must be large enough to
> + * hold the data returned by the DockRAM.
> + *
> + * This executes the DockRAM read based on the SMBus "block read" protocol
> + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> + * register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> +{
Have you considered using Regmap for register access instead of
implementing custom functions? Remaps already deals with caching and
locking mechanisms that you'd get for free.
This looks like it would be replaced with devm_regmap_init_i2c().
> + struct device *dev = &client->dev;
> + int ret;
> +
> + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> + ret = i2c_smbus_read_i2c_block_data(client, reg,
> + DOCKRAM_ENTRY_BUFSIZE, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> + return -EPROTO;
> + }
> +
> + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
Please remove all of these debug messages.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_read);
> +
> +/**
> + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to write to.
> + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> + *
> + * This executes the DockRAM write based on the SMBus "block write"
> + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> + * specified register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> +{
> + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> + return -EINVAL;
> +
> + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> +
> + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_write);
> +
> +/**
> + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> + * @client: Handle to the DockRAM device.
> + * @out: Pointer to a variable where the register value will be stored.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be set (via XOR).
> + *
> + * This performs a control register read if @out is provided and both @mask
> + * and @xor are zero. Otherwise, it performs a control register update if
> + * @mask and @xor are provided.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> + u64 xor)
> +{
> + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> + char *buf = priv->ctl_data;
> + u64 val;
> + int ret = 0;
> +
> + guard(mutex)(&priv->ctl_lock);
> +
> + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> + if (ret < 0)
Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
unless a positive return value has a specific meaning we need to handle?
> + goto exit;
> +
> + if (buf[0] != ASUSEC_CTL_SIZE) {
> + ret = -EPROTO;
> + goto exit;
> + }
> +
> + val = get_unaligned_le64(buf + 1);
> +
> + if (out)
> + *out = val;
> +
> + if (mask || xor) {
> + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> + }
> +
> +exit:
> + if (ret < 0)
> + dev_err(&client->dev, "Failed to access control flags: %d\n",
> + ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> +
> +static void asus_ec_remove_notifier(struct device *dev, void *res)
> +{
> + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> + struct notifier_block **nb = res;
> +
> + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> +}
> +
> +/**
> + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> + * ASUS EC blocking notifier chain.
> + * @pdev: Device requesting the notifier (used for resource management).
> + * @nb: Notifier block to be registered.
> + *
> + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> + * will be automatically unregistered when the requesting device is detached.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> + struct notifier_block *nb)
> +{
Hand-rolling devres managed resources is usually reserved for subsystem
level API calls. Why do the usual device driver .remove() handling work
for you?
> + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> + struct notifier_block **res;
> + int ret;
> +
> + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + *res = nb;
> + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> + if (ret) {
> + devres_free(res);
> + return ret;
> + }
> +
> + devres_add(&pdev->dev, res);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> +
> +static int asus_ec_signal_request(const struct asusec_info *ec)
> +{
> + struct asus_ec_data *priv = to_ec_data(ec);
> +
> + guard(mutex)(&priv->ecreq_lock);
> +
> + dev_dbg(&priv->self->dev, "EC request\n");
> +
> + gpiod_set_value_cansleep(priv->ecreq, 1);
> + msleep(50);
> +
> + gpiod_set_value_cansleep(priv->ecreq, 0);
> + msleep(200);
> +
> + return 0;
> +}
> +
> +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> +{
> + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> +
> + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> +
> + return ret;
> +}
> +
> +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> +{
> + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> + sizeof(priv->ec_data),
> + priv->ec_data);
> +
> + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> + sizeof(priv->ec_data), priv->ec_data,
> + ret, in_irq ? "; in irq" : "");
> +
> + return ret;
> +}
No abstractions for the sake of it. All this goes away with Regmap anyway.
> +
> +/**
> + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @data: The 16-bit command (word) to be sent.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> +{
> + return asus_ec_write(to_ec_data(ec), data);
> +}
Is this wrapper function strictly necessary? We should try to avoid
superfluous abstractions that do nothing but call another function.
> +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> +
> +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> +{
> + int retry = ASUSEC_RSP_BUFFER_SIZE;
> +
> + while (retry--) {
> + if (asus_ec_read(priv, false) < 0)
> + continue;
> +
> + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> + continue;
> +
> + break;
> + }
> +}
> +
> +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> + const char *name, char **out)
> +{
> + char buf[DOCKRAM_ENTRY_BUFSIZE];
> + int ret;
> +
> + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (!priv->logging_disabled)
> + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> + buf[0], buf + 1);
> +
> + if (out)
> + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> +
> + return 0;
> +}
> +
> +static int asus_ec_reset(struct asus_ec_data *priv)
> +{
> + int retry, ret;
> +
> + for (retry = 0; retry < 3; retry++) {
> + ret = asus_ec_write(priv, 0);
> + if (!ret)
> + return 0;
> +
> + msleep(300);
> + }
> +
> + return ret;
> +}
> +
> +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> +{
> + u64 flag;
> + int ret;
> +
> + ret = asus_ec_get_ctl(&priv->info, &flag);
> + if (ret < 0)
> + return ret;
> +
> + flag &= ASUSEC_CTL_SUSB_MODE;
> + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> + flag ? "susb on when receive ec_req" :
> + "susb on when system wakeup");
> +
> + return 0;
> +}
> +
> +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> +{
> + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> + "normal");
> +
> + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> +}
> +
> +static int asus_ec_detect(struct asus_ec_data *priv)
> +{
> + char *model = NULL;
> + int ret;
> +
> + ret = asus_ec_reset(priv);
> + if (ret)
> + goto err_exit;
> +
> + asus_ec_clear_buffer(priv);
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> + if (ret)
> + goto err_exit;
> +
> + priv->logging_disabled = true;
> +
> + ret = asus_ec_magic_debug(priv);
> + if (ret)
> + goto err_exit;
> +
> + priv->info.model = model;
> + priv->info.name = priv->data->name;
> +
> + if (priv->data->clr_fmode)
> + asus_ec_set_factory_mode(priv, false);
> +
> +err_exit:
> + if (ret)
> + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> +{
> + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> +
> + switch (code) {
> + case ASUSEC_SMI_HANDSHAKE:
> + case ASUSEC_SMI_RESET:
> + asus_ec_detect(priv);
> + break;
> + }
> +}
> +
> +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> +{
> + struct asus_ec_data *priv = dev_id;
> + unsigned long notify_action;
> + int ret;
> +
> + ret = asus_ec_read(priv, true);
> + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> + return IRQ_NONE;
> +
> + notify_action = priv->ec_data[1];
> + if (notify_action & ASUSEC_SMI_MASK) {
> + unsigned int code = priv->ec_data[2];
> +
> + asus_ec_handle_smi(priv, code);
> +
> + notify_action |= code << 8;
> + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> + }
> +
> + blocking_notifier_call_chain(&priv->info.notify_list,
> + notify_action, priv->ec_data);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static ssize_t dockram_read(struct file *filp, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct i2c_client *client = filp->private_data;
> + unsigned int reg, rsize;
> + ssize_t n_read = 0, val;
> + loff_t off = *ppos;
> + char *data;
> + int ret;
> +
> + reg = off / DOCKRAM_ENTRY_SIZE;
> + off %= DOCKRAM_ENTRY_SIZE;
> + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> +
> + if (!count)
> + return 0;
> +
> + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> +
> + while (reg < DOCKRAM_ENTRIES) {
> + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> +
> + if (len > rsize)
> + len = rsize;
> +
> + ret = asus_dockram_read(client, reg, data);
> + if (ret < 0) {
> + if (!n_read)
> + n_read = ret;
> + break;
> + }
> +
> + val = copy_to_user(buf, data + 1 + off, len);
> + if (val == len)
> + return -EFAULT;
> +
> + *ppos += len;
> + n_read += len;
> +
> + if (len == rsize)
> + break;
> +
> + rsize -= len;
> + buf += len;
> + off = 0;
> + ++reg;
> + }
> +
> + kfree(data);
> +
> + return n_read;
> +}
> +
> +static int dockram_write_one(struct i2c_client *client, int reg,
> + const char __user *buf, size_t count)
> +{
> + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> + int ret;
> +
> + if (!count || count > DOCKRAM_ENTRY_SIZE)
> + return -EINVAL;
> + if (buf[0] != count - 1)
> + return -EINVAL;
> +
> + guard(mutex)(&priv->ctl_lock);
> +
> + priv->ctl_data[0] = (u8)count;
> + memcpy(priv->ctl_data + 1, buf, count);
> + ret = asus_dockram_write(client, reg, priv->ctl_data);
> +
> + return ret;
> +}
> +
> +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct i2c_client *client = filp->private_data;
> + unsigned int reg;
> + loff_t off = *ppos;
> + int ret;
> +
> + if (off % DOCKRAM_ENTRY_SIZE != 0)
> + return -EINVAL;
> +
> + reg = off / DOCKRAM_ENTRY_SIZE;
> + if (reg >= DOCKRAM_ENTRIES)
> + return -EINVAL;
> +
> + ret = dockram_write_one(client, reg, buf, count);
> +
> + return ret < 0 ? ret : count;
> +}
> +
> +static const struct debugfs_short_fops dockram_fops = {
> + .read = dockram_read,
> + .write = dockram_write,
> + .llseek = default_llseek,
> +};
You should not be giving userspace free reign over device memory.
If you switch to Regmap, you can use regmap-debugfs.c for this.
> +static int control_reg_get(void *ec, u64 *val)
> +{
> + return asus_ec_get_ctl(ec, val);
> +}
> +
> +static int control_reg_set(void *ec, u64 val)
> +{
> + return asus_ec_update_ctl(ec, ~0ull, val);
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> + control_reg_set, "%016llx\n");
> +
> +static int ec_request_set(void *ec, u64 val)
> +{
> + if (val)
> + asus_ec_signal_request(ec);
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> +
> +static int ec_irq_set(void *ec, u64 val)
> +{
> + struct asus_ec_data *priv = to_ec_data(ec);
> +
> + if (val)
> + irq_wake_thread(priv->self->irq, priv);
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> +
> +static void asus_ec_debugfs_remove(void *debugfs_root)
> +{
> + debugfs_remove_recursive(debugfs_root);
> +}
> +
> +static void devm_asus_ec_debugfs_init(struct device *dev)
> +{
> + struct asusec_info *ec = dev_get_drvdata(dev);
> + struct asus_ec_data *priv = to_ec_data(ec);
> + struct dentry *debugfs_root, *dockram_dir;
> + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> + priv->data->name);
> +
> + debugfs_root = debugfs_create_dir(name, NULL);
> + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> +
> + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> + &ec_irq_fops);
> + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> + &ec_request_fops);
> + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> + &control_reg_fops);
> + debugfs_create_file("dockram", 0644, dockram_dir,
> + priv->info.dockram, &dockram_fops);
> +
> + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> +}
Why is this being controlled via debugfs?
Use the correct kernel APIs instead.
If this is just for development, keep it and all of the extra verbose
printing in the BSP tree. It does not belong in the upstream kernel.
> +static void asus_ec_release_dockram_dev(void *client)
> +{
> + i2c_unregister_device(client);
> +}
> +
> +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> +{
> + struct i2c_client *parent = to_i2c_client(dev);
> + struct i2c_client *dockram;
> + struct dockram_ec_data *priv;
> + int ret;
> +
> + dockram = i2c_new_ancillary_device(parent, "dockram",
> + parent->addr + 2);
> + if (IS_ERR(dockram))
> + return dockram;
> +
> + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> + dockram);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return ERR_PTR(-ENOMEM);
> +
> + i2c_set_clientdata(dockram, priv);
> + mutex_init(&priv->ctl_lock);
> +
> + return dockram;
> +}
> +
> +static int asus_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct asus_ec_data *priv;
Could we use a clearer, more specific name for the private data variable
instead of the generic term 'priv'? Using something like 'ddata' is usually
preferred.
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return dev_err_probe(dev, -ENXIO,
> + "I2C bus is missing required SMBus block mode support\n");
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->data = device_get_match_data(dev);
> + if (!priv->data)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, priv);
> + priv->self = client;
> +
> + priv->info.dockram = devm_asus_dockram_get(dev);
> + if (IS_ERR(priv->info.dockram))
> + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> + "failed to get dockram\n");
> +
> + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->ecreq))
> + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> + "failed to get request GPIO\n");
"get" or "request"
> + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> + mutex_init(&priv->ecreq_lock);
> +
> + asus_ec_signal_request(&priv->info);
> +
> + ret = asus_ec_detect(priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + &asus_ec_interrupt,
> + IRQF_ONESHOT | IRQF_SHARED,
> + client->name, priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> + client->dev.coherent_dma_mask = 0;
> + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> +
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + devm_asus_ec_debugfs_init(dev);
> +
> + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> + priv->data->num_devices, NULL, 0, NULL);
> +}
> +
> +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-kbc",
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> + .name = "dock",
> + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> + .clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 1,
Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
> + }, {
> + .name = "asus-transformer-ec-charger",
> + .id = 1,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 1,
> + }, {
> + .name = "asus-transformer-ec-keys",
> + }, {
> + .name = "asus-transformer-ec-kbc",
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> + .name = "dock",
> + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> + .clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 0,
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> + .name = "pad",
> + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> + .clr_fmode = true,
> +};
> +
> +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-charger",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 0,
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> + .name = "pad",
> + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> + .clr_fmode = true,
> +};
> +
> +static const struct of_device_id asus_ec_match[] = {
> + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
Could we use the match table's data field to store an 'enum' or integer ID
instead of passing platform data via the '.data' field? We could then use a
'switch' statement in the C code to select the correct 'mfd_cell' array.
> + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, asus_ec_match);
> +
> +static struct i2c_driver asus_ec_driver = {
> + .driver = {
> + .name = "asus-transformer-ec",
> + .of_match_table = asus_ec_match,
> + },
> + .probe = asus_ec_probe,
> +};
> +module_i2c_driver(asus_ec_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> new file mode 100644
> index 000000000000..0a72de40352e
> --- /dev/null
> +++ b/include/linux/mfd/asus-transformer-ec.h
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> +#define __MFD_ASUS_TRANSFORMER_EC_H
> +
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +
> +struct i2c_client;
> +
> +struct asusec_info {
> + const char *model;
> + const char *name;
> + struct i2c_client *dockram;
> + struct workqueue_struct *wq;
> + struct blocking_notifier_head notify_list;
> +};
> +
> +#define DOCKRAM_ENTRIES 0x100
> +#define DOCKRAM_ENTRY_SIZE 32
> +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> +
> +/* interrupt sources */
> +#define ASUSEC_OBF_MASK BIT(0)
> +#define ASUSEC_KEY_MASK BIT(2)
> +#define ASUSEC_KBC_MASK BIT(3)
> +#define ASUSEC_AUX_MASK BIT(5)
> +#define ASUSEC_SCI_MASK BIT(6)
> +#define ASUSEC_SMI_MASK BIT(7)
> +
> +/* SMI notification codes */
> +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> +#define ASUSEC_SMI_WAKE 0x53
> +#define ASUSEC_SMI_RESET 0x5f
> +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> +
> +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> + (ASUSEC_SMI_##code << 8))
> +
> +/* control register [0x0a] layout */
> +#define ASUSEC_CTL_SIZE 8
> +
> +/*
> + * EC reports power from 40-pin connector in the LSB of the control
> + * register. The following values have been observed (xor 0x02):
> + *
> + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> + */
> +
> +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> +
> +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> +
> +#define ASUSEC_WRITE_BUF 0x64
> +#define ASUSEC_READ_BUF 0x6a
> +
> +/* dockram comm */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> +int asus_dockram_access_ctl(struct i2c_client *client,
> + u64 *out, u64 mask, u64 xor);
> +
> +/* EC public API */
> +
> +/**
> + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> + *
> + * Returns a pointer to the asusec_info structure.
> + */
> +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> +{
> + return dev_get_drvdata(pdev->dev.parent);
> +}
This is both misleading and already exists.
> +
> +/**
> + * asus_ec_get_ctl - Read from the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @out: Pointer to the variable where the register value will be stored.
> + *
> + * Performs a control register read and stores the value in @out.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> +{
> + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> +}
> +
> +/**
> + * asus_ec_update_ctl - Update the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be toggled or set (via XOR).
> + *
> + * Performs a read-modify-write update on the control register using
> + * the provided @mask and @xor values.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> + u64 mask, u64 xor)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> +}
> +
> +/**
> + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be set.
> + *
> + * Sets bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> +}
> +
> +/**
> + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + *
> + * Clears bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> +}
We don't typically allow abstraction for the sake of abstraction.
> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> +int devm_asus_ec_register_notifier(struct platform_device *dev,
> + struct notifier_block *nb);
> +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH 1/6] tools/build: Allow versioning of all LLVM tools defined in Makefile.include
From: bot+bpf-ci @ 2026-05-14 10:02 UTC (permalink / raw)
To: james.clark, nathan, nick.desaulniers+lkml, morbo, justinstitt,
jikos, bentiss, shuah, peterz, mingo, acme, namhyung,
mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter,
andrii, eddyz87, ast, daniel, martin.lau, memxor, song,
yonghong.song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, james.clark, leo.yan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
In-Reply-To: <20260514-james-perf-llvm-version-v1-1-6cac1a9a4c8d@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
> tools/build: Allow versioning of all LLVM tools defined in Makefile.include
>
> The version of LLVM tools can be given on the build command with
> LLVM=-15, but this isn't applied to all tools. For example $(CC) gets
> versioned, but $(CLANG) doesn't. This causes a Perf build with LTO=1 to
> fail with an error about mixed clang versions:
>
> ld.lld: error: libperf/core.o: Unknown attribute kind (86)
> (Producer: 'LLVM18.1.8' Reader: 'LLVM 15.0.7')
>
> This file has two "ifneq ($(LLVM),)" blocks adjacent to each other, so
> merge these blocks making it obvious that all tools should be versioned
> consistently and there is nothing special about each block.
>
> This also reveals that ?= and "allow-override" are used inconsistently
> between the blocks. "allow-override" is technically only required for
> builtin variables, but isn't only used on them, and doesn't do any harm
> if used on a non-builtin. Make them all "allow-override" for
> consistency. The only functional difference this will cause is if there
> is a file level definition of one of the variables followed by an
> "#include of Makefile.include" which will now overwrite. But this isn't
> done and in a later commit some of the duplicate definitions will be
> removed for good measure.
>
> There are also some other LLVM tools that are not defined here and will
> be moved in a later commit.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
This looks like a bug fix that corrects inconsistent LLVM tool versioning
introduced in an earlier commit. Should this include:
Fixes: e9c281928c24 ("kbuild: Make $(LLVM) more flexible")
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25853166705
^ permalink raw reply
* Re: [PATCH v6 5/7] leds: Add driver for ASUS Transformer LEDs
From: Lee Jones @ 2026-05-14 10:09 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260502124055.22475-6-clamor95@gmail.com>
On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> ASUS Transformer tablets have a green and an amber LED on both the Pad
> and the Dock. If both LEDs are enabled simultaneously, the emitted light
> will be yellow.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/leds/Kconfig | 11 ++++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-asus-transformer-ec.c | 79 +++++++++++++++++++++++++
> 3 files changed, 91 insertions(+)
> create mode 100644 drivers/leds/leds-asus-transformer-ec.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..f637d23400a8 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> To compile this driver as a module, choose M here: the module
> will be called leds-as3668.
>
> +config LEDS_ASUS_TRANSFORMER_EC
> + tristate "LED Support for Asus Transformer charging LED"
> + depends on LEDS_CLASS
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + This option enables support for charging indicator on
> + Asus Transformer's Pad and it's Dock.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-asus-transformer-ec.
> +
> config LEDS_AW200XX
> tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..d5395c3f1124 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
> +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC) += leds-asus-transformer-ec.o
> obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
> obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 000000000000..3186038e3be7
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +static void asus_ec_led_set_brightness_amber(struct led_classdev *led,
> + enum led_brightness brightness)
> +{
> + const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> + if (brightness)
> + asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
Why not Regmap?
> + else
> + asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
> +}
> +
> +static void asus_ec_led_set_brightness_green(struct led_classdev *led,
> + enum led_brightness brightness)
> +{
> + const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> + if (brightness)
> + asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> + else
> + asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> +}
These are identical bar one variable.
> +static int asus_ec_led_probe(struct platform_device *pdev)
> +{
> + struct asusec_info *ec = cell_to_ec(pdev);
> + struct device *dev = &pdev->dev;
> + struct led_classdev *amber_led, *green_led;
> + int ret;
> +
> + platform_set_drvdata(pdev, ec);
This appears to be unused.
> + amber_led = devm_kzalloc(dev, sizeof(*amber_led), GFP_KERNEL);
> + if (!amber_led)
> + return -ENOMEM;
> +
> + amber_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::amber", ec->name);
> + amber_led->max_brightness = 1;
> + amber_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + amber_led->brightness_set = asus_ec_led_set_brightness_amber;
> +
> + ret = devm_led_classdev_register(dev, amber_led);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register amber LED\n");
> +
> + green_led = devm_kzalloc(dev, sizeof(*green_led), GFP_KERNEL);
> + if (!green_led)
> + return -ENOMEM;
> +
> + green_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::green", ec->name);
> + green_led->max_brightness = 1;
> + green_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + green_led->brightness_set = asus_ec_led_set_brightness_green;
> +
> + ret = devm_led_classdev_register(dev, green_led);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register green LED\n");
Imagine instead of 2 LEDs, you had 20. Would you copy and paste this 20
times? Please re-author this as though there were many more so it
remains efficient and scaleable.
> + return 0;
> +}
> +
> +static struct platform_driver asus_ec_led_driver = {
> + .driver.name = "asus-transformer-ec-led",
> + .probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH 1/6] tools/build: Allow versioning of all LLVM tools defined in Makefile.include
From: James Clark @ 2026-05-14 10:13 UTC (permalink / raw)
To: bot+bpf-ci
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, leo.yan, martin.lau, clm, ihor.solodrai, nathan,
nick.desaulniers+lkml, morbo, justinstitt, jikos, bentiss, shuah,
peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
jolsa, irogers, adrian.hunter, andrii, eddyz87, ast, daniel,
martin.lau, memxor, song, yonghong.song
In-Reply-To: <998802d0c3bb407e9bcc61e4fe6db2e3eed7ac06c7c6eca0ed0dd5409a8c2801@mail.kernel.org>
On 14/05/2026 11:02 am, bot+bpf-ci@kernel.org wrote:
>> tools/build: Allow versioning of all LLVM tools defined in Makefile.include
>>
>> The version of LLVM tools can be given on the build command with
>> LLVM=-15, but this isn't applied to all tools. For example $(CC) gets
>> versioned, but $(CLANG) doesn't. This causes a Perf build with LTO=1 to
>> fail with an error about mixed clang versions:
>>
>> ld.lld: error: libperf/core.o: Unknown attribute kind (86)
>> (Producer: 'LLVM18.1.8' Reader: 'LLVM 15.0.7')
>>
>> This file has two "ifneq ($(LLVM),)" blocks adjacent to each other, so
>> merge these blocks making it obvious that all tools should be versioned
>> consistently and there is nothing special about each block.
>>
>> This also reveals that ?= and "allow-override" are used inconsistently
>> between the blocks. "allow-override" is technically only required for
>> builtin variables, but isn't only used on them, and doesn't do any harm
>> if used on a non-builtin. Make them all "allow-override" for
>> consistency. The only functional difference this will cause is if there
>> is a file level definition of one of the variables followed by an
>> "#include of Makefile.include" which will now overwrite. But this isn't
>> done and in a later commit some of the duplicate definitions will be
>> removed for good measure.
>>
>> There are also some other LLVM tools that are not defined here and will
>> be moved in a later commit.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>
> This looks like a bug fix that corrects inconsistent LLVM tool versioning
> introduced in an earlier commit. Should this include:
>
> Fixes: e9c281928c24 ("kbuild: Make $(LLVM) more flexible")
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25853166705
I did consider a fixes tag, but the only real bug is with Perf and
LTO=1, and building it in this way has obviously never worked, so you
could also argue this is new behavior.
I think it's quite and edge case and has some risk to break other tools
if backported so I left the tag off.
^ permalink raw reply
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-05-14 10:31 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260514100205.GG305027@google.com>
чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/mfd/Kconfig | 14 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> > include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > 4 files changed, 939 insertions(+)
> > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> > additional drivers must be enabled in order to use the
> > functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > + tristate "ASUS Transformer's embedded controller"
> > + depends on I2C && OF
> > + help
> > + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > + This provides shared glue for functional part drivers:
> > + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > + leds-asus-transformer-ec, asus-transformer-ec-battery
> > + and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>
Ok, I will make some meaningful description.
> > + This driver can also be built as a module. If so, the module
> > + will be called asus-transformer-ec.
> > +
> > config MFD_AT91_USART
> > tristate "AT91 USART Driver"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> > obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> > obj-$(CONFIG_MFD_SM501) += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> > obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>
Yes, it is my bad, I will fix it.
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE 8
> > +
> > +struct asus_ec_chip_data {
> > + const char *name;
> > + const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT. It opens the gates to too much hackery. I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>
Yes, you have already made it clear in my CPCAP patchset. I have it
applied for the v7 here.
> > + unsigned int num_devices;
> > + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > + struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing. driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>
asusec_info is shared among all child devices and is exposed while
remaining elements of this struct are for internal use only.
> > + struct mutex ecreq_lock; /* prevent simultaneous access */
> > + struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor. Please improve the nomenclature throughout.
>
> > + struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>
ok, noted.
> > + const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'. This could be anything.
>
Do you have a comprehensive list of name conventions you find suitable?
> > + bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > + struct mutex ctl_lock; /* prevent simultaneous access */
> > + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > + container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + * hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions? Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>
I will consider this, thank you.
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(client, reg,
> > + DOCKRAM_ENTRY_BUFSIZE, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > + return -EPROTO;
> > + }
> > +
> > + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>
Why debug messages cannot be preserved? They are specifically marked as dev_dbg
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > +
> > + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > + u64 xor)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + char *buf = priv->ctl_data;
> > + u64 val;
> > + int ret = 0;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>
Sure, I will switch to 'if (ret)'
> > + goto exit;
> > +
> > + if (buf[0] != ASUSEC_CTL_SIZE) {
> > + ret = -EPROTO;
> > + goto exit;
> > + }
> > +
> > + val = get_unaligned_le64(buf + 1);
> > +
> > + if (out)
> > + *out = val;
> > +
> > + if (mask || xor) {
> > + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + }
> > +
> > +exit:
> > + if (ret < 0)
> > + dev_err(&client->dev, "Failed to access control flags: %d\n",
> > + ret);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > + struct notifier_block **nb = res;
> > +
> > + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + * ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > + struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls. Why do the usual device driver .remove() handling work
> for you?
>
This is used by 3 subdevices: serio, keys and charger, so this just
seems cleaner way to register and deregister notifier.
> > + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > + struct notifier_block **res;
> > + int ret;
> > +
> > + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + *res = nb;
> > + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > + if (ret) {
> > + devres_free(res);
> > + return ret;
> > + }
> > +
> > + devres_add(&pdev->dev, res);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + guard(mutex)(&priv->ecreq_lock);
> > +
> > + dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 1);
> > + msleep(50);
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 0);
> > + msleep(200);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > + sizeof(priv->ec_data),
> > + priv->ec_data);
> > +
> > + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > + sizeof(priv->ec_data), priv->ec_data,
> > + ret, in_irq ? "; in irq" : "");
> > +
> > + return ret;
> > +}
>
> No abstractions for the sake of it. All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > + return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>
This one is used by serio and keys too. I will see what I can do with regmaps.
> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > + int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > + while (retry--) {
> > + if (asus_ec_read(priv, false) < 0)
> > + continue;
> > +
> > + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > + continue;
> > +
> > + break;
> > + }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > + const char *name, char **out)
> > +{
> > + char buf[DOCKRAM_ENTRY_BUFSIZE];
> > + int ret;
> > +
> > + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!priv->logging_disabled)
> > + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > + buf[0], buf + 1);
> > +
> > + if (out)
> > + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > + int retry, ret;
> > +
> > + for (retry = 0; retry < 3; retry++) {
> > + ret = asus_ec_write(priv, 0);
> > + if (!ret)
> > + return 0;
> > +
> > + msleep(300);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > + u64 flag;
> > + int ret;
> > +
> > + ret = asus_ec_get_ctl(&priv->info, &flag);
> > + if (ret < 0)
> > + return ret;
> > +
> > + flag &= ASUSEC_CTL_SUSB_MODE;
> > + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > + flag ? "susb on when receive ec_req" :
> > + "susb on when system wakeup");
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > + "normal");
> > +
> > + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > + char *model = NULL;
> > + int ret;
> > +
> > + ret = asus_ec_reset(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + asus_ec_clear_buffer(priv);
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->logging_disabled = true;
> > +
> > + ret = asus_ec_magic_debug(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->info.model = model;
> > + priv->info.name = priv->data->name;
> > +
> > + if (priv->data->clr_fmode)
> > + asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > + if (ret)
> > + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > + switch (code) {
> > + case ASUSEC_SMI_HANDSHAKE:
> > + case ASUSEC_SMI_RESET:
> > + asus_ec_detect(priv);
> > + break;
> > + }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > + struct asus_ec_data *priv = dev_id;
> > + unsigned long notify_action;
> > + int ret;
> > +
> > + ret = asus_ec_read(priv, true);
> > + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > + return IRQ_NONE;
> > +
> > + notify_action = priv->ec_data[1];
> > + if (notify_action & ASUSEC_SMI_MASK) {
> > + unsigned int code = priv->ec_data[2];
> > +
> > + asus_ec_handle_smi(priv, code);
> > +
> > + notify_action |= code << 8;
> > + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > + }
> > +
> > + blocking_notifier_call_chain(&priv->info.notify_list,
> > + notify_action, priv->ec_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg, rsize;
> > + ssize_t n_read = 0, val;
> > + loff_t off = *ppos;
> > + char *data;
> > + int ret;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + off %= DOCKRAM_ENTRY_SIZE;
> > + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > + if (!count)
> > + return 0;
> > +
> > + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > + while (reg < DOCKRAM_ENTRIES) {
> > + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > + if (len > rsize)
> > + len = rsize;
> > +
> > + ret = asus_dockram_read(client, reg, data);
> > + if (ret < 0) {
> > + if (!n_read)
> > + n_read = ret;
> > + break;
> > + }
> > +
> > + val = copy_to_user(buf, data + 1 + off, len);
> > + if (val == len)
> > + return -EFAULT;
> > +
> > + *ppos += len;
> > + n_read += len;
> > +
> > + if (len == rsize)
> > + break;
> > +
> > + rsize -= len;
> > + buf += len;
> > + off = 0;
> > + ++reg;
> > + }
> > +
> > + kfree(data);
> > +
> > + return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > + const char __user *buf, size_t count)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + int ret;
> > +
> > + if (!count || count > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > + if (buf[0] != count - 1)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + priv->ctl_data[0] = (u8)count;
> > + memcpy(priv->ctl_data + 1, buf, count);
> > + ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg;
> > + loff_t off = *ppos;
> > + int ret;
> > +
> > + if (off % DOCKRAM_ENTRY_SIZE != 0)
> > + return -EINVAL;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + if (reg >= DOCKRAM_ENTRIES)
> > + return -EINVAL;
> > +
> > + ret = dockram_write_one(client, reg, buf, count);
> > +
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > + .read = dockram_read,
> > + .write = dockram_write,
> > + .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>
Noted
> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > + return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > + return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > + control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > + if (val)
> > + asus_ec_signal_request(ec);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + if (val)
> > + irq_wake_thread(priv->self->irq, priv);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > + debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev);
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > + struct dentry *debugfs_root, *dockram_dir;
> > + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > + priv->data->name);
> > +
> > + debugfs_root = debugfs_create_dir(name, NULL);
> > + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > + &ec_irq_fops);
> > + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > + &ec_request_fops);
> > + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > + &control_reg_fops);
> > + debugfs_create_file("dockram", 0644, dockram_dir,
> > + priv->info.dockram, &dockram_fops);
> > +
> > + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree. It does not belong in the upstream kernel.
>
Prints are required to show Firmware type, version and behavior.
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > + struct i2c_client *parent = to_i2c_client(dev);
> > + struct i2c_client *dockram;
> > + struct dockram_ec_data *priv;
> > + int ret;
> > +
> > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > + parent->addr + 2);
> > + if (IS_ERR(dockram))
> > + return dockram;
> > +
> > + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > + dockram);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + i2c_set_clientdata(dockram, priv);
> > + mutex_init(&priv->ctl_lock);
> > +
> > + return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>
fine
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > + return dev_err_probe(dev, -ENXIO,
> > + "I2C bus is missing required SMBus block mode support\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = device_get_match_data(dev);
> > + if (!priv->data)
> > + return -ENODEV;
> > +
> > + i2c_set_clientdata(client, priv);
> > + priv->self = client;
> > +
> > + priv->info.dockram = devm_asus_dockram_get(dev);
> > + if (IS_ERR(priv->info.dockram))
> > + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > + "failed to get dockram\n");
> > +
> > + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > + if (IS_ERR(priv->ecreq))
> > + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > + "failed to get request GPIO\n");
>
> "get" or "request"
>
request is gpio's name, request gpio
> > + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > + mutex_init(&priv->ecreq_lock);
> > +
> > + asus_ec_signal_request(&priv->info);
> > +
> > + ret = asus_ec_detect(priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + &asus_ec_interrupt,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + client->name, priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > + client->dev.coherent_dma_mask = 0;
> > + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + devm_asus_ec_debugfs_init(dev);
> > +
> > + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > + priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>
There may be 2 of this EC in the device with similar set of cells,
last time I have tested EC that was registered second caused clash
with first EC cause of these cells. I will check if this behavior
changed now.
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-keys",
> > + }, {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.
Yes
> > + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > + .driver = {
> > + .name = "asus-transformer-ec",
> > + .of_match_table = asus_ec_match,
> > + },
> > + .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > + const char *model;
> > + const char *name;
> > + struct i2c_client *dockram;
> > + struct workqueue_struct *wq;
> > + struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES 0x100
> > +#define DOCKRAM_ENTRY_SIZE 32
> > +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK BIT(0)
> > +#define ASUSEC_KEY_MASK BIT(2)
> > +#define ASUSEC_KBC_MASK BIT(3)
> > +#define ASUSEC_AUX_MASK BIT(5)
> > +#define ASUSEC_SCI_MASK BIT(6)
> > +#define ASUSEC_SMI_MASK BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE 0x53
> > +#define ASUSEC_SMI_RESET 0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > + (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE 8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register. The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> > + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> > +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> > +
> > +#define ASUSEC_WRITE_BUF 0x64
> > +#define ASUSEC_READ_BUF 0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > + u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > + return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > + u64 mask, u64 xor)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > + struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH 5/7] hid-asus: Fix input mapping on dedicated vendor HID interfaces
From: sashiko-bot @ 2026-05-14 10:32 UTC (permalink / raw)
To: Paolo Pisati; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513163248.16483-6-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The injected fake keyboard descriptor contains a malformed padding item that violates the HID specification.
--
commit 98dabe022a958136634ce6b477391fa33ffa21b6
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Fix input mapping on dedicated vendor HID interfaces
This commit adds a fake keyboard input to the vendor-specific interface's
report descriptor on some USB keyboards, such as the Zenbook Duo UX8406MA.
This allows the kernel to combine it with the vendor-specific collections
and process the custom mappings correctly.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index cf9184ed2d3ec..fe52fd63d61c6 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1376,6 +1376,30 @@ static void asus_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +/*
> + * Some USB keyboards, like the Zenbook Duo UX8406MA keyboard, have a dedicated
> + * USB interface for vendor-specific reports, separate to the generic HID
> + * keyboard or consumer control interfaces.
> + *
> + * The kernel does not register these vendor-specific interfaces as keyboards,
> + * or perform input mapping on them at all.
> + *
> + * To work around this, a fake keyboard input can be added to the
> + * vendor-specific interface's report descriptor. The kernel then combines it
> + * with the vendor-specific collections, and allows the interface to be used as
> + * a regular keyboard with our custom mappings.
> + */
> +static const __u8 asus_fake_keyboard_rdesc[] = {
> + 0x05, 0x01, /* Usage Page (Generic Desktop) */
> + 0x09, 0x06, /* Usage (Keyboard) */
> + 0xa1, 0x01, /* Collection (Application) */
> + 0x85, 0x01, /* Report ID (1) */
> + 0x75, 0x08, /* Report Size (8) */
> + 0x95, 0x01, /* Report Count (1) */
> + 0x81, 0x00, /* Input (Data,Arr,Abs) */
Does this Input item need to be declared as Constant?
According to the HID 1.11 specification, a field mapped as padding because
no usage is specified must be declared as Constant (e.g., 0x01 or 0x03).
Declaring it as Data (0x00) violates the protocol and could cause strict
user-space HID parsers reading the raw descriptor to reject it.
> + 0xc0, /* End Collection */
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-05-14 11:02 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260514100205.GG305027@google.com>
чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/mfd/Kconfig | 14 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> > include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > 4 files changed, 939 insertions(+)
> > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> > additional drivers must be enabled in order to use the
> > functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > + tristate "ASUS Transformer's embedded controller"
> > + depends on I2C && OF
> > + help
> > + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > + This provides shared glue for functional part drivers:
> > + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > + leds-asus-transformer-ec, asus-transformer-ec-battery
> > + and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>
> > + This driver can also be built as a module. If so, the module
> > + will be called asus-transformer-ec.
> > +
> > config MFD_AT91_USART
> > tristate "AT91 USART Driver"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> > obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> > obj-$(CONFIG_MFD_SM501) += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> > obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE 8
> > +
> > +struct asus_ec_chip_data {
> > + const char *name;
> > + const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT. It opens the gates to too much hackery. I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>
> > + unsigned int num_devices;
> > + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > + struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing. driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>
> > + struct mutex ecreq_lock; /* prevent simultaneous access */
> > + struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor. Please improve the nomenclature throughout.
>
> > + struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>
> > + const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'. This could be anything.
>
> > + bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > + struct mutex ctl_lock; /* prevent simultaneous access */
> > + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > + container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + * hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions? Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>
I don't see any instances regmap handling 256 bit registers.
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(client, reg,
> > + DOCKRAM_ENTRY_BUFSIZE, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > + return -EPROTO;
> > + }
> > +
> > + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > +
> > + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > + u64 xor)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + char *buf = priv->ctl_data;
> > + u64 val;
> > + int ret = 0;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>
> > + goto exit;
> > +
> > + if (buf[0] != ASUSEC_CTL_SIZE) {
> > + ret = -EPROTO;
> > + goto exit;
> > + }
> > +
> > + val = get_unaligned_le64(buf + 1);
> > +
> > + if (out)
> > + *out = val;
> > +
> > + if (mask || xor) {
> > + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + }
> > +
> > +exit:
> > + if (ret < 0)
> > + dev_err(&client->dev, "Failed to access control flags: %d\n",
> > + ret);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > + struct notifier_block **nb = res;
> > +
> > + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + * ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > + struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls. Why do the usual device driver .remove() handling work
> for you?
>
> > + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > + struct notifier_block **res;
> > + int ret;
> > +
> > + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + *res = nb;
> > + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > + if (ret) {
> > + devres_free(res);
> > + return ret;
> > + }
> > +
> > + devres_add(&pdev->dev, res);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + guard(mutex)(&priv->ecreq_lock);
> > +
> > + dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 1);
> > + msleep(50);
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 0);
> > + msleep(200);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > + sizeof(priv->ec_data),
> > + priv->ec_data);
> > +
> > + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > + sizeof(priv->ec_data), priv->ec_data,
> > + ret, in_irq ? "; in irq" : "");
> > +
> > + return ret;
> > +}
>
> No abstractions for the sake of it. All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > + return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>
> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > + int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > + while (retry--) {
> > + if (asus_ec_read(priv, false) < 0)
> > + continue;
> > +
> > + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > + continue;
> > +
> > + break;
> > + }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > + const char *name, char **out)
> > +{
> > + char buf[DOCKRAM_ENTRY_BUFSIZE];
> > + int ret;
> > +
> > + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!priv->logging_disabled)
> > + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > + buf[0], buf + 1);
> > +
> > + if (out)
> > + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > + int retry, ret;
> > +
> > + for (retry = 0; retry < 3; retry++) {
> > + ret = asus_ec_write(priv, 0);
> > + if (!ret)
> > + return 0;
> > +
> > + msleep(300);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > + u64 flag;
> > + int ret;
> > +
> > + ret = asus_ec_get_ctl(&priv->info, &flag);
> > + if (ret < 0)
> > + return ret;
> > +
> > + flag &= ASUSEC_CTL_SUSB_MODE;
> > + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > + flag ? "susb on when receive ec_req" :
> > + "susb on when system wakeup");
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > + "normal");
> > +
> > + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > + char *model = NULL;
> > + int ret;
> > +
> > + ret = asus_ec_reset(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + asus_ec_clear_buffer(priv);
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->logging_disabled = true;
> > +
> > + ret = asus_ec_magic_debug(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->info.model = model;
> > + priv->info.name = priv->data->name;
> > +
> > + if (priv->data->clr_fmode)
> > + asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > + if (ret)
> > + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > + switch (code) {
> > + case ASUSEC_SMI_HANDSHAKE:
> > + case ASUSEC_SMI_RESET:
> > + asus_ec_detect(priv);
> > + break;
> > + }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > + struct asus_ec_data *priv = dev_id;
> > + unsigned long notify_action;
> > + int ret;
> > +
> > + ret = asus_ec_read(priv, true);
> > + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > + return IRQ_NONE;
> > +
> > + notify_action = priv->ec_data[1];
> > + if (notify_action & ASUSEC_SMI_MASK) {
> > + unsigned int code = priv->ec_data[2];
> > +
> > + asus_ec_handle_smi(priv, code);
> > +
> > + notify_action |= code << 8;
> > + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > + }
> > +
> > + blocking_notifier_call_chain(&priv->info.notify_list,
> > + notify_action, priv->ec_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg, rsize;
> > + ssize_t n_read = 0, val;
> > + loff_t off = *ppos;
> > + char *data;
> > + int ret;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + off %= DOCKRAM_ENTRY_SIZE;
> > + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > + if (!count)
> > + return 0;
> > +
> > + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > + while (reg < DOCKRAM_ENTRIES) {
> > + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > + if (len > rsize)
> > + len = rsize;
> > +
> > + ret = asus_dockram_read(client, reg, data);
> > + if (ret < 0) {
> > + if (!n_read)
> > + n_read = ret;
> > + break;
> > + }
> > +
> > + val = copy_to_user(buf, data + 1 + off, len);
> > + if (val == len)
> > + return -EFAULT;
> > +
> > + *ppos += len;
> > + n_read += len;
> > +
> > + if (len == rsize)
> > + break;
> > +
> > + rsize -= len;
> > + buf += len;
> > + off = 0;
> > + ++reg;
> > + }
> > +
> > + kfree(data);
> > +
> > + return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > + const char __user *buf, size_t count)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + int ret;
> > +
> > + if (!count || count > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > + if (buf[0] != count - 1)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + priv->ctl_data[0] = (u8)count;
> > + memcpy(priv->ctl_data + 1, buf, count);
> > + ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg;
> > + loff_t off = *ppos;
> > + int ret;
> > +
> > + if (off % DOCKRAM_ENTRY_SIZE != 0)
> > + return -EINVAL;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + if (reg >= DOCKRAM_ENTRIES)
> > + return -EINVAL;
> > +
> > + ret = dockram_write_one(client, reg, buf, count);
> > +
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > + .read = dockram_read,
> > + .write = dockram_write,
> > + .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>
> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > + return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > + return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > + control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > + if (val)
> > + asus_ec_signal_request(ec);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + if (val)
> > + irq_wake_thread(priv->self->irq, priv);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > + debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev);
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > + struct dentry *debugfs_root, *dockram_dir;
> > + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > + priv->data->name);
> > +
> > + debugfs_root = debugfs_create_dir(name, NULL);
> > + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > + &ec_irq_fops);
> > + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > + &ec_request_fops);
> > + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > + &control_reg_fops);
> > + debugfs_create_file("dockram", 0644, dockram_dir,
> > + priv->info.dockram, &dockram_fops);
> > +
> > + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree. It does not belong in the upstream kernel.
>
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > + struct i2c_client *parent = to_i2c_client(dev);
> > + struct i2c_client *dockram;
> > + struct dockram_ec_data *priv;
> > + int ret;
> > +
> > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > + parent->addr + 2);
> > + if (IS_ERR(dockram))
> > + return dockram;
> > +
> > + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > + dockram);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + i2c_set_clientdata(dockram, priv);
> > + mutex_init(&priv->ctl_lock);
> > +
> > + return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > + return dev_err_probe(dev, -ENXIO,
> > + "I2C bus is missing required SMBus block mode support\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = device_get_match_data(dev);
> > + if (!priv->data)
> > + return -ENODEV;
> > +
> > + i2c_set_clientdata(client, priv);
> > + priv->self = client;
> > +
> > + priv->info.dockram = devm_asus_dockram_get(dev);
> > + if (IS_ERR(priv->info.dockram))
> > + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > + "failed to get dockram\n");
> > +
> > + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > + if (IS_ERR(priv->ecreq))
> > + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > + "failed to get request GPIO\n");
>
> "get" or "request"
>
> > + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > + mutex_init(&priv->ecreq_lock);
> > +
> > + asus_ec_signal_request(&priv->info);
> > +
> > + ret = asus_ec_detect(priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + &asus_ec_interrupt,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + client->name, priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > + client->dev.coherent_dma_mask = 0;
> > + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + devm_asus_ec_debugfs_init(dev);
> > +
> > + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > + priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-keys",
> > + }, {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.
>
> > + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > + .driver = {
> > + .name = "asus-transformer-ec",
> > + .of_match_table = asus_ec_match,
> > + },
> > + .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > + const char *model;
> > + const char *name;
> > + struct i2c_client *dockram;
> > + struct workqueue_struct *wq;
> > + struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES 0x100
> > +#define DOCKRAM_ENTRY_SIZE 32
> > +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK BIT(0)
> > +#define ASUSEC_KEY_MASK BIT(2)
> > +#define ASUSEC_KBC_MASK BIT(3)
> > +#define ASUSEC_AUX_MASK BIT(5)
> > +#define ASUSEC_SCI_MASK BIT(6)
> > +#define ASUSEC_SMI_MASK BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE 0x53
> > +#define ASUSEC_SMI_RESET 0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > + (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE 8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register. The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> > + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> > +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> > +
> > +#define ASUSEC_WRITE_BUF 0x64
> > +#define ASUSEC_READ_BUF 0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > + u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > + return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > + u64 mask, u64 xor)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > + struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH 6/7] hid-asus: Allow adding custom hotkey handler logic
From: sashiko-bot @ 2026-05-14 11:14 UTC (permalink / raw)
To: Paolo Pisati; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513163248.16483-7-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling platform_profile_cycle() from an atomic context causes a 'scheduling while atomic' bug.
- [High] Missing Kconfig dependency on ACPI_PLATFORM_PROFILE leads to a linker error.
- [Medium] Improper HID event return value causes spurious error logs and incorrect event propagation.
- [Low] Dead code due to unreachable break statement.
--
commit f4dcdddfbfb9d5acd09f6a961fcad0539b0cd8c7
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Allow adding custom hotkey handler logic
This commit modifies the Asus HID driver to handle a custom vendor hotkey
for cycling the platform profile when a specific usage code is received.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index fe52fd63d61c6..37e3aec612b6d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -29,6 +29,7 @@
> #include <linux/input/mt.h>
> #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
> #include <linux/power_supply.h>
> +#include <linux/platform_profile.h>
Does the HID_ASUS Kconfig option need to depend on or select
ACPI_PLATFORM_PROFILE? If the kernel is built with
CONFIG_ACPI_PLATFORM_PROFILE=n, won't this result in a linker error since
platform_profile_cycle() is used unconditionally and there are no empty dummy
stubs provided in the header?
[ ... ]
> @@ -362,8 +363,21 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
> if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
> (usage->hid & HID_USAGE) != 0x00 &&
> (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> - hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> - usage->hid & HID_USAGE);
> +
> + /*
> + * Some reports do not map directly to standard keys, and need special
> + * handling.
> + */
> + switch (usage->hid & HID_USAGE) {
> + case 0x9d:
> + if (!value)
> + break;
> + return platform_profile_cycle();
Is it safe to call platform_profile_cycle() from here?
The asus_event() callback is invoked by the HID core in an atomic softirq
context when processing USB URB completions. However,
platform_profile_cycle() acquires profile_lock, which is a mutex. Will this
result in a scheduling while atomic panic? Should this action be deferred to
a workqueue, similar to how fn_lock_sync_work is handled?
Also, if platform_profile_cycle() returns a negative error code (like -EINVAL
when no profiles are registered), returning it directly here will cause
hid_process_event() to spam the kernel log with error messages on every key
press. Conversely, if it returns 0 on success, won't the HID core incorrectly
continue processing this event instead of treating it as consumed? Does this
need to handle the return value and return 1 to halt further event
processing?
> + break;
This isn't a bug, but this break statement is unreachable since the previous
line always returns.
> + default:
> + hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> + usage->hid & HID_USAGE);
> + }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-14 11:51 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513231445.3213501-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
- [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
- [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
- [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
- [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
--
commit 13b388de96d7484eee908adeed127e8086fc289e
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It provides support for switching the gamepad mode and changing
the behavior of the M-keys.
> Additionally, the MCU RAM address for writing configuration data has
> changed twice. Checks are done during probe to enumerate these variances.
The commit message states that checks are done during probe to enumerate MCU
RAM address variances.
Looking at claw_probe() and msi_probe(), there does not appear to be any code
that checks firmware versions or handles these variances.
Was this functionality dropped during refactoring, or should the commit message
be updated?
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..8915942af15e6
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> + size_t len, unsigned int timeout)
> +{
> + unsigned char *dmabuf __free(kfree) = NULL;
> + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + size_t header_size = ARRAY_SIZE(header);
> + int ret;
> +
> + if (header_size + len > CLAW_PACKET_SIZE)
> + return -EINVAL;
> +
> + /* We can't use a devm_alloc reusable buffer without side effects during suspend */
> + dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + memcpy(dmabuf, header, header_size);
> + if (data && len)
> + memcpy(dmabuf + header_size, data, len);
> +
> + /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> + if (timeout) {
> + guard(mutex)(&drvdata->cfg_mutex);
> + reinit_completion(&drvdata->send_cmd_complete);
> + }
Does this drop the lock too early?
The guard() macro releases the lock at the end of its enclosing scope.
Because it is declared inside the if (timeout) block, the mutex will be
released immediately at the closing brace, leaving hid_hw_output_report() and
the subsequent wait unprotected against concurrent access.
> +
> + dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> + CLAW_PACKET_SIZE, dmabuf);
> +
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> + if (ret)
> + return ret;
> +
> + if (timeout) {
> + ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> + msecs_to_jiffies(timeout));
Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?
Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
function. A 1-jiffy timeout can expire almost instantaneously if the operation
begins near the end of the current tick boundary, potentially leading to
spurious -EBUSY timeouts.
[ ... ]
> +static void cfg_resume_fn(struct work_struct *work)
> +{
> + struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> + struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> + u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
If the system suspends before cfg_setup_fn() executes, or if the device read in
cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?
Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
at 1), will this send an uninitialized configuration state to the hardware?
> + int ret;
> +
> + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> + ARRAY_SIZE(data), 0);
> + if (ret)
> + dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> +}
> +
> +static int claw_probe(struct hid_device *hdev, u8 ep)
> +{
> + struct claw_drvdata *drvdata;
> + int ret;
> +
> + drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + hid_set_drvdata(hdev, drvdata);
> + drvdata->hdev = hdev;
> + drvdata->ep = ep;
> +
> + mutex_init(&drvdata->cfg_mutex);
> + init_completion(&drvdata->send_cmd_complete);
Is it safe to publish drvdata via hid_set_drvdata() before initializing the
mutex and completion object?
Since hid_hw_start() was already called earlier in msi_probe(), the hardware
is active. If an input report arrives right after hid_set_drvdata(), the
interrupt handler msi_raw_event() will process it and call complete() on an
uninitialized waitqueue.
> +
> + /* For control interface: open the HID transport for sending commands. */
> + ret = hid_hw_open(hdev);
> + if (ret)
> + return ret;
> +
> + INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> + INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> + schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=1
^ 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